Skip to content

Commit 8fbce54

Browse files
easonliang28easonliang28
committed
fix(webview): keep committed mode switches out of the reconcile rollback
The mode-switch reconcile rolled the mode back even when the mode-linked activation had already written shared state (profile, provider settings, mode mapping, sticky task profile): a later failure then paired the old mode with the new profile, the very mismatch the reconcile exists to prevent. The catch now keeps the switch once activation has started writing, and also when the mutation signal has aborted - the queue has advanced past the timed-out switch, so a newer mutation may own the mode stores. The trailing state post moved outside the reconciled region: a UI-refresh failure after a fully committed switch is logged, not rolled back. Three regression tests included.
1 parent c40a67a commit 8fbce54

4 files changed

Lines changed: 217 additions & 57 deletions

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 57 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -2255,6 +2255,11 @@ export class ClineProvider
22552255
const historyItemBefore = taskHistoryItem
22562256
rollbackTaskWrites = async () => {
22572257
task["_taskMode"] = previousTaskMode
2258+
// Listeners were told about the new mode before the failure landed: re-emit
2259+
// with the restored mode so API/IPC consumers track the rollback.
2260+
if (previousTaskMode) {
2261+
task.emit(RooCodeEventName.TaskModeSwitched, task.taskId, previousTaskMode)
2262+
}
22582263
if (historyItemBefore) {
22592264
// Restore only the field this switch changed: re-read the item so
22602265
// fields the running task persisted during the write survive.
@@ -2358,71 +2363,67 @@ export class ClineProvider
23582363
// restored mode would otherwise shadow the fresh switch for consumers.
23592364
sharedAttempted = true
23602365
await this.setValue("mode", newMode)
2361-
2362-
this.emit(RooCodeEventName.ModeChanged, newMode)
23632366
}
23642367

2365-
// If workspace lock is on, keep the current API config — don't load mode-specific config
2368+
// If workspace lock is on, keep the current API config: the mode-linked
2369+
// activation below is skipped, and the committed switch falls through to the
2370+
// emit/post tail outside the reconciled region.
23662371
const lockApiConfigAcrossModes = this.context.workspaceState.get("lockApiConfigAcrossModes", false)
2367-
if (lockApiConfigAcrossModes) {
2368-
// Keep the original post semantics: an explicit null target (pending child)
2369-
// posts its own state.
2370-
if (targetTask !== null) {
2371-
await this.postStateToWebview()
2372+
if (!lockApiConfigAcrossModes) {
2373+
if (signal?.aborted) return
2374+
if (!viewScopedSwitch) {
2375+
return
23722376
}
2373-
return
2374-
}
23752377

2376-
if (signal?.aborted) return
2377-
if (!viewScopedSwitch) {
2378-
return
2379-
}
2378+
// Load the saved API config for the new mode if it exists.
2379+
const savedConfigId = await this.providerSettingsManager.getModeConfigId(newMode)
2380+
const listApiConfig = await this.providerSettingsManager.listConfig()
23802381

2381-
// Load the saved API config for the new mode if it exists.
2382-
const savedConfigId = await this.providerSettingsManager.getModeConfigId(newMode)
2383-
const listApiConfig = await this.providerSettingsManager.listConfig()
2382+
if (signal?.aborted) return
23842383

2385-
if (signal?.aborted) return
2384+
// Update listApiConfigMeta first to ensure UI has latest data.
2385+
await this.updateGlobalState("listApiConfigMeta", listApiConfig)
23862386

2387-
// Update listApiConfigMeta first to ensure UI has latest data.
2388-
await this.updateGlobalState("listApiConfigMeta", listApiConfig)
2387+
// If this mode has a saved config, use it.
2388+
if (savedConfigId) {
2389+
const profile = listApiConfig.find(({ id }) => id === savedConfigId)
23892390

2390-
// If this mode has a saved config, use it.
2391-
if (savedConfigId) {
2392-
const profile = listApiConfig.find(({ id }) => id === savedConfigId)
2393-
2394-
if (profile?.name) {
2395-
// Check if the profile has actual API configuration (not just an id).
2396-
// In CLI mode, the ProviderSettingsManager may return empty default profiles
2397-
// that only contain 'id' and 'name' fields. Activating such a profile would
2398-
// overwrite the CLI's working API configuration with empty settings.
2399-
// Skip activation if the profile has no apiProvider set - this indicates
2400-
// an unconfigured/empty profile.
2401-
const fullProfile = await this.providerSettingsManager.getProfile({ name: profile.name })
2402-
const hasActualSettings = !!fullProfile.apiProvider
2403-
2404-
if (hasActualSettings) {
2405-
activationStarted = true
2406-
await this.activateProviderProfileUnlocked(
2407-
{ name: profile.name },
2408-
targetTask === null ? { skipCurrentTaskRebuild: true } : undefined,
2409-
signal,
2410-
)
2391+
if (profile?.name) {
2392+
// Check if the profile has actual API configuration (not just an id).
2393+
// In CLI mode, the ProviderSettingsManager may return empty default profiles
2394+
// that only contain 'id' and 'name' fields. Activating such a profile would
2395+
// overwrite the CLI's working API configuration with empty settings.
2396+
// Skip activation if the profile has no apiProvider set - this indicates
2397+
// an unconfigured/empty profile.
2398+
const fullProfile = await this.providerSettingsManager.getProfile({ name: profile.name })
2399+
const hasActualSettings = !!fullProfile.apiProvider
2400+
2401+
if (hasActualSettings) {
2402+
activationStarted = true
2403+
await this.activateProviderProfileUnlocked(
2404+
{ name: profile.name },
2405+
targetTask === null ? { skipCurrentTaskRebuild: true } : undefined,
2406+
signal,
2407+
)
2408+
} else {
2409+
// The task will continue with the current/default configuration.
2410+
}
24112411
} else {
24122412
// The task will continue with the current/default configuration.
24132413
}
24142414
} else {
2415-
// The task will continue with the current/default configuration.
2416-
}
2417-
} else {
2418-
// If no saved config for this mode, save current config as default.
2419-
const currentApiConfigNameAfter = this.getGlobalState("currentApiConfigName")
2415+
// If no saved config for this mode, save current config as default.
2416+
// Use this view's effective profile: with per-view pins the shared slot can
2417+
// belong to a sibling view, and recording that profile as this mode's default
2418+
// would activate the wrong profile for every later switch to it.
2419+
const currentApiConfigNameAfter = this.getValues().currentApiConfigName
24202420

2421-
if (currentApiConfigNameAfter) {
2422-
const config = listApiConfig.find((c) => c.name === currentApiConfigNameAfter)
2421+
if (currentApiConfigNameAfter) {
2422+
const config = listApiConfig.find((c) => c.name === currentApiConfigNameAfter)
24232423

2424-
if (config?.id) {
2425-
await this.providerSettingsManager.setModeConfig(newMode, config.id)
2424+
if (config?.id) {
2425+
await this.providerSettingsManager.setModeConfig(newMode, config.id)
2426+
}
24262427
}
24272428
}
24282429
}
@@ -2443,6 +2444,12 @@ export class ClineProvider
24432444
throw error
24442445
}
24452446

2447+
// The switch has committed: notify listeners only now, so a reconciled failure
2448+
// never leaves listeners tracking a mode the stores rolled back from.
2449+
if (viewScopedSwitch) {
2450+
this.emit(RooCodeEventName.ModeChanged, newMode)
2451+
}
2452+
24462453
// The switch has committed: a UI-refresh failure must not roll it back, so the
24472454
// trailing post runs outside the reconciled region and its failure is only logged.
24482455
if (targetTask !== null) {

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

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5595,13 +5595,11 @@ describe("ClineProvider", () => {
55955595
setModeConfig: vi.fn(),
55965596
} as any
55975597

5598-
// Mock the ContextProxy's getValue method to return the current config name
5598+
// Seed the shared current config name: the mode-default recording reads the
5599+
// view's effective profile, which falls back to the shared slot when this
5600+
// view pins no profile of its own.
55995601
const contextProxy = (provider as any).contextProxy
5600-
const getValueSpy = vi.spyOn(contextProxy, "getValue")
5601-
getValueSpy.mockImplementation((key: any) => {
5602-
if (key === "currentApiConfigName") return "current-config"
5603-
return undefined
5604-
})
5602+
await contextProxy.setValue("currentApiConfigName", "current-config")
56055603

56065604
// Register a stable view id so the durable per-view write is persisted
56075605
await provider["setViewStateId"]("stable-test-view")

‎src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -918,6 +918,161 @@ describe("ClineProvider - Sticky Mode", () => {
918918
expect(updateTaskHistorySpy).not.toHaveBeenCalledWith(expect.objectContaining({ mode: "code" }))
919919
})
920920

921+
it("does not report ModeChanged for a mode switch that gets reconciled", async () => {
922+
// Same typed double as the activation-failure rollback test above.
923+
const mockTask = Object.assign(
924+
{} as Task,
925+
{
926+
taskId: "test-task-id",
927+
taskMode: "code",
928+
_taskMode: "code",
929+
emit: vi.fn(),
930+
saveClineMessages: vi.fn(),
931+
clineMessages: [],
932+
apiConversationHistory: [],
933+
updateApiConfiguration: vi.fn(),
934+
} as Partial<Task>,
935+
)
936+
const historyItem: HistoryItem = {
937+
id: "test-task-id",
938+
ts: Date.now(),
939+
task: "Test task",
940+
mode: "code",
941+
number: 1,
942+
tokensIn: 0,
943+
tokensOut: 0,
944+
cacheWrites: 0,
945+
cacheReads: 0,
946+
totalCost: 0,
947+
}
948+
vi.spyOn(provider.taskHistoryStore, "get").mockReturnValue(historyItem)
949+
const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([])
950+
await provider.saveViewState("mode", "code")
951+
await provider.contextProxy.setValue("mode", "code")
952+
const emitSpy = vi.spyOn(provider, "emit")
953+
954+
// The task, pin, and shared writes land and the activation read then rejects:
955+
// the reconcile rolls every store back, so listeners must not be left tracking
956+
// the rolled-back mode: ModeChanged must not have fired for it, and the task's
957+
// TaskModeSwitched must be re-emitted with the restored mode.
958+
vi.spyOn(provider.providerSettingsManager, "getModeConfigId").mockRejectedValue(
959+
new Error("activation read failed"),
960+
)
961+
await provider.addClineToStack(mockTask)
962+
963+
await expect(provider["handleModeSwitchUnlocked"]("architect", mockTask)).rejects.toThrow(
964+
"activation read failed",
965+
)
966+
967+
expect(emitSpy).not.toHaveBeenCalledWith("modeChanged", "architect")
968+
expect(mockTask.emit).toHaveBeenCalledWith("taskModeSwitched", "test-task-id", "architect")
969+
expect(mockTask.emit).toHaveBeenLastCalledWith("taskModeSwitched", "test-task-id", "code")
970+
})
971+
972+
it("keeps the committed switch when the lock-path state refresh fails", async () => {
973+
// Same typed double as the activation-failure rollback test above.
974+
const mockTask = Object.assign(
975+
{} as Task,
976+
{
977+
taskId: "test-task-id",
978+
taskMode: "code",
979+
_taskMode: "code",
980+
emit: vi.fn(),
981+
saveClineMessages: vi.fn(),
982+
clineMessages: [],
983+
apiConversationHistory: [],
984+
updateApiConfiguration: vi.fn(),
985+
} as Partial<Task>,
986+
)
987+
const historyItem: HistoryItem = {
988+
id: "test-task-id",
989+
ts: Date.now(),
990+
task: "Test task",
991+
mode: "code",
992+
number: 1,
993+
tokensIn: 0,
994+
tokensOut: 0,
995+
cacheWrites: 0,
996+
cacheReads: 0,
997+
totalCost: 0,
998+
}
999+
vi.spyOn(provider.taskHistoryStore, "get").mockReturnValue(historyItem)
1000+
const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([])
1001+
await provider.saveViewState("mode", "code")
1002+
await provider.contextProxy.setValue("mode", "code")
1003+
1004+
// With the workspace profile lock on the switch commits without a mode-linked
1005+
// activation; the trailing post now runs outside the reconciled region, so its
1006+
// failure must leave the committed switch standing instead of rolling the mode
1007+
// stores back.
1008+
vi.spyOn(provider["context"].workspaceState, "get").mockImplementation((key: unknown, def?: unknown) =>
1009+
key === "lockApiConfigAcrossModes" ? true : def,
1010+
)
1011+
vi.spyOn(provider, "postStateToWebview").mockRejectedValue(new Error("post failed"))
1012+
await provider.addClineToStack(mockTask)
1013+
1014+
await provider["handleModeSwitchUnlocked"]("architect", mockTask)
1015+
1016+
expect(provider.getValue("mode")).toBe("architect")
1017+
expect(provider["viewLocalState"].mode).toBe("architect")
1018+
expect(mockTask["_taskMode"]).toBe("architect")
1019+
expect(updateTaskHistorySpy).not.toHaveBeenCalledWith(expect.objectContaining({ mode: "code" }))
1020+
})
1021+
1022+
it("records the mode default from the view's pinned profile, not the shared slot", async () => {
1023+
// Same typed double as the activation-failure rollback test above.
1024+
const mockTask = Object.assign(
1025+
{} as Task,
1026+
{
1027+
taskId: "test-task-id",
1028+
taskMode: "code",
1029+
_taskMode: "code",
1030+
emit: vi.fn(),
1031+
saveClineMessages: vi.fn(),
1032+
clineMessages: [],
1033+
apiConversationHistory: [],
1034+
updateApiConfiguration: vi.fn(),
1035+
} as Partial<Task>,
1036+
)
1037+
const historyItem: HistoryItem = {
1038+
id: "test-task-id",
1039+
ts: Date.now(),
1040+
task: "Test task",
1041+
mode: "code",
1042+
number: 1,
1043+
tokensIn: 0,
1044+
tokensOut: 0,
1045+
cacheWrites: 0,
1046+
cacheReads: 0,
1047+
totalCost: 0,
1048+
}
1049+
vi.spyOn(provider.taskHistoryStore, "get").mockReturnValue(historyItem)
1050+
const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([])
1051+
await provider.saveViewState("mode", "code")
1052+
await provider.contextProxy.setValue("mode", "code")
1053+
1054+
// The view pins profile-x while a sibling view's activation moved the shared
1055+
// slot to profile-y. Recording the mode default from the shared slot would make
1056+
// profile-y the default for the new mode in every view, though this view was
1057+
// running profile-x.
1058+
await provider.setValue("currentApiConfigName", "profile-x")
1059+
await provider.contextProxy.setValue("currentApiConfigName", "profile-y")
1060+
vi.spyOn(provider.providerSettingsManager, "getModeConfigId").mockResolvedValue(undefined)
1061+
vi.spyOn(provider.providerSettingsManager, "listConfig").mockResolvedValue([
1062+
{ id: "x-id", name: "profile-x", apiProvider: providerIdentifiers.openrouter },
1063+
{ id: "y-id", name: "profile-y", apiProvider: providerIdentifiers.anthropic },
1064+
])
1065+
const setModeConfig = vi
1066+
.spyOn(provider.providerSettingsManager, "setModeConfig")
1067+
.mockResolvedValue(undefined)
1068+
vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined)
1069+
await provider.addClineToStack(mockTask)
1070+
1071+
await provider["handleModeSwitchUnlocked"]("architect", mockTask)
1072+
1073+
expect(setModeConfig).toHaveBeenCalledWith("architect", "x-id")
1074+
})
1075+
9211076
it("keeps the cancellation result when the abort rollback write itself fails", async () => {
9221077
const mockTask = Object.assign(
9231078
{} as Task,

‎src/eslint-suppressions.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1021,7 +1021,7 @@
10211021
},
10221022
"core/webview/__tests__/ClineProvider.spec.ts": {
10231023
"@typescript-eslint/no-explicit-any": {
1024-
"count": 194
1024+
"count": 193
10251025
}
10261026
},
10271027
"core/webview/__tests__/ClineProvider.sticky-mode.spec.ts": {

0 commit comments

Comments
 (0)