Add commits dropdown in PR files view and allow commit by commit review #25528

sebastian-sauer · 2023-06-26T18:54:35Z

This PR adds a new dropdown to select a commit or a commit range (shift-click like github) of a Pull Request.
After selection of a commit only the changes of this commit will be shown. When selecting a range of commits the diff of this range is shown.

This allows to review a PR commit by commit or by viewing only commit ranges.

The "Show changes since your last review" mechanism github uses is implemented, too.

When reviewing a single commit or a commit range the "Viewed" functionality is disabled.

Screenshots

The commit dropdown

Selecting a commit range

Show changes of a single commit only

Show changes of a commit range

Fixes #20989
Fixes #19263

when selecting a commit only the changes of this commit will be shown.

delvh · 2023-06-26T19:15:37Z

Hmm…
I wonder how that will play with #19007.
I can foresee at least three bugs that will become more present compared to now:

Files are viewed on too new commits #24813
The architecture is built at the moment to use the newest review as the latest review to compare against.
As long as you only move forward in the time, this is not a problem.
There's no performant way around this issue, as it would otherwise mean a very expensive comparison what the latest commit a review happened on is.
With your proposed approach, we can also backtrack in time, potentially leading to comparing the state against an older one than necessary.
I mean this especially as

a user can place arbitrary many reviews per PR, separated by the commit SHA that was the HEAD commit at the time the review
we need to know which review is the most recent one, and we have no knowledge of the git internals to check which commit SHA comes after the other

