Guard store re-initialization to prevent divergent state in deferred hydration (#4572)#4591
Guard store re-initialization to prevent divergent state in deferred hydration (#4572)#4591AbanoubGhadban wants to merge 2 commits into
Conversation
…hydration Before hydrate_on scheduling (#4037), all components hydrated in one synchronous pass, reading the same store instance. Deferred hydration (visible/idle) breaks this: forEachStore can run again via reactOnRailsComponentLoaded (e.g., for a Turbo Frame), re-creating the store while an immediate sibling keeps the old instance and a deferred sibling will get the new one — divergent shared state with no error. The fix adds a guard in initializeStore to skip re-creating a store that already exists in hydratedStores, restoring the one-instance-per- name-per-page-lifetime invariant. Fixes #4572 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesStore Initialization Consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant StoreRegistry
participant ImmediateComponent
participant DeferredComponent
Renderer->>StoreRegistry: initialize shared store
Renderer->>ImmediateComponent: hydrate immediately
Renderer->>DeferredComponent: schedule visible hydration
Renderer->>StoreRegistry: check shared store on re-render
StoreRegistry-->>Renderer: return existing instance
DeferredComponent->>StoreRegistry: resolve shared store
StoreRegistry-->>DeferredComponent: return same instance
Renderer->>StoreRegistry: clear hydrated stores on page unload
StoreRegistry-->>Renderer: remove store instance
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (StoreRegistry.getStore(name, false) !== undefined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
When Turbo or Turbolinks replaces a page or frame with new markup that uses the same store name, hydratedStores can still contain the old page's store because the unload path only unmounts components. This early return then skips storeGenerator for the new store props, so the new components mount with stale state from the previous page or frame.
Context Used: CLAUDE.md (source)
|
+ci-run-hosted |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff5ec3ebb0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (StoreRegistry.getStore(name, false) !== undefined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This guard makes hydrated stores live beyond a single page: the core unload handler only unmounts components and clears renderedRoots (unmountAllComponents), but it never calls StoreRegistry.clearHydratedStores(), while clientStartup runs renderAllComponents() again on each Turbo/Turbolinks page load. After visiting one page with redux_store('appStore', props: old), navigating to another page that emits the same store name with new props will hit this early return, skip the store generator, and all components on the new page will read the previous page's store instead of the fresh store the helper is documented to create.
Useful? React with 👍 / 👎.
github-actions
Bot
added
the
ready-for-hosted-ci
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // instance and a not-yet-mounted :visible sibling will get the new one — divergent shared state. | ||
| // Skipping re-initialization restores the one-instance-per-name-per-page-lifetime invariant. | ||
| // See: https://github.com/shakacode/react_on_rails/issues/4572 | ||
| if (StoreRegistry.getStore(name, false) !== undefined) { |
There was a problem hiding this comment.
Correctness: this guard can pin a stale store across Turbo full-page navigations, not just within a single page's deferred hydration.
hydratedStores is only ever cleared via the public ReactOnRails.clearHydratedStores() API (called from generated SSR bundle code in helper.rb), never from the client's page-unload path. unmountAllComponents (registered via onPageUnloaded, i.e. turbo:before-render/turbolinks:before-render) tears down rendered roots but does not call StoreRegistry.clearHydratedStores().
Before this PR, initializeStore unconditionally re-created the store on every forEachStore() call, which incidentally gave "fresh store per Turbo page visit" as a side effect (in addition to causing the same-page divergence bug this PR fixes). With the new guard, once a store name (e.g. a common "CurrentUserStore" hydrated on every layout) is registered on page A, navigating via Turbo to page B — which re-renders a store element with the same name but different server-rendered props — will hit this early return and keep reusing page A's stale store/state instead of hydrating with page B's props.
Compare with the Pro package, which clears its per-page store cache on unload: packages/react-on-rails-pro/src/ClientSideRenderer.ts's unmountAll() calls unmountAllStores() (storeRenderers.clear()) in addition to unmountAllComponents(). This PR's core-package fix is missing the equivalent reset — e.g. calling StoreRegistry.clearHydratedStores() from unmountAllComponents() here — so the "restore one-instance-per-name" fix only holds within a single page's lifetime if something also resets the registry between Turbo page loads.
The added tests only exercise repeated forEachStore calls within the same simulated page (no runPageUnload() between renders), so this regression isn't covered. Worth adding a test that runs runPageUnload() (simulating Turbo navigation) between two renders with the same store name and different props, asserting the store is re-created in that case.
(Greptile's automated review already flagged this same issue on the latest commit ff5ec3eb.)
Review: Guard store re-initialization to prevent divergent state in deferred hydrationWhat this PR doesAdds a guard in Correctness issue (left as an inline comment)The guard as written checks only "does a hydrated store with this name exist," with no notion of which page it was hydrated for. Before this change, For comparison, the Pro package's client renderer clears its equivalent per-page cache on unload: Note: Greptile's automated review (posted on the current head commit Test coverage gapBoth new tests in Minor notes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-on-rails/tests/ClientRenderer.test.ts (1)
1452-1454: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove
IntersectionObservercleanup toafterEachto prevent mock leakage on test failure.The
IntersectionObservermock is restored at the end of the test body. If the test fails before reaching line 1452, the mock leaks to subsequent test suites. Move cleanup to theafterEachblock (or a dedicated teardown) so it runs regardless of test outcome.♻️ Proposed fix
afterEach(() => { StoreRegistry.clearHydratedStores(); StoreRegistry.clearStoreGenerators(); + const observerGlobal = globalThis as unknown as { IntersectionObserver?: unknown }; + delete observerGlobal.IntersectionObserver; });Then remove the inline cleanup at the end of the test:
- // Cleanup - const observerGlobal = globalThis as unknown as { IntersectionObserver?: unknown }; - delete observerGlobal.IntersectionObserver; });🤖 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 `@packages/react-on-rails/tests/ClientRenderer.test.ts` around lines 1452 - 1454, Move the IntersectionObserver cleanup from the test body into the surrounding afterEach or dedicated teardown, using the existing observerGlobal setup so it executes even when the test fails. Remove the inline delete at the end of the test while preserving cleanup of globalThis.IntersectionObserver.
🤖 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 `@packages/react-on-rails/tests/ClientRenderer.test.ts`:
- Around line 1452-1454: Move the IntersectionObserver cleanup from the test
body into the surrounding afterEach or dedicated teardown, using the existing
observerGlobal setup so it executes even when the test fails. Remove the inline
delete at the end of the test while preserving cleanup of
globalThis.IntersectionObserver.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e88de60-e766-4f5a-8be7-7dceecf8bce4
📒 Files selected for processing (2)
packages/react-on-rails/src/ClientRenderer.tspackages/react-on-rails/tests/ClientRenderer.test.ts
Addresses review feedback: the store re-initialization guard alone would cause stale state when navigating (via Turbo/Turbolinks) to a new page that uses the same store name with different props. Now stores are cleared on page unload, ensuring: 1. Within a single page: stores are stable (not re-created by forEachStore) 2. Across navigation: stores are fresh (re-created with new page's props) This matches the Pro package behavior where unmountAllStores() clears the storeRenderers map on page unload. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
size-limit report 📦
|
Addressed review feedback@greptile-apps raised a valid concern about stale stores surviving navigation. Fixed in d0015db: Added
This matches the Pro package behavior where Added test: |
|
The fix is well-implemented. The two-part solution correctly handles both scenarios:
A few observations:
Ordering of Test coverage — the added test This mirrors the Pro package behavior faithfully and the invariants are well-documented in the code comments. LGTM. |
| // instance and a not-yet-mounted :visible sibling will get the new one — divergent shared state. | ||
| // Skipping re-initialization restores the one-instance-per-name-per-page-lifetime invariant. | ||
| // See: https://github.com/shakacode/react_on_rails/issues/4572 | ||
| if (StoreRegistry.getStore(name, false) !== undefined) { |
There was a problem hiding this comment.
Good fix for the divergent-store bug. One trade-off worth calling out explicitly (in the comment or docs): with this guard, a data-js-react-on-rails-store element that reappears with different props (e.g. a Turbo Frame partial reload that re-renders the store's redux_store tag with fresh server data, without a full Turbo Drive navigation) will now be silently ignored — initializeStore returns before even parsing el.textContent. That's presumably intentional (it's what restores the one-instance invariant), but it's a behavior change for any app that was relying on reactOnRailsComponentLoaded to refresh store props mid-page. Might be worth a line in the hydrate_on docs (docs/oss/building-features/hydration-scheduling.md) noting that store props are only read once per page lifetime.
|
|
||
| // Register cleanup on page unload | ||
| onPageUnloaded(unmountAllComponents); | ||
| onPageUnloaded(clearAllStores); |
There was a problem hiding this comment.
Nice — this closes the gap the guard above would otherwise open (stores never re-initializing across Turbo navigations). Confirmed onPageUnloaded fires this after unmountAllComponents (Set insertion order), though the two don't currently depend on each other's side effects, so ordering isn't load-bearing today — just flagging in case that changes later.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0015dbb50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (StoreRegistry.getStore(name, false) !== undefined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
When a Turbo Frame/AJAX fragment that owns FrameStore is replaced without a full page unload, reactOnRailsComponentLoaded() still calls forEachStore, but this guard now returns solely because the old store name remains in StoreRegistry. The newly inserted redux_store JSON is ignored (the new page-unload clear only runs from onPageUnloaded, not component-loaded partials), so the replacement component mounts against stale props from the removed fragment. Fresh evidence versus the earlier navigation comment is that this path is a partial replacement where no unload callback clears the registry.
Useful? React with 👍 / 👎.
ReviewOverviewFixes #4572: Code quality
Tests
Notes / suggestions (non-blocking)
No correctness, security, or performance issues found otherwise. The change is small, well-targeted, and directly addresses the reported bug. |
Addressing review commentsAll automated reviewers (Greptile, Claude, Codex) flagged the same issue on the first commit: stores surviving Turbo navigations. This is now fixed in the second commit (d0015db) which adds Summary of behavior:
Re: documentation suggestionGood point about documenting this behavior. This is primarily relevant to the new |
Re: Codex P2 (Turbo Frame partial replacement)The P2 comment raises a valid edge case: when a Turbo Frame replaces content (without a full page unload), the store keeps its existing state rather than re-initializing with new props. This is intentional and correct for the bug being fixed:
Apps that need to update store state when a Turbo Frame loads should:
This matches the Pro package behavior where stores are stable within a page and cleared only on full navigation. |
1 participant
Summary
initializeStoreto skip re-creating a store that already exists inhydratedStoresProblem
Before
hydrate_onscheduling (#4037), all components hydrated in one synchronous pass, so every component read the same store instance. Deferred hydration (visible/idle) breaks this invariant:renderAllComponents()creates store S1:immediate) hydrates and gets S1:visible) is scheduled but doesn't mount yetreactOnRailsComponentLoaded(), which re-runsforEachStoreinitializeStoreunconditionally creates a new store S2, overwriting S1 in the registrysameNodeguard, so it keeps S1Result: A dispatches to S1, B dispatches to S2 — divergent shared state with no error.
Fix
Check if the store already exists in
hydratedStoresbefore creating a new one. This matches the Pro package's behavior which already guards via thestoreRenderersmap.Test plan
forEachStorerunning multiple times doesn't re-create storesFixes #4572
🤖 Generated with Claude Code
Summary by CodeRabbit