Skip to content

Conversation

@brymut
Copy link
Contributor

@brymut brymut commented Oct 14, 2025

Fixes #32257
/claim #32257

Implemented commenting on unchanged lines in Pull Request diffs, lines are accessed by expanding the diff preview. Comments also appear in the "Files Changed" tab on the unchanged lines where they were placed.

Screenshots

  • Comment form on expanded lines not in diff
Screenshot 2025-10-14 at 19 40 31
  • Comments available to view on Files Changed tab, with badge indicating that there are hidden comments available to view.
Screen.Recording.2025-10-16.at.17.34.43.mov
  • Comments available to view on Conversation tab, with line context preview to show where they are located.
Screenshot 2025-10-15 at 15 03 31
  • Full commenting process
Screen.Recording.2025-10-18.at.14.33.43.mp4
  • Shared link expands to hidden comment
Screen.Recording.2025-10-18.at.06.08.55.mov

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 14, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Oct 14, 2025
@brymut brymut force-pushed the enable-commenting-unchanged-line branch from 75d0beb to 75193bd Compare October 14, 2025 16:56
@brymut brymut marked this pull request as ready for review October 14, 2025 17:01
@brymut
Copy link
Contributor Author

brymut commented Oct 14, 2025

Hi @techknowlogick / @lunny, could you please have a look

@lunny
Copy link
Member

lunny commented Oct 14, 2025

One concern is how users can tell there are comments on lines that aren’t currently expanded. It would be helpful to display an icon or marker to indicate the presence of comments when those lines cannot be automatically expanded. I believe this is also one of the main challenges of this issue.

@brymut
Copy link
Contributor Author

brymut commented Oct 14, 2025

One concern is how users can tell there are comments on lines that aren’t currently expanded

Thanks, will look into a suitable way to address this.

@brymut brymut force-pushed the enable-commenting-unchanged-line branch from 75193bd to cb9d8aa Compare October 15, 2025 10:13
@brymut brymut force-pushed the enable-commenting-unchanged-line branch from cb9d8aa to 17be181 Compare October 15, 2025 11:00
@brymut
Copy link
Contributor Author

brymut commented Oct 15, 2025

Hi @lunny, I added badges on the code expanding buttons to indicate that there are hidden comments outside of the diff. I think this should cover it, in combination of the code context preview in the conversations tab to show where the comments are in context as well at a glance.

Screen.Recording.2025-10-15.at.15.02.42.mov
Screenshot 2025-10-15 at 15 03 31

@lunny lunny added this to the 1.26.0 milestone Oct 15, 2025
@lunny
Copy link
Member

lunny commented Oct 15, 2025

This floating counter looks great, but there’s a small issue: if the comments are part of the content loaded the second time, then after the first load, the floating box doesn’t appear.

@brymut brymut force-pushed the enable-commenting-unchanged-line branch from 497f690 to b234f31 Compare October 16, 2025 14:50
@brymut
Copy link
Contributor Author

brymut commented Oct 16, 2025

then after the first load, the floating box doesn’t appear.

Hadn't noticed, updated to include updated count of hidden comments if not revealed yet after code expansion.

Screen.Recording.2025-10-16.at.17.34.43.mov

@brymut
Copy link
Contributor Author

brymut commented Oct 16, 2025

Hi @lunny, thanks for the feedback. Addressed them in comments/changes & updated screen recording with fixes. Please have a look.

@brymut brymut requested a review from lunny October 16, 2025 14:56
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 19, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 19, 2025

Oh wait, a new bug:

image

Expand, there is only one comment (at the first line of that section)

(ps: the right side has more "deleted lines", so left 54 ---- right 42 )

image

@wxiaoguang wxiaoguang self-requested a review October 19, 2025 04:22
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Oct 19, 2025
@wxiaoguang
Copy link
Contributor

The root problem is that FillHiddenCommentIDsForDiffLine logic is not right.

  • Line < 0: use Left to calculate
  • Line > 0: use Right to calculate

@brymut

This comment was marked as resolved.

@brymut
Copy link
Contributor Author

brymut commented Oct 19, 2025

I can see the issue, even just adding the comment on the line 51, with no comment on the unchanged lines before, we're still getting a badge count of 1

Screenshot 2025-10-19 at 11 41 02

Edit, you were right, FillHiddenCommentIDsForDiffLine was problematic. Left-side comments were being counted as hidden comments in the right-side expand button badge.
Fixed:

  • before expansion
Screenshot 2025-10-19 at 11 56 23
  • after expansion
Screenshot 2025-10-19 at 11 56 29

@brymut brymut force-pushed the enable-commenting-unchanged-line branch from d920c87 to 78cd2ce Compare October 19, 2025 09:00
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 19, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 19, 2025

Oh wait, a new problem.

When using Split diff, then if the comment is added by "left" side, then the assumption if commentLineNum < 0 // Skip left-side, unchanged lines always use "right (proposed)" side for comments is wrong .....

image

Update: made a quick fix. af864c2

@brymut
Copy link
Contributor Author

brymut commented Oct 19, 2025

When using Split diff, the if the comment is added by "left" side, then the assumption if commentLineNum < 0 // Skip left-side, unchanged lines always use "right (proposed)" side for comments is wrong .....

Thanks, the semantics here have really got into the weeds for me. But I think I see what you mean

@wxiaoguang wxiaoguang merged commit c30d74d into go-gitea:main Oct 19, 2025
26 checks passed
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 20, 2025
* giteaofficial/main:
  Avoid emoji mismatch and allow to only enable chosen emojis (go-gitea#35692)
  feat(diff): Enable commenting on expanded lines in PR diffs (go-gitea#35662)
  Fix various bugs (go-gitea#35684)
  Fix workflow run event status while rerunning a failed job (go-gitea#35689)
  Use gitrepo.Repository instead of wikipath (go-gitea#35398)
  [skip ci] Updated translations via Crowdin
  Bump `actions/labeler` to v6 (go-gitea#35681)
  Use LFS object size instead of blob size when viewing a LFS file (go-gitea#35679)
saschazepter pushed a commit to saschazepter/gitea that referenced this pull request Oct 27, 2025
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 29, 2025
wxiaoguang added a commit that referenced this pull request Oct 29, 2025
Backport #35739 by wxiaoguang

This is a follow up for #35662, and also fix #31181, help #30275, fix
#31161

Co-authored-by: wxiaoguang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🙋 Bounty claim lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Commenting unchanged lines

4 participants