[WIP] fix(plugin-history-sync): prevent SSR hydration mismatch with non-empty defaultHistory#713
[WIP] fix(plugin-history-sync): prevent SSR hydration mismatch with non-empty defaultHistory#713ENvironmentSet wants to merge 13 commits into
Conversation
🦋 Changeset detectedLatest commit: 90c18e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDefers historySyncPlugin's initial defaultHistory setup from synchronous onInit to a guarded client post-commit useEffect, adds Jest polyfills and SSR/hydration tests, adjusts core tests to emulate the render-driven kickoff, and regenerates Yarn PnP entries for the new test dependencies. ChangesSSR Hydration Mismatch Fix
Dependency Updates
Sequence DiagramsequenceDiagram
participant Client as Client render (first paint)
participant WrapStack as wrapStack useEffect
participant Dispatcher as dispatchInitialSetupNavigation
participant CoreActions as coreActions / actions
Client->>WrapStack: first paint completes
WrapStack->>Dispatcher: call dispatchInitialSetupNavigation(coreActions)
Dispatcher->>CoreActions: read initialSetupProcess pending navigations
Dispatcher->>CoreActions: call actions.push / actions.stepPush for each navigation
Dispatcher->>Dispatcher: refresh activation monitors
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 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. Comment |
Deploying stackflow-demo with
|
| Latest commit: |
90c18e7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://07db8000.stackflow-demo.pages.dev |
| Branch Preview URL: | https://feature-fep-2349.stackflow-demo.pages.dev |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | 90c18e7 | Commit Preview URL | Jun 01 2026, 08:08 AM |
Design rationale / alternatives consideredThe mismatch only needs one thing fixed: the staged Premise: you can't SSR a stack that is mid-stacking-animation. A server snapshot is a single static frame, so it must be frame 0, with the animation continuing on the client after hydration. Every viable fix therefore keeps the server at frame 0 and defers the kickoff past the first commit. 1. Collapse the staged setup under SSR (render the whole stack at rest). Treat SSR like 2. Move 3. Defer only the kickoff under SSR via Chosen: kick off the staged setup from a
Trade-off: for a route with a non-empty |
ENvironmentSet
changed the title
…ty defaultHistory The staged `defaultHistory` setup navigation was kicked off synchronously in the `onInit` hook, which runs during the first client render. The client's first render therefore contained more activities than the server-rendered output, causing a React hydration mismatch. Kick off the setup navigation from a post-commit effect in `wrapStack` instead, so the server and the client's first render produce identical output. The staged "stacking" setup animation still plays after hydration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ENvironmentSet
force-pushed
the
feature/fep-2349
branch
from
b346473 to
0b48b62
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
203-209: ⚡ Quick winReplace ternary with if/else to avoid misleading return.
The ternary expression returns a value that
.forEach()ignores. While functionally correct, this style implies the return matters. Use explicit if/else for side-effect-only callbacks.♻️ Proposed fix
initialSetupProcess ?.captureNavigationOpportunity(stack) - .forEach((event) => - event.name === "Pushed" - ? actions.push(event) - : actions.stepPush(event), - ); + .forEach((event) => { + if (event.name === "Pushed") { + actions.push(event); + } else { + actions.stepPush(event); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/plugin-history-sync/src/historySyncPlugin.tsx` around lines 203 - 209, The forEach callback uses a ternary that suggests a return value even though it's used only for side effects; update the callback passed to initialSetupProcess?.captureNavigationOpportunity(stack).forEach(...) to use an explicit if/else: call actions.push(event) when event.name === "Pushed" otherwise call actions.stepPush(event). This change should be made where initialSetupProcess.captureNavigationOpportunity and the forEach are used so the intent is clear and no misleading return value is implied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@extensions/plugin-history-sync/src/historySyncPlugin.tsx`:
- Around line 203-209: The forEach callback uses a ternary that suggests a
return value even though it's used only for side effects; update the callback
passed to initialSetupProcess?.captureNavigationOpportunity(stack).forEach(...)
to use an explicit if/else: call actions.push(event) when event.name ===
"Pushed" otherwise call actions.stepPush(event). This change should be made
where initialSetupProcess.captureNavigationOpportunity and the forEach are used
so the intent is clear and no misleading return value is implied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c488c764-2cfe-4fb1-ac0f-22dcfdf0e31e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.changeset/fix-ssr-hydration-default-history.md.pnp.cjsextensions/plugin-history-sync/jest.setup.jsextensions/plugin-history-sync/package.jsonextensions/plugin-history-sync/src/historySyncPlugin.ssr.spec.tsxextensions/plugin-history-sync/src/historySyncPlugin.tsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-ssr-hydration-default-history.md
🚧 Files skipped from review as they are similar to previous changes (4)
- extensions/plugin-history-sync/jest.setup.js
- extensions/plugin-history-sync/src/historySyncPlugin.ssr.spec.tsx
- extensions/plugin-history-sync/package.json
- .pnp.cjs
commit: |
Resolve the v2 (future -> stable, #695) integration for the SSR hydration fix: - package.json: adopt @stackflow/* ^2.0.0; keep test-only devDeps (@stackflow/plugin-renderer-basic, react-dom, @testing-library/*, jest-environment-jsdom) - Rewrite the SSR/hydration spec for the v2 config API (defineConfig + stackflow({ config, components })) - In historySyncPlugin.spec.ts, trigger the staged defaultHistory kickoff explicitly in the one core-level test that asserts the target landed, since that push now runs in wrapStack's post-commit effect (not onInit). The shared test helper is left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ENvironmentSet
force-pushed
the
feature/fep-2349
branch
from
9d2f384 to
c554889
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extensions/plugin-history-sync/src/historySyncPlugin.tsx (1)
204-210: 💤 Low valuePrefer explicit branching over ternary in forEach callback.
The ternary returns a value that
forEachignores, triggering the Biome lint warning. Usingif/elsemakes the intent (execute side effects) explicit.♻️ Proposed fix
initialSetupProcess ?.captureNavigationOpportunity(stack) - .forEach((event) => - event.name === "Pushed" - ? actions.push(event) - : actions.stepPush(event), - ); + .forEach((event) => { + if (event.name === "Pushed") { + actions.push(event); + } else { + actions.stepPush(event); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/plugin-history-sync/src/historySyncPlugin.tsx` around lines 204 - 210, Replace the ternary used inside the forEach callback on initialSetupProcess.captureNavigationOpportunity(stack).forEach so the callback performs explicit branching for side effects: locate the forEach where the callback currently uses event.name === "Pushed" ? actions.push(event) : actions.stepPush(event) and change it to an if/else that calls actions.push(event) when event.name === "Pushed" and otherwise calls actions.stepPush(event), avoiding returning a value from the callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@extensions/plugin-history-sync/src/historySyncPlugin.tsx`:
- Around line 204-210: Replace the ternary used inside the forEach callback on
initialSetupProcess.captureNavigationOpportunity(stack).forEach so the callback
performs explicit branching for side effects: locate the forEach where the
callback currently uses event.name === "Pushed" ? actions.push(event) :
actions.stepPush(event) and change it to an if/else that calls
actions.push(event) when event.name === "Pushed" and otherwise calls
actions.stepPush(event), avoiding returning a value from the callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b703860-b16f-434a-b2b2-7a5a04cdf0e6
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.changeset/fix-ssr-hydration-default-history.md.pnp.cjsextensions/plugin-history-sync/jest.setup.jsextensions/plugin-history-sync/package.jsonextensions/plugin-history-sync/src/historySyncPlugin.spec.tsextensions/plugin-history-sync/src/historySyncPlugin.ssr.spec.tsxextensions/plugin-history-sync/src/historySyncPlugin.tsx
✅ Files skipped from review due to trivial changes (2)
- .changeset/fix-ssr-hydration-default-history.md
- .pnp.cjs
🚧 Files skipped from review as they are similar to previous changes (3)
- extensions/plugin-history-sync/jest.setup.js
- extensions/plugin-history-sync/package.json
- extensions/plugin-history-sync/src/historySyncPlugin.ssr.spec.tsx
…itions Review-driven fixes for the defaultHistory hydration test suite: - spec.ts (T-O-5): drive the render-only setup kickoff via kickOffDefaultHistorySetup and assert the "Article" destination lands before history.back(), so the destination replay can no longer silently regress now that init() no longer pushes it. - ssr.spec.tsx: delete global.window in test 3 so it faithfully simulates a non-browser server runtime; split the empty-defaultHistory coverage into explicit "empty" (() => []) and "none" route cases; wrap the hydration assertions in try/finally so fake timers and the container are always cleaned up on failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/plugin-history-sync/src/historySyncPlugin.ssr.spec.tsx`:
- Around line 51-77: The TypeScript error arises because the inferred union type
of the local variable articleRoute isn't being recognized as a RouteLike by the
compiler; update buildCoreStore by either casting the computed articleRoute to
the proper type (e.g., assert articleRoute as RouteLike<unknown>) before passing
it into the routes object for historySyncPlugin, or remove the intermediate
variable and inline the switch directly inside the routes.Article value so the
expression is inferred as RouteLike; modify the symbol articleRoute (or the
inline switch in buildCoreStore when constructing routes for historySyncPlugin)
accordingly.
🪄 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: 08c903cd-1ba5-4a2c-a810-4a4501e6800d
📒 Files selected for processing (2)
extensions/plugin-history-sync/src/historySyncPlugin.spec.tsextensions/plugin-history-sync/src/historySyncPlugin.ssr.spec.tsx
1 participant
Problem
When an activity route declares a non-empty
defaultHistory, the initial stack rendered on the server does not match the stack rendered on the client's first (hydration) render, producing a React hydration mismatch (Hydration failed because the initial UI does not match what was rendered on the server). React then discards the server-rendered HTML and re-renders on the client.Root cause
historySyncPluginbuilds thedefaultHistoryback stack as a stagedSerialNavigationProcess: the constructor (overrideInitialEvents) releases only the first entry, and the destination activity is pushed later by a kickoff inside theonInithook.onInitruns synchronously during the first client render (viastore.init()inside the<Stack>useMemo), but it is skipped on the server (it is browser-only). So the server renders the first frame ([firstEntry]) while the client's first render already contains[firstEntry, destination]— a structural mismatch.Fix
Kick off the staged setup navigation from a post-commit
useEffectin the plugin'swrapStackhook instead of fromonInit. Effects never run during SSR and run after the first commit on the client, so:onChangednotification path.The URL reconciliation and
popstatelistener stay inonInit(they don't touch the core stack, so their timing is unchanged). EmptydefaultHistoryroutes are unaffected, and the change is isolated to@stackflow/plugin-history-sync(both thestableandfutureintegrations are covered through the shared renderer).Tests
store.init()no longer advances a non-emptydefaultHistorysetup (the server frame), and an emptydefaultHistorystill resolves directly to its destination.renderToString→hydrateRootasserts no recoverable hydration error, then advances timers to confirm the staged setup animation still plays. These tests fail on the previous code (reproducing the mismatch) and pass with this change.yarn test,yarn typecheck, andyarn buildpass.🤖 Generated with Claude Code