From 4ca5bc974fbe97f8214b7eb4c859cebda98ddd8d Mon Sep 17 00:00:00 2001 From: Chad Miller Date: Sun, 30 Aug 2026 11:51:07 +0200 Subject: [PATCH] feat(git-ui): address a line, or a span of them, from the gutter A reader who wanted somebody else to look at one line had no way to point at it. Every line number in a diff and in the file explorer is now a link: a click marks the line and writes it into the address, and a shift-click takes the span from the line already marked to the one clicked. That is the gesture the review composer already uses for a multi-line comment. A diff names the file and the side, since one page holds many of them: `#line-src/pds.js-new:12-20`. A file's own page has the path in the address already, so there the fragment is the numbers alone, `#L12-20`, and a row's id is that fragment, which is what the browser scrolls to unasked. A link that arrives before the diff or the file has rendered is followed once it lands. The file explorer drew its body as one block of text beside one block of numbers. It draws a row per line now, coloured through the same `highlightToLines` the diff uses. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01G4wLfhyaFLGWducP1ow5b7 --- .../src/components/molecules/diff-file.jsx | 106 ++++++++-- .../src/components/molecules/source-lines.jsx | 98 ++++++++++ packages/git-ui/src/lib/line-links.js | 134 +++++++++++++ packages/git-ui/src/lib/navigation.jsx | 25 +++ packages/git-ui/src/pages/commit.jsx | 54 +++++- packages/git-ui/src/pages/file.jsx | 29 +-- packages/git-ui/test/line-links.test.js | 181 ++++++++++++++++++ 7 files changed, 574 insertions(+), 53 deletions(-) create mode 100644 packages/git-ui/src/components/molecules/source-lines.jsx create mode 100644 packages/git-ui/src/lib/line-links.js create mode 100644 packages/git-ui/test/line-links.test.js diff --git a/packages/git-ui/src/components/molecules/diff-file.jsx b/packages/git-ui/src/components/molecules/diff-file.jsx index cf09204..7bd05b8 100644 --- a/packages/git-ui/src/components/molecules/diff-file.jsx +++ b/packages/git-ui/src/components/molecules/diff-file.jsx @@ -9,6 +9,7 @@ import { inSpan, lineKey } from '#/lib/comments.js'; import { ROW_STYLE, SIGN, SIGN_STYLE } from '#/lib/diff-rows.js'; import { bytes } from '#/lib/format.js'; import { diffHighlighter, languageFor } from '#/lib/highlight.js'; +import { inSelection, lineFragment, lineId } from '#/lib/line-links.js'; import { useObjectUrl } from '#/lib/object-url.js'; import { cn } from '#/lib/utils.js'; import { verdictLook } from '#/lib/verdicts.js'; @@ -21,6 +22,40 @@ const STATUS_STYLE = { modified: '', }; +/** + * One number in the gutter, which is also the address of its line. A plain + * click selects the line, and shift takes the span from the line already + * selected to this one. The anchor is a real link, so the browser's own menu + * copies it and a middle click opens it. + * + * A column showing a number the line is not addressed by draws it plainly. A + * context line is addressed on the new side, the way a comment anchors to it, + * so its old number reads as a number and nothing more. + */ +function LineNumber({ value, fragment, style, className, onSelect }) { + if (fragment == null) { + return ( + + {value ?? ''} + + ); + } + return ( + { + event.preventDefault(); + onSelect(event.shiftKey); + }} + > + {value} + + ); +} + /** * One side of a changed image, with its size under it. A side the repository * holds as a Git LFS pointer has no picture to show, so the caption carries @@ -63,8 +98,19 @@ function DiffImage({ image, mediaType, label, path }) { * and `onComment` to publish a new one; * each renders under the line it speaks to, which is what an anchor is for. A * plain commit page passes neither and reads as it always did. + * + * `selection` is the span of lines the address names, when it names one in + * this file, and `onSelectLine` sets a new one from a click in the gutter. */ -export function DiffFile({ file, ref, comments, spans, onComment }) { +export function DiffFile({ + file, + ref, + comments, + spans, + onComment, + selection, + onSelectLine, +}) { const language = languageFor(file.path); const [open, setOpen] = useState(true); // Colouring a whole file twice costs more than colouring a hunk, so it @@ -279,39 +325,65 @@ export function DiffFile({ file, ref, comments, spans, onComment }) { {hunk.lines.map((line) => { const key = lineKey(line); const here = comments?.get(key) ?? []; + const onNew = line.newLine != null; + const at = line.newLine ?? line.oldLine; + const fragment = + onSelectLine && at != null + ? lineFragment({ + path: file.path, + side: onNew ? 'new' : 'old', + start: at, + end: at, + }) + : null; + const select = (shift) => onSelectLine(line, shift); + const linked = inSelection(selection, file.path, line); return (
{/* Two columns of numbers cost a quarter of a phone's width before a line of code begins. One column there instead: a line belongs to one side or the other, and the sign beside it says which. */} - - {line.newLine ?? line.oldLine ?? ''} - - + - {line.oldLine ?? ''} - - + - {line.newLine ?? ''} - + className={cn( + 'hidden shrink-0 select-none px-2 text-right text-faint tabular-nums sm:block', + linked && 'text-key', + )} + /> {/* At the seam between the numbers and the code, the way a forge's gutter button sits, floating so a long line scrolls under it rather than pushing it diff --git a/packages/git-ui/src/components/molecules/source-lines.jsx b/packages/git-ui/src/components/molecules/source-lines.jsx new file mode 100644 index 0000000..2b62e60 --- /dev/null +++ b/packages/git-ui/src/components/molecules/source-lines.jsx @@ -0,0 +1,98 @@ +import { useEffect, useMemo } from 'react'; +import { highlightToLines, languageFor } from '#/lib/highlight.js'; +import { + extendSpan, + parseSourceFragment, + sourceFragment, + sourceLineId, +} from '#/lib/line-links.js'; +import { useHash } from '#/lib/navigation.jsx'; +import { cn } from '#/lib/utils.js'; + +/** + * A file's text, a line to a row, each row addressed by its number. + * + * A click on a number selects that line and writes it into the address; a + * shift-click takes the span from the line already selected to this one. Every + * number is a real link, so the browser's own menu copies it. + * + * highlight.js escapes the source it is given, so nothing in the file can + * reach the page as markup. + */ +export function SourceLines({ text, path }) { + const [hash, setHash] = useHash(); + const span = parseSourceFragment(hash); + const lines = useMemo( + () => highlightToLines(text.replace(/\n$/, ''), languageFor(path)), + [text, path], + ); + // ch is the advance of a figure in the face the gutter is set in, so the + // column holds exactly its digits plus its own padding. + const digits = Math.max(2, String(lines.length).length); + const gutter = { width: `calc(${digits}ch + 2rem)` }; + + // A link from elsewhere names a line, and the file arrives after the first + // paint, so the browser's own fragment scroll finds nothing. This one runs + // once the rows are on the page. The page keys this component by the file, + // so opening another one runs it again. It reads the address from the window + // rather than from the hook above, so a click on a number moves nothing + // under the reader. + useEffect(() => { + const named = parseSourceFragment( + decodeURIComponent(window.location.hash.slice(1)), + ); + if (!named) return; + document + .getElementById(sourceLineId(named.start)) + ?.scrollIntoView({ block: 'center' }); + }, []); + + return ( + // One box as wide as the longest line, with every row filling it. Sized to + // each row instead, a short row's wash would stop where its text does and + // leave the rest of the scroll unpainted. +
+
+ {lines.map((html, index) => { + const number = index + 1; + const marked = span && number >= span.start && number <= span.end; + return ( + + ); + })} +
+
+ ); +} diff --git a/packages/git-ui/src/lib/line-links.js b/packages/git-ui/src/lib/line-links.js new file mode 100644 index 0000000..ffdf3cf --- /dev/null +++ b/packages/git-ui/src/lib/line-links.js @@ -0,0 +1,134 @@ +// The address of a line, so a reader can send somebody to it. +// +// In a diff the fragment names the file, the side, and one line or a span of +// them: `line-src/pds.js-new:12-20`. The path is written whole, and the side +// and the numbers close the fragment, so the reader of a link can see what it +// points at. A file's own page has the path in the address already, so there +// the fragment is the numbers alone: `L12-20`. + +/** + * The id one diff row answers to, so a link naming a line finds it. + * @param {string} path + * @param {string} key - the row's key, as `lineKey` writes it + * @returns {string} + */ +export function lineId(path, key) { + return `line-${path}-${key}`; +} + +/** + * The fragment naming a selected line or span. + * @param {{path: string, side: string, start: number, end: number}} selection + * @returns {string} + */ +export function lineFragment(selection) { + const at = + selection.end === selection.start + ? String(selection.start) + : `${selection.start}-${selection.end}`; + return `line-${selection.path}-${selection.side}:${at}`; +} + +/** A path may hold a hyphen, so the side and the numbers are read from the end. */ +const FRAGMENT = /^line-(.+)-(old|new):(\d+)(?:-(\d+))?$/; + +/** + * Read a selection back out of a fragment. + * @param {string} hash - the fragment, decoded, without its `#` + * @returns {{path: string, side: string, start: number, end: number}|null} + */ +export function parseLineFragment(hash) { + const found = FRAGMENT.exec(hash); + if (!found) return null; + const [, path, side, first, last] = found; + const one = Number(first); + const other = last === undefined ? one : Number(last); + return { + path, + side, + start: Math.min(one, other), + end: Math.max(one, other), + }; +} + +/** + * The id one source line answers to, which is the fragment naming that line + * alone. The browser scrolls to it without being asked. + * @param {number} number + * @returns {string} + */ +export function sourceLineId(number) { + return `L${number}`; +} + +/** + * The fragment naming a line or a span of them on a file's own page. + * @param {{start: number, end: number}} span + * @returns {string} + */ +export function sourceFragment(span) { + return span.end === span.start + ? `L${span.start}` + : `L${span.start}-${span.end}`; +} + +/** The second number reads with or without its own L, as a forge writes it. */ +const SOURCE = /^L(\d+)(?:-L?(\d+))?$/; + +/** + * Read a span back out of a fragment on a file's own page. + * @param {string} hash - the fragment, decoded, without its `#` + * @returns {{start: number, end: number}|null} + */ +export function parseSourceFragment(hash) { + const found = SOURCE.exec(hash); + if (!found) return null; + const one = Number(found[1]); + const other = found[2] === undefined ? one : Number(found[2]); + return { start: Math.min(one, other), end: Math.max(one, other) }; +} + +/** + * The span a click makes. Shift takes it from the line already selected to + * the line clicked; anything else selects the one line. + * @param {{start: number, end: number}|null|undefined} span + * @param {number} at + * @param {boolean} [shift] + * @returns {{start: number, end: number}} + */ +export function extendSpan(span, at, shift) { + if (shift && span) { + return { start: Math.min(span.start, at), end: Math.max(span.start, at) }; + } + return { start: at, end: at }; +} + +/** + * The selection a click makes in a diff. A shift-click reaches the line + * already selected only in the same file and on the same side. + * @param {{path: string, side: string, start: number, end: number}|null} selection + * @param {{path: string, side: string, at: number}} target + * @param {boolean} [shift] + * @returns {{path: string, side: string, start: number, end: number}} + */ +export function extendSelection(selection, target, shift) { + const { path, side, at } = target; + const here = + selection?.path === path && selection.side === side ? selection : null; + return { path, side, ...extendSpan(here, at, shift) }; +} + +/** + * Whether a diff row is one of the lines a selection names. + * @param {{path: string, side: string, start: number, end: number}|null|undefined} selection + * @param {string} path + * @param {{oldLine: number|null, newLine: number|null}} line + * @returns {boolean} + */ +export function inSelection(selection, path, line) { + if (!selection || selection.path !== path) return false; + const side = line.newLine != null ? 'new' : 'old'; + const at = line.newLine ?? line.oldLine; + if (at == null || side !== selection.side) return false; + return at >= selection.start && at <= selection.end; +} diff --git a/packages/git-ui/src/lib/navigation.jsx b/packages/git-ui/src/lib/navigation.jsx index e0e1f8a..c6d57b9 100644 --- a/packages/git-ui/src/lib/navigation.jsx +++ b/packages/git-ui/src/lib/navigation.jsx @@ -30,3 +30,28 @@ export function useNavigate() { [navigate], ); } + +/** + * The fragment on the address now, and a way to set one. + * + * A fragment names a place on the page the reader is already on, so setting + * one replaces the history entry rather than adding to it, and neither the + * router nor the browser moves the page to it. + * @returns {[string, (hash: string) => void]} + */ +export function useHash() { + const hash = useRouterState({ select: (state) => state.location.hash }); + const navigate = useRouterNavigate(); + const setHash = useCallback( + (next) => { + navigate({ + hash: next, + replace: true, + resetScroll: false, + hashScrollIntoView: false, + }); + }, + [navigate], + ); + return [hash, setHash]; +} diff --git a/packages/git-ui/src/pages/commit.jsx b/packages/git-ui/src/pages/commit.jsx index ea6459f..44ba847 100644 --- a/packages/git-ui/src/pages/commit.jsx +++ b/packages/git-ui/src/pages/commit.jsx @@ -38,6 +38,13 @@ import { threadSummaries, } from '#/lib/comments.js'; import { defaultRef, readChange, repoRecord, repoSize } from '#/lib/git.js'; +import { + extendSelection, + lineFragment, + lineId, + parseLineFragment, +} from '#/lib/line-links.js'; +import { useHash } from '#/lib/navigation.jsx'; import { publishReview } from '#/lib/publish.js'; import { useSession } from '#/lib/session.jsx'; import { cn } from '#/lib/utils.js'; @@ -168,6 +175,22 @@ export function CommitPage({ [pull, all], ); + // The lines the address names, so a link to a line marks it for whoever + // opens it and the address always says what is marked. + const [hash, setHash] = useHash(); + const selection = useMemo(() => parseLineFragment(hash), [hash]); + const selectLine = useCallback( + (path, line, shift) => { + const side = line.newLine != null ? 'new' : 'old'; + const at = line.newLine ?? line.oldLine; + if (at == null) return; + setHash( + lineFragment(extendSelection(selection, { path, side, at }, shift)), + ); + }, + [selection, setHash], + ); + const addStatement = useCallback((statement) => { setMine((held) => [...held, statement]); }, []); @@ -207,16 +230,25 @@ export function CommitPage({ [session, repo, tip, author, ref], ); - // A link from the pull request list names a thread in the fragment. The diff and - // its comments arrive after the first paint, so the browser's own fragment - // scroll finds nothing; this one waits for them. A hash changed in place on - // an already-open page re-renders nothing and is not chased. + // A link from the pull request list names a thread in the fragment, and a + // shared link names a line. The diff and its comments arrive after the first + // paint, so the browser's own fragment scroll finds nothing; this one waits + // for them. The address is read from the window rather than from the hook + // above, so a click in the gutter moves nothing under the reader. useEffect(() => { - if (!data || !showFiles || comments.byFile.size === 0) return; - const hash = decodeURIComponent(window.location.hash.slice(1)); - if (!hash.startsWith('comment-')) return; - document.getElementById(hash)?.scrollIntoView({ block: 'center' }); - }, [data, comments, showFiles]); + if (!data || !shown || !showFiles) return; + const at = decodeURIComponent(window.location.hash.slice(1)); + if (at.startsWith('comment-')) { + if (comments.byFile.size === 0) return; + document.getElementById(at)?.scrollIntoView({ block: 'center' }); + return; + } + const lines = parseLineFragment(at); + if (!lines) return; + document + .getElementById(lineId(lines.path, `${lines.side}:${lines.start}`)) + ?.scrollIntoView({ block: 'center' }); + }, [data, shown, comments, showFiles]); if (!record) { return

No such repository.

; @@ -483,6 +515,10 @@ export function CommitPage({ comments={comments.byFile.get(file.path)} spans={comments.spans.get(file.path)} onComment={reviewable && session ? onComment : undefined} + selection={selection} + onSelectLine={(line, shift) => + selectLine(file.path, line, shift) + } ref={(node) => { if (node) sections.current.set(file.path, node); else sections.current.delete(file.path); diff --git a/packages/git-ui/src/pages/file.jsx b/packages/git-ui/src/pages/file.jsx index ac900e7..1c54b04 100644 --- a/packages/git-ui/src/pages/file.jsx +++ b/packages/git-ui/src/pages/file.jsx @@ -7,10 +7,10 @@ import { Breadcrumbs } from '#/components/molecules/breadcrumbs.jsx'; import { FileCommit } from '#/components/molecules/file-commit.jsx'; import { ReadingRepo } from '#/components/molecules/reading-repo.jsx'; import { RefSelect } from '#/components/molecules/ref-select.jsx'; +import { SourceLines } from '#/components/molecules/source-lines.jsx'; import { SourcePanes } from '#/components/molecules/source-panes.jsx'; import { bytes } from '#/lib/format.js'; import { capabilities, rawUrl, repoRecord, repoSize } from '#/lib/git.js'; -import { highlightBlock, languageFor } from '#/lib/highlight.js'; import { useNavigate } from '#/lib/navigation.jsx'; import { useObjectUrl } from '#/lib/object-url.js'; import { fileKey, parentOf, readSource, useFileCommit } from '#/lib/source.js'; @@ -111,32 +111,7 @@ export function FilePage({ repo, refName, path }) { )} ) : data.file.text !== null ? ( -
-
- - {/* highlight.js escapes the source it is given, so nothing - in the file can reach the page as markup. */} - 
-
-
+ ) : (

{data.file.binary diff --git a/packages/git-ui/test/line-links.test.js b/packages/git-ui/test/line-links.test.js new file mode 100644 index 0000000..1339ce0 --- /dev/null +++ b/packages/git-ui/test/line-links.test.js @@ -0,0 +1,181 @@ +/** + * The address of a line in a diff: what a click writes into the fragment, and + * what a reader arriving on that fragment gets back. + */ + +import { describe, expect, it } from 'vitest'; +import { + extendSelection, + extendSpan, + inSelection, + lineFragment, + lineId, + parseLineFragment, + parseSourceFragment, + sourceFragment, + sourceLineId, +} from '../src/lib/line-links.js'; + +/** @param {number|null} oldLine @param {number|null} newLine */ +const row = (oldLine, newLine) => ({ oldLine, newLine }); + +describe('lineFragment and parseLineFragment', () => { + it('carries one line there and back', () => { + const selection = { path: 'src/pds.js', side: 'new', start: 12, end: 12 }; + const fragment = lineFragment(selection); + expect(fragment).toBe('line-src/pds.js-new:12'); + expect(parseLineFragment(fragment)).toEqual(selection); + }); + + it('carries a span there and back', () => { + const selection = { path: 'src/pds.js', side: 'old', start: 12, end: 20 }; + const fragment = lineFragment(selection); + expect(fragment).toBe('line-src/pds.js-old:12-20'); + expect(parseLineFragment(fragment)).toEqual(selection); + }); + + // A path holds hyphens more often than not, and the fragment ends with a + // number that could be read as part of one. + it('reads a path with hyphens in it', () => { + const selection = { + path: 'packages/git-ui/src/diff-file.jsx', + side: 'new', + start: 3, + end: 9, + }; + expect(parseLineFragment(lineFragment(selection))).toEqual(selection); + }); + + it('answers nothing to a fragment naming something else', () => { + expect(parseLineFragment('comment-src/pds.js-new:12')).toBeNull(); + expect(parseLineFragment('')).toBeNull(); + expect(parseLineFragment('line-src/pds.js-both:12')).toBeNull(); + }); + + it('orders the ends of a span written backwards', () => { + expect(parseLineFragment('line-a.js-new:20-12')).toEqual({ + path: 'a.js', + side: 'new', + start: 12, + end: 20, + }); + }); + + it('names the row a link lands on', () => { + expect(lineId('src/pds.js', 'new:12')).toBe('line-src/pds.js-new:12'); + }); +}); + +describe('extendSelection', () => { + const at = { path: 'a.js', side: 'new', at: 20 }; + + it('selects one line on a plain click', () => { + expect(extendSelection(null, at)).toEqual({ + path: 'a.js', + side: 'new', + start: 20, + end: 20, + }); + }); + + it('takes the span from the line already selected', () => { + const first = { path: 'a.js', side: 'new', start: 12, end: 12 }; + expect(extendSelection(first, at, true)).toEqual({ + path: 'a.js', + side: 'new', + start: 12, + end: 20, + }); + }); + + // The line already selected is one end of the span, so a shift-click above + // it takes the lines between rather than starting again. + it('takes a span upwards from the line already selected', () => { + const first = { path: 'a.js', side: 'new', start: 30, end: 34 }; + expect(extendSelection(first, at, true)).toEqual({ + path: 'a.js', + side: 'new', + start: 20, + end: 30, + }); + }); + + it('starts again on another file or another side', () => { + const elsewhere = { path: 'b.js', side: 'new', start: 12, end: 12 }; + expect(extendSelection(elsewhere, at, true).start).toBe(20); + const otherSide = { path: 'a.js', side: 'old', start: 12, end: 12 }; + expect(extendSelection(otherSide, at, true).start).toBe(20); + }); +}); + +describe('inSelection', () => { + const selection = { path: 'a.js', side: 'new', start: 12, end: 20 }; + + it('marks the rows the selection names', () => { + expect(inSelection(selection, 'a.js', row(8, 12))).toBe(true); + expect(inSelection(selection, 'a.js', row(null, 16))).toBe(true); + expect(inSelection(selection, 'a.js', row(8, 11))).toBe(false); + }); + + it('marks nothing in another file, or with nothing selected', () => { + expect(inSelection(selection, 'b.js', row(null, 16))).toBe(false); + expect(inSelection(null, 'a.js', row(null, 16))).toBe(false); + }); + + // A deleted row is on the old side alone, which is the side it is addressed + // by, so a selection on the new side passes over it. + it('marks a deleted row only from the old side', () => { + expect(inSelection(selection, 'a.js', row(16, null))).toBe(false); + const old = { path: 'a.js', side: 'old', start: 12, end: 20 }; + expect(inSelection(old, 'a.js', row(16, null))).toBe(true); + }); +}); + +describe('sourceFragment and parseSourceFragment', () => { + it('carries a line and a span there and back', () => { + expect(sourceFragment({ start: 12, end: 12 })).toBe('L12'); + expect(sourceFragment({ start: 12, end: 20 })).toBe('L12-20'); + expect(parseSourceFragment('L12')).toEqual({ start: 12, end: 12 }); + expect(parseSourceFragment('L12-20')).toEqual({ start: 12, end: 20 }); + }); + + // A forge writes the second number with its own L, and a reader pasting one + // of those addresses means the same span. + it('reads a span whose second number carries an L', () => { + expect(parseSourceFragment('L12-L20')).toEqual({ start: 12, end: 20 }); + }); + + it('answers nothing to a fragment naming something else', () => { + expect(parseSourceFragment('line-a.js-new:12')).toBeNull(); + expect(parseSourceFragment('readme')).toBeNull(); + }); + + it('names the row a link lands on', () => { + expect(sourceLineId(12)).toBe('L12'); + expect(sourceLineId(12)).toBe(sourceFragment({ start: 12, end: 12 })); + }); +}); + +describe('extendSpan', () => { + it('selects one line on a plain click', () => { + expect(extendSpan({ start: 4, end: 9 }, 20)).toEqual({ + start: 20, + end: 20, + }); + }); + + it('takes the span from the line already selected, either way', () => { + expect(extendSpan({ start: 12, end: 12 }, 20, true)).toEqual({ + start: 12, + end: 20, + }); + expect(extendSpan({ start: 30, end: 34 }, 20, true)).toEqual({ + start: 20, + end: 30, + }); + }); + + it('selects one line when there is nothing to reach from', () => { + expect(extendSpan(null, 20, true)).toEqual({ start: 20, end: 20 }); + }); +}); -- 2.51.2