[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 2 commits into
Conversation
🦋 Changeset detectedLatest commit: c554889 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 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR defers historySyncPlugin's initial defaultHistory setup from synchronous onInit execution to a guarded client-only post-commit useEffect, adds Jest polyfills and test infra, introduces SSR/hydration regression tests, and regenerates Yarn PnP entries for the added 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: |
c554889
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ae7c031b.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 | c554889 | Commit Preview URL | May 31 2026, 03:18 PM |
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
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