fix(webview): persistence logic + API sync (branch 2/3) - #1139
fix(webview): persistence logic + API sync (branch 2/3)#1139easonLiangWorldedtech wants to merge 13 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds stable view-state identifiers and durable per-view state for webview instances. ChangesPer-view state isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ExtensionStateContext
participant VSCodeAPIWrapper
participant webviewMessageHandler
participant ClineProvider
participant GlobalState
ExtensionStateContext->>VSCodeAPIWrapper: Get stable viewStateId
VSCodeAPIWrapper-->>ExtensionStateContext: Return viewStateId
ExtensionStateContext->>webviewMessageHandler: Send webviewDidLaunch(viewStateId)
webviewMessageHandler->>ClineProvider: Store viewStateId
ClineProvider->>GlobalState: Persist viewStates
ClineProvider-->>webviewMessageHandler: Return merged state
webviewMessageHandler-->>ExtensionStateContext: Post view-local mode and API profile
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 7
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/ClineProvider.ts (1)
3090-3113: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep direct
ContextProxymutations in sync withviewLocalState.
getValue()overlaysviewLocalState, but writes throughprovider.contextProxy.setValue,setValues, andsetProviderSettingsbypassClineProvider.setValue/setValuesand do not call_saveViewLocalStateFromMutation. This happens in the settings import flow, provider-profile creation/update, profile activation, profile deletion, and the SettingsupdateSettingshandler. Route these writes throughprovider.setValue/setValues, or invalidate the affectedviewLocalStatekeys fromContextProxy.🤖 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 3090 - 3113, Update the direct ContextProxy write paths in the settings import flow, provider-profile creation/update, profile activation/deletion, and Settings updateSettings handler to use ClineProvider.setValue or setValues so _saveViewLocalStateFromMutation keeps viewLocalState synchronized. Locate these callers via contextProxy.setValue, contextProxy.setValues, and setProviderSettings, while preserving existing mutation values and behavior.
🧹 Nitpick comments (7)
src/core/webview/ClineProvider.ts (3)
565-573: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
saveViewStategenerically instead of usingany.
value: anyand theaschain at lines 569-570 add to the@typescript-eslint/no-explicit-anycount for this file, which rose from 12 to 16 insrc/eslint-suppressions.json. A generic key parameter removes both.♻️ Proposed change
- private async saveViewState(key: keyof ExtensionState, value: any): Promise<void> { - await this._saveViewLocalStateFromMutation({ [key]: value } as Partial<RooCodeSettings> & - Partial<ExtensionState>) + private async saveViewState<K extends keyof ExtensionState>(key: K, value: ExtensionState[K]): Promise<void> { + await this._saveViewLocalStateFromMutation({ [key]: value })The coding guidelines state: "Suppression counts in
src/eslint-suppressions.jsonmust never increase; when touching a file, reduce its count when the fix is local and low-risk."🤖 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 565 - 573, Update saveViewState to use a generic key parameter constrained to keyof ExtensionState and derive the value type from that key, so callers receive key-specific typing. Replace the current any value and assertion chain passed to _saveViewLocalStateFromMutation with a type-safe mapped object compatible with its expected partial state types, without increasing eslint suppressions.Source: Coding guidelines
3146-3162: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuild the provider-settings delta without a spread inside
reduce.The accumulator is re-allocated for every matching key, which makes the reduction quadratic in the number of
PROVIDER_SETTINGS_KEYS. This runs on everysetValueandsetValuescall. Mutate a single accumulator instead.♻️ Proposed change
- 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) { + ;(providerSettingsUpdate as Record<string, unknown>)[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 3146 - 3162, Update the providerSettingsUpdate construction in the setValue/setValues handling branch to mutate one accumulator when a key exists in values, instead of returning a new object with a spread from reduce. Preserve the existing ProviderSettings result and subsequent apiConfiguration merge behavior.
1257-1258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the restored mode through the view-local persistence helper.
Line 1258 writes
this.viewLocalState.modedirectly. Every other mode mutation in this class now goes throughsaveViewStateor_saveViewLocalStateFromMutation, which also persist theviewStatesentry. Today the value survives a reload only because line 1257 still writes the shared globalmode. Use the helper so the two paths stay consistent.♻️ Proposed change
await this.updateGlobalState("mode", historyItem.mode) - this.viewLocalState.mode = historyItem.mode + await this.saveViewState("mode", historyItem.mode)🤖 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 1257 - 1258, Update the history restoration flow around the mode assignment in ClineProvider so restored modes use the class’s view-local persistence helper, such as saveViewState or _saveViewLocalStateFromMutation, instead of directly assigning this.viewLocalState.mode. Preserve the existing global-state update while ensuring the corresponding viewStates entry is persisted consistently.src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts (1)
872-881: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that a
viewStateswrite happened before inspecting the last call.If no
viewStateswrite occurs,lastViewStateCallisundefinedand the optional chaining turns the failure intoexpect(undefined).toMatchObject(...). That hides the actual cause. Add an explicit check first.💚 Proposed change
const lastViewStateCall = viewStateCalls[viewStateCalls.length - 1] + expect(viewStateCalls.length).toBeGreaterThan(0) + // Verify the last mode switch wins expect(lastViewStateCall?.[1]).toMatchObject({ [provider.viewId]: { mode: "code" }, })🤖 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.sticky-mode.spec.ts` around lines 872 - 881, In the sticky-mode test, add an explicit assertion after collecting viewStatesCalls and before reading lastViewStateCall to verify at least one write occurred. Keep the existing last-call inspection and mode assertion unchanged once the non-empty condition is established.webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx (1)
89-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unnecessary
as anycast.
Modeis declared asexport type Mode = stringinsrc/shared/modes.ts, sosetMode("ask")type-checks without a cast. The coding guidelines ask you to avoidas anyand to document any unavoidable cast.♻️ Proposed change
- <button data-testid="set-local-mode" onClick={() => setMode("ask" as any)}> + <button data-testid="set-local-mode" onClick={() => setMode("ask")}>🤖 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 `@webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx` at line 89, Remove the unnecessary as any cast from the setMode call in the set-local-mode test button, passing "ask" directly because it satisfies the Mode type.Source: Coding guidelines
src/core/webview/__tests__/webviewMessageHandler.spec.ts (1)
234-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the single-tick flush with
vi.waitFor.
webviewMessageHandlerdoes not await theproviderSettingsManager.listConfig().then(...)chain, sohasConfigruns on a later microtask/macrotask. A singlesetImmediatetick happens to be enough today, but the assertion breaks if any additionalawaitis added inside that chain. Usevi.waitForso the test waits for the observable effect.♻️ Proposed change
await webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch", viewStateId: "view-1" }) - await new Promise((resolve) => setImmediate(resolve)) - expect((mockClineProvider as any).setViewStateId).toHaveBeenCalledWith("view-1") - 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 234 - 239, In the webviewDidLaunch test around webviewMessageHandler, replace the single setImmediate-based flush with vi.waitFor that waits until providerSettingsManager.hasConfig has been called with "view-local-profile". Keep the existing assertions for setViewStateId and rejection of "shared-profile", ensuring assertions run only after the observable asynchronous effect completes.src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts (1)
694-695: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftIntroduce one typed accessor for the private members instead of repeating
(provider as any).This file adds 143
@typescript-eslint/no-explicit-anysuppressions insrc/eslint-suppressions.json, and most come from(provider as any)reads of private members such assaveViewState,setViewStateId,loadViewState,viewLocalState,prunePersistedViewStates, and_clearViewLocalState. The test at lines 849-853 already shows the pattern that avoids the cast. Declare that shape once and reuse it, or use bracket notation for the private members.♻️ Proposed helper
type ProviderInternals = { saveViewState: (key: keyof ExtensionState, value: unknown) => Promise<void> setViewStateId: (viewStateId: string) => Promise<void> loadViewState: () => Promise<void> prunePersistedViewStates: (states: Record<string, unknown>) => Record<string, unknown> _clearViewLocalState: () => void viewLocalState: Partial<ExtensionState> } const internals = (provider: ClineProvider) => provider as unknown as ProviderInternalsThe coding guidelines state: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards."Also applies to: 717-718, 1002-1002
🤖 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 repeated `(provider as any)` accesses in the parallel-mode tests with one typed `ProviderInternals` accessor, based on the existing pattern near the later test. Reuse it for private members including `saveViewState`, `setViewStateId`, `loadViewState`, `viewLocalState`, `prunePersistedViewStates`, and `_clearViewLocalState`, using `unknown` rather than `any` and removing the corresponding eslint suppressions.Source: Coding guidelines
🤖 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/__tests__/webviewMessageHandler.spec.ts`:
- Around line 217-230: Replace the per-property `as any` assignments in the test
setup with typed members declared on the `mockClineProvider` object literal,
including `setViewStateId`, `workspaceTracker`, `providerSettingsManager`,
`activateProviderProfile`, `getMcpHub`, and `getStateToPostToWebview`. In
`beforeEach`, reset these members through `vi.mocked(...)`; retain the existing
`as unknown as ClineProvider` cast on the full literal and remove the avoidable
explicit-any usage.
In `@src/core/webview/ClineProvider.ts`:
- Around line 1899-1903: Update deleteProviderProfile to call
_saveViewLocalStateFromMutation instead of only
_updateViewLocalStateFromMutation when replacing the deleted profile, so the new
currentApiConfigName and listApiConfigMeta are persisted in viewStates. Preserve
the existing mutation values and behavior for the in-memory cache.
- Around line 532-563: Update loadViewState to merge hydrated fields into the
existing this.viewLocalState rather than replacing the buffer, preserving live
per-view selections and only assigning values that were actually persisted and
resolved. Ensure both constructor-triggered hydration and setViewStateId
hydration cannot overwrite mutations made while getProfile is pending; if
replacement semantics remain necessary for setViewStateId, explicitly sequence
the constructor’s loadViewState promise before getState.
- Around line 517-526: Update setViewStateId to clean up the constructor-default
viewId entry when switching to the webview-reported ID, while preserving any
useful persisted values by migrating them if required before loading the new
state. Ensure cleanup only targets the stale ephemeral entry and does not remove
the newly selected viewStateId data.
- Around line 3215-3219: Update resetState() to clear view-local state for every
open ClineProvider in activeInstances, not only the current instance. Iterate
over activeInstances and invoke each provider’s _clearViewLocalState() after
resetting shared state, preserving the existing current-instance behavior
without leaving per-view mode or API profile overrides cached.
In `@src/eslint-suppressions.json`:
- Around line 1062-1076: Replace the any-typed saveViewState signature in
src/core/webview/ClineProvider.ts:565-573 with a generic key/value relationship
using ExtensionState[K], and remove the Partial<RooCodeSettings> &
Partial<ExtensionState> cast. In
src/core/webview/__tests__/webviewMessageHandler.spec.ts:217-230, declare the
listed provider members directly on mockClineProvider instead of assigning
through an any cast. After both fixes, update
src/eslint-suppressions.json:1062-1076 to restore ClineProvider.ts to 12
suppressions and webviewMessageHandler.spec.ts to 35; the parallelMode entry
requires no direct change.
In `@webview-ui/src/utils/__tests__/vscode.spec.ts`:
- Line 21: Replace the double assertions at the storage mock definitions in the
vscode tests with a narrow type such as Pick<Storage, "getItem" | "setItem" |
"removeItem" | "clear"> or an equivalent local mock interface, and update both
affected mocks to use it without casting to Storage.
---
Outside diff comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 3090-3113: Update the direct ContextProxy write paths in the
settings import flow, provider-profile creation/update, profile
activation/deletion, and Settings updateSettings handler to use
ClineProvider.setValue or setValues so _saveViewLocalStateFromMutation keeps
viewLocalState synchronized. Locate these callers via contextProxy.setValue,
contextProxy.setValues, and setProviderSettings, while preserving existing
mutation values and behavior.
---
Nitpick comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 694-695: Replace repeated `(provider as any)` accesses in the
parallel-mode tests with one typed `ProviderInternals` accessor, based on the
existing pattern near the later test. Reuse it for private members including
`saveViewState`, `setViewStateId`, `loadViewState`, `viewLocalState`,
`prunePersistedViewStates`, and `_clearViewLocalState`, using `unknown` rather
than `any` and removing the corresponding eslint suppressions.
In `@src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts`:
- Around line 872-881: In the sticky-mode test, add an explicit assertion after
collecting viewStatesCalls and before reading lastViewStateCall to verify at
least one write occurred. Keep the existing last-call inspection and mode
assertion unchanged once the non-empty condition is established.
In `@src/core/webview/__tests__/webviewMessageHandler.spec.ts`:
- Around line 234-239: In the webviewDidLaunch test around
webviewMessageHandler, replace the single setImmediate-based flush with
vi.waitFor that waits until providerSettingsManager.hasConfig has been called
with "view-local-profile". Keep the existing assertions for setViewStateId and
rejection of "shared-profile", ensuring assertions run only after the observable
asynchronous effect completes.
In `@src/core/webview/ClineProvider.ts`:
- Around line 565-573: Update saveViewState to use a generic key parameter
constrained to keyof ExtensionState and derive the value type from that key, so
callers receive key-specific typing. Replace the current any value and assertion
chain passed to _saveViewLocalStateFromMutation with a type-safe mapped object
compatible with its expected partial state types, without increasing eslint
suppressions.
- Around line 3146-3162: Update the providerSettingsUpdate construction in the
setValue/setValues handling branch to mutate one accumulator when a key exists
in values, instead of returning a new object with a spread from reduce. Preserve
the existing ProviderSettings result and subsequent apiConfiguration merge
behavior.
- Around line 1257-1258: Update the history restoration flow around the mode
assignment in ClineProvider so restored modes use the class’s view-local
persistence helper, such as saveViewState or _saveViewLocalStateFromMutation,
instead of directly assigning this.viewLocalState.mode. Preserve the existing
global-state update while ensuring the corresponding viewStates entry is
persisted consistently.
In `@webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx`:
- Line 89: Remove the unnecessary as any cast from the setMode call in the
set-local-mode test button, passing "ask" directly because it satisfies the Mode
type.
🪄 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: 14442d3b-62d6-44d7-85a0-264c45a0d1d0
📒 Files selected for processing (18)
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.tssrc/eslint-suppressions.jsonsrc/extension/__tests__/api-set-configuration.spec.tssrc/extension/api.tswebview-ui/src/App.tsxwebview-ui/src/__tests__/App.spec.tsxwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/context/__tests__/ExtensionStateContext.spec.tsxwebview-ui/src/utils/__tests__/vscode.spec.tswebview-ui/src/utils/vscode.ts
💤 Files with no reviewable changes (1)
- webview-ui/src/App.tsx
| ;(mockClineProvider as any).setViewStateId = vi.fn().mockResolvedValue(undefined) | ||
| ;(mockClineProvider as any).workspaceTracker = { initializeFilePaths: vi.fn() } | ||
| ;(mockClineProvider as any).providerSettingsManager = { | ||
| listConfig: vi.fn().mockResolvedValue([{ name: "shared-profile", apiProvider: "anthropic" }]), | ||
| hasConfig: vi.fn().mockResolvedValue(false), | ||
| } | ||
| ;(mockClineProvider as any).activateProviderProfile = vi.fn().mockResolvedValue(undefined) | ||
| ;(mockClineProvider as any).getMcpHub = vi.fn().mockReturnValue(undefined) | ||
| ;(mockClineProvider as any).getStateToPostToWebview = vi | ||
| .fn() | ||
| .mockResolvedValue({ telemetrySetting: "disabled" }) | ||
| vi.mocked(mockClineProvider.customModesManager.getCustomModes).mockResolvedValue([]) | ||
| vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue("shared-profile") | ||
| vi.mocked(mockClineProvider.contextProxy.setValue).mockResolvedValue(undefined) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Declare the new launch dependencies on the mockClineProvider literal instead of casting.
Lines 217-227 attach setViewStateId, workspaceTracker, providerSettingsManager, activateProviderProfile, getMcpHub, and getStateToPostToWebview through as any. Those casts are part of the reason the @typescript-eslint/no-explicit-any count for this file rose from 35 to 45 in src/eslint-suppressions.json. Add the members to the mockClineProvider object literal at lines 91-119 and reset them in beforeEach with vi.mocked(...). The literal is already narrowed with as unknown as ClineProvider, so no per-property cast is needed.
The coding guidelines state: "Avoid as any" and "Suppression counts in src/eslint-suppressions.json must never increase; when touching a file, reduce its count when the fix is local and low-risk."
🤖 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 217 -
230, Replace the per-property `as any` assignments in the test setup with typed
members declared on the `mockClineProvider` object literal, including
`setViewStateId`, `workspaceTracker`, `providerSettingsManager`,
`activateProviderProfile`, `getMcpHub`, and `getStateToPostToWebview`. In
`beforeEach`, reset these members through `vi.mocked(...)`; retain the existing
`as unknown as ClineProvider` cast on the full literal and remove the avoidable
explicit-any usage.
Source: Coding guidelines
| public async setViewStateId(viewStateId: string | undefined): Promise<void> { | ||
| const normalizedViewStateId = viewStateId?.trim() | ||
|
|
||
| if (!normalizedViewStateId || normalizedViewStateId === this.viewStateId) { | ||
| return | ||
| } | ||
|
|
||
| this.viewStateId = normalizedViewStateId.replace(/[^A-Za-z0-9_-]/g, "_") | ||
| await this.loadViewState() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find provider mutations that can run before webviewDidLaunch and therefore persist under the ephemeral viewId.
set -euo pipefail
# Locate setViewStateId and the persistence helpers.
ast-grep outline src/core/webview/ClineProvider.ts --items all --match 'setViewStateId|loadViewState|savePersistedViewState|clearPersistedViewState|resolveWebviewView'
# Find call sites that mutate provider state and could precede the launch handshake.
rg -nP -C4 '\b(setViewStateId|resolveWebviewView)\s*\(' --type=ts src
rg -nP -C3 'provider\.(setValue|setValues|activateProviderProfile|upsertProviderProfile|handleModeSwitch)\s*\(' --type=ts src -g '!**/__tests__/**'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ClineProvider outline matching public/mutating symbols =="
ast-grep outline src/core/webview/ClineProvider.ts --view expanded | rg -n 'public|private|constructor|viewStateId|saveViewState|loadViewState|clearPersistedViewState|resolveWebviewView|getValue|setValue|setValues|handleModeSwitch|activateProviderProfile|upsertProviderProfile' | head -n 200 || true
echo
echo "== Relevant ClineProvider sections =="
sed -n '430,535p' src/core/webview/ClineProvider.ts
sed -n '1030,1125p' src/core/webview/ClineProvider.ts
echo
echo "== View-id initialization and save/load helpers =="
rg -n -C3 'viewStateId\s*=|viewId\s*=|generateUniqueId|saveViewState|loadViewState|clearPersistedViewState|MAX_PERSISTED_VIEW_STATES|setViewStateId' src/core/webview/ClineProvider.ts
echo
echo "== Pre-launch mutation call sites excluding tests =="
rg -n -C3 'provider\.(setValue|setValues|handleModeSwitch|activateProviderProfile|upsertProviderProfile|setValue\()' src -g '*.ts' -g '!**/__tests__/**' | head -n 240Repository: Zoo-Code-Org/Zoo-Code
Length of output: 26258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== webviewDidLaunch and message handler sequencing =="
sed -n '535,585p' src/core/webview/webviewMessageHandler.ts
sed -n '1,80p' src/core/webview/webviewMessageHandler.ts
echo
echo "== All viewStateId usages excluding tests =="
rg -n -C2 'viewStateId|setViewStateId|viewStates\]' src -g '*.ts' -g '!**/__tests__/**' | head -n 220
echo
echo "== ClineProvider constructor and resolve context =="
sed -n '230,310p' src/core/webview/ClineProvider.ts
sed -n '1030,1065p' src/core/webview/ClineProvider.ts
echo
echo "== Programmatic check: mutating methods use current viewStateId before switch =="
python3 - <<'PY'
from pathlib import Path
text = Path('src/core/webview/ClineProvider.ts').read_text()
start = text.index("\n\t\tif (!alreadyMigrated)")
end = text.index("\n\t\tprivate async saveViewState")
init = text[start:end]
target_index = text.index("\n\tpublic async setViewStateId")
save_start = text.index("\n\tprivate async saveViewState", target_index)
save_load = text[target_index:save_start]
print("init_contains_constructor_default_viewStateId=", "this.viewStateId = this.viewId" in init)
print("viewStateId_assigned_before_saveViewState=", text.find("this.viewStateId = this.viewId") < text.find("private async saveViewState"))
print("saveViewState_uses_viewStateId_field=", "this.viewStateId" in save_load)
print("persisted_save_uses_viewStateId_field=", "constants saved by getPersistedViewStates(this.viewStateId)" in open('src/core/webview/ClineProvider.ts').read())
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 14377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== webviewDidLaunch message construction and send sites =="
rg -n -C4 'webviewDidLaunch|postStateToWebview|setState|sendHtml|setWebviewMessageListener|sendMessage|viewStateId' src -g '*.ts' -g '!**/__tests__/**' | head -n 260
echo
echo "== ClineProvider mutation persistence helpers =="
sed -n '3080,3225p' src/core/webview/ClineProvider.ts
echo
echo "== handleModeSwitch and activation-related mutation helpers =="
sed -n '1665,1715p' src/core/webview/ClineProvider.ts
sed -n '1030,1255p' src/core/webview/ClineProvider.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 31712
Remove stale pre-launch view-state entries when migrating to the webview-reported id.
setViewStateId is called from webviewDidLaunch, but mutations such as handleModeSwitch can persist mode and upsertProviderProfile can persist currentApiConfigName under the constructor default viewId before that handshake. setViewStateId switches viewStateId and loads the new key without removing the old one; only disposal/closing later clears the current viewStateId, which is already the new non-ephemeral id. When switching IDs, migrate useful persisted values if needed, or at least clear the ephemeral viewId entry to avoid accumulating discarded view-state keys.
🤖 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 517 - 526, Update
setViewStateId to clean up the constructor-default viewId entry when switching
to the webview-reported ID, while preserving any useful persisted values by
migrating them if required before loading the new state. Ensure cleanup only
targets the stale ephemeral entry and does not remove the newly selected
viewStateId data.
| 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.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
loadViewState replaces the whole viewLocalState buffer and can discard live per-view selections.
Line 556 assigns this.viewLocalState = loadedState, so every key not present in the persisted entry is dropped. Two reachable paths lose state:
- The constructor calls
void this.loadViewState()at line 297 without awaiting it, andloadViewStateawaitsproviderSettingsManager.getProfile. Any mutation that lands in that window —setValue,setValues,activateProviderProfile, orupsertProviderProfile, all of which now write intoviewLocalState— is overwritten when line 556 runs. setViewStateIdcallsloadViewStateonwebviewDidLaunch.loadedState.apiConfigurationis only populated when the persisted entry carries a resolvablecurrentApiConfigName. If it does not, a previously syncedviewLocalState.apiConfigurationis dropped andgetState()silently falls back to the shared provider settings.
Merge the hydrated values over the existing buffer instead of replacing it, and only overwrite keys that hydration actually resolved.
🐛 Proposed fix
- this.viewLocalState = loadedState
+ this.viewLocalState = { ...this.viewLocalState, ...loadedState }
this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`)If replacement is intentional for the setViewStateId path, sequence the constructor hydration explicitly (for example, store the promise and await it in getState) so it cannot race later mutations.
🤖 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 532 - 563, Update
loadViewState to merge hydrated fields into the existing this.viewLocalState
rather than replacing the buffer, preserving live per-view selections and only
assigning values that were actually persisted and resolved. Ensure both
constructor-triggered hydration and setViewStateId hydration cannot overwrite
mutations made while getProfile is pending; if replacement semantics remain
necessary for setViewStateId, explicitly sequence the constructor’s
loadViewState promise before getState.
| this._updateViewLocalStateFromMutation({ | ||
| currentApiConfigName: profileToActivate, | ||
| listApiConfigMeta: entries, | ||
| }) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
deleteProviderProfile updates the cache but does not persist the replacement profile.
upsertProviderProfile (line 1851) and activateProviderProfile (line 1953) both call _saveViewLocalStateFromMutation, which persists currentApiConfigName into the viewStates entry. Deletion calls _updateViewLocalStateFromMutation, which only updates the in-memory cache.
The persisted entry therefore keeps naming the deleted profile. After a window reload, loadViewState reads that name, providerSettingsManager.getProfile throws, the error is only logged, and viewLocalState.currentApiConfigName is left pointing at a profile that no longer exists. getStateToPostToWebview then reports that stale name to the webview.
🐛 Proposed fix
- this._updateViewLocalStateFromMutation({
- currentApiConfigName: profileToActivate,
- listApiConfigMeta: entries,
- })
+ await this._saveViewLocalStateFromMutation({
+ currentApiConfigName: profileToActivate,
+ listApiConfigMeta: entries,
+ })📝 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.
| this._updateViewLocalStateFromMutation({ | |
| currentApiConfigName: profileToActivate, | |
| listApiConfigMeta: entries, | |
| }) | |
| await this._saveViewLocalStateFromMutation({ | |
| currentApiConfigName: profileToActivate, | |
| listApiConfigMeta: entries, | |
| }) |
🤖 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 1899 - 1903, Update
deleteProviderProfile to call _saveViewLocalStateFromMutation instead of only
_updateViewLocalStateFromMutation when replacing the deleted profile, so the new
currentApiConfigName and listApiConfigMeta are persisted in viewStates. Preserve
the existing mutation values and behavior for the in-memory cache.
| await this.contextProxy.resetAllState() | ||
|
|
||
| // Clear view-local state cache so getState() falls back to ContextProxy defaults. | ||
| this._clearViewLocalState() | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine whether resetAllState clears the viewStates key and whether other provider instances are reset.
set -euo pipefail
# Inspect resetAllState and the key set it clears.
ast-grep outline src/core/config/ContextProxy.ts --items all --match 'resetAllState|GLOBAL_STATE_KEYS|globalSettings'
rg -nP -C10 'resetAllState' --type=ts src/core/config
# Confirm viewStates is declared as a global settings key.
rg -nP -C4 '\bviewStates\b' packages/types/src src/core/config
# Check whether resetState fans out to other instances.
rg -nP -C6 'activeInstances|getAllInstances' --type=ts src/core/webview/ClineProvider.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 12821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the referenced symbols and inspect compact relevant slices.
printf '\n--- ClineProvider reset/clear/load views slices ---\n'
rg -n -C 8 'resetState\(|_clearViewLocalState\(|loadViewState|viewLocalState|clearViews|activeInstances\.forEach|getAllInstances' --type=ts src/core/webview/ClineProvider.ts
printf '\n--- ContextProxy resetAllState slice ---\n'
sed -n '560,620p' src/core/config/ContextProxy.ts
printf '\n--- global-settings relevant keys ---\n'
sed -n '1,80p' packages/types/src/global-settings.ts
sed -n '80,135p' packages/types/src/global-settings.ts
printf '\n--- deterministic source verifier ---\n'
python3 - <<'PY'
from pathlib import Path
import re
cp = Path("src/core/webview/ClineProvider.ts").read_text()
cx = Path("src/core/config/ContextProxy.ts").read_text()
reset = cp[cp.find("resetState("):cp.find("resetState(", cp.find("resetState(")+1)]
clear = cp[cp.find("_clearViewLocalState("):cp.find("_clearViewLocalState(", cp.find("_clearViewLocalState(")+1)] if cp.count("_clearViewLocalState(") == 1 else cp[cp.find("_clearViewLocalState("):]
reset_all = cx[cx.find("public async resetAllState"):cx.find("public async resetAllState")+2000]
checks = {
"resetState calls contextProxy.resetAllState": "this.contextProxy.resetAllState()" in cp[cp.find("resetState("):cp.find("resetState(", cp.find("resetState(")+1)+1200] if "resetState(" in cp else False,
"resetState calls this._clearViewLocalState": "_clearViewLocalState()" in cp[cp.find("resetState("):cp.find("resetState(", cp.find("resetState(")+1)+1200] if "resetState(" in cp else False,
"resetState contains activeInstances or getAllInstances reference": any(id_ in cp[cp.find("resetState("):cp.find("resetState(", cp.find("resetState(")+1200)] for id_ in ("activeInstances", "getAllInstances")),
"_clearViewLocalState targets other providers": bool(re.search(r"activeInstances|getAllInstances|ClineProvider|viewLocalState", clear, re.S) and "other" in clear.lower()),
"resetAllState uses GLOBAL_STATE_KEYS.update(undefined)": bool(re.search(r"\.\.\.GLOBAL_STATE_KEYS\.map\(\s*key\s*=>\s*this\.originalContext\.globalState\.update\s*\(\s*key\s*,\s*undefined\s*\)", reset_all, re.S)),
"resetAllState uses SECRET_STATE_KEYS.delete": bool(re.search(r"\.\.\.SECRET_STATE_KEYS\.map\(\s*key\s*=>\s*this\.originalContext\.secrets\.delete\s*\(\s*key\s*\)", reset_all, re.S)),
"resetAllState uses GLOBAL_SECRET_KEYS.delete": bool(re.search(r"\.\.\.GLOBAL_SECRET_KEYS\.map\(\s*key\s*=>\s*this\.originalContext\.secrets\.delete\s*\(\s*key\s*\)", reset_all, re.S)),
}
for key, val in checks.items():
print(f"{key}: {val}")
# Print concise snippets for evidence.
print("\n-- resetState snippet --")
idx = cp.find("resetState(")
print(cp[idx:idx+1200] if idx != -1 else "NOT_FOUND")
print("\n-- _clearViewLocalState snippet --")
idx = cp.find("_clearViewLocalState(")
print(cp[idx:idx+500] if idx != -1 else "NOT_FOUND")
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 19287
Reset per-view state for every open ClineProvider.
resetState() updates viewStates and clears this.viewLocalState, but other open provider instances keep their own ClineProvider.viewLocalState. Those instances still merge per-view mode / API profile into getState() after reset, so resetState should also clear cached state for each instance in activeInstances.
🤖 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 3215 - 3219, Update
resetState() to clear view-local state for every open ClineProvider in
activeInstances, not only the current instance. Iterate over activeInstances and
invoke each provider’s _clearViewLocalState() after resetting shared state,
preserving the existing current-instance behavior without leaving per-view mode
or API profile overrides cached.
| "core/webview/ClineProvider.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 12 | ||
| "count": 16 | ||
| } | ||
| }, | ||
| "core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 34 | ||
| } | ||
| }, | ||
| "core/webview/__tests__/ClineProvider.parallelMode.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 143 | ||
| } | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
New untyped code raises the ESLint suppression baseline for two existing files. core/webview/ClineProvider.ts goes from 12 to 16 and core/webview/__tests__/webviewMessageHandler.spec.ts goes from 35 to 45. The shared root cause is new any usage that has a local, low-risk typed alternative; fix the two source sites and restore both baselines.
src/eslint-suppressions.json#L1062-L1076: restorecore/webview/ClineProvider.tsto 12 andcore/webview/__tests__/webviewMessageHandler.spec.tsto 35 after the two fixes below land.src/core/webview/ClineProvider.ts#L565-L573: replacesaveViewState(key: keyof ExtensionState, value: any)with a generic<K extends keyof ExtensionState>(key: K, value: ExtensionState[K])and drop theas Partial<RooCodeSettings> & Partial<ExtensionState>cast.src/core/webview/__tests__/webviewMessageHandler.spec.ts#L217-L230: declaresetViewStateId,workspaceTracker,providerSettingsManager,activateProviderProfile,getMcpHub, andgetStateToPostToWebviewon themockClineProviderliteral instead of assigning them through(mockClineProvider as any).
The coding guidelines state: "Suppression counts in src/eslint-suppressions.json must never increase; when touching a file, reduce its count when the fix is local and low-risk."
📍 Affects 3 files
src/eslint-suppressions.json#L1062-L1076(this comment)src/core/webview/ClineProvider.ts#L565-L573src/core/webview/__tests__/webviewMessageHandler.spec.ts#L217-L230
🤖 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/eslint-suppressions.json` around lines 1062 - 1076, Replace the any-typed
saveViewState signature in src/core/webview/ClineProvider.ts:565-573 with a
generic key/value relationship using ExtensionState[K], and remove the
Partial<RooCodeSettings> & Partial<ExtensionState> cast. In
src/core/webview/__tests__/webviewMessageHandler.spec.ts:217-230, declare the
listed provider members directly on mockClineProvider instead of assigning
through an any cast. After both fixes, update
src/eslint-suppressions.json:1062-1076 to restore ClineProvider.ts to 12
suppressions and webviewMessageHandler.spec.ts to 35; the parallelMode entry
requires no direct change.
Source: Coding guidelines
| delete state[key] | ||
| } | ||
| }), | ||
| } as unknown as Storage |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 ../webview-ui/src/utils/__tests__/vscode.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 1595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline webview-ui/src/utils/__tests__/vscode.spec.ts --view expanded || true
echo "== relevant lines =="
cat -n webview-ui/src/utils/__tests__/vscode.spec.ts | sed -n '1,110p'
echo "== storage usages in tests =="
rg -n "Storage|setItem|getItem|Object\.defineProperty" webview-ui/src/utils/__tests__/vscode.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4877
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig candidates =="
fd -a 'tsconfig.*|eslint.config.mjs|package.json' . | sed 's#^\./##' | sort
echo "== TypeScript config =="
for f in tsconfig.json tsconfig.base.json.web.json; do
[ -e "$f" ] && { echo "--- $f"; cat -n "$f"; }
done
echo "== vscode module/API declarations =="
rg -n 'declare module|function getPlatform|type Storage|interface VSCode|class VSCodeAPIWrapper|localStorage|api\.settings' webview-ui/src -g '*.{ts,tsx}' | head -n 120Repository: Zoo-Code-Org/Zoo-Code
Length of output: 4058
Replace the double assertions with narrow storage mocks.
Lines 21 and 76 cast incomplete objects to Storage. This hides missing mock members. Use Pick<Storage, "getItem" | "setItem" | "removeItem" | "clear"> or a local mock interface, and do not use as未知 as Storage here.
Also applies to: 76-76
🤖 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 `@webview-ui/src/utils/__tests__/vscode.spec.ts` at line 21, Replace the double
assertions at the storage mock definitions in the vscode tests with a narrow
type such as Pick<Storage, "getItem" | "setItem" | "removeItem" | "clear"> or an
equivalent local mock interface, and update both affected mocks to use it
without casting to Storage.
Source: Coding guidelines
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
Persistence logic, API integration, and refactoring built on Branch 1. This is Branch 2 of 3 split from PR #977 for easier review.
Changes
test(webview): restore parallel mode coverage(+165)fix(webview): sync after profile mutations(+133 / -3)fix(webview): persist view-local safely(+245 / -38)fix(provider): sync when activating profile(+35)fix(api): sync setConfiguration(+52 / -1)test: assert stale modelId absent(+1)fix(webview): preserve isolated writes(+99 / -16)chore(lint): update suppression baseline(+12 / -2)refactor: consolidate persistence(+35 / -21)~740 lines changed (incremental)
Related
Summary by CodeRabbit
New Features
Bug Fixes
Tests