-
-
Notifications
You must be signed in to change notification settings - Fork 6
Fix Resolution Search Tool Error Handling #340
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
The resolution search tool was failing silently when the AI model call failed, leaving the UI in a perpetual loading state. This commit adds a `try...catch` block around the `resolutionSearch` agent call within the `submit` server action in `app/actions.tsx`. - If the AI call is successful, the analysis is displayed as before. - If the AI call fails, the error is caught, logged to the console, and a user-friendly error message is streamed to the chat UI. - A `finally` block is used to ensure the `isGenerating` and `uiStream` values are correctly finalized, preventing the UI from getting stuck.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. |
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
The resolution search tool was failing silently when the AI model call failed, leaving the UI in a perpetual loading state. This commit adds a `try...catch` block around the `resolutionSearch` agent call within the `submit` server action in `app/actions.tsx`. - If the AI call is successful, the analysis is displayed as before. - If the AI call fails, the error is caught, logged to the console, and a user-friendly error message is streamed to the chat UI. - A `finally` block is used to ensure the `isGenerating` and `uiStream` values are correctly finalized, preventing the UI from getting stuck.
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
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.
Solid fix: the try/catch/finally prevents silent failures and guarantees UI finalization. The main improvement opportunity is consistency between success and error message shapes/types to simplify downstream handling. Minor observability and maintainability tweaks (error normalization, caching aiState.get()) would further improve robustness. No correctness issues found with the new control flow.
Additional notes (4)
-
Readability |
app/actions.tsx:104-108
Logging the raw error is good, but you can improve observability and make logs more consistent by normalizing the error and logging message + stack explicitly. This avoids odd objects when non-Error values are thrown and aids debugging. -
Maintainability |
app/actions.tsx:92-103
aiState.get()is invoked multiple times while building the same payload. While correct, it’s a bit wasteful and can be error-prone ifaiStatechanges between reads. Caching the current state once reduces duplication and potential racey reads. -
Readability |
app/actions.tsx:86-86
Using||will replace intentionally empty summaries with the fallback. If you only want to fall back whensummaryisnullorundefined, use the nullish coalescing operator. -
Maintainability |
app/actions.tsx:85-91
The blocks that create a streamable value and immediately mark it as done, then calluiStream.update(<BotMessage ... />), are duplicated in both success and error paths. This repetition increases maintenance overhead. Extracting a small helper would reduce duplication and make intent clearer (per the coding instructions to refactor repetitive code).
Summary of changes
- Wrapped the
resolutionSearch(messages)call in atry...catch...finallyto handle AI call failures. - On success: creates a
summaryStream, updatesuiStreamwith aBotMessage, and finalizesaiStatewith a new assistant message of typeresolution_search_resultcontainingJSON.stringify(analysisResult). - On failure: logs the error, streams a user-friendly error message to the UI, and finalizes
aiStatewith an assistant message of typeresponse. - In all cases: ensures
isGenerating.done(false)anduiStream.done()are called infinallyto prevent UI from getting stuck. - Kept the return object structure intact, returning
uiStream.value,isGenerating.value, andisCollapsed.value.
| id: nanoid(), | ||
| role: 'assistant', | ||
| content: errorMessage, | ||
| type: 'response' | ||
| } |
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.
The error-path message uses a different type (response) and unstructured string content, while the success path uses a domain-specific type (resolution_search_result) and structured JSON content. This inconsistency makes downstream handling harder (e.g., consumers may need special-case logic). Prefer a dedicated error type and structured payload for parity and future-proofing.
Suggestion
Consider using a domain-specific error type and a structured JSON payload in the error path to align with the success shape, for example:
const errorPayload = { message: errorMessage };
aiState.done({
...aiState.get(),
messages: [
...aiState.get().messages,
{
id: nanoid(),
role: 'assistant',
content: JSON.stringify(errorPayload),
type: 'resolution_search_error'
}
]
});Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
The resolution search tool was failing silently when the AI model call failed, leaving the UI in a perpetual loading state. This commit adds a `try...catch` block around the `resolutionSearch` agent call within the `submit` server action in `app/actions.tsx`. - If the AI call is successful, the analysis is displayed as before. - If the AI call fails, the error is caught, logged to the console, and a user-friendly error message is streamed to the chat UI. - A `finally` block is used to ensure the `isGenerating` and `uiStream` values are correctly finalized, preventing the UI from getting stuck.
User description
This commit fixes a bug in the resolution search tool where it would fail silently if the AI model call failed. I've added error handling to catch these errors, display a user-friendly message, and prevent the server from crashing. This makes the feature more robust and improves the user experience.
PR created automatically by Jules for task 14915266402971848909
PR Type
Bug fix, Error handling
Description
Added try-catch-finally block to resolution search agent call
Implemented error logging and user-friendly error messages
Fixed UI stuck in loading state on failures
Ensured proper cleanup of streaming values in all scenarios
Diagram Walkthrough
flowchart LR A["User submits query"] --> B["resolutionSearch call"] B --> C{"Success?"} C -- "Yes" --> D["Display analysis"] C -- "No" --> E["Catch error"] E --> F["Log error & show message"] D --> G["Finalize streams"] F --> GFile Walkthrough
actions.tsx
Add error handling to resolution search agentapp/actions.tsx
resolutionSearchagent call in try-catch-finally blockfailures