Adopt renderer-function teardown in dummy apps + docs (#3578)#3585
Conversation
|
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:
WalkthroughDocs add an optional renderer teardown return; many OSS and Pro dummy renderer/startup entrypoints now capture React roots and return ChangesRenderer teardown contract implementation and adoption
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adopts the
Confidence Score: 5/5Safe to merge — only spec fixtures, documentation, and a JSDoc comment; no changes to production runtime paths. All changed files are either test fixtures (dummy renderers), documentation, or test suites with no modifications to shipped runtime logic beyond what was already established in the parent PR #3576. The one finding is a mismatch between the suggested grep strings in the async-teardown note and the actual log messages in ClientRenderer.ts, which only affects developer debugging ergonomics. docs/oss/api-reference/view-helpers-api.md — the async-teardown console.error search strings in the [!NOTE] block don't match what ClientRenderer.ts actually logs. Important Files Changed
Sequence DiagramsequenceDiagram
participant RoR as ReactOnRails (core)
participant CR as ClientRenderer
participant Renderer as Renderer Function (3-arg)
participant Root as React Root
participant Turbo as Turbo/Turbolinks
RoR->>CR: renderElement(el, railsContext)
CR->>CR: delegateToRenderer(componentObj, props, ...)
CR->>Renderer: renderer(props, railsContext, domNodeId)
Renderer->>Root: createRoot / hydrateRoot
Renderer-->>CR: "{ teardown } or Promise<{ teardown }>"
CR->>CR: trackRendererMount(domNodeId, domNode, result)
CR->>CR: "renderedRoots.set(domNodeId, { kind: 'renderer', teardown })"
Note over CR: Page navigation triggered
Turbo->>CR: unmountAllComponents()
CR->>CR: renderedRoots.forEach(teardownEntry)
CR->>Renderer: teardown()
Renderer->>Root: root.unmount()
Note over CR: Pro path — async renderer race
CR->>Renderer: await renderer(...)
Renderer-->>CR: Promise (pending)
Turbo->>CR: "unmountAll() — state = 'unmounted'"
Renderer-->>CR: "Promise resolves { teardown }"
CR->>Renderer: invokeRendererTeardown(teardown) — runs late teardown
Reviews (3): Last reviewed commit: "Address renderer teardown review nits" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review: Adopt renderer-function teardown in dummy apps + docs (#3578)Overall: This is a clean, low-risk follow-up. The core teardown contract was established in the stacked PR (#3576); this PR correctly wires it into every in-tree dummy renderer and documents it. No public behavior changes. A few things worth addressing before merge: Bugs / Correctness1. HMR + In The pre-existing behavior was the opposite problem: calling const renderApp = (Komponent) => {
const element = wrapElementInStrictMode(...);
if (root) {
root.render(element); // update existing root on HMR; no unmount needed
} else {
root = hydrateOrRender(document.getElementById(domNodeId), element);
}
};These are spec/dummy fixtures so the real-world impact is low, but the current code swaps one hydration defect for another. Documentation2. The new example in the "Cleaning up on Turbo/Turbolinks navigation" subsection detects SSR content with let root;
if (domNode.innerHTML) {
root = ReactDOMClient.hydrateRoot(domNode, <MyComponent {...props} />);
} else {
root = ReactDOMClient.createRoot(domNode);
root.render(<MyComponent {...props} />);
}
return () => root.unmount();Also worth noting: the actual dummy apps use 3. The table at the top of Minor4. Redundant In the What's done well
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
size-limit report 📦
|
justin808
force-pushed
the
jg-conductor/3209-fix
branch
from
a97f367 to
6e51939
Compare
justin808
force-pushed
the
jg-conductor/3578-implement
branch
from
0b7771b to
bae8c50
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code ReviewOverall: This is a clean, low-risk follow-up that correctly adopts the renderer-function teardown contract across all in-tree dummy apps. The core implementation changes are sound — capturing root handles, returning teardown callbacks, and fixing the double- 🔴 Docs example uses unreliable
|
Apply review feedback to the renderer-function teardown docs (PR #3585): - view-helpers-api.md: rewrite the React 18 teardown example as an explicit if/else (no double innerHTML read) and add an `if (!domNode) throw` guard so a missing element yields a named-id error instead of a cryptic null deref; add the same guard to the React 16/17 example. - view-helpers-api.md: note that a dropped async teardown is logged via console.error (diagnosable, not silent), matching the core ClientRenderer. - render-functions.md: use react-dom/client's hydrateRoot (with import) to match the view-helpers example instead of the invalid ReactDOM.hydrateRoot, and clarify the LazyHydrate teardown is only registered after hydration runs. Docs-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review: Adopt renderer-function teardown in dummy apps + docs (#3578)Overall: well-structured follow-up to #3576. Approved with minor notes. This PR consistently adopts the new teardown contract across all in-tree renderer functions and documents it. No production runtime behavior is changed — all modifications are test fixtures, docs, and a JSDoc block. The pattern is applied uniformly and the code quality is good. What works well
IssuesMinor:
|
…th props.prerender (#3578) Address PR #3585 review feedback on the renderer-function teardown changes: - Redux dummy renderers (ReduxApp.client.jsx, ReduxSharedStoreApp.client.jsx): on hot reload, re-render the existing root in place instead of unmount-then-rehydrate. Re-creating the root emptied the DOM node and then called hydrateRoot on it, producing hydration-mismatch warnings whenever prerender was true. - Drop the dead `if (root)` guard in the returned teardown. renderApp runs unconditionally before the teardown is returned, so root is always set; this matches ManualRenderApp.jsx. - view-helpers-api.md: detect SSR via props.prerender (the Rails-set flag used by every in-tree renderer) instead of the fragile domNode.innerHTML heuristic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review feedback addressedThanks for the reviews. All actionable items (from @cursor[bot], @greptile-apps[bot], and @claude[bot]) are now resolved across two commits: 94a6e1398 —
1c5814b2f —
All corresponding review threads have been replied to and resolved. The remaining bot posts (size-limit/bencher reports, overall review summaries, coderabbit approval) were status/summary comments with no action required. |
Code ReviewOverviewThis follow-up to #3576 adopts the optional renderer-function teardown contract across all in-tree dummy renderers (OSS modern/React16, Pro ror-auto-load, Pro loadable) and documents it in three docs pages. No production runtime behavior changes — this is spec fixtures + docs + a JSDoc addition. Positive findingsHMR bug fix (OSS modern Redux apps) — the most important correctness improvement in this PR. The previous code called the top-level Loadable async teardown — React 16/17 teardown — using Docs quality — the new "Cleaning up on Turbo/Turbolinks navigation" subsection in Issue: documentation examples leak
|
…3578) Address review feedback on the renderer-function teardown docs (PR #3585): - view-helpers-api.md: the React 18 example led with an `if (props.prerender)` hydrate branch and called prerender "the Rails-set flag." prerender is not a prop React on Rails injects (the top-level prerender: option only controls SSR and is rejected for renderer functions); the in-tree renderers see it only because the spec/dummy views merge it into props themselves. Renderer functions are also client-only, so the container is empty. Rewrite the example to the correct common case (createRoot + render) and move hydration to an advanced note that explains you pass your own prop for a server/client split. - view-helpers-api.md: name the greppable console.error signatures ("resolved after its mount was removed" / "Error resolving renderer teardown") in the async best-effort note, and reword "(page unload)" to "(when the framework swaps in the next page)" since onPageUnloaded fires on turbo:before-render, not the browser beforeunload event. - render-functions.md: note the LazyHydrate concise-body arrow returns its whenVisible(...).then(...) promise implicitly, so switching to a block body without an explicit return would silently drop the teardown. Docs-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Code Review overview: This PR adopts the optional renderer-function teardown contract (from #3576) across all in-tree dummy renderers and adds documentation. Low-risk follow-up with no public API changes. Positives include HMR root-reuse bug fix (root.render path for hot reload), consistent unmount teardown pattern across React 16/17/18, safe loadableReady().then() conversion, clear @remarks block in types/index.ts, and good async teardown documentation. Issues: (1) Minor - verbose meta-commentary inside code examples in render-functions.md should move to prose/NOTE callout; (2) Minor - prerender prop name in view-helpers-api.md example collides confusingly with the prerender render option, consider renaming to serverRendered or isHydrating; (3) Informational - teardown callbacks are not asserted by any test (e.g. turbo:before-render simulation), worth a future integration test. Conclusion: Ready to merge after stacked PR (#3576) lands. Doc issues are polish items only. |
Code Review — PR #3585 (Adopt renderer-function teardown in dummy apps + docs)Overall: Well-scoped and well-documented. The teardown contract, HMR root-reuse fix, Issues1–2. Missing null guards in React 16 Redux renderers (minor, inconsistency)
3–4. Hardcoded
|
Code Review — PR #3585Overall: Well-structured, low-risk PR. The implementation is consistent, the HMR fix is correct, and the docs cover the important edge cases. A few things worth calling out below. Strengths
Issues1. Pre-existing
|
Pro Node Renderer Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Code ReviewOverall: LGTM with one minor fix needed. Low-risk follow-up to #3576 that adopts the already-merged teardown contract consistently across all in-tree renderer functions. The approach is correct, the HMR handling is thoughtful, and the documentation additions are clear. What the PR does
IssuesMinor bug — hardcoded
|
…lement * origin/main: Migrate OSS dummy client examples to TypeScript (#3606) Docs: use async props pattern in metadata and render-function examples (#3672) Require PR numbers in squash merge titles (#3684) Improve create-app smoke diagnostics (#3599) Suppress regression issues when only /posts_page: Pro regresses (#3668) Harden ShakaPerf artifact setup Polish RSC generator install follow-ups Harden renderer teardown polish fix(benchmark): harden target monitoring Docs: clarify rails new JavaScript skip flag (#3666) Tighten benchmark summary annotations Decouple Shakapacker config helpers from SystemChecker Derive Pro RSC client refs from the RSC graph (#3556) Docs: follow up v15 anchor and Rails 5.2 flag # Conflicts: # react_on_rails/spec/dummy/client/app/startup/ManualRenderApp.jsx # react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx # react_on_rails/spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx
Code ReviewOverviewThis PR correctly adopts the Critical: OSS modern startup files still leak roots after this PRThe PR description lists three OSS modern startup files as updated ( Current state on this branch (unchanged from main):
These three renderers will continue to leak their React roots on Turbo/Turbolinks navigation after this PR merges. The Medium:
|
Pro (shard 1/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Core Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 2/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
…cessing-flow * origin/main: Add maintainer +ci commands for full-CI dispatch and waivers (#3687) Adopt renderer-function teardown in dummy apps + docs (#3578) (#3585) Migrate OSS dummy client examples to TypeScript (#3606) Docs: use async props pattern in metadata and render-function examples (#3672) Require PR numbers in squash merge titles (#3684)
1 participant
Summary
Follow-up to #3209, after PR #3576 (the renderer-function teardown contract) merged into
mainas55fcdcf4af0629f32969dfa55f6fd71794ce200f. This adopts the explicit{ teardown }wrapper contract across the in-tree dummy renderer functions and documents the optional teardown wrapper return value. None of these change the public runtime contract — they adopt it in-tree and document it, exactly the checklist deferred in #3578.Closes #3578.
1. In-tree dummy renderers → return
{ teardown }Each renderer function (3-arg
(props, railsContext, domNodeId)form) now captures its React root and returns{ teardown: () => ... }so React on Rails unmounts it on Turbo/Turbolinks navigation (page unload) or same-id node replacement instead of leaking it.Decision: use the explicit
{ teardown }wrapper, not a bare callback. #3576 made the runtime/type contract explicit so legacy 3-argument render functions that happened to return a function are not misclassified as cleanup callbacks.OSS modern (
react_on_rails/spec/dummy/client/app/startup/):ManualRenderApp.jsxReduxApp.client.jsx—module.hotpath now reuses the existing root instead of creating a second rootReduxSharedStoreApp.client.jsx—module.hotpath now reuses the existing root instead of creating a second rootOSS legacy React 16 (
react_on_rails/spec/dummy/client/app-react16/startup/):ManualRenderApp.jsx,ReduxApp.client.jsx,ReduxSharedStoreApp.client.jsx— teardown unmounts viaReactDOM.unmountComponentAtNode(domNode)(no root handle in the React 16/17 API)Pro auto-load (
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/):ManualRenderApp.jsx,ReduxApp.client.jsx,ReduxSharedStoreApp.client.jsx,ApolloGraphQLApp.client.jsx,LazyApolloGraphQLApp.client.tsxPro loadable:
loadable/loadable-client.imports-loadable.jsx— returns a promise resolving to{ teardown }, since the root is created insideloadableReady()@loadable/componentversion confirmation: the Pro dummy app declares^5.16.3and the lockfile resolves5.16.7, above the Promise API floor (5.12.0).A note on the
module.hotpathsThe React 18
createRootrenderers (OSS modern Redux apps) now re-render on the same root during hot reload, so HMR no longer leaks a root or callscreateRoottwice on the same node. The React 16/17 renderers re-render into the same container idempotently (ReactDOM.render/hydrate), so there is no separate root to unmount first — their HMR path is unchanged, and the teardown handles unload/replacement cleanup.2. Docs
docs/oss/core-concepts/render-functions.md— theLazyHydrateexample captures the root and resolves to{ teardown }.docs/oss/api-reference/view-helpers-api.md— new "Cleaning up on Turbo/Turbolinks navigation" subsection with a Turbo-navigation example (React 18 + legacy) and the async best-effort caveat.docs/oss/api-reference/javascript-api.md— short reference note on the{ teardown }return value.3. Imperative
ReactOnRails.render(...)— decision recordedFlagged as out of scope in #3209 ("same family of problem; different surface; deferred").
Decision: document the caller's responsibility; do not auto-track. I added a
@remarksblock to therender()JSDoc inpackages/react-on-rails/src/types/index.tsstating that a root created by this imperative API is not tracked internally and the caller mustunmount()it themselves (e.g. onturbo:before-render), or use a renderer function for automatic cleanup.Rationale: auto-tracking these roots in
renderedRootswould be a behavior change that risks double-unmount for callers that already manageunmount()themselves, and #3209 explicitly deferred this surface. Documentation-only, no behavior change. If we later want tracked imperative renders, that can be a separate, opt-in API.Retarget/rebase status
mainas55fcdcf4af0629f32969dfa55f6fd71794ce200fon 2026-06-05.main.origin/mainand force-pushed with lease; current rebased top commit is667d1954.Follow-up
Non-blocking review nits from the final green #3585 pass are tracked in #3689 instead of delaying this PR. The
@loadable/componentPromise API floor is recorded above, and the legacy React 16 dummypropsmutation cleanup is intentionally deferred because #3585 preserves existing fixture behavior while adding teardown.The intentionally deferred surface remains the imperative
ReactOnRails.render(...)API documented below; if tracked imperative roots are desired later, that should be a separate opt-in API issue/PR.Acceptance criteria
{ teardown }that unmounts their root (and themodule.hotcreateRoot path reuses the previous root instead of creating a new one).{ teardown }return value with a Turbo-navigation example.ReactOnRails.render(...)path.Verification
origin/mainafter fix(client): clean up renderer-function mounts on unmount (#3209) #3576 merged; no conflicts.pnpm exec prettier --check ...changed files...— cleanpnpm exec eslint --report-unused-disable-directives ...changed JS/TS files...— cleanpnpm --filter react-on-rails run type-check— cleanpnpm --filter react-on-rails-pro run type-check— cleanpnpm --dir react_on_rails_pro/spec/dummy exec tsc -p tsconfig.json --noEmit --pretty false— no new errors; still reports the 4 pre-existingtests/strict-mode-support.test.tsxunknown-prop errors documented beforegit diff --check origin/main...HEAD— cleanpnpm run lint— currently blocked by unrelatedtest/shakaperf/rsc-foucartifact lint errors on main; those files are not touched by this PR.No CHANGELOG entry: per the project changelog guidelines these are test fixtures + docs + a JSDoc comment (not user-visible behavior), and #3576 already documents the user-facing contract.
Note
Low Risk
Changes are limited to test fixtures, documentation, and JSDoc; public teardown behavior was already introduced in the prior contract PR.
Overview
Adopts the optional
{ teardown }wrapper from the merged renderer teardown contract across in-tree dummy 3-argument renderer functions, plus documentation and a JSDoc note—no new runtime behavior in this PR.Dummy apps: OSS React 16 and Pro auto-load/loadable client startups now resolve the mount DOM node up front (throw if missing), capture the React root (or legacy container), and return
{ teardown: () => root.unmount() }(or a promise that resolves to it forloadableReady). That lets React on Rails unmount renderer-owned trees on Turbo/Turbolinks navigation and same-id node replacement instead of leaking roots.Docs:
render-functions.mdandview-helpers-api.mddocument optional teardown (React 18 + legacy examples, hydrate-vs-render notes, async teardown best-effort in OSS).javascript-api.mdbriefly mentions the return shape on 3-param renderers.Types:
ReactOnRails.render()JSDoc now states that imperative mounts are not auto-tracked—callers mustunmount()themselves or use a renderer function with{ teardown }for automatic cleanup.Reviewed by Cursor Bugbot for commit 200865b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes