Skip to content

(partial) Fix pinned hovercard rendering with using back/forward buttons

Warren Gifford requested to merge fkling/blob-handle-pinned-urls into main

Created by: fkling

This is marked as draft because I cannot reliably avoid the hovercard flickering when clicking copy URL :-/ (also visible in video)

As already mentioned in #36039 pinning a code intel hovercard (via the "Copy link" button) and using the back/forward buttons doesn't work as expected. The hovercard isn't shown when navigating to the pinned URL (only when the page is completely refreshed). That alone isn't bad but it also breaks the hovercards as a whole. The system thinks that there is already a pinned hovercard and therefore doesn't show cards when hovering over other tokens. This can be resolved by the user by selecting another line.

I spend some time to figure out what the issue is. The tl;dr is that some parts of the rxjs pipelines operate on older positioning information, which causes the hover system to not update because it thinks nothing has changed.

More specifically lets look at the pipeline that processes position changes from the URL:

    const jumpTargets = positionJumps.pipe(
        withLatestFrom(container.updates),
        filter(([, { pinned }]) => pinned),                     // A
        map(([position]) => position),
        [...]
        distinctUntilChanged((a, b) => isEqual(a, b)),          // B
        [...]

Lets assume we already have a pinned hovercard, e.g. our URL contains something like L42&popover=pinned. If we navigate back, lets say the previously selected line was 21, we are at L21. This new position doesn't make it code location marked as B because the new URL isn't pinned anymore and thus it's filter out be the filter in code location A. Later when we navigate forward (to L42&popover=pinned), code location B will prevent that event from propagating because from its perspective the position hasn't changed (it thinks the previous position was L42).

This can be easily fixed by flipping changing the order of the filter() and distinctUntilChanged operators.

However that alone is not enough. Further down in the file we encounter the same problem:

    const resolvedPositions = resolvedPositionEvents.pipe(
        // Suppress emissions from other events that refer to the same position as the current one.
        distinctUntilChanged((a, b) => isEqual(a.position, b.position))
    )

Since we are still not propagating position changes that are not pinned, this distinctUntilChanged operator causes the same problem.

My undestanding is that this was introduced to prevent the popover from flickering when it is pinned (because that updates the URL, which triggers the jumpTargets pipeline above).

Based on my understanding and my abilities I'm suggesting to update this part to only apply the distinctUntilChanged operator when the hovercard is currently open (or about to be opened). I don't know whether the logic I added to test that state is correct though.

IMPORTANT: This is a partial fix because jumping between a selected line and a pinned hovercard on the same line still doesn't work (because in that case the position is actually the same). But at this moment I don't have a good solution for this.

Befor/after video:

https://user-images.githubusercontent.com/179026/177359282-1b0b2526-8d87-43cf-80f2-29dc77c523d2.mp4

Test plan

App preview:

Check out the client app preview documentation to learn more.

Merge request reports