fix(compact): surface real error in toast instead of swallowing it#950
Conversation
When Compact & Continue failed, the toast told users to "Check the tab for details" but no tab actually rendered details — the source tab's overlay only renders while state === 'summarizing', not on error. Meanwhile the underlying error was discarded by a bare catch in summarizeContext that re-threw a generic "Context summarization failed", hiding the real cause from both the toast and Sentry. Stop wrapping with a generic message: rethrow the original Error so "Agent X is not available", spawn ENAMETOOLONG, etc. propagate up. The Compaction Failed toast now reads "Failed to compact context: <actual cause>" so users can act on the message without hunting for a phantom details tab. closes #949
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPreserve original errors thrown during context summarization and surface per-tab error details in the Compact-and-Continue UI flow. The service rethrows original Error objects, the hook reads per-tab operationStore error details for toasts and builds unexpected-error messages from caught error messages, and tests assert the propagated IPC message. ChangesError Message Preservation for Context Summarization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 fixes a "silent failure" bug in Compact & Continue: the catch block in
Confidence Score: 4/5Safe to merge; the fix is targeted and correct, with one minor P2 nit around non-Error throw handling in the catch branch. All findings are P2. The core fix — re-throwing the original Error and reading it from the store for the toast — is correct and well-tested. The only nit is that String(err) in the .catch branch can produce noisy values like 'null' in the toast for exotic (non-Error, non-string) throws, which is an edge case unlikely to occur in practice. No files require special attention; the one nit is in useSummarizeAndContinue.ts line 425. Important Files Changed
Reviews (1): Last reviewed commit: "fix(compact): surface real error in toas..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const detail = err instanceof Error ? err.message : String(err); | ||
| notifyToast({ | ||
| type: 'error', | ||
| title: 'Compaction Failed', | ||
| message: 'An unexpected error occurred during compaction.', | ||
| message: detail | ||
| ? `An unexpected error occurred during compaction: ${detail}` | ||
| : 'An unexpected error occurred during compaction.', |
There was a problem hiding this comment.
String(err) may surface noise in the toast
String(err) on a non-Error thrown value produces strings like "[object Object]", "null", or "undefined" that would appear verbatim in the user-facing toast. The existing ternary only guards against an empty string, but String(null) → "null" is truthy, so those internal values still reach the UI. Consider typeof err === 'string' ? err : '' as the non-Error fallback so only genuine string throws pass through.
![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 the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts`:
- Around line 425-431: The fallback message currently uses String(err) which
renders plain objects as "[object Object]" and hides useful info; update the
error serialization in useSummarizeAndContinue.ts (the block that sets detail
from err and calls notifyToast) to safely stringify non-Error thrown values: if
err is an Error use err.message, else if typeof err === 'object' and err !==
null use a JSON.stringify(err) (with try/catch and a length cap) to extract
useful fields, otherwise fall back to String(err); ensure the resulting detail
is used in the notifyToast call so the toast shows meaningful error information.
🪄 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: 9b90bf1f-852f-417e-b20f-8514b098e2d0
📒 Files selected for processing (3)
src/__tests__/renderer/services/contextSummarizer.test.tssrc/renderer/hooks/agent/useSummarizeAndContinue.tssrc/renderer/services/contextSummarizer.ts
| const detail = err instanceof Error ? err.message : String(err); | ||
| notifyToast({ | ||
| type: 'error', | ||
| title: 'Compaction Failed', | ||
| message: 'An unexpected error occurred during compaction.', | ||
| message: detail | ||
| ? `An unexpected error occurred during compaction: ${detail}` | ||
| : 'An unexpected error occurred during compaction.', |
There was a problem hiding this comment.
Avoid stringifying arbitrary thrown values.
String(err) turns plain-object failures into [object Object], so the fallback toast can still hide the real cause.
♻️ Small improvement for unexpected-error messaging
- const detail = err instanceof Error ? err.message : String(err);
+ const detail =
+ err instanceof Error
+ ? err.message
+ : typeof err === 'object' && err !== null && 'message' in err
+ ? String((err as { message?: unknown }).message ?? '')
+ : typeof err === 'string'
+ ? err
+ : '';📝 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.
| const detail = err instanceof Error ? err.message : String(err); | |
| notifyToast({ | |
| type: 'error', | |
| title: 'Compaction Failed', | |
| message: 'An unexpected error occurred during compaction.', | |
| message: detail | |
| ? `An unexpected error occurred during compaction: ${detail}` | |
| : 'An unexpected error occurred during compaction.', | |
| const detail = | |
| err instanceof Error | |
| ? err.message | |
| : typeof err === 'object' && err !== null && 'message' in err | |
| ? String((err as { message?: unknown }).message ?? '') | |
| : typeof err === 'string' | |
| ? err | |
| : ''; | |
| notifyToast({ | |
| type: 'error', | |
| title: 'Compaction Failed', | |
| message: detail | |
| ? `An unexpected error occurred during compaction: ${detail}` | |
| : 'An unexpected error occurred during compaction.', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts` around lines 425 - 431,
The fallback message currently uses String(err) which renders plain objects as
"[object Object]" and hides useful info; update the error serialization in
useSummarizeAndContinue.ts (the block that sets detail from err and calls
notifyToast) to safely stringify non-Error thrown values: if err is an Error use
err.message, else if typeof err === 'object' and err !== null use a
JSON.stringify(err) (with try/catch and a length cap) to extract useful fields,
otherwise fall back to String(err); ensure the resulting detail is used in the
notifyToast call so the toast shows meaningful error information.
chr1syy
left a comment
chr1syy
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the diagnosis is spot-on. I confirmed both bugs are still present on main and rc (the bare catch in summarizeContext and the toast pointing at a SummarizeProgressOverlay that only renders while state === 'summarizing'). The fix matches the codebase's "don't silently swallow exceptions" guidance in CLAUDE.md.
A few things to clear before this is mergeable.
Blockers
1. Merge conflicts on both branches. mergeable: CONFLICTING. I reproduced locally:
- Against
main: conflict insrc/__tests__/renderer/services/contextSummarizer.test.tsbecause a new test'should handle an empty groomed response'was added directly above the renamed test. - Against
rc(if retargeted): conflicts inuseSummarizeAndContinue.ts—rcswappedconsole.error→logger.erroron the same line you touch — plus the async prompt-loading refactor that landed incontextSummarizer.ts.
Both sets of conflicts are mechanical.
2. lint-and-format CI failing — but not from this PR. prettier --check . flagged docs/releases.md and src/__tests__/renderer/hooks/useAgentListeners.test.ts. Both pass at current main HEAD and the PR's three touched files pass prettier cleanly. This is stale-base inheritance — a rebase + prettier --write . clears it.
3. String(err) fallback can leak "[object Object]" / "null" into the toast. Both Greptile and CodeRabbit flagged this and they're right. At useSummarizeAndContinue.ts:425:
const detail = err instanceof Error ? err.message : String(err);String({ foo: 1 }) → "[object Object]" (truthy, slips past the ternary). String(null) → "null" (also truthy). Suggested shape:
const detail =
err instanceof Error
? err.message
: typeof err === 'string'
? err
: typeof err === 'object' && err !== null && 'message' in err
? String((err as { message?: unknown }).message ?? '')
: '';Base branch — likely should be rc
The PR currently targets main, but recent bug-fix PRs all land on rc (#1066, #1067, #1043, #1023, #1021) so they ride the next release. Functionally the rewritten lines are identical on both branches — this is convention, not correctness.
Non-blocking observations
contextSummarizer.tsfallback has the same[object Object]risk.throw new Error(typeof err === 'string' ? err : 'Context summarization failed')silently drops detail for non-Error non-string throws. Worth tightening alongside the toast fix.- No length cap on
detail. A multi-line spawn error or longError.messageproduces an ugly oversized toast. A short cap (e.g. ~200 chars + ellipsis) would be friendlier — and matches how other Maestro toasts handle long error text. - Reaching into operationStore for the error.
useOperationStore.getState().summarizeStates.get(targetTabId)?.errorworks because the store update happens synchronously insidestartSummarizebefore it resolves null, but it couples the toast to store internals. Returning the error fromstartSummarizewould be cleaner. Fine to defer.
Verdict
Right idea, right files, real bug. Ship-blocking items are (1) rebase to clear conflicts + stale lint, (2) the String(err) hardening. I'd retarget to rc while you're rebasing.
Resolve semantic conflicts from PR #950's error-surfacing change: - contextSummarizer.test.ts: keep main's empty-groomed-response test plus the PR's renamed IPC-error-propagation test (identical assertion body). - useSummarizeHandler.test.ts: update two toast expectations to the new messages that surface the real error instead of 'Check the tab for details'. - contextSummarizer.integration.test.ts: assert the inner 'IPC failed' error propagates now that the wrapper string is gone.
Take RC's tree wholesale. main's 5 commits since the merge base are the test-coverage campaign (#1040, #1053) plus generated audit docs, all written against the pre-cutover file layout that RC has since restructured. Obsolete against RC's commits. The one production fix on main (#950, surface real compaction error) is applied as a follow-up.
Apply the production-code portion of main's #950 onto the v0.17.0 cutover. Kept RC's logger.error style and added the detail extraction so the Compaction Failed toast reads 'Failed to compact context: <cause>' instead of pointing users at a phantom details tab. contextSummarizer now rethrows the original Error rather than a generic wrapper. Updated RC's contextSummarizer test to assert the underlying error is surfaced ('IPC failed') instead of the old generic wrapper. closes #949
2 participants
Summary
When
Compact & Continuefailed, the toast told users to "Check the tab for details" — but no tab actually rendered details. The source tab'sSummarizeProgressOverlayonly renders whileisSummarizing(state'summarizing') is true, so the post-failure error state never reaches the screen. Meanwhile, a barecatchinsummarizeContextdiscarded the underlying error and re-threw a generic"Context summarization failed", hiding the real cause from both the toast and Sentry.This is the path #949 is hitting: the user sees
"Failed to compact context. Check the tab for details."and clicking the source tab shows nothing.Changes
src/renderer/services/contextSummarizer.ts— preserve the underlyingError(rethrow original) so callers see the real cause (Agent X is not available, spawnENAMETOOLONG, etc.) instead of a generic wrapper string.src/renderer/hooks/agent/useSummarizeAndContinue.ts— read the actual error out ofoperationStore(anderr.messagein the unexpected-throw branch) and put it in the toast message. The toast no longer references a phantom "tab for details".src/__tests__/renderer/services/contextSummarizer.test.ts— update the IPC-error test to assert the inner error propagates.Test plan
vitest run src/__tests__/renderer/services/contextSummarizer src/__tests__/renderer/hooks/useSummarizeHandler— all 39 tests passtsc -p tsconfig.lint.jsoncleaneslint src/renderer/services/contextSummarizer.ts src/renderer/hooks/agent/useSummarizeAndContinue.tscleanprettier --checkon touched files cleanFailed to compact context: <real reason>rather thanCheck the tab for detailscloses #949
Summary by CodeRabbit