Page MenuHomePhabricator

Improve behavior when jumping to inline comments
Closed, WontfixPublic

Description

When jumping to inline comments, behaviors could be improved after T430:

  • If the comment is on the current diff, but the inline hasn't loaded yet, it would be nice to prioritize loading of that content block.
  • If the comment is on a different diff, it would be nice to link to the other diff with a hint in the URI about which content block to load first.
  • The 5 second timeout where we stop looking for an anchor is sort of arbitrary. Showing some kind of UI might be useful, or changing the rule to consider scrolling, content loading, have a longer timeout, etc.
  • The comment link should also start at the line(s) highlighted, not just the comment.

Event Timeline

epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Differential.
epriestley added subscribers: epriestley, csilvers.

When viewing a URI like /D123, we'll generate links to, e.g,, /D123#inline-456 to avoid reloading the page. These will eventually expire when a diff is updated. Instead, the client and server can coordinate to figure out where the inline actually is and load that instead.

To deal with that last one more easily: we could generate permanent links instead (/D123?id=789#inline-456) since we have to capture them anyway to implement the "on the current diff..." case above.

When viewing a URI like /D123, we'll generate links to, e.g,, /D123#inline-456 to avoid reloading the page. These will eventually expire when a diff is updated. Instead, the client and server can coordinate to figure out where the inline actually is and load that instead.

We could use window.pushState to change the URL without a reload, though of course that may be confusing.

epriestley edited this Maniphest Task.May 25 2014, 2:16 PM

https://github.com/phacility/phabricator/issues/447 discusses a similar set of issues for Diffusion.

chad updated the task description. (Show Details)Jan 19 2015, 11:24 PM

D12497 doesn't directly fix any of this, but it makes links to other diffs much less common.

epriestley moved this task from Backlog to Glitches on the Inline Comments board.Apr 21 2017, 12:25 PM
epriestley closed this task as Wontfix.May 16 2017, 5:34 PM
epriestley claimed this task.

I'm not sure this task has any specific, actionable issues remaining. Partly, it has just been open for twenty five decades and become a bit of a dumping ground.

I think at least some of this (in T8592) was probably T11784, which is really a different beast.

T7846 wasn't really related and we're at least trying a version without those actions now.

T6324 and T7846 discuss jumping above comments, but at HEAD we do jump a little bit above comments (3-4 lines on my screen) which seems pretty reasonable to me. It would be potentially bad to jump to the top of the highlighted range in the general case because the rage may be arbitrarily tall. We could still pursue some version of this, but I'd like to see more clearly-communicated, modern interest in it (ideally, after the cloud of issues surrounding T12616 start to settle).

(If something you care about got merged here and isn't covered at HEAD / by T12616, feel free to file a new report or feature request adhering to modern guidelines.)