Don't reclassify an emptied chat session as never-used - #333629
Open
Ahmed Mahdy (abmahdy) wants to merge 3 commits into
Open
Don't reclassify an emptied chat session as never-used#333629Ahmed Mahdy (abmahdy) wants to merge 3 commits into
Ahmed Mahdy (abmahdy) wants to merge 3 commits into
Conversation
`isEmpty` keeps never-used sessions out of the history list, which filters on it in `getHistorySessionItems`. Removing every request from a session that did have them - restoring the checkpoint on the first request, for example - flipped the stored entry back to `isEmpty: true`, so the session was filtered out of history permanently even though its title and full transcript were still on disk. Before that final eviction it also flickered: the live-model path (`shouldBeInHistory`) applies no emptiness filter, while the history path skips sessions that are currently loaded, so the same session was listed while its model was in memory and gone once it was evicted. Keep `isEmpty` false once a session has held requests. Sessions that never had any are still reported empty and stay out of the list. Fixes microsoft#333623 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7544f2e8-0346-4710-91b2-4abf34955191
Contributor
There was a problem hiding this comment.
Pull request overview
Preserves previously used chat sessions in history after all requests are removed.
Changes:
- Retains non-empty metadata across all index-write paths.
- Updates mocks and adds regression coverage for emptied sessions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
chatSessionStore.ts |
Preserves prior non-empty state. |
chatSessionStore.test.ts |
Tests empty-session classification. |
mockChatModel.ts |
Exposes configurable mock requests. |
Suppressed comments (1)
src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts:906
isEmptyis optional on legacy index entries, so this falsy check treats an unknown value as proof that the session was previously used. When such an entry is next written with zero requests, it is forced tofalseand can be surfaced as a used session. Preserve only an explicitly recordedfalsevalue.
if (next.isEmpty && previous && !previous.isEmpty) {
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
karelyvalenzuela879-lang
left a comment
There was a problem hiding this comment.
abmahdy:fix/empty-session-hidden-from-history
Addressed inline review comment ids: 3897745274,3897745343,3897745401
The macOS/Browser job failed on 9 CSS/layout assertions in vs/sessions/contrib/chat/test/browser/chatView.test, a suite this PR does not touch. Linux/Browser and Windows/Browser both passed on the same commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7544f2e8-0346-4710-91b2-4abf34955191
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
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.
Fixes #333623
A local chat session whose requests are all removed — restoring the checkpoint on the first request, for example — keeps its title and full transcript on disk but disappears from the Chat Sessions list for good. Before it goes it also flickers in and out.
Why
Two paths decide session-list membership and they disagree about empty sessions.
getHistorySessionItemsskips sessions that are currently loaded, and excludes empty ones:shouldBeInHistory, which covers the loaded models, applies no emptiness filter at all:So the same session is visible while its model is in memory and hidden once it is evicted — the flicker — and permanently gone after the last eviction.
isEmptyitself is written correctly from the live model (getSessionMetadataSync), so this is not a stale index. The problem is that "has no requests right now" is conflated with "was never used", and only the latter is what the filter is for.The fix
Once a session has held requests, keep
isEmpty: false. Sessions that never had any are still reported as empty and stay out of the list, so never-used "New Chat" entries remain hidden exactly as before.The guard is applied at every site that writes an index entry —
writeSession,writeSessionMetadataOnly, and the synchronous shutdown flushupdateAndFlushIndexSync— so a shutdown flush can't undo it.Tests
Two tests in
chatSessionStore.test.ts:storeSessions marks a session with no requests as empty— pins the existing behaviour so the guard can't over-apply and start listing never-used sessions.storeSessions keeps a session non-empty after all of its requests are removed— the regression.MockChatModel.getRequests()returned a hard-coded[]while the class already carried arequestsfield; it now returns that field, which is what makes request counts expressible in tests.Negative control: with the guard disabled, the regression test fails with
true !== falsewhile the companion test still passes.Validation
npm run typecheck-client— no errors incontrib/chat. (21 pre-existing errors remain inplatform/agentHost, all from a stale local@github/copilot-sdk; untouched by this change.)test-nodesuite: 15059 passing. The wholeChatSessionStoresuite is green. The one failure isRequest Service > Kerberos lookup, an environment artifact on a corporate network, unrelated to this change.