-
-
Notifications
You must be signed in to change notification settings - Fork 179
feat: improve error handling #1313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughAdds a structured error model (FormattedError) and MCPJAM_PLATFORM_CODES, and updates formatErrorMessage to return FormattedError | null. ErrorBox props/signature expanded to include code, statusCode, isRetryable, isMCPJamPlatformError, and onRetry, and its UI was updated for platform-aware styling and an optional retry action. ChatTabV2 and PlaygroundMain now pass the additional error fields and relocate ErrorBox rendering. iteration-details removes MCPJam-specific branching and always renders errorDetails when present. No other public API signatures were changed. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/components/ChatTabV2.tsx (1)
574-586: MissingonRetryprop leaves retry capability dormant.The new error metadata flows through correctly, yet
onRetryis absent. Consequently, even whenisRetryableis true, no retry button materializes. If retry functionality is intended for this view, wire up an appropriate handler; otherwise, consider omittingisRetryableto avoid silent dead code.mcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx (1)
645-657: Consistent with ChatTabV2—sameonRetrygap.Props propagate correctly, but as with ChatTabV2,
onRetryremains unbound. The retry button inErrorBoxwill never appear here.
🤖 Fix all issues with AI agents
In `@mcpjam-inspector/client/src/components/chat-v2/error.tsx`:
- Line 1: Prettier formatting issues in error.tsx; run the project's formatter
(e.g. npm run prettier-fix) or apply Prettier to this file to fix style
violations, then commit the updated file; ensure the import line that references
CircleAlert, ChevronDown, ChevronRight, RefreshCw from "lucide-react" is
formatted according to the repo's Prettier settings (split/align imports and
trailing semicolons as required) so the pipeline passes.
In `@mcpjam-inspector/client/src/components/ChatTabV2.tsx`:
- Around line 579-582: Prettier formatting issues in the JSX props (the lines
passing code={errorMessage.code}, statusCode={errorMessage.statusCode},
isRetryable={errorMessage.isRetryable},
isMCPJamPlatformError={errorMessage.isMCPJamPlatformError} in ChatTabV2.tsx)
need fixing; run the project's formatter (npm run prettier-fix) or apply the
repo's Prettier rules to reformat the JSX block so it matches the configured
style, then commit the changed file.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/chat-v2/shared/chat-helpers.ts (1)
147-147: Consideras constfor tighter type inference.-const MCPJAM_PLATFORM_CODES = ['mcpjam_rate_limit', 'mcpjam_api_error', 'mcpjam_config_error']; +const MCPJAM_PLATFORM_CODES = ['mcpjam_rate_limit', 'mcpjam_api_error', 'mcpjam_config_error'] as const;This yields a readonly tuple type, enabling TypeScript to narrow
codevalues if needed downstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mcpjam-inspector/client/src/components/chat-v2/error.tsx`:
- Around line 91-94: The platform error note rendered when isPlatformError is
true is missing the second clause from the PR objective; update the JSX text
inside the component that renders the conditional block for isPlatformError (the
paragraph element currently containing "This is a temporary issue on our end.")
to the full copy "This is a temporary issue on our end, not something you did
wrong." so the displayed message matches the requested wording.
In `@mcpjam-inspector/client/src/components/chat-v2/shared/chat-helpers.ts`:
- Around line 169-186: The parsed JSON branch currently returns parsed.error,
parsed.message, and parsed.details directly which can be objects and render as
"[object Object]"; update the JSON-parsing block in chat-helpers.ts to normalize
those fields to strings (e.g., if typeof value === "string" use it, otherwise
JSON.stringify(value, null, 2) or fallback to a default message) before
returning, and ensure code, statusCode, isRetryable and isMCPJamPlatformError
(using MCPJAM_PLATFORM_CODES) are preserved as-is so the returned object
consistently provides string message/details for UI consumption.
| {isPlatformError && ( | ||
| <p className="text-xs opacity-75 mt-0.5"> | ||
| This is a temporary issue on our end. | ||
| </p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align platform note copy with the PR objective.
The objective calls for: “This is a temporary issue on our end, not something you did wrong.” The current copy omits the latter clause.
✏️ Suggested copy tweak
- <p className="text-xs opacity-75 mt-0.5">
- This is a temporary issue on our end.
- </p>
+ <p className="text-xs opacity-75 mt-0.5">
+ This is a temporary issue on our end, not something you did wrong.
+ </p>📝 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.
| {isPlatformError && ( | |
| <p className="text-xs opacity-75 mt-0.5"> | |
| This is a temporary issue on our end. | |
| </p> | |
| {isPlatformError && ( | |
| <p className="text-xs opacity-75 mt-0.5"> | |
| This is a temporary issue on our end, not something you did wrong. | |
| </p> |
🤖 Prompt for AI Agents
In `@mcpjam-inspector/client/src/components/chat-v2/error.tsx` around lines 91 -
94, The platform error note rendered when isPlatformError is true is missing the
second clause from the PR objective; update the JSX text inside the component
that renders the conditional block for isPlatformError (the paragraph element
currently containing "This is a temporary issue on our end.") to the full copy
"This is a temporary issue on our end, not something you did wrong." so the
displayed message matches the requested wording.
| // Try to parse as JSON to extract structured error | ||
| try { | ||
| const parsed = JSON.parse(errorString); | ||
| if (parsed && typeof parsed === "object" && parsed.message) { | ||
| if (parsed && typeof parsed === "object") { | ||
| // Handle structured error with code | ||
| const code = parsed.code; | ||
| const message = parsed.error || parsed.message || "An error occurred"; | ||
|
|
||
| return { | ||
| message: parsed.message, | ||
| message, | ||
| details: parsed.details, | ||
| code, | ||
| statusCode: parsed.statusCode, | ||
| isRetryable: parsed.isRetryable, | ||
| isMCPJamPlatformError: code | ||
| ? MCPJAM_PLATFORM_CODES.includes(code) | ||
| : false, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normalize parsed fields to avoid non-string UI output.
If parsed.error, parsed.message, or parsed.details are objects, the UI can end up showing "[object Object]" or lose JSON details. Consider normalizing types before returning.
💡 Suggested normalization
- const code = parsed.code;
- const message = parsed.error || parsed.message || "An error occurred";
+ const code = typeof parsed.code === "string" ? parsed.code : undefined;
+ const message =
+ typeof parsed.error === "string"
+ ? parsed.error
+ : typeof parsed.message === "string"
+ ? parsed.message
+ : "An error occurred";
+ const details =
+ typeof parsed.details === "string"
+ ? parsed.details
+ : parsed.details != null
+ ? JSON.stringify(parsed.details)
+ : undefined;
return {
message,
- details: parsed.details,
+ details,
code,
- statusCode: parsed.statusCode,
- isRetryable: parsed.isRetryable,
+ statusCode:
+ typeof parsed.statusCode === "number" ? parsed.statusCode : undefined,
+ isRetryable:
+ typeof parsed.isRetryable === "boolean" ? parsed.isRetryable : undefined,📝 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.
| // Try to parse as JSON to extract structured error | |
| try { | |
| const parsed = JSON.parse(errorString); | |
| if (parsed && typeof parsed === "object" && parsed.message) { | |
| if (parsed && typeof parsed === "object") { | |
| // Handle structured error with code | |
| const code = parsed.code; | |
| const message = parsed.error || parsed.message || "An error occurred"; | |
| return { | |
| message: parsed.message, | |
| message, | |
| details: parsed.details, | |
| code, | |
| statusCode: parsed.statusCode, | |
| isRetryable: parsed.isRetryable, | |
| isMCPJamPlatformError: code | |
| ? MCPJAM_PLATFORM_CODES.includes(code) | |
| : false, | |
| }; | |
| // Try to parse as JSON to extract structured error | |
| try { | |
| const parsed = JSON.parse(errorString); | |
| if (parsed && typeof parsed === "object") { | |
| // Handle structured error with code | |
| const code = typeof parsed.code === "string" ? parsed.code : undefined; | |
| const message = | |
| typeof parsed.error === "string" | |
| ? parsed.error | |
| : typeof parsed.message === "string" | |
| ? parsed.message | |
| : "An error occurred"; | |
| const details = | |
| typeof parsed.details === "string" | |
| ? parsed.details | |
| : parsed.details != null | |
| ? JSON.stringify(parsed.details) | |
| : undefined; | |
| return { | |
| message, | |
| details, | |
| code, | |
| statusCode: | |
| typeof parsed.statusCode === "number" ? parsed.statusCode : undefined, | |
| isRetryable: | |
| typeof parsed.isRetryable === "boolean" ? parsed.isRetryable : undefined, | |
| isMCPJamPlatformError: code | |
| ? MCPJAM_PLATFORM_CODES.includes(code) | |
| : false, |
🤖 Prompt for AI Agents
In `@mcpjam-inspector/client/src/components/chat-v2/shared/chat-helpers.ts` around
lines 169 - 186, The parsed JSON branch currently returns parsed.error,
parsed.message, and parsed.details directly which can be objects and render as
"[object Object]"; update the JSON-parsing block in chat-helpers.ts to normalize
those fields to strings (e.g., if typeof value === "string" use it, otherwise
JSON.stringify(value, null, 2) or fallback to a default message) before
returning, and ensure code, statusCode, isRetryable and isMCPJamPlatformError
(using MCPJAM_PLATFORM_CODES) are preserved as-is so the returned object
consistently provides string message/details for UI consumption.
Summary
code,statusCode,isRetryable,isMCPJamPlatformError)Test plan
🤖 Generated with Claude Code