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

Fuzzy search for @-ing files and symbols #1889

Merged
merged 40 commits into from
Nov 28, 2023
Merged

Conversation

toolmantim
Copy link
Contributor

@toolmantim toolmantim commented Nov 27, 2023

This updates the @-file UI with cleaned up styles, fuzzy matching, and putting symbol search behind it's own @# syntax.

Fixes #1838
Fixes #1837
Fixes #1840
Replaces #1851

Changes:

  • Shows the UI immediately after typing @
  • Does fuzzy matching, similar to VS Code’s finder, over files and symbols
  • Symbol search is behind @#
  • Symbol search API is finicky and can easily return zero results, so added a little UI hint to help them (alternatively: we could disable it and do per-file drill-down instead like originally intended)
  • Rewrite labels and clean up styles
  • Handle file search cancellations properly
  • De-dupe tab file names
  • Tests

Not included:

  • Handling of large files
  • Drilling down from a file to its symbols (i.e. @some-file.ts#SymbolName)
Before After
Screenshot 2023-11-27 at 7 24 48 pm Screenshot 2023-11-27 at 7 33 11 pm
Screenshot 2023-11-27 at 7 24 03 pm Screenshot 2023-11-27 at 7 33 31 pm
Screenshot 2023-11-27 at 7 23 34 pm Screenshot 2023-11-27 at 7 33 46 pm
Screenshot 2023-11-27 at 7 23 04 pm
Screenshot 2023-11-27 at 5 10 26 pm Screenshot 2023-11-27 at 7 34 54 pm
Screenshot 2023-11-27 at 7 23 24 pm
Screenshot 2023-11-27 at 8 19 15 pm

Test plan

  • Manual:
    • Tested on sourcegraph/cody, soucregraph/sourcegraph & chromium/chromium
    • Open the repo, start a chat, type @
    • Tested both sidebar chat UI and new chat UI
  • Automated tests

@@ -1181,6 +1181,7 @@
"classnames": "^2.3.2",
"date-fns": "^2.30.0",
"detect-indent": "^7.0.1",
"fuzzysort": "^2.0.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a few but this was the fastest and smallest

@toolmantim toolmantim requested a review from a team November 27, 2023 10:12
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Great job on supporting fuzzy search! It doesn't fixes the issue mentioned in #1837 as I mentioned in #1851 that the only one to get perfectly consistent results is to remove the limit for MAX_RESULTS which is currently set to 20 in your branch. You can see the last item is sometimes different when doing the same search when testing from your branch:

Screen.Recording.2023-11-27.at.10.54.28.AM.mov

Other than that, I think we should group the editorTab into ContextFileType and this should be good to go? Wdyt?

lib/shared/src/codebase-context/messages.ts Outdated Show resolved Hide resolved
vscode/src/editor/utils/editor-context.ts Outdated Show resolved Hide resolved
{contextSelection?.length ? (
<div className={classNames(styles.selectionsContainer)}>
{contextSelection?.map((match, i) => {
const icon =
Copy link
Contributor

Choose a reason for hiding this comment

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

is it intended to not display icon for file results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, unless we can do extension-specific file icons (there's no support for that in webviews is there?) there's no need, especially now that files aren't intermingled with symbols.

@toolmantim
Copy link
Contributor Author

It doesn't fixes the issue mentioned in #1837 as I mentioned in #1851 that the only one to get perfectly consistent results is to remove the limit for MAX_RESULTS which is currently set to 20 in your branch.

That's unexpected… it should return every file in the workspace because of undefined here:
https://github.com/sourcegraph/cody/pull/1889/files#diff-315ecedea695e410a04a36543d5d788b07f05d75885cedffceb922c3f33cd569R18

I'll check what’s going on here… 🤔

@abeatrix
Copy link
Contributor

@toolmantim I found a solution that works and created a separate PR to merge it into this PR (to avoid making direct changes to your PR in case it doesn't work for you): #1917

@toolmantim
Copy link
Contributor Author

Thanks @abeatrix ! I've updated this with your code from #1917 but tweaked a little. I think this is ready to go!

Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Confirmed:

  • Results are consistently sorted
  • Support fuzzy search
  • Search is not case-sensitive
  • Search support extension search
Screen.Recording.2023-11-27.at.7.12.29.PM.mov

I noticed the search results are not sorted alphabetically. If this is the intended behavior then I think it's good to go. Thanks for working on this!

@toolmantim
Copy link
Contributor Author

Thanks @abeatrix!

Yep that's intended… the fuzzy sorting is smarter than alphabetical, it's only if two items are scored the same we fall back to alphabetical.

@toolmantim toolmantim merged commit 09a943e into main Nov 28, 2023
14 checks passed
@toolmantim toolmantim deleted the tl/fuzzy-search-results branch November 28, 2023 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants