feat(svelte): Make fuzzy finder matching more reliable#63397
Conversation
Currently the fuzzy finder filters and ranks results received from the server locally. This was done to improve performance since local filtering is much faster. However it can introduce inconsistencies (as reported) because the local filtering logic works differently that the one on the server. That means the shown results is depedent on on the local cache, which is not obvious to the user. This commit removes the client side filtering and ranking and instead relies only on the server for this. This makes things more conisten and predicatable, at the expense of being a little slower. However it still feels quite fast to me. Note that I didn't implement some aspects due to the limitations of the GraphQL-based search API: - No match highlighting. Personally I didn't miss is it so far. I don't know if the highlighting actually provides a lot of value. - No total result count. It seems since we are adding `count:50`, the server cannot actually give use an approximate total count. But we should probably still convey somehow that we are limiting results to the top 50.
|
Meta: for the changelog entries, should we be specifying that these changes are for the svelte app only? |
| let selectedTab = tabs[0] | ||
| let selectedOption: number = 0 | ||
|
|
||
| $: useScope = scope && selectedTab.id !== 'repos' | ||
| $: source = selectedTab.source | ||
| $: if (open) { | ||
| source.next(query) | ||
| } | ||
| $: if (open) { | ||
| dialog?.showModal() | ||
| input?.select() | ||
| } else { | ||
| dialog?.close() | ||
| } |
There was a problem hiding this comment.
I just moved all of this up to keep variables/constants together.
There was a problem hiding this comment.
I think this was left over from something else. I'll remove this and the related changes.
|
@camdencheek I have |
camdencheek
left a comment
There was a problem hiding this comment.
Looks great!
One thing that feels weird: the All tab being scoped means that I can't switch to other repos in that view. So it's not really a superset of the three other tabs.
| switch (token.type) { | ||
| case 'pattern': | ||
| if (token.delimited) { | ||
| return `${token.delimiter}${token.value}${token.delimiter}` | ||
| } | ||
| return token.value | ||
| case 'filter': | ||
| return `"${token.negated ? '-' : ''}${token.field.value}:${escapeQuotes(token.value?.value ?? '')}"` | ||
| case 'keyword': | ||
| return `"${token.value}"` | ||
| default: | ||
| return '' | ||
| } |
There was a problem hiding this comment.
Rather than reconstructing the input from parsing it as a normal search query, I wonder if we could do something like zeroOrMore(oneOf<Term>(scanPattern(Literal), scanPattern(Regexp))) (stolen from here)
Feel free to ignore, just thinking it might be nice to reuse some of the underlying machinery rather than undoing the work done by the higher-level machinery.
There was a problem hiding this comment.
@camdencheek That's a great idea! Have a look at my recent changes. I'm not happy to have to export a special purpose function form the scanner module but hopefully we can improve that later.
I still need to wrap literals with quotes though so that e.g. repo:foo isn't interpreted in a special way.
|
@camdencheek yep, scope switching comes in a separate PR |
|
@camdencheek or I could of course change the query so that the "scope" isn't applied to repositories. Something like |
I think that's the behavior I want, but I'm not positive it will be easy to communicate to the user. |
|
@camdencheek I'd say lets try it and see what feedback we get. |
|
@camdencheek Actually that doesn't seem to work as I expected. E.g. the query gives me symbol results for repos other than |
8ef996f to
3a5f444
Compare
9f58614 to
87751a4
Compare
Currently the fuzzy finder filters and ranks results received from the server locally. This was done to improve performance since local filtering is much faster.
However it can introduce inconsistencies (as reported) because the local filtering logic works differently that the one on the server. That means the shown results is depedent on on the local cache, which is not obvious to the user.
This commit removes the client side filtering and ranking and instead relies only on the server for this. This makes things more consistent and predictable, at the expense of being a little slower. However it still feels quite fast to me.
Note that I didn't implement some aspects due to the limitations of the GraphQL-based search API:
count:50, the server cannot actually give use an approximate total count. But we should probably still convey somehow that we are limiting results to the top 50.Because we don't have locally cached data anymore that can be shown immediately I decided to increase the throttle time to prevent the result list flickering in and out when typing with a moderate speed.
This change enables three additional features: 'search all' mode, multi word search and regex search via
/.../literals (just like for normal search queries). This is consistent with our existing search query language (currently regex literals are not syntax highlighted, but we should consider doing that).Fixes srch-139
Fixes srch-133
Fixes srch-134
Fixes srch-543
sg-sk-resize-.2.mp4
Test plan
Manual testing.
Changelog