Svelte: implement new reference panel against new Usages API by camdencheek · Pull Request #63724 · sourcegraph/sourcegraph-public-snapshot

camdencheek · 2024-07-09T15:30:14Z

This implements a first pass at the new references panel built against the new usages API.

Stacked on https://github.com/sourcegraph/sourcegraph/pull/63816

Notable omissions:

Precise code intel. Not implemented by the backend yet, will get automatic support once that rolls out. Will need to update the UI to surface the preciseness of results.
In-panel filtering. Just ran out of time since. It's surprisingly complex to define (and implement) the exact behavior here.
Pagination. It's actually theoretically implemented in the client (it uses infinityQuery), but the backend just returns everything, so we just render a massive list.
Collapsing file names. The logic to collapse file names is pretty performance heavy (requires the browser to do a full layout calculation), and since we have large lists from not paginating, it's very noticeable. I spent a fair amount of time trying to find alternatives, and still haven't found anything better than what we have now. Am open to being nerd sniped.
Full browser history support. The reference panel inputs are snapshotted on navigation, but opening the reference panel or adding a filter does not constitute a "navigation". I did not find an obvious way of making pushState and Snapshot work together nicely, but would love to pair on this.

Notable decisions:

No preview panel. It added complexity, was way to small with the additional filter panel on the left, and didn't actually feel good when you already have a perfectly good file view above.
The currently-open tab no longer lives in the URL, however it is still stored in the history entry. I want the state of the explore panel to persist across navigations, and I want state other than just the open tab to live with the history entry. IMO, it's not really useful to navigate to directly to "file with an open history/preview" panel, so I don't think we lose much with this. Certainly willing to be convinced otherwise though.

Test plan

Added a couple of storybook states. Are currently not very useful though because global styles do not seem to be applying to storybook.
Skipping playwright tests for now since they would need to rely on an unhealthy amount of mocking.
Manual testing of basic functionality

camdencheek · 2024-07-15T22:44:45Z

-                box-shadow: var(--focus-box-shadow);
+                box-shadow: var(--focus-shadow-inner);


Unrelated: use an inner box shadow so that stack order of siblings doesn't interfere and so we show it on all four sides (the left and right get cut off)

camdencheek · 2024-07-15T23:34:01Z

-            // Wait until DOM was updated to possibly show the history panel
-            await tick()


@fkling do you have more context on this? It seemed to be causing some very weird bugs when I tried to add explore panel stuff to the snapshot.

The historyPanel component is only rendered when it's selected. When we restore selectedTab we have to wait for the DOM to update before historyPanel is available and before we can restore its data.
Which problems did you encounter?

vovakulikov · 2024-07-17T20:42:34Z

Did try this locally, works cool, found a few small layout problems (mostly about content overflow, nothing that big)

File breadcrumbs cause horizontal scroll in the panel content container

Long repo and file name cut off if the left panel is too narrow

Not a problem but a suggestion, maybe it's time to increase the max height for the ref/history panel up to 75%

cc @camdencheek

camdencheek · 2024-07-17T21:17:52Z

Thanks @vovakulikov!

Made long paths wrap rather than creating a scroll container:

Added word-break: break-all to the file list in the tree to ensure that we can always wrap even given a small column size:

Increased the max panel height to 70%:

fkling

I feel like we should still update the URL to include a reference to the line/range of the symbol you just clicked. That will also highlight that line in the blob view.
After clicking 'find references', or rather after navigating back to a page with the reference panel open, it's not obvious where I opened the reference panel from (if that makes sense).

Compare that with the current reference panel implementation:

That aside, I love that we got rid of the preview 😃

fkling · 2024-07-18T11:16:11Z

+    export let repo: string
+    export let path: string
+    export let usages: ExplorePanel_Usage[]
+    export let scrollContainer: HTMLElement | undefined


Makes me wonder whether we should pass the scrollContainer as context. Then we wouldn't have to pass props around. Not sure if it's possible to have optional contexts though.

But in general I find the idea reasonable: We have one "Scroller" component that's supposed to be used whenever you want to react to visibility or scroll events and that component provides the context for other components.

Oh, I like the idea of tying this in to context with a Scroller. Will save that for a followup though.

fkling · 2024-07-18T11:20:00Z

-            // Wait until DOM was updated to possibly show the history panel
-            await tick()


The historyPanel component is only rendered when it's selected. When we restore selectedTab we have to wait for the DOM to update before historyPanel is available and before we can restore its data.
Which problems did you encounter?

fkling · 2024-07-18T11:40:14Z

The last commit message is not truncated anymore, it's overflowing:

camdencheek · 2024-07-18T16:23:10Z

After clicking 'find references', or rather after navigating back to a page with the reference panel open, it's not obvious where I opened the reference panel from (if that makes sense).

Yep, that makes perfect sense, and if I'm understanding you right, that's exactly what this issue is all about.

cla-bot Bot added the cla-signed label Jul 9, 2024

camdencheek force-pushed the cc/usages-api branch 9 times, most recently from c5c3f13 to 140667a Compare July 15, 2024 17:34

camdencheek changed the base branch from main to cc/optimize-observe-intersection July 15, 2024 17:39

camdencheek changed the title ~~Cc/usages api~~ Svelte: implement new reference panel against new Usages API Jul 15, 2024

camdencheek force-pushed the cc/usages-api branch from bf17cfd to b0cb9c4 Compare July 15, 2024 22:38

camdencheek commented Jul 15, 2024

View reviewed changes

Comment thread client/web-sveltekit/src/lib/TreeNode.svelte

camdencheek commented Jul 15, 2024

View reviewed changes

Comment thread client/web-sveltekit/src/lib/codenav/ExplorePanel.stories.svelte

camdencheek commented Jul 16, 2024

View reviewed changes

camdencheek mentioned this pull request Jul 16, 2024

chore(svelte): Refactor infinity query implementation #63824

Merged

camdencheek marked this pull request as ready for review July 16, 2024 00:20

camdencheek force-pushed the cc/optimize-observe-intersection branch from c65e8b2 to 5ac65b3 Compare July 17, 2024 00:08

camdencheek force-pushed the cc/usages-api branch from 8498092 to d3b11fc Compare July 17, 2024 00:21

Base automatically changed from cc/optimize-observe-intersection to main July 17, 2024 02:13

camdencheek force-pushed the cc/usages-api branch 2 times, most recently from 23d02d4 to 64de261 Compare July 17, 2024 02:45

camdencheek added 4 commits July 17, 2024 12:28

implement new references panel against new usages API

a8a7537

use scss variables instead of css variables at root

e95539a

fix storybook styling

b98d8b1

only show tree if there are any entries

5908096

camdencheek force-pushed the cc/usages-api branch from 69afe5c to 5908096 Compare July 17, 2024 18:28

appease linter

84dd512

camdencheek added 3 commits July 17, 2024 13:22

use new enum

ca68602

add no results message

eaebbd9

handle undefined scroller

2966f46

camdencheek added 3 commits July 17, 2024 14:45

appease linter

f703c1e

fix scroll overflow

9d096e3

break all in file list

3439685

better wrapping

edbd178

camdencheek requested a review from a team July 18, 2024 00:02

fkling approved these changes Jul 18, 2024

View reviewed changes

vovakulikov approved these changes Jul 18, 2024

View reviewed changes

camdencheek added 2 commits July 18, 2024 10:40

remove TODO

b6871fc

review followups

f03ab6f

camdencheek merged commit f7511c4 into main Jul 18, 2024

camdencheek deleted the cc/usages-api branch July 18, 2024 19:32

		box-shadow: var(--focus-box-shadow);
		box-shadow: var(--focus-shadow-inner);

		// Wait until DOM was updated to possibly show the history panel
		await tick()

Conversation

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

Uh oh!

Test plan

Uh oh!

Uh oh!

camdencheek Jul 15, 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!

Uh oh!

camdencheek Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

fkling Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vovakulikov commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camdencheek commented Jul 17, 2024

Uh oh!

fkling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fkling Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

camdencheek Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fkling Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fkling commented Jul 18, 2024

Uh oh!

camdencheek commented Jul 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

camdencheek commented Jul 9, 2024 •

edited

Loading

camdencheek Jul 15, 2024 •

edited

Loading

vovakulikov commented Jul 17, 2024 •

edited

Loading