-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: isolate state between duplicate VSCode workspaces #7699
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,13 +36,18 @@ const globalSettingsExportSchema = globalSettingsSchema.omit({ | |||||||||||
|
|
||||||||||||
| export class ContextProxy { | ||||||||||||
| private readonly originalContext: vscode.ExtensionContext | ||||||||||||
| private readonly sessionId: string | ||||||||||||
|
|
||||||||||||
| private stateCache: GlobalState | ||||||||||||
| private secretCache: SecretState | ||||||||||||
| private _isInitialized = false | ||||||||||||
|
|
||||||||||||
| constructor(context: vscode.ExtensionContext) { | ||||||||||||
| this.originalContext = context | ||||||||||||
| // Use sessionId to isolate state between multiple VSCode windows | ||||||||||||
| // This ensures each window maintains its own independent state | ||||||||||||
| // Fallback to empty string if sessionId is not available (e.g., in tests) | ||||||||||||
| this.sessionId = vscode.env.sessionId || "" | ||||||||||||
| this.stateCache = {} | ||||||||||||
| this.secretCache = {} | ||||||||||||
| this._isInitialized = false | ||||||||||||
|
|
@@ -55,8 +60,9 @@ export class ContextProxy { | |||||||||||
| public async initialize() { | ||||||||||||
| for (const key of GLOBAL_STATE_KEYS) { | ||||||||||||
| try { | ||||||||||||
| // Revert to original assignment | ||||||||||||
| this.stateCache[key] = this.originalContext.globalState.get(key) | ||||||||||||
| // Use session-specific key for state isolation | ||||||||||||
| const sessionKey = this.getSessionKey(key) | ||||||||||||
| this.stateCache[key] = this.originalContext.globalState.get(sessionKey) | ||||||||||||
| } catch (error) { | ||||||||||||
| logger.error(`Error loading global ${key}: ${error instanceof Error ? error.message : String(error)}`) | ||||||||||||
| } | ||||||||||||
|
|
@@ -91,13 +97,38 @@ export class ContextProxy { | |||||||||||
| this._isInitialized = true | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Creates a session-specific key by combining the base key with the session ID. | ||||||||||||
| * This ensures state isolation between multiple VSCode windows. | ||||||||||||
| * | ||||||||||||
| * @param key The base state key | ||||||||||||
| * @returns The session-specific key | ||||||||||||
| */ | ||||||||||||
| private getSessionKey(key: string): string { | ||||||||||||
| // For certain keys that should be shared across sessions (like API configs), | ||||||||||||
| // we don't add the session prefix | ||||||||||||
| const sharedKeys = ["listApiConfigMeta", "currentApiConfigName", "apiProvider"] | ||||||||||||
| if (sharedKeys.includes(key)) { | ||||||||||||
| return key | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // If no sessionId is available (e.g., in tests), use the key as-is | ||||||||||||
| if (!this.sessionId) { | ||||||||||||
| return key | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // For all other keys, add session prefix to isolate state | ||||||||||||
| return `session_${this.sessionId}_${key}` | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Migrates old nested openRouterImageGenerationSettings to the new flattened structure | ||||||||||||
| */ | ||||||||||||
| private async migrateImageGenerationSettings() { | ||||||||||||
| try { | ||||||||||||
| // Check if there's an old nested structure | ||||||||||||
| const oldNestedSettings = this.originalContext.globalState.get<any>("openRouterImageGenerationSettings") | ||||||||||||
| // Check if there's an old nested structure (use session-specific key) | ||||||||||||
| const sessionKey = this.getSessionKey("openRouterImageGenerationSettings") | ||||||||||||
|
Contributor
Author
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. Migration edge case: This checks for session-specific keys, but if settings existed before this PR, they wouldn't have session prefixes. Should we check both sessionKey and the original key to handle pre-existing data?
Suggested change
|
||||||||||||
| const oldNestedSettings = this.originalContext.globalState.get<any>(sessionKey) | ||||||||||||
|
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. In migrateImageGenerationSettings, consider checking both the session-prefixed key and the legacy plain key to ensure smooth migration for users updating from earlier versions. |
||||||||||||
|
|
||||||||||||
| if (oldNestedSettings && typeof oldNestedSettings === "object") { | ||||||||||||
| logger.info("Migrating old nested image generation settings to flattened structure") | ||||||||||||
|
|
@@ -114,16 +145,14 @@ export class ContextProxy { | |||||||||||
|
|
||||||||||||
| // Migrate the selected model if it exists and we don't already have one | ||||||||||||
| if (oldNestedSettings.selectedModel && !this.stateCache.openRouterImageGenerationSelectedModel) { | ||||||||||||
| await this.originalContext.globalState.update( | ||||||||||||
| "openRouterImageGenerationSelectedModel", | ||||||||||||
| oldNestedSettings.selectedModel, | ||||||||||||
| ) | ||||||||||||
| const modelSessionKey = this.getSessionKey("openRouterImageGenerationSelectedModel") | ||||||||||||
| await this.originalContext.globalState.update(modelSessionKey, oldNestedSettings.selectedModel) | ||||||||||||
| this.stateCache.openRouterImageGenerationSelectedModel = oldNestedSettings.selectedModel | ||||||||||||
| logger.info("Migrated openRouterImageGenerationSelectedModel to global state") | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Clean up the old nested structure | ||||||||||||
| await this.originalContext.globalState.update("openRouterImageGenerationSettings", undefined) | ||||||||||||
| await this.originalContext.globalState.update(sessionKey, undefined) | ||||||||||||
| logger.info("Removed old nested openRouterImageGenerationSettings") | ||||||||||||
| } | ||||||||||||
| } catch (error) { | ||||||||||||
|
|
@@ -166,7 +195,9 @@ export class ContextProxy { | |||||||||||
| 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) | ||||||||||||
| // Use session-specific key for pass-through state as well | ||||||||||||
| const sessionKey = this.getSessionKey(key) | ||||||||||||
| const value = this.originalContext.globalState.get<GlobalState[K]>(sessionKey) | ||||||||||||
| return value === undefined || value === null ? defaultValue : value | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
|
@@ -175,12 +206,14 @@ export class ContextProxy { | |||||||||||
| } | ||||||||||||
|
|
||||||||||||
| updateGlobalState<K extends GlobalStateKey>(key: K, value: GlobalState[K]) { | ||||||||||||
| const sessionKey = this.getSessionKey(key) | ||||||||||||
|
|
||||||||||||
| if (isPassThroughStateKey(key)) { | ||||||||||||
| return this.originalContext.globalState.update(key, value) | ||||||||||||
| return this.originalContext.globalState.update(sessionKey, value) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| this.stateCache[key] = value | ||||||||||||
| return this.originalContext.globalState.update(key, value) | ||||||||||||
| return this.originalContext.globalState.update(sessionKey, value) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private getAllGlobalState(): GlobalState { | ||||||||||||
|
|
@@ -362,7 +395,10 @@ export class ContextProxy { | |||||||||||
| this.secretCache = {} | ||||||||||||
|
|
||||||||||||
| await Promise.all([ | ||||||||||||
| ...GLOBAL_STATE_KEYS.map((key) => this.originalContext.globalState.update(key, undefined)), | ||||||||||||
| ...GLOBAL_STATE_KEYS.map((key) => { | ||||||||||||
| const sessionKey = this.getSessionKey(key) | ||||||||||||
| return this.originalContext.globalState.update(sessionKey, undefined) | ||||||||||||
| }), | ||||||||||||
| ...SECRET_STATE_KEYS.map((key) => this.originalContext.secrets.delete(key)), | ||||||||||||
| ...GLOBAL_SECRET_KEYS.map((key) => this.originalContext.secrets.delete(key)), | ||||||||||||
| ]) | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,9 @@ vi.mock("vscode", () => ({ | |
| Production: 2, | ||
| Test: 3, | ||
| }, | ||
| env: { | ||
| sessionId: "test-session-id-12345", | ||
| }, | ||
| })) | ||
|
|
||
| describe("ContextProxy", () => { | ||
|
|
@@ -72,11 +75,21 @@ describe("ContextProxy", () => { | |
| 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) | ||
|
|
||
| // When sessionId is available, session-specific keys are used for non-shared keys | ||
| // In test environment with mocked sessionId, check for session-specific keys | ||
| const sharedKeys = ["listApiConfigMeta", "currentApiConfigName", "apiProvider"] | ||
|
Contributor
Author
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. Nice test coverage! The tests properly verify the session-specific key behavior. |
||
| for (const key of GLOBAL_STATE_KEYS) { | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith(key) | ||
| if (sharedKeys.includes(key)) { | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith(key) | ||
| } else { | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith(`session_test-session-id-12345_${key}`) | ||
| } | ||
| } | ||
| // Also check for migration call | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("openRouterImageGenerationSettings") | ||
| // Also check for migration call with session-specific key | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith( | ||
| "session_test-session-id-12345_openRouterImageGenerationSettings", | ||
| ) | ||
| }) | ||
|
|
||
| it("should initialize secret cache with all secret keys", () => { | ||
|
|
@@ -116,9 +129,9 @@ describe("ContextProxy", () => { | |
| // Use a pass-through key (taskHistory) | ||
| const result = proxy.getGlobalState("taskHistory") | ||
|
|
||
| // Should get value directly from original context | ||
| // Should get value directly from original context with session-specific key (when sessionId is available) | ||
| expect(result).toBe("pass-through-value") | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("taskHistory") | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("session_test-session-id-12345_taskHistory") | ||
| }) | ||
|
|
||
| it("should respect default values for pass-through state keys", async () => { | ||
|
|
@@ -149,7 +162,7 @@ describe("ContextProxy", () => { | |
| it("should update state directly in original context", async () => { | ||
| await proxy.updateGlobalState("apiProvider", "deepseek") | ||
|
|
||
| // Should have called original context | ||
| // Should have called original context (apiProvider is a shared key, no session prefix) | ||
| expect(mockGlobalState.update).toHaveBeenCalledWith("apiProvider", "deepseek") | ||
|
|
||
| // Should have stored the value in cache | ||
|
|
@@ -172,16 +185,19 @@ describe("ContextProxy", () => { | |
|
|
||
| await proxy.updateGlobalState("taskHistory", historyItems) | ||
|
|
||
| // Should update original context | ||
| expect(mockGlobalState.update).toHaveBeenCalledWith("taskHistory", historyItems) | ||
| // Should update original context with session-specific key (when sessionId is available) | ||
| expect(mockGlobalState.update).toHaveBeenCalledWith( | ||
| "session_test-session-id-12345_taskHistory", | ||
| historyItems, | ||
| ) | ||
|
|
||
| // Setup mock for subsequent get | ||
| mockGlobalState.get.mockReturnValue(historyItems) | ||
|
|
||
| // Should get fresh value from original context | ||
| // Should get fresh value from original context with session-specific key (when sessionId is available) | ||
| const storedValue = proxy.getGlobalState("taskHistory") | ||
| expect(storedValue).toBe(historyItems) | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("taskHistory") | ||
| expect(mockGlobalState.get).toHaveBeenCalledWith("session_test-session-id-12345_taskHistory") | ||
| }) | ||
| }) | ||
|
|
||
|
|
@@ -387,9 +403,17 @@ describe("ContextProxy", () => { | |
| // Reset all state | ||
| await proxy.resetAllState() | ||
|
|
||
| // Should have called update with undefined for each key | ||
| // Should have called update with undefined for each key using session-specific keys (when sessionId is available) | ||
| const sharedKeys = ["listApiConfigMeta", "currentApiConfigName", "apiProvider"] | ||
| for (const key of GLOBAL_STATE_KEYS) { | ||
| expect(mockGlobalState.update).toHaveBeenCalledWith(key, undefined) | ||
| if (sharedKeys.includes(key)) { | ||
| expect(mockGlobalState.update).toHaveBeenCalledWith(key, undefined) | ||
| } else { | ||
| expect(mockGlobalState.update).toHaveBeenCalledWith( | ||
| `session_test-session-id-12345_${key}`, | ||
| undefined, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Total calls should include initial setup + reset operations | ||
|
|
||
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.
Consider extracting this list to a constant like
SHARED_STATE_KEYSsince it's duplicated in the test file. This would make maintenance easier and prevent drift between implementation and tests.Also, could we add a comment explaining why these specific keys need to be shared across sessions? This would help future maintainers understand the design decision.