-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat: implement task resumption from web UI #7116
Conversation
- Add optional taskId field to StartNewTask IPC command - Update API handler to resume tasks when taskId is provided - Add handleStartTask method to ClineProvider for web UI integration - Ensure bridge service properly connects to resumed tasks - Fix message routing to correct task after resumption
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.
Thank you for your contribution! I've reviewed the changes and found several issues that need attention before this can be merged.
| text: z.string(), | ||
| images: z.array(z.string()).optional(), | ||
| newTab: z.boolean().optional(), | ||
| taskId: z.string().optional(), // Optional taskId for resuming existing tasks |
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.
| // after the task is restored and ready | ||
| if (payload.text) { | ||
| // Give a small delay to ensure the task is fully restored | ||
| setTimeout(async () => { |
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.
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.
| const resumedTask = this.getCurrentCline() | ||
| if (resumedTask) { | ||
| try { | ||
| const { ExtensionBridgeService } = await import("@roo-code/cloud") |
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.
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?
| return | ||
| } | ||
| } catch (error) { | ||
| this.log(`Failed to resume task ${payload.taskId}: ${error}. Creating new task instead.`) |
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.
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.
| 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`) |
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 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.
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.
Thank you @jdilla1277!
| * | ||
| * @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> { |
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.
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!
| text: z.string(), | ||
| images: z.array(z.string()).optional(), | ||
| newTab: z.boolean().optional(), | ||
| taskId: z.string().optional(), // Optional taskId for resuming existing tasks |
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.
Would it be helpful to add JSDoc documentation for this new field? It could help future developers understand the resumption flow better:
| // after the task is restored and ready | ||
| if (payload.text) { | ||
| // Give a small delay to ensure the task is fully restored | ||
| setTimeout(async () => { |
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.
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.
| const resumedTask = this.getCurrentCline() | ||
| if (resumedTask) { | ||
| try { | ||
| const { ExtensionBridgeService } = await import("@roo-code/cloud") |
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.
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?
| 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`) |
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.
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?
Related GitHub Issue
Closes: #
Roo Code Task Context (Optional)
Description
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
Implement task resumption from web UI by adding
taskIdtoStartNewTaskcommand and updating API andClineProviderto handle resumption.taskIdfield toStartNewTaskcommand inipc.tsfor task resumption.startNewTask()inapi.tsto resume tasks iftaskIdis provided.handleStartTask()inClineProvider.tsto handle task resumption from web UI.handleStartTask().ClineProvider.ts.This description was created by
for 3293d78. You can customize this summary. It will automatically update as commits are pushed.