From ceeafbaa59215a91c389d9489ab1e512244d8530 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 19 Oct 2026 03:24:27 +0000 Subject: [PATCH 1/8] fix(desktop): minimize inline comments to one chip per line, as stage-cli does Minimize inline comments folded each open thread to its head row, and left resolved and outdated threads as they were, since those already stand folded; on a pull request whose threads were resolved it changed nothing to see. It now means what stage-cli's means: each line's threads draw as one chip at the end of the line, in a row of no height, so the diff reads as if the line held no comments. The chip names who wrote on the line, how many comments, and how many are open; pressing it shows or hides the line's threads, and the tab's find reads them and opens the line it brings into view. I turns the setting on and off inside Files changed. Co-authored-by: luke-agent[bot] <339870089+luke-agent[bot]@users.noreply.github.com> --- apps/desktop/src/renderer/styles/tasks.css | 69 ++++++++ .../tasks/agent-pull-request-tab.test.ts | 17 +- .../panel/src/tasks/agent-changes-tab.test.ts | 63 ++++++- .../panel/src/tasks/agent-changes-tab.tsx | 160 ++++++++++++++++-- .../src/tasks/agent-pull-request-tab.tsx | 12 +- packages/panel/src/tasks/changes-list.tsx | 9 +- .../panel/src/tasks/minimized-threads.tsx | 99 +++++++++++ .../panel/src/tasks/pull-request-people.tsx | 2 +- 8 files changed, 403 insertions(+), 28 deletions(-) create mode 100644 packages/panel/src/tasks/minimized-threads.tsx diff --git a/apps/desktop/src/renderer/styles/tasks.css b/apps/desktop/src/renderer/styles/tasks.css index aae59720a3..f32a3414a5 100644 --- a/apps/desktop/src/renderer/styles/tasks.css +++ b/apps/desktop/src/renderer/styles/tasks.css @@ -2414,6 +2414,75 @@ white-space: normal; } +/* A line's review threads minimized to one chip (minimized-threads.tsx), as + stage-cli draws them: the chip, its fold's summary, floats up over the end + of the line in a row of no height while closed, so the diff reads as if the + line held no comments, and a note holding the fold alone keeps no padding + of its own. Open, the chip stands at the end of its own row over the + threads it shows. */ +.diff-note:has(> .diff-thread-chip-row:only-child:not([open])) { + padding-block: 0; +} + +.diff-thread-chip-row { + position: relative; +} + +.diff-thread-chip-row:not([open]) { + height: 0; +} + +.diff-thread-chip { + display: flex; + align-items: center; + gap: var(--luke-icon-gap); + width: fit-content; + margin-inline-start: auto; + padding: var(--luke-space-2) var(--luke-space-6); + border: 1px solid var(--luke-color-border); + border-radius: var(--luke-control-radius); + background: var(--luke-color-surface); + color: var(--luke-color-text-muted); + font-size: var(--luke-type-caption-size); +} + +.diff-thread-chip:hover { + color: var(--luke-color-text); +} + +.diff-thread-chip-row:not([open]) > .diff-thread-chip { + position: absolute; + right: 0; + bottom: var(--luke-space-2); +} + +.diff-thread-chip-row[open] > .diff-thread-chip { + margin-bottom: var(--luke-space-4); +} + +.diff-thread-chip > svg { + width: var(--luke-space-12); + height: var(--luke-space-12); +} + +.diff-thread-chip-faces { + display: inline-flex; +} + +.diff-thread-chip-faces > .actor-mark + .actor-mark { + margin-inline-start: calc(-1 * var(--luke-space-4)); +} + +.diff-thread-chip-resolved { + color: var(--luke-color-complete); +} + +.diff-thread-chip-threads { + display: flex; + flex-direction: column; + gap: var(--luke-space-6); +} + .diff-inline-thread { --luke-focus-ring-offset: var(--luke-focus-ring-inset); max-width: 720px; diff --git a/apps/desktop/src/renderer/tasks/agent-pull-request-tab.test.ts b/apps/desktop/src/renderer/tasks/agent-pull-request-tab.test.ts index a64aeb8511..7bce5e604c 100644 --- a/apps/desktop/src/renderer/tasks/agent-pull-request-tab.test.ts +++ b/apps/desktop/src/renderer/tasks/agent-pull-request-tab.test.ts @@ -2863,7 +2863,7 @@ test("the title's edit is kept as the focus leaves its field, as it is on Enter" assert.equal(tab.querySelector('[aria-label="Pull request title"]'), null, "the field closes"); }); -test("Files changed draws each review thread under its line, its comments through the overview's own drawing, folded while Minimize inline comments is on", async () => { +test("Files changed draws each review thread under its line, its comments through the overview's own drawing, and while Minimize inline comments is on, as one chip at the line's end that shows it", async () => { const review = { kind: CONVERSATION_ENTRY.REVIEW, by: CHARLES_ON_GITHUB, @@ -2928,7 +2928,20 @@ test("Files changed draws each review thread under its line, its comments throug ); assert.equal(note.querySelector("details")?.open, true, "a current thread stands open"); await minimize(); - assert.equal(note.querySelector("details")?.open, false, "minimized, the thread stands folded"); + const chip = note.querySelector(".diff-thread-chip") ?? assert.fail("a chip stands"); + assert.equal(chip.getAttribute("aria-label"), "1 comment by Charles Pan · 1 open, show"); + assert.equal( + note.querySelector(".pull-request-thread-comment"), + null, + "minimized, no thread is drawn", + ); + act(() => chip.click()); + await settle(); + assert.ok(note.querySelector(".pull-request-thread-comment"), "the chip shows the line's thread"); + assert.equal( + note.querySelector(".diff-thread-chip")?.getAttribute("aria-label"), + "1 comment by Charles Pan · 1 open, hide", + ); }); test("a review thread on its line in Files changed is replied on and resolved from under its comments, as in the Conversation", async () => { diff --git a/packages/panel/src/tasks/agent-changes-tab.test.ts b/packages/panel/src/tasks/agent-changes-tab.test.ts index 2c5fc4d62b..c704cff63b 100644 --- a/packages/panel/src/tasks/agent-changes-tab.test.ts +++ b/packages/panel/src/tasks/agent-changes-tab.test.ts @@ -23,6 +23,7 @@ import { import { ACT_KIND, ACT_OUTCOME_STATUS, type Act, type ActOutcome } from "@luke/host/acts"; import { DIFF_LAYOUT } from "@luke/settings/diff-view"; import { APP_COMMAND } from "@luke/settings/shortcuts"; +import { DisclosureFind } from "@luke/ui"; import { Effect } from "effect"; import * as AtomRegistry from "effect/reactivity/AtomRegistry"; import { act, createElement, type ReactElement } from "react"; @@ -33,6 +34,7 @@ import { useAppCommand, useAppKeymap } from "../app-commands.js"; import type { SideBundles } from "../bundle-loader.js"; import type { DiffListProps, DiffViewModule } from "../diff-view/contract.js"; import type { FileTreeModule } from "../file-tree/contract.js"; +import { findRanges, reveal } from "../find-matches.js"; import { fakeAppBridge, fakeAppBridgeCalls } from "../testing/app-bridge.js"; import { handSettings } from "../testing/app-state.js"; import { onRegistry } from "../testing/copy-feedback.js"; @@ -231,7 +233,12 @@ let registry = AtomRegistry.make(); /** Draws the view again in the root standing, its tree open or not, beside whatever else is given. */ async function redraw( - options: { threads?: ChangesThreads; treeOpen?: boolean; beside?: ReactElement } = {}, + options: { + threads?: ChangesThreads; + treeOpen?: boolean; + beside?: ReactElement; + finding?: boolean; + } = {}, ): Promise { const { AgentChangesTab } = await import("./agent-changes-tab.js"); const tab = createElement(AgentChangesTab, { @@ -246,7 +253,13 @@ async function redraw( root?.render( onRegistry( registry, - onSideBundles(bundles, createElement("div", null, tab, options.beside ?? null)), + onSideBundles( + bundles, + createElement(DisclosureFind, { + finding: options.finding ?? false, + children: createElement("div", null, tab, options.beside ?? null), + }), + ), ), ), ); @@ -256,7 +269,12 @@ async function redraw( /** Draws the agent's Files changed view afresh, its read landed, and answers what the list was last drawn with. */ async function drawTab( - options: { threads?: ChangesThreads; treeOpen?: boolean; beside?: ReactElement } = {}, + options: { + threads?: ChangesThreads; + treeOpen?: boolean; + beside?: ReactElement; + finding?: boolean; + } = {}, ): Promise { act(() => root?.unmount()); document.body.innerHTML = ""; @@ -1371,3 +1389,42 @@ test("a refused mark leaves a fold chosen since, as Collapse all chose it", asyn ["b.ts", true], ]); }); + +test("I turns Minimize inline comments on and off inside Files changed, each line's threads then one chip that a find still reads and opens", async () => { + const threads: ChangesThreads = { + list: [seatsThread(2, "why-42")], + draw: (thread) => createElement("p", { className: "drawn-thread" }, thread.comments[0]?.body), + }; + await drawTab({ threads }); + const note = () => lastDrawn().renderAnnotation("src/seats.ts", `${PAGE}#why-42`); + assert.equal(note().querySelector(".drawn-thread")?.textContent, "why-42"); + + press(listBox(), { key: "i" }); + await flush(); + const chip = + note().querySelector(".diff-thread-chip") ?? assert.fail("a chip stands"); + assert.equal(chip.getAttribute("aria-label"), "1 comment by octo · 1 open, show"); + assert.equal(note().querySelector(".drawn-thread"), null, "minimized, the thread is put away"); + press(document.body, { key: "i" }); + await flush(); + assert.ok(note().querySelector(".diff-thread-chip"), "outside the view, I is not the view's"); + + // A find looking through the tab reads the put-away thread's words, and opens the line it brings into view. + await redraw({ threads, finding: true }); + const slot = note(); + document.body.append(slot); + const [match] = findRanges(slot, "why-42"); + assert.ok(match, "the find reads the minimized thread"); + // Note that jsdom lays nothing out, so a range is measured where the find scrolls it into view as standing nowhere. + Range.prototype.getBoundingClientRect = () => new DOMRect(); + act(() => reveal(match, slot)); + await flush(); + assert.equal( + note().querySelector(".diff-thread-chip")?.getAttribute("aria-label"), + "1 comment by octo · 1 open, hide", + ); + + press(listBox(), { key: "i" }); + await flush(); + assert.equal(note().querySelector(".diff-thread-chip"), null, "I again draws every thread whole"); +}); diff --git a/packages/panel/src/tasks/agent-changes-tab.tsx b/packages/panel/src/tasks/agent-changes-tab.tsx index 665df63ef9..c0660d036a 100644 --- a/packages/panel/src/tasks/agent-changes-tab.tsx +++ b/packages/panel/src/tasks/agent-changes-tab.tsx @@ -38,7 +38,7 @@ import { ImageIcon, RotateCwIcon, } from "lucide-react"; -import { type ReactNode, useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { Fragment, type ReactNode, useCallback, useEffect, useMemo, useRef, useState } from "react"; import { appPlatform } from "../app-platform.js"; import { useCopyToast } from "../copy-feedback.js"; import { @@ -82,6 +82,7 @@ import { FileViewBar, usePaneRoom } from "./file-view.js"; import { useFileViewSettings } from "./file-view-settings.js"; import { ImageDiff, type ImageSides, imageDrawable } from "./image-diff.js"; import { LineComment, type PendingReview } from "./line-comment.js"; +import { MinimizedThreads } from "./minimized-threads.js"; import { PullRequestMark } from "./pull-request-mark.js"; import { OpenedBy, ReviewersLine } from "./pull-request-people.js"; import { PullRequestWriteNote } from "./pull-request-writes.js"; @@ -322,6 +323,62 @@ function AboveThreads({ ); } +/** Which of a file's lines show their threads while Minimize inline comments is on, and the door that shows or hides one line's. */ +interface FileMinimized { + readonly shown: (side: DiffSelection["side"], line: number) => boolean; + readonly show: (side: DiffSelection["side"], line: number, shown: boolean) => void; +} + +/** + * A file's threads on their lines while Minimize inline comments is on, as + * stage-cli draws them: each line's threads as one chip at the line's end + * (`MinimizedThreads`), standing in a row of no height, and under it, once + * the chip shows them, the threads as they are drawn otherwise. A line a + * comment is being written on always shows its threads, since the comment + * stands among them. + */ +function minimizedNotes( + placed: ReturnType["placed"], + draw: ChangesThreads["draw"] | undefined, + minimized: FileMinimized, + comment: DiffNote | undefined, +): DiffNote[] { + const lines = new Map>(); + for (const each of placed) { + const side = lines.get(each.side) ?? new Map(); + lines.set(each.side, side.set(each.line, [...(side.get(each.line) ?? []), each])); + } + return [...lines].flatMap(([side, onSide]) => + [...onSide].flatMap(([line, threads]) => { + const drawn = threads.map((each) => ( + {draw?.(each.thread, THREAD_PLACE.ON_LINE)} + )); + const [first] = threads; + if (first === undefined) return []; + // A line a comment is being written on shows its threads as they are, the comment among them. + const writing = comment?.side === side && comment.line === line; + return writing + ? threads.map((each, at) => ({ key: each.key, side, line, content: drawn[at] })) + : [ + { + key: first.key, + side, + line, + content: ( + each.thread)} + shown={minimized.shown(side, line)} + onShown={(shown) => minimized.show(side, line, shown)} + > + {drawn} + + ), + }, + ]; + }), + ); +} + /** * A file of the list as the bundle is handed it: its patch, its fold, its * header, and its notes: each review thread under its line, and those its @@ -341,6 +398,8 @@ function entryOf( draw: ChangesThreads["draw"] | undefined, images: ImageSides | undefined, comment: DiffNote | undefined, + /** Which of the file's lines show their threads while Minimize inline comments is on, and the door that shows or hides one; none while it is off. */ + minimized: FileMinimized | undefined, ): DiffListEntry { const path = file.path; const image = imageTypeOfPath(path) === undefined || images === undefined ? undefined : images; @@ -350,12 +409,29 @@ function entryOf( // Note that a pictured file draws no lines, so its threads stand above it, as on a binary file. const drawnAs = pictured === undefined ? file : { ...file, patch: null }; const { placed, above } = threadsOn(threads, drawnAs); - const notes: DiffNote[] = placed.map((each) => ({ - key: each.key, - side: each.side, - line: each.line, - content: draw?.(each.thread, THREAD_PLACE.ON_LINE), - })); + const notes: DiffNote[] = + minimized === undefined + ? placed.map((each) => ({ + key: each.key, + side: each.side, + line: each.line, + content: draw?.(each.thread, THREAD_PLACE.ON_LINE), + })) + : minimizedNotes(placed, draw, minimized, comment); + // The threads above the diff, as one list, or minimized to a chip of their own. + const aboveOf = (unplaced: typeof above, side: DiffSelection["side"]) => { + const list = ; + if (minimized === undefined) return list; + return ( + each.thread)} + shown={minimized.shown(side, FILE_NOTE_LINE)} + onShown={(shown) => minimized.show(side, FILE_NOTE_LINE, shown)} + > + {list} + + ); + }; // A pictured image, or a file with no hunks, stands as what is drawn in the place of its lines. const notice = fileNotice(file); const lead = @@ -372,7 +448,7 @@ function entryOf( content: ( <> {lead} - {above.length === 0 ? null : } + {above.length === 0 ? null : aboveOf(above, DIFF_SELECTION_SIDE.ADDITIONS)} ), }); @@ -384,7 +460,7 @@ function entryOf( key: ABOVE_KEY[side], side, line: FILE_NOTE_LINE, - content: , + content: aboveOf(unplaced, side), }); } if (comment !== undefined) notes.push(comment); @@ -462,18 +538,20 @@ function useEntries( threads: ChangesThreads | undefined, images: ImageSides | undefined, commenting: ListCommenting | undefined, + minimizedOf: (path: string) => FileMinimized | undefined, ): readonly DiffListEntry[] { const kept = useRef(new Map()); const next = new Map(); const entries = list.kept.map((file) => { const own = threads?.list.filter((thread) => thread.path === file.path) ?? []; const comment = commenting?.note?.path === file.path ? commenting.note.content : undefined; - const inputs = entryInputs(file, list, own, threads?.draw, images, comment); + const minimized = minimizedOf(file.path); + const inputs = [...entryInputs(file, list, own, threads?.draw, images, comment), minimized]; const was = kept.current.get(file.path); const entry = was !== undefined && sameInputs(was.inputs, inputs) ? was.entry - : entryOf(file, list, own, threads?.draw, images, comment); + : entryOf(file, list, own, threads?.draw, images, comment, minimized); next.set(file.path, { inputs, entry }); return entry; }); @@ -481,6 +559,57 @@ function useEntries( return entries; } +/** The lines that show their threads while Minimize inline comments is on: by file, by side, by line. */ +type ShownLines = ReadonlyMap>>; + +/** No line showing its threads. */ +const NONE_SHOWN: ShownLines = new Map(); + +/** + * Which lines show their threads while Minimize inline comments is on, + * each file's answer the same object while none of its lines was shown or + * hidden since, so the files whose lines did not change keep their entries; + * none while it is off, and every line minimized again each time it is + * turned on, as stage-cli does. + */ +function useMinimizedLines(minimize: boolean): (path: string) => FileMinimized | undefined { + const [shown, setShown] = useState<{ readonly of: boolean; readonly lines: ShownLines }>({ + of: minimize, + lines: NONE_SHOWN, + }); + const lines = shown.of === minimize ? shown.lines : NONE_SHOWN; + const toggle = useCallback( + (path: string, side: DiffSelection["side"], line: number, open: boolean) => + setShown((was) => { + const all = was.of === minimize ? was.lines : NONE_SHOWN; + const sides = all.get(path) ?? new Map>(); + const onSide = new Set(sides.get(side)); + if (open) onSide.add(line); + else onSide.delete(line); + return { of: minimize, lines: new Map(all).set(path, new Map(sides).set(side, onSide)) }; + }), + [minimize], + ); + const kept = useRef( + new Map< + string, + { readonly of: unknown; readonly toggle: unknown; readonly answer: FileMinimized } + >(), + ); + return (path) => { + if (!minimize) return undefined; + const own = lines.get(path); + const held = kept.current.get(path); + if (held !== undefined && held.of === own && held.toggle === toggle) return held.answer; + const answer: FileMinimized = { + shown: (side, line) => own?.get(side)?.has(line) === true, + show: (side, line, open) => toggle(path, side, line, open), + }; + kept.current.set(path, { of: own, toggle, answer }); + return answer; + }; +} + /** The key a comment being written stands under its lines by; a review thread's note is keyed by its page, which this never is. */ const COMMENT_KEY = "line-comment"; @@ -657,7 +786,9 @@ function FilesList({ boxRef: (element: HTMLDivElement | null) => void; }): React.JSX.Element { const look = useDiffLook(roomy); - const entries = useEntries(list, threads, images, commenting); + const { settings } = useFileViewSettings(); + const minimizedOf = useMinimizedLines(settings.diffMinimizeComments); + const entries = useEntries(list, threads, images, commenting, minimizedOf); const query = list.query.trim(); return (
@@ -801,7 +932,10 @@ export function AgentChangesTab({ control.readPullRequestViewed, pullRequest === null ? undefined : reads.current.count, ); - const list = useChangesList(changed.files, viewedFiles.marks); + const viewSettings = useFileViewSettings(); + const minimize = () => + viewSettings.change("diffMinimizeComments", !viewSettings.settings.diffMinimizeComments); + const list = useChangesList(changed.files, viewedFiles.marks, minimize); const answered = reads.current.count; // Note that the images are named afresh only with an answer, because every changed image's entry is drawn from them. const images = useMemo( diff --git a/packages/panel/src/tasks/agent-pull-request-tab.tsx b/packages/panel/src/tasks/agent-pull-request-tab.tsx index 42701a0408..10523867fc 100644 --- a/packages/panel/src/tasks/agent-pull-request-tab.tsx +++ b/packages/panel/src/tasks/agent-pull-request-tab.tsx @@ -103,7 +103,6 @@ import { useConversationFilters, } from "./conversation-filters.js"; import { PierreDiff, useDiffLook } from "./file-diff.js"; -import { useFileViewSettings } from "./file-view-settings.js"; import type { PendingReview } from "./line-comment.js"; import { blockedWords, MergeActions } from "./merge-box.js"; import { @@ -966,9 +965,9 @@ function ThreadComments({ /** * A review thread drawn on its line in Files changed, as GitHub's own diff * draws one: whether it was resolved or a later push outdated it, at its - * head, which folds it closed until opened, as Minimize inline comments - * folds every one; then its comments. An outdated one, whose line is gone, - * names the line it was made on. + * head, which folds it closed until opened; then its comments. An outdated + * one, whose line is gone, names the line it was made on. Minimize inline + * comments draws a line's threads as a chip instead (`minimized-threads.tsx`). */ function InlineThread({ thread, @@ -982,15 +981,12 @@ function InlineThread({ now: number; picture: GitHubPictureDrawer; }): React.JSX.Element { - const { settings } = useFileViewSettings(); const labels = threadLabels(thread); const lines = place === THREAD_PLACE.ABOVE ? threadLines(thread) : undefined; - const folded = labels.length > 0 || settings.diffMinimizeComments; const first = thread.comments[0]; return (
- {/* Note that the fold is drawn anew as the setting changes, so minimizing folds every thread at once. */} - + {first === undefined ? null : } {lines === undefined ? null : {lines}} diff --git a/packages/panel/src/tasks/changes-list.tsx b/packages/panel/src/tasks/changes-list.tsx index 80a9252076..ba3b6d69cc 100644 --- a/packages/panel/src/tasks/changes-list.tsx +++ b/packages/panel/src/tasks/changes-list.tsx @@ -30,7 +30,8 @@ import type { DiffListDoor } from "./diff-list.js"; * next and the previous file, `;` collapses or expands the file stood on, * ⇧; collapses every file the filter keeps, or expands them all once all * are collapsed, V marks the file stood on viewed on GitHub, or unmarks it, - * and steps on to the next, and Escape lets go of the file stood on. They answer only + * and steps on to the next, I turns Minimize inline comments on or off, as + * stage-cli's does, and Escape lets go of the file stood on. They answer only * where the press lands inside Files changed and is not typing, and go no * further once answered, so J and K keep their meaning everywhere else in * the window (the Inbox's next and previous item, `@luke/settings/shortcuts`), an @@ -57,6 +58,7 @@ const FILE_KEY = { TOGGLE_ALL: "toggle-all", LET_GO: "let-go", VIEWED: "viewed", + COMMENTS: "comments", } as const; type FileKey = (typeof FILE_KEY)[keyof typeof FILE_KEY]; @@ -102,6 +104,7 @@ function fileKeyOf(event: React.KeyboardEvent, standing: boolean): FileKey | und const key = event.key.toLowerCase(); if (key === "j") return FILE_KEY.NEXT; if (key === "v") return FILE_KEY.VIEWED; + if (key === "i") return FILE_KEY.COMMENTS; return key === "k" ? FILE_KEY.PREVIOUS : undefined; } @@ -174,6 +177,8 @@ export interface ChangesList { export function useChangesList( files: readonly CodingAgentChangedFile[], marks?: ViewedMarks | undefined, + /** Turns Minimize inline comments on or off, as I does; none leaves I to the window. */ + onMinimize?: (() => void) | undefined, ): ChangesList { const [query, onQuery] = useState(""); const [chosen, setChosen] = useState>(new Map()); @@ -285,10 +290,12 @@ export function useChangesList( if (key === undefined || current === undefined) return; // V is the list's only while the marks are read, so there is something to mark. if (key === FILE_KEY.VIEWED && marks === undefined) return; + if (key === FILE_KEY.COMMENTS && onMinimize === undefined) return; // Note that an answered press goes no further, because the window's keymap would run the Inbox's J and K too. event.preventDefault(); event.stopPropagation(); if (key === FILE_KEY.LET_GO) return letGo(); + if (key === FILE_KEY.COMMENTS) return onMinimize?.(); if (key === FILE_KEY.TOGGLE_ALL) return toggleAll(); if (key === FILE_KEY.VIEWED) { // The file marked folds, and the keys step on to the next, so pressing V again marks it. diff --git a/packages/panel/src/tasks/minimized-threads.tsx b/packages/panel/src/tasks/minimized-threads.tsx new file mode 100644 index 0000000000..4c24ef7f19 --- /dev/null +++ b/packages/panel/src/tasks/minimized-threads.tsx @@ -0,0 +1,99 @@ +import type { ReviewThread } from "@luke/features/coding-agent-wire"; +import { actorName } from "@luke/ui/actor-model"; +import { CircleCheckIcon, CircleIcon } from "lucide-react"; +import type { ReactNode } from "react"; +import { ACTOR_SIZE, ActorMark } from "../actor-ref.js"; +import { Fold, FoldBody, FoldSummary } from "../ai-elements/fold.js"; +import { Tooltip } from "../tooltip.js"; +import { useNamer } from "./pull-request-people.js"; + +/** + * minimized-threads.tsx -- a line's review threads drawn small while Minimize inline comments is on, as stage-cli draws them: one chip at the end of the line, the faces of who wrote on it, how many comments, and whether every thread is resolved, which shows the threads or puts them away. + * + * The chip stands in a row of no height of its own, floated up over the end + * of the line it is on, so the diff reads as if the line held no comments; + * the threads, once shown, are drawn under the chip as they are while + * nothing is minimized. Shown or put away, the line is told the view + * (`onShown`), which keeps it so while the setting stands. + */ + +/** How many faces a chip draws before the rest go uncounted, as stage-cli's does. */ +const CHIP_FACES = 4; + +/** What a chip says of a line's threads, and what pressing it does. */ +const CHIP_WORDS = { + comments: (count: number) => (count === 1 ? "1 comment" : `${count} comments`), + open: (count: number) => (count === 0 ? "all resolved" : `${count} open`), + SHOW: "show", + HIDE: "hide", + SHOW_HINT: "Show comments", + HIDE_HINT: "Hide comments", +} as const; + +/** Everyone who wrote on the threads, each once, in the order they first wrote. */ +function writersOf(threads: readonly ReviewThread[]) { + const seen = new Map(); + for (const comment of threads.flatMap((thread) => thread.comments)) { + if (!seen.has(comment.by.login)) seen.set(comment.by.login, comment.by); + } + return [...seen.values()]; +} + +/** + * One line's threads minimized: the chip, a fold's summary, which says who + * wrote on them, how many comments, and how many are still open, said whole + * to a screen reader with what pressing it does ("3 comments by dana, sam · + * 1 open, show"); and the threads it folds, drawn under it once shown. Note + * that a fold is what lets the tab's find reach the threads while they are + * put away, and open the line whose words it brings into view. + */ +export function MinimizedThreads({ + threads, + shown, + onShown, + children, +}: { + threads: readonly ReviewThread[]; + /** Whether the threads stand under the chip. */ + shown: boolean; + /** Told the threads were shown or put away, by the chip or by the find. */ + onShown: (shown: boolean) => void; + /** The threads as they are drawn while nothing is minimized. */ + children: ReactNode; +}): React.JSX.Element { + const name = useNamer(); + const writers = writersOf(threads).map(name); + const count = threads.reduce((sum, thread) => sum + thread.comments.length + thread.more, 0); + const open = threads.filter((thread) => !thread.resolved).length; + const label = [ + `${CHIP_WORDS.comments(count)} by ${writers.map((writer) => actorName(writer)).join(", ")}`, + `${CHIP_WORDS.open(open)}, ${shown ? CHIP_WORDS.HIDE : CHIP_WORDS.SHOW}`, + ].join(" · "); + return ( + { + if (event.currentTarget.open !== shown) onShown(event.currentTarget.open); + }} + > + + + {open === 0 ? ( + + + {children} + + ); +} diff --git a/packages/panel/src/tasks/pull-request-people.tsx b/packages/panel/src/tasks/pull-request-people.tsx index e62ccb68ea..4f1578e108 100644 --- a/packages/panel/src/tasks/pull-request-people.tsx +++ b/packages/panel/src/tasks/pull-request-people.tsx @@ -51,7 +51,7 @@ const REVIEW_GLYPHS = { } as const satisfies Record; /** Names the people as the members around the caller say. */ -function useNamer(): (person: GitHubPerson) => Actor { +export function useNamer(): (person: GitHubPerson) => Actor { const { members, readerId, former } = useMemberDirectory(); return (person) => githubActor(person, { members, readerId, former }); } From b5b312d8b892bb0a2f04d63ebce980d6e6337098 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 19 Oct 2026 03:43:13 +0000 Subject: [PATCH 2/8] fix(desktop): keep a minimized line's fold mounted, and give threads on no line a row for their chip Two findings from review. The chip's fold was drawn anew each time it opened or closed, so a find that opened a resolved thread's own fold inside it had that fold drawn closed again, and a keyboard press lost the focus; the fold is now kept and told whether it is open. And the chip of threads on no line, above the diff or under a file's picture, floated up over what stood before it; it now takes a row of its own. Co-authored-by: luke-agent[bot] <339870089+luke-agent[bot]@users.noreply.github.com> --- packages/panel/src/ai-elements/fold.tsx | 11 +++++-- packages/panel/src/styles/tasks.css | 9 +++--- .../panel/src/tasks/agent-changes-tab.test.ts | 31 ++++++++++++++++++- .../panel/src/tasks/agent-changes-tab.tsx | 1 + .../panel/src/tasks/minimized-threads.tsx | 11 ++++--- 5 files changed, 51 insertions(+), 12 deletions(-) diff --git a/packages/panel/src/ai-elements/fold.tsx b/packages/panel/src/ai-elements/fold.tsx index 09cdba5261..1283f962e2 100644 --- a/packages/panel/src/ai-elements/fold.tsx +++ b/packages/panel/src/ai-elements/fold.tsx @@ -19,19 +19,26 @@ import type { ComponentProps, MouseEvent, ReactNode } from "react"; * summary row is `Disclosure` itself, not this. */ -/** Note that the element's own `open` is not a prop: the state is this component's, and a fold meant to start open says `defaultOpen`. */ +/** Note that the element's own `open` is not a prop: a fold meant to start open says `defaultOpen`, and one whose holder keeps its state says `open` and hears `onOpenChange`, each press and each toggle a find makes. */ export type FoldProps = Omit, "open"> & { defaultOpen?: boolean; + open?: boolean | undefined; + onOpenChange?: ((open: boolean) => void) | undefined; }; export function Fold({ className, children, defaultOpen = false, + open, + onOpenChange, onToggle, ...props }: FoldProps): ReactNode { - const { state, details } = useDisclosureFold(defaultOpen); + const { state, details } = useDisclosureFold( + defaultOpen, + open === undefined ? undefined : { open, onOpenChange: onOpenChange ?? (() => undefined) }, + ); return (
.diff-thread-chip-row:only-child:not([open])) { + threads it shows; so does the chip of threads on no line, above the diff + or under a file's picture, which has no line to float over. */ +.diff-note:has(> .diff-thread-chip-row:only-child:not([open], [data-above])) { padding-block: 0; } @@ -2435,7 +2436,7 @@ position: relative; } -.diff-thread-chip-row:not([open]) { +.diff-thread-chip-row:not([open], [data-above]) { height: 0; } @@ -2457,7 +2458,7 @@ color: var(--luke-color-text); } -.diff-thread-chip-row:not([open]) > .diff-thread-chip { +.diff-thread-chip-row:not([open], [data-above]) > .diff-thread-chip { position: absolute; right: 0; bottom: var(--luke-space-2); diff --git a/packages/panel/src/tasks/agent-changes-tab.test.ts b/packages/panel/src/tasks/agent-changes-tab.test.ts index c704cff63b..fe516d3a0a 100644 --- a/packages/panel/src/tasks/agent-changes-tab.test.ts +++ b/packages/panel/src/tasks/agent-changes-tab.test.ts @@ -1391,9 +1391,16 @@ test("a refused mark leaves a fold chosen since, as Collapse all chose it", asyn }); test("I turns Minimize inline comments on and off inside Files changed, each line's threads then one chip that a find still reads and opens", async () => { + // Each thread drawn as a resolved one is, folded closed under its head. const threads: ChangesThreads = { list: [seatsThread(2, "why-42")], - draw: (thread) => createElement("p", { className: "drawn-thread" }, thread.comments[0]?.body), + draw: (thread) => + createElement( + "details", + { className: "drawn-card" }, + createElement("summary", null, "head"), + createElement("p", { className: "drawn-thread" }, thread.comments[0]?.body), + ), }; await drawTab({ threads }); const note = () => lastDrawn().renderAnnotation("src/seats.ts", `${PAGE}#why-42`); @@ -1423,8 +1430,30 @@ test("I turns Minimize inline comments on and off inside Files changed, each lin note().querySelector(".diff-thread-chip")?.getAttribute("aria-label"), "1 comment by octo · 1 open, hide", ); + assert.equal( + note().querySelector(".drawn-card")?.open, + true, + "the found words' own fold stays open", + ); press(listBox(), { key: "i" }); await flush(); assert.equal(note().querySelector(".diff-thread-chip"), null, "I again draws every thread whole"); }); + +test("minimized, the threads on no line take a row of their own for their chip, which has no line to float over", async () => { + const threads: ChangesThreads = { + list: [seatsThread(2, "why-42"), seatsThread(null, "was-3")], + draw: (thread) => createElement("p", null, thread.comments[0]?.body), + }; + await drawTab({ threads }); + press(listBox(), { key: "i" }); + await flush(); + const [outdated, current] = lastDrawn().files[0]?.annotations ?? []; + const chipOf = (key: string | undefined) => + lastDrawn() + .renderAnnotation("src/seats.ts", key ?? "") + .querySelector(".diff-thread-chip-row"); + assert.equal(chipOf(outdated?.key)?.hasAttribute("data-above"), true, "above the diff"); + assert.equal(chipOf(current?.key)?.hasAttribute("data-above"), false, "on its line"); +}); diff --git a/packages/panel/src/tasks/agent-changes-tab.tsx b/packages/panel/src/tasks/agent-changes-tab.tsx index c0660d036a..c3b71e9f27 100644 --- a/packages/panel/src/tasks/agent-changes-tab.tsx +++ b/packages/panel/src/tasks/agent-changes-tab.tsx @@ -427,6 +427,7 @@ function entryOf( threads={unplaced.map((each) => each.thread)} shown={minimized.shown(side, FILE_NOTE_LINE)} onShown={(shown) => minimized.show(side, FILE_NOTE_LINE, shown)} + above > {list} diff --git a/packages/panel/src/tasks/minimized-threads.tsx b/packages/panel/src/tasks/minimized-threads.tsx index 4c24ef7f19..987724d6c8 100644 --- a/packages/panel/src/tasks/minimized-threads.tsx +++ b/packages/panel/src/tasks/minimized-threads.tsx @@ -51,9 +51,12 @@ export function MinimizedThreads({ threads, shown, onShown, + above = false, children, }: { threads: readonly ReviewThread[]; + /** Whether the threads stand above the diff, on no line, where the chip takes a row of its own rather than floating over a line. */ + above?: boolean; /** Whether the threads stand under the chip. */ shown: boolean; /** Told the threads were shown or put away, by the chip or by the find. */ @@ -71,12 +74,10 @@ export function MinimizedThreads({ ].join(" · "); return ( { - if (event.currentTarget.open !== shown) onShown(event.currentTarget.open); - }} + data-above={above ? "" : undefined} + open={shown} + onOpenChange={onShown} > From 6eafcabf3c315ffe13508d30b10ad2c42ba3cfc1 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 19 Oct 2026 04:13:29 +0000 Subject: [PATCH 3/8] style(desktop): Files changed as cards that share one edge, with a quiet fold and a clearer header Each file's code area kept room for a scroll bar up and down, which a Mac that always shows scroll bars drew as a strip at the end of every line, so the rows and the bands between hunks stopped short of the file's header. The code area keeps none now, and the header, the bands, the rows, and the threads share one left and one right edge. Each file is a card on the panel's ground, rounded whole and set apart by the room between files; its header stuck at the top stands square, the card's rounded top scrolled away. The fold is a compact quiet mark with no fill while its file is open. The header names the file's folder quietly and its name in the full ink, cutting the folder first where the row runs short; its letter stands on a tint of its own colour; and Viewed says V in its hint. The bands between hunks are 24 px and quieter, and the tree's search stands level with the first card. Co-authored-by: luke-agent[bot] <339870089+luke-agent[bot]@users.noreply.github.com> --- packages/panel/src/diff-view/index.ts | 49 ++++++++++++++-- packages/panel/src/styles/tasks.css | 57 ++++++++++++++++--- .../panel/src/tasks/agent-changes-tab.tsx | 16 ++++-- packages/panel/src/tasks/changes-list.tsx | 33 +++++++++-- packages/settings/src/shortcuts.ts | 2 + 5 files changed, 134 insertions(+), 23 deletions(-) diff --git a/packages/panel/src/diff-view/index.ts b/packages/panel/src/diff-view/index.ts index 94d3dbafd8..fcf2757060 100644 --- a/packages/panel/src/diff-view/index.ts +++ b/packages/panel/src/diff-view/index.ts @@ -169,6 +169,13 @@ const LOOK_CSS = ` [data-gutter] { scrollbar-width: thin; } + /* Note that a line's code scrolls sideways alone, so it keeps no room for a + scroll bar up and down, which a system that draws its bars in the + layout would otherwise leave as a strip at the end of every line, its + rows and its gaps stopping short of the file's header. */ + [data-code] { + scrollbar-gutter: auto; + } /* A file drawn as its notes alone, laid out as blocks: its one empty line, the old side, the numbers, and the hatching beside a side's missing lines are hidden, so its notes run across its whole width, as tall as Pierre measured them. */ :host([${LEAD_ATTRIBUTE}]) pre, :host([${LEAD_ATTRIBUTE}]) [data-code], @@ -206,19 +213,50 @@ const FILL_CSS = ` } `; -/** How tall a list's file header stands, in pixels, which Pierre reserves for it before it is measured: the view's `.diff-list-head` (`styles/tasks.css`). */ -const LIST_HEADER_HEIGHT = 34; +/** How tall a list's file header stands, in pixels, which Pierre reserves for it before it is measured: the view's `.diff-list-head` (`styles/tasks.css`), its row and its rule. */ +const LIST_HEADER_HEIGHT = 37; + +/** How tall a band between two hunks of a list's file stands, in pixels, which Pierre lays the list out by (`LIST_CSS`). */ +const LIST_GAP_HEIGHT = 24; + +/** The room between two files of a list, and above the first, in pixels: the panel's ground between their cards. */ +const LIST_FILE_GAP = 12; /** The room below a list's last file, in pixels, so its last lines can be scrolled up off the foot of the pane. */ const LIST_FOOT_ROOM = 48; +/** + * A file in a list: a card, its edge and its corners the panel's, holding + * its header and its lines, each gap between hunks a quiet band of its own + * height (`LIST_GAP_HEIGHT`). Note that the card's corners are clipped at + * the card's own edge, so its header, stuck at the top of the list while + * its lines scroll by, stands square there, the rounded top scrolled away. + */ +const LIST_CSS = ` +@layer unsafe { + :host { + box-sizing: border-box; + overflow: clip; + border: 1px solid var(--luke-color-border); + border-radius: var(--luke-control-radius); + --diffs-bg-separator-override: var(--luke-color-fill-hover); + } + [data-separator="line-info-basic"] { + height: ${LIST_GAP_HEIGHT}px; + color: var(--luke-color-text-subtle); + font-family: var(--luke-font-sans); + font-size: var(--luke-type-caption-size); + } +} +`; + let sheets: { look: CSSStyleSheet; fill: CSSStyleSheet } | undefined; -/** The look's sheets, made once and adopted by every diff's shadow root: the look every diff in a list wears, and the same with the fill one diff alone wears. */ +/** The look's sheets, made once and adopted by every diff's shadow root: the look with the card every file of a list wears, and the look with the fill one diff alone wears. */ function lookSheets(): { look: CSSStyleSheet; fill: CSSStyleSheet } { if (sheets !== undefined) return sheets; const look = new CSSStyleSheet(); - look.replaceSync(LOOK_CSS); + look.replaceSync(`${LOOK_CSS}${LIST_CSS}`); const fill = new CSSStyleSheet(); fill.replaceSync(`${LOOK_CSS}${FILL_CSS}`); sheets = { look, fill }; @@ -424,8 +462,9 @@ function listOptions( itemMetrics: { lineHeight: Number(drawn.fontSize) * Number(LINE_HEIGHT_RATIO[drawn.lineHeight]), diffHeaderHeight: LIST_HEADER_HEIGHT, + hunkSeparatorHeight: LIST_GAP_HEIGHT, }, - layout: { paddingTop: 0, paddingBottom: LIST_FOOT_ROOM, gap: 0 }, + layout: { paddingTop: LIST_FILE_GAP, paddingBottom: LIST_FOOT_ROOM, gap: LIST_FILE_GAP }, renderCustomHeader: ( _drawn: FileDiffMetadata | FileContents, context: { item: { id: string } }, diff --git a/packages/panel/src/styles/tasks.css b/packages/panel/src/styles/tasks.css index d219369d84..9238be919d 100644 --- a/packages/panel/src/styles/tasks.css +++ b/packages/panel/src/styles/tasks.css @@ -1497,12 +1497,19 @@ user-select: text; } -/* The list fills the File tab's place, its scroll area the whole of it. */ +/* The list fills the File tab's place, its scroll area the whole of it, its + files cards on the panel's ground, inset from its edges by the room between + two of them (diff-view/index.ts's LIST_FILE_GAP). */ .diff-list { display: flex; min-height: 0; flex: 1; flex-direction: column; + background: var(--luke-color-ground); +} + +.diff-list > .luke-scroll-area > :first-child { + padding-inline: var(--luke-space-12); } /* Files changed's list of diffs (diff-list.tsx): every file under a header @@ -1511,21 +1518,20 @@ bundle reserves for it (diff-view/index.ts's LIST_HEADER_HEIGHT). The file stage-cli's keys stand on is ringed as a focused control is. */ .diff-list-head { - border-top: 1px solid var(--luke-color-border); - background: var(--code-ground); + background: var(--luke-color-surface); color: var(--luke-color-text); font-family: var(--luke-font-sans); + cursor: default; } .diff-list-head-row { display: flex; box-sizing: border-box; - height: 33px; + height: 36px; min-width: 0; align-items: center; gap: var(--luke-space-8); - padding: 0 var(--luke-space-12) 0 var(--luke-space-6); - background: var(--luke-color-surface); + padding: 0 var(--luke-space-12) 0 var(--luke-space-4); font-size: var(--luke-type-caption-size); white-space: nowrap; } @@ -1548,14 +1554,43 @@ transform: none; } +/* A file's path: its folder quiet and cut first where the row runs short, so + its name, in the full ink, stands whole as long as it can. */ .diff-list-path { + display: flex; min-width: 0; flex: 0 1 auto; - overflow: hidden; font-family: var(--luke-font-mono); +} + +.diff-list-folder { + min-width: 0; + overflow: hidden; + color: var(--luke-color-text-muted); + text-overflow: ellipsis; +} + +.diff-list-name { + flex: none; + max-width: 100%; + overflow: hidden; + font-weight: var(--luke-weight-medium); text-overflow: ellipsis; } +/* A file's letter in its header stands on a tint of its own colour, the + letter itself what says what became of it. */ +.diff-list-head .file-view-status { + display: inline-flex; + min-width: var(--luke-space-16); + height: var(--luke-space-16); + align-items: center; + justify-content: center; + border-radius: var(--luke-radius-6); + background: color-mix(in srgb, currentcolor 14%, transparent); + font-size: var(--luke-type-caption-size); +} + /* A file's Viewed box, GitHub's own (viewed-files.ts), at the end of its header's row. */ .diff-list-viewed { @@ -1677,6 +1712,12 @@ flex: 1; } +/* Beside Files changed's cards, the search stands where the first card does, + the room between files above it, so the tree and the list share a top. */ +.file-tab:has(> .diff-list) + .file-tree-pane .file-tree-search { + padding-top: var(--luke-space-12); +} + .file-tree-notice { display: flex; flex-direction: column; @@ -2416,7 +2457,7 @@ so the window's own type and card draw it, inset from the code's edges as GitHub's are; one the diff draws no row for stands in a list above it. */ .diff-note { - padding: var(--luke-space-6) var(--luke-space-12); + padding: var(--luke-space-8) var(--luke-space-12); font-family: var(--luke-font-sans); white-space: normal; } diff --git a/packages/panel/src/tasks/agent-changes-tab.tsx b/packages/panel/src/tasks/agent-changes-tab.tsx index c3b71e9f27..256793a8eb 100644 --- a/packages/panel/src/tasks/agent-changes-tab.tsx +++ b/packages/panel/src/tasks/agent-changes-tab.tsx @@ -57,6 +57,7 @@ import { FOLD_CHORD, LIST_WORDS, useChangesList, + VIEWED_CHORD, } from "./changes-list.js"; import { CHANGES_WORDS, @@ -693,11 +694,16 @@ function ViewedBox({ onToggle: () => void; }): React.JSX.Element { return ( - - - {LIST_WORDS.VIEWED} - - + + + + {LIST_WORDS.VIEWED} + + + ); } diff --git a/packages/panel/src/tasks/changes-list.tsx b/packages/panel/src/tasks/changes-list.tsx index ba3b6d69cc..1ba926b1a9 100644 --- a/packages/panel/src/tasks/changes-list.tsx +++ b/packages/panel/src/tasks/changes-list.tsx @@ -1,6 +1,6 @@ import type { CodingAgentChangedFile } from "@luke/features/coding-agent-wire"; import { SHORTCUT_KEY, type ShortcutChord, shortcutAria } from "@luke/settings/shortcuts"; -import { IconButton } from "@luke/ui"; +import { BUTTON_SIZE, IconButton } from "@luke/ui"; import { ChevronRightIcon } from "lucide-react"; import { type ReactNode, @@ -69,6 +69,9 @@ export const FOLD_CHORD = { ALL: { key: SHORTCUT_KEY.SEMICOLON, plain: true, shift: true }, } as const satisfies Record; +/** The key that marks the file stood on viewed, or unmarks it, as GitHub's own V does. */ +export const VIEWED_CHORD = { key: SHORTCUT_KEY.V, plain: true } as const satisfies ShortcutChord; + /** What the list says where a filter keeps no file. */ export const LIST_WORDS = { NONE_KEPT: "No changed file's path holds that.", @@ -79,6 +82,8 @@ export const LIST_WORDS = { SHOW_SOURCE: "Show source", SHOW_IMAGE: "Show image", VIEWED: "Viewed", + MARK_VIEWED: "Mark as viewed", + UNMARK_VIEWED: "Mark as not viewed", /** How far through the files the member is, as GitHub counts it. */ viewedOf: (viewed: number, files: number) => `${viewed} / ${files} viewed`, } as const; @@ -335,6 +340,23 @@ export function useChangesList( }; } +/** A path as a header names it: its folder, quiet, then its name. */ +function PathParts({ path }: { path: string }): React.JSX.Element { + const cut = path.lastIndexOf("/") + 1; + return ( + <> + {cut === 0 ? null : ( + + {path.slice(0, cut)} + + )} + + {path.slice(cut)} + + + ); +} + /** A pointer arriving over a control, as the window says it. */ function pointerOver(): PointerEvent { return new PointerEvent("pointerover", { bubbles: true, pointerType: "mouse" }); @@ -390,6 +412,7 @@ export function FileHead({ ref={foldRef} icon={