Skip to content

Commit 0b08bb0

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 c820dd0 commit 0b08bb0

2 files changed

Lines changed: 207 additions & 5 deletions

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2394,6 +2394,10 @@ export class ClineProvider
23942394
}
23952395
}
23962396

2397+
// Once the mode-linked activation below has started writing shared state, a
2398+
// mode-only rollback would pair the old mode with the new profile: the catch must
2399+
// then keep the switch instead of reconciling.
2400+
let activationStarted = false
23972401
try {
23982402
if (viewScopedSwitch) {
23992403
// Pin the view's durable per-view mode first: the pin is this view's own
@@ -2453,6 +2457,7 @@ export class ClineProvider
24532457
const hasActualSettings = !!fullProfile.apiProvider
24542458

24552459
if (hasActualSettings) {
2460+
activationStarted = true
24562461
await this.activateProviderProfileUnlocked(
24572462
{ name: profile.name },
24582463
targetTask === null ? { skipCurrentTaskRebuild: true } : undefined,
@@ -2476,17 +2481,36 @@ export class ClineProvider
24762481
}
24772482
}
24782483
}
2479-
2480-
if (targetTask !== null) {
2481-
await this.postStateToWebview()
2482-
}
24832484
} catch (error) {
2484-
await reconcileSwitchWrites()
2485+
// Two reasons to keep the switch instead of reconciling: once the mode-linked
2486+
// activation has started writing shared state, a mode-only rollback would pair
2487+
// the old mode with the new profile; and after a timeout abort the queue has
2488+
// advanced, so a newer mutation may own these fields and replaying the
2489+
// pre-switch values would clobber it.
2490+
if (!activationStarted && !signal?.aborted) {
2491+
await reconcileSwitchWrites()
2492+
} else if (signal?.aborted) {
2493+
this.log(`[handleModeSwitch] Skipping rollback for aborted switch to "${newMode}"`)
2494+
}
24852495
this.log(
24862496
`[handleModeSwitch] Failed to persist mode "${newMode}": ${error instanceof Error ? error.message : String(error)}`,
24872497
)
24882498
throw error
24892499
}
2500+
2501+
// The switch has committed: a UI-refresh failure must not roll it back, so the
2502+
// trailing post runs outside the reconciled region and its failure is only logged.
2503+
if (targetTask !== null) {
2504+
try {
2505+
await this.postStateToWebview()
2506+
} catch (postError: unknown) {
2507+
this.log(
2508+
`[handleModeSwitch] Mode switch to "${newMode}" committed but the state post failed: ${
2509+
postError instanceof Error ? postError.message : String(postError)
2510+
}`,
2511+
)
2512+
}
2513+
}
24902514
}
24912515

24922516
// Provider Profile Management

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

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -740,6 +740,184 @@ describe("ClineProvider - Sticky Mode", () => {
740740
expect(provider.getValue("mode")).toBe("code")
741741
})
742742

