patch: 1.4.22 #1657
patch: 1.4.22 #1657
Conversation
WalkthroughThe PR adds version 1.4.22 to the changelog, refactors the example error handling code to use a centralized onError handler pattern with scoped configuration, and updates internal error type structures to support optional summary fields in a new ValueErrorWithSummary interface. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
example/a.ts (1)
25-39: Consider type narrowing for safer status code extraction.The status code logic handles cases where
error.statusmight be a string or invalid number. However, casting withNumber()on non-numeric strings yieldsNaN, which then falls back to500via|| 500. This works but could be clearer.🔎 Slightly cleaner approach
default: { - const statusCode = - "status" in error - ? (typeof error.status === "number" ? error.status : Number(error.status)) || 500 - : 500 + let statusCode = 500 + if ("status" in error && typeof error.status === "number") { + statusCode = error.status + } let errMsg = "An error occurred"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdexample/a.tssrc/error.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/error.ts (1)
src/index.ts (2)
mapValueError(8224-8224)error(3088-3128)
🔇 Additional comments (5)
CHANGELOG.md (1)
1-4: LGTM!The changelog entry accurately documents the bug fix for missing
summarytypes onValueError, which corresponds to the newValueErrorWithSummaryinterface introduced insrc/error.ts.src/error.ts (4)
47-49: LGTM!Formatting change improves readability.
159-161: LGTM!The new
ValueErrorWithSummaryinterface cleanly extendsValueErrorwith an optionalsummaryfield, addressing the type-safety issue mentioned in the changelog.
487-487: LGTM!Return type correctly updated to
ValueErrorWithSummary[]to reflect the mapped error structure.
599-601: LGTM!Safe access pattern correctly guards against undefined
valueErrorbefore callingmapValueError, avoiding potential runtime errors.
| case "VALIDATION": { | ||
| const allErrors = error.detail | ||
|
|
||
| return status(422, { | ||
| status: 422, | ||
| message: error.valueError?.message, | ||
| details: { | ||
| location: error.type, | ||
| rejected_value: error.value, | ||
| expected: error.expected, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
error.detail is a method, not a property—and allErrors is unused.
detail() in ValidationError is a method requiring parameters (message, allowUnsafeValidatorDetails). Line 7 assigns the method reference itself (not its result) to allErrors, and the variable is never used.
Either remove the dead code or call the method properly:
🔎 Option 1: Remove unused code
case "VALIDATION": {
- const allErrors = error.detail
-
return status(422, {🔎 Option 2: If you intended to get all errors
case "VALIDATION": {
- const allErrors = error.detail
+ const allErrors = error.all // Use the getter for all validation errors
return status(422, {
status: 422,
message: error.valueError?.message,
details: {
location: error.type,
rejected_value: error.value,
expected: error.expected,
+ errors: allErrors,
},
})
}📝 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.
| case "VALIDATION": { | |
| const allErrors = error.detail | |
| return status(422, { | |
| status: 422, | |
| message: error.valueError?.message, | |
| details: { | |
| location: error.type, | |
| rejected_value: error.value, | |
| expected: error.expected, | |
| }, | |
| }) | |
| case "VALIDATION": { | |
| return status(422, { | |
| status: 422, | |
| message: error.valueError?.message, | |
| details: { | |
| location: error.type, | |
| rejected_value: error.value, | |
| expected: error.expected, | |
| }, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In example/a.ts around lines 6 to 17, the code assigns error.detail (a method)
to allErrors and never uses it; either remove the unused assignment or call
detail(...) correctly and use its result. If you meant to discard it, delete the
const allErrors = error.detail line. If you meant to retrieve all validation
errors, call error.detail(message?, allowUnsafeValidatorDetails?) with
appropriate args, assign the returned value to a variable, and include that
value in the response (e.g., replace the current details payload or add an
errors field). Ensure the call matches the ValidationError.detail signature and
that the returned data is used instead of leaving a dead variable.
| export const mapValueError = (error: ValueError): ValueErrorWithSummary => { | ||
| if (!error) return error | ||
|
|
||
| let { message, path, value, type } = error |
There was a problem hiding this comment.
Type signature inconsistency with runtime guard.
The parameter is typed as ValueError (non-nullable), but line 164 guards against falsy values and returns error (potentially undefined). This creates a mismatch: the return type is ValueErrorWithSummary, but the function can actually return undefined.
Consider either:
- Updating the signature to reflect nullable input/output:
(error: ValueError | undefined): ValueErrorWithSummary | undefined - Removing the guard if callers are guaranteed to pass non-null values
🔎 Option 1: Reflect nullable types
-export const mapValueError = (error: ValueError): ValueErrorWithSummary => {
- if (!error) return error
+export const mapValueError = (error: ValueError | undefined): ValueErrorWithSummary | undefined => {
+ if (!error) return undefined🤖 Prompt for AI Agents
In src/error.ts around lines 163 to 166, the function mapValueError is typed to
accept a non-nullable ValueError but contains a runtime guard that returns the
(possibly undefined) error, causing a signature mismatch; update the function
signature to accept and return nullable types like (error: ValueError |
undefined): ValueErrorWithSummary | undefined, or if callers never pass
undefined, remove the guard and keep the non-nullable signature—choose one
approach and adjust the type annotations accordingly so runtime behavior matches
the TypeScript types.
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.