feat: Add smart error messages with actionable solutions #1834
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis PR significantly expands React on Rails with improved error handling (SmartError, PrerenderError), comprehensive JavaScript client-side infrastructure (component/store registries, rendering lifecycle, client startup), React Server Components (RSC) streaming support, and extensive TypeScript type definitions. It adds debug logging, authentication utilities, and multi-environment entry points (client, full, node, RSC bundles). Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser<br/>(Client)
participant Rails as Rails<br/>(Server)
participant RSC as RSC<br/>(Server)
Browser->>Rails: Initial Request
Rails->>RSC: generateRSCPayload(componentName, props)
RSC-->>Rails: ReadableStream (pipeable)
Rails->>Rails: renderToPipeableStream(element)
Rails->>Rails: injectRSCPayload(htmlStream, rscTracker)
par Buffered Multi-Source Flush
Rails->>Browser: RSC Init Scripts
Rails->>Browser: HTML Chunks
Rails->>Browser: RSC Payload Scripts
end
Browser->>Browser: hydrate/render (with RSC data)
Browser->>Browser: Replay Console Logs
sequenceDiagram
participant App as React App<br/>(Component)
participant Provider as RSCProvider
participant Router as RSCRoute
participant Fetcher as getReactServerComponent
participant Server as Rails/RSC
App->>Provider: wrap with RSCProvider
Provider->>Router: RSCRoute(componentName, props)
Router->>Fetcher: getComponent(componentName, props)
alt Preloaded Payload
Fetcher->>Fetcher: Check window.REACT_ON_RAILS_RSC_PAYLOADS
Fetcher-->>Router: React.use(promise from cache)
else HTTP Fetch
Fetcher->>Server: POST rscPayloadGenerationUrl
Server-->>Fetcher: ReadableStream (RSC)
Fetcher->>Fetcher: createFromReadableStream + consoleReplay
Fetcher-->>Router: React.use(promise from stream)
end
Router->>App: Rendered Component
sequenceDiagram
participant Page as Page Load
participant Setup as clientStartup()
participant Registry as ComponentRegistry
participant Renderer as ClientSideRenderer
participant DOM as DOM Elements
Page->>Setup: Initialize
Setup->>Setup: Detect Turbo/Turbolinks
Setup->>Renderer: renderOrHydrateImmediateHydratedComponents()
loop For each [data-react-class]
Renderer->>Registry: getComponent(name)
Renderer->>DOM: Read data-props, data-hydrate-from-store
par Load Dependencies
Renderer->>Renderer: hydrateStores(dependencies)
end
Renderer->>Renderer: hydrate() or render()
end
Setup->>Setup: Attach Turbo/Turbolinks listeners
Page->>Page: onPageUnloaded → unmountAll()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Specific areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
PR Review: Smart Error Messages with Actionable SolutionsThank you for this excellent PR! The addition of smart error messages is a significant improvement to the React on Rails developer experience. Here's my comprehensive review: 🎯 Overall AssessmentRating: 9/10 - This is a high-quality implementation that will greatly improve developer experience with minimal breaking changes. ✅ Strengths1. Excellent Developer Experience Focus
2. Well-Structured Implementation
3. Comprehensive Test Coverage
4. Backwards Compatibility
🔍 Code Quality ObservationsPositive Aspects:
Minor Suggestions for Improvement:
🐛 Potential Issues1. Ruby Version CompatibilityThe code uses some Ruby 3.0+ syntax patterns. Ensure compatibility with the minimum Ruby version specified in the gemspec. 2. Missing Dependency CheckThe Rainbow gem is used but should be verified as a dependency in the gemspec file. 3. Edge Case HandlingConsider handling the case where available_components is nil or empty in the fuzzy matching logic. 📝 Documentation QualityExcellent:
Could Be Enhanced:
🔒 Security Considerations✅ No security concerns identified. Error messages appropriately avoid exposing sensitive information. 🚀 Performance Impact
💡 Suggestions for Future Enhancements
✨ Highlights
📋 Checklist
🎉 ConclusionThis PR represents a significant improvement to React on Rails and successfully achieves its goals of providing better error messages with actionable solutions. The implementation is thoughtful, well-tested, and maintains backward compatibility while pushing users toward better practices (auto-bundling). Recommendation: APPROVE ✅ The PR is ready to merge after addressing the minor suggestions above. Great work on this improvement - it will make React on Rails much more developer-friendly! Review generated with assistance from Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/guides/improved-error-messages.md (1)
44-53: Add language hint to the code fencemarkdownlint (MD040) is flagging these undecorated fences. Annotate them with a language (e.g.,
```text,```bash,```erb) so the linter passes and readers get proper syntax highlighting. Apply the same treatment to the other bare fences in this guide.lib/react_on_rails/prerender_error.rb (1)
50-144: Consider truncating huge console sectionsThe new troubleshooting blocks are great, but they pull full
console_messagesinto the Ruby exception. For noisy SSR logs this can explode size and overwhelm logs/honeybadger. Maybe trim withUtils.smart_trimor cap at a few KB (similar to props/js code). Worth a follow-up so the message stays readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
IMPROVEMENT_SUMMARY.md(1 hunks)REACT_ON_RAILS_IMPROVEMENTS.md(1 hunks)demo_improved_errors.rb(1 hunks)docs/guides/improved-error-messages.md(1 hunks)lib/react_on_rails/helper.rb(2 hunks)lib/react_on_rails/prerender_error.rb(2 hunks)lib/react_on_rails/smart_error.rb(1 hunks)node_package/src/ReactOnRails.client.ts(2 hunks)node_package/src/types/index.ts(1 hunks)node_package/tests/debugLogging.test.js(1 hunks)spec/react_on_rails/smart_error_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
node_package/src/types/index.tsnode_package/src/ReactOnRails.client.tsREACT_ON_RAILS_IMPROVEMENTS.mdnode_package/tests/debugLogging.test.jsspec/react_on_rails/smart_error_spec.rbdemo_improved_errors.rblib/react_on_rails/helper.rbdocs/guides/improved-error-messages.mdlib/react_on_rails/prerender_error.rblib/react_on_rails/smart_error.rbIMPROVEMENT_SUMMARY.md
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JavaScript/TypeScript and fix all linter offenses before committing
Files:
node_package/src/types/index.tsnode_package/src/ReactOnRails.client.tsnode_package/tests/debugLogging.test.js
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
node_package/src/types/index.tsnode_package/src/ReactOnRails.client.tsREACT_ON_RAILS_IMPROVEMENTS.mdnode_package/tests/debugLogging.test.jsdocs/guides/improved-error-messages.mdIMPROVEMENT_SUMMARY.md
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}: Before every commit/push, runbundle exec rubocopand fix all violations in Ruby code
Let RuboCop handle all Ruby code formatting; never manually format Ruby files
Files:
spec/react_on_rails/smart_error_spec.rbdemo_improved_errors.rblib/react_on_rails/helper.rblib/react_on_rails/prerender_error.rblib/react_on_rails/smart_error.rb
🧠 Learnings (7)
📓 Common learnings
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
node_package/src/types/index.tsnode_package/src/ReactOnRails.client.tsnode_package/tests/debugLogging.test.jsspec/react_on_rails/smart_error_spec.rbdemo_improved_errors.rblib/react_on_rails/helper.rbdocs/guides/improved-error-messages.mdlib/react_on_rails/prerender_error.rblib/react_on_rails/smart_error.rbIMPROVEMENT_SUMMARY.md
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
node_package/src/types/index.tsnode_package/src/ReactOnRails.client.ts
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
node_package/src/types/index.tslib/react_on_rails/helper.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
REACT_ON_RAILS_IMPROVEMENTS.mdlib/react_on_rails/helper.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
lib/react_on_rails/helper.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
lib/react_on_rails/helper.rb
🧬 Code graph analysis (5)
node_package/src/ReactOnRails.client.ts (2)
node_package/src/pro/ComponentRegistry.ts (1)
components(61-61)node_package/src/types/index.ts (1)
ReactComponentOrRenderFunction(178-178)
node_package/tests/debugLogging.test.js (1)
node_package/src/types/index.ts (1)
ReactOnRails(281-349)
spec/react_on_rails/smart_error_spec.rb (1)
lib/react_on_rails/smart_error.rb (1)
solution(36-53)
lib/react_on_rails/prerender_error.rb (1)
lib/react_on_rails/utils.rb (2)
smart_trim(277-291)default_troubleshooting_section(315-323)
lib/react_on_rails/smart_error.rb (3)
lib/react_on_rails/prerender_error.rb (2)
attr_reader(7-145)initialize(14-25)lib/react_on_rails/react_component/render_options.rb (1)
auto_load_bundle(78-80)lib/react_on_rails/utils.rb (1)
default_troubleshooting_section(315-323)
🪛 ESLint
node_package/src/ReactOnRails.client.ts
[error] 42-42: Replace ``[ReactOnRails]·Registering·${componentNames.length}·component(s):·${componentNames.join(',·')}); with `⏎········`[ReactOnRails]·Registering·${componentNames.length}·component(s):·${componentNames.join(',·')}`,`
(prettier/prettier)
[error] 43-43: Insert );⏎
(prettier/prettier)
[error] 45-45: Delete ······
(prettier/prettier)
[error] 48-48: Delete ······
(prettier/prettier)
[error] 51-51: Replace name with (name)
(prettier/prettier)
node_package/tests/debugLogging.test.js
[error] 4-4: 'ReactOnRails' is assigned a value but never used.
(no-unused-vars)
[error] 11-11: Delete ····
(prettier/prettier)
[error] 44-44: Unexpected require().
(global-require)
[error] 55-55: Delete ······
(prettier/prettier)
[error] 56-56: 'globalThis' is not defined.
(no-undef)
[error] 57-57: Delete ······
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 65-65: 'globalThis' is not defined.
(no-undef)
[error] 66-66: 'globalThis' is not defined.
(no-undef)
[error] 67-67: Delete ······
(prettier/prettier)
[error] 69-69: Replace log with (log)
(prettier/prettier)
[error] 70-70: Replace log with (log)
(prettier/prettier)
[error] 71-71: Replace (log with ((log)
(prettier/prettier)
[error] 72-72: Replace log with (log)
(prettier/prettier)
[error] 77-77: Delete ······
(prettier/prettier)
[error] 78-78: 'globalThis' is not defined.
(no-undef)
[error] 79-79: 'globalThis' is not defined.
(no-undef)
[error] 80-80: Delete ······
(prettier/prettier)
[error] 82-82: Replace log with (log)
(prettier/prettier)
[error] 83-83: Replace log with (log)
(prettier/prettier)
[error] 90-90: Delete ······
(prettier/prettier)
[error] 91-91: 'globalThis' is not defined.
(no-undef)
[error] 92-92: 'globalThis' is not defined.
(no-undef)
[error] 93-93: Delete ······
(prettier/prettier)
[error] 94-94: Replace log with (log)
(prettier/prettier)
[error] 103-103: 'globalThis' is not defined.
(no-undef)
[error] 105-105: Delete ······
(prettier/prettier)
[error] 106-106: 'globalThis' is not defined.
(no-undef)
[error] 111-111: 'globalThis' is not defined.
(no-undef)
[error] 113-113: Delete ······
(prettier/prettier)
[error] 114-114: 'globalThis' is not defined.
(no-undef)
[error] 118-118: 'globalThis' is not defined.
(no-undef)
[error] 118-118: Delete ·
(prettier/prettier)
[error] 120-120: Replace · with ,
(prettier/prettier)
[error] 122-122: Delete ······
(prettier/prettier)
[error] 123-123: 'globalThis' is not defined.
(no-undef)
[error] 124-124: 'globalThis' is not defined.
(no-undef)
[error] 128-128: 'globalThis' is not defined.
(no-undef)
[error] 130-130: Delete ······
(prettier/prettier)
[error] 132-132: 'globalThis' is not defined.
(no-undef)
[error] 136-136: Insert ⏎
(prettier/prettier)
🪛 GitHub Actions: JS unit tests for Renderer package
node_package/src/ReactOnRails.client.ts
[error] 239-239: TypeError: ClientStartup.clientStartup is not a function.
node_package/tests/debugLogging.test.js
[error] 44-44: Test failed in debugLogging.test.js: TypeError: ClientStartup.clientStartup is not a function.
🪛 markdownlint-cli2 (0.18.1)
REACT_ON_RAILS_IMPROVEMENTS.md
25-25: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
96-96: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
193-193: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
291-291: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
383-383: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
493-493: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
608-608: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
703-703: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/guides/improved-error-messages.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
104-104: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
124-124: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
174-174: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
179-179: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
349-349: Bare URL used
(MD034, no-bare-urls)
350-350: Bare URL used
(MD034, no-bare-urls)
351-351: Bare URL used
(MD034, no-bare-urls)
352-352: Bare URL used
(MD034, no-bare-urls)
IMPROVEMENT_SUMMARY.md
80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-and-test
🔇 Additional comments (2)
lib/react_on_rails/helper.rb (1)
798-803: Clarify SmartError migration pathThanks for switching the missing auto-loaded bundle check to the new
SmartErrorflow. A follow-up note: anywhere else we were rescuingReactOnRails::Errorfrom this code path should be reviewed because the concrete type has now changed. If you already swept callers/tests, feel free to ignore. Otherwise, consider a quick search so those handlers keep working.spec/react_on_rails/smart_error_spec.rb (1)
106-127: Solid coverage of solution pathways.The parameterized check ensures every error type gets a non-empty, string solution response—nice way to safeguard future regressions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
IMPROVEMENT_SUMMARY.md(1 hunks)REACT_ON_RAILS_IMPROVEMENTS.md(1 hunks)docs/guides/improved-error-messages.md(1 hunks)node_package/src/ReactOnRails.client.ts(2 hunks)node_package/tests/debugLogging.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- node_package/src/ReactOnRails.client.ts
🧰 Additional context used
📓 Path-based instructions (3)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
IMPROVEMENT_SUMMARY.mddocs/guides/improved-error-messages.mdREACT_ON_RAILS_IMPROVEMENTS.mdnode_package/tests/debugLogging.test.js
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
IMPROVEMENT_SUMMARY.mddocs/guides/improved-error-messages.mdREACT_ON_RAILS_IMPROVEMENTS.mdnode_package/tests/debugLogging.test.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JavaScript/TypeScript and fix all linter offenses before committing
Files:
node_package/tests/debugLogging.test.js
🧠 Learnings (5)
📓 Common learnings
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
IMPROVEMENT_SUMMARY.mddocs/guides/improved-error-messages.mdnode_package/tests/debugLogging.test.js
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
REACT_ON_RAILS_IMPROVEMENTS.md
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
node_package/tests/debugLogging.test.js
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
node_package/tests/debugLogging.test.js
🪛 markdownlint-cli2 (0.18.1)
docs/guides/improved-error-messages.md
29-29: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
107-107: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
211-211: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
316-316: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 GitHub Actions: JS unit tests for Renderer package
node_package/tests/debugLogging.test.js
[error] 44-44: Test failed in debugLogging.test.js due to ReactOnRails overwrite protection error observed in ReactOnRails.client.ts.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: markdown-link-check
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: claude-review
- GitHub Check: build-and-test
| // Import ReactOnRails after mocking using dynamic import | ||
| await import('../src/ReactOnRails.client.ts'); | ||
| }); |
There was a problem hiding this comment.
Reset the global ReactOnRails handle between tests
ReactOnRails.client.ts guards against being loaded twice; because beforeEach dynamically re-imports the module without clearing window.ReactOnRails, the second import raises the overwrite-protection error that is currently failing CI. Please delete (and optionally restore) the global before re-importing so every test starts from a clean slate.
Apply this diff to isolate each test run:
@@
- // Mock console.log to capture output
+ // Mock console.log to capture output
consoleOutput = [];
originalConsoleLog = console.log;
console.log = jest.fn((...args) => {
consoleOutput.push(args.join(' '));
});
+ // Ensure ReactOnRails can reinitialize safely
+ delete window.ReactOnRails;
@@
afterEach(() => {
console.log = originalConsoleLog;
+ delete window.ReactOnRails;
jest.clearAllMocks();
});Also applies to: 48-50
🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 44-44: Test failed in debugLogging.test.js due to ReactOnRails overwrite protection error observed in ReactOnRails.client.ts.
🤖 Prompt for AI Agents
In node_package/tests/debugLogging.test.js around lines 43-45 (and also 48-50),
the test re-imports ../src/ReactOnRails.client.ts without clearing the global
window.ReactOnRails, causing the module's overwrite-protection to throw on
repeated loads; before each dynamic import remove window.ReactOnRails (e.g.,
delete window.ReactOnRails) and after the test optionally restore any previous
value if needed so each test starts with a clean global and the dynamic import
won't trigger the guard.
justin808
force-pushed
the
better-error-messages
branch
from
e0abadf to
af79d57
Compare
|
@claude review this PR |
|
Claude finished @justin808's task —— View job PR Review: Smart Error Messages with Actionable SolutionsI've completed a comprehensive review of this PR. Overall, this is an excellent addition that significantly improves the developer experience. Here's my detailed feedback: ✅ Strengths1. Outstanding Developer Experience EnhancementThe new
Example impact: A developer seeing "Component 'HelloWorld' not found" now gets:
2. Well-Architected ImplementationRuby side (
JavaScript side (
3. Excellent Test CoverageThe test file
4. Strong DocumentationThe guide at
5. Backward Compatibility
🔍 Areas for Improvement1. Prerender Error Console Message Handling (lib/react_on_rails/prerender_error.rb:82-85)if console_messages && console_messages.strip.present?
message << Rainbow("Console Output:").magenta.bright << "\n"
message << "#{console_messages}\n\n"
endIssue: Large console output from SSR could make error messages overwhelming. Recommendation: Consider truncating very large console messages: if console_messages && console_messages.strip.present?
message << Rainbow("Console Output:").magenta.bright << "\n"
trimmed_console = Utils.smart_trim(console_messages, MAX_ERROR_SNIPPET_TO_LOG)
message << "#{trimmed_console}\n\n"
end2. Component Size Calculation Performance (packages/react-on-rails/src/base/client.ts:207)const size = component.toString().length;Issue: Calling Recommendation:
3. Dependency DeclarationThe
Recommendation: Ensure 4. Error Type CoverageThe current implementation handles these well:
Suggestion for future: Consider adding patterns for:
📋 Code Quality AssessmentRuby Code
TypeScript Code
🎯 Specific Recommendations1. Enhanced Error ContextConsider adding to def additional_info
info = []
info << "#{Rainbow('Component:').blue} #{component_name}" if component_name
info << "#{Rainbow('Rails Environment:').blue} #{Rails.env}" # Show actual env
info << "#{Rainbow('React on Rails Version:').blue} #{ReactOnRails::VERSION}"
# ... rest of method
endThis helps with debugging version-specific issues. 2. Auto-bundling Promotion StrategyThe PR does an excellent job promoting auto-bundling as the primary solution. Consider:
3. Integration with Error Tracking ServicesThe existing
🚀 Impact AssessmentDeveloper Experience: ⭐⭐⭐⭐⭐ (5/5)
Code Quality: ⭐⭐⭐⭐½ (4.5/5)
Risk Level: 🟢 Low
Maintenance Impact: 🟢 Low
🎉 ConclusionThis PR successfully achieves its goal of providing "Better Error Messages with actionable solutions." The implementation is thoughtful, well-tested, and will significantly improve the developer experience for React on Rails users. Recommendation: APPROVE ✅The PR is ready to merge. The suggestions above are minor enhancements that can be addressed in follow-up PRs if desired. What Makes This PR Excellent:
Great work on this implementation! This will make React on Rails much more approachable, especially for new users. The colored, actionable error messages are a game-changer for debugging. Minor Notes for Merge:
|
Major improvements to developer experience through enhanced error messages: - Add SmartError class with contextual, actionable error messages - Prioritize auto-bundling (no registration required) in component errors - Include component name suggestions for typos - Enhanced prerender errors with pattern-based troubleshooting - Add debug mode for component registration logging - Show registration timing and component sizes in debug mode - Improve error formatting with colored output - Add comprehensive documentation for new error features Key benefits: - Developers see exact steps to fix issues - Auto-bundling promoted as primary solution (simpler than manual registration) - Reduced debugging time with contextual solutions - Better visibility into component registration process 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Fix performance API compatibility with polyfill - Add missing clientStartup mock in tests - Replace globalThis with window in test file - Use async import instead of require - Add language hints to markdown code blocks - Ensure files end with newline character
Deleted demo file that was causing RuboCop violations in CI. The demo content has been incorporated into proper documentation and implementation files. Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
better-error-messages
branch
from
af79d57 to
0b30c04
Compare
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (9)
node_package/lib/loadJsonFile.js (1)
5-22: Consider enhancing error diagnostics.The error handling could provide more actionable information by distinguishing between file system errors (file not found, permissions) and JSON parsing errors.
try { const file = JSON.parse(await fs.readFile(filePath, 'utf8')); loadedJsonFiles.set(filePath, file); return file; - } catch (error) { - console.error(`Failed to load JSON file: ${filePath}`, error); + } catch (error) { + if (error.code === 'ENOENT') { + console.error(`JSON file not found: ${filePath}`); + } else if (error instanceof SyntaxError) { + console.error(`Invalid JSON in file: ${filePath}`, error.message); + } else { + console.error(`Failed to load JSON file: ${filePath}`, error); + } throw error; }IMPROVEMENT_SUMMARY.md (1)
1-169: Excellent documentation of the improvement.The summary effectively communicates the value of the enhanced error messages, with concrete before/after examples showing significant UX improvements. The emphasis on auto-bundling as the primary solution (lines 121-129) is a smart UX decision that reduces configuration burden.
Optional style refinement:
Line 123 contains a minor redundancy flagged by static analysis. Consider simplifying:
-The improved error messages now **prioritize React on Rails' auto-bundling feature**, which completely eliminates the need for manual component registration. +The improved error messages now **prioritize React on Rails' auto-bundling feature**, which eliminates the need for manual component registration.node_package/lib/RenderUtils.d.ts (1)
1-1: Consider aligning type signature with implementation's falsy handling.The implementation in
RenderUtils.jshandles falsyscriptBodyvalues by returning an empty string, but the type signature requiresscriptBody: string. While the current strict typing encourages callers to explicitly pass strings, you might considerscriptBody: string | null | undefinedfor better alignment with the runtime behavior.Apply this diff if you want to align the types:
-export declare function wrapInScriptTags(scriptId: string, scriptBody: string): string; +export declare function wrapInScriptTags(scriptId: string, scriptBody: string | null | undefined): string;node_package/lib/scriptSanitizedVal.js (1)
1-5: Consider more precise regex pattern for script tag sanitization.The current regex
/\<\/\W*script/giuses\W*which matches any non-word characters, not just whitespace. This could match unintended patterns. Consider using\s*for whitespace-only matching, or a more explicit pattern.Apply this diff for more precise whitespace handling:
- const re = /<\/\W*script/gi; + const re = /<\/\s*script/gi; return val.replace(re, '(/script');docs/guides/improved-error-messages.md (1)
362-366: Resolve markdownlint MD034 warning.markdownlint is flagging the raw URLs in the Support section. Swapping in descriptive link text keeps the doc lint-clean while preserving the destinations.
- React + Rails Slack: [https://invite.reactrails.com](https://invite.reactrails.com) - 🆓 GitHub Issues: [https://github.com/shakacode/react_on_rails/issues](https://github.com/shakacode/react_on_rails/issues) - 📖 Discussions: [https://github.com/shakacode/react_on_rails/discussions](https://github.com/shakacode/react_on_rails/discussions) + React + Rails Slack: [React + Rails Slack invite](https://invite.reactrails.com) + 🆓 GitHub Issues: [Issue tracker](https://github.com/shakacode/react_on_rails/issues) + 📖 Discussions: [Community discussions](https://github.com/shakacode/react_on_rails/discussions)node_package/lib/pro/injectRSCPayload.d.ts (1)
4-33: Align parameter docs with the real signature.The second argument is an
RSCRequestTracker, not arailsContext. Leaving the old name in place misleads anyone consuming the generated typings.- * @param railsContext - Context for the current request + * @param rscRequestTracker - Coordinates RSC payload streams for this requestnode_package/lib/isRenderFunction.js (1)
18-20: Arity check may not handle all function patterns correctly.The
component.length >= 2check assumes that render functions have 2+ explicit parameters. This may produce incorrect results for:
- Functions with rest parameters:
(...args) => {}haslength = 0- Functions with default parameters:
(props, railsContext = {}) => {}haslength = 1- Destructured parameters:
({ prop1, prop2 }, railsContext) => {}still haslength = 2(this works)Consider documenting these edge cases or adding the explicit
renderFunctionproperty as the recommended approach for marking render functions.node_package/lib/pro/RSCProvider.js (1)
30-36: Docstring still referencesrailsContext.The factory no longer accepts a
railsContextargument, but the JSDoc still documents it. Please drop or update that param description so consumers aren’t hunting for a missing argument.Apply this diff to remove the stale line:
- * @param railsContext - Context for the current requestnode_package/lib/pro/PostSSRHookTracker.d.ts (1)
19-32: JSDoc overstates error behavior.Runtime just logs when hooks are added late or
notifySSREndruns twice (see node_package/src/pro/PostSSRHookTracker.ts:27-76), so documenting@throwsis misleading. Please align the comment with the actual behavior or update the implementation to match the docs.A minimal doc tweak could look like:
- * @throws Error if called after SSR has already ended + * Logs a warning if called after SSR has already ended
| export const authenticityHeaders = (otherHeaders = {}) => | ||
| Object.assign(otherHeaders, { | ||
| 'X-CSRF-Token': authenticityToken(), | ||
| 'X-Requested-With': 'XMLHttpRequest', | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid mutating the caller's otherHeaders object.
Object.assign mutates the first argument, which means the caller's otherHeaders object is modified. This could cause unexpected side effects if the caller reuses the object.
Apply this diff to prevent mutation:
-export const authenticityHeaders = (otherHeaders = {}) =>
- Object.assign(otherHeaders, {
+export const authenticityHeaders = (otherHeaders = {}) => ({
+ ...otherHeaders,
- 'X-CSRF-Token': authenticityToken(),
- 'X-Requested-With': 'XMLHttpRequest',
- });
+ 'X-CSRF-Token': authenticityToken(),
+ 'X-Requested-With': 'XMLHttpRequest',
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const authenticityHeaders = (otherHeaders = {}) => | |
| Object.assign(otherHeaders, { | |
| 'X-CSRF-Token': authenticityToken(), | |
| 'X-Requested-With': 'XMLHttpRequest', | |
| }); | |
| export const authenticityHeaders = (otherHeaders = {}) => ({ | |
| ...otherHeaders, | |
| 'X-CSRF-Token': authenticityToken(), | |
| 'X-Requested-With': 'XMLHttpRequest', | |
| }); |
🤖 Prompt for AI Agents
In node_package/lib/Authenticity.js around lines 8 to 12, the function uses
Object.assign(otherHeaders, {...}) which mutates the caller's otherHeaders;
change it to return a new object instead (e.g., Object.assign({}, otherHeaders,
{ 'X-CSRF-Token': authenticityToken(), 'X-Requested-With': 'XMLHttpRequest' })
or use the object spread ({...otherHeaders, 'X-CSRF-Token': authenticityToken(),
'X-Requested-With': 'XMLHttpRequest'})) so the original otherHeaders is not
modified.
| import type { CreateParams, CreateReactOutputResult } from './types/index.ts'; | ||
| /** | ||
| * Logic to either call the renderFunction or call React.createElement to get the | ||
| * React.Component | ||
| * @param options | ||
| * @param options.componentObj | ||
| * @param options.props | ||
| * @param options.domNodeId | ||
| * @param options.trace | ||
| * @param options.location | ||
| * @returns {ReactElement} | ||
| */ | ||
| export default function createReactOutput({ | ||
| componentObj, | ||
| props, | ||
| railsContext, | ||
| domNodeId, | ||
| trace, | ||
| shouldHydrate, | ||
| }: CreateParams): CreateReactOutputResult; |
There was a problem hiding this comment.
Align the type import with the emitted files
Line [1] imports ./types/index.ts, but only index.js/index.d.ts ship in lib/types. Downstream TypeScript builds will fail. Please switch to the emitted module:
-import type { CreateParams, CreateReactOutputResult } from './types/index.ts';
+import type { CreateParams, CreateReactOutputResult } from './types/index.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { CreateParams, CreateReactOutputResult } from './types/index.ts'; | |
| /** | |
| * Logic to either call the renderFunction or call React.createElement to get the | |
| * React.Component | |
| * @param options | |
| * @param options.componentObj | |
| * @param options.props | |
| * @param options.domNodeId | |
| * @param options.trace | |
| * @param options.location | |
| * @returns {ReactElement} | |
| */ | |
| export default function createReactOutput({ | |
| componentObj, | |
| props, | |
| railsContext, | |
| domNodeId, | |
| trace, | |
| shouldHydrate, | |
| }: CreateParams): CreateReactOutputResult; | |
| import type { CreateParams, CreateReactOutputResult } from './types/index.js'; | |
| /** | |
| * Logic to either call the renderFunction or call React.createElement to get the | |
| * React.Component | |
| * @param options | |
| * @param options.componentObj | |
| * @param options.props | |
| * @param options.domNodeId | |
| * @param options.trace | |
| * @param options.location | |
| * @returns {ReactElement} | |
| */ | |
| export default function createReactOutput({ | |
| componentObj, | |
| props, | |
| railsContext, | |
| domNodeId, | |
| trace, | |
| shouldHydrate, | |
| }: CreateParams): CreateReactOutputResult; |
🤖 Prompt for AI Agents
In node_package/lib/createReactOutput.d.ts lines 1-20, the import currently
references './types/index.ts' which won't exist in the distributed package;
change the import to the emitted module (remove the .ts extension) — e.g. import
type { CreateParams, CreateReactOutputResult } from './types'; — so downstream
TypeScript resolves to the shipped index.d.ts/index.js files.
| const handleError = (options) => { | ||
| const { e, jsCode, serverSide } = options; | ||
| console.error('Exception in rendering!'); | ||
| let msg = handleRenderFunctionIssue(options); | ||
| if (jsCode) { | ||
| console.error(`JS code was: ${jsCode}`); | ||
| } | ||
| if (e.fileName) { | ||
| console.error(`location: ${e.fileName}:${e.lineNumber}`); | ||
| } | ||
| console.error(`message: ${e.message}`); | ||
| console.error(`stack: ${e.stack}`); | ||
| if (serverSide) { | ||
| msg += `Exception in rendering! | ||
| ${e.fileName ? `\nlocation: ${e.fileName}:${e.lineNumber}` : ''} | ||
| Message: ${e.message} | ||
| ${e.stack}`; | ||
| // In RSC (React Server Components) bundles, renderToString is not available. | ||
| // Therefore, we return the raw error message as a string instead of converting it to HTML. | ||
| if (typeof renderToString === 'function') { | ||
| const reactElement = React.createElement('pre', null, msg); | ||
| return renderToString(reactElement); | ||
| } | ||
| return msg; | ||
| } | ||
| return 'undefined'; | ||
| }; | ||
| export default handleError; |
There was a problem hiding this comment.
Stack traces unconditionally exposed in server-side errors.
Lines 39-45 and 52 include full stack traces and file paths in error output without environment checks. This exposes sensitive information (internal paths, application structure, implementation details) in production environments.
Based on learnings, error stacks should be conditionally included in development and test environments only. Consider gating detailed error information behind environment checks (e.g., NODE_ENV !== 'production').
Apply this pattern to conditionally include sensitive details:
const handleError = (options) => {
const { e, jsCode, serverSide } = options;
+ const isDevelopment = process.env.NODE_ENV !== 'production';
+
console.error('Exception in rendering!');
let msg = handleRenderFunctionIssue(options);
if (jsCode) {
console.error(`JS code was: ${jsCode}`);
}
- if (e.fileName) {
+ if (isDevelopment && e.fileName) {
console.error(`location: ${e.fileName}:${e.lineNumber}`);
}
console.error(`message: ${e.message}`);
- console.error(`stack: ${e.stack}`);
+ if (isDevelopment) {
+ console.error(`stack: ${e.stack}`);
+ }
if (serverSide) {
msg += `Exception in rendering!
-${e.fileName ? `\nlocation: ${e.fileName}:${e.lineNumber}` : ''}
+${isDevelopment && e.fileName ? `\nlocation: ${e.fileName}:${e.lineNumber}` : ''}
Message: ${e.message}
-${e.stack}`;
+${isDevelopment ? e.stack : ''}`;| // Hydrate if available and was server rendered | ||
| const shouldHydrate = supportsHydrate && !!domNode.innerHTML; | ||
| const reactElementOrRouterResult = createReactOutput({ | ||
| componentObj, | ||
| props, | ||
| domNodeId, | ||
| trace, | ||
| railsContext, | ||
| shouldHydrate, | ||
| }); | ||
| if (isServerRenderHash(reactElementOrRouterResult)) { | ||
| throw new Error(`\ | ||
| You returned a server side type of react-router error: ${JSON.stringify(reactElementOrRouterResult)} | ||
| You should return a React.Component always for the client side entry point.`); | ||
| } else { | ||
| const rootOrElement = reactHydrateOrRender(domNode, reactElementOrRouterResult, shouldHydrate); | ||
| this.state = 'rendered'; | ||
| if (supportsRootApi) { | ||
| this.root = rootOrElement; | ||
| } | ||
| } |
There was a problem hiding this comment.
Await the async render output before hydrating.
createReactOutput now returns a Promise whenever a render-function resolves asynchronously (the exact case enabled for streaming). Because this code immediately passes the unresolved Promise to reactHydrateOrRender, React blows up with “Objects are not valid as a React child,” and the client fails to hydrate streamed components. Please resolve the output before running the server-render-hash guard or calling reactHydrateOrRender.
- const reactElementOrRouterResult = createReactOutput({
- componentObj,
- props,
- domNodeId,
- trace,
- railsContext,
- shouldHydrate,
- });
+ const reactElementOrRouterResult = await Promise.resolve(
+ createReactOutput({
+ componentObj,
+ props,
+ domNodeId,
+ trace,
+ railsContext,
+ shouldHydrate,
+ }),
+ );Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In node_package/lib/pro/ClientSideRenderer.js around lines 104 to 124,
createReactOutput can return a Promise for async/render-streaming cases so the
code must await/resolve its result before using it; change the code to await (or
Promise.resolve) the reactElementOrRouterResult, then run isServerRenderHash on
the resolved value and pass that resolved value into reactHydrateOrRender (so
you never pass a Promise into React). Ensure the await is inside an async
context (or chain with .then) and preserve the existing flow for setting
this.state and this.root after successful render.
| * @param name | ||
| * @returns { name, component, isRenderFunction, isRenderer } | ||
| */ | ||
| export declare const get: (name: string) => RegisteredComponent; |
There was a problem hiding this comment.
Return type must allow for missing registrations
componentRegistry.get(name) returns undefined when the component hasn’t registered yet. The .d.ts currently narrows this to RegisteredComponent, so TS consumers will happily dereference fields that may not exist and hit runtime errors. Please widen the return type to include undefined.
-export declare const get: (name: string) => RegisteredComponent;
+export declare const get: (name: string) => RegisteredComponent | undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export declare const get: (name: string) => RegisteredComponent; | |
| export declare const get: (name: string) => RegisteredComponent | undefined; |
🤖 Prompt for AI Agents
In node_package/lib/pro/ComponentRegistry.d.ts around line 10, the exported get
function's return type is currently RegisteredComponent but the implementation
can return undefined for unregistered names; update the declaration to widen the
return type to RegisteredComponent | undefined so callers are aware it may be
missing and can handle the undefined case.
| export function wrapInScriptTags(scriptId, scriptBody) { | ||
| if (!scriptBody) { | ||
| return ''; | ||
| } | ||
| return ` | ||
| <script id="${scriptId}"> | ||
| ${scriptBody} | ||
| </script>`; | ||
| } |
There was a problem hiding this comment.
Critical: Potential XSS vulnerability from unescaped HTML interpolation.
The scriptId and scriptBody parameters are directly interpolated into HTML without escaping. This creates XSS risk if these values contain user-controlled data or aren't properly sanitized upstream:
scriptIdcould contain quotes that break out of the attribute contextscriptBodycould contain</script>sequences that break out of the script tag
Apply defensive escaping:
+function escapeHtml(str) {
+ return str.replace(/[&<>"']/g, (char) => {
+ const map = { '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' };
+ return map[char];
+ });
+}
+
export function wrapInScriptTags(scriptId, scriptBody) {
if (!scriptBody) {
return '';
}
+ // Escape scriptId for attribute context
+ const escapedId = escapeHtml(scriptId);
+ // Escape closing script tags in body to prevent breaking out
+ const escapedBody = scriptBody.replace(/<\/script>/gi, '<\\/script>');
+
return `
-<script id="${scriptId}">
-${scriptBody}
+<script id="${escapedId}">
+${escapedBody}
</script>`;
}Alternatively, verify that callers always provide safe, sanitized values and document this requirement clearly.
🤖 Prompt for AI Agents
In node_package/lib/RenderUtils.js around lines 2–10, the function directly
interpolates scriptId into an attribute and scriptBody into a script tag,
creating an XSS risk; fix by (1) sanitizing/encoding scriptId for attribute
context (prefer a whitelist: allow only [A-Za-z0-9_-] or run an HTML-attribute
escape for quotes/angles), and (2) preventing scriptBody from breaking out of
the tag by either not doing raw interpolation (create the script node and set
textContent in code that runs in the safe environment) or defensively escaping
the closing sequence and unsafe characters (e.g. replace every occurrence of
"</script>" with "<\\/script>" and apply necessary JS/string escaping or
base64-encode the body and decode inside a controlled script) so untrusted input
cannot close the tag or inject attributes; implement input validation and/or
escaping and add a comment documenting the expectation if callers must supply
already-sanitized values.
| import type { | ||
| RegisteredComponent, | ||
| RenderResult, | ||
| RenderState, | ||
| StreamRenderState, | ||
| FinalHtmlResult, | ||
| } from './types/index.ts'; | ||
|
|
||
| export declare function createResultObject( | ||
| html: FinalHtmlResult | null, | ||
| consoleReplayScript: string, | ||
| renderState: RenderState | StreamRenderState, | ||
| ): RenderResult; |
There was a problem hiding this comment.
Fix the declaration import path.
The published node_package/lib bundle does not ship ./types/index.ts, so TypeScript consumers hit “Cannot find module './types/index.ts'” when loading this d.ts. Point the import at the emitted JS entry (which resolves to the colocated .d.ts) so downstream builds keep working.
-import type {
+import type {
RegisteredComponent,
RenderResult,
RenderState,
StreamRenderState,
FinalHtmlResult,
-} from './types/index.ts';
+} from './types/index.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { | |
| RegisteredComponent, | |
| RenderResult, | |
| RenderState, | |
| StreamRenderState, | |
| FinalHtmlResult, | |
| } from './types/index.ts'; | |
| export declare function createResultObject( | |
| html: FinalHtmlResult | null, | |
| consoleReplayScript: string, | |
| renderState: RenderState | StreamRenderState, | |
| ): RenderResult; | |
| import type { | |
| RegisteredComponent, | |
| RenderResult, | |
| RenderState, | |
| StreamRenderState, | |
| FinalHtmlResult, | |
| } from './types/index.js'; | |
| export declare function createResultObject( | |
| html: FinalHtmlResult | null, | |
| consoleReplayScript: string, | |
| renderState: RenderState | StreamRenderState, | |
| ): RenderResult; |
🤖 Prompt for AI Agents
In node_package/lib/serverRenderUtils.d.ts around lines 1 to 13, the type import
currently points to './types/index.ts' which is not shipped; change the import
to the emitted JS entry (for example './types/index.js' or simply './types') so
TypeScript resolves the colocated .d.ts at runtime; update the import statement
accordingly (and mirror the same pattern for any other .d.ts files that
reference './types/index.ts').
| export function debugTurbolinks(...msg) { | ||
| if (!window) { | ||
| return; | ||
| } | ||
| if (globalThis.ReactOnRails?.option('traceTurbolinks')) { | ||
| console.log('TURBO:', ...msg); | ||
| } |
There was a problem hiding this comment.
Guard against window being undefined
Calling debugTurbolinks during SSR or tests (where window isn’t defined) will throw a ReferenceError before the guard executes. Swap the check to typeof window === 'undefined' so the helper safely no-ops outside the browser.
-export function debugTurbolinks(...msg) {
- if (!window) {
+export function debugTurbolinks(...msg) {
+ if (typeof window === 'undefined') {
return;
}🤖 Prompt for AI Agents
In node_package/lib/turbolinksUtils.js around lines 6 to 12, the current check
uses "if (!window)" which throws a ReferenceError when window is undefined
(SSR/tests); replace that check with "if (typeof window === 'undefined')" so the
function no-ops safely outside the browser, then retain the existing
globalThis.ReactOnRails option check and console.log behavior.
| export function turbolinksVersion5() { | ||
| return typeof Turbolinks.controller !== 'undefined'; | ||
| } | ||
| export function turbolinksSupported() { | ||
| return Turbolinks.supported; |
There was a problem hiding this comment.
Handle missing Turbolinks gracefully
When Turbolinks isn’t loaded—which is the default for many installs—accessing Turbolinks.controller or Turbolinks.supported throws immediately. These helpers should short-circuit if Turbolinks is undefined so optional integrations don’t crash.
export function turbolinksVersion5() {
- return typeof Turbolinks.controller !== 'undefined';
+ return typeof Turbolinks !== 'undefined' && typeof Turbolinks.controller !== 'undefined';
}
export function turbolinksSupported() {
- return Turbolinks.supported;
+ return typeof Turbolinks !== 'undefined' && !!Turbolinks.supported;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function turbolinksVersion5() { | |
| return typeof Turbolinks.controller !== 'undefined'; | |
| } | |
| export function turbolinksSupported() { | |
| return Turbolinks.supported; | |
| export function turbolinksVersion5() { | |
| return typeof Turbolinks !== 'undefined' && typeof Turbolinks.controller !== 'undefined'; | |
| } | |
| export function turbolinksSupported() { | |
| return typeof Turbolinks !== 'undefined' && !!Turbolinks.supported; | |
| } |
🤖 Prompt for AI Agents
In node_package/lib/turbolinksUtils.js around lines 20 to 24, the helpers access
Turbolinks properties directly which throws when Turbolinks is undefined; change
both functions to first check that typeof Turbolinks !== 'undefined' (and
Turbolinks != null if you prefer) and only then access controller or supported,
returning false (or a safe default) when Turbolinks is not present so optional
integrations do not crash.
| * (see "What is a root?" in https://github.com/reactwg/react-18/discussions/5). | ||
| * Under React 16/17: Reference to your component's backing instance or `null` for stateless components. | ||
| */ | ||
| render(name: string, props: Record<string, string>, domNodeId: string, hydrate?: boolean): RenderReturnType; |
There was a problem hiding this comment.
Critical: Fix props type to accept any values, not just strings.
The render method signature restricts props to Record<string, string>, but React components accept props of any type (numbers, booleans, objects, arrays, functions, etc.). This is inconsistent with the Params interface (line 145) which correctly uses Record<string, unknown>.
Apply this diff to fix the type:
- render(name: string, props: Record<string, string>, domNodeId: string, hydrate?: boolean): RenderReturnType;
+ render(name: string, props: Record<string, unknown>, domNodeId: string, hydrate?: boolean): RenderReturnType;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| render(name: string, props: Record<string, string>, domNodeId: string, hydrate?: boolean): RenderReturnType; | |
| render(name: string, props: Record<string, unknown>, domNodeId: string, hydrate?: boolean): RenderReturnType; |
🤖 Prompt for AI Agents
In node_package/lib/types/index.d.ts around line 323, the render method
currently types props as Record<string, string> which incorrectly restricts
values to strings; change the props parameter to Record<string, unknown>
(matching the Params interface) so props can accept numbers, booleans, objects,
arrays, functions, etc., and update the function signature accordingly.
|
@claude review this PR |
|
Claude finished @justin808's task —— View job PR Review: Smart Error Messages with Actionable SolutionsReview Tasks
Executive SummaryThis PR adds intelligent error messages with actionable solutions to React on Rails, significantly improving the developer experience. The implementation is well-architected, thoroughly tested, and maintains backward compatibility. Overall Assessment: ⭐⭐⭐⭐½ (4.5/5) - Excellent work with minor refinements needed. Recommendation: APPROVE with minor suggestions ✅ 🎯 Strengths1. Outstanding Developer ExperienceThe new
Example Impact: "Component HelloWorld not found" → Multi-line guide with directory structure, exact commands, and alternative approaches. 2. Well-Architected Ruby Implementation
3. JavaScript Debug Capabilities
4. Excellent Test Coverage
5. Comprehensive Documentation
6. Backward Compatibility
|
| Aspect | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Clean separation, clear patterns |
| Test Coverage | ⭐⭐⭐⭐½ | Excellent Ruby tests, missing JS tests |
| Documentation | ⭐⭐⭐⭐⭐ | Comprehensive guide with examples |
| Security | ⭐⭐⭐ | XSS vulnerability needs fixing |
| Performance | ⭐⭐⭐⭐⭐ | No production impact |
| Backward Compatibility | ⭐⭐⭐⭐⭐ | 100% compatible |
🎯 Action Items Before Merge
Must Fix (Critical)
- Fix XSS vulnerability in
RenderUtils.js - Fix React 19 compatibility in
reactApis.cjs - Fix UTF-8 corruption in
injectRSCPayload.js - Add railsContext to client rendering
- Fix type import paths (
.ts→.js)
Should Fix (Major)
- Prevent object mutation in
authenticityHeaders - Await async render results in
ClientSideRenderer.js - Fix ComponentRegistry return type
- Add stream error handling in
RSCRequestTracker.js - Fix error boundary reset in
RSCRoute.js
Nice to Have
- Add environment checks to error stack exposure
- Verify
node_package/lib/files should be committed - Add JavaScript tests for debug mode
- Add integration tests for view helper
🎉 Conclusion
This PR represents a significant improvement to React on Rails' developer experience. The smart error messages will save developers countless hours of debugging time, and the emphasis on auto-bundling simplifies the setup process.
The implementation is well-thought-out and thoroughly tested on the Ruby side. However, several critical issues in the JavaScript/TypeScript code need attention before merging, particularly around:
- Security (XSS)
- React 19 compatibility
- Stream handling
- Type definitions
Once the critical issues are addressed, this will be an excellent addition to the framework.
Final Recommendation: APPROVE after fixing critical issues ✅
Great work on this feature! The before/after error message examples speak for themselves—this is exactly the kind of DX improvement that makes frameworks a joy to use.
…thod Fixes a critical regression where the render() method was not passing railsContext to createReactOutput(), causing render functions that depend on railsContext (e.g., for location, locale) to receive undefined. Also adds guards for: - Server render hash objects (react-router errors) - Promise returns (async render functions) These guards ensure the synchronous render() API doesn't silently accept async operations that should use reactOnRailsComponentLoaded() instead. Addresses code review comment from PR #1834: #1834 (comment) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds contextual, actionable error messages for React on Rails server-side errors. ## Key Features - **SmartError class** with pattern detection and fuzzy component name matching - **Enhanced PrerenderError** with troubleshooting tips for common SSR issues (window/document undefined, hydration mismatches) - **Auto-bundling suggestions** as the primary recommended approach - **Colored, formatted error output** for better readability ## Benefits - ⚡ Faster debugging with specific, actionable guidance - 🎯 Reduced time spent on common mistakes (typos, missing registration) - 📝 Clear examples showing how to fix each error type - 🚀 Prioritizes simpler auto-bundling over manual registration ## Testing - ✅ All RSpec tests passing - ✅ RuboCop clean (with appropriate disable directives for complexity) ## Documentation Ruby-focused error message guide included in `docs/guides/improved-error-messages.md` ## Part of Improvement Series This is PR #1 of 3 focused PRs split from #1834: - **PR #1 (this):** Ruby-side error improvements ✨ - PR #2: JavaScript debug logging (coming next) - PR #3: Documentation and roadmap (optional) Closes part of #1834 <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/1934) <!-- Reviewable:end --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Smart, context-aware error messages with actionable solutions and fuzzy component-name suggestions. * Optional flags to enable debugMode and logComponentRegistration for richer registration visibility. * **Documentation** * New guide documenting improved error messages, examples, troubleshooting steps, and best practices. * **Bug Fixes** * Improved server-side error formatting with structured sections and clearer troubleshooting guidance. * **Tests** * Added comprehensive tests covering smart error messages and solution generation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <[email protected]>
Add comprehensive test coverage for the debug logging features implemented in PR #1934 (Part 2 of 3 in the improvement series). The tests verify: - logComponentRegistration option for opt-in component registration logging - debugMode option for detailed debug output including component sizes - Performance metrics tracking using performance.now() with Date.now() fallback - Non-intrusive behavior with zero production impact - Option validation and reset functionality All tests pass and ensure the feature works as expected without affecting normal component registration functionality. Related to #1834 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add comprehensive test coverage for the debug logging features implemented in PR #1934 (Part 2 of 3 in the improvement series). The tests verify: - logComponentRegistration option for opt-in component registration logging - debugMode option for detailed debug output including component sizes - Performance metrics tracking using performance.now() with Date.now() fallback - Non-intrusive behavior with zero production impact - Option validation and reset functionality All tests pass and ensure the feature works as expected without affecting normal component registration functionality. Related to #1834 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
This PR implements the first phase of the React on Rails incremental improvements plan: Better Error Messages with actionable solutions. It transforms generic error messages into intelligent, contextual guides that help developers quickly resolve issues.
Key Changes
1. Smart Error System
SmartErrorclass provides contextual, actionable error messages2. Enhanced Developer Experience
3. JavaScript Debug Capabilities
Example of Improvement
Before:
After:
Benefits
Testing
Documentation
docs/guides/improved-error-messages.mdImpact
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Documentation