-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Enhance Diff View #7841
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
Enhance Diff View #7841
Conversation
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 that this is a solid implementation of the diff view enhancements. The three new modes (from-init, to-current, and full) provide valuable functionality for viewing checkpoint differences. I have some suggestions for improvement, particularly around edge case handling.
| const checkpoints = task.clineMessages.filter(({ say }) => say === "checkpoint_saved").map(({ text }) => text!) | ||
|
|
||
| const idx = checkpoints.indexOf(commitHash) | ||
| switch (mode) { |
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.
Consider extracting this switch logic into a separate function for better testability and readability. Something like:
This would make the logic easier to unit test independently.
…pdate tooltip text in ChatRow
|
Hey @NaccOll I like this idea! It does worry me that the "View Diff With First Checkpoint" button might be a bit too prominent and also a bit confusing, should we look into a different way of showing the diff at the end of the task? I would love to have some feedback from @brunobergher. |
Once the team has final ideas on the UI, let me know and I'll make changes. |
|
I can see the value in this, but I worry about adding so many buttons to a UI element which appears multiple times in a conversation, especially for a relatively secondary scenario (basic diffs are probably enough for 80% of the case, and the remainder would be served by this). I'd propose moving the selector of the type of differ elsewhere
|
This sounds like a major UI/UX overhaul, and I'll just have to wait for the team to finish it if they choose to do it.
This is a small adjustment, but there seems to be no mention of the logic of how full mode should be triggered. Perhaps I could create a checkpoint before calling attempt_completion to ensure UI consistency? @daniel-lxs What do you think about this? |
|
I'm not sure what the best way to do this. Bruno has more knowledge about what this should look like and where to place it. I would recommend following either one of Bruno's recommendation since ultimately he is making the calls on how the UI should look like nowadays. |
Related GitHub Issue
Relate: #7285
Description
The diff view only shows the differences between the current checkpoint and the next one.
This PR enhances the diff view by adding three modes.
full: Shows the changes between the first checkpoint and the current workspace.
Currently, this can only be triggered via the button that appears when a task is completed.(Delete this button according to the discussion)from-init: Shows the changes between the first checkpoint and the selected checkpoint. This can be triggered from any checkpoint menu.
to-current: Shows the changes between the selected checkpoint and the current checkpoint. This can be triggered from any checkpoint menu.
Test Procedure
Simply turn on the checkpoint feature and trigger the file modification with llm. The checkpoint menu will have a more button, click it to see two new view buttons
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Need to update the documentation description of the checkpoint section
Additional Notes
Get in Touch
NaccOll
Important
Enhance diff view with new modes
full,from-init, andto-currentfor improved checkpoint comparisons.full,from-init, andto-currentincheckpointDifffunction inindex.ts.full: Compares from the first checkpoint to the current workspace.from-init: Compares from the first checkpoint to the selected checkpoint.to-current: Compares from the selected checkpoint to the current workspace.CheckpointMenu.tsxto include options for new diff modes.hasCheckpointprop toChatRow.tsxandChatView.tsxto manage UI state.checkpoint.test.tsto testto-currentmode.WebviewMessage.tsto include new modes incheckoutDiffPayloadSchema.This description was created by
for 588fae7. You can customize this summary. It will automatically update as commits are pushed.