Skip to content
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

feat(vscode): support attach file context to inline edit #3766

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

zhanba
Copy link
Contributor

@zhanba zhanba commented Jan 27, 2025

Feature

image

image

image

Need help

  • how we update prompt template to include file content

@zhanba zhanba changed the title feat: support attach file context to inline edit feat(vscode): support attach file context to inline edit Jan 27, 2025
@zhanba zhanba force-pushed the feat-file-context-chat branch 3 times, most recently from 08cdcf0 to 9a06bf4 Compare January 28, 2025 07:30
@zhanba zhanba marked this pull request as ready for review January 28, 2025 07:44
@@ -189,13 +207,16 @@ export class ChatEditProvider implements Feature {
return userCommand;
case "{{languageId}}":
return document.languageId;
case "{{fileContext}}":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wsxiaoys we should update prompt template to include {{fileContext}} part. Can you guide me on that?

@icycodes
Copy link
Member

icycodes commented Feb 5, 2025

Hi @zhanba, Thank you for the PR!

The current design is good, but I have some concerns.

Use the same or seperated quick pick widget

The current design use the same quick pick widget for input commands and selecting files, but

  1. The quick pick widget auto-filters items by the current value. If we include both command list and file list in one quick pick list, all items need to be marked as alwaysShow to be visible, which may not be the ideal behavior of a quick pick widget.
  2. The modal stack is not clear if we blend the quick pick widget. When the user triggers file selection, accepting (Enter or Click on item) will complete the file selection and continue the input command, but dismissing (Esc) will stop the input command instead of continue. By use a new quick pick widget for file selection, we can temporarily hide the command quick pick widget and show the file quick pick widget. Once it is hidden (by accept or dismiss), we update and show the command quick pick widget again to continue.

So, I suggest using a new quick pick for file selection with pushing modal stack.

How to trigger file selection

  1. Using @:
    This keeps the UX consistent with the chat side panel. But, to achieve this in a quick pick widget, we need some tricky processing. Detecting whether the last character is @ to trigger should be good enough. However, there are some cons:
    • The @file is not highlighted to mark it includes a file context.
    • When the user moves the cursor and is not at the end and inputs something, the @ is not properly triggered.
  2. Adding a button to trigger selecting files:
    This avoids a lot of tricky implementations but is not as convenient as using @.
  3. Using a more customizable editor for inputting command:
    This should solve these problems in a cleaner way but requires a side view instead of using the quick pick widget.

I suggest keeping plan 1 for now.

The resolved data interface for the promise of showQuickPick

I think the @file should be kept in the command, as it can be part of a command. For example:
A command could be Fix the use of function foo according to @foo.ts, where @foo.ts is included by selecting a file. Then the promise of showQuickPick should resolve as:

{
   "command": "Fix the use of function foo according to @foo.ts",
   "fileContexts": [{
      "referer": "@foo.ts",
      "uri": "file://path/to/foo.ts"
   }]
}

Let me know if you have any thoughts about these points.
Also cc @wsxiaoys for design review.

@zhanba
Copy link
Contributor Author

zhanba commented Feb 6, 2025

@icycodes thanks for your reply with so many details!

The quick pick widget auto-filters items by the current value. If we include both command list and file list in one quick pick list, all items need to be marked as alwaysShow to be visible, which may not be the ideal behavior of a quick pick widget.

Use alwaysShow and filter items by ourself is acceptable, the problem is that we can't handle the sort of items, so the order of items may be inconsistent in a few case.

The modal stack is not clear if we blend the quick pick widget. When the user triggers file selection, accepting (Enter or Click on item) will complete the file selection and continue the input command, but dismissing (Esc) will stop the input command instead of continue. By use a new quick pick widget for file selection, we can temporarily hide the command quick pick widget and show the file quick pick widget. Once it is hidden (by accept or dismiss), we update and show the command quick pick widget again to continue.

I'm not sure if esc on file selection stop input would make real trouble for user. By using two quick pick, we can avoid this problem. My concern for two quick pick is that this may interrupt the user input and cause some context switch between two input which may confusing user.

On How to trigger file selection, I'm agree with you. Using @ is convenient but with some limitation due to the limited capability of quick pick widget.

On The resolved data interface for the promise of showQuickPick, I'm agree with you and I will update command to include the @file part.

cc @wsxiaoys

@zhanba
Copy link
Contributor Author

zhanba commented Feb 6, 2025

Hi @zhanba, Thank you for the PR!

The current design is good, but I have some concerns.

Use the same or seperated quick pick widget

The current design use the same quick pick widget for input commands and selecting files, but

  1. The quick pick widget auto-filters items by the current value. If we include both command list and file list in one quick pick list, all items need to be marked as alwaysShow to be visible, which may not be the ideal behavior of a quick pick widget.
  2. The modal stack is not clear if we blend the quick pick widget. When the user triggers file selection, accepting (Enter or Click on item) will complete the file selection and continue the input command, but dismissing (Esc) will stop the input command instead of continue. By use a new quick pick widget for file selection, we can temporarily hide the command quick pick widget and show the file quick pick widget. Once it is hidden (by accept or dismiss), we update and show the command quick pick widget again to continue.

So, I suggest using a new quick pick for file selection with pushing modal stack.

How to trigger file selection

  1. Using @:
    This keeps the UX consistent with the chat side panel. But, to achieve this in a quick pick widget, we need some tricky processing. Detecting whether the last character is @ to trigger should be good enough. However, there are some cons:

    • The @file is not highlighted to mark it includes a file context.
    • When the user moves the cursor and is not at the end and inputs something, the @ is not properly triggered.
  2. Adding a button to trigger selecting files:
    This avoids a lot of tricky implementations but is not as convenient as using @.

  3. Using a more customizable editor for inputting command:
    This should solve these problems in a cleaner way but requires a side view instead of using the quick pick widget.

I suggest keeping plan 1 for now.

The resolved data interface for the promise of showQuickPick

I think the @file should be kept in the command, as it can be part of a command. For example: A command could be Fix the use of function foo according to @foo.ts, where @foo.ts is included by selecting a file. Then the promise of showQuickPick should resolve as:

{
   "command": "Fix the use of function foo according to @foo.ts",
   "fileContexts": [{
      "referer": "@foo.ts",
      "uri": "file://path/to/foo.ts"
   }]
}

Let me know if you have any thoughts about these points. Also cc @wsxiaoys for design review.

I will try to use two quick pick widget and see how it works.

@zhanba
Copy link
Contributor Author

zhanba commented Feb 6, 2025

two quick pick demo:
https://jam.dev/c/ce025a95-397d-47f4-8401-d9c41e56f6b8

@icycodes
Copy link
Member

icycodes commented Feb 7, 2025

two quick pick demo: https://jam.dev/c/ce025a95-397d-47f4-8401-d9c41e56f6b8

Thank you for the quick update. This demo aligns with my thoughts, it looks much better to me.

@zhanba
Copy link
Contributor Author

zhanba commented Feb 7, 2025

add smooth file deletion:
https://jam.dev/c/41e820ac-9f92-4736-b113-e21f8f9a7d39

@zhanba zhanba force-pushed the feat-file-context-chat branch 5 times, most recently from 6609f06 to e73ceea Compare February 8, 2025 01:10
@zhanba zhanba force-pushed the feat-file-context-chat branch 4 times, most recently from a5fefec to e748c93 Compare February 10, 2025 12:14
@zhanba zhanba force-pushed the feat-file-context-chat branch 4 times, most recently from 37b05ae to ecb169a Compare February 11, 2025 01:11
@icycodes
Copy link
Member

A map of label to file can't make this parseUserCommand just return string, because user may delete file mentions in command.

Before resolving the InlineEditComment, we can filter the context list by checking if the referer is still a substring included in the command.
This is just my idea. The current implementaion is also good and acceptable.

@icycodes
Copy link
Member

Otherwise LGTM. Please rebase on main and resolve confilicts.

@zhanba zhanba force-pushed the feat-file-context-chat branch 2 times, most recently from 947f3c5 to 4c4be75 Compare February 11, 2025 12:15
@zhanba zhanba force-pushed the feat-file-context-chat branch from 4c4be75 to 99a140f Compare February 11, 2025 12:21
@zhanba
Copy link
Contributor Author

zhanba commented Feb 11, 2025

Otherwise LGTM. Please rebase on main and resolve confilicts.

Rebased and Fixed

@icycodes
Copy link
Member

The next task is to update the chat prompt template to include file context.
I think it can be done in a separate PR.
cc @wsxiaoys

@wsxiaoys wsxiaoys merged commit ef96594 into TabbyML:main Feb 12, 2025
4 checks passed
@zhanba zhanba deleted the feat-file-context-chat branch February 12, 2025 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants