-
Notifications
You must be signed in to change notification settings - Fork 2.6k
refactor(history): store task history in external file #8524
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,7 +24,7 @@ type GlobalStateKey = keyof GlobalState | |||||||||
| type SecretStateKey = keyof SecretState | ||||||||||
| type RooCodeSettingsKey = keyof RooCodeSettings | ||||||||||
|
|
||||||||||
| const PASS_THROUGH_STATE_KEYS = ["taskHistory"] | ||||||||||
| const PASS_THROUGH_STATE_KEYS: string[] = [] | ||||||||||
|
|
||||||||||
| export const isPassThroughStateKey = (key: string) => PASS_THROUGH_STATE_KEYS.includes(key) | ||||||||||
|
|
||||||||||
|
|
@@ -52,6 +52,31 @@ export class ContextProxy { | |||||||||
| return this._isInitialized | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private async readTaskHistoryFile(): Promise<any[]> { | ||||||||||
| try { | ||||||||||
| const taskHistoryUri = vscode.Uri.joinPath(this.globalStorageUri, "taskHistory.jsonl") | ||||||||||
| const fileContent = await vscode.workspace.fs.readFile(taskHistoryUri) | ||||||||||
| const lines = fileContent.toString().split("\n").filter(Boolean) | ||||||||||
| return lines.map((line) => JSON.parse(line)) | ||||||||||
| } catch (error) { | ||||||||||
| if (error instanceof vscode.FileSystemError && error.code === "FileNotFound") { | ||||||||||
| return [] | ||||||||||
| } | ||||||||||
| logger.error(`Error reading task history file: ${error instanceof Error ? error.message : String(error)}`) | ||||||||||
| return [] | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private async writeTaskHistoryFile(tasks: any[]): Promise<void> { | ||||||||||
| try { | ||||||||||
| const taskHistoryUri = vscode.Uri.joinPath(this.globalStorageUri, "taskHistory.jsonl") | ||||||||||
| const content = tasks.map((task) => JSON.stringify(task)).join("\n") + "\n" | ||||||||||
| await vscode.workspace.fs.writeFile(taskHistoryUri, Buffer.from(content)) | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Atomicity/consistency: writing directly to the final file risks partial writes and bypasses our atomic write conventions. If we keep JSONL, write to a temp file then rename to the target to approximate an atomic replace.
Suggested change
|
||||||||||
| } catch (error) { | ||||||||||
| logger.error(`Error writing task history file: ${error instanceof Error ? error.message : String(error)}`) | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| public async initialize() { | ||||||||||
| for (const key of GLOBAL_STATE_KEYS) { | ||||||||||
| try { | ||||||||||
|
|
@@ -62,6 +87,31 @@ export class ContextProxy { | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Load task history from file | ||||||||||
| if (GLOBAL_STATE_KEYS.includes("taskHistory")) { | ||||||||||
| const tasks = await this.readTaskHistoryFile() | ||||||||||
| this.stateCache.taskHistory = tasks | ||||||||||
|
|
||||||||||
| // Migrate task history from global state if global state has data | ||||||||||
| const globalStateTasks = this.originalContext.globalState.get("taskHistory") | ||||||||||
| if (Array.isArray(globalStateTasks) && globalStateTasks.length > 0) { | ||||||||||
| try { | ||||||||||
| // Append global state tasks to existing file content | ||||||||||
| const combinedTasks = [...tasks, ...globalStateTasks] | ||||||||||
| await this.writeTaskHistoryFile(combinedTasks) | ||||||||||
| this.stateCache.taskHistory = combinedTasks | ||||||||||
| await this.originalContext.globalState.update("taskHistory", undefined) | ||||||||||
| vscode.window.showInformationMessage( | ||||||||||
| "Task history has been migrated using an append strategy to preserve existing entries.", | ||||||||||
| ) | ||||||||||
| } catch (error) { | ||||||||||
| logger.error( | ||||||||||
| `Error migrating task history: ${error instanceof Error ? error.message : String(error)}`, | ||||||||||
| ) | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const promises = [ | ||||||||||
| ...SECRET_STATE_KEYS.map(async (key) => { | ||||||||||
| try { | ||||||||||
|
|
@@ -165,18 +215,17 @@ export class ContextProxy { | |||||||||
| getGlobalState<K extends GlobalStateKey>(key: K): GlobalState[K] | ||||||||||
| getGlobalState<K extends GlobalStateKey>(key: K, defaultValue: GlobalState[K]): GlobalState[K] | ||||||||||
| getGlobalState<K extends GlobalStateKey>(key: K, defaultValue?: GlobalState[K]): GlobalState[K] { | ||||||||||
| if (isPassThroughStateKey(key)) { | ||||||||||
| const value = this.originalContext.globalState.get<GlobalState[K]>(key) | ||||||||||
| return value === undefined || value === null ? defaultValue : value | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const value = this.stateCache[key] | ||||||||||
| return value !== undefined ? value : defaultValue | ||||||||||
| } | ||||||||||
|
|
||||||||||
| updateGlobalState<K extends GlobalStateKey>(key: K, value: GlobalState[K]) { | ||||||||||
| if (isPassThroughStateKey(key)) { | ||||||||||
| return this.originalContext.globalState.update(key, value) | ||||||||||
| async updateGlobalState<K extends GlobalStateKey>(key: K, value: GlobalState[K]) { | ||||||||||
| if (key === "taskHistory") { | ||||||||||
| this.stateCache[key] = value | ||||||||||
| if (Array.isArray(value)) { | ||||||||||
| await this.writeTaskHistoryFile(value) | ||||||||||
| } | ||||||||||
| return | ||||||||||
| } | ||||||||||
|
|
||||||||||
| this.stateCache[key] = value | ||||||||||
|
|
@@ -361,6 +410,18 @@ export class ContextProxy { | |||||||||
| this.stateCache = {} | ||||||||||
| this.secretCache = {} | ||||||||||
|
|
||||||||||
| // Delete task history file | ||||||||||
| try { | ||||||||||
| const taskHistoryUri = vscode.Uri.joinPath(this.globalStorageUri, "taskHistory.jsonl") | ||||||||||
| await vscode.workspace.fs.delete(taskHistoryUri) | ||||||||||
| } catch (error) { | ||||||||||
| if (!(error instanceof vscode.FileSystemError && error.code === "FileNotFound")) { | ||||||||||
| logger.error( | ||||||||||
| `Error deleting task history file: ${error instanceof Error ? error.message : String(error)}`, | ||||||||||
| ) | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| await Promise.all([ | ||||||||||
| ...GLOBAL_STATE_KEYS.map((key) => this.originalContext.globalState.update(key, undefined)), | ||||||||||
| ...SECRET_STATE_KEYS.map((key) => this.originalContext.secrets.delete(key)), | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,12 +9,30 @@ import { ContextProxy } from "../ContextProxy" | |
| vi.mock("vscode", () => ({ | ||
| Uri: { | ||
| file: vi.fn((path) => ({ path })), | ||
| joinPath: vi.fn((base, ...paths) => ({ path: `${base.path}/${paths.join("/")}` })), | ||
| }, | ||
| ExtensionMode: { | ||
| Development: 1, | ||
| Production: 2, | ||
| Test: 3, | ||
| }, | ||
| FileSystemError: class FileSystemError extends Error { | ||
| code: string | ||
| constructor(message: string) { | ||
| super(message) | ||
| this.code = "FileNotFound" | ||
| } | ||
| }, | ||
| workspace: { | ||
| fs: { | ||
| readFile: vi.fn().mockRejectedValue(Object.assign(new Error("FileNotFound"), { code: "FileNotFound" })), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Missing positive-path coverage: readFile is always mocked to reject, so readTaskHistoryFile's success path (decoding and parsing JSONL) is never exercised. Add a test case that resolves readFile with a valid Uint8Array (e.g., new TextEncoder().encode('{"id":"1"}\n')) and assert that taskHistory is populated. |
||
| writeFile: vi.fn().mockResolvedValue(undefined), | ||
| delete: vi.fn().mockResolvedValue(undefined), | ||
| }, | ||
| }, | ||
| window: { | ||
| showInformationMessage: vi.fn(), | ||
| }, | ||
| })) | ||
|
|
||
| describe("ContextProxy", () => { | ||
|
|
@@ -71,12 +89,14 @@ describe("ContextProxy", () => { | |
| describe("constructor", () => { | ||
| it("should initialize state cache with all global state keys", () => { | ||
| // +1 for the migration check of old nested settings | ||
| expect(mockGlobalState.get).toHaveBeenCalledTimes(GLOBAL_STATE_KEYS.length + 1) | ||
| // +1 for the taskHistory migration check | ||
| expect(mockGlobalState.get).toHaveBeenCalledTimes(GLOBAL_STATE_KEYS.length + 2) | ||
| for (const key of GLOBAL_STATE_KEYS) { | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith(key) | ||
| } | ||
| // Also check for migration call | ||
| // Also check for migration calls | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("openRouterImageGenerationSettings") | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("taskHistory") | ||
| }) | ||
|
|
||
| it("should initialize secret cache with all secret keys", () => { | ||
|
|
@@ -99,8 +119,10 @@ describe("ContextProxy", () => { | |
| const result = proxy.getGlobalState("apiProvider") | ||
| expect(result).toBe("deepseek") | ||
|
|
||
| // Original context should be called once during updateGlobalState (+1 for migration check) | ||
| expect(mockGlobalState.get).toHaveBeenCalledTimes(GLOBAL_STATE_KEYS.length + 1) // From initialization + migration check | ||
| // Original context should be called during initialization | ||
| // +1 for openRouterImageGenerationSettings migration check | ||
| // +1 for taskHistory migration check | ||
| expect(mockGlobalState.get).toHaveBeenCalledTimes(GLOBAL_STATE_KEYS.length + 2) | ||
| }) | ||
|
|
||
| it("should handle default values correctly", async () => { | ||
|
|
@@ -109,23 +131,16 @@ describe("ContextProxy", () => { | |
| expect(result).toBe("deepseek") | ||
| }) | ||
|
|
||
| it("should bypass cache for pass-through state keys", async () => { | ||
| // Setup mock return value | ||
| mockGlobalState.get.mockReturnValue("pass-through-value") | ||
|
|
||
| // Use a pass-through key (taskHistory) | ||
| it("should return value from cache for taskHistory", async () => { | ||
| // taskHistory is now loaded from file and stored in cache | ||
| const result = proxy.getGlobalState("taskHistory") | ||
|
|
||
| // Should get value directly from original context | ||
| expect(result).toBe("pass-through-value") | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("taskHistory") | ||
| // Should return the cached value (empty array from file read) | ||
| expect(result).toEqual([]) | ||
| }) | ||
|
|
||
| it("should respect default values for pass-through state keys", async () => { | ||
| // Setup mock to return undefined | ||
| mockGlobalState.get.mockReturnValue(undefined) | ||
|
|
||
| // Use a pass-through key with default value | ||
| it("should respect default values for taskHistory", async () => { | ||
| // Use taskHistory with default value | ||
| const historyItems = [ | ||
| { | ||
| id: "1", | ||
|
|
@@ -140,8 +155,8 @@ describe("ContextProxy", () => { | |
|
|
||
| const result = proxy.getGlobalState("taskHistory", historyItems) | ||
|
|
||
| // Should return default value when original context returns undefined | ||
| expect(result).toBe(historyItems) | ||
| // Should return cached value (empty array) since it exists | ||
| expect(result).toEqual([]) | ||
| }) | ||
| }) | ||
|
|
||
|
|
@@ -157,7 +172,7 @@ describe("ContextProxy", () => { | |
| expect(storedValue).toBe("deepseek") | ||
| }) | ||
|
|
||
| it("should bypass cache for pass-through state keys", async () => { | ||
| it("should write taskHistory to file instead of global state", async () => { | ||
| const historyItems = [ | ||
| { | ||
| id: "1", | ||
|
|
@@ -172,16 +187,15 @@ describe("ContextProxy", () => { | |
|
|
||
| await proxy.updateGlobalState("taskHistory", historyItems) | ||
|
|
||
| // Should update original context | ||
| expect(mockGlobalState.update).toHaveBeenCalledWith("taskHistory", historyItems) | ||
| // Should NOT update original context for taskHistory | ||
| expect(mockGlobalState.update).not.toHaveBeenCalledWith("taskHistory", historyItems) | ||
|
|
||
| // Setup mock for subsequent get | ||
| mockGlobalState.get.mockReturnValue(historyItems) | ||
| // Should write to file (mocked in vscode.workspace.fs.writeFile) | ||
| expect(vscode.workspace.fs.writeFile).toHaveBeenCalled() | ||
|
|
||
| // Should get fresh value from original context | ||
| // Should get value from cache | ||
| const storedValue = proxy.getGlobalState("taskHistory") | ||
| expect(storedValue).toBe(historyItems) | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("taskHistory") | ||
| expect(storedValue).toEqual(historyItems) | ||
| }) | ||
| }) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: fileContent is a Uint8Array; calling toString() returns "[object Uint8Array]" in Node, breaking JSONL parsing and causing an empty history. Decode the buffer first.