diff --git a/CONTEXT.md b/CONTEXT.md index d69fa52..4a9302d 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -298,45 +298,54 @@ buckets its integer `(file, line)` notes into single-line spans. In the viewer a span is a label, and it shows only where its code does (see [[fold-level]]). At `files` and `hunks` nothing mentions a span. At -`code` a span lives in the **centre gutter** — the new half's sticky -columns after its line numbers, `lineno · text · bars`, a fixed width +`code` a span lives in the **span gutter** at the diff's right edge — +the new half's last two columns, `lineno · code · bars · text`, sticky +to the half's right edge as the line numbers are to its left, so the +strip stays put while the code scrolls under it. It has 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. 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 +for a file with no span), split between the halves so the two code +viewports stay equal and sit beside each other; the bars sit against the +code they mark, the text outside them. 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; the outermost span's column is +against the text and each level of nesting is one column nearer the code +— 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 +with a pill row, so it sits beside the bar's first row — the span's +smell pills, each promotable to a [[reviewer-comment]] by a click, and a +`+ comment` affordance that promotes the span's intent the same way, by +the one-click path (`Comments.promote`), as a comment on the span's +first line — 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. 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 +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. 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 whose intent 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) loses its text block and, on one line, its +dot — the comment stands in their place — but keeps its bar, which marks +a range the comment does not. `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 the bars alone at the +right edge — 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 @@ -401,7 +410,7 @@ 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). The gutter's own fold (`g`) +([[annotation-span]], in the span 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. diff --git a/semantic_code_review/viewer/assets/comments.ts b/semantic_code_review/viewer/assets/comments.ts index 43d93a0..adde335 100644 --- a/semantic_code_review/viewer/assets/comments.ts +++ b/semantic_code_review/viewer/assets/comments.ts @@ -84,30 +84,15 @@ function isPromoted(annotationId: string): boolean { return false; } -/** Open the comment editor pre-filled with text from an LLM annotation. - * On save the new local comment carries `derived_from` set to the - * annotation's id, so the renderer can hide the source annotation. */ -function openPromotionEditor(opts: { - rowEl: HTMLElement; - side: "old" | "new"; - line: number; - file: string; - body: string; +/** One-click promote: save a local comment with the observation's body + * immediately and detach what the observation showed as. No mid-flight + * editor — a smell often lives in a folded hunk header where there's no + * row to anchor an inline editor on, and the user can edit/delete via + * the comment's normal edit affordance once it's saved. `derivedFrom` + * is the observation's stable id: a smell's (`:smell:`) or + * a span's, for its intent. */ +function promote(opts: { derivedFrom: string; -}): void { - _openEditor({ - rowEl: opts.rowEl, side: opts.side, line: opts.line, file: opts.file, - prefillBody: opts.body, derivedFrom: opts.derivedFrom, - }); -} - -/** One-click promote: save a local comment with the smell's body - * immediately and detach the source pill. No mid-flight editor — - * smells often live in a folded hunk header where there's no row to - * anchor an inline editor on, and the user can edit/delete via the - * comment's normal edit affordance once it's saved. */ -function promoteSmell(opts: { - smellId: string; file: string; side: "old" | "new"; line: number; @@ -118,10 +103,10 @@ function promoteSmell(opts: { const c: ReviewerComment = { id, file: opts.file, side: opts.side, line: opts.line, body: opts.body, created_at: now, updated_at: now, - derived_from: opts.smellId, + derived_from: opts.derivedFrom, }; _store.save(c).then(() => { - _removeAnnotationByDerivedId(opts.smellId); + _removeAnnotationByDerivedId(opts.derivedFrom); renderAll(); _onChange?.(); }); @@ -220,26 +205,18 @@ interface EditorOpts { /** When set, the new comment is saved as a reply (in_reply_to_id pinned * to this id). Ignored for edits of an existing comment. */ replyTo?: string | null; - /** Prefill text for the editor (new comments only — edits use existing.body). */ - prefillBody?: string; - /** Stable id of the LLM annotation this comment is being promoted from. - * Persisted on the saved comment so the renderer can hide the source - * annotation. Ignored for edits of an existing comment. */ - derivedFrom?: string | null; } -function _openEditor({ rowEl, side, line, file, existing, replyTo, prefillBody, derivedFrom }: EditorOpts): void { +function _openEditor({ rowEl, side, line, file, existing, replyTo }: EditorOpts): void { const bodyWrap = _el("div", "comment-editor-body"); const ta = _el("textarea", "comment-editor-input") as HTMLTextAreaElement; ta.rows = 1; ta.placeholder = existing ? "Edit comment… (Enter to save, Shift-Enter for newline, Esc to cancel)" - : derivedFrom - ? "Edit and save as a comment… (Enter to save, Shift-Enter for newline, Esc to cancel)" - : replyTo - ? "Write a reply… (Enter to save, Shift-Enter for newline, Esc to cancel)" - : "Write a comment… (Enter to save, Shift-Enter for newline, Esc to cancel)"; - ta.value = existing ? existing.body : (prefillBody ?? ""); + : replyTo + ? "Write a reply… (Enter to save, Shift-Enter for newline, Esc to cancel)" + : "Write a comment… (Enter to save, Shift-Enter for newline, Esc to cancel)"; + ta.value = existing ? existing.body : ""; bodyWrap.appendChild(ta); const bar = _el("div", "comment-editor-bar"); const save = _el("button", "comment-btn comment-btn-save", @@ -278,16 +255,10 @@ function _openEditor({ rowEl, side, line, file, existing, replyTo, prefillBody, in_reply_to_id: existing ? existing.in_reply_to_id ?? null : (replyTo ?? null), - derived_from: existing - ? existing.derived_from ?? null - : (derivedFrom ?? null), + derived_from: existing ? existing.derived_from ?? null : null, }; _store.save(c).then(() => { close(); - // If this save came from "Add as comment" on an LLM annotation, - // remove the source annotation row from the DOM so the - // observation visibly transitions into the comment. - if (c.derived_from) _removeAnnotationByDerivedId(c.derived_from); _refreshForAnchor(rowEl, { file, side, line }); _onChange?.(); }); @@ -620,13 +591,16 @@ function _refreshForAnchor(anchorRowEl: HTMLElement, anchor: Anchor): void { * source observation visibly transitions into the comment. * * 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. */ + * (`.span-mark`) in the span gutter, both carrying `data-span-id`; + * smells render as inline `.smell` pills with `data-smell-id`. Of a + * span, the block and a dot go — the comment stands in their place — + * but a bar stays: it marks a range the comment, on one line, does + * not (the renderer draws a promoted span the same way). All are plain + * elements; the gutter's placement pass notices a removed block on its + * next run. */ function _removeAnnotationByDerivedId(derivedId: string): void { document.querySelectorAll( - `.span-text[data-span-id="${derivedId}"], .span-mark[data-span-id="${derivedId}"]`, + `.span-text[data-span-id="${derivedId}"], .span-mark.span-dot[data-span-id="${derivedId}"]`, ).forEach((el) => el.remove()); document.querySelectorAll( `.smell[data-smell-id="${derivedId}"]`, @@ -688,8 +662,7 @@ export const Comments = { renderAll, getAll, isPromoted, - openPromotionEditor, - promoteSmell, + promote, openBlockEditor, attachBlockThreads, }; diff --git a/semantic_code_review/viewer/assets/render.ts b/semantic_code_review/viewer/assets/render.ts index 63b0a76..f9f3c9e 100644 --- a/semantic_code_review/viewer/assets/render.ts +++ b/semantic_code_review/viewer/assets/render.ts @@ -2,7 +2,7 @@ // // Owns the layout pass that turns DATA into the on-page DOM: PR // panel, file blocks, hunk headers, the side-by-side row grid, gap -// chips for unchanged context, the centre gutter's span bars and text, the label tree +// chips for unchanged context, the span gutter's bars and text, the label tree // a collapsed fold shows, refs, smell pills. Carries the fold state too // (STATE.fold / overrides / renderedDiffs cache) because all of that // exists to feed the renderer, and binds the user inputs that drive it @@ -71,7 +71,7 @@ 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 +// Whether the span gutter is folded to its bars alone — 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 @@ -538,8 +538,8 @@ function _smellPill(smell: Smell, promotion?: SmellPromotion): HTMLElement { const body = smell.note ? `${smell.tag}: ${smell.note}` : smell.tag; - Comments.promoteSmell({ - ...promotion, body, smellId: promotion.smellId, + Comments.promote({ + derivedFrom: promotion.smellId, file: promotion.file, side: promotion.side, line: promotion.line, body, }); }); } @@ -1190,16 +1190,17 @@ function _renderHunkDiff(h: HunkBlock, file: FileBlock, scope: PaneScope): HTMLE return diff; } -// --- Spans on visible code: the centre gutter ------------------------------ +// --- Spans on visible code: the span gutter at the right edge -------------- // -// The right half's grid has four columns — line number, text, bars, code — -// 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. 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. +// The right half's grid has four columns — line number, code, bars, text — +// the last two sticky to the half's right edge so they read as a fixed +// strip at the diff's right edge while the code scrolls beneath. Every row +// carries an empty bars and text cell for the gutter's background; a span +// puts its marks in them. Every span takes one form: a mark in the bars +// column — a bar over its rows, one column nearer the code 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 @@ -1266,11 +1267,12 @@ const _SPAN_BAR_COLUMN_PX = 6; * 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. */ + * warned about and left out. One the reviewer has turned into a comment + * loses its text block and, on one line, its dot — the comment stands + * in their place — but keeps its bar: the bar marks a range the comment + * does not. 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( rowElsNew: HTMLElement[], rows: RowBlock[], spans: AnnotationSpan[], hunkId: string, filePath: string, @@ -1283,8 +1285,9 @@ function _attachSpans( const blocks: SpanTextBlock[] = []; const bars: SpanBar[] = []; for (const ps of placed) { - if (_spanPromoted(ps.span, hunkId)) continue; + const promoted = _spanPromoted(ps.span, hunkId); if (ps.first === ps.last) { + if (promoted) continue; _gutterBars(rowElsNew[ps.first]).appendChild(_spanMark(ps.span, ps.depth, "dot")); } else { for (let i = ps.first; i <= ps.last; i++) { @@ -1292,6 +1295,7 @@ function _attachSpans( _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 }); + if (promoted) continue; } const el = _el("div", "span-text"); el.dataset.spanId = ps.span.id; @@ -1303,7 +1307,9 @@ function _attachSpans( half.appendChild(el); blocks.push({ el, body, row: rowElsNew[ps.first] }); } - if (!blocks.length) return; + // A bar with no block still needs the pass: it runs through the + // non-code rows that arrive inside it. + if (!blocks.length && !bars.length) return; const gutter: SpanGutter = { blocks, bars, stretched: [], observer: null }; _SPAN_GUTTERS.set(half, gutter); _observeHalf(half, gutter); @@ -1406,9 +1412,9 @@ function _onGutterClick(e: MouseEvent): void { /** 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). */ +/** A row's bars cell (`children[2]`; `[3]` is its text cell). */ function _gutterBars(rowEl: HTMLElement): HTMLElement { - const cell = rowEl.children[3] as HTMLElement | undefined; + const cell = rowEl.children[2] as HTMLElement | undefined; if (!cell || !cell.classList.contains("cell-gutter-bars")) throw new Error("row has no gutter bars cell"); return cell; } @@ -1431,32 +1437,49 @@ function _spanTooltip(span: AnnotationSpan): string { 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 - * same observation under. */ +/** Whether the reviewer has already turned this span's intent 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 +/** One span's text: a pill row on the block's first line — beside the + * bar's start, since it describes the span — holding its smells as + * promotable pills and the affordance that promotes its intent, then the * 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); - } + const pills = _el("span", "span-text-pills"); + for (const sm of span.smells || []) pills.appendChild(_smellPill(sm, { + smellId: `${span.id}:smell:${sm.tag}`, file: filePath, side: "new", line: span.start, + })); + if (span.intent) pills.appendChild(_spanPromoteButton(span, filePath)); + el.appendChild(pills); el.appendChild(_el("span", span.intent ? "span-text-intent" : "span-text-intent empty", span.intent || "(no intent)")); return el; } +/** The affordance that turns a span's intent into a reviewer comment on + * the span's first line, by the one-click path the smell pills use; + * the saved comment is `derived_from` the span's id, which is what + * hides the block from then on. */ +function _spanPromoteButton(span: AnnotationSpan, filePath: string): HTMLElement { + const btn = _el("button", "span-promote", "+ comment") as HTMLButtonElement; + btn.type = "button"; + btn.title = `Add this intent as a comment on line ${span.start}`; + btn.addEventListener("click", (e) => { + e.stopPropagation(); + Comments.promote({ + derivedFrom: span.id, file: filePath, side: "new", line: span.start, body: span.intent, + }); + }); + return btn; +} + /** Re-run the placement pass whenever the half's rows change or its * size (or a text body's) does. One pair of observers per half; the * WeakMap entry outlives them. A row mutation runs the pass after the @@ -1646,13 +1669,13 @@ function _renderRow( const newRow = _el("div", `row row-${row.kind}`); newRow.appendChild(_renderLineno(row.new_line, "new", hasNew)); 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. - const text = _el("span", "cell-gutter-text"); + // The span gutter's cells, in column order after the content cell: + // bars against the code, text at the half's right edge. const bars = _el("span", "cell-gutter-bars"); - text.title = bars.title = _GUTTER_CELL_TITLE; - newRow.appendChild(text); + const text = _el("span", "cell-gutter-text"); + bars.title = text.title = _GUTTER_CELL_TITLE; newRow.appendChild(bars); + newRow.appendChild(text); return { old: oldRow, new: newRow }; } diff --git a/semantic_code_review/viewer/assets/viewer.css b/semantic_code_review/viewer/assets/viewer.css index 3770ff0..b9686e5 100644 --- a/semantic_code_review/viewer/assets/viewer.css +++ b/semantic_code_review/viewer/assets/viewer.css @@ -707,10 +707,11 @@ html.dragging-divider { cursor: col-resize; user-select: none; } /* --- Diff grid ----------------------------------------------------------- * Two halves side by side, each its own horizontal-scroll container and * its own grid: the old half lineno + content, the new half lineno + - * the centre gutter's two columns (span text, span bars) + content. The - * gutter columns are sticky, so with the lineno they read as a fixed strip - * between the halves while the code scrolls beneath. One shared scrollbar - * per side, zero mid-hunk scrollbars. + * content + the span gutter's two columns (span bars, span text). The + * gutter columns are sticky to the half's right edge, as the lineno is to + * its left, so they read as a fixed strip at the diff's right edge while + * the code scrolls beneath. One shared scrollbar per side, zero mid-hunk + * scrollbars. * * Vertical alignment across halves: code rows use a uniform line-height * and an enforced min-height on every cell, so every code row on both @@ -726,7 +727,8 @@ html.dragging-divider { cursor: col-resize; user-select: none; } * * 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` + * it and the code columns give up the same amount each; the two code + * viewports stay equal with the strip at the far right. `--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 @@ -776,36 +778,38 @@ html.dragging-divider { cursor: col-resize; user-select: none; } .diff-only-new .half-old, .diff-only-old .half-new { display: none; } -/* The new half's grid: number · text · bars · code — the bars against the - * code they mark, the rationale reading from the line number outward. - * Cells are placed by column, not DOM order — the gutter cells come after - * the content cell in the DOM so `children[1]` stays the content cell - * everywhere. */ +/* The new half's grid: number · code · bars · text — the line numbers on + * the left of each half, the two code columns beside each other, the + * bars against the code they mark and the rationale outside them. The + * cells follow the same order in the DOM, so `children[1]` is the content + * cell in both halves. The strip's two cells are sticky to the half's + * right edge: `right` offsets, the mirror of the lineno's `left: 0`. */ .diff .half-new { - grid-template-columns: 3.5em var(--gutter-text-w, 0px) var(--span-bars-w, 0px) minmax(max-content, 1fr); + grid-template-columns: 3.5em minmax(max-content, 1fr) var(--span-bars-w, 0px) var(--gutter-text-w, 0px); } .diff .cell-lineno { grid-column: 1; } -.diff .half-old .cell-content { grid-column: 2; } -.diff .half-new .cell-content { grid-column: 4; } +.diff .cell-content { grid-column: 2; } .diff .cell-gutter-bars, .diff .cell-gutter-text, .diff .span-text { position: sticky; z-index: 1; } .diff .cell-gutter-bars, .diff .cell-gutter-text { background: var(--bg-alt); } -.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(--gutter-text-w, 0px)); - box-shadow: inset -1px 0 var(--border); + right: var(--gutter-text-w, 0px); + box-shadow: inset 1px 0 var(--border); } +.diff .cell-gutter-text, .diff .span-text { grid-column: 4; right: 0; } /* 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. The mark is its column's full 6px for the row's - * height — the hover and click target; the stroke it draws is 2px. */ + * single-line span's row. The outermost span's column is the one against + * the text, and each level of nesting is one column further left, nearer + * the code; the cell's spare 4px sit between the innermost bar and the + * code. 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; top: 0; bottom: 0; - left: calc(var(--depth, 0) * 6px); + right: calc(var(--depth, 0) * 6px); width: 6px; cursor: pointer; } @@ -841,9 +845,9 @@ html.dragging-divider { cursor: col-resize; user-select: none; } white-space: normal; overflow-wrap: anywhere; } -/* The size lives on the body, not the block: the block's sticky `left` - * is in the half's `em` and must match the cells'. The body paints the - * strip's background itself, since it may lie over rows with none. */ +/* The size lives on the body, not the block: the block is a grid item of + * the half and sticks like the cells do. The body paints the strip's + * background itself, since it may lie over rows with none. */ .diff .span-text-body { margin: 0; padding: 2px 6px; @@ -854,29 +858,46 @@ html.dragging-divider { cursor: col-resize; user-select: none; } color: var(--fg-muted); } /* 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. */ + * without gutter cells; a sticky pseudo-item over the strip's two columns + * keeps its background continuous through the row, 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. The annotation cell takes the number and code columns + * only, so its box wraps before the strip rather than under it (a row + * wider than the half still lets the box run under the strip, as it ran + * past the half's edge before; scrolling brings it out). */ .diff .half-new .row-annotation::before, .diff .half-new .row-placeholder::before { content: ""; - grid-column: 1 / 4; + grid-column: 3 / 5; grid-row: 1; position: sticky; - left: 0; + right: 0; z-index: 1; visibility: visible; background: var(--bg-alt); - box-shadow: inset -1px 0 var(--border); + box-shadow: inset 1px 0 var(--border); } .diff .half-new .row-placeholder > .cell-gutter-bars { visibility: visible; } +.diff .half-new .cell-annotation { grid-column: 1 / 3; } .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 - * 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; } +/* The pill row leads the block, on its first line, so it sits beside the + * row the span starts on rather than wherever its intent ends: the smell + * pills, then the promote affordance at the row's far end — an outlined + * pill, dim until hovered, so it reads as a control and not a smell. */ +.diff .span-text-pills { display: flex; flex-wrap: wrap; align-items: center; gap: 3px; margin-bottom: 2px; } +.diff .span-text .smell { font-size: 10px; padding: 0 6px; margin-left: 0; } +.diff .span-promote { + margin-left: auto; + padding: 0 6px; + font: inherit; font-size: 10px; line-height: 14px; + border: 1px solid var(--border); border-radius: 10px; + background: transparent; color: var(--fg-dim); + cursor: pointer; + white-space: nowrap; +} +.diff .span-promote:hover { color: var(--accent); border-color: var(--accent); } /* 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 @@ -890,9 +911,10 @@ html.dragging-divider { cursor: col-resize; user-select: none; } grid-template-columns: subgrid; grid-column: 1 / -1; } -/* Every cell sits in the row's one track: the new half's cells are placed - * by column out of DOM order, and auto-placement would otherwise wrap a - * cell whose column precedes the previous one onto a second track. */ +/* Every cell sits in the row's one track: cells are placed by column, and + * the explicit row keeps auto-placement from opening a second track for a + * cell whose column precedes its DOM predecessor's — the bars cell the + * placement pass appends to an annotation row, for one. */ .diff .row > * { grid-row: 1; } .diff .cell { diff --git a/tests/js/viewer.test.ts b/tests/js/viewer.test.ts index b544784..1c79e71 100644 --- a/tests/js/viewer.test.ts +++ b/tests/js/viewer.test.ts @@ -1126,7 +1126,7 @@ describe("streaming events", () => { }); -describe("the centre gutter: spans on visible code (ADR 0008)", () => { +describe("the span gutter at the right edge: spans on visible code (ADR 0008)", () => { type Row = Record; const span = (id: string, start: number, end: number, intent: string, smells: unknown[] = []): Row => ({ id, start, end, intent, smells, context: "", refs: [] }); @@ -1203,12 +1203,12 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { await tick(); } - test("every new-half row carries the gutter's cells after its content cell", async () => { + test("every new-half row carries the gutter's cells after its content cell, bars then text", async () => { await bootViewer(makeData({ pending: false, files: [gutterFile([])], symbols: [] })); fold("code"); for (const row of newRows()) { expect(Array.from(row.children).map((c) => c.className.split(" ")[0])) - .toEqual(["cell", "cell", "cell-gutter-text", "cell-gutter-bars"]); + .toEqual(["cell", "cell", "cell-gutter-bars", "cell-gutter-text"]); expect(row.children[1].classList.contains("cell-content")).toBe(true); } // No span: the gutter is zero wide and no row is placed explicitly. @@ -1253,11 +1253,16 @@ describe("the centre gutter: spans on visible code (ADR 0008)", () => { 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 - // a long intent happens to end — and a span with none has no pill row. + // The pill row leads the block — beside the bar's first row, not + // wherever a long intent happens to end — with the smells, then the + // promote affordance; a span with no smells still has the row, for + // the affordance. 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(); + expect(Array.from(body.children).map((c) => c.className)).toEqual(["span-text-pills", "span-text-intent"]); + expect(Array.from(body.querySelector(".span-text-pills")!.children).map((c) => c.className.split(" ")[0])) + .toEqual(["smell", "span-promote"]); + expect(Array.from(textOf("H0_0:span:4-7")!.querySelector(".span-text-pills")!.children).map((c) => c.className)) + .toEqual(["span-promote"]); // 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 @@ -1606,6 +1611,149 @@ describe("LLM observation → comment promotion", () => { for (const row of document.querySelectorAll(".half-new .row")) expect(row.style.minHeight).toBe(""); }); + /** A nine-line file whose one hunk inserts lines 4..8, with `spans`. */ + function spanFile(spans: Array>): Record { + const rows = [4, 5, 6, 7, 8].map((n) => + ({ kind: "ins", old_line: null, new_line: n, old_text: "", new_text: `l${n}` })); + return { + id: "F0", path: "a.py", status: "modified", language: "python", + adds: 5, dels: 0, summary: "", head_line_count: 9, + symbols: { added: [], modified: [], removed: [] }, fold_regions: [], + hunks: [makeHunkBlock("H0_0", "adds five lines", { + old_start: 3, old_count: 0, new_start: 4, new_count: 5, rows, spans, + })], + }; + } + const span = (id: string, start: number, end: number, intent: string, smells: unknown[] = []): Record => + ({ id, start, end, intent, smells, context: "", refs: [] }); + /** Capture the next POST /comments body. */ + function capturePost(): () => Record | null { + 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); + return () => posted; + } + /** The kinds of the marks on code rows for `spanId` (a thread row's + * segment, added by the placement pass, is checked on its own). */ + const markKinds = (spanId: string): string[] => + Array.from(document.querySelectorAll(`.row:not(.row-annotation) > .cell-gutter-bars > .span-mark[data-span-id="${spanId}"]`)) + .map((m) => Array.from(m.classList).find((c) => c.startsWith("span-") && c !== "span-mark")!); + + test("every block's pill row carries a promote affordance for its intent; a span with no intent has none", async () => { + window.location.hash = "#fold=code"; + await bootViewer(makeData({ pending: false, files: [spanFile([ + span("H0_0:span:4-7", 4, 7, "a multi-line span", [{ tag: "dead-code", note: "" }]), + span("H0_0:span:5-6", 5, 6, "a nested one, no smells"), + span("H0_0:span:8-8", 8, 8, "a callout"), + span("H0_0:span:7-7", 7, 7, "", []), + ])] })); + await new Promise((r) => setTimeout(r, 0)); + const btn = (id: string): HTMLButtonElement | null => + document.querySelector(`.span-text[data-span-id="${id}"] .span-text-pills > .span-promote`); + for (const id of ["H0_0:span:4-7", "H0_0:span:5-6", "H0_0:span:8-8"]) { + const b = btn(id)!; + expect(b, id).not.toBeNull(); + expect(b.type).toBe("button"); + expect(b.title).toContain(`line ${id.split(":")[2].split("-")[0]}`); + } + // Nothing to promote: the block shows "(no intent)" and no button. + expect(document.querySelector('.span-text[data-span-id="H0_0:span:7-7"] .span-text-intent.empty')).not.toBeNull(); + expect(btn("H0_0:span:7-7")).toBeNull(); + }); + + test("promoting a multi-line span's intent saves a comment on its first line, hides its block and keeps its bar", async () => { + window.location.hash = "#fold=code"; + await bootViewer(makeData({ pending: false, files: [spanFile([ + span("H0_0:span:4-7", 4, 7, "the outer edit", [{ tag: "dead-code", note: "" }]), + span("H0_0:span:6-7", 6, 7, "the inner edit"), + ])] })); + await new Promise((r) => setTimeout(r, 0)); + expect(markKinds("H0_0:span:4-7")).toEqual(["span-bar-top", "span-bar", "span-bar", "span-bar-bottom"]); + const posted = capturePost(); + + document.querySelector('.span-text[data-span-id="H0_0:span:4-7"] .span-promote')!.click(); + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + + const c = posted()!; + expect(c).not.toBeNull(); + expect(c.body).toBe("the outer edit"); + expect(c.derived_from).toBe("H0_0:span:4-7"); + expect(c.file).toBe("a.py"); + expect(c.side).toBe("new"); + expect(c.line).toBe(4); + // The block (pills and intent) is gone; the bar still marks the range + // the comment, on one line, does not; the thread sits under line 4. + expect(document.querySelector('.span-text[data-span-id="H0_0:span:4-7"]')).toBeNull(); + expect(markKinds("H0_0:span:4-7")).toEqual(["span-bar-top", "span-bar", "span-bar", "span-bar-bottom"]); + const row4 = Array.from(document.querySelectorAll(".half-new .row")) + .find((r) => r.querySelector(".cell-lineno")!.textContent === "4")!; + const thread = row4.nextElementSibling as HTMLElement; + expect(thread.classList.contains("annot-comment")).toBe(true); + expect(thread.querySelector(':scope > .cell-gutter-bars > .span-mark.span-bar[data-span-id="H0_0:span:4-7"]')).not.toBeNull(); + // The nested span is untouched. + expect(document.querySelector('.span-text[data-span-id="H0_0:span:6-7"]')).not.toBeNull(); + expect(markKinds("H0_0:span:6-7")).toEqual(["span-bar-top", "span-bar-bottom"]); + }); + + test("promoting a single-line span's intent removes its dot with its block", async () => { + window.location.hash = "#fold=code"; + await bootViewer(makeData({ pending: false, files: [spanFile([span("H0_0:span:5-5", 5, 5, "a callout")])] })); + await new Promise((r) => setTimeout(r, 0)); + expect(markKinds("H0_0:span:5-5")).toEqual(["span-dot"]); + const posted = capturePost(); + document.querySelector('.span-text[data-span-id="H0_0:span:5-5"] .span-promote')!.click(); + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + expect(posted()!.line).toBe(5); + expect(posted()!.derived_from).toBe("H0_0:span:5-5"); + expect(document.querySelector('[data-span-id="H0_0:span:5-5"]')).toBeNull(); + }); + + test("a multi-line span promoted in an earlier session draws its bar and no block, on load and after a re-augment", async () => { + window.location.hash = "#fold=code"; + const spans = [ + span("H0_0:span:4-7", 4, 7, "the outer edit", [{ tag: "dead-code", note: "" }]), + span("H0_0:span:6-7", 6, 7, "the inner edit"), + ]; + await bootViewer(makeData({ pending: false, files: [spanFile(spans)] }), { + comments: [{ + id: "local-1", file: "a.py", side: "new", line: 4, + body: "the outer edit", created_at: 1, updated_at: 1, + source: "local", derived_from: "H0_0:span:4-7", + }], + }); + await new Promise((r) => setTimeout(r, 0)); + const check = (): void => { + expect(document.querySelector('.span-text[data-span-id="H0_0:span:4-7"]')).toBeNull(); + expect(markKinds("H0_0:span:4-7")).toEqual(["span-bar-top", "span-bar", "span-bar", "span-bar-bottom"]); + expect(document.querySelector('.span-text[data-span-id="H0_0:span:6-7"]')).not.toBeNull(); + }; + check(); + expect(document.querySelector('.comment-thread-entry[data-comment-id="local-1"]')).not.toBeNull(); + // The bar runs through the thread row under line 4, as any bar does. + const row4 = Array.from(document.querySelectorAll(".half-new .row")) + .find((r) => r.querySelector(".cell-lineno")!.textContent === "4")!; + const thread = row4.nextElementSibling as HTMLElement; + expect(thread.classList.contains("annot-comment")).toBe(true); + expect(thread.querySelector(':scope > .cell-gutter-bars > .span-mark[data-span-id="H0_0:span:4-7"]')).not.toBeNull(); + // A re-augment rebuilds the hunk with the comments known: the + // renderer itself leaves the block out and draws the bar. + lastEventSource().dispatch("hunk", { + file_idx: 0, hunk_idx: 0, ok: true, + block: makeHunkBlock("H0_0", "re-run", { + old_start: 3, old_count: 0, new_start: 4, new_count: 5, spans, + rows: [4, 5, 6, 7, 8].map((n) => ({ kind: "ins", old_line: null, new_line: n, old_text: "", new_text: `l${n}` })), + }), + }); + await new Promise((r) => setTimeout(r, 0)); + check(); + }); + test("a comment store written before spans hides the span under its line_note id", async () => { window.location.hash = "#fold=code"; const spans = [