fix: preserve causes when wrapping render errors#3230
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 (1)
WalkthroughReworks server-render error handling: ChangesServer-render error wrapping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
Greptile SummaryThis PR fixes Confidence Score: 4/5Safe to merge — the logic change is minimal, correct, and well-tested; only P2 style notes remain. Only P2 findings: the TypeScript constructor cast is unconventional but functionally correct, and the '[object Object]' message behaviour is pre-existing. No logic or security issues introduced. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[convertToError called with unknown value] --> B{e instanceof Error?}
B -- Yes --> C[Return e unchanged]
B -- No --> D["new Error(String(e), { cause: e })"]
D --> E[Error.message = String representation]
D --> F[Error.cause = original thrown value]
Reviews (1): Last reviewed commit: "Add changelog for error cause preservati..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const ErrorWithCause = Error as new (message?: string, options?: { cause?: unknown }) => Error; | ||
| return new ErrorWithCause(String(e), { cause: e }); |
There was a problem hiding this comment.
Error constructor
The ErrorWithCause cast is a workaround for TypeScript targets older than ES2022, where ErrorOptions (and the cause option) is not included in the built-in Error constructor type. If the project's tsconfig.json already targets ES2022 or higher, the cast is unnecessary and new Error(String(e), { cause: e }) works directly. If it must stay, an inline assertion is slightly more idiomatic:
return new Error(String(e), { cause: e } as { cause?: unknown });| expect(error.message).toBe('[object Object]'); | ||
| expect((error as Error & { cause?: unknown }).cause).toBe(thrownValue); |
There was a problem hiding this comment.
The test asserts error.message equals '[object Object]', which confirms that throwing a plain object with a meaningful message property produces an unhelpful wrapper message. While this documents the current behaviour, it may be worth adding a comment that callers who need the original message should inspect error.cause rather than error.message. This is not a regression (the original new Error(String(e)) had the same limitation), but the new cause field makes the full value recoverable.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| const ErrorWithCause = Error as new (message?: string, options?: { cause?: unknown }) => Error; | ||
| return new ErrorWithCause(String(e), { cause: e }); |
There was a problem hiding this comment.
The type cast is a workaround for lib: es2020 in tsconfig.json not including the ErrorOptions type (which gained the cause field in ES2022). Node 22 and all modern environments support it at runtime, but TypeScript's type-checker doesn't know about it under the current lib setting.
A cleaner fix would be to add "es2022.error" to the lib array in the root tsconfig.json (a targeted addition that avoids pulling in all of ES2022):
| const ErrorWithCause = Error as new (message?: string, options?: { cause?: unknown }) => Error; | |
| return new ErrorWithCause(String(e), { cause: e }); | |
| const ErrorWithCause = Error as new (message?: string, options?: { cause?: unknown }) => Error; | |
| return new ErrorWithCause(String(e), { cause: e }); |
If you update tsconfig.json with "lib": ["dom", "es2020", "es2022.error"], both the cast here and the (error as Error & { cause?: unknown }) cast in the test can be removed, and TypeScript will validate the option natively.
Not a blocker — the cast is safe — but it leaves the type lie in place for the lifetime of the code.
|
|
||
| const error = convertToError(thrownValue); | ||
|
|
||
| expect(error).toBeInstanceOf(Error); |
There was a problem hiding this comment.
This assertion locks in the fact that throwing a plain object yields the unhelpful message "[object Object]". That's pre-existing behaviour (the old code did String(e) too), but now that cause is available it might be worth considering a better message for objects, e.g. attempting JSON.stringify with a fallback:
// e.g.
function safeStringify(e: unknown): string {
if (typeof e === 'object' && e !== null) {
try { return JSON.stringify(e); } catch { /* circular */ }
}
return String(e);
}Not required for this PR — the message is unchanged from before — but since the PR's stated goal is better downstream debugging, a more readable message would complement the cause improvement.
Code ReviewOverviewSmall, targeted fix to What works well
Issues / suggestions1. Type-cast workaround vs. tsconfig lib update (inline comment on
2.
3. The test correctly documents that a plain object thrown value produces the message 4. Minor: missing edge-case tests The following thrown values are common in practice and quick to add:
None of these change the implementation, but they document the contract and would catch a future regression if the branch is touched again. SummaryThe fix is correct and a clear improvement. The two actionable items before merge are:
The |
size-limit report 📦
|
Code ReviewOverall: This is a clean, well-scoped fix. The intent is correct — attaching the original thrown value as Correctness
Code Quality
Test CoverageThe new tests cover the two main paths (passthrough of Error, wrapping of plain object), but there are several common throw patterns not covered:
At minimum, a test for Summary
|
| return Object.prototype.toString.call(e); | ||
| } | ||
|
|
||
| return Object.prototype.toString.call(e); |
There was a problem hiding this comment.
The Object.prototype.toString.call(e) fallback appears twice — once in the catch block and once here. Both paths end up doing the same thing. Consider simplifying with a nullish coalesce, which removes the duplication and makes the intent clearer:
| return Object.prototype.toString.call(e); | |
| if (typeof e === 'object' && e !== null) { | |
| try { | |
| return JSON.stringify(e) ?? Object.prototype.toString.call(e); | |
| } catch { | |
| return Object.prototype.toString.call(e); | |
| } | |
| } |
| expect((error as Error & { cause?: unknown }).cause).toBe(thrownValue); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The two existing cases are good, but a few more would meaningfully increase coverage of real-world throws:
| }); | |
| it('wraps plain object thrown values with a readable message while preserving the original cause', () => { | |
| const thrownValue = { message: 'plain object failure' }; | |
| const error = convertToError(thrownValue); | |
| expect(error).toBeInstanceOf(Error); | |
| expect(error.message).toBe('{"message":"plain object failure"}'); | |
| expect((error as Error & { cause?: unknown }).cause).toBe(thrownValue); | |
| }); | |
| it('wraps a thrown string with the string as the message and as cause', () => { | |
| const error = convertToError('something went wrong'); | |
| expect(error).toBeInstanceOf(Error); | |
| expect(error.message).toBe('something went wrong'); | |
| expect((error as Error & { cause?: unknown }).cause).toBe('something went wrong'); | |
| }); | |
| it('wraps a thrown number', () => { | |
| const error = convertToError(42); | |
| expect(error).toBeInstanceOf(Error); | |
| expect(error.message).toBe('42'); | |
| }); | |
| it('wraps a circular-reference object without throwing (exercises the catch branch)', () => { | |
| const circular: Record<string, unknown> = {}; | |
| circular.self = circular; | |
| const error = convertToError(circular); | |
| expect(error).toBeInstanceOf(Error); | |
| // JSON.stringify throws on circular refs; falls back to [object Object] | |
| expect(error.message).toBe('[object Object]'); | |
| expect((error as Error & { cause?: unknown }).cause).toBe(circular); | |
| }); | |
| }); |
|
|
||
| if (typeof e === 'object' && e !== null) { | ||
| try { | ||
| return JSON.stringify(e) ?? Object.prototype.toString.call(e); |
There was a problem hiding this comment.
The ?? here is intentionally correct over ||: JSON.stringify can return undefined (not just throw) when an object's toJSON() method returns undefined, and an empty string "" from toJSON: () => '' should be kept as-is. Worth a short comment since the || vs ?? distinction is subtle in this context.
| return JSON.stringify(e) ?? Object.prototype.toString.call(e); | |
| return JSON.stringify(e) ?? Object.prototype.toString.call(e); // ?? not ||: JSON.stringify can return undefined without throwing (e.g. custom toJSON) |
| }; | ||
| } | ||
|
|
||
| function isCrossRealmError(e: unknown): e is { message?: unknown } { |
There was a problem hiding this comment.
Minor edge case: if a cross-realm error has no string message (e.g. new Error() where the message property is somehow non-string), isCrossRealmError returns true but the first if doesn't match. Execution falls through to JSON.stringify(e), which returns "{}" for typical Error objects (their message/stack properties are non-enumerable). The result would be [object Error] from the ?? fallback — not wrong, but potentially confusing.
Not a blocker — the original value is still preserved as cause — just worth knowing this is the fallback path.
|
|
||
| const message = stringifyThrownValue(e); | ||
| // tsconfig uses es2020 libs, which do not type ErrorOptions even though supported runtimes provide Error.cause. | ||
| const ErrorWithCause = Error as new (message?: string, options?: { cause?: unknown }) => Error; |
There was a problem hiding this comment.
The cast works, and the comment explains it well. An alternative that avoids the comment altogether is adding "ES2022" to the lib array in the root tsconfig.json — ErrorOptions was added in ES2022. That's a broader change, but it's purely additive for lib types and would let you write the constructor call directly. Happy to leave this as-is if keeping the tsconfig conservative is preferred.
| expect((error as Error & { cause?: unknown }).cause).toBe(thrownValue); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Good suite overall. Two thrown values that are easy to miss and worth adding:
null— passes throughString(null)→"null"(not the object branch, sincenull !== nullguard excludes it)undefined— passes throughString(undefined)→"undefined"
Both are real-world cases (e.g. throw null in legacy code). A quick pair of tests would lock in the expected message and confirm cause is set correctly for each.
Review: fix: preserve causes when wrapping render errorsOverall: solid change, looks good to merge with minor nits. This is a well-scoped improvement to SSR error normalization. The core fix — preserving the original thrown value via What works well
Observations (see inline comments for specifics)
Not a concernThe |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Review: fix: preserve causes when wrapping render errorsOverall: This is a clean, well-tested improvement. The logic is correct and the test suite covers the important edge cases. A few minor notes below. What the PR does
Concerns / suggestions1. 2. Uncovered branch in 3. Repeated type cast in tests 4. Behaviour change for plain objects (documented, strictly better) 5. VerdictNo blocking issues. Items 1 and 5 are worth a quick check; items 2 and 3 are optional polish. The core fix is correct. |
| }; | ||
| } | ||
|
|
||
| function isCrossRealmError(e: unknown): e is { message?: unknown } { |
There was a problem hiding this comment.
This check correctly handles cross-realm errors but will also match any plain object where a user has set [Symbol.toStringTag] = 'Error'. For that object, stringifyThrownValue would then try to read .message as a string instead of JSON-serialising it, producing a misleading or empty message string.
Extremely unlikely in SSR in practice, but a belt-and-suspenders guard could be:
| function isCrossRealmError(e: unknown): e is { message?: unknown } { | |
| function isCrossRealmError(e: unknown): e is { message?: unknown } { | |
| return typeof e === 'object' && e !== null && Object.prototype.toString.call(e) === '[object Error]'; | |
| } |
The explicit typeof e === 'object' && e !== null guard is redundant (toString handles those), but it makes the intent unmistakable and harmless.
|
|
||
| import { convertToError } from '../src/serverRenderUtils.ts'; | ||
|
|
||
| describe('serverRenderUtils', () => { |
There was a problem hiding this comment.
The cast (error as Error & { cause?: unknown }) is repeated 7 times across the suite. A file-level type alias would reduce noise:
| describe('serverRenderUtils', () => { | |
| type ErrorWithCause = Error & { cause?: unknown }; | |
| describe('serverRenderUtils', () => { |
Then each assertion becomes (error as ErrorWithCause).cause, which is easier to scan.
| expect((error as Error & { cause?: unknown }).cause).toBe(circular); | ||
| }); | ||
|
|
||
| it('wraps errors thrown from another JavaScript realm with their original message', () => { |
There was a problem hiding this comment.
This test covers the happy-path of the cross-realm branch, but the branch inside isCrossRealmError for when .message is not a string (Object.prototype.toString.call(e) fallback in stringifyThrownValue) is untested. A companion case would close that gap:
it('wraps a cross-realm error whose message is non-string using the toString tag', () => {
const thrownValue: unknown = new Script('const e = new Error(); e.message = 42; e').runInNewContext();
const error = convertToError(thrownValue);
expect(error).toBeInstanceOf(Error);
expect(error.message).toBe('[object Error]');
expect((error as ErrorWithCause).cause).toBe(thrownValue);
});
|
|
||
| const message = stringifyThrownValue(e); | ||
| // tsconfig uses es2020 libs, which do not type ErrorOptions even though supported runtimes provide Error.cause. | ||
| const ErrorWithCause = Error as new (message?: string, options?: { cause?: unknown }) => Error; |
There was a problem hiding this comment.
Error.cause is available from Node 16.9 / V8 9.3. The project has no engines field in its package.json, so there's no guarantee this option is honoured at runtime. On Node ≤ 16.8 the second argument is silently ignored and error.cause would be undefined, making the feature a no-op rather than an error.
If the minimum supported Node version is already ≥ 16.9 (which is likely, given the overall toolchain), adding "engines": { "node": ">=16.9.0" } to package.json would make that explicit and protect against accidental regressions. Alternatively, a narrower fix here avoids the cast entirely:
| const ErrorWithCause = Error as new (message?: string, options?: { cause?: unknown }) => Error; | |
| const error = new Error(message); | |
| // Error.cause available since Node 16.9 / ES2022; the tsconfig lib is es2020 so we assign directly. | |
| (error as Error & { cause?: unknown }).cause = e; | |
| return error; |
| } | ||
|
|
||
| function isCrossRealmError(e: unknown): e is { message?: unknown } { | ||
| return typeof e === 'object' && e !== null && Object.prototype.toString.call(e) === '[object Error]'; |
There was a problem hiding this comment.
Object.prototype.toString.call(e) reads Symbol.toStringTag, so any plain object with [Symbol.toStringTag]: 'Error' will pass this check and be treated as a cross-realm error. In practice that means such an object gets e.message (or '[object Error]') as the error message instead of a JSON representation, which could be surprising.
If this edge case is worth guarding, you could tighten the check:
| return typeof e === 'object' && e !== null && Object.prototype.toString.call(e) === '[object Error]'; | |
| function isCrossRealmError(e: unknown): e is { message?: unknown } { | |
| if (typeof e !== 'object' || e === null) return false; | |
| if (Symbol.toStringTag in e && (e as Record<symbol, unknown>)[Symbol.toStringTag] !== undefined) return false; | |
| return Object.prototype.toString.call(e) === '[object Error]'; | |
| } |
Alternatively, a comment acknowledging the limitation is fine since it's a very unlikely throw value in practice.
| if (typeof e === 'object' && e !== null) { | ||
| try { | ||
| // JSON.stringify can return undefined without throwing, for example when toJSON returns undefined. | ||
| return JSON.stringify(e) ?? Object.prototype.toString.call(e); |
There was a problem hiding this comment.
The comment correctly calls out the toJSON-returning-undefined edge case, but it isn't covered by the test suite. Consider adding a test:
it('wraps objects whose toJSON returns undefined', () => {
const obj = { toJSON: () => undefined };
const error = convertToError(obj);
expect(error.message).toBe('[object Object]');
expect((error as ErrorWithCause).cause).toBe(obj);
});
|
|
||
| const message = stringifyThrownValue(e); | ||
| // tsconfig uses es2020 libs, which do not type Error.cause even though supported runtimes provide it. | ||
| const error = new Error(message) as Error & { cause?: unknown }; |
There was a problem hiding this comment.
The cast works, but a cleaner long-term option is to add a global type augmentation rather than casting at each use site. Alternatively, if there's appetite to bump lib to es2022 in tsconfig, Error.cause becomes natively typed. The comment explains the rationale well either way — just flagging it in case the tsconfig target is ever raised.
Code ReviewOverviewThis PR fixes #1746 by improving Correctness ✅The logic is sound:
Potential Issues
Missing test for The code comment calls out Breaking-change riskThe Cursor Bugbot note is accurate: the error Test Coverage ✅The new
All meaningful branches are exercised. Only the Code Quality
VerdictApprove with minor suggestions. The core logic is correct, the test coverage is solid, and the change is a clear improvement. The three inline comments are all optional/minor — the |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-on-rails/src/serverRenderUtils.ts (2)
27-29: 💤 Low value
isCrossRealmErrorcan produce false positives for objects with a customSymbol.toStringTagIn ES2015+,
Object.prototype.toStringrespectsSymbol.toStringTag, so any plain object carrying[Symbol.toStringTag]: 'Error'will passisCrossRealmError. When that happens,stringifyThrownValueextracts only.message(line 33) and ignores all other properties thatJSON.stringifywould have captured. The original value is still accessible viaerror.cause, so no information is permanently lost — but the error message will be less descriptive than the JSON-serialised alternative.If tighter fidelity is needed, the guard can additionally verify the absence of
Symbol.toStringTagon the instance itself:🔎 Proposed tightening (optional)
function isCrossRealmError(e: unknown): e is { message?: unknown } { - return typeof e === 'object' && e !== null && Object.prototype.toString.call(e) === '[object Error]'; + return ( + typeof e === 'object' && + e !== null && + // Exclude objects that merely set Symbol.toStringTag to 'Error' + !Object.prototype.hasOwnProperty.call(e, Symbol.toStringTag) && + Object.prototype.toString.call(e) === '[object Error]' + ); }🤖 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/src/serverRenderUtils.ts` around lines 27 - 29, isCrossRealmError can misidentify plain objects with a custom Symbol.toStringTag as Error, causing stringifyThrownValue to drop useful properties; update isCrossRealmError (the type guard used by stringifyThrownValue) to also ensure the target does not have an own Symbol.toStringTag property (use Object.prototype.hasOwnProperty.call(e, Symbol.toStringTag) or equivalent) so only genuine cross-realm Error instances pass the guard and other objects fall back to JSON.stringify handling.
36-43: 💤 Low value
JSON.stringifysilently returns'{}'for objects with only non-serializable propertiesWhen
eis an object whose properties are all non-serializable (e.g.,{ fn: () => {} },{ sym: Symbol() }),JSON.stringify(e)returns the string'{}'. Because'{}'is truthy, the??fallback never fires and the error message becomes the unhelpful literal'{}'.The original value is preserved in
error.cause, so this is not a data-loss problem, but it does make the error message misleading. A cheap guard catches the common case:💡 Proposed improvement (optional)
if (typeof e === 'object' && e !== null) { try { - return JSON.stringify(e) ?? Object.prototype.toString.call(e); + const json = JSON.stringify(e); + // JSON.stringify returns undefined when toJSON() returns undefined, + // and '{}' when all properties are non-serializable — both are unhelpful. + if (!json || json === '{}') { + return Object.prototype.toString.call(e); + } + return json; } catch { return Object.prototype.toString.call(e); } }🤖 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/src/serverRenderUtils.ts` around lines 36 - 43, The current logic returns JSON.stringify(e) without checking for the misleading '{}' result for objects with only non-serializable properties; update the branch handling object errors so that you capture the JSON.stringify(e) result into a variable and, if it equals '{}' (or is undefined), fall back to Object.prototype.toString.call(e) instead of returning '{}'. Locate the block that calls JSON.stringify(e) (the branch testing typeof e === 'object' && e !== null) and replace the direct JSON.stringify(e) ?? ... return with a guarded flow: compute const serialized = JSON.stringify(e); if (!serialized || serialized === '{}') return Object.prototype.toString.call(e); else return serialized.
🤖 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 `@packages/react-on-rails/src/serverRenderUtils.ts`:
- Around line 27-29: isCrossRealmError can misidentify plain objects with a
custom Symbol.toStringTag as Error, causing stringifyThrownValue to drop useful
properties; update isCrossRealmError (the type guard used by
stringifyThrownValue) to also ensure the target does not have an own
Symbol.toStringTag property (use Object.prototype.hasOwnProperty.call(e,
Symbol.toStringTag) or equivalent) so only genuine cross-realm Error instances
pass the guard and other objects fall back to JSON.stringify handling.
- Around line 36-43: The current logic returns JSON.stringify(e) without
checking for the misleading '{}' result for objects with only non-serializable
properties; update the branch handling object errors so that you capture the
JSON.stringify(e) result into a variable and, if it equals '{}' (or is
undefined), fall back to Object.prototype.toString.call(e) instead of returning
'{}'. Locate the block that calls JSON.stringify(e) (the branch testing typeof e
=== 'object' && e !== null) and replace the direct JSON.stringify(e) ?? ...
return with a guarded flow: compute const serialized = JSON.stringify(e); if
(!serialized || serialized === '{}') return Object.prototype.toString.call(e);
else return serialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b6381c4-9814-48fb-b50c-844008ded89d
📒 Files selected for processing (2)
packages/react-on-rails/src/serverRenderUtils.tspackages/react-on-rails/tests/serverRenderUtils.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react-on-rails/tests/serverRenderUtils.test.ts
| }; | ||
| } | ||
|
|
||
| function isCrossRealmError(e: unknown): e is { message?: unknown } { |
There was a problem hiding this comment.
The name isCrossRealmError is slightly misleading — Object.prototype.toString.call(new Error()) also returns '[object Error]' for same-realm errors. In practice this is harmless because convertToError returns same-realm Error instances early via instanceof, but a future reader might wonder why a same-realm new Error() passes this guard.
A name like isErrorLike or hasErrorTag would be more accurate:
| function isCrossRealmError(e: unknown): e is { message?: unknown } { | |
| function isErrorLike(e: unknown): e is { message?: unknown } { |
| } | ||
|
|
||
| const message = stringifyThrownValue(e); | ||
| // tsconfig uses es2020 libs, which do not type Error.cause even though supported runtimes provide it. |
There was a problem hiding this comment.
The cast is fine for now, but this can be replaced once the root tsconfig moves to "lib": ["dom", "es2022"] — ErrorOptions with cause is included in the ES2022 type definitions. If that change lands later, this cast and the comment can be removed. Just flagging in case there's an appetite to bump the lib version in a follow-up.
| describe('serverRenderUtils', () => { | ||
| describe('convertToError', () => { | ||
| it('returns Error instances unchanged', () => { | ||
| const error = new Error('Already an error'); |
There was a problem hiding this comment.
Consider adding a test for built-in Error subclasses (e.g. TypeError, RangeError). Since instanceof Error is true for all subclasses, they're returned unchanged — but an explicit test guards against any future refactor that might accidentally re-wrap them:
it('returns Error subclass instances unchanged', () => {
const typeError = new TypeError('wrong type');
expect(convertToError(typeError)).toBe(typeError);
});
| expect(onError).toHaveBeenCalled(); | ||
| expect(onError).toHaveBeenCalledWith(new Error(expectedError)); | ||
| const [emittedError] = onError.mock.calls[0]; | ||
| expect(Object.prototype.toString.call(emittedError)).toBe('[object Error]'); |
There was a problem hiding this comment.
Using Object.prototype.toString.call here is intentionally defensive because the error may be created inside the VM context (a different JavaScript realm), making instanceof Error unreliable. The approach is correct — just leaving a note so it's clear this isn't accidental verbosity.
Code ReviewOverviewThis is a clean, focused fix for Issue 1746. The change preserves the original thrown value as What works well
IssuesMinor (naming clarity): Minor (test gap): No test for Informational (TypeScript cast): The SummaryThe implementation is correct and the test coverage is solid. The two minor points above are suggestions, not blockers. Ready to merge once you decide whether to address the naming nit. |
…y-adapter * origin/main: fix: support nested JavaScript package roots in doctor (#3220) docs: add Claude verify command (#3235) fix: serialize generated pack regeneration (#3231) docs: amend CHANGELOG entry for #3229 scope drift fix: validate selected JavaScript package manager fix: preserve causes when wrapping render errors (#3230) # Conflicts: # react_on_rails/spec/lib/react_on_rails/doctor_spec.rb
…y-patch-DrhDk * origin/main: docs: document legacy Webpacker compatibility shims (#3225) chore: sync address-review command and import prompt variant (#3271) feat: add REACT_ON_RAILS_BASE_PORT for concurrent worktree port management (#3142) feat: align renderer cache staging with bundle-hash layout (#3124) docs: document benchmark gate tuning plan (#3234) docs: document Gumroad RSC benchmark signal (#3233) docs: document migration proof artifact template (#3227) fix: support nested JavaScript package roots in doctor (#3220) docs: add Claude verify command (#3235) fix: serialize generated pack regeneration (#3231) docs: amend CHANGELOG entry for #3229 scope drift fix: validate selected JavaScript package manager fix: preserve causes when wrapping render errors (#3230) # Conflicts: # CHANGELOG.md
1 participant
Summary
causewhen server rendering wraps a non-Error valueFixes #1746
Test plan
Note
Medium Risk
Touches server-side render error handling, which could subtly change logged/returned error messages and how downstream code inspects errors, but is limited in scope and covered by new tests.
Overview
Improves SSR error wrapping to retain original throw context.
convertToErrornow stringifies non-Errorthrown values more defensively (including circular objects and cross-realm errors) and attaches the original thrown value aserror.causewhen wrapping.Adds focused test coverage for these cases in
serverRenderUtils.test.ts, and updates the Pro node-renderer RSC streaming test to assert emitted errors by message/type rather than strictnew Error(...)equality. Also records the change inCHANGELOG.md.Reviewed by Cursor Bugbot for commit 0edc319. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation