Skip to content

Commit e0e540e

Browse files
easonliang28easonliang28
committed
fix(webview): refresh pinned views on non-activating saves; treat the view overlay as a complete profile
Two review findings on the view-local profile buffer: 1. A save that does not activate (upsertProviderProfile with activate=false, and the zoo-gateway callback's non-active profile path) rewrote the profile's settings on disk but left every view pinned to it serving the pre-save snapshot from its buffer. The non-activating branch now refreshes pinned sibling views (and this view when it holds the pin), and the callback routes non-active profiles through upsertProviderProfile instead of a bare saveConfig so the refresh cannot be forgotten at that call site. 2. getState() spread the pinned overlay over the shared provider settings, so any key the pinned profile never defined leaked in from whatever profile the shared store points at (another profile's model id, region, tuning). The overlay is a complete profile snapshot: it is now used in place, with only apiProvider falling back to the shared state when the profile omits one. Proofs: ClineProvider.spec 283/283 (2 new tests: pinned-sibling refresh on non-activating save; no shared-key leak through the overlay; callback test updated to assert the upsert path); negative controls: dropping the else-branch refresh reddens exactly the refresh test, reverting the merge semantics reddens exactly the leak test, restores byte-exact (hash match); eslint --max-warnings=0 clean; tsc clean except the two known local telemetry-dist noise lines; prettier clean on touched files.
1 parent 8c9c55f commit e0e540e

2 files changed

Lines changed: 141 additions & 10 deletions

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2411,6 +2411,15 @@ export class ClineProvider
24112411
await this.persistStickyProviderProfileToCurrentTask(name)
24122412
} else {
24132413
await this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig())
2414+
2415+
// A save that does not activate still changes the profile's settings, and
2416+
// every view pinned to it buffers a full copy of them. Refresh those views
2417+
// (and this one when it is the pinned view) or their getState() keeps
2418+
// serving the settings captured before the save under the same name.
2419+
if (this.pinnedProfileName === name) {
2420+
await this._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings })
2421+
}
2422+
await this.refreshViewLocalStateForUpdatedProfile(name, providerSettings)
24142423
}
24152424

24162425
await this.postStateToWebview()
@@ -2842,8 +2851,10 @@ export class ClineProvider
28422851
// picks up the new token immediately for the current task.
28432852
await this.upsertProviderProfile(entry.name, updated, true)
28442853
} else {
2845-
// Non-active profiles just need the token saved to disk.
2846-
await this.providerSettingsManager.saveConfig(entry.name, updated)
2854+
// Non-active profiles need the token saved to disk and the views pinned
2855+
// to them refreshed: a direct saveConfig leaves a pinned view serving the
2856+
// old token until it reloads.
2857+
await this.upsertProviderProfile(entry.name, updated, false)
28472858
}
28482859
}
28492860
}
@@ -3656,11 +3667,21 @@ export class ClineProvider
36563667
const taskSyncEnabled: boolean = false
36573668

