fix(ai): replace summarized chat history - #238
Conversation
|
Warning Review limit reached
Next review available in: 3 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 (2)
📝 WalkthroughWalkthrough
ChangesChat compaction flow
Estimated code review effort: 2 (Simple) | ~10 minutes 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@electron/ai-edition/chat-compaction.ts`:
- Line 93: Update tests covering applyCompaction to assert the summaryMessageId
returned in the first message slot, including the persisted value read from
compacted[0].id, rather than checking only message content order.
In `@electron/ai-edition/chat-service.ts`:
- Around line 703-704: Update the compaction flow around inserted and
session.messages to compare the estimated token count of the proposed compacted
history with session.messages.slice(0, splitIndex) before assignment. Only
replace session.messages when the compacted result is strictly smaller;
otherwise leave the session unchanged, including when the summary is empty or
equal in size.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 64a69dc0-d195-4bae-9f34-2b885ea279bb
📒 Files selected for processing (3)
electron/ai-edition/chat-compaction.test.tselectron/ai-edition/chat-compaction.tselectron/ai-edition/chat-service.ts
89e07a8 to
545043d
Compare
1d048b0 to
e1c574c
Compare
e1c574c to
c7202ee
Compare
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
The bug is real and the diagnosis is exactly right: head and tail are complementary slices of the same array, so [...head, summaryMessage, ...tail] was the original history plus a summary — compaction grew the context it existed to shrink. That went unnoticed for a while, and compactionReducesHistory is a sensible way to keep it from coming back.
I can't take it as written, though, because the fix turns applyCompaction from a rewrite of the model's history into a destructive edit of the user's transcript. session.messages is the array the renderer displays via chatSelectSession, and sessions are in-memory only, so the dropped prefix is gone for the rest of the process. Details inline.
Second problem: history = session.messages.slice(-20) (chat-service.ts:336) can slice the summary straight back off, so compaction can pay for an LLM call and then never show the model the result.
Third: there are no chat-service tests for either behaviour change — chat-service.test.ts doesn't mention compaction at all, so tryCompactSession is entirely uncovered and the new guard is only exercised as a pure function.
What I'd like is the summary replacing what the model sees, without destroying the stored transcript — or, if it does destroy it, something that tells the user. Either shape works for me; say which you'd rather do and I'll review it.
Compaction returned `[...head, summaryMessage, ...tail]`, where head and tail were complementary slices of the same array — so the thing that exists to shrink the context handed back the entire history plus a summary, and every turn past the trip point paid for a summarizer call that made the problem slightly worse. Replacing the prefix fixes the growth, but `session.messages` is also the transcript the renderer displays, and it only refetches when the project or the active session changes. Deleting the older half there happens invisibly mid-turn and resurfaces later as half the conversation missing, with no marker, no explanation, and nothing to recover from — sessions live in memory only. So the two concerns are now separate: `session.messages` stays whole as the transcript of record, and compaction records a boundary plus a summary that is applied when the payload for the model is built. Compaction is a fact about the model's input, not about what the user wrote. That also settles which list the heuristic reads. It measures the payload, not the transcript — measuring the transcript would re-trip on every remaining turn of the session now that compaction never shrinks it. And the summary is pinned to the front of the sliding window instead of being left to `slice(-20)`: after a compaction the tail is roughly half the session, so at the token counts that trip compaction in the first place the window dropped exactly the summary we had just paid for. Finally, a summary that comes back no shorter than the messages it replaces is now remembered. Before, the guard rejected it and left the session untouched, so the next turn tripped the same heuristic and bought the same useless summary again, indefinitely. Automatic compaction stops after such a failure; the Compact button ignores the flag, because pressing it is an explicit request to spend a call, and a success clears it.
`planCompaction` measures the payload, not the transcript, and that choice was load-bearing but unpinned: reverting it to `shouldCompact(session.messages)` left all three compaction tests green. It is not a cosmetic difference. `shouldCompact` returns `splitIndex` as an index into the list it was handed, and that index is then applied to the payload. Measure the transcript — which never shrinks now, so it keeps tripping — and once the two lists have diverged the index runs off the end of the much shorter payload, so `payload.slice(0, splitIndex)` takes all of it, the message the user just sent included. The model is then asked to answer a question it was never shown, and it happens from the sixth long turn on. Asserting on the last entry of every payload rather than its length: the collapse leaves `[summary]`, so the tail is the summary instead of the user's message, and turn 0 is legitimately a one-message payload. Checking every turn rather than the last, because the collapse is intermittent and a spot-check on the final history walks past it.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Threads cleared. The transcript is the record and compaction is now a fact about the model payload, which is the separation this needed — the old shape deleted the conversation the user could see, from an in-memory store with nothing to recover from.
Approving. The one thing left open is cosmetic but visible: the renderer's "% context" pill reads the transcript, which no longer shrinks, so it stays pinned and the Compact button gives no feedback. Not a regression — it was pinned before too, since compaction never actually worked — but worth a follow-up.
3 participants
Summary
Related issue
No linked issue; found by validating the context-budget invariant.
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable; chat history management logic.
Testing
Summary by CodeRabbit