fix(feedback): surface provider binary + error when Send Feedback spawn fails#1067
fix(feedback): surface provider binary + error when Send Feedback spawn fails#1067pedramamini wants to merge 1 commit into
Conversation
Send Feedback showed a generic "Something went wrong" when the auto-selected provider failed to start. This is common with multiple Codex installs where the wrong binary (a codex-multi-auth wrapper or a shadowed nvm install) gets picked and can't run. The failure message now names the resolved binary path and includes the provider's own output (auth errors, "command not found", etc.) so the cause is actionable instead of opaque. The detected-but-not-runnable and not-found paths also report the resolved binary. Refs #1064
📝 WalkthroughWalkthroughThis PR enhances the feedback conversation manager to provide actionable error messages when the AI provider fails. It computes and includes the resolved binary path in errors, captures and summarizes provider output, and ensures the UI displays diagnostic details instead of generic fallback messages. ChangesFeedback Provider Error Diagnostics
Sequence DiagramsequenceDiagram
participant FeedbackChatView
participant FeedbackConversationManager
participant MaestroAgent
FeedbackChatView->>FeedbackConversationManager: sendMessage()
FeedbackConversationManager->>FeedbackConversationManager: resolve binaryPath from agent.path or agent.command
alt Agent not runnable
FeedbackConversationManager->>FeedbackConversationManager: throw error with resolved binaryPath
FeedbackConversationManager-->>FeedbackChatView: rejected Promise
else Spawn process
FeedbackConversationManager->>MaestroAgent: spawn(binaryPath)
alt Process exits non-zero
MaestroAgent-->>FeedbackConversationManager: onExit(code)
FeedbackConversationManager->>FeedbackConversationManager: summarizeProcessFailure(output)
FeedbackConversationManager->>FeedbackConversationManager: build error with binaryPath + code + output tail
FeedbackConversationManager-->>FeedbackChatView: rejected Promise
else Success
MaestroAgent-->>FeedbackConversationManager: onData, onExit(0)
FeedbackConversationManager-->>FeedbackChatView: resolved Promise
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR improves the feedback chat UX when a provider binary fails to start. Previously, all spawn failures resolved with a generic "Something went wrong" string; now they surface the resolved binary path and the provider's own stderr output (via the new
Confidence Score: 4/5Safe to merge; the changes are narrow, additive error-surfacing improvements with good test coverage and no modifications to the happy path. The non-zero exit path now builds a Markdown string that embeds provider output raw inside a fenced code block. If the stderr contains triple-backtick sequences the block breaks, causing cosmetic rendering issues in the chat UI. Separately, src/renderer/services/feedbackConversation.ts — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as FeedbackChatView
participant Mgr as FeedbackConversationManager
participant Maestro as window.maestro
UI->>Mgr: sendMessage(text, history, callbacks)
Mgr->>Maestro: agents.get(agentType)
alt agent not found
Maestro-->>Mgr: null
Mgr-->>UI: throw Error("provider could not be found")
UI->>UI: catch → display error.message in chat
else agent not available
Maestro-->>Mgr: "{available: false, path, command}"
Mgr-->>UI: "throw Error("binary=binaryPath, not runnable")"
UI->>UI: catch → display error.message in chat
else agent available
Maestro-->>Mgr: "{available: true, path, command}"
Note over Mgr: binaryPath = agent.path || agent.command
Mgr->>Maestro: "process.spawn({command: binaryPath, ...})"
loop data events
Maestro-->>Mgr: onData(sid, chunk)
Mgr->>Mgr: "outputBuffer += chunk"
end
Maestro-->>Mgr: onExit(sid, code)
alt "code === 0"
Mgr->>Mgr: extractJsonFromOutput(outputBuffer)
Mgr-->>UI: resolve(parsedResponse)
UI->>UI: display response.message in chat
else "code !== 0"
Mgr->>Mgr: summarizeProcessFailure(outputBuffer)
Note over Mgr: strips ANSI, keeps last 8 lines up to 600 chars
Mgr-->>UI: "resolve({message: binary + output detail})"
UI->>UI: display error message with binary + output in chat
end
end
Reviews (1): Last reviewed commit: "fix(feedback): surface provider binary +..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // The binary Maestro resolved for this provider. Surfaced in every | ||
| // failure path below so the user can tell which install was used - | ||
| // critical when multiple Codex binaries (incl. wrappers) are present. | ||
| const binaryPath = agent.path || agent.command; |
There was a problem hiding this comment.
binaryPath can be undefined when both path and command are absent
agent.path || agent.command resolves to undefined if neither field is populated. The value flows into the spawn command field (likely a pre-existing crash) and, newly in this PR, into every error-message template — so users would see "binary to \"undefined\"" or "Binary: \undefined`". A narrow guard here would make the fallback explicit: agent.path || agent.command || this.agentType`.
| `The ${agent.name || this.agentType} provider exited with code ${code} before it could respond.\n\n` + | ||
| `**Binary:** \`${binaryPath}\`\n\n` + | ||
| (detail | ||
| ? `**Output:**\n\n\`\`\`\n${detail}\n\`\`\`` |
There was a problem hiding this comment.
detail is inserted verbatim into a ``` fence. If the provider's stderr contains a triple-backtick sequence (e.g. a CLI printing a Markdown-formatted help text, or a stack trace that quotes a code block), the fence is closed prematurely and MarkdownRenderer renders the remainder as raw Markdown. detail.replace(/```/g, '\\\\\')` before embedding it would prevent the malformed output.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/components/FeedbackChatView.tsx`:
- Around line 358-373: The catch block in FeedbackChatView.tsx that handles
errors from the managerRef.current.sendMessage flow should report the error to
Sentry before updating UI; import and call captureException (from
src/utils/sentry.ts) with the caught error at the top of that catch, then
continue to setMessages with the user-facing detail as currently
implemented—this mirrors the error reporting used in agentDetect and
startConversation and ensures exceptions are visible in production.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c678444-8175-4686-94e9-47aad149bc25
📒 Files selected for processing (3)
src/__tests__/renderer/services/feedbackConversation.test.tssrc/renderer/components/FeedbackChatView.tsxsrc/renderer/services/feedbackConversation.ts
| } catch (error) { | ||
| // Surface the real reason (e.g. provider binary not found / not | ||
| // runnable, with its resolved path) rather than a generic message, | ||
| // so multi-install / auth problems are actionable. | ||
| const detail = | ||
| error instanceof Error && error.message | ||
| ? error.message | ||
| : 'Something went wrong. Please try again.'; | ||
| setMessages((prev) => [ | ||
| ...prev, | ||
| { | ||
| role: 'assistant', | ||
| content: 'Something went wrong. Please try again.', | ||
| content: detail, | ||
| timestamp: Date.now(), | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
Report the caught error to Sentry before surfacing it.
This catch now renders error.message to the user but never reports it. Unexpected failures from managerRef.current.sendMessage are silently swallowed here, unlike the sibling handlers agentDetect (Line 201) and startConversation (Line 307), which both call captureException. Add the same reporting so production breakage stays visible while still showing the actionable detail.
🛡️ Proposed fix
} catch (error) {
+ // Report so unexpected provider/runtime failures stay visible in
+ // production, while still surfacing an actionable detail to the user.
+ captureException(error, { extra: { source: 'FeedbackChatView.sendMessage' } });
// Surface the real reason (e.g. provider binary not found / not
// runnable, with its resolved path) rather than a generic message,
// so multi-install / auth problems are actionable.
const detail =
error instanceof Error && error.message
? error.message
: 'Something went wrong. Please try again.';As per coding guidelines: "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry... Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts."
📝 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.
| } catch (error) { | |
| // Surface the real reason (e.g. provider binary not found / not | |
| // runnable, with its resolved path) rather than a generic message, | |
| // so multi-install / auth problems are actionable. | |
| const detail = | |
| error instanceof Error && error.message | |
| ? error.message | |
| : 'Something went wrong. Please try again.'; | |
| setMessages((prev) => [ | |
| ...prev, | |
| { | |
| role: 'assistant', | |
| content: 'Something went wrong. Please try again.', | |
| content: detail, | |
| timestamp: Date.now(), | |
| }, | |
| ]); | |
| } catch (error) { | |
| // Report so unexpected provider/runtime failures stay visible in | |
| // production, while still surfacing an actionable detail to the user. | |
| captureException(error, { extra: { source: 'FeedbackChatView.sendMessage' } }); | |
| // Surface the real reason (e.g. provider binary not found / not | |
| // runnable, with its resolved path) rather than a generic message, | |
| // so multi-install / auth problems are actionable. | |
| const detail = | |
| error instanceof Error && error.message | |
| ? error.message | |
| : 'Something went wrong. Please try again.'; | |
| setMessages((prev) => [ | |
| ...prev, | |
| { | |
| role: 'assistant', | |
| content: detail, | |
| timestamp: Date.now(), | |
| }, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/components/FeedbackChatView.tsx` around lines 358 - 373, The
catch block in FeedbackChatView.tsx that handles errors from the
managerRef.current.sendMessage flow should report the error to Sentry before
updating UI; import and call captureException (from src/utils/sentry.ts) with
the caught error at the top of that catch, then continue to setMessages with the
user-facing detail as currently implemented—this mirrors the error reporting
used in agentDetect and startConversation and ensures exceptions are visible in
production.
Review — PR #1067Reviewed end-to-end against issue #1064 and the bot feedback. Net read: clean, narrow, scope-disciplined regression fix. CI green, mergeable, base One concrete change requested, two nice-to-haves seconded from Greptile. 🟠 Should fix before merge — Sentry parity in the new
|
2 participants
closes #1064
Problem
Send Feedback auto-selects the first detected supported provider, then
FeedbackConversationManagerspawnsagent.path || agent.command. When the wrong Codex binary gets picked among multiple installs (acodex-multi-auth-codexwrapper, or a shadowed nvm install that isn't authenticated), the spawn exits non-zero and the user only saw a generic "Something went wrong processing your message" / "Something went wrong. Please try again." with no clue which binary ran or why it failed.Fix
Make every provider-startup failure in the feedback flow actionable:
command not found, stack traces), distilled by a newsummarizeProcessFailure()helper (strips ANSI, keeps the meaningful tail). Falls back to a multi-install hint when the binary produced no output.FeedbackChatView's catch block renders the real error message instead of swallowing it behind the generic string.This directly covers the issue's acceptance criterion: "If provider startup fails, the feedback UI shows the selected binary path and the underlying spawn/auth error instead of only 'Something went wrong.'"
Scope note
This PR addresses the error-surfacing half of the issue, which is independently shippable. The remaining acceptance criteria (feedback consuming a persisted/selected provider path when several Codex installs exist) depend on the multiple-install chooser in #1050 / #1048; once that lands, the feedback path should read the persisted selection. Flagged so that work explicitly covers this flow too.
Testing
src/__tests__/renderer/services/feedbackConversation.test.tscovering the multi-install Codex scenario: spawn uses the resolved binary, non-zero exit surfaces the path + provider output, empty-output failures still name the binary, and unavailable providers throw with the path (and are never spawned).FeedbackChatViewtests still pass (10 tests total).npm run lint(tsc, all configs) and ESLint clean on changed files.Branch base
Targets
rcbecause the inline-feedback chat feature lives onrc, notmain.Summary by CodeRabbit
Bug Fixes
Tests