Add pull request files line selections #36014

lunny · 2025-11-24T01:15:47Z

This PR allows select multiple lines on the pull request files view page.

wxiaoguang · 2025-11-25T23:20:02Z

templates/repo/diff/blob_excerpt.tmpl

 		{{else}}
 			{{$inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale}}
-			<td class="lines-num lines-num-old" data-line-num="{{if $line.LeftIdx}}{{$line.LeftIdx}}{{end}}"><span rel="{{if $line.LeftIdx}}diff-{{$.FileNameHash}}L{{$line.LeftIdx}}{{end}}"></span></td>
+			<td class="lines-num lines-num-old" data-line-num="{{if $line.LeftIdx}}{{$line.LeftIdx}}{{end}}"><span rel="{{if $line.LeftIdx}}diff-{{$.FileNameHash}}L{{$line.LeftIdx}}{{end}}"{{if $line.LeftIdx}} id="diff-{{$.FileNameHash}}L{{$line.LeftIdx}}"{{end}}></span></td>


Why duplicate rel= and id=?

wxiaoguang · 2025-11-25T23:21:16Z

templates/repo/diff/blob_excerpt.tmpl

 		{{else}}
 			{{$inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale}}
-			<td class="lines-num lines-num-old" data-line-num="{{if $line.LeftIdx}}{{$line.LeftIdx}}{{end}}"><span rel="{{if $line.LeftIdx}}diff-{{$.FileNameHash}}L{{$line.LeftIdx}}{{end}}"></span></td>
+			<td class="lines-num lines-num-old" data-line-num="{{if $line.LeftIdx}}{{$line.LeftIdx}}{{end}}"><span rel="{{if $line.LeftIdx}}diff-{{$.FileNameHash}}L{{$line.LeftIdx}}{{end}}"{{if $line.LeftIdx}} id="diff-{{$.FileNameHash}}L{{$line.LeftIdx}}"{{end}}></span></td>


Why not have a clear defination like diff-{hash}-{L|R}xxx?

wxiaoguang · 2025-11-25T23:21:52Z

web_src/js/features/repo-diff-selection.ts

+function changeHash(hash: string) {
+  if (window.history.pushState) {
+    window.history.pushState(null, null, hash);
+  } else {
+    window.location.hash = hash;
+  }
+}


If it stays, it should be moved to web_src/js/utils/dom.ts as a general DOM utility.

But I think it's pointless to test for existance window.history.pushState, every browser released in the last 15 years should have that function, so we can just assume it's there, and in which case this function makes no sense and it should just use window.history.pushState directly.

Oh and please pass '' as second argument to it, typescript wants it that way.

Oh and likely this should use window.history.replaceState (see other comment below).

wxiaoguang · 2025-11-25T23:23:44Z

web_src/js/features/repo-diff-selection.ts

+}
+
+export function parseDiffHashRange(hashValue: string): DiffSelectionRange | null {
+  if (!hashValue.startsWith('diff-')) return null;


Not the first time see the same logic startsWith('diff-' ....

wxiaoguang · 2025-11-25T23:25:02Z

web_src/js/features/repo-diff-selection.ts

+    }
+  }
+
+  if (!applyDiffLineSelection(container, range, {updateHash: false})) return false;


updateHash option is an over-design.

You can simply:

if (!applyDiffLineSelection(container, range)) return false; updateHash(.....);

wxiaoguang · 2025-11-25T23:25:16Z

web_src/js/features/repo-diff-selection.ts

+  // Scroll to the first selected line (scroll to the tr element, not the span)
+  // The span is an inline element inside td, we need to scroll to the tr for better visibility
+  await sleep(10);


It can't be right

Maybe it needs a sleep(0) or requestAnimationFrame? Both should flush the JS event loop of pending tasks with the latter being faster.

wxiaoguang · 2025-11-25T23:34:31Z

web_src/js/features/repo-diff-selection.ts

