Test source-mapped TSX prerender errors#4135
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new end-to-end test fixture for source-mapped prerender errors: a TSX probe component that throws on render, an ERB view, a controller action with route, a config change to copy ChangesSource-mapped prerender error probe
Build script i18n preparation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 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. Comment |
|
+ci-status |
|
+ci-run-hosted |
CI StatusHead SHA: Only the required gate is active unless hosted CI is requested. |
github-actions
Bot
added
the
ready-for-hosted-ci
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails_pro/spec/dummy/spec/requests/source_mapped_prerender_error_spec.rb (1)
21-31: ⚡ Quick winAvoid mutating global
Rails.loggerin the example.Reassigning
Rails.loggercan leak across concurrently running specs. Prefer stubbingRails.loggerfor this example to keep isolation deterministic.Proposed refactor
- log_output = StringIO.new - original_logger = Rails.logger - Rails.logger = ActiveSupport::Logger.new(log_output) + log_output = StringIO.new + test_logger = ActiveSupport::Logger.new(log_output) + allow(Rails).to receive(:logger).and_return(test_logger) @@ - ensure - Rails.logger = original_logger end end🤖 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 `@react_on_rails_pro/spec/dummy/spec/requests/source_mapped_prerender_error_spec.rb` around lines 21 - 31, The test is directly mutating the global Rails.logger by reassigning it to a new ActiveSupport::Logger instance, which can cause test isolation issues when specs run concurrently. Instead of reassigning Rails.logger and using an ensure block to restore it, stub Rails.logger using RSpec's stubbing mechanism (with allow or expect) to capture log output while maintaining test isolation. This approach prevents the global state from being modified across concurrent test runs.
🤖 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.
Nitpick comments:
In
`@react_on_rails_pro/spec/dummy/spec/requests/source_mapped_prerender_error_spec.rb`:
- Around line 21-31: The test is directly mutating the global Rails.logger by
reassigning it to a new ActiveSupport::Logger instance, which can cause test
isolation issues when specs run concurrently. Instead of reassigning
Rails.logger and using an ensure block to restore it, stub Rails.logger using
RSpec's stubbing mechanism (with allow or expect) to capture log output while
maintaining test isolation. This approach prevents the global state from being
modified across concurrent test runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ad08fd9-8e1b-49a3-a3e7-1001d89d414a
📒 Files selected for processing (6)
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/app/views/pages/source_mapped_prerender_error_probe.html.erbreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/SourceMappedPrerenderErrorProbe.tsxreact_on_rails_pro/spec/dummy/config/initializers/react_on_rails_pro.rbreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/spec/requests/source_mapped_prerender_error_spec.rb
Greptile SummaryThis PR adds an end-to-end test for source-mapped SSR prerender errors. A minimal TSX probe component (always throws), a Rails route/view/controller action, and a request spec are wired together to verify that when
Confidence Score: 4/5The change adds test infrastructure only — a new probe component, route, view, controller action, and request spec. No production rendering paths are modified. The only runtime-affecting change is the assets_to_copy initializer, which adds the source map file in the test environment, leaving other environments unchanged. The test globally replaces Rails.logger mid-process rather than using a scoped capture, which works under serial test execution but would produce intermittent failures if the suite ever runs concurrently. The 'use client' directive on a component expected to execute during SSR prerender also warrants a clarifying comment, given that RSC support is enabled in the dummy app. Neither concern affects the current verified behavior, but both could silently break under future changes to concurrency settings or renderer RSC handling. The request spec (source_mapped_prerender_error_spec.rb) and the probe component (SourceMappedPrerenderErrorProbe.tsx) are the two files worth a second look before merging. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Spec as RSpec
participant Rails as Rails (test)
participant Controller as PagesController
participant View as ERB view
participant Helper as react_component helper
participant Renderer as Node Renderer
participant AppCtrl as ApplicationController
Spec->>Rails: swap Rails.logger to StringIO
Spec->>Rails: GET /source_mapped_prerender_error_probe
Rails->>Controller: source_mapped_prerender_error_probe action
Controller->>View: render template
View->>Helper: react_component with prerender true and raise_on_prerender_error true
Helper->>Renderer: SSR request with server-bundle.js and .js.map
Renderer-->>Helper: Error at SourceMappedPrerenderErrorProbe.tsx LINE COL
Helper->>Helper: raise ReactOnRails::PrerenderError
Helper-->>AppCtrl: PrerenderError bubbles up
AppCtrl->>AppCtrl: rescue_from calls log_prerender_error
AppCtrl->>Rails: Rails.logger.error captures TSX frame
AppCtrl->>Rails: redirect_to server_side_log_throw_raise_invoker_path
Rails-->>Spec: 302 redirect
Spec->>Spec: assert redirect and TSX frame in log
Spec->>Rails: ensure restore original Rails.logger
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Spec as RSpec
participant Rails as Rails (test)
participant Controller as PagesController
participant View as ERB view
participant Helper as react_component helper
participant Renderer as Node Renderer
participant AppCtrl as ApplicationController
Spec->>Rails: swap Rails.logger to StringIO
Spec->>Rails: GET /source_mapped_prerender_error_probe
Rails->>Controller: source_mapped_prerender_error_probe action
Controller->>View: render template
View->>Helper: react_component with prerender true and raise_on_prerender_error true
Helper->>Renderer: SSR request with server-bundle.js and .js.map
Renderer-->>Helper: Error at SourceMappedPrerenderErrorProbe.tsx LINE COL
Helper->>Helper: raise ReactOnRails::PrerenderError
Helper-->>AppCtrl: PrerenderError bubbles up
AppCtrl->>AppCtrl: rescue_from calls log_prerender_error
AppCtrl->>Rails: Rails.logger.error captures TSX frame
AppCtrl->>Rails: redirect_to server_side_log_throw_raise_invoker_path
Rails-->>Spec: 302 redirect
Spec->>Spec: assert redirect and TSX frame in log
Spec->>Rails: ensure restore original Rails.logger
Reviews (1): Last reviewed commit: "Test source-mapped TSX prerender errors" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Code Review Summary The approach is sound and well-scoped: all changes are confined to the Pro dummy app test harness with no production SSR or error-handling paths touched. Four issues are flagged via inline comments; two are worth resolving before merge. Issues requiring attention before merge 'use client' directive on the probe component (inline comment on SourceMappedPrerenderErrorProbe.tsx:18) — In an RSC-aware renderer, 'use client' marks the component as a client boundary and the node renderer may skip it during SSR entirely. If so, no error is thrown, no log is written, and the spec fails in a confusing place (redirect assertion rather than the regex). No other deliberate-error fixture in this repo carries this directive. Please verify it can be removed, or document why it is necessary despite the component's SSR-only purpose. Incorrect functional component signature (inline comment on SourceMappedPrerenderErrorProbe.tsx:19) — The (props, context) two-argument form is the class component API. A functional component's second argument is the forwarded ref (React.forwardRef only). Harmless since the component always throws, but misleading. Lower-priority improvements Logger replacement pattern (inline comment on source_mapped_prerender_error_spec.rb:21-22) — The bare ActiveSupport::Logger replacement drops the original's broadcast chain (relevant on Rails 7.1+). An around hook is more idiomatic and composes better with other hooks. Local variable inside conditional expression (inline comment on react_on_rails_pro.rb:66-68) — Assigning assets = [...] and reading it as the implicit return of the if branch is unusual Ruby style. Renaming to paths and keeping the same logic is slightly clearer. Positives
|
|
Review follow-up pushed in Addressed:
Validation after the push:
Hosted checks are rerunning on the new head. |
Code Review — PR #4135: Test source-mapped TSX prerender errorsOverview: Adds regression coverage verifying that when the Node renderer raises What works well
Issues1. Missing file-existence guard for
|
Code ReviewOverviewThis PR adds regression coverage for source-mapped TSX prerender errors in the Pro dummy app. The feature verifies that when SSR raises Strengths
Issues and ObservationsMedium
The new
Previously the value was a single MinorSpec doesn't assert the handler's own log line The test verifies the error message and the source-mapped frame in the log, but skips asserting Hardcoded
SummaryThe PR is well-scoped and correct. The two medium items are worth discussing before merge (particularly the |
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 `@react_on_rails_pro/spec/dummy/tests/package-scripts.test.js`:
- Around line 38-39: The test assertion at line 38-39 only verifies that the
string 'pnpm run prepare:i18n' is present in the script using toContain(), but
does not verify its ordering relative to 'bin/shakapacker-precompile-hook'.
Replace the toContain assertion with one that explicitly checks the index
positions of both strings to ensure 'pnpm run prepare:i18n' appears before
'bin/shakapacker-precompile-hook' in the script, confirming the required
execution order for the prepare-before-precompile contract.
🪄 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: 22900719-9a00-4052-9763-31b4e6c6b1db
📒 Files selected for processing (2)
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/tests/package-scripts.test.js
|
CI failure follow-up pushed in Root cause:
Fix:
Validation on the pushed head:
Hosted CI and review bots are rerunning on |
|
Review follow-up for the current CodeRabbit package-script ordering request:
Head pushed: |
Code ReviewPR summary: Adds end-to-end coverage for source-mapped TSX prerender errors in the Pro dummy app — a new Overall the approach is clean and well-scoped (all changes stay in the dummy app / test fixtures). A few things to look at: Potential issues1. Missing map file → confusing renderer failure (initializer, line 67) 2. Style / minor notes3. Intermediate variable inside 4. New logger has no formatter (spec, line 22) 5. What's good
|
|
Second review follow-up pushed: Review fixes included:
Local validation:
Note: stopping the local node renderer after the passing specs emitted a SIGINT shutdown |
Code ReviewGood, focused PR. The changes are tightly scoped to the Pro dummy app's test infrastructure and close a real coverage gap: source-mapped TSX frames in the Rails prerender error path. What works well
Observations1. No file-existence guard on
2. Logger stub and cached logger instances
3. Adding 4. Regex pattern in spec (non-blocking)
SummaryThe core implementation is correct and the test strategy is sound. The |
|
Confidence note:
Current blocker: external Cursor Bugbot pending; no failing checks or unresolved review threads observed. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_539b240c-ffde-469b-9413-7bc9a2fb4008) |
|
Readiness update after Cursor Bugbot completion:
Current blocker: needs explicit maintainer approval/waiver for the ledger |
Manual verification: confirmed end-to-end against a live renderer ✅This PR (Closes #4112) is test-only coverage (no shipped product code; the merge touches only ReproductionResults
This is the real end-to-end confirmation of #3940 that I'd previously only checked at the mechanism level: a built bundle → live node renderer → SSR CaveatFull E2E (real |
1 participant
Summary
server-bundle.js.mapwith test node-renderer uploads.PrerenderErrorraised during SSR..tsxsource-mapped frame.Closes #4112
Verification
pnpm run build:testfromreact_on_rails_pro/spec/dummyRENDERER_PORT=3812 pnpm run node-renderer:freshREACT_RENDERER_URL=http://localhost:3812 bundle exec rspec spec/requests/source_mapped_prerender_error_spec.rb --format documentationREACT_RENDERER_URL=http://localhost:3812 bundle exec rspec spec/requests/console_logging_spec.rb --format documentationpnpm run type-checkpnpm --filter react-on-rails-pro-node-renderer run buildpnpm run lintpnpm exec prettier --check -- react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/SourceMappedPrerenderErrorProbe.tsxBUNDLE_GEMFILE=../Gemfile bundle exec rubocop --ignore-parent-exclusion spec/dummy/config/initializers/react_on_rails_pro.rb spec/dummy/config/routes.rb spec/dummy/app/controllers/pages_controller.rb spec/dummy/spec/requests/source_mapped_prerender_error_spec.rbgit diff --cached --checkscript/ci-changes-detector origin/mainNotes
EPIPE; specs completed before shutdown.UNKNOWN, not passed.Note
Low Risk
Changes are limited to the Pro dummy app’s tests, routes, and build scripts; production initializer behavior is unchanged outside test env source-map copying.
Overview
Adds dummy-app coverage so request specs can prove Rails logs source-mapped
.tsxframes when aReactOnRails::PrerenderErroris raised during classicreact_componentSSR (not the RSC path).A new route, view, and
SourceMappedPrerenderErrorProbeclient component ('use client', intentional throw) exerciseprerender: truewithraise_on_prerender_errorandtrace. The probe is wired into the existing error-hubwith_config_overrideslist. A request spec stubs the logger and expects the handler message, probe error text, and aSourceMappedPrerenderErrorProbe.tsx:line:colframe, plus the usual redirect to the invoker path.Test node-renderer staging now includes
ssr-generated/server-bundle.js.mapinassets_to_copywhenRails.env.test?, so the remote renderer can resolve maps during those specs.Build hygiene: dummy
package.jsongainsprepare:i18n(mkdir -p client/app/i18n/generated) ahead ofshakapacker-precompile-hookon all main build scripts; Jest asserts that ordering.Reviewed by Cursor Bugbot for commit 4abdf24. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores