feat(webview): persist viewStates in ClineProvider with setValues method - #1184
feat(webview): persist viewStates in ClineProvider with setValues method#1184easonLiangWorldedtech wants to merge 2 commits into
Conversation
- Export VSCodeAPIWrapper class and add getViewStateId() method - Generate unique viewStateId using crypto.randomUUID() with fallback - Persist viewStateId in localStorage for dev server compatibility - Add viewStateSchema to global-settings.ts for type safety - Send viewStateId during webviewDidLaunch handshake
- Add setValues method to update view-local state without affecting global settings - Persist mode selection per viewStateId for tab isolation - Update webviewMessageHandler to support new view state flow - Add parallel mode switching tests for sidebar and tab panel
📝 WalkthroughWalkthroughThe PR adds durable per-view webview state for modes and provider profiles. It introduces view-state schemas and identifiers, isolates and persists provider state, updates launch and message handling, improves typing, and adds extensive persistence and multi-instance tests. ChangesPer-view state isolation and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/core/webview/__tests__/webviewMessageHandler.spec.ts (1)
233-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse typed test fixtures or document the double assertions.
The changed tests use
as unknown aswithout documenting why a typed fixture cannot represent the value.
src/core/webview/__tests__/webviewMessageHandler.spec.ts#L233-L242: Use a typed provider-access fixture. Document any required double assertion.src/core/webview/__tests__/webviewMessageHandler.spec.ts#L344-L344: Use a typed task fixture. Document any required double assertion.src/core/webview/__tests__/webviewMessageHandler.spec.ts#L1065-L1095: Use a shared typed task fixture. Document any required double assertion.src/core/webview/__tests__/webviewMessageHandler.spec.ts#L1316-L1316: Document that the cast intentionally sends malformed runtime input.src/core/webview/__tests__/webviewMessageHandler.spec.ts#L1601-L1604: Use a typed diagnostics-task fixture. Document any required double assertion.🤖 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__/webviewMessageHandler.spec.ts` around lines 233 - 242, Replace the untyped double assertions in webviewMessageHandler.spec.ts with typed fixtures: create a typed provider-access fixture at lines 233-242, typed task fixtures at lines 344 and 1065-1095, and a typed diagnostics-task fixture at lines 1601-1604; document any unavoidable double assertions. At line 1316, retain the cast only if it intentionally supplies malformed runtime input and add a comment documenting that purpose.Source: Coding guidelines
src/core/webview/ClineProvider.ts (4)
1927-1943: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the duplicated view-state persistence in
upsertProviderProfile.
saveViewState("currentApiConfigName", name)andsaveViewState("apiConfiguration", providerSettings)already call_saveViewLocalStateFromMutation. Line 1938 then calls the same helper with the same fields. One profile activation therefore enqueues the durable write twice and updates the local cache three times. Keep only the explicit_saveViewLocalStateFromMutationcall and drop the twosaveViewStatecalls from thePromise.all. Note that the test atsrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tsline 1307 assertssaveViewStateis called with"currentApiConfigName", so update that assertion together with this change.🤖 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 1927 - 1943, Remove the two saveViewState calls for "currentApiConfigName" and "apiConfiguration" from the Promise.all in upsertProviderProfile, retaining the explicit _saveViewLocalStateFromMutation call. Update the related parallelMode test assertion to reflect that saveViewState is no longer called with "currentApiConfigName".
642-648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
as anycasts on theon/offoverrides.Both overrides use
(super.on as any)and(super.off as any)with inline ESLint suppressions. Cast the listener to the emitter's expected signature instead, so the suppressions are not needed.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members, or precise test doubles and type guards. Use double assertions only as a last resort and document them." and "Fix lint violations in new TypeScript code rather than suppressing them."♻️ Proposed typed forwarding
override on<K extends keyof TaskProviderEvents>( event: K, listener: (...args: TaskProviderEvents[K]) => void | Promise<void>, ): this { - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - return (super.on as any)(event, listener) + return super.on(event, listener as (...args: TaskProviderEvents[K]) => void) }Apply the same change to
off.Also applies to: 657-658
🤖 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 642 - 648, Update the on and off overrides in ClineProvider to remove the inline ESLint suppressions and `(super.on as any)`/`(super.off as any)` casts. Cast each listener to the emitter’s expected typed listener signature before forwarding to the superclass, preserving the existing event and listener behavior.Source: Coding guidelines
3269-3285: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuild the provider-settings delta without a spread inside
reduce.The reducer allocates a new object for every matching key.
PROVIDER_SETTINGS_KEYSholds a large number of entries, and this runs on everysetValueandsetValuescall. Mutate a single accumulator instead.♻️ Proposed refactor
- const providerSettingsUpdate = PROVIDER_SETTINGS_KEYS.reduce((acc, key) => { - if (key in values) { - return { ...acc, [key]: values[key as keyof RooCodeSettings] } - } - - return acc - }, {} as ProviderSettings) + const providerSettingsUpdate: ProviderSettings = {} + for (const key of PROVIDER_SETTINGS_KEYS) { + if (key in values) { + Object.assign(providerSettingsUpdate, { [key]: values[key as keyof RooCodeSettings] }) + } + }🤖 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 3269 - 3285, Update the providerSettingsUpdate construction in the PROVIDER_SETTINGS_KEYS.reduce callback to assign matching values directly onto the existing accumulator instead of returning a newly spread object for each key. Preserve the current filtering and resulting ProviderSettings shape.
519-573: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unreachable
clearPersistedViewState.No tracked file references
clearPersistedViewState, including TypeScript source, and noclearPersistedViewState(...)call exists. If disposal should not delete persisted view states, keep onlysavePersistedViewState, which already removes an entry when bothmodeandcurrentApiConfigNameare empty.🤖 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 519 - 573, Remove the unused clearPersistedViewState method from ClineProvider. Preserve savePersistedViewState’s existing behavior of deleting the persisted entry when both mode and currentApiConfigName are absent.src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts (1)
890-891: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated assertion.
Lines 890 and 891 assert the same condition. Delete one of them.
♻️ Proposed fix
expect(asProviderAccess(provider).viewLocalState).not.toHaveProperty("mode") - expect(asProviderAccess(provider).viewLocalState).not.toHaveProperty("mode") expect(provider.contextProxy.getValue("viewStates")).toBeUndefined()🤖 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 890 - 891, Remove the duplicated expect assertion in the relevant parallel-mode test, keeping one assertion that viewLocalState does not have the "mode" property.
🤖 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 623-637: Update saveViewState so persistence errors from
_saveViewLocalStateFromMutation propagate instead of being swallowed, allowing
handleModeSwitch to avoid emitting ModeChanged when the view state write fails.
Preserve the existing success logging and error context while ensuring the
in-memory state and task mode remain consistent.
- Line 1315: Persist both view-local mutations: in
src/core/webview/ClineProvider.ts lines 1315-1315, replace the direct mode
assignment with await saveViewState("mode", historyItem.mode); in lines
1987-1991, replace _updateViewLocalStateFromMutation with await
_saveViewLocalStateFromMutation so the deleted profile is removed from the
durable view state.
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 616-617: In the invalid-profile recovery flow around currentState
and currentConfigName, replace the contextProxy.updateGlobalState write for
currentApiConfigName with provider.setValues({ currentApiConfigName: name })
before calling activateProviderProfile, keeping the fallback change local to the
current view.
---
Nitpick comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 890-891: Remove the duplicated expect assertion in the relevant
parallel-mode test, keeping one assertion that viewLocalState does not have the
"mode" property.
In `@src/core/webview/__tests__/webviewMessageHandler.spec.ts`:
- Around line 233-242: Replace the untyped double assertions in
webviewMessageHandler.spec.ts with typed fixtures: create a typed
provider-access fixture at lines 233-242, typed task fixtures at lines 344 and
1065-1095, and a typed diagnostics-task fixture at lines 1601-1604; document any
unavoidable double assertions. At line 1316, retain the cast only if it
intentionally supplies malformed runtime input and add a comment documenting
that purpose.
In `@src/core/webview/ClineProvider.ts`:
- Around line 1927-1943: Remove the two saveViewState calls for
"currentApiConfigName" and "apiConfiguration" from the Promise.all in
upsertProviderProfile, retaining the explicit _saveViewLocalStateFromMutation
call. Update the related parallelMode test assertion to reflect that
saveViewState is no longer called with "currentApiConfigName".
- Around line 642-648: Update the on and off overrides in ClineProvider to
remove the inline ESLint suppressions and `(super.on as any)`/`(super.off as
any)` casts. Cast each listener to the emitter’s expected typed listener
signature before forwarding to the superclass, preserving the existing event and
listener behavior.
- Around line 3269-3285: Update the providerSettingsUpdate construction in the
PROVIDER_SETTINGS_KEYS.reduce callback to assign matching values directly onto
the existing accumulator instead of returning a newly spread object for each
key. Preserve the current filtering and resulting ProviderSettings shape.
- Around line 519-573: Remove the unused clearPersistedViewState method from
ClineProvider. Preserve savePersistedViewState’s existing behavior of deleting
the persisted entry when both mode and currentApiConfigName are absent.
🪄 Autofix
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: 9877f071-bddd-4931-ad13-cd96ebd9c575
📒 Files selected for processing (14)
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.routerModels.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tssrc/eslint-suppressions.jsonwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/utils/__tests__/vscode.spec.tswebview-ui/src/utils/vscode.ts
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
| /** | ||
| * Save a single view-local state value. Only non-secret selections are persisted durably. | ||
| */ | ||
| private async saveViewState(key: keyof ExtensionState, value: unknown): Promise<void> { | ||
| try { | ||
| await this._saveViewLocalStateFromMutation({ [key]: value } as Partial<RooCodeSettings> & | ||
| Partial<ExtensionState>) | ||
|
|
||
| this.log(`[saveViewState] Saved ${String(key)} for viewId ${this.viewId}`) | ||
| } catch (error) { | ||
| this.log( | ||
| `[saveViewState] Error saving state for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
saveViewState swallows persistence failures, so handleModeSwitch can diverge.
saveViewState catches every error and only logs it. _saveViewLocalStateFromMutation persists first and updates viewLocalState second, so a failed write leaves the in-memory mode unchanged. handleModeSwitch (line 1777) depends on this call to apply the new mode. When the write fails, the task's _taskMode was already set at line 1764 and ModeChanged is still emitted at line 1779, but getState().mode keeps the old value. Propagate the failure to handleModeSwitch, or emit ModeChanged only after the view state is applied.
🤖 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 623 - 637, Update
saveViewState so persistence errors from _saveViewLocalStateFromMutation
propagate instead of being swallowed, allowing handleModeSwitch to avoid
emitting ModeChanged when the view state write fails. Preserve the existing
success logging and error context while ensuring the in-memory state and task
mode remain consistent.
| } | ||
|
|
||
| await this.updateGlobalState("mode", historyItem.mode) | ||
| this.viewLocalState.mode = historyItem.mode |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Two mutation sites update the view-local cache without a durable write. Every other mode and profile mutation routes through saveViewState or _saveViewLocalStateFromMutation, which persists mode and currentApiConfigName into the viewStates map. These two sites write only the in-memory cache, so the persisted entry for this view keeps the previous value and a reload restores a stale selection.
src/core/webview/ClineProvider.ts#L1315-L1315: replace the directthis.viewLocalState.mode = historyItem.modeassignment withawait this.saveViewState("mode", historyItem.mode)so the restored mode is durable for this view.src/core/webview/ClineProvider.ts#L1987-L1991: replace_updateViewLocalStateFromMutationwithawait this._saveViewLocalStateFromMutation, otherwise the persisted entry still names the deleted profile andloadViewStatefails to resolve it on the next launch.
📍 Affects 1 file
src/core/webview/ClineProvider.ts#L1315-L1315(this comment)src/core/webview/ClineProvider.ts#L1987-L1991
🤖 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` at line 1315, Persist both view-local
mutations: in src/core/webview/ClineProvider.ts lines 1315-1315, replace the
direct mode assignment with await saveViewState("mode", historyItem.mode); in
lines 1987-1991, replace _updateViewLocalStateFromMutation with await
_saveViewLocalStateFromMutation so the deleted profile is removed from the
durable view state.
| const currentState = await provider.getState() | ||
| const currentConfigName = currentState.currentApiConfigName |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep invalid-profile recovery view-local.
Line 616 reads currentApiConfigName from the view-local provider state. Line 623 then writes the fallback name through updateGlobalState. One view with a stale profile can therefore change the selected profile for other views.
Use provider.setValues({ currentApiConfigName: name }) before activateProviderProfile instead of writing currentApiConfigName through contextProxy.
Proposed fix
- await updateGlobalState("currentApiConfigName", name)
+ await provider.setValues({ currentApiConfigName: name })🤖 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/webviewMessageHandler.ts` around lines 616 - 617, In the
invalid-profile recovery flow around currentState and currentConfigName, replace
the contextProxy.updateGlobalState write for currentApiConfigName with
provider.setValues({ currentApiConfigName: name }) before calling
activateProviderProfile, keeping the fallback change local to the current view.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
Summary
Add
setValues()method in ClineProvider to implement per-view state persistence. Each tab/sidebar panel can independently save mode, API profile selections without interfering with each other.Changes
setValues()method — Update view-local state without affecting global settingsFiles Changed (8 files, +1930 / -120)
ClineProvider.parallelMode.spec.tswebviewMessageHandler.spec.tsExtensionStateContext.spec.tsxClineProvider.spec.tsClineProvider.sticky-mode.spec.tswebviewMessageHandler.routerModels.spec.tswebviewMessageHandler.tssrc/eslint-suppressions.jsonKey Design
setValues()only updates state for the current viewStateId, does not trigger global state changesTest Notes
setValues()method that base-1's persistence logic depends on. The infrastructure is in place but not yet wired to actual state updates.Related