+  return {anchor, fragment, side, line};
+}
+
+function applyDiffLineSelection(container: HTMLElement, range: DiffSelectionRange, options?: {updateHash?: boolean}): boolean {


All the complex logic needs tests if it is testable in frontend unit test.

Basic DOM interactions should be testable, thought I'm not sure if it's really worth it for this function.

If you can fully understand the logic and confirm its right, then not worth.

Replicating the DOM structure in a unit test will be hard. Ideally we should have e2e tests for this kind of stuff, but we don't (yet). If possible, extract testable functions here which ideally have no DOM interaction.

Purely logic code:

And DOM operations without layout & backend:

Yeah such non-DOM logic is easily testable and should be tested.

unit test added at 4f65ae3

silverwind · 2025-11-28T00:13:04Z

web_src/js/features/repo-diff-selection.ts

+      // Expand the file using the setFileFolding utility
+      setFileFolding(container, foldBtn, false);
+      // Wait a bit for the expansion animation
+      await sleep(100);


Can use https://developer.mozilla.org/en-US/docs/Web/API/Element/transitionend_event to detect when a transition has finished but it may be better to make setFileFolding return a Promise that resolves when the transition has ended.

silverwind · 2025-11-28T00:16:52Z

web_src/js/features/repo-diff-selection.ts

+  await sleep(10);
+  const targetTr = targetSpan.closest('tr');
+  if (targetTr) {
+    targetTr.scrollIntoView({behavior: 'smooth', block: 'center'});


I personally hate smooth scrolling, maybe scroll instantly?

silverwind · 2025-11-28T00:18:27Z

web_src/js/features/repo-diff-selection.ts

+          window.history.pushState(null, null, window.location.pathname + window.location.search);
+        } else {
+          window.location.hash = '';
+        }


Maybe you actually want to use window.history.replaceState? I hate sites that push useless history entries because it pollutes the "back" stack, making the user hit the back button more than is necessary.

wxiaoguang · 2025-12-14T11:12:28Z

web_src/js/features/file-fold.ts

+  return max;
+}
+
+function waitForTransitionEnd(element: Element): Promise<void> {


It is 100% garbage function .....

wxiaoguang · 2025-12-14T11:16:45Z

web_src/js/features/repo-diff.ts

  // avoid reentrance when we are changing the hash to scroll and trigger ":target" selection
-  const attrAutoScrollRunning = 'data-auto-scroll-running';
-  if (document.body.hasAttribute(attrAutoScrollRunning)) return;
+  if (document.body.hasAttribute(diffAutoScrollAttr)) return;


Why it is called diffAutoScrollAttr? See the comment, the attrAutoScrollRunning was designed for "issue comment scrolling"

wxiaoguang · 2025-12-14T11:20:41Z

web_src/js/features/repo-diff-selection.ts

+  let targetSpan = document.querySelector<HTMLElement>(`#${CSS.escape(targetId)}`);
+  if (!targetSpan) {
+    // Flush pending DOM mutations (htmx, folding animations, etc.) before giving up
+    await waitNextAnimationFrame();


Why it needs to wait for the animation?

wxiaoguang · 2025-12-14T11:25:39Z

The logic is quite complex, I would suggest to carefully review every line and make sure every line is clear. Or you can also help to answer my questions. Thank you. @lafriks @Zettat123

wxiaoguang · 2025-12-14T12:11:47Z

web_src/js/features/repo-diff-selection.ts

+    if (e.defaultPrevented) return;
+    handleDiffLineNumberClick(cell, e);
+  });
+  window.addEventListener('hashchange', () => {


Why duplicate "hashchange" listeners? Won't it conflict with window.addEventListener('hashchange', onLocationHashChange);?

wxiaoguang · 2025-12-14T13:23:26Z

web_src/js/features/repo-diff-selection.ts

+  window.location.hash = '';
+  window.location.hash = currentHash;


Do you think readers can understand why it needs these two lines?

When you copy code, why not also copy the comments? Or, why not introduce a common function for such requirement?

wxiaoguang · 2025-12-14T13:30:12Z

web_src/js/features/repo-diff.ts

+      if (targetElement) {
+        // Try again to highlight and scroll now that the element is loaded
+        const success = await highlightDiffSelectionFromHash();
+        if (success) return;


Is this loop still right?

If the selected line are in the expanded area (blob excerpt), now you just keep "loading more files", but the selected range can never be really loaded.

lunny added 3 commits November 23, 2025 16:45

Add pull request files line selections

1ec71e8

Fix bug

c144b64

Fix unfold when loading with line selections

373b530

lunny added this to the 1.26.0 milestone Nov 24, 2025

lunny added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Nov 24, 2025

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 24, 2025

github-actions bot added modifies/templates This PR modifies the template files modifies/frontend labels Nov 24, 2025

Support select the blank position of the line numbers

89989c5

lunny marked this pull request as draft November 24, 2025 01:40

lunny added 3 commits November 23, 2025 17:42

Fix bug when selecting plain lines

35b8f11

Fix bug

5580cc7

Fix bug

83d18d6

lunny marked this pull request as ready for review November 24, 2025 18:41

lunny added 2 commits November 25, 2025 09:29

fix selection css

45d2fbc

Merge branch 'main' into lunny/add_pr_files_selection

f8567d0

Zettat123 approved these changes Nov 25, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 25, 2025

lafriks approved these changes Nov 25, 2025

View reviewed changes

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 Nov 25, 2025

lunny mentioned this pull request Nov 25, 2025

[Feature Request] Add code suggestion for PR review comments #14765

Open

wxiaoguang reviewed Nov 25, 2025

View reviewed changes

wxiaoguang marked this pull request as draft November 25, 2025 23:33

wxiaoguang reviewed Nov 25, 2025

View reviewed changes

silverwind reviewed Nov 28, 2025

View reviewed changes

lunny added 11 commits December 11, 2025 14:12

some improvements

41857b4

Merge branch 'main' into lunny/add_pr_files_selection

a5738ef

Fix conflicts

91fc220

Fix format

ac5f96b

Fix format

dbfa95b

Remove unnecessary code

3140aac

Remove unnecessary code

ff5544a

improvements

a9d887a

Merge branch 'main' into lunny/add_pr_files_selection

cd8130e

Fix bug

cf809d3

Add unit test for applyDiffLineSelection

4f65ae3

lunny marked this pull request as ready for review December 14, 2025 01:39

wxiaoguang reviewed Dec 14, 2025

View reviewed changes

wxiaoguang requested a review from lafriks December 14, 2025 11:25

GiteaBot removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Dec 14, 2025

wxiaoguang requested a review from Zettat123 December 14, 2025 11:25

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 14, 2025

wxiaoguang reviewed Dec 14, 2025

View reviewed changes

		window.location.hash = '';
		window.location.hash = currentHash;

Uh oh!

Add pull request files line selections #36014

Are you sure you want to change the base?

Add pull request files line selections #36014

Conversation

lunny commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunny commented Nov 24, 2025 •

edited

Loading

silverwind Nov 28, 2025 •

edited

Loading

silverwind Nov 28, 2025 •

edited

Loading

wxiaoguang Nov 28, 2025 •

edited

Loading

wxiaoguang Nov 28, 2025 •

edited

Loading

silverwind Nov 28, 2025 •

edited

Loading

silverwind Nov 28, 2025 •

edited

Loading