feat: Owner Stacks in development error reports (#3887)#4089
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughAdds a ChangesReact Owner-Stack Capture and Enrichment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
justin808
force-pushed
the
jg/3887-owner-stacks-error-reports
branch
from
6c2a7cd to
76e2556
Compare
|
+ci-run-hosted |
github-actions
Bot
added
the
ready-for-hosted-ci
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
size-limit report 📦
|
Greptile SummaryThis PR adds dev-only React 19.1+ owner-stack enrichment to React on Rails' SSR and client-side error reporting paths, turning opaque JS stack traces into human-readable component chains (e.g.
Confidence Score: 4/5Safe to merge. The owner-stack enrichment is additive, production is a strict no-op confirmed by tests, and the two issues found affect only a rare non-Error throw edge case and a parameter-naming clarity concern. The feature is well-scoped with thorough tests. The non-Error throw edge case in
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant React as React 19.1+ (dev)
participant onError as renderToPipeableStream onError
participant capture as captureReactOwnerStack
participant reportError as reportError (renderState.error)
participant onShellError as onShellError
participant sendErrorHtml as sendErrorHtml
React->>onError: component throws Error e
onError->>capture: captureReactOwnerStack()
capture-->>onError: "\n at Avatar\n at PostCard"
onError->>onError: "e.stack += "\n\nOwner stack:...""
onError->>reportError: reportError(e) [e.stack has owner chain]
React->>onShellError: same Error object e (shell-fatal only)
onShellError->>sendErrorHtml: convertToError(e) → same e
sendErrorHtml-->>onShellError: shell HTML with enriched stack ✓
Note over onError,capture: No-op on React < 19.1 or production build
%%{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 React as React 19.1+ (dev)
participant onError as renderToPipeableStream onError
participant capture as captureReactOwnerStack
participant reportError as reportError (renderState.error)
participant onShellError as onShellError
participant sendErrorHtml as sendErrorHtml
React->>onError: component throws Error e
onError->>capture: captureReactOwnerStack()
capture-->>onError: "\n at Avatar\n at PostCard"
onError->>onError: "e.stack += "\n\nOwner stack:...""
onError->>reportError: reportError(e) [e.stack has owner chain]
React->>onShellError: same Error object e (shell-fatal only)
onShellError->>sendErrorHtml: convertToError(e) → same e
sendErrorHtml-->>onShellError: shell HTML with enriched stack ✓
Note over onError,capture: No-op on React < 19.1 or production build
Reviews (1): Last reviewed commit: "feat: Owner Stacks in development error ..." | 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: feat Owner Stacks in development error reports (#3887)OverviewThis PR adds dev-only React owner-stack enrichment to SSR and client-side error reports, landing on top of the root error-callbacks foundation from #3933. The design decisions (capability-check gating, no auto-attach for caught/uncaught, append-to-stack for the SSR channel) are well-reasoned and clearly documented in the decision log. The implementation is clean and the test coverage for the helper and the client path is solid. A few issues worth addressing before merge: Issues1. No guard against double-appending the owner stack in streaming SSR
More pressingly: if React ever calls // before mutating error.stack
if (ownerStack && typeof error.stack === 'string' && !error.stack.includes('\n\nOwner stack')) {2. Misleading parameter name in
|
| Path | Positive mock test | No-op (absent API) | Throws guard |
|---|---|---|---|
captureReactOwnerStack helper |
✅ | ✅ | ✅ |
Client onUncaughtError enrichment |
✅ (mocked module) | ✅ (real React 19.0) | — |
| Client no-auto-attach | ✅ | ✅ | — |
Pro streaming SSR onError |
❌ missing | — | — |
Overall solid work. The SSR test gap and the double-append risk are the two items worth addressing before merge.
|
Code Review Summary for PR 4089 — Owner Stacks in development error reports Overall: well-designed and carefully-scoped. The production no-op guarantee is solid, the decision log explains the key tradeoffs clearly, and the mocked test strategy is well-reasoned. Inline comments filed on the three most actionable items; summary below. --- Issues (inline comments filed) ---
--- Minor ---
--- What is done well ---
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/react-on-rails/tests/rootErrorCallbacksOwnerStack.test.tsx (1)
91-104: ⚡ Quick winAdd a direct test for the React 19.2
errorInfo.ownerStackbranch.Current coverage exercises only the fallback
captureReactOwnerStack()path. Add a case that passes{ ownerStack: ... }and asserts that exact stack is logged, so the preferred path can’t regress silently.Suggested test addition
+ it('prefers ownerStack from errorInfo when React provides it', () => { + setupRailsContext('development'); + const appOnUncaughtError = jest.fn(); + setRootErrorHandlers({ onUncaughtError: appOnUncaughtError }); + const context = { componentName: 'OwnerStackThrower', domNodeId: 'owner-dom-id' }; + + const options = buildRootErrorCallbackOptions(context, false); + const ownerStackFromInfo = '\n at ProvidedByReactParent\n at ProvidedByReactChild'; + options.onUncaughtError?.(new Error('render error'), { ownerStack: ownerStackFromInfo }); + + const line = brandedRenderErrorLine(); + expect(line).toContain('ProvidedByReactParent'); + expect(line).toContain('ProvidedByReactChild'); + });🤖 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 `@packages/react-on-rails/tests/rootErrorCallbacksOwnerStack.test.tsx` around lines 91 - 104, Add a new test case after the existing test in the rootErrorCallbacksOwnerStack.test.tsx file that specifically tests the React 19.2 branch where errorInfo contains an ownerStack field. In this new test, pass an errorInfo object with a populated ownerStack property when calling options.onUncaughtError(), then verify that the logged output contains the exact owner stack content that was provided in the errorInfo object (rather than a mocked fallback stack). This ensures the preferred code path using the provided ownerStack property is properly tested and cannot regress silently.react_on_rails/spec/dummy/client/app/startup/OwnerStackThrower.tsx (1)
23-31: ⚡ Quick winUse interfaces for component prop object shapes.
Replace inline
{ shouldThrow: boolean }prop types with named interfaces to match TypeScript conventions in this repository.Proposed refactor
+interface OwnerStackInnerProps { + shouldThrow: boolean; +} + -const OwnerStackInner: React.FC<{ shouldThrow: boolean }> = ({ shouldThrow }) => { +const OwnerStackInner: React.FC<OwnerStackInnerProps> = ({ shouldThrow }) => { if (shouldThrow) { throw new Error('Deliberate owner-stack render error from OwnerStackInner'); } return <p>Owner stack inner ready</p>; }; -const OwnerStackMiddle: React.FC<{ shouldThrow: boolean }> = ({ shouldThrow }) => ( +interface OwnerStackMiddleProps { + shouldThrow: boolean; +} + +const OwnerStackMiddle: React.FC<OwnerStackMiddleProps> = ({ shouldThrow }) => ( <OwnerStackInner shouldThrow={shouldThrow} /> );As per coding guidelines:
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript.🤖 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/spec/dummy/client/app/startup/OwnerStackThrower.tsx` around lines 23 - 31, Replace the inline prop type definitions in the component function signatures with named interfaces. Create separate interfaces for the props of OwnerStackInner and OwnerStackMiddle components (for example, OwnerStackInnerProps and OwnerStackMiddleProps), each defining the shouldThrow boolean property, then update the React.FC generic parameter to use these named interfaces instead of the inline object type notation. This follows the TypeScript conventions used throughout the repository for defining component prop shapes.Source: Coding guidelines
🤖 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/spec/dummy/client/app/startup/OwnerStackThrower.tsx`:
- Line 1: Add the 'use client' directive as the first line at the top of the
OwnerStackThrower.tsx module, before the import statements. Since the component
uses React.useState (a client-side hook), the file must be marked as a Client
Component with the 'use client' string literal directive to function correctly
in RSC-aware environments and prevent runtime errors.
---
Nitpick comments:
In `@packages/react-on-rails/tests/rootErrorCallbacksOwnerStack.test.tsx`:
- Around line 91-104: Add a new test case after the existing test in the
rootErrorCallbacksOwnerStack.test.tsx file that specifically tests the React
19.2 branch where errorInfo contains an ownerStack field. In this new test, pass
an errorInfo object with a populated ownerStack property when calling
options.onUncaughtError(), then verify that the logged output contains the exact
owner stack content that was provided in the errorInfo object (rather than a
mocked fallback stack). This ensures the preferred code path using the provided
ownerStack property is properly tested and cannot regress silently.
In `@react_on_rails/spec/dummy/client/app/startup/OwnerStackThrower.tsx`:
- Around line 23-31: Replace the inline prop type definitions in the component
function signatures with named interfaces. Create separate interfaces for the
props of OwnerStackInner and OwnerStackMiddle components (for example,
OwnerStackInnerProps and OwnerStackMiddleProps), each defining the shouldThrow
boolean property, then update the React.FC generic parameter to use these named
interfaces instead of the inline object type notation. This follows the
TypeScript conventions used throughout the repository for defining component
prop shapes.
🪄 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: b93292ab-9e8e-4bc7-b6db-cc12e45c1f83
📒 Files selected for processing (10)
CHANGELOG.mdpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/package.jsonpackages/react-on-rails/src/captureReactOwnerStack.tspackages/react-on-rails/src/rootErrorHandlers.tspackages/react-on-rails/tests/captureReactOwnerStack.test.tspackages/react-on-rails/tests/rootErrorCallbacks.test.tsxpackages/react-on-rails/tests/rootErrorCallbacksOwnerStack.test.tsxreact_on_rails/spec/dummy/app/views/pages/root_error_callbacks.html.erbreact_on_rails/spec/dummy/client/app/startup/OwnerStackThrower.tsx
Enrich React on Rails error reporting with React 19.1+'s dev-only captureOwnerStack output (the owner chain, e.g. `at Avatar` / `at PostCard` / `at PostList`) on both the client (CSR/hydrate) and SSR paths. - Add captureReactOwnerStack helper: guarded so it is a strict no-op unless React >= 19.1 AND React's development build is active (the API is exported only from dev builds). Production = no capture, no call. Never throws. - Client: rootErrorHandlers attaches a dev-mode owner-stack logger to React 19 onCaughtError/onUncaughtError (and the hydration recoverable-error logger), preserving React's default reporting channel (console.error for caught, reportError for uncaught/recoverable). - Server (Pro streaming): capture the owner stack synchronously inside the onError/onShellError callbacks and thread it into the rendering error so it reaches the Rails-side PrerenderError via the renderingError metadata. - generateRenderingErrorMessage threads an optional ownerStack into the server-side message and console output. - SmartError :server_rendering_error surfaces an owner-stack section when provided (ROR005), ignoring blank/absent stacks. ExecJS is out of scope: capture must happen JS-side, synchronously, inside React's error callback. Tests: production/old-React no-op (captureReactOwnerStack unit test, generateRenderingErrorMessage, SmartError spec), dev-mode logging (rootErrorCallbacks), and a dummy-app Playwright e2e (OwnerStackThrower nested fixture) asserting the owner chain in the browser console under React 19.2. Closes #3887 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
justin808
force-pushed
the
jg/3887-owner-stacks-error-reports
branch
from
76e2556 to
922d606
Compare
|
+ci-run-hosted |
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
|
Overall review for this PR: solid feature addition with good production safety. See inline comments for specific findings. Summary: (1) OWNER_STACK_MARKER and ownerStackAugmentedStack should be at module scope in streamServerRenderedReactComponent.ts rather than re-created per render call -- minor but real, worth fixing before merge. (2) ownerStackSuffix ternary could be clearer with a named intermediate variable -- nit. (3) logDevRenderError silent early-return when no owner stack is available is correct but worth a brief comment -- nit. (4) The SSR idempotency logic (OWNER_STACK_MARKER guard preventing double-append when onError and onShellError both fire for the same Error object) is the most non-obvious invariant and has no unit test -- worth a follow-up issue if the Pro suite does not cover it. Feature is well-designed, production is a strict no-op, and the decision log documents all key tradeoffs clearly. |
Manual verification: confirmed the feature behaves as documented ✅Reproduced via the merged suites ( ReproductionSquash-merged at Results
The CaveatMechanism-level in jsdom with a stubbed |
1 participant
Why
SSR/CSR errors surfaced from the Node renderer / ExecJS are the single most-reported React on Rails debugging pain: a JS stack with no component context (
Exception in rendering!+ a minified stack). React 19.1's dev-onlycaptureOwnerStackreturns the owner chain — the components that rendered the failing one (e.g.at Avatar/at PostCard/at PostList) — turning a multi-minute hunt into a quick read. Better errors are also exactly what AI coding agents consume to self-correct.Closes #3887 (child of #3865). Builds on the merged root error-callbacks work (#3933) and error classification (#3614/#3724).
What changed
Dev-mode-only owner-stack enrichment on both error paths, with a strict production no-op:
packages/react-on-rails/src/captureReactOwnerStack.ts— wrapsReact.captureOwnerStack()with a guard that resolves the dev-build/version question (see decision log). Returnsundefinedunless React ≥ 19.1's development build is active. Never throws. Also exportsisOwnerStackSupported().streamServerRenderedReactComponent.ts— the owner stack is captured synchronously inside React'sonErrorcallback (which fires for every render error and feedsrenderState.error) and appended toerror.stack. That single mechanism delivers it to both the Rails-side rendering error (PrerenderError/SmartError,:server_rendering_error) and the streamed shell-error HTML.onShellErrorreuses the same already-appended error object, so it is not captured twice.rootErrorHandlers.ts— recoverable hydration mismatches (onRecoverableError, already attached by RoR in dev per Expose React 19 root error callbacks (onRecoverableError& co.) + hydration-mismatch debugging guide #3892) now also carry an owner-stack line in the branded dev log. For client render errors, RoR enrichesonCaughtError/onUncaughtErrorwith an additive[ReactOnRails] Render error … Owner stack:line only when the app registered its own handler — it does not auto-attach handlers just to log owner stacks (see decision log). Prefers React 19.2'serrorInfo.ownerStackwhen present, falling back to a livecaptureOwnerStack()for 19.1.Codex Decision Log
Non-blocking: how to gate "dev React build on the server".
typeof React.captureOwnerStack === 'function'rather than parsingReact.versionor inspectingNODE_ENV.captureOwnerStackis exported only from React's development build, and only on React ≥ 19.1. A single capability check therefore covers both the version requirement and the dev-vs-prod build requirement, and is automatically a no-op in production SSR bundles (which run React's production build). Asserted by tests.Non-blocking: do not auto-attach caught/uncaught handlers just to log owner stacks.
onCaughtError/onUncaughtError. RoR does not install these handlers on the app's behalf solely for owner-stack logging.Non-blocking: deliver SSR owner stacks by appending to
error.stackrather than a separate structured field.onErrorinstead of threading a dedicatedowner_stackfield through the JS→Ruby boundary intoSmartError.PrerenderError/SmartErrortoday; a separate field would require new end-to-end serialization that nothing currently populates. Appending is one mechanism that covers both the Rails output and the shell-error HTML and avoids dead code.SmartErrorsection if the renderingError payload starts carrying it as a structured field.Scope notes
captureOwnerStackcall — asserted by tests.Validation (local)
pnpm --filter react-on-rails run type-check✓ ·pnpm --filter react-on-rails-pro run type-check✓eslint(changed src) ✓ 0 errors ·prettier --check✓jest captureReactOwnerStack rootErrorCallbacks rootErrorCallbacksOwnerStack✓ (incl. a mocked positive-path test for the React ≥19.1 owner-stack line)codex review --base origin/mainrun to clean (caught and fixed: an e2e that couldn't run under thetestharness, a React 19.0 default-diagnostics regression, and an owner-stack duplication on pre-shell errors).OwnerStackThrower.tsxdummy demo is a documented manual-verification fixture (the dev-only branded line can't be asserted by the Playwright suite, which boots Rails intest); auto-registered viaauto_load_bundle, renders cleanly on page load.Note
Low Risk
Changes are gated to development builds and React ≥19.1 capability checks; production and ExecJS paths are unchanged no-ops, with additive logging only on error paths RoR already owns or when apps opt in via existing handlers.
Overview
Adds development-only owner-stack context to React on Rails error output using React 19.1+’s dev-only
captureOwnerStack, with a strict no-op on older React and in production.A new
captureReactOwnerStackhelper (exported from the package) wraps the API behind atypeof captureOwnerStack === 'function'guard and never throws. Pro streaming SSR appends the owner chain toerror.stacksynchronously insideonError/onShellError(idempotent via a marker), so it flows through existingrenderingErrormetadata to RailsPrerenderError/SmartErrorand shell-error HTML. Client dev logs gain owner stacks on recoverable hydration errors (onRecoverableError), and when the app already registersonCaughtError/onUncaughtError, an additive[ReactOnRails] Render error … Owner stack:line is logged without replacing React’s default diagnostics when no app handler exists.Tests cover the capture helper, the no-auto-wrap behavior, and mocked positive paths; the dummy app adds an
OwnerStackThrowermanual demo fixture.Reviewed by Cursor Bugbot for commit 922d606. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
onCaughtError/onUncaughtErrorhandlers now receive enriched error context with owner stack details.