Skip to content

Commit

Permalink
Adding fixing save chat session overwriting (#6457)
Browse files Browse the repository at this point in the history
[Linear Issue

](https://linear.app/sourcegraph/issue/QA-216/jetbrains-clicking-on-delete-button-does-nothing-in-first-attempt-for)
### Problem
Previously, whenever a user deleted a chat, disposeChat would call
clearAndRestartSession(), which in turn triggered saveSession(). This
saved the just-deleted chat back into local storage, negating the
deletion.
## Solution
Introduce a skipSave boolean to conditionally skip the
clearAndRestartSession() flow during deletion. If skipSave is true
(e.g., for a deletion), clearAndRestartSession() is not called. This
prevents the race condition of re-saving an already-deleted chat.
## Why It Works
1. Avoid Conflicts – If the user explicitly wants to remove a chat, we
no longer re-create or re-save the session.
2. Maintain Clean State – Normal operations (skipSave = false) still
clear and restart sessions. Deletions (skipSave = true) skip that path
to maintain data consistency.
3. Prevent Race Conditions – The skip logic ensures no stale data
overwrites the freshly updated local storage.


Using the skipSave flag to conditionally skip clearAndRestartSession()
is safe because:
1. During normal operations (when skipSave is false), you want to clear
and restart the session to maintain a clean state
2. During deletion (when skipSave is true), you want to skip this step
because:
The chat is being deleted anyway
clearAndRestartSession() includes a saveSession() call which could race
with the deletion


3. There's no need to create a new session when we're removing the chat
The condition helps prevent potential issues where: 
- The deletion operation is trying to remove chat history
- While simultaneously, clearAndRestartSession() might try to save the
session
- Which could lead to inconsistent state or race conditions

So yes, using the skipSave flag to conditionally skip
clearAndRestartSession() during deletion is a good practice for
maintaining data consistency.

## Test plan
Run this code in Vscode/Jetbrains debugger and delete a chat from
history



<!-- Required. See
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles.
-->

## Changelog

<!-- OPTIONAL; info at
https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c
-->
  • Loading branch information
arafatkatze authored Dec 27, 2024
1 parent 0ce34ff commit 1234884
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 3 deletions.
12 changes: 9 additions & 3 deletions vscode/src/chat/chat-view/ChatsController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -446,8 +446,10 @@ export class ChatsController implements vscode.Disposable {
return
}

// For single chat deletion
await chatHistory.deleteChat(authStatus, chatID)
this.disposeChat(chatID, true)
// Don't save the session when disposing after delete
this.disposeChat(chatID, true, { skipSave: true })
}

/**
Expand Down Expand Up @@ -535,7 +537,11 @@ export class ChatsController implements vscode.Disposable {
})
}

private disposeChat(chatID: string, includePanel: boolean): void {
private disposeChat(
chatID: string,
includePanel: boolean,
options: { skipSave?: boolean } = {}
): void {
if (chatID === this.activeEditor?.sessionID) {
this.activeEditor = undefined
}
Expand All @@ -549,7 +555,7 @@ export class ChatsController implements vscode.Disposable {
removedProvider.dispose()
}

if (includePanel && chatID === this.panel?.sessionID) {
if (includePanel && chatID === this.panel?.sessionID && !options.skipSave) {
this.panel.clearAndRestartSession()
}
}
Expand Down
38 changes: 38 additions & 0 deletions vscode/test/e2e/chat-history.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,41 @@ test.extend<ExpectedV2Events>({
await expect(newHistoryItem).toBeVisible()
await newHistoryItem.click()
})

test.extend<ExpectedV2Events>({
// list of events we expect this test to log, add to this list as needed
expectedV2Events: [
'cody.extension:installed',
'cody.auth.login:clicked',
'cody.auth.login:firstEver',
'cody.auth.login.token:clicked',
'cody.auth:connected',
'cody.chat-question:submitted',
'cody.chat-question:executed',
'cody.chatResponse:noCode',
],
})('delete chat from sidebar history view', async ({ page, sidebar }) => {
await sidebarSignin(page, sidebar)

const sidebarChat = getChatSidebarPanel(page)

const sidebarTabHistoryButton = sidebarChat.getByTestId('tab-history')

// Ensure the chat view is ready before we start typing
await expect(sidebarTabHistoryButton).toBeVisible()

const chatInput = getChatInputs(sidebarChat).first()
await chatInput.fill('Hey')
await chatInput.press('Enter')

await sidebarTabHistoryButton.click()

const newHistoryItem = sidebarChat.getByRole('button', { name: 'Hey' })
await expect(newHistoryItem).toBeVisible()

const deleteButton = sidebarChat.getByRole('button', { name: 'Delete chat' })
await expect(deleteButton).toBeVisible()
await deleteButton.click()

await expect(newHistoryItem).not.toBeVisible()
})

0 comments on commit 1234884

Please sign in to comment.