fix(plugin-history-sync): unify useActivityParams runtime type across navigation paths#705
Open
fix(plugin-history-sync): unify useActivityParams runtime type across navigation paths#705
Conversation
…boundary (FEP-1061)
Before this change, `push("X", { visible: true })` stored the boolean `true`
in the core store while URL arrival parsed the same URL as `{ visible: "true" }`.
`useActivityParams<K>()` therefore returned different runtime types depending
on how the user reached the activity.
This change coerces non-string values to `string | undefined` inside
`@stackflow/plugin-history-sync`:
- New `coerceParamsToString` utility that converts booleans/numbers/bigint/
symbols/objects/arrays/functions to strings, maps `null`→`undefined`, and
preserves `undefined`. Circular objects fall back to `String(value)`.
- `makeTemplate` gains `fillWithoutEncode(preEncodedParams)` alongside the
existing `fill(typedParams)`. Both share an internal `_buildUrl` helper to
prevent drift (pre-mortem scenario 3).
- `onBeforePush` / `onBeforeReplace` / `onBeforeStepPush` / `onBeforeStepReplace`
hooks call `overrideActionParams` with coerced params AFTER `encode`
consumed the typed values for URL generation. `encode` still receives
typed `U` exactly as before.
- Decode-arrival path in `createTargetActivityPushEvent` and
`historyEntryToEvents` coerces the `template.parse()` result (including
`decode` output) so the initial events reaching the store are also strings.
- `onPushed` / `onReplaced` / `onStepPushed` / `onStepReplaced` / `onInit`
switch from `template.fill(activity.params)` to
`template.fillWithoutEncode(...)` because `activity.params` is now
guaranteed to be strings — re-running `encode` on strings would violate the
`encode` contract.
The core store now always contains `{ [key: string]: string | undefined }` at
runtime, matching `ActivityBaseParams`. Consumers relying on `decode` to
inject typed values into the store must coerce at the usage site instead
(e.g. `Number(params.count)`). See the docs update and changeset for the full
migration note.
New tests: 13 `coerceParamsToString` unit cases, 7 additional `makeTemplate`
cases (fill/encode contract, fillWithoutEncode, parity), 5 `historySyncPlugin`
integration cases including the CRITICAL decode-path parity test.
Zero changes to `@stackflow/core` or any other package.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
regression harness Adds 11 targeted tests on top of commit 0f791c8, and refactors the coerceParamsToString spec to match stackflow's flat `test(...)` house style. Breakdown: 1. coerceParamsToString.spec.ts (15 → 17) - Removed the `describe(...)` wrapper; flattened to top-level `test(...)` per the convention in `last.spec.ts` / `normalizeRouteInput.spec.ts`. - Added idempotency test: `coerce(coerce(x)) === coerce(x)` across every input branch. - Added 1000-key large-input sanity test. 2. makeTemplate.spec.ts (+3) - fillWithoutEncode with empty-string values — documents the falsy guard in `_buildUrl` that drops empty strings from the query. - fill propagates synchronous errors from user-supplied encode — fill does not try/catch. - parse with custom decode receives raw URL strings unchanged (pre-coercion input). 3. historySyncPlugin.spec.ts (+9 + `pushUntyped` helper) - Path 2: falsy primitives (`false` / `0`) coerced to `"false"` / `"0"`. - Path 3: NaN / Infinity stringify to `"NaN"` / `"Infinity"`. - Path 4: nested objects are JSON.stringified in the store. - Path 5 (Risk #6 — KNOWN LIMITATION): a plugin registered AFTER history-sync can re-inject typed values via `overrideActionParams` and bypass the coercion. Assertion records the CURRENT behavior (number in store) so any future refactor that resolves Risk #6 will flip this test — making it a citable regression harness. - Path 6: push → pop → URL-arrival produces the same store shape. - Path 7: replace after stepPush coerces all step + activity params. - Path 8: decode returning `boolean` via `===` is coerced to string (complements the existing CRITICAL decode-parity test that covers `Number(...)` — delta is the return type). - Path 9: per-route encode runs only on matching routes; Home (no encode) and Article (with encode) both yield string params in the store. - Path 10: chained stepPush / stepReplace — each cycle's params are independently coerced. Introduced a `pushUntyped(actions, activityName, params, [id])` helper to replace `as unknown as string` casts with a single, legible boundary where runtime coercion is intentionally exercised. Existing FEP-1061 tests left untouched to keep the diff surface small for a first contribution (per architect antithesis). Red-green verification: each new test was reasoned against commit `163e8643` (pre-fix) to ensure it would plausibly fail without the coercion wiring. Reproducible locally via: git revert --no-commit 0f791c8 yarn workspace @stackflow/plugin-history-sync test then restore with `git checkout .`. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gle overrideActionParams call `core`'s `overrideActionParams` is a spread-merge (see `triggerPreEffectHooks.ts:53-56`). The previous two-call shape in FEP-1061 — first call sets `activityContext.path`, second call spreads the ORIGINAL `actionParams` plus coerced `activityParams` — clobbered the just-set path because the second call's spread included `actionParams.activityContext` (no path), which overwrote `nextActionParams.activityContext` set by the first call. Refactor both hooks to a single `overrideActionParams` call that merges path (when needed) and coerced params in one atomic operation. Also expand the changeset with explicit migration notes for decode authors (coerced return values) and consumers with plugins registered after historySyncPlugin (Risk #6 plugin-order limitation). Surfaced by First Principles Engineer audit pass before DRAFT PR. 87/87 tests pass; typecheck exit 0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eturn for cross-deploy hydration (FEP-1061 follow-up) `overrideInitialEvents` had an early-return path at historySyncPlugin.tsx:198-213 that spread `initialState.activity.enteredBy` directly into a synthetic Pushed event without running `coerceParamsToString` over `activityParams`. Within-deploy this was safe because the writer side already coerced before serializing to history.state. But in a cross-deploy scenario — old version writes typed values to history.state, user reloads on a new deploy that includes the FEP-1061 fix — the early-return left non-string values in the store, breaking the `useActivityParams()` string-only runtime contract for users coming back through serialized state. Coerce both `activityParams` and `stepParams` inside the early-return. Idempotent on already-coerced strings, so no behavioral change for the within-deploy case. Closes the 7th entry path identified by FEP-1061 verification (path-convergence matrix T-I4 + cross-deploy hydration test T-I5). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…FEP-1061 invariants
Expands the FEP-1061 test surface from 87 to 129 (+42) tests, closing all gaps
identified by the verification pass.
Unit (coerceParamsToString.spec.ts):
- T-U1: pin Date → JSON.stringify via Date.prototype.toJSON (NOT '{}')
- T-U2: pin Map/Set → JSON.stringify produces '{}' (NOT String() fallback)
- T-U3: replace 1000-key length-only test with typeof assertions on every cycle branch
Integration (historySyncPlugin.spec.ts):
- T-I1: encode-ORDER LOCK — encode receives typed boolean before coerce
- T-I2: NON-IDENTITY DRIFT — fill(typed) URL ≡ fillWithoutEncode(coerce(encode(typed)))
- T-I3: NO-DECODE URL-arrival → store has string-typed query params
- T-I4: PATH-CONVERGENCE PROPERTY — 7 entry paths × 5 typed-value classes (31 sub-tests)
- T-I5: CROSS-DEPLOY HYDRATION — parseState early-return coerces typed activityParams
- T-I6: ENCODE-THROWS — encode error propagates without leaving half-coerced entry
- F3: replace-to-different-route updates path AND coerces params atomically (c80d597 FPE regression lock)
- F4: history.back() preserves coerced params on the active activity
- F5: useHash mode coerces URL-arrival params identically to history mode
- F9: store layer backing useActivityParams() returns string-only after typed push
Template (makeTemplate.spec.ts):
- T-M1: replace 2 vacuous identity-encode parity tests with non-identity counterparts
proving fillWithoutEncode skips encode
All 7 suites green; typecheck and build exit 0.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 3a6476f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds runtime coercion of activity and step params to Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Core as Core Stack
participant Plugin as HistorySyncPlugin
participant History as Browser History
User->>Core: push/replace/stepPush/stepReplace(typedParams)
Core->>Plugin: onBefore*(typedParams)
Plugin->>Plugin: encode(typedParams) -> compute context.path
Plugin->>Plugin: coerceParamsToString(params) -> Record<string,string|undefined>
Plugin->>Core: overrideActionParams(coercedParams + context.path)
Core->>History: history.pushState(encoded URL)
History->>Plugin: popstate / URL arrival
Plugin->>Plugin: parse URL -> raw params
Plugin->>Plugin: coerceParamsToString(raw) -> normalized params
Plugin->>Core: push with normalized params + context.path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Deploying stackflow-demo with
|
| Latest commit: |
3a6476f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cfee75d9.stackflow-demo.pages.dev |
| Branch Preview URL: | https://feature-fep-1061.stackflow-demo.pages.dev |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | 3a6476f | Commit Preview URL | Apr 30 2026, 10:06 AM |
@stackflow/core
@stackflow/compat-await-push
@stackflow/link
@stackflow/plugin-basic-ui
@stackflow/plugin-blocker
@stackflow/plugin-devtools
@stackflow/plugin-google-analytics-4
@stackflow/plugin-history-sync
@stackflow/plugin-lifecycle
@stackflow/plugin-map-initial-activity
@stackflow/plugin-preload
@stackflow/plugin-renderer-basic
@stackflow/plugin-renderer-web
@stackflow/plugin-sentry
@stackflow/plugin-stack-depth-change
@stackflow/react-ui-core
@stackflow/react
commit: |
Collaborator
Author
|
공유용) @ENvironmentSet 우선 PR 올려봤는데, 의도대로 구현한지 확신이 안서서 자체 검증중이에요 |
moel-kim
changed the title
Collaborator
Author
Test plan iteration follow-upAdds 8 new active tests + 4 skipped (intentional), bringing the suite from 129 → 137 active / 141 total. All checks green. What was addedNew integration tests (7 active in
New unit (1 active in
Skipped (4, intentional):
Why this iteration was neededThe original test plan went ITERATE on both architect and critic review with overlapping findings: a The leaner plan aligns with explicit guardrails:
Durable artifact:
|
Collaborator
Issues found1.
|
…vent (FEP-1061 follow-up)
Adds an optional `stepContext?: {}` field to `StepPushedEvent` and
`StepReplacedEvent` (mirrors the existing `activityContext?: {}` precedent on
`PushedEvent`), and a corresponding `context?: {}` on `ActivityStep`.
`makeActivityReducer.ts` and `aggregate.ts` propagate the field through to
`step.context`.
Purely additive. No breaking change. Used by `@stackflow/plugin-history-sync`
to preserve `encode`-output URLs through the store across every step
navigation path — including `popstate` forward across step boundaries —
instead of relying on plugin-internal state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s all entry paths (FEP-1061 / orionmiz #1+#2) After FEP-1061's coercion landed, post-effect hooks called `template.fillWithoutEncode(activity.params)` against coerced strings, which skipped `encode` and wrote coerced values into `history.location`. Routes with non-identity `encode` (e.g. `({ visible }) => ({ visible: visible ? "y" : "n" })`) regressed: `history.location` showed `?visible=true` instead of the expected `?visible=y`. The popstate `isForward` and `isStepForward` branches reconstructed push events without preserving `activityContext` / `stepContext`, so `onBeforePush` / `onBeforeStepPush` re-ran `template.fill(coerced_strings)` and called `encode` with strings — violating the encode-receives-typed-U contract. Fixes: - `onBeforePush` / `onBeforeReplace`: short-circuit when `actionParams.activityContext.path` is already present (preserves the encoded URL set by a prior pre-effect or by popstate re-push). Never re-run `encode` on coerced strings. - `onBeforeStepPush` / `onBeforeStepReplace`: same predicate, plus pre-encode the step URL into `stepContext.path` (using the new core field) when typed step params are still available. - `onPushed` / `onReplaced` / `onStepPushed` / `onStepReplaced`: read `*.context.path` directly. `template.fillWithoutEncode(coerced)` remains as a defensive fallback for plugins that bypass the pre-effect chain. - `onInit` URL-replay: same — trusts pre-computed paths from `overrideInitialEvents` / pre-effect, falls back to `fillWithoutEncode` only when missing. Server-emitted `activity.context.path` is honored (no client-side recompute that could divergent on encode-version mismatch). - `historyEntryToEvents` (defaultHistory ancestors): computes per-ancestor encoded URL via the ancestor's own route's `template.fill(typed)`. Sets both `activityContext.path` and `stepContext.path` per ancestor. - popstate `isForward`: passes `activityContext: targetActivity.context` so the encoded URL flows through unchanged; pre-effect short-circuits. - popstate `isStepForward`: passes `stepContext: targetStep.context` (the new core field), same flow. Updated `INTENT.md` with the URL output contract section, the SSR consideration, and the pre-Option-B legacy `history.state` transitional fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ons and TDD coverage for orionmiz #3 Adds 15 new tests asserting `path(history.location)` under non-identity `encode` — the surface that the previous FEP-1061 verification missed. Strengthens existing T-I1 (encode-order lock) and F3 (replace-route atomicity) with `path(history.location)` assertions. Also strengthens the unit suite's catch-branch determinism and idempotent-typeof checks. New tests: - T-O-1..T-O-4: push / replace / stepPush / stepReplace with non-identity encode → `history.location` reflects encode output. - T-O-5: defaultHistory ancestor URL uses ancestor's route's encode (not currentPath). - T-O-6: popstate forward (activity boundary) preserves encoded URL via `activityContext`; encode mock is NOT called with coerced strings. - T-O-7: popstate stepForward preserves encoded URL via the new `stepContext.path` core field. (True GREEN under Option B; no documented limitation.) - T-O-8: onInit URL-replay reflects encode output for parsed-state restoration. - T-O-12 (regression bar): route WITHOUT encode → history.location output byte-identical to fillWithoutEncode (proves no regression for the common case). - T-O-14 (SSR pin): server-emitted `activity.context.path` is trusted on client onInit. - T-O-16 (replace-with-3-active-steps): no orphan step entries after replace; new activity URL is encode-output-correct. Pin tests (current-behavior locks, distinct from Phase A RED): - T-O-13: plugin dispatches Pushed without activityContext → post-effect falls back to fillWithoutEncode (S1 mitigation). - T-O-15: no module-scope plugin state survives plugin re-instantiation (HMR safety, S5). Plus T-U-NEW-8 (deterministic JSON.stringify catch-branch via spy) and T-U-NEW-9 (idempotent test asserts typeof per key) for the unit suite. Final test count: 137 → 152 active / 156 total (4 intentional skips). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Addresses PR review feedback (#1, #2, #3)3 new commits landed: Issues fixed#1 — #2 — popstate forward branches now preserve #3 — test gap closed. Every existing test is strengthened with Core change (additive, non-breaking)This iteration adds Changeset bumps Test surface delta
Final test counts: plugin 152 active / 156 total; core 84. Verification gates
Process notesThis iteration went through a consensus planning cycle (architect ITERATE → critic ITERATE → applied 7+ findings → architect ITERATE on N1-N4 → applied → critic APPROVE). The architect originally proposed a plugin-internal Could the reviewer re-review when convenient? The 3 issues should all be addressed. 🤖 Generated with Claude Code |
moel-kim
changed the title
Per maintainer review: remove internal-only references from public-facing documents (PR-visible artifacts in this repo). - `INTENT.md` rewritten without internal user names, Slack URLs, internal tracker URLs, or Korean quotes. Decision rationale and tradeoff documentation preserved. - `historySyncPlugin.spec.ts` (6 lines): replace internal user name + internal tooling paths + direct internal-tracker URL with abstract phrasing. Test semantics unchanged. The literal `FEP-1061` identifier is preserved in test names and source comments — issue tracker references are normal in OSS and the maintainer scoped the sanitization request narrowly. - Changeset filenames renamed to drop tracker-id prefix; content cleaned of user names and internal-tracker URLs while preserving technical migration notes. No behavior change. No source-code logic touched. Tests still pass at 152 active / 4 skipped / 156 total. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
moel-kim
force-pushed
the
feature/fep-1061
branch
2 times, most recently
from
77b5933 to
33df89c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/pages/api-references/future-api/changes.en.mdx`:
- Line 135: The example uses push("Article", { visible: true }) which is
inconsistent with the rest of the doc naming; update the activity name to
push("ArticleActivity", { visible: true }) so the example matches the page’s
config naming and avoids copy/paste confusion—search for the push(...) call in
the example and replace "Article" with "ArticleActivity".
In `@docs/pages/api-references/future-api/changes.ko.mdx`:
- Line 135: The example uses push("Article", { visible: true }) which is
inconsistent with the rest of the doc that uses ArticleActivity; update the
activity name to match the document convention (e.g., replace "Article" with
"ArticleActivity" wherever push("Article", ...) appears) so examples are
consistent with ArticleActivity naming throughout the file.
In `@extensions/plugin-history-sync/src/historySyncPlugin.tsx`:
- Around line 341-350: The fallback branch re-encodes URL-derived params when
computing targetPath which can corrupt values; change the logic around matched,
targetParams, and targetTemplate.fill so that when matched === null you pass the
raw string map from urlSearchParamsToMap(pathToUrl(currentPath).searchParams)
(i.e., already-string values) into targetTemplate.fill without applying any
encoder/typing step — or call the fill variant that accepts raw string params —
ensuring functions/symbols involved are matched, targetParams, pathToUrl,
urlSearchParamsToMap, and targetTemplate.fill.
In `@extensions/plugin-history-sync/src/makeTemplate.ts`:
- Around line 78-89: The reducer building URLSearchParams from searchParamsMap
drops empty-string values because it checks truthiness; update the condition in
the reducer (the code constructing searchParams in _buildUrl / where
searchParamsMap is reduced) to include empty strings by only excluding undefined
(and optionally null) values rather than using value truthiness — e.g., test
value !== undefined (or value != null) before adding [key]: value so ?key= is
preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa74ebd5-d890-43d4-8eae-712ba1f8cd81
📒 Files selected for processing (16)
.changeset/coerce-activity-params-to-string.md.changeset/step-context-path.mdcore/src/Stack.tscore/src/activity-utils/makeActivityReducer.tscore/src/aggregate.tscore/src/event-types/StepPushedEvent.tscore/src/event-types/StepReplacedEvent.tsdocs/pages/api-references/future-api/changes.en.mdxdocs/pages/api-references/future-api/changes.ko.mdxextensions/plugin-history-sync/INTENT.mdextensions/plugin-history-sync/src/coerceParamsToString.spec.tsextensions/plugin-history-sync/src/coerceParamsToString.tsextensions/plugin-history-sync/src/historySyncPlugin.spec.tsextensions/plugin-history-sync/src/historySyncPlugin.tsxextensions/plugin-history-sync/src/makeTemplate.spec.tsextensions/plugin-history-sync/src/makeTemplate.ts
|
|
||
| ```tsx | ||
| // These two paths used to produce different runtime types. They don't anymore. | ||
| push("Article", { visible: true }) // store: { visible: "true" } |
There was a problem hiding this comment.
Align activity name in example with the page’s own config naming.
Line 135 uses push("Article", ...), but this doc consistently uses names like ArticleActivity. Keeping names aligned avoids copy/paste confusion.
Suggested patch
-push("Article", { visible: true }) // store: { visible: "true" }
+push("ArticleActivity", { visible: true }) // store: { visible: "true" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/pages/api-references/future-api/changes.en.mdx` at line 135, The example
uses push("Article", { visible: true }) which is inconsistent with the rest of
the doc naming; update the activity name to push("ArticleActivity", { visible:
true }) so the example matches the page’s config naming and avoids copy/paste
confusion—search for the push(...) call in the example and replace "Article"
with "ArticleActivity".
|
|
||
| ```tsx | ||
| // 이전에는 두 경로가 런타임에 서로 다른 타입을 반환했지만, 이제는 동일해요. | ||
| push("Article", { visible: true }) // 스토어: { visible: "true" } |
There was a problem hiding this comment.
예시 액티비티 이름을 문서 내 다른 예시와 맞춰주세요.
Line 135의 push("Article", ...)는 이 문서의 ArticleActivity 네이밍과 달라서 혼동을 줄 수 있어요.
Suggested patch
-push("Article", { visible: true }) // 스토어: { visible: "true" }
+push("ArticleActivity", { visible: true }) // 스토어: { visible: "true" }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| push("Article", { visible: true }) // 스토어: { visible: "true" } | |
| push("ArticleActivity", { visible: true }) // 스토어: { visible: "true" } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/pages/api-references/future-api/changes.ko.mdx` at line 135, The example
uses push("Article", { visible: true }) which is inconsistent with the rest of
the doc that uses ArticleActivity; update the activity name to match the
document convention (e.g., replace "Article" with "ArticleActivity" wherever
push("Article", ...) appears) so examples are consistent with ArticleActivity
naming throughout the file.
Comment on lines
+341
to
+350
| const targetParams = | ||
| matched ?? urlSearchParamsToMap(pathToUrl(currentPath).searchParams); | ||
| // FEP-1061 (Option B, B8): when the URL matched the target route, | ||
| // use currentPath (the user's URL); when fallback was triggered | ||
| // (no match), compute the target route's URL from its template | ||
| // so onInit writes a route-correct URL (was previously | ||
| // currentPath, e.g. "/" instead of "/home/"). | ||
| const targetPath = matched | ||
| ? currentPath | ||
| : targetTemplate.fill(targetParams); |
There was a problem hiding this comment.
Avoid re-running encode on URL-derived fallback params.
In the matched === null branch, targetParams come from the current URL, so they're already string-shaped. Passing them through fill() can mis-encode fallback URLs for non-identity encoders ("false" is truthy, etc.) even though no typed navigation input exists on this path.
Suggested fix
const targetPath = matched
? currentPath
- : targetTemplate.fill(targetParams);
+ : targetTemplate.fillWithoutEncode(
+ coerceParamsToString(targetParams),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const targetParams = | |
| matched ?? urlSearchParamsToMap(pathToUrl(currentPath).searchParams); | |
| // FEP-1061 (Option B, B8): when the URL matched the target route, | |
| // use currentPath (the user's URL); when fallback was triggered | |
| // (no match), compute the target route's URL from its template | |
| // so onInit writes a route-correct URL (was previously | |
| // currentPath, e.g. "/" instead of "/home/"). | |
| const targetPath = matched | |
| ? currentPath | |
| : targetTemplate.fill(targetParams); | |
| const targetParams = | |
| matched ?? urlSearchParamsToMap(pathToUrl(currentPath).searchParams); | |
| // FEP-1061 (Option B, B8): when the URL matched the target route, | |
| // use currentPath (the user's URL); when fallback was triggered | |
| // (no match), compute the target route's URL from its template | |
| // so onInit writes a route-correct URL (was previously | |
| // currentPath, e.g. "/" instead of "/home/"). | |
| const targetPath = matched | |
| ? currentPath | |
| : targetTemplate.fillWithoutEncode( | |
| coerceParamsToString(targetParams), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/plugin-history-sync/src/historySyncPlugin.tsx` around lines 341 -
350, The fallback branch re-encodes URL-derived params when computing targetPath
which can corrupt values; change the logic around matched, targetParams, and
targetTemplate.fill so that when matched === null you pass the raw string map
from urlSearchParamsToMap(pathToUrl(currentPath).searchParams) (i.e.,
already-string values) into targetTemplate.fill without applying any
encoder/typing step — or call the fill variant that accepts raw string params —
ensuring functions/symbols involved are matched, targetParams, pathToUrl,
urlSearchParamsToMap, and targetTemplate.fill.
Comment on lines
+78
to
+89
| const searchParams = new URLSearchParams( | ||
| Object.entries(searchParamsMap).reduce( | ||
| (acc, [key, value]) => ({ | ||
| ...acc, | ||
| ...(value | ||
| ? { | ||
| [key]: value, | ||
| } | ||
| : null), | ||
| }), | ||
| {} as Record<string, string>, | ||
| ), |
There was a problem hiding this comment.
Preserve empty-string params in _buildUrl.
This reducer filters on truthiness, so "" is dropped even though parse() preserves ?key= as "". That makes URL round-trips lossy and will strip empty-string params on replay/reload.
Suggested fix
const searchParams = new URLSearchParams(
Object.entries(searchParamsMap).reduce(
(acc, [key, value]) => ({
...acc,
- ...(value
+ ...(value !== undefined
? {
[key]: value,
}
: null),
}),
{} as Record<string, string>,
),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/plugin-history-sync/src/makeTemplate.ts` around lines 78 - 89, The
reducer building URLSearchParams from searchParamsMap drops empty-string values
because it checks truthiness; update the condition in the reducer (the code
constructing searchParams in _buildUrl / where searchParamsMap is reduced) to
include empty strings by only excluding undefined (and optionally null) values
rather than using value truthiness — e.g., test value !== undefined (or value !=
null) before adding [key]: value so ?key= is preserved.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
344-354:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
fillWithoutEncodein the unmatched fallback path.This branch still feeds URL-derived string params back into
template.fill(). For non-identity encoders, that can rewrite fallback URLs incorrectly and re-break theencode(U)contract you fixed elsewhere.Suggested fix
+ const fallbackParams = urlSearchParamsToMap( + pathToUrl(currentPath).searchParams, + ); const targetParams = - matched ?? urlSearchParamsToMap(pathToUrl(currentPath).searchParams); + matched ?? fallbackParams; @@ const targetPath = matched ? currentPath - : targetTemplate.fill(targetParams); + : targetTemplate.fillWithoutEncode(fallbackParams);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/plugin-history-sync/src/historySyncPlugin.tsx` around lines 344 - 354, The unmatched-fallback branch for computing targetPath uses targetTemplate.fill(targetParams), which re-encodes URL-derived string params and can violate non-identity encoder contracts; change that call to targetTemplate.fillWithoutEncode(targetParams) so the parsed/unmatched params are inserted without additional encoding. Update the assignment to targetPath (the code around targetTemplate.parse(currentPath), targetParams and targetPath) to use fillWithoutEncode in the else branch and keep the matched branch as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@extensions/plugin-history-sync/src/historySyncPlugin.tsx`:
- Around line 344-354: The unmatched-fallback branch for computing targetPath
uses targetTemplate.fill(targetParams), which re-encodes URL-derived string
params and can violate non-identity encoder contracts; change that call to
targetTemplate.fillWithoutEncode(targetParams) so the parsed/unmatched params
are inserted without additional encoding. Update the assignment to targetPath
(the code around targetTemplate.parse(currentPath), targetParams and targetPath)
to use fillWithoutEncode in the else branch and keep the matched branch as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a30065ae-8a96-461c-9a10-0ca50b14d99b
📒 Files selected for processing (2)
extensions/plugin-history-sync/src/historySyncPlugin.spec.tsextensions/plugin-history-sync/src/historySyncPlugin.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note on direction (for reviewers)
This PR's runtime behavior change ("always-string at plugin boundary") may sound opposite to "remove the type restriction" — that ambiguity comes from the originating user request, which actually asked for the opposite of widening the type:
The user's pain:
useActivityParams()returned different runtime types depending on entry path —push({ visible: true })left a boolean in the store while URL arrival parsed the same value as"true". Consumers were string-converting params before every push as a workaround.This PR implements the user's actual request (always string at runtime, consistent across all entry paths) rather than widening the type to
unknown. TheActivityBaseParamsdeclaration ({ [key: string]?: string }) is unchanged — this PR brings the runtime contract into alignment with the existing type.If the project maintainers prefer widening the type instead, this PR is the wrong direction and a different approach is needed. Please confirm direction before promoting from draft.
Summary
activityParams/stepParamstostring | undefinedat the@stackflow/plugin-history-syncboundary souseActivityParams()returns the same runtime shape regardless of how the activity was entered (push/replace/stepPush/stepReplace/ URL arrival with or withoutdecode/ cross-deployhistoryStatehydration).encodestill receives the original typed paramsU— coercion runs afterencodeconsumes them to build the URL.fillWithoutEncodetomakeTemplateso post-effect URL writers (onPushed,onReplaced,onStepPushed,onStepReplaced,onInit) do not re-runencodeon already-coerced store values.parseStateearly-return: typed values serialized intohistory.stateby an old deploy are coerced to strings on the new deploy.stepContext.path?: stringtoStepPushedEvent/StepReplacedEvent(purely additive, no breaking change) so post-effect hooks can read pre-encoded URLs from the store andhistory.locationreflectsencodeoutput across all navigation paths including popstate forward across step boundaries.Test plan
yarn workspace @stackflow/plugin-history-sync test— 7 suites, 152 active passing (4 intentional skips, 156 total)yarn workspace @stackflow/plugin-history-sync typecheck— exit 0yarn workspace @stackflow/plugin-history-sync build— exit 0 (esbuild + tsc)yarn workspace @stackflow/core test/typecheck/build— exit 0 (additivestepContext.pathdoes not break existing behavior)extensions/plugin-history-sync/src/index.ts) byte-identical vsmaincoerceParamsToStringconfirmed module-private (not re-exported)encodecontract preserved — encode receives typedU, never pre-stringified strings (T-I1 ORDER LOCK)decode(T-I3 + existing decode-path test)parseStateearly-return hydration (T-I5)useHashmode coerces identically to history mode (F5)replace-to-different-route updatesactivityContext.pathAND coerces params atomically (F3)useActivityParams()-backing store layer returns string-only after typed push (F9)history.locationreflectsencodeoutput across all navigation paths (T-O-1..T-O-8) including popstate forward across step boundaries (T-O-7)*.context.pathpropagation (noencodere-call on coerced strings)Migration notes
See
.changeset/coerce-activity-params-to-string.mdand.changeset/step-context-path.mdfor the full migration. tl;dr:decodeauthors: if you returned typed values (e.g.decode: (p) => ({ count: Number(p.count) })) and read them back as numbers viauseActivityParams(), those values are now strings in the store. Move runtime type coercion to the usage site:Number(useActivityParams().count).historySyncPluginlast among plugins that mutateactivityParamsto preserve the string-only invariant (locked by regression test).history.state, the params are coerced at hydration time. No consumer change required.stepContext.path: new optional field onStepPushedEvent/StepReplacedEvent. Purely additive — existing consumers see no API surface change.🤖 Generated with Claude Code