From 2d44395d5498e3fa7184b682bf0a6f3637f76c3f Mon Sep 17 00:00:00 2001 From: Ahmed Mahdy <44652453+abmahdy@users.noreply.github.com> Date: Mon, 31 Aug 2026 12:31:39 -0700 Subject: [PATCH 1/3] Don't reclassify an emptied chat session as never-used `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 #333623 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7544f2e8-0346-4710-91b2-4abf34955191 --- .../chat/common/model/chatSessionStore.ts | 25 ++++++++++--- .../common/model/chatSessionStore.test.ts | 36 ++++++++++++++++++- .../chat/test/common/model/mockChatModel.ts | 3 +- 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts b/src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts index db15ec62dadcb..463952d9f91cc 100644 --- a/src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts +++ b/src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts @@ -396,7 +396,7 @@ export class ChatSessionStore extends Disposable { // Write succeeded, update index const newMetadata = await getSessionMetadata(session); - index.entries[session.sessionId] = newMetadata; + index.entries[session.sessionId] = preserveNonEmpty(index.entries[session.sessionId], newMetadata); } catch (e) { this.reportError('sessionWrite', 'Error writing chat session', e); } @@ -413,7 +413,7 @@ export class ChatSessionStore extends Disposable { // TODO get this class on sessionResource const externalSessionId = session.sessionResource.toString(); - index.entries[externalSessionId] = await getSessionMetadata(session); + index.entries[externalSessionId] = preserveNonEmpty(index.entries[externalSessionId], await getSessionMetadata(session)); } catch (e) { this.reportError('sessionMetadataWrite', 'Error writing chat session metadata', e); } @@ -759,11 +759,11 @@ export class ChatSessionStore extends Disposable { updateAndFlushIndexSync(localSessions: ChatModel[], externalSessions: ChatModel[]): void { const index = this.internalGetIndex(); for (const session of localSessions) { - index.entries[session.sessionId] = getSessionMetadataSync(session); + index.entries[session.sessionId] = preserveNonEmpty(index.entries[session.sessionId], getSessionMetadataSync(session)); } for (const session of externalSessions) { const externalSessionId = session.sessionResource.toString(); - index.entries[externalSessionId] = getSessionMetadataSync(session); + index.entries[externalSessionId] = preserveNonEmpty(index.entries[externalSessionId], getSessionMetadataSync(session)); } try { this.storageService.store(ChatIndexStorageKey, index, this.getIndexStorageScope(), StorageTarget.MACHINE); @@ -893,6 +893,23 @@ function getSessionMetadataSync(session: ChatModel): IChatSessionEntryMetadata { }; } +/** + * `isEmpty` exists to keep never-used sessions out of the history list, which + * filters on it. Removing every request from a session that *did* have them — + * restoring the checkpoint on the first request, for example — must therefore + * not re-classify it as never-used: doing so drops it from the list permanently + * even though its title and full transcript are still on disk, and makes it + * flicker in and out beforehand, since the live-model path applies no such + * filter. + */ +function preserveNonEmpty(previous: IChatSessionEntryMetadata | undefined, next: IChatSessionEntryMetadata): IChatSessionEntryMetadata { + if (next.isEmpty && previous && !previous.isEmpty) { + next.isEmpty = false; + } + + return next; +} + async function getSessionMetadata(session: ChatModel | ISerializableChatData): Promise { if (session instanceof ChatModel) { const metadata = getSessionMetadataSync(session); diff --git a/src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts b/src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts index 1b6a12deff8b7..8a059dac65f9b 100644 --- a/src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts @@ -29,7 +29,7 @@ import { MockChatModel } from './mockChatModel.js'; import { IConfigurationService } from '../../../../../../platform/configuration/common/configuration.js'; import { TestConfigurationService } from '../../../../../../platform/configuration/test/common/testConfigurationService.js'; -function createMockChatModel(sessionResource: URI, options?: { customTitle?: string }): ChatModel { +function createMockChatModel(sessionResource: URI, options?: { customTitle?: string; requestCount?: number }): ChatModel { const sessionId = LocalChatSessionUri.parseLocalSessionId(sessionResource); if (!sessionId) { throw new Error('createMockChatModel requires a local session URI'); @@ -39,10 +39,18 @@ function createMockChatModel(sessionResource: URI, options?: { customTitle?: str if (options?.customTitle) { model.customTitle = options.customTitle; } + if (options?.requestCount) { + setRequestCount(model, options.requestCount); + } // Cast to ChatModel - the mock implements enough of the interface for testing return model as unknown as ChatModel; } +/** Only the request count matters for the index metadata under test. */ +function setRequestCount(model: MockChatModel, count: number): void { + model.requests = Array.from({ length: count }, () => ({} as MockChatModel['requests'][number])); +} + class MockWorkspaceEditingService extends Disposable implements Partial { private readonly _onDidEnterWorkspace = this._register(new Emitter()); readonly onDidEnterWorkspace = this._onDidEnterWorkspace.event; @@ -183,6 +191,32 @@ suite('ChatSessionStore', () => { assert.strictEqual(index['session-1'].title, 'My Custom Title'); }); + test('storeSessions marks a session with no requests as empty', async () => { + const store = createChatSessionStore(); + const model = testDisposables.add(createMockChatModel(LocalChatSessionUri.forSession('session-1'))); + + await store.storeSessions([model]); + + const index = await store.getIndex(); + assert.strictEqual(index['session-1'].isEmpty, true); + }); + + test('storeSessions keeps a session non-empty after all of its requests are removed', async () => { + const store = createChatSessionStore(); + const model = testDisposables.add(createMockChatModel(LocalChatSessionUri.forSession('session-1'), { requestCount: 2 })); + + await store.storeSessions([model]); + assert.strictEqual((await store.getIndex())['session-1'].isEmpty, false); + + // Restoring the checkpoint on the first request removes every request. + // The session still has its title and transcript on disk, so it must not + // be reclassified as never-used and filtered out of the history list. + setRequestCount(model as unknown as MockChatModel, 0); + await store.storeSessions([model]); + + assert.strictEqual((await store.getIndex())['session-1'].isEmpty, false); + }); + test('readSession returns stored session data', async () => { const store = createChatSessionStore(); const model = testDisposables.add(createMockChatModel(LocalChatSessionUri.forSession('session-1'))); diff --git a/src/vs/workbench/contrib/chat/test/common/model/mockChatModel.ts b/src/vs/workbench/contrib/chat/test/common/model/mockChatModel.ts index 12bb0f6268e73..8846bf87a3791 100644 --- a/src/vs/workbench/contrib/chat/test/common/model/mockChatModel.ts +++ b/src/vs/workbench/contrib/chat/test/common/model/mockChatModel.ts @@ -19,6 +19,7 @@ export class MockChatModel extends Disposable implements IChatModel { readonly timestamp = 0; readonly timing: IChatSessionTiming = { created: Date.now(), lastRequestStarted: undefined, lastRequestEnded: undefined }; readonly initialLocation = ChatAgentLocation.Chat; + readonly sessionTypeSelectionReason = undefined; readonly title = ''; readonly hasCustomTitle = false; customTitle: string | undefined; @@ -67,7 +68,7 @@ export class MockChatModel extends Disposable implements IChatModel { } startEditingSession(isGlobalEditingSession?: boolean, transferFromSession?: IChatEditingSession): void { } - getRequests(): IChatRequestModel[] { return []; } + getRequests(): IChatRequestModel[] { return this.requests; } setCheckpoint(requestId: string | undefined): void { } setRepoData(data: IExportableRepoData | undefined): void { this.repoData = data; } workingDirectory: URI | undefined = undefined; From 4b73a9f6bb805cdb8e7c0c27e877ff8e17aae9cf Mon Sep 17 00:00:00 2001 From: Ahmed Mahdy <44652453+abmahdy@users.noreply.github.com> Date: Mon, 31 Aug 2026 13:43:26 -0700 Subject: [PATCH 2/3] fix: address Copilot review comments (iteration 1) Addressed inline review comment ids: 3897745274,3897745343,3897745401 --- .../chat/common/model/chatSessionStore.ts | 9 ++------- .../common/model/chatSessionStore.test.ts | 20 ++++++++++++++++--- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts b/src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts index 463952d9f91cc..eb34862dc0fca 100644 --- a/src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts +++ b/src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts @@ -894,13 +894,8 @@ function getSessionMetadataSync(session: ChatModel): IChatSessionEntryMetadata { } /** - * `isEmpty` exists to keep never-used sessions out of the history list, which - * filters on it. Removing every request from a session that *did* have them — - * restoring the checkpoint on the first request, for example — must therefore - * not re-classify it as never-used: doing so drops it from the list permanently - * even though its title and full transcript are still on disk, and makes it - * flicker in and out beforehand, since the live-model path applies no such - * filter. + * Once a session's `isEmpty` flag has been cleared, keep it cleared even if a later + * metadata update would otherwise mark it empty again. */ function preserveNonEmpty(previous: IChatSessionEntryMetadata | undefined, next: IChatSessionEntryMetadata): IChatSessionEntryMetadata { if (next.isEmpty && previous && !previous.isEmpty) { diff --git a/src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts b/src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts index 8a059dac65f9b..80e6625ea0193 100644 --- a/src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts @@ -208,15 +208,29 @@ suite('ChatSessionStore', () => { await store.storeSessions([model]); assert.strictEqual((await store.getIndex())['session-1'].isEmpty, false); - // Restoring the checkpoint on the first request removes every request. - // The session still has its title and transcript on disk, so it must not - // be reclassified as never-used and filtered out of the history list. + // Simulate restoring the checkpoint on the first request, which removes every request. setRequestCount(model as unknown as MockChatModel, 0); await store.storeSessions([model]); assert.strictEqual((await store.getIndex())['session-1'].isEmpty, false); }); + test('updateAndFlushIndexSync persists preserved non-empty state to storage', async () => { + const store = createChatSessionStore(); + const model = testDisposables.add(createMockChatModel(LocalChatSessionUri.forSession('session-1'), { requestCount: 2 })); + + await store.storeSessions([model]); + assert.strictEqual((await store.getIndex())['session-1'].isEmpty, false); + + setRequestCount(model as unknown as MockChatModel, 0); + store.updateAndFlushIndexSync([model], []); + + // Recreate the store so the index is read back from storage rather than the in-memory cache. + const reloadedStore = createChatSessionStore(); + const index = await reloadedStore.getIndex(); + assert.strictEqual(index['session-1'].isEmpty, false); + }); + test('readSession returns stored session data', async () => { const store = createChatSessionStore(); const model = testDisposables.add(createMockChatModel(LocalChatSessionUri.forSession('session-1'))); From 9e8526f77adf054bf3cd4f2837257d8765e8025e Mon Sep 17 00:00:00 2001 From: Ahmed Mahdy <44652453+abmahdy@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:06:17 -0700 Subject: [PATCH 3/3] chore: re-trigger CI 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