search: highlight file path matches#41296
Conversation
During construction of zoekt.GlobalTextSearchJob and zoekt.RepoSubsetTextSearchJob, compile the query patterns being sent to zoekt to regexps. These regexps are used during match construction to compute path match ranges. These path match ranges are propagated back up to the client (web app) where they are then highlighted in the UI.
During construction of searcher.TextSearchJob, compile the text pattern into a regexp. When converting searcher matches to result.FileMatches, use that regexp to compute path match ranges, which are then propagated back up to the client (web app) where they are highlighted in the UI.
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff b38f7e3...b026def.
|
There was a problem hiding this comment.
Direction looks great! Small comments inline.
Right now, paths will only be highlighted if the search result is explicitly a PathMatch.
I'm not sure how I feel about this. In my mind, this violates the principle of least surprise. It looks weird to me to have paths highlighted on some results but not others.
This is especially true because we don't emit path matches if there are any content matches in the file. For example, in your screenshot, we don't emit a path match for internal/database/access_tokens.go because there is a content match for that file. This means the results won't visually represent the fact that the path internal/database/access_tokens.go is a match for your query.
Thanks for the feedback on this @camdencheek, I wasn't sure which way to go here. Initially I was worried about adding too much new visual noise, but I also think it makes sense to be consistent in how we display results. I can see how the current inconsistency could be surprising to a user. I can add |
719461a to
4ddf85a
Compare
limitedmage
left a comment
There was a problem hiding this comment.
Frontend code looks good to me. I think we should update the color used to highlight though, the contrast is way too low. I opened https://github.com/sourcegraph/sourcegraph/issues/41378 and assigned to @almeidapaulooliveira to pick the right color. I don't know if that should be a blocker for this feature, @benvenker and @almeidapaulooliveira can decide.
camdencheek
left a comment
There was a problem hiding this comment.
A couple of small comments inline, but this is looking good 🥳
I can add PathMatches as a field to our other match types in a follow-up PR to keep the scope of this one limited to getting the behavior right.
Sounds great!
| Features: *searchInputs.Features, | ||
| } | ||
|
|
||
| if patternInfo.PatternMatchesPath { |
There was a problem hiding this comment.
I think this condition should be inside getPathRegexpsFromTextPatternInfo, right? Because, even if PatternMatchesPath == false, we still want to get the highlighted ranges from patternInfo.IncludePatterns, yes?
There was a problem hiding this comment.
had to think about it for a minute but yes I think you're right
| Since func(time.Time) time.Duration `json:"-"` // since if non-nil will be used instead of time.Since. For tests | ||
| Repos *IndexedRepoRevs // the set of indexed repository revisions to search. | ||
| Query zoektquery.Q | ||
| ZoektQueryRegexps []*regexp.Regexp |
There was a problem hiding this comment.
Since it's not immiediately clear from context, could we add a comment explaining what this is for? Same applies to other places we add this field to a struct.
I'm not very good about this, but since jobs are now one of our primary forms of abstraction, it would be nice if we could understand them better at a glance without needing to go find where the field is used or how it's populated.
Part of #18374
This PR adds highlighting for path matches in the search UI.
Right now, paths will only be highlighted if the search result is explicitly a
PathMatch. In other words, if content and/or symbol matches are available to be rendered, then the rendering and highlighting of those match types takes precedent over path matches.For example, if a file contains both a content and a path match on the string
foo, and the query does not containselect:fileortype:path, then the rendered result will show the content match highlighted and the path will not be highlighted. The reason for this decision is mainly to minimize added visual noise, as certain queries could potentially produce a huge amount of highlighting on the results page if we always highlighted path matches for every result type. Note that for the content match in the screenshot below, the path is not highlighted despite containing a match:Looking for feedback and thoughts on what I just outlined above.
Implementation details:
FileMatchor a ZoektFileMatchto aresult.FileMatch, match the file name against the regex and append the submatches asresult.Ranges to the resultingresult.FileMatch.result.FileMatchdoes not have content or symbol matches, thenfrontendwill send anEventPathMatchto the client, which then highlights the matched ranges in the file path using thehighlightNode()function.Test plan
Added unit tests
Manual testing to feel good about the additions