Skip to content

Commit fcb06c4

Browse files
committed
fix: eliminate UI flicker during task cancellation
- Modify ClineProvider.createTaskWithHistoryItem() to detect when rehydrating current task - Implement in-place task replacement to avoid empty stack state that causes UI flicker - Add comprehensive unit tests for flicker-free cancel behavior - Maintain backward compatibility and proper event listener cleanup Fixes the jarring navigation to home view when cancelling tasks
1 parent 54745fc commit fcb06c4

File tree

2 files changed

+355
-5
lines changed

2 files changed

+355
-5
lines changed

src/core/webview/ClineProvider.ts

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -858,7 +858,13 @@ export class ClineProvider
858858
}
859859

860860
public async createTaskWithHistoryItem(historyItem: HistoryItem & { rootTask?: Task; parentTask?: Task }) {
861-
await this.removeClineFromStack()
861+
// Check if we're rehydrating the current task to avoid flicker
862+
const currentTask = this.getCurrentTask()
863+
const isRehydratingCurrentTask = currentTask && currentTask.taskId === historyItem.id
864+
865+
if (!isRehydratingCurrentTask) {
866+
await this.removeClineFromStack()
867+
}
862868

863869
// If the history item has a saved mode, restore it and its associated API configuration.
864870
if (historyItem.mode) {
@@ -932,11 +938,35 @@ export class ClineProvider
932938
enableBridge: BridgeOrchestrator.isEnabled(cloudUserInfo, taskSyncEnabled),
933939
})
934940

935-
await this.addClineToStack(task)
941+
if (isRehydratingCurrentTask) {
942+
// Replace the current task in-place to avoid UI flicker
943+
const stackIndex = this.clineStack.length - 1
936944

937-
this.log(
938-
`[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`,
939-
)
945+
// Remove event listeners from the old task
946+
const oldTask = this.clineStack[stackIndex]
947+
const cleanupFunctions = this.taskEventListeners.get(oldTask)
948+
if (cleanupFunctions) {
949+
cleanupFunctions.forEach((cleanup) => cleanup())
950+
this.taskEventListeners.delete(oldTask)
951+
}
952+
953+
// Replace the task in the stack
954+
this.clineStack[stackIndex] = task
955+
task.emit(RooCodeEventName.TaskFocused)
956+
957+
// Perform preparation tasks and set up event listeners
958+
await this.performPreparationTasks(task)
959+
960+
this.log(
961+
`[createTaskWithHistoryItem] rehydrated task ${task.taskId}.${task.instanceId} in-place (flicker-free)`,
962+
)
963+
} else {
964+
await this.addClineToStack(task)
965+
966+
this.log(
967+
`[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`,
968+
)
969+
}
940970

941971
// Check if there's a pending edit after checkpoint restoration
942972
const operationId = `task-${task.taskId}`
Lines changed: 320 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,320 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest"
2+
import * as vscode from "vscode"
3+
4+
import { ClineProvider } from "../ClineProvider"
5+
import { Task } from "../../task/Task"
6+
import { ContextProxy } from "../../config/ContextProxy"
7+
import type { ProviderSettings, HistoryItem } from "@roo-code/types"
8+
9+
// Mock dependencies
10+
vi.mock("vscode", () => {
11+
const mockDisposable = { dispose: vi.fn() }
12+
return {
13+
workspace: {
14+
getConfiguration: vi.fn(() => ({
15+
get: vi.fn().mockReturnValue([]),
16+
update: vi.fn().mockResolvedValue(undefined),
17+
})),
18+
workspaceFolders: [],
19+
onDidChangeConfiguration: vi.fn(() => mockDisposable),
20+
},
21+
env: {
22+
uriScheme: "vscode",
23+
language: "en",
24+
},
25+
EventEmitter: vi.fn().mockImplementation(() => ({
26+
event: vi.fn(),
27+
fire: vi.fn(),
28+
})),
29+
Disposable: {
30+
from: vi.fn(),
31+
},
32+
window: {
33+
showErrorMessage: vi.fn(),
34+
createTextEditorDecorationType: vi.fn().mockReturnValue({
35+
dispose: vi.fn(),
36+
}),
37+
onDidChangeActiveTextEditor: vi.fn(() => mockDisposable),
38+
},
39+
Uri: {
40+
file: vi.fn().mockReturnValue({ toString: () => "file://test" }),
41+
},
42+
}
43+
})
44+
45+
vi.mock("../../task/Task")
46+
vi.mock("../../config/ContextProxy")
47+
vi.mock("../../../services/mcp/McpServerManager", () => ({
48+
McpServerManager: {
49+
getInstance: vi.fn().mockResolvedValue({
50+
registerClient: vi.fn(),
51+
}),
52+
unregisterProvider: vi.fn(),
53+
},
54+
}))
55+
vi.mock("../../../services/marketplace")
56+
vi.mock("../../../integrations/workspace/WorkspaceTracker")
57+
vi.mock("../../config/ProviderSettingsManager")
58+
vi.mock("../../config/CustomModesManager")
59+
vi.mock("../../../utils/path", () => ({
60+
getWorkspacePath: vi.fn().mockReturnValue("/test/workspace"),
61+
}))
62+
63+
// Mock TelemetryService
64+
vi.mock("@roo-code/telemetry", () => ({
65+
TelemetryService: {
66+
instance: {
67+
setProvider: vi.fn(),
68+
captureTaskCreated: vi.fn(),
69+
},
70+
},
71+
}))
72+
73+
// Mock CloudService
74+
vi.mock("@roo-code/cloud", () => ({
75+
CloudService: {
76+
hasInstance: vi.fn().mockReturnValue(false),
77+
instance: {
78+
isAuthenticated: vi.fn().mockReturnValue(false),
79+
},
80+
},
81+
BridgeOrchestrator: {
82+
isEnabled: vi.fn().mockReturnValue(false),
83+
},
84+
getRooCodeApiUrl: vi.fn().mockReturnValue("https://api.roo-code.com"),
85+
}))
86+
87+
vi.mock("../../../shared/embeddingModels", () => ({
88+
EMBEDDING_MODEL_PROFILES: [],
89+
}))
90+
91+
describe("ClineProvider flicker-free cancel", () => {
92+
let provider: ClineProvider
93+
let mockContext: any
94+
let mockOutputChannel: any
95+
let mockTask1: any
96+
let mockTask2: any
97+
98+
const mockApiConfig: ProviderSettings = {
99+
apiProvider: "anthropic",
100+
apiKey: "test-key",
101+
} as ProviderSettings
102+
103+
beforeEach(() => {
104+
vi.clearAllMocks()
105+
106+
// Setup mock extension context
107+
mockContext = {
108+
globalState: {
109+
get: vi.fn().mockReturnValue(undefined),
110+
update: vi.fn().mockResolvedValue(undefined),
111+
keys: vi.fn().mockReturnValue([]),
112+
},
113+
globalStorageUri: { fsPath: "/test/storage" },
114+
secrets: {
115+
get: vi.fn().mockResolvedValue(undefined),
116+
store: vi.fn().mockResolvedValue(undefined),
117+
delete: vi.fn().mockResolvedValue(undefined),
118+
},
119+
workspaceState: {
120+
get: vi.fn().mockReturnValue(undefined),
121+
update: vi.fn().mockResolvedValue(undefined),
122+
keys: vi.fn().mockReturnValue([]),
123+
},
124+
extensionUri: { fsPath: "/test/extension" },
125+
}
126+
127+
// Setup mock output channel
128+
mockOutputChannel = {
129+
appendLine: vi.fn(),
130+
dispose: vi.fn(),
131+
}
132+
133+
// Setup mock context proxy
134+
const mockContextProxy = {
135+
getValues: vi.fn().mockReturnValue({}),
136+
getValue: vi.fn().mockReturnValue(undefined),
137+
setValue: vi.fn().mockResolvedValue(undefined),
138+
getProviderSettings: vi.fn().mockReturnValue(mockApiConfig),
139+
extensionUri: mockContext.extensionUri,
140+
globalStorageUri: mockContext.globalStorageUri,
141+
}
142+
143+
// Create provider instance
144+
provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", mockContextProxy as any)
145+
146+
// Mock provider methods
147+
provider.getState = vi.fn().mockResolvedValue({
148+
apiConfiguration: mockApiConfig,
149+
mode: "code",
150+
})
151+
152+
provider.postStateToWebview = vi.fn().mockResolvedValue(undefined)
153+
// Mock private method using any cast
154+
;(provider as any).updateGlobalState = vi.fn().mockResolvedValue(undefined)
155+
provider.activateProviderProfile = vi.fn().mockResolvedValue(undefined)
156+
provider.performPreparationTasks = vi.fn().mockResolvedValue(undefined)
157+
provider.getTaskWithId = vi.fn().mockImplementation((id) =>
158+
Promise.resolve({
159+
historyItem: {
160+
id,
161+
number: 1,
162+
ts: Date.now(),
163+
task: "test task",
164+
tokensIn: 100,
165+
tokensOut: 200,
166+
totalCost: 0.001,
167+
workspace: "/test/workspace",
168+
},
169+
}),
170+
)
171+
172+
// Setup mock tasks
173+
mockTask1 = {
174+
taskId: "task-1",
175+
instanceId: "instance-1",
176+
emit: vi.fn(),
177+
abortTask: vi.fn().mockResolvedValue(undefined),
178+
dispose: vi.fn(),
179+
on: vi.fn(),
180+
off: vi.fn(),
181+
}
182+
183+
mockTask2 = {
184+
taskId: "task-1", // Same ID for rehydration scenario
185+
instanceId: "instance-2", // Different instance
186+
emit: vi.fn(),
187+
on: vi.fn(),
188+
off: vi.fn(),
189+
}
190+
191+
// Mock Task constructor
192+
vi.mocked(Task).mockImplementation(() => mockTask2 as any)
193+
})
194+
195+
it("should not remove current task from stack when rehydrating same taskId", async () => {
196+
// Setup: Add a task to the stack first
197+
;(provider as any).clineStack = [mockTask1]
198+
199+
// Mock event listeners for cleanup
200+
;(provider as any).taskEventListeners = new WeakMap()
201+
const mockCleanupFunctions = [vi.fn(), vi.fn()]
202+
;(provider as any).taskEventListeners.set(mockTask1, mockCleanupFunctions)
203+
204+
// Spy on removeClineFromStack to verify it's NOT called
205+
const removeClineFromStackSpy = vi.spyOn(provider, "removeClineFromStack")
206+
207+
// Create history item with same taskId as current task
208+
const historyItem: HistoryItem = {
209+
id: "task-1", // Same as mockTask1.taskId
210+
number: 1,
211+
task: "test task",
212+
ts: Date.now(),
213+
tokensIn: 100,
214+
tokensOut: 200,
215+
totalCost: 0.001,
216+
workspace: "/test/workspace",
217+
}
218+
219+
// Act: Create task with history item (should rehydrate in-place)
220+
await provider.createTaskWithHistoryItem(historyItem)
221+
222+
// Assert: removeClineFromStack should NOT be called
223+
expect(removeClineFromStackSpy).not.toHaveBeenCalled()
224+
225+
// Verify the task was replaced in-place
226+
expect((provider as any).clineStack).toHaveLength(1)
227+
expect((provider as any).clineStack[0]).toBe(mockTask2)
228+
229+
// Verify old event listeners were cleaned up
230+
expect(mockCleanupFunctions[0]).toHaveBeenCalled()
231+
expect(mockCleanupFunctions[1]).toHaveBeenCalled()
232+
233+
// Verify new task received focus event
234+
expect(mockTask2.emit).toHaveBeenCalledWith("taskFocused")
235+
})
236+
237+
it("should remove task from stack when creating different task", async () => {
238+
// Setup: Add a task to the stack first
239+
;(provider as any).clineStack = [mockTask1]
240+
241+
// Spy on removeClineFromStack to verify it IS called
242+
const removeClineFromStackSpy = vi.spyOn(provider, "removeClineFromStack").mockResolvedValue(undefined)
243+
244+
// Create history item with different taskId
245+
const historyItem: HistoryItem = {
246+
id: "task-2", // Different from mockTask1.taskId
247+
number: 2,
248+
task: "different task",
249+
ts: Date.now(),
250+
tokensIn: 150,
251+
tokensOut: 250,
252+
totalCost: 0.002,
253+
workspace: "/test/workspace",
254+
}
255+
256+
// Act: Create task with different history item
257+
await provider.createTaskWithHistoryItem(historyItem)
258+
259+
// Assert: removeClineFromStack should be called
260+
expect(removeClineFromStackSpy).toHaveBeenCalled()
261+
})
262+
263+
it("should handle empty stack gracefully during rehydration attempt", async () => {
264+
// Setup: Empty stack
265+
;(provider as any).clineStack = []
266+
267+
// Spy on removeClineFromStack
268+
const removeClineFromStackSpy = vi.spyOn(provider, "removeClineFromStack").mockResolvedValue(undefined)
269+
270+
// Create history item
271+
const historyItem: HistoryItem = {
272+
id: "task-1",
273+
number: 1,
274+
task: "test task",
275+
ts: Date.now(),
276+
tokensIn: 100,
277+
tokensOut: 200,
278+
totalCost: 0.001,
279+
workspace: "/test/workspace",
280+
}
281+
282+
// Act: Should not error and should call removeClineFromStack
283+
await provider.createTaskWithHistoryItem(historyItem)
284+
285+
// Assert: removeClineFromStack should be called (no current task to rehydrate)
286+
expect(removeClineFromStackSpy).toHaveBeenCalled()
287+
})
288+
289+
it("should maintain task stack integrity during flicker-free replacement", async () => {
290+
// Setup: Stack with multiple tasks
291+
const mockParentTask = {
292+
taskId: "parent-task",
293+
instanceId: "parent-instance",
294+
emit: vi.fn(),
295+
}
296+
297+
;(provider as any).clineStack = [mockParentTask, mockTask1]
298+
;(provider as any).taskEventListeners = new WeakMap()
299+
;(provider as any).taskEventListeners.set(mockTask1, [vi.fn()])
300+
301+
// Act: Rehydrate the current (top) task
302+
const historyItem: HistoryItem = {
303+
id: "task-1",
304+
number: 1,
305+
task: "test task",
306+
ts: Date.now(),
307+
tokensIn: 100,
308+
tokensOut: 200,
309+
totalCost: 0.001,
310+
workspace: "/test/workspace",
311+
}
312+
313+
await provider.createTaskWithHistoryItem(historyItem)
314+
315+
// Assert: Stack should maintain parent task and replace current task
316+
expect((provider as any).clineStack).toHaveLength(2)
317+
expect((provider as any).clineStack[0]).toBe(mockParentTask)
318+
expect((provider as any).clineStack[1]).toBe(mockTask2)
319+
})
320+
})

0 commit comments

Comments
 (0)