fix(webview): add view-local state base - #1138
Conversation
📝 WalkthroughWalkthroughThe PR adds durable, bounded per-view state for parallel webviews. It propagates stable view identifiers from the webview, persists non-secret mode and API configuration selections, hydrates and merges local state, and updates related tests. ChangesPer-view state persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Webview
participant VSCodeAPIWrapper
participant webviewMessageHandler
participant ClineProvider
participant ContextProxy
participant ProviderSettingsManager
Webview->>VSCodeAPIWrapper: getViewStateId()
VSCodeAPIWrapper-->>Webview: stable viewStateId
Webview->>webviewMessageHandler: webviewDidLaunch(viewStateId)
webviewMessageHandler->>ClineProvider: setViewStateId(viewStateId)
ClineProvider->>ContextProxy: load persisted viewStates
ClineProvider->>ProviderSettingsManager: resolve API configuration
ProviderSettingsManager-->>ClineProvider: provider configuration
ClineProvider-->>webviewMessageHandler: merged extension state
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 💡 1🛠️ Fix failing CI checks 💡
🧪 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
packages/types/src/__tests__/index.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/types/src/global-settings.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. packages/types/src/vscode-extension-host.tsESLint skipped: the ESLint configuration for this file references a package that is not available in 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/webview/webviewMessageHandler.ts (1)
612-626: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear the view-local
currentApiConfigNamewhen the fallback profile is activated.
currentConfigNamenow comes fromprovider.getState(), so it can be the view-local value hydrated fromviewStates. WhenhasConfig(currentConfigName)returnsfalse, this block repairs only shared state:updateGlobalStatecallsprovider.contextProxy.setValueandactivateProviderProfilealso writes throughcontextProxy.setValue. Neither path runs_updateViewLocalStateFromMutation, soviewLocalState.currentApiConfigNamekeeps the invalid name.getState()overlaysviewLocalStateon shared state, so the view continues to report a profile that does not exist.Route the repair through
provider.setValueso the view-local buffer is updated as well.🛠️ Proposed fix
if (currentConfigName) { if (!(await provider.providerSettingsManager.hasConfig(currentConfigName))) { // Current config name not valid, get first config in list. const name = listApiConfig[0]?.name - await updateGlobalState("currentApiConfigName", name) + // Use provider.setValue so the view-local override is updated too, + // otherwise getState() keeps reporting the invalid profile name. + await provider.setValue("currentApiConfigName", name) if (name) { await provider.activateProviderProfile({ name }) return } } }🤖 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 612 - 626, Route the fallback current-config repair in the invalid-currentConfigName branch through provider.setValue so it updates both shared state and the view-local buffer. Replace the updateGlobalState path for currentApiConfigName while preserving the existing fallback activation and return behavior.
🧹 Nitpick comments (3)
src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts (1)
694-695: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse bracket notation instead of
as anyto reach private members.The spec accesses private members through
(provider as any)in roughly thirty places, for examplesaveViewState,loadViewState,prunePersistedViewStates,setViewStateId,viewLocalState, and_clearViewLocalState. Bracket notation keeps the assertions type-checked against the real member names, so a rename breaks the test at compile time instead of silently returningundefined.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards."♻️ Example conversion
- const saveViewState1 = (provider1 as any).saveViewState.bind(provider1) - const saveViewState2 = (provider2 as any).saveViewState.bind(provider2) + const saveViewState1 = provider1["saveViewState"].bind(provider1) + const saveViewState2 = provider2["saveViewState"].bind(provider2)- const pruned = (provider as any).prunePersistedViewStates(states) + const pruned = provider["prunePersistedViewStates"](states)Also applies to: 745-745, 936-936, 1038-1038
🤖 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 694 - 695, Replace all `(provider as any)` private-member accesses in the parallel-mode spec with bracket notation using the actual member names, including saveViewState, loadViewState, prunePersistedViewStates, setViewStateId, viewLocalState, and _clearViewLocalState. Apply this consistently to the roughly thirty affected accesses while preserving the existing test behavior and avoiding any `as any` casts.Source: Coding guidelines
src/core/webview/ClineProvider.ts (1)
554-559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
anycasts with typed keys.
saveViewStatetakesvalue: any, and_updateViewLocalStateFromMutationcasts withval as anyat Line 3084 and Line 3093 and reads(values as any).apiConfigurationat Line 3098. The coding guidelines require typed APIs instead ofas any, and require a comment when a cast is unavoidable. Typed assignments here also catch a mismatch betweenRooCodeSettingsvalues andExtensionStatefields at compile time.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards."♻️ Proposed typing changes
- private async saveViewState(key: keyof ExtensionState, value: any): Promise<void> { + private async saveViewState<K extends keyof ExtensionState>( + key: K, + value: ExtensionState[K] | undefined | null, + ): Promise<void> { if (value === undefined || value === null) { delete this.viewLocalState[key] } else { this.viewLocalState[key] = value }if ("mode" in values) { const val = values.mode if (val === undefined || val === null) { delete this.viewLocalState.mode } else { - this.viewLocalState.mode = val as any + this.viewLocalState.mode = val } } if ("currentApiConfigName" in values) { const val = values.currentApiConfigName if (val === undefined || val === null) { delete this.viewLocalState.currentApiConfigName } else { - this.viewLocalState.currentApiConfigName = val as any + this.viewLocalState.currentApiConfigName = val } } - if ("apiConfiguration" in values) { - const val = (values as any).apiConfiguration + if ("apiConfiguration" in values) { + const val = (values as { apiConfiguration?: ProviderSettings }).apiConfigurationAlso applies to: 3078-3118
🤖 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 554 - 559, Replace the any-typed value and casts in saveViewState and _updateViewLocalStateFromMutation with typed key/value relationships between ExtensionState and RooCodeSettings. Use keyed generic parameters or an explicit type guard for mutation values, access apiConfiguration through its typed shape, and preserve compile-time validation that each RooCodeSettings value matches the corresponding ExtensionState field; only retain a cast if unavoidable and document its justification.Source: Coding guidelines
src/core/webview/__tests__/webviewMessageHandler.spec.ts (1)
233-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWait for the assertion instead of flushing one tick.
webviewDidLaunchstartsproviderSettingsManager.listConfig()as a fire-and-forget chain. A singlesetImmediateflush is sufficient for the current chain length only. If anotherawaitis added to that chain, the test fails intermittently.vi.waitForremoves that coupling.♻️ Proposed change
it("validates the view-local currentApiConfigName on launch", async () => { await webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch", viewStateId: "view-1" }) - await new Promise((resolve) => setImmediate(resolve)) - expect((mockClineProvider as any).providerSettingsManager.hasConfig).toHaveBeenCalledWith("view-local-profile") + await vi.waitFor(() => { + expect((mockClineProvider as any).providerSettingsManager.hasConfig).toHaveBeenCalledWith( + "view-local-profile", + ) + }) expect((mockClineProvider as any).providerSettingsManager.hasConfig).not.toHaveBeenCalledWith("shared-profile") })🤖 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 - 239, Update the “validates the view-local currentApiConfigName on launch” test to wait with vi.waitFor until providerSettingsManager.hasConfig has been called with “view-local-profile”, instead of relying on a single setImmediate flush. Keep the existing assertion that “shared-profile” is not checked.
🤖 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 518-549: The loadViewState method needs three changes to guard
against concurrent execution and prevent loss of in-memory state. First, capture
the current viewStateId at the start of the method and compare it against the
current viewStateId after the async getProfile operation completes - if the id
has changed during the async operations, discard the result without updating.
Second, instead of replacing this.viewLocalState entirely with the loadedState
object, merge the newly loaded state properties into the existing
this.viewLocalState to preserve any values that were previously set by other
code paths (such as mode set elsewhere or apiConfiguration updated by
_updateViewLocalStateFromMutation). This ensures that missing persisted entries
do not reset previously established state values.
---
Outside diff comments:
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 612-626: Route the fallback current-config repair in the
invalid-currentConfigName branch through provider.setValue so it updates both
shared state and the view-local buffer. Replace the updateGlobalState path for
currentApiConfigName while preserving the existing fallback activation and
return behavior.
---
Nitpick comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 694-695: Replace all `(provider as any)` private-member accesses
in the parallel-mode spec with bracket notation using the actual member names,
including saveViewState, loadViewState, prunePersistedViewStates,
setViewStateId, viewLocalState, and _clearViewLocalState. Apply this
consistently to the roughly thirty affected accesses while preserving the
existing test behavior and avoiding any `as any` casts.
In `@src/core/webview/__tests__/webviewMessageHandler.spec.ts`:
- Around line 233-239: Update the “validates the view-local currentApiConfigName
on launch” test to wait with vi.waitFor until providerSettingsManager.hasConfig
has been called with “view-local-profile”, instead of relying on a single
setImmediate flush. Keep the existing assertion that “shared-profile” is not
checked.
In `@src/core/webview/ClineProvider.ts`:
- Around line 554-559: Replace the any-typed value and casts in saveViewState
and _updateViewLocalStateFromMutation with typed key/value relationships between
ExtensionState and RooCodeSettings. Use keyed generic parameters or an explicit
type guard for mutation values, access apiConfiguration through its typed shape,
and preserve compile-time validation that each RooCodeSettings value matches the
corresponding ExtensionState field; only retain a cast if unavoidable and
document its justification.
🪄 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: b53a3839-bb65-4b0b-bdfd-14dc9a7c4822
📒 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
| private async loadViewState(): Promise<void> { | ||
| try { | ||
| const persisted = this.getPersistedViewStates()[this.viewStateId] | ||
| const loadedState: Partial<ExtensionState> = {} | ||
|
|
||
| if (persisted?.mode) { | ||
| loadedState.mode = persisted.mode as Mode | ||
| } | ||
|
|
||
| if (persisted?.currentApiConfigName) { | ||
| loadedState.currentApiConfigName = persisted.currentApiConfigName | ||
|
|
||
| try { | ||
| const { name: _name, ...apiConfiguration } = await this.providerSettingsManager.getProfile({ | ||
| name: persisted.currentApiConfigName, | ||
| }) | ||
| loadedState.apiConfiguration = apiConfiguration as ProviderSettings | ||
| } catch (error) { | ||
| this.log( | ||
| `[loadViewState] Unable to resolve API profile '${persisted.currentApiConfigName}' for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| this.viewLocalState = loadedState | ||
| this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`) | ||
| } catch (error) { | ||
| this.log( | ||
| `[loadViewState] Error loading state for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Guard loadViewState against a changed viewStateId and against replacing in-memory overrides.
Two hazards exist in this method:
-
Ordering hazard. The constructor calls
loadViewState()at Line 296 withviewStateId === viewId(for examplesidebar-0).setViewStateId()later callsloadViewState()again with the stable webview id. Both runsawait providerSettingsManager.getProfile(...), then assignthis.viewLocalState = loadedState. If the first run resolves last, it overwrites the correctly hydrated state. This is reachable becausesavePersistedViewState()persists underviewIdwhenever a mode switch happens before the webview reports its id, andnextViewIdrestarts at 0 in every session, so a stalesidebar-0entry can exist and force the slowgetProfilepath. -
Replacement instead of merge. The final assignment discards any existing
viewLocalStateentries, includingapiConfigurationrecorded by_updateViewLocalStateFromMutationandmodeset at Line 1244. When the persisted entry is missing, the buffer is reset to{}.
Capture the id at entry, discard the result if the id changed, and merge into the existing buffer.
🛠️ Proposed fix
private async loadViewState(): Promise<void> {
+ const requestedViewStateId = this.viewStateId
+
try {
- const persisted = this.getPersistedViewStates()[this.viewStateId]
+ const persisted = this.getPersistedViewStates()[requestedViewStateId]
const loadedState: Partial<ExtensionState> = {}
if (persisted?.mode) {
loadedState.mode = persisted.mode as Mode
}
if (persisted?.currentApiConfigName) {
loadedState.currentApiConfigName = persisted.currentApiConfigName
try {
const { name: _name, ...apiConfiguration } = await this.providerSettingsManager.getProfile({
name: persisted.currentApiConfigName,
})
loadedState.apiConfiguration = apiConfiguration as ProviderSettings
} catch (error) {
this.log(
`[loadViewState] Unable to resolve API profile '${persisted.currentApiConfigName}' for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`,
)
}
}
- this.viewLocalState = loadedState
+ // A newer setViewStateId() call may have superseded this load.
+ if (this.viewStateId !== requestedViewStateId) {
+ this.log(`[loadViewState] Discarding stale load for ${requestedViewStateId}`)
+ return
+ }
+
+ this.viewLocalState = { ...this.viewLocalState, ...loadedState }
this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`)📝 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.
| private async loadViewState(): Promise<void> { | |
| try { | |
| const persisted = this.getPersistedViewStates()[this.viewStateId] | |
| const loadedState: Partial<ExtensionState> = {} | |
| if (persisted?.mode) { | |
| loadedState.mode = persisted.mode as Mode | |
| } | |
| if (persisted?.currentApiConfigName) { | |
| loadedState.currentApiConfigName = persisted.currentApiConfigName | |
| try { | |
| const { name: _name, ...apiConfiguration } = await this.providerSettingsManager.getProfile({ | |
| name: persisted.currentApiConfigName, | |
| }) | |
| loadedState.apiConfiguration = apiConfiguration as ProviderSettings | |
| } catch (error) { | |
| this.log( | |
| `[loadViewState] Unable to resolve API profile '${persisted.currentApiConfigName}' for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | |
| ) | |
| } | |
| } | |
| this.viewLocalState = loadedState | |
| this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`) | |
| } catch (error) { | |
| this.log( | |
| `[loadViewState] Error loading state for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | |
| ) | |
| } | |
| } | |
| private async loadViewState(): Promise<void> { | |
| const requestedViewStateId = this.viewStateId | |
| try { | |
| const persisted = this.getPersistedViewStates()[requestedViewStateId] | |
| const loadedState: Partial<ExtensionState> = {} | |
| if (persisted?.mode) { | |
| loadedState.mode = persisted.mode as Mode | |
| } | |
| if (persisted?.currentApiConfigName) { | |
| loadedState.currentApiConfigName = persisted.currentApiConfigName | |
| try { | |
| const { name: _name, ...apiConfiguration } = await this.providerSettingsManager.getProfile({ | |
| name: persisted.currentApiConfigName, | |
| }) | |
| loadedState.apiConfiguration = apiConfiguration as ProviderSettings | |
| } catch (error) { | |
| this.log( | |
| `[loadViewState] Unable to resolve API profile '${persisted.currentApiConfigName}' for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | |
| ) | |
| } | |
| } | |
| // A newer setViewStateId() call may have superseded this load. | |
| if (this.viewStateId !== requestedViewStateId) { | |
| this.log(`[loadViewState] Discarding stale load for ${requestedViewStateId}`) | |
| return | |
| } | |
| this.viewLocalState = { ...this.viewLocalState, ...loadedState } | |
| this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`) | |
| } catch (error) { | |
| this.log( | |
| `[loadViewState] Error loading state for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | |
| ) | |
| } | |
| } |
🤖 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 518 - 549, The loadViewState
method needs three changes to guard against concurrent execution and prevent
loss of in-memory state. First, capture the current viewStateId at the start of
the method and compare it against the current viewStateId after the async
getProfile operation completes - if the id has changed during the async
operations, discard the result without updating. Second, instead of replacing
this.viewLocalState entirely with the loadedState object, merge the newly loaded
state properties into the existing this.viewLocalState to preserve any values
that were previously set by other code paths (such as mode set elsewhere or
apiConfiguration updated by _updateViewLocalStateFromMutation). This ensures
that missing persisted entries do not reset previously established state values.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
Add the foundational per-view state infrastructure for parallel mode. This is Branch 1 of 3 split from PR #977 for easier review.
Changes
feat(webview): add view-local state base(+1,364 / -97)fix(webview): persist per-view selections(+175 / -49)fix(webview): route mode switches(+43 / -15)chore: remove invisible chars(+1 / -1)~1,583 lines changed
Related
Summary by CodeRabbit
New Features
Bug Fixes
Tests