diff --git a/apps/hue/src/app.d b/apps/hue/src/app.d index 93e5fa11b..f1ad873f1 100644 --- a/apps/hue/src/app.d +++ b/apps/hue/src/app.d @@ -24,6 +24,7 @@ import std.stdio : stderr, write; import std.string : chompPrefix; import sparkles.syntax; +import sparkles.syntax.md.model : MdDoc; import sparkles.twoslash; import sparkles.core_cli.args; @@ -34,8 +35,8 @@ import sparkles.base.term_caps : isTerminal, StdStream; import ansi_model : BackgroundMode, backgroundOptions; import document : ContentKind, Document, DocumentPipeline, hueFenceRenderer; import diff_commutative : CommutativeKind; -import diff_session : SessionHeader; -import forge : PullRequest; +import diff_session : AnchoredThread, SessionHeader, ThreadComment; +import forge : CommentThread, PullRequest, ThreadSide; import diff_structural : StructuralPolicy; import diff_view : DiffLayout, DiffViewOptions; import sparkles.diff : WhitespaceMode; @@ -391,6 +392,37 @@ private SessionHeader prHeader(ref GrammarRegistry reg, in PullRequest pr) @syst return h; } +/// `DPR3`: the forge's conversations as the view's own anchored threads — +/// each comment's markdown parsed here, so the renderer draws a review +/// comment the way it draws every other piece of markdown hue shows. +private AnchoredThread[] prThreads(ref GrammarRegistry reg, + in CommentThread[] threads) @system +{ + import sparkles.syntax.md.model : extractMarkdown; + + AnchoredThread[] out_; + foreach (ref t; threads) + { + AnchoredThread a = { + path: t.path, + line: t.line, + oldSide: t.side == ThreadSide.oldSide, + resolved: t.resolved, + outdated: t.outdated, + }; + foreach (ref c; t.comments) + a.comments ~= ThreadComment(c.author, shortDate(c.createdAt), + c.body_.length ? extractMarkdown(reg, c.body_) : MdDoc.init); + out_ ~= a; + } + return out_; +} + +/// An ISO-8601 timestamp trimmed to its date. A review thread wants "when, +/// roughly"; the clock time is noise beside the comment it labels. +private string shortDate(string iso) @safe pure nothrow + => iso.length >= 10 ? iso[0 .. 10] : iso; + private StructuralPolicy parseStructural(string spelling) @safe { import diff_structural : parseStructuralPolicy; @@ -661,6 +693,9 @@ int main(string[] args) text(got.repo.owner, "/", got.repo.name, " #", got.pr.number), got.patch); doc.diffSession.header = prHeader(registry, got.pr); + doc.diffSession.threads = prThreads(registry, got.threads); + if (got.threadsNote.length) + warning(i"review threads unavailable: $(got.threadsNote)"); } else if (cli.diff || cli.staged) { diff --git a/apps/hue/src/diff_session.d b/apps/hue/src/diff_session.d index ef3b5b6af..4e61454a6 100644 --- a/apps/hue/src/diff_session.d +++ b/apps/hue/src/diff_session.d @@ -77,12 +77,48 @@ struct SessionHeader MdDoc description; } +/// One message of a review conversation, ready to render. +/// +/// The body is an `MdDoc` for the same reason a PR description is: a review +/// comment is markdown, and rendering it as raw text would be the one place +/// in hue where markdown is not markdown. +struct ThreadComment +{ + string author; + string when; + MdDoc body_; +} + +/** +`DPR3`: a review conversation anchored to a file and a line. + +Forge-neutral on purpose. This is the shape the VIEW renders, and `DCM2`'s +local comments produce exactly the same value — which is what "the thread +block is one widget" means concretely: no forge type reaches the renderer, and +a locally-authored thread is not a second kind of thing. +*/ +struct AnchoredThread +{ + string path; + /// 1-based line on its side; zero when the forge could not place it. + uint line; + /// The thread hangs on the old text (a comment on a removed line). + bool oldSide; + bool resolved; + /// The code it was written against has changed since. + bool outdated; + ThreadComment[] comments; +} + struct DiffSession { SessionEntry[] entries; size_t index; /// the selected entry (always < `entries.length` when non-empty) /// `DPR2`: the header this session renders above its files, if any. SessionHeader header; + /// `DPR3`/`DCM2`: every conversation on this session, any file. The view + /// selects each file's own by path. + AnchoredThread[] threads; @safe pure nothrow @nogc: diff --git a/apps/hue/src/diff_view.d b/apps/hue/src/diff_view.d index 4fcd2ff32..54f1229ff 100644 --- a/apps/hue/src/diff_view.d +++ b/apps/hue/src/diff_view.d @@ -19,8 +19,8 @@ module diff_view; import std.conv : text; -import diff_session : DiffSession, FileChange, SessionEntry, SessionHeader, - statusGlyph; +import diff_session : AnchoredThread, DiffSession, FileChange, SessionEntry, + SessionHeader, statusGlyph; import document : DiffSides; import sparkles.diff.model : Degradation, DiffDoc, FileEntry, Hunk, Row, RowKind, Span; @@ -75,6 +75,10 @@ struct DiffViewOptions /// badge. Demote, never hide — the badge says how many rows it stands for /// and the reviewer can always expand. bool foldFormattingOnly = true; + /// `DPR3`/`DCM2`: this file's review conversations. Each renders as a + /// block under the row it is anchored to; a resolved one folds to a + /// single line, because a settled argument is context, not a task. + const(AnchoredThread)[] threads; /// `DVT1`: the per-side type payloads for this file. Each attaches only /// when its `code` is byte-identical to that side's diff text — see /// $(LREF anchors) — so a decoration can never land on the wrong token. @@ -253,6 +257,12 @@ WidgetTree viewDiffDoc(const ref DiffDoc doc, DiffViewOptions opt = DiffViewOpti // expanded unchanged region is read from. if (fi < sides.length) fopt.sideText = sides[fi].newText; + // `DPR3`: the conversations on THIS file, by path — the session holds + // them all, and a collapsed file renders none. + if (!fopt.entry.collapsed && session.threads.length) + fopt.threads = threadsFor(session.threads, + doc.pathText(doc.files[fi].newPath), + doc.pathText(doc.files[fi].oldPath)); // `DVT1`: the file's per-side type overlays, already anchored (or // refused) by whoever attached them. if (!fopt.entry.collapsed && fi < types.length) @@ -466,6 +476,90 @@ private Slot statusSlot(FileChange c) @safe pure nothrow @nogc } } +/// The threads belonging to one file. Matched on either path so a rename +/// does not orphan a conversation written before it. +private const(AnchoredThread)[] threadsFor( + return scope const(AnchoredThread)[] all, scope const(char)[] newPath, + scope const(char)[] oldPath) @safe +{ + const(AnchoredThread)[] mine; + foreach (ref t; all) + if (t.path == newPath || t.path == oldPath) + mine ~= t; + return mine; +} + +/// Does this thread hang on this row? A thread on the old side anchors to a +/// removed or context row's old line; one on the new side to an added or +/// context row's new line. +private bool anchoredHere(in AnchoredThread t, in Row row) @safe pure nothrow @nogc +{ + if (t.line == 0) + return false; // outdated: no line to hang on + return t.oldSide ? row.oldLine == t.line : row.newLine == t.line; +} + +/** +`DPR3`/`DCM2`: one review conversation, rendered under its anchor line. + +A resolved thread folds to a single line. That is not tidiness — an +unresolved conversation is a thing the reviewer must act on, and rendering a +settled argument at the same weight buries the live one. The badge still says +who and how many, so nothing is hidden, only demoted; the same +demote-never-hide contract `DVN2` holds for noise. + +Indented to the code's column so the conversation reads as belonging to that +line rather than to the file. +*/ +private uint threadBlock(ref Builder b, in AnchoredThread t, int gutterWidth, + in DiffViewOptions opt) @safe +{ + import sparkles.syntax.md.render_widgets : MdViewOptions, viewMarkdownInto; + import sparkles.ui.geometry : Insets; + import sparkles.ui.style : TextStyle; + + const pad = gutterWidth > 0 ? gutterWidth + 2 : 2; + auto indent = new char[](pad); + indent[] = ' '; + + if (t.resolved) + { + const who = t.comments.length ? t.comments[0].author : "someone"; + return b.add(Widget(kind: WidgetKind.rich, spans: [ + TextSpan(indent.idup, slot: Slot.gutter), + TextSpan(text("✓ resolved — ", t.comments.length, + t.comments.length == 1 ? " comment by " : " comments, from ", + who), slot: Slot.muted), + ])); + } + + auto rows = new uint[](0); + foreach (i, ref c; t.comments) + { + TextSpan[] head = [TextSpan(indent.idup, slot: Slot.gutter)]; + head ~= TextSpan(i == 0 ? "▌ " : "│ ", slot: Slot.chromeAccent); + head ~= TextSpan(c.author.idup, slot: Slot.chromeAccent, + textStyle: TextStyle(bold: true)); + if (c.when.length) + head ~= TextSpan(" " ~ c.when.idup, slot: Slot.muted); + if (t.outdated && i == 0) + head ~= TextSpan(" (outdated)", slot: Slot.muted); + rows ~= b.add(Widget(kind: WidgetKind.rich, spans: head)); + + if (c.body_.root.children.length) + { + // Indented by PADDING, not by a prefix span: a comment body wraps, + // and a prefix only ever lands on the first row — leaving the rest + // hanging out at the code's own column, which reads as code. + MdViewOptions mopt; + rows ~= b.add(Widget(kind: WidgetKind.column, + children: [viewMarkdownInto(b, c.body_, mopt)], + padding: Insets(0, 0, 0, pad + 2))); + } + } + return b.container(WidgetKind.column, rows); +} + /** `DPR2`: the session header — what this change is, then its description. @@ -480,6 +574,7 @@ private uint sessionHeader(ref Builder b, const SessionHeader h, SideRenderer render, in DiffViewOptions opt) @safe { import sparkles.syntax.md.render_widgets : MdViewOptions, viewMarkdownInto; + import sparkles.ui.geometry : Insets; import sparkles.ui.style : TextStyle; auto rows = new uint[](0); @@ -759,7 +854,15 @@ private uint viewHunk(ref Builder b, const ref DiffDoc doc, in Hunk hunk, rows ~= hunkHeader(b, hunk); foreach (ref row; doc.hunkRows(hunk)) + { rows ~= viewRow(b, doc, row, gutterWidth, opt); + // `DPR3`: the conversation goes UNDER the line it is about, the way + // a reviewer reads it — not in a margin, and not in a separate pane + // where the code it refers to is no longer on screen. + foreach (ref t; opt.threads) + if (anchoredHere(t, row)) + rows ~= threadBlock(b, t, gutterWidth, opt); + } return b.container(WidgetKind.column, rows); } @@ -1477,3 +1580,99 @@ import sparkles.ui.style : Slot; } assert(!plainText.canFind("feat: a thing")); } + +@("diff_view.threads.anchorUnderTheirLineAndFoldWhenResolved") +@safe unittest +{ + import diff_session : AnchoredThread, buildDiffSession, ThreadComment; + import sparkles.syntax.md.model : MdBlock, MdBlockKind, MdDoc, MdInline, + MdInlineKind, Span; + + static MdDoc prose(string text) @safe + { + MdDoc d = { + source: text, + root: MdBlock(kind: MdBlockKind.document, children: [ + MdBlock(kind: MdBlockKind.paragraph, span: Span(0, text.length), + inlines: [MdInline(kind: MdInlineKind.text, + span: Span(0, text.length))]), + ]), + }; + return d; + } + + auto doc = diffText("one\ntwo\nthree\n", "one\nTWO\nthree\n", "f.d", "f.d"); + auto session = buildDiffSession(doc); + session.threads = [ + AnchoredThread(path: "f.d", line: 2, resolved: false, + comments: [ThreadComment("reviewer", "2026-08-07", + prose("this needs a why"))]), + AnchoredThread(path: "f.d", line: 3, resolved: true, + comments: [ThreadComment("reviewer", "2026-08-06", + prose("settled long ago"))]), + ]; + + auto tree = viewDiffDoc(doc, DiffViewOptions.init, null, null, session); + + const(char)[] all; + size_t threadRow = size_t.max, anchorRow = size_t.max; + foreach (i, ref n; tree.nodes) + { + const(char)[] row = n.text; + foreach (sp; n.spans) + row ~= sp.text; + all ~= row; + + import std.algorithm.searching : canFind; + + if (row.canFind("this needs a why")) + threadRow = i; + if (row.canFind("TWO")) + anchorRow = i; + } + + import std.algorithm.searching : canFind; + + assert(threadRow != size_t.max, "an unresolved thread renders its comments"); + assert(anchorRow != size_t.max && anchorRow < threadRow, + "the conversation goes UNDER the line it is about"); + assert(all.canFind("reviewer") && all.canFind("2026-08-07")); + + // A resolved thread demotes to one line: still there, no longer shouting. + assert(!all.canFind("settled long ago"), "a resolved body folds away"); + assert(all.canFind("resolved"), "but the badge says it exists"); + + // Threads belong to their own file: another path's conversations are not + // borrowed by this one. + session.threads[0].path = "other.d"; + auto elsewhere = viewDiffDoc(doc, DiffViewOptions.init, null, null, session); + const(char)[] other; + foreach (ref n; elsewhere.nodes) + foreach (sp; n.spans) + other ~= sp.text; + assert(!other.canFind("this needs a why")); +} + +@("diff_view.threads.anOutdatedThreadHasNoLineToHangOn") +@safe unittest +{ + import diff_session : AnchoredThread, buildDiffSession, ThreadComment; + + // GitHub reports an outdated thread with a null line, which decodes to + // zero. Zero must not match row 0 of anything — it means "nowhere". + auto doc = diffText("a\n", "b\n", "f.d", "f.d"); + auto session = buildDiffSession(doc); + session.threads = [AnchoredThread(path: "f.d", line: 0, outdated: true, + comments: [ThreadComment("reviewer", "2026-08-01")])]; + + auto tree = viewDiffDoc(doc, DiffViewOptions.init, null, null, session); + const(char)[] all; + foreach (ref n; tree.nodes) + foreach (sp; n.spans) + all ~= sp.text; + + import std.algorithm.searching : canFind; + + assert(!all.canFind("reviewer"), + "a thread with no line anchors nowhere rather than at the top"); +} diff --git a/apps/hue/src/forge.d b/apps/hue/src/forge.d index dfa8bec5b..908b07302 100644 --- a/apps/hue/src/forge.d +++ b/apps/hue/src/forge.d @@ -91,11 +91,15 @@ struct ForgeError alias ForgeResult(T) = Expected!(T, ForgeError); /// A request the transport is asked to make. Deliberately minimal: an adapter -/// composes the URL and the headers, and knows nothing about how they travel. +/// composes the URL, the headers and any body, and knows nothing about how +/// they travel. struct HttpRequest { string url; string[] headers; /// `"Name: value"`, ready to send + /// Empty means GET. A body makes it a POST — which is what a GraphQL + /// query is, and the only reason this seam knows about methods at all. + string body_; } /// What came back. @@ -140,6 +144,38 @@ struct PullRequest PrFile[] files; } +/// One message in a review conversation. +struct Comment +{ + string author; + string body_; /// markdown, as the author wrote it + string createdAt; +} + +/// Which side of the diff a thread hangs on. A comment on a removed line +/// belongs to the old text, and putting it under the new one would attach it +/// to a line its author never saw. +enum ThreadSide : ubyte +{ + newSide, + oldSide, +} + +/// A review conversation anchored to a file and a line (`DPR3`). +struct CommentThread +{ + string path; + /// 1-based line on `side`. Zero when the forge could not place it — + /// an outdated thread whose line no longer exists. + uint line; + ThreadSide side; + bool resolved; + /// The code it was written against has changed since. Still worth + /// showing; no longer worth anchoring precisely. + bool outdated; + Comment[] comments; +} + // ── The adapter vocabulary ────────────────────────────────────────────────── /// Is `T` a forge adapter? The core surface every adapter owes: which hosts diff --git a/apps/hue/src/forge_client.d b/apps/hue/src/forge_client.d index bde7eea80..e182ac768 100644 --- a/apps/hue/src/forge_client.d +++ b/apps/hue/src/forge_client.d @@ -16,9 +16,9 @@ module forge_client; import expected : err; -import forge : discoverToken, ForgeError, ForgeErrorKind, ForgeResult, - HttpRequest, HttpResponse, parsePrTarget, PrRef, PullRequest, - repoFromRemote, RepoId; +import forge : CommentThread, discoverToken, ForgeError, ForgeErrorKind, + ForgeResult, hasCapability, HttpRequest, HttpResponse, parsePrTarget, + PrRef, PullRequest, repoFromRemote, RepoId; import forge_github : GitHubForge; version (HueCurl) @@ -44,7 +44,12 @@ ForgeResult!HttpResponse fetchHttp(in HttpRequest req) @system import std.string : indexOf, strip; auto http = HTTP(req.url); - http.method = HTTP.Method.get; + // A body makes it a POST — the GraphQL query `DPR3` sends. `setPostData` + // sets the method as well as the payload, so the two cannot disagree. + if (req.body_.length) + http.setPostData(req.body_, "application/json"); + else + http.method = HTTP.Method.get; foreach (h; req.headers) { const colon = h.indexOf(':'); @@ -140,6 +145,13 @@ struct FetchedPr PullRequest pr; RepoId repo; string patch; + /// `DPR3`: the review conversations, when the forge has the capability + /// and the session is authenticated. Empty is a legitimate answer — no + /// threads, or no token — and never a reason to fail the whole fetch. + CommentThread[] threads; + /// Why there are no threads, when that is worth saying (an anonymous + /// session cannot read them at all). Empty when nothing went wrong. + string threadsNote; } /** @@ -171,8 +183,26 @@ ForgeResult!FetchedPr fetchPullRequest(string target) @system if (fetched.hasError) return err!FetchedPr(fetched.error); - return ForgeResult!FetchedPr(FetchedPr(fetched.value, pr.repo, - assemblePatch(fetched.value.files))); + FetchedPr out_ = { + pr: fetched.value, + repo: pr.repo, + patch: assemblePatch(fetched.value.files), + }; + + // `DPR3`: threads are a CAPABILITY, probed by presence — a forge without + // one simply lacks the member and this block compiles away. A failure + // here degrades the session to a thread-less review rather than losing + // the diff the reviewer came for. + static if (hasCapability!(GitHubForge, "reviewThreads")) + { + auto threads = forge.reviewThreads(pr, (in HttpRequest req) + => fetchHttp(req)); + if (threads.hasError) + out_.threadsNote = threads.error.toString(); + else + out_.threads = threads.value; + } + return ForgeResult!FetchedPr(out_); } // ── Tests ─────────────────────────────────────────────────────────────────── diff --git a/apps/hue/src/forge_github.d b/apps/hue/src/forge_github.d index bcd65d7e7..4080d860a 100644 --- a/apps/hue/src/forge_github.d +++ b/apps/hue/src/forge_github.d @@ -15,10 +15,12 @@ module forge_github; import expected : err; -import forge : ForgeError, ForgeErrorKind, ForgeResult, HttpRequest, - HttpResponse, isForge, PrFile, PrRef, PullRequest, RepoId, Transport; +import forge : Comment, CommentThread, ForgeError, ForgeErrorKind, + ForgeResult, HttpRequest, HttpResponse, isForge, PrFile, PrRef, + PullRequest, RepoId, ThreadSide, Transport; -import sparkles.wired.policy : CaseStyle, WireCase, WireName, WireOptional; +import sparkles.wired.policy : CaseStyle, WireCase, WireInvalid, + WireName, WireOptional; /// GitHub's own page size for the files endpoint, and the cap on how many /// pages hue will walk. 3000 files is GitHub's own hard limit for a PR diff; @@ -56,6 +58,71 @@ struct GitHubForge /// applyable suggestion (`DCM4`'s first emitter). enum suggestionFence = "suggestion"; + /** + `DPR3`: the PR's review conversations, with their resolved state. + + GraphQL rather than REST, and not by preference: REST's review comments + carry no notion of a resolved THREAD at all — only individual comments and + `in_reply_to_id` links to rebuild the grouping from. Resolution is the + thing that decides whether a conversation folds to a badge or demands + attention, so a view built on REST would have to show every settled + argument at full size forever. + + That costs authentication: GitHub's GraphQL endpoint refuses anonymous + requests entirely, where its REST endpoints serve public repositories. So + a tokenless session still reads the diff and simply has no threads — + `noAuth`, which the caller can report once rather than per thread. + */ + ForgeResult!(CommentThread[]) reviewThreads(in PrRef pr, scope Transport http) + @system + { + import std.conv : text; + + if (token.length == 0) + return err!(CommentThread[])(ForgeError(ForgeErrorKind.noAuth, + "review threads need a token (GitHub's GraphQL API refuses " + ~ "anonymous requests)")); + + const query = `{"query":"query($o:String!,$n:String!,$p:Int!){` + ~ `repository(owner:$o,name:$n){pullRequest(number:$p){` + ~ `reviewThreads(first:100){nodes{isResolved isOutdated path line ` + ~ `diffSide comments(first:50){nodes{body createdAt ` + ~ `author{login}}}}}}}}","variables":{"o":"` ~ pr.repo.owner + ~ `","n":"` ~ pr.repo.name ~ `","p":` ~ text(pr.number) ~ `}}`; + + auto res = post(graphqlUrl, query, http); + if (res.hasError) + return err!(CommentThread[])(res.error); + return decodeThreads(res.value); + } + + /// The GraphQL endpoint beside `apiBase`. On github.com the REST root is + /// `api.github.com` and GraphQL hangs off the same host; an Enterprise + /// instance puts it beside `/api/v3` as `/api/graphql`. + private string graphqlUrl() const @safe pure + { + import std.string : endsWith; + + return apiBase.endsWith("/api/v3") + ? apiBase[0 .. $ - 3] ~ "graphql" : apiBase ~ "/graphql"; + } + + /// One POST, with the failure vocabulary applied (`DPR6`). + private ForgeResult!string post(string url, string body_, scope Transport http) + @system + { + string[] headers = [ + "Accept: application/vnd.github+json", + "Content-Type: application/json", + "User-Agent: hue", + "Authorization: Bearer " ~ token, + ]; + auto res = http(HttpRequest(url, headers, body_)); + if (res.hasError) + return err!string(res.error); + return classify(res.value, url, true); + } + /** Fetches a pull request: metadata, description and file list. @@ -280,6 +347,106 @@ ForgeResult!(PrFile[]) decodeFiles(scope const(char)[] json) @safe return typeof(return)(out_); } +// GraphQL answers in camelCase, which is D's own spelling — so unlike the +// REST shapes above these need no recasing, only the `body` keyword dodge. + +private struct GqlAuthor +{ + @WireOptional() string login; +} + +private struct GqlComment +{ + @WireOptional() GqlAuthor author; + @WireOptional() @WireName("body") string body_; + @WireOptional() string createdAt; +} + +private struct GqlCommentNodes +{ + @WireOptional() GqlComment[] nodes; +} + +private struct GqlThread +{ + @WireOptional() bool isResolved; + @WireOptional() bool isOutdated; + @WireOptional() string path; + /// Null for a thread whose line no longer exists. That is not a + /// malformed answer, it is exactly what `isOutdated` reports — so the + /// field takes the default rather than rejecting the whole payload and + /// costing the reviewer every OTHER thread in it. + @WireOptional(onInvalid: WireInvalid.useDefault) uint line; + @WireOptional() string diffSide; + @WireOptional() GqlCommentNodes comments; +} + +private struct GqlThreadNodes +{ + @WireOptional() GqlThread[] nodes; +} + +private struct GqlPullRequest +{ + @WireOptional() GqlThreadNodes reviewThreads; +} + +private struct GqlRepository +{ + @WireOptional() GqlPullRequest pullRequest; +} + +private struct GqlData +{ + @WireOptional() GqlRepository repository; +} + +private struct GqlResponse +{ + @WireOptional() GqlData data; +} + +/** +Decodes a `reviewThreads` GraphQL answer (`DPR3`). + +A GraphQL error arrives with HTTP 200 and an `errors` array, so a decode that +only looked at the status would report success on a refusal. An answer with no +`repository` is that case: it becomes a `malformed` failure rather than an +empty thread list, because "no threads" and "the query was rejected" must not +look the same to a reviewer. +*/ +ForgeResult!(CommentThread[]) decodeThreads(scope const(char)[] json) @safe +{ + import std.string : indexOf; + + auto res = parse!GqlResponse(json); + if (res.hasError) + return err!(CommentThread[])(res.error); + + const nodes = res.value.data.repository.pullRequest.reviewThreads.nodes; + if (nodes.length == 0 && json.indexOf(`"errors"`) >= 0) + return err!(CommentThread[])(ForgeError(ForgeErrorKind.malformed, + "the forge rejected the thread query")); + + CommentThread[] out_; + foreach (ref t; nodes) + { + CommentThread thread = { + path: t.path, + line: t.line, + // GraphQL says LEFT for the old side; anything else is the new + // one, which is also the right default for a missing field. + side: t.diffSide == "LEFT" ? ThreadSide.oldSide : ThreadSide.newSide, + resolved: t.isResolved, + outdated: t.isOutdated, + }; + foreach (ref c; t.comments.nodes) + thread.comments ~= Comment(c.author.login, c.body_, c.createdAt); + out_ ~= thread; + } + return ForgeResult!(CommentThread[])(out_); +} + private ForgeResult!T parse(T)(scope const(char)[] json) @safe { import std.json : JSONValue, parseJSON; @@ -441,3 +608,99 @@ private ForgeResult!T parse(T)(scope const(char)[] json) @safe assert(res.hasError && res.error.kind == ForgeErrorKind.rateLimited, "a rate limit must survive as itself, not as a generic failure"); } + +@("forge_github.decodeThreads.groupsWhatTheReviewerMustRead") +@safe unittest +{ + import forge : ThreadSide; + + // A real-shaped answer: one live thread on the new side, one resolved, + // and one outdated whose line GitHub reports as null. + enum json = `{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[ + {"isResolved": false, "isOutdated": false, "path": "src/a.d", + "line": 42, "diffSide": "RIGHT", "comments": {"nodes": [ + {"body": "why this way?", "createdAt": "2026-08-07T10:00:00Z", + "author": {"login": "reviewer"}}, + {"body": "because X", "createdAt": "2026-08-07T11:00:00Z", + "author": {"login": "author"}}]}}, + {"isResolved": true, "isOutdated": false, "path": "src/b.d", + "line": 7, "diffSide": "LEFT", "comments": {"nodes": [ + {"body": "settled", "createdAt": "2026-08-06T09:00:00Z", + "author": {"login": "reviewer"}}]}}, + {"isResolved": false, "isOutdated": true, "path": "src/c.d", + "line": null, "diffSide": "RIGHT", "comments": {"nodes": [ + {"body": "stale", "createdAt": "2026-08-05T09:00:00Z", + "author": {"login": "reviewer"}}]}}]}}}}}`; + + auto res = decodeThreads(json); + assert(!res.hasError, res.hasError ? res.error.toString() : ""); + const threads = res.value; + assert(threads.length == 3); + + assert(threads[0].path == "src/a.d" && threads[0].line == 42); + assert(threads[0].side == ThreadSide.newSide && !threads[0].resolved); + assert(threads[0].comments.length == 2); + assert(threads[0].comments[1].author == "author"); + + // A comment on a removed line belongs to the OLD text; anchoring it to + // the new one would attach it to a line its author never saw. + assert(threads[1].side == ThreadSide.oldSide && threads[1].resolved); + + // A null line decodes to zero rather than failing the whole answer — + // which is precisely what `isOutdated` is reporting. + assert(threads[2].outdated && threads[2].line == 0); +} + +@("forge_github.decodeThreads.aRejectedQueryIsNotAnEmptyList") +@safe unittest +{ + // GraphQL answers a refusal with HTTP 200 and an `errors` array, so a + // decode that trusted the status would report success. "No threads" and + // "the query was rejected" must not look the same to a reviewer. + enum refused = `{"data":{"repository":null},"errors":[ + {"message":"Could not resolve to a Repository."}]}`; + auto res = decodeThreads(refused); + assert(res.hasError && res.error.kind == ForgeErrorKind.malformed); + + // A genuinely empty list stays a success. + enum empty = `{"data":{"repository":{"pullRequest":{"reviewThreads": + {"nodes":[]}}}}}`; + auto none = decodeThreads(empty); + assert(!none.hasError && none.value.length == 0); +} + +@("forge_github.reviewThreads.needsATokenAndSaysSo") +@system unittest +{ + import forge : HttpRequest; + + // Unauthenticated: GitHub's GraphQL endpoint refuses outright, so this + // must be reported once — not attempted and failed per thread. + auto never = delegate ForgeResult!HttpResponse(in HttpRequest req) @system { + assert(false, "an anonymous thread query must not be sent"); + }; + auto anon = GitHubForge(); + auto res = anon.reviewThreads(PrRef(RepoId("github.com", "o", "r"), 1), + never); + assert(res.hasError && res.error.kind == ForgeErrorKind.noAuth); + + // With a token the query is a POST carrying a body, to the GraphQL root. + string url, body_; + auto capture = delegate ForgeResult!HttpResponse(in HttpRequest req) @system { + url = req.url; + body_ = req.body_; + return ForgeResult!HttpResponse(HttpResponse(200, + `{"data":{"repository":{"pullRequest":{"reviewThreads": + {"nodes":[]}}}}}`)); + }; + auto gh = GitHubForge(token: "t"); + assert(!gh.reviewThreads(PrRef(RepoId("github.com", "o", "r"), 9), + capture).hasError); + assert(url == "https://api.github.com/graphql"); + assert(body_.length != 0, "a GraphQL query travels as a body"); + + import std.algorithm.searching : canFind; + + assert(body_.canFind(`"p":9`) && body_.canFind(`"n":"r"`), + "the variables name the PR being asked about"); +} diff --git a/docs/specs/hue/diff-view.md b/docs/specs/hue/diff-view.md index fadb13696..81841ec3a 100644 --- a/docs/specs/hue/diff-view.md +++ b/docs/specs/hue/diff-view.md @@ -230,15 +230,15 @@ _removed_ side at all. ## Pull-request viewing (`DPR`) — second wave -| ID | Requirement | Status | Traces to | -| ---- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| DPR1 | `hue --pr ` must fetch a pull/merge request **read-only** through the forge seam (`DPR7`) and open it as a diff session. Fetch is **native**: the forge's REST/GraphQL API directly from D — no `gh`/`glab` binary dependency; the token is discovered per forge (`$GITHUB_TOKEN` / `$GH_TOKEN`, then `gh`'s own config file as a courtesy; analogous sources per adapter), never prompted for interactively. On Android — no environment variables — the token comes from the config file ([`CFG12`](./config.md)), documented as plain-text storage. | full (`a677f13e`) | `--pr `: `forge_client` resolves the target against the checkout's remote (`origin`, then `upstream`), picks the adapter by host, discovers a token and fetches. Native over libcurl (`std.net.curl`) — no `gh`binary; reading`gh`'s `hosts.yml` is a config file, and hue never runs the tool. libcurl is opt-in per dub configuration (`libs "curl"`+`versions "HueCurl"`), so the Android and unittest builds link none and `fetchHttp`reports`unsupported` instead of not existing. NOT done: the Android config-file token path (`CFG12`) | -| DPR2 | A PR session = **description** (rendered through the markdown preview — dogfooding), **metadata** (author, state, branches, checks), and the **file list** as a `DVS4` diff session. | full (`7f512bef`) | `assemblePatch` restores the `diff --git`/`---`/`+++` preamble the forge omits, so the PR's files become exactly the input the engine's parser takes — a PR session IS a `DVS4` diff session, same model, same noise layers, same renderers. `DiffSession.header` carries title/state/author/branches plus the description as an `MdDoc`, rendered through hue's own markdown view (the diff's `DVM5` fence renderer doubles as the markdown view's, so a fence in a description is highlighted by the pipeline highlighting the diff below it). A session HEADER, not a PR header: nothing in it names a forge, so `DPR3` and the `W` wave reuse it | -| DPR3 | **Review comment threads** anchored to file+line render as inline blocks under their anchor line in the diff (Gerrit `gr-diff` prior art); resolved threads fold to a one-line badge. The thread-block widget is shared with local comments (`DCM2`). | not started | proposed thread blocks | -| DPR4 | **Revision comparison** — diff between pushed revisions of the PR, surviving force-pushes (Reviewable prior art) — researched, not in the second wave's first cut. | not started | deferred | -| DPR5 | **Stacked-PR awareness** (Graphite / av / git-spice / ReviewStack prior art) — researched only; this spec records the model but commits to nothing. | not started | deferred; research catalog | -| DPR6 | Degradation: no network / no auth / unrecognized-forge remote must produce a clear error, never a crash; rate-limited fetches surface as such. | full (`556b575c`) | A closed `ForgeErrorKind` vocabulary (`unknownRemote`/`noAuth`/`notFound`/`rateLimited`/`network`/`malformed`/`unsupported`), because the user's next move differs per kind. The distinctions that matter and are tested: the same 404 means "log in" without a token and "you cannot see this" with one; GitHub answers a rate limit with 403, the same status as a permission denial. Targets that cannot resolve never become a request | -| DPR7 | **Forge interface** (architecture): the PR session is built against a **forge seam**, not the GitHub API — a Design-by-Introspection adapter vocabulary where each forge type implements the core surface (resolve remote → repo, fetch PR/MR metadata + file list + threads) and declares **optional capabilities by presence** (revision timelines, suggestion syntax, draft reviews, stack metadata), probed by introspection à la git-spice's optional interfaces — never `if (forge == …)` branches in session/UI code. GitHub is the first adapter; **GitLab, Gitea, Forgejo, Codeberg** must be addable as adapters only. Forge detection from the remote URL (host mapping user-extendable via config, since self-hosted instances have arbitrary hosts); a capability a forge lacks degrades that one feature with an in-band notice. | full (`556b575c`) | `apps/hue/src/forge.d` is the seam and `forge_github.d` its first adapter: `isForge!T` states the core surface, `hasCapability!(T, name)` probes optional ones by PRESENCE (git-spice's pattern) so a forge without a capability lacks the member rather than stubbing it. The transport is injected (`Transport`), which is what leaves URL composition, pagination, decoding and every failure path under test without a socket; `forge_client` holds the three things that must touch the outside world | +| ID | Requirement | Status | Traces to | +| ---- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| DPR1 | `hue --pr ` must fetch a pull/merge request **read-only** through the forge seam (`DPR7`) and open it as a diff session. Fetch is **native**: the forge's REST/GraphQL API directly from D — no `gh`/`glab` binary dependency; the token is discovered per forge (`$GITHUB_TOKEN` / `$GH_TOKEN`, then `gh`'s own config file as a courtesy; analogous sources per adapter), never prompted for interactively. On Android — no environment variables — the token comes from the config file ([`CFG12`](./config.md)), documented as plain-text storage. | full (`a677f13e`) | `--pr `: `forge_client` resolves the target against the checkout's remote (`origin`, then `upstream`), picks the adapter by host, discovers a token and fetches. Native over libcurl (`std.net.curl`) — no `gh`binary; reading`gh`'s `hosts.yml` is a config file, and hue never runs the tool. libcurl is opt-in per dub configuration (`libs "curl"`+`versions "HueCurl"`), so the Android and unittest builds link none and `fetchHttp`reports`unsupported` instead of not existing. NOT done: the Android config-file token path (`CFG12`) | +| DPR2 | A PR session = **description** (rendered through the markdown preview — dogfooding), **metadata** (author, state, branches, checks), and the **file list** as a `DVS4` diff session. | full (`7f512bef`) | `assemblePatch` restores the `diff --git`/`---`/`+++` preamble the forge omits, so the PR's files become exactly the input the engine's parser takes — a PR session IS a `DVS4` diff session, same model, same noise layers, same renderers. `DiffSession.header` carries title/state/author/branches plus the description as an `MdDoc`, rendered through hue's own markdown view (the diff's `DVM5` fence renderer doubles as the markdown view's, so a fence in a description is highlighted by the pipeline highlighting the diff below it). A session HEADER, not a PR header: nothing in it names a forge, so `DPR3` and the `W` wave reuse it | +| DPR3 | **Review comment threads** anchored to file+line render as inline blocks under their anchor line in the diff (Gerrit `gr-diff` prior art); resolved threads fold to a one-line badge. The thread-block widget is shared with local comments (`DCM2`). | full (`1ef35569`) | Threads fetched over GraphQL, not REST: REST carries no notion of a resolved THREAD at all, only comments and `in_reply_to_id` links — and resolution is what decides whether a conversation folds. That costs auth (GitHub's GraphQL refuses anonymous requests), so a tokenless session reads the diff with no threads, reported once. `diff_session.AnchoredThread` is forge-neutral and lives with the session, so `DCM2`'s local comments produce the same value and no forge type reaches the renderer. Bodies are markdown through hue's own view; a resolved thread folds to a badge; a thread on the old side hangs on the line its author saw. Verified live on `dlang/dmd#23530`. NOT done: the split layout renders threads only in unified | +| DPR4 | **Revision comparison** — diff between pushed revisions of the PR, surviving force-pushes (Reviewable prior art) — researched, not in the second wave's first cut. | not started | deferred | +| DPR5 | **Stacked-PR awareness** (Graphite / av / git-spice / ReviewStack prior art) — researched only; this spec records the model but commits to nothing. | not started | deferred; research catalog | +| DPR6 | Degradation: no network / no auth / unrecognized-forge remote must produce a clear error, never a crash; rate-limited fetches surface as such. | full (`556b575c`) | A closed `ForgeErrorKind` vocabulary (`unknownRemote`/`noAuth`/`notFound`/`rateLimited`/`network`/`malformed`/`unsupported`), because the user's next move differs per kind. The distinctions that matter and are tested: the same 404 means "log in" without a token and "you cannot see this" with one; GitHub answers a rate limit with 403, the same status as a permission denial. Targets that cannot resolve never become a request | +| DPR7 | **Forge interface** (architecture): the PR session is built against a **forge seam**, not the GitHub API — a Design-by-Introspection adapter vocabulary where each forge type implements the core surface (resolve remote → repo, fetch PR/MR metadata + file list + threads) and declares **optional capabilities by presence** (revision timelines, suggestion syntax, draft reviews, stack metadata), probed by introspection à la git-spice's optional interfaces — never `if (forge == …)` branches in session/UI code. GitHub is the first adapter; **GitLab, Gitea, Forgejo, Codeberg** must be addable as adapters only. Forge detection from the remote URL (host mapping user-extendable via config, since self-hosted instances have arbitrary hosts); a capability a forge lacks degrades that one feature with an in-band notice. | full (`556b575c`) | `apps/hue/src/forge.d` is the seam and `forge_github.d` its first adapter: `isForge!T` states the core surface, `hasCapability!(T, name)` probes optional ones by PRESENCE (git-spice's pattern) so a forge without a capability lacks the member rather than stubbing it. The transport is injected (`Transport`), which is what leaves URL composition, pagination, decoding and every failure path under test without a socket; `forge_client` holds the three things that must touch the outside world | ## Staging & editing (`DST`) — third wave @@ -317,7 +317,7 @@ multi-file git session to hang it on. | V5 | Structural pass: classification oracle + opt-in structural view + commutativity profiles | full (`17d0ad14`) | `DVN3`, `DVN7` | | V6 | Rendered-preview diff for markdown (full block coverage) | full (`f86c66ca`) | `DVN6` | | P0 | Forge seam + GitHub adapter; PR session: description, metadata, file-list diff | full (`a677f13e`) | `DPR1`, `DPR2`, `DPR6`, `DPR7` | -| P1 | Inline review-comment threads | not started | `DPR3` | +| P1 | Inline review-comment threads | full (`1ef35569`) | `DPR3` | | W0 | Staging: stable ids, hunk/line stage/unstage, selection modes, discard, display/apply invariant | not started | `DST1`–`DST4`, `DST6` | | W1 | Inline editing of the worktree side (**requires the `UIA9` editor component first**) | not started | `DST5`; [`UIA9`](./ui-architecture.md) | | W2 | Local comments: content anchors, thread blocks, `refs/hue/data` store + viewed marks | not started | `DCM1`, `DCM2`, `DCM5` |