✨ feat(tui): inline review comments in the rich PR view (GraphQL) - #532
Open
kbrdn1 wants to merge 5 commits into
Open
✨ feat(tui): inline review comments in the rich PR view (GraphQL)#532kbrdn1 wants to merge 5 commits into
kbrdn1 wants to merge 5 commits into
Conversation
`gh pr view --json comments` returns the conversation only. The comments anchored to a diff hunk, their path / line / diffHunk, and the reply chains inside a thread are reachable through GraphQL alone, so this adds a second transport rather than widening the field list. `ReviewThreads` is an enum, not a `Vec`: "this backend has no path to inline comments" and "the forge answered, there are none" are both terminal states that must render differently, and a bare list collapses them into one. `Forge::fetch_pr_threads` has no default implementation for the same reason, so a third backend has to answer the question instead of inheriting an empty list that reads as clean. The query refuses a slug it cannot split into owner/repo. `gh pr view` copes with an empty selector by resolving from the working directory; a GraphQL document cannot, and guessing the owner back would send the request to whichever instance is ambient. That is the #458 finding on a new transport. Every field carries `null_to_default`: `startLine` is null on every single-line anchor, which is the common case rather than an edge one. refs #528
Each thread renders as its anchor (`src/tui/app.rs:7-11`, plus `resolved` / `outdated` when it applies), the diff hunk it hangs from, then the reply chain, under the reviews section. Hunk lines are truncated, never wrapped. The wrap path splits on whitespace, so a wrapped `+` line's continuation rows carry no sigil and read as context; in a diff the leading `+` / `-` / space IS the meaning. A long hunk drops its head rather than its tail, because the forge puts the anchored line last. The test identifies hunk rows by their ROLE, not by their first character, since a filter that looks for a sigil cannot see a row that lost one. The fetch is a separate `TaskKind` with its own per-number cache, not a field on `PrStatus`: a PR refresh replaces its whole cache entry and would silently drop threads that had already landed. It is fired when the view opens rather than alongside every PR fetch, because `gwm status` calls the same fetch on every invocation and never reads these. Landing goes through one helper wired into BOTH the drain and the `apply_*` test seam, and re-runs `sync_rich_overlay` rather than touching the rows directly, so the view keeps rendering the side the link prefers. A backend with no forge resolved never marks `Loading`, which would otherwise leave the section spinning on a request nobody made. refs #528
The #420 entry closed on "inline review comments are not part of this change", and both roadmaps carried #528 as still tracked. The keybindings pages said the view fetches nothing beyond the status refresh, which the second transport makes untrue. Also states the GitLab behaviour explicitly: the section is present and says the backend cannot reach these, because that is a different fact from "this merge request has none". closes #528
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Every threads test went through `apply_pr_threads_fetch_result`, so the request itself, the worker and the drain arm were green-but-unrun: the App fixtures resolve no forge, and `spawn_github_pr_threads` returns early on exactly that. Both new tests drive a `gh` stand-in through the real spawn, as the bulk-refresh test already does, and assert on the result rather than on `is_loading` (issue #425: whether a task is still running at an arbitrary instant is a timing detail). The refresh test asserts on the CACHE, not on the rows. Deleting the re-spawn left it green, because `refresh_github_status` does not rebuild the overlay (a landing does), so the previous run's rows are still on screen either way. `Idle` is what the invalidation leaves behind when nothing asks again, and that is what the assertion now reads. Gate the re-spawn on the view being open, not on the overlay kind alone: `close_detail_overlay` leaves `kind` holding its last value, so a refresh that closed the overlay (the link moved) would still have fired a GraphQL request for a view no longer on screen. Also pins the GitLab side of the three-state contract at the backend: `fetch_pr_threads` answers `Unsupported` without spawning `glab`. Driving that end to end through the App needs a trusted GitLab config for the #458 origin reconciliation, which is more setup than the claim is worth. refs #528
…w-comments # Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The rich PR view (#420) shipped two of the three steps its issue laid out.
This is the third: the comments anchored to a diff hunk, which
gh pr view --json commentscannot return at all.Those threads, their
path/line/diffHunk, and the reply chainsinside them are reachable through GraphQL alone. So this is a second
transport, not a wider field list, and it is deliberately kept off the
existing PR fetch:
gwm statuscalls that same fetch on every invocationand has no use for review threads.
Closes #528
Type of change
Changes
Forge::fetch_pr_threadswith no default implementation, returning aReviewThreadsenum rather than aVec. "This backend has no path toinline comments" and "the forge answered, there are none" are both
terminal states that render differently, and a bare list collapses them
into one. GitLab answers
Unsupportedexplicitly.gh api graphqlquery + parser insrc/github.rs, flattening GraphQL'snodeswrappers into the shape the REST parser already produces, so onerenderer serves both transports.
gh pr viewcopes with an empty selector by resolving from the working directory; a
GraphQL document cannot, and guessing the owner back is the ✨ feat(forge): Forge trait and GitLab (glab) backend #458 finding
on a new transport.
rich_view: anchor (src/tui/app.rs:7-11, plusresolved/outdated), the diff hunk, then the chain.TaskKind+ own per-number cache, fired when the view opens.Tests
cargo testpasses locally (2700 passed, 0 failed)cargo fmt --checkpassescargo clippy --all-targets -- -D warningspassestests/(6 transport, 16 render, 5 wiring)Also re-ran the whole suite under a CI-like minimal PATH
(
PATH="$(dirname "$(command -v cargo)"):/usr/bin:/bin"): 2700 passed.Screenshots / TUI captures
Not captured. The section is text rows inside the existing overlay shell;
its layout is pinned by the row-builder tests rather than by a recording.
Checklist
<type>/#<issue>-<description>## [Unreleased]examples/gwm.toml.exampleupdated if config schema changed (no schema change)unwrap()on user-facing pathsprintln!in TUI render codeLinked issues / docs
Notes for reviewers
Where the payload shape comes from. Not from the issue, which quoted
snacks.nvim's Lua. It was read off a live response:gh api graphqlagainstPR #514 of this repo. Two things that came out of it and drove the code:
startLineisnullon every single-line anchor (the common case), andauthoris a nullable object. Every field carriesnull_to_default.Hunk lines are truncated, never wrapped.
wrap_linesplits onwhitespace, so a wrapped
+line's continuation rows carry no sigil andread as context; in a diff the leading
+/-/ space is the meaning.A long hunk drops its head rather than its tail, because the forge puts the
anchored line last.
That test is worth a second look: it identifies hunk rows by their role,
not by their first character. The first version filtered on the sigil, which
made half of it vacant, since a filter looking for a sigil cannot see a row
that lost one. Verified by reverting the fix and watching it fail on the
right assertion.
Landing paths. The threads land through one helper wired into both the
drain and the
apply_*test seam, and it re-runssync_rich_overlayratherthan touching rows directly. #420 hit that desync once with a warning two
lines below in the same file; this is the third landing path.
Pre-existing, not addressed here. A CodeRabbit body is mostly
<details>blocks and
<!-- fingerprinting:… -->comments, so a capped render showsHTML comments instead of the finding. That is already true of the
conversation section on
dev; fixing it is a markdown-stripping questionfor its own issue, not this one.