(feat) add repository metadata to repo match search result items #50567

erzhtor · 2023-04-12T17:41:30Z

Part of https://github.com/sourcegraph/pr-faqs/issues/96.

This PR:

Adds repository metadata (aka keyValuePairs) to search repo match result items if repository-metadata feature flag is enabled

Test plan

sg start
Enable repository-metadata feature flag
Add some key value pairs through API (currently no UI for adding) GQL mutation addRepoKeyValuePair API
Search and check search results

Screenshots

Description	Screenshot
Repo match

App preview:

Web

Check out the client app preview documentation to learn more.

sourcegraph-bot · 2023-04-12T17:45:18Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff c9ddb0d...68b9168.

Notify	File(s)
@fkling	client/branded/src/search-ui/components/RepoSearchResult.tsx client/branded/src/search-ui/components/SearchResult.module.scss client/shared/src/search/stream.ts client/web/src/search/results/StreamingSearchResults.tsx
@limitedmage	client/branded/src/search-ui/components/RepoSearchResult.tsx client/branded/src/search-ui/components/SearchResult.module.scss client/web/src/search/results/StreamingSearchResults.tsx

camdencheek

Woot! Thanks for tackling this.

A couple of quick first impressions:

It seems a little strange that we are adding this to all result types since they are logically associated only with the repos, not a file or a commit. I would expect these to only be displayed on repository result types.
The added gap between the title line and the result card is a little visually jarring. Might be worth a design consultation here
What does this look like with a null value? I don't see any screenshots with that case.

