From 5bf3c81f6f3be42e495f13daf36b2c7342852553 Mon Sep 17 00:00:00 2001 From: omartelo Date: Thu, 30 Jul 2026 13:00:17 -0300 Subject: [PATCH 01/10] feat: read and write pull request reviews inside the diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pulls only ever showed the parts of a pull request that travel one way: the checks rollup, the commits, the changed files, and the two writes that end it. The conversation was missing entirely, so a review that asked for a change was invisible here and reading it meant the browser — which is the thing this screen set out to stop. The whole review now happens in the diff. A thread opens inline, in a gap between two lines of the file it is about. The gap is a CodeMirror block widget that owns nothing but an empty element; React fills it through a portal. That split is the point: a comment box holds text somebody is typing and the editor rebuilds its DOM whenever it likes, so the widget is compared by a key that outlives a rebuild and the text lives outside it. Slots arrive through a StateEffect rather than through the view's extensions — resolving a thread must not rebuild the editor under the cursor. Comments of your own are written the same way and held back. The diff's context menu grows "Comment on lines N-M" beside "Inject lines N-M" — the same gesture, the two destinations it has always had to choose between: the session next door, or GitHub. Each comment waits with the others until Submit review sends them as one call, with a verdict and a summary above them. That is what makes it a review rather than a pile of separate remarks, and it is why the pending state is a store backed by localStorage: the panel refetches on focus and while checks run, a collapsed file destroys its editor, and an app update reloads the page. A review written over an afternoon has to survive all three. Everything a line cannot hold goes to a new Conversation tab, in the order it was said: the verdicts, the comments on the pull request itself, and the threads whose lines the branch has since rewritten — GitHub nulls their anchor, so no diff can show them. Settled threads fold themselves away behind a count wherever they render. Behind it: - One `gh api graphql` read brings back reviews, comments and threads together. gh's own `pr view` cannot answer this: --json reviews has a review's body but never the threads under it, and no REST endpoint reports whether a thread was resolved. - A thread carries two identifiers because GitHub needs both — the node id resolves it, the comment's database id is what a reply is addressed to. - A submitted review is one REST call carrying every line comment. Its body goes to gh through a temporary file: --field cannot express an array of objects, and widening every gh call site with a stdin argument to serve this one call is the worse trade. - ApprovePullRequest is gone. Approving is that same submit with no body and no comments, so the button keeps its single click and grows into "Submit review N" the moment a comment is waiting. - A reply posts immediately; a new comment always waits. It is GitHub's own split between answering something already said and starting a review. Deliberately new-file lines only for a comment of your own: a comment on a deleted line is a thread GitHub anchors on the other side of the diff, and that needs a gesture of its own. Verified by driving a headless instance over CDP, since the frontend suite is node-only and cannot render a widget: a draft planted in the store reappears inside the right file's diff after a reload, a real drag-selection produces the menu item, and the composer it opens files the comment as pending with its line span intact. Co-authored-by: Claude --- CHANGELOG.md | 14 + frontend/src/components/diff/FileDiff.tsx | 154 ++++++++-- frontend/src/components/diff/InjectMenu.tsx | 12 + frontend/src/components/diff/ReviewSlots.tsx | 88 ++++++ frontend/src/components/diff/useDiffEditor.ts | 23 +- frontend/src/components/pulls/CommentBox.tsx | 72 +++++ .../src/components/pulls/PullRequestView.tsx | 177 +++++++++-- frontend/src/components/pulls/Pulls.tsx | 9 + .../components/pulls/PullsConversation.tsx | 166 +++++++++++ frontend/src/components/pulls/PullsFiles.tsx | 40 ++- .../src/components/pulls/ReviewThread.tsx | 239 +++++++++++++++ .../components/pulls/SubmitReviewDialog.tsx | 100 +++++++ frontend/src/lib/api-types.ts | 69 +++++ frontend/src/lib/codemirror-threads.ts | 144 +++++++++ frontend/src/lib/codemirror.ts | 14 + frontend/src/lib/git/diff.test.ts | 30 ++ frontend/src/lib/git/diff.ts | 20 ++ frontend/src/lib/prefs.ts | 4 + .../lib/pulls/conversation-timeline.test.ts | 74 +++++ .../src/lib/pulls/conversation-timeline.ts | 68 +++++ .../lib/pulls/pending-review-store.test.ts | 195 ++++++++++++ .../src/lib/pulls/pending-review-store.ts | 191 ++++++++++++ frontend/src/lib/pulls/review-slots.test.ts | 113 +++++++ frontend/src/lib/pulls/review-slots.ts | 87 ++++++ .../pulls/use-pull-request-conversation.ts | 37 +++ frontend/src/lib/rpc.ts | 33 ++- frontend/src/lib/use-codemirror-view.ts | 11 +- internal/project/gherror.go | 14 + internal/project/pr.go | 15 - internal/project/pr_test.go | 38 --- internal/project/prreview.go | 200 +++++++++++++ internal/project/prreview_test.go | 230 ++++++++++++++ internal/project/prthreads.go | 280 ++++++++++++++++++ internal/project/prthreads_test.go | 174 +++++++++++ 34 files changed, 3018 insertions(+), 117 deletions(-) create mode 100644 frontend/src/components/diff/ReviewSlots.tsx create mode 100644 frontend/src/components/pulls/CommentBox.tsx create mode 100644 frontend/src/components/pulls/PullsConversation.tsx create mode 100644 frontend/src/components/pulls/ReviewThread.tsx create mode 100644 frontend/src/components/pulls/SubmitReviewDialog.tsx create mode 100644 frontend/src/lib/codemirror-threads.ts create mode 100644 frontend/src/lib/pulls/conversation-timeline.test.ts create mode 100644 frontend/src/lib/pulls/conversation-timeline.ts create mode 100644 frontend/src/lib/pulls/pending-review-store.test.ts create mode 100644 frontend/src/lib/pulls/pending-review-store.ts create mode 100644 frontend/src/lib/pulls/review-slots.test.ts create mode 100644 frontend/src/lib/pulls/review-slots.ts create mode 100644 frontend/src/lib/pulls/use-pull-request-conversation.ts create mode 100644 internal/project/prreview.go create mode 100644 internal/project/prreview_test.go create mode 100644 internal/project/prthreads.go create mode 100644 internal/project/prthreads_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b531f9d80..291ac954e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Review a pull request without leaving lich.** Pulls only ever showed the + parts that travel one way — the checks, the commits, the diff — so a review + that asked for a change was invisible here and reading it meant the browser. + The diff now carries the conversation: a thread opens inline under the line it + is about, with its replies, and can be answered and resolved where it sits. + Right-click a selection in the diff and *Comment on lines* starts one of your + own; each comment waits with the others until **Submit review** sends them + together as a single review — approving, commenting, or requesting changes, + with a summary of your own above them. Comments waiting to be sent are marked + as pending and survive a refresh, a collapsed file and a restart, so a review + written over an afternoon is still there. A new **Conversation** tab holds the + whole exchange in the order it happened — verdicts, comments on the pull + request itself, and the threads whose lines the branch has since rewritten, + which no diff can show. Settled threads fold themselves away behind a count. - **Every worktree gets a dev-server port of its own.** Two worktrees of one project both ran `pnpm dev` and both wanted the same port, so the second one lost — and the setup script that had just installed the dependencies had no diff --git a/frontend/src/components/diff/FileDiff.tsx b/frontend/src/components/diff/FileDiff.tsx index d727f5098..8781f210a 100644 --- a/frontend/src/components/diff/FileDiff.tsx +++ b/frontend/src/components/diff/FileDiff.tsx @@ -1,20 +1,25 @@ -import { ChevronDown, ChevronRight, Paperclip, Undo2 } from "lucide-react" +import { ChevronDown, ChevronRight, MessageSquare, Paperclip, Undo2 } from "lucide-react" import { useEffect, useMemo, useRef, useState } from "react" +import { createPortal } from "react-dom" import { IconAction } from "@/components/common/IconAction" import { DiffStat } from "@/components/DiffStat" import { Checkbox } from "@/components/ui/checkbox" +import { threadSlots, type SlotElements, type ThreadSlot } from "@/lib/codemirror-threads" import { buildFileDoc, formatLineRef, newLineRange, type DiffFile, type FileDoc, + type NewLineRange, } from "@/lib/git/diff" import { languageAbbr, splitPath } from "@/lib/git/lang-badge" +import { reviewSlots } from "@/lib/pulls/review-slots" import { cn } from "@/lib/utils" import type { DiffBulk } from "./diff-bulk" import { InjectMenu } from "./InjectMenu" import { useDiffEditor } from "./useDiffEditor" +import { type Composer, type DiffReview, ReviewSlot } from "./ReviewSlots" // Files whose rendered diff exceeds this many lines start collapsed, so one // giant lockfile doesn't swamp the panel (expanding is one click away). @@ -42,11 +47,23 @@ interface FileDiffProps { /** Handle the tick. Absent = no Viewed control (the dock's working diff, * where every file is the current one). */ onViewed?: (next: boolean) => void + /** The pull request review over this file: threads to render inline, drafts + * waiting to be sent, and what a new comment does. Absent for the working + * diff, which has no pull request behind it. */ + review?: DiffReview } // The card must not clip overflow — a clipping ancestor would break the // sticky header. -export function FileDiff({ file, onInject, onDiscard, bulk, viewed, onViewed }: FileDiffProps) { +export function FileDiff({ + file, + onInject, + onDiscard, + bulk, + viewed, + onViewed, + review, +}: FileDiffProps) { const doc = useMemo(() => buildFileDoc(file), [file]) const [expanded, setExpanded] = useState(!file.binary && doc.lineMeta.length <= LARGE_FILE_LINES) // The nonce guard skips the initial mount so each file keeps its own @@ -61,6 +78,9 @@ export function FileDiff({ file, onInject, onDiscard, bulk, viewed, onViewed }: const Chevron = expanded ? ChevronDown : ChevronRight const badge = languageAbbr(file.newPath) const { dir, base } = splitPath(file.newPath) + const talking = review + ? review.threads.filter((thread) => !thread.isResolved).length + review.drafts.length + : 0 return (
@@ -93,6 +113,15 @@ export function FileDiff({ file, onInject, onDiscard, bulk, viewed, onViewed }: {dir && {dir}} + {/* A collapsed file must still say it is being talked about — the + threads inside it are invisible until it is opened. Settled threads + are left out of the count: they are not what is outstanding. */} + {talking > 0 && ( + + + {talking} + + )} @@ -127,7 +156,7 @@ export function FileDiff({ file, onInject, onDiscard, bulk, viewed, onViewed }: (file.binary ? (

Binary file

) : ( - + ))}
) @@ -137,6 +166,7 @@ interface DiffBodyProps { doc: FileDoc path: string onInject: (text: string) => void + review?: DiffReview } // A panel holds one CodeMirror view per expanded file, and a wide diff (a PR @@ -144,7 +174,7 @@ interface DiffBodyProps { // page. LazyDiffBody defers each one until its card comes near the viewport; // once built, the editor stays, so scrolling back keeps its selection and // highlighting. Collapsing the file still destroys it. -function LazyDiffBody({ doc, path, onInject }: DiffBodyProps) { +function LazyDiffBody({ doc, path, onInject, review }: DiffBodyProps) { const placeholder = useRef(null) const [mounted, setMounted] = useState(false) @@ -166,37 +196,115 @@ function LazyDiffBody({ doc, path, onInject }: DiffBodyProps) { }, []) if (mounted) { - return + return } // The placeholder has to stand in for the file's height: zero-height cards // would all sit in the viewport at once and nothing would be deferred. return
} +const NO_SLOTS: SlotElements = new Map() + // DiffBody exists as its own component so collapsing the file unmounts it, // destroying the CodeMirror view instead of keeping it alive off-screen. // // A diff's document is not the file: the selection lands on doc lines, which -// newLineRange maps back to the line numbers the agent needs to be told. -function DiffBody({ doc, path, onInject }: DiffBodyProps) { - const { containerRef, getSelectedDocLines } = useDiffEditor(doc, path) - const [lineRef, setLineRef] = useState(null) +// newLineRange maps back to the line numbers the agent needs to be told — and, +// under a pull request, the line numbers GitHub files a comment against. +function DiffBody({ doc, path, onInject, review }: DiffBodyProps) { + // One slot extension per view. It is memoised because it *is* part of the + // view's identity: a new one would rebuild the editor on every render. + const [elements, setElements] = useState(NO_SLOTS) + const slots = useMemo(() => threadSlots(setElements), []) + const { containerRef, getSelectedDocLines, view } = useDiffEditor( + doc, + path, + review ? slots.extension : undefined, + ) + const [range, setRange] = useState(null) + const [composer, setComposer] = useState(null) + + // Where the gaps go, never what is in them: keyed off the composer's line + // rather than the composer, so typing into it does not re-dispatch the whole + // slot set on every keystroke. + const composerLine = composer?.end ?? 0 + const wanted = useMemo( + (): ThreadSlot[] => + review + ? reviewSlots({ + lineMeta: doc.lineMeta, + threads: review.threads, + drafts: review.drafts.map(({ comment }) => comment), + composerLine, + }) + : [], + [review, doc.lineMeta, composerLine], + ) + + useEffect(() => { + if (view && review) { + slots.update(view, wanted) + } + }, [view, review, slots, wanted]) + + const addComment = (): void => { + if (!review || !composer || composer.body.trim() === "") { + return + } + review.onAdd({ + path, + line: composer.end, + startLine: composer.start === composer.end ? 0 : composer.start, + side: "RIGHT", + body: composer.body.trim(), + }) + setComposer(null) + } return ( - { - if (!open) { - return + <> + { + if (!open) { + return + } + const selected = getSelectedDocLines() + setRange(selected ? newLineRange(doc.lineMeta, selected.from, selected.to) : null) + }} + onInject={onInject} + // Commenting only exists under a pull request — but the item is offered + // whenever there is one, not only once a range is resolved: the + // selection is read as the menu opens, so gating the item on it would + // hide it on the very open that produced it. With nothing selected the + // menu disables it, like Inject lines beside it. + // + // Deliberately new-file lines only: a comment on a deleted line is a + // thread GitHub anchors on the other side, and that needs its own gesture. + onComment={ + review + ? () => range && setComposer({ start: range.start, end: range.end, body: "" }) + : undefined } - const selected = getSelectedDocLines() - const range = selected ? newLineRange(doc.lineMeta, selected.from, selected.to) : null - setLineRef(range && formatLineRef(range)) - }} - onInject={onInject} - /> + /> + {review && + [...elements].map(([key, element]) => + createPortal( + setComposer((held) => held && { ...held, body })} + onComposerSubmit={addComment} + onComposerCancel={() => setComposer(null)} + />, + element, + key, + ), + )} + ) } diff --git a/frontend/src/components/diff/InjectMenu.tsx b/frontend/src/components/diff/InjectMenu.tsx index 9433e9d5a..63fa550f7 100644 --- a/frontend/src/components/diff/InjectMenu.tsx +++ b/frontend/src/components/diff/InjectMenu.tsx @@ -16,6 +16,10 @@ interface InjectMenuProps { /** Resolve the selection; fired as the menu opens, not on every change. */ onOpenChange: (open: boolean) => void onInject: (text: string) => void + /** Start a review comment on the selected lines. Absent where there is no + * pull request to comment on — the dock's working diff — and while nothing is + * selected. */ + onComment?: () => void } // The right-click menu over a code view — a file's diff, or the read-only @@ -30,6 +34,7 @@ export function InjectMenu({ lineRef, onOpenChange, onInject, + onComment, }: InjectMenuProps) { return ( @@ -42,6 +47,13 @@ export function InjectMenu({ > {lineRef === null ? "Inject lines" : `Inject lines ${lineRef}`} + {/* The same selection, the other destination: the session next door, or + the pull request on GitHub. */} + {onComment && ( + + {lineRef === null ? "Comment on lines" : `Comment on lines ${lineRef}`} + + )} ) diff --git a/frontend/src/components/diff/ReviewSlots.tsx b/frontend/src/components/diff/ReviewSlots.tsx new file mode 100644 index 000000000..3f357cd60 --- /dev/null +++ b/frontend/src/components/diff/ReviewSlots.tsx @@ -0,0 +1,88 @@ +import type { DraftReviewComment, ReviewThread as Thread } from "@/lib/api-types" +import { COMPOSER_KEY, draftSlotKey, threadSlotKey } from "@/lib/pulls/review-slots" +import { CommentBox } from "@/components/pulls/CommentBox" +import { PendingComments, ReviewThread, type ThreadActions } from "@/components/pulls/ReviewThread" + +// The pull request review laid over one file's diff. Absent for the dock's +// working diff, which has no pull request behind it and therefore no threads, +// no drafts and nothing to submit. +export interface DiffReview { + /** Threads anchored in this file. */ + threads: Thread[] + /** Draft comments in this file, each with its index in the pending review — + * the index is what editing and discarding address. */ + drafts: { comment: DraftReviewComment; index: number }[] + actions: ThreadActions + onAdd: (comment: DraftReviewComment) => void + onEdit: (index: number, body: string) => void + onRemove: (index: number) => void +} + +/** The comment being written: the lines it covers and the text so far. Held by + * the diff body rather than by the box, so a refetch that rebuilds the editor + * moves the box without emptying it. */ +export interface Composer { + start: number + end: number + body: string +} + +interface ReviewSlotProps { + slotKey: string + review: DiffReview + composer: Composer | null + onComposerChange: (body: string) => void + onComposerSubmit: () => void + onComposerCancel: () => void +} + +// What goes in one gap, resolved from the key the widget carries. A key with +// nothing behind it renders nothing: the slots and the data behind them are +// updated in separate passes, and a thread resolved in between must not throw +// on the way out. +export function ReviewSlot({ + slotKey, + review, + composer, + onComposerChange, + onComposerSubmit, + onComposerCancel, +}: ReviewSlotProps) { + if (slotKey === COMPOSER_KEY) { + if (!composer) { + return null + } + return ( +
+ + {composer.start === composer.end + ? `Comment on line ${composer.end}` + : `Comment on lines ${composer.start}–${composer.end}`} + + +
+ ) + } + + if (slotKey.startsWith("t:")) { + const thread = review.threads.find((held) => threadSlotKey(held) === slotKey) + return thread ? ( + + ) : null + } + + const drafts = review.drafts.filter(({ comment }) => draftSlotKey(comment) === slotKey) + return drafts.length > 0 ? ( +
+ +
+ ) : null +} diff --git a/frontend/src/components/diff/useDiffEditor.ts b/frontend/src/components/diff/useDiffEditor.ts index f9a967d98..1fd0591a0 100644 --- a/frontend/src/components/diff/useDiffEditor.ts +++ b/frontend/src/components/diff/useDiffEditor.ts @@ -1,5 +1,7 @@ import { useMemo } from "react" import type { RefObject } from "react" +import type { Extension } from "@codemirror/state" +import type { EditorView } from "@codemirror/view" import { buildLineDecorations, diffGutter, type DocLineSelection } from "@/lib/codemirror" import { useCodeMirrorView } from "@/lib/use-codemirror-view" import type { FileDoc } from "@/lib/git/diff" @@ -8,6 +10,8 @@ export interface DiffEditor { containerRef: RefObject /** 1-based doc line span of the current selection, or null when empty. */ getSelectedDocLines: () => DocLineSelection | null + /** The live view, for a caller that dispatches into it. */ + view: EditorView | null } // useDiffEditor is the diff card's read-only CodeMirror view: the shared editor @@ -15,14 +19,23 @@ export interface DiffEditor { // background. Its selection lands on *doc* lines, which are the diff's own // interleaving of added and deleted, not the file's numbering; mapping them // back is newLineRange's job at the call site. -export function useDiffEditor(doc: FileDoc, filename: string): DiffEditor { +// +// `extra` is laid on top for a caller that needs more of the editor — the pull +// request's review threads open their gaps through it. It has to be a stable +// reference: it rides the view's identity, so a new one on every render would +// rebuild the editor on every render. +export function useDiffEditor(doc: FileDoc, filename: string, extra?: Extension): DiffEditor { const source = useMemo( () => ({ text: doc.text, - extensions: [diffGutter(doc.lineMeta), buildLineDecorations(doc.lineMeta)], + extensions: [ + diffGutter(doc.lineMeta), + buildLineDecorations(doc.lineMeta), + ...(extra ? [extra] : []), + ], }), - [doc], + [doc, extra], ) - const { containerRef, getSelectedLines } = useCodeMirrorView(source, filename) - return { containerRef, getSelectedDocLines: getSelectedLines } + const { containerRef, getSelectedLines, view } = useCodeMirrorView(source, filename) + return { containerRef, getSelectedDocLines: getSelectedLines, view } } diff --git a/frontend/src/components/pulls/CommentBox.tsx b/frontend/src/components/pulls/CommentBox.tsx new file mode 100644 index 000000000..13656a46e --- /dev/null +++ b/frontend/src/components/pulls/CommentBox.tsx @@ -0,0 +1,72 @@ +import { Button } from "@/components/ui/button" +import { cn } from "@/lib/utils" + +interface CommentBoxProps { + value: string + onChange: (value: string) => void + onSubmit: () => void + /** Absent leaves no way out — for a box that is always open (the review + * summary), where cancelling means clearing the text. */ + onCancel?: () => void + submitLabel: string + busy?: boolean + placeholder?: string + autoFocus?: boolean + rows?: number + className?: string +} + +// The one place review text is typed: a line comment, a reply, the summary above +// a review. It holds no draft of its own — the caller owns the text, because the +// editor under this box is rebuilt by CodeMirror whenever the diff refetches and +// a box that kept its own state would lose it there. +// +// Cmd/Ctrl+Enter sends and Escape backs out, so a comment written mid-review +// never needs the mouse. +export function CommentBox({ + value, + onChange, + onSubmit, + onCancel, + submitLabel, + busy = false, + placeholder, + autoFocus = false, + rows = 3, + className, +}: CommentBoxProps) { + const empty = value.trim() === "" + return ( +
+