Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding fixing save chat session overwriting #6457

Merged
merged 3 commits into from
Dec 27, 2024
Merged

Adding fixing save chat session overwriting #6457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fcc1c2d
Adding fixing save chat session overwriting
arafatkatze Dec 24, 2024
2fb48f0
adding test
arafatkatze Dec 27, 2024
7205eb7
adding test
arafatkatze Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions vscode/src/chat/chat-view/ChatsController.ts
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked into the code yet but wonder if it would work if we dispose the chat before we deleting the chat?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great if we can have a test covering this behavior!

So I could possibly write some kind of "number of times called" kind of test on chatcontroller but I would ideally love to have webview based UI test(see slack)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if it would work if we dispose the chat before we deleting the chat?

While disposing before deleting seems like a good approach to prevent race conditions, it doesn't fully solve the problem. Here's why:

private async clearHistory(chatID?: string): Promise<void> {
    // ... other code ...

    // Even if we do this:
    this.disposeChat(chatID, true)  // Step 1: Dispose first
    await chatHistory.deleteChat(authStatus, chatID)  // Step 2: Then delete

    // The issue is that disposeChat() itself triggers a save operation:
    // 1. When disposing the chat controller
    // 2. When clearing/restarting the session

The race condition still exists because:
disposeChat() triggers a save operation as part of its cleanup
Then we immediately try to delete the chat history
These operations can overlap since saves are asynchronous
That's why the current solution uses a skipSave flag:

this.disposeChat(chatID, true, { skipSave: true }) // Prevents the save during disposal
await chatHistory.deleteChat(authStatus, chatID)

This ensures we're not trying to save and delete the same chat data simultaneously. Simply changing the order of operations (dispose then delete) isn't enough to prevent the race condition.

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
View file
Open in desktop
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>({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

// 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()
})
Loading