fix: clean up renderer functions on unmount (#3209)#3210
fix: clean up renderer functions on unmount (#3209)#3210AbanoubGhadban wants to merge 1 commit intomainfrom
Conversation
Establishes the TDD baseline for issue #3209. The framework currently discards anything a renderer function returns, so renderer-mounted React roots leak across Turbo navigation and same-id node replacement. Unit (jsdom): packages/react-on-rails/tests/ClientRenderer.test.ts gains an Issue #3209 describe block with a pageLifecycle mock that exposes __triggerPageUnload, plus three tests: - teardown invoked on page unload (fails today) - teardown invoked on same-id node replacement (fails today) - renderer returning undefined does not throw (passes today, regression guard for the optional-teardown contract) E2E (Playwright): Adds renderer_cleanup.spec.js exercising both paths through the dummy app, with the canary being useEffect cleanup inside the rendered tree (only fires when root.unmount() actually runs). Supporting dummy-app scaffolding: RendererCleanupTest.jsx (renderer that returns the proposed teardown), renderer_cleanup_test.html.erb view, route, and client-bundle.js registration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
AbanoubGhadban
changed the title
size-limit report 📦
|
Code Review — PR #3210 (TDD baseline for #3209)Good TDD discipline: the failing-tests-first approach makes the contract explicit before implementation. The mock design for 🐛 Critical: E2E node-replacement test will fail even after the fix
The assertion
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| }; | ||
|
|
||
| const RendererCleanupTest = (_props, _railsContext, domNodeId) => { | ||
| window.__rendererCleanupRan__ = false; |
There was a problem hiding this comment.
Bug: This reset fires on every call to RendererCleanupTest, including the second call that happens after the teardown runs during node replacement. The order of events after the fix will be:
- Framework detects stale node → calls first teardown →
root.unmount()→ useEffect cleanup →window.__rendererCleanupRan__ = true - Framework calls
RendererCleanupTest(...)again for the new node → this line resets the flag tofalse
The E2E node-replacement assertion then reads false and fails even with a correct implementation.
Fix: Remove this line from the renderer and set the flag to false in the beforeEach of the E2E test only (or use a monotonic counter):
| window.__rendererCleanupRan__ = false; | |
| // Flag is initialised to false in the test's beforeEach, not here, | |
| // so the value set by the useEffect cleanup isn't overwritten by re-renders. |
| jest.mock('../src/pageLifecycle.ts', () => { | ||
| const unloadCallbacks: Array<() => void | Promise<void>> = []; | ||
| return { | ||
| __esModule: true, | ||
| onPageUnloaded: (cb: () => void | Promise<void>) => { | ||
| unloadCallbacks.push(cb); | ||
| }, | ||
| onPageLoaded: () => {}, | ||
| __triggerPageUnload: async () => { | ||
| for (const cb of [...unloadCallbacks]) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await cb(); | ||
| } | ||
| }, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The unloadCallbacks array is created once per test file and is never cleared. ClientRenderer.ts registers unmountAllComponents into it at module-load time; that's fine for the current tests. But if any future test path exercises code that calls onPageUnloaded a second time (e.g., testing re-initialization), those callbacks will double-register and triggerPageUnload will fire unmountAllComponents twice — silently incorrect behavior.
Consider adding a reset hook to the mock:
| jest.mock('../src/pageLifecycle.ts', () => { | |
| const unloadCallbacks: Array<() => void | Promise<void>> = []; | |
| return { | |
| __esModule: true, | |
| onPageUnloaded: (cb: () => void | Promise<void>) => { | |
| unloadCallbacks.push(cb); | |
| }, | |
| onPageLoaded: () => {}, | |
| __triggerPageUnload: async () => { | |
| for (const cb of [...unloadCallbacks]) { | |
| // eslint-disable-next-line no-await-in-loop | |
| await cb(); | |
| } | |
| }, | |
| }; | |
| }); | |
| jest.mock('../src/pageLifecycle.ts', () => { | |
| let unloadCallbacks: Array<() => void | Promise<void>> = []; | |
| return { | |
| __esModule: true, | |
| onPageUnloaded: (cb: () => void | Promise<void>) => { | |
| unloadCallbacks.push(cb); | |
| }, | |
| onPageLoaded: () => {}, | |
| __triggerPageUnload: async () => { | |
| for (const cb of [...unloadCallbacks]) { | |
| // eslint-disable-next-line no-await-in-loop | |
| await cb(); | |
| } | |
| }, | |
| __resetCallbacks: () => { | |
| unloadCallbacks = []; | |
| }, | |
| }; | |
| }); |
Then call (pageLifecycle as any).__resetCallbacks() in beforeEach if any test needs a clean slate.
1 participant
Summary
TDD baseline for issue #3209 — renderer functions (3-arg
(props, railsContext, domNodeId) => …) are never cleaned up on Turbo navigation or same-id node replacement, leaking the React roots they mount.This PR is intentionally just the failing tests. The framework implementation (extending
RenderFunction's return contract, tracking the teardown inrenderedRoots, invoking it on unmount + replaced-node) lands in follow-up commits on this same branch.packages/react-on-rails/tests/ClientRenderer.test.tsgains anIssue #3209describe block with apageLifecyclemock that exposes__triggerPageUnload. Two tests fail today (teardown not invoked on page unload; teardown not invoked when the samedomNodeIdis replaced); one regression-guard test passes today (renderer returningundefineddoes not throw on unmount).react_on_rails/spec/dummy/e2e/playwright/e2e/react_on_rails/renderer_cleanup.spec.jsexercises both paths through the dummy app. The canary isuseEffectcleanup inside the rendered tree — it only fires whenroot.unmount()actually runs, so today it never fires and the assertion fails.RendererCleanupTest.jsx(renderer returning the proposed teardown),renderer_cleanup_test.html.erbview, route entry, andclient-bundle.jsregistration.Refs #3209.
Status
Expected: 1 / Received: 0, 1 passes as guard)--listconfirms parse across chromium/firefox/webkit)RenderFunctiontype widening +ClientRenderertracking)wrapServerComponentRenderer/client.tsxreturns its teardownClientSideRenderermirror change + Pro test analogrender-functions.md,view-helpers-api.md)Test plan
npx jest tests/ClientRenderer.test.ts -t 'Issue #3209'after the framework change — all three tests should pass.react-on-railsjest suite stays green (the newpageLifecyclemock is scoped to one file)./renderer_cleanup_test, click the Turbo link, observewindow.__rendererCleanupRan__ === trueafter navigation.🤖 Generated with Claude Code