The other way around is most likely true as well (didn't test it):
If you review a PR on the newest commit, and then switch to an older commit, files you haven't viewed (yet) will probably be marked as viewed.

I think for now, the easiest thing to combat all these problems is to disable the viewed checkboxes (at best with a tooltip explaining why) (or alternatively hiding it completely) when you are only reviewing a specific commit of the PR.

sebastian-sauer · 2023-06-26T19:19:10Z

Hmm… I wonder how that will play with #19007. I can foresee at least three bugs that will become more present compared to now:

Files are viewed on too new commits #24813

The architecture is built at the moment to use the newest review as the latest review to compare against.
As long as you only move forward in the time, this is not a problem.
There's no performant way around this issue, as it would otherwise mean a very expensive comparison what the latest commit a review happened on is.
With your proposed approach, we can also backtrack in time, potentially leading to comparing the state against an older one than necessary.

The other way around is most likely true as well (didn't test it):
If you review a PR on the newest file, and then switch to an older commit, files you haven't viewed yet will probably be marked as viewed.

I think for now, the easiest thing to combat all these problems is to disable the viewed checkboxes (at best with a tooltip explaining why) (or alternatively hiding it completely) when you are only reviewing a specific commit of the PR.

Thanks for your feedback - disabling the Viewed Checkboxes should be an easy thing. I'll add this.
... by the way this reminds me to analyse the missing headcommit infomation ;-)

silverwind · 2023-06-26T19:56:53Z

As for UI:

I don't think radio button is the right. If at all, it should have a checkbox, but checkbox needs to be restricted to unbroken commit ranges, which will require a bit of JS to get working. If you decide to not support range, just remove the radio button.
There needs to be a border between dropdown elements.
All text-holding elements need to ellipsis-wrap, there is a helper class for it.

sebastian-sauer · 2023-06-26T20:06:27Z

thanks @silverwind - do you mean all span (class description and text) should have the class gt-ellipsis?

silverwind · 2023-06-26T20:33:46Z

At least the commit message and author line will needed it. Commit hash is fixed width, so doesn't.

sebastian-sauer · 2023-06-26T21:01:28Z

At least the commit message and author line will needed it. Commit hash is fixed width, so doesn't.

Thanks - i've pushed an updated version:

sebastian-sauer · 2023-06-27T19:59:42Z

@silverwind are there any more ui / review comments that I'll need to address?

@delvh is the current solution (disabling file viewed when loading specific commit) ok for you?

silverwind · 2023-06-27T23:02:30Z

Looks good. Something to test:

What happens when number of commits is large. Will the dropdown become scrollable?

delvh · 2023-06-28T08:15:31Z

@delvh is the current solution (disabling file viewed when loading specific commit) ok for you?

Yep, of course. Otherwise I wouldn't have suggested it.

web_src/css/repo.css

silverwind · 2023-06-28T18:55:26Z

@sebastian-sauer install a https://editorconfig.org/ plugin in your editor.

sebastian-sauer · 2023-06-28T18:55:44Z

Thanks @silverwind - i've now added a max height and overflow-y auto to add scrollbar for long lists.

The following screenshot is with 500 commits in one PR:

silverwind · 2023-06-28T18:57:08Z

I guess we can reduce item padding a bit in that dropdown specifically, maybe padding: 6px 3px.

puni9869 · 2023-06-28T18:58:04Z

padding: 6px 3p

I was thinking the same.

web_src/js/components/DiffCommitSelector.vue

sebastian-sauer · 2023-07-28T05:21:22Z

Thanks @wxiaoguang for your changes - these look good to me!

delvh · 2023-07-28T15:46:18Z

@sebastian-sauer I've updated your screenshots now with the current state.

GiteaBot · 2023-07-28T17:12:56Z

@sebastian-sauer please fix the merge conflicts. 🍵

delvh · 2023-07-28T18:06:45Z

templates/repo/diff/box.tmpl

-					{{$isReviewFile := and $.IsSigned $.PageIsPullFiles (not $.IsArchived) $.IsShowingAllCommits}}
-					<div class="diff-file-box diff-box file-content {{TabSizeClass $.Editorconfig $file.Name}} gt-mt-3" id="diff-{{$file.NameHash}}" data-old-filename="{{$file.OldName}}" data-new-filename="{{$file.Name}}" {{if or ($file.ShouldBeHidden) (not $isExpandable)}}data-folded="true"{{end}}>
+          {{$isReviewFile := and $.IsSigned $.PageIsPullFiles (not $.IsArchived) $.IsShowingAllCommits}}


Hmm… Somehow this line lost half of its indentation in the merge.

sebastian-sauer · 2023-07-28T18:32:44Z

Thank you @delvh for resolving the merge conflict. And thanks to all of you for all your support! It's always fun contributing to gitea!

delvh · 2023-07-28T18:35:20Z

But apparently the web editor used spaces instead of tabs which is why the CI fails.
Furthermore, that explains why the line lost half its indentation: The web merge conflict resolver probably used tab = 8 spaces…
@sebastian-sauer can you please fix the formatting?

sebastian-sauer · 2023-07-28T18:36:07Z

I'll check this and push a fix asap.

sebastian-sauer · 2023-07-28T18:38:39Z

Fixed.

delvh · 2023-07-28T19:07:39Z

I've just noticed something strange, which is present both in the screenshots I added, and in the original ones:
The count of added and deleted lines doesn't add up with the reality, especially when only reviewing a single commit of a PR:
My third screenshot for example shows 2 additions, 1 deletion. At the same only 1 file changed, and this file has 1 addition, 0 deletions. How can that be?

sebastian-sauer · 2023-07-28T19:12:40Z

hm - is there anything special with your commit? in my local environment it shows the exact number of lines changed.

sebastian-sauer · 2023-07-28T19:14:57Z

@delvh do you got Ignore Whitespace active? This could be the reason

With ignore whitespace - looks wrong (but is just wrong cause white space changes are not shown):

Without ignore whitespace:

delvh · 2023-07-28T19:16:02Z

I mean, it doesn't appear to occur on every commit.
Let's just book it under Cannot reproduce and be happy.
We'll see it again when a bug report about is opened.

sebastian-sauer · 2023-07-28T19:16:52Z

I mean, it doesn't appear to occur on every commit. Let's just book it under Cannot reproduce and be happy. We'll see it again when a bug report about is opened.

Please see my last comment - maybe it's caused by ignore whitespace option?

* origin/main: (43 commits) Add `/public/assets` to `.ignore` (go-gitea#26232) Fix attachment clipboard copy on insecure origin (go-gitea#26224) Fix commit compare style (go-gitea#26209) Fix unable to display individual-level project (go-gitea#26198) Fix access check for org-level project (go-gitea#26182) Fixed incorrect locale references (go-gitea#26218) Use calendar icon for `Joined on...` in profiles (go-gitea#26215) Add changelog for 1.20.2 (go-gitea#26208) Add commits dropdown in PR files view and allow commit by commit review (go-gitea#25528) Warn instead of reporting an error when a webhook cannot be found (go-gitea#26039) Fixing the align of commit stats in commit_page template. (go-gitea#26161) Fix allowed user types setting problem (go-gitea#26200) Hide branch/tag icon if branches/tags are empty (go-gitea#26204) Prevent primary key update on migration (go-gitea#26192) improve unit test for caching (go-gitea#26185) Render plaintext task list items for markdown files (go-gitea#26186) Add tooltip to describe LFS table column and color `delete LFS file` button red (go-gitea#26181) Show branches and tags that contain a commit (go-gitea#25180) Release attachments duplicated check (go-gitea#26176) Calculate MAX_WORKERS default value by CPU number (go-gitea#26177) ...

…#26434) Replace #26197 Since #25528 merged, the links of pull request commits should be redirect to pull file changes UI but not the generic one. --------- Co-authored-by: Giteabot <teabot@gitea.io>

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 26, 2023

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2023

Add commits dropdown in PR files view

acbd8dd

when selecting a commit only the changes of this commit will be shown.

sebastian-sauer force-pushed the feature/allow_review_commit_by_commit branch from 1a5797d to acbd8dd Compare June 26, 2023 18:57

sebastian-sauer changed the title ~~Add commits dropdown in PR files view~~ Add commits dropdown in PR files view and allow commit by commit review Jun 26, 2023

Disable File-Viewed when reviewing a single commit

bd1551c

Add ellipsis and divider

862ffc8

Add styles and ellipsis

836ee9b

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 26, 2023

sebastian-sauer mentioned this pull request Jun 27, 2023

Commits picker in PR change files view #19263

Closed

silverwind added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/ui Change the appearance of the Gitea UI labels Jun 27, 2023

Add max-height and scroll for list of commits

77a81bb

silverwind reviewed Jun 28, 2023

View reviewed changes

web_src/css/repo.css Outdated Show resolved Hide resolved

Update web_src/css/repo.css

45b7567

puni9869 mentioned this pull request Jun 28, 2023

Add branch name-copy buttons on the branches list #25569

Closed

wxiaoguang reviewed Jul 28, 2023

View reviewed changes

web_src/js/components/DiffCommitSelector.vue Outdated Show resolved Hide resolved

lunny mentioned this pull request Jul 28, 2023

Introduce pull request commit view files page #26197

Closed

refactor dom event listener

57f4864

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 28, 2023

GiteaBot added 2 commits July 28, 2023 23:50

Merge branch 'main' into feature/allow_review_commit_by_commit

39b4d0c

Merge branch 'main' into feature/allow_review_commit_by_commit

59588a5

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 28, 2023

Merge branch 'main' into feature/allow_review_commit_by_commit

275750e

delvh reviewed Jul 28, 2023

View reviewed changes

delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 28, 2023

Fix indentation

f4438d1

delvh merged commit 5553206 into go-gitea:main Jul 28, 2023
23 checks passed

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 28, 2023

sebastian-sauer deleted the feature/allow_review_commit_by_commit branch July 28, 2023 19:32

lunny mentioned this pull request Aug 10, 2023

Use correct pull request commit link instead of a generic commit link #26434

Merged

go-gitea locked as resolved and limited conversation to collaborators Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commits dropdown in PR files view and allow commit by commit review #25528

Add commits dropdown in PR files view and allow commit by commit review #25528

sebastian-sauer commented Jun 26, 2023 •

edited by delvh

Loading

delvh commented Jun 26, 2023 •

edited

Loading

sebastian-sauer commented Jun 26, 2023

silverwind commented Jun 26, 2023 •

edited

Loading

sebastian-sauer commented Jun 26, 2023

silverwind commented Jun 26, 2023

sebastian-sauer commented Jun 26, 2023

sebastian-sauer commented Jun 27, 2023

silverwind commented Jun 27, 2023

delvh commented Jun 28, 2023

silverwind commented Jun 28, 2023

sebastian-sauer commented Jun 28, 2023

silverwind commented Jun 28, 2023

puni9869 commented Jun 28, 2023 •

edited

Loading

sebastian-sauer commented Jul 28, 2023

delvh commented Jul 28, 2023

GiteaBot commented Jul 28, 2023

delvh Jul 28, 2023

sebastian-sauer commented Jul 28, 2023

delvh commented Jul 28, 2023 •

edited

Loading

sebastian-sauer commented Jul 28, 2023

sebastian-sauer commented Jul 28, 2023

delvh commented Jul 28, 2023 •

edited

Loading

sebastian-sauer commented Jul 28, 2023

sebastian-sauer commented Jul 28, 2023 •

edited

Loading

delvh commented Jul 28, 2023

sebastian-sauer commented Jul 28, 2023

Add commits dropdown in PR files view and allow commit by commit review #25528

Add commits dropdown in PR files view and allow commit by commit review #25528

Conversation

sebastian-sauer commented Jun 26, 2023 • edited by delvh Loading

Screenshots

The commit dropdown

Selecting a commit range

Show changes of a single commit only

Show changes of a commit range

delvh commented Jun 26, 2023 • edited Loading

sebastian-sauer commented Jun 26, 2023

silverwind commented Jun 26, 2023 • edited Loading

sebastian-sauer commented Jun 26, 2023

silverwind commented Jun 26, 2023

sebastian-sauer commented Jun 26, 2023

sebastian-sauer commented Jun 27, 2023

silverwind commented Jun 27, 2023

delvh commented Jun 28, 2023

silverwind commented Jun 28, 2023

sebastian-sauer commented Jun 28, 2023

silverwind commented Jun 28, 2023

puni9869 commented Jun 28, 2023 • edited Loading

sebastian-sauer commented Jul 28, 2023

delvh commented Jul 28, 2023

GiteaBot commented Jul 28, 2023

delvh Jul 28, 2023

Choose a reason for hiding this comment

sebastian-sauer commented Jul 28, 2023

delvh commented Jul 28, 2023 • edited Loading

sebastian-sauer commented Jul 28, 2023

sebastian-sauer commented Jul 28, 2023

delvh commented Jul 28, 2023 • edited Loading

sebastian-sauer commented Jul 28, 2023

sebastian-sauer commented Jul 28, 2023 • edited Loading

delvh commented Jul 28, 2023

sebastian-sauer commented Jul 28, 2023

sebastian-sauer commented Jun 26, 2023 •

edited by delvh

Loading

delvh commented Jun 26, 2023 •

edited

Loading

silverwind commented Jun 26, 2023 •

edited

Loading

puni9869 commented Jun 28, 2023 •

edited

Loading

delvh commented Jul 28, 2023 •

edited

Loading

delvh commented Jul 28, 2023 •

edited

Loading

sebastian-sauer commented Jul 28, 2023 •

edited

Loading