fix(webview): add loading skeleton for view state initialization#928
Draft
easonLiangWorldedtech wants to merge 13 commits into
Draft
Conversation
Add view-local state loading and syncing for parallel ClineProvider instances, with safe initialization and configuration listener handling for test/shim environments. Key changes: - Add viewId property for unique provider instance identification - Implement viewLocalState buffer to isolate mode, profile, and apiConfig per view - Load view-local state after provider dependencies are initialized - Merge viewLocalState in getState() with local apiConfiguration taking precedence - Clear local overrides when saveViewState receives undefined or null values - Sync mode/profile changes through saveViewState(), loadViewState(), and profile activation - Preserve restored task mode in both global state and viewLocalState - Guard global configuration listener setup when VS Code workspace events are unavailable - Keep sticky-mode task restore compatible with view-local state isolation Tests: - Add 22 parallel mode cases covering viewId uniqueness, state isolation, save/load behavior, merge precedence, local override clearing, config listener handling, mode switching, profile activation, and multi-instance isolation - Verify sticky-mode restore compatibility
Read launch profile state from the active provider view and route mode changes through handleModeSwitch. Remove the duplicate webviewDidLaunch handshake from App so initialization only runs once via ExtensionStateContextProvider.
…ared data snapshot
…er settings import
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
July 17, 2026 18:05
Contributor
📝 WalkthroughWalkthroughAdds per-view mode and provider configuration isolation, persistent view identifiers, cross-instance reset propagation, import synchronization, and webview hydration gating with loading-state coverage. ChangesParallel view-state isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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__/ClineProvider.parallelMode.spec.ts`:
- Around line 1471-1680: Clean up every ClineProvider created in the
broadcastResetToAllInstances tests, including the additional tests around the
referenced later range, by disposing all providers after each test. Ensure
cleanup removes instances from the static ClineProvider.activeInstances set,
preferably through the provider’s existing disposal mechanism, so each
test—including “should handle single instance gracefully”—starts with no
providers from prior tests.
In `@src/core/webview/__tests__/ClineProvider.spec.ts`:
- Line 1742: Update the test setup around ClineProvider.setValue to await the
asynchronous state mutation before exercising the mode switch, ensuring
currentApiConfigName is persisted through ContextProxy.setValue before
subsequent assertions or actions run.
In `@src/core/webview/ClineProvider.ts`:
- Around line 3106-3125: Update _clearViewLocalState() to remove the persisted
__view_state_* overrides in addition to clearing the in-memory viewLocalState.
Await this helper from both broadcastResetToAllInstances() and resetState(),
ensuring imports, resets, configuration events, and webview rebinds cannot
restore stale view state.
- Around line 1843-1849: Update the profile-deletion flow in ClineProvider to
stop spreading getState() into ContextProxy.setValues(). Persist only
currentApiConfigName and listApiConfigMeta, and when the deleted profile was
active, activate the selected fallback through the existing profile-activation
path so its flat provider settings and task handler remain synchronized.
- Around line 436-442: Update loadViewState() and its corresponding view-state
persistence logic to store per-view values durably across extension-host
restarts, using a typed per-view map in workspaceState/globalState or a
dedicated registered persistent key. Ensure restored tabs use their stable view
ID to retrieve their own saved state instead of falling back to shared global
keys, while preserving existing values through migration if needed.
- Around line 1211-1212: Centralize view-specific updates for mode,
currentApiConfigName, and apiConfiguration in an async helper that updates
viewLocalState and persists the corresponding key through saveViewState().
Replace the direct mutations and _updateViewLocalStateFromMutation() calls in
the history, profile, API, and related update paths, while preserving the
existing global-state writes and loadViewState() behavior.
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 917-921: Update the provider argument passed to
importSettingsWithFeedback to use the existing provider instance directly
instead of the inline object literal. Preserve the provider’s settingsImportedAt
mutations and optional broadcastResetToAllInstances behavior by removing the
wrapper and passing provider unchanged.
In `@webview-ui/src/utils/vscode.ts`:
- Around line 77-81: Guard the localStorage checks in both the state-read branch
around lines 77-81 and the state-write branch around lines 100-102 of
webview-ui/src/utils/vscode.ts with typeof localStorage !== "undefined" before
accessing getItem or setItem, preserving the existing behavior when localStorage
is unavailable.
🪄 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: e6865ef3-23d8-4601-b3cb-172574b102fc
📒 Files selected for processing (17)
packages/types/src/vscode-extension-host.tssrc/core/config/__tests__/importExport.spec.tssrc/core/config/importExport.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tssrc/extension/__tests__/api-set-configuration.spec.tssrc/extension/api.tswebview-ui/src/App.tsxwebview-ui/src/__tests__/App.spec.tsxwebview-ui/src/components/modes/ModesView.tsxwebview-ui/src/components/modes/__tests__/ModesView.spec.tsxwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/context/__tests__/ExtensionStateContext.spec.tsxwebview-ui/src/utils/vscode.ts
💤 Files with no reviewable changes (1)
- webview-ui/src/App.tsx
Comment on lines
+1471
to
+1680
| describe("broadcastResetToAllInstances", () => { | ||
| it("should clear viewLocalState in other instances and post updated state", async () => { | ||
| const mockPostMessage1 = vi.fn() | ||
| const mockPostMessage2 = vi.fn() | ||
|
|
||
| const mockWebviewView1: any = { | ||
| webview: { | ||
| postMessage: mockPostMessage1, | ||
| html: "", | ||
| options: {}, | ||
| onDidReceiveMessage: vi.fn(), | ||
| asWebviewUri: vi.fn(), | ||
| cspSource: "vscode-webview://test-csp-source", | ||
| }, | ||
| visible: true, | ||
| onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| onDidDispose: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| } | ||
|
|
||
| const mockWebviewView2: any = { | ||
| webview: { | ||
| postMessage: mockPostMessage2, | ||
| html: "", | ||
| options: {}, | ||
| onDidReceiveMessage: vi.fn(), | ||
| asWebviewUri: vi.fn(), | ||
| cspSource: "vscode-webview://test-csp-source", | ||
| }, | ||
| visible: true, | ||
| onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| onDidDispose: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| } | ||
|
|
||
| const provider1 = new ClineProvider( | ||
| mockContext, | ||
| mockOutputChannel, | ||
| "sidebar", | ||
| new ContextProxy(mockContext), | ||
| ) | ||
| const provider2 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) | ||
|
|
||
| await (provider1 as any).resolveWebviewView(mockWebviewView1) | ||
| await (provider2 as any).resolveWebviewView(mockWebviewView2) | ||
|
|
||
| // Set different modes for each provider | ||
| await (provider1 as any).saveViewState("mode", "architect") | ||
| await (provider2 as any).saveViewState("mode", "debugger") | ||
|
|
||
| expect((provider1 as any).viewLocalState.mode).toBe("architect") | ||
| expect((provider2 as any).viewLocalState.mode).toBe("debugger") | ||
|
|
||
| // Call broadcastResetToAllInstances on provider1 | ||
| await provider1.broadcastResetToAllInstances() | ||
|
|
||
| // provider1 should NOT clear its own state | ||
| expect((provider1 as any).viewLocalState.mode).toBe("architect") | ||
|
|
||
| // provider2 should have cleared its viewLocalState | ||
| expect(Object.prototype.hasOwnProperty.call((provider2 as any).viewLocalState, "mode")).toBe(false) | ||
|
|
||
| // provider2 should have posted updated state to webview | ||
| expect(mockPostMessage2).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should not broadcast to itself but clear others", async () => { | ||
| const mockPostMessage1 = vi.fn() | ||
| const mockPostMessage2 = vi.fn() | ||
|
|
||
| const mockWebviewView1: any = { | ||
| webview: { | ||
| postMessage: mockPostMessage1, | ||
| html: "", | ||
| options: {}, | ||
| onDidReceiveMessage: vi.fn(), | ||
| asWebviewUri: vi.fn(), | ||
| cspSource: "vscode-webview://test-csp-source", | ||
| }, | ||
| visible: true, | ||
| onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| onDidDispose: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| } | ||
|
|
||
| const mockWebviewView2: any = { | ||
| webview: { | ||
| postMessage: mockPostMessage2, | ||
| html: "", | ||
| options: {}, | ||
| onDidReceiveMessage: vi.fn(), | ||
| asWebviewUri: vi.fn(), | ||
| cspSource: "vscode-webview://test-csp-source", | ||
| }, | ||
| visible: true, | ||
| onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| onDidDispose: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| } | ||
|
|
||
| const provider1 = new ClineProvider( | ||
| mockContext, | ||
| mockOutputChannel, | ||
| "sidebar", | ||
| new ContextProxy(mockContext), | ||
| ) | ||
| const provider2 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) | ||
|
|
||
| await (provider1 as any).resolveWebviewView(mockWebviewView1) | ||
| await (provider2 as any).resolveWebviewView(mockWebviewView2) | ||
|
|
||
| await (provider1 as any).saveViewState("mode", "architect") | ||
| await (provider2 as any).saveViewState("mode", "debugger") | ||
|
|
||
| // Call broadcastResetToAllInstances on provider2 | ||
| await provider2.broadcastResetToAllInstances() | ||
|
|
||
| // provider2 should NOT have posted to its own webview (it's the broadcaster) | ||
| expect(mockPostMessage2).not.toHaveBeenCalled() | ||
|
|
||
| // provider1 should have been cleared and posted state | ||
| expect((provider1 as any).viewLocalState.mode).toBeUndefined() | ||
| expect(mockPostMessage1).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should handle single instance gracefully", async () => { | ||
| const mockPostMessage = vi.fn() | ||
|
|
||
| const mockWebviewView: any = { | ||
| webview: { | ||
| postMessage: mockPostMessage, | ||
| html: "", | ||
| options: {}, | ||
| onDidReceiveMessage: vi.fn(), | ||
| asWebviewUri: vi.fn(), | ||
| cspSource: "vscode-webview://test-csp-source", | ||
| }, | ||
| visible: true, | ||
| onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| onDidDispose: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| } | ||
|
|
||
| const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) | ||
|
|
||
| await (provider as any).resolveWebviewView(mockWebviewView) | ||
|
|
||
| await (provider as any).saveViewState("mode", "architect") | ||
|
|
||
| // Single instance broadcast should not throw | ||
| await provider.broadcastResetToAllInstances() | ||
|
|
||
| // Single instance should keep its own state | ||
| expect((provider as any).viewLocalState.mode).toBe("architect") | ||
|
|
||
| // Should not have posted to webview (no other instances) | ||
| expect(mockPostMessage).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should broadcast to all instances in getAllInstances", async () => { | ||
| const mockPostMessage1 = vi.fn() | ||
| const mockPostMessage2 = vi.fn() | ||
| const mockPostMessage3 = vi.fn() | ||
|
|
||
| const createMockWebviewView = (postMessage: any) => ({ | ||
| webview: { | ||
| postMessage, | ||
| html: "", | ||
| options: {}, | ||
| onDidReceiveMessage: vi.fn(), | ||
| asWebviewUri: vi.fn(), | ||
| cspSource: "vscode-webview://test-csp-source", | ||
| }, | ||
| visible: true, | ||
| onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| onDidDispose: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), | ||
| }) | ||
|
|
||
| const provider1 = new ClineProvider( | ||
| mockContext, | ||
| mockOutputChannel, | ||
| "sidebar", | ||
| new ContextProxy(mockContext), | ||
| ) | ||
| const provider2 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) | ||
| const provider3 = new ClineProvider( | ||
| mockContext, | ||
| mockOutputChannel, | ||
| "sidebar", | ||
| new ContextProxy(mockContext), | ||
| ) | ||
|
|
||
| await (provider1 as any).resolveWebviewView(createMockWebviewView(mockPostMessage1)) | ||
| await (provider2 as any).resolveWebviewView(createMockWebviewView(mockPostMessage2)) | ||
| await (provider3 as any).resolveWebviewView(createMockWebviewView(mockPostMessage3)) | ||
|
|
||
| await (provider1 as any).saveViewState("mode", "code") | ||
| await (provider2 as any).saveViewState("mode", "architect") | ||
| await (provider3 as any).saveViewState("mode", "debugger") | ||
|
|
||
| // Broadcast from provider1 | ||
| await provider1.broadcastResetToAllInstances() | ||
|
|
||
| // provider1 keeps its state | ||
| expect((provider1 as any).viewLocalState.mode).toBe("code") | ||
|
|
||
| // provider2 and provider3 should be cleared | ||
| expect(Object.prototype.hasOwnProperty.call((provider2 as any).viewLocalState, "mode")).toBe(false) | ||
| expect(Object.prototype.hasOwnProperty.call((provider3 as any).viewLocalState, "mode")).toBe(false) | ||
|
|
||
| // provider2 and provider3 should have posted state | ||
| expect(mockPostMessage2).toHaveBeenCalled() | ||
| expect(mockPostMessage3).toHaveBeenCalled() | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Dispose all provider instances after each test.
These tests leave instances in the static ClineProvider.activeInstances set. Later broadcasts therefore include providers from earlier tests; notably, the “single instance” test is not actually running with one instance.
Proposed cleanup
+afterEach(async () => {
+ for (const provider of ClineProvider.getAllInstances()) {
+ await provider.dispose()
+ }
+})Also applies to: 1732-1782
🤖 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
1471 - 1680, Clean up every ClineProvider created in the
broadcastResetToAllInstances tests, including the additional tests around the
referenced later range, by disposing all providers after each test. Ensure
cleanup removes instances from the static ClineProvider.activeInstances set,
preferably through the provider’s existing disposal mechanism, so each
test—including “should handle single instance gracefully”—starts with no
providers from prior tests.
| if (key === "currentApiConfigName") return "current-config" | ||
| return undefined | ||
| }) | ||
| provider.setValue("currentApiConfigName", "current-config") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Await the state mutation before exercising the mode switch.
setValue() updates viewLocalState only after awaiting ContextProxy.setValue(). Without awaiting it, this test is race-prone.
- provider.setValue("currentApiConfigName", "current-config")
+ await provider.setValue("currentApiConfigName", "current-config")📝 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.
Suggested change
| provider.setValue("currentApiConfigName", "current-config") | |
| await provider.setValue("currentApiConfigName", "current-config") |
🤖 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.spec.ts` at line 1742, Update the
test setup around ClineProvider.setValue to await the asynchronous state
mutation before exercising the mode switch, ensuring currentApiConfigName is
persisted through ContextProxy.setValue before subsequent assertions or actions
run.
Comment on lines
+436
to
+442
| * PERSISTENCE NOTE (Finding #2): These keys (`__view_state_{id}_mode`, etc.) are NOT | ||
| * included in `GLOBAL_STATE_KEYS` from @roo-code/types, so they survive within a single | ||
| * extension session but do NOT persist across extension host restarts. If the extension is | ||
| * reloaded, view-local state falls back to the shared global keys (e.g. `mode`, | ||
| * `currentApiConfigName`). For cross-session persistence of view-specific values, either: | ||
| * - Add these keys to GLOBAL_STATE_KEYS, or | ||
| * - Use vscode.workspaceState instead of contextProxy.globalState. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist view-scoped state across extension-host restarts.
Lines 436-442 confirm these keys are not durable. A restored tab reports the same stable ID, but loadViewState() then falls back to another tab’s shared global values, recreating the stale-state flash this change targets.
Store a typed per-view map in workspaceState/globalState, or register a dedicated persistent key and migrate existing values.
🤖 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 436 - 442, Update
loadViewState() and its corresponding view-state persistence logic to store
per-view values durably across extension-host restarts, using a typed per-view
map in workspaceState/globalState or a dedicated registered persistent key.
Ensure restored tabs use their stable view ID to retrieve their own saved state
instead of falling back to shared global keys, while preserving existing values
through migration if needed.
Comment on lines
1211
to
+1212
| await this.updateGlobalState("mode", historyItem.mode) | ||
| this.viewLocalState.mode = historyItem.mode |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Route every view-specific mutation through persistent view-state writes.
These paths mutate viewLocalState directly or through _updateViewLocalStateFromMutation(), bypassing saveViewState(). Consequently, mode/profile/API changes disappear or revert when loadViewState() runs or the same webview is recreated.
Centralize mode, currentApiConfigName, and composed apiConfiguration updates in an async helper that updates both the cache and the view-specific key.
Also applies to: 1800-1803, 1851-1852, 1901-1904, 3041-3056
🤖 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 1211 - 1212, Centralize
view-specific updates for mode, currentApiConfigName, and apiConfiguration in an
async helper that updates viewLocalState and persists the corresponding key
through saveViewState(). Replace the direct mutations and
_updateViewLocalStateFromMutation() calls in the history, profile, API, and
related update paths, while preserving the existing global-state writes and
loadViewState() behavior.
Comment on lines
+1843
to
1849
| const globalSettings = await this.getState() | ||
|
|
||
| await this.contextProxy.setValues({ | ||
| ...globalSettings, | ||
| currentApiConfigName: profileToActivate, | ||
| listApiConfigMeta: entries, | ||
| }) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not write the ExtensionState projection into settings storage.
getState() contains computed defaults and a nested apiConfiguration, while ContextProxy.setValues() expects flat RooCodeSettings. This can persist unrelated derived fields while leaving the fallback profile name paired with the deleted profile’s flat provider settings.
Update only currentApiConfigName and listApiConfigMeta; when deleting the active profile, activate the selected fallback so its provider settings and task handler are synchronized.
🤖 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 1843 - 1849, Update the
profile-deletion flow in ClineProvider to stop spreading getState() into
ContextProxy.setValues(). Persist only currentApiConfigName and
listApiConfigMeta, and when the deleted profile was active, activate the
selected fallback through the existing profile-activation path so its flat
provider settings and task handler remain synchronized.
Comment on lines
+3106
to
+3125
| /** | ||
| * Clear view-local state cache so that getState() falls back to ContextProxy defaults. | ||
| */ | ||
| private _clearViewLocalState(): void { | ||
| this.viewLocalState = {} | ||
| } | ||
|
|
||
| /** | ||
| * Broadcast a reset event to all other live ClineProvider instances, clearing their | ||
| * view-local state caches and posting updated state so parallel tabs stay in sync. | ||
| * Also exposed for use by importSettingsWithFeedback (via broadcastResetToAllInstances callback). | ||
| */ | ||
| async broadcastResetToAllInstances(): Promise<void> { | ||
| const allInstances = ClineProvider.getAllInstances() | ||
| for (const instance of allInstances) { | ||
| if (instance !== this) { | ||
| instance._clearViewLocalState() | ||
| await instance.postStateToWebview() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove persisted view overrides when clearing or resetting state.
_clearViewLocalState() only empties memory. The __view_state_* values remain available to loadViewState(), so an import, reset, configuration event, or webview rebind can restore the stale mode/profile immediately.
Proposed direction
-private _clearViewLocalState(): void {
+private async _clearViewLocalState(): Promise<void> {
this.viewLocalState = {}
+ await Promise.all(
+ (["mode", "currentApiConfigName", "apiConfiguration"] as const).map((key) =>
+ this.contextProxy.setValue(this.viewStateKeyFor(key) as any, undefined),
+ ),
+ )
}Await this helper in both broadcastResetToAllInstances() and resetState().
Also applies to: 3153-3161
🤖 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 3106 - 3125, Update
_clearViewLocalState() to remove the persisted __view_state_* overrides in
addition to clearing the in-memory viewLocalState. Await this helper from both
broadcastResetToAllInstances() and resetState(), ensuring imports, resets,
configuration events, and webview rebinds cannot restore stale view state.
Comment on lines
+917
to
+921
| provider: { | ||
| settingsImportedAt: provider.settingsImportedAt, | ||
| postStateToWebview: () => provider.postStateToWebview(), | ||
| broadcastResetToAllInstances: () => provider.broadcastResetToAllInstances(), | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the provider instance directly to preserve mutations and optional methods.
Wrapping the provider in an inline object literal introduces two logic bugs:
- Lost state mutations:
importSettingsWithFeedbacktemporarily mutatesprovider.settingsImportedAt = Date.now()so thatpostStateToWebview()knows to include the imported flag. Mutating this wrapper object does not update the underlyingClineProviderinstance. - TypeError on optional methods: If
broadcastResetToAllInstancesis not yet implemented on the provider, the wrapper function() => provider.broadcastResetToAllInstances()is still truthy. When invoked, it evaluates toundefined(), throwing aTypeErrorthat suppresses the intended silent skip.
Passing the provider directly satisfies the expected interface while resolving both issues.
🐛 Proposed fix to pass the provider directly
case "importSettings": {
await importSettingsWithFeedback({
providerSettingsManager: provider.providerSettingsManager,
contextProxy: provider.contextProxy,
customModesManager: provider.customModesManager,
- provider: {
- settingsImportedAt: provider.settingsImportedAt,
- postStateToWebview: () => provider.postStateToWebview(),
- broadcastResetToAllInstances: () => provider.broadcastResetToAllInstances(),
- },
+ provider,
})
break📝 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.
Suggested change
| provider: { | |
| settingsImportedAt: provider.settingsImportedAt, | |
| postStateToWebview: () => provider.postStateToWebview(), | |
| broadcastResetToAllInstances: () => provider.broadcastResetToAllInstances(), | |
| }, | |
| provider, |
🤖 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 917 - 921, Update the
provider argument passed to importSettingsWithFeedback to use the existing
provider instance directly instead of the inline object literal. Preserve the
provider’s settingsImportedAt mutations and optional
broadcastResetToAllInstances behavior by removing the wrapper and passing
provider unchanged.
Comment on lines
+77
to
+81
| } else if (typeof localStorage?.getItem === "function") { | ||
| const state = localStorage.getItem("vscodeState") | ||
| return state ? JSON.parse(state) : undefined | ||
| } else { | ||
| return undefined |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node -e '
try {
console.log(typeof localStorage?.getItem);
} catch (err) {
console.error("Syntax trap caught:", err.toString());
}
'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files webview-ui/src/utils/vscode.ts
wc -l webview-ui/src/utils/vscode.ts
cat -n webview-ui/src/utils/vscode.ts | sed -n '1,180p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 4500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether localStorage is declared/imported anywhere relevant in this file or nearby tests.
rg -n "localStorage|vscodeState" webview-ui/src/utils/vscode.ts webview-ui/src -g '!**/dist/**' -g '!**/build/**'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 1684
Guard localStorage before property checks. typeof localStorage?.getItem and typeof localStorage?.setItem still touch an undeclared global in non-DOM environments and can throw ReferenceError. Use typeof localStorage !== "undefined" && ... at both sites.
📍 Affects 1 file
webview-ui/src/utils/vscode.ts#L77-L81(this comment)webview-ui/src/utils/vscode.ts#L100-L102
🤖 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/vscode.ts` around lines 77 - 81, Guard the localStorage
checks in both the state-read branch around lines 77-81 and the state-write
branch around lines 100-102 of webview-ui/src/utils/vscode.ts with typeof
localStorage !== "undefined" before accessing getItem or setItem, preserving the
existing behavior when localStorage is unavailable.
easonLiangWorldedtech
left a comment
easonLiangWorldedtech
left a comment
Contributor
Author
There was a problem hiding this comment.
Reply to all CodeRabbit comments — scope clarification
PR #928's branch contains parent PR #909 commits + loading skeleton (last commit). CodeRabbit reviewed ALL files across the entire branch, so these findings belong to parent PR #909, not PR #928.
PR #928's last commit only changed:
webview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/components/modes/ModesView.tsxwebview-ui/src/context/__tests__/ExtensionStateContext.spec.tsxwebview-ui/src/components/modes/__tests__/ModesView.spec.tsx
All 8 comments below target files from parent PR #909 commits. Resolved as "out of scope for this PR".
easonLiangWorldedtech
left a comment
easonLiangWorldedtech
left a comment
Contributor
Author
There was a problem hiding this comment.
@coderabbitai All 8 inline comments target files changed in parent PR #909 (not PR #928).
PR #928 only adds the loading skeleton (last commit) to:
- ExtensionStateContext.tsx + spec
- ModesView.tsx + spec
Every other file reviewed here belongs to parent PR #909 commits. Resolved as out-of-scope for this PR.
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🐇🔍 |
Contributor
Author
CodeRabbit Comments — Scope ClarificationAll inline comments from @coderabbitai target files changed in parent PR #909, not PR #928. PR #928 only adds the loading skeleton (last commit) to:
Every other file reviewed here belongs to parent PR #909 commits. Resolved as out-of-scope for this PR. Related Tracking IssueThe 5 CodeRabbit findings from PR #909 are tracked separately in:
This is distinct from the UX follow-ups tracked in issue #913. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🐇🔍 |
github-actions
Bot
added
the
awaiting-review
easonLiangWorldedtech
marked this pull request as draft
July 19, 2026 17:11
github-actions
Bot
removed
the
awaiting-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
viewStateLoadedflag to the webview extension state context.Tests
Fixes #915
Summary by CodeRabbit
New Features
Bug Fixes