feat(search/svelte): Add context specific suggestions to repo search input by fkling · Pull Request #62880 · sourcegraph/sourcegraph-public-snapshot

fkling · 2024-05-23T16:19:35Z

This PR adds a new suggestions source to the repository search input. It currently offers the following suggestions:

Repository group/org (on every repo page)
Current file and/or directory (on blob and tree pages)
Language of current file (on blob pages)

When on a commit page it adds the revision of the commit to the repo filter.

A new context store is introduced to propagate those values to the search input. I also had to update our query processing logic to preserve pattern and filter token ranges (first commit) so that we can also replace filters, not just add the.

NOTE: I'm not convinced the way the information is propagated via context is the best way to do, but it's the simplest I could think of now. I can imagine in the future that individual pages create/register suggestions directly.

sg-sk-fuzzy-finder-.3.mp4

Test plan

Manual testing.

taiyab · 2024-05-23T17:08:39Z

This is amazing! (And you wanna hide the search bar?)

camdencheek · 2024-06-07T13:57:26Z

Design feedback (not blocking): I find it pretty difficult to see that the text describing the suggestion is not part of the suggestion. My eye scans right past that text because it's not very high contrast compared to the suggestion text.

The new designs also don't really fix this, and this scenario (a query snippet that is not just a filter, which is highlighted blue) is not covered in the new designs. @taiyab could we add some examples of "query snippet with a description" to the design?

camdencheek · 2024-06-07T13:58:25Z

+            expect(parse('abc content:"with  space" def /regex literal/ ghi |')).toMatchInlineSnapshot(`
+              abc content:"with  space" def /regex literal/ ghi
+              ^^^ (pattern)
+                  ^^^^^^^ (filter.field: literal)
+                          ^^^^^^^^^^^^^ (filter.value: literal)
+                  ^^^^^^^^^^^^^^^^^^^^^ (filter)
+                                        ^^^ (pattern)
+                                            ^^^^^^^^^^^^^^^ (pattern)
+                                                            ^^^ (pattern)
+            `)


Oooh, pretty 🙂

camdencheek · 2024-06-07T14:00:24Z

+ * Converts a parse node into a sequence of Token's. This function generates
+ * new tokens as needed to represent the parse tree in a flat list. Those
+ * tokens won't have useful ranges. Range information is only preserved for
+ * pattern and parameter nodes.


I don't quite understand this. What use is a tokenize function if we can't trust the ranges it yields?

Agree, plus to this - what ranges we can trust?

This is all part of getRelevantTokens which takes in a a tree and basically prunes nodes that are irrelevant. E.g. for the query foo OR file:bar lang:JS baz, if we only want to keep all file: and lang: filters, we would convert

OR / \ foo AND / \ file:bar AND / \ lang:file baz

to just

AND / \ file:bar lang:JS

Later we "flatten" the tree into a list of tokens again (which is what tokenize is doing).

So far we only had the need to read the list of tokens but now we also want to use them to change them in the original search query. But some of the tokens returned here are basically synthetic, due to the transformation.

Or maybe it makes more sense to think about it this way: We have an input query and transform it into another query. Some tokens exist in both queries and we want to make changes to the original query based on the new query.
So maybe what should be returned is a list of tokens and some kind of source map.

@vovakulikov, @camdencheek I changed the implementation of this. Please have another and let me know if it makes more sense now.

camdencheek · 2024-06-07T15:09:51Z

        { path: '/-/batch-changes', label: 'Batch changes', visibility: 'admin' },
        { path: '/-/settings', icon: mdiCog, label: 'Settings', visibility: 'admin' },
    ]
+    const repositoryContext = writable<RepositoryPageContext>({})


Q: rather than all the beforeNavigate/afterNavigate hooks, why not populate repositoryContext with the information we already have from data? We should already have the current repo and current revision, and we can add current file in the child layout, right?

I don't know; I agree with Camden that this feels a bit off when the parent level should know about lower lever child data. This repository context is about to create yet another horizontal stream of data in a world where we already have not trivial data passing through data-loaders.

