From 00da324c3d4fa85f680e825a3cb064526a9556c8 Mon Sep 17 00:00:00 2001 From: Tobias Sargeant Date: Fri, 4 Sep 2026 13:49:30 +1000 Subject: [PATCH 1/5] fix(viewer): a span's smell pills lead its text block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pills sat at the end of the rationale, so a long intent put them rows below the bar they describe — in gcp.py's +779..+783 the backward-incompatible pill landed at row 786, beside the +787 span's dot, and read as that span's. Smells describe the span; they go on the block's first line, beside the bar's start, with the intent below. --- semantic_code_review/viewer/assets/render.ts | 15 ++++++++++----- semantic_code_review/viewer/assets/viewer.css | 5 ++++- tests/js/viewer.test.ts | 5 +++++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/semantic_code_review/viewer/assets/render.ts b/semantic_code_review/viewer/assets/render.ts index 865ec37..5b37db1 100644 --- a/semantic_code_review/viewer/assets/render.ts +++ b/semantic_code_review/viewer/assets/render.ts @@ -1336,15 +1336,20 @@ function _spanMark(spanId: string, depth: number, kind: "bar" | "bar-top" | "bar return mark; } -/** One span's text: its intent, wrapping at the gutter's width, then its - * smells as promotable pills anchored at the span's first line. */ +/** One span's text: its smells as promotable pills on the block's first + * line — beside the bar's start, since they describe the span — then its + * intent, wrapping at the gutter's width. */ function _spanText(span: AnnotationSpan, filePath: string): HTMLElement { const el = _el("p", "span-text-body"); el.dataset.spanId = span.id; + if (span.smells && span.smells.length) { + const pills = _el("span", "span-text-smells"); + for (const sm of span.smells) pills.appendChild(_smellPill(sm, { + smellId: `${span.id}:smell:${sm.tag}`, file: filePath, side: "new", line: span.start, + })); + el.appendChild(pills); + } el.appendChild(_el("span", span.intent ? "span-text-intent" : "span-text-intent empty", span.intent || "(no intent)")); - for (const sm of span.smells || []) el.appendChild(_smellPill(sm, { - smellId: `${span.id}:smell:${sm.tag}`, file: filePath, side: "new", line: span.start, - })); return el; } diff --git a/semantic_code_review/viewer/assets/viewer.css b/semantic_code_review/viewer/assets/viewer.css index 1bcbb19..c3b5566 100644 --- a/semantic_code_review/viewer/assets/viewer.css +++ b/semantic_code_review/viewer/assets/viewer.css @@ -851,7 +851,10 @@ html.dragging-divider { cursor: col-resize; user-select: none; } } .diff .span-text-body.chained { padding-top: 4px; border-top: 1px dashed var(--border); } .diff .span-text-intent.empty { color: var(--fg-dim); font-style: italic; } -.diff .span-text .smell { font-size: 10px; padding: 0 6px; margin-left: 3px; vertical-align: middle; } +/* The pills lead the block, on its first line, so they sit beside the + * row the span starts on rather than wherever its intent ends. */ +.diff .span-text-smells { display: flex; flex-wrap: wrap; gap: 3px; margin-bottom: 2px; } +.diff .span-text .smell { font-size: 10px; padding: 0 6px; } /* Each `.row` is a real grid box spanning both of `.half`'s columns, * and uses `subgrid` so its two cells land in `.half`'s column tracks diff --git a/tests/js/viewer.test.ts b/tests/js/viewer.test.ts index 1ee54f1..18f8822 100644 --- a/tests/js/viewer.test.ts +++ b/tests/js/viewer.test.ts @@ -1247,6 +1247,11 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { expect(gridRow(inner)).toBe("4"); expect(inner.querySelector(".span-text-intent")!.textContent).toBe("the inner edit"); expect(inner.querySelector(".smell")!.textContent).toBe("dead-code"); + // The pills lead the block — beside the bar's first row, not wherever + // a long intent happens to end — and a span with none has no pill row. + const body = inner.querySelector(".span-text-body")!; + expect(Array.from(body.children).map((c) => c.className)).toEqual(["span-text-smells", "span-text-intent"]); + expect(textOf("H0_0:span:4-7")!.querySelector(".span-text-smells")).toBeNull(); // Single-line spans: a dot on their row at their depth, and the note // beneath the row as before. No text block. From 6ab489098c9ce1b252f0b9daca7935569c5dc554 Mon Sep 17 00:00:00 2001 From: Tobias Sargeant Date: Fri, 4 Sep 2026 13:53:17 +1000 Subject: [PATCH 2/5] fix(viewer): a span's bar runs through the annotation rows inside it A comment thread or fold label row inside a span's rows is inserted after the bar is drawn on the code rows, and is built without gutter cells, so the bar broke around it (google_group_settings.py's +19..+72 with the two-comment thread at 20). The placement pass already walks every row of the half; it now gives each non-code row lying inside a bar a bars cell with a segment per enclosing span at that span's depth, and the strip's background pseudo-item covers an old-side thread's placeholder too. annotations.ts's own layout is untouched. --- semantic_code_review/viewer/assets/render.ts | 51 +++++++++++++++++-- semantic_code_review/viewer/assets/viewer.css | 12 +++-- tests/js/viewer.test.ts | 35 ++++++++++++- 3 files changed, 90 insertions(+), 8 deletions(-) diff --git a/semantic_code_review/viewer/assets/render.ts b/semantic_code_review/viewer/assets/render.ts index 5b37db1..9ca1dae 100644 --- a/semantic_code_review/viewer/assets/render.ts +++ b/semantic_code_review/viewer/assets/render.ts @@ -1221,10 +1221,23 @@ interface SpanTextBlock { row: HTMLElement; } -/** A right half's gutter state: its text blocks, the code rows the last - * pass stretched, and the observer whose records the pass discards. */ +/** A multi-line span's bar: the code rows it runs from and to, and its + * column. The rows between them that are not code — a comment thread's + * row, a fold's label row — arrive after the bar is drawn, so the + * placement pass gives them their segment. */ +interface SpanBar { + spanId: string; + first: HTMLElement; + last: HTMLElement; + depth: number; +} + +/** A right half's gutter state: its text blocks and bars, the code rows + * the last pass stretched, and the observer whose records the pass + * discards. */ interface SpanGutter { blocks: SpanTextBlock[]; + bars: SpanBar[]; stretched: HTMLElement[]; observer: MutationObserver | null; } @@ -1256,6 +1269,7 @@ function _attachSpans( const half = rowElsNew[0].parentElement; if (!half) throw new Error(`${hunkId}: rows are not in a half`); const blocks: SpanTextBlock[] = []; + const bars: SpanBar[] = []; for (const ps of placed) { if (ps.span.start === ps.span.end) { _gutterBars(rowElsNew[ps.first]).appendChild(_spanMark(ps.span.id, ps.depth, "dot")); @@ -1266,6 +1280,7 @@ function _attachSpans( const pos = i === ps.first ? "bar-top" : i === ps.last ? "bar-bottom" : "bar"; _gutterBars(rowElsNew[i]).appendChild(_spanMark(ps.span.id, ps.depth, pos)); } + bars.push({ spanId: ps.span.id, first: rowElsNew[ps.first], last: rowElsNew[ps.last], depth: ps.depth }); const el = _el("div", "span-text"); el.dataset.spanId = ps.span.id; const body = _spanText(ps.span, filePath); @@ -1277,7 +1292,7 @@ function _attachSpans( blocks.push({ el, body, row: rowElsNew[ps.first] }); } if (!blocks.length) return; - const gutter: SpanGutter = { blocks, stretched: [], observer: null }; + const gutter: SpanGutter = { blocks, bars, stretched: [], observer: null }; _SPAN_GUTTERS.set(half, gutter); _observeHalf(half, gutter); _layoutSpanTexts(half); @@ -1404,15 +1419,18 @@ function _layoutSpanTexts(half: HTMLElement): void { gutter.stretched = []; const track = new Map(); const visible: HTMLElement[] = []; + const rows: HTMLElement[] = []; let n = 0; for (const child of Array.from(half.children) as HTMLElement[]) { if (!child.classList.contains("row")) continue; + rows.push(child); if (child.style.display === "none") { child.style.gridRow = ""; continue; } n++; child.style.gridRow = String(n); track.set(child, n); visible.push(child); } + _barNonCodeRows(gutter.bars, rows); const shown: SpanTextBlock[] = []; for (const block of gutter.blocks) { const start = track.get(block.row); @@ -1480,6 +1498,33 @@ function _codeRowAbove(visible: HTMLElement[], from: number): HTMLElement { throw new Error("no code row above a colliding span text"); } +/** Give every non-code row lying inside a bar — a comment thread's row, + * a fold's label row, the placeholder for an old-side thread — a bars + * cell with a segment per enclosing bar at its depth, so a bar runs + * unbroken through them. `rows` is the half's rows in DOM order, hidden + * ones included (a bar may open on a hidden row); a hidden row is left + * alone. Idempotent: a cell already carrying the right segments stays. */ +function _barNonCodeRows(bars: SpanBar[], rows: HTMLElement[]): void { + if (!bars.length) return; + const open = new Set(); + for (const row of rows) { + for (const bar of bars) if (bar.first === row) open.add(bar); + const code = !row.classList.contains("row-annotation") && !row.classList.contains("row-placeholder"); + if (!code && row.style.display !== "none") { + const key = Array.from(open, (b) => b.spanId).join("\n"); + let cell = row.querySelector(":scope > .cell-gutter-bars"); + if (cell && cell.dataset.spans !== key) { cell.remove(); cell = null; } + if (!cell && open.size) { + cell = _el("span", "cell-gutter-bars"); + cell.dataset.spans = key; + for (const bar of open) cell.appendChild(_spanMark(bar.spanId, bar.depth, "bar")); + row.appendChild(cell); + } + } + for (const bar of bars) if (bar.last === row) open.delete(bar); + } +} + /** The old-half row paired with a new-half code row. */ function _pairOf(row: HTMLElement): HTMLElement { const pair = (row as { _scrPair?: HTMLElement })._scrPair; diff --git a/semantic_code_review/viewer/assets/viewer.css b/semantic_code_review/viewer/assets/viewer.css index c3b5566..79dbb5c 100644 --- a/semantic_code_review/viewer/assets/viewer.css +++ b/semantic_code_review/viewer/assets/viewer.css @@ -837,18 +837,24 @@ html.dragging-divider { cursor: col-resize; user-select: none; } line-height: 1.35; color: var(--fg-muted); } -/* An annotation row has no gutter cells; a sticky pseudo-item keeps the - * strip's background continuous through it. */ -.diff .half-new .row-annotation::before { +/* An annotation row (or the placeholder for an old-side one) is built + * without gutter cells; a sticky pseudo-item keeps the strip's background + * continuous through it, and the placement pass adds a bars cell to one + * that lies inside a span so the span's bar runs through it too. The + * placeholder is `visibility: hidden`; its bars cell shows regardless. */ +.diff .half-new .row-annotation::before, +.diff .half-new .row-placeholder::before { content: ""; grid-column: 1 / 4; grid-row: 1; position: sticky; left: 0; z-index: 1; + visibility: visible; background: var(--bg-alt); box-shadow: inset -1px 0 var(--border); } +.diff .half-new .row-placeholder > .cell-gutter-bars { visibility: visible; } .diff .span-text-body.chained { padding-top: 4px; border-top: 1px dashed var(--border); } .diff .span-text-intent.empty { color: var(--fg-dim); font-style: italic; } /* The pills lead the block, on its first line, so they sit beside the diff --git a/tests/js/viewer.test.ts b/tests/js/viewer.test.ts index 18f8822..ddbce54 100644 --- a/tests/js/viewer.test.ts +++ b/tests/js/viewer.test.ts @@ -982,8 +982,12 @@ describe("streaming events", () => { fold("code"); expect(document.querySelectorAll(".fold-chev").length).toBe(0); expect(document.querySelector(".label-tree")).toBeNull(); - expect(document.querySelectorAll('.span-mark[data-span-id="H0_0:span:1-4"]').length).toBe(4); - expect(document.querySelectorAll('.span-mark[data-span-id="H0_0:span:2-3"]').length).toBe(2); + // Bars over exactly their code rows (the callout's note row at 3 gets + // its own segment of the bars running through it). + const codeMarks = (id: string): number => + document.querySelectorAll(`.row:not(.row-annotation) > .cell-gutter-bars > .span-mark[data-span-id="${id}"]`).length; + expect(codeMarks("H0_0:span:1-4")).toBe(4); + expect(codeMarks("H0_0:span:2-3")).toBe(2); expect(document.querySelector('.span-text[data-span-id="H0_0:span:1-4"] .span-text-intent')!.textContent).toBe("region"); expect(document.querySelectorAll(".row-annotation.annot-note").length).toBe(2); }); @@ -1379,6 +1383,33 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { // continuous down the file. expect(document.querySelector(".gap-expansion .half-new .row .cell-gutter-text")).not.toBeNull(); }); + + test("a bar runs through the comment rows inside its span, at the span's depth", async () => { + // Threads on 5 (inside the outer span 4..7), on 6 (inside it and the + // inner span 6..7) and on 7 (the outer span's last row: outside). + const comment = (id: string, side: "old" | "new", line: number): Record => + ({ id, file: "a.py", side, line, body: id, created_at: 1, updated_at: 1, source: "local", derived_from: null }); + await bootViewer( + makeData({ pending: false, files: [gutterFile(NESTED)], symbols: [] }), + { comments: [comment("c5", "new", 5), comment("c6", "new", 6), comment("c7", "new", 7)] }, + ); + fold("code"); + await tick(); + const segments = (row: HTMLElement): Array<[string, string]> => + Array.from(row.querySelectorAll(":scope > .cell-gutter-bars > .span-mark")) + .map((m) => [m.dataset.spanId!, m.style.getPropertyValue("--depth")]); + const threadAfter = (line: number): HTMLElement => { + const next = rowOfLine(line).nextElementSibling as HTMLElement; + expect(next.classList.contains("annot-comment")).toBe(true); + return next; + }; + expect(segments(threadAfter(5))).toEqual([["H0_0:span:4-7", "0"]]); + expect(segments(threadAfter(6))).toEqual([["H0_0:span:4-7", "0"], ["H0_0:span:6-7", "1"]]); + expect(segments(threadAfter(7))).toEqual([]); + expect(threadAfter(7).querySelector(":scope > .cell-gutter-bars")).toBeNull(); + // The code rows' own marks are untouched by the pass. + expect(marks("H0_0:span:4-7")).toEqual([[4, "span-bar-top"], [5, "span-bar"], [6, "span-bar"], [7, "span-bar-bottom"]]); + }); }); From 50413dccb0a8d1b7e4c460c087c45ec24e00e026 Mon Sep 17 00:00:00 2001 From: Tobias Sargeant Date: Fri, 4 Sep 2026 14:11:28 +1000 Subject: [PATCH 3/5] fix(viewer): a single-line span takes the gutter form like every other span MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A span of one line kept the legacy line-note form — a dot on its row and a full-width note box in the code column, with an arrow from the dot — while multi-line spans moved to the centre gutter. config.py's +81 (686 characters, nested in +76..+81) rendered as a code-side box pushing every line below it down, beside a parent that lived in the gutter. Every span now takes one form: its mark in the bars column (a bar, or a dot for one line) and its text block in the text column at its first row, joining the placement pass — the +81 text collides with its parent's and stretches row 80, and both read in the strip. Its smell pills lead the block. The code-side note, its arrow, and the note's "Add as comment" button are retired with it; a comment already derived from a span (by its id, or the line_note id a store written before spans used) still hides it, and comments.ts's post-promotion sweep addresses the gutter block and marks instead of the note row. Reviewer comment threads are unchanged: they keep their code-side boxes and arrows, and annotations.ts is untouched. Every extra-review note is a single-line span, so this moves all of them into the gutter. --- .../viewer/assets/comments.ts | 15 +- semantic_code_review/viewer/assets/render.ts | 131 +++++-------- semantic_code_review/viewer/assets/viewer.css | 27 +-- tests/js/viewer.test.ts | 175 +++++++++--------- 4 files changed, 147 insertions(+), 201 deletions(-) diff --git a/semantic_code_review/viewer/assets/comments.ts b/semantic_code_review/viewer/assets/comments.ts index be4fe6e..43d93a0 100644 --- a/semantic_code_review/viewer/assets/comments.ts +++ b/semantic_code_review/viewer/assets/comments.ts @@ -619,14 +619,15 @@ function _refreshForAnchor(anchorRowEl: HTMLElement, anchor: Anchor): void { * matches `derivedId`. Used right after a promotion save so the * source observation visibly transitions into the comment. * - * Two shapes today: single-line spans attach as `.row-annotation` rows with - * `data-span-id`; smells render as inline `.smell` pills with - * `data-smell-id`. Annotation rows need Annotations.detach (resize - * observers etc.); the pills are plain elements. */ + * Two shapes today: a span is its text block (`.span-text`) and marks + * (`.span-mark`) in the centre gutter, both carrying `data-span-id`; + * smells render as inline `.smell` pills with `data-smell-id`. All are + * plain elements; the gutter's placement pass notices a removed block + * on its next run. */ function _removeAnnotationByDerivedId(derivedId: string): void { document.querySelectorAll( - `.row-annotation[data-span-id="${derivedId}"]`, - ).forEach((el) => Annotations.detach(el)); + `.span-text[data-span-id="${derivedId}"], .span-mark[data-span-id="${derivedId}"]`, + ).forEach((el) => el.remove()); document.querySelectorAll( `.smell[data-smell-id="${derivedId}"]`, ).forEach((el) => el.remove()); @@ -635,7 +636,7 @@ function _removeAnnotationByDerivedId(derivedId: string): void { /** Remove the comment rows already rendered for `anchor`. * * Keyed rather than positional. Every annotation inserts itself - * directly after its anchor row, so an LLM line-note — or the + * directly after its anchor row, so a fold's label row — or the * `.row-placeholder` a side-by-side shadow anchor adds — can sit * between the anchor and its comment rows. A sibling walk that * stopped at the first non-comment row then removed nothing, and diff --git a/semantic_code_review/viewer/assets/render.ts b/semantic_code_review/viewer/assets/render.ts index 9ca1dae..2ae6aaa 100644 --- a/semantic_code_review/viewer/assets/render.ts +++ b/semantic_code_review/viewer/assets/render.ts @@ -1140,9 +1140,9 @@ function _renderLabelNode(node: LabelNode, f: FileBlock, rows: RowBlock[], pick: /** The labels a collapsed region shows (the `FoldLabels` folds.ts asks * for): the tree over the rows the fold hid. Clicking one opens the fold - * and lands on what it names — a span's text or note, a definition's - * first row — found within this pane's copy of the file, - * so the explainer panel never scrolls the diff pane. */ + * and lands on what it names — a span's text, a definition's first row + * — found within this pane's copy of the file, so the explainer panel + * never scrolls the diff pane. */ function _foldLabels(f: FileBlock): FoldLabels { return (headerRow, bodyRows, open) => { const scope: LabelScope = { visibleRow: headerRow }; @@ -1151,7 +1151,7 @@ function _foldLabels(f: FileBlock): FoldLabels { open(); const pane = tree?.closest(".file") ?? document; const target = node.kind === "span" - ? pane.querySelector(`.span-text[data-span-id="${_cssEscape(node.span.id)}"], .row-annotation[data-span-id="${_cssEscape(node.span.id)}"]`) + ? pane.querySelector(`.span-text[data-span-id="${_cssEscape(node.span.id)}"]`) : (bodyRows[node.first].newEl.children[1].classList.contains("empty") ? bodyRows[node.first].oldEl : bodyRows[node.first].newEl); if (target) target.scrollIntoView({ block: "nearest" }); @@ -1174,7 +1174,7 @@ function _renderHunkDiff(h: HunkBlock, file: FileBlock, scope: PaneScope): HTMLE const rows = h.rows || []; const marks = _blockMarks(rows); const { diff, oldEls, newEls } = _renderDiffRows(file, rows, marks); - _attachSpans(oldEls, newEls, rows, h.spans || [], h.id, file.path); + _attachSpans(newEls, rows, h.spans || [], h.id, file.path); // Record this hunk's rows so folds.ts can build a unified row stream // across the hunk and adjacent expanded context. FileRows.record(diff, { rows, oldEls, newEls }); @@ -1188,9 +1188,10 @@ function _renderHunkDiff(h: HunkBlock, file: FileBlock, scope: PaneScope): HTMLE // the first three sticky so they read as a fixed centre gutter between the // halves while the code scrolls beneath. Every row carries an empty text // and bars cell for the gutter's background; a span puts its marks in -// them. A multi-line span is a bar over its rows, one column further right -// per level of nesting, and its intent as a text block beside them; a -// single-line span is a dot on its row and the note beneath it. +// them. Every span takes one form: a mark in the bars column — a bar over +// its rows, one column further right per level of nesting, or a dot on a +// span of one line — and its text block in the text column, starting on +// its first row. Nothing of a span lives in the code column. // // A text block is a zero-height grid item on its span's first row, so it // takes no part in the grid's row sizing: its body hangs down the strip @@ -1213,8 +1214,8 @@ interface PlacedSpan { depth: number; } -/** A multi-line span's text block: the grid item, the body whose height - * is the block's, and the span's first row. */ +/** A span's text block: the grid item, the body whose height is the + * block's, and the span's first row. */ interface SpanTextBlock { el: HTMLElement; body: HTMLElement; @@ -1226,7 +1227,7 @@ interface SpanTextBlock { * row, a fold's label row — arrive after the bar is drawn, so the * placement pass gives them their segment. */ interface SpanBar { - spanId: string; + span: AnnotationSpan; first: HTMLElement; last: HTMLElement; depth: number; @@ -1254,13 +1255,16 @@ const _SPAN_TEXT_WIDTH = "26ch"; const _SPAN_BAR_COLUMN_PX = 6; /** Spans on visible code — the one owner of the form a span takes when - * its rows are on screen. A span whose rows are not in the hunk is - * warned about and left out. Everything hangs off the rows themselves, - * so it survives the hunk's `.diff` being reused across repaints and - * rows arriving above or below it from a chip. */ + * its rows are on screen: its marks in the bars column (a bar over its + * rows, or a dot for a span of one line) and its text block in the text + * column, whatever its length. A span whose rows are not in the hunk is + * warned about and left out; one the reviewer has turned into a comment + * is left out too — the comment stands in its place. Everything hangs + * off the rows themselves, so it survives the hunk's `.diff` being + * reused across repaints and rows arriving above or below it from a + * chip. */ function _attachSpans( - rowElsOld: HTMLElement[], rowElsNew: HTMLElement[], - rows: RowBlock[], spans: AnnotationSpan[], + rowElsNew: HTMLElement[], rows: RowBlock[], spans: AnnotationSpan[], hunkId: string, filePath: string, ): void { if (!spans.length || !rows.length) return; @@ -1271,16 +1275,16 @@ function _attachSpans( const blocks: SpanTextBlock[] = []; const bars: SpanBar[] = []; for (const ps of placed) { - if (ps.span.start === ps.span.end) { - _gutterBars(rowElsNew[ps.first]).appendChild(_spanMark(ps.span.id, ps.depth, "dot")); - _attachSpanNote(ps.span, ps.first, rowElsOld, rowElsNew, hunkId, filePath); - continue; - } - for (let i = ps.first; i <= ps.last; i++) { - const pos = i === ps.first ? "bar-top" : i === ps.last ? "bar-bottom" : "bar"; - _gutterBars(rowElsNew[i]).appendChild(_spanMark(ps.span.id, ps.depth, pos)); + if (_spanPromoted(ps.span, hunkId)) continue; + if (ps.first === ps.last) { + _gutterBars(rowElsNew[ps.first]).appendChild(_spanMark(ps.span, ps.depth, "dot")); + } else { + for (let i = ps.first; i <= ps.last; i++) { + const pos = i === ps.first ? "bar-top" : i === ps.last ? "bar-bottom" : "bar"; + _gutterBars(rowElsNew[i]).appendChild(_spanMark(ps.span, ps.depth, pos)); + } + bars.push({ span: ps.span, first: rowElsNew[ps.first], last: rowElsNew[ps.last], depth: ps.depth }); } - bars.push({ spanId: ps.span.id, first: rowElsNew[ps.first], last: rowElsNew[ps.last], depth: ps.depth }); const el = _el("div", "span-text"); el.dataset.spanId = ps.span.id; const body = _spanText(ps.span, filePath); @@ -1343,14 +1347,23 @@ function _gutterBars(rowEl: HTMLElement): HTMLElement { } /** A bar segment or dot in a row's bars cell, at its depth's column. */ -function _spanMark(spanId: string, depth: number, kind: "bar" | "bar-top" | "bar-bottom" | "dot"): HTMLElement { +function _spanMark(span: AnnotationSpan, depth: number, kind: "bar" | "bar-top" | "bar-bottom" | "dot"): HTMLElement { const mark = _el("span", `span-mark span-${kind}`); - mark.dataset.spanId = spanId; + mark.dataset.spanId = span.id; mark.style.setProperty("--depth", String(depth)); mark.setAttribute("aria-hidden", "true"); return mark; } +/** Whether the reviewer has already turned this span into a comment: a + * local comment derived from its id, or — for a span of one line — from + * the `line_note` id a comment store written before spans recorded the + * same observation under. */ +function _spanPromoted(span: AnnotationSpan, hunkId: string): boolean { + if (Comments.isPromoted(span.id)) return true; + return span.start === span.end && Comments.isPromoted(`${hunkId}:line_note:${span.start}`); +} + /** One span's text: its smells as promotable pills on the block's first * line — beside the bar's start, since they describe the span — then its * intent, wrapping at the gutter's width. */ @@ -1410,6 +1423,11 @@ function _observeHalf(half: HTMLElement, gutter: SpanGutter): void { function _layoutSpanTexts(half: HTMLElement): void { const gutter = _SPAN_GUTTERS.get(half); if (!gutter) return; + // A span promoted to a comment after the attach has had its block + // removed from the half (comments.ts's sweep); it places nothing from + // then on. Not `isConnected`: the first pass runs before the hunk's + // `.diff` is in the document. + gutter.blocks = gutter.blocks.filter((block) => block.el.parentElement === half); // Release the last pass's stretches: the rows measure at their natural // heights and the pass recomputes every one. for (const row of gutter.stretched) { @@ -1511,13 +1529,13 @@ function _barNonCodeRows(bars: SpanBar[], rows: HTMLElement[]): void { for (const bar of bars) if (bar.first === row) open.add(bar); const code = !row.classList.contains("row-annotation") && !row.classList.contains("row-placeholder"); if (!code && row.style.display !== "none") { - const key = Array.from(open, (b) => b.spanId).join("\n"); + const key = Array.from(open, (b) => b.span.id).join("\n"); let cell = row.querySelector(":scope > .cell-gutter-bars"); if (cell && cell.dataset.spans !== key) { cell.remove(); cell = null; } if (!cell && open.size) { cell = _el("span", "cell-gutter-bars"); cell.dataset.spans = key; - for (const bar of open) cell.appendChild(_spanMark(bar.spanId, bar.depth, "bar")); + for (const bar of open) cell.appendChild(_spanMark(bar.span, bar.depth, "bar")); row.appendChild(cell); } } @@ -1532,59 +1550,6 @@ function _pairOf(row: HTMLElement): HTMLElement { return pair; } -function _attachSpanNote( - note: AnnotationSpan, idx: number, - rowElsOld: HTMLElement[], rowElsNew: HTMLElement[], - hunkId: string, filePath: string, -): void { - // If this span has already been promoted to a local comment, skip - // rendering it — the comment now stands in its place. Keeps a - // re-augment from resurrecting an observation the reviewer has - // already turned into a comment. A comment store written before - // spans recorded the note under its `line_note` id; honour that too. - if (Comments.isPromoted(note.id) || Comments.isPromoted(`${hunkId}:line_note:${note.start}`)) return; - Annotations.attach({ - anchor: rowElsNew[idx], - shadowAnchor: rowElsOld[idx], - variant: "note", - content: _buildSpanNoteContent(note, filePath, rowElsNew[idx]), - onInsert: (el) => { el.dataset.spanId = note.id; }, - }); -} - -/** Compose a single-line span's annotation body: the LLM's text plus a - * small "Add as comment" affordance that hands the body to the comment - * editor pre-filled and anchored at the same row. */ -function _buildSpanNoteContent( - note: AnnotationSpan, filePath: string, rowEl: HTMLElement, -): HTMLElement { - const wrap = document.createElement("div"); - wrap.className = "line-note-body"; - const text = document.createElement("div"); - text.className = "line-note-text"; - text.textContent = note.intent || ""; - wrap.appendChild(text); - - const actions = document.createElement("div"); - actions.className = "line-note-actions"; - const promote = document.createElement("button"); - promote.className = "comment-btn comment-btn-promote"; - promote.type = "button"; - promote.textContent = "Add as comment"; - promote.title = "Open the comment editor pre-filled with this observation"; - promote.addEventListener("click", (e) => { - e.stopPropagation(); - Comments.openPromotionEditor({ - rowEl, side: "new", line: note.start, - file: filePath, body: note.intent || "", - derivedFrom: note.id, - }); - }); - actions.appendChild(promote); - wrap.appendChild(actions); - return wrap; -} - function _renderRow( row: RowBlock, file: FileBlock, diff --git a/semantic_code_review/viewer/assets/viewer.css b/semantic_code_review/viewer/assets/viewer.css index 79dbb5c..c226fe0 100644 --- a/semantic_code_review/viewer/assets/viewer.css +++ b/semantic_code_review/viewer/assets/viewer.css @@ -1027,8 +1027,8 @@ html.dragging-divider { cursor: col-resize; user-select: none; } font-weight: 400; white-space: normal; max-width: 64ch; - /* Fold summaries clamp to ~3 lines; line-notes and comments get to - * wrap freely. The annotations module applies the actual per-variant + /* Fold summaries clamp to ~3 lines; comments get to wrap freely. + * The annotations module applies the actual per-variant * sizing inline via `layout.maxHeight`; this CSS default is only a * pre-JS fallback. */ max-height: 5em; @@ -1059,11 +1059,6 @@ html.dragging-divider { cursor: col-resize; user-select: none; } max-height: none; /* comments can wrap freely */ } -/* Line-note variant: same palette as fold summaries, but the body is - * often several sentences and should not be clipped. */ -.diff .annot-note .annot-box { - max-height: none; -} .comment-body { white-space: pre-wrap; font-size: 100%; } /* Header strip on ingested comments: avatar, @author, permalink. Pure @@ -1177,24 +1172,6 @@ html.dragging-divider { cursor: col-resize; user-select: none; } } .comment-btn-reply { padding: 2px 10px; } -/* LLM line-note annotation — the LLM observation plus a small - * "Add as comment" affordance. Clicking the button opens the comment - * editor pre-filled with the line-note text and the source annotation - * row is removed once a comment is saved. */ -.line-note-body { display: flex; flex-direction: column; gap: 4px; } -.line-note-text { white-space: pre-wrap; } -.line-note-actions { - display: flex; - justify-content: flex-end; - margin-top: 2px; -} -.comment-btn-promote { - padding: 1px 8px; - font-size: 10px; - opacity: 0.7; -} -.comment-btn-promote:hover { opacity: 1; } - /* Smell pills that are click-to-promote get a subtle hover cue — * pointer cursor + a hint underline. Non-promotable smells (the * file-overview aggregator) stay static. */ diff --git a/tests/js/viewer.test.ts b/tests/js/viewer.test.ts index ddbce54..584c503 100644 --- a/tests/js/viewer.test.ts +++ b/tests/js/viewer.test.ts @@ -935,21 +935,22 @@ describe("streaming events", () => { expect(text).not.toContain(s.intent as string); expect(text).not.toContain(`+${s.start}`); } - expect(hunk.querySelector(".span-mark, .span-text, .annot-note, .label-tree, .label-row")).toBeNull(); + expect(hunk.querySelector(".span-mark, .span-text, .label-tree, .label-row")).toBeNull(); expect(codeRows(".hunk")).toBe(0); }); - test("at 'code' a hunk's spans are bars and notes on its rows; the definitions are chevrons", async () => { + test("at 'code' a hunk's spans are marks and text blocks in the gutter; the definitions are chevrons", async () => { await bootViewer(makeData({ pending: false, files: [spansFile(SPANS)], symbols: [] })); fold("code"); expect(codeRows(".hunk")).toBe(9); - // Multi-line spans bar; single-line spans dot and note; no tree until - // a definition is collapsed. + // Multi-line spans bar, single-line spans dot; every span has a text + // block in the gutter and nothing in the code column; no tree until a + // definition is collapsed. expect(document.querySelectorAll('.span-mark[data-span-id="H0_0:span:5-7"]').length).toBe(3); expect(document.querySelectorAll('.span-mark[data-span-id="H0_0:span:9-10"]').length).toBe(2); - expect(document.querySelectorAll(".half-new > .span-text").length).toBe(2); + expect(document.querySelectorAll(".half-new > .span-text").length).toBe(4); expect(document.querySelectorAll(".span-dot").length).toBe(2); - expect(document.querySelectorAll(".row-annotation.annot-note").length).toBe(2); + expect(document.querySelectorAll(".row-annotation[data-span-id]").length).toBe(0); // The label trees exist only inside the (hidden) fold boxes. for (const tree of document.querySelectorAll(".label-tree")) { expect((tree.closest(".row-annotation") as HTMLElement).style.display).toBe("none"); @@ -989,7 +990,8 @@ describe("streaming events", () => { expect(codeMarks("H0_0:span:1-4")).toBe(4); expect(codeMarks("H0_0:span:2-3")).toBe(2); expect(document.querySelector('.span-text[data-span-id="H0_0:span:1-4"] .span-text-intent')!.textContent).toBe("region"); - expect(document.querySelectorAll(".row-annotation.annot-note").length).toBe(2); + expect(document.querySelector('.span-text[data-span-id="H0_0:span:3-3"] .span-text-intent')!.textContent).toBe("callout"); + expect(document.querySelectorAll(".row-annotation").length).toBe(0); }); test("focus reveals the focused hunk's code; the slider still folds it to level", async () => { @@ -1226,11 +1228,11 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { expect(body.style.getPropertyValue("--span-bars-w")).toBe("16px"); expect(body.style.getPropertyValue("--span-text-w")).toBe("26ch"); - // Every row is placed explicitly — the callout's note hangs off line 5 - // as track 3, so lines 6..8 are tracks 4..6 — and the outer span's text - // block sits on its first row, line 4, hanging down from there. - expect(newRows().map(gridRow)).toEqual(["1", "2", "4", "5", "6"]); - expect((rowOfLine(5).nextElementSibling as HTMLElement).style.gridRow).toBe("3"); + // Every row is placed explicitly, and nothing of a span sits between + // the rows — the outer span's text block sits on its first row, line + // 4, hanging down from there. + expect(newRows().map(gridRow)).toEqual(["1", "2", "3", "4", "5"]); + expect(document.querySelector(".row-annotation")).toBeNull(); const outer = textOf("H0_0:span:4-7")!; expect(outer).not.toBeNull(); expect(outer.querySelector(".span-text-intent")!.textContent).toBe("the outer edit"); @@ -1248,7 +1250,7 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { expect(depth("H0_0:span:4-7")).toBe("0"); expect(depth("H0_0:span:6-7")).toBe("1"); const inner = textOf("H0_0:span:6-7")!; - expect(gridRow(inner)).toBe("4"); + expect(gridRow(inner)).toBe("3"); expect(inner.querySelector(".span-text-intent")!.textContent).toBe("the inner edit"); expect(inner.querySelector(".smell")!.textContent).toBe("dead-code"); // The pills lead the block — beside the bar's first row, not wherever @@ -1257,14 +1259,38 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { expect(Array.from(body.children).map((c) => c.className)).toEqual(["span-text-smells", "span-text-intent"]); expect(textOf("H0_0:span:4-7")!.querySelector(".span-text-smells")).toBeNull(); - // Single-line spans: a dot on their row at their depth, and the note - // beneath the row as before. No text block. + // Single-line spans take the same form: a dot on their row at their + // depth, and their text block in the gutter on that row — nothing in + // the code column, no arrow. expect(marks("H0_0:span:5-5")).toEqual([[5, "span-dot"]]); expect(depth("H0_0:span:5-5")).toBe("1"); expect(marks("H0_0:span:8-8")).toEqual([[8, "span-dot"]]); expect(depth("H0_0:span:8-8")).toBe("0"); - expect(document.querySelectorAll(".row-annotation.annot-note").length).toBe(2); - expect(textOf("H0_0:span:5-5")).toBeNull(); + const callout = textOf("H0_0:span:5-5")!; + expect(callout).not.toBeNull(); + expect(gridRow(callout)).toBe("2"); + expect(callout.querySelector(".span-text-intent")!.textContent).toBe("a callout"); + expect(gridRow(textOf("H0_0:span:8-8")!)).toBe("5"); + expect(document.querySelector(".row-annotation, .annot-arrow")).toBeNull(); + }); + + test("a single-line span's text joins the waterfall: it is pushed below its parent's, stretching the row before it", async () => { + // config.py's +81 inside +76..+81: the parent's text runs down from 76 + // and the child's starts at 81 — inside the parent's — so the child + // moves down by the overlap and line 80 grows by it. + await bootViewer(makeData({ pending: false, files: [gutterFile([ + span("H0_0:span:4-8", 4, 8, "the parent, at length"), + span("H0_0:span:8-8", 8, 8, "the child, longer still"), + ])], symbols: [] })); + fold("code"); + expect(marks("H0_0:span:8-8")).toEqual([[8, "span-dot"]]); + // Line 8 is 80..100; a 90px parent block from 0 ends at 90, so the + // child's block moves to 90 and line 7 grows by 10; its 30px then run + // 10 past the hunk's shifted bottom (110), so line 8 grows by 10 too. + await layoutGeometry({ "H0_0:span:4-8": 90, "H0_0:span:8-8": 30 }); + expect(textOf("H0_0:span:8-8")!.style.display).toBe(""); + expect(minHeights(newRows())).toEqual(["", "", "", "30px", "30px"]); + expect(minHeights(oldRows())).toEqual(["", "", "", "30px", "30px"]); }); test("a row inserted later is placed too, and the text blocks move with the rows", async () => { @@ -1278,9 +1304,10 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { const editor = rowOfLine(4).nextElementSibling as HTMLElement; expect(editor.classList.contains("row-annotation")).toBe(true); expect(editor.style.gridRow).toBe("2"); - expect(newRows().map(gridRow)).toEqual(["1", "3", "5", "6", "7"]); + expect(newRows().map(gridRow)).toEqual(["1", "3", "4", "5", "6"]); expect(gridRow(textOf("H0_0:span:4-7")!)).toBe("1"); - expect(gridRow(textOf("H0_0:span:6-7")!)).toBe("5"); + expect(gridRow(textOf("H0_0:span:5-5")!)).toBe("3"); + expect(gridRow(textOf("H0_0:span:6-7")!)).toBe("4"); }); test("a rationale taller than the hunk stretches only the last row; the old half's pair follows", async () => { @@ -1414,60 +1441,6 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { describe("LLM observation → comment promotion", () => { - test("Add as comment opens the editor pre-filled and saves with derived_from", async () => { - window.location.hash = "#fold=code"; - const data = makeData({ - pending: false, - files: [{ - id: "F0", path: "a.py", status: "modified", language: "python", - adds: 0, dels: 0, summary: "", head_line_count: null, - symbols: { added: [], modified: [], removed: [] }, - hunks: [makeHunkBlock("H0_0", "real intent", { - spans: [{ id: "H0_0:span:2-2", start: 2, end: 2, intent: "consider using Path", smells: [], context: "", refs: [] }], - })], - }], - }); - await bootViewer(data); - await new Promise((r) => setTimeout(r, 0)); - - // The single-line span attaches as a note on the row at line 2 and - // carries the span id on its dataset. - const noteEl = document.querySelector( - `.row-annotation.annot-note[data-span-id="H0_0:span:2-2"]`, - ); - expect(noteEl).not.toBeNull(); - const promote = noteEl!.querySelector(".comment-btn-promote"); - expect(promote).not.toBeNull(); - - promote!.click(); - const ta = document.querySelector(".comment-editor-input"); - expect(ta).not.toBeNull(); - expect(ta!.value).toBe("consider using Path"); - - // Capture the POST payload so we can assert derived_from is set. - let posted: Record | null = null; - (globalThis.fetch as unknown as { mockImplementationOnce: (fn: typeof fetch) => void }) - .mockImplementationOnce(((url: string, init?: RequestInit) => { - fetchCalls.push({ url, init }); - posted = JSON.parse(init!.body as string); - return Promise.resolve({ - status: 200, ok: true, json: () => Promise.resolve(posted), - } as Response); - }) as typeof fetch); - - document.querySelector(".comment-btn-save")!.click(); - await new Promise((r) => setTimeout(r, 0)); - - expect(posted).not.toBeNull(); - expect(posted!.body).toBe("consider using Path"); - expect(posted!.derived_from).toBe("H0_0:span:2-2"); - // Source annotation is gone from the DOM — observation transitioned - // into the comment. - expect(document.querySelector( - `.row-annotation.annot-note[data-span-id="H0_0:span:2-2"]`, - )).toBeNull(); - }); - test("smell pill click saves a comment immediately and detaches the pill", async () => { window.location.hash = "#fold=code"; const data = makeData({ @@ -1539,15 +1512,45 @@ describe("LLM observation → comment promotion", () => { }); await new Promise((r) => setTimeout(r, 0)); - // The annotation source is hidden because a local comment with - // matching derived_from already exists. - expect(document.querySelector( - `.row-annotation.annot-note[data-span-id="H0_0:span:1-1"]`, - )).toBeNull(); - // The promoted comment is rendered at the same line. + // The span is gone from the gutter — block and dot — because a local + // comment derived from it exists; the comment stands in its place. + expect(document.querySelector('[data-span-id="H0_0:span:1-1"]')).toBeNull(); expect(document.querySelector( '.comment-thread-entry[data-comment-id="local-1"]', )).not.toBeNull(); + // The gutter's pass, re-run by the removal, has nothing left to place + // and holds no stretch. + for (const row of document.querySelectorAll(".half-new .row")) expect(row.style.minHeight).toBe(""); + }); + + test("a comment store written before spans hides the span under its line_note id", async () => { + window.location.hash = "#fold=code"; + const spans = [ + { id: "H0_0:span:1-1", start: 1, end: 1, intent: "old observation", smells: [], context: "", refs: [] }, + { id: "H0_0:span:1-2", start: 1, end: 2, intent: "a span over the same start", smells: [], context: "", refs: [] }, + ]; + await bootViewer(makeData({ files: [{ + id: "F0", path: "a.py", status: "modified", language: "python", + adds: 0, dels: 0, summary: "", head_line_count: null, + symbols: { added: [], modified: [], removed: [] }, + hunks: [makeHunkBlock("H0_0", "", { spans })], + }] }), { + comments: [{ + id: "local-1", file: "a.py", side: "new", line: 1, + body: "promoted version", created_at: 1, updated_at: 1, + source: "local", + derived_from: "H0_0:line_note:1", + }], + }); + await new Promise((r) => setTimeout(r, 0)); + // A re-augment rebuilds the hunk with the comments known: the legacy + // id keeps the one-line span from resurrecting, and never touches the + // multi-line span starting on the same line. + lastEventSource().dispatch("hunk", { + file_idx: 0, hunk_idx: 0, ok: true, block: makeHunkBlock("H0_0", "re-run", { spans }), + }); + expect(document.querySelector('[data-span-id="H0_0:span:1-1"]')).toBeNull(); + expect(document.querySelector('.span-text[data-span-id="H0_0:span:1-2"]')).not.toBeNull(); }); }); @@ -2210,13 +2213,13 @@ describe("fold regions (server-computed) and lazy fold summaries", () => { expect(labelRows(tree)).toEqual(["H0_0:span:9-10: beta's guard", "H0_0:span:10-10: callout"]); expect(tree.querySelector('.label-node > .label-row[data-id="H0_0:span:9-10"]')).not.toBeNull(); expect(tree.querySelector('.label-children > .label-row[data-id="H0_0:span:10-10"] .smell')!.textContent).toBe("dead-code"); - // The guard's text block (its first row is hidden) and the callout's - // note fold with their rows. + // The guard's and the callout's text blocks (their first rows are + // hidden) fold with their rows. await tick(); const guardText = document.querySelector('.span-text[data-span-id="H0_0:span:9-10"]')!; - const calloutNote = document.querySelector('.annot-note[data-span-id="H0_0:span:10-10"]')!; + const calloutText = document.querySelector('.span-text[data-span-id="H0_0:span:10-10"]')!; expect(guardText.style.display).toBe("none"); - expect(calloutNote.style.display).toBe("none"); + expect(calloutText.style.display).toBe("none"); // The summary lands: the line changes, the tree stays. await tick(); @@ -2229,7 +2232,7 @@ describe("fold regions (server-computed) and lazy fold summaries", () => { expect(chevronOnLine(8).classList.contains("open")).toBe(true); expect([9, 10, 11].map((n) => rowOfLine(n).style.display)).toEqual(["", "", ""]); expect(guardText.style.display).toBe(""); - expect(calloutNote.style.display).toBe(""); + expect(calloutText.style.display).toBe(""); expect(rowOfLine(9).querySelector('.span-mark[data-span-id="H0_0:span:9-10"]')).not.toBeNull(); expect(box.closest(".row-annotation")!.style.display).toBe("none"); }); @@ -2255,8 +2258,8 @@ describe("fold regions (server-computed) and lazy fold summaries", () => { const betaNode = tree.querySelector('.label-row[data-def="Foo.beta"]')!.parentElement!; expect(labelRows(betaNode.querySelector(":scope > .label-children")!)) .toEqual(["H0_0:span:9-10: beta's guard", "H0_0:span:10-10: callout"]); - // The module constant's note, outside the fold, stays visible. - expect(document.querySelector('.annot-note[data-span-id="H0_0:span:12-12"]')!.style.display).toBe(""); + // The module constant's text block, outside the fold, stays visible. + expect(document.querySelector('.span-text[data-span-id="H0_0:span:12-12"]')!.style.display).toBe(""); // Clicking a definition opens the fold and lands on its opener. clickEl(tree.querySelector('.label-row[data-def="Foo.beta"]')!); From 391cd1cea73b5fce94d09dcf79268e982008519f Mon Sep 17 00:00:00 2001 From: Tobias Sargeant Date: Fri, 4 Sep 2026 14:22:19 +1000 Subject: [PATCH 4/5] =?UTF-8?q?feat(viewer):=20the=20span=20gutter=20folds?= =?UTF-8?q?=20to=20lineno=20=C2=B7=20bars?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A global reading preference, like the fold level but not a rung of it: folded, the gutter's text column is zero wide, every span's block is hidden and no row is stretched — the compact view is the undistorted diff — while the bars and dots still show where each span is and how they nest. The .diff split follows the live text width (a derived --gutter-text-w, zeroed by html.gutter-collapsed), so both code columns widen equally, in the diff pane and the explainer's panel alike. Toggled by g (free beside the 1-3 ladder keys) or by clicking the strip's empty area; a mark's tooltip is its span's rationale (smells, then intent — the native title holds the 686-character case whole), and clicking a mark unfolds the gutter and brings that span's text into view. Marks widen to their 6px column as the hover and click target, drawing the 2px stroke as a pseudo-element. Default expanded, remembered in localStorage (scr-gutter-fold). The placement pass re-runs on every half at each toggle, releasing its stretches on collapse and re-applying them on expand; comment arrows are reflowed since the code columns moved. Rendered mode has no gutter and is unaffected. The status bar's stale 'space toggle' (no such key) becomes 'g gutter'. CONTEXT.md: Annotation span describes the one form, the pill position, the bar through annotation rows and the fold; Fold level notes the fold is a preference, not a rung. --- CONTEXT.md | 66 ++++++++----- semantic_code_review/viewer/assets/render.ts | 97 ++++++++++++++++++- semantic_code_review/viewer/assets/viewer.css | 58 +++++++---- tests/js/viewer.test.ts | 83 ++++++++++++++++ 4 files changed, 256 insertions(+), 48 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 85225c5..d69fa52 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -302,28 +302,47 @@ In the viewer a span is a label, and it shows only where its code does columns after its line numbers, `lineno · text · bars`, a fixed width per file (26ch of text, one 6px bar column per level of nesting; nothing for a file with no span) split between the halves; the bars sit against -the code they mark. A multi-line span is a bar over exactly its rows, -one column further right per level of nesting, with its intent as a -text block at a smaller size, wrapping at the gutter's width, that -starts on the span's first row and hangs down for as long as it needs — -past the end of its bar if it must; nothing is clipped. The block is a -zero-height grid item, so it sizes no row; the one placement rule is -that blocks do not overlap. A downward pass over the half's rows -(`render._layoutSpanTexts`) enforces it: where a block would start -inside the one above, the nearest code row above it is stretched by the -overlap (a `min-height`, mirrored onto the old half's paired row so the -halves stay aligned), and text running past the hunk's last row -stretches its last code row; every other row keeps its natural height. -Two spans starting on one row chain their blocks, outermost first. The -pass runs when the half's rows change (an annotation inserted, a fold -hiding rows — a `MutationObserver`) or its size does (a +the code they mark. Every span takes one form, whatever its length: a +mark in the bars column — a bar over exactly its rows, or a dot on a +span of one line, one column further right per level of nesting — and +a text block in the text column, at a smaller size, wrapping at the +gutter's width, that starts on the span's first row and hangs down for +as long as it needs — past the end of its bar if it must; nothing is +clipped, and nothing of a span is in the code column. The block leads +with the span's smell pills, so they sit beside the bar's first row, +then the intent. The block is a zero-height grid item, so it sizes no +row; the one placement rule is that blocks do not overlap. A downward +pass over the half's rows (`render._layoutSpanTexts`) enforces it: +where a block would start inside the one above, the nearest code row +above it is stretched by the overlap (a `min-height`, mirrored onto the +old half's paired row so the halves stay aligned), and text running +past the hunk's last row stretches its last code row; every other row +keeps its natural height. Two spans starting on one row chain their +blocks, outermost first. The same pass gives every non-code row inside +a bar — a comment thread's row, a fold's label row — a bars cell with +the bar's segment at its depth, so a bar runs unbroken through them. +The pass runs when the half's rows change (an annotation inserted, a +fold hiding rows — a `MutationObserver`) or its size does (a `ResizeObserver`, on the next frame); it measures once, writes once, -and discards the mutation records its own writes queue. A single-line -span is a dot on its row at its depth and the inline note beneath the -row. When a fold hides a span's first row its text hides too and the -fold's label tree lists it ([[fold-region]]). `render._attachSpans` -owns all of this; the explicit `grid-row` on every row of a half with -spans is what places the blocks. On disk, +and discards the mutation records its own writes queue. When a fold +hides a span's first row its text hides too and the fold's label tree +lists it ([[fold-region]]). A span the reviewer has turned into a +[[reviewer-comment]] (a local comment `derived_from` its id, or the +`line_note` id a store written before spans used) is not drawn; the +comment stands in its place. `render._attachSpans` owns all of this; +the explicit `grid-row` on every row of a half with spans is what +places the blocks. + +The gutter **folds**, globally: folded, it is `lineno · bars` — the text +column zero wide, every block hidden, no row stretched, the bars and +dots still showing where each span is and how they nest, a mark's +tooltip its rationale. Clicking a mark unfolds it and brings that +span's text into view; clicking the strip's empty area, or `g`, +toggles. A reading preference like the [[fold-level]], not a rung of +it: kept in localStorage (`scr-gutter-fold`), default expanded, and +followed by every pane — the explainer's panel included — through one +class on the document (`html.gutter-collapsed`) that the stylesheet +zeroes the text column by. On disk, `scr-span: +a..+b "intent"` is an intent-only span and `scr-span-begin` … `scr-span-end` a block with smells/context/refs; the retired `scr-segment-*` / `scr-line` directives and a sidecar's `segments` / @@ -382,7 +401,10 @@ the AST) is a projection, not a summary, and neither exists. The three axes the ADR names are affordances at `code`, not rungs: the expand chip hides ([[collapsible-region]]), the definition chevron folds ([[fold-region]] — a collapsed one shows its labels), the span labels -([[annotation-span]], in the centre gutter). +([[annotation-span]], in the centre gutter). The gutter's own fold (`g`) +is not a rung either: it hides the spans' text, never rows, and is a +preference kept in localStorage rather than a level carried by the +hash. Per-item exceptions live in `RenderState.overrides` — a reviewer expanding/collapsing one file (`F0`) or hunk (`H0_1`, header open or diff --git a/semantic_code_review/viewer/assets/render.ts b/semantic_code_review/viewer/assets/render.ts index 2ae6aaa..63b0a76 100644 --- a/semantic_code_review/viewer/assets/render.ts +++ b/semantic_code_review/viewer/assets/render.ts @@ -71,6 +71,13 @@ let _smells: Record = {}; // it up in _renderContent; setSymbolSearch repaints cells already in the // DOM. See setSymbolSearch / sidebar's active-pill callback. let _symbolSearch: string | null = null; +// Whether the centre gutter is folded to `lineno · bars` — its text column +// zero wide, the rationales hidden, the bars and dots still showing where +// every span is. A reading preference like the fold level, global across +// files and panes, kept in localStorage rather than the hash: it is the +// reader's, not the link's. Default expanded. +let _gutterCollapsed = false; +const _GUTTER_FOLD_KEY = "scr-gutter-fold"; // Whether the change explainer exists for this review (`--no-augment` // and `[augment].explainer = false` both switch it off server-side). // Gates the mode entirely: with it false there is no button, and a @@ -167,6 +174,7 @@ function renderInit(data: ViewerData): void { // A filter restored from localStorage is not a gesture: the diff opens // at its level, filtered, with nothing focused. _state.focus = null; + _applyGutterFold(_readGutterFold()); _wireInputs(); _restoreHash(); render(); @@ -1339,6 +1347,65 @@ function _setGutterWidths(body: HTMLElement, f: FileBlock): void { body.style.setProperty("--span-text-w", cols ? _SPAN_TEXT_WIDTH : "0px"); } +// --- The gutter fold --------------------------------------------------------- + +/** The stored preference; expanded when nothing (or nothing this code + * wrote) is stored. */ +function _readGutterFold(): boolean { + try { + return localStorage.getItem(_GUTTER_FOLD_KEY) === "collapsed"; + } catch (_) { + return false; + } +} + +/** Fold or unfold the gutter everywhere it is on screen. The class on + * the document is what the stylesheet zeroes the text column by, in the + * diff pane and the explainer's panel alike; the placement pass is then + * re-run on every half that has one, so a collapse releases its + * stretches and an expand puts them back. */ +function _applyGutterFold(collapsed: boolean): void { + _gutterCollapsed = collapsed; + document.documentElement.classList.toggle("gutter-collapsed", collapsed); + for (const half of document.querySelectorAll(".half-new[data-span-observed]")) _layoutSpanTexts(half); + // The code columns moved; the comment arrows point at characters in them. + Annotations.reflowAll(); +} + +function _setGutterFold(collapsed: boolean): void { + if (collapsed === _gutterCollapsed) return; + _applyGutterFold(collapsed); + try { + localStorage.setItem(_GUTTER_FOLD_KEY, collapsed ? "collapsed" : "expanded"); + } catch (_) { /* localStorage may be unavailable */ } +} + +/** The strip's own affordances, delegated from the document so one + * listener covers every pane: a click on a span's mark expands the + * gutter (if folded) and brings that span's text into view; a click on + * the strip's empty area — a gutter cell itself, not a block or mark on + * it — toggles the fold. */ +function _onGutterClick(e: MouseEvent): void { + const target = e.target as HTMLElement | null; + if (!target) return; + const mark = target.closest(".span-mark[data-span-id]"); + if (mark) { + e.stopPropagation(); + _setGutterFold(false); + const half = mark.closest(".half-new"); + const body = half?.querySelector(`.span-text-body[data-span-id="${_cssEscape(mark.dataset.spanId!)}"]`); + if (body) body.scrollIntoView({ block: "nearest" }); + return; + } + if (target.classList.contains("cell-gutter-text") || target.classList.contains("cell-gutter-bars")) { + e.stopPropagation(); + _setGutterFold(!_gutterCollapsed); + } +} + +/** What a gutter cell says on hover; the strip has no chrome of its own. */ +const _GUTTER_CELL_TITLE = "Click to fold or unfold the span gutter (g)"; + /** A row's bars cell (`children[3]`; `[2]` is its text cell). */ function _gutterBars(rowEl: HTMLElement): HTMLElement { const cell = rowEl.children[3] as HTMLElement | undefined; @@ -1346,15 +1413,24 @@ function _gutterBars(rowEl: HTMLElement): HTMLElement { return cell; } -/** A bar segment or dot in a row's bars cell, at its depth's column. */ +/** A bar segment or dot in a row's bars cell, at its depth's column. Its + * tooltip is the span's rationale — what a folded gutter shows of it on + * hover; the native tooltip holds a 700-character intent whole. */ function _spanMark(span: AnnotationSpan, depth: number, kind: "bar" | "bar-top" | "bar-bottom" | "dot"): HTMLElement { const mark = _el("span", `span-mark span-${kind}`); mark.dataset.spanId = span.id; mark.style.setProperty("--depth", String(depth)); mark.setAttribute("aria-hidden", "true"); + mark.title = _spanTooltip(span); return mark; } +function _spanTooltip(span: AnnotationSpan): string { + const smells = (span.smells || []).map((sm) => sm.tag); + const intent = span.intent || "(no intent)"; + return smells.length ? `${smells.join(" · ")}\n${intent}` : intent; +} + /** Whether the reviewer has already turned this span into a comment: a * local comment derived from its id, or — for a span of one line — from * the `line_note` id a comment store written before spans recorded the @@ -1452,7 +1528,9 @@ function _layoutSpanTexts(half: HTMLElement): void { const shown: SpanTextBlock[] = []; for (const block of gutter.blocks) { const start = track.get(block.row); - if (start === undefined) { + // Folded, the gutter has no text to place: every block hides and no + // row is stretched — the compact view is the undistorted diff. + if (start === undefined || _gutterCollapsed) { block.el.style.display = "none"; block.el.style.transform = ""; continue; @@ -1461,6 +1539,10 @@ function _layoutSpanTexts(half: HTMLElement): void { block.el.style.gridRow = String(start); shown.push(block); } + if (!shown.length) { + gutter.observer?.takeRecords(); + return; + } // The one measurement. const rects = new Map(); for (const row of visible) rects.set(row, row.getBoundingClientRect()); @@ -1566,8 +1648,11 @@ function _renderRow( newRow.appendChild(_renderContent(row.new_text, "new", hasNew, file, newMarks)); // The centre gutter's cells, after the content so `children[1]` stays // the content cell; the grid places them between number and code. - newRow.appendChild(_el("span", "cell-gutter-text")); - newRow.appendChild(_el("span", "cell-gutter-bars")); + const text = _el("span", "cell-gutter-text"); + const bars = _el("span", "cell-gutter-bars"); + text.title = bars.title = _GUTTER_CELL_TITLE; + newRow.appendChild(text); + newRow.appendChild(bars); return { old: oldRow, new: newRow }; } @@ -1747,7 +1832,7 @@ function _updateStatus(): void { } } } - s.textContent = `${_data.files.length} files · ${smells} smells · ${critical} critical · keys 1-3 fold · space toggle · ? help`; + s.textContent = `${_data.files.length} files · ${smells} smells · ${critical} critical · keys 1-3 fold · g gutter · ? help`; } function _syncHash(): void { @@ -1799,6 +1884,7 @@ function _onKeydown(e: KeyboardEvent): void { case "1": _setGlobalFold("files"); e.preventDefault(); break; case "2": _setGlobalFold("hunks"); e.preventDefault(); break; case "3": _setGlobalFold("code"); e.preventDefault(); break; + case "g": _setGutterFold(!_gutterCollapsed); e.preventDefault(); break; case "?": _toggleHelp(); e.preventDefault(); break; case "Escape": _onEscape(); break; } @@ -1870,6 +1956,7 @@ function _wireInputs(): void { if (e.target === overlay) _closeHelp(); }); document.addEventListener("keydown", _onKeydown); + document.addEventListener("click", _onGutterClick); window.addEventListener("hashchange", () => { _state.overrides = Object.create(null); _restoreHash(); diff --git a/semantic_code_review/viewer/assets/viewer.css b/semantic_code_review/viewer/assets/viewer.css index c226fe0..3770ff0 100644 --- a/semantic_code_review/viewer/assets/viewer.css +++ b/semantic_code_review/viewer/assets/viewer.css @@ -724,15 +724,20 @@ html.dragging-divider { cursor: col-resize; user-select: none; } * for free but requires explicit outer row tracks, which we don't know at * CSS compile time. * - * The gutter's width (`--span-bars-w` + `--span-text-w`, set per file on - * `.file-body`; 0 for a file with no span) is split between the halves, - * so the new half is wider by it and the code columns give up the same - * amount each. */ + * The gutter's width (`--span-bars-w` + `--gutter-text-w`; 0 for a file + * with no span) is split between the halves, so the new half is wider by + * it and the code columns give up the same amount each. `--span-bars-w` + * and `--span-text-w` are set per file on `.file-body`; `--gutter-text-w` + * is the text column's live width — `--span-text-w` expanded, 0 when the + * gutter is folded (`html.gutter-collapsed`, a global preference), so + * both code columns widen equally and the bars stay. */ +.file-body { --gutter-text-w: var(--span-text-w, 0px); } +.gutter-collapsed .file-body { --gutter-text-w: 0px; } .diff { display: grid; grid-template-columns: - minmax(0, calc(50% - (var(--span-bars-w, 0px) + var(--span-text-w, 0px)) / 2)) - minmax(0, calc(50% + (var(--span-bars-w, 0px) + var(--span-text-w, 0px)) / 2)); + minmax(0, calc(50% - (var(--span-bars-w, 0px) + var(--gutter-text-w, 0px)) / 2)) + minmax(0, calc(50% + (var(--span-bars-w, 0px) + var(--gutter-text-w, 0px)) / 2)); font-family: var(--mono); font-size: 12.5px; line-height: 1.5; @@ -777,7 +782,7 @@ html.dragging-divider { cursor: col-resize; user-select: none; } * the content cell in the DOM so `children[1]` stays the content cell * everywhere. */ .diff .half-new { - grid-template-columns: 3.5em var(--span-text-w, 0px) var(--span-bars-w, 0px) minmax(max-content, 1fr); + grid-template-columns: 3.5em var(--gutter-text-w, 0px) var(--span-bars-w, 0px) minmax(max-content, 1fr); } .diff .cell-lineno { grid-column: 1; } .diff .half-old .cell-content { grid-column: 2; } @@ -790,32 +795,43 @@ html.dragging-divider { cursor: col-resize; user-select: none; } .diff .cell-gutter-text, .diff .span-text { grid-column: 2; left: 3.5em; } .diff .cell-gutter-bars { grid-column: 3; - left: calc(3.5em + var(--span-text-w, 0px)); + left: calc(3.5em + var(--gutter-text-w, 0px)); box-shadow: inset -1px 0 var(--border); } /* A span's marks in the bars cell: a bar over its rows, a dot on a * single-line span's row, one column further right (nearer the code) per - * level of nesting. */ + * level of nesting. The mark is its column's full 6px for the row's + * height — the hover and click target; the stroke it draws is 2px. */ .diff .span-mark { position: absolute; - left: calc(2px + var(--depth, 0) * 6px); + top: 0; bottom: 0; + left: calc(var(--depth, 0) * 6px); + width: 6px; + cursor: pointer; +} +.diff .span-mark::before { + content: ""; + position: absolute; + top: 0; bottom: 0; left: 2px; width: 2px; background: var(--accent); opacity: 0.75; - pointer-events: none; } -.diff .span-bar, .diff .span-bar-top, .diff .span-bar-bottom { top: 0; bottom: 0; } -.diff .span-bar-top { top: 3px; border-top-left-radius: 2px; border-top-right-radius: 2px; } -.diff .span-bar-bottom { bottom: 3px; border-bottom-left-radius: 2px; border-bottom-right-radius: 2px; } -.diff .span-dot { - top: 50%; margin-top: -2px; margin-left: -1px; +.diff .span-mark:hover::before { opacity: 1; } +.diff .span-bar-top::before { top: 3px; border-top-left-radius: 2px; border-top-right-radius: 2px; } +.diff .span-bar-bottom::before { bottom: 3px; border-bottom-left-radius: 2px; border-bottom-right-radius: 2px; } +.diff .span-dot::before { + top: 50%; bottom: auto; margin-top: -2px; left: 1px; width: 4px; height: 4px; border-radius: 50%; } -/* A multi-line span's text: a zero-height item on the span's first row - * (`grid-row`, set by the renderer), so the row's size owes it nothing; - * the body overflows downward over the strip for as long as it needs. - * `transform` is the renderer's chaining offset for a block that starts - * on the same row as the one above it. */ +/* The empty strip is the fold's own affordance. */ +.diff .cell-gutter-text, .diff .cell-gutter-bars { cursor: pointer; } +/* A span's text: a zero-height item on the span's first row (`grid-row`, + * set by the renderer), so the row's size owes it nothing; the body + * overflows downward over the strip for as long as it needs. `transform` + * is the renderer's chaining offset for a block that starts on the same + * row as the one above it. The renderer hides every block while the + * gutter is folded. */ .diff .span-text { z-index: 2; align-self: start; diff --git a/tests/js/viewer.test.ts b/tests/js/viewer.test.ts index 584c503..b544784 100644 --- a/tests/js/viewer.test.ts +++ b/tests/js/viewer.test.ts @@ -1437,6 +1437,89 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { // The code rows' own marks are untouched by the pass. expect(marks("H0_0:span:4-7")).toEqual([[4, "span-bar-top"], [5, "span-bar"], [6, "span-bar"], [7, "span-bar-bottom"]]); }); + + describe("the gutter fold", () => { + const key = (k: string): void => { document.dispatchEvent(new KeyboardEvent("keydown", { key: k })); }; + const collapsed = (): boolean => document.documentElement.classList.contains("gutter-collapsed"); + /** A tall block, so the expanded gutter stretches the hunk's last row. */ + const TALL = [span("H0_0:span:4-5", 4, 5, "tall", [{ tag: "dead-code", note: "" }])]; + + test("g folds the gutter: blocks hide, no row is stretched, marks stay; g again restores both", async () => { + await bootViewer(makeData({ pending: false, files: [gutterFile(TALL)], symbols: [] })); + fold("code"); + await layoutGeometry({ "H0_0:span:4-5": 130 }); + expect(collapsed()).toBe(false); + expect(minHeights(newRows())).toEqual(["", "", "", "", "50px"]); + + key("g"); + expect(collapsed()).toBe(true); + expect(textOf("H0_0:span:4-5")!.style.display).toBe("none"); + expect(minHeights(newRows())).toEqual(["", "", "", "", ""]); + expect(minHeights(oldRows())).toEqual(["", "", "", "", ""]); + expect(marks("H0_0:span:4-5")).toEqual([[4, "span-bar-top"], [5, "span-bar-bottom"]]); + expect(localStorage.getItem("scr-gutter-fold")).toBe("collapsed"); + // A pass the rows trigger while folded places nothing either. + await layoutGeometry({ "H0_0:span:4-5": 130 }); + expect(minHeights(newRows())).toEqual(["", "", "", "", ""]); + + key("g"); + expect(collapsed()).toBe(false); + expect(textOf("H0_0:span:4-5")!.style.display).toBe(""); + expect(minHeights(newRows())).toEqual(["", "", "", "", "50px"]); + expect(minHeights(oldRows())).toEqual(["", "", "", "", "50px"]); + expect(localStorage.getItem("scr-gutter-fold")).toBe("expanded"); + }); + + test("the fold is remembered across a reload, and applies to every half", async () => { + localStorage.setItem("scr-gutter-fold", "collapsed"); + const file = gutterFile(TALL); + const hunks = file.hunks as Record[]; + hunks.push(makeHunkBlock("H0_1", "adds more", { + old_start: 8, old_count: 0, new_start: 12, new_count: 2, + rows: [12, 13].map((n) => ({ kind: "ins", old_line: null, new_line: n, old_text: "", new_text: `l${n}` })), + spans: [span("H0_1:span:12-13", 12, 13, "later")], + })); + await bootViewer(makeData({ pending: false, files: [file], symbols: [] })); + fold("code"); + expect(collapsed()).toBe(true); + const blocks = Array.from(document.querySelectorAll(".half-new > .span-text")); + expect(blocks.length).toBe(2); + expect(blocks.map((b) => b.style.display)).toEqual(["none", "none"]); + key("g"); + expect(blocks.map((b) => b.style.display)).toEqual(["", ""]); + }); + + test("the strip's empty area toggles the fold; a mark expands it and brings its text into view", async () => { + await bootViewer(makeData({ pending: false, files: [gutterFile(TALL)], symbols: [] })); + fold("code"); + const cell = rowOfLine(6).querySelector(".cell-gutter-text")!; + expect(cell.title).toContain("(g)"); + cell.click(); + expect(collapsed()).toBe(true); + (rowOfLine(6).querySelector(".cell-gutter-bars") as HTMLElement).click(); + expect(collapsed()).toBe(false); + cell.click(); + expect(collapsed()).toBe(true); + + // Folded, a mark's tooltip is the span's rationale: its smells, then + // its intent. + const mark = rowOfLine(5).querySelector('.span-mark[data-span-id="H0_0:span:4-5"]')!; + expect(mark.title).toBe("dead-code\ntall"); + const scrolled: Element[] = []; + const orig = Element.prototype.scrollIntoView; + Element.prototype.scrollIntoView = function scrollIntoView(this: Element): void { scrolled.push(this); }; + try { + mark.click(); + } finally { + Element.prototype.scrollIntoView = orig; + } + expect(collapsed()).toBe(false); + expect(scrolled).toEqual([textOf("H0_0:span:4-5")!.querySelector(".span-text-body")]); + // A click on a block's own text is not a fold gesture. + (textOf("H0_0:span:4-5")!.querySelector(".span-text-intent") as HTMLElement).click(); + expect(collapsed()).toBe(false); + }); + }); }); From b1a4db71efd5fdb6f7695ca1eb18877d056faf04 Mon Sep 17 00:00:00 2001 From: Tobias Sargeant Date: Fri, 4 Sep 2026 14:24:59 +1000 Subject: [PATCH 5/5] docs(viewer): the help overlay names the gutter fold key --- semantic_code_review/viewer/assets/index.html | 1 + 1 file changed, 1 insertion(+) diff --git a/semantic_code_review/viewer/assets/index.html b/semantic_code_review/viewer/assets/index.html index 07cc3a2..c3e612e 100644 --- a/semantic_code_review/viewer/assets/index.html +++ b/semantic_code_review/viewer/assets/index.html @@ -55,6 +55,7 @@

Keyboard shortcuts

1 fold to files · 2 hunks · 3 code

+

g fold the span gutter to line numbers and bars

? toggle this help · Esc close

Click any region header to toggle its fold state independently. "Reset" clears per-region overrides and applies the global fold level.