743+
it("keeps the committed mode switch when the post-activation state refresh fails", async () => {
744+
// Same typed double as the activation-failure rollback test above.
745+
const mockTask = Object.assign(
746+
{} as Task,
747+
{
748+
taskId: "test-task-id",
749+
taskMode: "code",
750+
_taskMode: "code",
751+
emit: vi.fn(),
752+
saveClineMessages: vi.fn(),
753+
clineMessages: [],
754+
apiConversationHistory: [],
755+
updateApiConfiguration: vi.fn(),
756+
} as Partial<Task>,
757+
)
758+
const historyItem: HistoryItem = {
759+
id: "test-task-id",
760+
ts: Date.now(),
761+
task: "Test task",
762+
mode: "code",
763+
number: 1,
764+
tokensIn: 0,
765+
tokensOut: 0,
766+
cacheWrites: 0,
767+
cacheReads: 0,
768+
totalCost: 0,
769+
}
770+
vi.spyOn(provider.taskHistoryStore, "get").mockReturnValue(historyItem)
771+
const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([])
772+
await provider.saveViewState("mode", "code")
773+
await provider.contextProxy.setValue("mode", "code")
774+
775+
// The mode-linked profile resolves and its activation lands (writing the
776+
// shared profile like the real activation does); only the trailing state
777+
// post rejects. By then the switch has fully committed - ModeChanged was
778+
// emitted and the activation persisted the new profile - so a UI refresh
779+
// failure must not roll the mode back: that would pair the old mode with
780+
// the new profile, the very mismatch the reconcile exists to prevent.
781+
vi.spyOn(provider.providerSettingsManager, "getModeConfigId").mockResolvedValue("architect-profile-id")
782+
vi.spyOn(provider.providerSettingsManager, "listConfig").mockResolvedValue([
783+
{ id: "architect-profile-id", name: "architect-profile", apiProvider: providerIdentifiers.openrouter },
784+
])
785+
vi.spyOn(provider.providerSettingsManager, "getProfile").mockResolvedValue({
786+
name: "architect-profile",
787+
id: "architect-profile-id",
788+
apiProvider: providerIdentifiers.openrouter,
789+
})
790+
provider["activateProviderProfileUnlocked"] = vi.fn().mockImplementation(async () => {
791+
await provider.contextProxy.setValue("currentApiConfigName", "architect-profile")
792+
})
793+
vi.spyOn(provider, "postStateToWebview").mockRejectedValue(new Error("post failed"))
794+
await provider.addClineToStack(mockTask)
795+
796+
await provider["handleModeSwitchUnlocked"]("architect", mockTask)
797+
798+
expect(provider.getValue("mode")).toBe("architect")
799+
expect(provider["viewLocalState"].mode).toBe("architect")
800+
expect(mockTask["_taskMode"]).toBe("architect")
801+
expect(provider.contextProxy.getValue("currentApiConfigName")).toBe("architect-profile")
802+
expect(updateTaskHistorySpy).not.toHaveBeenCalledWith(expect.objectContaining({ mode: "code" }))
803+
})
804+
805+
it("keeps the switched mode when the mode-linked activation fails after writing", async () => {
806+
// Same typed double as the activation-failure rollback test above.
807+
const mockTask = Object.assign(
808+
{} as Task,
809+
{
810+
taskId: "test-task-id",
811+
taskMode: "code",
812+
_taskMode: "code",
813+
emit: vi.fn(),
814+
saveClineMessages: vi.fn(),
815+
clineMessages: [],
816+
apiConversationHistory: [],
817+
updateApiConfiguration: vi.fn(),
818+
} as Partial<Task>,
819+
)
820+
const historyItem: HistoryItem = {
821+
id: "test-task-id",
822+
ts: Date.now(),
823+
task: "Test task",
824+
mode: "code",
825+
number: 1,
826+
tokensIn: 0,
827+
tokensOut: 0,
828+
cacheWrites: 0,
829+
cacheReads: 0,
830+
totalCost: 0,
831+
}
832+
vi.spyOn(provider.taskHistoryStore, "get").mockReturnValue(historyItem)
833+
const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([])
834+
await provider.saveViewState("mode", "code")
835+
await provider.contextProxy.setValue("mode", "code")
836+
837+
// The mode-linked activation lands its shared writes and only then rejects
838+
// (e.g. its own state post or setModeConfig step failing). A mode-only rollback
839+
// would pair the old mode with the profile the activation already switched to,
840+
// the mismatch the reconcile exists to prevent: the switch must stand and the
841+
// error must still surface.
842+
vi.spyOn(provider.providerSettingsManager, "getModeConfigId").mockResolvedValue("architect-profile-id")
843+
vi.spyOn(provider.providerSettingsManager, "listConfig").mockResolvedValue([
844+
{ id: "architect-profile-id", name: "architect-profile", apiProvider: providerIdentifiers.openrouter },
845+
])
846+
vi.spyOn(provider.providerSettingsManager, "getProfile").mockResolvedValue({
847+
name: "architect-profile",
848+
id: "architect-profile-id",
849+
apiProvider: providerIdentifiers.openrouter,
850+
})
851+
provider["activateProviderProfileUnlocked"] = vi.fn().mockImplementation(async () => {
852+
await provider.contextProxy.setValue("currentApiConfigName", "architect-profile")
853+
throw new Error("activation step failed")
854+
})
855+
await provider.addClineToStack(mockTask)
856+
857+
await expect(provider["handleModeSwitchUnlocked"]("architect", mockTask)).rejects.toThrow(
858+
"activation step failed",
859+
)
860+
861+
expect(provider.getValue("mode")).toBe("architect")
862+
expect(provider["viewLocalState"].mode).toBe("architect")
863+
expect(mockTask["_taskMode"]).toBe("architect")
864+
expect(provider.contextProxy.getValue("currentApiConfigName")).toBe("architect-profile")
865+
expect(updateTaskHistorySpy).not.toHaveBeenCalledWith(expect.objectContaining({ mode: "code" }))
866+
})
867+
868+
it("keeps the switched mode when the mutation signal aborted before the failure", async () => {
869+
// Same typed double as the activation-failure rollback test above.
870+
const mockTask = Object.assign(
871+
{} as Task,
872+
{
873+
taskId: "test-task-id",
874+
taskMode: "code",
875+
_taskMode: "code",
876+
emit: vi.fn(),
877+
saveClineMessages: vi.fn(),
878+
clineMessages: [],
879+
apiConversationHistory: [],
880+
updateApiConfiguration: vi.fn(),
881+
} as Partial<Task>,
882+
)
883+
const historyItem: HistoryItem = {
884+
id: "test-task-id",
885+
ts: Date.now(),
886+
task: "Test task",
887+
mode: "code",
888+
number: 1,
889+
tokensIn: 0,
890+
tokensOut: 0,
891+
cacheWrites: 0,
892+
cacheReads: 0,
893+
totalCost: 0,
894+
}
895+
vi.spyOn(provider.taskHistoryStore, "get").mockReturnValue(historyItem)
896+
const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([])
897+
await provider.saveViewState("mode", "code")
898+
await provider.contextProxy.setValue("mode", "code")
899+
900+
// The pin and shared writes land; then the mutation timeout aborts this
901+
// switch and a later step rejects. The queue has already advanced past the
902+
// timed-out switch, so a newer mutation may own the mode stores: the late
903+
// rollback must not replay the pre-switch values over them.
904+
const controller = new AbortController()
905+
vi.spyOn(provider.providerSettingsManager, "getModeConfigId").mockImplementation(async () => {
906+
controller.abort()
907+
throw new Error("late activation read failure")
908+
})
909+
await provider.addClineToStack(mockTask)
910+
911+
await expect(
912+
provider["handleModeSwitchUnlocked"]("architect", mockTask, controller.signal),
913+
).rejects.toThrow("late activation read failure")
914+
915+
expect(provider.getValue("mode")).toBe("architect")
916+
expect(provider["viewLocalState"].mode).toBe("architect")
917+
expect(mockTask["_taskMode"]).toBe("architect")
918+
expect(updateTaskHistorySpy).not.toHaveBeenCalledWith(expect.objectContaining({ mode: "code" }))
919+
})
920+
743921
it("keeps the cancellation result when the abort rollback write itself fails", async () => {
744922
const mockTask = Object.assign(
745923
{} as Task,

0 commit comments

Comments
 (0)