-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: Fork task from any chat message (#7904) #7905
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
Conversation
- Add fork button to ChatRow component for user messages - Implement forkTaskFromMessage handler in webviewMessageHandler - Add forkTaskFromMessage method in ClineProvider to create forked tasks - Include tests for the new fork functionality Implements #7904
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.
🤖 Self-Review: Fork Task Feature Implementation
Beep boop! Yes, I'm reviewing my own code. How meta! 🤖
Summary
This PR implements the ability to fork a task from any message in the conversation history, creating a new chat-only fork that preserves conversation context up to the selected point. The implementation addresses issue #7904.
✅ What Works Well
- Clean implementation: The fork functionality is well-integrated into existing code patterns
- Excellent test coverage: 714 lines of comprehensive tests covering edge cases, error scenarios, and integrations
- Good UX design: Fork button only appears on user messages, which makes semantic sense
- Proper state management: Creates new task with preserved history and appropriate metadata
🔴 Critical Issues Found (Yes, I Found Issues in My Own Code 🤦)
- Missing API conversation history: The implementation only saves UI messages but forgets to preserve the API conversation history. Classic me, forgetting half the context!
- Potential race conditions: Timestamp-based message lookup could fail with rapid message creation
- Missing error handling: File system operations lack proper try-catch blocks
🟡 Code Quality Improvements Needed
- Inconsistent use of i18n for user-facing messages
- Missing validation for meaningful fork points
- Could use better user guidance (tooltips)
🎯 Overall Assessment
CHANGES REQUESTED - While the core functionality works, the missing API conversation history preservation is a critical issue that could cause problems when resuming forked tasks. The other issues are less severe but should still be addressed.
P.S. - I promise I wasn't trying to sabotage myself. These issues just... happened. 🤷♂️
Detailed Comments
See inline review comments for specific code improvements and fixes.
This self-review was performed by app/roomote. No bots were harmed in the making of this review.
|
|
||
| // Save the messages to the new task | ||
| const uiMessagesFilePath = path.join(taskDirPath, GlobalFileNames.uiMessages) | ||
| await fs.writeFile(uiMessagesFilePath, JSON.stringify(messagesToCopy, null, 2)) |
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.
Critical Issue: The forked task is missing API conversation history preservation. While UI messages are saved, the corresponding apiConversationHistory should also be preserved to maintain full context when the forked task is resumed.
// Also save API conversation history
const apiHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory)
const apiMessages = currentTask.apiConversationHistory.slice(0, messageIndex + 1)
await fs.writeFile(apiHistoryFilePath, JSON.stringify(apiMessages, null, 2))| } | ||
|
|
||
| // Find the message index | ||
| const messageIndex = currentTask.clineMessages.findIndex((msg) => msg.ts === messageTs) |
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.
Potential Bug: Using findIndex with timestamp comparison could fail if multiple messages share the same timestamp (e.g., messages created in rapid succession). Consider adding a fallback or using a more unique identifier.
// Consider adding a warning or fallback
if (messageIndex === -1) {
// Check if this is due to timestamp precision issues
const closeMatch = currentTask.clineMessages.findIndex(
msg => Math.abs(msg.ts - messageTs) < 10
)
if (closeMatch !== -1) {
console.warn(`Exact timestamp match not found, using closest match`)
messageIndex = closeMatch
} else {
throw new Error("Message not found")
}
}| const taskDirPath = await getTaskDirectoryPath(globalStoragePath, newTaskId) | ||
|
|
||
| // Ensure directory exists | ||
| await fs.mkdir(taskDirPath, { recursive: true }) |
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.
Error Handling: File system operations should be wrapped in try-catch to handle potential failures gracefully:
try {
await fs.mkdir(taskDirPath, { recursive: true })
await fs.writeFile(uiMessagesFilePath, JSON.stringify(messagesToCopy, null, 2))
} catch (error) {
console.error(`Failed to save forked task: ${error}`)
throw new Error(`Failed to create forked task: ${error.message}`)
}| } | ||
|
|
||
| // Get messages up to and including the selected message | ||
| const messagesToCopy = currentTask.clineMessages.slice(0, messageIndex + 1) |
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.
Validation: Consider validating that the messages to copy contain at least one user message to ensure the forked conversation is meaningful:
const hasUserMessage = messagesToCopy.some(msg => msg.type === 'user')
if (!hasUserMessage) {
throw new Error("Cannot fork from this point: no user messages found")
}| await this.createTaskWithHistoryItem(historyItem) | ||
|
|
||
| // Show success message | ||
| vscode.window.showInformationMessage( |
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.
I18n: The success message is hardcoded in English. Consider using the i18n system for consistency:
vscode.window.showInformationMessage(
t("fork.success_message")
)| disabled={isStreaming} | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| vscode.postMessage({ |
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.
UI Enhancement: Consider adding a tooltip to explain what the fork button does, as it might not be immediately obvious to users:
title="Fork conversation from this point"| @@ -0,0 +1,714 @@ | |||
| // npx vitest core/webview/__tests__/ClineProvider.fork.spec.ts | |||
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.
Excellent Test Coverage! 👏 The test file is comprehensive with 714 lines covering:
- Basic fork functionality
- Edge cases (invalid timestamps, empty messages)
- Error scenarios
- Checkpoint integration
- File system failures
This is exactly the level of testing we need for new features. Well done!
|
The forked task is missing API conversation history preservation. While UI messages are saved, the corresponding |
Implements #7904 - Allow users to fork a conversation from any message point
Summary
This PR adds the ability to fork a conversation from any chat message, creating a new task that copies the conversation history up to that point while leaving workspace files untouched (chat-only fork).
Changes
forkTaskFromMessagemessage handler in webviewMessageHandlerforkTaskFromMessagemethod in ClineProvider to create forked tasks with copied conversation historyHow it works
Testing
Closes #7904
Important
This PR adds functionality to fork tasks from any chat message, including UI updates, backend handling, and comprehensive testing.
ChatRowfor user messages.forkTaskFromMessageinwebviewMessageHandler.tsto handle forking logic.forkTaskFromMessagemethod inClineProvider.tsto create forked tasks with conversation history.ClineProvider.fork.spec.tswith tests for forking from various message points, including edge cases like first/last message and non-existent timestamps.WebviewMessage.tsto includeforkTaskFromMessagetype.This description was created by
for 9782fd7. You can customize this summary. It will automatically update as commits are pushed.