fix(ai): report compacted model context - #312
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe chat budget meter now uses ChangesChat budget integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LeftPanel
participant useChatBudget
participant MainProcessChatBudget
LeftPanel->>useChatBudget: Pass project, session, and messages
useChatBudget->>useChatBudget: Compute renderer fallback
useChatBudget->>MainProcessChatBudget: Request native budget
MainProcessChatBudget-->>useChatBudget: Return usage or unavailable result
useChatBudget-->>LeftPanel: Return native usage or fallback budget
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
@coderabbitai review The earlier automated review was rate-limited; requesting a fresh substantive review now that the cooldown has elapsed. |
|
✅ Action performedReview finished.
|
![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/components/ai-edition/useChatBudget.test.ts`:
- Around line 27-105: Add tests in the useChatBudget suite covering the
no-session path, an undefined native budget result, and a rejected chatBudget
call. For each scenario, render useChatBudget with an appropriate setup and
assert usedTokens remains the renderer estimate, including handling the rejected
promise without replacing it with native usage.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe59ab43-48f5-4885-84be-fe1e5958669c
📒 Files selected for processing (4)
src/components/ai-edition/LeftPanel.tsxsrc/components/ai-edition/chatBudget.tssrc/components/ai-edition/useChatBudget.test.tssrc/components/ai-edition/useChatBudget.ts
|
Addressed the fresh review in |
|
Follow-up test harness fix is now on exact head |
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Reviewed at the PR head (76331d64). Verdict: needs work — the plumbing is sound, but the number it surfaces measures the wrong thing, and the fallback re-introduces exactly the quantity the PR exists to remove.
First, the good part: I mutation-tested the async contract on the real test file and it holds. Deleting the requestIdRef race guard fails a test; dropping fallback from the effect deps fails two. No NaN/Infinity path exists (budgetTokens > 0 ? used/budgetTokens : 0 guards both sides, Math.min(100, Math.round(...)) clamps). No unhandled rejection — requireNativeBridgeData is async, so a throw from getElectronBridge() becomes a rejection rather than escaping useEffect synchronously. No setState-after-unmount. ChatStripPanel has no early return above :1147, so swapping a plain call for a hook is safe. // @vitest-environment jsdom is on line 1. Both typecheck configs and biome clean, 6/6 tests pass. No hardcoded model IDs or context sizes added.
On the three test-fix commits: the churn wasn't an unsound contract, it was that the first two versions of the rejection test asserted usedTokens === 100, which is the fallback value both before and after the promise settles — so they couldn't fail. The final form (resolve 12, change messages, reject, assert the pill moves to the new fallback 200) has teeth. Worth noting the reason that test needs a second fetch to be meaningful is §2 below.
1. The budget measures modelMessages, but modelHistory is what gets sent
chat-service.ts:636 — getSessionContextUsage calls budgetSnapshot(modelMessages(session), …), the whole post-compaction transcript. What actually reaches the provider is modelHistory(session), windowed to MODEL_HISTORY_WINDOW = 20.
40-message session, no compaction: the model receives the last 20 messages, the pill reports all 40. Press Compact → modelMessages becomes summary + 20, so the pill roughly halves — while modelHistory goes from "last 20 raw" to "summary + last 19", essentially unchanged. The user watches the meter drop by half for a compaction that changed nothing about what gets sent. That's the same class of wrong number this PR was written to fix, just relocated to the main process.
The comment on that function ("Measured on what the model is given") is false above 20 messages.
// chat-service.ts:636 and :669
budgetSnapshot(modelHistory(session), budgetTokens)Secondary, same area: estimateHistoryTokens bills name.length + summary.length + 16 per tool call, but chat-service.ts:376 maps history to {role, content} only — tool-call text is counted and never sent.
2. The fallback substitutes a different quantity, not a degraded one
useChatBudget.ts:32,36 — on reject or undefined, the hook installs computeBudget(messages), computed over the full visible transcript.
User compacts a long session; pill reads 21% from native. The next messages change fires a new chat.budget call which rejects (main-process restart, session evicted from sessionsByProject, handler exception). The .catch writes the raw-transcript estimate into state → the pill jumps back to ~45%, with no indication anything failed, and stays there. The compaction visibly un-happens.
These two numbers aren't interchangeable and shouldn't be presented as if they were. On reject/null, keeping the last known native budget for that session — or rendering an explicit degraded state — would be honest. The current behaviour is the PR's own premise turned against it.
3. Caching the fallback into state makes the pill lag a round-trip
useChatBudget.ts:32,36,44 — writing fallback into nativeState makes the render guard nativeState?.sessionKey === sessionKey evaluate true, so the freshly memoized fallback of the current render is discarded.
In the null/reject regime the pill lags one IPC round-trip behind on every message: send a 4000-char message and the number doesn't move until the (failing) call settles; if the bridge hangs it never moves, even though the correct estimate is already computed and sitting in fallback. This is why test 4 needs waitFor instead of a synchronous assertion.
setNativeState(budget ? { sessionKey, budget } : null);
// .catch(() => setNativeState(null));
// deps: [projectId, sessionId, sessionKey, messages]That removes the stale closure and the redundant state together.
4. The error is discarded entirely
useChatBudget.ts:34 — .catch(() => {…}) with no log. Rename the chat.budget action, or throw inside getSessionContextUsage, and every desktop user permanently sees the wrong quantity with zero signal: no toast, no console, no test. Every other nativeBridgeClient call in LeftPanel.tsx (chatCompact, chatRewind, chatDeleteSession) at least toasts. Silence is defensible for a pill; silence plus a wrong number isn't. console.warn("chat.budget failed", err) would do.
5. The deleted policy note should come back
chatBudget.ts:1-7 removed this without re-establishing it anywhere in the renderer:
DEFAULT_CHAT_BUDGET_TOKENSis a made-up denominator … Read the pill as "the conversation is about this big", never as "you are this close to a limit", and do not let this number regain a decision.
The replacement ("the main process's model-message estimate, which understands compaction") reads as authoritative — while the denominator is the same invented 80,000 that chat-compaction.ts:5-14 spends 14 lines explaining was wrong by an order of magnitude for Gemini. A contributor reading only chatBudget.ts now sees no reason not to re-add the auto-compact-at-70% gate that was deliberately deleted. Please keep the "made-up denominator / never a decision" sentence and add the compaction clarification alongside it.
Nits
useChatBudget.ts:19,22,31,35,39-41— therequestIdRefcounter spreads one invariant across four sites.src/native/hooks/useCursorRecordingData.ts:17,49-51already does this job in this repo withlet cancelled = false+return () => { cancelled = true; }— two lines, impossible to get wrong. The cleanup half here is uncovered: deleting:39-41leaves all 6 tests green.useChatBudget.ts:5-8,17,44—NativeBudgetState+ the\0-joinedsessionKeyis ~10 of 45 lines of redundant state. Replacing:44withreturn nativeState?.budget ?? fallbackand deleting the interface leaves all 6 tests green. The flicker it nominally prevents isn't real: at a session switchmessagesis also still the previous session's, so the fallback it swaps in is equally stale — it trades one wrong number for another.useChatBudget.test.ts:60-69— "keeps the transcript estimate when native usage is unavailable" is near-vacuous:usedTokensis 100 before and aftermockResolvedValue(undefined)settles, so it can't distinguish "fell back correctly" from "the effect never ran". OnlytoHaveBeenCalledTimes(1)has teeth. Same defect you fixed in the sibling reject test across commits 2 and 3, left standing here.useChatBudget.ts:42— the refresh trigger is accidental:fallbackis in the deps to satisfyuseExhaustiveDependenciesand happens to double as the "messages changed, refetch" signal. If anyone memoizesfallbackby content instead of array identity — a natural-looking optimization — the pill silently stops updating and neither lint nor types catch it. Depending onmessagesexplicitly with a one-line comment would make the intent visible.LeftPanel.tsx:1144-1146— the comment says the hook "falls back to a renderer estimate in browser/shim … cases", butbrowserShim.ts:580-586resolves with its own raw-transcript estimate, so the shim path never reaches the fallback. In web builds this pays an IPC round-trip to recompute a number the renderer already had (both areceil(chars/4)/80_000).
§1 is the one that matters — everything else is polish on top. As it stands the PR's headline claim only holds for sessions under 20 messages.
2 participants
Summary
chatBudgetendpoint, which measures the messages actually sent to the modelThis is a follow-up to #238: preserving the complete transcript was correct, but it meant the transcript-based meter no longer reflected the compacted model payload.
Related issue
Related to #238
Type of change
Release impact
Desktop impact
Screenshots / video
Not included; the context pill retains its existing UI and now receives the correct post-compaction value.
Testing
npx vitest --run src/components/ai-edition/useChatBudget.test.ts(3 passed)npm run test(1,680 passed, 1 skipped across 141 files)npx tsc --noEmitnpx tsc -p tsconfig.test.json --noEmitnpx biome checkon all four changed filesnpm run docs:checknpm run i18n:checknpm run build-viteAuthored with Codex assistance and manually verified against the native
modelMessages(session)budget path.Summary by CodeRabbit
New Features
Bug Fixes
Tests