[ONCALL-313] fix: get path from type-box error#298
Conversation
WalkthroughThis pull request refactors error handling in the TypeBox validation pipeline. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/renderer/actions/local-sync/common-utils.ts (3)
113-126:headingis misleadingly named — it contains all errors, not a heading.
headingis assigned the fullnestedErrorMessagesarray (line 123), whileadditionalErrorsisnestedErrorMessages.slice(1)(line 124). This makesadditionalErrorsa strict subset ofheading, and the nameheadingsuggests a single summary or the first error, not the complete list.Consider renaming for clarity and removing the redundancy:
♻️ Suggested refactor
function formatValidationErrors(validator: TSchema, content: any): { error: any; - heading: ValidationErrorEntry[]; - additionalErrors: ValidationErrorEntry[]; + allErrors: ValidationErrorEntry[]; } { const allErrors = [...Value.Errors(validator, content)]; const nestedErrorMessages = collectVerboseErrors(allErrors); return { error: allErrors[0], - heading: nestedErrorMessages, - additionalErrors: nestedErrorMessages.slice(1), + allErrors: nestedErrorMessages, }; }Then in
parseRaw, derive the heading and additional errors from the single list:- const { error, heading, additionalErrors } = formatValidationErrors(validator, content); + const { error, allErrors } = formatValidationErrors(validator, content); const headingMessage = - heading.length > 0 - ? (heading[0].path ? `[${heading[0].path}] ` : "") + heading[0].message + allErrors.length > 0 + ? (allErrors[0].path ? `[${allErrors[0].path}] ` : "") + allErrors[0].message : "Validation failed"; - captureException(headingMessage, { extra: { additionalErrors, allErrors: heading } }); + captureException(headingMessage, { extra: { additionalErrors: allErrors.slice(1), allErrors } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/actions/local-sync/common-utils.ts` around lines 113 - 126, formatValidationErrors currently returns misleadingly named properties: it assigns the full nestedErrorMessages array to heading and a subset to additionalErrors; rename and restructure to avoid redundancy by returning a single errors list (e.g., allErrors or nestedErrorMessages) from formatValidationErrors instead of heading/additionalErrors, then update parseRaw to derive heading = errors[0] (or a computed summary) and additionalErrors = errors.slice(1). Update references to the old heading/additionalErrors properties accordingly (look for usages of formatValidationErrors, heading, additionalErrors) and ensure types/signatures for formatValidationErrors reflect the new single-list return value.
96-97: Simplify redundant path conditional.The ternary on line 96 checks if
nestedError.pathis notundefinedand not"", defaulting to"". This can be simplified.♻️ Suggested simplification
- const path = nestedError.path !== undefined && nestedError.path !== "" ? nestedError.path : ""; - messages.push({ path, message: nestedError.message }); + messages.push({ path: nestedError.path || "", message: nestedError.message });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/actions/local-sync/common-utils.ts` around lines 96 - 97, Replace the redundant ternary that computes path with a concise nullish-coalescing expression: set path to nestedError.path ?? "" (instead of nestedError.path !== undefined && nestedError.path !== "" ? nestedError.path : "") and then call messages.push({ path, message: nestedError.message }); this keeps behavior but simplifies the conditional.
145-149: UsecaptureMessageor wrap inErrorwhen reporting string messages to Sentry.
captureExceptionis designed forErrorobjects. Passing a plain string (line 149) causes Sentry to generate a synthetic stack trace, which is less accurate and may group poorly in the Sentry dashboard. Either wrap the message in anErroror usecaptureMessagefor string-based reports.♻️ Option A: Wrap in Error
- captureException(headingMessage, { extra: { additionalErrors, allErrors: heading } }); + captureException(new Error(headingMessage), { extra: { additionalErrors, allErrors: heading } });♻️ Option B: Use captureMessage (recommended)
-import { captureException } from "@sentry/browser"; +import { captureException, captureMessage } from "@sentry/browser";- captureException(headingMessage, { extra: { additionalErrors, allErrors: heading } }); + captureMessage(headingMessage, { level: "error", extra: { additionalErrors, allErrors: heading } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/actions/local-sync/common-utils.ts` around lines 145 - 149, The current code builds a string headingMessage and passes it to Sentry.captureException (headingMessage), which creates a synthetic stack; instead either call Sentry.captureMessage(headingMessage, { level: "error", extra: { additionalErrors, allErrors: heading } }) or wrap the string in an Error before calling captureException (e.g., captureException(new Error(headingMessage), { extra: { additionalErrors, allErrors: heading } })); update the call site where headingMessage is used and ensure the same extra payload (additionalErrors, allErrors: heading) is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/actions/local-sync/common-utils.ts`:
- Around line 113-126: formatValidationErrors currently returns misleadingly
named properties: it assigns the full nestedErrorMessages array to heading and a
subset to additionalErrors; rename and restructure to avoid redundancy by
returning a single errors list (e.g., allErrors or nestedErrorMessages) from
formatValidationErrors instead of heading/additionalErrors, then update parseRaw
to derive heading = errors[0] (or a computed summary) and additionalErrors =
errors.slice(1). Update references to the old heading/additionalErrors
properties accordingly (look for usages of formatValidationErrors, heading,
additionalErrors) and ensure types/signatures for formatValidationErrors reflect
the new single-list return value.
- Around line 96-97: Replace the redundant ternary that computes path with a
concise nullish-coalescing expression: set path to nestedError.path ?? ""
(instead of nestedError.path !== undefined && nestedError.path !== "" ?
nestedError.path : "") and then call messages.push({ path, message:
nestedError.message }); this keeps behavior but simplifies the conditional.
- Around line 145-149: The current code builds a string headingMessage and
passes it to Sentry.captureException (headingMessage), which creates a synthetic
stack; instead either call Sentry.captureMessage(headingMessage, { level:
"error", extra: { additionalErrors, allErrors: heading } }) or wrap the string
in an Error before calling captureException (e.g., captureException(new
Error(headingMessage), { extra: { additionalErrors, allErrors: heading } }));
update the call site where headingMessage is used and ensure the same extra
payload (additionalErrors, allErrors: heading) is preserved.
Summary by CodeRabbit