-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat: implement task resumption from web UI #7116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,7 @@ export const taskCommandSchema = z.discriminatedUnion("commandName", [ | |
| text: z.string(), | ||
| images: z.array(z.string()).optional(), | ||
| newTab: z.boolean().optional(), | ||
| taskId: z.string().optional(), // Optional taskId for resuming existing tasks | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be helpful to add JSDoc documentation for this new field? It could help future developers understand the resumption flow better: |
||
| }), | ||
| }), | ||
| z.object({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -744,6 +744,71 @@ export class ClineProvider | |
| return task | ||
| } | ||
|
|
||
| /** | ||
| * Handles starting a task with optional resumption support. | ||
| * This method is called by the ExtensionBridgeService when receiving StartTask commands from the web UI. | ||
| * | ||
| * @param payload - The task payload containing text, images, and optional taskId for resumption | ||
| */ | ||
| public async handleStartTask(payload: { text: string; images?: string[]; taskId?: string }): Promise<void> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that handleStartTask isn't being called from anywhere yet. The comment mentions it should be called by ExtensionBridgeService when receiving StartTask commands from the web UI. Are you planning to add the ExtensionBridgeService integration in a follow-up commit, or is there perhaps a missing file in this PR? I'd be happy to help figure out where this should be wired up! |
||
| if (payload.taskId) { | ||
| // Attempt to resume existing task | ||
| try { | ||
| const { historyItem } = await this.getTaskWithId(payload.taskId) | ||
| if (historyItem) { | ||
| this.log(`Resuming task ${payload.taskId} from web UI`) | ||
|
|
||
| // Clear any current tasks and restore the specific task | ||
| await this.initClineWithHistoryItem(historyItem) | ||
|
|
||
| // Ensure the webview shows the chat interface | ||
| await this.postMessageToWebview({ type: "action", action: "chatButtonClicked" }) | ||
|
|
||
| // Update state to reflect the new current task | ||
| await this.postStateToWebview() | ||
|
|
||
| // Ensure bridge service is connected to the resumed task | ||
| const resumedTask = this.getCurrentCline() | ||
| if (resumedTask) { | ||
| try { | ||
| const { ExtensionBridgeService } = await import("@roo-code/cloud") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This bridge service connection logic is duplicated from the remote control toggle method. Could we extract this into a shared helper method like connectBridgeServiceToTask(task: Task) to reduce duplication and ensure consistency?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed this bridge service connection pattern is similar to what's in handleRemoteControlToggle (around line 2213). Would it make sense to extract this into a shared helper method? Something like could reduce duplication and make it easier to maintain consistent behavior. What do you think? |
||
| const bridgeService = ExtensionBridgeService.getInstance() | ||
| if (bridgeService && !resumedTask.bridgeService) { | ||
| resumedTask.bridgeService = bridgeService | ||
| await bridgeService.subscribeToTask(resumedTask) | ||
| this.log(`Bridge service connected to resumed task ${payload.taskId}`) | ||
| } | ||
| } catch (error) { | ||
| this.log(`Failed to connect bridge service to resumed task: ${error}`) | ||
| } | ||
| } | ||
|
|
||
| // The user's new message (payload.text) will be sent through the normal message flow | ||
| // after the task is restored and ready | ||
| if (payload.text) { | ||
| // Give a small delay to ensure the task is fully restored | ||
| setTimeout(async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this 200ms delay reliable across different system loads? Consider using a more deterministic approach, such as waiting for a specific state change event rather than using setTimeout. This would eliminate the race condition risk.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious about this 200ms delay - have you found this timing works reliably in your testing? I'm wondering if we might encounter race conditions on slower systems or under heavy load. What do you think about using an event-based approach instead? For example, we could wait for a 'ready' signal from the webview, or queue messages until the task restoration is complete. This might be more reliable across different environments. |
||
| await this.postMessageToWebview({ | ||
| type: "invoke", | ||
| invoke: "sendMessage", | ||
| text: payload.text, | ||
| images: payload.images, | ||
| }) | ||
| }, 200) | ||
| } | ||
|
|
||
| this.log(`Task ${payload.taskId} successfully resumed and is now active`) | ||
| return | ||
| } | ||
| } catch (error) { | ||
| this.log(`Failed to resume task ${payload.taskId}: ${error}. Creating new task instead.`) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When task resumption fails, we silently fall back to creating a new task. Should we notify the user that their requested task couldn't be resumed? This might be confusing if they expected to continue a specific task. |
||
| } | ||
| } | ||
|
|
||
| // Create new task (normal flow) | ||
| await this.initClineWithTask(payload.text, payload.images) | ||
| } | ||
|
|
||
| public async initClineWithHistoryItem(historyItem: HistoryItem & { rootTask?: Task; parentTask?: Task }) { | ||
| await this.removeClineFromStack() | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,12 +97,25 @@ export class API extends EventEmitter<RooCodeEvents> implements RooCodeAPI { | |
| text, | ||
| images, | ||
| newTab, | ||
| taskId, | ||
| }: { | ||
| configuration: RooCodeSettings | ||
| text?: string | ||
| images?: string[] | ||
| newTab?: boolean | ||
| taskId?: string | ||
| }) { | ||
| // If taskId is provided, attempt to resume the task | ||
| if (taskId) { | ||
| const isInHistory = await this.isTaskInHistory(taskId) | ||
| if (isInHistory) { | ||
| await this.resumeTask(taskId) | ||
| return taskId | ||
| } | ||
| // If task is not found in history, continue with creating a new task | ||
| this.log(`[API] Task ${taskId} not found in history, creating new task`) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function returns the taskId on success, but what happens when resumption fails and we fall back to creating a new task? The caller won't know that resumption failed. Consider returning a different value or throwing an error to indicate resumption failed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that when a taskId isn't found in history, we fall back to creating a new task. This is a nice graceful fallback! One thought: since both paths return a taskId string, callers might not know whether resumption succeeded or a new task was created. Would it be useful to return something like so callers can handle each case if needed? |
||
| } | ||
|
|
||
| let provider: ClineProvider | ||
|
|
||
| if (newTab) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is helpful, but consider adding JSDoc documentation for this new field to explain when and how it should be used. This would help other developers understand the resumption flow better.