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 }); + }); +});