feat(ui): improve the user experience to review individual commits in a pull request #7155
Labels
No labels
arch
riscv64
backport/v1.19
backport/v1.20
backport/v1.21/forgejo
backport/v10.0/forgejo
backport/v11.0/forgejo
backport/v12.0/forgejo
backport/v13.0/forgejo
backport/v14.0/forgejo
backport/v7.0/forgejo
backport/v8.0/forgejo
backport/v9.0/forgejo
breaking
bug
bug
confirmed
bug
duplicate
bug
needs-more-info
bug
new-report
bug
reported-upstream
code/actions
code/api
code/auth
code/auth/faidp
code/auth/farp
code/email
code/federation
code/git
code/migrations
code/packages
code/wiki
database
MySQL
database
PostgreSQL
database
SQLite
dependency-upgrade
dependency
certmagic
dependency
chart.js
dependency
Chi
dependency
Chroma
dependency
citation.js
dependency
codespell
dependency
css-loader
dependency
devcontainers
dependency
dropzone
dependency
editorconfig-checker
dependency
elasticsearch
dependency
enmime
dependency
F3
dependency
ForgeFed
dependency
garage
dependency
Git
dependency
git-backporting
dependency
Gitea
dependency
gitignore
dependency
go-ap
dependency
go-enry
dependency
go-gitlab
dependency
Go-org
dependency
go-rpmutils
dependency
go-sql-driver mysql
dependency
go-swagger
dependency
go-version
dependency
go-webauthn
dependency
gocron
dependency
Golang
dependency
goldmark
dependency
goquery
dependency
Goth
dependency
grpc-go
dependency
happy-dom
dependency
Helm
dependency
image-spec
dependency
jsonschema
dependency
KaTeX
dependency
lint
dependency
MariaDB
dependency
Mermaid
dependency
minio-go
dependency
misspell
dependency
Monaco
dependency
PDFobject
dependency
playwright
dependency
postcss
dependency
postcss-plugins
dependency
pprof
dependency
prometheus client_golang
dependency
protobuf
dependency
relative-time-element
dependency
renovate
dependency
reply
dependency
ssh
dependency
swagger-ui
dependency
tailwind
dependency
temporal-polyfill
dependency
terminal-to-html
dependency
tests-only
dependency
text-expander-element
dependency
urfave
dependency
vfsgen
dependency
vite
dependency
Woodpecker CI
dependency
x tools
dependency
XORM
Discussion
duplicate
enhancement/feature
forgejo/accessibility
forgejo/branding
forgejo/ci
forgejo/commit-graph
forgejo/documentation
forgejo/furnace cleanup
forgejo/i18n
forgejo/interop
forgejo/moderation
forgejo/privacy
forgejo/release
forgejo/scaling
forgejo/security
forgejo/ui
Gain
High
Gain
Nice to have
Gain
Undefined
Gain
Very High
good first issue
i18n/backport-stable
impact
large
impact
medium
impact
small
impact
unknown
Incompatible license
issue
closed
issue
do-not-exist-yet
issue
open
manual test
Manually tested during feature freeze
OS
FreeBSD
OS
Linux
OS
macOS
OS
Windows
problem
QA
regression
release blocker
Release Cycle
Feature Freeze
release-blocker
v7.0
release-blocker
v7.0.1
release-blocker
v7.0.2
release-blocker
v7.0.3
release-blocker
v7.0.4
release-blocker
v8.0.0
release-blocker/v9.0.0
run-all-playwright-tests
run-end-to-end-tests
test
manual
test
needed
test
needs-help
test
not-needed
test
present
untested
User research - time-tracker
valuable code
worth a release-note
User research - Accessibility
User research - Blocked
User research - Community
User research - Config (instance)
User research - Errors
User research - Filters
User research - Future backlog
User research - Git workflow
User research - Labels
User research - Moderation
User research - Needs input
User research - Notifications/Dashboard
User research - Rendering
User research - Repo creation
User research - Repo units
User research - Security
User research - Settings (in-app)
No milestone
No project
No assignees
10 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/forgejo!7155
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "sclu1034/forgejo:feat/single-commit-review"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This implements the UI controls and information displays necessary to allow reviewing pull requests by stepping through commits individually.
Notable changes:
{owner}/{repo}/pulls/{id}/commits/{sha}Talking points:
Closes: #5670 #5126 #5671 #2281 #8084
Checklist
The contributor guide contains information that will be helpful to first time contributors. There also are a few conditions for merging Pull Requests in Forgejo repositories. You are also welcome to join the Forgejo development chatroom.
Tests
*_test.gofor unit tests.tests/integrationdirectory if it involves interactions with a live Forgejo server.web_src/js/*.test.jsif it can be unit tested.tests/e2e/*.test.e2e.jsif it requires interactions with a live Forgejo server (see also the developer guide for JavaScript testing).Documentation
Release notes
release-notes/<pull request number>.mdto be be used for the release notes instead of the title.Release notes
052d49cab6af3d4e7c14Will do a proper review later.
@ -932,0 +957,4 @@}if curCommit == nil {ctx.ServerError("Repo.GitRepo.viewPullFiles", git.ErrNotExist{ID: commitID})add
returnhere.Fixed in
13a6b16993@ -1511,1 +1511,3 @@m.Get("/{sha:[a-f0-9]{4,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)m.Group("/{sha:[a-f0-9]{4,40}}", func() {m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)m.Post("/reviews/submit", web.Bind(forms.SubmitReviewForm{}), repo.SubmitReview)Please add
context.RepoMustNotBeArchived()to the middleware (beforeweb.Bind)Fixed in
c2f32ded99Hi, I'd just like to comment that this is a very good concept that would solve one of my biggest problems with the "GitHub-like workflows" and I hope that this makes it into Forgejo. Thank you for working on this!
I didn't have a closer look so far, but I think what we want is the possibility to leave an actual review comment (that can be resolved, etc.) on the commit message and not a commit note. I share your doubts here and also think that this will probably cause confusion more than it actually helps to solve the problem.
I created a separate issue for the "review commit messages" aspect here: #7353.
I've just started migrating to codeberg and the lack of contextual PR navigation was the only thing so far that was limiting. Many thanks and kudos for implementing it! 👏🏼👏🏼
Please let's try and get this feature in. I'm not a go programmer, but will help out in any way I can if anyone requests it.
It needs conflict resolution and addressing review comments to move forward.
If you mean the comments from Gusted, those have already been addressed by the commits afterwards.
In that case you need to answer the review comment to say so. The reviewer can't be expected to read each commit that is pushed to figure out which one may address a comment that they made. Establishing that relationship falls on you, otherwise it may be missed. And it was evidently missed.
Then there is the matter of marking the review "resolved" or not. I personally think it is better to let the reviewer decide, based on the answer you made, if their comment was correctly addressed or not. But that's not a fixed rule.
85bb983c0d61583a3b58@sclu1034 sorry for the inconvenience, the CI failure is not your fault. You need to rebase when #7772 is merged.
61583a3b58d59b2c12d6I now have an understanding of what you're after and that's something I'd like in theory: using the single commit view is currently rarely convenient. I like that you've made individual commits that are well designed and isolated. The path is quite clear and that's an areas that needed love for some time ❤️
As with every UX / UI change, there will be discussions and opinions at every turn. For that you'll benefit from having individual pull requests so you're not frustrated by very long and hairy reviews.
A first (and rather non controversial) PR could be to restructure the template, add tests (which you did for JavaScript already but I suspect you'll need a few more to verify the template is evaluated properly and that the test breaks if you insert a typo somewhere in a template variable). This is the tedious part (not good) but also an immediate merge (very good) as it would change nothing and improve the test coverage of the existing code.
Once this is done it will be comparatively a lot less code for a PR that adds Next / Prev but more UX design discussions.
I have opinions on how the code added to the function should be split in a different function to not inflate the complexity too much, but that will, I think, get resolved as a side effect of having a series of PR rather than a single one.
TestPullDifftests are failing.@sclu1034 I made a note to spend time reviewing this pull request (and possibly other related pull requests) early June. Unless someone beats me to it, of course, I don't mean that as blocking other reviewers 😇 Just that I won't forget about it.
As promised I'm back to take a looks but see that it did not change and you did not reply @sclu1034. Did you notice my comment? What do you think about my suggestions?
@earl-warren wrote in #7155 (comment):
I did notice them, but they weren't directly actionable to me. And I refrained from replying at the time, both to give myself the opportunity for a another look at this later, and because it was already clear that a discussion could only happen at a later time.
You mentioned splitting off the template restructure and adding tests with the only specific example being that it should "[break] if you insert a typo somewhere in a template variable". As far as I'm aware that would already be covered by the template failing to compile.
So bar further examples, and with my lack of experience with the code base at the time, I did not come up with suitable test cases to work on back then.
That did change when I looked at this again yesterday, so that's something I started working on.
As for the general concept of splitting into small PRs, in my opinion, very few of the changes here make sense by themselves.
If, for example, "Link to PR-scoped commit" was merged on its own, it would make for an inferior product, because without the commit header, the view is less useful than the commit view, and without also adding the the prev/next buttons to the header it is tedious to navigate.
And I think it would also make it harder to keep the context of the change known to everyone involved, both reviewers and myself, in terms of how a discussion/change in one thing might affect everything else.
Additionally, I feel like a lot of small PRs, where each one can only start when the previous one has been merged, would lead to the longer reviews overall compared to the single one where, for example, the UX of the prev/next buttons can already be commented on while I might still be working on tests for the commit header.
As mentioned, since it is a reasonably independent change, the template extraction is now also a separate PR at #8061.
This PR has been rebased on top of that.
d59b2c12d61a3fd1e649@sclu1034 wrote in #7155 (comment):
Great.
I agree and your approach to have cleanly organized commits is helpful to review this pull request.
I also agree.
Please rebase. In the meantime I'll play with it manually and make a detailed review.
First hand UX experience - per commit review selection
Preparation
Without this PR
With this PR
Here is how I see the "needs and benefits" section of an hypothetical feature request (it may be a good idea to create one, even if only for the sake of the UX debate). I find best if this section focuses on collecting evidence of the UX problem rather than hinting on a possible solution. This PR is already a solution and my intent here is more to establish a clear problem statement a-posteriori rather than revisiting the proposed UX entirely. In other words, even if there could be a different UX to solve that same problem, I would prefer this one because I tried it and find it solves the problem for me. And also because an existing working solution should only be discarded if it tries to solve a non-existing problem or if it is horribly implemented.
Needs and benefits
When reviewing a pull request that has multiple commits well organized in a series, it may be convenient to review them one by one instead of browsing the entire diff of the accumulated commits. This is possible with a select menu.
To better understand the diff, the commit message can be displayed by opening the "Commits" in another browser tab, visually finding the SHA of the corresponding commit (the title is not displayed in the per-commit review page) and clicking on it.
The menu can then be used again to select another commit. To do so the review needs visually match the SHA of the current commit in the menu if they want to work on them sequentially.
Once the reviewer is done, they need to either select "Show all commits" from the menu to submit the review (it is not active when reviewing a single commit) or click on "Files changed".
First hand testimonies:
First hand UX experience - navigating commits linked from a pull request
Preparation
Without this PR
With this PR
Great piece of work 👍
From my point of view this is ready to merge but it needs review from @forgejo/UI, an area in which I'm not comfortable with. I hope my manual tests will help them and I'm sure the JavaScript tests you wrote will go a long way to speed up things.
I've been personally discouraged by the current UX and I wellcome this change. However, I think it is worth creating a minimal feature request issue (maybe re-using the "needs and benefits" section I suggested, or something else if you like). And with that collecting a few more first hand testimonies regarding how people have used the current UX and the problems they had, if any. It is, I think, enough to have a few. People expressing support for the feature request are absolutely not needed (although it is good for morale 😁). The goal is to collect facts, not speculations or opinions. If something think this is bad UX, they are entitled to their opinion but this is not helpful. If people remember being confused and can describe how, this is super useful.
@ -49,6 +49,9 @@<span class="commit-summary {{if gt .ParentCount 1}} grey text{{end}}" title="{{.Summary}}">{{.Summary | RenderEmoji $.Context}}</span>{{else}}{{$commitLink:= printf "%s/commit/%s" $commitRepoLink (PathEscape .ID.String)}}{{if $.PageIsPullCommits}}Non blocking suggestion: if - else instead of overwriting.
The variable would still need to be declared outside the if block, otherwise it fails with
So if the
elseis desired, it would have to look like this:I don't have strong feelings about that, it is fine as it is.
@ -53,7 +53,7 @@{{if not .DiffNotAvailable}}{{if and .IsShowingOnlySingleCommit .PageIsPullFiles}}<div class="ui info message"><div>{{ctx.Locale.Tr "repo.pulls.showing_only_single_commit" (ShortSha .AfterCommitID)}} - <a href="{{$.Issue.Link}}/files?style={{if $.IsSplitStyle}}split{{else}}unified{{end}}&whitespace={{$.WhitespaceBehavior}}&show-outdated={{$.ShowOutdatedComments}}">{{ctx.Locale.Tr "repo.pulls.show_all_commits"}}</a></div>I'm not sure to understand why this change is required, could you please explain? Or link to where you already explained and I missed.
Before this change, there were always two properties,
.BeforeCommitIDand.AfterCommitID, even when there was only a single commit in context.From a technical point of view, that's not that big of a deal, they can simply be set to the same value. But I did get confused multiple times about which one would be the "correct" one, and it has the potential to diverge if some part of the template uses "BeforeCommit", the other uses "AfterCommit".
If I only glance at the code,
.AfterCommitIDsuggests to me that there should be more than one commit in context.So, since I already had to touch the part of the backend code that sets these two properties, I chose to also change it to only create one property when there is only a single commit.
Got it, thanks for explaining.
1a3fd1e64954934b9109Rebased and added a few tests for the commit header in the diff view, akin to the other ones.
Interestingly, even though there a PRs with multiple commits in the test data, those would not show up in the corresponding timeline view. So I could only test the dedicated "Commits" tab.
Could you please add R#7155 to the list of issues this closes?
@earl-warren wrote in #7155 (comment):
7155 would be this PR itself. I've added #8084, though
@ -12,3 +12,3 @@{{end}}{{$commitLink:= printf "%s/commit/%s" $.comment.Issue.PullRequest.BaseRepo.Link (PathEscape .ID.String)}}{{$commitLink:= printf "%s/pulls/%d/commits/%s" $.comment.Issue.PullRequest.BaseRepo.Link $.comment.Issue.PullRequest.ID (PathEscape .ID.String)}}This returns an incorrect ID (if there are issues created before this PR), it must be
$.comment.Issue.ID.When you fix it, it would be great if you could also add a test case covering this.
I had tried to add tests for this with the other ones, but when I clicked through PRs in the test data, including the ones I used for the other tests, none of them had their commits show up in the timeline.
Is there a specific pull request in the fixtures that's been set up to have a proper timeline?
Fixed the link. Though I needed the repo scoped
Issue.Indexinstead.A test has been added as well.
!7155 (commit
f65aece6cd)This is fine. I would have nitpicking comments but this works and that is what matters.
I think there is the need for some furhter consideration... 🤔
I can comment on commits, but the comment is added to the latest state. This means if e.g. in commit 1 a new file is added, in commit 2 it is deleted, I can comment on it when I view commit 1:
In the conversation view, the diff is empty (because the comment is not really added to the specific state of the comment):
I have current no good idea how this can be fixed. Just changing how the comment works and add it to the specific commit is probably not the best solution 🤔
Otherwise: I would say the UI changes look good, but still need to take a look at the tests.
@ -21,0 +24,4 @@</a><a class="ui tiny button {{if .NextCommitLink}}" href="{{.NextCommitLink}}"{{else}}disabled"{{end}}>{{ctx.Locale.Tr "repo.diff.commit.next-short"}} {{svg "octicon-chevron-right"}}</a>Because of 16px icons, the buttons end up larger than usual. In this case the class
smallworks better thantiny.Because contents of conditions
{{if .PageIsPullFiles}}and{{else if not $.PageIsWiki}}are never shown together, it should be alright to not maintain complete consistency of buttons here - so usingsmallon the new buttons only is fine.I propose a quick UI fix, something better can be figured out later after we have some better foundation for buttons outside of Fomantic.
Change the class to
small.@ -19,2 +19,2 @@{{if not $.PageIsWiki}}<div class="commit-header-buttons"><div class="commit-header-buttons">{{if .PageIsPullFiles}}Mobile usability is pretty bad, the new buttons are quite long even with the shorter English variant.
Please add
max-sm:tw-flex-colto the Tailwind mess above that houses"tw-flex tw-mb-4 tw-gap-1".Add the class as suggested.
@Beowulf wrote in #7155 (comment):
If I'm not mistaken, this is an inconsistency that already exists in the current codebase and would deserve a bug report of its own. In that case I think it is out of scope for the current pull request. What do you think?
@earl-warren wrote in #7155 (comment):
I would say it is new, because previously yes, your comment is outdated when new commits are added or there is a force push changing the line you commented on. (But isn't the preview in the conversation tab still the old state?). Additionally the comment is marked as outdated.
Now you can comment and it isn't marked e.g. as outdated even if there is already a later commit changing it and it can produce empty previous as shown above. - But if you would say it is a separate future task, or an acceptable "regression" I'm also fine with it.
I now understand and it would lead to confusion indeed, good catch.
I just tried to manually craft !7155 (commit
b7177a23f4) which is an outdated commit of #7155 still in the repository atb7177a23f4and it is 404, presumably because there is a check to verify if it is part of the current PR commits and not an outdated one.@sclu1034 does it sound reasonable to change the current code so that links to the per-commit review is only displayed for commits that are not outdated?
pending the change to avoid displaying per-commit review link to outdated commits
@earl-warren wrote in #7155 (comment):
I haven't looked into it, but I would assume it's possible to treat outdated ones differently. Can you be more specific about what behavior you'd prefer instead of the current one?
I think #240 also covers the area of outdated commits in the history, but hasn't seen consensus, yet.
54934b9109c5c2e8c34c@sclu1034 wrote in #7155 (comment):
Yes and that is a more complicated one.
@sclu1034 wrote in #7155 (comment):
Let say the conversation tab is as follows:
and the
52f...commit (the first one), is outdated.Current behavior
It links to a commit specific review tab.
Desired behavior
It links to a display of the commit.
Does that sound reasonable? AFAIK, if this is done, there would be no way for the user to accidentally review an outdated commit. Except if they did not refresh a page that was displayed before the commit was outdated but I don't think addressing this border case is necessary.
Not sure if I'd consider my solution elegant.
Comment.Invalidatedwouldn't work since multiple commits group into oneCommitmight not all be outdated, and I didn't want to change any of the commit structs.This looks fine to me in principle. It may not be the most elegant solution but it is easy to understand/read/review.
Could you add a case in the tests that cover this? It is a strong candidate for future regressions 😁
@earl-warren wrote in #7155 (comment):
As mentioned earlier, I cannot find a PR in the test data that would allow me to test this.
b73df5be34c8675d25cb@sclu1034 wrote in #7155 (comment):
Maybe not in the existing fixtures but you could create one? AFAIU
TestProtectedBranch_AdminEnforcementneeds this as well. Or other tests that rely on force pushing. I'm not very surprised that there is no ready-to-use / mimic pull request integration test that involves a force push.Or is there a particular difficult that I'm not seeing?
c8675d25cbf65aece6cd@earl-warren wrote in #7155 (comment):
The difficulty of not being familiar with the code base, I guess. I was simply waiting for an answer, though.
That may be the case for you or the other maintainers, but I'm not yet at the point where I could have intuitions like that.
From my perspective, an answer like "Yes, you can use
user5/pull4711" could have been just as likely.I'll research how to write that test.
@earl-warren wrote in #7155 (comment):
Have you seen the most recent force push, specifically
f65aece6cd?I was not able to find a test for pull requests that:
.comment-list .timeline-itemSo it will be more work. I would
TestPullBranchDelete, copy/paste it and adaptTestPullCreate_TitleEscapeto get the relevant URLhtmlDoc.doc.Find(".comment-list .timeline-item....And before that probably ask @Gusted @christopher-besch and in the dev chatroom in case someone has a simpler way out of this.
Is this helpful?
@sclu1034 wrote in #7155 (comment):
I did not and as you can see I went in another direction 😅
The things raised in #7155 (comment) are unchanged. Where there a decision on it I cannot find or are they unadressed?
They are mitigated in a way that is, IMHO quite acceptable. But for clarification a comment from @sclu1034 to articulate how would help, I agree.
I may misunderstood what you meant on matrix @earl-warren with:
What I mean:
The history now looks like:
Previously:
Result:
In the conversation tab I have a comment with a valid preview and it is marked as outdated:
@Beowulf wrote in #7155 (comment):
I don't understand why you can no longer comment on the file. Here is how I would run the reproducer on the main forgejo branch (without this PR). I can start a per-review commit there too:
What am I missing?
@earl-warren wrote in #7155 (comment):
Oh, never realized I can also comment in the single commit view and thought that is new. My fault... Then it is nothing new.
The changes introduced regarding the force push, don't seem to work. I have a PR with 6 commits and amended the last commit and force pushed it. Now all commits link to the "default" commit view instead of the review commit view.
@Beowulf wrote in #7155 (comment):
I cannot reproduce this. I
The sixth commit now links to the commit view, while the other five still link to the review tab. Comparing the Conversation and Commits tab, the first five commits are the same (by SHA), the sixth is different.
I don't know what I saw, but youre right.
Looks good to me. Only thing you may want to add is that the sha in the force push (if the commit is still valid and included) also links to the single commit review.
Where does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/7155.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.This message and the release notes originate from a call to the release-notes-assistant.
Release notes
@sclu1034 I suspect this will make per-commit review actually used, if only because people will have an easy access to them via the links in the pull request and not via the icon in file review that is currently awkward to use.
For release notes,
suggests it did not exist before. What about something like this?
Implement single-commit PR review flowto feat(ui): improve the user experience to review individual commits in a pull requestWhere does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/7155.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.This message and the release notes originate from a call to the release-notes-assistant.
Release notes
congrats on the merge @sclu1034 :D
Where does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/7155.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.This message and the release notes originate from a call to the release-notes-assistant.
Release notes
forgejo-actions referenced this pull request from forgejo/website2025-08-11 19:03:56 +02:00
Create review from commitflaky e2e test #9049Unfortunately, it seems that the
Create review from commite2e test introduced in this PR is flaky.In the interest of it not blocking work in unrelated areas, I have proposed to disable it in #9049.
It would be fantastic if someone with knowledge of this PR could work on re-enabling the test, making it 100% reliable.
forgejo-actions referenced this pull request from forgejo/website2025-10-01 19:04:07 +02:00
forgejo-actions referenced this pull request from forgejo/website2025-10-05 19:04:05 +02:00
forgejo-actions referenced this pull request from forgejo/website2025-11-27 18:11:41 +01:00
forgejo-actions referenced this pull request from forgejo/website2025-12-08 18:03:53 +01:00
forgejo-actions referenced this pull request from forgejo/website2025-12-18 18:03:25 +01:00