From 41857b4fcd3c9419d6e734137acca128c6406fa8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Dec 2025 14:12:33 -0800 Subject: [PATCH] some improvements --- templates/repo/diff/blob_excerpt.tmpl | 28 +++--- templates/repo/diff/section_split.tmpl | 12 ++- templates/repo/diff/section_unified.tmpl | 8 +- web_src/js/features/file-fold.ts | 47 +++++++++- web_src/js/features/repo-code.ts | 10 +-- web_src/js/features/repo-diff-selection.ts | 100 +++++++++++++-------- web_src/js/features/repo-diff.ts | 51 +++++------ 7 files changed, 161 insertions(+), 95 deletions(-) diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index a9323d4778..c08ee935af 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -11,7 +11,8 @@ {{else}} {{$inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale}} - + {{- $leftAnchor := Iif $line.LeftIdx (printf "diff-%sL%d" $.FileNameHash $line.LeftIdx) "" -}} + {{if and $line.LeftIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.LeftIdx}}{{end}} @@ -27,7 +28,8 @@ {{- end -}} - + {{- $rightAnchor := Iif $line.RightIdx (printf "diff-%sR%d" $.FileNameHash $line.RightIdx) "" -}} + {{if and $line.RightIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.RightIdx}}{{end}} @@ -59,16 +61,18 @@ {{end}} {{end}} -{{else}} - {{range $k, $line := $.section.Lines}} - - {{if eq .GetType 4}} - {{$line.RenderBlobExcerptButtons $.FileNameHash $diffBlobExcerptData}} - {{else}} - - - {{end}} - {{$inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale}} + {{else}} + {{range $k, $line := $.section.Lines}} + + {{if eq .GetType 4}} + {{$line.RenderBlobExcerptButtons $.FileNameHash $diffBlobExcerptData}} + {{else}} + {{- $leftAnchor := Iif $line.LeftIdx (printf "diff-%sL%d" $.FileNameHash $line.LeftIdx) "" -}} + + {{- $rightAnchor := Iif $line.RightIdx (printf "diff-%sR%d" $.FileNameHash $line.RightIdx) "" -}} + + {{end}} + {{$inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale}} {{if $inlineDiff.EscapeStatus.Escaped}}{{end}} diff --git a/templates/repo/diff/section_split.tmpl b/templates/repo/diff/section_split.tmpl index da5390978a..0cc8303d5b 100644 --- a/templates/repo/diff/section_split.tmpl +++ b/templates/repo/diff/section_split.tmpl @@ -24,7 +24,8 @@ {{$match := index $section.Lines $line.Match}} {{- $leftDiff := ""}}{{if $line.LeftIdx}}{{$leftDiff = $section.GetComputedInlineDiffFor $line ctx.Locale}}{{end}} {{- $rightDiff := ""}}{{if $match.RightIdx}}{{$rightDiff = $section.GetComputedInlineDiffFor $match ctx.Locale}}{{end}} - + {{- $leftAnchor := Iif $line.LeftIdx (printf "diff-%sL%d" $file.NameHash $line.LeftIdx) "" -}} + {{if $line.LeftIdx}}{{if $leftDiff.EscapeStatus.Escaped}}{{end}}{{end}} @@ -39,7 +40,8 @@ {{- end -}} - + {{- $matchRightAnchor := Iif $match.RightIdx (printf "diff-%sR%d" $file.NameHash $match.RightIdx) "" -}} + {{if $match.RightIdx}}{{if $rightDiff.EscapeStatus.Escaped}}{{end}}{{end}} {{if $match.RightIdx}}{{end}} @@ -56,7 +58,8 @@ {{else}} {{$inlineDiff := $section.GetComputedInlineDiffFor $line ctx.Locale}} - + {{- $leftAnchor := Iif $line.LeftIdx (printf "diff-%sL%d" $file.NameHash $line.LeftIdx) "" -}} + {{if $line.LeftIdx}}{{if $inlineDiff.EscapeStatus.Escaped}}{{end}}{{end}} {{if $line.LeftIdx}}{{end}} @@ -71,7 +74,8 @@ {{- end -}} - + {{- $rightAnchor := Iif $line.RightIdx (printf "diff-%sR%d" $file.NameHash $line.RightIdx) "" -}} + {{if $line.RightIdx}}{{if $inlineDiff.EscapeStatus.Escaped}}{{end}}{{end}} {{if $line.RightIdx}}{{end}} diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index ddd48b1d84..b66206446c 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -19,9 +19,11 @@ {{end}} {{else}} - - - {{end}} + {{- $leftAnchor := Iif $line.LeftIdx (printf "diff-%sL%d" $file.NameHash $line.LeftIdx) "" -}} + + {{- $rightAnchor := Iif $line.RightIdx (printf "diff-%sR%d" $file.NameHash $line.RightIdx) "" -}} + + {{end}} {{$inlineDiff := $section.GetComputedInlineDiffFor $line ctx.Locale -}} {{- if $inlineDiff.EscapeStatus.Escaped -}} diff --git a/web_src/js/features/file-fold.ts b/web_src/js/features/file-fold.ts index 74b36c0096..fc68f9031f 100644 --- a/web_src/js/features/file-fold.ts +++ b/web_src/js/features/file-fold.ts @@ -1,19 +1,60 @@ import {svg} from '../svg.ts'; +function parseTransitionValue(value: string): number { + let max = 0; + for (const current of value.split(',')) { + const trimmed = current.trim(); + if (!trimmed) continue; + const isMs = trimmed.endsWith('ms'); + const numericPortion = Number.parseFloat(trimmed.replace(/ms|s$/u, '')); + if (Number.isNaN(numericPortion)) continue; + const duration = numericPortion * (isMs ? 1 : 1000); + max = Math.max(max, duration); + } + return max; +} + +function waitForTransitionEnd(element: Element): Promise { + if (!(element instanceof HTMLElement)) return Promise.resolve(); + const transitionTarget = element.querySelector('.diff-file-body') ?? element; + const styles = window.getComputedStyle(transitionTarget); + const transitionDuration = parseTransitionValue(styles.transitionDuration); + const transitionDelay = parseTransitionValue(styles.transitionDelay); + const total = transitionDuration + transitionDelay; + if (total === 0) return Promise.resolve(); + + return new Promise((resolve) => { + let resolved = false; + function cleanup() { + if (resolved) return; + resolved = true; + transitionTarget.removeEventListener('transitionend', onTransitionEnd); + resolve(); + } + function onTransitionEnd(event: TransitionEvent) { + if (event.target !== transitionTarget) return; + cleanup(); + } + transitionTarget.addEventListener('transitionend', onTransitionEnd); + window.setTimeout(cleanup, total + 50); + }); +} + // Hides the file if newFold is true, and shows it otherwise. The actual hiding is performed using CSS. // // The fold arrow is the icon displayed on the upper left of the file box, especially intended for components having the 'fold-file' class. // The file content box is the box that should be hidden or shown, especially intended for components having the 'file-content' class. // -export function setFileFolding(fileContentBox: Element, foldArrow: HTMLElement, newFold: boolean) { +export function setFileFolding(fileContentBox: Element, foldArrow: HTMLElement, newFold: boolean): Promise { foldArrow.innerHTML = svg(`octicon-chevron-${newFold ? 'right' : 'down'}`, 18); fileContentBox.setAttribute('data-folded', String(newFold)); if (newFold && fileContentBox.getBoundingClientRect().top < 0) { fileContentBox.scrollIntoView(); } + return waitForTransitionEnd(fileContentBox); } // Like `setFileFolding`, except that it automatically inverts the current file folding state. -export function invertFileFolding(fileContentBox:HTMLElement, foldArrow: HTMLElement) { - setFileFolding(fileContentBox, foldArrow, fileContentBox.getAttribute('data-folded') !== 'true'); +export function invertFileFolding(fileContentBox:HTMLElement, foldArrow: HTMLElement): Promise { + return setFileFolding(fileContentBox, foldArrow, fileContentBox.getAttribute('data-folded') !== 'true'); } diff --git a/web_src/js/features/repo-code.ts b/web_src/js/features/repo-code.ts index bf7fd762b0..c7e34f2fd7 100644 --- a/web_src/js/features/repo-code.ts +++ b/web_src/js/features/repo-code.ts @@ -3,14 +3,6 @@ import {createTippy} from '../modules/tippy.ts'; import {toAbsoluteUrl} from '../utils.ts'; import {addDelegatedEventListener} from '../utils/dom.ts'; -function changeHash(hash: string) { - if (window.history.pushState) { - window.history.pushState(null, null, hash); - } else { - window.location.hash = hash; - } -} - // it selects the code lines defined by range: `L1-L3` (3 lines) or `L2` (singe line) function selectRange(range: string): Element { for (const el of document.querySelectorAll('.code-view tr.active')) el.classList.remove('active'); @@ -65,7 +57,7 @@ function selectRange(range: string): Element { for (let i = startLineNum - 1; i <= stopLineNum - 1 && i < elLineNums.length; i++) { elLineNums[i].closest('tr').classList.add('active'); } - changeHash(`#${range}`); + window.history.replaceState(null, '', `#${range}`); updateIssueHref(range); updateViewGitBlameFragment(range); updateCopyPermalinkUrl(range); diff --git a/web_src/js/features/repo-diff-selection.ts b/web_src/js/features/repo-diff-selection.ts index 906bff3a02..7d4f70efeb 100644 --- a/web_src/js/features/repo-diff-selection.ts +++ b/web_src/js/features/repo-diff-selection.ts @@ -1,10 +1,10 @@ import {addDelegatedEventListener} from '../utils/dom.ts'; -import {sleep} from '../utils.ts'; import {setFileFolding} from './file-fold.ts'; const diffLineNumberCellSelector = '#diff-file-boxes .code-diff td.lines-num[data-line-num]'; const diffAnchorSuffixRegex = /([LR])(\d+)$/; const diffHashRangeRegex = /^(diff-[0-9a-f]+)([LR]\d+)(?:-([LR]\d+))?$/i; +export const diffAutoScrollAttr = 'data-auto-scroll-running'; type DiffAnchorSide = 'L' | 'R'; type DiffAnchorInfo = {anchor: string, fragment: string, side: DiffAnchorSide, line: number}; @@ -13,16 +13,20 @@ type DiffSelectionRange = {fragment: string, startSide: DiffAnchorSide, startLin let diffSelectionStart: DiffSelectionState | null = null; -function changeHash(hash: string) { - if (window.history.pushState) { - window.history.pushState(null, null, hash); - } else { - window.location.hash = hash; - } +function scrollDiffAnchorIntoView(targetElement: HTMLElement, currentHash: string) { + targetElement.scrollIntoView(); + document.body.setAttribute(diffAutoScrollAttr, 'true'); + window.location.hash = ''; + window.location.hash = currentHash; + setTimeout(() => document.body.removeAttribute(diffAutoScrollAttr), 0); +} + +function isDiffAnchorId(id: string | null): boolean { + return id !== null && id.startsWith('diff-'); } function parseDiffAnchor(anchor: string | null): DiffAnchorInfo | null { - if (!anchor || !anchor.startsWith('diff-')) return null; + if (!isDiffAnchorId(anchor)) return null; const suffixMatch = diffAnchorSuffixRegex.exec(anchor); if (!suffixMatch) return null; const line = Number.parseInt(suffixMatch[2]); @@ -32,7 +36,7 @@ function parseDiffAnchor(anchor: string | null): DiffAnchorInfo | null { return {anchor, fragment, side, line}; } -function applyDiffLineSelection(container: HTMLElement, range: DiffSelectionRange, options?: {updateHash?: boolean}): boolean { +function applyDiffLineSelection(container: HTMLElement, range: DiffSelectionRange): boolean { // Find the start and end anchor elements const startId = `${range.fragment}${range.startSide}${range.startLine}`; const endId = `${range.fragment}${range.endSide}${range.endLine}`; @@ -50,8 +54,10 @@ function applyDiffLineSelection(container: HTMLElement, range: DiffSelectionRang tr.classList.remove('active'); } - // Get all rows in the diff section - const allRows = Array.from(container.querySelectorAll('.code-diff tbody tr')); + // gather rows from the actual table that contains the selection to avoid missing hunks + const codeDiffTable = startSpan.closest('.code-diff'); + if (!codeDiffTable || !codeDiffTable.contains(endSpan)) return false; + const allRows = Array.from(codeDiffTable.querySelectorAll('tbody tr')); const startIndex = allRows.indexOf(startTr); const endIndex = allRows.indexOf(endTr); @@ -70,18 +76,25 @@ function applyDiffLineSelection(container: HTMLElement, range: DiffSelectionRang } } - if (options?.updateHash !== false) { - const startAnchor = `${range.fragment}${range.startSide}${range.startLine}`; - const hashValue = (range.startSide === range.endSide && range.startLine === range.endLine) ? - startAnchor : - `${startAnchor}-${range.endSide}${range.endLine}`; - changeHash(`#${hashValue}`); - } return true; } +function buildDiffHash(range: DiffSelectionRange): string { + const startAnchor = `${range.fragment}${range.startSide}${range.startLine}`; + if (range.startSide === range.endSide && range.startLine === range.endLine) { + return startAnchor; + } + return `${startAnchor}-${range.endSide}${range.endLine}`; +} + +function updateDiffHash(range: DiffSelectionRange) { + const hashValue = `#${buildDiffHash(range)}`; + if (window.location.hash === hashValue) return; + window.history.replaceState(null, '', hashValue); +} + export function parseDiffHashRange(hashValue: string): DiffSelectionRange | null { - if (!hashValue.startsWith('diff-')) return null; + if (!isDiffAnchorId(hashValue)) return null; const match = diffHashRangeRegex.exec(hashValue); if (!match) return null; const startInfo = parseDiffAnchor(`${match[1]}${match[2]}`); @@ -105,19 +118,36 @@ export function parseDiffHashRange(hashValue: string): DiffSelectionRange | null }; } +async function waitNextAnimationFrame() { + await new Promise((resolve) => requestAnimationFrame(() => resolve(undefined))); +} + export async function highlightDiffSelectionFromHash(): Promise { const {hash} = window.location; if (!hash || !hash.startsWith('#diff-')) return false; - const range = parseDiffHashRange(hash.substring(1)); - if (!range) return false; + const hashValue = hash.substring(1); + const range = parseDiffHashRange(hashValue); + if (!range) { + if (document.body.hasAttribute(diffAutoScrollAttr)) return false; + // eslint-disable-next-line unicorn/prefer-query-selector + const targetElement = document.getElementById(hashValue); + if (!targetElement) return false; + scrollDiffAnchorIntoView(targetElement, hash); + return true; + } const targetId = `${range.fragment}${range.startSide}${range.startLine}`; // Wait for the target element to be available (in case it needs to be loaded) - const targetSpan = document.querySelector(`#${CSS.escape(targetId)}`); + let targetSpan = document.querySelector(`#${CSS.escape(targetId)}`); if (!targetSpan) { - // Target not found - it might need to be loaded via "show more files" - // Return false to let onLocationHashChange handle the loading - return false; + // Flush pending DOM mutations (htmx, folding animations, etc.) before giving up + await waitNextAnimationFrame(); + targetSpan = document.querySelector(`#${CSS.escape(targetId)}`); + if (!targetSpan) { + // Target not found - it might need to be loaded via "show more files" + // Return false to let onLocationHashChange handle the loading + return false; + } } const container = targetSpan.closest('.diff-file-box'); @@ -127,14 +157,13 @@ export async function highlightDiffSelectionFromHash(): Promise { if (container.getAttribute('data-folded') === 'true') { const foldBtn = container.querySelector('.fold-file'); if (foldBtn) { - // Expand the file using the setFileFolding utility - setFileFolding(container, foldBtn, false); - // Wait a bit for the expansion animation - await sleep(100); + // Expand the file and wait for any transition to finish before selecting lines + await setFileFolding(container, foldBtn, false); } } - if (!applyDiffLineSelection(container, range, {updateHash: false})) return false; + if (!applyDiffLineSelection(container, range)) return false; + updateDiffHash(range); diffSelectionStart = { anchor: targetId, fragment: range.fragment, @@ -145,10 +174,10 @@ export async function highlightDiffSelectionFromHash(): Promise { // 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); + await waitNextAnimationFrame(); const targetTr = targetSpan.closest('tr'); if (targetTr) { - targetTr.scrollIntoView({behavior: 'smooth', block: 'center'}); + targetTr.scrollIntoView({block: 'center'}); } return true; } @@ -189,11 +218,7 @@ function handleDiffLineNumberClick(cell: HTMLElement, e: MouseEvent) { clickedRow.classList.remove('active'); diffSelectionStart = null; // Remove hash from URL completely - if (window.history.pushState) { - window.history.pushState(null, null, window.location.pathname + window.location.search); - } else { - window.location.hash = ''; - } + window.history.replaceState(null, '', window.location.pathname + window.location.search); window.getSelection().removeAllRanges(); return; } @@ -216,6 +241,7 @@ function handleDiffLineNumberClick(cell: HTMLElement, e: MouseEvent) { }; if (applyDiffLineSelection(container, range)) { + updateDiffHash(range); if (!e.shiftKey || !diffSelectionStart || diffSelectionStart.container !== container || diffSelectionStart.fragment !== info.fragment) { diffSelectionStart = {...info, container}; } diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index 73c77dc795..2e414a4f75 100644 --- a/web_src/js/features/repo-diff.ts +++ b/web_src/js/features/repo-diff.ts @@ -11,7 +11,7 @@ import {createTippy} from '../modules/tippy.ts'; import {invertFileFolding} from './file-fold.ts'; import {parseDom, sleep} from '../utils.ts'; import {registerGlobalSelectorFunc} from '../modules/observer.ts'; -import {parseDiffHashRange, highlightDiffSelectionFromHash, initDiffLineSelection} from './repo-diff-selection.ts'; +import {parseDiffHashRange, highlightDiffSelectionFromHash, initDiffLineSelection, diffAutoScrollAttr} from './repo-diff-selection.ts'; function initRepoDiffFileBox(el: HTMLElement) { // switch between "rendered" and "source", for image and CSV files @@ -221,56 +221,53 @@ function initRepoDiffShowMore() { async function onLocationHashChange() { // try to scroll to the target element by the current hash const currentHash = window.location.hash; - if (!currentHash.startsWith('#diff-') && !currentHash.startsWith('#issuecomment-')) return; + const issueCommentPrefix = '#issuecomment-'; + const isDiffHash = currentHash.startsWith('#diff-'); + const isIssueCommentHash = currentHash.startsWith(issueCommentPrefix); + if (!isDiffHash && !isIssueCommentHash) return; // 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; - // Check if this is a diff line selection hash (e.g., #diff-xxxL10 or #diff-xxxL10-R20) const hashValue = currentHash.substring(1); - const range = parseDiffHashRange(hashValue); - if (range) { - // This is a line selection hash, try to highlight it first + let targetElementId = hashValue; + + if (isDiffHash) { const success = await highlightDiffSelectionFromHash(); if (success) { // Successfully highlighted and scrolled, we're done return; } - // If not successful, fall through to load more files + const range = parseDiffHashRange(hashValue); + if (range) { + targetElementId = `${range.fragment}${range.startSide}${range.startLine}`; + } } - const targetElementId = hashValue; while (currentHash === window.location.hash) { - // For line selections, check the range-based target - let targetElement; - if (range) { - const targetId = `${range.fragment}${range.startSide}${range.startLine}`; + if (isDiffHash) { // eslint-disable-next-line unicorn/prefer-query-selector - targetElement = document.getElementById(targetId); + const targetElement = document.getElementById(targetElementId); if (targetElement) { // Try again to highlight and scroll now that the element is loaded - await highlightDiffSelectionFromHash(); - return; + const success = await highlightDiffSelectionFromHash(); + if (success) return; } - } else { - // use getElementById to avoid querySelector throws an error when the hash is invalid + } else if (isIssueCommentHash) { // eslint-disable-next-line unicorn/prefer-query-selector - targetElement = document.getElementById(targetElementId); - if (targetElement) { - // need to change hash to re-trigger ":target" CSS selector, let's manually scroll to it - targetElement.scrollIntoView(); - document.body.setAttribute(attrAutoScrollRunning, 'true'); + const commentElement = document.getElementById(hashValue); + if (commentElement) { + commentElement.scrollIntoView(); + document.body.setAttribute(diffAutoScrollAttr, 'true'); window.location.hash = ''; window.location.hash = currentHash; - setTimeout(() => document.body.removeAttribute(attrAutoScrollRunning), 0); + setTimeout(() => document.body.removeAttribute(diffAutoScrollAttr), 0); return; } } // If looking for a hidden comment, try to expand the section that contains it - const issueCommentPrefix = '#issuecomment-'; - if (currentHash.startsWith(issueCommentPrefix)) { + if (isIssueCommentHash) { const commentId = currentHash.substring(issueCommentPrefix.length); const expandButton = document.querySelector(`.code-expander-button[data-hidden-comment-ids*=",${commentId},"]`); if (expandButton) {