From 1265c4eb15c03dc368b934febcca253886ea8682 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Tue, 20 Oct 2026 22:18:26 +0000 Subject: [PATCH 1/2] feat(panel): the diff bundle draws a whole file, and notes under one patch's lines mountFile draws a whole file with no diff through Pierre's CodeView holding the one file: virtualized, coloured in the kept pool, its lit lines Pierre's own, notes under their lines in the view's elements, lines picked by their numbers or a text selection, and its numbers left out of the find and of what a screen reader reads. mount takes notes under its lines again, for the Code cards' snippets. Co-authored-by: luke-agent[bot] <339870089+luke-agent[bot]@users.noreply.github.com> --- packages/panel/src/diff-view/contract.ts | 81 +++++- packages/panel/src/diff-view/index.ts | 262 +++++++++++++++++- .../panel/src/diff-view/whole-file.test.ts | 203 ++++++++++++++ .../panel/src/tasks/agent-changes-tab.test.ts | 6 + .../src/tasks/agent-chapters-tab.test.ts | 6 + .../src/tasks/agent-pull-request-tab.test.ts | 6 + packages/panel/src/tasks/themed-lines.test.ts | 6 + 7 files changed, 565 insertions(+), 5 deletions(-) create mode 100644 packages/panel/src/diff-view/whole-file.test.ts diff --git a/packages/panel/src/diff-view/contract.ts b/packages/panel/src/diff-view/contract.ts index 76e933bb2a..43251be1d0 100644 --- a/packages/panel/src/diff-view/contract.ts +++ b/packages/panel/src/diff-view/contract.ts @@ -10,7 +10,7 @@ import type { } from "@luke/settings/diff-view"; /** - * contract.ts -- what the panel and the diff bundle say to each other: a mount for one patch and one for a list of files, the handles the view redraws them through, and a door that colours a run of a file's lines as a diff is coloured. + * contract.ts -- what the panel and the diff bundle say to each other: a mount for one patch, one for a list of files, and one for a whole file with no diff, the handles the view redraws them through, and a door that colours a run of a file's lines as a diff is coloured. * * A changed file's patch is drawn by Pierre's diffs (`@pierre/diffs`), with * Shiki colouring it, which is too large for the bundle every window parses, @@ -101,13 +101,17 @@ export function sameNotes(a: readonly DiffAnnotation[], b: readonly DiffAnnotati ); } -/** What the view hands the bundle: one file's patch, headed as git writes one, how to draw it, the lines lit in it, if any, and whether it is an excerpt. */ +/** What the view hands the bundle: one file's patch, headed as git writes one, how to draw it, the lines lit in it, if any, whether it is an excerpt, and the notes under its lines, each drawn in the element the view keeps for it. */ export interface DiffViewProps { readonly patch: string; readonly look: DiffLook; readonly selected?: DiffSelection | undefined; /** Whether the patch is an excerpt, a few lines of a file as a review thread's, drawn with nothing said of the lines around them. */ readonly excerpt?: boolean | undefined; + /** The notes under the patch's lines; none where absent. */ + readonly annotations?: readonly DiffAnnotation[] | undefined; + /** The element each note is drawn in, by its key, which the view keeps and fills; asked only for a note `annotations` names. */ + readonly renderAnnotation?: ((key: string) => HTMLElement) | undefined; } /** @@ -195,6 +199,73 @@ export interface DiffListProps { readonly onDrawn?: ((path: string, lines: readonly number[]) => void) | undefined; } +/** A run of a whole file's lines, from its first to its last, counted from 1. */ +export interface FileLineRange { + readonly startLine: number; + readonly endLine: number; +} + +/** A note drawn under a line of a whole file, one of Luke's: its key, and the line it stands under. */ +export interface FileAnnotation { + readonly key: string; + readonly line: number; +} + +/** Whether two runs of a whole file's notes stand under the same lines by the same keys. */ +export function sameFileNotes(a: readonly FileAnnotation[], b: readonly FileAnnotation[]): boolean { + return ( + a.length === b.length && + a.every((note, at) => note.key === b[at]?.key && note.line === b[at]?.line) + ); +} + +/** Whether two runs of lines are the same lines, or both none. */ +export function sameFileLines(a: FileLineRange | undefined, b: FileLineRange | undefined): boolean { + if (a === undefined || b === undefined) return a === b; + return a.startLine === b.startLine && a.endLine === b.endLine; +} + +/** + * What the view hands the bundle for a whole file with no diff, as a File + * tab, a page's source, or a text file opened over the window draws one: + * its name, which its grammar is read from; its text, every line of it; + * the revision the text was read at, which names it in the pool's cache, so + * the same file at the same revision opened again takes its colours at + * once; how to draw it, the diff's own look less what only a diff draws; + * the lines lit, the ones a snippet pointed at or a note's under the + * pointer; and its notes, each drawn under its line in the element the view + * keeps for it. `onDrawn` is told each time the lines drawn change, as the + * file scrolls, so what the view paints over them, a find's matches, is + * painted again. + */ +export interface FileViewProps { + readonly name: string; + readonly text: string; + readonly revision: string; + readonly look: DiffLook; + readonly lit?: FileLineRange | undefined; + readonly annotations: readonly FileAnnotation[]; + readonly renderAnnotation: (key: string) => HTMLElement; + readonly onDrawn?: (() => void) | undefined; +} + +/** + * The file as mounted: drawn again for new props, scrolled to a run of its + * lines, asked which lines the reader picked, and taken down. + */ +export interface FileViewHandle { + readonly update: (props: FileViewProps) => void; + /** Scrolls a run of lines to the middle of the view at once, its first line where the run is taller than the view. */ + readonly revealLines: (range: FileLineRange) => void; + /** + * The lines the reader picked: those picked by pressing their numbers, as + * GitHub's file does, or else those code selected as text runs over; + * none where neither is. + */ + readonly pickedLines: () => FileLineRange | undefined; + readonly unmount: () => void; +} + /** How many unchanged lines one press of a gap's arrow opens, as GitHub's diff opens them. */ export const GAP_LINES = 20; @@ -303,6 +374,12 @@ export interface DiffViewModule { * drawn at all. */ readonly mountList: (host: HTMLElement, props: DiffListProps) => DiffListHandle; + /** + * Draws a whole file under `host`, which is its scroller, in a shadow root + * the tab's find reads, only the lines near the view drawn at all, and + * coloured off the window's thread where the pool's workers run. + */ + readonly mountFile: (host: HTMLElement, props: FileViewProps) => FileViewHandle; /** * Colours a run of a file's lines in the look's syntax themes, as a diff * is coloured, off the window's thread where the pool's workers run; none diff --git a/packages/panel/src/diff-view/index.ts b/packages/panel/src/diff-view/index.ts index f04f4c4b8f..5533381419 100644 --- a/packages/panel/src/diff-view/index.ts +++ b/packages/panel/src/diff-view/index.ts @@ -2,6 +2,7 @@ import { DIFF_LAYOUT } from "@luke/settings/diff-view"; import { CodeView, type CodeViewDiffItem, + type CodeViewItem, type CodeViewOptions, type DiffLineAnnotation, type FileContents, @@ -10,6 +11,7 @@ import { getFiletypeFromFileName, getSharedHighlighter, getSingularPatch, + type LineAnnotation, type File as PierreFile, FileDiff as PierreFileDiff, type PostRenderPhase, @@ -41,10 +43,16 @@ import { type DiffViewHandle, type DiffViewModule, type DiffViewProps, + type FileAnnotation, + type FileLineRange, + type FileViewHandle, + type FileViewProps, GAP_CONTROL, GAP_CONTROL_ATTRIBUTE, GAP_LINES, LINE_FLASH_HOLD_MS, + sameFileLines, + sameFileNotes, sameLines, sameListSelection, sameLook, @@ -140,6 +148,15 @@ import { wholeOf } from "./whole-diff.js"; * (`./annotation-bands.ts`), so a comment inside an added block reads as * standing in it. * + * A whole file with no diff, a File tab's, a page's source, or a text file + * opened over the window, is drawn through the same \`CodeView\` holding the + * one file (\`mountFile\`): only the lines near the view drawn at all, however + * long the file, coloured in the same pool, its lines lit as a thread's are, + * and each note under its line drawn in the element the view keeps for it, + * as a list's are. Pierre numbers a file from its first line, so the file + * is handed whole. One patch alone takes notes under its lines the same way + * (\`mount\`), as a Code card's snippet, an excerpt of a file, carries Luke's. + * * Where the view comments on lines, code selected as text is read once the * drag that began in a file lets go (`watchTextSelection`): the lines it * runs over in that file's shadow root (`./text-selection.ts`) and where its @@ -150,6 +167,9 @@ import { wholeOf } from "./whole-diff.js"; /** The longest line, in characters, whose changed words are marked; a longer one is tinted whole. */ const MAX_LINE_DIFF_LENGTH = 1000; +/** No notes. */ +const NO_NOTES: readonly DiffAnnotation[] = []; + /** * The text of the worker a list's files are highlighted in, set by the * build (`scripts/build.mjs`); a run that never went through the bundler @@ -535,10 +555,14 @@ function readPatch(patch: string): FileDiffMetadata { /** Pierre's options for one patch: a whole file's, or an excerpt with nothing said of the lines around it. */ function optionsOf(props: DiffViewProps): FileDiffOptions { + const { renderAnnotation } = props; return { ...sharedOptions(props.look), hunkSeparators: props.excerpt === true ? "simple" : "line-info-basic", disableFileHeader: true, + ...(renderAnnotation === undefined + ? {} + : { renderAnnotation: (note: { metadata: string }) => renderAnnotation(note.metadata) }), }; } @@ -562,7 +586,11 @@ function mount(host: HTMLElement, props: DiffViewProps): DiffViewHandle { const view = new PierreFileDiff(optionsOf(props)); let patch = props.patch; let fileDiff = getSingularPatch(patch); - view.render({ fileDiff, fileContainer: container }); + view.render({ + fileDiff, + fileContainer: container, + lineAnnotations: annotationsOf(props.annotations ?? NO_NOTES), + }); view.setSelectedLines(props.selected ?? null); return { update: (next) => { @@ -572,7 +600,12 @@ function mount(host: HTMLElement, props: DiffViewProps): DiffViewHandle { } typeset(container, next.look); view.setOptions(optionsOf(next)); - view.render({ fileDiff, fileContainer: container, forceRender: true }); + view.render({ + fileDiff, + fileContainer: container, + lineAnnotations: annotationsOf(next.annotations ?? NO_NOTES), + forceRender: true, + }); view.setSelectedLines(next.selected ?? null); }, unmount: () => { @@ -1070,6 +1103,229 @@ function mountList(host: HTMLElement, props: DiffListProps): DiffListHandle { }; } +/** The id a whole file's one item stands under in its view. */ +const WHOLE_FILE_ITEM = "file"; + +/** How much room stands above a whole file's first line and below its last, in pixels: the File tab's rows' own (`styles/tasks.css`'s `.code-lines`). */ +const WHOLE_FILE_EDGE = 6; + +/** How many files' keys `wholeFileKey` keeps, as many as the pool keeps results for and more, the oldest let go first. */ +const WHOLE_FILE_KEYS_KEPT = 64; + +/** The key each whole file's colours were last kept under in the pool's cache, by its name and then its revision, with the text it was handed. */ +const wholeFileKeys = new Map< + string, + Map +>(); + +/** How many whole files were read under a key of their own, which names each in the pool's cache. */ +let wholeFilesRead = 0; + +/** + * The key a whole file's colours are kept under in the pool's cache: the one + * it was last read under, where the same name at the same revision holds the + * same text, so a file opened again takes its colours at once; and one + * counted afresh for any other, so a text that changed never takes another's + * colours. Note that a count names it, not the name and revision, because a + * key is never built from identifiers (`AGENTS.md`'s TypeScript rules). + */ +function wholeFileKey(name: string, revision: string, text: string): string { + const revisions = wholeFileKeys.get(name) ?? new Map(); + const held = revisions.get(revision); + if (held?.text === text) return held.key; + wholeFilesRead += 1; + const key = `whole-${wholeFilesRead}`; + revisions.set(revision, { text, key }); + // Note that a name read again moves to the end, so the oldest is the one let go. + wholeFileKeys.delete(name); + wholeFileKeys.set(name, revisions); + const oldest = wholeFileKeys.keys().next(); + if (wholeFileKeys.size > WHOLE_FILE_KEYS_KEPT && !oldest.done) wholeFileKeys.delete(oldest.value); + return key; +} + +/** + * A file's text as Pierre draws it: without the newline that ends its last + * line, which Pierre would draw as an empty line of its own, where GitHub + * and an editor count the file as ending on the line before it. + */ +function withoutFinalNewline(text: string): string { + return text.endsWith("\n") ? text.slice(0, -1) : text; +} + +/** A whole file's notes as Pierre takes them, each key carried as the note's own. */ +function fileAnnotationsOf(notes: readonly FileAnnotation[]): LineAnnotation[] { + return notes.map((note) => ({ lineNumber: note.line, metadata: note.key })); +} + +/** A run of lines as Pierre lights it. */ +function litRange(range: FileLineRange): SelectedLineRange { + return { start: range.startLine, end: range.endLine }; +} + +/** A range Pierre picked, as a run of lines from its first to its last, whichever way it was dragged. */ +function fileLinesOf(range: SelectedLineRange): FileLineRange { + return { startLine: Math.min(range.start, range.end), endLine: Math.max(range.start, range.end) }; +} + +/** + * Leaves a drawn file's numbers out of what a screen reader reads: Pierre + * stands them in a column of their own beside the code, which would be read + * as every number and then every line, and they are left out of a copy and + * of the find already. + */ +function quietGutter(element: HTMLElement): void { + for (const gutter of element.shadowRoot?.querySelectorAll("[data-gutter]") ?? []) { + gutter.setAttribute("aria-hidden", "true"); + } +} + +/** + * Pierre's options for a whole file: the diff's theme, wrap, and numbers; + * no header, since the view's bar names the file; lines picked by pressing + * their numbers; and each note drawn in the element the view keeps for it, + * read off the latest props. + */ +function wholeFileOptions( + drawn: DiffLook, + latest: () => FileViewProps, + dress: (element: HTMLElement, phase: PostRenderPhase) => void, + pick: (range: SelectedLineRange | null) => void, +): CodeViewOptions { + return { + theme: drawn.themes, + themeType: "system", + preferredHighlighter: "shiki-js", + tokenizeMaxLineLength: MAX_LINE_DIFF_LENGTH, + disableLineNumbers: !drawn.lineNumbers, + overflow: drawn.wrap ? "wrap" : "scroll", + disableFileHeader: true, + enableLineSelection: true, + lineHoverHighlight: "number", + // Note that only the end of a press or a drag on the numbers is heard, since Pierre tells of the lines the view lights as picked too. + onLineSelectionEnd: pick, + itemMetrics: { lineHeight: lineHeightOf(drawn) }, + layout: { paddingTop: WHOLE_FILE_EDGE, paddingBottom: WHOLE_FILE_EDGE, gap: 0 }, + renderAnnotation: (note: { metadata: string }) => latest().renderAnnotation(note.metadata), + onPostRender: (node: HTMLElement, _instance: unknown, phase: PostRenderPhase) => + dress(node, phase), + }; +} + +/** + * Draws a whole file, as a File tab, a page's source, or a text file opened + * over the window shows one, through Pierre's own \`CodeView\` holding the one + * file: only the lines near the view drawn at all, however long the file, + * in a shadow root the find reads and the look's sheet dresses, and + * coloured in the kept pool's workers, plain until the colours land. The + * lines lit are Pierre's own lit lines, as a review thread's are; a press of + * the numbers picks lines in their place until the view lights others. + * Note that the item is handed anew, its version counted up, only when what + * it draws changes, because Pierre draws again whatever it is handed anew. + */ +function mountFile(host: HTMLElement, props: FileViewProps): FileViewHandle { + let latest = props; + const releases: Array<() => void> = []; + const dressed = new WeakSet(); + // The lines the reader picked by their numbers, none until they pick or once the view lights others. + let picked: FileLineRange | undefined; + // The file's element, the one the selection is read in, once Pierre has drawn it. + let drawnElement: HTMLElement | undefined; + const dress = (element: HTMLElement, phase: PostRenderPhase) => { + if (phase === "unmount") { + if (drawnElement === element) drawnElement = undefined; + return; + } + drawnElement = element; + quietGutter(element); + if (!dressed.has(element) && element.shadowRoot !== null) { + dressed.add(element); + element.setAttribute(FIND_SHADOW_ATTRIBUTE, FIND_SKIPPED); + releases.push(adoptShadowStyles(element.shadowRoot, lookSheets().look)); + } + latest.onDrawn?.(); + }; + const pick = (range: SelectedLineRange | null) => { + picked = range === null ? undefined : fileLinesOf(range); + }; + const optionsFor = (next: FileViewProps) => + wholeFileOptions(next.look, () => latest, dress, pick); + let version = 0; + const itemOf = (next: FileViewProps): CodeViewItem => { + version += 1; + return { + id: WHOLE_FILE_ITEM, + type: "file", + file: { + name: next.name, + contents: withoutFinalNewline(next.text), + cacheKey: wholeFileKey(next.name, next.revision, next.text), + }, + annotations: fileAnnotationsOf(next.annotations), + version, + }; + }; + const light = (lit: FileLineRange | undefined) => { + picked = undefined; + if (lit === undefined) view.clearSelectedLines({ notify: false }); + // Note that Pierre is told not to tell of the lines it is handed, which are no pick of the reader's. + else view.setSelectedLines({ id: WHOLE_FILE_ITEM, range: litRange(lit) }, { notify: false }); + }; + typeset(host, props.look); + const highlighting = listHighlighting(props.look); + const view = new CodeView(optionsFor(props), highlighting.pool); + view.setup(host); + view.setItems([itemOf(props)]); + view.render(true); + if (props.lit !== undefined) light(props.lit); + return { + update: (next) => { + const was = latest; + latest = next; + typeset(host, next.look); + if (!sameLook(next.look, was.look)) { + // A look the pool's workers do not hold yet is handed them first, since the pool's colours stand over the file's own. + void highlighting.follow(next.look); + view.setOptions(optionsFor(next)); + } + if ( + !sameLook(next.look, was.look) || + next.name !== was.name || + next.text !== was.text || + next.revision !== was.revision || + !sameFileNotes(next.annotations, was.annotations) + ) { + view.setItems([itemOf(next)]); + view.render(); + } + if (!sameFileLines(next.lit, was.lit)) light(next.lit); + }, + revealLines: (range) => + view.scrollTo({ + type: "range", + id: WHOLE_FILE_ITEM, + range: litRange(range), + align: "center", + behavior: "instant", + }), + pickedLines: () => { + if (picked !== undefined) return picked; + if (document.getSelection()?.isCollapsed !== false) return undefined; + const root = drawnElement?.shadowRoot; + const range = root === null || root === undefined ? undefined : rangeIn(root); + const selection = range === undefined ? undefined : selectedLines(range); + return selection === undefined + ? undefined + : fileLinesOf({ start: selection.start, end: selection.end }); + }, + unmount: () => { + view.cleanUp(); + for (const release of releases) release(); + host.replaceChildren(); + }, + }; +} + /** * A run of a file's lines coloured in the look's syntax themes, as a list's * files are: in the kept pool's workers once they hold the look, or on the @@ -1118,4 +1374,4 @@ let colouringsAsked = 0; /** The name Pierre gives a file whose name names no grammar. */ const PLAIN_TEXT = "text"; -export const diffView: DiffViewModule = { mount, mountList, colourLines }; +export const diffView: DiffViewModule = { mount, mountList, mountFile, colourLines }; diff --git a/packages/panel/src/diff-view/whole-file.test.ts b/packages/panel/src/diff-view/whole-file.test.ts new file mode 100644 index 0000000000..59ca58f7fe --- /dev/null +++ b/packages/panel/src/diff-view/whole-file.test.ts @@ -0,0 +1,203 @@ +// @vitest-environment jsdom + +import assert from "node:assert/strict"; +import { + DIFF_FONT, + DIFF_INDICATORS, + DIFF_LAYOUT, + DIFF_LINE_HEIGHT, + INLINE_DIFF, +} from "@luke/settings/diff-view"; +import { afterEach, test } from "vitest"; +import type { DiffLook, FileViewHandle, FileViewProps } from "./contract.js"; +import { diffView } from "./index.js"; + +/** + * whole-file.test.ts -- a whole file drawn by the diff bundle with no diff, + * as a File tab draws one (`mountFile`), and a patch drawn alone with notes + * under its lines (`mount`): Pierre draws them in jsdom on the window's own + * thread, since no worker text was built in, and the tests read what it drew + * in the shadow root and what the view's own elements hold. + */ + +const LOOK: DiffLook = { + layout: DIFF_LAYOUT.UNIFIED, + indicators: DIFF_INDICATORS.BARS, + inlineDiff: INLINE_DIFF.WORD_ALT, + backgrounds: true, + lineNumbers: true, + wrap: false, + themes: { dark: "github-dark-default", light: "github-light-default" }, + font: DIFF_FONT.SF_MONO, + fontSize: "11.5", + lineHeight: DIFF_LINE_HEIGHT.NORMAL, + ligatures: false, +}; + +const TEXT = "const a = 1;\nconst b = 2;\nconst c = 3;\nconst d = 4;\n"; + +/** The element each note was drawn in, by its key, as the view keeps them. */ +let kept = new Map(); +let mounted: Array<{ unmount: () => void }> = []; + +afterEach(() => { + for (const each of mounted) each.unmount(); + mounted = []; + kept = new Map(); + document.body.replaceChildren(); +}); + +/** The view's element for a note, made the first time it is asked for. */ +function noteElement(key: string): HTMLElement { + const held = kept.get(key); + if (held !== undefined) return held; + const element = document.createElement("div"); + element.textContent = `note ${key}`; + kept.set(key, element); + return element; +} + +function props(overrides: Partial = {}): FileViewProps { + return { + name: "src/seats.ts", + text: TEXT, + revision: "abc123", + look: LOOK, + annotations: [], + renderAnnotation: noteElement, + ...overrides, + }; +} + +/** A file mounted under a host of its own, and the host. */ +function mountFile(drawn: FileViewProps): { host: HTMLElement; handle: FileViewHandle } { + const host = document.createElement("div"); + document.body.append(host); + const handle = diffView.mountFile(host, drawn); + mounted.push(handle); + return { host, handle }; +} + +/** The shadow root Pierre drew the file in. */ +function drawnRoot(host: HTMLElement): ShadowRoot { + const root = host.querySelector("diffs-container")?.shadowRoot; + assert.ok(root, "the file is drawn in a shadow root"); + return root; +} + +/** Waits for Pierre's next frame, where it draws what it was handed. */ +function nextFrame(): Promise { + return new Promise((done) => requestAnimationFrame(() => done())); +} + +/** The numbers of the rows of code drawn, in order. */ +function drawnLines(root: ShadowRoot, selector = "[data-line]"): number[] { + return [...root.querySelectorAll(selector)].map((row) => Number(row.getAttribute("data-line"))); +} + +test("a whole file is drawn numbered from its first line, with no empty line after its final newline", async () => { + const { host } = mountFile(props()); + await nextFrame(); + const root = drawnRoot(host); + assert.deepEqual(drawnLines(root), [1, 2, 3, 4]); + assert.equal(root.querySelector('[data-line="2"]')?.textContent, "const b = 2;"); +}); + +test("the file's numbers are left out of the find and of what a screen reader reads", async () => { + const { host } = mountFile(props()); + await nextFrame(); + const container = host.querySelector("diffs-container"); + assert.equal(container?.getAttribute("data-find-shadow"), "[data-gutter]"); + const gutters = [...drawnRoot(host).querySelectorAll("[data-gutter]")]; + assert.ok(gutters.length > 0); + assert.ok(gutters.every((gutter) => gutter.getAttribute("aria-hidden") === "true")); +}); + +test("the lit lines are lit, others take their place as they change, and none stand once let go", async () => { + const { host, handle } = mountFile(props({ lit: { startLine: 2, endLine: 3 } })); + await nextFrame(); + const root = drawnRoot(host); + assert.deepEqual(drawnLines(root, "[data-line][data-selected-line]"), [2, 3]); + handle.update(props({ lit: { startLine: 4, endLine: 4 } })); + await nextFrame(); + assert.deepEqual(drawnLines(root, "[data-line][data-selected-line]"), [4]); + handle.update(props()); + await nextFrame(); + assert.deepEqual(drawnLines(root, "[data-line][data-selected-line]"), []); +}); + +test("each note is drawn in the view's own element under its line, and moves as the notes change", async () => { + const { host, handle } = mountFile(props({ annotations: [{ key: "n1", line: 2 }] })); + await nextFrame(); + const slotted = (key: string) => kept.get(key)?.parentElement?.getAttribute("slot"); + assert.equal(slotted("n1"), "annotation-2"); + assert.ok(drawnRoot(host).querySelector('slot[name="annotation-2"]')); + handle.update( + props({ + annotations: [ + { key: "n1", line: 2 }, + { key: "n2", line: 4 }, + ], + }), + ); + await nextFrame(); + assert.equal(slotted("n2"), "annotation-4"); + assert.ok(drawnRoot(host).querySelector('slot[name="annotation-4"]')); +}); + +test("a file whose text changes draws its new lines", async () => { + const { host, handle } = mountFile(props()); + await nextFrame(); + handle.update(props({ text: "let only = true;\n", revision: "def456" })); + await nextFrame(); + const root = drawnRoot(host); + assert.deepEqual(drawnLines(root), [1]); + assert.equal(root.querySelector('[data-line="1"]')?.textContent, "let only = true;"); +}); + +test("no lines are picked until the reader picks some", async () => { + const { handle } = mountFile(props({ lit: { startLine: 2, endLine: 2 } })); + await nextFrame(); + assert.equal(handle.pickedLines(), undefined); +}); + +test("lines picked by pressing their numbers are the reader's pick, until the view lights others", async () => { + const { host, handle } = mountFile(props({ lit: { startLine: 1, endLine: 1 } })); + await nextFrame(); + const press = (line: number, shiftKey = false) => { + const number = drawnRoot(host).querySelector(`[data-column-number="${line}"]`); + assert.ok(number); + const at = { + bubbles: true, + composed: true, + button: 0, + pointerId: 1, + pointerType: "mouse", + shiftKey, + }; + number.dispatchEvent(new PointerEvent("pointerdown", at)); + document.dispatchEvent(new PointerEvent("pointerup", at)); + }; + press(3); + press(2, true); + assert.deepEqual(handle.pickedLines(), { startLine: 2, endLine: 3 }); + handle.update(props({ lit: { startLine: 4, endLine: 4 } })); + assert.equal(handle.pickedLines(), undefined); +}); + +test("a patch drawn alone draws each note in the view's own element under its line", async () => { + const host = document.createElement("div"); + document.body.append(host); + const patch = + "--- a/seats.ts\n+++ b/seats.ts\n@@ -10,2 +10,2 @@\n const kept = 1;\n-const gone = 2;\n+const come = 2;\n"; + const handle = diffView.mount(host, { + patch, + look: LOOK, + excerpt: true, + annotations: [{ key: "t1", side: "additions", line: 11 }], + renderAnnotation: noteElement, + }); + mounted.push(handle); + await nextFrame(); + assert.equal(kept.get("t1")?.parentElement?.getAttribute("slot"), "annotation-additions-11"); +}); diff --git a/packages/panel/src/tasks/agent-changes-tab.test.ts b/packages/panel/src/tasks/agent-changes-tab.test.ts index 0f28968785..32f91e72f6 100644 --- a/packages/panel/src/tasks/agent-changes-tab.test.ts +++ b/packages/panel/src/tasks/agent-changes-tab.test.ts @@ -179,6 +179,12 @@ let root: Root | undefined; const standIn: DiffViewModule = { colourLines: () => Promise.resolve(undefined), mount: () => ({ update: () => undefined, unmount: () => undefined }), + mountFile: () => ({ + update: () => undefined, + revealLines: () => undefined, + pickedLines: () => undefined, + unmount: () => undefined, + }), mountList: (_host, props) => { drawn.push(props); return { diff --git a/packages/panel/src/tasks/agent-chapters-tab.test.ts b/packages/panel/src/tasks/agent-chapters-tab.test.ts index fae21b3b10..f17a9945fd 100644 --- a/packages/panel/src/tasks/agent-chapters-tab.test.ts +++ b/packages/panel/src/tasks/agent-chapters-tab.test.ts @@ -239,6 +239,12 @@ let marked: PullRequestActionRequest[] = []; const DIFF_STAND_IN: DiffViewModule = { colourLines: () => Promise.resolve(undefined), mount: () => ({ update: () => undefined, unmount: () => undefined }), + mountFile: () => ({ + update: () => undefined, + revealLines: () => undefined, + pickedLines: () => undefined, + unmount: () => undefined, + }), mountList: (host, props) => { lists.push({ host, props }); return { diff --git a/packages/panel/src/tasks/agent-pull-request-tab.test.ts b/packages/panel/src/tasks/agent-pull-request-tab.test.ts index b74237cfd4..b6f5f63cc9 100644 --- a/packages/panel/src/tasks/agent-pull-request-tab.test.ts +++ b/packages/panel/src/tasks/agent-pull-request-tab.test.ts @@ -221,6 +221,12 @@ const DIFF_STAND_IN: DiffViewModule = { previews.push(props); return { update: () => undefined, unmount: () => undefined }; }, + mountFile: () => ({ + update: () => undefined, + revealLines: () => undefined, + pickedLines: () => undefined, + unmount: () => undefined, + }), mountList: (_host, props) => { lists.push(props); return { diff --git a/packages/panel/src/tasks/themed-lines.test.ts b/packages/panel/src/tasks/themed-lines.test.ts index 3eeb15c55d..2d75c822e9 100644 --- a/packages/panel/src/tasks/themed-lines.test.ts +++ b/packages/panel/src/tasks/themed-lines.test.ts @@ -55,6 +55,12 @@ let answer = PINK; const STAND_IN: DiffViewModule = { mount: () => ({ update: () => undefined, unmount: () => undefined }), + mountFile: () => ({ + update: () => undefined, + revealLines: () => undefined, + pickedLines: () => undefined, + unmount: () => undefined, + }), mountList: () => ({ update: () => 0, reveal: () => 0, From d3d08a2f1b6d7c71dd4bbe44a2bcd008a3a5460c Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Tue, 20 Oct 2026 22:28:45 +0000 Subject: [PATCH 2/2] fix(panel): keep a whole file's keys bounded across revisions, and let a pick go with its file Co-authored-by: luke-agent[bot] <339870089+luke-agent[bot]@users.noreply.github.com> --- packages/panel/src/diff-view/index.ts | 56 +++++++++++-------- .../panel/src/diff-view/whole-file.test.ts | 13 +++++ 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/packages/panel/src/diff-view/index.ts b/packages/panel/src/diff-view/index.ts index 5533381419..a8265eda3d 100644 --- a/packages/panel/src/diff-view/index.ts +++ b/packages/panel/src/diff-view/index.ts @@ -1109,14 +1109,24 @@ const WHOLE_FILE_ITEM = "file"; /** How much room stands above a whole file's first line and below its last, in pixels: the File tab's rows' own (`styles/tasks.css`'s `.code-lines`). */ const WHOLE_FILE_EDGE = 6; -/** How many files' keys `wholeFileKey` keeps, as many as the pool keeps results for and more, the oldest let go first. */ -const WHOLE_FILE_KEYS_KEPT = 64; +/** + * How many files' keys `wholeFileKey` keeps, each a name at a revision with + * the text it was handed: as many as the pool keeps results for + * (\`HIGHLIGHT_CACHE_ENTRIES\`), since a key past those names colours the pool + * no longer holds, the oldest let go first. + */ +const WHOLE_FILE_KEYS_KEPT = 16; + +/** A whole file's key, kept with the text it was handed under its name at its revision. */ +interface WholeFileKey { + readonly name: string; + readonly revision: string; + readonly text: string; + readonly key: string; +} -/** The key each whole file's colours were last kept under in the pool's cache, by its name and then its revision, with the text it was handed. */ -const wholeFileKeys = new Map< - string, - Map ->(); +/** The keys kept, oldest first, each a file's name at a revision. */ +let wholeFileKeys: readonly WholeFileKey[] = []; /** How many whole files were read under a key of their own, which names each in the pool's cache. */ let wholeFilesRead = 0; @@ -1127,20 +1137,20 @@ let wholeFilesRead = 0; * same text, so a file opened again takes its colours at once; and one * counted afresh for any other, so a text that changed never takes another's * colours. Note that a count names it, not the name and revision, because a - * key is never built from identifiers (`AGENTS.md`'s TypeScript rules). + * key is never built from identifiers (\`AGENTS.md\`'s TypeScript rules); and + * that only the newest \`WHOLE_FILE_KEYS_KEPT\` are kept, whatever their names, + * so no file's texts pile up across its revisions. */ function wholeFileKey(name: string, revision: string, text: string): string { - const revisions = wholeFileKeys.get(name) ?? new Map(); - const held = revisions.get(revision); - if (held?.text === text) return held.key; - wholeFilesRead += 1; - const key = `whole-${wholeFilesRead}`; - revisions.set(revision, { text, key }); - // Note that a name read again moves to the end, so the oldest is the one let go. - wholeFileKeys.delete(name); - wholeFileKeys.set(name, revisions); - const oldest = wholeFileKeys.keys().next(); - if (wholeFileKeys.size > WHOLE_FILE_KEYS_KEPT && !oldest.done) wholeFileKeys.delete(oldest.value); + const same = (each: WholeFileKey) => each.name === name && each.revision === revision; + const held = wholeFileKeys.find(same); + if (held?.text !== text) wholeFilesRead += 1; + const key = held?.text === text ? held.key : `whole-${wholeFilesRead}`; + // Note that the key read moves to the end, so the oldest is the one let go. + wholeFileKeys = [ + ...wholeFileKeys.filter((each) => !same(each)), + { name, revision, text, key }, + ].slice(-WHOLE_FILE_KEYS_KEPT); return key; } @@ -1288,11 +1298,13 @@ function mountFile(host: HTMLElement, props: FileViewProps): FileViewHandle { void highlighting.follow(next.look); view.setOptions(optionsFor(next)); } + const another = + next.name !== was.name || next.text !== was.text || next.revision !== was.revision; + // Note that lines picked in a file are no pick in another. + if (another) picked = undefined; if ( + another || !sameLook(next.look, was.look) || - next.name !== was.name || - next.text !== was.text || - next.revision !== was.revision || !sameFileNotes(next.annotations, was.annotations) ) { view.setItems([itemOf(next)]); diff --git a/packages/panel/src/diff-view/whole-file.test.ts b/packages/panel/src/diff-view/whole-file.test.ts index 59ca58f7fe..d72ca597de 100644 --- a/packages/panel/src/diff-view/whole-file.test.ts +++ b/packages/panel/src/diff-view/whole-file.test.ts @@ -185,6 +185,19 @@ test("lines picked by pressing their numbers are the reader's pick, until the vi assert.equal(handle.pickedLines(), undefined); }); +test("lines picked in one file are no pick once another file takes its place", async () => { + const { host, handle } = mountFile(props()); + await nextFrame(); + const number = drawnRoot(host).querySelector('[data-column-number="2"]'); + assert.ok(number); + const at = { bubbles: true, composed: true, button: 0, pointerId: 1, pointerType: "mouse" }; + number.dispatchEvent(new PointerEvent("pointerdown", at)); + document.dispatchEvent(new PointerEvent("pointerup", at)); + assert.deepEqual(handle.pickedLines(), { startLine: 2, endLine: 2 }); + handle.update(props({ name: "src/other.ts", text: "let other = 1;\nlet more = 2;\n" })); + assert.equal(handle.pickedLines(), undefined); +}); + test("a patch drawn alone draws each note in the view's own element under its line", async () => { const host = document.createElement("div"); document.body.append(host);