From 3396c9e9c3b1f3ec6e59b35c75598852c93662a7 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sat, 5 Sep 2026 02:18:07 +0800 Subject: [PATCH 01/18] fix(activate): target title-bar commands to their click-origin instance --- packages/types/src/vscode.ts | 8 + .../__tests__/registerCommands.spec.ts | 299 +++++++++++++++--- src/activate/registerCommands.ts | 126 ++++++-- src/core/webview/ClineProvider.ts | 10 + .../webview/__tests__/ClineProvider.spec.ts | 13 + src/package.json | 28 +- 6 files changed, 411 insertions(+), 73 deletions(-) diff --git a/packages/types/src/vscode.ts b/packages/types/src/vscode.ts index fd4e31116d..6a1a08b821 100644 --- a/packages/types/src/vscode.ts +++ b/packages/types/src/vscode.ts @@ -35,6 +35,14 @@ export const commandIds = [ "popoutButtonClicked", "settingsButtonClicked", + // Editor-tab (popped-out) surface variants of the title-bar buttons. The + // shared ids above target the sidebar click origin, so the tab surface + // needs its own ids (see registerCommands.ts getTabProvider). + "plusButtonClickedInTab", + "settingsButtonClickedInTab", + "marketplaceButtonClickedInTab", + "historyButtonClickedInTab", + "openInNewTab", "newTask", diff --git a/src/activate/__tests__/registerCommands.spec.ts b/src/activate/__tests__/registerCommands.spec.ts index 67a2b935ec..e3b5b887fa 100644 --- a/src/activate/__tests__/registerCommands.spec.ts +++ b/src/activate/__tests__/registerCommands.spec.ts @@ -1,5 +1,7 @@ import type { Mock } from "vitest" import * as vscode from "vscode" +import { TelemetryService } from "@roo-code/telemetry" + import { ClineProvider } from "../../core/webview/ClineProvider" import { getVisibleProviderOrLog, openClineInNewTab, registerCommands, setPanel } from "../registerCommands" @@ -192,44 +194,104 @@ describe("registerCommands handlers", () => { expect(mockContext.subscriptions).toContain(disposable) }) - it("settingsButtonClicked posts both settingsButtonClicked and didBecomeVisible actions", () => { + // The sidebar title-bar handlers target the registered provider (the + // sidebar click origin) directly, not the visible-instance heuristic. + it("settingsButtonClicked posts both settingsButtonClicked and didBecomeVisible actions on the registered provider", () => { handlers["zoo-code.settingsButtonClicked"]() - expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({ + expect(TelemetryService.instance.captureTitleButtonClicked).toHaveBeenCalledWith("settings") + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "settingsButtonClicked", }) - expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({ + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "didBecomeVisible", }) - expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledTimes(2) - }) - - it("settingsButtonClicked is a no-op when no visible provider", () => { - ;(ClineProvider.getVisibleInstance as Mock).mockReturnValue(undefined) - - handlers["zoo-code.settingsButtonClicked"]() - + expect(mockProvider.postMessageToWebview).toHaveBeenCalledTimes(2) expect(mockVisibleProvider.postMessageToWebview).not.toHaveBeenCalled() }) - it("historyButtonClicked posts historyButtonClicked action", () => { + it("historyButtonClicked posts historyButtonClicked action on the registered provider", () => { handlers["zoo-code.historyButtonClicked"]() - expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({ + expect(TelemetryService.instance.captureTitleButtonClicked).toHaveBeenCalledWith("history") + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "historyButtonClicked", }) + expect(mockVisibleProvider.postMessageToWebview).not.toHaveBeenCalled() }) - it("marketplaceButtonClicked posts marketplaceButtonClicked action", () => { + it("marketplaceButtonClicked posts marketplaceButtonClicked action on the registered provider", () => { handlers["zoo-code.marketplaceButtonClicked"]() - expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({ + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "marketplaceButtonClicked", }) + expect(mockVisibleProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + // The `*InTab` handlers serve the `editor/title` menu: they target the + // instance that owns the tracked tab panel, resolved via + // ClineProvider.getInstanceForView. + const tabHandlerCases: { command: string; actions: string[]; telemetry?: string }[] = [ + { + command: "zoo-code.settingsButtonClickedInTab", + actions: ["settingsButtonClicked", "didBecomeVisible"], + telemetry: "settings", + }, + { command: "zoo-code.historyButtonClickedInTab", actions: ["historyButtonClicked"], telemetry: "history" }, + { command: "zoo-code.marketplaceButtonClickedInTab", actions: ["marketplaceButtonClicked"] }, + ] + it.each(tabHandlerCases)( + "$command targets the tab instance for the tracked tab panel", + ({ command, actions, telemetry }) => { + const mockTabProvider = { postMessageToWebview: vi.fn().mockResolvedValue(undefined) } + setPanel({} as vscode.WebviewPanel, "tab") + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(mockTabProvider) + + handlers[command]() + + for (const action of actions) { + expect(mockTabProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action }) + } + expect(mockTabProvider.postMessageToWebview).toHaveBeenCalledTimes(actions.length) + if (telemetry) { + expect(TelemetryService.instance.captureTitleButtonClicked).toHaveBeenCalledWith(telemetry) + } + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }, + ) + + // The `*InTab` handlers must no-op when there is no live tab instance: a + // missing or disposed tab must not crash the handler or fall back to + // another instance. Every handler is awaited, so an async handler that + // slipped past its guard (rejecting on the missing instance) fails the + // test instead of settling as an unhandled rejection. + const inTabNoOpCommands = [ + "zoo-code.plusButtonClickedInTab", + "zoo-code.settingsButtonClickedInTab", + "zoo-code.historyButtonClickedInTab", + "zoo-code.marketplaceButtonClickedInTab", + ] + it.each(inTabNoOpCommands)("$command is a no-op when no tab panel is tracked", async (command) => { + await handlers[command]() + + expect(ClineProvider.getInstanceForView as Mock).not.toHaveBeenCalled() + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + expect(mockVisibleProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it.each(inTabNoOpCommands)("$command is a no-op when the tab instance is disposed", async (command) => { + setPanel({} as vscode.WebviewPanel, "tab") + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(undefined) + + await handlers[command]() + + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + expect(mockVisibleProvider.postMessageToWebview).not.toHaveBeenCalled() }) it("acceptInput posts acceptInput message", () => { @@ -302,44 +364,138 @@ describe("registerCommands handlers", () => { }) }) - it("focusInput does not post when no sidebar panel is active", async () => { + it("focusInput does not post when no sidebar panel is tracked", async () => { await handlers["zoo-code.focusInput"]() expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) - // Representative coverage for the .catch arm on all five void-prefixed - // postMessageToWebview sites in registerCommands.ts (settingsButtonClicked - // posts twice, plus historyButtonClicked, marketplaceButtonClicked, and - // acceptInput). Each handler is synchronous, so the .catch arm runs on a - // microtask; setImmediate ensures all microtasks are flushed before we assert. The + it("focusInput does not post when a tab panel is tracked alongside the sidebar", async () => { + setPanel({} as vscode.WebviewView, "sidebar") + setPanel({} as vscode.WebviewPanel, "tab") + + await handlers["zoo-code.focusInput"]() + + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("setPanel keeps independent refs: clearing only the tab ref re-enables the sidebar post", async () => { + setPanel({} as vscode.WebviewView, "sidebar") + setPanel({} as vscode.WebviewPanel, "tab") + + // The tab ref does not wipe the sidebar ref... + await handlers["zoo-code.focusInput"]() + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + + // ...and clearing only the tab ref re-enables the sidebar post. + setPanel(undefined, "tab") + await handlers["zoo-code.focusInput"]() + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "focusInput" }) + }) + + // Coverage for the .catch arm on the sidebar title-bar post sites + // (settingsButtonClicked posts twice, plus historyButtonClicked and + // marketplaceButtonClicked) and acceptInput (the visible-provider path). + // Each handler is synchronous, so the .catch arm runs on a microtask; + // setImmediate ensures all microtasks are flushed before we assert. The // log messages carry a `[]` prefix so multi-failure logs // remain unambiguous; the prefix is per-handler, not per-call (both of - // settingsButtonClicked's posts share the same prefix). + // settingsButtonClicked's posts share the same prefix). Each post rejects + // with its own error and call N is pinned to post N, so a mutant that + // alters one catch's message cannot hide behind the other post's + // identical log. it.each([ - { command: "zoo-code.settingsButtonClicked", prefix: "settingsButtonClicked", expectedCalls: 2 }, - { command: "zoo-code.historyButtonClicked", prefix: "historyButtonClicked", expectedCalls: 1 }, - { command: "zoo-code.marketplaceButtonClicked", prefix: "marketplaceButtonClicked", expectedCalls: 1 }, - { command: "zoo-code.acceptInput", prefix: "acceptInput", expectedCalls: 1 }, + { + command: "zoo-code.settingsButtonClicked", + prefix: "settingsButtonClicked", + errorLabels: ["first post", "second post"], + target: "sidebar" as const, + }, + { + command: "zoo-code.historyButtonClicked", + prefix: "historyButtonClicked", + errorLabels: ["post"], + target: "sidebar" as const, + }, + { + command: "zoo-code.marketplaceButtonClicked", + prefix: "marketplaceButtonClicked", + errorLabels: ["post"], + target: "sidebar" as const, + }, + { command: "zoo-code.acceptInput", prefix: "acceptInput", errorLabels: ["post"], target: "visible" as const }, ])( "$command logs to outputChannel when postMessageToWebview rejects", - async ({ command, prefix, expectedCalls }) => { - const boom = new Error("boom") - mockVisibleProvider.postMessageToWebview.mockReset() - mockVisibleProvider.postMessageToWebview.mockRejectedValue(boom) + async ({ command, prefix, errorLabels, target }) => { + const post = + target === "sidebar" ? mockProvider.postMessageToWebview : mockVisibleProvider.postMessageToWebview + post.mockReset() + const booms = errorLabels.map((label) => new Error(label)) + booms.forEach((boom) => post.mockRejectedValueOnce(boom)) handlers[command]() - // Flush microtasks so the chained .catch arm runs. + // Flush microtasks so the chained .catch arms run. await new Promise((resolve) => setImmediate(resolve)) - expect(mockOutputChannel.appendLine).toHaveBeenCalledTimes(expectedCalls) - expect(mockOutputChannel.appendLine).toHaveBeenCalledWith( - `[${prefix}] postMessageToWebview failed: ${boom}`, - ) + expect(mockOutputChannel.appendLine).toHaveBeenCalledTimes(booms.length) + booms.forEach((boom, index) => { + expect(mockOutputChannel.appendLine).toHaveBeenNthCalledWith( + index + 1, + `[${prefix}] postMessageToWebview failed: ${boom}`, + ) + }) }, ) + // The two posts reject with distinct errors and the nth-call assertions + // pin each catch's message, so neither template literal can survive + // behind the other post's identical log. + it("settingsButtonClickedInTab logs to outputChannel when postMessageToWebview rejects", async () => { + const booms = [new Error("first post"), new Error("second post")] + const mockTabProvider = { + postMessageToWebview: vi.fn().mockRejectedValueOnce(booms[0]).mockRejectedValueOnce(booms[1]), + } + setPanel({} as vscode.WebviewPanel, "tab") + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(mockTabProvider) + + handlers["zoo-code.settingsButtonClickedInTab"]() + + // Flush microtasks so the chained .catch arms run. + await new Promise((resolve) => setImmediate(resolve)) + + expect(mockOutputChannel.appendLine).toHaveBeenCalledTimes(2) + expect(mockOutputChannel.appendLine).toHaveBeenNthCalledWith( + 1, + `[settingsButtonClickedInTab] postMessageToWebview failed: ${booms[0]}`, + ) + expect(mockOutputChannel.appendLine).toHaveBeenNthCalledWith( + 2, + `[settingsButtonClickedInTab] postMessageToWebview failed: ${booms[1]}`, + ) + }) + + // The history and marketplace InTab catch sites share the identical + // single-post pattern (their sidebar equivalents are covered by the + // it.each above); pin their exact messages too. + it.each([ + { command: "zoo-code.historyButtonClickedInTab", prefix: "historyButtonClickedInTab" }, + { command: "zoo-code.marketplaceButtonClickedInTab", prefix: "marketplaceButtonClickedInTab" }, + ])("$command logs to outputChannel when the tab postMessageToWebview rejects", async ({ command, prefix }) => { + const boom = new Error("post") + const mockTabProvider = { postMessageToWebview: vi.fn().mockRejectedValue(boom) } + setPanel({} as vscode.WebviewPanel, "tab") + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(mockTabProvider) + + handlers[command]() + + // Flush microtasks so the chained .catch arm runs. + await new Promise((resolve) => setImmediate(resolve)) + + expect(mockOutputChannel.appendLine).toHaveBeenCalledTimes(1) + expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(`[${prefix}] postMessageToWebview failed: ${boom}`) + }) + it("toggleAutoApprove logs to outputChannel when postMessageToWebview rejects", async () => { // toggleAutoApprove is `async` and awaits postMessageToWebview inside a // try/catch (rather than relying on a `.catch` microtask like the @@ -357,22 +513,40 @@ describe("registerCommands handlers", () => { ) }) - it("plusButtonClicked calls evictCurrentTask on the visible provider", async () => { + it("plusButtonClicked calls evictCurrentTask on the registered sidebar provider", async () => { const evictCurrentTask = vi.fn().mockResolvedValue(undefined) const refreshWorkspace = vi.fn().mockResolvedValue(undefined) - ;(mockVisibleProvider as any).evictCurrentTask = evictCurrentTask - ;(mockVisibleProvider as any).refreshWorkspace = refreshWorkspace + ;(mockProvider as any).evictCurrentTask = evictCurrentTask + ;(mockProvider as any).refreshWorkspace = refreshWorkspace await handlers["zoo-code.plusButtonClicked"]() + expect(TelemetryService.instance.captureTitleButtonClicked).toHaveBeenCalledWith("plus") expect(evictCurrentTask).toHaveBeenCalledTimes(1) + expect(refreshWorkspace).toHaveBeenCalledTimes(1) + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "chatButtonClicked" }) + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "focusInput" }) }) - it("plusButtonClicked is a no-op when no visible provider", async () => { - ;(ClineProvider.getVisibleInstance as Mock).mockReturnValue(undefined) + it("plusButtonClickedInTab evicts and posts on the tab instance for the tracked tab panel", async () => { + const mockTabProvider = { + postMessageToWebview: vi.fn().mockResolvedValue(undefined), + evictCurrentTask: vi.fn().mockResolvedValue(undefined), + refreshWorkspace: vi.fn().mockResolvedValue(undefined), + } + setPanel({} as vscode.WebviewPanel, "tab") + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(mockTabProvider) - // Should not throw even with no visible provider - await handlers["zoo-code.plusButtonClicked"]() + await handlers["zoo-code.plusButtonClickedInTab"]() + + expect(TelemetryService.instance.captureTitleButtonClicked).toHaveBeenCalledWith("plus") + expect(mockTabProvider.evictCurrentTask).toHaveBeenCalledTimes(1) + expect(mockTabProvider.refreshWorkspace).toHaveBeenCalledTimes(1) + expect(mockTabProvider.postMessageToWebview).toHaveBeenCalledWith({ + type: "action", + action: "chatButtonClicked", + }) + expect(mockTabProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "focusInput" }) }) }) @@ -414,6 +588,9 @@ describe("openClineInNewTab", () => { it("creates a webview panel with title 'Zoo Code'", async () => { await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + // No tab was tracked, so the reuse path (and its instance lookup) + // must not run. + expect(ClineProvider.getInstanceForView as Mock).not.toHaveBeenCalled() expect(vscode.window.createWebviewPanel).toHaveBeenCalledWith( "zoo-code.TabPanelProvider", "Zoo Code", @@ -424,4 +601,42 @@ describe("openClineInNewTab", () => { }), ) }) + + it("reveals the existing tab instead of creating a second panel", async () => { + const mockExistingProvider = { postMessageToWebview: vi.fn().mockResolvedValue(undefined) } + const mockPanel = { + webview: { postMessage: vi.fn() }, + onDidChangeViewState: vi.fn(), + onDidDispose: vi.fn(), + reveal: vi.fn().mockResolvedValue(undefined), + } as unknown as vscode.WebviewPanel + setPanel(mockPanel, "tab") + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(mockExistingProvider) + + const result = await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + expect(result).toBe(mockExistingProvider) + expect(mockPanel.reveal).toHaveBeenCalledTimes(1) + expect(vscode.window.createWebviewPanel).not.toHaveBeenCalled() + expect(mockExistingProvider.postMessageToWebview).toHaveBeenCalledWith({ + type: "action", + action: "didBecomeVisible", + }) + }) + + it("creates a new tab panel when the tracked tab's provider has been disposed", async () => { + const mockPanel = { + webview: { postMessage: vi.fn() }, + onDidChangeViewState: vi.fn(), + onDidDispose: vi.fn(), + reveal: vi.fn().mockResolvedValue(undefined), + } as unknown as vscode.WebviewPanel + setPanel(mockPanel, "tab") + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(undefined) + + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + expect(mockPanel.reveal).not.toHaveBeenCalled() + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) + }) }) diff --git a/src/activate/registerCommands.ts b/src/activate/registerCommands.ts index 692aabfd68..500e7752bc 100644 --- a/src/activate/registerCommands.ts +++ b/src/activate/registerCommands.ts @@ -41,7 +41,12 @@ export function getPanel(): vscode.WebviewPanel | vscode.WebviewView | undefined } /** - * Set panel references + * Set panel references. + * + * The two refs are independent: each surface keeps its own ref for its whole + * lifetime, so resolving the sidebar view never wipes a live tab panel (and + * vice versa). Callers pass `undefined` only when the surface itself is + * disposed (see the `onDidDispose` wiring in `openClineInNewTab`). */ export function setPanel( newPanel: vscode.WebviewPanel | vscode.WebviewView | undefined, @@ -49,13 +54,22 @@ export function setPanel( ): void { if (type === "sidebar") { sidebarPanel = newPanel as vscode.WebviewView - tabPanel = undefined } else { tabPanel = newPanel as vscode.WebviewPanel - sidebarPanel = undefined } } +/** + * The instance that owns the tracked tab panel, if it is still alive. + * + * Title-bar commands on the editor-tab surface use this instead of the + * visible-instance heuristic, so a click on the tab's title bar always + * targets that tab even when the sidebar is visible side-by-side. + */ +function getTabProvider(): ClineProvider | undefined { + return tabPanel ? ClineProvider.getInstanceForView(tabPanel) : undefined +} + export type RegisterCommandOptions = { context: vscode.ExtensionContext outputChannel: vscode.OutputChannel @@ -91,21 +105,35 @@ const getCommandsMap = ({ provider, }: RegisterCommandOptions): Record, CommandCallback> => ({ activationCompleted: () => {}, + // The `view/title` menu is scoped to the sidebar view, so the click + // origin of these handlers is the sidebar provider wired in at + // activation (`provider`). Target it directly instead of the + // visible-instance heuristic, which would follow the user's focus to a + // tab instance when both surfaces are open side-by-side. The `*InTab` + // variants serve the `editor/title` menu and target the tab instance + // through `getTabProvider()` instead. plusButtonClicked: async () => { - const visibleProvider = getVisibleProviderOrLog(outputChannel) + TelemetryService.instance.captureTitleButtonClicked("plus") - if (!visibleProvider) { + await provider.evictCurrentTask() + await provider.refreshWorkspace() + await provider.postMessageToWebview({ type: "action", action: "chatButtonClicked" }) + // Send focusInput action immediately after chatButtonClicked + // This ensures the focus happens after the view has switched + await provider.postMessageToWebview({ type: "action", action: "focusInput" }) + }, + plusButtonClickedInTab: async () => { + const tabProvider = getTabProvider() + if (!tabProvider) { return } TelemetryService.instance.captureTitleButtonClicked("plus") - await visibleProvider.evictCurrentTask() - await visibleProvider.refreshWorkspace() - await visibleProvider.postMessageToWebview({ type: "action", action: "chatButtonClicked" }) - // Send focusInput action immediately after chatButtonClicked - // This ensures the focus happens after the view has switched - await visibleProvider.postMessageToWebview({ type: "action", action: "focusInput" }) + await tabProvider.evictCurrentTask() + await tabProvider.refreshWorkspace() + await tabProvider.postMessageToWebview({ type: "action", action: "chatButtonClicked" }) + await tabProvider.postMessageToWebview({ type: "action", action: "focusInput" }) }, popoutButtonClicked: () => { TelemetryService.instance.captureTitleButtonClicked("popout") @@ -114,44 +142,74 @@ const getCommandsMap = ({ }, openInNewTab: () => openClineInNewTab({ context, outputChannel }), settingsButtonClicked: () => { - const visibleProvider = getVisibleProviderOrLog(outputChannel) + TelemetryService.instance.captureTitleButtonClicked("settings") - if (!visibleProvider) { + void provider + .postMessageToWebview({ type: "action", action: "settingsButtonClicked" }) + .catch((error) => outputChannel.appendLine(`[settingsButtonClicked] postMessageToWebview failed: ${error}`)) + // Also explicitly post the visibility message to trigger scroll reliably + void provider + .postMessageToWebview({ type: "action", action: "didBecomeVisible" }) + .catch((error) => outputChannel.appendLine(`[settingsButtonClicked] postMessageToWebview failed: ${error}`)) + }, + settingsButtonClickedInTab: () => { + const tabProvider = getTabProvider() + if (!tabProvider) { return } TelemetryService.instance.captureTitleButtonClicked("settings") - void visibleProvider + void tabProvider .postMessageToWebview({ type: "action", action: "settingsButtonClicked" }) - .catch((error) => outputChannel.appendLine(`[settingsButtonClicked] postMessageToWebview failed: ${error}`)) - // Also explicitly post the visibility message to trigger scroll reliably - void visibleProvider + .catch((error) => + outputChannel.appendLine(`[settingsButtonClickedInTab] postMessageToWebview failed: ${error}`), + ) + void tabProvider .postMessageToWebview({ type: "action", action: "didBecomeVisible" }) - .catch((error) => outputChannel.appendLine(`[settingsButtonClicked] postMessageToWebview failed: ${error}`)) + .catch((error) => + outputChannel.appendLine(`[settingsButtonClickedInTab] postMessageToWebview failed: ${error}`), + ) }, historyButtonClicked: () => { - const visibleProvider = getVisibleProviderOrLog(outputChannel) + TelemetryService.instance.captureTitleButtonClicked("history") - if (!visibleProvider) { + void provider + .postMessageToWebview({ type: "action", action: "historyButtonClicked" }) + .catch((error) => outputChannel.appendLine(`[historyButtonClicked] postMessageToWebview failed: ${error}`)) + }, + historyButtonClickedInTab: () => { + const tabProvider = getTabProvider() + if (!tabProvider) { return } TelemetryService.instance.captureTitleButtonClicked("history") - void visibleProvider + void tabProvider .postMessageToWebview({ type: "action", action: "historyButtonClicked" }) - .catch((error) => outputChannel.appendLine(`[historyButtonClicked] postMessageToWebview failed: ${error}`)) + .catch((error) => + outputChannel.appendLine(`[historyButtonClickedInTab] postMessageToWebview failed: ${error}`), + ) }, marketplaceButtonClicked: () => { - const visibleProvider = getVisibleProviderOrLog(outputChannel) - if (!visibleProvider) return - void visibleProvider + void provider .postMessageToWebview({ type: "action", action: "marketplaceButtonClicked" }) .catch((error) => outputChannel.appendLine(`[marketplaceButtonClicked] postMessageToWebview failed: ${error}`), ) }, + marketplaceButtonClickedInTab: () => { + const tabProvider = getTabProvider() + if (!tabProvider) { + return + } + void tabProvider + .postMessageToWebview({ type: "action", action: "marketplaceButtonClicked" }) + .catch((error) => + outputChannel.appendLine(`[marketplaceButtonClickedInTab] postMessageToWebview failed: ${error}`), + ) + }, newTask: handleNewTask, setCustomStoragePath: async () => { const { promptForCustomStoragePath } = await import("../utils/storage") @@ -177,8 +235,11 @@ const getCommandsMap = ({ try { await focusPanel(tabPanel, sidebarPanel) - // Send focus input message only for sidebar panels - if (sidebarPanel && getPanel() === sidebarPanel) { + // Send focus input message only when the sidebar panel was + // focused: the tab takes selection priority in focusPanel, so + // the sidebar receives the message only when no tab panel is + // tracked. + if (sidebarPanel && !tabPanel) { await provider.postMessageToWebview({ type: "action", action: "focusInput" }) } } catch (error) { @@ -222,6 +283,17 @@ const getCommandsMap = ({ }) export const openClineInNewTab = async ({ context, outputChannel }: Omit) => { + // Reuse the tracked tab instead of opening a second one: a repeated + // "Open in editor" click reveals the existing tab's panel. + if (tabPanel) { + const existingProvider = ClineProvider.getInstanceForView(tabPanel) + if (existingProvider) { + await tabPanel.reveal() + await existingProvider.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) + return existingProvider + } + } + // (This example uses webviewProvider activation event which is necessary to // deserialize cached webview, but since we use retainContextWhenHidden, we // don't need to use that event). diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 87a899344c..29c32ae9cf 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -903,6 +903,16 @@ export class ClineProvider return Array.from(this.activeInstances) } + /** + * Returns the live instance whose current view is the given view or panel, + * if any. Title-bar commands on a specific surface use this to target the + * instance that owns that surface rather than the visible-instance + * heuristic (which picks whichever surface the user last focused). + */ + public static getInstanceForView(view: vscode.WebviewView | vscode.WebviewPanel): ClineProvider | undefined { + return Array.from(this.activeInstances).find((instance) => instance.view === view) + } + public static async getInstance(): Promise { let visibleProvider = ClineProvider.getVisibleInstance() diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 1a6a82a5b0..3f1bf0875e 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -567,6 +567,19 @@ describe("ClineProvider", () => { expect(ClineProvider.getVisibleInstance()).toBe(provider) }) + describe("getInstanceForView", () => { + it("returns the instance that owns the given view", () => { + // @ts-ignore - accessing private property for testing + provider.view = mockWebviewView + + expect(ClineProvider.getInstanceForView(mockWebviewView)).toBe(provider) + }) + + it("returns undefined when no live instance owns the view", () => { + expect(ClineProvider.getInstanceForView({} as vscode.WebviewView)).toBeUndefined() + }) + }) + test("loads full model details when preparing an LM Studio task", async () => { await provider.performPreparationTasks({ apiConfiguration: { diff --git a/src/package.json b/src/package.json index 4e9bfcfcf7..6753513658 100644 --- a/src/package.json +++ b/src/package.json @@ -95,6 +95,26 @@ "title": "%command.settings.title%", "icon": "$(settings-gear)" }, + { + "command": "zoo-code.plusButtonClickedInTab", + "title": "%command.newTask.title%", + "icon": "$(edit)" + }, + { + "command": "zoo-code.settingsButtonClickedInTab", + "title": "%command.settings.title%", + "icon": "$(settings-gear)" + }, + { + "command": "zoo-code.marketplaceButtonClickedInTab", + "title": "%command.marketplace.title%", + "icon": "$(extensions)" + }, + { + "command": "zoo-code.historyButtonClickedInTab", + "title": "%command.history.title%", + "icon": "$(history)" + }, { "command": "zoo-code.openInNewTab", "title": "%command.openInNewTab.title%", @@ -241,22 +261,22 @@ ], "editor/title": [ { - "command": "zoo-code.plusButtonClicked", + "command": "zoo-code.plusButtonClickedInTab", "group": "navigation@1", "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" }, { - "command": "zoo-code.settingsButtonClicked", + "command": "zoo-code.settingsButtonClickedInTab", "group": "navigation@2", "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" }, { - "command": "zoo-code.marketplaceButtonClicked", + "command": "zoo-code.marketplaceButtonClickedInTab", "group": "navigation@3", "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" }, { - "command": "zoo-code.historyButtonClicked", + "command": "zoo-code.historyButtonClickedInTab", "group": "overflow@1", "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" }, From a14326782a21a8d7d26bd9f2e5e894d875009a07 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Mon, 7 Sep 2026 14:55:00 +0800 Subject: [PATCH 02/18] ci: re-trigger PR review-state labeler reconciliation (no-op commit) From 723d871c7a0a0509bce73c40bf31715d6cd200d1 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Mon, 7 Sep 2026 15:46:00 +0800 Subject: [PATCH 03/18] fix(activate): serialize overlapping openClineInNewTab calls and harden view-identity tests Track the in-flight tab panel creation with a module-level promise so concurrent openClineInNewTab calls reuse one panel and provider (adds a Promise.all regression test). ClineProvider.spec sets the private view via the public resolveWebviewView() instead of a ts-ignore assignment. registerCommands.spec types evictCurrentTask/refreshWorkspace on the fixture and drops the as any attachment. eslint-suppressions: prune the registerCommands.spec.ts entry (two as any suppressions removed). --- .../__tests__/registerCommands.spec.ts | 30 ++- src/activate/registerCommands.ts | 171 ++++++++++-------- .../webview/__tests__/ClineProvider.spec.ts | 5 +- src/eslint-suppressions.json | 5 - 4 files changed, 123 insertions(+), 88 deletions(-) diff --git a/src/activate/__tests__/registerCommands.spec.ts b/src/activate/__tests__/registerCommands.spec.ts index e3b5b887fa..1672c74d21 100644 --- a/src/activate/__tests__/registerCommands.spec.ts +++ b/src/activate/__tests__/registerCommands.spec.ts @@ -136,7 +136,11 @@ describe("registerCommands handlers", () => { let mockOutputChannel: vscode.OutputChannel let mockContext: vscode.ExtensionContext let mockVisibleProvider: { postMessageToWebview: Mock } - let mockProvider: { postMessageToWebview: Mock } + let mockProvider: { + postMessageToWebview: Mock + evictCurrentTask: Mock + refreshWorkspace: Mock + } let handlers: Record unknown> beforeEach(() => { @@ -164,6 +168,8 @@ describe("registerCommands handlers", () => { mockProvider = { postMessageToWebview: vi.fn().mockResolvedValue(undefined), + evictCurrentTask: vi.fn().mockResolvedValue(undefined), + refreshWorkspace: vi.fn().mockResolvedValue(undefined), } ;(ClineProvider.getVisibleInstance as Mock).mockReturnValue(mockVisibleProvider) ;(vscode.commands.registerCommand as Mock).mockImplementation( @@ -514,16 +520,11 @@ describe("registerCommands handlers", () => { }) it("plusButtonClicked calls evictCurrentTask on the registered sidebar provider", async () => { - const evictCurrentTask = vi.fn().mockResolvedValue(undefined) - const refreshWorkspace = vi.fn().mockResolvedValue(undefined) - ;(mockProvider as any).evictCurrentTask = evictCurrentTask - ;(mockProvider as any).refreshWorkspace = refreshWorkspace - await handlers["zoo-code.plusButtonClicked"]() expect(TelemetryService.instance.captureTitleButtonClicked).toHaveBeenCalledWith("plus") - expect(evictCurrentTask).toHaveBeenCalledTimes(1) - expect(refreshWorkspace).toHaveBeenCalledTimes(1) + expect(mockProvider.evictCurrentTask).toHaveBeenCalledTimes(1) + expect(mockProvider.refreshWorkspace).toHaveBeenCalledTimes(1) expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "chatButtonClicked" }) expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "focusInput" }) }) @@ -639,4 +640,17 @@ describe("openClineInNewTab", () => { expect(mockPanel.reveal).not.toHaveBeenCalled() expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) }) + + it("serializes concurrent opens so overlapping calls create one panel and share one provider", async () => { + const [first, second] = await Promise.all([ + openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }), + openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }), + ]) + + // Overlapping "Open in editor" calls must share the in-flight + // creation: exactly one tab panel is created and both callers + // receive the same provider. + expect(first).toBe(second) + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) + }) }) diff --git a/src/activate/registerCommands.ts b/src/activate/registerCommands.ts index 500e7752bc..5fd5171a83 100644 --- a/src/activate/registerCommands.ts +++ b/src/activate/registerCommands.ts @@ -32,6 +32,12 @@ export function getVisibleProviderOrLog(outputChannel: vscode.OutputChannel): Cl let sidebarPanel: vscode.WebviewView | undefined = undefined let tabPanel: vscode.WebviewPanel | undefined = undefined +// In-flight "open in editor" creation shared by overlapping calls: a +// double-click starts before the first call tracks its new panel, so +// concurrent callers must share one creation instead of racing to create +// two tab panels. +let pendingTabPanelCreation: Promise | undefined + /** * Get the currently active panel * @returns WebviewPanelꈖWebviewView @@ -283,88 +289,109 @@ const getCommandsMap = ({ }) export const openClineInNewTab = async ({ context, outputChannel }: Omit) => { - // Reuse the tracked tab instead of opening a second one: a repeated - // "Open in editor" click reveals the existing tab's panel. - if (tabPanel) { - const existingProvider = ClineProvider.getInstanceForView(tabPanel) - if (existingProvider) { - await tabPanel.reveal() - await existingProvider.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) - return existingProvider - } + // Serialize overlapping "Open in editor" calls: a double-click starts + // before the first call tracks its new panel, so without a shared + // in-flight creation both calls would race to create two tab panels. + // Concurrent callers await the same promise: exactly one panel is + // created and every caller receives the same provider. + if (pendingTabPanelCreation) { + return pendingTabPanelCreation } - // (This example uses webviewProvider activation event which is necessary to - // deserialize cached webview, but since we use retainContextWhenHidden, we - // don't need to use that event). - // https://github.com/microsoft/vscode-extension-samples/blob/main/webview-sample/src/extension.ts - const contextProxy = await ContextProxy.getInstance(context) - const codeIndexManager = CodeIndexManager.getInstance(context) + const creation = (async () => { + // Reuse the tracked tab instead of opening a second one: a repeated + // "Open in editor" click reveals the existing tab's panel. + if (tabPanel) { + const existingProvider = ClineProvider.getInstanceForView(tabPanel) + if (existingProvider) { + await tabPanel.reveal() + await existingProvider.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) + return existingProvider + } + } - // Get the existing MDM service instance to ensure consistent policy enforcement - let mdmService: MdmService | undefined - try { - mdmService = MdmService.getInstance() - } catch (error) { - // MDM service not initialized, which is fine - extension can work without it - mdmService = undefined - } + // (This example uses webviewProvider activation event which is necessary to + // deserialize cached webview, but since we use retainContextWhenHidden, we + // don't need to use that event). + // https://github.com/microsoft/vscode-extension-samples/blob/main/webview-sample/src/extension.ts + const contextProxy = await ContextProxy.getInstance(context) + const codeIndexManager = CodeIndexManager.getInstance(context) - const tabProvider = new ClineProvider(context, outputChannel, "editor", contextProxy, mdmService) - const lastCol = Math.max(...vscode.window.visibleTextEditors.map((editor) => editor.viewColumn || 0)) + // Get the existing MDM service instance to ensure consistent policy enforcement + let mdmService: MdmService | undefined + try { + mdmService = MdmService.getInstance() + } catch (error) { + // MDM service not initialized, which is fine - extension can work without it + mdmService = undefined + } - // Check if there are any visible text editors, otherwise open a new group - // to the right. - const hasVisibleEditors = vscode.window.visibleTextEditors.length > 0 + const tabProvider = new ClineProvider(context, outputChannel, "editor", contextProxy, mdmService) + const lastCol = Math.max(...vscode.window.visibleTextEditors.map((editor) => editor.viewColumn || 0)) - if (!hasVisibleEditors) { - await vscode.commands.executeCommand("workbench.action.newGroupRight") - } + // Check if there are any visible text editors, otherwise open a new group + // to the right. + const hasVisibleEditors = vscode.window.visibleTextEditors.length > 0 - const targetCol = hasVisibleEditors ? Math.max(lastCol + 1, 1) : vscode.ViewColumn.Two + if (!hasVisibleEditors) { + await vscode.commands.executeCommand("workbench.action.newGroupRight") + } - const newPanel = vscode.window.createWebviewPanel(ClineProvider.tabPanelId, "Zoo Code", targetCol, { - enableScripts: true, - retainContextWhenHidden: true, - localResourceRoots: [context.extensionUri], - }) + const targetCol = hasVisibleEditors ? Math.max(lastCol + 1, 1) : vscode.ViewColumn.Two - // Save as tab type panel. - setPanel(newPanel, "tab") + const newPanel = vscode.window.createWebviewPanel(ClineProvider.tabPanelId, "Zoo Code", targetCol, { + enableScripts: true, + retainContextWhenHidden: true, + localResourceRoots: [context.extensionUri], + }) - // TODO: Use better svg icon with light and dark variants (see - // https://stackoverflow.com/questions/58365687/vscode-extension-iconpath). - newPanel.iconPath = { - light: vscode.Uri.joinPath(context.extensionUri, "assets", "icons", "panel_light.png"), - dark: vscode.Uri.joinPath(context.extensionUri, "assets", "icons", "panel_dark.png"), - } + // Save as tab type panel. + setPanel(newPanel, "tab") + + // TODO: Use better svg icon with light and dark variants (see + // https://stackoverflow.com/questions/58365687/vscode-extension-iconpath). + newPanel.iconPath = { + light: vscode.Uri.joinPath(context.extensionUri, "assets", "icons", "panel_light.png"), + dark: vscode.Uri.joinPath(context.extensionUri, "assets", "icons", "panel_dark.png"), + } - await tabProvider.resolveWebviewView(newPanel) + await tabProvider.resolveWebviewView(newPanel) - // Add listener for visibility changes to notify webview - newPanel.onDidChangeViewState( - (e) => { - const panel = e.webviewPanel - if (panel.visible) { - panel.webview.postMessage({ type: "action", action: "didBecomeVisible" }) // Use the same message type as in SettingsView.tsx - } - }, - null, // First null is for `thisArgs` - context.subscriptions, // Register listener for disposal - ) - - // Handle panel closing events. - newPanel.onDidDispose( - () => { - setPanel(undefined, "tab") - }, - null, - context.subscriptions, // Also register dispose listener - ) - - // Lock the editor group so clicking on files doesn't open them over the panel. - await delay(100) - await vscode.commands.executeCommand("workbench.action.lockEditorGroup") - - return tabProvider + // Add listener for visibility changes to notify webview + newPanel.onDidChangeViewState( + (e) => { + const panel = e.webviewPanel + if (panel.visible) { + panel.webview.postMessage({ type: "action", action: "didBecomeVisible" }) // Use the same message type as in SettingsView.tsx + } + }, + null, // First null is for `thisArgs` + context.subscriptions, // Register listener for disposal + ) + + // Handle panel closing events. + newPanel.onDidDispose( + () => { + setPanel(undefined, "tab") + }, + null, + context.subscriptions, // Also register dispose listener + ) + + // Lock the editor group so clicking on files doesn't open them over the panel. + await delay(100) + await vscode.commands.executeCommand("workbench.action.lockEditorGroup") + + return tabProvider + })() + + pendingTabPanelCreation = creation + + try { + return await creation + } finally { + // Clear once settled (success or failure) so the next call starts + // fresh: the reuse path above then takes over for the tracked panel. + pendingTabPanelCreation = undefined + } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 3f1bf0875e..84b80cf945 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -568,9 +568,8 @@ describe("ClineProvider", () => { }) describe("getInstanceForView", () => { - it("returns the instance that owns the given view", () => { - // @ts-ignore - accessing private property for testing - provider.view = mockWebviewView + it("returns the instance that owns the given view", async () => { + await provider.resolveWebviewView(mockWebviewView) expect(ClineProvider.getInstanceForView(mockWebviewView)).toBe(provider) }) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 381cf0c1e0..544886c2d7 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -64,11 +64,6 @@ "count": 14 } }, - "activate/__tests__/registerCommands.spec.ts": { - "@typescript-eslint/no-explicit-any": { - "count": 2 - } - }, "activate/registerCodeActions.ts": { "@typescript-eslint/no-explicit-any": { "count": 2 From c3027c8f1408bb0293c3ba36dc27059c7c5251d5 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Mon, 7 Sep 2026 16:13:46 +0800 Subject: [PATCH 04/18] test(activate): pin openClineInNewTab creation branches and strengthen the concurrency assertion --- .../__tests__/registerCommands.spec.ts | 122 +++++++++++++++++- 1 file changed, 118 insertions(+), 4 deletions(-) diff --git a/src/activate/__tests__/registerCommands.spec.ts b/src/activate/__tests__/registerCommands.spec.ts index 1672c74d21..05e71824bf 100644 --- a/src/activate/__tests__/registerCommands.spec.ts +++ b/src/activate/__tests__/registerCommands.spec.ts @@ -3,8 +3,9 @@ import * as vscode from "vscode" import { TelemetryService } from "@roo-code/telemetry" import { ClineProvider } from "../../core/webview/ClineProvider" +import { MdmService } from "../../services/mdm/MdmService" -import { getVisibleProviderOrLog, openClineInNewTab, registerCommands, setPanel } from "../registerCommands" +import { getPanel, getVisibleProviderOrLog, openClineInNewTab, registerCommands, setPanel } from "../registerCommands" vi.mock("execa", () => ({ execa: vi.fn(), @@ -641,6 +642,113 @@ describe("openClineInNewTab", () => { expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) }) + it("falls back to an undefined MdmService when MdmService.getInstance throws", async () => { + ;(MdmService.getInstance as Mock).mockImplementation(() => { + throw new Error("MDM service not initialized") + }) + + const provider = await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + // The creation must survive the MDM lookup failure: the provider is + // constructed with an undefined MDM service and the tab panel is + // still created. + const ctor = ClineProvider as unknown as Mock + expect(ctor.mock.instances[0]).toBeDefined() + expect(ctor).toHaveBeenCalledWith(mockContext, mockOutputChannel, "editor", undefined, undefined) + expect(provider).toBe(ctor.mock.instances[0]) + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) + }) + + it("opens a new group to the right and targets ViewColumn.Two when no editors are visible", async () => { + ;(vscode.window as unknown as { visibleTextEditors: vscode.TextEditor[] }).visibleTextEditors = [] + + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + expect(vscode.commands.executeCommand).toHaveBeenCalledWith("workbench.action.newGroupRight") + expect(vscode.commands.executeCommand).toHaveBeenCalledWith("workbench.action.lockEditorGroup") + expect(vscode.window.createWebviewPanel).toHaveBeenCalledWith( + "zoo-code.TabPanelProvider", + "Zoo Code", + vscode.ViewColumn.Two, + { + enableScripts: true, + retainContextWhenHidden: true, + localResourceRoots: [mockContext.extensionUri], + }, + ) + + // The panel icon points at the extension's asset files. + const panel = (vscode.window.createWebviewPanel as Mock).mock.results[0].value as { + iconPath?: { light: { path: string }; dark: { path: string } } + } + expect(panel.iconPath).toEqual({ + light: { path: "assets/icons/panel_light.png" }, + dark: { path: "assets/icons/panel_dark.png" }, + }) + }) + + it("treats editors without a viewColumn as column 0 when computing the target column", async () => { + // openClineInNewTab only reads viewColumn from each editor, so the + // fixture keeps that single field. + const editorWithoutColumn = { viewColumn: undefined } as unknown as vscode.TextEditor + ;(vscode.window as unknown as { visibleTextEditors: vscode.TextEditor[] }).visibleTextEditors = [ + editorWithoutColumn, + ] + + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + // lastCol falls back to 0, so the panel lands on column 1 instead of + // opening a new editor group. + expect(vscode.commands.executeCommand).not.toHaveBeenCalledWith("workbench.action.newGroupRight") + expect(vscode.window.createWebviewPanel).toHaveBeenCalledWith( + "zoo-code.TabPanelProvider", + "Zoo Code", + 1, + expect.objectContaining({ enableScripts: true }), + ) + }) + + it("constructs the tab provider with the 'editor' context and the live MdmService instance", async () => { + const mockMdm = { name: "mock-mdm" } + // MdmService has a private constructor, so pin a sentinel stand-in. + ;(MdmService.getInstance as Mock).mockReturnValue(mockMdm as unknown as MdmService) + + const provider = await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + const ctor = ClineProvider as unknown as Mock + expect(ctor).toHaveBeenCalledTimes(1) + expect(ctor).toHaveBeenCalledWith(mockContext, mockOutputChannel, "editor", undefined, mockMdm) + expect(provider).toBe(ctor.mock.instances[0]) + }) + + it("posts didBecomeVisible only for visible state changes and clears the tracked tab on dispose", async () => { + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + expect(getPanel()).toBeDefined() + + const panel = (vscode.window.createWebviewPanel as Mock).mock.results[0].value as { + onDidChangeViewState: Mock + onDidDispose: Mock + } + const stateHandler = panel.onDidChangeViewState.mock.calls[0][0] as (event: { + webviewPanel: { visible: boolean; webview: { postMessage: (message: unknown) => void } } + }) => void + const visibleEvent = { webviewPanel: { visible: true, webview: { postMessage: vi.fn() } } } + stateHandler(visibleEvent) + expect(visibleEvent.webviewPanel.webview.postMessage).toHaveBeenCalledWith({ + type: "action", + action: "didBecomeVisible", + }) + + const hiddenEvent = { webviewPanel: { visible: false, webview: { postMessage: vi.fn() } } } + stateHandler(hiddenEvent) + expect(hiddenEvent.webviewPanel.webview.postMessage).not.toHaveBeenCalled() + + const disposeHandler = panel.onDidDispose.mock.calls[0][0] as () => void + disposeHandler() + expect(getPanel()).toBeUndefined() + }) + it("serializes concurrent opens so overlapping calls create one panel and share one provider", async () => { const [first, second] = await Promise.all([ openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }), @@ -648,9 +756,15 @@ describe("openClineInNewTab", () => { ]) // Overlapping "Open in editor" calls must share the in-flight - // creation: exactly one tab panel is created and both callers - // receive the same provider. - expect(first).toBe(second) + // creation: exactly one tab panel is created and both callers receive + // the same constructed provider. Pinning both results against the + // mocked constructor (not just against each other) keeps the test + // failing if the shared result is undefined. + const ctor = ClineProvider as unknown as Mock + const constructed = ctor.mock.instances[0] + expect(constructed).toBeDefined() + expect(first).toBe(constructed) + expect(second).toBe(constructed) expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) }) }) From 00eb15fd3c0d88725ee3887d4b1086f9d550210a Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 00:11:44 +0800 Subject: [PATCH 05/18] fix(activate): harden tab creation serialization and centralize title-bar posts - openClineInNewTab: extract the unserialized creation body into createTabPanelUnlocked and guard the in-flight slot clear so a settled creation cannot clobber a replacement already stored in the slot. - onDidDispose: clear the tracked tab ref only when the disposing panel is still the tracked one, so a late disposal of a replaced panel cannot clobber the replacement's ref. - MDM lookup failure: log the fallback to the output channel instead of swallowing it silently. - Route the six title-bar button handlers through a shared postActions helper that posts each action in order and logs failures with the handler-specific prefix. - package.json: add the four InTab commands to the command palette, scoped to the active tab panel. - Tests: handler-level regression for openInNewTab + popoutButtonClicked started before the first creation resolves; fresh-creation test for a settled in-flight promise; stale-panel disposal regression; retained panel assertion for disposed tab instances; rightmost-editor column placement assertion; MDM fallback output assertion; %s placeholders for primitive it.each titles. - Stryker directives for the two equivalent setPanel type-literal mutants (setPanel branches only on type === sidebar). --- .../__tests__/registerCommands.spec.ts | 152 ++++++++++- src/activate/registerCommands.ts | 252 ++++++++++-------- src/package.json | 18 ++ 3 files changed, 301 insertions(+), 121 deletions(-) diff --git a/src/activate/__tests__/registerCommands.spec.ts b/src/activate/__tests__/registerCommands.spec.ts index 05e71824bf..d57c6f8e9f 100644 --- a/src/activate/__tests__/registerCommands.spec.ts +++ b/src/activate/__tests__/registerCommands.spec.ts @@ -2,6 +2,7 @@ import type { Mock } from "vitest" import * as vscode from "vscode" import { TelemetryService } from "@roo-code/telemetry" +import { ContextProxy } from "../../core/config/ContextProxy" import { ClineProvider } from "../../core/webview/ClineProvider" import { MdmService } from "../../services/mdm/MdmService" @@ -283,7 +284,7 @@ describe("registerCommands handlers", () => { "zoo-code.historyButtonClickedInTab", "zoo-code.marketplaceButtonClickedInTab", ] - it.each(inTabNoOpCommands)("$command is a no-op when no tab panel is tracked", async (command) => { + it.each(inTabNoOpCommands)("%s is a no-op when no tab panel is tracked", async (command) => { await handlers[command]() expect(ClineProvider.getInstanceForView as Mock).not.toHaveBeenCalled() @@ -291,12 +292,14 @@ describe("registerCommands handlers", () => { expect(mockVisibleProvider.postMessageToWebview).not.toHaveBeenCalled() }) - it.each(inTabNoOpCommands)("$command is a no-op when the tab instance is disposed", async (command) => { - setPanel({} as vscode.WebviewPanel, "tab") + it.each(inTabNoOpCommands)("%s is a no-op when the tab instance is disposed", async (command) => { + const disposedPanel = {} as vscode.WebviewPanel + setPanel(disposedPanel, "tab") ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(undefined) await handlers[command]() + expect(ClineProvider.getInstanceForView as Mock).toHaveBeenCalledWith(disposedPanel) expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() expect(mockVisibleProvider.postMessageToWebview).not.toHaveBeenCalled() }) @@ -657,6 +660,11 @@ describe("openClineInNewTab", () => { expect(ctor).toHaveBeenCalledWith(mockContext, mockOutputChannel, "editor", undefined, undefined) expect(provider).toBe(ctor.mock.instances[0]) expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) + + // The fallback is observable in the output channel. + expect(mockOutputChannel.appendLine).toHaveBeenCalledWith( + "[openClineInNewTab] MDM service unavailable, continuing without it: Error: MDM service not initialized", + ) }) it("opens a new group to the right and targets ViewColumn.Two when no editors are visible", async () => { @@ -708,6 +716,27 @@ describe("openClineInNewTab", () => { ) }) + it("places the tab panel one column right of the rightmost visible editor", async () => { + // openClineInNewTab only reads viewColumn from each editor, so the + // fixtures keep that single field. + ;(vscode.window as unknown as { visibleTextEditors: vscode.TextEditor[] }).visibleTextEditors = [ + { viewColumn: 1 } as unknown as vscode.TextEditor, + { viewColumn: 3 } as unknown as vscode.TextEditor, + ] + + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + // lastCol is 3, so the panel lands on column 4 without opening a new + // editor group. + expect(vscode.commands.executeCommand).not.toHaveBeenCalledWith("workbench.action.newGroupRight") + expect(vscode.window.createWebviewPanel).toHaveBeenCalledWith( + "zoo-code.TabPanelProvider", + "Zoo Code", + 4, + expect.objectContaining({ enableScripts: true }), + ) + }) + it("constructs the tab provider with the 'editor' context and the live MdmService instance", async () => { const mockMdm = { name: "mock-mdm" } // MdmService has a private constructor, so pin a sentinel stand-in. @@ -767,4 +796,121 @@ describe("openClineInNewTab", () => { expect(second).toBe(constructed) expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) }) + + it("shares one in-flight creation when openInNewTab and popoutButtonClicked start before it resolves", async () => { + // Defer the first creation at ContextProxy.getInstance so both command + // handlers can start while the creation is still in flight. + let resolveContextProxy!: () => void + ;(ContextProxy.getInstance as Mock).mockReturnValue( + new Promise((resolve) => { + resolveContextProxy = resolve + }), + ) + + const commandHandlers: Record unknown> = {} + ;(vscode.commands.registerCommand as Mock).mockImplementation( + (id: string, cb: (...args: unknown[]) => unknown) => { + commandHandlers[id] = cb + return { dispose: vi.fn() } + }, + ) + const sidebarProvider = { postMessageToWebview: vi.fn().mockResolvedValue(undefined) } + registerCommands({ + context: mockContext, + outputChannel: mockOutputChannel, + provider: sidebarProvider as unknown as ClineProvider, + }) + + const started = [commandHandlers["zoo-code.openInNewTab"](), commandHandlers["zoo-code.popoutButtonClicked"]()] + + // While the shared creation is suspended at ContextProxy.getInstance, + // neither caller has created a panel yet. + expect(vscode.window.createWebviewPanel).not.toHaveBeenCalled() + + resolveContextProxy() + const [first, second] = await Promise.all(started) + + // Both command entry points await the shared in-flight creation: + // exactly one tab panel is created and both results are the same + // constructed provider. + const ctor = ClineProvider as unknown as Mock + const constructed = ctor.mock.instances[0] + expect(constructed).toBeDefined() + expect(first).toBe(constructed) + expect(second).toBe(constructed) + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) + }) + + it("creates a fresh panel for a new call once the previous creation settled and its provider disposed", async () => { + // The first open settles and tracks its panel. + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) + + // The tracked provider is disposed, so the next open cannot reuse the + // existing tab: the settled (and cleared) in-flight promise must not + // be returned, and a fresh panel is created. + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(undefined) + + const secondProvider = await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + const ctor = ClineProvider as unknown as Mock + const second = ctor.mock.instances[1] + expect(second).toBeDefined() + expect(secondProvider).toBe(second) + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(2) + }) + + it("keeps the replacement panel tracked when a stale panel's disposal fires late", async () => { + // Capture each created panel so the first panel's (stale) dispose + // handler can fire after the replacement is already tracked. + const createdPanels: { onDidDispose: Mock }[] = [] + ;(vscode.window.createWebviewPanel as Mock).mockImplementation(() => { + const panel = { + webview: { postMessage: vi.fn() }, + onDidChangeViewState: vi.fn(), + onDidDispose: vi.fn(), + } + createdPanels.push(panel) + return panel + }) + + // First open creates and tracks panel A. + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + expect(getPanel()).toBe(createdPanels[0]) + + // Panel A's provider is disposed before the second open, so the + // second open creates the replacement panel B. + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(undefined) + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(2) + expect(getPanel()).toBe(createdPanels[1]) + + // Panel A's stale dispose handler fires after the replacement is + // tracked; it must not clobber the replacement's ref. + createdPanels[0].onDidDispose.mock.calls[0][0]() + + expect(getPanel()).toBe(createdPanels[1]) + + // Tab-surface commands still reach the provider that owns the + // replacement panel after the stale disposal. + const replacementProvider = { postMessageToWebview: vi.fn().mockResolvedValue(undefined) } + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(replacementProvider) + const commandHandlers: Record unknown> = {} + ;(vscode.commands.registerCommand as Mock).mockImplementation( + (id: string, cb: (...args: unknown[]) => unknown) => { + commandHandlers[id] = cb + return { dispose: vi.fn() } + }, + ) + registerCommands({ + context: mockContext, + outputChannel: mockOutputChannel, + provider: {} as ClineProvider, + }) + await commandHandlers["zoo-code.historyButtonClickedInTab"]() + expect(replacementProvider.postMessageToWebview).toHaveBeenCalledWith({ + type: "action", + action: "historyButtonClicked", + }) + }) }) diff --git a/src/activate/registerCommands.ts b/src/activate/registerCommands.ts index 5fd5171a83..336111e72e 100644 --- a/src/activate/registerCommands.ts +++ b/src/activate/registerCommands.ts @@ -1,7 +1,7 @@ import * as vscode from "vscode" import delay from "delay" -import type { CommandId } from "@roo-code/types" +import type { CommandId, ExtensionMessage } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { Package } from "../shared/package" @@ -105,6 +105,23 @@ export const registerCommands = (options: RegisterCommandOptions) => { // `filePath?: string`, others take none) and VS Code dispatches positional // args dynamically. type CommandCallback = (...args: any[]) => unknown + +// Posts each action in order to the target instance. Failures are logged +// (not thrown) with the handler-specific prefix so a failed post stays +// attributable in the output channel. +const postActions = ( + outputChannel: vscode.OutputChannel, + target: ClineProvider, + actions: readonly NonNullable[], + logPrefix: string, +) => { + for (const action of actions) { + void target + .postMessageToWebview({ type: "action", action }) + .catch((error) => outputChannel.appendLine(`[${logPrefix}] postMessageToWebview failed: ${error}`)) + } +} + const getCommandsMap = ({ context, outputChannel, @@ -150,13 +167,8 @@ const getCommandsMap = ({ settingsButtonClicked: () => { TelemetryService.instance.captureTitleButtonClicked("settings") - void provider - .postMessageToWebview({ type: "action", action: "settingsButtonClicked" }) - .catch((error) => outputChannel.appendLine(`[settingsButtonClicked] postMessageToWebview failed: ${error}`)) - // Also explicitly post the visibility message to trigger scroll reliably - void provider - .postMessageToWebview({ type: "action", action: "didBecomeVisible" }) - .catch((error) => outputChannel.appendLine(`[settingsButtonClicked] postMessageToWebview failed: ${error}`)) + // Also explicitly post the visibility message to trigger scroll reliably. + postActions(outputChannel, provider, ["settingsButtonClicked", "didBecomeVisible"], "settingsButtonClicked") }, settingsButtonClickedInTab: () => { const tabProvider = getTabProvider() @@ -166,23 +178,17 @@ const getCommandsMap = ({ TelemetryService.instance.captureTitleButtonClicked("settings") - void tabProvider - .postMessageToWebview({ type: "action", action: "settingsButtonClicked" }) - .catch((error) => - outputChannel.appendLine(`[settingsButtonClickedInTab] postMessageToWebview failed: ${error}`), - ) - void tabProvider - .postMessageToWebview({ type: "action", action: "didBecomeVisible" }) - .catch((error) => - outputChannel.appendLine(`[settingsButtonClickedInTab] postMessageToWebview failed: ${error}`), - ) + postActions( + outputChannel, + tabProvider, + ["settingsButtonClicked", "didBecomeVisible"], + "settingsButtonClickedInTab", + ) }, historyButtonClicked: () => { TelemetryService.instance.captureTitleButtonClicked("history") - void provider - .postMessageToWebview({ type: "action", action: "historyButtonClicked" }) - .catch((error) => outputChannel.appendLine(`[historyButtonClicked] postMessageToWebview failed: ${error}`)) + postActions(outputChannel, provider, ["historyButtonClicked"], "historyButtonClicked") }, historyButtonClickedInTab: () => { const tabProvider = getTabProvider() @@ -192,29 +198,17 @@ const getCommandsMap = ({ TelemetryService.instance.captureTitleButtonClicked("history") - void tabProvider - .postMessageToWebview({ type: "action", action: "historyButtonClicked" }) - .catch((error) => - outputChannel.appendLine(`[historyButtonClickedInTab] postMessageToWebview failed: ${error}`), - ) + postActions(outputChannel, tabProvider, ["historyButtonClicked"], "historyButtonClickedInTab") }, marketplaceButtonClicked: () => { - void provider - .postMessageToWebview({ type: "action", action: "marketplaceButtonClicked" }) - .catch((error) => - outputChannel.appendLine(`[marketplaceButtonClicked] postMessageToWebview failed: ${error}`), - ) + postActions(outputChannel, provider, ["marketplaceButtonClicked"], "marketplaceButtonClicked") }, marketplaceButtonClickedInTab: () => { const tabProvider = getTabProvider() if (!tabProvider) { return } - void tabProvider - .postMessageToWebview({ type: "action", action: "marketplaceButtonClicked" }) - .catch((error) => - outputChannel.appendLine(`[marketplaceButtonClickedInTab] postMessageToWebview failed: ${error}`), - ) + postActions(outputChannel, tabProvider, ["marketplaceButtonClicked"], "marketplaceButtonClickedInTab") }, newTask: handleNewTask, setCustomStoragePath: async () => { @@ -292,106 +286,128 @@ export const openClineInNewTab = async ({ context, outputChannel }: Omit { - // Reuse the tracked tab instead of opening a second one: a repeated - // "Open in editor" click reveals the existing tab's panel. - if (tabPanel) { - const existingProvider = ClineProvider.getInstanceForView(tabPanel) - if (existingProvider) { - await tabPanel.reveal() - await existingProvider.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) - return existingProvider - } - } - - // (This example uses webviewProvider activation event which is necessary to - // deserialize cached webview, but since we use retainContextWhenHidden, we - // don't need to use that event). - // https://github.com/microsoft/vscode-extension-samples/blob/main/webview-sample/src/extension.ts - const contextProxy = await ContextProxy.getInstance(context) - const codeIndexManager = CodeIndexManager.getInstance(context) + const creation = createTabPanelUnlocked({ context, outputChannel }) + pendingTabPanelCreation = creation - // Get the existing MDM service instance to ensure consistent policy enforcement - let mdmService: MdmService | undefined - try { - mdmService = MdmService.getInstance() - } catch (error) { - // MDM service not initialized, which is fine - extension can work without it - mdmService = undefined + try { + return await creation + } finally { + // Clear once settled (success or failure) so the next call starts + // fresh: the reuse path in createTabPanelUnlocked then takes over + // for the tracked panel. Guard the clear so this settlement cannot + // clobber a replacement already stored in the slot. That clobber is + // unreachable in single-threaded settlement order: while the slot + // holds this in-flight creation, every other caller receives that + // same promise (guard above), so no replacement can be stored before + // this finally block runs ā€” the equality check pins the invariant. + // Stryker disable next-line ConditionalExpression: defensive clobber guard, unreachable per the ordering argument above. + if (pendingTabPanelCreation === creation) { + pendingTabPanelCreation = undefined } + } +} - const tabProvider = new ClineProvider(context, outputChannel, "editor", contextProxy, mdmService) - const lastCol = Math.max(...vscode.window.visibleTextEditors.map((editor) => editor.viewColumn || 0)) +// The unserialized tab-creation body. Only openClineInNewTab may call it, +// after it has stored the shared in-flight promise. +const createTabPanelUnlocked = async ({ context, outputChannel }: Omit) => { + // Reuse the tracked tab instead of opening a second one: a repeated + // "Open in editor" click reveals the existing tab's panel. + if (tabPanel) { + const existingProvider = ClineProvider.getInstanceForView(tabPanel) + if (existingProvider) { + await tabPanel.reveal() + await existingProvider.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) + return existingProvider + } + } - // Check if there are any visible text editors, otherwise open a new group - // to the right. - const hasVisibleEditors = vscode.window.visibleTextEditors.length > 0 + // (This example uses webviewProvider activation event which is necessary to + // deserialize cached webview, but since we use retainContextWhenHidden, we + // don't need to use that event). + // https://github.com/microsoft/vscode-extension-samples/blob/main/webview-sample/src/extension.ts + const contextProxy = await ContextProxy.getInstance(context) + const codeIndexManager = CodeIndexManager.getInstance(context) - if (!hasVisibleEditors) { - await vscode.commands.executeCommand("workbench.action.newGroupRight") - } + // Get the existing MDM service instance to ensure consistent policy enforcement + let mdmService: MdmService | undefined + try { + mdmService = MdmService.getInstance() + } catch (error) { + // MDM service unavailable: log the fallback and continue without it. + outputChannel.appendLine(`[openClineInNewTab] MDM service unavailable, continuing without it: ${error}`) + mdmService = undefined + } - const targetCol = hasVisibleEditors ? Math.max(lastCol + 1, 1) : vscode.ViewColumn.Two + const tabProvider = new ClineProvider(context, outputChannel, "editor", contextProxy, mdmService) + const lastCol = Math.max(...vscode.window.visibleTextEditors.map((editor) => editor.viewColumn || 0)) - const newPanel = vscode.window.createWebviewPanel(ClineProvider.tabPanelId, "Zoo Code", targetCol, { - enableScripts: true, - retainContextWhenHidden: true, - localResourceRoots: [context.extensionUri], - }) + // Check if there are any visible text editors, otherwise open a new group + // to the right. + const hasVisibleEditors = vscode.window.visibleTextEditors.length > 0 - // Save as tab type panel. - setPanel(newPanel, "tab") + if (!hasVisibleEditors) { + await vscode.commands.executeCommand("workbench.action.newGroupRight") + } - // TODO: Use better svg icon with light and dark variants (see - // https://stackoverflow.com/questions/58365687/vscode-extension-iconpath). - newPanel.iconPath = { - light: vscode.Uri.joinPath(context.extensionUri, "assets", "icons", "panel_light.png"), - dark: vscode.Uri.joinPath(context.extensionUri, "assets", "icons", "panel_dark.png"), - } + const targetCol = hasVisibleEditors ? Math.max(lastCol + 1, 1) : vscode.ViewColumn.Two - await tabProvider.resolveWebviewView(newPanel) + const newPanel = vscode.window.createWebviewPanel(ClineProvider.tabPanelId, "Zoo Code", targetCol, { + enableScripts: true, + retainContextWhenHidden: true, + localResourceRoots: [context.extensionUri], + }) - // Add listener for visibility changes to notify webview - newPanel.onDidChangeViewState( - (e) => { - const panel = e.webviewPanel - if (panel.visible) { - panel.webview.postMessage({ type: "action", action: "didBecomeVisible" }) // Use the same message type as in SettingsView.tsx - } - }, - null, // First null is for `thisArgs` - context.subscriptions, // Register listener for disposal - ) + // Save as tab type panel. + // Stryker disable next-line StringLiteral: setPanel branches only on type === "sidebar", so any other literal routes to the identical tab-ref assignment + setPanel(newPanel, "tab") - // Handle panel closing events. - newPanel.onDidDispose( - () => { - setPanel(undefined, "tab") - }, - null, - context.subscriptions, // Also register dispose listener - ) + // TODO: Use better svg icon with light and dark variants (see + // https://stackoverflow.com/questions/58365687/vscode-extension-iconpath). + newPanel.iconPath = { + light: vscode.Uri.joinPath(context.extensionUri, "assets", "icons", "panel_light.png"), + dark: vscode.Uri.joinPath(context.extensionUri, "assets", "icons", "panel_dark.png"), + } - // Lock the editor group so clicking on files doesn't open them over the panel. - await delay(100) - await vscode.commands.executeCommand("workbench.action.lockEditorGroup") + await tabProvider.resolveWebviewView(newPanel) - return tabProvider - })() + // Add listener for visibility changes to notify webview + newPanel.onDidChangeViewState( + (e) => { + const panel = e.webviewPanel + if (panel.visible) { + panel.webview.postMessage({ type: "action", action: "didBecomeVisible" }) // Use the same message type as in SettingsView.tsx + } + }, + null, // First null is for `thisArgs` + context.subscriptions, // Register listener for disposal + ) + + // Handle panel closing events: clear the tracked ref only if this panel + // is still the tracked one, so a late disposal of an already-replaced + // panel cannot clobber the replacement's ref. + newPanel.onDidDispose( + () => { + if (tabPanel === newPanel) { + // Stryker disable next-line StringLiteral: setPanel branches only on type === "sidebar", so any other literal routes to the identical tab-ref assignment + setPanel(undefined, "tab") + } + }, + null, + context.subscriptions, // Also register dispose listener + ) - pendingTabPanelCreation = creation + // Lock the editor group so clicking on files doesn't open them over the panel. + await delay(100) + await vscode.commands.executeCommand("workbench.action.lockEditorGroup") - try { - return await creation - } finally { - // Clear once settled (success or failure) so the next call starts - // fresh: the reuse path above then takes over for the tracked panel. - pendingTabPanelCreation = undefined - } + return tabProvider } diff --git a/src/package.json b/src/package.json index 6753513658..2b7c018bf4 100644 --- a/src/package.json +++ b/src/package.json @@ -287,6 +287,24 @@ } ] }, + "commandPalette": [ + { + "command": "zoo-code.plusButtonClickedInTab", + "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + }, + { + "command": "zoo-code.settingsButtonClickedInTab", + "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + }, + { + "command": "zoo-code.marketplaceButtonClickedInTab", + "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + }, + { + "command": "zoo-code.historyButtonClickedInTab", + "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + } + ], "keybindings": [ { "command": "zoo-code.addToContext", From ae038ab615784bda694c1776cfe972169c98253b Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 04:22:14 +0800 Subject: [PATCH 06/18] test(activate): pin tracked tab identity against the created panel Replace the weak toBeDefined() assertion in the dispose spec with an identity check against the panel returned during creation, per the CodeRabbit actionable comment on this PR (review run 7c4cfeb3-6dd9-4615- 9a58-70cfc705eca2). The tracked tab is now pinned with toBe(panel) before the dispose assertions, so a wrong or duplicated tracked panel fails the suite instead of passing a defined-only check. Upstream: Zoo-Code-Org/Zoo-Code#1528 (vps2 F0) --- src/activate/__tests__/registerCommands.spec.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/activate/__tests__/registerCommands.spec.ts b/src/activate/__tests__/registerCommands.spec.ts index d57c6f8e9f..7b4b4c1080 100644 --- a/src/activate/__tests__/registerCommands.spec.ts +++ b/src/activate/__tests__/registerCommands.spec.ts @@ -753,12 +753,15 @@ describe("openClineInNewTab", () => { it("posts didBecomeVisible only for visible state changes and clears the tracked tab on dispose", async () => { await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) - expect(getPanel()).toBeDefined() - + // Retain the panel returned during creation and pin the tracked tab + // against it with identity (not a weak defined check), so a wrong or + // duplicated tracked panel fails before the dispose assertions. const panel = (vscode.window.createWebviewPanel as Mock).mock.results[0].value as { onDidChangeViewState: Mock onDidDispose: Mock } + expect(getPanel()).toBe(panel) + const stateHandler = panel.onDidChangeViewState.mock.calls[0][0] as (event: { webviewPanel: { visible: boolean; webview: { postMessage: (message: unknown) => void } } }) => void From 99662bdb8d3f85d490038fb104b036c1030c2ef1 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 04:51:48 +0800 Subject: [PATCH 07/18] test(activate): pin the tracked tab panel passed to getInstanceForView Retain the tracked tab panel in the InTab handler cases and assert that getInstanceForView was called with that exact panel, per the CodeRabbit actionable comment on this PR (review run 4afe1273-8739-4235-90d3-311db5f6ccb9, inline comment 3952466254 on the tabHandlerCases spec). A handler resolving any other view now fails instead of passing on the stubbed provider result alone; the same identity pin is applied to plusButtonClickedInTab. Upstream: Zoo-Code-Org/Zoo-Code#1528 (vps2 F0) --- src/activate/__tests__/registerCommands.spec.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/activate/__tests__/registerCommands.spec.ts b/src/activate/__tests__/registerCommands.spec.ts index 7b4b4c1080..e53a1bc2c2 100644 --- a/src/activate/__tests__/registerCommands.spec.ts +++ b/src/activate/__tests__/registerCommands.spec.ts @@ -257,11 +257,17 @@ describe("registerCommands handlers", () => { "$command targets the tab instance for the tracked tab panel", ({ command, actions, telemetry }) => { const mockTabProvider = { postMessageToWebview: vi.fn().mockResolvedValue(undefined) } - setPanel({} as vscode.WebviewPanel, "tab") + // Retain the tracked tab panel and pin the instance lookup + // against its identity: a handler that resolved the sidebar view + // or any other view must fail instead of passing on the stubbed + // provider result alone. + const tabPanel = {} as vscode.WebviewPanel + setPanel(tabPanel, "tab") ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(mockTabProvider) handlers[command]() + expect(ClineProvider.getInstanceForView as Mock).toHaveBeenCalledWith(tabPanel) for (const action of actions) { expect(mockTabProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action }) } @@ -539,11 +545,15 @@ describe("registerCommands handlers", () => { evictCurrentTask: vi.fn().mockResolvedValue(undefined), refreshWorkspace: vi.fn().mockResolvedValue(undefined), } - setPanel({} as vscode.WebviewPanel, "tab") + // Same identity pin as the other InTab cases: the eviction must run + // against the provider resolved from the exact tracked tab panel. + const tabPanel = {} as vscode.WebviewPanel + setPanel(tabPanel, "tab") ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(mockTabProvider) await handlers["zoo-code.plusButtonClickedInTab"]() + expect(ClineProvider.getInstanceForView as Mock).toHaveBeenCalledWith(tabPanel) expect(TelemetryService.instance.captureTitleButtonClicked).toHaveBeenCalledWith("plus") expect(mockTabProvider.evictCurrentTask).toHaveBeenCalledTimes(1) expect(mockTabProvider.refreshWorkspace).toHaveBeenCalledTimes(1) From 7be370aa8f2e7dd706829f3cf6ddb751caa066d9 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sat, 5 Sep 2026 15:00:35 +0800 Subject: [PATCH 08/18] feat(provider): persist per-view view-state identity and durable viewStates Each ClineProvider instance now owns a unique viewId (renderContext plus a monotonic counter) and registers a stable viewStateId for durable persistence. - Per-view state buffer (viewLocalState) holds mode / currentApiConfigName / apiConfiguration overrides in memory; saveViewState persists the non-secret subset durably under the active view id, rekeyed to the stable id on registration. - viewStates is stored as a map pruned to the newest 50 entries; writes go through a serialized queue so concurrent provider instances merge without lost updates. - setViewStateId sanitizes ids and rejects "__proto__" so a per-view entry can never be keyed through the Object.prototype setter. - postMessageToWebview no longer awaits the webview ack: a remounted or disposed page never acknowledges, and awaiting would wedge task-critical callers. - History restore falls back to the default mode view-locally instead of writing the shared global mode. - GlobalState gains the "viewStates" key and GLOBAL_STATE_KEYS tracks it. Adds F1a coverage in ClineProvider.spec.ts (viewId uniqueness, saveViewState persistence semantics, loadViewState fallback and failure, pruning, the __proto__ guard) and adapts the two history-restore tests in ClineProvider.sticky-mode.spec.ts to the view-local restore. getState() merging of hydrated per-view values and the remaining view-state suites land in the follow-up (F1b). --- packages/types/src/__tests__/index.test.ts | 5 + packages/types/src/global-settings.ts | 10 + packages/types/src/vscode-extension-host.ts | 1 + src/core/webview/ClineProvider.ts | 391 +++++++++++- .../webview/__tests__/ClineProvider.spec.ts | 575 +++++++++++++++++- .../ClineProvider.sticky-mode.spec.ts | 15 +- src/eslint-suppressions.json | 2 +- 7 files changed, 976 insertions(+), 23 deletions(-) diff --git a/packages/types/src/__tests__/index.test.ts b/packages/types/src/__tests__/index.test.ts index 15441d48fd..b4cee22f8c 100644 --- a/packages/types/src/__tests__/index.test.ts +++ b/packages/types/src/__tests__/index.test.ts @@ -3,6 +3,10 @@ import { GLOBAL_STATE_KEYS } from "../index.js" describe("GLOBAL_STATE_KEYS", () => { + it("should contain registered durable per-view state", () => { + expect(GLOBAL_STATE_KEYS).toContain("viewStates") + }) + it("should contain provider settings keys", () => { expect(GLOBAL_STATE_KEYS).toContain("autoApprovalEnabled") }) @@ -13,6 +17,7 @@ describe("GLOBAL_STATE_KEYS", () => { it("should not contain secret state keys", () => { expect(GLOBAL_STATE_KEYS).not.toContain("openRouterApiKey") + expect(GLOBAL_STATE_KEYS).not.toContain("apiKey") }) it("should contain OpenAI Compatible base URL setting", () => { diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index 95f246dbe7..d3bc3efd1a 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -99,6 +99,15 @@ export const MAX_CHECKPOINT_TIMEOUT_SECONDS = 60 */ export const DEFAULT_CHECKPOINT_TIMEOUT_SECONDS = 15 +/** + * Persisted non-secret selections for a stable webview instance. + */ +export const viewStateSchema = z.object({ + mode: z.string().optional(), + currentApiConfigName: z.string().optional(), + updatedAt: z.number().optional(), +}) + /** * GlobalSettings */ @@ -107,6 +116,7 @@ export const globalSettingsSchema = z.object({ currentApiConfigName: z.string().optional(), listApiConfigMeta: z.array(providerSettingsEntrySchema).optional(), pinnedApiConfigs: z.record(z.string(), z.boolean()).optional(), + viewStates: z.record(z.string(), viewStateSchema).optional(), lastShownAnnouncementId: z.string().optional(), customInstructions: z.string().optional(), diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 5f6b579779..26d9aeb240 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -647,6 +647,7 @@ export interface WebviewMessage { | "openRulesDirectory" | "themeFixtureProbeResponse" text?: string + viewStateId?: string taskId?: string editedMessageContent?: string tab?: "settings" | "history" | "mcp" | "modes" | "chat" | "marketplace" | "cloud" diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 29c32ae9cf..a4583e6bc6 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -56,6 +56,7 @@ import { getModelId, isRetiredProvider, providerIdentifiers, + PROVIDER_SETTINGS_KEYS, } from "@roo-code/types" import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock" import { TaskRegistry } from "../task/TaskRegistry" @@ -128,6 +129,14 @@ import { REQUESTY_BASE_URL } from "../../shared/utils/requesty" import { validateAndFixToolResultIds } from "../task/validateToolResultIds" import { PendingEditOperationStore, type PendingEditOperationInput } from "./PendingEditOperationStore" +type PersistedViewState = NonNullable[string] + +/** + * Values that can be held in a view-local state buffer (in-memory) and, for the + * non-secret subset, persisted durably per stable view id. + */ +type ViewLocalStateValues = Partial & Partial + /** * https://github.com/microsoft/vscode-webview-ui-toolkit-samples/blob/main/default/weather-webview/src/providers/WeatherViewProvider.ts * https://github.com/KumarVariable/vscode-extension-sidebar-html/blob/master/src/customSidebarViewProvider.ts @@ -183,6 +192,9 @@ export class ClineProvider public static readonly sideBarId = `${Package.name}.SidebarProvider` public static readonly tabPanelId = `${Package.name}.TabPanelProvider` private static activeInstances: Set = new Set() + private static nextViewId = 0 + private static readonly MAX_PERSISTED_VIEW_STATES = 50 + private static persistedViewStateWriteQueue: Promise = Promise.resolve() private disposables: vscode.Disposable[] = [] private webviewDisposables: vscode.Disposable[] = [] private pendingThemeFixtureProbes = new Map< @@ -307,6 +319,25 @@ export class ClineProvider */ private clineMessagesSeq = 0 + /** + * Unique identifier for this provider instance's view. + * Based on renderContext and a monotonically increasing counter to ensure uniqueness across multiple instances. + */ + public readonly viewId: string + + /** + * Stable identifier for persisted per-view state keys. + * Defaults to viewId until the webview reports its VS Code-persisted id. + */ + private viewStateId: string + + /** + * Local state buffer for this specific view instance. + * Used to isolate mode, apiConfiguration, and other fields from the shared ContextProxy singleton + * when running in parallel (multi-tab) mode. + */ + private viewLocalState: Partial = {} + public isViewLaunched = false public settingsImportedAt?: number public readonly latestAnnouncementId = "sep-2026-v3.82.0-gateway-portability-free-models" // v3.82.0 portable Zoo Gateway keys, free MiniMax-M3, and new models @@ -321,14 +352,17 @@ export class ClineProvider mdmService?: MdmService, ) { super() + // Initialize viewId based on renderContext and monotonically increasing instance identifier for uniqueness. + // activeInstances is used for visibility/iteration checks, so we keep tracking instances separately. + this.viewId = `${renderContext}-${ClineProvider.nextViewId++}` + this.viewStateId = this.viewId + ClineProvider.activeInstances.add(this) this.currentWorkspacePath = getWorkspacePath() this.pendingEditOperations = new PendingEditOperationStore( ClineProvider.PENDING_OPERATION_TIMEOUT_MS, (message) => this.log(message), ) - ClineProvider.activeInstances.add(this) - this.mdmService = mdmService void this.updateGlobalState("codebaseIndexModels", EMBEDDING_MODEL_PROFILES) @@ -359,6 +393,9 @@ export class ClineProvider await this.postStateToWebviewWithoutClineMessages() }) + // Load initial state from global state into viewLocalState buffer after dependencies used by getState are ready. + void this.loadViewState() + // Initialize MCP Hub through the singleton manager McpServerManager.getInstance(this.context, this) .then((hub) => { @@ -509,6 +546,217 @@ export class ClineProvider } } + /** + * Reads the registered viewStates map, returning a defensive copy. + * When fresh is set, the map is read directly from globalState (bypassing the + * ContextProxy cache) so serialized writes never observe a stale in-memory value. + */ + private getPersistedViewStates(options: { fresh?: boolean } = {}): Record { + const viewStates = options.fresh + ? this.context.globalState.get("viewStates") + : this.contextProxy.getValue("viewStates") + + if (!viewStates || typeof viewStates !== "object" || Array.isArray(viewStates)) { + return {} + } + + return { ...viewStates } + } + + /** + * Persists this view's non-secret selections through the serialized write queue. + * The write re-reads the map fresh and merges into the existing entry, removing the + * entry entirely when nothing persistable remains, so concurrent views cannot clobber it. + * The entry is keyed by the view id active when the change was made. Writes captured + * while the provider still holds its temporary (pre-launch) id persist under that id + * and are re-keyed to the stable view id when the webview registers one, so a change + * that lands before the launch message stays durable instead of being lost. + */ + private async savePersistedViewState(values: Partial): Promise { + // Capture the id at change time: a write belongs to the view that was active + // when the change was made, even if a newer id is registered while it is queued. + const viewStateId = this.viewStateId + const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + const states = this.getPersistedViewStates({ fresh: true }) + const current = states[viewStateId] ?? {} + const next: PersistedViewState = { ...current } + + if ("mode" in values) { + if (values.mode === undefined || values.mode === null) { + delete next.mode + } else { + next.mode = values.mode + } + } + + if ("currentApiConfigName" in values) { + if (values.currentApiConfigName === undefined || values.currentApiConfigName === null) { + delete next.currentApiConfigName + } else { + next.currentApiConfigName = values.currentApiConfigName + } + } + + if (!next.mode && !next.currentApiConfigName) { + delete states[viewStateId] + } else { + next.updatedAt = values.updatedAt ?? Date.now() + states[viewStateId] = next + } + + await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + }) + + ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) + await write + } + + /** + * Removes the given view's entry from the registered viewStates map. + * Runs through the serialized write queue to avoid racing concurrent view-state writes. + */ + private async clearPersistedViewState(viewStateId = this.viewStateId): Promise { + const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + const states = this.getPersistedViewStates({ fresh: true }) + delete states[viewStateId] + await this.contextProxy.setValue("viewStates", states) + }) + + ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) + await write + } + + /** + * Keeps only the most recently updated entries of the persisted view states map, + * bounded by MAX_PERSISTED_VIEW_STATES so the global key cannot grow unboundedly. + */ + private prunePersistedViewStates(states: Record): Record { + return Object.fromEntries( + Object.entries(states) + .sort(([, a], [, b]) => (b.updatedAt ?? 0) - (a.updatedAt ?? 0)) + .slice(0, ClineProvider.MAX_PERSISTED_VIEW_STATES), + ) + } + + /** + * Re-keys this provider's temporary pre-launch viewStates entry to the newly + * registered stable id so pre-launch writes become durable under the stable key + * instead of orphaning under a session-local temporary id. Only the provider's own + * temporary id is eligible: an entry under a previously registered stable id belongs + * to that webview's storage and is left alone. When the stable entry already exists + * it wins and the temporary entry is dropped, because temporary ids are session + * counters that can collide across window reloads. Runs through the serialized write + * queue like every other viewStates mutation. + */ + private async rekeyPersistedViewStateEntry(nextViewStateId: string): Promise { + const previousViewStateId = this.viewId + + const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + const states = this.getPersistedViewStates({ fresh: true }) + const previous = states[previousViewStateId] + + if (!previous) { + return + } + + delete states[previousViewStateId] + + if (!states[nextViewStateId]) { + states[nextViewStateId] = previous + } + + await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + }) + + ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) + await write + } + + /** + * Registers this provider's stable view identifier and loads any persisted selections it owns. + * The identifier is sanitized so it remains a safe object key in the shared viewStates map. + */ + public async setViewStateId(viewStateId: string | undefined): Promise { + const normalizedViewStateId = viewStateId?.trim().replace(/[^A-Za-z0-9_-]/g, "_") + + if ( + !normalizedViewStateId || + normalizedViewStateId === this.viewStateId || + // Reject "__proto__": writing states["__proto__"] would go through the + // Object.prototype setter and be silently dropped by the later spread. + normalizedViewStateId === "__proto__" + ) { + return + } + + this.viewStateId = normalizedViewStateId + + // Re-key any durable entry written under the temporary pre-launch id before + // loading, so the load sees the view's own pre-registration selections. + await this.rekeyPersistedViewStateEntry(this.viewStateId) + + await this.loadViewState() + } + + /** + * Loads non-secret persisted selections from the registered viewStates map. + * Missing entries are intentionally left unset so getState() falls back to shared ContextProxy values. + */ + private async loadViewState(): Promise { + // Capture the id this load is for: a newer id registered while an async + // profile lookup is in flight must not be overwritten by this stale load. + const loadedForViewId = this.viewStateId + try { + const persisted = this.getPersistedViewStates()[loadedForViewId] + const loadedState: Partial = {} + + 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)}`, + ) + } + } + + if (this.viewStateId !== loadedForViewId) { + this.log(`[loadViewState] Discarding stale state for superseded view id ${loadedForViewId}`) + return + } + + 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)}`, + ) + } + } + + /** + * Saves a single view-local state value. The in-memory buffer is always updated; the + * non-secret subset (mode, currentApiConfigName) is persisted durably under the view + * id active when the change was made, re-keyed to the stable id on registration. + */ + public async saveViewState( + key: K, + value: ViewLocalStateValues[K] | undefined, + ): Promise { + await this._saveViewLocalStateFromMutation({ [key]: value } as ViewLocalStateValues) + + this.log(`[saveViewState] Saved ${String(key)} for viewId ${this.viewId}`) + } + /** * Override EventEmitter's on method to match TaskProviderLike interface */ @@ -1265,7 +1513,10 @@ export class ClineProvider historyItem.mode = defaultModeSlug } - await this.updateGlobalState("mode", historyItem.mode) + // Persist the restored mode through this view's per-view pin rather than the + // shared global: a global write would leak the restored mode into other views + // in parallel mode, and a buffer-only write would be lost after a reload. + await this.saveViewState("mode", historyItem.mode) // Load the saved API config for the restored mode if it exists. // Skip mode-based profile activation if historyItem.apiConfigName exists, @@ -1478,11 +1729,20 @@ export class ClineProvider return } - try { - await this.view?.webview.postMessage(message) - } catch { - // View disposed, drop message silently + const webview = this.view?.webview + if (!webview) { + return } + + // Dispatch without awaiting the renderer ack: VS Code settles postMessage only when the + // webview page acknowledges the message, and a page reload or view dispose in flight + // orphans that promise forever. Awaiting it could wedge every caller on the task critical + // path (e.g. the trailing postStateToWebview in handleModeSwitchUnlocked gates the next + // turn after a mode switch). Message ordering is enforced by the message seq, not the ack. + // Promise.resolve() normalizes non-promise returns (e.g. test doubles) before the catch. + void Promise.resolve(webview.postMessage(message)).catch(() => { + // Swallow: postMessage rejects when the webview is disposed in flight. + }) } public requestWebviewThemeFixture(timeoutMs = 5_000): Promise { @@ -3200,6 +3460,7 @@ export class ClineProvider public async setValue(key: K, value: RooCodeSettings[K]) { await this.contextProxy.setValue(key, value) + await this._saveViewLocalStateFromMutation({ [key]: value }) } public getValue(key: K) { @@ -3207,11 +3468,115 @@ export class ClineProvider } public getValues() { - return this.contextProxy.getValues() + return { ...this.contextProxy.getValues(), ...this.viewLocalState } } public async setValues(values: RooCodeSettings) { - await this.contextProxy.setValues(values) + const sanitizedValues = { ...values } + + if ( + typeof sanitizedValues.mode === "string" && + !getModeBySlug(sanitizedValues.mode, await this.customModesManager.getCustomModes()) + ) { + // An unknown mode (e.g. from an API payload) must not be persisted: a new Task + // would read it from getState() and persist it into task history. + this.log(`[ClineProvider#setValues] Ignoring unknown mode "${sanitizedValues.mode}"`) + delete sanitizedValues.mode + } + + await this.contextProxy.setValues(sanitizedValues) + await this._saveViewLocalStateFromMutation(sanitizedValues) + } + + /** + * Persists the view-local subset of a ContextProxy mutation, then updates the in-memory + * viewLocalState buffer. Persistence is awaited first so a failed durable write cannot + * leave the local cache ahead of the persisted state. + */ + private async _saveViewLocalStateFromMutation( + values: Partial & Partial, + ): Promise { + await this._persistViewLocalStateFromMutation(values) + this._updateViewLocalStateFromMutation(values) + } + + /** + * Update or invalidate viewLocalState when ContextProxy is mutated via setValues, setValue, + * profile upsert/activation/deletion, or resetState. This ensures the local cache stays in + * sync with global state changes that would otherwise be invisible behind mergedStateValues. + */ + private _updateViewLocalStateFromMutation(values: Partial & Partial): void { + if ("mode" in values) { + const val = values.mode + if (val === undefined || val === null) { + delete this.viewLocalState.mode + } else { + 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 + } + } + + if ("apiConfiguration" in values) { + const val = values.apiConfiguration + if (val === undefined || val === null) { + delete this.viewLocalState.apiConfiguration + } else { + this.viewLocalState.apiConfiguration = val + } + } else if (PROVIDER_SETTINGS_KEYS.some((key) => key in values)) { + const providerSettingsUpdate = PROVIDER_SETTINGS_KEYS.reduce((acc, key) => { + if (key in values) { + return { ...acc, [key]: values[key as keyof RooCodeSettings] } + } + + return acc + }, {} as ProviderSettings) + + this.viewLocalState.apiConfiguration = + "apiProvider" in providerSettingsUpdate + ? providerSettingsUpdate + : { + ...(this.viewLocalState.apiConfiguration ?? {}), + ...providerSettingsUpdate, + } + } + } + + /** + * Writes the durably persisted subset of a mutation (mode and currentApiConfigName) + * into the registered viewStates map for this view. + */ + private async _persistViewLocalStateFromMutation( + values: Partial & Partial, + ): Promise { + const persistedValues: Partial = {} + + if ("mode" in values) { + persistedValues.mode = values.mode as PersistedViewState["mode"] + } + + if ("currentApiConfigName" in values) { + persistedValues.currentApiConfigName = values.currentApiConfigName + } + + if ("mode" in persistedValues || "currentApiConfigName" in persistedValues) { + await this.savePersistedViewState(persistedValues) + } + } + + /** + * Clear view-local state cache so that getState() falls back to ContextProxy defaults. + */ + private _clearViewLocalState(): void { + this.viewLocalState = {} } // dev @@ -3240,6 +3605,14 @@ export class ClineProvider } await this.contextProxy.resetAllState() + + // Clear view-local state cache so getState() falls back to ContextProxy defaults. + this._clearViewLocalState() + + // Clear this view's persisted entry too, so the reset selections are not + // re-applied from the durable viewStates pin after a reload. + await this.clearPersistedViewState() + await this.providerSettingsManager.resetAllConfigs() await this.customModesManager.resetCustomModes() await this.removeClineFromStack() diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 84b80cf945..b1672084f3 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -12,6 +12,7 @@ import { type ClineMessage, type ExtensionMessage, type ExtensionState, + type RooCodeSettings, type WebviewMessage, ORGANIZATION_ALLOW_ALL, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, @@ -27,6 +28,7 @@ import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" import { Task, TaskOptions } from "../../task/Task" import { safeWriteJson } from "../../../utils/safeWriteJson" +import { t } from "../../../i18n" import { ClineProvider } from "../ClineProvider" import { webviewMessageHandler } from "../webviewMessageHandler" @@ -592,7 +594,7 @@ describe("ClineProvider", () => { }) test("does not reload full model details when the LM Studio model is already loaded", async () => { - vi.mocked(hasLoadedFullDetails).mockReturnValue(true) + vi.mocked(hasLoadedFullDetails).mockReturnValueOnce(true) await provider.performPreparationTasks({ apiConfiguration: { @@ -785,6 +787,26 @@ describe("ClineProvider", () => { await expect(provider.postMessageToWebview(message)).resolves.toBeUndefined() }) + test("postMessageToWebview does not await the webview ack", async () => { + await provider.resolveWebviewView(mockWebviewView) + + let releaseAck!: () => void + const ack = new Promise((resolve) => { + releaseAck = resolve + }) + mockPostMessage.mockImplementationOnce(() => ack) + + const message: ExtensionMessage = { type: "action", action: "chatButtonClicked" } + + // The caller must not wait for the renderer ack: a webview page remounted or disposed + // while the post is in flight never acknowledges it, and awaiting that promise would + // wedge every caller on the task critical path. + await provider.postMessageToWebview(message) + + expect(mockPostMessage).toHaveBeenCalledWith(message) + releaseAck() + }) + describe("theme fixture probes", () => { const fixture = { themeId: "Default Dark Modern", @@ -988,6 +1010,541 @@ describe("ClineProvider", () => { expect(state.taskHistory).toEqual([historyItem]) }) + describe("viewId uniqueness", () => { + it("should assign unique viewId to each instance", async () => { + const provider1 = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + ) + const provider2 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + + // Each instance should have a unique viewId + expect(provider1.viewId).toBeDefined() + expect(provider2.viewId).toBeDefined() + expect(provider1.viewId).not.toBe(provider2.viewId) + + await provider1.dispose() + await provider2.dispose() + }) + + it("should have viewId in correct format: {renderContext}-{instanceCount}", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + expect(provider.viewId).toMatch(/^sidebar-\d+$/) + + await provider.dispose() + }) + + it("should increment instance count for each new instance", async () => { + const provider1 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + const provider2 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + + // First editor instance should be "editor-0" (or next available) + // Second editor instance should have a different number + const num1 = parseInt(provider1.viewId.split("-")[1]!) + const num2 = parseInt(provider2.viewId.split("-")[1]!) + + expect(num2).toBeGreaterThan(num1) + + await provider1.dispose() + await provider2.dispose() + }) + }) + + describe("saveViewState", () => { + it("should update viewLocalState and persist mode through registered viewStates", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + const contextProxySpy = vi.spyOn(provider.contextProxy, "setValue") + await provider["setViewStateId"]("stable-sidebar-view") + + await provider.saveViewState("mode", "architect") + + expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "stable-sidebar-view": { mode: "architect" }, + }) + expect(contextProxySpy).toHaveBeenCalledWith( + "viewStates", + expect.objectContaining({ + "stable-sidebar-view": expect.objectContaining({ + mode: "architect", + updatedAt: expect.any(Number), + }), + }), + ) + expect(contextProxySpy).not.toHaveBeenCalledWith("__view_state_stable-sidebar-view_mode", expect.anything()) + + await provider.dispose() + }) + + it("should update viewLocalState and persist currentApiConfigName through registered viewStates", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("currentApiConfigName", "my-profile") + + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "stable-sidebar-view": { currentApiConfigName: "my-profile" }, + }) + + await provider.dispose() + }) + + it("should update viewLocalState for apiConfiguration without persisting provider settings or secrets", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + const testApiConfig = { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "claude-3.5-sonnet", + openRouterApiKey: "secret-key", + } + + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("apiConfiguration", testApiConfig) + + expect(provider["viewLocalState"].apiConfiguration).toEqual(testApiConfig) + expect(provider.contextProxy.getValue("viewStates")).toBeUndefined() + + await provider.dispose() + }) + + it("should clear local override when saveViewState receives undefined", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider.saveViewState("mode", "architect") + expect(provider["viewLocalState"].mode).toBe("architect") + + await provider.saveViewState("mode", undefined) + + expect(Object.prototype.hasOwnProperty.call(provider["viewLocalState"], "mode")).toBe(false) + + await provider.dispose() + }) + + it("should clear the currentApiConfigName override when saveViewState receives undefined", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider.saveViewState("currentApiConfigName", "my-profile") + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + + await provider.saveViewState("currentApiConfigName", undefined) + + expect(Object.prototype.hasOwnProperty.call(provider["viewLocalState"], "currentApiConfigName")).toBe(false) + + await provider.dispose() + }) + + it("should not update viewLocalState when durable view-state persistence fails", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const providerAccess = provider as unknown as { + setViewStateId: (viewStateId: string) => Promise + saveViewState: (key: keyof ExtensionState, value: unknown) => Promise + viewLocalState: Partial + } + vi.spyOn(provider.contextProxy, "setValue").mockRejectedValueOnce(new Error("persist failed")) + + await providerAccess.setViewStateId("stable-sidebar-view") + + await expect(providerAccess.saveViewState("mode", "architect")).rejects.toThrow("persist failed") + expect(providerAccess.viewLocalState).not.toHaveProperty("mode") + expect(provider.contextProxy.getValue("viewStates")).toBeUndefined() + + await provider.dispose() + }) + + it("should merge concurrent persisted updates from separate provider instances without lost viewStates", async () => { + const provider1 = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + ) + const provider2 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + + await provider1["setViewStateId"]("stable-sidebar-view") + await provider2["setViewStateId"]("stable-editor-view") + + await Promise.all([ + provider1.saveViewState("mode", "architect"), + provider2.saveViewState("currentApiConfigName", "editor-profile"), + ]) + + expect(mockContext.globalState.get("viewStates")).toMatchObject({ + "stable-sidebar-view": { mode: "architect" }, + "stable-editor-view": { currentApiConfigName: "editor-profile" }, + }) + + await provider1.dispose() + await provider2.dispose() + }) + }) + + describe("loadViewState", () => { + it("should keep viewLocalState empty when no stable per-view values exist", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await vi.waitFor(() => { + expect(provider["viewLocalState"]).toEqual({}) + }) + + const state = await provider.getState() + // No per-view entry exists and the proxy's global-state cache is empty + // (initialize() is never called in this fixture; only "taskHistory" passes + // through to the context store), so getState() falls back to the shared + // defaults: mode "code" (defaultModeSlug) and currentApiConfigName "default". + expect(state.mode).toBe("code") + expect(state.currentApiConfigName).toBe("default") + + await provider.dispose() + }) + + it("should log and keep existing viewLocalState when loadViewState fails", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + + provider["viewLocalState"] = { mode: "architect" } + vi.spyOn(provider.contextProxy, "getValue").mockImplementation(() => { + throw new Error("load failed") + }) + + await provider["loadViewState"]() + + expect(provider["viewLocalState"].mode).toBe("architect") + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Error loading state")) + + await provider.dispose() + }) + }) + + describe("persisted view state pruning", () => { + it("should keep the newest 50 persisted view states", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const states = Object.fromEntries( + Array.from({ length: 55 }, (_, index) => [ + `view-${index}`, + { mode: `mode-${index}`, updatedAt: index }, + ]), + ) + + const pruned = provider["prunePersistedViewStates"](states) + + expect(Object.keys(pruned)).toHaveLength(50) + expect(pruned["view-54"]).toBeDefined() + expect(pruned["view-5"]).toBeDefined() + expect(pruned["view-4"]).toBeUndefined() + + await provider.dispose() + }) + }) + + describe("setViewStateId", () => { + it('should ignore "__proto__" and keep the temporary viewId', async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider["setViewStateId"]("__proto__") + + // "__proto__" is rejected before assignment so a per-view entry can never be + // keyed through the Object.prototype setter: the temporary id stays active and + // nothing is persisted under the reserved name. + expect(provider["viewStateId"]).toBe(provider.viewId) + expect(mockContext.globalState.get("viewStates")).toBeUndefined() + expect(provider["viewLocalState"]).toEqual({}) + + await provider.dispose() + }) + }) + + describe("view state persistence edge cases", () => { + it("should read viewStates from the ContextProxy cache when not fresh", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + await provider.contextProxy.setValue("viewStates", { "stable-sidebar-view": { mode: "architect" } }) + expect(provider["getPersistedViewStates"]()).toEqual({ "stable-sidebar-view": { mode: "architect" } }) + await provider.dispose() + }) + + it("should treat a corrupted non-object viewStates value as an empty map", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + // A string in storage is corrupt: the fresh-read guard must not spread it. + mockContext.globalState.update("viewStates", "corrupted-storage-value") + expect(provider["getPersistedViewStates"]({ fresh: true })).toEqual({}) + await provider.dispose() + }) + + it("should merge saved fields, drop cleared fields and delete emptied entries", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + const save = provider.saveViewState.bind(provider) as (key: string, value: unknown) => Promise + const states = () => mockContext.globalState.get>("viewStates") ?? {} + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("mode", "architect") + await provider.saveViewState("currentApiConfigName", "profile-a") + const merged = states()["stable-sidebar-view"] + expect(merged).toMatchObject({ mode: "architect", currentApiConfigName: "profile-a" }) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Saved mode for viewId")) + await save("mode", undefined) + expect(states()["stable-sidebar-view"]).toStrictEqual({ + currentApiConfigName: "profile-a", + updatedAt: expect.any(Number), + }) + await save("mode", null) + expect(states()["stable-sidebar-view"]).not.toHaveProperty("mode") + await provider.saveViewState("mode", "architect") + await save("currentApiConfigName", undefined) + expect(states()["stable-sidebar-view"]).toStrictEqual({ + mode: "architect", + updatedAt: expect.any(Number), + }) + await provider.saveViewState("currentApiConfigName", "profile-c") + await provider.saveViewState("mode", "architect") + expect(states()["stable-sidebar-view"]).toMatchObject({ + mode: "architect", + currentApiConfigName: "profile-c", + }) + await save("currentApiConfigName", null) + expect(states()["stable-sidebar-view"]).not.toHaveProperty("currentApiConfigName") + await save("mode", null) + expect(states()["stable-sidebar-view"]).toBeUndefined() + expect(provider["viewLocalState"]).toStrictEqual({}) // buffer ends fully cleared + await provider.dispose() + }) + + it("should rekey a pre-launch entry under the temporary id to the registered stable id", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + // Seed storage directly (bypassing the ContextProxy cache) so only the fresh read sees it. + mockContext.globalState.update("viewStates", { [provider.viewId]: { mode: "architect", updatedAt: 1 } }) + await provider["setViewStateId"]("stable-sidebar-view") + expect(mockContext.globalState.get("viewStates")).toEqual({ + "stable-sidebar-view": { mode: "architect", updatedAt: 1 }, + }) + await provider.dispose() + }) + + it("should keep the stable entry and drop the temporary entry when both exist", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + mockContext.globalState.update("viewStates", { + [provider.viewId]: { mode: "temp-mode", updatedAt: 1 }, + "stable-sidebar-view": { mode: "stable-mode", updatedAt: 5 }, + }) + await provider["setViewStateId"]("stable-sidebar-view") + expect(mockContext.globalState.get("viewStates")).toEqual({ + "stable-sidebar-view": { mode: "stable-mode", updatedAt: 5 }, + }) + await provider.dispose() + }) + + it("should clear only this view's entry without clobbering an entry only storage knows about", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + await provider["setViewStateId"]("stable-sidebar-view") + // The cache only knows this view's entry; storage gains an extra view directly. + await provider.contextProxy.setValue("viewStates", { "stable-sidebar-view": { mode: "architect" } }) + mockContext.globalState.update("viewStates", { + "stable-sidebar-view": { mode: "architect" }, + "stable-editor-view": { mode: "code" }, + }) + await provider["clearPersistedViewState"]() + expect(mockContext.globalState.get("viewStates")).toEqual({ "stable-editor-view": { mode: "code" } }) + await provider.dispose() + }) + + it("should prune by updatedAt regardless of insertion order", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const states = Object.fromEntries( + Array.from({ length: 55 }, (_, index) => [ + `view-${index}`, + { mode: `mode-${index}`, updatedAt: (index * 7) % 55 }, + ]), + ) + const pruned = provider["prunePersistedViewStates"](states) + expect(Object.keys(pruned)).toHaveLength(50) + // view-1/view-54 survive the true newest-50 selection; view-8 (updatedAt 1) does not. + expect(pruned["view-1"]).toBeDefined() + expect(pruned["view-54"]).toBeDefined() + expect(pruned["view-8"]).toBeUndefined() + await provider.dispose() + }) + + it("should sanitize, reject blank and undefined ids, and no-op on the active id", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + await provider["setViewStateId"]("a b/c") + expect(provider["viewStateId"]).toBe("a_b_c") + await provider["setViewStateId"](undefined) + await provider["setViewStateId"](" ") + expect(provider["viewStateId"]).toBe("a_b_c") + logSpy.mockClear() + await provider["setViewStateId"]("a_b_c") + expect(logSpy).not.toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId")) + await provider.dispose() + }) + + it("should load persisted mode, profile name and resolved profile into viewLocalState", async () => { + const writer = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + await writer["setViewStateId"]("shared-view") + await writer.saveViewState("mode", "architect") + await writer.saveViewState("currentApiConfigName", "my-profile") + + const provider = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + const getProfileSpy = vi.fn().mockResolvedValue({ + name: "my-profile", + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-x", + }) + // @ts-ignore - Replace providerSettingsManager with a test double for the profile lookup. + provider.providerSettingsManager = { getProfile: getProfileSpy } + await provider.contextProxy.setValue( + "viewStates", + mockContext.globalState.get("viewStates"), + ) + await provider["setViewStateId"]("shared-view") + expect(provider["viewLocalState"]).toEqual({ + mode: "architect", + currentApiConfigName: "my-profile", + apiConfiguration: { apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }, + }) + expect(getProfileSpy).toHaveBeenCalledWith({ name: "my-profile" }) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId")) + await writer.dispose() + await provider.dispose() + }) + + it("should log a successful empty load when no persisted entry exists", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + await provider["setViewStateId"]("stable-sidebar-view") + expect(provider["viewLocalState"]).toEqual({}) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId")) + await provider.dispose() + }) + + it("should keep the persisted profile name and log when the profile lookup fails", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + // @ts-ignore - Replace providerSettingsManager with a failing test double. + provider.providerSettingsManager = { getProfile: vi.fn().mockRejectedValue(new Error("profile missing")) } + await provider.saveViewState("currentApiConfigName", "my-profile") + await provider["setViewStateId"]("stable-sidebar-view") + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Unable to resolve API profile 'my-profile'")) + await provider.dispose() + }) + + it("should discard a stale load when the viewStateId changes during the profile lookup", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + // @ts-ignore - Replace providerSettingsManager with a test double that registers a newer id. + provider.providerSettingsManager = { + getProfile: vi.fn().mockImplementation(() => { + provider["viewStateId"] = "superseded-view" + return Promise.resolve({ name: "my-profile", apiProvider: providerIdentifiers.openrouter }) + }), + } + await provider.saveViewState("currentApiConfigName", "my-profile") + await provider["setViewStateId"]("stable-sidebar-view") + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + const staleMsg = expect.stringContaining("Discarding stale state for superseded view id") + expect(logSpy).toHaveBeenCalledWith(staleMsg) + await provider.dispose() + }) + + it("should persist known modes, ignore unknown modes and pass through non-string modes", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + // @ts-ignore - Replace customModesManager with a test double (no custom modes). + provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + // The file-level modes mock resolves every slug to a mode; narrow it to the slugs under test. + const modesModule = vi.mocked(await import("../../../shared/modes")) + const originalMode = modesModule.getModeBySlug("code") + modesModule.getModeBySlug.mockImplementation(((slug: string) => + slug === "refactor" ? { slug } : undefined) as typeof modesModule.getModeBySlug) + try { + await provider.setValues({ mode: "refactor" }) + expect(mockContext.globalState.get("mode")).toBe("refactor") + expect(provider["viewLocalState"].mode).toBe("refactor") + await provider.setValues({ mode: "bogus-mode" }) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Ignoring unknown mode "bogus-mode"')) + expect(mockContext.globalState.get("mode")).toBe("refactor") + expect(provider["viewLocalState"].mode).toBe("refactor") + // A non-string mode bypasses the slug validation (double assertion: the type excludes non-strings). + await provider.setValues({ mode: 42 } as unknown as RooCodeSettings) + expect(mockContext.globalState.get("mode")).toBe(42) + expect(provider["viewLocalState"].mode).toBe(42) + } finally { + modesModule.getModeBySlug.mockReturnValue(originalMode) + } + await provider.dispose() + }) + + it("should apply setValue mutations to global state and keep or clear the right buffer fields", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const apiConfiguration = { apiProvider: providerIdentifiers.openrouter } + await provider.saveViewState("mode", "architect") + await provider.saveViewState("currentApiConfigName", "my-profile") + await provider.saveViewState("apiConfiguration", apiConfiguration) + // A mutation of an unrelated key reaches global state without dropping buffered fields. + await provider.setValue("writeDelayMs", 500) + expect(mockContext.globalState.get("writeDelayMs")).toBe(500) + expect(provider.getValues().writeDelayMs).toBe(500) + expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + expect(provider["viewLocalState"].apiConfiguration).toBe(apiConfiguration) + await provider.setValue("mode", undefined) + expect(provider["viewLocalState"]).not.toHaveProperty("mode") + await provider.setValue("currentApiConfigName", undefined) + expect(provider["viewLocalState"]).not.toHaveProperty("currentApiConfigName") + await provider.dispose() + }) + + it("should build the buffered apiConfiguration from provider settings keys", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + provider["viewLocalState"] = { apiConfiguration: { openRouterApiKey: "key-1" } } + await provider.setValues({ apiProvider: providerIdentifiers.openrouter }) + expect(provider["viewLocalState"].apiConfiguration).toStrictEqual({ + apiProvider: providerIdentifiers.openrouter, + }) + await provider.setValues({ openRouterModelId: "model-x" }) + expect(provider["viewLocalState"].apiConfiguration).toEqual({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-x", + }) + await provider.dispose() + }) + + it("should remove the buffered apiConfiguration when it is cleared", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const save = provider.saveViewState.bind(provider) as (key: string, value: unknown) => Promise + await provider.saveViewState("apiConfiguration", { apiProvider: providerIdentifiers.openrouter }) + await provider.saveViewState("apiConfiguration", undefined) + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + await provider.saveViewState("apiConfiguration", { apiProvider: providerIdentifiers.openrouter }) + await save("apiConfiguration", null) + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + await provider.dispose() + }) + + it("should clear viewLocalState and the persisted entry when resetting state", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + // @ts-ignore - Replace customModesManager with a test double (the real reset writes to disk). + provider.customModesManager = { resetCustomModes: vi.fn().mockResolvedValue(undefined), dispose: vi.fn() } + // The modal answer is a string label; the last-typed vscode overload expects a MessageItem. + vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( + t("common:answers.yes") as unknown as vscode.MessageItem, + ) + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("mode", "architect") + await provider.resetState() + expect(provider["viewLocalState"]).toEqual({}) + expect(mockContext.globalState.get("viewStates")).toEqual({}) + await provider.dispose() + }) + }) + describe("postStateToWebviewThrottled", () => { beforeEach(() => { vi.useFakeTimers() @@ -2497,8 +3054,10 @@ describe("ClineProvider", () => { expect(mockCustomModesManager.getCustomModes).toHaveBeenCalled() expect(getModeBySlug).toHaveBeenCalledWith("non-existent-mode", expect.any(Array)) - // Verify fallback to default mode - expect(mockContext.globalState.update).toHaveBeenCalledWith("mode", "code") + // Verify fallback to default mode, view-locally: history restore no longer + // writes the shared global mode + expect(provider["viewLocalState"].mode).toBe("code") + expect(mockContext.globalState.update).not.toHaveBeenCalledWith("mode", "code") expect(logSpy).toHaveBeenCalledWith( "Mode 'non-existent-mode' from history no longer exists. Falling back to default mode 'code'.", ) @@ -2570,8 +3129,9 @@ describe("ClineProvider", () => { expect(mockCustomModesManager.getCustomModes).toHaveBeenCalled() expect(getModeBySlug).toHaveBeenCalledWith("custom-mode", expect.any(Array)) - // Verify mode was preserved - expect(mockContext.globalState.update).toHaveBeenCalledWith("mode", "custom-mode") + // Verify mode was preserved view-locally (no shared global mode write) + expect(provider["viewLocalState"].mode).toBe("custom-mode") + expect(mockContext.globalState.update).not.toHaveBeenCalledWith("mode", "custom-mode") expect(logSpy).not.toHaveBeenCalledWith(expect.stringContaining("no longer exists")) // Verify history item mode was not changed @@ -2618,8 +3178,9 @@ describe("ClineProvider", () => { // Initialize with history item await provider.createTaskWithHistoryItem(historyItem) - // Verify mode was preserved - expect(mockContext.globalState.update).toHaveBeenCalledWith("mode", "architect") + // Verify mode was preserved view-locally (no shared global mode write) + expect(provider["viewLocalState"].mode).toBe("architect") + expect(mockContext.globalState.update).not.toHaveBeenCalledWith("mode", "architect") // Verify history item mode was not changed expect(historyItem.mode).toBe("architect") diff --git a/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts b/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts index fedfa13030..414c368aad 100644 --- a/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts @@ -472,14 +472,15 @@ describe("ClineProvider - Sticky Mode", () => { mode: "architect", // Saved mode } - // Mock updateGlobalState to track mode updates - const updateGlobalStateSpy = vi.spyOn(provider as any, "updateGlobalState").mockResolvedValue(undefined) + // Register a stable view id so the durable per-view write is persisted + await provider["setViewStateId"]("stable-test-view") // Initialize task with history item await provider.createTaskWithHistoryItem(historyItem) - // Verify mode was restored via updateGlobalState - expect(updateGlobalStateSpy).toHaveBeenCalledWith("mode", "architect") + // Verify mode was restored into the view-local pin (no shared global write) + expect(provider["viewLocalState"].mode).toBe("architect") + expect(mockContext.globalState.update).not.toHaveBeenCalledWith("mode", "architect") }) it("should use current mode if history item has no saved mode", async () => { @@ -760,9 +761,11 @@ describe("ClineProvider - Sticky Mode", () => { // Restore the task from history await provider.createTaskWithHistoryItem(historyItem) - // Verify that the mode was restored + // Verify that the mode was restored into this view's durable pin. The + // getState() merge of hydrated per-view values lands with the F1b follow-up. + expect(provider["viewLocalState"].mode).toBe("architect") + const state = await provider.getState() - expect(state.mode).toBe("architect") // Verify that the API configuration was also restored expect(state.currentApiConfigName).toBe("architect-config") diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 544886c2d7..335ce8aeb2 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1036,7 +1036,7 @@ }, "core/webview/__tests__/ClineProvider.sticky-mode.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 37 + "count": 36 } }, "core/webview/__tests__/ClineProvider.sticky-profile.spec.ts": { From 8c139755337d00ebbecb86b0f27267be3958e871 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 00:39:16 +0800 Subject: [PATCH 09/18] fix(provider): track in-flight view-state mutations per field and exclude viewStates from settings transfer --- src/core/config/ContextProxy.ts | 3 + .../config/__tests__/ContextProxy.spec.ts | 16 +++ .../config/__tests__/importExport.spec.ts | 45 +++++++++ src/core/config/importExport.ts | 8 ++ src/core/webview/ClineProvider.ts | 34 ++++++- .../webview/__tests__/ClineProvider.spec.ts | 99 +++++++++++++++++++ 6 files changed, 204 insertions(+), 1 deletion(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 97d4104afc..38c2c9ce99 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -36,6 +36,9 @@ const globalSettingsExportSchema = globalSettingsSchema.omit({ taskHistory: true, listApiConfigMeta: true, currentApiConfigName: true, + // Per-view selection state is machine-local: it keeps flowing through the + // normal runtime and pruning paths but must not transfer between settings. + viewStates: true, }) export class ContextProxy { diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 2319a6b1a5..d389d31337 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -721,4 +721,20 @@ Output only the summary of the conversation so far, without any additional comme expect(customSupportPromptsUpdateCalls.length).toBe(0) }) }) + + describe("export", () => { + it("should exclude viewStates from the exported settings", async () => { + await proxy.setValue("viewStates", { + "stable-sidebar-view": { mode: "architect", currentApiConfigName: "profile-a", updatedAt: 1 }, + }) + await proxy.setValue("customInstructions", "global instructions") + + const exported = await proxy.export() + + // Per-view selection state is machine-local and must never transfer + // between settings, while ordinary global settings keep round-tripping. + expect(exported).not.toHaveProperty("viewStates") + expect(exported?.customInstructions).toBe("global instructions") + }) + }) }) diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index 6a99adaa7c..c15c103be7 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -332,6 +332,51 @@ describe("importExport", () => { ]) }) + it("should not apply imported viewStates to the context proxy", async () => { + const fileContent = JSON.stringify({ + providerProfiles: { + currentApiConfigName: "test", + apiConfigs: { + test: { apiProvider: providerIdentifiers.openai, apiKey: "test-key", id: "test-id" }, + }, + }, + globalSettings: { + mode: "code", + viewStates: { + "stable-sidebar-view": { + mode: "architect", + currentApiConfigName: "profile-a", + updatedAt: 1, + }, + }, + }, + }) + + ;(fs.readFile as Mock).mockResolvedValue(fileContent) + + mockProviderSettingsManager.export.mockResolvedValue({ + currentApiConfigName: "default", + apiConfigs: { default: { apiProvider: providerIdentifiers.anthropic, id: "default-id" } }, + }) + + mockProviderSettingsManager.listConfig.mockResolvedValue([ + { name: "test", id: "test-id", apiProvider: providerIdentifiers.openai }, + { name: "default", id: "default-id", apiProvider: providerIdentifiers.anthropic }, + ]) + + const result = await importSettingsFromPath("/mock/path/settings.json", { + providerSettingsManager: mockProviderSettingsManager, + contextProxy: mockContextProxy, + customModesManager: mockCustomModesManager, + }) + + expect(result.success).toBe(true) + // Per-view selection state is machine-local: importing settings must not + // apply another machine's view pins, while other settings round-trip. + expect(mockContextProxy.setValues).toHaveBeenCalledWith({ mode: "code" }) + expect(result).not.toHaveProperty("globalSettings.viewStates") + }) + it("should return success: false when file content is invalid", async () => { ;(vscode.window.showOpenDialog as Mock).mockResolvedValue([{ fsPath: "/mock/path/settings.json" }]) diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index 7b3b5aa231..399d5c1507 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -97,6 +97,14 @@ function sanitizeGlobalSettings(rawGlobalSettings: unknown): { for (const [key, rawValue] of Object.entries(rawGlobalSettings)) { const path = `globalSettings.${key}` + + // Per-view selection state is machine-local: it round-trips through the + // normal runtime and pruning paths, but importing it would pin selections + // from another machine's views on this one. + if (key === "viewStates") { + continue + } + const schema = globalSettingsShape[key as keyof GlobalSettings] if (!schema) { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index a4583e6bc6..0130266b06 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -701,6 +701,8 @@ export class ClineProvider /** * Loads non-secret persisted selections from the registered viewStates map. * Missing entries are intentionally left unset so getState() falls back to shared ContextProxy values. + * Fields mutated while the async profile lookup is in flight are reapplied on top of the + * loaded state, field by field, so in-flight user selections are not clobbered by the load. */ private async loadViewState(): Promise { // Capture the id this load is for: a newer id registered while an async @@ -710,6 +712,11 @@ export class ClineProvider const persisted = this.getPersistedViewStates()[loadedForViewId] const loadedState: Partial = {} + // Snapshot the in-memory buffer before the async profile lookup. The + // mutation paths update viewLocalState in place, so a shallow copy is + // what makes fields mutated during the load window observable below. + const preLoadBuffer = { ...this.viewLocalState } + if (persisted?.mode) { loadedState.mode = persisted.mode as Mode } @@ -734,7 +741,32 @@ export class ClineProvider return } - this.viewLocalState = loadedState + // Reapply only the fields mutated while the load was in flight: untouched + // fields keep the persisted values authoritative, and the pre-load buffer is + // never merged wholesale so stale temporary-id state or a cleared field cannot + // override the stable persisted state. + const postLoadBuffer = this.viewLocalState + const mergedState: Partial = { ...loadedState } + + if (postLoadBuffer.mode !== preLoadBuffer.mode && postLoadBuffer.mode !== undefined) { + mergedState.mode = postLoadBuffer.mode + } + + if ( + postLoadBuffer.currentApiConfigName !== preLoadBuffer.currentApiConfigName && + postLoadBuffer.currentApiConfigName !== undefined + ) { + mergedState.currentApiConfigName = postLoadBuffer.currentApiConfigName + } + + if ( + postLoadBuffer.apiConfiguration !== preLoadBuffer.apiConfiguration && + postLoadBuffer.apiConfiguration !== undefined + ) { + mergedState.apiConfiguration = postLoadBuffer.apiConfiguration + } + + this.viewLocalState = mergedState this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`) } catch (error) { this.log( diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index b1672084f3..8f1daff948 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1452,6 +1452,105 @@ describe("ClineProvider", () => { await provider.dispose() }) + it("should reapply fields mutated while the load is in flight and keep persisted values for untouched fields", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + let resolveProfile: (value: { + name: string + apiProvider: string + openRouterModelId: string + }) => void = () => {} + // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. + provider.providerSettingsManager = { + getProfile: vi + .fn() + .mockImplementation( + () => + new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( + (resolve) => (resolveProfile = resolve), + ), + ), + } + await provider.saveViewState("currentApiConfigName", "cfg-a") + const load = provider["setViewStateId"]("stable-sidebar-view") + + // Selections made while the profile lookup is in flight must survive the load. + await provider.saveViewState("mode", "architect") + await provider.saveViewState("apiConfiguration", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-y", + }) + + resolveProfile({ name: "cfg-a", apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }) + await load + + // Dirty fields win over the loaded state; the untouched field keeps the persisted value. + expect(provider["viewLocalState"]).toEqual({ + mode: "architect", + currentApiConfigName: "cfg-a", + apiConfiguration: { apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-y" }, + }) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId")) + await provider.dispose() + }) + + it("should keep the persisted mode authoritative when the pre-load buffer is untouched", async () => { + const writer = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + await writer["setViewStateId"]("shared-view") + await writer.saveViewState("mode", "code") + await writer.saveViewState("currentApiConfigName", "my-profile") + + const provider = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + // @ts-ignore - Replace providerSettingsManager with a test double for the profile lookup. + provider.providerSettingsManager = { + getProfile: vi.fn().mockResolvedValue({ + name: "my-profile", + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-x", + }), + } + // A pre-load buffer write that was never persisted must not be merged over the load. + provider["viewLocalState"] = { mode: "architect" } + await provider.contextProxy.setValue( + "viewStates", + mockContext.globalState.get("viewStates"), + ) + await provider["setViewStateId"]("shared-view") + expect(provider["viewLocalState"].mode).toBe("code") + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + expect(provider["viewLocalState"].apiConfiguration).toEqual({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-x", + }) + await writer.dispose() + await provider.dispose() + }) + + it("should not resurrect a field cleared mid-load from the pre-load buffer", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + let resolveProfile: (value: { name: string }) => void = () => {} + // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. + provider.providerSettingsManager = { + getProfile: vi + .fn() + .mockImplementation(() => new Promise<{ name: string }>((resolve) => (resolveProfile = resolve))), + } + await provider.saveViewState("currentApiConfigName", "cfg-a") + provider["viewLocalState"] = { ...provider["viewLocalState"], mode: "architect" } + const load = provider["setViewStateId"]("stable-sidebar-view") + + // The user clears the mode while the load is in flight. + await provider.saveViewState("mode", undefined) + resolveProfile({ name: "cfg-a" }) + await load + + // The cleared field must stay absent rather than keeping the persisted or + // pre-load value; the untouched field keeps the persisted value. + expect(provider["viewLocalState"]).not.toHaveProperty("mode") + expect(provider["viewLocalState"].currentApiConfigName).toBe("cfg-a") + await provider.dispose() + }) + it("should persist known modes, ignore unknown modes and pass through non-string modes", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) const logSpy = vi.spyOn(provider, "log") From dc2e9cde5563b98575d4d7fcce4253dd46b30cc0 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 01:16:21 +0800 Subject: [PATCH 10/18] fix(provider): restore previous view-state id when registration persistence fails --- src/core/webview/ClineProvider.ts | 19 ++++++++++--- .../webview/__tests__/ClineProvider.spec.ts | 27 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 0130266b06..982aa3be45 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -689,13 +689,24 @@ export class ClineProvider return } + const previousViewStateId = this.viewStateId + this.viewStateId = normalizedViewStateId - // Re-key any durable entry written under the temporary pre-launch id before - // loading, so the load sees the view's own pre-registration selections. - await this.rekeyPersistedViewStateEntry(this.viewStateId) + try { + // Re-key any durable entry written under the temporary pre-launch id before + // loading, so the load sees the view's own pre-registration selections. + await this.rekeyPersistedViewStateEntry(this.viewStateId) - await this.loadViewState() + await this.loadViewState() + } catch (error) { + // A persistence failure must not leave the provider holding an id that was + // never registered: restore the previous id so a later launch retries the + // registration and the load instead of the guard above early-returning for + // the failed id. + this.viewStateId = previousViewStateId + throw error + } } /** diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 8f1daff948..2f0e52826f 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1256,6 +1256,33 @@ describe("ClineProvider", () => { await provider.dispose() }) + + it("restores the previous view id when the registration write fails so a later launch retries", async () => { + const contextProxy = new ContextProxy(mockContext) + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", contextProxy) + // Seed a pre-launch entry under the temporary id so the re-key has real work to do. + mockContext.globalState.update("viewStates", { [provider.viewId]: { mode: "architect", updatedAt: 1 } }) + + const setValueSpy = vi.spyOn(contextProxy, "setValue").mockRejectedValue(new Error("storage down")) + + await expect(provider["setViewStateId"]("stable-sidebar-view")).rejects.toThrow("storage down") + + // The failed id must not stick: the provider keeps its previous (temporary) + // id so a later launch retries registration and the load instead of the + // guard early-returning for an id that was never persisted. + expect(provider["viewStateId"]).toBe(provider.viewId) + + // A later retry succeeds once the storage write works again, and the + // pre-launch entry lands under the registered id. + setValueSpy.mockRestore() + await provider["setViewStateId"]("stable-sidebar-view") + expect(provider["viewStateId"]).toBe("stable-sidebar-view") + expect(mockContext.globalState.get("viewStates")).toEqual({ + "stable-sidebar-view": { mode: "architect", updatedAt: 1 }, + }) + + await provider.dispose() + }) }) describe("view state persistence edge cases", () => { From d677e7bc803e013e7b1410b29c242aa98ccf89d6 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 08:32:35 +0800 Subject: [PATCH 11/18] fix(provider): reject invalid view-state modes and sync profile mutations through the view-local buffer --- src/core/webview/ClineProvider.ts | 49 ++- .../webview/__tests__/ClineProvider.spec.ts | 311 +++++++++++++++++- 2 files changed, 332 insertions(+), 28 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 982aa3be45..2df9bf591a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -729,7 +729,15 @@ export class ClineProvider const preLoadBuffer = { ...this.viewLocalState } if (persisted?.mode) { - loadedState.mode = persisted.mode as Mode + // A persisted mode may reference a custom mode that was deleted after it was + // pinned: restore it only when the slug still resolves, so a stale slug cannot + // shadow the shared mode from getState(). + const customModes = await this.customModesManager.getCustomModes() + if (getModeBySlug(persisted.mode, customModes)) { + loadedState.mode = persisted.mode as Mode + } else { + this.log(`[loadViewState] Ignoring unknown persisted mode "${persisted.mode}"`) + } } if (persisted?.currentApiConfigName) { @@ -2219,7 +2227,10 @@ export class ClineProvider // I left the original implementation in just to be safe. await Promise.all([ this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig()), - this.updateGlobalState("currentApiConfigName", name), + // Route through setValue so the in-memory viewLocalState buffer tracks the + // activated profile: a plain global write would leave a stale loaded + // currentApiConfigName shadowing the new value in getValues(). + this.setValue("currentApiConfigName", name), this.providerSettingsManager.setModeConfig(mode, id), this.contextProxy.setProviderSettings(providerSettings), ]) @@ -2261,12 +2272,19 @@ export class ClineProvider const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) + // Write the other settings in one bulk call, then route the current-profile write + // through setValue so the in-memory viewLocalState buffer tracks the activated + // profile: a plain ContextProxy write would leave a stale loaded + // currentApiConfigName shadowing the new value in getValues(). + const { currentApiConfigName: _previousApiConfigName, ...globalSettingsWithoutCurrent } = globalSettings + await this.contextProxy.setValues({ - ...globalSettings, - currentApiConfigName: profileToActivate, + ...globalSettingsWithoutCurrent, listApiConfigMeta: entries, }) + await this.setValue("currentApiConfigName", profileToActivate) + await this.postStateToWebview() } @@ -2336,7 +2354,10 @@ export class ClineProvider // See `upsertProviderProfile` for a description of what this is doing. await Promise.all([ this.contextProxy.setValue("listApiConfigMeta", await this.providerSettingsManager.listConfig()), - this.contextProxy.setValue("currentApiConfigName", name), + // Route through setValue so the in-memory viewLocalState buffer tracks the + // activated profile: a plain ContextProxy write would leave a stale loaded + // currentApiConfigName shadowing the new value in getValues(). + this.setValue("currentApiConfigName", name), this.contextProxy.setProviderSettings(providerSettings), ]) } @@ -3517,14 +3538,16 @@ export class ClineProvider public async setValues(values: RooCodeSettings) { const sanitizedValues = { ...values } - if ( - typeof sanitizedValues.mode === "string" && - !getModeBySlug(sanitizedValues.mode, await this.customModesManager.getCustomModes()) - ) { - // An unknown mode (e.g. from an API payload) must not be persisted: a new Task - // would read it from getState() and persist it into task history. - this.log(`[ClineProvider#setValues] Ignoring unknown mode "${sanitizedValues.mode}"`) - delete sanitizedValues.mode + if (sanitizedValues.mode !== undefined) { + // An unknown or non-string mode (e.g. from an API payload) must not be persisted: + // a new Task would read it from getState() and persist it into task history. + if ( + typeof sanitizedValues.mode !== "string" || + !getModeBySlug(sanitizedValues.mode, await this.customModesManager.getCustomModes()) + ) { + this.log(`[ClineProvider#setValues] Ignoring invalid mode "${String(sanitizedValues.mode)}"`) + delete sanitizedValues.mode + } } await this.contextProxy.setValues(sanitizedValues) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 2f0e52826f..d823755829 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1488,19 +1488,23 @@ describe("ClineProvider", () => { openRouterModelId: string }) => void = () => {} // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. - provider.providerSettingsManager = { - getProfile: vi - .fn() - .mockImplementation( - () => - new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( - (resolve) => (resolveProfile = resolve), - ), - ), - } + const getProfileSpy = vi + .fn() + .mockImplementation( + () => + new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( + (resolve) => (resolveProfile = resolve), + ), + ) + // @ts-ignore - The spy-backed double only needs the stalled getProfile member. + provider.providerSettingsManager = { getProfile: getProfileSpy } await provider.saveViewState("currentApiConfigName", "cfg-a") const load = provider["setViewStateId"]("stable-sidebar-view") + // Let the stalled lookup begin so the in-flight mutations and the resolver + // target the pending promise rather than the initial no-op. + await vi.waitFor(() => expect(getProfileSpy).toHaveBeenCalledTimes(1)) + // Selections made while the profile lookup is in flight must survive the load. await provider.saveViewState("mode", "architect") await provider.saveViewState("apiConfiguration", { @@ -1553,6 +1557,32 @@ describe("ClineProvider", () => { await provider.dispose() }) + it("should not restore a persisted mode whose custom mode no longer exists", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const logSpy = vi.spyOn(provider, "log") + // @ts-ignore - Replace customModesManager with a test double (no custom modes). + provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + // The file-level modes mock resolves every slug to a mode; drop the pinned slug. + const modesModule = vi.mocked(await import("../../../shared/modes")) + const originalMode = modesModule.getModeBySlug("code") + modesModule.getModeBySlug.mockImplementation(((slug: string) => + slug === "deleted-custom-mode" ? undefined : originalMode) as typeof modesModule.getModeBySlug) + try { + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("mode", "deleted-custom-mode") + // Reload the persisted entry: the custom mode was deleted in the meantime. + await provider["loadViewState"]() + // The stale slug must not be restored into the buffer. + expect(provider["viewLocalState"]).not.toHaveProperty("mode") + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining('Ignoring unknown persisted mode "deleted-custom-mode"'), + ) + } finally { + modesModule.getModeBySlug.mockReturnValue(originalMode) + } + await provider.dispose() + }) + it("should not resurrect a field cleared mid-load from the pre-load buffer", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) let resolveProfile: (value: { name: string }) => void = () => {} @@ -1578,7 +1608,166 @@ describe("ClineProvider", () => { await provider.dispose() }) - it("should persist known modes, ignore unknown modes and pass through non-string modes", async () => { + it("should reapply an independently mutated mode when the load settles", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + let resolveProfile: (value: { + name: string + apiProvider: string + openRouterModelId: string + }) => void = () => {} + // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. + const getProfileSpy = vi + .fn() + .mockImplementation( + () => + new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( + (resolve) => (resolveProfile = resolve), + ), + ) + // @ts-ignore - The spy-backed double only needs the stalled getProfile member. + provider.providerSettingsManager = { getProfile: getProfileSpy } + await provider.saveViewState("currentApiConfigName", "cfg-a") + const load = provider["setViewStateId"]("stable-sidebar-view") + + // Let the stalled lookup begin so the in-flight mutation and the resolver + // target the pending promise rather than the initial no-op. + await vi.waitFor(() => expect(getProfileSpy).toHaveBeenCalledTimes(1)) + // Only the mode is mutated while the profile lookup is in flight. + await provider.saveViewState("mode", "architect") + + resolveProfile({ name: "cfg-a", apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }) + await load + + expect(provider["viewLocalState"]).toEqual({ + mode: "architect", + currentApiConfigName: "cfg-a", + apiConfiguration: { apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }, + }) + await provider.dispose() + }) + + it("should reapply an independently mutated profile name when the load settles", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + let resolveProfile: (value: { + name: string + apiProvider: string + openRouterModelId: string + }) => void = () => {} + // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. + const getProfileSpy = vi + .fn() + .mockImplementation( + () => + new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( + (resolve) => (resolveProfile = resolve), + ), + ) + // @ts-ignore - The spy-backed double only needs the stalled getProfile member. + provider.providerSettingsManager = { getProfile: getProfileSpy } + await provider.saveViewState("currentApiConfigName", "cfg-a") + const load = provider["setViewStateId"]("stable-sidebar-view") + + // Let the stalled lookup begin so the in-flight mutation and the resolver + // target the pending promise rather than the initial no-op. + await vi.waitFor(() => expect(getProfileSpy).toHaveBeenCalledTimes(1)) + // The profile name changes while the lookup is in flight; the load still + // resolves the name persisted at load start. + await provider.saveViewState("currentApiConfigName", "cfg-b") + + resolveProfile({ name: "cfg-a", apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }) + await load + + // The in-flight selection wins over the loaded state. + expect(provider["viewLocalState"].currentApiConfigName).toBe("cfg-b") + expect(provider["viewLocalState"].apiConfiguration).toEqual({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-x", + }) + expect(provider["viewLocalState"]).not.toHaveProperty("mode") + await provider.dispose() + }) + + it("should reapply an independently mutated apiConfiguration when the load settles", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + let resolveProfile: (value: { + name: string + apiProvider: string + openRouterModelId: string + }) => void = () => {} + // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. + const getProfileSpy = vi + .fn() + .mockImplementation( + () => + new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( + (resolve) => (resolveProfile = resolve), + ), + ) + // @ts-ignore - The spy-backed double only needs the stalled getProfile member. + provider.providerSettingsManager = { getProfile: getProfileSpy } + await provider.saveViewState("currentApiConfigName", "cfg-a") + const load = provider["setViewStateId"]("stable-sidebar-view") + + // Let the stalled lookup begin so the in-flight mutation and the resolver + // target the pending promise rather than the initial no-op. + await vi.waitFor(() => expect(getProfileSpy).toHaveBeenCalledTimes(1)) + // Only the apiConfiguration is mutated while the profile lookup is in flight. + await provider.saveViewState("apiConfiguration", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-y", + }) + + resolveProfile({ name: "cfg-a", apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }) + await load + + expect(provider["viewLocalState"].apiConfiguration).toEqual({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-y", + }) + expect(provider["viewLocalState"].currentApiConfigName).toBe("cfg-a") + await provider.dispose() + }) + + it("should keep every persisted field authoritative when the load is untouched", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + let resolveProfile: (value: { + name: string + apiProvider: string + openRouterModelId: string + }) => void = () => {} + // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. + const getProfileSpy = vi + .fn() + .mockImplementation( + () => + new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( + (resolve) => (resolveProfile = resolve), + ), + ) + // @ts-ignore - The spy-backed double only needs the stalled getProfile member. + provider.providerSettingsManager = { getProfile: getProfileSpy } + // @ts-ignore - Replace customModesManager with a test double (no custom modes). + provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + await provider.saveViewState("mode", "code") + await provider.saveViewState("currentApiConfigName", "cfg-a") + const load = provider["setViewStateId"]("stable-sidebar-view") + + // Let the stalled lookup begin so the in-flight mutation and the resolver + // target the pending promise rather than the initial no-op. + await vi.waitFor(() => expect(getProfileSpy).toHaveBeenCalledTimes(1)) + // No mutation while the lookup is in flight: the persisted values must win. + resolveProfile({ name: "cfg-a", apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }) + await load + + expect(provider["viewLocalState"]).toEqual({ + mode: "code", + currentApiConfigName: "cfg-a", + apiConfiguration: { apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }, + }) + await provider.dispose() + }) + + it("should persist known modes and reject unknown or non-string modes", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) const logSpy = vi.spyOn(provider, "log") // @ts-ignore - Replace customModesManager with a test double (no custom modes). @@ -1593,13 +1782,15 @@ describe("ClineProvider", () => { expect(mockContext.globalState.get("mode")).toBe("refactor") expect(provider["viewLocalState"].mode).toBe("refactor") await provider.setValues({ mode: "bogus-mode" }) - expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Ignoring unknown mode "bogus-mode"')) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Ignoring invalid mode "bogus-mode"')) expect(mockContext.globalState.get("mode")).toBe("refactor") expect(provider["viewLocalState"].mode).toBe("refactor") - // A non-string mode bypasses the slug validation (double assertion: the type excludes non-strings). + // A non-string mode must be rejected before persistence (double assertion: the type + // excludes non-strings), so it must not reach global state or the buffer. await provider.setValues({ mode: 42 } as unknown as RooCodeSettings) - expect(mockContext.globalState.get("mode")).toBe(42) - expect(provider["viewLocalState"].mode).toBe(42) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Ignoring invalid mode "42"')) + expect(mockContext.globalState.get("mode")).toBe("refactor") + expect(provider["viewLocalState"].mode).toBe("refactor") } finally { modesModule.getModeBySlug.mockReturnValue(originalMode) } @@ -1671,6 +1862,96 @@ describe("ClineProvider", () => { }) }) + describe("provider profile mutations", () => { + it("should sync the view-local buffer when activating a profile over a loaded view state", async () => { + const writer = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + await writer["setViewStateId"]("shared-view") + await writer.saveViewState("currentApiConfigName", "old-profile") + + const profile: ProviderSettingsEntry = { + name: "new-profile", + id: "new-id", + apiProvider: providerIdentifiers.openrouter, + } + + const provider = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + // @ts-ignore - Replace providerSettingsManager with a test double. + provider.providerSettingsManager = { + activateProfile: vi.fn().mockResolvedValue(profile), + listConfig: vi.fn().mockResolvedValue([profile]), + setModeConfig: vi.fn(), + getProfile: vi + .fn() + .mockResolvedValue({ + name: "old-profile", + id: "old-id", + apiProvider: providerIdentifiers.anthropic, + }), + } + vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + await provider.contextProxy.setValue( + "viewStates", + mockContext.globalState.get("viewStates"), + ) + await provider["setViewStateId"]("shared-view") + expect(provider.getValues().currentApiConfigName).toBe("old-profile") + + await provider.activateProviderProfile({ name: "new-profile" }) + + // The buffer must track the activated profile so getValues() agrees with the proxy. + expect(provider.getValues().currentApiConfigName).toBe("new-profile") + expect(provider.contextProxy.getValue("currentApiConfigName")).toBe("new-profile") + await writer.dispose() + await provider.dispose() + }) + + it("should sync the view-local buffer when creating and activating a profile", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const profile: ProviderSettingsEntry = { + name: "fresh-profile", + id: "fresh-id", + apiProvider: providerIdentifiers.openrouter, + } + // @ts-ignore - Replace providerSettingsManager with a test double. + provider.providerSettingsManager = { + saveConfig: vi.fn().mockResolvedValue("fresh-id"), + listConfig: vi.fn().mockResolvedValue([profile]), + setModeConfig: vi.fn(), + } + vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + await provider.setValue("currentApiConfigName", "stale-profile") + + await provider.upsertProviderProfile("fresh-profile", { apiProvider: providerIdentifiers.openrouter }) + + expect(provider.getValues().currentApiConfigName).toBe("fresh-profile") + await provider.dispose() + }) + + it("should sync the view-local buffer when deleting the current profile", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const oldProfile: ProviderSettingsEntry = { + name: "old-profile", + id: "old-id", + apiProvider: providerIdentifiers.openrouter, + } + const keeperProfile: ProviderSettingsEntry = { + name: "keeper-profile", + id: "keeper-id", + apiProvider: providerIdentifiers.anthropic, + } + await provider.contextProxy.setValue("listApiConfigMeta", [oldProfile, keeperProfile]) + await provider.setValue("currentApiConfigName", "old-profile") + vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + + await provider.deleteProviderProfile(oldProfile) + + // The fallback profile must replace the deleted one in both the proxy and the buffer. + expect(provider.getValues().currentApiConfigName).toBe("keeper-profile") + expect(provider.contextProxy.getValue("currentApiConfigName")).toBe("keeper-profile") + await provider.dispose() + }) + }) + describe("postStateToWebviewThrottled", () => { beforeEach(() => { vi.useFakeTimers() From 20c3892e125627889ca4585619357edf8b6ba1b1 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 08:32:57 +0800 Subject: [PATCH 12/18] fix(package): move command palette entries to the commandPalette menu contribution --- src/package.json | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/package.json b/src/package.json index 2b7c018bf4..78519ba2fc 100644 --- a/src/package.json +++ b/src/package.json @@ -285,26 +285,26 @@ "group": "overflow@2", "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" } + ], + "commandPalette": [ + { + "command": "zoo-code.plusButtonClickedInTab", + "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + }, + { + "command": "zoo-code.settingsButtonClickedInTab", + "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + }, + { + "command": "zoo-code.marketplaceButtonClickedInTab", + "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + }, + { + "command": "zoo-code.historyButtonClickedInTab", + "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + } ] }, - "commandPalette": [ - { - "command": "zoo-code.plusButtonClickedInTab", - "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" - }, - { - "command": "zoo-code.settingsButtonClickedInTab", - "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" - }, - { - "command": "zoo-code.marketplaceButtonClickedInTab", - "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" - }, - { - "command": "zoo-code.historyButtonClickedInTab", - "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" - } - ], "keybindings": [ { "command": "zoo-code.addToContext", From 4f4f69a10376b470d2e656e6dc9f9d9098c94ddd Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 11:11:49 +0800 Subject: [PATCH 13/18] fix(webview): sync view-local state with mode and profile mutations and target tab-instance commands Reapply in-flight view-local fields with Object.is identity so a field cleared during the load window stays cleared; route mode switches through setValue so the in-memory buffer and durable write agree, with rollback on failure; refresh cross-instance view-local state on profile upsert, activate and delete and re-pin the buffer after a delete; point focusInput and active-panel re-registration at the tracked tab provider and panel; log dropped webview postMessage failures with the message type; pin tab-instance, focusInput and active-panel identity in the registerCommands tests and type the mdm double in the provider spec. --- .../__tests__/registerCommands.spec.ts | 83 ++++++- src/activate/registerCommands.ts | 22 +- src/core/webview/ClineProvider.ts | 206 +++++++++++++++--- .../webview/__tests__/ClineProvider.spec.ts | 152 +++++-------- src/eslint-suppressions.json | 2 +- src/package.json | 8 +- 6 files changed, 337 insertions(+), 136 deletions(-) diff --git a/src/activate/__tests__/registerCommands.spec.ts b/src/activate/__tests__/registerCommands.spec.ts index e53a1bc2c2..eccb87ad69 100644 --- a/src/activate/__tests__/registerCommands.spec.ts +++ b/src/activate/__tests__/registerCommands.spec.ts @@ -267,7 +267,9 @@ describe("registerCommands handlers", () => { handlers[command]() - expect(ClineProvider.getInstanceForView as Mock).toHaveBeenCalledWith(tabPanel) + // Identity pin: the lookup must receive the exact tracked panel + // object, not a different object that merely compares equal. + expect((ClineProvider.getInstanceForView as Mock).mock.calls[0]![0]).toBe(tabPanel) for (const action of actions) { expect(mockTabProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action }) } @@ -386,12 +388,28 @@ describe("registerCommands handlers", () => { expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) - it("focusInput does not post when a tab panel is tracked alongside the sidebar", async () => { + it("focusInput does not post when a tab panel is tracked without a live tab instance", async () => { setPanel({} as vscode.WebviewView, "sidebar") setPanel({} as vscode.WebviewPanel, "tab") await handlers["zoo-code.focusInput"]() + // The tab takes selection priority, so the sidebar must not receive + // the message; with no live tab instance there is no other target. + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("focusInput posts the focus message on the tab instance when a tab panel is tracked", async () => { + const mockTabProvider = { postMessageToWebview: vi.fn().mockResolvedValue(undefined) } + setPanel({} as vscode.WebviewView, "sidebar") + const tabPanel = {} as vscode.WebviewPanel + setPanel(tabPanel, "tab") + ;(ClineProvider.getInstanceForView as Mock).mockReturnValue(mockTabProvider) + + await handlers["zoo-code.focusInput"]() + + expect(ClineProvider.getInstanceForView as Mock).toHaveBeenCalledWith(tabPanel) + expect(mockTabProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "focusInput" }) expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) @@ -553,7 +571,9 @@ describe("registerCommands handlers", () => { await handlers["zoo-code.plusButtonClickedInTab"]() - expect(ClineProvider.getInstanceForView as Mock).toHaveBeenCalledWith(tabPanel) + // Identity pin: the eviction must run against the provider resolved + // from the exact tracked panel object, not a merely-equal stub. + expect((ClineProvider.getInstanceForView as Mock).mock.calls[0]![0]).toBe(tabPanel) expect(TelemetryService.instance.captureTitleButtonClicked).toHaveBeenCalledWith("plus") expect(mockTabProvider.evictCurrentTask).toHaveBeenCalledTimes(1) expect(mockTabProvider.refreshWorkspace).toHaveBeenCalledTimes(1) @@ -655,6 +675,63 @@ describe("openClineInNewTab", () => { expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1) }) + it("re-points the tracked tab ref at the panel that becomes active", async () => { + // Panel A is created first and tracked... + const panelA = { + marker: "panel-A", + webview: { postMessage: vi.fn() }, + onDidChangeViewState: vi.fn(), + onDidDispose: vi.fn(), + } as unknown as vscode.WebviewPanel + ;(vscode.window.createWebviewPanel as Mock).mockReturnValueOnce(panelA) + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + // ...then panel B is created, which re-points the tracked tab ref. + const panelB = { + marker: "panel-B", + webview: { postMessage: vi.fn() }, + onDidChangeViewState: vi.fn(), + onDidDispose: vi.fn(), + } as unknown as vscode.WebviewPanel + ;(vscode.window.createWebviewPanel as Mock).mockReturnValueOnce(panelB) + await openClineInNewTab({ context: mockContext, outputChannel: mockOutputChannel }) + + // Activating A must reassign the tracked tab ref to A's panel... + const stateChange = (panelA.onDidChangeViewState as Mock).mock.calls[0]![0] as (e: { + webviewPanel: vscode.WebviewPanel + }) => void + stateChange({ webviewPanel: { ...panelA, active: true, visible: true } }) + + // ...so plusButtonClickedInTab targets A's provider, not B's. + const mockProviderA = { + postMessageToWebview: vi.fn().mockResolvedValue(undefined), + evictCurrentTask: vi.fn().mockResolvedValue(undefined), + refreshWorkspace: vi.fn().mockResolvedValue(undefined), + } + ;(ClineProvider.getInstanceForView as Mock).mockImplementation((view: unknown) => + (view as { marker?: string }).marker === "panel-A" ? mockProviderA : undefined, + ) + const handlers = new Map unknown>() + ;(vscode.commands.registerCommand as Mock).mockImplementation( + (id: string, cb: (...args: unknown[]) => unknown) => { + handlers.set(id, cb) + return { dispose: vi.fn() } + }, + ) + const mockSidebarProvider = { postMessageToWebview: vi.fn().mockResolvedValue(undefined) } + registerCommands({ + context: mockContext, + outputChannel: mockOutputChannel, + provider: mockSidebarProvider as unknown as ClineProvider, + }) + + await handlers.get("zoo-code.plusButtonClickedInTab")!() + + expect(mockProviderA.evictCurrentTask).toHaveBeenCalledTimes(1) + expect(mockProviderA.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "chatButtonClicked" }) + expect(mockProviderA.postMessageToWebview).toHaveBeenCalledWith({ type: "action", action: "focusInput" }) + }) + it("falls back to an undefined MdmService when MdmService.getInstance throws", async () => { ;(MdmService.getInstance as Mock).mockImplementation(() => { throw new Error("MDM service not initialized") diff --git a/src/activate/registerCommands.ts b/src/activate/registerCommands.ts index 336111e72e..97e6d68234 100644 --- a/src/activate/registerCommands.ts +++ b/src/activate/registerCommands.ts @@ -235,11 +235,15 @@ const getCommandsMap = ({ try { await focusPanel(tabPanel, sidebarPanel) - // Send focus input message only when the sidebar panel was - // focused: the tab takes selection priority in focusPanel, so - // the sidebar receives the message only when no tab panel is - // tracked. - if (sidebarPanel && !tabPanel) { + // Post to the surface focusPanel selected: the tab takes + // selection priority, so the sidebar is targeted only when no + // tab panel is tracked. + if (tabPanel) { + const tabProvider = getTabProvider() + if (tabProvider) { + await tabProvider.postMessageToWebview({ type: "action", action: "focusInput" }) + } + } else if (sidebarPanel) { await provider.postMessageToWebview({ type: "action", action: "focusInput" }) } } catch (error) { @@ -383,6 +387,14 @@ const createTabPanelUnlocked = async ({ context, outputChannel }: Omit { const panel = e.webviewPanel + // Re-point the tracked tab ref at the panel the user is actually + // looking at: several tab panels can stay visible at once, but + // only the active one is the current tab, and the title-bar + // commands must resolve that instance, not the last created one. + if (panel.active) { + // Stryker disable next-line StringLiteral: setPanel only distinguishes "sidebar"; any other value routes to the tab-ref assignment + setPanel(panel, "tab") + } if (panel.visible) { panel.webview.postMessage({ type: "action", action: "didBecomeVisible" }) // Use the same message type as in SettingsView.tsx } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 2df9bf591a..2dc7b3ba08 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -760,29 +760,38 @@ export class ClineProvider return } - // Reapply only the fields mutated while the load was in flight: untouched - // fields keep the persisted values authoritative, and the pre-load buffer is - // never merged wholesale so stale temporary-id state or a cleared field cannot - // override the stable persisted state. + // Reapply the buffer fields that changed while the load was in flight, + // tracking the change instead of testing against undefined: a field cleared + // during the load window must stay cleared (the loaded value must not + // resurrect it), and a field written to a new value must win over it. + // Untouched fields keep the persisted values authoritative, and the + // pre-load buffer is never merged wholesale so stale temporary-id state + // cannot override the stable persisted state. const postLoadBuffer = this.viewLocalState const mergedState: Partial = { ...loadedState } - if (postLoadBuffer.mode !== preLoadBuffer.mode && postLoadBuffer.mode !== undefined) { - mergedState.mode = postLoadBuffer.mode + if (!Object.is(preLoadBuffer.mode, postLoadBuffer.mode)) { + if (postLoadBuffer.mode === undefined) { + delete mergedState.mode + } else { + mergedState.mode = postLoadBuffer.mode + } } - if ( - postLoadBuffer.currentApiConfigName !== preLoadBuffer.currentApiConfigName && - postLoadBuffer.currentApiConfigName !== undefined - ) { - mergedState.currentApiConfigName = postLoadBuffer.currentApiConfigName + if (!Object.is(preLoadBuffer.currentApiConfigName, postLoadBuffer.currentApiConfigName)) { + if (postLoadBuffer.currentApiConfigName === undefined) { + delete mergedState.currentApiConfigName + } else { + mergedState.currentApiConfigName = postLoadBuffer.currentApiConfigName + } } - if ( - postLoadBuffer.apiConfiguration !== preLoadBuffer.apiConfiguration && - postLoadBuffer.apiConfiguration !== undefined - ) { - mergedState.apiConfiguration = postLoadBuffer.apiConfiguration + if (!Object.is(preLoadBuffer.apiConfiguration, postLoadBuffer.apiConfiguration)) { + if (postLoadBuffer.apiConfiguration === undefined) { + delete mergedState.apiConfiguration + } else { + mergedState.apiConfiguration = postLoadBuffer.apiConfiguration + } } this.viewLocalState = mergedState @@ -1791,8 +1800,13 @@ export class ClineProvider // path (e.g. the trailing postStateToWebview in handleModeSwitchUnlocked gates the next // turn after a mode switch). Message ordering is enforced by the message seq, not the ack. // Promise.resolve() normalizes non-promise returns (e.g. test doubles) before the catch. - void Promise.resolve(webview.postMessage(message)).catch(() => { + void Promise.resolve(webview.postMessage(message)).catch((error) => { // Swallow: postMessage rejects when the webview is disposed in flight. + // Log the dropped message type so a wedged webview channel is diagnosable + // instead of silently losing state updates. + this.log( + `[postMessageToWebview] dropped message type=${message.type}: ${error instanceof Error ? error.message : String(error)}`, + ) }) } @@ -2075,7 +2089,36 @@ export class ClineProvider } } - await this.updateGlobalState("mode", newMode) + // A cancelled or timed-out switch must not write the mode or emit + // ModeChanged: check the mutation signal right before the durable write. + if (signal?.aborted) { + return + } + + // setValue (not the deprecated updateGlobalState) so the in-memory viewLocalState + // buffer stays in sync with the durable global write: getValues() merges + // viewLocalState on top of the ContextProxy values, so an unsynced stale + // restored mode would otherwise shadow the fresh switch for consumers. + // If the durable write fails, roll the shared write back so getValues() + // cannot mix a fresh shared mode with the stale pre-switch buffer. + const previousMode = this.getValue("mode") + try { + await this.setValue("mode", newMode) + } catch (error) { + try { + await this.contextProxy.setValue("mode", previousMode) + } catch (rollbackError) { + this.log( + `[handleModeSwitch] Failed to roll back shared mode after persistence failure: ${ + rollbackError instanceof Error ? rollbackError.message : String(rollbackError) + }`, + ) + } + this.log( + `[handleModeSwitch] Failed to persist mode "${newMode}": ${error instanceof Error ? error.message : String(error)}`, + ) + throw error + } this.emit(RooCodeEventName.ModeChanged, newMode) @@ -2233,8 +2276,18 @@ export class ClineProvider this.setValue("currentApiConfigName", name), this.providerSettingsManager.setModeConfig(mode, id), this.contextProxy.setProviderSettings(providerSettings), + // setProviderSettings writes the shared store directly, bypassing the + // view-local mutation path: also refresh this view's buffer so a stale + // loaded apiConfiguration cannot keep shadowing the new settings in + // getState(). + this._saveViewLocalStateFromMutation(providerSettings), ]) + // Other live views may have buffered this profile's settings earlier; + // refresh them so their getState() cannot report the updated profile's + // name with stale settings. + await this.refreshViewLocalStateForUpdatedProfile(name, providerSettings) + // Change the provider for the current task. // TODO: We should rename `buildApiHandler` for clarity (e.g. `getProviderClient`). this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) @@ -2272,19 +2325,44 @@ export class ClineProvider const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) - // Write the other settings in one bulk call, then route the current-profile write - // through setValue so the in-memory viewLocalState buffer tracks the activated - // profile: a plain ContextProxy write would leave a stale loaded - // currentApiConfigName shadowing the new value in getValues(). - const { currentApiConfigName: _previousApiConfigName, ...globalSettingsWithoutCurrent } = globalSettings + // Write only the profile list back: replaying the full settings snapshot + // captured above would also rewrite unrelated keys (including viewStates, + // which ClineProvider mutates directly in storage for concurrent views) + // with this view's stale cached copy. + await this.contextProxy.setValue("listApiConfigMeta", entries) - await this.contextProxy.setValues({ - ...globalSettingsWithoutCurrent, - listApiConfigMeta: entries, - }) + // Resolve the surviving profile's settings so this view and any other + // live view still pinned to the deleted profile can be re-pinned with + // a matching configuration. + let survivingSettings: ProviderSettings | undefined + try { + const { name: _survivingName, ...settings } = await this.providerSettingsManager.getProfile({ + name: profileToActivate, + }) + survivingSettings = settings as ProviderSettings + } catch (error) { + this.log( + `[deleteProviderProfile] Unable to resolve API profile '${profileToActivate}': ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } await this.setValue("currentApiConfigName", profileToActivate) + if (profileToDelete.name === globalSettings.currentApiConfigName && survivingSettings) { + // The deleted profile was the active one, so the shared provider keys + // and this view's buffer still carry its settings; replace both so + // getState() reports the surviving profile's configuration. + await this.contextProxy.setProviderSettings(survivingSettings) + await this._saveViewLocalStateFromMutation(survivingSettings) + } + + // Re-pin other live views still buffered on the deleted profile: their + // buffer and durable viewStates entry would otherwise keep serving the + // deleted profile's name and configuration. + await this.rePinViewLocalStateForDeletedProfile(profileToDelete.name, profileToActivate, survivingSettings) + await this.postStateToWebview() } @@ -2359,7 +2437,17 @@ export class ClineProvider // currentApiConfigName shadowing the new value in getValues(). this.setValue("currentApiConfigName", name), this.contextProxy.setProviderSettings(providerSettings), + // setProviderSettings writes the shared store directly, bypassing the + // view-local mutation path: also refresh this view's buffer so a stale + // loaded apiConfiguration cannot keep shadowing the new settings in + // getState(). + this._saveViewLocalStateFromMutation(providerSettings), ]) + + // Other live views may have buffered this profile's settings earlier; + // refresh them so their getState() cannot report the activated profile's + // name with stale settings. + await this.refreshViewLocalStateForUpdatedProfile(name, providerSettings) } const { mode } = await this.getState() @@ -2386,6 +2474,70 @@ export class ClineProvider } } + /** + * Refresh the view-local apiConfiguration buffer of the other live views + * pinned to the given profile. An upsert/activation rewrites the profile's + * settings in the shared store and the store-backed manager, but a view + * whose buffer loaded the profile earlier keeps shadowing the stale + * settings in its getState() until its own next mutation. The originating + * view refreshes its buffer at the mutation site itself. + */ + private async refreshViewLocalStateForUpdatedProfile( + name: string, + providerSettings: ProviderSettings, + ): Promise { + const affected = ClineProvider.getAllInstances().filter( + (instance) => instance !== this && instance["viewLocalState"].currentApiConfigName === name, + ) + + if (affected.length === 0) { + return + } + + await Promise.all( + affected.map(async (instance) => { + await instance["_saveViewLocalStateFromMutation"]({ apiConfiguration: providerSettings }) + await instance.postStateToWebview() + }), + ) + } + + /** + * Re-pin the other live views whose buffer still names a deleted profile: + * without this their in-memory buffer and durable viewStates entry keep + * serving the deleted profile's name and configuration on top of the + * surviving shared state. Each affected view's durable entry is re-pinned + * through the serialized write queue, so the rename survives reloads. + */ + private async rePinViewLocalStateForDeletedProfile( + deletedProfileName: string, + replacementName: string, + replacementSettings: ProviderSettings | undefined, + ): Promise { + const affected = ClineProvider.getAllInstances().filter( + (instance) => instance !== this && instance["viewLocalState"].currentApiConfigName === deletedProfileName, + ) + + if (affected.length === 0) { + return + } + + await Promise.all( + affected.map(async (instance) => { + const values: Partial & Partial = { + currentApiConfigName: replacementName, + } + + if (replacementSettings) { + values.apiConfiguration = replacementSettings + } + + await instance["_saveViewLocalStateFromMutation"](values) + await instance.postStateToWebview() + }), + ) + } + async updateCustomInstructions(instructions?: string) { // User may be clearing the field. await this.updateGlobalState("customInstructions", instructions || undefined) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index d823755829..4cd650c65e 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -32,6 +32,7 @@ import { t } from "../../../i18n" import { ClineProvider } from "../ClineProvider" import { webviewMessageHandler } from "../webviewMessageHandler" +import type { MdmService } from "../../../services/mdm/MdmService" import { Terminal } from "../../../integrations/terminal/Terminal" import { MessageManager } from "../../message-manager" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../../api/providers/fetchers/lmstudio" @@ -410,6 +411,35 @@ afterAll(() => { vi.restoreAllMocks() }) +/** + * Minimal profile shape the stalled getProfile double resolves to. The + * settings fields are optional so a lookup that resolves name-only (a + * profile with no configured provider) also type-checks. + */ +type StalledProfile = { + name: string + apiProvider?: string + openRouterModelId?: string +} + +/** + * Swap the provider's ProviderSettingsManager for a double whose getProfile + * stalls until the test resolves it, so tests can mutate view state while + * loadViewState is in flight. The double only backs getProfile, the member + * loadViewState awaits; the documented @ts-ignore replaces the per-test + * suppressions the inlined copies used. + */ +function stallProviderSettingsProfile(provider: ClineProvider) { + let resolveProfile: (value: StalledProfile) => void = () => {} + const getProfileSpy = vi.fn(() => new Promise((resolve) => (resolveProfile = resolve))) + // @ts-ignore - Reassign the readonly providerSettingsManager for the test; the double only backs getProfile. + provider.providerSettingsManager = { getProfile: getProfileSpy } + // Return a stable wrapper around the closure binding: resolveProfile is + // reassigned to the pending promise's resolver once getProfile is called, + // so returning the variable directly would hand the test the initial no-op. + return { getProfile: getProfileSpy, resolveProfile: (value: StalledProfile) => resolveProfile(value) } +} + describe("ClineProvider", () => { beforeAll(() => { vi.mocked(Task).mockImplementation(function (options: any) { @@ -872,10 +902,13 @@ describe("ClineProvider", () => { }) test("postStateToWebview does not force action navigation for non-compliant MDM state", async () => { + // Structural double: the post path only reads these two members, and + // MdmService cannot be constructed as a plain object, so a double + // assertion is the last-resort cast here. const mdmService = { requiresCloudAuth: vi.fn().mockReturnValue(true), isCompliant: vi.fn().mockReturnValue({ compliant: false, reason: "auth required" }), - } as any + } as unknown as MdmService provider = new ClineProvider( mockContext, @@ -886,7 +919,9 @@ describe("ClineProvider", () => { ) const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockImplementation(async () => undefined) - vi.spyOn(provider as any, "getStateToPostToWebview").mockResolvedValue({ version: "1.0.0" }) + vi.spyOn(provider, "getStateToPostToWebview").mockResolvedValue({ + version: "1.0.0", + } as unknown as ExtensionState) await provider.postStateToWebview() @@ -1482,22 +1517,7 @@ describe("ClineProvider", () => { it("should reapply fields mutated while the load is in flight and keep persisted values for untouched fields", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) const logSpy = vi.spyOn(provider, "log") - let resolveProfile: (value: { - name: string - apiProvider: string - openRouterModelId: string - }) => void = () => {} - // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. - const getProfileSpy = vi - .fn() - .mockImplementation( - () => - new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( - (resolve) => (resolveProfile = resolve), - ), - ) - // @ts-ignore - The spy-backed double only needs the stalled getProfile member. - provider.providerSettingsManager = { getProfile: getProfileSpy } + const { getProfile: getProfileSpy, resolveProfile } = stallProviderSettingsProfile(provider) await provider.saveViewState("currentApiConfigName", "cfg-a") const load = provider["setViewStateId"]("stable-sidebar-view") @@ -1585,17 +1605,19 @@ describe("ClineProvider", () => { it("should not resurrect a field cleared mid-load from the pre-load buffer", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) - let resolveProfile: (value: { name: string }) => void = () => {} - // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. - provider.providerSettingsManager = { - getProfile: vi - .fn() - .mockImplementation(() => new Promise<{ name: string }>((resolve) => (resolveProfile = resolve))), - } + const { getProfile: getProfileSpy, resolveProfile } = stallProviderSettingsProfile(provider) + // Persist a mode too: without it the load has no mode to resurrect, so the + // cleared-field assertion below would pass even if the loaded value clobbered + // the in-flight clear. await provider.saveViewState("currentApiConfigName", "cfg-a") + await provider.saveViewState("mode", "code") provider["viewLocalState"] = { ...provider["viewLocalState"], mode: "architect" } const load = provider["setViewStateId"]("stable-sidebar-view") + // Let the stalled lookup begin so the in-flight clear and the resolver + // target the pending promise rather than the initial no-op. + await vi.waitFor(() => expect(getProfileSpy).toHaveBeenCalledTimes(1)) + // The user clears the mode while the load is in flight. await provider.saveViewState("mode", undefined) resolveProfile({ name: "cfg-a" }) @@ -1610,22 +1632,7 @@ describe("ClineProvider", () => { it("should reapply an independently mutated mode when the load settles", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) - let resolveProfile: (value: { - name: string - apiProvider: string - openRouterModelId: string - }) => void = () => {} - // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. - const getProfileSpy = vi - .fn() - .mockImplementation( - () => - new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( - (resolve) => (resolveProfile = resolve), - ), - ) - // @ts-ignore - The spy-backed double only needs the stalled getProfile member. - provider.providerSettingsManager = { getProfile: getProfileSpy } + const { getProfile: getProfileSpy, resolveProfile } = stallProviderSettingsProfile(provider) await provider.saveViewState("currentApiConfigName", "cfg-a") const load = provider["setViewStateId"]("stable-sidebar-view") @@ -1648,22 +1655,7 @@ describe("ClineProvider", () => { it("should reapply an independently mutated profile name when the load settles", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) - let resolveProfile: (value: { - name: string - apiProvider: string - openRouterModelId: string - }) => void = () => {} - // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. - const getProfileSpy = vi - .fn() - .mockImplementation( - () => - new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( - (resolve) => (resolveProfile = resolve), - ), - ) - // @ts-ignore - The spy-backed double only needs the stalled getProfile member. - provider.providerSettingsManager = { getProfile: getProfileSpy } + const { getProfile: getProfileSpy, resolveProfile } = stallProviderSettingsProfile(provider) await provider.saveViewState("currentApiConfigName", "cfg-a") const load = provider["setViewStateId"]("stable-sidebar-view") @@ -1689,22 +1681,7 @@ describe("ClineProvider", () => { it("should reapply an independently mutated apiConfiguration when the load settles", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) - let resolveProfile: (value: { - name: string - apiProvider: string - openRouterModelId: string - }) => void = () => {} - // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. - const getProfileSpy = vi - .fn() - .mockImplementation( - () => - new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( - (resolve) => (resolveProfile = resolve), - ), - ) - // @ts-ignore - The spy-backed double only needs the stalled getProfile member. - provider.providerSettingsManager = { getProfile: getProfileSpy } + const { getProfile: getProfileSpy, resolveProfile } = stallProviderSettingsProfile(provider) await provider.saveViewState("currentApiConfigName", "cfg-a") const load = provider["setViewStateId"]("stable-sidebar-view") @@ -1730,22 +1707,7 @@ describe("ClineProvider", () => { it("should keep every persisted field authoritative when the load is untouched", async () => { const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) - let resolveProfile: (value: { - name: string - apiProvider: string - openRouterModelId: string - }) => void = () => {} - // @ts-ignore - Replace providerSettingsManager with a test double that stalls the profile lookup. - const getProfileSpy = vi - .fn() - .mockImplementation( - () => - new Promise<{ name: string; apiProvider: string; openRouterModelId: string }>( - (resolve) => (resolveProfile = resolve), - ), - ) - // @ts-ignore - The spy-backed double only needs the stalled getProfile member. - provider.providerSettingsManager = { getProfile: getProfileSpy } + const { getProfile: getProfileSpy, resolveProfile } = stallProviderSettingsProfile(provider) // @ts-ignore - Replace customModesManager with a test double (no custom modes). provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } await provider.saveViewState("mode", "code") @@ -1880,13 +1842,11 @@ describe("ClineProvider", () => { activateProfile: vi.fn().mockResolvedValue(profile), listConfig: vi.fn().mockResolvedValue([profile]), setModeConfig: vi.fn(), - getProfile: vi - .fn() - .mockResolvedValue({ - name: "old-profile", - id: "old-id", - apiProvider: providerIdentifiers.anthropic, - }), + getProfile: vi.fn().mockResolvedValue({ + name: "old-profile", + id: "old-id", + apiProvider: providerIdentifiers.anthropic, + }), } vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) await provider.contextProxy.setValue( diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 335ce8aeb2..5f58a74874 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1031,7 +1031,7 @@ }, "core/webview/__tests__/ClineProvider.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 198 + "count": 196 } }, "core/webview/__tests__/ClineProvider.sticky-mode.spec.ts": { diff --git a/src/package.json b/src/package.json index 78519ba2fc..95f109ec67 100644 --- a/src/package.json +++ b/src/package.json @@ -289,19 +289,19 @@ "commandPalette": [ { "command": "zoo-code.plusButtonClickedInTab", - "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + "when": "false" }, { "command": "zoo-code.settingsButtonClickedInTab", - "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + "when": "false" }, { "command": "zoo-code.marketplaceButtonClickedInTab", - "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + "when": "false" }, { "command": "zoo-code.historyButtonClickedInTab", - "when": "activeWebviewPanelId == zoo-code.TabPanelProvider" + "when": "false" } ] }, From 93524f936f5e680d5cdb80f8d8516dfd4b01234d Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Mon, 7 Sep 2026 03:11:34 +0800 Subject: [PATCH 14/18] fix(core/webview): merge view-local state into getState for per-view overrides Fold ClineProvider viewLocalState on top of ContextProxy values in getState() (mode, apiConfiguration, and all per-view fields) so each webview reports its own selections while falling back to shared global state for everything else. Ports the getState-merging and local-state-isolation spec coverage from the superseded vps2 source. Also pins the full default surface of the merged read path, including the apiConfiguration provider fill-in when provider settings sanitize the raw value away (mutation-diff gate). --- src/core/webview/ClineProvider.ts | 199 +++---- .../webview/__tests__/ClineProvider.spec.ts | 518 ++++++++++++++++++ 2 files changed, 622 insertions(+), 95 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 2dc7b3ba08..6a146e3e78 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3394,12 +3394,18 @@ export class ClineProvider > > { const stateValues = this.contextProxy.getValues() + + // Merge viewLocalState on top of global state so a provider can serve + // state values scoped to its own view while preserving ContextProxy defaults. + const mergedStateValues = { ...stateValues, ...this.viewLocalState } + const customModes = await this.customModesManager.getCustomModes() // Determine apiProvider with the same logic as before, while filtering retired providers. + // Use mergedStateValues to prioritize viewLocalState for parallel mode support const apiProvider: ProviderName = - stateValues.apiProvider && !isRetiredProvider(stateValues.apiProvider) - ? stateValues.apiProvider + mergedStateValues.apiProvider && !isRetiredProvider(mergedStateValues.apiProvider) + ? mergedStateValues.apiProvider : providerIdentifiers.anthropic // Build the apiConfiguration object combining state values and secrets. @@ -3461,119 +3467,122 @@ export class ClineProvider // Return the same structure as before. return { - apiConfiguration: providerSettings, - lastShownAnnouncementId: stateValues.lastShownAnnouncementId, - customInstructions: stateValues.customInstructions, - apiModelId: stateValues.apiModelId, - alwaysAllowReadOnly: stateValues.alwaysAllowReadOnly ?? false, - alwaysAllowReadOnlyOutsideWorkspace: stateValues.alwaysAllowReadOnlyOutsideWorkspace ?? false, - allowedReadFiles: stateValues.allowedReadFiles ?? [], - alwaysAllowWrite: stateValues.alwaysAllowWrite ?? false, - alwaysAllowWriteOutsideWorkspace: stateValues.alwaysAllowWriteOutsideWorkspace ?? false, - alwaysAllowWriteProtected: stateValues.alwaysAllowWriteProtected ?? false, - allowedWriteFiles: stateValues.allowedWriteFiles ?? [], - alwaysAllowExecute: stateValues.alwaysAllowExecute ?? false, + apiConfiguration: { + ...providerSettings, + ...mergedStateValues.apiConfiguration, + }, + lastShownAnnouncementId: mergedStateValues.lastShownAnnouncementId, + customInstructions: mergedStateValues.customInstructions, + apiModelId: mergedStateValues.apiModelId, + alwaysAllowReadOnly: mergedStateValues.alwaysAllowReadOnly ?? false, + alwaysAllowReadOnlyOutsideWorkspace: mergedStateValues.alwaysAllowReadOnlyOutsideWorkspace ?? false, + allowedReadFiles: mergedStateValues.allowedReadFiles ?? [], + alwaysAllowWrite: mergedStateValues.alwaysAllowWrite ?? false, + alwaysAllowWriteOutsideWorkspace: mergedStateValues.alwaysAllowWriteOutsideWorkspace ?? false, + alwaysAllowWriteProtected: mergedStateValues.alwaysAllowWriteProtected ?? false, + allowedWriteFiles: mergedStateValues.allowedWriteFiles ?? [], + alwaysAllowExecute: mergedStateValues.alwaysAllowExecute ?? false, destructiveCommandGuardEnabled: - stateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED, - alwaysAllowMcp: stateValues.alwaysAllowMcp ?? false, - alwaysAllowModeSwitch: stateValues.alwaysAllowModeSwitch ?? false, - alwaysAllowSubtasks: stateValues.alwaysAllowSubtasks ?? false, - alwaysAllowFollowupQuestions: stateValues.alwaysAllowFollowupQuestions ?? false, - followupAutoApproveTimeoutMs: stateValues.followupAutoApproveTimeoutMs ?? 60000, - diagnosticsEnabled: stateValues.diagnosticsEnabled ?? true, - allowedMaxRequests: stateValues.allowedMaxRequests, - allowedMaxCost: stateValues.allowedMaxCost, - autoCondenseContext: stateValues.autoCondenseContext ?? true, - autoCondenseContextPercent: stateValues.autoCondenseContextPercent ?? 100, + mergedStateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED, + alwaysAllowMcp: mergedStateValues.alwaysAllowMcp ?? false, + alwaysAllowModeSwitch: mergedStateValues.alwaysAllowModeSwitch ?? false, + alwaysAllowSubtasks: mergedStateValues.alwaysAllowSubtasks ?? false, + alwaysAllowFollowupQuestions: mergedStateValues.alwaysAllowFollowupQuestions ?? false, + followupAutoApproveTimeoutMs: mergedStateValues.followupAutoApproveTimeoutMs ?? 60000, + diagnosticsEnabled: mergedStateValues.diagnosticsEnabled ?? true, + allowedMaxRequests: mergedStateValues.allowedMaxRequests, + allowedMaxCost: mergedStateValues.allowedMaxCost, + autoCondenseContext: mergedStateValues.autoCondenseContext ?? true, + autoCondenseContextPercent: mergedStateValues.autoCondenseContextPercent ?? 100, taskHistory: includeTaskHistory ? this.taskHistoryStore.getAll() : [], - allowedCommands: stateValues.allowedCommands, - deniedCommands: stateValues.deniedCommands, - soundEnabled: stateValues.soundEnabled ?? false, - ttsEnabled: stateValues.ttsEnabled ?? false, - ttsSpeed: stateValues.ttsSpeed ?? 1.0, - enableCheckpoints: stateValues.enableCheckpoints ?? true, - checkpointTimeout: stateValues.checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, - soundVolume: stateValues.soundVolume, - writeDelayMs: stateValues.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, - diffFuzzyThreshold: stateValues.diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, + allowedCommands: mergedStateValues.allowedCommands, + deniedCommands: mergedStateValues.deniedCommands, + soundEnabled: mergedStateValues.soundEnabled ?? false, + ttsEnabled: mergedStateValues.ttsEnabled ?? false, + ttsSpeed: mergedStateValues.ttsSpeed ?? 1.0, + enableCheckpoints: mergedStateValues.enableCheckpoints ?? true, + checkpointTimeout: mergedStateValues.checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + soundVolume: mergedStateValues.soundVolume, + writeDelayMs: mergedStateValues.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, + diffFuzzyThreshold: mergedStateValues.diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, terminalShellIntegrationTimeout: - stateValues.terminalShellIntegrationTimeout ?? Terminal.defaultShellIntegrationTimeout, - terminalShellIntegrationDisabled: stateValues.terminalShellIntegrationDisabled ?? true, - terminalCommandDelay: stateValues.terminalCommandDelay ?? 0, - terminalPowershellCounter: stateValues.terminalPowershellCounter ?? false, - terminalZshClearEolMark: stateValues.terminalZshClearEolMark ?? true, - terminalZshOhMy: stateValues.terminalZshOhMy ?? false, - terminalZshP10k: stateValues.terminalZshP10k ?? false, - terminalZdotdir: stateValues.terminalZdotdir ?? false, - terminalProfile: stateValues.terminalProfile, - mode: stateValues.mode ?? defaultModeSlug, - language: stateValues.language ?? formatLanguage(vscode.env.language), - mcpEnabled: stateValues.mcpEnabled ?? true, + mergedStateValues.terminalShellIntegrationTimeout ?? Terminal.defaultShellIntegrationTimeout, + terminalShellIntegrationDisabled: mergedStateValues.terminalShellIntegrationDisabled ?? true, + terminalCommandDelay: mergedStateValues.terminalCommandDelay ?? 0, + terminalPowershellCounter: mergedStateValues.terminalPowershellCounter ?? false, + terminalZshClearEolMark: mergedStateValues.terminalZshClearEolMark ?? true, + terminalZshOhMy: mergedStateValues.terminalZshOhMy ?? false, + terminalZshP10k: mergedStateValues.terminalZshP10k ?? false, + terminalZdotdir: mergedStateValues.terminalZdotdir ?? false, + terminalProfile: mergedStateValues.terminalProfile, + mode: (mergedStateValues.mode as Mode) ?? defaultModeSlug, + language: mergedStateValues.language ?? formatLanguage(vscode.env.language), + mcpEnabled: mergedStateValues.mcpEnabled ?? true, mcpServers: this.mcpHub?.getAllServers() ?? [], - currentApiConfigName: stateValues.currentApiConfigName ?? "default", - listApiConfigMeta: stateValues.listApiConfigMeta ?? [], - pinnedApiConfigs: stateValues.pinnedApiConfigs ?? {}, - modeApiConfigs: stateValues.modeApiConfigs ?? ({} as Record), - customModePrompts: stateValues.customModePrompts ?? {}, - customSupportPrompts: stateValues.customSupportPrompts ?? {}, - enhancementApiConfigId: stateValues.enhancementApiConfigId, - experiments: stateValues.experiments ?? experimentDefault, - autoApprovalEnabled: stateValues.autoApprovalEnabled ?? false, + currentApiConfigName: mergedStateValues.currentApiConfigName ?? "default", + listApiConfigMeta: mergedStateValues.listApiConfigMeta ?? [], + pinnedApiConfigs: mergedStateValues.pinnedApiConfigs ?? {}, + modeApiConfigs: (mergedStateValues.modeApiConfigs as Record) ?? ({} as Record), + customModePrompts: mergedStateValues.customModePrompts ?? {}, + customSupportPrompts: mergedStateValues.customSupportPrompts ?? {}, + enhancementApiConfigId: mergedStateValues.enhancementApiConfigId, + experiments: mergedStateValues.experiments ?? experimentDefault, + autoApprovalEnabled: mergedStateValues.autoApprovalEnabled ?? false, customModes, - maxOpenTabsContext: stateValues.maxOpenTabsContext ?? 20, - maxWorkspaceFiles: stateValues.maxWorkspaceFiles ?? 200, - disabledTools: stateValues.disabledTools, - telemetrySetting: stateValues.telemetrySetting || "unset", - showRooIgnoredFiles: stateValues.showRooIgnoredFiles ?? false, - enableSubfolderRules: stateValues.enableSubfolderRules ?? false, - maxImageFileSize: stateValues.maxImageFileSize ?? 5, - maxTotalImageSize: stateValues.maxTotalImageSize ?? 20, - historyPreviewCollapsed: stateValues.historyPreviewCollapsed ?? false, - reasoningBlockCollapsed: stateValues.reasoningBlockCollapsed ?? true, - chatFontSize: stateValues.chatFontSize, - enterBehavior: stateValues.enterBehavior ?? "send", + maxOpenTabsContext: mergedStateValues.maxOpenTabsContext ?? 20, + maxWorkspaceFiles: mergedStateValues.maxWorkspaceFiles ?? 200, + disabledTools: mergedStateValues.disabledTools, + telemetrySetting: mergedStateValues.telemetrySetting || "unset", + showRooIgnoredFiles: mergedStateValues.showRooIgnoredFiles ?? false, + enableSubfolderRules: mergedStateValues.enableSubfolderRules ?? false, + maxImageFileSize: mergedStateValues.maxImageFileSize ?? 5, + maxTotalImageSize: mergedStateValues.maxTotalImageSize ?? 20, + historyPreviewCollapsed: mergedStateValues.historyPreviewCollapsed ?? false, + reasoningBlockCollapsed: mergedStateValues.reasoningBlockCollapsed ?? true, + chatFontSize: mergedStateValues.chatFontSize, + enterBehavior: mergedStateValues.enterBehavior ?? "send", cloudUserInfo, cloudIsAuthenticated, sharingEnabled, publicSharingEnabled, organizationAllowList, organizationSettingsVersion, - customCondensingPrompt: stateValues.customCondensingPrompt, - codebaseIndexModels: stateValues.codebaseIndexModels ?? EMBEDDING_MODEL_PROFILES, + customCondensingPrompt: mergedStateValues.customCondensingPrompt, + codebaseIndexModels: mergedStateValues.codebaseIndexModels ?? EMBEDDING_MODEL_PROFILES, codebaseIndexConfig: { - codebaseIndexEnabled: stateValues.codebaseIndexConfig?.codebaseIndexEnabled ?? false, + codebaseIndexEnabled: mergedStateValues.codebaseIndexConfig?.codebaseIndexEnabled ?? false, codebaseIndexQdrantUrl: - stateValues.codebaseIndexConfig?.codebaseIndexQdrantUrl ?? "http://localhost:6333", + mergedStateValues.codebaseIndexConfig?.codebaseIndexQdrantUrl ?? "http://localhost:6333", codebaseIndexEmbedderProvider: - stateValues.codebaseIndexConfig?.codebaseIndexEmbedderProvider ?? providerIdentifiers.openai, - codebaseIndexEmbedderBaseUrl: stateValues.codebaseIndexConfig?.codebaseIndexEmbedderBaseUrl ?? "", - codebaseIndexEmbedderModelId: stateValues.codebaseIndexConfig?.codebaseIndexEmbedderModelId ?? "", + mergedStateValues.codebaseIndexConfig?.codebaseIndexEmbedderProvider ?? providerIdentifiers.openai, + codebaseIndexEmbedderBaseUrl: mergedStateValues.codebaseIndexConfig?.codebaseIndexEmbedderBaseUrl ?? "", + codebaseIndexEmbedderModelId: mergedStateValues.codebaseIndexConfig?.codebaseIndexEmbedderModelId ?? "", codebaseIndexEmbedderModelDimension: - stateValues.codebaseIndexConfig?.codebaseIndexEmbedderModelDimension, + mergedStateValues.codebaseIndexConfig?.codebaseIndexEmbedderModelDimension, codebaseIndexOpenAiCompatibleBaseUrl: - stateValues.codebaseIndexConfig?.codebaseIndexOpenAiCompatibleBaseUrl, - codebaseIndexSearchMaxResults: stateValues.codebaseIndexConfig?.codebaseIndexSearchMaxResults, - codebaseIndexSearchMinScore: stateValues.codebaseIndexConfig?.codebaseIndexSearchMinScore, - codebaseIndexBedrockRegion: stateValues.codebaseIndexConfig?.codebaseIndexBedrockRegion, - codebaseIndexBedrockProfile: stateValues.codebaseIndexConfig?.codebaseIndexBedrockProfile, + mergedStateValues.codebaseIndexConfig?.codebaseIndexOpenAiCompatibleBaseUrl, + codebaseIndexSearchMaxResults: mergedStateValues.codebaseIndexConfig?.codebaseIndexSearchMaxResults, + codebaseIndexSearchMinScore: mergedStateValues.codebaseIndexConfig?.codebaseIndexSearchMinScore, + codebaseIndexBedrockRegion: mergedStateValues.codebaseIndexConfig?.codebaseIndexBedrockRegion, + codebaseIndexBedrockProfile: mergedStateValues.codebaseIndexConfig?.codebaseIndexBedrockProfile, codebaseIndexOpenRouterSpecificProvider: - stateValues.codebaseIndexConfig?.codebaseIndexOpenRouterSpecificProvider, + mergedStateValues.codebaseIndexConfig?.codebaseIndexOpenRouterSpecificProvider, }, - profileThresholds: stateValues.profileThresholds ?? {}, + profileThresholds: mergedStateValues.profileThresholds ?? {}, lockApiConfigAcrossModes: this.context.workspaceState.get("lockApiConfigAcrossModes", false), - includeDiagnosticMessages: stateValues.includeDiagnosticMessages ?? true, - maxDiagnosticMessages: stateValues.maxDiagnosticMessages ?? 50, - includeTaskHistoryInEnhance: stateValues.includeTaskHistoryInEnhance ?? true, - includeCurrentTime: stateValues.includeCurrentTime ?? true, - includeCurrentCost: stateValues.includeCurrentCost ?? true, - maxGitStatusFiles: stateValues.maxGitStatusFiles ?? 0, + includeDiagnosticMessages: mergedStateValues.includeDiagnosticMessages ?? true, + maxDiagnosticMessages: mergedStateValues.maxDiagnosticMessages ?? 50, + includeTaskHistoryInEnhance: mergedStateValues.includeTaskHistoryInEnhance ?? true, + includeCurrentTime: mergedStateValues.includeCurrentTime ?? true, + includeCurrentCost: mergedStateValues.includeCurrentCost ?? true, + maxGitStatusFiles: mergedStateValues.maxGitStatusFiles ?? 0, taskSyncEnabled, - imageGenerationProvider: stateValues.imageGenerationProvider, - openRouterImageApiKey: stateValues.openRouterImageApiKey, - openRouterImageGenerationSelectedModel: stateValues.openRouterImageGenerationSelectedModel, - autoCloseZooOpenedFiles: stateValues.autoCloseZooOpenedFiles, - autoCloseZooOpenedFilesAfterUserEdited: stateValues.autoCloseZooOpenedFilesAfterUserEdited, - autoCloseZooOpenedNewFiles: stateValues.autoCloseZooOpenedNewFiles, + imageGenerationProvider: mergedStateValues.imageGenerationProvider, + openRouterImageApiKey: mergedStateValues.openRouterImageApiKey, + openRouterImageGenerationSelectedModel: mergedStateValues.openRouterImageGenerationSelectedModel, + autoCloseZooOpenedFiles: mergedStateValues.autoCloseZooOpenedFiles, + autoCloseZooOpenedFilesAfterUserEdited: mergedStateValues.autoCloseZooOpenedFilesAfterUserEdited, + autoCloseZooOpenedNewFiles: mergedStateValues.autoCloseZooOpenedNewFiles, } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 4cd650c65e..5f178370af 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -24,6 +24,7 @@ import { TelemetryService } from "@roo-code/telemetry" import { defaultModeSlug } from "../../../shared/modes" import { experimentDefault } from "../../../shared/experiments" +import { EMBEDDING_MODEL_PROFILES } from "../../../shared/embeddingModels" import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" import { Task, TaskOptions } from "../../task/Task" @@ -1912,6 +1913,523 @@ describe("ClineProvider", () => { }) }) + describe("local state isolation", () => { + it("should isolate mode state between instances", async () => { + const provider1 = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + ) + const provider2 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + + await provider2.saveViewState("mode", "debugger") + await provider1.saveViewState("mode", "architect") + + const state1 = await provider1.getState() + const state2 = await provider2.getState() + + expect(state1.mode).toBe("architect") + expect(state2.mode).toBe("debugger") + + await provider1.dispose() + await provider2.dispose() + }) + + it("should isolate currentApiConfigName between instances", async () => { + const provider1 = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + ) + const provider2 = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + + const saveViewState1 = provider1.saveViewState.bind(provider1) + const saveViewState2 = provider2.saveViewState.bind(provider2) + + await saveViewState1("currentApiConfigName", "profile-a") + await saveViewState2("currentApiConfigName", "profile-b") + + const state1 = await provider1.getState() + const state2 = await provider2.getState() + + expect(state1.currentApiConfigName).toBe("profile-a") + expect(state2.currentApiConfigName).toBe("profile-b") + + await provider1.dispose() + await provider2.dispose() + }) + }) + + describe("getState merging", () => { + it("should merge viewLocalState on top of global state", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + // Initially, getState should return values from contextProxy (global state) + let state = await provider.getState() + expect(state.mode).toBe("code") + + // After saveViewState, viewLocalState should take precedence + await provider.saveViewState("mode", "architect") + + state = await provider.getState() + expect(state.mode).toBe("architect") + + await provider.dispose() + }) + + it("should preserve global state values not overridden by viewLocalState", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider.saveViewState("mode", "architect") + + const state = await provider.getState() + + // mode should come from viewLocalState + expect(state.mode).toBe("architect") + + // Other values should still come from global state / contextProxy + expect(state.language).toBeDefined() + expect(state.customModes).toBeDefined() + + await provider.dispose() + }) + + it("should let viewLocalState apiConfiguration override provider settings", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider.saveViewState("apiConfiguration", { + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "local-key", + }) + + const state = await provider.getState() + + expect(state.apiConfiguration.apiProvider).toBe("openrouter") + expect(state.apiConfiguration.openRouterApiKey).toBe("local-key") + + await provider.dispose() + }) + + it("should merge getValues from ContextProxy with view-local values taking precedence", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const providerAccess = provider as unknown as { + saveViewState: (key: keyof ExtensionState, value: unknown) => Promise + } + const contextProxyAccess = provider.contextProxy as unknown as { + setValues: (values: Partial) => Promise + } + await contextProxyAccess.setValues({ + mode: "debugger", + currentApiConfigName: "shared-profile", + apiConfiguration: { + apiProvider: providerIdentifiers.anthropic, + apiKey: "shared-key", + }, + customModePrompts: { code: { roleDefinition: "shared" } }, + }) + + await providerAccess.saveViewState("mode", "architect") + await providerAccess.saveViewState("currentApiConfigName", "view-profile") + await providerAccess.saveViewState("apiConfiguration", { + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "view-key", + }) + + const values = provider.getValues() + + expect(values.mode).toBe("architect") + expect(values.currentApiConfigName).toBe("view-profile") + expect(values.apiConfiguration).toEqual({ + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "view-key", + }) + expect(values.customModePrompts).toEqual({ code: { roleDefinition: "shared" } }) + + await provider.dispose() + }) + + it("should update viewLocalState apiConfiguration when setValues receives flat provider settings", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider.saveViewState("apiConfiguration", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "openrouter/old-model", + }) + + await provider.setValues({ + apiProvider: providerIdentifiers.bedrock, + awsUseApiKey: true, + awsApiKey: "mock-key", + awsRegion: "us-east-1", + apiModelId: "anthropic.claude-opus-4-8-20261215-v1:0", + awsBedrockEndpoint: "http://127.0.0.1:4567", + awsBedrockEndpointEnabled: true, + }) + + const state = await provider.getState() + + expect(state.apiConfiguration.apiProvider).toBe("bedrock") + expect(state.apiConfiguration.awsBedrockEndpoint).toBe("http://127.0.0.1:4567") + expect(provider["viewLocalState"].apiConfiguration?.apiProvider).toBe("bedrock") + expect(provider["viewLocalState"].apiConfiguration).not.toHaveProperty("openRouterModelId") + + await provider.dispose() + }) + + it("should persist setValue mutations for view-local mode", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider["setViewStateId"]("stable-sidebar-view") + await provider.setValue("mode", "architect") + + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "stable-sidebar-view": { mode: "architect" }, + }) + + await provider.dispose() + }) + + it("should persist setValues mutations for view-local API profile", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider["setViewStateId"]("stable-sidebar-view") + await provider.setValues({ currentApiConfigName: "profile-from-set-values" }) + + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "stable-sidebar-view": { currentApiConfigName: "profile-from-set-values" }, + }) + + await provider.dispose() + }) + + it("should drop an unknown mode from setValues while keeping valid modes", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + // This file's getModeBySlug mock resolves every slug; narrow it to the slugs + // under test so "not-a-real-mode" is rejected like the real lookup would. + const modesModule = vi.mocked(await import("../../../shared/modes")) + const originalMode = modesModule.getModeBySlug("code") + modesModule.getModeBySlug.mockImplementation(((slug: string) => + ["code", "architect"].includes(slug) ? { slug } : undefined) as typeof modesModule.getModeBySlug) + + try { + await provider.setValues({ mode: "not-a-real-mode" }) + + expect(provider.contextProxy.getValue("mode")).toBeUndefined() + expect(provider["viewLocalState"].mode).toBeUndefined() + + await provider.setValues({ mode: "architect" }) + + expect(provider.contextProxy.getValue("mode")).toBe("architect") + expect(provider["viewLocalState"].mode).toBe("architect") + } finally { + modesModule.getModeBySlug.mockReturnValue(originalMode) + } + + await provider.dispose() + }) + + it("should sanitize raw viewStateId before using it as persisted viewStates key", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider["setViewStateId"]("tab panel/with.dots and spaces") + await provider.setValue("mode", "architect") + + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + tab_panel_with_dots_and_spaces: { mode: "architect" }, + }) + expect(provider.contextProxy.getValue("viewStates")).not.toHaveProperty("tab panel/with.dots and spaces") + + await provider.dispose() + }) + + it("should persist queued writes under the viewStateId active when the change was made", async () => { + let releaseFirstWrite!: () => void + const firstWriteStarted = new Promise((resolve) => { + mockContext.globalState.update = vi + .fn() + .mockImplementationOnce((key: string, value: unknown) => { + mockContext.globalState.get = vi + .fn() + .mockImplementation((lookupKey: string) => (lookupKey === key ? value : undefined)) + resolve() + return new Promise((writeResolve) => { + releaseFirstWrite = writeResolve + }) + }) + .mockImplementation((key: string, value: unknown) => { + mockContext.globalState.get = vi + .fn() + .mockImplementation((lookupKey: string) => (lookupKey === key ? value : undefined)) + return Promise.resolve() + }) + }) + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider["setViewStateId"]("view-a") + const firstSave = provider.saveViewState("mode", "architect") + await firstWriteStarted + await provider["setViewStateId"]("view-b") + releaseFirstWrite() + await firstSave + + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "view-a": { mode: "architect" }, + }) + expect(provider.contextProxy.getValue("viewStates")).not.toHaveProperty("view-b") + + await provider.dispose() + }) + + it("should preserve persisted viewStates entry when an editor provider is disposed during teardown", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "editor", new ContextProxy(mockContext)) + + await provider["setViewStateId"]("tab-to-preserve") + await provider.saveViewState("mode", "architect") + expect(provider.contextProxy.getValue("viewStates")).toHaveProperty("tab-to-preserve") + + await provider.dispose() + + expect(provider.contextProxy.getValue("viewStates")).toHaveProperty("tab-to-preserve") + }) + + it("should read viewStates fresh from storage so out-of-proxy writes are not clobbered", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + await provider["setViewStateId"]("view-a") + await provider.saveViewState("mode", "architect") + + // Simulate a concurrent writer (another view's provider) updating the shared + // map directly in storage, bypassing this proxy's cache. + const stored = (await mockContext.globalState.get>("viewStates")) ?? {} + await mockContext.globalState.update("viewStates", { + ...stored, + "view-b": { mode: "debug", updatedAt: 1 }, + }) + + await provider.saveViewState("mode", "code") + + // The serialized write must have merged on top of the fresh storage value, not + // on top of this proxy's stale cache. + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "view-a": { mode: "code" }, + "view-b": { mode: "debug" }, + }) + + await provider.dispose() + }) + + it("should re-key durable viewStates entries from the temporary pre-launch view id", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + // A change made before the stable id is registered persists under the + // temporary id so it is not lost; registration re-keys it to the stable id. + await provider.saveViewState("mode", "architect") + + expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + [provider.viewId]: { mode: "architect" }, + }) + + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("mode", "debugger") + + const viewStates = provider.contextProxy.getValue("viewStates") as Record + expect(viewStates["stable-sidebar-view"]).toMatchObject({ mode: "debugger" }) + expect(viewStates[provider.viewId]).toBeUndefined() + + await provider.dispose() + }) + + it("should drop the temporary viewStates entry when a stable entry already exists", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + // A stable entry already exists (e.g. a previous session persisted under a + // colliding temporary id); it must win over the temporary entry. + await provider.contextProxy.setValue("viewStates", { + [provider.viewId]: { mode: "architect", updatedAt: 1 }, + "stable-sidebar-view": { mode: "debugger", updatedAt: 2 }, + }) + + await provider["setViewStateId"]("stable-sidebar-view") + + const viewStates = provider.contextProxy.getValue("viewStates") as Record + expect(viewStates["stable-sidebar-view"]).toMatchObject({ mode: "debugger" }) + expect(viewStates[provider.viewId]).toBeUndefined() + expect(provider["viewLocalState"].mode).toBe("debugger") + + await provider.dispose() + }) + + it("should discard a stale loadViewState when a newer view id is registered during the load", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const providerAccess = provider as unknown as { + viewId: string + viewLocalState: { mode?: string; currentApiConfigName?: string } + loadViewState(): Promise + setViewStateId(id: string): Promise + } + + // Seed persisted entries under both ids through the proxy so the loads + // observe them via the cached read path: the temporary entry holds a + // pre-registration selection, the stable entry the post-registration one. + await provider.contextProxy.setValue("viewStates", { + [providerAccess.viewId]: { mode: "architect", currentApiConfigName: "ghost-profile", updatedAt: 1 }, + "stable-sidebar-view": { mode: "debug", updatedAt: 2 }, + }) + + // Hang the temporary entry's profile lookup so that load is still in flight + // when the stable id is registered. + let releaseGhost!: () => void + const ghostLoad = new Promise((resolve) => { + releaseGhost = resolve + }) + vi.spyOn(provider.providerSettingsManager, "getProfile").mockReturnValue( + ghostLoad.then( + () => + ({ + name: "ghost-profile", + id: "ghost-id", + apiProvider: providerIdentifiers.anthropic, + }) as unknown as Awaited>, + ), + ) + + const staleLoad = providerAccess.loadViewState() + + // Register the stable id without awaiting its load: the re-key drops the + // temporary entry (the stable one already exists) and the registration's own + // load settles on the stable entry immediately. + const register = providerAccess.setViewStateId("stable-sidebar-view") + await register + + releaseGhost() + await staleLoad + + // The stale (temporary-id) load must not overwrite the stable id's load. + expect(providerAccess.viewLocalState).toEqual({ mode: "debug" }) + + await provider.dispose() + }) + }) + + describe("getState default values", () => { + it("should fall back to defaults for unset state values", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + const state = await provider.getState() + + expect(state.mode).toBe("code") + expect(state.currentApiConfigName).toBe("default") + expect(state.apiConfiguration.apiProvider).toBe(providerIdentifiers.anthropic) + expect(state.alwaysAllowReadOnly).toBe(false) + expect(state.alwaysAllowReadOnlyOutsideWorkspace).toBe(false) + expect(state.alwaysAllowWrite).toBe(false) + expect(state.alwaysAllowWriteOutsideWorkspace).toBe(false) + expect(state.alwaysAllowWriteProtected).toBe(false) + expect(state.alwaysAllowExecute).toBe(false) + expect(state.alwaysAllowMcp).toBe(false) + expect(state.alwaysAllowModeSwitch).toBe(false) + expect(state.alwaysAllowSubtasks).toBe(false) + expect(state.alwaysAllowFollowupQuestions).toBe(false) + expect(state.followupAutoApproveTimeoutMs).toBe(60000) + expect(state.diagnosticsEnabled).toBe(true) + expect(state.soundEnabled).toBe(false) + expect(state.ttsEnabled).toBe(false) + expect(state.ttsSpeed).toBe(1) + expect(state.enableCheckpoints).toBe(true) + expect(state.checkpointTimeout).toBe(DEFAULT_CHECKPOINT_TIMEOUT_SECONDS) + expect(state.terminalPowershellCounter).toBe(false) + expect(state.terminalZshClearEolMark).toBe(true) + expect(state.terminalZshOhMy).toBe(false) + expect(state.terminalZshP10k).toBe(false) + expect(state.terminalZdotdir).toBe(false) + expect(state.mcpEnabled).toBe(true) + expect(state.listApiConfigMeta).toEqual([]) + expect(state.pinnedApiConfigs).toEqual({}) + expect(state.modeApiConfigs).toEqual({}) + expect(state.customSupportPrompts).toEqual({}) + expect(state.experiments).toEqual(experimentDefault) + expect(state.autoApprovalEnabled).toBe(false) + expect(state.maxOpenTabsContext).toBe(20) + expect(state.maxWorkspaceFiles).toBe(200) + expect(state.telemetrySetting).toBe("unset") + expect(state.enableSubfolderRules).toBe(false) + expect(state.maxImageFileSize).toBe(5) + expect(state.maxTotalImageSize).toBe(20) + expect(state.historyPreviewCollapsed).toBe(false) + expect(state.reasoningBlockCollapsed).toBe(true) + expect(state.enterBehavior).toBe("send") + expect(state.codebaseIndexModels).toEqual(EMBEDDING_MODEL_PROFILES) + expect(state.codebaseIndexConfig).toEqual({ + codebaseIndexEnabled: false, + codebaseIndexQdrantUrl: "http://localhost:6333", + codebaseIndexEmbedderProvider: providerIdentifiers.openai, + codebaseIndexEmbedderBaseUrl: "", + codebaseIndexEmbedderModelId: "", + }) + expect(state.profileThresholds).toEqual({}) + expect(state.includeDiagnosticMessages).toBe(true) + expect(state.maxDiagnosticMessages).toBe(50) + expect(state.includeTaskHistoryInEnhance).toBe(true) + expect(state.includeCurrentTime).toBe(true) + expect(state.includeCurrentCost).toBe(true) + expect(state.maxGitStatusFiles).toBe(0) + expect(state.language).toBe("en") + + await provider.dispose() + }) + + it("should report a non-retired apiProvider from state instead of the anthropic fallback", async () => { + const contextProxy = new ContextProxy(mockContext) + await contextProxy.setValues({ apiProvider: providerIdentifiers.openrouter }) + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", contextProxy) + + const state = await provider.getState() + + expect(state.apiConfiguration.apiProvider).toBe(providerIdentifiers.openrouter) + + await provider.dispose() + }) + + it("should fill the apiConfiguration apiProvider from the raw state value when provider settings sanitize it away", async () => { + // "bogus-provider" is neither an active nor a retired provider, so + // ContextProxy.sanitizeProviderValues drops it from the provider + // settings; the raw state value still reaches apiConfiguration via + // the getState fill-in, which is what this assertion pins. + const contextProxy = new ContextProxy(mockContext) + const contextProxyAccess = contextProxy as unknown as { + setValues: (values: Record) => Promise + } + await contextProxyAccess.setValues({ apiProvider: "bogus-provider" }) + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", contextProxy) + + const state = await provider.getState() + + expect(state.apiConfiguration.apiProvider).toBe("bogus-provider") + + await provider.dispose() + }) + + it("should serve the embedding model profiles default when the stored value is cleared", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + + // The constructor seeds codebaseIndexModels into the context; with a truthy + // stored value the ?? default is unobservable (both ?? and && forms return + // the same profiles object). Clear the stored value so the read-time default + // is the one under test. + await provider.contextProxy.setValue("codebaseIndexModels", undefined) + + const state = await provider.getState() + + expect(state.codebaseIndexModels).toBe(EMBEDDING_MODEL_PROFILES) + + await provider.dispose() + }) + }) + describe("postStateToWebviewThrottled", () => { beforeEach(() => { vi.useFakeTimers() From 36247adbd0029989118c5d0ef27d4cfe64bfe18c Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Mon, 7 Sep 2026 21:01:32 +0800 Subject: [PATCH 15/18] fix(core/webview): purge deleted provider profile from the settings store deleteProviderProfile only rewrote the UI-facing listApiConfigMeta and currentApiConfigName in ContextProxy, leaving the profile's settings in the ProviderSettingsManager store (context.secrets). Per-mode mappings (modeApiConfigs) that still pointed at the deleted profile re-activated its stale settings on the next handleModeSwitch, clobbering the active configuration: the subtask child profile's gpt-4.1-mini leaked into ask-mode tasks, breaking downstream e2e suites (60s timeouts on search_files no-match and terminal reuse after zero-chunk shell race). Purge the profile from the store on delete so dangling mode mappings can no longer resolve it: listConfig().find(id) fails and handleModeSwitch continues with the current configuration. The F3 mode/profile isolation commit further up the chain introduces the same purge plus per-view pin handling. Regression test: sticky-profile spec "deleteProviderProfile removes the stored profile so a dangling mode mapping can no longer re-activate it". --- src/core/webview/ClineProvider.ts | 4 ++ .../ClineProvider.sticky-profile.spec.ts | 55 +++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6a146e3e78..e932530ed5 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2323,6 +2323,10 @@ export class ClineProvider throw new Error("You cannot delete the last profile") } + // Remove the profile from the settings store (context.secrets) so it cannot be + // resurrected by a later listApiConfigMeta sync. + await this.providerSettingsManager.deleteConfig(profileToDelete.name) + const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) // Write only the profile list back: replaying the full settings snapshot diff --git a/src/core/webview/__tests__/ClineProvider.sticky-profile.spec.ts b/src/core/webview/__tests__/ClineProvider.sticky-profile.spec.ts index 7d8493fba3..b92c197b61 100644 --- a/src/core/webview/__tests__/ClineProvider.sticky-profile.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.sticky-profile.spec.ts @@ -1015,4 +1015,59 @@ describe("ClineProvider - Sticky Provider Profile", () => { ) }) }) + + describe("deleteProviderProfile", () => { + it("removes the stored profile so a dangling mode mapping can no longer re-activate it", async () => { + await provider.resolveWebviewView(mockWebviewView) + + // Seed the stored profile settings: the default profile plus a child + // profile (mirroring the cross-profile subtasks e2e scenario), with the + // "ask" mode mapped to the child profile. + const defaultId = await provider.providerSettingsManager.saveConfig("default", { + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "mock-key", + openRouterModelId: "openai/gpt-4.1", + }) + const childId = await provider.providerSettingsManager.saveConfig("subtask-child-profile", { + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "mock-key", + openRouterModelId: "openai/gpt-4.1-mini", + }) + await provider.providerSettingsManager.setModeConfig("ask", childId) + + // The UI-facing list mirrors the store, as maintained by upsert/activate. + await provider.contextProxy.setValues({ + listApiConfigMeta: [ + { name: "default", id: defaultId, apiProvider: providerIdentifiers.openrouter }, + { name: "subtask-child-profile", id: childId, apiProvider: providerIdentifiers.openrouter }, + ], + currentApiConfigName: "subtask-child-profile", + }) + + await provider.deleteProviderProfile({ + name: "subtask-child-profile", + id: childId, + apiProvider: providerIdentifiers.openrouter, + }) + + // The deleted profile's settings are gone from the manager store. + const remaining = await provider.providerSettingsManager.listConfig() + expect(remaining.some((config) => config.name === "subtask-child-profile")).toBe(false) + await expect( + provider.providerSettingsManager.getProfile({ name: "subtask-child-profile" }), + ).rejects.toThrow() + + // The mode mapping still points at the deleted id, but it no longer + // resolves to a stored profile, so handleModeSwitch falls through to the + // current configuration instead of re-activating the deleted profile. + const savedConfigId = await provider.providerSettingsManager.getModeConfigId("ask") + expect(savedConfigId).toBe(childId) + expect(remaining.find(({ id }) => id === savedConfigId)).toBeUndefined() + + // The context was repointed at the surviving profile. + const values = provider.contextProxy.getValues() + expect(values.currentApiConfigName).toBe("default") + expect(values.listApiConfigMeta?.map((entry) => entry.name)).toEqual(["default"]) + }) + }) }) From 7f8c5f90f3d69512d5963631f9e4f3432651be6c Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 01:28:53 +0800 Subject: [PATCH 16/18] fix(core/webview): treat already-gone profile secrets as prunable on delete --- src/core/webview/ClineProvider.ts | 18 ++++- .../ClineProvider.sticky-profile.spec.ts | 69 ++++++++++++++++++- 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index e932530ed5..f9a51bbee1 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2324,8 +2324,22 @@ export class ClineProvider } // Remove the profile from the settings store (context.secrets) so it cannot be - // resurrected by a later listApiConfigMeta sync. - await this.providerSettingsManager.deleteConfig(profileToDelete.name) + // resurrected by a later listApiConfigMeta sync. A "not found" rejection means + // the secret was already gone (e.g. pruned by an earlier run): treat it as an + // idempotent success so the stale list entry below is still pruned, while any + // other failure (e.g. refusing to delete the last remaining configuration) + // propagates. + try { + await this.providerSettingsManager.deleteConfig(profileToDelete.name) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + if (!message.includes("not found")) { + throw error + } + this.log( + `deleteProviderProfile: settings for '${profileToDelete.name}' were not found; pruning the stale list entry only`, + ) + } const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) diff --git a/src/core/webview/__tests__/ClineProvider.sticky-profile.spec.ts b/src/core/webview/__tests__/ClineProvider.sticky-profile.spec.ts index b92c197b61..306602ad14 100644 --- a/src/core/webview/__tests__/ClineProvider.sticky-profile.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.sticky-profile.spec.ts @@ -1055,7 +1055,7 @@ describe("ClineProvider - Sticky Provider Profile", () => { expect(remaining.some((config) => config.name === "subtask-child-profile")).toBe(false) await expect( provider.providerSettingsManager.getProfile({ name: "subtask-child-profile" }), - ).rejects.toThrow() + ).rejects.toThrow(/subtask-child-profile.*not found/) // The mode mapping still points at the deleted id, but it no longer // resolves to a stored profile, so handleModeSwitch falls through to the @@ -1069,5 +1069,72 @@ describe("ClineProvider - Sticky Provider Profile", () => { expect(values.currentApiConfigName).toBe("default") expect(values.listApiConfigMeta?.map((entry) => entry.name)).toEqual(["default"]) }) + + it("treats an already-gone secret as success so the stale list entry is still pruned", async () => { + await provider.resolveWebviewView(mockWebviewView) + + // Only the default profile exists in the store: the ghost profile's secret + // was already gone (e.g. pruned by an earlier run) but its list entry + // survived. + const defaultId = await provider.providerSettingsManager.saveConfig("default", { + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "mock-key", + openRouterModelId: "openai/gpt-4.1", + }) + await provider.contextProxy.setValues({ + listApiConfigMeta: [ + { name: "default", id: defaultId, apiProvider: providerIdentifiers.openrouter }, + { name: "ghost-profile", id: "ghost-id", apiProvider: providerIdentifiers.openrouter }, + ], + currentApiConfigName: "ghost-profile", + }) + + // The manager's "not found" rejection must not surface to the caller ... + await expect( + provider.deleteProviderProfile({ + name: "ghost-profile", + id: "ghost-id", + apiProvider: providerIdentifiers.openrouter, + }), + ).resolves.not.toThrow() + + // ... it prunes the stale list entry and repoints the selection. + const values = provider.contextProxy.getValues() + expect(values.currentApiConfigName).toBe("default") + expect(values.listApiConfigMeta?.map((entry) => entry.name)).toEqual(["default"]) + }) + + it("still refuses to delete the last stored profile when a stale list entry remains", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const defaultId = await provider.providerSettingsManager.saveConfig("default", { + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "mock-key", + openRouterModelId: "openai/gpt-4.1", + }) + // A ghost list entry survives next to the only real profile, so the + // provider-level "last profile" guard does not fire: the refusal must come + // from the settings store itself. + await provider.contextProxy.setValues({ + listApiConfigMeta: [ + { name: "default", id: defaultId, apiProvider: providerIdentifiers.openrouter }, + { name: "ghost-profile", id: "ghost-id", apiProvider: providerIdentifiers.openrouter }, + ], + currentApiConfigName: "default", + }) + + await expect( + provider.deleteProviderProfile({ + name: "default", + id: defaultId, + apiProvider: providerIdentifiers.openrouter, + }), + ).rejects.toThrow("Cannot delete the last remaining configuration") + + // Nothing was repointed or pruned. + const values = provider.contextProxy.getValues() + expect(values.currentApiConfigName).toBe("default") + expect(values.listApiConfigMeta?.map((entry) => entry.name)).toEqual(["default", "ghost-profile"]) + }) }) }) From a440db18556dbdc6c6f30fef79d7fdc5c91d0be4 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 09:33:16 +0800 Subject: [PATCH 17/18] fix(webview): sync view-local mode buffer on mode switch handleModeSwitchUnlocked persisted the switched mode only through the deprecated updateGlobalState, so the in-memory viewLocalState buffer kept serving a stale restored mode: getValues() merges viewLocalState on top of the ContextProxy values and would shadow the fresh switch for consumers. Route the write through setValue so the buffer and the durable global state stay in sync, and cover it with a regression test for switching after a restored view state. Also assert the restored mode in public state, and harden the import viewStates test with a write-tracking proxy across all write paths (setValues/setValue/setProviderSettings) for seeded and fresh machines. --- .../config/__tests__/importExport.spec.ts | 50 ++++++++++++++++++- .../ClineProvider.sticky-mode.spec.ts | 22 +++++++- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index c15c103be7..c161c266f0 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -364,17 +364,63 @@ describe("importExport", () => { { name: "default", id: "default-id", apiProvider: providerIdentifiers.anthropic }, ]) + // Stateful write-tracking proxy: every write path (setValues, setValue, + // setProviderSettings) is merged into `state` and recorded in `writes`, + // so the assertions below cover every payload rather than one call. + // Plain-function doubles cast once, matching the file-level mock pattern. + const makeStatefulProxy = (seed: Record) => { + const state: Record = { ...seed } + const writes: Record[] = [] + const record = (values: Record) => { + writes.push(values) + Object.assign(state, values) + } + return { + state, + writes, + proxy: { + setValues: vi.fn(async (values: Record) => record(values)), + setValue: vi.fn(async (key: string, value: unknown) => record({ [key]: value })), + setProviderSettings: vi.fn(async (settings: Record) => record(settings)), + } as unknown as ReturnType>, + } + } + + // This machine already has view state: it must survive the import untouched. + const existingViewStates = { + "existing-view": { mode: "code", currentApiConfigName: "default", updatedAt: 0 }, + } + const seeded = makeStatefulProxy({ viewStates: existingViewStates }) const result = await importSettingsFromPath("/mock/path/settings.json", { providerSettingsManager: mockProviderSettingsManager, - contextProxy: mockContextProxy, + contextProxy: seeded.proxy, customModesManager: mockCustomModesManager, }) expect(result.success).toBe(true) // Per-view selection state is machine-local: importing settings must not // apply another machine's view pins, while other settings round-trip. - expect(mockContextProxy.setValues).toHaveBeenCalledWith({ mode: "code" }) + expect(seeded.state.viewStates).toEqual(existingViewStates) + expect(seeded.state.mode).toBe("code") expect(result).not.toHaveProperty("globalSettings.viewStates") + + // A machine without view state must not gain any from the import. + const fresh = makeStatefulProxy({}) + const freshResult = await importSettingsFromPath("/mock/path/settings.json", { + providerSettingsManager: mockProviderSettingsManager, + contextProxy: fresh.proxy, + customModesManager: mockCustomModesManager, + }) + + expect(freshResult.success).toBe(true) + expect(fresh.state).not.toHaveProperty("viewStates") + expect(fresh.state.mode).toBe("code") + expect(freshResult).not.toHaveProperty("globalSettings.viewStates") + + // No write path may carry the imported machine's view pins. + for (const payload of [...seeded.writes, ...fresh.writes]) { + expect(payload).not.toHaveProperty("viewStates") + } }) it("should return success: false when file content is invalid", async () => { diff --git a/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts b/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts index 414c368aad..e8245078ec 100644 --- a/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts @@ -452,6 +452,24 @@ describe("ClineProvider - Sticky Mode", () => { }), ) }) + + it("should sync the view-local mode buffer when switching modes after a restored view state", async () => { + // Simulate the history-restore path: saveViewState is what + // createTaskWithHistoryItem uses to pin a saved mode into the + // view-local buffer, leaving a stale mode there until the next mutation. + await provider.saveViewState("mode", "code") + + // Global-only mode switch with no active task. + await provider.handleModeSwitch("architect") + + // The durable global write still happens... + expect(mockContext.globalState.update).toHaveBeenCalledWith("mode", "architect") + + // ...and the in-memory buffer must not keep serving the stale restored + // mode: getValues() merges viewLocalState on top of the ContextProxy + // values, so an unsynced buffer would hide the fresh mode from consumers. + expect(provider["viewLocalState"].mode).toBe("architect") + }) }) describe("createTaskWithHistoryItem", () => { @@ -761,11 +779,11 @@ describe("ClineProvider - Sticky Mode", () => { // Restore the task from history await provider.createTaskWithHistoryItem(historyItem) - // Verify that the mode was restored into this view's durable pin. The - // getState() merge of hydrated per-view values lands with the F1b follow-up. + // Verify that history restoration reaches both the view-local pin and public state. expect(provider["viewLocalState"].mode).toBe("architect") const state = await provider.getState() + expect(state.mode).toBe("architect") // Verify that the API configuration was also restored expect(state.currentApiConfigName).toBe("architect-config") From 31a1d80c41e1ca351f5bf194618b7410c71ef206 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 8 Sep 2026 11:28:05 +0800 Subject: [PATCH 18/18] fix(config): branch profile deletion on a typed not-found signal and isolate launch-suite provider doubles ProviderSettingsManager.deleteConfig now throws ProviderSettingsNotFoundError for a missing config and rethrows it unwrapped, so ClineProvider.deleteProviderProfile branches on the type instead of matching the not-found message text that a profile name could spoof; the sticky-mode handleModeSwitch test additionally pins the durable per-view persisted mode and getValues(); the webviewDidLaunch tests restore the mockClineProvider members they replace after each test so launch stubs cannot leak. --- src/core/config/ProviderSettingsManager.ts | 20 ++++++- .../__tests__/ProviderSettingsManager.spec.ts | 12 +++- src/core/webview/ClineProvider.ts | 17 +++--- .../webview/__tests__/ClineProvider.spec.ts | 60 +++++++++++++++++++ .../ClineProvider.sticky-mode.spec.ts | 7 +++ .../__tests__/webviewMessageHandler.spec.ts | 28 +++++++++ 6 files changed, 134 insertions(+), 10 deletions(-) diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 51f79cff35..73424ba3c9 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -52,6 +52,19 @@ export const providerProfilesSchema = z.object({ export type ProviderProfiles = z.infer +/** + * Signals that a profile's configuration no longer exists. Callers that treat an + * already-deleted profile as an idempotent no-op branch on this type instead of + * matching error message text, which a profile name containing the phrase could + * otherwise spoof. + */ +export class ProviderSettingsNotFoundError extends Error { + constructor(message: string) { + super(message) + this.name = "ProviderSettingsNotFoundError" + } +} + export class ProviderSettingsManager { private static readonly SCOPE_PREFIX = "roo_cline_config_" private readonly defaultConfigId = this.generateId() @@ -474,7 +487,7 @@ export class ProviderSettingsManager { const providerProfiles = await this.load() if (!providerProfiles.apiConfigs[name]) { - throw new Error(`Config '${name}' not found`) + throw new ProviderSettingsNotFoundError(`Config '${name}' not found`) } if (Object.keys(providerProfiles.apiConfigs).length === 1) { @@ -485,6 +498,11 @@ export class ProviderSettingsManager { await this.store(providerProfiles) }) } catch (error) { + // A missing config is a caller-meaningful signal, not a failure: rethrow it + // unwrapped so callers can branch on the type instead of message text. + if (error instanceof ProviderSettingsNotFoundError) { + throw error + } throw new Error(`Failed to delete config: ${error}`) } } diff --git a/src/core/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index be0cbfec92..6f9aeb4d0f 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -11,7 +11,12 @@ import { import { clearAllMocks } from "../../../test-utils/reset" import { makeExtensionContext } from "../../../test-utils/vscode" -import { ProviderSettingsManager, ProviderProfiles, SyncCloudProfilesResult } from "../ProviderSettingsManager" +import { + ProviderSettingsManager, + ProviderSettingsNotFoundError, + ProviderProfiles, + SyncCloudProfilesResult, +} from "../ProviderSettingsManager" // `export()` builds an API handler per profile to read model capabilities. Mock // buildApiHandler with the real @roo-code/types model definitions so the token-field @@ -706,6 +711,11 @@ describe("ProviderSettingsManager", () => { }), ) + // The typed not-found signal is the contract callers branch on: a profile + // name containing "not found" must not be matchable via message text. + await expect(providerSettingsManager.deleteConfig("nonexistent")).rejects.toBeInstanceOf( + ProviderSettingsNotFoundError, + ) await expect(providerSettingsManager.deleteConfig("nonexistent")).rejects.toThrow( "Config 'nonexistent' not found", ) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index f9a51bbee1..0292ebca48 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -105,7 +105,7 @@ import { buildApiHandler } from "../../api" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/providers/fetchers/lmstudio" import { ContextProxy } from "../config/ContextProxy" -import { ProviderSettingsManager } from "../config/ProviderSettingsManager" +import { ProviderSettingsManager, ProviderSettingsNotFoundError } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" import { Task } from "../task/Task" @@ -2324,16 +2324,17 @@ export class ClineProvider } // Remove the profile from the settings store (context.secrets) so it cannot be - // resurrected by a later listApiConfigMeta sync. A "not found" rejection means - // the secret was already gone (e.g. pruned by an earlier run): treat it as an - // idempotent success so the stale list entry below is still pruned, while any - // other failure (e.g. refusing to delete the last remaining configuration) - // propagates. + // resurrected by a later listApiConfigMeta sync. A not-found rejection means + // the secret was already gone (e.g. pruned by an earlier run): branch on the + // typed ProviderSettingsNotFoundError so the stale list entry below is still + // pruned as an idempotent success, while any other failure (e.g. refusing to + // delete the last remaining configuration) propagates. Matching message text + // instead would let a profile whose name contains "not found" swallow an + // unrelated failure. try { await this.providerSettingsManager.deleteConfig(profileToDelete.name) } catch (error) { - const message = error instanceof Error ? error.message : String(error) - if (!message.includes("not found")) { + if (!(error instanceof ProviderSettingsNotFoundError)) { throw error } this.log( diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 5f178370af..586bddea14 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -27,6 +27,7 @@ import { experimentDefault } from "../../../shared/experiments" import { EMBEDDING_MODEL_PROFILES } from "../../../shared/embeddingModels" import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" +import { ProviderSettingsNotFoundError } from "../../config/ProviderSettingsManager" import { Task, TaskOptions } from "../../task/Task" import { safeWriteJson } from "../../../utils/safeWriteJson" import { t } from "../../../i18n" @@ -1911,6 +1912,65 @@ describe("ClineProvider", () => { expect(provider.contextProxy.getValue("currentApiConfigName")).toBe("keeper-profile") await provider.dispose() }) + + it("should swallow only the typed not-found signal when pruning a stale profile entry", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const staleProfile: ProviderSettingsEntry = { + name: "stale-profile", + id: "stale-id", + apiProvider: providerIdentifiers.openrouter, + } + const keeperProfile: ProviderSettingsEntry = { + name: "keeper-profile", + id: "keeper-id", + apiProvider: providerIdentifiers.anthropic, + } + await provider.contextProxy.setValue("listApiConfigMeta", [staleProfile, keeperProfile]) + await provider.setValue("currentApiConfigName", "keeper-profile") + vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + // The secret was already pruned: the typed not-found must be an idempotent + // success so the stale list entry is still removed. + vi.spyOn(provider.providerSettingsManager, "deleteConfig").mockRejectedValue( + new ProviderSettingsNotFoundError(`Config 'stale-profile' not found`), + ) + + await provider.deleteProviderProfile(staleProfile) + + expect(provider.contextProxy.getValue("listApiConfigMeta")).toEqual([keeperProfile]) + await provider.dispose() + }) + + it("should propagate a non-not-found deletion failure for a profile named like the not-found message", async () => { + const provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) + const profile: ProviderSettingsEntry = { + name: "not found config", + id: "nf-id", + apiProvider: providerIdentifiers.openrouter, + } + const keeperProfile: ProviderSettingsEntry = { + name: "keeper-profile", + id: "keeper-id", + apiProvider: providerIdentifiers.anthropic, + } + await provider.contextProxy.setValue("listApiConfigMeta", [profile, keeperProfile]) + await provider.setValue("currentApiConfigName", "keeper-profile") + vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + // An unrelated failure (wrapped the way deleteConfig wraps storage errors) + // must not be mistaken for the idempotent not-found path just because the + // profile name contains "not found". + const deleteConfigSpy = vi + .spyOn(provider.providerSettingsManager, "deleteConfig") + .mockRejectedValue( + new Error(`Failed to delete config: Error: storage write failed for 'not found config'`), + ) + + await expect(provider.deleteProviderProfile(profile)).rejects.toThrow("storage write failed") + + // The list entry must remain untouched when the deletion failed. + expect(provider.contextProxy.getValue("listApiConfigMeta")).toEqual([profile, keeperProfile]) + expect(deleteConfigSpy).toHaveBeenCalledTimes(1) + await provider.dispose() + }) }) describe("local state isolation", () => { diff --git a/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts b/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts index e8245078ec..dd095e667b 100644 --- a/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts @@ -469,6 +469,13 @@ describe("ClineProvider - Sticky Mode", () => { // mode: getValues() merges viewLocalState on top of the ContextProxy // values, so an unsynced buffer would hide the fresh mode from consumers. expect(provider["viewLocalState"].mode).toBe("architect") + + // The durable per-view write must land too: a regression that left the + // persisted entry on the stale restored mode would reload it on restart. + // setValue awaits the serialized write queue, so the entry is settled here. + const persisted = provider["getPersistedViewStates"]()[provider["viewStateId"]] + expect(persisted.mode).toBe("architect") + expect(provider.getValues().mode).toBe("architect") }) }) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 4c2a301965..99979608f7 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -2104,6 +2104,23 @@ describe("webviewMessageHandler - telemetrySetting", () => { expect(calls.at(-1)).toEqual([true]) }) + // The webviewDidLaunch tests below replace these mockClineProvider members with + // per-test doubles. Snapshot the module-level originals at collection time and + // restore them in the afterEach below so the launch stubs never leak into other + // tests of this file. + const launchSuiteSnapshot = (() => { + const view = mockClineProvider as unknown as { + getMcpHub: unknown + providerSettingsManager: unknown + getStateToPostToWebview: unknown + } + return { + getMcpHub: view.getMcpHub, + providerSettingsManager: view.providerSettingsManager, + getStateToPostToWebview: view.getStateToPostToWebview, + } + })() + // CodeRabbit follow-up on the finding #12 fix: webviewDidLaunch's telemetry init read state // via an async provider.getStateToPostToWebview().then(...) continuation, outside // telemetrySettingQueue -- so it could resolve after a concurrent "telemetrySetting" message @@ -2272,4 +2289,15 @@ describe("webviewMessageHandler - telemetrySetting", () => { expect(TelemetryService.instance.updateTelemetryState).not.toHaveBeenCalled() }) + + afterEach(() => { + const view = mockClineProvider as unknown as { + getMcpHub: unknown + providerSettingsManager: unknown + getStateToPostToWebview: unknown + } + view.getMcpHub = launchSuiteSnapshot.getMcpHub + view.providerSettingsManager = launchSuiteSnapshot.providerSettingsManager + view.getStateToPostToWebview = launchSuiteSnapshot.getStateToPostToWebview + }) })