-
Notifications
You must be signed in to change notification settings - Fork 49
Modified Files Tracker component #444
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
base: feature/upgrade-modified-files-ux
Are you sure you want to change the base?
Modified Files Tracker component #444
Conversation
… etc. consistent with initial commit for LS
… etc. consistent with initial commit for LS
…le in diff mode. Logging enabled, disable later. Undo buttons still not working
…le in diff mode. Logging enabled, disable later. Undo buttons still not working
…le in diff mode. Logging enabled, disable later. Undo buttons still not working
…le in diff mode. Logging enabled, disable later. Undo buttons still not working
…le in diff mode. Logging enabled, disable later. Undo buttons still not working
- Currently gets file details just like chatItemCard - Files are listed but other functionalities are lacking
- Currently gets file details just like chatItemCard - Files are listed but other functionalities are lacking
- undo buttons are showing per file - Upon clicking the button it undoes the chnages correctly
- Added chat=wrapper support to handle title - Should ideally be controller but no title property in chatMessage
example/src/main.ts
Outdated
| }); | ||
|
|
||
| // Demo will be handled through chatItem approach now | ||
| mynahUI.addChatItem(tabId, { |
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.
Just curious: Why we need two addChatItem here?
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 will remove this and find a way to provide demo differently
example/src/samples/sample-data.ts
Outdated
| details: { | ||
| 'Running': { | ||
| description: 'Work in progress!', | ||
| description: 'Work in progress...', |
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.
not necessary!
| this.hideTooltip(); | ||
| if (this.props.details?.clickable !== false) { | ||
| const fileMessageId = this.props.details?.data?.messageId ?? this.props.messageId; | ||
| console.log('[ChatItemTreeFile] File clicked - originalFilePath:', this.props.originalFilePath); |
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.
Why do we need multiple console logs here? Can we combine into one 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.
Will remove unnecessary logging
- Added session functionality for every new chat the modified files are getting cleared
achieves all the functionalities needed might need testing and use set for storing files needs more styling, refer previous versions
- removed previous unnecesary edits - removed example for now
- Switched buttons to header and kept undoall button at top level
6abed0a to
5535430
Compare
… height. Temporarily updated the snapshots. Once the component is made invisible, will revert to correct snapshots
- Aligned the per file undo buttons correctly - Set undoall button to the right for view consistency
- Fixed on file click propogation, so the component doesn't collapse now unexpectedly - Removed unnecessary code changes from previous PR feedback
- Fixed failing unit tests due to new changes for preventing collapsing
| this.renderModifiedFiles(null); | ||
| } | ||
|
|
||
| public setVisible (visible: boolean): 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.
Do we need this function?
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.
Will remove it for now, as I am no longer marking the component invisible yet
| // Normal flow on initially opening ui requires the currentMessageId; | ||
| this.removeEmptyCardsAndFollowups(); | ||
| const currentMessageId: string = (chatItem.messageId != null && chatItem.messageId !== '') ? chatItem.messageId : `TEMP_${generateUID()}`; | ||
| // Check if messageId contains "modified-files-" prefix |
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.
Can you add this to documentation or add some comments why we are checking for this prefix?
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.
Added comments for now
| if (chatItem.header?.fileList !== null && chatItem.header?.fileList !== undefined) { | ||
| this.allRenderedModifiedFileChatItems[chatItem.messageId] = chatItem; | ||
| const size = Object.keys(this.allRenderedModifiedFileChatItems).length; | ||
| chatItem.title = size === 1 ? '1 file modified!' : `${size} files modified!`; |
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.
nit:
chatItem.title = `${fileCount} file${fileCount === 1 ? '' : 's'} modified!`;
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.
Changed
|
|
||
| if (chatItem.type === ChatItemType.PROMPT || chatItem.type === ChatItemType.SYSTEM_PROMPT) { | ||
| // Clear modified files tracker on new prompt | ||
| this.allRenderedModifiedFileChatItems = {}; |
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.
not a blocker but this does not work if we introduce intermediate input handling from the user.
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.
Ack
| render: ExtendedHTMLElement; | ||
| private readonly props: ModifiedFilesTrackerProps; | ||
| private readonly collapsibleContent: CollapsibleContent; | ||
| public titleText: string = 'No files modified!'; |
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.
Was this a default value ? Can we reset this from LS?
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.
Currently we do not have any property in chatMessage, that can be used for titleText
| children: [ this.collapsibleContent.render ] | ||
| }); | ||
|
|
||
| console.log('[ModifiedFilesTracker] Render element created:', this.render); |
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.
Do we need this console.log?
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.
removed
|
|
||
| public clear (): void { | ||
| this.allFiles.clear(); | ||
| this.titleText = 'No files modified!'; |
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.
Can you add const and assign this !
| if (chatItem.messageId !== undefined && chatItem.messageId !== null && chatItem.messageId !== '') { | ||
| undoAllMessageId = this.getOriginalMessageId(chatItem.messageId); | ||
| } else { | ||
| return; |
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.
Add early return
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.
Done!
Problem
Modified files are scattered across multiple chatItem cards and can seem lost to users if too many files are modified and has verbose explanation. Needed access to all files modified as part of the conversation in one place.
Solution
Created a new separate component which renders just modified files along with undo button per each file. Still working on styling the component a little better.
10.17.2025.mov
ToDos :
Tests
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.