fix: preserve causes when wrapping render errors#3230
fix: preserve causes when wrapping render errors#3230
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/8 reviews remaining, refill in 3 minutes and 4 seconds.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 📦
|
1 participant
Summary
causewhen server rendering wraps a non-Error valueFixes #1746
Test plan
Note
Low Risk
Small, localized change to SSR error wrapping plus new unit tests; behavior only affects error reporting paths.
Overview
Fixes SSR error normalization so
convertToErrorpreserves the original non-Errorthrown value viaError(..., { cause }), improving downstream debugging while leaving realErrorinstances untouched.Adds a focused unit test for
convertToErrorand documents the behavior change inCHANGELOG.md.Reviewed by Cursor Bugbot for commit e5d2867. Bugbot is set up for automated code reviews on this repo. Configure here.