feat(pro): add imperative refetch API to RSCRoute (#3106)#3215
feat(pro): add imperative refetch API to RSCRoute (#3106)#3215AbanoubGhadban wants to merge 8 commits into
Conversation
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds an imperative refetch API for React Server Components: ChangesImperative RSC refetch feature
Sequence DiagramsequenceDiagram
participant User
participant InlineRefreshButton as InlineRefreshButton (Client)
participant RSCRoute as RSCRoute
participant RSCProvider as RSCProvider (context)
participant Server as getServerComponent (Server)
User->>InlineRefreshButton: click
InlineRefreshButton->>RSCRoute: useCurrentRSCRoute().refetch()
RSCRoute->>RSCProvider: refetchComponent(enforceRefetch: true)
RSCProvider->>Server: getServerComponent(name, props, enforceRefetch: true)
Server-->>RSCProvider: Promise<RSC payload>
RSCProvider->>RSCProvider: update cache & bump version (startTransition)
RSCProvider->>RSCRoute: resolved promise -> re-render
RSCRoute->>RSCRoute: stream new payload while preserving old content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 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. Review rate limit: 0/8 reviews remaining, refill in 56 minutes and 48 seconds.Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| #### Added | ||
|
|
||
| - **[Pro]** **Imperative refetch API for `<RSCRoute>`**: `<RSCRoute>` now accepts an optional `ref` typed as `RSCRouteHandle`, exposing `refetch()` so a parent or sibling can refetch a server component without knowing its `componentName` or `componentProps`. A new `useCurrentRSCRoute()` hook returns the same handle for client components rendered inside the RSC subtree (e.g. an inline "Refresh" button rendered by the server component itself); calling it outside an `<RSCRoute>` ancestor throws `useCurrentRSCRoute must be used inside an <RSCRoute>`. Both APIs auto-update the rendered tree — no caller-side `setKey`/`useState` workaround — and propagate to every `<RSCRoute>` instance bound to the same cache key. The internal cache invalidation runs inside a React transition, so old content stays visible while the new RSC payload streams in (no Suspense-fallback flash). The existing `useRSC().refetchComponent(name, props)` API and the `ServerComponentFetchError`-based retry flow are unchanged. Fixes [Issue 3106](https://github.com/shakacode/react_on_rails/issues/3106). [PR 3231](https://github.com/shakacode/react_on_rails/pull/3231) by [AbanoubGhadban](https://github.com/AbanoubGhadban). |
There was a problem hiding this comment.
Wrong PR number in the link. The entry says [PR 3231] (linking to /pull/3231) but this is PR #3215. Please update both the display text and the URL.
| - **[Pro]** **Imperative refetch API for `<RSCRoute>`**: `<RSCRoute>` now accepts an optional `ref` typed as `RSCRouteHandle`, exposing `refetch()` so a parent or sibling can refetch a server component without knowing its `componentName` or `componentProps`. A new `useCurrentRSCRoute()` hook returns the same handle for client components rendered inside the RSC subtree (e.g. an inline "Refresh" button rendered by the server component itself); calling it outside an `<RSCRoute>` ancestor throws `useCurrentRSCRoute must be used inside an <RSCRoute>`. Both APIs auto-update the rendered tree — no caller-side `setKey`/`useState` workaround — and propagate to every `<RSCRoute>` instance bound to the same cache key. The internal cache invalidation runs inside a React transition, so old content stays visible while the new RSC payload streams in (no Suspense-fallback flash). The existing `useRSC().refetchComponent(name, props)` API and the `ServerComponentFetchError`-based retry flow are unchanged. Fixes [Issue 3106](https://github.com/shakacode/react_on_rails/issues/3106). [PR 3231](https://github.com/shakacode/react_on_rails/pull/3231) by [AbanoubGhadban](https://github.com/AbanoubGhadban). | |
| - **[Pro]** **Imperative refetch API for `<RSCRoute>`**: `<RSCRoute>` now accepts an optional `ref` typed as `RSCRouteHandle`, exposing `refetch()` so a parent or sibling can refetch a server component without knowing its `componentName` or `componentProps`. A new `useCurrentRSCRoute()` hook returns the same handle for client components rendered inside the RSC subtree (e.g. an inline "Refresh" button rendered by the server component itself); calling it outside an `<RSCRoute>` ancestor throws `useCurrentRSCRoute must be used inside an <RSCRoute>`. Both APIs auto-update the rendered tree — no caller-side `setKey`/`useState` workaround — and propagate to every `<RSCRoute>` instance bound to the same cache key. The internal cache invalidation runs inside a React transition, so old content stays visible while the new RSC payload streams in (no Suspense-fallback flash). The existing `useRSC().refetchComponent(name, props)` API and the `ServerComponentFetchError`-based retry flow are unchanged. Fixes [Issue 3106](https://github.com/shakacode/react_on_rails/issues/3106). [PR 3215](https://github.com/shakacode/react_on_rails/pull/3215) by [AbanoubGhadban](https://github.com/AbanoubGhadban). |
| getServerComponent: (props: ClientGetReactServerComponentProps) => Promise<ReactNode>; | ||
| }) => { | ||
| const fetchRSCPromises: Record<string, Promise<ReactNode>> = {}; | ||
| return ({ children }: { children: ReactNode }) => { |
There was a problem hiding this comment.
The returned component is an anonymous function, so it shows up as Component (or nothing) in React DevTools. Consider assigning a displayName after the closing brace — matching the pattern used for RSCRoute:
| return ({ children }: { children: ReactNode }) => { | |
| const RSCProvider = ({ children }: { children: ReactNode }) => { |
Then after the closing }; add RSCProvider.displayName = 'RSCProvider'; and return RSCProvider;. This makes DevTools profiles and error stacks readable.
| // `versions` is intentionally listed in deps so the value identity | ||
| // changes on each refetch — that is what propagates the re-render to | ||
| // every useRSC() consumer. | ||
| const contextValue = useMemo( | ||
| () => ({ getComponent, refetchComponent }), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [getComponent, refetchComponent, versions], | ||
| ); |
There was a problem hiding this comment.
Using versions as a useMemo dependency while intentionally not using it inside the factory function is semantically incorrect (the linter is right to flag it — hence the eslint-disable). A cleaner way to achieve the same propagation is to include versions directly in the context value:
const contextValue = useMemo(
() => ({ getComponent, refetchComponent, _v: versions }),
[getComponent, refetchComponent, versions],
);_v is a non-enumerable-by-convention marker that consumers ignore (nothing currently reads it). The deps are now semantically correct, the lint suppression goes away, and the re-render propagation is explicit in the value shape. Consumers that care only about getComponent/refetchComponent can destructure and ignore _v — no behaviour change.
| export type RSCRouteProps = { | ||
| componentName: string; | ||
| componentProps: unknown; | ||
| ref?: Ref<RSCRouteHandle>; | ||
| }; |
There was a problem hiding this comment.
The ref field in RSCRouteProps is a forward-compat hint for React 19 (where refs are plain props), but the component is currently implemented with forwardRef (React 18 API). Including ref in the exported props type while the component uses forwardRef<…, Omit<RSCRouteProps, 'ref'>> is subtle and can confuse callers.
If React 19 compat is the goal, it's worth a brief inline note explaining that ref is intentionally in the public type for forward-compat, and that it's stripped via Omit in the forwardRef overload today. Otherwise, removing it from RSCRouteProps and letting forwardRef's own type inference provide ref is cleaner.
size-limit report 📦
|
Code Review — PR #3215: Imperative refetch API for
|
Greptile SummaryThis PR adds two ergonomic imperative-refetch APIs to Confidence Score: 4/5Safe to merge; only P2 style findings with no impact on runtime behavior. No P0 or P1 issues found. The No files require special attention; both changed source files are straightforward to reason about. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller (ref / useCurrentRSCRoute)
participant RSCRoute as RSCRoute
participant RSCProvider as RSCProvider
participant Cache as fetchRSCPromisesRef
participant Server as getServerComponent
Caller->>RSCRoute: refetch()
RSCRoute->>RSCProvider: refetchComponent(name, props)
RSCProvider->>Server: getServerComponent({ enforceRefetch: true })
Server-->>RSCProvider: promise (pending)
RSCProvider->>Cache: cache[key] = promise (sync write)
RSCProvider->>RSCProvider: startTransition → setVersions(key++)
Note over RSCProvider: versions change → new contextValue reference
RSCProvider-->>RSCRoute: promise returned
RSCProvider->>RSCRoute: context re-render (transition)
RSCRoute->>Cache: getComponent(name, props) → promise
RSCRoute->>RSCRoute: use(promise) — suspends (old UI stays visible)
Server-->>RSCRoute: promise resolves → new ReactNode
RSCRoute-->>Caller: UI committed with new content
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const getComponent = useCallback((componentName: string, componentProps: unknown) => { | ||
| const key = createRSCPayloadKey(componentName, componentProps); | ||
| if (key in fetchRSCPromisesRef.current) { | ||
| return fetchRSCPromisesRef.current[key]; | ||
| } | ||
|
|
||
| const promise = getServerComponent({ componentName, componentProps }); | ||
| fetchRSCPromises[key] = promise; | ||
| return promise; | ||
| }; | ||
| const promise = getServerComponent({ componentName, componentProps }); | ||
| fetchRSCPromisesRef.current[key] = promise; | ||
| return promise; | ||
| }, []); |
There was a problem hiding this comment.
useCallback deps hides getServerComponent dependency
getComponent uses getServerComponent from the outer factory closure but declares deps = []. This is intentionally correct because getServerComponent is captured once when createRSCProvider is called and is stable for the lifetime of the returned component — it is not React state or a prop. However, the absence of any comment here (unlike the well-documented versions trick in useMemo below) means a future maintainer or an automated exhaustive-deps lint pass could add getServerComponent to the array, which would break the caching semantics (a new callback reference on every render). A short // getServerComponent is captured from the factory closure and never changes comment would protect this invariant.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails-pro/README.md (1)
7-13:⚠️ Potential issue | 🟡 MinorAdd exact-version pinning flags to the install commands.
Floating versions can drift from the gem-matched Pro build. Update the examples to use
--save-exact(npm),--exact(yarn), and--exact(pnpm):Suggested fix
npm install --save-exact react-on-rails-pro # or yarn add --exact react-on-rails-pro # or pnpm add --exact react-on-rails-pro🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro/README.md` around lines 7 - 13, Update the install examples in README.md to pin exact package versions: replace the npm command to include --save-exact and update both yarn and pnpm commands to include --exact so the provided install snippets use exact-version pinning (npm: add --save-exact; yarn: add --exact; pnpm: add --exact) to prevent drift from the gem-matched Pro build.
🧹 Nitpick comments (1)
packages/react-on-rails-pro/src/RSCProvider.tsx (1)
63-107: Scope the refetch rerender to the affected cache key.Because
versionsis part of the provider value, any refetch changes the context identity and rerenders everyuseRSC()consumer, even when only one key changed. If this provider can wrap multiple independent<RSCRoute>trees, a keyed subscription/store would avoid unnecessary churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro/src/RSCProvider.tsx` around lines 63 - 107, The provider currently includes the whole `versions` state in `contextValue`, so any refetch bumps the context identity and rerenders all `useRSC()` consumers; replace this with a keyed subscription mechanism so only the affected cache key consumers rerender. Add a subscription registry (e.g. a Map from payload key to Set of callbacks) alongside `fetchRSCPromisesRef`, expose `subscribe(key, cb)` and `unsubscribe(key, cb)` in the context returned by useMemo (include `getComponent` and `refetchComponent`), and in `refetchComponent` after replacing the promise invoke only the callbacks for that key; remove `versions` from the context and from the useMemo deps so context identity no longer changes for unrelated keys. Ensure symbols referenced are `fetchRSCPromisesRef`, `getComponent`, `refetchComponent`, `contextValue`, `versions`, and `createRSCPayloadKey`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 29: The changelog entry in CHANGELOG.md contains a broken link to "PR
3231" which 404s and breaks CI; update the PR reference so it points to the
correct existing pull request URL (or remove/replace the link with the correct
PR number or plain text) in the paragraph mentioning "PR 3231" (which also
references Issue 3106 and AbanoubGhadban) so the markdown-link check passes.
In `@react_on_rails_pro/spec/dummy/e2e-tests/refetch-stress.spec.ts`:
- Around line 5-8: The comment and code disagree: the BASE constant only reads
process.env.PLAYWRIGHT_BASE_URL, but the comment claims BASE_URL is supported;
update the bootstrap so BASE checks both environment variables (e.g.,
process.env.PLAYWRIGHT_BASE_URL first, then process.env.BASE_URL) before falling
back to the default 'http://127.0.0.1:3002'; locate the BASE constant in
refetch-stress.spec.ts and change its initialization to consult both
PLAYWRIGHT_BASE_URL and BASE_URL in that order so runs that set BASE_URL
continue to work.
---
Outside diff comments:
In `@packages/react-on-rails-pro/README.md`:
- Around line 7-13: Update the install examples in README.md to pin exact
package versions: replace the npm command to include --save-exact and update
both yarn and pnpm commands to include --exact so the provided install snippets
use exact-version pinning (npm: add --save-exact; yarn: add --exact; pnpm: add
--exact) to prevent drift from the gem-matched Pro build.
---
Nitpick comments:
In `@packages/react-on-rails-pro/src/RSCProvider.tsx`:
- Around line 63-107: The provider currently includes the whole `versions` state
in `contextValue`, so any refetch bumps the context identity and rerenders all
`useRSC()` consumers; replace this with a keyed subscription mechanism so only
the affected cache key consumers rerender. Add a subscription registry (e.g. a
Map from payload key to Set of callbacks) alongside `fetchRSCPromisesRef`,
expose `subscribe(key, cb)` and `unsubscribe(key, cb)` in the context returned
by useMemo (include `getComponent` and `refetchComponent`), and in
`refetchComponent` after replacing the promise invoke only the callbacks for
that key; remove `versions` from the context and from the useMemo deps so
context identity no longer changes for unrelated keys. Ensure symbols referenced
are `fetchRSCPromisesRef`, `getComponent`, `refetchComponent`, `contextValue`,
`versions`, and `createRSCPayloadKey`.
🪄 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: c8dd0853-2590-4157-a72c-5b7d01b6d6d0
📒 Files selected for processing (13)
CHANGELOG.mddocs/oss/migrating/rsc-troubleshooting.mddocs/pro/react-server-components/inside-client-components.mdpackages/react-on-rails-pro/README.mdpackages/react-on-rails-pro/jest.config.jspackages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails-pro/tests/imperativeRefetch.client.test.tsxreact_on_rails_pro/spec/dummy/client/app/components/InlineRefreshButton.tsxreact_on_rails_pro/spec/dummy/client/app/components/RefetchStressPage.tsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RefetchStressServerComponent.jsxreact_on_rails_pro/spec/dummy/e2e-tests/refetch-stress.spec.ts
…ution, e2e baseURL) Three real failures + one infra flake observed on PR #3215: 1. markdown-link-check: CHANGELOG entry linked to placeholder PR 3231 which 404s. Fix: use the real PR number 3215. 2. build (22) → JS unit tests for react-on-rails-pro: the `moduleNameMapper` introduced for the non-rsc bucket (to dedupe React across the dual install) ALSO ran for the rsc bucket. A directory-path redirect bypasses Jest's customExportConditions resolution, so `*.rsc.test.tsx` ended up loading `react/index.js` instead of `react.react-server.js` and all four suites died with 'The "react" package in this environment is not configured correctly'. Fix: branch the mapper on `NODE_CONDITIONS` and, under react-server, redirect to the explicit `*.react-server.js` files so dedupe still happens but Jest no longer needs to consult package exports. 7/7 rsc tests pass; 44/44 non-rsc tests still pass. 3. dummy-app-node-renderer-e2e-tests: the new Playwright spec hardcoded http://127.0.0.1:3002 (my local Rails port). CI runs Rails on 3000. Fix: use a relative URL so Playwright prepends its configured baseURL (matches CI). The accompanying comment about BASE_URL/PLAYWRIGHT_BASE_URL env-var overrides was also wrong; the project's playwright.config.ts doesn't read either, so the comment now correctly points users at the config file for non-default ports. Reviewed in two rounds with codex (gpt-5.4, high reasoning); first round rejected an empty `{}` mapper for the rsc branch as fragile against future hook-using rsc tests; this commit's targeted file- path mapper was approved on round 2. CodeQL's 'Requires authentication' result-upload error is an infra/permissions issue and is left for CI re-run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| const contextValue = useMemo( | ||
| () => ({ getComponent, refetchComponent }), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [getComponent, refetchComponent, versions], | ||
| ); |
There was a problem hiding this comment.
The versions side-channel in useMemo deps works, but it requires suppressing the exhaustive-deps rule for a non-obvious reason. An alternative that's equally correct and lint-clean is to include the version counter directly in the context value:
| const contextValue = useMemo( | |
| () => ({ getComponent, refetchComponent }), | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [getComponent, refetchComponent, versions], | |
| ); | |
| const contextValue = useMemo( | |
| () => ({ getComponent, refetchComponent, _v: versions }), | |
| [getComponent, refetchComponent, versions], | |
| ); |
_v would need to be stripped from the RSCContextType or the type updated — alternatively, a dedicated _cacheVersion field with a clear doc comment. Either way, the intent becomes explicit and the eslint-disable goes away.
If keeping the current shape is important (don't expose _v to consumers), the current approach is fine, but the eslint-disable should carry a stronger explanation of why the factory function intentionally doesn't reference versions (it's a re-render trigger, not a computation input).
| ref?: Ref<RSCRouteHandle>; | ||
| }; | ||
|
|
||
| const CurrentRSCRouteContext = createContext<RSCRouteHandle | null>(null); |
There was a problem hiding this comment.
Missing displayName on CurrentRSCRouteContext. This makes React DevTools show the generic Context.Provider label in the component tree instead of something meaningful.
| const CurrentRSCRouteContext = createContext<RSCRouteHandle | null>(null); | |
| const CurrentRSCRouteContext = createContext<RSCRouteHandle | null>(null); | |
| CurrentRSCRouteContext.displayName = 'CurrentRSCRouteContext'; |
| /** | ||
| * Imperative handle exposed by `<RSCRoute>` via `ref`. | ||
| * | ||
| * `refetch()` re-fetches the server component using the RSCRoute's currently | ||
| * rendered `componentName` and `componentProps`. It resolves with the new | ||
| * rendered ReactNode and rejects if the fetch fails. | ||
| * | ||
| * Behavior caveats: | ||
| * - **Concurrent refetches:** only the most-recent cache write wins; earlier | ||
| * returned promises may resolve with stale data while the UI has already | ||
| * moved on to a later refetch. | ||
| * - **Unmount:** if the owning `<RSCRoute>` unmounts before resolution, the | ||
| * shared cache still updates (so other RSCRoutes bound to the same key | ||
| * reflect the new payload) but no visible re-render happens in the | ||
| * unmounted instance. | ||
| */ |
There was a problem hiding this comment.
Two consecutive JSDoc blocks here, both ending immediately before export type RSCRouteHandle, creates a layout mismatch: the first block (the original RSCRoute component doc, lines 64-84) is now orphaned from its component definition at line 150, while this second block documents RSCRouteHandle correctly.
A reader scanning top-to-bottom will associate the first block with RSCRouteHandle and miss the connection to RSCRoute. Consider:
- Moving the
RSCRoutecomponent docblock directly above itsforwardRefcall (line 150), or - Adding a small visual separator comment (
// --- types ---) between the two doc blocks to make the split explicit.
| }; | ||
|
|
||
| return ( | ||
| <button type="button" data-testid={testId} disabled={pending} onClick={handleClick}> | ||
| {pending ? 'Refreshing…' : label} | ||
| </button> | ||
| ); | ||
| }; | ||
|
|
||
| export default InlineRefreshButton; |
There was a problem hiding this comment.
handleClick is recreated on every render. This component is 'use client' and is used as a demo, but it will be copy-pasted by users as a reference pattern. Wrapping in useCallback is the idiomatic form for an event handler that calls a stable hook-returned function:
| }; | |
| return ( | |
| <button type="button" data-testid={testId} disabled={pending} onClick={handleClick}> | |
| {pending ? 'Refreshing…' : label} | |
| </button> | |
| ); | |
| }; | |
| export default InlineRefreshButton; | |
| const handleClick = React.useCallback(() => { | |
| setPending(true); | |
| refetch() | |
| .catch((err: unknown) => { | |
| console.error('InlineRefreshButton refetch failed', err); | |
| }) | |
| .finally(() => { | |
| setPending(false); | |
| }); | |
| }, [refetch]); |
refetch from useCurrentRSCRoute() is already stable (memoized inside RSCRoute), so the useCallback dep array won't thrash.
| export type RSCRouteHandle = { | ||
| refetch: () => Promise<ReactNode>; |
There was a problem hiding this comment.
The refetch() return value has a subtle trap in the concurrent-refetch case that isn't captured by the type: refetch() returns the promise for this particular network request, but after a second rapid refetch() call the cache has already moved on to the newer promise. The first returned promise will resolve with a payload that the UI never renders — it's been superseded.
The behaviour caveats block above mentions this for concurrent refetches ("earlier returned promises may resolve with stale data"), which is good. Consider adding a one-liner to the JSDoc reinforcing that callers should not rely on the resolved value for rendering decisions:
| export type RSCRouteHandle = { | |
| refetch: () => Promise<ReactNode>; | |
| export type RSCRouteHandle = { | |
| /** | |
| * Re-fetches the server component with its currently-rendered props. | |
| * | |
| * The returned promise resolves when this specific network request | |
| * completes. **Do not use the resolved value to drive rendering** — the | |
| * component updates itself automatically, and in a concurrent-refetch | |
| * scenario the UI may have already moved past this request's payload | |
| * before it resolves. | |
| */ | |
| refetch: () => Promise<ReactNode>; | |
| }; |
| '^react$': '<rootDir>/../../node_modules/react/react.react-server.js', | ||
| '^react/jsx-runtime$': '<rootDir>/../../node_modules/react/jsx-runtime.react-server.js', | ||
| '^react/jsx-dev-runtime$': '<rootDir>/../../node_modules/react/jsx-dev-runtime.react-server.js', |
There was a problem hiding this comment.
These paths hard-code React 19.x internal filenames. They're correct today but will silently fail with an inscrutable "module not found" if React renames these exports in a future major/minor. The PR description already tracks "Removing the duplicate React install" as the proper fix. In the meantime, adding an explicit note about which React version range these are stable for will help upgraders know to recheck:
| '^react$': '<rootDir>/../../node_modules/react/react.react-server.js', | |
| '^react/jsx-runtime$': '<rootDir>/../../node_modules/react/jsx-runtime.react-server.js', | |
| '^react/jsx-dev-runtime$': '<rootDir>/../../node_modules/react/jsx-dev-runtime.react-server.js', | |
| '^react$': '<rootDir>/../../node_modules/react/react.react-server.js', | |
| // jsx-runtime and jsx-dev-runtime filenames are stable in React 19.x; | |
| // verify against react/package.json#exports["react-server"] on React upgrades. | |
| '^react/jsx-runtime$': '<rootDir>/../../node_modules/react/jsx-runtime.react-server.js', | |
| '^react/jsx-dev-runtime$': '<rootDir>/../../node_modules/react/jsx-dev-runtime.react-server.js', |
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_pro/spec/dummy/e2e-tests/refetch-stress.spec.ts`:
- Around line 74-79: The test "6. rapid double-click: UI ends up showing the
latest payload" only issues a single click; change the interaction with the
'rapid-button' to perform a rapid double-click (either call click twice in
immediate succession on page.getByTestId('rapid-button') or use Playwright's
dblclick/click({ clickCount: 2 })) so the test actually triggers two quick
refetches and then poll the 'stress-time-rapid' textContent as before to assert
it changes from the original value.
🪄 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: 019e5991-ed96-4bea-8bd0-a650c393de8d
📒 Files selected for processing (3)
CHANGELOG.mdpackages/react-on-rails-pro/jest.config.jsreact_on_rails_pro/spec/dummy/e2e-tests/refetch-stress.spec.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Code Review — PR #3215: Imperative refetch API for
|
| File | Topic | Severity |
|---|---|---|
RSCProvider.tsx:103-107 |
versions side-channel in useMemo deps — suggest cleaner alternative or stronger comment |
Nit |
RSCRoute.tsx:102-103 |
Add to RSCRouteHandle JSDoc: don't rely on resolved value for rendering |
Minor |
RSCRoute.tsx:86-101 |
Two consecutive orphaned doc blocks — move RSCRoute's block next to its forwardRef |
Nit |
RSCRoute.tsx:112 |
Missing CurrentRSCRouteContext.displayName |
Nit |
jest.config.js:46-48 |
Hardcoded React 19.x internal filenames — add version-range note for upgraders | Minor |
InlineRefreshButton.tsx:24-33 |
handleClick should be useCallback-wrapped (it's the reference pattern users will copy) |
Minor |
Pre-existing items called out in PR description (no action needed on this PR)
- Duplicate React install (
pnpm.overridesfix) — tracked as follow-up test:non-rscregex hole silently skipping several test files — tracked as follow-upregisterServerComponent.client.test.jsxbroken import since PR convert testUtils to ts #2074 — tracked as follow-up
Overall: ready to merge after addressing the minor items above (at minimum the RSCRouteHandle JSDoc clarification and the InlineRefreshButton useCallback). The core implementation is sound.
🤖 Generated with Claude Code
Adds two ergonomic ways to refetch a `<RSCRoute>` without the caller having
to know its `componentName` or `componentProps`:
1. Ref handle — `<RSCRoute ref={...} />` exposes `RSCRouteHandle` with
`refetch()`. A parent or sibling can drive a refresh.
2. `useCurrentRSCRoute()` hook — a client component rendered inside the
server component's subtree can refetch the surrounding RSCRoute
without being passed any props. Throws "useCurrentRSCRoute must be
used inside an <RSCRoute>" if used outside one.
Implementation:
- RSCProvider now holds the per-cache-key `versions` map in `useState` and
bumps it inside `startTransition` on every refetchComponent call. That
propagates to every `useRSC()` consumer and lets React keep old content
visible while the new RSC payload streams in (no Suspense fallback
flash). Cache itself stays in `useRef` — only the version map is
reactive.
- RSCRoute is now a `forwardRef` component. It memoizes a stable handle
via `useImperativeHandle`, exposes the same handle to descendants via
`CurrentRSCRouteContext`, and uses `latestPropsRef` so a captured
refetch handle always reads the current name+props (regression-tested).
- Existing `refetchComponent(name, props)` signature and the
`ServerComponentFetchError` retry flow are untouched — purely additive.
Tests:
- `tests/imperativeRefetch.client.test.tsx` covers all 7 acceptance
criteria from the issue plus a multi-instance fan-out test (two
RSCRoutes sharing a cache key both update on a single refetch).
- `jest.config.js` adds a `react`/`react-dom` moduleNameMapper to dedupe
the workspace's two React copies into the test process. Without it,
hooks called from the package source see a null dispatcher because
`@testing-library/react` resolves React from root while the package
source resolves it from `packages/react-on-rails-pro/node_modules`.
Latent issue introduced by #2155 when react was added as a devDep.
Dummy app stress page (`/server_router/refetch-stress`):
Eight scenarios — ref handle, hook from inside the RSC subtree,
multi-instance fan-out, independent siblings (different props),
captured-handle survives a prop change, rapid double-click, many siblings
with distinct keys, mount/unmount cycle. None of the scenario components
use a caller-side `setKey`/`useState` workaround — that's the whole
point.
Playwright spec at `e2e-tests/refetch-stress.spec.ts` drives all eight
scenarios end-to-end (8/8 passing on Chromium and Firefox locally).
Closes #3106.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- CHANGELOG.md: add `#### Added` entry under [Unreleased] for the
imperative refetch API.
- docs/pro/react-server-components/inside-client-components.md: new
"Manually refetching a server component" section with the three
patterns (ref handle, useCurrentRSCRoute hook, useRSC().refetchComponent)
and a "when to use which" table; new rows in the API reference table
for `RSCRouteHandle` and `useCurrentRSCRoute`.
- InlineRefreshButton, RefetchStressPage: extract handlers into named
functions and replace `void` operators with explicit
`.catch(() => {})` to satisfy the project's
`@typescript-eslint/no-misused-promises`, `no-void`, and
`no-confusing-void-expression` rules.
- imperativeRefetch.client.test.tsx: prettier auto-formatting only.
No behavior changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ix README imports
- docs/oss/migrating/rsc-troubleshooting.md: at the end of "Finer-Grained
Retry with refetchComponent", add a pointer to the new
`<RSCRoute ref={...}>` and `useCurrentRSCRoute()` APIs in
inside-client-components.md for refetch triggers that aren't part of
an error-recovery flow.
- packages/react-on-rails-pro/README.md: fix the RSC code snippet that
used named imports (`import { RSCRoute }`,
`import { wrapServerComponentRenderer }`) for what are actually
default exports, and corrected the registerServerComponent client
call to use the string-name signature instead of the server-side
object form. Pre-existing bug, but worth fixing now that the same
module also publishes new named exports (`useCurrentRSCRoute`,
`RSCRouteHandle`) — readers shouldn't be left guessing which is
which.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eview nits
Addresses code-review feedback on the imperative refetch PR:
- Add test 9 to imperativeRefetch.client.test.tsx: a deterministic
last-write-wins concurrency test. Two refetches are issued back-to-
back without resolving either, then the SECOND refetch's promise is
resolved BEFORE the first's. Asserts the UI shows the second
payload, then resolves the first promise and asserts the UI does
NOT regress to the first payload. This proves the cache-write +
transition ordering survives out-of-order resolution; previously
only single refetch and "rapid double-click changed text" were
covered.
- inside-client-components.md: clarify that
`useRSC().refetchComponent()` invoked from inside an error-boundary
fallback only refreshes the cache. The route is unmounted while the
fallback is showing, so the boundary still has to be reset (e.g. via
`resetErrorBoundary()`) for the route to re-mount and pick up the
new payload. The new "ref" / `useCurrentRSCRoute()` APIs do not have
this constraint.
- RefetchStressPage scenario 8 + matching e2e: the page used to
initialize the displayed ref-state to "set" so the e2e's first
assertion was a tautology against seeded UI text rather than a real
ref observation. Initialize to "unchecked" instead and have the e2e
click "Check ref.current" before each assertion.
No source-code change in `RSCProvider.tsx` or `RSCRoute.tsx` — the
implementation already had the right behavior, the gap was test
coverage.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ution, e2e baseURL) Three real failures + one infra flake observed on PR #3215: 1. markdown-link-check: CHANGELOG entry linked to placeholder PR 3231 which 404s. Fix: use the real PR number 3215. 2. build (22) → JS unit tests for react-on-rails-pro: the `moduleNameMapper` introduced for the non-rsc bucket (to dedupe React across the dual install) ALSO ran for the rsc bucket. A directory-path redirect bypasses Jest's customExportConditions resolution, so `*.rsc.test.tsx` ended up loading `react/index.js` instead of `react.react-server.js` and all four suites died with 'The "react" package in this environment is not configured correctly'. Fix: branch the mapper on `NODE_CONDITIONS` and, under react-server, redirect to the explicit `*.react-server.js` files so dedupe still happens but Jest no longer needs to consult package exports. 7/7 rsc tests pass; 44/44 non-rsc tests still pass. 3. dummy-app-node-renderer-e2e-tests: the new Playwright spec hardcoded http://127.0.0.1:3002 (my local Rails port). CI runs Rails on 3000. Fix: use a relative URL so Playwright prepends its configured baseURL (matches CI). The accompanying comment about BASE_URL/PLAYWRIGHT_BASE_URL env-var overrides was also wrong; the project's playwright.config.ts doesn't read either, so the comment now correctly points users at the config file for non-default ports. Reviewed in two rounds with codex (gpt-5.4, high reasoning); first round rejected an empty `{}` mapper for the rsc branch as fragile against future hook-using rsc tests; this commit's targeted file- path mapper was approved on round 2. CodeQL's 'Requires authentication' result-upload error is an infra/permissions issue and is left for CI re-run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drive-by formatting-only fix. The file was added to main by PR #3207 ("Add /stress-test Claude Code command for adversarial QA") with prettier-violating markdown table alignment and italic syntax. This has left main's `lint-js-and-ruby` workflow red since 2026-04-30 and is blocking my PR's `build` check via the merged-with-main CI gate. The diff is purely formatter output (`prettier --write`): - markdown table column padding - `*(empty)*` -> `_(empty)_` italic-syntax normalization No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AbanoubGhadban
force-pushed
the
worktree-issue-3106-rsc-refetch-api
branch
from
bc5b97b to
51b152a
Compare
|
placeholder - will edit |
|
Test body with no special chars |
|
Code Review: feat(pro) — Imperative Refetch API for RSCRoute Overview This PR adds two clean ergonomic APIs to let callers imperatively refresh an RSCRoute without knowing its componentName or componentProps:
The design is principled: latestPropsRef prevents stale-closure bugs, the internal startTransition avoids Suspense fallback flashes, and multi-instance fan-out works correctly because all RSCRoute instances sharing the same cache key pick up the version bump. The unit-test suite is excellent — eight targeted scenarios including a deterministic out-of-order concurrency test. Issues Worth Addressing
fetchRSCPromisesRef.current accumulates an entry for every unique componentName + JSON.stringify(componentProps) pair seen across the lifetime of the provider and never removes any entry. For an SPA that renders server components with many different prop values (e.g., per-user cards, per-post pages), this accumulates stale promises indefinitely — a latent memory leak that grows proportionally to distinct prop combinations seen, regardless of whether those components are still mounted. At minimum, a @remarks in the JSDoc acknowledging the trade-off and recommending short-lived / low-cardinality props would set correct expectations. Ideally a simple LRU bound or WeakRef-based eviction would prevent growth entirely.
versions is listed in the useMemo deps of contextValue specifically so that any change to versions recreates the context object and forces every useRSC() consumer to re-render. This is correct for propagating the new payload, but the side-effect is that a refetch of any single RSCRoute re-renders every RSCRoute in the same provider tree, not just the ones matching the updated cache key. For the typical router use case (one RSCRoute visible at a time) this is negligible. But on pages that render many independent RSCRoutes (e.g., the scenario-7 stress page with 5 instances), a refresh of one route causes 4 unnecessary sibling re-renders, each calling createRSCPayloadKey / JSON.stringify for their props. At larger scales (dashboards, feed pages) this could cause measurable jank. A targeted fix is a larger refactor; for now, documenting the known limitation and tracking as a follow-up is sufficient.
RSCRouteHandle.refetch() returns the raw fetch promise from refetchComponent, which resolves when the HTTP response is decoded, NOT when React transitions have committed the new content to the DOM. The PR description notes the caveat clearly, but the JSDoc on RSCRouteHandle does not mention it, and the type Promise implies the resolved value is meaningful — it rarely is for callers who just want to wait for the UI to stabilize. A one-line note in the type JSDoc would prevent confusion. Minor Observations
Test Coverage The unit test suite is thorough — the deterministic concurrency test (out-of-order resolution) is particularly good. The Playwright e2e suite validates all 8 stress scenarios end-to-end. One gap: there is no test verifying that ref.current is null when RSCRoute is inside an error boundary currently showing its fallback. Given the documented caveats about the error-recovery flow, a unit test confirming this boundary condition would strengthen confidence. Summary The implementation is correct and the test coverage is solid. The two items to prioritise before merge are: (1) unbounded cache growth — at minimum a JSDoc note, ideally a size cap; and (2) a JSDoc note on the refetch() promise semantics (resolves on fetch completion, not DOM commit). The fan-out re-render concern is real but acceptable for now as long as it is tracked as a follow-up. |
| }) => { | ||
| const fetchRSCPromises: Record<string, Promise<ReactNode>> = {}; | ||
| return ({ children }: { children: ReactNode }) => { | ||
| const fetchRSCPromisesRef = useRef<Record<string, Promise<ReactNode>>>({}); |
There was a problem hiding this comment.
Unbounded cache growth — no eviction policy
fetchRSCPromisesRef.current accumulates entries for every unique componentName + JSON.stringify(componentProps) pair seen during the provider's lifetime and never removes any of them. Once a promise is written here (whether from the initial fetch or a subsequent refetch), it lives forever — including for component+prop combinations that haven't been rendered in hours.
For the router use case (a small, finite set of routes), this is harmless. But for callers who pass high-cardinality props (per-user IDs, per-post slugs, paginated offsets), the map grows indefinitely. The promises themselves keep the resolved RSC payloads (entire React trees) in memory.
Suggested mitigations (in order of preference):
- LRU eviction — cap the map at N entries (e.g. 50 or 100) and evict the oldest on overflow.
- WeakRef +
FinalizationRegistry— storeWeakRef<Promise<ReactNode>>and drop the entry when the promise is GC'd. - JSDoc
@remarks— at minimum, document the trade-off and recommend low-cardinality props.
If a full fix is out of scope for this PR, please open a follow-up issue so it is tracked.
| const contextValue = useMemo( | ||
| () => ({ getComponent, refetchComponent }), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [getComponent, refetchComponent, versions], |
There was a problem hiding this comment.
Whole-tree fan-out on every refetch
Including versions in useMemo deps causes the contextValue object identity to change on every refetch, which forces all useRSC() consumers in the tree to re-render — not just the <RSCRoute> whose key changed. On a page with N independent RSCRoutes, refetching one causes N−1 unnecessary re-renders, each calling getComponent → createRSCPayloadKey → JSON.stringify(componentProps).
This is also why the eslint-disable is needed: the deps contain versions, which is never referenced inside the memo body. That is a recognised anti-pattern (side-effectful deps). An alternative that avoids both the lint suppression and the implicit coupling:
// Instead of a hidden dep, expose versions as a private field:
const contextValue = useMemo(
() => ({ getComponent, refetchComponent, _v: versions }),
[getComponent, refetchComponent, versions],
);…then keep _v out of the public RSCContextType. Object identity still changes on refetch, consumers still re-render, but the dep is actually used and no lint disable is needed.
The fan-out re-render is acceptable for typical router use (one visible RSCRoute at a time), but worth a follow-up to introduce per-key subscriptions via useSyncExternalStore for dashboard / feed-style pages.
| export type RSCRouteHandle = { | ||
| refetch: () => Promise<ReactNode>; |
There was a problem hiding this comment.
refetch() promise resolves on fetch completion, not on DOM commit
The returned Promise<ReactNode> resolves when getServerComponent resolves (i.e., the HTTP payload is decoded), not when React has committed the new tree to the DOM. The DOM update happens asynchronously inside a startTransition in RSCProvider.refetchComponent, so code like:
await ref.current.refetch();
// DOM may still show the old content here — the transition is still pending…will not reliably observe the new content immediately after await.
The PR description documents this, but the JSDoc on this type is the canonical API reference. Suggest adding a @remarks:
export type RSCRouteHandle = {
/**
* Re-fetches the server component using the RSCRoute's current name and props.
*
* @returns A promise that resolves with the new RSC payload when the **fetch**
* completes. This is NOT a signal that React has committed the updated tree —
* that happens asynchronously via `startTransition`. Most callers can ignore
* the return value and let `<RSCRoute>` update on its own.
*/
refetch: () => Promise<ReactNode>;
};
Code Review — PR #3215: Imperative refetch API for ``SummaryThe implementation is solid and well-tested. The core design decisions are sound:
Issues to address before merge1.
|
| // refetch (inside startTransition) is what makes <RSCRoute> consumers re- | ||
| // render with the new promise from the cache while React keeps the old | ||
| // tree visible until the new payload resolves. | ||
| const [versions, setVersions] = useState<Record<string, number>>({}); |
There was a problem hiding this comment.
Memory growth: versions is never evicted. Every distinct cache key that has ever been refetched adds a permanent { [key]: number } entry. On a long-lived SPA that renders many different (componentName, componentProps) pairs this will grow without bound.
Consider either (a) pruning old keys when the RSCRoute unmounts, (b) capping growth by using % Number.MAX_SAFE_INTEGER, or at minimum add a comment documenting that this map is unbounded so a future maintainer can weigh the tradeoff.
| const contextValue = useMemo( | ||
| () => ({ getComponent, refetchComponent }), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [getComponent, refetchComponent, versions], |
There was a problem hiding this comment.
The eslint-disable is correct — versions is in deps intentionally as a render-trigger, not because the callback uses it. But this is a non-obvious pattern and will confuse reviewers (they'll wonder why the disable is here and whether it hides a bug).
One way to make the intent self-documenting without changing behaviour:
| [getComponent, refetchComponent, versions], | |
| // `versions` is not used inside the callback but is listed in deps so that | |
| // the memoized context value gets a new identity on every refetch, which | |
| // propagates the re-render to all useRSC() consumers. | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [getComponent, refetchComponent, versions], |
Or rename versions → refetchGeneration so the "trigger" role is obvious at a glance.
|
|
||
| const componentPromise = getComponent(componentName, componentProps); | ||
| return ( | ||
| <CurrentRSCRouteContext.Provider value={handle}> |
There was a problem hiding this comment.
Context accessibility during error states: CurrentRSCRouteContext.Provider wraps the error boundary, so a client component rendered in the error boundary's fallback can still call useCurrentRSCRoute() and get a valid handle. The documentation states this hook "can only be invoked while the route is… not in an error state," which is slightly inaccurate.
If the intent is to restrict access to non-error states, move the provider to inside the error boundary (or even just inside PromiseWrapper). If the intent is to allow refetch-from-fallback (which could be useful!), update the documentation accordingly.
| // steps that. The exact filenames used here are the targets | ||
| // listed in `react`'s own `package.json#exports` for the | ||
| // `react-server` condition (stable across React 19.x). | ||
| '^react$': '<rootDir>/../../node_modules/react/react.react-server.js', |
There was a problem hiding this comment.
This maps ^react$ to a hardcoded internal filename (react.react-server.js) that is specific to React 19's exports map. The comment says it's "stable across React 19.x", but these paths have changed between major versions before.
Suggest adding an inline TODO/version guard comment so the next React major upgrade doesn't silently break RSC tests:
| '^react$': '<rootDir>/../../node_modules/react/react.react-server.js', | |
| // Hardcoded to React 19 internal export targets. Re-verify after upgrading React past 19.x. | |
| '^react$': '<rootDir>/../../node_modules/react/react.react-server.js', |
Better long-term fix: remove the Pro package's local react/react-dom devDependencies (tracked in the PR description as out-of-scope follow-up), which makes this entire branch unnecessary.
| .catch((err: unknown) => { | ||
| console.error('InlineRefreshButton refetch failed', err); | ||
| }) | ||
| .finally(() => { |
There was a problem hiding this comment.
.finally(() => setPending(false)) can fire after the component has unmounted (e.g. user navigates away mid-fetch). React 18+ suppresses the old "setState on unmounted component" warning, but this can still cause unexpected state transitions if the component remounts quickly.
Since this is demo/example code that users are likely to copy, consider modeling the idiomatic safe pattern:
| .finally(() => { | |
| .finally(() => { | |
| // Guard is optional in React 18+ (no setState-on-unmount warning), | |
| // but protects against races if the component remounts before the fetch settles. | |
| setPending(false); |
Or use an AbortController to cancel the in-flight request and skip the state update entirely.
2 participants
Summary
Closes Issue #3106. Adds two ergonomic ways to refetch a
<RSCRoute>without the caller having to know itscomponentNameorcomponentProps:<RSCRoute ref={...} />exposesRSCRouteHandlewithrefetch(). A parent or sibling can drive the refresh.useCurrentRSCRoute()hook — a client component rendered inside the server component's subtree can refetch the surrounding<RSCRoute>without being passed any props. Throws the spec-mandated messageuseCurrentRSCRoute must be used inside an <RSCRoute>if called outside one.Both APIs auto-update the rendered tree — no caller-side
setKey/useStateworkaround. The internal cache invalidation runs inside a React transition so old content stays visible while the new RSC payload streams in (no Suspense-fallback flash). Refetching propagates to every<RSCRoute>instance bound to the same cache key.The existing
useRSC().refetchComponent(name, props)API and theServerComponentFetchError-based retry flow are unchanged — purely additive.Implementation highlights
RSCProvideris now a real React component. The cache lives in auseRef(not reactive) and a per-cache-keyversions: Record<string, number>map lives inuseState.refetchComponentwrites the new promise into the cache synchronously, then bumpsversions[key]insidestartTransition. Listingversionsin the context value'suseMemodeps is what propagates the re-render to everyuseRSC()consumer; the surroundingstartTransitionmakes those re-renders transition commits.<RSCRoute>is converted toforwardRefand holds alatestPropsRefso a captured-then-stalerefetchcallback always reads the current name+props. It provides its handle via a privateCurrentRSCRouteContextso descendants in the RSC subtree can read it viauseCurrentRSCRoute().See Issue #3106 for the full acceptance criteria.
Stress demo
A new dummy-app page at
/server_router/refetch-stressexercises 8 scenarios end-to-end:useCurrentRSCRoute()from inside the RSC subtree.ref.currentnull after unmount, set after re-mount).None of the scenarios use a caller-side
setKey/useStateworkaround — that is the whole point.Test plan
pnpm --filter react-on-rails-pro run type-check— passes.pnpm --filter react-on-rails-pro run test:non-rsc— 44 / 44 pass (9 new, 35 pre-existing).react_on_rails_pro/spec/dummy/e2e-tests/refetch-stress.spec.ts) covers all 8 stress scenarios — 8 / 8 pass on Chromium against the local dev stack.67500f13f; round 2 verdict: APPROVED.Side fixes
packages/react-on-rails-pro/jest.config.js— addedmoduleNameMapperto dedupereact/react-domto the workspace root copy. Without it, hooks called from the Pro package source see a null React dispatcher because@testing-library/reactresolves React from root (19.2.3) while the package source resolves it frompackages/react-on-rails-pro/node_modules(19.2.0). Latent bug introduced by PR #2155 whenreact/react-domwere added as devDependencies of the Pro package — the mismatch wasn't surfaced because thetest:non-rscjest pattern excludes every test that combines@testing-library/reactrendering with hook-bearing source paths.packages/react-on-rails-pro/README.md— fixed pre-existing import-style bug (named-import syntax for what are actually default exports).Out of scope (worth follow-up issues, not this PR)
pnpm.overridesor dropping the redundant Pro devDep) so thejest.config.jsmapper becomes unnecessary.test:non-rscregex hole that has been silently skippingregisterServerComponent.client.test.jsx,SuspenseHydration.test.tsx,streamServerRenderedReactComponent.test.jsx, and others — including, ironically, several tests that would have caught the React-duplication issue in CI before this PR.registerServerComponent.client.test.jsxis independently broken since PR #2074 (Nov 20, 2025) which renamedtestUtils.js→testUtils.tsbut didn't update the test's import path.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Changelog