Open Bug 1470340 Opened Last year Updated 5 months ago

make it clear which version of a diff my review comments applied to

Categories

(Conduit :: Phabricator, enhancement, P2)

enhancement

Tracking

(Not tracked)

People

(Reporter: heycam, Unassigned)

References

Details

(Keywords: conduit-triaged, conduit-upstream)

A suggestion for an improvement to the UI when reviewing.  Please let me know if this is already possible and I just couldn't see it. :-)

Sometimes I will be reviewing a diff and while I am reviewing, the author uploads a new version of the diff.  When I submit my comments, they should be associated with the old version of the diff, but I can't find anything in the UI that indicates this.  Is there a way to make it obvious that my comments were on a specific version of the diff?

Relatedly, even when the author doesn't upload a new version while I am reviewing, as a reviewer I want to know what new changes I need to review when there have been multiple versions.  I want to know what the last version of a diff I reviewed was, so I can go to the interdiff view to show only the changes since that version.

Is this possible, or is there a way we can add this?  mozreview showed a little slider that also indicated what version of a patch was last reviewed.

(FWIW I find mozreview's use of version numbers starting from 1, 2, 3, ... to be easier to get my head around than some global version number like 4232, 4233, ...)
This can *sort of* be done manually. If you scroll your browser down to the diff a toolbar will appear along the top with a hamburger menu. You can click the "List Inline Comments" menu item to be taken to a page listing them all[1], along with the diff numbers they apply to. From this you can tell what the last diff you reviewed is.

That being said, maybe upstream would be open to exposing this information in a more friendly way.

[1] https://phabricator.services.mozilla.com/differential/revision/inlines/<revision-id-without-D>/
Keywords: conduit-triaged
Whiteboard: [phabricator-backlog][phabricator-upstream]
On https://phabricator.services.mozilla.com/D13125?id=46240#378319 one link correctly identifies diff 46233.
But the others do not.

https://phabricator.services.mozilla.com/differential/revision/inlines/13125/ does know, and the greyed out comment is another hint.
Blocks: 1515556
Duplicate of this bug: 1522030
Whiteboard: [phabricator-backlog][phabricator-upstream]
Keywords: conduit-backlog
Priority: -- → P5
Duplicate of this bug: 1547264
Duplicate of this bug: 1546419
Priority: P5 → P2
No longer blocks: 1515556
You need to log in before you can comment on or make changes to this bug.