Test React on Rails 17.0.0.rc.7#775
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR reworks LiveActivityRefresher to normalize server-component refetch failures into ServerComponentFetchError, show a dedicated fallback UI, and retry with corrected props. It also updates related copy, tests, and dependency version targets. ChangesServer component error recovery UI
Dependency version bumps
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
ReviewThis PR is a mechanical dependency bump ( However, the PR's own RC Test Report documents a confirmed regression and marks this lane BLOCKED — I'd treat that as a hard blocker, not just informational:
Given that, this shouldn't be merged as-is per the report's own conclusion — either the upstream rc.7 regression needs a fix/explanation, or this needs to be explicitly waived by whoever owns the react_on_rails 17.0.0 release gate. No other issues found in the diff itself. |
|
Fix pushed in 3a8788b. What changed:
Local validation:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const handleSimulateError = () => { | ||
| setSimulateError(true); | ||
| setRefreshKey((k) => k + 1); | ||
| const nextKey = refreshKey + 1; | ||
| const errorProps = { simulateError: true, refreshKey: nextKey }; | ||
|
|
||
| setFetchError(null); | ||
| setRefreshKey(nextKey); | ||
| refetchLiveActivity(errorProps).catch((error) => { | ||
| setFetchError(toServerComponentFetchError(error, errorProps)); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Redundant fetch on "Simulate Error"
handleSimulateError bumps refreshKey (line 78) and fires a manual refetchLiveActivity(errorProps) (line 79) with different props (simulateError: true).
Since refreshKey is also embedded in the componentProps used by the visible <RSCRoute> at line 105/143 ({ simulateError: false, refreshKey }), bumping it causes the mounted RSCRoute to independently re-fetch with simulateError: false — a legitimate, successful fetch that briefly renders (or holds Suspense) before being discarded the moment fetchError resolves and swaps the tree to <ThrowActivityError>.
So every "Simulate Error" click triggers two RSC round-trips: one real fetch that's thrown away, and one error-only fetch used to derive the failure. Net effect (besides the wasted request) is a possible flicker where real stats render for a moment before the error fallback replaces them.
Consider not bumping the display refreshKey for the visible RSCRoute when simulating an error (e.g. track "refresh count" in separate state from the value used as componentProps.refreshKey), so only the intentional error-fetch happens.
| }); | ||
| }; | ||
|
|
||
| const buildBoundaryRetry = (resetErrorBoundary) => () => { |
There was a problem hiding this comment.
Minor: buildBoundaryRetry calls handleRetry() (which already bumps refreshKey/clears fetchError, causing the ErrorBoundary's resetKeys to change and auto-reset per react-error-boundary's own reset-on-resetKeys-change behavior) and then also explicitly calls resetErrorBoundary(). The explicit call looks redundant given resetKeys={[refreshKey]} below — harmless, but worth a comment explaining why both are needed if there's a reason (e.g. avoiding a one-render flash), otherwise consider dropping one.
|
|
||
| gem "cpflow", "5.1.1", require: false | ||
| gem "react_on_rails_pro", "17.0.0.rc.6" | ||
| gem "react_on_rails_pro", "17.0.0.rc.7" |
There was a problem hiding this comment.
Version bump to 17.0.0.rc.7 — per the PR description, this lane is reported as BLOCKED: one system spec (spec/system/server_components_demo_spec.rb:35) regresses under rc.7 vs. the rc.6 baseline. Given this, and that the RC report explicitly says "Do not count this hard-gate lane as green... until the rc.7 RSC error-boundary regression is fixed, explained, or explicitly waived," this PR shouldn't be merged as-is — worth confirming CI is green after the LiveActivityRefresher workaround before merging, and that the workaround doesn't just paper over the underlying regression for other (non-simulated) error paths.
Review SummaryOverviewThis is a demo-fleet RC-validation PR bumping Status flagThe PR's own description reports this lane as BLOCKED: a system spec regression ( Code quality —
|
Greptile SummaryThis PR tests the next React on Rails RC package set in the server-components demo. The main changes are:
Confidence Score: 4/5The changed RSC error recovery path can leave the demo showing stale content instead of the fallback.
client/app/bundles/server-components/components/LiveActivityRefresher.jsx Important Files Changed
Reviews (1): Last reviewed commit: "Fix RSC error recovery demo" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| setRefreshKey(nextKey); | ||
| refetchLiveActivity(errorProps).catch((error) => { | ||
| setFetchError(toServerComponentFetchError(error, errorProps)); |
There was a problem hiding this comment.
When Simulate Error is clicked, this path advances refreshKey but the rendered <RSCRoute> always receives simulateError: false; the fallback only appears if the imperative refetchComponent call rejects and sets fetchError. If the RSC refetch API returns the failed request without rejecting, the boundary never sees an error and the page keeps showing the stale LiveActivity content instead of Server component fetch failed.
| const buildBoundaryRetry = (resetErrorBoundary) => () => { | ||
| handleRetry(); | ||
| resetErrorBoundary(); | ||
| }; |
There was a problem hiding this comment.
The retry handler starts refetchLiveActivity(correctedProps) and then resets the boundary immediately, before the async refetch can prime the cache described above. On retry, the boundary can remount <RSCRoute> before the corrected payload is cached, causing an avoidable duplicate fetch or skeleton flash instead of the intended cache-hit recovery path.
| // eslint-disable-next-line no-console | ||
| .catch((err) => console.error('Retry refetch failed:', err)) | ||
| .finally(() => resetErrorBoundary()); | ||
| .catch((err) => { | ||
| console.error('Retry refetch failed:', err); | ||
| setFetchError(toServerComponentFetchError(err, correctedProps)); | ||
| }); |
There was a problem hiding this comment.
eslint-disable-next-line only suppresses the rule on the line immediately following it. Here it precedes .catch((err) => { (line 94), not the actual console.error(...) call on line 95 — so the no-console violation is no longer suppressed where it matters. This worked in the old code because the callback was a single expression (.catch((err) => console.error(...))), but now that the callback has a multi-line body, the disable comment misses its target.
| // eslint-disable-next-line no-console | |
| .catch((err) => console.error('Retry refetch failed:', err)) | |
| .finally(() => resetErrorBoundary()); | |
| .catch((err) => { | |
| console.error('Retry refetch failed:', err); | |
| setFetchError(toServerComponentFetchError(err, correctedProps)); | |
| }); | |
| .catch((err) => { | |
| // eslint-disable-next-line no-console | |
| console.error('Retry refetch failed:', err); | |
| setFetchError(toServerComponentFetchError(err, correctedProps)); | |
| }); |
| const buildBoundaryRetry = (resetErrorBoundary) => () => { | ||
| handleRetry(); | ||
| resetErrorBoundary(); | ||
| }; |
There was a problem hiding this comment.
This contradicts the comment above handleRetry (lines 84-85): "refetchComponent primes the cache with corrected props before resetting the boundary, so the post-reset render hits cache instead of re-fetching."
buildBoundaryRetry calls handleRetry() and then resetErrorBoundary() synchronously, back-to-back — it does not wait for the refetchLiveActivity(correctedProps) promise inside handleRetry to settle. The old code used .finally(() => resetErrorBoundary()) to guarantee the boundary only reset after the refetch completed (success or failure); that guarantee is gone now, so:
- The boundary resets and
RSCRoutere-renders/re-fetches before the priming request necessarily finished, which can defeat the cache-hit optimization the comment describes. - If the corrected-props refetch subsequently rejects,
fetchErrorgets set again after the boundary already reset, producing a flicker (error → brief success/loading → error again).
Consider only resetting the boundary once the refetch promise resolves, e.g.:
const buildBoundaryRetry = (resetErrorBoundary) => () => {
const newKey = refreshKey + 1;
const correctedProps = { simulateError: false, refreshKey: newKey };
setFetchError(null);
setRefreshKey(newKey);
refetchLiveActivity(correctedProps)
.catch((err) => {
console.error('Retry refetch failed:', err);
setFetchError(toServerComponentFetchError(err, correctedProps));
})
.finally(() => resetErrorBoundary());
};
Review SummaryOverviewThis is a hard-gate RC-soak PR bumping Most significant concern: the app-code change may mask the regression the PR itself flags as a release blockerThe PR description explicitly calls out a blocking rc.7 regression: "RSC client error recovery via However, This means the demo/spec ( Two concrete bugs found in the same file (see inline comments)
Dependency/version bump review
Test coverage
Overall: the dependency bump itself looks mechanically sound, but the accompanying workaround in |
![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: 3a8788b11e
ℹ️ 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".
| refetchLiveActivity(errorProps).catch((error) => { | ||
| setFetchError(toServerComponentFetchError(error, errorProps)); |
There was a problem hiding this comment.
With a slow RSC payload, if the user clicks Simulate Error and then recovers by clicking Refresh before this request rejects, this catch still stores the old { simulateError: true } failure in state. Because the current render always uses simulateError: false and there is no request id or refreshKey guard before setFetchError, a stale failure can replace the successfully refreshed card with the error fallback; ignore superseded refetches before updating fetchError.
Useful? React with 👍 / 👎.
|
Follow-up pushed in 6da5a7d to address the review feedback on the first workaround pass. Review feedback addressed:
Intentional scope note: this PR keeps the tutorial demo on the explicit RSC refetch recovery path. It does not claim to fix the upstream declarative Fresh local validation on 6da5a7d:
|
| const handleSimulateError = () => { | ||
| setSimulateError(true); | ||
| setRefreshKey((k) => k + 1); | ||
| const requestId = nextRequestId(); | ||
| const nextKey = refreshCount + 1; | ||
| const errorProps = { simulateError: true, refreshKey: nextKey }; | ||
|
|
||
| setFetchError(null); | ||
| setRefreshCount(nextKey); | ||
| refetchLiveActivity(errorProps).catch((error) => { | ||
| if (latestRequestIdRef.current === requestId) { | ||
| setFetchError(toServerComponentFetchError(error, errorProps)); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
handleSimulateError only reacts to the rejected case of refetchLiveActivity. If the fetch were ever to resolve successfully here (e.g. the server stops honoring simulateError), nothing updates routeRefreshKey/re-renders RSCRoute, so the UI is left showing the previous card forever while "Refresh count" has already incremented — silently stale with no error and no new data. Since this path is only reached via the "Simulate Error" button (which is expected to always fail), this is low risk today, but it's worth at least a .then() branch (or a comment noting the assumption) so the code doesn't silently drift out of sync with refreshCount if the demo/server behavior changes.
| end | ||
|
|
||
| it "shows the ErrorBoundary fallback when Simulate Error is clicked, then recovers on Retry" do | ||
| it "shows the error recovery fallback when Simulate Error is clicked, then recovers on Retry" do |
There was a problem hiding this comment.
Per the PR description, this exact spec (server_components_demo_spec.rb:35) is reported as FAILING against the rc.7 node renderer (RENDERER_URL=http://localhost:3801 bundle exec rspec → 1 failure), while it passes against the rc.6 baseline. The PR's own "Release Impact" section says not to count this lane as green until the regression is fixed/waived.
Merging this PR as-is means master will carry a spec that's known to fail against the RC dependency set it just pinned to (rc.7 gem/npm pins in Gemfile/package.json). Suggest either:
- Holding this PR until the upstream RSC error-boundary regression (react_on_rails#4357-adjacent) is resolved in rc.8+, or
- If landing intentionally as a documented-red RC probe, marking this example
pending/skipped with a comment linking the tracking issue, so CI doesn't silently show a real regression as routine noise.
ReviewOverviewThis is the demo-fleet RC test lane bumping Correctness / Test Coverage
Code Quality
Security / Performance
Dependency Bump
RecommendationGiven this PR's own validation section reports a failing system spec caused by the very rc.7 bump it introduces, I'd treat this as not mergeable as green until the RSC error-boundary regression is resolved (or the failing spec is explicitly quarantined with a tracking link), per the PR's own "Release Impact" note. |
✅ Review App DeletedReview app for PR #775 is deleted |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/app/bundles/server-components/components/LiveActivityRefresher.jsx`:
- Around line 61-66: The refetchLiveActivity flow is passing an extra boolean to
refetchComponent and then checking for a resolved Error payload, which doesn’t
match the documented API or the existing rejection handling. Update
refetchLiveActivity in LiveActivityRefresher to call refetchComponent with only
the component name and props, and remove the payload instanceof Error branch so
failures are handled through the existing .catch path.
🪄 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: 5a26ca9c-6f84-4c02-9e54-fac995fa5205
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
GemfileREADME.mdclient/app/bundles/server-components/components/LiveActivityRefresher.jsxclient/app/bundles/server-components/ror_components/ServerComponentsPage.jsxpackage.jsonspec/system/server_components_demo_spec.rb
| const refetchLiveActivity = (componentProps) => | ||
| refetchComponent('LiveActivity', componentProps, true).then((payload) => { | ||
| if (payload instanceof Error) { | ||
| throw payload; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does react-on-rails-pro's useRSC().refetchComponent accept a third boolean parameter, and can its returned promise resolve with an Error instead of rejecting?
💡 Result:
Based on the current React on Rails Pro documentation and implementation, here are the answers to your questions: 1. Does refetchComponent accept a third boolean parameter? No. The refetchComponent(name, props) method accepts only two parameters: the component name and the component props [1][2][3]. 2. Can its returned promise resolve with an Error instead of rejecting? The behavior of the returned promise depends on the nature of the failure: - Promise Rejection: The underlying RSC fetch typically results in a rejected promise when a network, renderer, or deployment failure occurs [4]. Recent updates have ensured that these failures consistently reject, and the internal cache is updated to evict failed promises so that subsequent retries can succeed rather than returning a "sticky" failed state [4][5]. - Error as a Value: Technical documentation notes that payloads which explicitly resolve with an Error value (rather than throwing/rejecting) are intentionally left in the cache [4]. This is distinct from a failed fetch that causes a rejection [4]. In summary, refetchComponent is designed for error-retry flows where you provide the component name and props; it does not take a third boolean parameter [1][6], and it expects standard promise rejection patterns for fetch failures [4][5].
Citations:
- 1: https://github.com/shakacode/react_on_rails/blob/main/docs/pro/react-server-components/inside-client-components.md
- 2: Add imperative refetch API to RSCRoute:
refhandle anduseCurrentRSCRoutehook react_on_rails#3106 - 3: https://reactonrails.com/docs/upgrading/changelog
- 4: shakacode/react_on_rails@bf052b1
- 5: shakacode/react_on_rails@9e81bba
- 6: https://github.com/shakacode/react_on_rails/blob/main/docs/oss/migrating/rsc-troubleshooting.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'FILES:\n'
git ls-files | rg 'LiveActivityRefresher\.jsx|refetchComponent|useRSC|react_on_rails|server-components'
printf '\nUSAGES OF refetchComponent:\n'
rg -n "refetchComponent\(" client app . -g '!node_modules' -g '!dist' -g '!build' || true
printf '\nDEFINITIONS/EXPORTS AROUND useRSC:\n'
rg -n "useRSC|refetchComponent" client app . -g '!node_modules' -g '!dist' -g '!build' || trueRepository: shakacode/react-webpack-rails-tutorial
Length of output: 2370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'LIVEACTIVITYREFRESHER CONTEXT:\n'
cat -n client/app/bundles/server-components/components/LiveActivityRefresher.jsx | sed -n '1,140p'
printf '\nREADME CONTEXT:\n'
sed -n '210,250p' README.md
printf '\nREACT-ON-RAILS-PRO INITIALIZER:\n'
cat -n config/initializers/react_on_rails_pro.rbRepository: shakacode/react-webpack-rails-tutorial
Length of output: 9589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'PACKAGE/VERSION HINTS:\n'
rg -n "react-on-rails-pro|17\.0\.0-rc\.7|refetchComponent" package.json Gemfile Gemfile.lock yarn.lock pnpm-lock.yaml package-lock.json README.md config client -g '!node_modules' -g '!dist' -g '!build' || true
printf '\nRELEVANT DOCS SEARCH:\n'
rg -n "useRSC\(\)\.refetchComponent|refetchComponent\(name, props|refetchComponent" README.md docs client config -g '!node_modules' -g '!dist' -g '!build' || trueRepository: shakacode/react-webpack-rails-tutorial
Length of output: 5982
Drop the extra true and handle rejections directly. useRSC().refetchComponent(name, props) is documented as a two-argument call, and this path already has a .catch(...); the payload instanceof Error branch reads like it depends on an undocumented resolve-with-Error behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/app/bundles/server-components/components/LiveActivityRefresher.jsx`
around lines 61 - 66, The refetchLiveActivity flow is passing an extra boolean
to refetchComponent and then checking for a resolved Error payload, which
doesn’t match the documented API or the existing rejection handling. Update
refetchLiveActivity in LiveActivityRefresher to call refetchComponent with only
the component name and props, and remove the payload instanceof Error branch so
failures are handled through the existing .catch path.
1 participant
RC Test Report
Status: BLOCKED
This hard-gate demo-fleet PR tests the React on Rails 17.0.0.rc.7 package set in
shakacode/react-webpack-rails-tutorial.Target Versions
react_on_railsgem:17.0.0.rc.7(transitive throughreact_on_rails_pro)react-on-railsnpm package:17.0.0-rc.7(transitive throughreact-on-rails-pro)react_on_rails_progem:17.0.0.rc.7react-on-rails-pronpm package:17.0.0-rc.7react-on-rails-pro-node-renderernpm package:17.0.0-rc.7react-on-rails-rscnpm package:19.2.1-rc.0(required by the rc.7 node renderer during RC soak)shakapackergem/npm package: unchanged at10.2.0Baseline
production-2026-06-04-034633-26929107280, deployment4928065047, commitc5f3b16f7e45dbf53c951eb6c81252e5102800d4, deployment statussuccess.react_on_rails/react_on_rails_pro16.7.0.rc.3, Pro npm packages16.7.0-rc.3,react-on-rails-rsc19.0.4, shakapacker10.1.0.origin/masterat196504a, withreact_on_rails/react_on_rails_pro17.0.0.rc.6, Pro npm packages17.0.0-rc.6,react-on-rails-rsc19.2.0, shakapacker10.2.0.Changes
react_on_railsgem andreact-on-railsnpm package at rc.7.react-on-rails-rscfrom19.2.0to19.2.1-rc.0because the rc.7 node renderer refuses to start with19.2.0.Gemfile.lock,yarn.lock, and the README version-target block.Feature Accounting
.ruby-version/Gemfile Ruby3.4.6.config.server_render_method: not present.generated_assets_dirs,skip_display_none,defer_generated_component_packs: not present. Existing singularconfig.generated_assets_dirremains test-only app config.hydrate_onscheduling/fixes: not used in this app.createRailsAction/ TanStack: not applicable; app has classic Rails/Jbuilder JSON and Redux, no TanStack orcreateRailsActionsurface.bin/devclean and generator-created app files: not applicable to this dependency bump; this repo is not exercising install/generator output in this lane.getStore(false): applicable throughNavigationBarApp; comment route/system specs passed with the rc.7 renderer running.Validation
bin/conductor-exec bundle install: PASS. Resolvedreact_on_railsandreact_on_rails_proto17.0.0.rc.7; Bundler also updated transitiveasync/io-event.bin/conductor-exec yarn install: PASS after correcting a local command typo (--frozen-lockfile=falseis not valid Yarn classic syntax). Yarn warnings are existing peer warnings for React 19-era deps.bin/conductor-exec yarn res:build && bin/conductor-exec bin/rails react_on_rails:locale && bin/conductor-exec bin/rails react_on_rails:generate_packs && bin/conductor-exec bin/shakapacker: PASS.bin/conductor-exec bin/shakapacker: PASS after generated ReScript/locale/pack files existed. Cold direct run failed first because this app requires generatedclient/app/generated/server-bundle-generated.js; that matches the app initializer comments.RENDERER_URL=http://localhost:3801 bin/conductor-exec bundle exec rspec: FAIL,48 examples, 1 failure.spec/system/server_components_demo_spec.rb:35ErrorBoundaryfallback textServer component fetch failedafter clickingSimulate Error.LiveActivitycontent (Refresh count: 1), while the rc.7 node renderer loggedSimulated server-side render failure (demo)for theLiveActivityRSC payload request.origin/master(17.0.0.rc.6+react-on-rails-rsc 19.2.0) passes the same isolated example with its own renderer on port 3802:RENDERER_URL=http://localhost:3802 bin/conductor-exec bundle exec rspec spec/system/server_components_demo_spec.rb:35: PASS,1 example, 0 failures./:http://127.0.0.1:3027withRENDERER_URL=http://localhost:3801.curl http://127.0.0.1:3027/: HTTP 200./; titleReact Webpack Rails Tutorial, H1A cleaner way to explore Rails, React, and server rendering together.,#RouterApp-react-component-0present,#git-commit-shapresent.Known Failures / Issues
RSCRoute+ErrorBoundaryno longer shows the fallback for the simulated server-component error in this demo. rc.6 baseline passes; rc.7 fails. No new React on Rails issue created from this lane per release-gate instructions.Release Impact
Do not count this hard-gate lane as green for React on Rails 17.0.0 final until the rc.7 RSC error-boundary regression is fixed, explained, or explicitly waived.
Summary by CodeRabbit