fix(webview): add durable per-view state base - #977
fix(webview): add durable per-view state base#977easonLiangWorldedtech wants to merge 37 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:
📝 SummarySummary by CodeRabbit
WalkthroughAdds durable per-view state schemas, stable identifiers, persistence, restoration, and state merging across webview and extension layers. It also adds task-specific API controls, browser storage fallbacks, resilient model loading, debug logging, and parallel-view integration coverage. ChangesPer-view state and task control
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: 🟡 Moderate · up to The PR adds durable per-view mode/profile state and task-scoped controls, but the current behavior can cross-contaminate views or tasks, restore the wrong API profile, and retain invalid or deleted selections; unconditional debug instrumentation also runs in production paths. These are concrete merge-readiness risks that should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant Webview
participant ExtensionStateContext
participant webviewMessageHandler
participant ClineProvider
participant GlobalState
Webview->>ExtensionStateContext: obtain stable viewStateId
ExtensionStateContext->>webviewMessageHandler: send webviewDidLaunch with viewStateId
webviewMessageHandler->>ClineProvider: setViewStateId(viewStateId)
ClineProvider->>GlobalState: load or save non-secret viewStates
ClineProvider-->>Webview: return merged view-local state
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The implementation satisfies issue Full details: Out of Scope Changes checkExplanation Most changes support per-view state and its E2E coverage, including task-scoped API controls and open-tab preservation. However, Kimi Code router-model error handling, broad E2EDEBUG logging changes, and the .gitignore Husky rule are not directly required by issue Resolution Remove the unrelated Kimi Code error-handling, broad E2EDEBUG logging, and .gitignore changes, or link approved issues that explicitly require them. Keep the task-scoped API and open-tab changes only if they remain necessary for the documented parallel-mode E2E coverage and are accepted as supporting scope. Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 26 files. (3 skipped: 3 unsupported.) Full details: Regression EvidenceExplanation
Resolution Add a focused unit test in Full details: Trust And Persistence InvariantsExplanation The new per-view mode overlay can bypass the MCP allowlist after a normal custom-mode deletion. Resolution When a custom mode is deleted, invalidate or replace that mode in every active provider's local state and in every persisted Full details: Description checkExplanation The description includes the linked issue, implementation details, reviewer focus areas, test procedures, manual verification steps, checklist, and documentation status. It is complete and aligned with the changes. ✨ 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
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.
github-actions
Bot
added
awaiting-author
2ef16bb to
37a5dd1
Compare
A just-resolved globalState write can momentarily lag a synchronous globalState.get in the extension host. The per-view writes are already awaited through the serialized view-state write queue before the tasks complete, so poll until both the sidebar and tab persisted selections are visible before asserting, instead of reading globalState once.
Address the CodeRabbit docstring coverage warning on the durable per-view state PR by documenting the new view-state persistence/merge helpers in ClineProvider, the task-scoped API controls in the extension API, and the viewStateId generation/restoration helpers in the webview wrapper.
…sted view-local secrets - selectTaskFollowupSuggestion() now passes the registered task explicitly to handleModeSwitch(), so answering a follow-up on task B no longer switches the mode of the provider's currently focused task A (resolves review comment on src/extension/api.ts). - getConfiguration() flattens the nested view-local apiConfiguration onto the top level before the isSecretStateKey() filter, so nested provider secrets (apiKey, openRouterApiKey, ...) cannot leak through the API — a regression introduced by the per-view state base's nested apiConfiguration shape. - Consolidate the duplicate kimi-code oauth vi.mock in the routerModels spec and replace raw provider identifier literals flagged by the merged zoo/no-raw-provider-identifiers rule with providerIdentifiers.* constants. Validated: 112 targeted vitest tests pass, pnpm --dir src run check-types clean, eslint --max-warnings=0 clean on all touched files.
The .husky/_ directory contains husky-generated internal shims that were committed by accident. Ignore the directory and untrack the existing shims so they no longer show up as modified files on every install.
- Scope handleModeSwitch to the target task: switches for non-focused tasks no longer rewrite the view's durable mode pin, emit ModeChanged, or activate provider profiles. - History restore persists the mode through the view's own pin instead of the shared global mode setting. - loadViewState discards stale results when a newer view id is registered during the load, and pre-launch temporary view ids never write durable entries (orphan prevention). - deleteProviderProfile re-points persisted view pins that referenced the deleted profile, and only overwrites this view's in-memory profile pin when it actually pinned the deleted profile. - resetState also clears this view's persisted entry. - saveViewState is now public and fully typed (no explicit any left in the provider). - Mock ContextProxy mirrors the real state cache so tests can exercise stale-cache and fresh-read behavior; add regression tests for all of the above.
…iable - removeRegisteredTask only drops the registration when the stored controller is the same instance, so a replaced task reusing a taskId is not torn down by the old instance's abort/unfocus events. - selectTaskFollowupSuggestion delivers the answer even when the follow-up mode switch fails, logging the failure instead of losing the user's response.
…selection - webviewDidLaunch rescue: when the view's own pin is invalid, re-pin the view to the shared global selection if it is still valid instead of overwriting the global setting and activating a global profile from one view's launch path. - updateSettings persists provider settings through the provider-level setValue so the durable write path is the one the provider serializes. - Lower the recorded no-explicit-any suppression counts for the touched files (fixes, not new suppressions).
…red fixtures - view-state.test.ts derives the round count from the follow-up isolation fixture instead of a hardcoded 10, and drops an unused map. - Document the new mode-switch predicate fixtures alongside the legacy model-scoped fixtures in runTest.ts.
- The webviewDidLaunch describe now assigns its runtime members through a structural LaunchProviderFixture cast instead of per-line as-any, and the getState mock return is typed against the provider signature. - Drops the webviewMessageHandler.spec.ts suppression count back to the PR base level (35).
…ion spec Use typed structural casts (ClineProvider / OutputChannel) for the API double and remove the now-empty suppression entry for the file.
CI e2e-mock regression: the mode switch inside a task lands before the tab webview's launch message registers its stable viewStateId, so the ephemeral-skip silently dropped the view's durable mode write and the "sidebar and tab panel keep mode isolated" e2e timed out waiting for the persisted entries. Pre-launch writes now persist under the temporary view id and are re-keyed to the stable id when the webview registers it (setViewStateId runs the re-key through the serialized write queue before loadViewState). A pre-existing stable entry wins and the temporary entry is dropped, since temporary ids are session counters that can collide across window reloads. The stale-load guard is unchanged. Unit tests: the ephemeral-skip assertion is replaced with re-key tests (write under temporary id, re-key on registration, stable entry wins) and the stale-load test is restructured so it genuinely exercises the guard through the cached read path.
…elMode spec Address the CodeRabbit maintainability finding: drop the blanket no-explicit-any suppression (137) for ClineProvider.parallelMode.spec.ts and type the spec properly instead. - Private member access moves from (provider as any).x to bracket notation (provider["x"]); public members (saveViewState, setValue, setValues, handleModeSwitch, resolveWebviewView, log) drop the cast entirely and keep their native generics. - MockContextProxy now takes vscode.ExtensionContext; memento and mock callbacks use unknown instead of any; the webview structural double is cast once as unknown as vscode.WebviewView. - Key/value casts are removed where the key is a valid RooCodeSettings key; the one genuine exception (apiConfiguration is a GlobalState key outside the proxy's generic) keeps a documented double assertion. - api-configuration.spec.ts: document the as-unknown-as-ClineProvider structural double in the new test (API.getConfiguration only reads sidebarProvider.getValues). check-types clean; parallelMode 49/49 and api-configuration 3/3 green; eslint --prune-suppressions clean with the parallelMode entry removed from eslint-suppressions.json and every other count unchanged.
Review of every mode-change entry point surfaced three inconsistencies:
- handleModeSwitch accepted any slug (the webview "mode" message sends
message.text as Mode with no server-side validation), so unvalidated
callers could persist invalid modes into task history and the view's
durable pin. Validate the slug against built-in + custom modes and
no-op (with a log) on unknown slugs, mirroring
selectTaskFollowupSuggestion.
- Task.submitUserMessage wrote the mode through setValues (raw global
ContextProxy write, no history entry, no TaskModeSwitched/ModeChanged,
no view pin) while every other switch goes through handleModeSwitch.
Route it through handleModeSwitch(mode, this) so an API-initiated
switch is recorded like any other.
- delegateParentAndOpenChild passed the child's mode as as any; drop the
cast now that handleModeSwitch validates.
Test updates:
- sticky-mode: the "invalid mode" test now asserts the ignore behavior;
the module-level getModeBySlug mock's undefined override (leaked past
vi.clearAllMocks, which does not clear implementations) is restored in
the top-level beforeEach so later tests validate through the default;
the slow-init ordering test settles the restore's early durable write
before issuing the mid-init switch, matching the production order in
which a user's switch is issued after the restore starts.
- Task.spec: the submitUserMessage mode test now expects
handleModeSwitch("code", task); the mock provider gains the method.
check-types clean; 336/336 across the six affected spec files; eslint
--prune-suppressions clean (ClineProvider.ts no-explicit-any 12 -> 11).
The "sidebar and tab panel keep mode isolated" e2e test timed out on its 15s viewStates poll at 30c4f0c while 85 other tests passed and the previous head (f91e19c) was green. Log the serialized viewStates write queue outcomes (write/clear/rekey) and snapshot the raw globalState read at the start and timeout of the poll so the next CI run pinpoints where the ask/debug entries go missing. Revert this commit once the cause is found.
The 15s viewStates poll timed out once at 30c4f0c while 85 other e2e tests passed; the same code with diagnostics (16c6c64) ran green, confirming a timing flake rather than a regression. The DIAG logs showed the serialized write queue produced the correct ask/debug entries. Remove the temporary write/clear/rekey and read logging from ClineProvider (back to the 30c4f0c content) and the test, and raise the poll budget from 15s to 30s to match the suite's other waits (waitUntilCompleted, follow-up polling) so a slow memento flush under CI load cannot turn a correct write into a failure.
Restore api-task-control.spec.ts (2-arg handleModeSwitch expectations), api-configuration.spec.ts (providerIdentifiers import), and webviewMessageHandler.spec.ts (single telemetry mock block) from pre-rebase head bac74f1; the rebase re-edit conflict resolutions had downgraded them.
…derer ack postMessageToWebview awaited the webview postMessage promise, which VS Code only settles once the webview page acknowledges the message. When the page is remounted or reloaded, or the view is disposed while the post is in flight, that promise is orphaned forever and every caller awaiting it wedges on the task critical path. This wedged the tab task in the e2e view-state test: switch_mode awaited handleModeSwitch, whose trailing postStateToWebview was blocked on the orphaned ack during a webview remount, so the task's next turn never started and the 30s waitUntilCompleted timed out. Dispatch the post without awaiting the ack (with a rejection catch). Message ordering is enforced by the message seq, not by the ack. Add a unit regression test asserting postMessageToWebview returns without waiting for the renderer ack.
Scope the mode-switch handler to the target task (SwitchModeTool, Task, extension api) so the slug is validated and an unknown slug leaves the task mode untouched instead of recording a bad one. On webviewDidLaunch, re-pin the view to the still-valid shared global profile rather than the first listed profile; add a regression test. Convert the remaining as-any casts in the sticky-mode and parallelMode specs to bracket notation / typed doubles; add a deterministic view-state id fallback test; surface follow-up delivery failures in the e2e view-state diagnostics. Reduce the sticky-mode no-explicit-any suppression count to match the cleanup.
3 participants
Related GitHub Issue
Closes: #984
Description
Add the foundational per-view state infrastructure for parallel mode. This is the root PR that all subsequent parallel-mode PRs depend on.
How:
webview-ui/src/utils/vscode.tsviagetViewStateId(), sent during launch).ClineProvider.setViewStateId()sanitizes it into a safe object key.viewLocalStatebuffer: transient per-view state that merges on top of the sharedContextProxyvalues ingetState()(themergedStateValueslayer), so a tab's mode never overwrites the sidebar's.viewStatespersistence: registered global setting key storing only non-secret selections (mode,currentApiConfigName,updatedAt), bounded to the most recent 50 entries byupdatedAtordering.viewStatesmutation goes through a static write queue (persistedViewStateWriteQueue) that re-reads the map fresh fromglobalStateon each write, so concurrent sidebar/tab providers cannot clobber each other.Reviewers should pay attention to:
mode,currentApiConfigName). FullapiConfiguration(API keys, Kimi Code keys) is never written to globalState; e2e asserts no secret paths leak into persisted entries.viewStatesentries. Retention uses the 50-entry pruning cap. Follow-up#1065tracks explicit stale-entry cleanup.approveTaskAsk,selectTaskFollowupSuggestion) andpreserveOpenTabsfor new tasks. These are required so parallel views can be driven per-task by the orchestrator e2e foundation (test(vscode-e2e): add orchestrator E2E foundation for parallel-mode coverage #1064), which is why they stay in this root PR rather than being split out.Test Procedure
Unit / integration (all green in CI):
pnpm --dir src test— full core suite (129 files / 2184 passed / 9 skipped), including the newClineProvider.parallelMode.spec.tscovering persistence, restoration, isolation, pruning, and concurrent-write serializationpnpm --dir packages/types test(6 passed) andpnpm --dir webview-ui test(viewStateId generation/restoration)pnpm --dir src run check-types, pluspackages/types,webview-ui, andapps/vscode-e2epnpm --dir src exec eslint --prune-suppressions --max-warnings=0 <touched files>— suppression counts unchangedE2E (real VS Code extension host, mock API):
USE_MOCK=true TEST_FILE=view-state.test VSCODE_VERSION=1.100.0 pnpm --dir apps/vscode-e2e run test:runviewStatesentries are visible viaapi.getGlobalState("viewStates")and no secret keys appear in persisted entriesManual verification:
codemode and a new tab task indebugmodecurrentApiConfigNameis unaffectedPre-Submission Checklist
viewStatesis an internal registered setting.Visual Snapshots
Not applicable — no user-visible rendered state changes.
Videos (interaction / animation only)
Not applicable.
Summary by CodeRabbit
New Features
Bug Fixes
Tests