Skip to content

Commit

Permalink
remove code for old chat history migrations
Browse files Browse the repository at this point in the history 
The migrations in #2473, #2695, and #2665 have been in the code for one major release and more than a month. We can remove them to simplify the code now.

Also remove other unused code.
  • Loading branch information
@sqs
sqs committed Mar 5, 2024
1 parent 16c4db3 commit a060422
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 129 deletions.
20 changes: 7 additions & 13 deletions vscode/src/services/LocalStorageProvider.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import assert from 'assert'

import { beforeEach, describe, it } from 'vitest'
import { beforeEach, describe, expect, it } from 'vitest'
import type * as vscode from 'vscode'

import type { AuthStatus } from '../chat/protocol'

import { URI } from 'vscode-uri'
import { localStorage } from './LocalStorageProvider'

describe('LocalStorageProvider', () => {
Expand All @@ -22,21 +21,16 @@ describe('LocalStorageProvider', () => {
localStorageData = {}
})

it('converts chat history without context files upon loading', async () => {
it('sets and gets chat history', async () => {
await localStorage.setChatHistory(DUMMY_AUTH_STATUS, {
chat: { a: null as any },
input: ['a', 'b', 'c'] as any, // API expects new format so cast any.
input: [{ inputText: 'a', inputContextFiles: [{ type: 'file', uri: URI.file('a') }] }],
})

const loadedHistory = localStorage.getChatHistory(DUMMY_AUTH_STATUS)
assert.deepStrictEqual(loadedHistory, {
chat: { a: null },
input: [
// Expect new format with context files.
{ inputText: 'a', inputContextFiles: [] },
{ inputText: 'b', inputContextFiles: [] },
{ inputText: 'c', inputContextFiles: [] },
],
expect(loadedHistory).toEqual({
chat: { a: null as any },
input: [{ inputText: 'a', inputContextFiles: [{ type: 'file', uri: URI.file('a') }] }],
})
})
})
Expand Down
120 changes: 4 additions & 116 deletions vscode/src/services/LocalStorageProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,11 @@ import { type AuthStatus, isSourcegraphToken } from '../chat/protocol'
type ChatHistoryKey = `${string}-${string}`
type AccountKeyedChatHistory = {
[key: ChatHistoryKey]: PersistedUserLocalHistory
} & {
// For backward compatibility, we do not want to delete the `chat` and `input` keys.
// As otherwise, downgrading to a prior version would completely block the startup
// as the client would throw.
//
// TODO: This can be removed in a future version
chat: ChatHistory
input: []
}

/**
* Persisted chat history may contain string inputs without context
* that we must migrate during loading.
*/
interface PersistedUserLocalHistory {
chat: ChatHistory
input: (ChatInputHistory | string)[]
input: ChatInputHistory[]
}

class LocalStorage {
Expand All @@ -33,7 +21,6 @@ class LocalStorage {
public readonly ANONYMOUS_USER_ID_KEY = 'sourcegraphAnonymousUid'
public readonly LAST_USED_ENDPOINT = 'SOURCEGRAPH_CODY_ENDPOINT'
protected readonly CODY_ENDPOINT_HISTORY = 'SOURCEGRAPH_CODY_ENDPOINT_HISTORY'
protected readonly KEY_LAST_USED_RECIPES = 'SOURCEGRAPH_CODY_LAST_USED_RECIPE_NAMES'

/**
* Should be set on extension activation via `localStorage.setStorage(context.globalState)`
Expand Down Expand Up @@ -90,10 +77,6 @@ class LocalStorage {
return this.storage.get<string[] | null>(this.CODY_ENDPOINT_HISTORY, null)
}

public async deleteEndpointHistory(): Promise<void> {
await this.storage.update(this.CODY_ENDPOINT_HISTORY, null)
}

private async addEndpointHistory(endpoint: string): Promise<void> {
// Do not save sourcegraph tokens as endpoint
if (isSourcegraphToken(endpoint)) {
Expand All @@ -108,59 +91,9 @@ class LocalStorage {
}

public getChatHistory(authStatus: AuthStatus): UserLocalHistory {
let history = this.storage.get<AccountKeyedChatHistory | PersistedUserLocalHistory | null>(
this.KEY_LOCAL_HISTORY,
null
)
if (!history) {
return { chat: {}, input: [] }
}

const key = getKeyForAuthStatus(authStatus)

// For backwards compatibility, we upgrade the local storage key from the old layout that is
// not scoped to individual user accounts to be scoped instead.
if (history && !isMigratedChatHistory2261(history)) {
// HACK: We spread both parts here as TS would otherwise have issues validating the type
// of AccountKeyedChatHistory. This is only three fields though.
history = {
...{ [key]: history },
...{
chat: {},
input: [],
},
} satisfies AccountKeyedChatHistory
// We use a raw write here to ensure we do not _append_ a key but actually replace
// existing `chat` and `input` keys.
// The result is not awaited to avoid changing this API to be async.
this.storage.update(this.KEY_LOCAL_HISTORY, history).then(() => {}, console.error)
}

if (!Object.hasOwn(history, key)) {
return { chat: {}, input: [] }
}

const accountHistory = (history as AccountKeyedChatHistory)[key]

// Persisted history might contain only string inputs without context so these may need converting too.
const inputs = accountHistory?.input
if (inputs?.length) {
let didUpdate = false
for (let i = 0; i < inputs.length; i++) {
const input = inputs[i]
// Convert strings to ChatInputHistory
if (typeof input === 'string') {
inputs[i] = { inputText: input, inputContextFiles: [] }
didUpdate = true
}
}
// Persist back to disk if we made any changes.
if (didUpdate) {
this.storage.update(this.KEY_LOCAL_HISTORY, history).then(() => {}, console.error)
}
}

return accountHistory as UserLocalHistory
const history = this.storage.get<AccountKeyedChatHistory | null>(this.KEY_LOCAL_HISTORY, null)
const accountKey = getKeyForAuthStatus(authStatus)
return history?.[accountKey] ?? { chat: {}, input: [] }
}

public async setChatHistory(authStatus: AuthStatus, history: UserLocalHistory): Promise<void> {
Expand All @@ -180,9 +113,6 @@ class LocalStorage {
}

await this.storage.update(this.KEY_LOCAL_HISTORY, fullHistory)

// MIGRATION: Delete old/orphaned storage data from a previous migration.
this.migrateChatHistory2665(fullHistory as AccountKeyedChatHistory)
} catch (error) {
console.error(error)
}
Expand Down Expand Up @@ -227,21 +157,6 @@ class LocalStorage {
return { anonymousUserID: id, created }
}

public async setLastUsedCommands(commands: string[]): Promise<void> {
if (commands.length === 0) {
return
}
try {
await this.storage.update(this.KEY_LAST_USED_RECIPES, commands)
} catch (error) {
console.error(error)
}
}

public getLastUsedCommands(): string[] | null {
return this.storage.get<string[] | null>(this.KEY_LAST_USED_RECIPES, null)
}

public get(key: string): string | null {
return this.storage.get(key, null)
}
Expand All @@ -257,22 +172,6 @@ class LocalStorage {
public async delete(key: string): Promise<void> {
await this.storage.update(key, undefined)
}

/**
* In https://github.com/sourcegraph/cody/pull/2665 we migrated the chat history key to use the
* user's username instead of their email address. This means that the storage would retain the chat
* history under the old key indefinitely. Large storage data slows down extension host activation
* and each `Memento#update` call, so we don't want it to linger.
*/
private migrateChatHistory2665(history: AccountKeyedChatHistory): void {
const needsMigration = Object.keys(history).some(key => key.includes('@'))
if (needsMigration) {
const cleanedHistory = Object.fromEntries(
Object.entries(history).filter(([key]) => !key.includes('@'))
)
this.storage.update(this.KEY_LOCAL_HISTORY, cleanedHistory).then(() => {}, console.error)
}
}
}

/**
Expand All @@ -284,14 +183,3 @@ export const localStorage = new LocalStorage()
function getKeyForAuthStatus(authStatus: AuthStatus): ChatHistoryKey {
return `${authStatus.endpoint}-${authStatus.username}`
}

/**
* As part of #2261, we migrated the storage format of the chat history to be keyed by the current
* user account. This checks if the new format is used by checking if any key contains a hyphen (the
* separator between endpoint and email in the new format).
*/
function isMigratedChatHistory2261(
history: AccountKeyedChatHistory | PersistedUserLocalHistory
): history is AccountKeyedChatHistory {
return !!Object.keys(history).find(k => k.includes('-'))
}

0 comments on commit a060422

Please sign in to comment.