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/2] 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/2] 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} >