-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: auto-scroll to edit location after user edits in diff view #7997
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
- Added ref to user_feedback_diff element - Implemented smooth scroll behavior when user edits appear - Scrolls to center of viewport for better visibility Fixes #7996
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| if (tool?.diff) { | ||
| // Use a small delay to ensure the element is rendered | ||
| setTimeout(() => { | ||
| userEditRef.current?.scrollIntoView({ |
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 intentional? The code assumes scrollIntoView is always available, but it might not be supported in all environments (e.g., during SSR or in test environments). Consider adding a safety check:
| userEditRef.current?.scrollIntoView({ | |
| if (userEditRef.current?.scrollIntoView) { | |
| userEditRef.current.scrollIntoView({ | |
| behavior: "smooth", | |
| block: "center", | |
| }) | |
| } |
| // Scroll to user edits when they appear | ||
| useEffect(() => { | ||
| if (message.say === "user_feedback_diff" && userEditRef.current) { | ||
| const tool = safeJsonParse<ClineSayTool>(message.text) |
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.
Could we improve performance here? This useEffect runs on every render when message.say === "user_feedback_diff". The tool is parsed twice - once in the condition check and once inside. Consider memoizing the parsed tool to avoid unnecessary JSON parsing on each render.
| const tool = safeJsonParse<ClineSayTool>(message.text) | ||
| if (tool?.diff) { | ||
| // Use a small delay to ensure the element is rendered | ||
| setTimeout(() => { |
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.
Minor suggestion: Consider extracting this magic number to a named constant like SCROLL_DELAY_MS = 100 at the top of the file for better maintainability and clarity about its purpose.
Summary
This PR implements automatic scrolling to the edit location when users make manual edits in the diff view and approve changes, improving the user experience as described in #7996.
Changes
Technical Details
The implementation:
Testing
Related Issue
Fixes #7996
Future Considerations
As suggested in the review, we might consider adding a user preference setting to disable auto-scroll if some users find it disruptive. The current implementation should work well across different viewport sizes with the 'center' block alignment.
Screenshots/Demo
The feature automatically scrolls to show user edits in the center of the viewport after they are applied, making it easier to see and continue working with the changes.
Important
Adds auto-scroll to edit location in diff view after user edits in
ChatRow.tsx, triggered byuser_feedback_diffmessage.user_feedback_diffmessage.userEditRefusinguseRefinChatRowContent.useEffectinChatRowContentlistens foruser_feedback_diffand scrolls touserEditRef.This description was created by
for 28cf54b. You can customize this summary. It will automatically update as commits are pushed.