fix(webview): isolate parallel mode and provider profile writes#979
fix(webview): isolate parallel mode and provider profile writes#979easonLiangWorldedtech wants to merge 10 commits into
Conversation
📝 WalkthroughWalkthroughAdds stable per-webview identifiers and durable non-secret view state. ChangesPer-view state isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1b8285b to
1b64431
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/webview/ClineProvider.ts (2)
3069-3099: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resetState()doesn't clearviewLocalState.The doc comment on
_updateViewLocalStateFromMutation(lines 3013-3017) explicitly listsresetStateas one of the mutation sources that should invalidateviewLocalState, butresetState()never calls_clearViewLocalState()(or_updateViewLocalStateFromMutation). After a "Reset State" action, this tab's stale in-memorymode/currentApiConfigName/apiConfigurationoverrides survive and continue to be layered on top of the freshly-reset global state viagetState()'s merge, so the reset appears incomplete for this view.🐛 Proposed fix
await this.contextProxy.resetAllState() + this._clearViewLocalState() await this.providerSettingsManager.resetAllConfigs()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/ClineProvider.ts` around lines 3069 - 3099, Update resetState() to clear the tab’s cached view-local state by calling _clearViewLocalState() after the reset mutations complete and before postStateToWebview(). Ensure the reset invalidates stale mode, currentApiConfigName, and apiConfiguration overrides while preserving the existing reset sequence.
1801-1826: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winSpreading merged
getState()intocontextProxy.setValues()leaks view-local overrides into shared global state.
globalSettingshere is the merged state (view-local overlaid on global), not just the shared global state. Spreading it (...globalSettings) intocontextProxy.setValues()writes every merged field — including this tab's view-localmode,apiConfiguration, and other overrides — back into the shared/global ContextProxy. This is exactly the kind of parallel-tab leak issue#978is meant to fix: deleting a provider profile in one tab can silently overwrite another tab's global fallback state (mode, terminal settings, etc.) with this tab's current values. Every other write site in this file (upsertProviderProfile,activateProviderProfile) only ever targets specific fields, never the full merged state.🐛 Proposed fix
const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) - const globalSettings = await this.getState() - - await this.contextProxy.setValues({ - ...globalSettings, - currentApiConfigName: profileToActivate, - listApiConfigMeta: entries, - }) + + await this.contextProxy.setValues({ + currentApiConfigName: profileToActivate, + listApiConfigMeta: entries, + }) await this.saveViewState("currentApiConfigName", profileToActivate)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/ClineProvider.ts` around lines 1801 - 1826, Update deleteProviderProfile so contextProxy.setValues does not spread the merged state returned by getState(). Write only the shared fields required for deletion—currentApiConfigName and listApiConfigMeta—while preserving the existing profile selection and view-state update behavior.
🧹 Nitpick comments (1)
webview-ui/src/utils/vscode.ts (1)
25-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd dedicated unit tests for
createViewStateId/getViewStateId.This new persistence/serialization logic isn't directly unit-tested here —
App.spec.tsxonly stubsgetViewStateIdaway rather than exercising the real implementation (id reuse, generation, fallback for missingcrypto.randomUUID).Based on the coding guideline for
webview-ui/src/**/*.{ts,tsx}: "Prefer local webview-ui tests for React/webview behavior such as component rendering, local state, hooks, form dirty-state, validation, or prop wiring. Add or update Vitest coverage under webview-ui/src/**/tests instead of reaching for apps/vscode-e2e."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/utils/vscode.ts` around lines 25 - 49, Add focused Vitest unit tests under webview-ui/src/**/__tests__ covering getViewStateId’s reuse of a persisted non-empty viewStateId, generation and persistence when it is missing or invalid, and createViewStateId’s fallback when crypto.randomUUID is unavailable. Exercise the real implementation rather than stubbing getViewStateId, and use controlled crypto/time/randomness so the generated ID behavior is deterministic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 3069-3099: Update resetState() to clear the tab’s cached
view-local state by calling _clearViewLocalState() after the reset mutations
complete and before postStateToWebview(). Ensure the reset invalidates stale
mode, currentApiConfigName, and apiConfiguration overrides while preserving the
existing reset sequence.
- Around line 1801-1826: Update deleteProviderProfile so contextProxy.setValues
does not spread the merged state returned by getState(). Write only the shared
fields required for deletion—currentApiConfigName and listApiConfigMeta—while
preserving the existing profile selection and view-state update behavior.
---
Nitpick comments:
In `@webview-ui/src/utils/vscode.ts`:
- Around line 25-49: Add focused Vitest unit tests under
webview-ui/src/**/__tests__ covering getViewStateId’s reuse of a persisted
non-empty viewStateId, generation and persistence when it is missing or invalid,
and createViewStateId’s fallback when crypto.randomUUID is unavailable. Exercise
the real implementation rather than stubbing getViewStateId, and use controlled
crypto/time/randomness so the generated ID behavior is deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c32288c-039b-4dc0-90e0-75631b055485
📒 Files selected for processing (13)
packages/types/src/__tests__/index.test.tspackages/types/src/global-settings.tspackages/types/src/vscode-extension-host.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/core/webview/__tests__/ClineProvider.sticky-mode.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tswebview-ui/src/App.tsxwebview-ui/src/__tests__/App.spec.tsxwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/utils/vscode.ts
💤 Files with no reviewable changes (1)
- webview-ui/src/App.tsx
34b322e to
e21ff41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 1178-1206: Strengthen the assertions in the “should sync
view-local apiConfiguration when activating an upserted profile” test so the
resulting apiConfiguration and viewLocalState.apiConfiguration exactly match the
new providerSettings rather than merely containing its fields. Explicitly verify
stale openRouterModelId data is absent after upserting the zai profile, while
preserving the existing state and provider assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5648ba45-1360-408c-8afe-7732b5dd8558
📒 Files selected for processing (4)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/webview/tests/webviewMessageHandler.spec.ts
- src/core/webview/ClineProvider.ts
| it("should sync view-local apiConfiguration when activating an upserted profile", async () => { | ||
| const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) | ||
| await (provider as any).saveViewState("apiConfiguration", { | ||
| apiProvider: "openrouter", | ||
| openRouterModelId: "openai/gpt-4.1", | ||
| }) | ||
|
|
||
| const providerSettings = { | ||
| apiProvider: "zai" as const, | ||
| zaiApiKey: "mock-key", | ||
| zaiApiLine: "international_api" as const, | ||
| apiModelId: "glm-5.1", | ||
| } | ||
| vi.spyOn(provider.providerSettingsManager, "saveConfig").mockResolvedValue("zai-profile-id") | ||
| vi.spyOn(provider.providerSettingsManager, "listConfig").mockResolvedValue([ | ||
| { name: "default", id: "zai-profile-id", apiProvider: "zai" }, | ||
| ]) | ||
|
|
||
| await provider.upsertProviderProfile("default", providerSettings, true) | ||
|
|
||
| const state = await provider.getState() | ||
| expect(state.currentApiConfigName).toBe("default") | ||
| expect(state.apiConfiguration).toMatchObject(providerSettings) | ||
| expect(state.apiConfiguration.apiProvider).toBe("zai") | ||
| expect((provider as any).viewLocalState.apiConfiguration).toMatchObject(providerSettings) | ||
|
|
||
| await provider.dispose() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assertion is too weak to catch stale apiConfiguration leakage.
This test pre-seeds viewLocalState.apiConfiguration with openrouter settings (line 1180-1183), then upserts a completely different zai profile. The final check on line 1200 (toMatchObject(providerSettings)) passes even if stale keys like openRouterModelId remain merged into state.apiConfiguration, because toMatchObject only verifies the specified keys are present — it ignores extras. This is precisely the scenario the PR is meant to guard against (stale provider profile data across a profile switch), so the test should assert the object no longer contains prior fields.
🧪 Proposed stricter assertion
const state = await provider.getState()
expect(state.currentApiConfigName).toBe("default")
expect(state.apiConfiguration).toMatchObject(providerSettings)
expect(state.apiConfiguration.apiProvider).toBe("zai")
+ expect(state.apiConfiguration).not.toHaveProperty("openRouterModelId")
expect((provider as any).viewLocalState.apiConfiguration).toMatchObject(providerSettings)As per coding guidelines, **/*.{test,spec}.{ts,tsx,js} should test state transitions rigorously; strengthening this assertion better validates the isolation guarantee.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should sync view-local apiConfiguration when activating an upserted profile", async () => { | |
| const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) | |
| await (provider as any).saveViewState("apiConfiguration", { | |
| apiProvider: "openrouter", | |
| openRouterModelId: "openai/gpt-4.1", | |
| }) | |
| const providerSettings = { | |
| apiProvider: "zai" as const, | |
| zaiApiKey: "mock-key", | |
| zaiApiLine: "international_api" as const, | |
| apiModelId: "glm-5.1", | |
| } | |
| vi.spyOn(provider.providerSettingsManager, "saveConfig").mockResolvedValue("zai-profile-id") | |
| vi.spyOn(provider.providerSettingsManager, "listConfig").mockResolvedValue([ | |
| { name: "default", id: "zai-profile-id", apiProvider: "zai" }, | |
| ]) | |
| await provider.upsertProviderProfile("default", providerSettings, true) | |
| const state = await provider.getState() | |
| expect(state.currentApiConfigName).toBe("default") | |
| expect(state.apiConfiguration).toMatchObject(providerSettings) | |
| expect(state.apiConfiguration.apiProvider).toBe("zai") | |
| expect((provider as any).viewLocalState.apiConfiguration).toMatchObject(providerSettings) | |
| await provider.dispose() | |
| }) | |
| }) | |
| it("should sync view-local apiConfiguration when activating an upserted profile", async () => { | |
| const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) | |
| await (provider as any).saveViewState("apiConfiguration", { | |
| apiProvider: "openrouter", | |
| openRouterModelId: "openai/gpt-4.1", | |
| }) | |
| const providerSettings = { | |
| apiProvider: "zai" as const, | |
| zaiApiKey: "mock-key", | |
| zaiApiLine: "international_api" as const, | |
| apiModelId: "glm-5.1", | |
| } | |
| vi.spyOn(provider.providerSettingsManager, "saveConfig").mockResolvedValue("zai-profile-id") | |
| vi.spyOn(provider.providerSettingsManager, "listConfig").mockResolvedValue([ | |
| { name: "default", id: "zai-profile-id", apiProvider: "zai" }, | |
| ]) | |
| await provider.upsertProviderProfile("default", providerSettings, true) | |
| const state = await provider.getState() | |
| expect(state.currentApiConfigName).toBe("default") | |
| expect(state.apiConfiguration).toMatchObject(providerSettings) | |
| expect(state.apiConfiguration.apiProvider).toBe("zai") | |
| expect(state.apiConfiguration).not.toHaveProperty("openRouterModelId") | |
| expect((provider as any).viewLocalState.apiConfiguration).toMatchObject(providerSettings) | |
| await provider.dispose() | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts` around lines
1178 - 1206, Strengthen the assertions in the “should sync view-local
apiConfiguration when activating an upserted profile” test so the resulting
apiConfiguration and viewLocalState.apiConfiguration exactly match the new
providerSettings rather than merely containing its fields. Explicitly verify
stale openRouterModelId data is absent after upserting the zai profile, while
preserving the existing state and provider assertions.
Source: Coding guidelines
Summary
Isolate parallel mode switches and provider profile write operations to use per-view state instead of global state. This is a sibling follow-up PR that should be reviewed after PR #977 lands.
Blocked until PR #977 (feat/view-local-state-base) lands/rebase.
Changes
handleModeSwitch: Routes mode writes through view-local persistence layer instead of writing to global stateScope
handleModeSwitch→ view-local mode persistenceTests
Related
Summary by CodeRabbit
New Features
Bug Fixes
Tests