test(dummy): enable StrictMode in OSS and Pro dummies#3206
Conversation
Wrap registered components and manually-rendered trees in StrictMode so the spec/dummy app surfaces deprecated lifecycle, unsafe side-effects, and other StrictMode-detected issues during integration testing. Render functions and renderer functions are intentionally skipped — they own their own root and would double-wrap. Adds a small strictModeSupport helper plus unit tests for the wrapping logic and caching behavior. Closes #1465 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughAdds StrictMode support across spec/dummy: new strict-mode utility modules, updates to app startups and ReactOnRails registration to wrap elements/components in React.StrictMode, new pro entry shims and webpack alias changes, and tests validating wrapping behavior and caching. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer Code
participant Bundle as Client Bundle
participant ROR as ReactOnRails
participant Registry as Component Registry
Dev->>Bundle: call ReactOnRails.register(components)
Bundle->>Bundle: wrapRegisteredComponentsWithStrictMode(components)
Bundle->>ROR: originalRegister(wrappedComponents)
ROR-->>Registry: components registered (wrapped)
sequenceDiagram
participant Startup as App Startup
participant Wrapper as strictModeSupport.wrapElementInStrictMode
participant React as React hydrate/render
Startup->>Wrapper: pass created React element (Provider/App)
Wrapper-->>Startup: wrapped element (React.StrictMode(...))
Startup->>React: hydrateRoot/createRoot/render(wrapped element)
React-->>Startup: app initialized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 adds Confidence Score: 4/5Safe to merge for the dummy/spec use case; the P2 findings are design fragilities that won't break the integration test suite but are worth addressing. All findings are P2. The arity heuristic and Map vs WeakMap issues are minor concerns that only affect correctness in edge cases unlikely to appear in the dummy app. No security or data-loss risks. react_on_rails/spec/dummy/client/app/strictModeSupport.jsx — arity heuristic and Map vs WeakMap concerns. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ReactOnRails.register called] --> B[wrapRegisteredComponentsWithStrictMode]
B --> C{isRenderFunction?}
C -- "renderFunction flag OR\nlength >= 2" --> D[Pass through unchanged]
C -- "regular component" --> E[wrapComponentInStrictMode]
E --> F{typeof function?}
F -- yes --> G[Check WeakMap cache\nwrappedFunctionComponents]
F -- no --> H[Check Map cache\nwrappedOtherComponents]
G --> I[Return cached or create\nStrictModeWrapper]
H --> I
I --> J[originalRegister with wrapped components]
K[Renderer function startup file] --> L[wrapElementInStrictMode]
L --> M[React.StrictMode element]
M --> N[ReactDOM render / hydrateRoot]
Reviews (1): Last reviewed commit: "test(dummy): enable React StrictMode acr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code ReviewOverviewThis PR enables
The goal is sound and the implementation is mostly clean. A few things worth addressing: IssuesDuplication of
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
react_on_rails/spec/dummy/tests/strict-mode-support.test.jsx (1)
34-45: Consider also covering therenderFunction-flag detection branch.The test exercises arity-based skipping (
renderFunctionwith arity 2,rendererFunctionwith arity 3), but per the PR description the helper also skips functions tagged via arenderFunctionflag. A short additional case would lock in that branch and prevent regressions if arity heuristics change.♻️ Suggested test addition
it('does not wrap render functions or renderer functions', () => { const renderFunction = (props, railsContext) => ({ props, railsContext }); const rendererFunction = (props, railsContext, domNodeId) => ({ props, railsContext, domNodeId }); const wrappedComponents = wrapRegisteredComponentsWithStrictMode({ renderFunction, rendererFunction, }); expect(wrappedComponents.renderFunction).toBe(renderFunction); expect(wrappedComponents.rendererFunction).toBe(rendererFunction); }); + + it('does not wrap functions explicitly flagged as render functions', () => { + const flaggedRenderFunction = (props) => ({ props }); + flaggedRenderFunction.renderFunction = true; + + const wrappedComponents = wrapRegisteredComponentsWithStrictMode({ flaggedRenderFunction }); + + expect(wrappedComponents.flaggedRenderFunction).toBe(flaggedRenderFunction); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/dummy/tests/strict-mode-support.test.jsx` around lines 34 - 45, Add a test case to exercise the branch where a function is detected as a render function via the renderFunction flag (not just by arity): create a function whose length/arity would normally cause wrapping, set its renderFunction property to true (e.g., renderFunction.flag = true or renderFunction.renderFunction = true according to how detection is implemented), call wrapRegisteredComponentsWithStrictMode with that function in the renderFunction slot, and assert that the returned wrappedComponents.renderFunction strictly equals the original function; reference the helper wrapRegisteredComponentsWithStrictMode and the renderFunction rendererFunction symbols to locate where to add the test.react_on_rails/spec/dummy/client/app/strictModeSupport.jsx (1)
22-38: Redundant branching increateStrictModeWrapper.Both arms of the ternary on line 26 produce the same output: JSX
<Component {...props} />compiles toReact.createElement(Component, props), which is exactly what theelsebranch does. React happily accepts strings, function components, class components,forwardRef/memoobjects, etc. as the type argument, so a single expression covers all cases.♻️ Proposed simplification
const createStrictModeWrapper = (Component) => { function StrictModeWrapper(props) { return ( <React.StrictMode> - {typeof Component === 'function' ? <Component {...props} /> : React.createElement(Component, props)} + <Component {...props} /> </React.StrictMode> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/dummy/client/app/strictModeSupport.jsx` around lines 22 - 38, The StrictModeWrapper in createStrictModeWrapper has a redundant ternary when rendering Component; remove the typeof check and always render the Component using a single expression (either JSX <Component {...props} /> or React.createElement(Component, props)) inside the React.StrictMode in the StrictModeWrapper function, leaving the componentName/displayName logic intact; update StrictModeWrapper so it returns React.StrictMode with that single render expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react_on_rails/spec/dummy/client/app/packs/client-bundle.js`:
- Around line 12-16: The monkey-patch replacing ReactOnRails.register is being
re-applied on every HMR update, nesting wrappers; guard it with an idempotency
flag (e.g. ReactOnRails.__registerWrapped or a module-scoped boolean) so the
patch runs only once: check the flag before binding originalRegister and
assigning ReactOnRails.register, and set the flag after assignment; keep using
wrapRegisteredComponentsWithStrictMode to wrap the incoming components and
preserve originalRegister by binding it once (references: ReactOnRails.register,
originalRegister, wrapRegisteredComponentsWithStrictMode).
---
Nitpick comments:
In `@react_on_rails/spec/dummy/client/app/strictModeSupport.jsx`:
- Around line 22-38: The StrictModeWrapper in createStrictModeWrapper has a
redundant ternary when rendering Component; remove the typeof check and always
render the Component using a single expression (either JSX <Component {...props}
/> or React.createElement(Component, props)) inside the React.StrictMode in the
StrictModeWrapper function, leaving the componentName/displayName logic intact;
update StrictModeWrapper so it returns React.StrictMode with that single render
expression.
In `@react_on_rails/spec/dummy/tests/strict-mode-support.test.jsx`:
- Around line 34-45: Add a test case to exercise the branch where a function is
detected as a render function via the renderFunction flag (not just by arity):
create a function whose length/arity would normally cause wrapping, set its
renderFunction property to true (e.g., renderFunction.flag = true or
renderFunction.renderFunction = true according to how detection is implemented),
call wrapRegisteredComponentsWithStrictMode with that function in the
renderFunction slot, and assert that the returned
wrappedComponents.renderFunction strictly equals the original function;
reference the helper wrapRegisteredComponentsWithStrictMode and the
renderFunction rendererFunction symbols to locate where to add the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1026fda-cee0-475f-bca2-d885bd7fdac4
📒 Files selected for processing (9)
react_on_rails/spec/dummy/client/app-react16/startup/ManualRenderApp.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsxreact_on_rails/spec/dummy/client/app/packs/client-bundle.jsreact_on_rails/spec/dummy/client/app/startup/ManualRenderApp.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsxreact_on_rails/spec/dummy/client/app/strictModeSupport.jsxreact_on_rails/spec/dummy/tests/strict-mode-support.test.jsx
justin808
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx (1)
157-167: Nit:patchedReactOnRailsalias reads as if it's a separate object.
patchedReactOnRailsis just a same-reference alias forreactOnRails, so all assignments below mutate the input singleton. The naming hides the in-place mutation; either drop the alias and operate onreactOnRailsdirectly, or add a brief comment that this intentionally mutates the passed-in instance.♻️ Proposed simplification
- const originalRegister = reactOnRails.register.bind(reactOnRails); - const patchedReactOnRails = reactOnRails; - - patchedReactOnRails.register = (components: ComponentRegistry) => { - originalRegister(wrapRegisteredComponentsWithStrictMode(components)); - }; - - Object.defineProperty(patchedReactOnRails, STRICT_MODE_PATCHED, { value: true }); - - return patchedReactOnRails; + // Mutates `reactOnRails` in place so all default-import consumers observe the patch. + const originalRegister = reactOnRails.register.bind(reactOnRails); + reactOnRails.register = (components: ComponentRegistry) => { + originalRegister(wrapRegisteredComponentsWithStrictMode(components)); + }; + Object.defineProperty(reactOnRails, STRICT_MODE_PATCHED, { value: true }); + + return reactOnRails;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx` around lines 157 - 167, The name patchedReactOnRails misleadingly suggests a new object when it is just an alias to the input reactOnRails and the code mutates the singleton; either remove the alias and call reactOnRails.register directly (use originalRegister = reactOnRails.register.bind(reactOnRails); reactOnRails.register = (components) => originalRegister(wrapRegisteredComponentsWithStrictMode(components)); Object.defineProperty(reactOnRails, STRICT_MODE_PATCHED, { value: true }); return reactOnRails) or keep patchedReactOnRails but add a concise comment above its definition stating it intentionally aliases and mutates the passed-in reactOnRails instance; update usages of patchedReactOnRails accordingly and ensure symbols originalRegister, wrapRegisteredComponentsWithStrictMode, and STRICT_MODE_PATCHED are referenced as shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsPro.js`:
- Around line 1-5: This file re-exports the original module's named exports via
"export *" which bypasses the strict-mode wrapper applied to the default with
enableStrictModeForReactOnRails(ReactOnRails); remove the export * line in
strictModeReactOnRailsPro.js so consumers must use the wrapped default (or
alternatively explicitly re-export wrapped versions of specific symbols like
RailsContext or ReactComponentOrRenderFunction by obtaining them from the
wrapped ReactOnRails and exporting those), ensuring all public APIs go through
enableStrictModeForReactOnRails and not the unwrapped originals.
---
Nitpick comments:
In `@react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx`:
- Around line 157-167: The name patchedReactOnRails misleadingly suggests a new
object when it is just an alias to the input reactOnRails and the code mutates
the singleton; either remove the alias and call reactOnRails.register directly
(use originalRegister = reactOnRails.register.bind(reactOnRails);
reactOnRails.register = (components) =>
originalRegister(wrapRegisteredComponentsWithStrictMode(components));
Object.defineProperty(reactOnRails, STRICT_MODE_PATCHED, { value: true });
return reactOnRails) or keep patchedReactOnRails but add a concise comment above
its definition stating it intentionally aliases and mutates the passed-in
reactOnRails instance; update usages of patchedReactOnRails accordingly and
ensure symbols originalRegister, wrapRegisteredComponentsWithStrictMode, and
STRICT_MODE_PATCHED are referenced as shown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c0c97ca-883a-4887-a5e0-75731facab02
📒 Files selected for processing (10)
react_on_rails_pro/spec/dummy/client/app/loadable/loadable-client.imports-loadable.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ApolloGraphQLApp.client.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ManualRenderApp.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxApp.client.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsxreact_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsPro.jsreact_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsProClient.jsreact_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsxreact_on_rails_pro/spec/dummy/config/webpack/alias.js
Code ReviewOverall this is a clean, well-structured PR. The approach — patching A few issues worth addressing: Bugs / Correctness
Code Quality
Clarity / Documentation
Test CoverageThe OSS test suite covers the core paths well. Two gaps worth considering:
Minor
|
![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: d466e91b3d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Non-React plain object incorrectly wrapped as component
- Updated strict mode registration wrapping to leave non-React plain object registrations unchanged while still wrapping valid React component element types.
Or push these changes by commenting:
@cursor push 57f91ed27a
Preview (57f91ed27a)
diff --git a/react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx b/react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx
--- a/react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx
+++ b/react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx
@@ -17,7 +17,13 @@
prototype?: ReactClassPrototype;
};
-type RegisteredComponent = string | ComponentWithMetadata | RenderFunction;
+type ReactElementTypeObject = {
+ $$typeof?: unknown;
+ displayName?: string;
+ name?: string;
+};
+type StrictModeWrappableComponent = string | ComponentWithMetadata | RenderFunction | ReactElementTypeObject;
+type RegisteredComponent = StrictModeWrappableComponent | object;
type ComponentRegistry = Record<string, RegisteredComponent>;
type ReactOnRailsWithRegister = {
register: (components: ComponentRegistry) => void;
@@ -26,9 +32,12 @@
const STRICT_MODE_PATCHED = '__reactOnRailsProDummyStrictModePatched';
-const wrappedFunctionComponents = new WeakMap<ComponentWithMetadata | RenderFunction, RegisteredComponent>();
+const wrappedFunctionComponents = new WeakMap<
+ ComponentWithMetadata | RenderFunction,
+ StrictModeWrappableComponent
+>();
const wrappedRenderFunctions = new WeakMap<RenderFunction, RenderFunction>();
-const wrappedOtherComponents = new Map<RegisteredComponent, RegisteredComponent>();
+const wrappedOtherComponents = new Map<StrictModeWrappableComponent, StrictModeWrappableComponent>();
const isPromiseLike = (value: unknown): value is Promise<unknown> =>
typeof value === 'object' &&
@@ -41,11 +50,11 @@
return false;
}
- if (component.prototype?.isReactComponent) {
+ if ((component.prototype as ReactClassPrototype | undefined)?.isReactComponent) {
return false;
}
- if (component.renderFunction) {
+ if ('renderFunction' in component && component.renderFunction) {
return true;
}
@@ -55,7 +64,12 @@
const isRendererFunction = (component: RegisteredComponent): component is RenderFunction =>
isRenderFunction(component) && component.length === 3;
-const createStrictModeWrapper = (Component: RegisteredComponent): React.FC<Record<string, unknown>> => {
+const isReactElementTypeObject = (component: RegisteredComponent): component is ReactElementTypeObject =>
+ typeof component === 'object' && '$$typeof' in component && typeof component.$$typeof === 'symbol';
+
+const createStrictModeWrapper = (
+ Component: StrictModeWrappableComponent,
+): React.FC<Record<string, unknown>> => {
function StrictModeWrapper(props: Record<string, unknown>) {
const childElement =
typeof Component === 'string'
@@ -74,7 +88,7 @@
return StrictModeWrapper;
};
-const wrapComponentInStrictMode = (component: RegisteredComponent): RegisteredComponent => {
+const wrapComponentInStrictMode = (component: StrictModeWrappableComponent): StrictModeWrappableComponent => {
if (typeof component === 'function') {
const cachedComponent = wrappedFunctionComponents.get(component);
if (cachedComponent) {
@@ -110,7 +124,7 @@
}
if (typeof result === 'function') {
- return wrapComponentInStrictMode(result as RegisteredComponent);
+ return wrapComponentInStrictMode(result as StrictModeWrappableComponent);
}
return result;
@@ -145,9 +159,17 @@
return [name, wrapRenderFunctionInStrictMode(component)];
}
+ if (
+ typeof component !== 'function' &&
+ typeof component !== 'string' &&
+ !isReactElementTypeObject(component)
+ ) {
+ return [name, component];
+ }
+
return [name, wrapComponentInStrictMode(component)];
}),
- );
+ ) as ComponentRegistry;
export const enableStrictModeForReactOnRails = <T extends ReactOnRailsWithRegister>(reactOnRails: T): T => {
if (reactOnRails[STRICT_MODE_PATCHED]) {You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit d466e91. Configure here.
Code Review: Enable StrictMode in OSS and Pro DummiesOverviewThis PR adds React StrictMode wrappers to the OSS and Pro dummy apps via two complementary approaches: monkey-patching The implementation is well-structured, the Pro version's TypeScript types are precise, and the caching design (WeakMap for functions/objects, Map for strings) prevents wrapper duplication. Good work overall — a few issues below worth addressing before merge. Issues1. Potential Bug — RSC config only removes one of two StrictMode aliases
The fix is one extra line: delete rscAliases['react-on-rails-pro$'];
delete rscAliases['react-on-rails-pro/client$']; // ← add this2. Subtle edge case —
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
react_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.js (1)
37-43: Consider adding a comment explaining the intentional asymmetry in the alias map, or defensively removereact-on-rails-pro/client$.You scrub
react-on-rails-pro$so the RSC bundle resolves via the package'sreact-serverconditional export. However, the inheritedreact-on-rails-pro/client$alias still points tostrictModeReactOnRailsProClient.js(which importsReactOnRails.client). The Pro dummy's RSC graph does not currently import the/clientsubpath, but adding a comment clarifying this intentional asymmetry—or optionally removing the alias as a defensive measure—would prevent future accidental inclusion of client-only code in the react-server bundle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.js` around lines 37 - 43, The alias map currently deletes 'react-on-rails-pro$' via rscAliases but leaves 'react-on-rails-pro/client$' pointing to strictModeReactOnRailsProClient.js, which risks pulling client-only code into the RSC bundle; update the rscWebpackConfig logic to either remove 'react-on-rails-pro/client$' from rscAliases or add a concise comment next to the rscAliases deletion explaining the intentional asymmetry (that the RSC graph must resolve via the package's react-server conditional export and does not import the /client subpath), referencing rscAliases, rscConfig.resolve.alias, 'react-on-rails-pro$', and 'react-on-rails-pro/client$' so future maintainers understand or defensively avoid the client alias.react_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js (1)
28-35: Server alias only overridesreact-on-rails-pro$, notreact-on-rails-pro/client$.The base
alias.jsresolves bothreact-on-rails-pro$andreact-on-rails-pro/client$. Here you only override the bare specifier to the Node strict-mode wrapper, so any server-side import ofreact-on-rails-pro/clientwill still pull the client wrapper (strictModeReactOnRailsProClient.js, which wrapsReactOnRails.client). That's currently unused on the server, but it's a latent footgun if/when SSR code starts importing the/clientsubpath.Consider either also overriding
react-on-rails-pro/client$here (e.g., to the Node wrapper or tofalse) or adding a comment that the/clientalias is intentionally inherited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js` around lines 28 - 35, The server webpack alias currently only overrides react-on-rails-pro$ to strictModeReactOnRailsProNode.js but leaves react-on-rails-pro/client$ pointing to the client wrapper (strictModeReactOnRailsProClient.js); update serverWebpackConfig.resolve.alias to also set 'react-on-rails-pro/client$' to the Node wrapper (or to false) to prevent server bundles from resolving the client wrapper, or alternatively add a clear comment next to serverWebpackConfig.resolve.alias documenting that the client subpath is intentionally inherited; reference the alias map (serverWebpackConfig.resolve.alias), the specifier 'react-on-rails-pro/client$', and the files strictModeReactOnRailsProNode.js and strictModeReactOnRailsProClient.js when making the change.react_on_rails/spec/dummy/client/app/strictModeSupport.jsx (1)
1-68: LGTM — clean, well-scoped StrictMode helper.The
isRenderFunctionheuristic faithfully mirrors the publicreact-on-railsconvention (class detection viaprototype.isReactComponent, explicit.renderFunctionflag, arity ≥ 2 covering both 2‑arg render and 3‑arg renderer signatures). Caching is sensible:WeakMapfor function/class components allows GC when originals go unreferenced, whileMapcorrectly handles non‑function component types (e.g.,React.memo/React.forwardRefobjects, which aretypeof === 'object').displayNameresolution including the string‑component branch is a nice touch for debuggability.One minor optional consideration: the
wrappedOtherComponentsMapis module‑scoped and never pruned, so any non‑function components registered over time stay referenced for the lifetime of the JS runtime. In a dummy app this is fine — just noting it in case the helper ever gets reused outsidespec/dummy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/dummy/client/app/strictModeSupport.jsx` around lines 1 - 68, The module-scoped Map wrappedOtherComponents can hold non-function component keys indefinitely; to avoid memory growth, add an explicit cleanup/unregister API (e.g., export a function like unregisterWrappedComponent(nameOrComponent) and/or clearWrappedOtherComponents()) and call it when dummy registered components are torn down; implement the functions near wrapComponentInStrictMode and update any test dummy teardown to call them so wrappedOtherComponents.delete(key) or wrappedOtherComponents.clear() runs as part of lifecycle to release references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.js`:
- Around line 37-43: The alias map currently deletes 'react-on-rails-pro$' via
rscAliases but leaves 'react-on-rails-pro/client$' pointing to
strictModeReactOnRailsProClient.js, which risks pulling client-only code into
the RSC bundle; update the rscWebpackConfig logic to either remove
'react-on-rails-pro/client$' from rscAliases or add a concise comment next to
the rscAliases deletion explaining the intentional asymmetry (that the RSC graph
must resolve via the package's react-server conditional export and does not
import the /client subpath), referencing rscAliases, rscConfig.resolve.alias,
'react-on-rails-pro$', and 'react-on-rails-pro/client$' so future maintainers
understand or defensively avoid the client alias.
In `@react_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js`:
- Around line 28-35: The server webpack alias currently only overrides
react-on-rails-pro$ to strictModeReactOnRailsProNode.js but leaves
react-on-rails-pro/client$ pointing to the client wrapper
(strictModeReactOnRailsProClient.js); update serverWebpackConfig.resolve.alias
to also set 'react-on-rails-pro/client$' to the Node wrapper (or to false) to
prevent server bundles from resolving the client wrapper, or alternatively add a
clear comment next to serverWebpackConfig.resolve.alias documenting that the
client subpath is intentionally inherited; reference the alias map
(serverWebpackConfig.resolve.alias), the specifier 'react-on-rails-pro/client$',
and the files strictModeReactOnRailsProNode.js and
strictModeReactOnRailsProClient.js when making the change.
In `@react_on_rails/spec/dummy/client/app/strictModeSupport.jsx`:
- Around line 1-68: The module-scoped Map wrappedOtherComponents can hold
non-function component keys indefinitely; to avoid memory growth, add an
explicit cleanup/unregister API (e.g., export a function like
unregisterWrappedComponent(nameOrComponent) and/or
clearWrappedOtherComponents()) and call it when dummy registered components are
torn down; implement the functions near wrapComponentInStrictMode and update any
test dummy teardown to call them so wrappedOtherComponents.delete(key) or
wrappedOtherComponents.clear() runs as part of lifecycle to release references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e83f9b0-e1a5-430c-8f09-32cf06266714
📒 Files selected for processing (9)
react_on_rails/spec/dummy/client/app/packs/client-bundle.jsreact_on_rails/spec/dummy/client/app/strictModeSupport.jsxreact_on_rails/spec/dummy/tests/strict-mode-support.test.jsxreact_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsPro.jsreact_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsProClient.jsreact_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsProNode.jsreact_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsxreact_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.jsreact_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js
✅ Files skipped from review due to trivial changes (1)
- react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsProClient.js
- react_on_rails/spec/dummy/client/app/packs/client-bundle.js
Picks up the optional-tier review feedback on PR #3206: - RSC bundle now also drops the `react-on-rails-pro/client$` alias so the client-only StrictMode shim cannot resolve inside the React server bundle. - `serverWebpackConfig` writes its alias override via a defensive spread rather than mutating `resolve.alias` directly. - OSS `strictModeSupport` splits the non-function cache into a WeakMap for object components (memo/forwardRef/lazy) and a Map for string keys, so registered objects are no longer pinned for the lifetime of the module. - OSS `isRenderFunction` documents the `length >= 2` heuristic and how to opt out of it. - `client-bundle.js` documents the bundle-scope limitation of the register patch. - Adds a Jest case that pins down 2-arg functional component handling. - React 16 dummy startup files document the intentional cross-tree import of `app/strictModeSupport`. - Pro `wrapRenderFunctionInStrictMode` documents the `length === 2` invariant and why STRICT_MODE_PATCHED keeps it safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review summary update —
|
Code ReviewOverviewThis PR adds React StrictMode to the OSS and Pro Strengths
Issues1. Server-side StrictMode is a no-op (adds dead weight)
2. OSS and Pro render-function handling are asymmetric
This means a 2-arg render function registered in the OSS dummy gets no StrictMode coverage, while the same pattern in the Pro dummy does. If the OSS dummy has 2-arg render functions that return React elements, they're silently excluded. Is this intentional, or should OSS be brought to parity? 3. Missing test coverage for Pro's additional logicThe Jest suite only tests the OSS helper. Pro's
At minimum, a 4. Minor:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react_on_rails/spec/dummy/client/app/strictModeSupport.jsx`:
- Around line 23-27: The current detection in strictModeSupport.jsx treats only
truthy component.renderFunction as an override, so setting
component.renderFunction = false doesn't opt out; update the logic in the
detection function to first check whether the component has an explicit
renderFunction property (e.g., via "renderFunction" in component or
hasOwnProperty) and if present return its boolean value (false to opt out, true
to opt in); otherwise fall back to the existing length check (component.length
>= 2). Ensure you update the branch that currently uses if
(component.renderFunction) and the final return using component.length to follow
this explicit-property-first behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdb11ff6-2ec2-408d-b03d-a2218c79ac8f
📒 Files selected for processing (9)
react_on_rails/spec/dummy/client/app-react16/startup/ManualRenderApp.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsxreact_on_rails/spec/dummy/client/app/packs/client-bundle.jsreact_on_rails/spec/dummy/client/app/strictModeSupport.jsxreact_on_rails/spec/dummy/tests/strict-mode-support.test.jsxreact_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsxreact_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.jsreact_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js
✅ Files skipped from review due to trivial changes (1)
- react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- react_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js
- react_on_rails/spec/dummy/client/app/packs/client-bundle.js
- react_on_rails/spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx
- react_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.js
- react_on_rails/spec/dummy/client/app-react16/startup/ManualRenderApp.jsx
- react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
- react_on_rails/spec/dummy/tests/strict-mode-support.test.jsx
Address remaining review feedback on PR #3206: - OSS strictModeSupport.jsx now mirrors the Pro structure: adds isRendererFunction, isReactComponent, wrapRenderFunctionResult, and wrapRenderFunctionInStrictMode so 2-arg render functions (e.g. ContextFunctionReturnInvalidJSX, HelloWorldHooksContext) get StrictMode coverage via return-value interception instead of being skipped entirely. - isRenderFunction in OSS and Pro now honors explicit `renderFunction = false` so legacy 2-arg `(props, context)` components can opt out of the render-function classification (matches the contract in the comment block). - Pro RenderFunction type widens `renderFunction?: true` to `renderFunction?: boolean` to reflect the new opt-out. - Pro Node shim documents that <React.StrictMode> is a no-op in SSR; we keep the wrapping for client/server uniformity. - Adds inline comments on Pro `wrappedStringComponents` Map and the wrapper-arity edge case for direct callers; adds TODO for Pro test coverage of `enableStrictModeForReactOnRails` / Promise paths. - Updates jest tests to cover the new wrapping paths plus the `renderFunction = false` opt-out. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review summary update —
|
|
Review rate limit: 6/8 reviews remaining, refill in 8 minutes and 47 seconds. |
Code Review — PR #3206: Enable StrictMode in OSS and Pro DummiesOverall: This is a well-thought-out implementation. The design rationale (monkey-patching Bugs / CorrectnessString component wrapping produces invalid output. Server bundle inherits Test CoveragePro-specific paths have no tests. Code QualityDuplication between OSS Cross-tree React-16 imports are a silent fragility. The three React 16 startup files import from Wrapped render-function arity footgun. Minor
|
… scope comment, Pro tests Addresses the four unresolved review threads on #3206: - Pro `serverWebpackConfig.js` now drops the `react-on-rails-pro/client$` alias before applying its own `react-on-rails-pro$` override, mirroring the RSC config so the SSR bundle never resolves to the browser-only StrictMode shim. - Removed the unreachable `typeof component === 'string'` branch (and `wrappedStringComponents` Map) from both OSS `strictModeSupport.jsx` and Pro `strictModeSupport.tsx`. ReactOnRails registrations only ever pass actual component values; the dead branch would have rendered an unknown HTML element if hit. - Expanded the scope comment in `client-bundle.js` to enumerate what's currently covered (this pack, the auto-generated `packs/generated/*.js`, ERB-inline `register` calls) and to spell out how a future independent pack should opt in. - Added a Jest harness in `react_on_rails_pro/spec/dummy/tests/` (jest.config.cjs + ts-jest) with five tests pinning the Pro-specific behaviors that previously had only a TODO: register-time wrapping, `enableStrictModeForReactOnRails` idempotency, async `Promise<ReactElement>` results, 3-arg renderer skip, and `wrapElementInStrictMode`. Removed the now-resolved TODO comment. Verification: - `pnpm --filter react_on_rails_pro_dummy run test:js` — 5/5 passing - `pnpm --filter react-on-rails run test:js` — 14/14 passing - `pnpm run type-check` (workspace) — clean - ESLint + Prettier on all touched files — clean (root and Pro configs) - `RAILS_ENV=test NODE_ENV=test bin/shakapacker` for Pro dummy — succeeds; resulting `ssr-generated/server-bundle.js` references `strictModeReactOnRailsProNode` only (no `strictModeReactOnRailsProClient` strings) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review summary update —
|
* origin/main: Document examples catalog and naming plan (#3191) test(dummy): enable StrictMode in OSS and Pro dummies (#3206) ci: exclude bot-blocking URLs from lychee link check (#3214) Remove stale Coveralls integration (#3204) docs: normalize external GitHub repo slugs in links and generators (#3198) # Conflicts: # .lychee.toml
1 participant
Summary
spec/dummystartup paths by wrapping normalReactOnRails.registerregistrations and direct manual renderer roots.client/app/path and the React 16client/app-react16/path.Closes #1465
Test plan
pnpm --dir react_on_rails/spec/dummy run test:jspnpm --dir react_on_rails/spec/dummy run build:test(passes with existing DefinePlugin warnings)pnpm exec eslint --no-ignore react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx react_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsPro.js react_on_rails_pro/spec/dummy/client/app/strictModeReactOnRailsProClient.js react_on_rails_pro/spec/dummy/config/webpack/alias.js react_on_rails_pro/spec/dummy/client/app/loadable/loadable-client.imports-loadable.jsx react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ManualRenderApp.jsx react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxApp.client.jsx react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsxpnpm --dir react_on_rails_pro/spec/dummy run build:test(passes with existing DefinePlugin warnings)Summary by CodeRabbit
New Features
Tests
Note
Low Risk
Low risk because changes are confined to
spec/dummyapps and webpack configs used for dummy/test builds; main risk is unexpected StrictMode double-invocation effects causing flaky dummy tests or examples.Overview
Enables React
StrictModeacross the OSS and Pro dummy apps by adding shared helpers to wrap rendered element trees and to automatically wrap registered components.In OSS
spec/dummy, startup entrypoints (including the React 16 variant) now wrap manual render roots and Redux provider trees viawrapElementInStrictMode, andclient-bundle.jsmonkey-patchesReactOnRails.registerto StrictMode-wrap non-render-function registrations.In Pro
spec/dummy, a TypeScriptstrictModeSupportadds component/render-function wrapping (including async render results), webpack aliases redirectreact-on-rails-proimports to StrictMode-enabled shims for client/server builds (with RSC config explicitly removing that alias), and several startup apps wrap their roots explicitly; Jest coverage is added for the OSS helper.Reviewed by Cursor Bugbot for commit 45da8de. Bugbot is set up for automated code reviews on this repo. Configure here.