-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Files Changed Overview (Rebased from #5626) #7816
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
update checkpoint test verbiage and don't suppress messages in testing Pulled all I could from fco clean and got it into a working state. The changes implement a Files Changed Overview (FCO) feature that tracks and displays file modifications made by │ │ │ │ the AI assistant during task execution. This is a major feature addition with: │ │ │ │ │ │ │ │ - New experiment flag: filesChangedOverview to control the feature │ │ │ │ - Core service layer: FileChangeManager and FCOMessageHandler for managing file change state │ │ │ │ - Checkpoint integration: Enhanced checkpoint system to work with FCO for diff calculations │ │ │ │ - UI components: React-based FilesChangedOverview component with virtualization │ │ │ │ - Message system: New message types for FCO communication between webview and backend │ │ │ │ - Tool integration: All file editing tools now track changes for FCO │ │ │ │ - Type definitions: New TypeScript types for FileChange, FileChangeset, etc. │ │ │ │ - Internationalization: Translation files added for all supported languages final changes for fco fixing type checks after rebase
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 this substantial contribution! The Files Changed Overview feature is a valuable addition that addresses a real need for better control over agent-made modifications. The implementation shows good code quality with comprehensive tests and proper TypeScript typing.
I've identified some issues that need attention before merging, particularly around the missing translation files and the checkpoint system changes. Please see my inline comments for specific suggestions.
| * through VS Code message passing. | ||
| */ | ||
| const FilesChangedOverview: React.FC = () => { | ||
| const { t } = useTranslation() |
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.
Missing Translation Files: The component uses translation keys like file-changes:summary.count_with_changes but I cannot find the corresponding translation JSON files mentioned in the PR description. Could you verify that all 18 locale translation files are included in the PR?
Without these files, the UI will show translation keys instead of proper text for users.
| ) | ||
| return true | ||
| const errorMessage = error instanceof Error ? error.message : String(error) | ||
|
|
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.
Breaking Change Concern: The switch from git-based to .roo directory for shadow checkpoints is a significant architectural change. Is there a migration path for users with existing git-based checkpoints? This could potentially break existing workflows.
Consider adding migration logic or at least documenting the upgrade path.
| }) | ||
|
|
||
| // Clean up temporary files after a delay | ||
| 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.
Potential Memory Leak: Temporary files are cleaned up after 30 seconds, but if users view many diffs rapidly, temp files could accumulate.
Consider tracking open diff views and cleaning up immediately when they're closed, or implementing a cleanup queue to manage temp files more efficiently.
| */ | ||
| export class FileChangeManager { | ||
| private changeset: FileChangeset | ||
| private acceptedBaselines: Map<string, string> // uri -> baseline checkpoint (for both accept and reject) |
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.
State Consistency: The FileChangeManager maintains complex state with acceptedBaselines Map and file tracking. Consider adding validation methods to ensure state consistency, especially after operations that modify the baseline or accepted files.
| // Detect when FCO is enabled mid-task and request fresh file changes | ||
| React.useEffect(() => { | ||
| const prevEnabled = prevFilesChangedEnabledRef.current | ||
| const currentEnabled = filesChangedEnabled |
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.
Race Condition Risk: The FCO enablement detection by comparing previous and current state could miss rapid toggles. Consider using a more robust state transition detection mechanism, such as a state machine or transition tracking to prevent edge cases where rapid setting changes could leave the component in an inconsistent state.
| } | ||
|
|
||
| // Check if the file was newly created (didn't exist in the fromCheckpoint) | ||
| if (!previousContent) { |
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 Recovery: The file reversion in revertFileToCheckpoint could leave files in an inconsistent state if it fails mid-operation. Consider implementing atomic operations with backup/restore mechanisms to ensure files can be recovered if the reversion process fails.
| * Calculate line differences between two file contents | ||
| * Uses a simple line-by-line comparison to count actual changes | ||
| */ | ||
| public static calculateLineDifferences( |
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.
Performance Suggestion: For large files, the simple line-by-line comparison in calculateLineDifferences could be slow. Consider using a more efficient diff algorithm like Myers' algorithm or leveraging existing diff libraries for better performance and accuracy.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
|
outside of scope, closing |
This is a rebased version of PR #5626 to resolve merge conflicts.
Original PR: #5626
Original Title: Files Changed Overview (FCO)
What was done:
Closes Issue: #4454 when merged.
Description
This adds a collapsable window in chat to show files that have been modified by agents.
Features include accepting changes, diffing and rejecting file changes outright.
This prevents you from needing to keep git open or going back to a previous checkpoint after every big modifications to the codebase. I definitely had times where some good changes have happened but then the agent decides theya ren't done! You can easily reject some of the changes instead of all of them. If you don't like this feature, you can also turn it off in the settings menu.
Checkpoints system did need modifications to get this to work properly. You can see what was done there and let me know what you think.
ShadowCheckpoints are now stored in a .roo folder in root. This prevents the need to search for all git files and turn off the system if some are found not in root. This was a real bummer when I went to use roo on larger codebases that have .gits throughout.
Test Procedure
run the following tests:
src/core/webview/tests/ClineProvider.spec.ts
src/services/checkpoints/tests/ShadowCheckpointService.spec.ts
src/services/file-changes/tests/FileChangeManager.test.ts
webview-ui/src/components/file-changes/tests/FilesChangedOverview.spec.tsx
webview-ui/src/components/settings/tests/UISettings.spec.tsx
webview-ui/src/context/tests/ExtensionStateContext.spec.tsx
check all languages exist.
open chat and ask it to modify some files, make new files, different types of tool calls, etc. accept and reject the changes
open settings menu and under interfaces, turn off the feature, go through the steps again
turn on the feature on last time and go through the steps again.
make sure checkpoints still work.
when restoring a checkpoint, fco clears and disappears.
Pre-Submission Checklist
Screenshots / Videos
Before:
After Checkpoint is hit:
Settings option:
Documentation Updates
You can add a line like, "After every checkpoint, Files changed will appear in chat. You have the option to accept or reject them on a file by file basis. to turn this feature off, find it in the settings menu" but I don't think it makes much to consider it significant.
Additional Notes
urisparameter to WebviewMessage typeGet in Touch
Playcations on Discord
Important
Introduces a Files Changed Overview feature with UI component, checkpoint integration, and comprehensive tests, supporting 18 locales.
FilesChangedOverviewcomponent to display modified files in a collapsible UI.FCOMessageHandler,FileChangeManager, andFilesChangedOverview.ExtensionStateContextto manage FCO state.This description was created by
for 64ab719. You can customize this summary. It will automatically update as commits are pushed.