svelte: First pass on fuzzy finder#62494
Conversation
6bb75bc to
e155bac
Compare
camdencheek
left a comment
There was a problem hiding this comment.
Looks great! Leaving a few notes that are fine to leave for followup:
-
I expected control+n and control+p to work for next/previous as it does in the search input
-
The double border on the tabs looks a little funny when they are not selected
- It looks like the keyboard shortcuts get propagated and it's still activating menu items on mac?
screenshot-2024-05-08_09-13-08.mp4
- There doesn't appear to be a way to expand the scope outside of the active repo. Definitely skippable for now though.
While working on #62494 I noticed that the file tree didn't work as expected. In particular, when being at the repo root and navigating to a file via the fuzzy finder the file tree would switch to the parent directory, just like as if the file was directly navigated to. However, what should happen is that the tree expands all directories to reveal the file, because the repo root is the top level directory and we should always be showing the most top level directory that was visited. The issue is that we are not properly detecting the repo root level (I think we switched from using `'.'` to `''` at some point). To avoid this problem I'm now using a constant instead. I also changed how the tree entries are scrolled into view. The current solution doesn't work for deeply nested entries because at the time the scroll logic is invoked the entry hasn't been loaded yet. By moving the logic to the node it can scroll itself into view when it's available. The waterfall loading nature of the file tree also became much more apparent. We can avoid this by using making use of the `ancestors` field but that needs to be done in a separate PR because that entails larger changes.
This commit adds a first version of the fuzzy finder. Supported features: - Repos, symbols, files search - Automatic scoping of files and symbol search to current repository - Activting search modes via keyboard shortcuts - Keyboard support Not yet supported: - "All" search - Result counts - Scope toggle (i.e. search all files even when inside repository) - Full a11y support Additional changes: - Introduced separate tab headers component. In this case I wanted to reuse the same panel/UI and only change the data source. - Added a function for formatting keyboard shortcuts that work with our hotkey implementation. Follow up work: - Cleanup and tweak sources (matching, ranking, etc) - Design updates - Work on "not yet supported" items
What's nice is that the `url` fields will point to the right revision.
f1b93e1 to
0720771
Compare
While working on #62494 I noticed that the file tree didn't work as expected. In particular, when being at the repo root and navigating to a file via the fuzzy finder the file tree would switch to the parent directory, just like as if the file was directly navigated to. However, what should happen is that the tree expands all directories to reveal the file, because the repo root is the top level directory and we should always be showing the most top level directory that was visited. The issue is that we are not properly detecting the repo root level (I think we switched from using `'.'` to `''` at some point). To avoid this problem I'm now using a constant instead. I also changed how the tree entries are scrolled into view. The current solution doesn't work for deeply nested entries because at the time the scroll logic is invoked the entry hasn't been loaded yet. By moving the logic to the node it can scroll itself into view when it's available. The waterfall loading nature of the file tree also became much more apparent. We can avoid this by using making use of the `ancestors` field but that needs to be done in a separate PR because that entails larger changes.
|
@fkling Nice work getting this in Felix! |
vovakulikov
left a comment
There was a problem hiding this comment.
I know it's merged but left a few comments
Just for any other further improvements, thanks for the finder!
| } | ||
| </script> | ||
|
|
||
| <script lang="ts"> |
There was a problem hiding this comment.
It looks like that most of our tabs should have keyboard navigation suffixes and shortcuts.
Might be a good idea to have API over tabs that allows you to set shortcuts without writing
any logic in consumer directly
| { id: 'files', title: 'Files', source: createFileSource(client, () => scope) }, | ||
| ] | ||
|
|
||
| function selectNext() { |
There was a problem hiding this comment.
Something that we discussed in the pair session, this solution is fine for the migration part
since it's been in the React version but we probably should use our standard (melt-ui) logic
around keyboard navigation like in this component, even if melt-ui doesn't provide a clean
abstraction for only list navigation (but with Combobox logic). It would be easier for us to
migrate it to our unified logic later.
| } | ||
| </script> | ||
|
|
||
| <dialog bind:this={dialog} on:close> |
There was a problem hiding this comment.
Have been seeing need for the dialog component, so probably should have Dialog in the svelte
wildcard package.
|
|
||
| const client = getGraphQLClient() | ||
| const tabs: (Tab & { source: CompletionSource<FuzzyFinderResult> })[] = [ | ||
| { id: 'repos', title: 'Repos', source: createRepositorySource(client) }, |
There was a problem hiding this comment.
It's not a big deal but referring to your own comment about having a consistent logic
around data fetching (even if it's not always an optimal solution), we probably could have it
inside loader function (data sources calls)
Part of #62426
This commit adds a first version of the fuzzy finder. To get things moving quickly I'm using the same mechanism to fetch data as the search input.
Supported features:
Not yet supported:
Additional changes:
Follow up work:
sg-sk-fuzzy-finder-.2.mp4
Test plan
Manual testing