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:28:22Z

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

+    // Ignore clicks on or inside code-expander-buttons
+    const target = e.target as HTMLElement;
+    if (target.closest('.code-expander-button') || target.closest('.code-expander-buttons') ||
+      target.closest('button, a, input, select, textarea, summary, [role="button"]')) {


Where the list comes from? Won't it miss something?

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

web_src/js/features/repo-diff.ts

  initImageDiff();
  initDiffHeaderPopup();
+  // Re-apply hash selection in case the target was just loaded
+  await highlightDiffSelectionFromHash();


I think you should follow the TODO, but not keep adding unwanted code.

wxiaoguang · 2025-11-25T23:32:45Z

web_src/js/features/repo-diff.ts

+  const range = parseDiffHashRange(hashValue);
+  if (range) {
+    // This is a line selection hash, try to highlight it first
+    const success = await highlightDiffSelectionFromHash();


You should merge the old logic targetElement.scrollIntoView(); into highlightDiffSelectionFromHash

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

web_src/js/features/repo-diff.ts

+      const targetId = `${range.fragment}${range.startSide}${range.startLine}`;
+      // eslint-disable-next-line unicorn/prefer-query-selector
+      targetElement = document.getElementById(targetId);
+      if (targetElement) {
+        // Try again to highlight and scroll now that the element is loaded
+        await highlightDiffSelectionFromHash();
+        return;
+      }


It doesn't seem right.

What if then "endLine" is still not loaded.

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.

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.

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

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lunny commented Nov 24, 2025 •

edited

Loading

silverwind Nov 28, 2025 •

edited

Loading

silverwind Nov 28, 2025 •

edited

Loading

silverwind Nov 28, 2025 •

edited

Loading

silverwind Nov 28, 2025 •

edited

Loading