sg-e2e-regression-test-bob · 2023-04-12T17:50:47Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	0.01% (+1.63 kb)	0.01% (+1.63 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 68b9168 and 3f84c0f or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

limitedmage · 2023-04-12T17:54:26Z

This is a great addition! I agree with Camden's comments here. I recommend reconsidering the design to put these key/value pairs inside the box and only for repo results.

I also recommend showing them in the repo page (eg. https://sourcegraph.com/github.com/sourcegraph/sourcegraph), but this can be a separate PR.

erzhtor · 2023-04-12T18:02:53Z

Thanks for a quick review, @camdencheek.

It seems a little strange that we are adding this to all result types since they are logically associated only with the repos, not a file or a commit. I would expect these to only be displayed on repository result types.

I intentionally added to all result types except own since they all associate with repos, but we can remove and leave only for repo match too. cc @rrhyne @ryphil would appreciate your feedback.

The added gap between the title line and the result card is a little visually jarring. Might be worth a design consultation here

Yep, already asked Rob Rhyne for help with designs.

What does this look like with a null value? I don't see any screenshots with that case.

Good point, I missed that values are nullable, will take a look at this scenario too.

kopancek

Mostly naming and styling questions/suggestions

client/branded/src/search-ui/components/ResultContainer.tsx

client/branded/src/search-ui/components/ResultContainer.module.scss

kopancek · 2023-04-13T08:25:43Z

client/branded/src/search-ui/components/FileContentSearchResult.tsx

@@ -256,6 +256,7 @@ export const FileContentSearchResult: React.FunctionComponent<React.PropsWithChi
            className={classNames(styles.copyButtonContainer, containerClassName)}
            resultClassName={resultContainerStyles.highlightResult}
            rankingDebug={result.debug}
+            keyValuePairs={result.keyValuePairs}


If we call the feature repository metadata, why do we call everything that actually implements it key value pairs? Does not make sense to me to be honest.

I believe metadata is implemented as simple key-value pairs for more flexibility.

client/branded/src/search-ui/components/ResultContainer.tsx

…lt item

style(ResultContainer.module.scss): rename .repo-kvps class to .repo-metadata

…alue classes feat(ResultContainer.tsx): replace key-value pairs with badges in RepoMetadata component

… RepoMatch

sashaostrikov

Nice! LGTM after your changes and fixes.
I guess you'll move it as Juliana proposed in the next PR?

erzhtor · 2023-04-13T09:44:36Z

Thanks, everyone, for the reviews and feedback. So after thinking a bit,

I followed @camdencheek suggestion on showing repository metadata only on RepoMatch results. We can add to other search result types if needed in future
- Also added handling of null value cases (see screenshot for oss badge)
I've followed @limitedmage suggestion and moved badges to header section
- I'll also follow up in a separate PR @limitedmage's suggestion to add these badges to repo page too
I've also followed @kopancek suggestion and reused badges with little styling from our current wildcard. Metadata key is info variant badge and value is secondary variant badge

Here is how it looks now:

erzhtor · 2023-04-13T17:47:25Z

I also recommend showing them in the repo page (eg. https://sourcegraph.com/github.com/sourcegraph/sourcegraph), but this can be a separate PR.

@limitedmage Follow-up PR adding repo metadata to repo root page. I would appreciate reviews. cc @camdencheek @camdencheek

)

erzhtor self-assigned this Apr 12, 2023

cla-bot bot added the cla-signed label Apr 12, 2023

erzhtor marked this pull request as ready for review April 12, 2023 17:43

erzhtor requested review from keegancsmith, limitedmage, rrhyne, camdencheek, stefanhengl and a team April 12, 2023 17:44

camdencheek reviewed Apr 12, 2023

View reviewed changes

kopancek reviewed Apr 13, 2023

View reviewed changes

client/branded/src/search-ui/components/ResultContainer.tsx Outdated Show resolved Hide resolved

erzhtor force-pushed the erzhtor/add-repo-metadata-to-search-results branch from 3bec0e0 to 159c93c Compare April 13, 2023 08:54

erzhtor added 5 commits April 13, 2023 15:08

(feat) add repository metadata (keyValuePairs) to search results resu…

f490ed9

…lt item

refactor(ResultContainer): rename RepoKVPs component to RepoMetadata

590452b

style(ResultContainer.module.scss): rename .repo-kvps class to .repo-metadata

style(ResultContainer.module.scss): change border radius of key and v…

a60c085

…alue classes feat(ResultContainer.tsx): replace key-value pairs with badges in RepoMetadata component

feat: add support for null value case

dbc8b4b

refactor: remove repository metadata from other search results except…

52c6bf9

… RepoMatch

erzhtor force-pushed the erzhtor/add-repo-metadata-to-search-results branch from 90f4e33 to 52c6bf9 Compare April 13, 2023 09:18

sashaostrikov approved these changes Apr 13, 2023

View reviewed changes

Move badges to repository match header section

c0a148f

erzhtor force-pushed the erzhtor/add-repo-metadata-to-search-results branch from d344949 to c0a148f Compare April 13, 2023 09:40

erzhtor requested a review from kopancek April 13, 2023 10:15

erzhtor requested a review from camdencheek April 13, 2023 10:15

kopancek approved these changes Apr 13, 2023

View reviewed changes

minor refactoring

68b9168

erzhtor changed the title ~~(feat) add repository metadata "keyValuePairs" to search result items~~ (feat) add repository metadata to repo match search result items Apr 13, 2023

erzhtor merged commit 4c07309 into main Apr 13, 2023

erzhtor deleted the erzhtor/add-repo-metadata-to-search-results branch April 13, 2023 11:52

This was referenced Apr 13, 2023

Epic: repository metadata #43961

Open

(feat) add repo metadata to repo root page #50607

Merged

almeidapaulooliveira pushed a commit that referenced this pull request Apr 13, 2023

(feat) add repository metadata to repo match search result items (#50567

6d9f178

)

cesrjimenez pushed a commit that referenced this pull request Apr 14, 2023

(feat) add repository metadata to repo match search result items (#50567

322e1d1

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) add repository metadata to repo match search result items #50567

(feat) add repository metadata to repo match search result items #50567

erzhtor commented Apr 12, 2023 •

edited

Loading

sourcegraph-bot commented Apr 12, 2023 •

edited

Loading

camdencheek left a comment

sg-e2e-regression-test-bob commented Apr 12, 2023 •

edited

Loading

limitedmage commented Apr 12, 2023 •

edited

Loading

erzhtor commented Apr 12, 2023

kopancek left a comment

kopancek Apr 13, 2023

erzhtor Apr 13, 2023

sashaostrikov left a comment

erzhtor commented Apr 13, 2023

erzhtor commented Apr 13, 2023

(feat) add repository metadata to repo match search result items #50567

(feat) add repository metadata to repo match search result items #50567

Conversation

erzhtor commented Apr 12, 2023 • edited Loading

Test plan

Screenshots

App preview:

sourcegraph-bot commented Apr 12, 2023 • edited Loading

camdencheek left a comment

Choose a reason for hiding this comment

sg-e2e-regression-test-bob commented Apr 12, 2023 • edited Loading

Bundle size report 📦

limitedmage commented Apr 12, 2023 • edited Loading

erzhtor commented Apr 12, 2023

kopancek left a comment

Choose a reason for hiding this comment

kopancek Apr 13, 2023

Choose a reason for hiding this comment

erzhtor Apr 13, 2023

Choose a reason for hiding this comment

sashaostrikov left a comment

Choose a reason for hiding this comment

erzhtor commented Apr 13, 2023

erzhtor commented Apr 13, 2023

erzhtor commented Apr 12, 2023 •

edited

Loading

sourcegraph-bot commented Apr 12, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Apr 12, 2023 •

edited

Loading

limitedmage commented Apr 12, 2023 •

edited

Loading