36583669
// Return the same structure as before.
3670+
// A view-local profile is a complete snapshot, not a patch over the shared store.
3671+
// Spreading it on top of providerSettings would let a key the profile never defined
3672+
// leak in from another profile, which ContextProxy.setProviderSettings deliberately
3673+
// prevents by clearing omitted keys. Use the view's own settings in place and take
3674+
// only apiProvider from the shared state when the profile does not carry one.
3675+
const viewApiConfiguration = mergedStateValues.apiConfiguration
3676+
const resolvedApiConfiguration = viewApiConfiguration
3677+
? {
3678+
...viewApiConfiguration,
3679+
apiProvider: viewApiConfiguration.apiProvider ?? providerSettings.apiProvider,
3680+
}
3681+
: providerSettings
3682+
36593683
return {
3660-
apiConfiguration: {
3661-
...providerSettings,
3662-
...mergedStateValues.apiConfiguration,
3663-
},
3684+
apiConfiguration: resolvedApiConfiguration,
36643685
lastShownAnnouncementId: mergedStateValues.lastShownAnnouncementId,
36653686
customInstructions: mergedStateValues.customInstructions,
36663687
apiModelId: mergedStateValues.apiModelId,

‎src/core/webview/__tests__/ClineProvider.spec.ts‎

Lines changed: 114 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,13 @@ describe("ClineProvider", () => {
500500
let mockPostMessage: any
501501
let updateGlobalStateSpy: any
502502

503+
afterEach(() => {
504+
// Tests construct providers directly and many never dispose them; clear the
505+
// static registry so a leaked instance cannot claim stable view state ids
506+
// ahead of a later test.
507+
ClineProvider["activeInstances"] = new Set()
508+
})
509+
503510
beforeEach(() => {
504511
vi.clearAllMocks()
505512

@@ -2129,6 +2136,10 @@ describe("ClineProvider", () => {
21292136
})
21302137
// @ts-ignore - Replace providerSettingsManager with a test double for the profile lookup.
21312138
provider.providerSettingsManager = { getProfile: getProfileSpy }
2139+
// The ownership check requires the writer to release the id before the
2140+
// next view adopts it; the durable entry it wrote stays in storage.
2141+
await writer.dispose()
2142+
21322143
await provider.contextProxy.setValue(
21332144
"viewStates",
21342145
mockContext.globalState.get<RooCodeSettings["viewStates"]>("viewStates"),
@@ -2141,7 +2152,6 @@ describe("ClineProvider", () => {
21412152
})
21422153
expect(getProfileSpy).toHaveBeenCalledWith({ name: "my-profile" })
21432154
expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId"))
2144-
await writer.dispose()
21452155
await provider.dispose()
21462156
})
21472157

@@ -2269,6 +2279,10 @@ describe("ClineProvider", () => {
22692279
}
22702280
// A pre-load buffer write that was never persisted must not be merged over the load.
22712281
provider["viewLocalState"] = { mode: "architect" }
2282+
// The ownership check requires the writer to release the id before the
2283+
// next view adopts it; the durable entry it wrote stays in storage.
2284+
await writer.dispose()
2285+
22722286
await provider.contextProxy.setValue(
22732287
"viewStates",
22742288
mockContext.globalState.get<RooCodeSettings["viewStates"]>("viewStates"),
@@ -2280,7 +2294,6 @@ describe("ClineProvider", () => {
22802294
apiProvider: providerIdentifiers.openrouter,
22812295
openRouterModelId: "model-x",
22822296
})
2283-
await writer.dispose()
22842297
await provider.dispose()
22852298
})
22862299

@@ -2671,6 +2684,10 @@ describe("ClineProvider", () => {
26712684
}),
26722685
}
26732686
vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined)
2687+
// The ownership check requires the writer to release the id before the
2688+
// next view adopts it; the durable entry it wrote stays in storage.
2689+
await writer.dispose()
2690+
26742691
await provider.contextProxy.setValue(
26752692
"viewStates",
26762693
mockContext.globalState.get<RooCodeSettings["viewStates"]>("viewStates"),
@@ -2683,7 +2700,6 @@ describe("ClineProvider", () => {
26832700
// The buffer must track the activated profile so getValues() agrees with the proxy.
26842701
expect(provider.getValues().currentApiConfigName).toBe("new-profile")
26852702
expect(provider.contextProxy.getValue("currentApiConfigName")).toBe("new-profile")
2686-
await writer.dispose()
26872703
await provider.dispose()
26882704
})
26892705

@@ -6563,6 +6579,11 @@ describe("getTelemetryProperties", () => {
65636579
const { CloudService } = await import("@roo-code/cloud")
65646580
const mockCloudService = {
65656581
isAuthenticated: vi.fn().mockReturnValue(true),
6582+
6583+
// Provider dispose detaches the settings listener; later tests construct
6584+
// and dispose providers against this surviving instance getter.
6585+
on: vi.fn(),
6586+
off: vi.fn(),
65666587
}
65676588

65686589
// Update the existing mock
@@ -6581,6 +6602,11 @@ describe("getTelemetryProperties", () => {
65816602
const { CloudService } = await import("@roo-code/cloud")
65826603
const mockCloudService = {
65836604
isAuthenticated: vi.fn().mockReturnValue(false),
6605+
6606+
// Provider dispose detaches the settings listener; later tests construct
6607+
// and dispose providers against this surviving instance getter.
6608+
on: vi.fn(),
6609+
off: vi.fn(),
65846610
}
65856611

65866612
// Update the existing mock
@@ -6622,6 +6648,11 @@ describe("getTelemetryProperties", () => {
66226648
isAuthenticated: vi.fn().mockImplementation(() => {
66236649
throw new Error("Authentication check error")
66246650
}),
6651+
6652+
// Provider dispose detaches the settings listener; later tests construct
6653+
// and dispose providers against this surviving instance getter.
6654+
on: vi.fn(),
6655+
off: vi.fn(),
66256656
}
66266657

66276658
// Update the existing mock
@@ -8126,12 +8157,15 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
81268157
}),
81278158
true,
81288159
)
8129-
expect(saveConfig).toHaveBeenCalledWith(
8160+
// The non-active profile now goes through upsertProviderProfile so the views
8161+
// pinned to it are refreshed, not just the file on disk.
8162+
expect(upsertSpy).toHaveBeenCalledWith(
81308163
"Backup Zoo",
81318164
expect.objectContaining({
81328165
zooSessionToken: "new-token",
81338166
zooGatewayBaseUrl: "https://www.zoocode.dev/api/gateway/v1",
81348167
}),
8168+
false,
81358169
)
81368170
})
81378171

@@ -8228,4 +8262,80 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
82288262
})
82298263
})
82308264
})
8265+
8266+
describe("profile save without activation and overlay completeness", () => {
8267+
it("refreshes views pinned to a profile saved without activation", async () => {
8268+
// Defensive: the doubles in this suite can leave console writes pending
8269+
// at worker teardown.
8270+
vi.spyOn(console, "log").mockImplementation(() => {})
8271+
vi.spyOn(console, "warn").mockImplementation(() => {})
8272+
vi.spyOn(console, "error").mockImplementation(() => {})
8273+
const sibling = new ClineProvider(
8274+
mockContext,
8275+
mockOutputChannel,
8276+
"editor",
8277+
new ContextProxy(mockContext),
8278+
new WebviewFocusTracker(),
8279+
)
8280+
try {
8281+
// The sibling loaded the profile earlier: its buffer pins the name and
8282+
// carries the settings snapshot taken at that load.
8283+
sibling["viewLocalState"].currentApiConfigName = "saved-profile"
8284+
sibling["viewLocalState"].apiConfiguration = {
8285+
apiProvider: providerIdentifiers.anthropic,
8286+
apiKey: "old-key",
8287+
}
8288+
vi.spyOn(sibling, "postStateToWebview").mockResolvedValue(undefined)
8289+
vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined)
8290+
// @ts-ignore - Replace providerSettingsManager with a test double.
8291+
provider.providerSettingsManager = {
8292+
saveConfig: vi.fn().mockResolvedValue("saved-id"),
8293+
listConfig: vi.fn().mockResolvedValue([]),
8294+
setModeConfig: vi.fn(),
8295+
}
8296+
8297+
await provider.upsertProviderProfile(
8298+
"saved-profile",
8299+
{ apiProvider: providerIdentifiers.anthropic, apiKey: "new-key" },
8300+
false,
8301+
)
8302+
8303+
// A save that does not activate still rewrites the profile; the pinned
8304+
// sibling must not keep serving the pre-save snapshot.
8305+
expect(sibling["viewLocalState"].apiConfiguration).toEqual({
8306+
apiProvider: providerIdentifiers.anthropic,
8307+
apiKey: "new-key",
8308+
})
8309+
} finally {
8310+
await sibling.dispose()
8311+
}
8312+
})
8313+
8314+
it("does not leak shared provider keys into a pinned view's api configuration", async () => {
8315+
vi.spyOn(console, "log").mockImplementation(() => {})
8316+
vi.spyOn(console, "warn").mockImplementation(() => {})
8317+
// The shared store sits on a profile with keys the pinned profile never
8318+
// sets; spreading the overlay over the shared keys would report a mixed
8319+
// configuration (e.g. another profile's model id and region).
8320+
await provider.contextProxy.setValue("apiProvider", providerIdentifiers.bedrock)
8321+
await provider.contextProxy.setValue("apiModelId", "anthropic.claude-v1")
8322+
await provider.contextProxy.setValue("awsRegion", "us-east-1")
8323+
provider["viewLocalState"].currentApiConfigName = "pinned-profile"
8324+
provider["viewLocalState"].apiConfiguration = {
8325+
apiProvider: providerIdentifiers.anthropic,
8326+
apiKey: "k",
8327+
}
8328+
8329+
// The cloud doubles in this suite make getState() log its defensive catch
8330+
// branches; silence them so teardown has no pending console writes.
8331+
vi.spyOn(console, "error").mockImplementation(() => {})
8332+
8333+
const state = await provider.getState()
8334+
8335+
expect(state.apiConfiguration.apiProvider).toBe(providerIdentifiers.anthropic)
8336+
expect(state.apiConfiguration.apiKey).toBe("k")
8337+
expect(state.apiConfiguration.apiModelId).toBeUndefined()
8338+
expect(state.apiConfiguration.awsRegion).toBeUndefined()
8339+
})
8340+
})
82318341
})

0 commit comments

Comments
 (0)