I think I'm not against this idea about global context but maybe we should make it a bit more global in this case
rather than a specific repositoryContext thing.

why not populate repositoryContext with the information we already have from data

data will only return what the loader (and any parent loader) returns, not "child" loaders. We can access all page data via $page.data but that is not strongly typed. We can add optional fields to the PageData type, but that would then apply to all routes.
Would you find it less confusing if we used $page.data and data instead of repositoryContext and data?

The core issue IMO is that we want to propagate data up from pages to the layout. Context solves that but I understand that it's "yet another thing".

Because I think of this more as page data than component data (which I understand is kinda odd since pages are components), this feels like it belongs in page data loaders. We can make one of the pieces of page data writable so child pages can add their own information. In particular, the before/after hooks feel pretty weird since we're effectively manually implementing page data loading.

I'm having difficulty describing what I mean, so I think it might be easier to just show it. This draft moves all the propagation into the page data loaders so the page itself doesn't need to worry about it. Personally, I think that feels cleaner since it reduces the amount that the pages themselves need to know about the data loading lifecycle.

(Also, feel free to just ignore me. I do not feel strongly about any of this, but wanted to at least bring it up in case I was missing something about how this needs to work)

@camdencheek I don't think this would work due to data preloading. It would cause the context to updated if the user hovers over a file without clicking it. Using the page lifecycle ensures that the context is only changed when the page is actually navigated to/away from.

As I said in the PR, it doesn't feel ideal to me either, but I can't think of anything else right now apart from extending the global PageData object.

Ah, interesting. I was thinking that preloading would create a new data object with a new writable that would be updated by its child data fetchers.

A data loader is only executed when it's dependencies change (e.g. a URL parameter that is accessed in the loader). So in general you cannot make assumptions about when which loader is executed.

taiyab · 2024-06-07T15:16:49Z

Design feedback (not blocking): I find it pretty difficult to see that the text describing the suggestion is not part of the suggestion. My eye scans right past that text because it's not very high contrast compared to the suggestion text.

The new designs also don't really fix this, and this scenario (a query snippet that is not just a filter, which is highlighted blue) is not covered in the new designs. @taiyab could we add some examples of "query snippet with a description" to the design?

Took me a while to understand what you were saying here as I scanned the image first. Once I completely read it, and looked back at the image, it took me at least 3-5 seconds to even see that a description was been appended after the suggestion :D

Yes — I'll fix this in the designs.

taiyab · 2024-06-07T15:18:32Z

@fkling Note: The search suggestions that are queries should probably have the same styling treatment as a keyword search query in the search input itself.

