feat(terminal): show auto-compaction indicator - #111
Conversation
|
Warning Review limit reached
More reviews will be available in 3 hours, 25 minutes, and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR replaces a single generic compaction stream event with three lifecycle-scoped events (start, done, error) and updates assistant emitters, overflow recovery, client/persistence mapping, terminal async routing and UI state, plus tests to assert the new lifecycle semantics. ChangesContext compaction lifecycle events
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
+ Coverage 77.41% 77.45% +0.03%
==========================================
Files 260 260
Lines 21377 21484 +107
==========================================
+ Hits 16550 16640 +90
- Misses 3619 3635 +16
- Partials 1208 1209 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/terminal/compact_commands.go (1)
94-121:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Auto-compaction done/error events are silently dropped.
handleCompactAsyncEventinterceptsasyncEventCompactDoneandasyncEventCompactError(lines 96-101) and returnstrue(handled), but thenapplyCompactDoneandapplyCompactErrorimmediately return early viaignoreCompactEventwhenactiveCompactionisnil(line 152).For auto-compaction flows (where
activeCompactionis never set), this prevents the events from reachinghandlePromptAsyncEvent→applyPromptContextEventinasync_events.go:341-373, which is the intended handler for auto-compaction lifecycle events. As a result, the UI never updatesapp.compacting, status messages, or transcript notices for auto-compaction.The asymmetry confirms the bug:
asyncEventCompactStartreturnsfalseat line 105 (allowing it to reachapplyPromptContextEvent), but done/error returntrue, blocking them.Fix: Check
ignoreCompactEventbefore returningtrue, allowing auto-compaction events to fall through to the prompt handler:🐛 Proposed fix
func (app *App) handleCompactAsyncEvent(ctx context.Context, payload *asyncEvent) bool { switch payload.Kind { case asyncEventCompactDone: + if app.ignoreCompactEvent(payload) { + return false + } app.applyCompactDone(ctx, payload) return true case asyncEventCompactError: + if app.ignoreCompactEvent(payload) { + return false + } app.applyCompactError(payload) return true🤖 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 `@internal/terminal/compact_commands.go` around lines 94 - 121, handleCompactAsyncEvent currently unconditionally claims asyncEventCompactDone and asyncEventCompactError as handled which blocks auto-compaction flows; update handleCompactAsyncEvent to check app.ignoreCompactEvent(payload) first and if it returns true return false so the event falls through to the prompt handler, otherwise call applyCompactDone/applyCompactError and return true; reference the functions handleCompactAsyncEvent, ignoreCompactEvent, applyCompactDone and applyCompactError when making the change.
🧹 Nitpick comments (1)
internal/terminal/async_events_test.go (1)
435-446: ⚡ Quick winConsider adding a test case for empty-text compaction done events.
The "prompt context done" test always passes non-empty text (
asyncTestCompactat line 436), but according tointernal/terminal/async_events.golines 349-351, the done handler only setsapp.statusMessagewhenpayload.Text != "". The edge case where a done event arrives with empty text is not validated.✅ Proposed test case for empty-text done event
Add this case to
promptLifecycleEventCases()after the existing "prompt context done" case:{ name: "prompt context done", payload: asyncTestEvent(asyncEventCompactDone, "", asyncTestCompact, 1), setup: func(app *App) { app.compacting = true }, assert: func(t *testing.T, app *App) { t.Helper() assert.False(t, app.compacting) assert.Equal(t, compactedStatusMessage, app.statusMessage) }, wantHandled: true, }, + { + name: "prompt context done with empty text", + payload: asyncTestEvent(asyncEventCompactDone, "", "", 1), + setup: func(app *App) { + app.compacting = true + app.statusMessage = "previous status" + }, + assert: func(t *testing.T, app *App) { + t.Helper() + assert.False(t, app.compacting) + assert.Equal(t, "previous status", app.statusMessage, "status should not change when text is empty") + }, + wantHandled: true, + }, { name: "prompt retry",🤖 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 `@internal/terminal/async_events_test.go` around lines 435 - 446, Add a test case in promptLifecycleEventCases() that sends an asyncTestEvent(asyncEventCompactDone, "", asyncTestCompact, 1) with an empty payload.Text and app.compacting = true in setup, then assert that app.compacting becomes false but app.statusMessage is NOT set to compactedStatusMessage (i.e., remains unchanged); this verifies the handler in async_events.go only updates statusMessage when payload.Text != "" and covers the empty-text edge case.
🤖 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 `@internal/terminal/async_events_test.go`:
- Around line 422-446: Add a new test case to promptLifecycleEventCases that
validates the error path for compaction: create a case named "prompt context
error" with payload asyncTestEvent(asyncEventCompactError, "", asyncTestCompact,
1), a setup that sets app.compacting = true, and an assert that checks
assert.False(t, app.compacting) and assert.Equal(t, compactedStatusMessage,
app.statusMessage); set wantHandled: true so applyPromptContextEvent handling of
asyncEventCompactError is verified.
---
Outside diff comments:
In `@internal/terminal/compact_commands.go`:
- Around line 94-121: handleCompactAsyncEvent currently unconditionally claims
asyncEventCompactDone and asyncEventCompactError as handled which blocks
auto-compaction flows; update handleCompactAsyncEvent to check
app.ignoreCompactEvent(payload) first and if it returns true return false so the
event falls through to the prompt handler, otherwise call
applyCompactDone/applyCompactError and return true; reference the functions
handleCompactAsyncEvent, ignoreCompactEvent, applyCompactDone and
applyCompactError when making the change.
---
Nitpick comments:
In `@internal/terminal/async_events_test.go`:
- Around line 435-446: Add a test case in promptLifecycleEventCases() that sends
an asyncTestEvent(asyncEventCompactDone, "", asyncTestCompact, 1) with an empty
payload.Text and app.compacting = true in setup, then assert that app.compacting
becomes false but app.statusMessage is NOT set to compactedStatusMessage (i.e.,
remains unchanged); this verifies the handler in async_events.go only updates
statusMessage when payload.Text != "" and covers the empty-text edge case.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f649c159-aeff-44d7-9ec6-b247af7b9ed6
📒 Files selected for processing (13)
internal/assistant/client_adapter.gointernal/assistant/context_auto_compaction.gointernal/assistant/context_auto_compaction_internal_extra_test.gointernal/assistant/context_auto_compaction_test.gointernal/assistant/context_overflow_compaction.gointernal/assistant/context_overflow_compaction_test.gointernal/assistant/context_post_response_auto_compaction_test.gointernal/assistant/runtime.gointernal/assistant/runtime_persist.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/async_events_test.gointernal/terminal/compact_commands.go
5aec7aa to
5523226
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/assistant/context_auto_compaction.go`:
- Around line 172-177: Move the runtime.emitContextCompactionEvent call with
StreamEventContextCompactionDone/autoCompactionMessage so it runs only after the
rebuilt request has been reconstructed and the budget re-validation succeeds;
specifically, remove the early emit before the rebuild/validation block and
place it after the rebuild (the code that constructs the rebuilt request) and
after the call that re-validates the budget (the validate/revalidateBudget
logic), mirroring the overflow path so the "done" event is emitted only on
successful rebuild+validation.
In `@internal/assistant/context_overflow_compaction.go`:
- Around line 104-108: The done event is reporting the post-compaction budget
(recoveredBuild.Budget) but should report the original pre-compaction estimated
budget; update the call to runtime.emitContextCompactionEvent that builds the
compaction message to pass input.build.Budget instead of recoveredBuild.Budget
so compactionMessage("context auto-compacted after provider overflow",
input.build.Budget, recoveredEntry) is used (same
runtime.emitContextCompactionEvent and parameters: ctx,
input.preparation.onEvent, StreamEventContextCompactionDone,
compactionMessage(...)).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f9c45ef-75f7-40ef-b196-e6ebe98c180e
📒 Files selected for processing (13)
internal/assistant/client_adapter.gointernal/assistant/context_auto_compaction.gointernal/assistant/context_auto_compaction_internal_extra_test.gointernal/assistant/context_auto_compaction_test.gointernal/assistant/context_overflow_compaction.gointernal/assistant/context_overflow_compaction_test.gointernal/assistant/context_post_response_auto_compaction_test.gointernal/assistant/runtime.gointernal/assistant/runtime_persist.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/async_events_test.gointernal/terminal/compact_commands.go
✅ Files skipped from review due to trivial changes (1)
- internal/assistant/client_adapter.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/terminal/app.go
- internal/assistant/runtime.go
- internal/assistant/context_auto_compaction_internal_extra_test.go
- internal/assistant/runtime_persist.go
- internal/assistant/context_post_response_auto_compaction_test.go
- internal/assistant/context_auto_compaction_test.go
- internal/assistant/context_overflow_compaction_test.go
- internal/terminal/async_events.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/terminal/compact_commands_test.go (1)
529-529: 💤 Low valueConsider renaming test case for consistency.
The test case name
"completion event"breaks the naming pattern established by the other cases ("start"and"error"). Consider renaming it to"done"to match the event kind suffix and maintain consistency.✨ Proposed naming fix
- {name: "completion event", kind: asyncEventCompactDone}, + {name: "done", kind: asyncEventCompactDone},🤖 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 `@internal/terminal/compact_commands_test.go` at line 529, Rename the test case string "completion event" to match the existing naming pattern by using "done" so it aligns with the event kind suffix; update the test table entry where the case is defined (the entry with name: "completion event", kind: asyncEventCompactDone) to name: "done" to keep consistency with the other cases ("start" and "error").
🤖 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.
Nitpick comments:
In `@internal/terminal/compact_commands_test.go`:
- Line 529: Rename the test case string "completion event" to match the existing
naming pattern by using "done" so it aligns with the event kind suffix; update
the test table entry where the case is defined (the entry with name: "completion
event", kind: asyncEventCompactDone) to name: "done" to keep consistency with
the other cases ("start" and "error").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b9654be-7daa-4082-b717-eba51087b001
📒 Files selected for processing (4)
internal/assistant/context_auto_compaction.gointernal/assistant/context_overflow_compaction.gointernal/terminal/async_events_test.gointernal/terminal/compact_commands_test.go
|
1 participant
Summary
Validation