fix(context): stabilize usage status updates - #95
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces a fresh usage-snapshot event system where ChangesUsage Snapshot Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ 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)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
==========================================
+ Coverage 69.89% 69.94% +0.05%
==========================================
Files 219 219
Lines 19610 19660 +50
==========================================
+ Hits 13706 13752 +46
- Misses 4738 4741 +3
- Partials 1166 1167 +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
🧹 Nitpick comments (1)
internal/assistant/context_post_response_auto_compaction_test.go (1)
110-122: 💤 Low valueLGTM!
Optional: Consider more descriptive test failure message.
The current failure message is sufficient, but you could optionally enhance it to indicate what was actually found (e.g., "found N snapshot events but all had ContextTokens == 0" or "found no snapshot events"). This would make test failures easier to diagnose, though the current message works fine for this helper.
💡 Optional enhancement example
func assertPostResponseUsageSnapshot(t *testing.T, events []assistant.StreamEvent) { t.Helper() + snapshotCount := 0 for _, event := range events { if event.Kind != assistant.StreamEventUsageSnapshot || event.Usage == nil { continue } + snapshotCount++ if event.Usage.ContextTokens > 0 { return } } - assert.Fail(t, "expected post-response auto-compaction usage snapshot") + if snapshotCount == 0 { + assert.Fail(t, "expected post-response auto-compaction usage snapshot, but found no snapshot events") + } else { + assert.Fail(t, "expected post-response auto-compaction usage snapshot with ContextTokens > 0, but all snapshots had zero tokens") + } }🤖 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/assistant/context_post_response_auto_compaction_test.go` around lines 110 - 122, The helper assertPostResponseUsageSnapshot currently fails with a generic message; update assertPostResponseUsageSnapshot to build and include more descriptive failure text (e.g., count of snapshot events seen and whether any had ContextTokens>0 or that no snapshot events were found) so test failures show either "found X snapshot events but all had ContextTokens == 0" or "found no snapshot events"; locate the function assertPostResponseUsageSnapshot and change the final assert.Fail call to include the constructed descriptive message.
🤖 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/compact_commands.go`:
- Around line 57-61: The code calls app.postCompactError when
app.runtime.ContextUsage(compactCtx, app.sessionID, app.cwd) fails, which
incorrectly marks a successful compaction as failed; instead, change the error
path to not call postCompactError—log or ignore the usage lookup error and
proceed with the normal success/completion flow for the compact operation
(retain compactID success handling and any UI update that signals compaction
completion). Locate the ContextUsage call and replace the
postCompactError(compactID, err) branch with a non-fatal handling (e.g.,
processLogger.Warn or similar) so the session remains marked compacted while
avoiding routing UI through the error recovery path.
---
Nitpick comments:
In `@internal/assistant/context_post_response_auto_compaction_test.go`:
- Around line 110-122: The helper assertPostResponseUsageSnapshot currently
fails with a generic message; update assertPostResponseUsageSnapshot to build
and include more descriptive failure text (e.g., count of snapshot events seen
and whether any had ContextTokens>0 or that no snapshot events were found) so
test failures show either "found X snapshot events but all had ContextTokens ==
0" or "found no snapshot events"; locate the function
assertPostResponseUsageSnapshot and change the final assert.Fail call to include
the constructed descriptive message.
🪄 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: 7881b5cf-9abb-4753-93dd-2088df624fe3
📒 Files selected for processing (17)
internal/assistant/context_auto_compaction.gointernal/assistant/context_overflow_compaction.gointernal/assistant/context_post_response_auto_compaction_test.gointernal/assistant/export_test.gointernal/assistant/runtime.gointernal/assistant/runtime_persist.gointernal/assistant/usage_events.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/compact_commands.gointernal/terminal/compact_commands_test.gointernal/terminal/context_contributors_extra_test.gointernal/terminal/prompt_send.gointernal/terminal/prompt_send_test.gointernal/terminal/token_usage.gointernal/terminal/token_usage_export_test.gointernal/terminal/token_usage_test.go
💤 Files with no reviewable changes (1)
- internal/terminal/prompt_send.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/terminal/async_events_test.go (1)
531-539: ⚡ Quick winConsider pre-initializing
tokenUsageto verify replacement semantics.The test name claims "replaces token usage," but the app starts with empty
tokenUsage. To confirm snapshot events replace (not merge) prior usage, consider adding:{ name: "usage snapshot replaces token usage", payload: asyncTestEventWithUsage(asyncEventPromptUsageSnapshot, usage), assert: func(t *testing.T, app *App) { t.Helper() + // Verify replacement: if it were a merge, ContextTokens would be 50 (25+25). assert.Equal(t, 25, app.tokenUsage.ContextTokens) }, canceledActive: false, },and in the test body (line 472):
app := newRenderTestApp(t) +if testCase.name == "usage snapshot replaces token usage" { + app.tokenUsage.ContextTokens = 25 // Pre-existing usage to be replaced +} if testCase.canceledActive {This would distinguish snapshot-replace from incremental-update behavior.
🤖 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 531 - 539, Pre-initialize the App's tokenUsage before running the "usage snapshot replaces token usage" test so the test verifies replacement (not merge): set a non-zero tokenUsage (e.g., App.tokenUsage.ContextTokens = 123) in the test setup, then call asyncTestEventWithUsage(asyncEventPromptUsageSnapshot, usage) and keep the assert that checks app.tokenUsage.ContextTokens == 25; this ensures the snapshot event produced by asyncEventPromptUsageSnapshot replaces the prior tokenUsage rather than merging with it.
🤖 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/async_events_test.go`:
- Around line 531-539: Pre-initialize the App's tokenUsage before running the
"usage snapshot replaces token usage" test so the test verifies replacement (not
merge): set a non-zero tokenUsage (e.g., App.tokenUsage.ContextTokens = 123) in
the test setup, then call asyncTestEventWithUsage(asyncEventPromptUsageSnapshot,
usage) and keep the assert that checks app.tokenUsage.ContextTokens == 25; this
ensures the snapshot event produced by asyncEventPromptUsageSnapshot replaces
the prior tokenUsage rather than merging with it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6590dd65-741b-4a5a-9b79-e77e17b40a98
📒 Files selected for processing (5)
internal/assistant/context_post_response_auto_compaction_test.gointernal/terminal/async_events_test.gointernal/terminal/compact_commands.gointernal/terminal/compact_commands_test.gointernal/terminal/prompt_send_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/assistant/context_post_response_auto_compaction_test.go
- internal/terminal/compact_commands.go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Keep the previous context usage visible while prompts are in flight, hide misleading zero-token window-only usage, and emit fresh usage snapshots after manual and automatic compaction so the footer reflects real context reductions.