(This is provided it was ran as a keyword search query (and not if it wasn't)).

camdencheek

Played around with it locally -- works great, thank you!

vovakulikov

Left a few minor comments, nothing bit. I think though we should address our data flows as soon as possible while we have not that many data sources and fields. Complexity around contexts and different data types in data loaders look a bit scary every time I have to touch it. (maybe we can simplify it)

vovakulikov · 2024-06-10T13:56:47Z


    input = input.replaceAll('|', '')
-    const result = parseSearchQuery(input)
+    const tokens = scanSearchQuery(input, false, SearchPatternType.standard)


Minor: it's nothing but IMO would be a bit easier to read if it was with object-like argument with key fields
like this

const tokens = scanSearchQuery({ input, interpreteComments: false, pattern: SearchPatternType.standard })

vovakulikov · 2024-06-10T13:57:25Z

+    return getRelevantTokens(result.node, { start: inputPosition, end: inputPosition }, filter)
+}
+
+function annotateToken(token: Token, prefix?: string): string {


I like the idea with annotations, very cool

vovakulikov · 2024-06-10T13:59:18Z

+ * Converts a parse node into a sequence of Token's. This function generates
+ * new tokens as needed to represent the parse tree in a flat list. Those
+ * tokens won't have useful ranges. Range information is only preserved for
+ * pattern and parameter nodes.


Agree, plus to this - what ranges we can trust?

vovakulikov · 2024-06-10T14:02:04Z

+        (tokens: Token[], position: number, { repoName, revision }: ScopeInformation): Option[] => {
+            const options: Option[] = []
+
+            {


I don't think we need these blocks. Since both have no overlap scoped-variables this should be fine, no?

I added them to not "pollute" the function scope. The variables used here are only relevant for this specific computation.

vovakulikov · 2024-06-10T14:13:17Z

        { path: '/-/batch-changes', label: 'Batch changes', visibility: 'admin' },
        { path: '/-/settings', icon: mdiCog, label: 'Settings', visibility: 'admin' },
    ]
+    const repositoryContext = writable<RepositoryPageContext>({})


I don't know; I agree with Camden that this feels a bit off when the parent level should know about lower lever child data. This repository context is about to create yet another horizontal stream of data in a world where we already have not trivial data passing through data-loaders.

I think I'm not against this idea about global context but maybe we should make it a bit more global in this case
rather than a specific repositoryContext thing.

camdencheek

The new approach makes more sense to me! Thanks for the refactor

This allows us to replace the corresponding tokens in the original query. It's not ideal because the token sequence will contain a mix of generated and preserved tokens but I hope it's good enough.

This commit adds a new suggestions source to the repository search input. It currently offers the following suggestions: - Repository group/org - Current file and/or directory - Language of current file A new context store is introduced to propagate those values to the search input. When on a commit page it adds the revision of the commit to the repo filter.

Co-authored-by: Camden Cheek <camden@ccheek.com>

fkling requested a review from a team May 23, 2024 16:19

fkling self-assigned this May 23, 2024

cla-bot Bot added the cla-signed label May 23, 2024

fkling force-pushed the fkling/srch-145-search-in-current-directory-not-working-as-expected-unable branch from b344425 to 9a980d2 Compare June 7, 2024 10:31

fkling changed the title ~~svelte: Add context specific suggestions to repo search input~~ feat(search/svelte): Add context specific suggestions to repo search input Jun 7, 2024

camdencheek reviewed Jun 7, 2024

View reviewed changes

camdencheek approved these changes Jun 7, 2024

View reviewed changes

Comment thread client/web-sveltekit/src/routes/[...repo=reporev]/context.ts Outdated

vovakulikov approved these changes Jun 10, 2024

View reviewed changes

camdencheek approved these changes Jun 14, 2024

View reviewed changes

fkling commented Jun 14, 2024

View reviewed changes

Comment thread client/shared/src/search/query/analyze.ts Outdated

fkling and others added 7 commits June 14, 2024 12:33

Preserve ranges when converting a parse tree to a token sequence

6851f55

This allows us to replace the corresponding tokens in the original query. It's not ideal because the token sequence will contain a mix of generated and preserved tokens but I hope it's good enough.

Add clarifying comment

ba9ae16

Remove revision from group/org search suggestion

4eee756

Refactor getRelevantToken logic to make it more predictable

426dd03

Add suggestion option icon

592e76c

Apply suggestions from code review

d43d493

Co-authored-by: Camden Cheek <camden@ccheek.com>

fkling force-pushed the fkling/srch-145-search-in-current-directory-not-working-as-expected-unable branch from 4e64c59 to d43d493 Compare June 14, 2024 10:35

fkling added 4 commits June 14, 2024 12:54

Address lint issues

d9bccf5

Add back @mdi/js

f277cc4

Use more specific suggestion icons

1416cc0

More lint fixes

fcce8ef

fkling enabled auto-merge (squash) June 14, 2024 12:41

fkling merged commit 8474caf into main Jun 14, 2024

fkling deleted the fkling/srch-145-search-in-current-directory-not-working-as-expected-unable branch June 14, 2024 12:45

Conversation

fkling commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

taiyab commented May 23, 2024

Uh oh!

camdencheek commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkling Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taiyab commented Jun 7, 2024

Uh oh!

taiyab commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camdencheek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vovakulikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camdencheek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fkling commented May 23, 2024 •

edited

Loading

camdencheek commented Jun 7, 2024 •

edited

Loading

fkling Jun 10, 2024 •

edited

Loading

taiyab commented Jun 7, 2024 •

edited

Loading