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.githubusercontent.com/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
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