Skip to content

feat(hue): P1 — inline review-comment threads (DPR3) - #250

Merged
PetarKirov merged 3 commits into
mainfrom
feat/diff/p1-threads
Aug 7, 2026
Merged

feat(hue): P1 — inline review-comment threads (DPR3)#250
PetarKirov merged 3 commits into
mainfrom
feat/diff/p1-threads

Conversation

@PetarKirov

@PetarKirov PetarKirov commented Aug 7, 2026

Copy link
Copy Markdown
Owner

DPR3: a PR's review conversations render under the line they are about — the Gerrit gr-diff shape, where the code a comment refers to is still on screen while you read it.

Verified live on dlang/dmd#23530 (this repository has no PR with review threads to test against):

 7    - This has some side effects, it prevent separate compilation, function po
    6 + No attributes have been implemented to date; before they are considered,
    ▌ thewilsonator  2026-08-06
      what's with the semi colons? The first one should be full stop, the second
      one should be a comma.
    7 + This has some side effects: it prevents separate compilation, function p

GraphQL, 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 exactly what decides whether a conversation folds to a badge or demands attention, so a view built on REST would show every settled argument at full size forever.

That costs authentication: GitHub's GraphQL endpoint refuses anonymous requests where its REST endpoints serve public repositories. So a tokenless session still reads the diff and simply has no threads — reported once, as noAuth, rather than attempted and failed per thread. A test asserts the anonymous query is never even sent.

Threads are a capability, probed by presence, so the fetch compiles away for an adapter without one and a thread failure degrades to a thread-less review rather than losing the diff the reviewer came for.

A resolved thread folds to one line

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 — the same demote-never-hide contract DVN2 holds for noise.

A thread on the old side hangs on the removed line its author actually saw, never on the new one.

The widget is shared, and that had to be structural

AnchoredThread is forge-neutral and lives with the session, so DCM2's local comments will produce exactly the same value: no forge type reaches the renderer, and a locally-authored thread is not a second kind of thing. Comment bodies are markdown rendered through hue's own view — the one place it would be strange for markdown not to be markdown.

Three things the tests and the live run pinned

A GraphQL refusal arrives with HTTP 200 and an errors array, so a decode trusting the status reports success. "No threads" and "the query was rejected" must not look alike to a reviewer.

An outdated thread's line is JSON null. That is not a malformed answer — it is precisely what isOutdated reports — so the field takes its default instead of rejecting the payload and costing the reviewer every other thread in it. Zero then means "nowhere", which a test pins so it can never match row 0.

A prefix span only lands on the first row of a wrapped body. The live run showed the second and later lines of a multi-line comment hanging at the code's own column, where they read as code. Padding indents every row.

Not in this wave

The split layout renders threads only in unified — named in the spec row.

Testing

168 hue, 48 diff, 113 syntax, 284 ui — all green, plus prek run --all-files in the CI form. New coverage: thread decoding against a real-shaped payload (live, resolved, and outdated-with-null-line), the rejected-query case, the anonymous refusal, anchoring under the right row, per-file selection, and the resolved fold.

https://claude.ai/code/session_01YQJw7nA1exWvgh7w5tEZsu

GraphQL, 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 what decides whether a
conversation folds to a badge or demands attention, so a view built on REST
would show every settled argument at full size forever.

That costs authentication: GitHub's GraphQL endpoint refuses anonymous
requests where its REST endpoints serve public repositories. A tokenless
session therefore still reads the diff and simply has no threads - reported
once, as `noAuth`, rather than attempted and failed per thread. The test asserts
the anonymous query is never even sent.

Threads are a CAPABILITY, probed by presence, so the fetch compiles away for
an adapter without one and a thread failure degrades to a thread-less review
rather than losing the diff the reviewer came for.

Two shapes of wrongness the tests pin. A GraphQL refusal arrives with HTTP 200
and an `errors` array, so a decode trusting the status reports success - "no
threads" and "the query was rejected" must not look alike. And an outdated
thread's line is JSON null, which is not a malformed answer but exactly what
`isOutdated` reports: the field takes its default instead of rejecting the
payload and costing the reviewer every other thread in it.

The transport seam grows a body, which is the only reason it knows about
methods at all.

Claude-Session: https://claude.ai/code/session_01YQJw7nA1exWvgh7w5tEZsu
Where a reviewer reads them: under the anchor line, not in a margin and not in
a separate pane where the code they refer to has scrolled away (the Gerrit
`gr-diff` shape). A thread on the old side hangs on the removed line its
author actually saw, never on the new one.

A resolved thread folds to one line. 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 - the
demote-never-hide contract `DVN2` already holds for noise.

`AnchoredThread` is forge-neutral and lives with the session, so `DCM2`'s
local comments will produce exactly the same value: no forge type reaches the
renderer, and a locally-authored thread is not a second kind of thing. That is
what "the thread block is one widget" has to mean to be worth saying.

Comment bodies are markdown, rendered through hue's own view - the one place
it would be strange for markdown not to be markdown.

Verified live on dlang/dmd#23530 (this repository has no PR with review
threads to test against). The live run also corrected the indentation
mechanism: a prefix span only ever lands on the FIRST row of a wrapped body,
leaving the rest hanging at the code's own column where it reads as code.
Padding indents every row.

Claude-Session: https://claude.ai/code/session_01YQJw7nA1exWvgh7w5tEZsu
DPR3 full, so P1 closes and hue reads a PR's conversations as well as its
diff.

The row records why the fetch is GraphQL rather than REST - REST has no
resolved-thread concept at all, and resolution is the whole basis of the fold
- and what that costs: threads need a token where the diff does not.

NOT done and named: the split layout renders threads only in unified.

Claude-Session: https://claude.ai/code/session_01YQJw7nA1exWvgh7w5tEZsu
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

📄 Docs preview deployed to: https://feat-diff-p1-threads.sparkles-docs.pages.dev

@PetarKirov
PetarKirov merged commit f70f1aa into main Aug 7, 2026
12 checks passed
@PetarKirov
PetarKirov deleted the feat/diff/p1-threads branch August 7, 2026 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant