Add error scenarios hub and test pages#2497
Add error scenarios hub and test pages#2497AbanoubGhadban wants to merge 14 commits into2402-streaming-hang-on-errorfrom
Conversation
…2402) Node.js stream.pipe() does not propagate errors from source to destination. When a source stream errors, pipe() silently unpipes but leaves the destination PassThrough open, causing any consumer awaiting stream.end (like Fastify's res.send) to hang forever. Fix at 5 locations across the streaming pipeline: - handleStreamError: end PassThrough when source errors - RSCRequestTracker: end tee'd streams when source errors - injectRSCPayload: flush + end resultStream in all error paths - pipeToTransform: end transformStream when piped stream errors - consumer_stream_async: resolve first_chunk_var on exception Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Only end destination streams when the source is truly destroyed (stream.destroyed === true), not on non-fatal errors. When throwJsErrors is true, emitError() emits an error event without destroying the stream — React continues rendering other suspense boundaries. Ending the destination on any error caused "write after end" as pipe() tried to forward subsequent data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Async component that always errors after 2s during streaming SSR. Visit /stream_error_demo to reproduce: - On master: browser hangs forever (loading spinner) - On fix branch: response completes with error handled Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Throws immediately at the top level of the component, before React can render any shell HTML. Visit /stream_shell_error_demo to reproduce. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mination 'error' alone is not the end of a stream — non-fatal errors (e.g., emitError for throwJsErrors) emit 'error' without destroying, and React may continue rendering. The 'close' event fires after both normal 'end' and destroy(), making it the reliable termination signal. Changes: - handleStreamError: split error handler (report only) from close handler (end PassThrough) - RSCRequestTracker: use 'close' instead of 'error' to end tee'd streams - injectRSCPayload: add 'close' listener on pipeableHtmlStream to end htmlStream when source is destroyed (closes gap where pipe unpipes silently), use 'close' on htmlStream for resultStream termination - streamingUtils: use 'close' instead of 'error' + destroyed check - Add test verifying non-fatal errors don't prematurely end the PassThrough Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move endResultStream after flush definition to fix ESLint no-use-before-define - Remove redundant begin block in async do...end to fix RuboCop Style/RedundantBegin Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The 'close' handler on htmlStream was firing in the normal path (via
autoDestroy after 'end') and prematurely ending resultStream before RSC
data could be flushed. Node.js runs process.nextTick ('close' from
autoDestroy) before Promise microtasks (.then from the 'end' handler),
so endResultStream() always ran before rscPromise.then(cleanup).
Fix: check htmlStream.readableEnded in the 'close' handler — it's true
after normal 'end' (let the 'end' handler manage cleanup) and false
when destroyed without ending (the error case we actually want to
handle).
Also adds tests for RSC payloads arriving after the HTML stream
finishes, which were previously untested and would hang indefinitely
without this fix.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove issue references and reproduction language from test files. Tests now describe the expected stream error handling behavior directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix prettier formatting in test files - Fix ESLint errors in demo components (unreachable code, line length) - Remove issue references from demo component text - Guard cleanup() against double-ending resultStream by delegating to endResultStream() which has writableEnded check - Use bare `raise` instead of `raise e` to preserve original backtrace Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The `=> e` capture is unused after changing `raise e` to bare `raise`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request adds comprehensive error scenario testing infrastructure to the React on Rails Pro dummy application, including a new error scenarios hub view with configuration overrides, multiple test action endpoints for SSR and streaming error scenarios, new React test components for deterministic error demonstrations, and corresponding routes and UI navigation elements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Review: PR #2497 — Add error scenarios hub and test pagesOverall this is a well-documented and useful addition for manual testing of error paths. The hub page is genuinely informative with its config explanations and history. A few issues worth addressing before merge: Critical: apply_config_overrides runs on ALL controller actionsThe before_action :apply_config_overrides is placed at the top of PagesController with no only: or except: guard, meaning any request to any page (hello world, shared store, server router, etc.) can mutate the global ReactOnRails.configuration and ReactOnRailsPro.configuration singletons by appending query params. This is likely unintentional. Suggested fix — scope it to error-related actions only: before_action :apply_config_overrides, only: %i[
error_scenarios_hub
ssr_shell_error ssr_async_error ssr_sync_error ssr_async_prop_error
rsc_component_error non_existing_react_component
non_existing_stream_react_component non_existing_rsc_payload
stream_error_demo stream_shell_error_demo
]Critical: Global config mutation is not thread-safeapply_config_overrides writes to global ReactOnRails.configuration and ReactOnRailsPro.configuration singletons, which are shared across all threads in a multi-threaded Puma server. If two test requests execute concurrently (e.g., in parallel test suites), one request's config change can affect the other's rendering — causing flaky test results that are hard to diagnose. Consider adding a comment at minimum, or using a request-scoped config approach (e.g., thread-local storage or per-request options if supported). Bug: reset_error_configs uses wrong default for throw_js_errorsThe hub page's own documentation says the default for throw_js_errors is true (changed in Jun 2023, Pro PR #328), but reset_error_configs resets it to false: ReactOnRailsPro.configuration.throw_js_errors = false # should be true per documented defaultThis means 'Reset to defaults' doesn't actually restore the real defaults. Minor: Non-standard async component pattern in StreamErrorDemoAlwaysFailsAsync is a regular (non-async) function that returns a Promise on the server side: const AlwaysFailsAsync = () => {
if (typeof window !== 'undefined') {
return <div>Client-side fallback</div>;
}
return new Promise((_resolve, reject) => { ... });
};React 19 async server components are expected to be declared with async function. A non-async function returning a raw Promise is technically undefined behavior in React's reconciler. Using async function AlwaysFailsAsync() would be more correct and less likely to break across React versions. Minor: Config mutations via GET requestapply_config_overrides mutates global state on GET requests (via URL query params and the onchange auto-submit). This bypasses CSRF protection since the form uses method: :get. Since this is a dev/test-only dummy app this is acceptable, but worth a comment explaining the choice is intentional. Nit: 1041-line ERB fileerror_scenarios_hub.html.erb is 1041 lines. Consider extracting each config card into a partial for easier navigation and future maintenance. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| session[:something_useful] = "REALLY USEFUL" | ||
| end | ||
|
|
||
| before_action :apply_config_overrides |
There was a problem hiding this comment.
The apply_config_overrides before_action has no only: or except: scope, so it runs on every controller action. Any page in this app (hello world, server router, shared store, etc.) can have its global configuration mutated by a visitor who appends ?raise_on_prerender_error=false to any URL.
| before_action :apply_config_overrides | |
| before_action :apply_config_overrides, only: %i[ | |
| error_scenarios_hub | |
| ssr_shell_error ssr_async_error ssr_sync_error ssr_async_prop_error | |
| rsc_component_error non_existing_react_component | |
| non_existing_stream_react_component non_existing_rsc_payload | |
| stream_error_demo stream_shell_error_demo | |
| ] |
| def reset_error_configs | ||
| ReactOnRails.configuration.raise_on_prerender_error = Rails.env.development? | ||
| ReactOnRailsPro.configuration.throw_js_errors = false | ||
| ReactOnRailsPro.configuration.raise_non_shell_server_rendering_errors = false |
There was a problem hiding this comment.
This resets throw_js_errors to false, but the hub page's own documentation states the default is true (changed in Jun 2023, Pro PR #328). "Reset to defaults" will leave users in a non-default state.
| ReactOnRailsPro.configuration.raise_non_shell_server_rendering_errors = false | |
| ReactOnRailsPro.configuration.throw_js_errors = true |
| ReactOnRails.configuration.raise_on_prerender_error = | ||
| bool.cast(params[:raise_on_prerender_error]) | ||
| end | ||
| if params[:throw_js_errors].present? |
There was a problem hiding this comment.
Thread-safety concern: This method mutates global configuration singletons (ReactOnRails.configuration, ReactOnRailsPro.configuration) which are shared across all threads in a multi-threaded Puma server. If two requests are processed concurrently — which can happen in parallel test suites — one request's config override can bleed into another's render, producing flaky, hard-to-diagnose failures.
Consider adding a comment here to document this limitation, e.g.:
# NOTE: This mutates global config singletons and is NOT thread-safe.
# Intended only for single-threaded manual testing in the dummy app.
# Running tests with concurrency enabled may cause flaky results.
| if (typeof window !== 'undefined') { | ||
| return <div>Client-side fallback</div>; | ||
| } | ||
| return new Promise((_resolve, reject) => { |
There was a problem hiding this comment.
A regular (non-async) function returning a Promise on the server side is not the standard pattern for async React components in React 19. React's reconciler expects async server components to be declared as async function. A non-async function returning a raw Promise may behave inconsistently across React versions.
Consider:
const AlwaysFailsAsync = async () => {
if (typeof window !== 'undefined') {
return <div>Client-side fallback</div>;
}
await new Promise((_resolve, reject) => {
setTimeout(() => reject(new Error('Async component crashed during server rendering!')), 2000);
});
};![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
react_on_rails_pro/spec/dummy/app/views/pages/error_scenarios_hub.html.erb (1)
56-60: Minor text inconsistency with auto-submit behavior.The instruction says "Change values and click Apply" but the form auto-submits on change (via
onchange="this.form.submit()"). The Apply button only appears in the<noscript>fallback. Consider updating the text to reflect the auto-submit behavior.📝 Suggested text update
<p style="margin-top: 0; font-size: 0.9em;"> - Change values and click <strong>Apply</strong> to mutate the global config singletons. + Change values to automatically apply and mutate the global config singletons. Linked "Open page" test pages will use the overridden values. </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/dummy/app/views/pages/error_scenarios_hub.html.erb` around lines 56 - 60, Update the explanatory text to reflect the form's auto-submit behavior: replace "Change values and click Apply to mutate the global config singletons." with wording that mentions changes are submitted automatically on change (because of onchange="this.form.submit()") and that the Apply button only appears as a <noscript> fallback for users without JavaScript; adjust the copy around the "Open page" reference if needed so it clearly communicates that overridden values are applied immediately in JS-enabled browsers and the Apply button is only for non-JS fallback.react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (1)
22-22: Consider scopingapply_config_overridesto error scenario actions only.The
before_actionapplies to all controller actions, meaning every request will check params and potentially modify global configuration. While this is likely intentional to allow config overrides to persist when navigating from the hub to test pages, you might consider limiting it to relevant actions usingonly:to avoid unintended config changes on unrelated pages.If the current behavior is intentional (to allow any page to accept config override params), this is fine for a test app.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb` at line 22, PagesController currently runs before_action :apply_config_overrides for every action which can unintentionally mutate global config; restrict this to only the actions that render the hub/test pages by changing the before_action to scope apply_config_overrides (use the only: option) and list the relevant action names that accept override params (e.g., the hub and test page actions handled by PagesController such as the actions that render test pages or show pages); update the before_action line referencing apply_config_overrides in PagesController accordingly.
🤖 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/app/controllers/pages_controller.rb`:
- Line 22: PagesController currently runs before_action :apply_config_overrides
for every action which can unintentionally mutate global config; restrict this
to only the actions that render the hub/test pages by changing the before_action
to scope apply_config_overrides (use the only: option) and list the relevant
action names that accept override params (e.g., the hub and test page actions
handled by PagesController such as the actions that render test pages or show
pages); update the before_action line referencing apply_config_overrides in
PagesController accordingly.
In `@react_on_rails_pro/spec/dummy/app/views/pages/error_scenarios_hub.html.erb`:
- Around line 56-60: Update the explanatory text to reflect the form's
auto-submit behavior: replace "Change values and click Apply to mutate the
global config singletons." with wording that mentions changes are submitted
automatically on change (because of onchange="this.form.submit()") and that the
Apply button only appears as a <noscript> fallback for users without JavaScript;
adjust the copy around the "Open page" reference if needed so it clearly
communicates that overridden values are applied immediately in JS-enabled
browsers and the Apply button is only for non-JS fallback.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/app/views/pages/error_scenarios_hub.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/non_existing_react_component.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/non_existing_rsc_payload.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/non_existing_stream_react_component.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/rsc_component_error.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/ssr_async_error.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/ssr_async_prop_error.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/ssr_shell_error.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/ssr_sync_error.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/stream_error_demo.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/stream_shell_error_demo.html.erbreact_on_rails_pro/spec/dummy/app/views/shared/_menu.erbreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/DeterministicRSCErrorComponent.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/StreamErrorDemo.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/StreamShellErrorDemo.jsxreact_on_rails_pro/spec/dummy/config/routes.rb
Greptile SummaryThis PR adds comprehensive error testing infrastructure to the dummy app:
Issue found:
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: fdda266 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return new Promise((_resolve, reject) => { | ||
| setTimeout(() => reject(new Error('Async component crashed during server rendering!')), 2000); | ||
| }); |
There was a problem hiding this comment.
Returning a Promise isn't valid React - components must return JSX or primitives. To trigger Suspense error handling, throw the Promise instead:
| return new Promise((_resolve, reject) => { | |
| setTimeout(() => reject(new Error('Async component crashed during server rendering!')), 2000); | |
| }); | |
| throw new Promise((_resolve, reject) => { | |
| setTimeout(() => reject(new Error('Async component crashed during server rendering!')), 2000); | |
| }); |
![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: fdda266fbc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <span class="<%= throw_js_errors ? 'config-val-true' : 'config-val-false' %>"><%= throw_js_errors %></span> | ||
| </h4> | ||
| <p><strong>Gem:</strong> <code>react_on_rails_pro</code> | | ||
| <strong>Default:</strong> <code>true</code> | |
There was a problem hiding this comment.
The hub hard-codes throw_js_errors as Default: true, but this dummy app config initializes that setting to false (spec/dummy/config/initializers/react_on_rails_pro.rb) and reset_error_configs also resets it to false, so clicking “Reset to defaults” leaves the page contradicting its own default documentation. This makes the scenario matrix misleading and can send debugging toward the wrong expected behavior.
Useful? React with 👍 / 👎.
AbanoubGhadban
changed the base branch from
master
to
2402-streaming-hang-on-error
Add a comprehensive error scenarios hub page with: - Interactive config override controls for raise_on_prerender_error, throw_js_errors, and raise_non_shell_server_rendering_errors - Auto-submit dropdowns with config param forwarding to linked pages - Detailed documentation of 10 error scenarios (A-I + N) with config-aware behavior descriptions - Summary table showing expected behavior under each config combination Add test pages for each error scenario: - SSR shell error, async error, sync error, async prop error - RSC component error (via server router) - Stream error demo and stream shell error demo - Non-existing component pages (non-streaming, streaming, RSC) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AbanoubGhadban
force-pushed
the
abanoub/2450-error-scenarios-hub
branch
from
fdda266 to
c09d44a
Compare
| session[:something_useful] = "REALLY USEFUL" | ||
| end | ||
|
|
||
| before_action :apply_config_overrides |
There was a problem hiding this comment.
Consider scoping this before_action to error scenario actions only.
apply_config_overrides mutates global ReactOnRails.configuration and ReactOnRailsPro.configuration singletons. With no only: guard, every request to every page in the dummy app (hello-world, shared store, server router, etc.) will check URL params and potentially overwrite these globals.
This means a leftover ?throw_js_errors=true param in a copy-pasted URL could silently affect unrelated test pages without the developer realising why.
| before_action :apply_config_overrides | |
| before_action :apply_config_overrides, only: %i[ | |
| error_scenarios_hub reset_error_configs | |
| ssr_shell_error ssr_async_error ssr_sync_error ssr_async_prop_error | |
| rsc_component_error non_existing_react_component | |
| non_existing_stream_react_component non_existing_rsc_payload | |
| stream_error_demo stream_shell_error_demo | |
| ] |
| <div style="padding: 1em; border: 2px solid #4caf50; border-radius: 6px; background: #e8f5e9; margin-bottom: 1.5em;"> | ||
| <h3 style="margin-top: 0;">Override Configs (runtime)</h3> | ||
| <p style="margin-top: 0; font-size: 0.9em;"> | ||
| Change values and click <strong>Apply</strong> to mutate the global config singletons. |
There was a problem hiding this comment.
The text says "click Apply" but the form auto-submits via onchange="this.form.submit()" — the Apply button only appears inside <noscript> for non-JS fallback. The instruction is misleading for JS-enabled browsers.
| Change values and click <strong>Apply</strong> to mutate the global config singletons. | |
| Change values to automatically apply and mutate the global config singletons. |
AbanoubGhadban
force-pushed
the
2402-streaming-hang-on-error
branch
from
b6204e6 to
2185635
Compare
2 participants
Summary
/error_scenarios_hub) with interactive config override controls forraise_on_prerender_error,throw_js_errors, andraise_non_shell_server_rendering_errorsContext
Split from #2486 (Fix silent error absorption in RSC payload injection) to separate the dummy app test/documentation pages from the core package/gem bug fixes.
Test plan
/error_scenarios_huband verify the 3 config dropdowns show current values🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation