Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1623,6 +1623,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
this.isPaused = false
this.childTaskId = undefined

const provider = this.providerRef.deref()
if (provider) {
await provider.handleModeSwitch(this.pausedModeSlug)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good defensive programming with the null check! However, I noticed that handleModeSwitch() in ClineProvider already calls postStateToWebview() internally. Since finishSubTask() also calls handleModeSwitch() and then explicitly calls postStateToWebview() again (line 444 in ClineProvider), could this cause duplicate UI updates? Would it be cleaner to rely on just one of these calls?

}

this.emit(RooCodeEventName.TaskUnpaused, this.taskId)

// Fake an answer from the subtask that it has completed running and
Expand Down
71 changes: 71 additions & 0 deletions src/core/task/__tests__/Task.subtask-mode-restore.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { describe, it, expect, vi, beforeEach } from "vitest"
import { Task } from "../Task"
import { ClineProvider } from "../../webview/ClineProvider"
import { TodoItem } from "@roo-code/types"

describe("Task subtask mode restoration", () => {
let parentTask: Task
let mockProvider: any

beforeEach(() => {
mockProvider = {
handleModeSwitch: vi.fn().mockResolvedValue(undefined),
log: vi.fn(),
deref: vi.fn().mockReturnValue({
handleModeSwitch: vi.fn().mockResolvedValue(undefined),
}),
}
})

it("should restore parent task mode when subtask completes", async () => {
// Create parent task with orchestrator mode
parentTask = new Task({
provider: mockProvider as any,
apiConfiguration: {} as any,
task: "Parent task",
})

// Set parent task to orchestrator mode
parentTask.pausedModeSlug = "orchestrator"

// Mock the provider reference
parentTask.providerRef = {
deref: () => mockProvider.deref(),
} as any

// Complete the subtask
await parentTask.completeSubtask("Subtask completed")

// Verify handleModeSwitch was called with the pausedModeSlug
expect(mockProvider.deref().handleModeSwitch).toHaveBeenCalledWith("orchestrator")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test coverage for the core functionality! Consider adding a test case that verifies the order of operations - specifically that handleModeSwitch() is called before any UI updates. This would help ensure the mode is properly set before the UI refreshes.


// Verify task is unpaused
expect(parentTask.isPaused).toBe(false)

// Verify childTaskId is cleared
expect(parentTask.childTaskId).toBeUndefined()
})

it("should handle missing provider gracefully", async () => {
// Create parent task
parentTask = new Task({
provider: mockProvider as any,
apiConfiguration: {} as any,
task: "Parent task",
})

// Set parent task to orchestrator mode
parentTask.pausedModeSlug = "orchestrator"

// Mock provider as unavailable
parentTask.providerRef = {
deref: () => undefined,
} as any

// Complete the subtask - should not throw
await expect(parentTask.completeSubtask("Subtask completed")).resolves.not.toThrow()

// Verify task is still unpaused
expect(parentTask.isPaused).toBe(false)
})
})
3 changes: 3 additions & 0 deletions src/core/webview/ClineProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,9 @@ export class ClineProvider
// Resume the last cline instance in the stack (if it exists - this is
// the 'parent' calling task).
await this.getCurrentTask()?.completeSubtask(lastMessage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add error handling here similar to what's done in Task.completeSubtask()? If getCurrentTask() returns undefined or completeSubtask() throws an error, it might leave the UI in an inconsistent state.


// Explicitly trigger UI state update so that webview/pane transitions back to the parent thread.
await this.postStateToWebview()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit postStateToWebview() call is crucial for the fix, but since completeSubtask() now calls handleModeSwitch() which internally calls postStateToWebview(), are we potentially triggering duplicate UI updates? Consider whether we need both calls or if we can optimize to avoid unnecessary re-renders.

}

/*
Expand Down
Loading