Clean up renderer dummy teardown follow-ups#3698
Merged
Merged
Conversation
Contributor
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. 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 (3)
💤 Files with no reviewable changes (1)
WalkthroughThis PR refactors React 16 and Pro dummy app startup files to eliminate prop mutations and improve version documentation. The Redux startup functions now destructure props instead of deleting properties, and the loadable client documentation explicitly records version constraints. ChangesDummy app fixture cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
🚥 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 |
Greptile SummaryThis PR makes two targeted cleanups to the React 16 dummy app startup paths and adds a version-constraint comment to the Pro loadable dummy.
Confidence Score: 5/5All three files are dummy/spec app startup code; the changes are narrowly scoped and straightforward to verify. The props-mutation removal in ReduxApp.client.jsx is mechanically correct — destructuring rest produces a new object without prerender, which is exactly what composeInitialState should receive. In ReduxSharedStoreApp.client.jsx, props is not passed anywhere after extraction, so removing the mutation is a no-op in terms of behavior. The comment addition in the Pro loadable file is documentation-only. No production paths are touched. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["renderer fn(props, railsContext, domNodeId)"] --> B["Destructure: { prerender, ...componentProps } = props"]
B --> C["render = prerender ? hydrate : render"]
B --> D["composeInitialState(componentProps, railsContext)"]
D --> E["createStore(reducer, combinedProps, middleware)"]
E --> F["renderApp → render(element, domNode)"]
F --> G["return { teardown: () => unmountComponentAtNode(domNode) }"]
subgraph "Before (mutation)"
X1["const { prerender } = props"]
X2["delete props.prerender ❌ mutates caller's object"]
X3["composeInitialState(props, railsContext)"]
X1 --> X2 --> X3
end
subgraph "After (immutable)"
Y1["const { prerender, ...componentProps } = props"]
Y2["composeInitialState(componentProps, railsContext) ✅"]
Y1 --> Y2
end
Reviews (1): Last reviewed commit: "Clean up renderer dummy teardown follow-..." | Re-trigger Greptile |
Contributor
Code ReviewOverviewA clean, well-scoped cleanup (4 additions, 6 deletions across 3 test/dummy files). No production library code is touched.
FindingsCorrectness — correct The rest-destructuring fix in Incomplete sweep — flagging for a follow-up, out of scope for this PR Two similar
Both still carry the Comment freshness — minor nit (see inline) The added comment in Risk assessmentLow. All changes are in test/dummy startup files. No library behaviour, no auth, no data path. Safe to merge as-is; the Pro-package cleanup is a suitable follow-up. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // (which resolves to a teardown wrapper) lets React on Rails unmount this root on Turbo/Turbolinks | ||
| // navigation or same-id node replacement instead of leaking it. The callback form would discard it. | ||
| // Keep this Pro dummy dependency at @loadable/component >= 5.12.0; package.json requests ^5.16.3 | ||
| // and the lockfile resolves 5.16.7, both of which support the Promise-returning loadableReady API. |
Contributor
There was a problem hiding this comment.
The lockfile-resolved version (5.16.7) will silently become stale after any pnpm update. Dropping it and keeping only the floor and the package.json range makes this comment maintenance-free:
Suggested change
| // and the lockfile resolves 5.16.7, both of which support the Promise-returning loadableReady API. | |
| // Keep this Pro dummy dependency at @loadable/component >= 5.12.0; package.json requests ^5.16.3, | |
| // which supports the Promise-returning loadableReady API. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Core Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Contributor
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 |
Contributor
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@loadable/componentPromise-returningloadableReady()version assumption.props.prerenderin the legacy React 16 dummy Redux startup path.Closes #3689.
Validation
gh issue view 3689 --repo shakacode/react_on_rails --json number,title,state,author,body,comments,labels,createdAt,updatedAtgh search issues --repo shakacode/react_on_rails "delete props.prerender"(only Renderer dummy teardown follow-ups from PR #3585 review #3689 found)pnpm exec prettier --check react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx react_on_rails/spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx react_on_rails_pro/spec/dummy/client/app/loadable/loadable-client.imports-loadable.jsxpnpm exec eslint react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx react_on_rails/spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx react_on_rails_pro/spec/dummy/client/app/loadable/loadable-client.imports-loadable.jsxgit diff --cached --checkscript/ci-changes-detector origin/main fix/3689-dummy-renderer-props-cleanupNote:
bundle exec rubocopwas run per repo policy but failed on unrelated existing/generated files outside this PR's diff, includingreact_on_rails/spec/react_on_rails/dummy-for-generators/*andreact_on_rails/spike/3313_prism_gemfile_rewriter/*.Note
Low Risk
Test/dummy client startup changes only; no production auth, data, or core library behavior.
Overview
Follow-up cleanup for dummy renderer teardown (#3689).
ReduxApp.client.jsx no longer mutates incoming
propsby deletingprerender. It destructuresprerenderintocomponentPropsand passes onlycomponentPropsintocomposeInitialState, avoiding param reassignment and eslint suppressions.ReduxSharedStoreApp.client.jsx drops the same
delete props.prerenderpattern (it only neededprerenderfor hydrate vs render).loadable-client.imports-loadable.jsx adds a comment documenting the Pro dummy’s
@loadable/componentversion floor (≥5.12.0) for the Promise-returningloadableReady()API used for teardown on navigation.Reviewed by Cursor Bugbot for commit 60afe44. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit