Improve backwards navigation through code intel events

[shrouxm]

• on Sourcegraph.com, we create a new browser history entry on every code intel related click. we should only generate browser history entries when a definition or reference is jumped to, not when a symbol is clicked or when a list of definitions or references is retrieved. we should not generate a new entry when jumping from a symbol to itself.
• this will disable the currently possible workflow where you can click a line, causing the line number to be appended to your current URL address, and then copy that address to link someone to a file at a specific location. if it's possible to update the address bar without creating a history entry, we should do this for backwards compat. a better long term solution would be to add anchors next to the line numbers (similar to those next to headers in a Markdown document) which when clicked jump to that line and create a history entry. this matches my expectations from how other web documents work, but may get punted to a future issue.
• when jumping to a symbol (because following a def/ref or moving backwards through browser history), it should be visually clear what is happening:
  • we should open a hover popover on the symbol being jumped to (we currently only do this when navigating backwards, but not when following a def/ref)
  • when jumping within the same file, we should do a smooth scroll rather than an instant jump
• question for someone who knows web better than i do: is it possible to add this behaviour to code hosts, or do we think it's a big yikes do mess with browser history on other sites?

[shrouxm]

@sourcegraph/web tagging you on this question in the above:

is it possible to add this behaviour to code hosts, or do we think it's a big yikes to mess with browser history on other sites?

[nicksnyder]

Related feedback from a customer https://sourcegraph.slack.com/archives/C0W2E592M/p1591051793299800

[nicksnyder]

Related issue: https://github.com/sourcegraph/sourcegraph/issues/4816

[rrhyne]

+1 for anchors next to line numbers, or an interactive line number gutter. The latter would enable the ability to add bookmarks, trigger annotations, etc. to lines.

[felixfbecker]

Here are my notes through the web team glasses on each point ✌️

we should only generate browser history entries when a definition or reference is jumped to, not when a symbol is clicked

Agree. Here's where we handle clicked lines/tokens and update the URL: https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/web/src/repo/blob/Blob.tsx#L271:33

or when a list of definitions or references is retrieved.

Agree. This is more difficult, because we don't explicitely "generate a history entry", we just render a link (<a> tag) and when the user clicks that, the browser naturally navigates to it and adds the previous page to the history.

The implementation here would be to add a new parameter to the open command to control this behavior and if set, manually handle clicks in ActionItem and replace the history instead of letting the browser navigate. See the logic here: https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/shared/src/actions/ActionItem.tsx#L286:64

this will disable the currently possible workflow where you can click a line, causing the line number to be appended to your current URL address, and then copy that address to link someone to a file at a specific location. if it's possible to update the address bar without creating a history entry, we should do this for backwards compat.

That would be possible and I think that is fine/the better implementation of this use case. I think the use case is very important though.

a better long term solution would be to add anchors next to the line numbers (similar to those next to headers in a Markdown document) which when clicked jump to that line and create a history entry.

This is not a "better" solution is my eyes because it doesn't link to the token itself (with an open hover and CTAs) but only to the line (and the line numbers are already clickable to select the line, so that part is already implemented, just without a 🔗 icon). 🔗 buttons sound cool to me but not sure how much space there is in the line number gutter for them. One possible thing they could do is make it a permalink (change the rev to a commit). It's a known interaction on other code hosts though, so the icon is not strictly necessary to communicate the line numbers are clickable I think.

we should open a hover popover on the symbol being jumped to (we currently only do this when navigating backwards, but not when following a def/ref)

I'm pretty sure we're already doing this - did you see it not working somewhere? I just tested it:

when jumping within the same file, we should do a smooth scroll rather than an instant jump

I like this idea. I think this is handled here: https://sourcegraph.com/github.com/sourcegraph/codeintellify/-/blob/src/helpers.ts#L36-54 Not 100% sure why we're not using the native scrollIntoView() (which has a behavior: smooth option), but it may be because it didn't support scrolling it into the center at the time. It seems to now though: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView

question for someone who knows web better than i do: is it possible to add this behaviour to code hosts, or do we think it's a big yikes do mess with browser history on other sites?

I'm not sure, it may depend on the code host. I don't think it's a big yikes though. It should just be ensured that GitHub for example highlights the line being jumped to too and we use GitHub's URL patterns for that (important especially on diffs). Improvements here would be great though because right now for example you may also jump to a definition in the same PR and the actual line is usually hidden behind the sticky PR header toolbar.

[shrouxm]

thanks for all the feedback!

This is not a "better" solution is my eyes because it doesn't link to the token itself...

agree

we should open a hover popover on the symbol being jumped to (we currently only do this when navigating backwards, but not when following a def/ref)

I'm pretty sure we're already doing this - did you see it not working somewhere? I just tested it:

there was a case where this wasn't working, will try to repro

I'm not sure, it may depend on the code host. I don't think it's a big yikes though. It should just be ensured that GitHub for example highlights the line being jumped to too and we use GitHub's URL patterns for that (important especially on diffs). Improvements here would be great though because right now for example you may also jump to a definition in the same PR and the actual line is usually hidden behind the sticky PR header toolbar.

hmm ok, i'll look into a bit but will probably punt to a future issue.

[uwedeportivo]

Dear all,

This is your release captain speaking. 🚂🚂🚂

Branch cut for the 3.17 release is scheduled for tomorrow.

Is this issue / PR going to make it in time? Please change the milestone accordingly. When in doubt, reach out!

Thank you

[efritz]

Related RFC 278 - Overhaul web app code navigation & history

[github-actions[bot]]

Heads up @macraig - the "team/code-intelligence" label was applied to this issue.

[emchap]

Received feedback on this from https://github.com/sourcegraph/accounts/issues/250. User provided some great feedback about ideal state in Slack:

My first instinct was to go check my profile/settings for an option to disable that, which is a little arcane to get to but is where I would expect it to be. Maybe something like a dismissible toast after the first five back clicks discreetly saying “click here if you don’t want to use your browser forward/back to travel between code clicks” would get it across succinctly. Generally, though, while I can see the benefit if I was used to it, there’s no other UX that has me use forward/back to travel spatially within a browser page rather than temporally. Even anchor tags in html are usually broad enough in scope that it feels like conceptually different content when I click back (e.g. Table of Contents => Chapter => back again), whereas this is just shuffling through the exact same stuff (maybe a threshold for entry? I don’t care about keeping track of two line movement, but maybe two hundred lines would be helpful/native-feeling if I’m scrolling quickly). To be totally honest, I’m so unaware of my idle clicking (as it usually has no effect) that I couldn’t estimate how many clicks it’ll take me to get back to results if you made me guess haha, so it’s bewildering to feel like I’ve unknowingly boxed myself into a facet of the UI, especially when my p95 is entering it from search results that I immediately want to go back and continue browsing through.

[macraig]

Feedback from @bobheadxi:

I also noticed clicking different parts of a line has different behaviour when it comes to adding a line to browser history - clicking the red and orange lines adds an anchor link, but clicking the black circle doesn't add an anchor link. I think the ideal behaviour would be only clicking the red circle adds an anchor link, which aligns somewhat with how it works in GitHub

[macraig]

Feedback from customer:

https://github.com/sourcegraph/accounts/issues/285 feedback Spoke with an engineer who uses us. His biggest complaint was how we handle back buttons. He said he likes how Figma does it—URL changes as you click, but back button brings you back to your search results or whatever. He also would like a way to see his full search history so he can click into old searches (like how browsers do). He said he doesn't feel like he gets any in-app education, joked that it "feels like a developer tool". He doesn't have time to learn the query language; said it felt like JQL. (He used to work at https://github.com/sourcegraph/accounts/issues/250, that's how I know him.)

[fkling]

I think this is large solved by not selecting the line when its clicked. Instead the line number has to be clicked now. The question which interactions should create browser history entries is still relevant but needs to be explored in the current environment.