Menu ▾ ▴

#23 feat(pr,review-threads,git): inline review threads anchored in diff, apply suggestions

closed
nobody
enhancement (85)
2026-07-06
2026-07-05
Anonymous
No

Originally created by: theBGuy
Originally owned by: theBGuy

This change introduces native, file-anchored review threads for GitHub, GitLab, and Bitbucket PRs/MRs, rendering line-anchored review comments (including suggestions) grouped by file in the conversation and directly under their exact lines in the Files diff. It also adds the ability to apply GitHub reviewer suggestions directly to the working tree (with file validation and staging), and brings AI Generate to the PR/MR edit dialog. The goal is to make GitDesktop handle review flows with the fidelity (and more) of the hosted platforms, unifying inline code conversations, suggestions, and bot/AI reviews locally.

Inline Review Threads and Conversation UI

  • Adds file:line-anchored review threads UI (src/features/pulls/ReviewThreads.tsx, src/features/pulls/RemotePrView.tsx, src/features/pulls/RemotePrViewParts.tsx), rendering threads grouped by file in the PR conversation and anchored under the exact diff line in the Files view via DiffSurface.tsx.
  • Review threads support reply, resolve/unresolve, copy-as-markdown (including all context), collapse of resolved threads, "outdated" badges, and full diff/hunk excerpt rendering.
  • Supports rendering and interacting with suggestions, including suggested change diffs and Apply buttons in the UI.
  • Handles quote-reply linkage to the main composer (useLocalConversation.ts, DiscussionView.tsx, RemoteIssueView.tsx).

Provider Backends for Review Threads

  • Adds unified ReviewThreadOut structure in src-tauri/src/github/pr.rs and provider glue in src-tauri/src/forge/model.rs, src-tauri/src/forge/mod.rs for reading threads, replying, and resolving—wired for GitHub, GitLab, and Bitbucket.
  • Groups Bitbucket inline comments into threads (src-tauri/src/forge/bitbucket.rs) and parses GitLab discussions/notes for anchoring and resolution (src-tauri/src/forge/gitlab.rs).
  • Hooks up the appropriate forge_pr_review_threads, forge_pr_thread_reply, forge_pr_thread_resolve commands in the Rust API and surfaces their readiness via the Implemented flags.

Suggestion Apply (Local Commit Suggestion)

  • Implements a generic, robust backend for suggestion-application (src-tauri/src/git/ops.rs, src/lib/git/api.ts, src/lib/git/queries.ts, src/lib/git/types.ts), validating the suggested lines before touch, preserving encoding and EOLs, and conditionally staging.
  • Wires up Apply buttons in the UI (shown only when branch and thread state allow), with appropriate disabled reasons and post-apply messages.
  • Documents user-facing and changelog aspects (changelog.d/added-apply-suggestions.md).

AI Generate for PR/MR Edit

  • Adds the Generate button to the PR/MR edit dialog (src/features/conversations/EditTitleBodyDialog.tsx, src/features/pulls/LocalPrView.tsx, src/features/pulls/RemotePrView.tsx), so existing PRs (including those without a checked-out branch) get the same AI title/description flow as creation.
  • Surfaces Generate functionality for forks and remotes lacking a local head branch, referencing the correct diff and subject context.
  • Documents the new capability (changelog.d/added-pr-edit-generate.md, README).

Documentation, Help, and Polish

  • Updates README.md, help text, capability lists, and changelogs for new features and usage patterns, including visual and behavioral details of inline threads.
  • Details behaviors for copying, collapsing, and editing review threads.
  • Fixes expand/collapse synchrony and path tooltip polish for threads (changelog.d/fixed-review-threads-polish.md).
  • Fixes Bitbucket thread pagination and conversation filtering bugs (changelog.d/fixed-bb-thread-pagination.md, changes to Bitbucket backend).

Hooks, Data, Library

  • Updates diff rendering library and surfaces a lineAnchors extension API in src/features/diff/DiffSurface.tsx for in-diff review thread annotation.
  • Refactors or augments related conversation, reaction, and UI state flows as needed for correct review-thread support and interaction.

These changes collectively bring a full-featured, file-anchored review thread experience—matching and in some areas surpassing hosted platform UIs—for all three providers, and introduce local suggestion-application to the working tree, along with major workflow and polish improvements.

Related

Tickets: #142

Discussion

  • Anonymous

    Anonymous - 2026-07-05

    Originally posted by: theBGuy

    AI review (sonnet) · automated

    This is a large, coherent feature that correctly threads review comments from all three forges through a single neutral type, anchors them in the diff view, and implements a robust local "apply suggestion" primitive on the backend. No blockers. A few real issues below.

    Correctness

    should-fix · src/features/pulls/RemotePrViewParts.tsx PrFilesPane · lineAnchors useMemo
    The dependency array [effectivePath, threads, onQuote, onReply, onResolve, apply] includes onReply, onResolve, onQuote, and apply.onApply — all constructed as fresh arrow function literals on every RemotePrView render:

    onReply={canThreadReply ? (threadId, body) => threadReply.mutateAsync({...}) : undefined}

    Every parent re-render (polling, mutation status updates, etc.) creates new arrow references, which busts the lineAnchors memo, which in turn busts the extendData memo in RenderedDiff. The author's own comment says "a fresh object each render would thrash it", so this directly undermines the intended stability guarantee. The fix is useCallback on each of the four callbacks in RemotePrView before they're passed down (or stable mutation references from a wrapper).

    should-fix · src-tauri/src/forge/bitbucket.rs review_threads
    Only the first page (100 comments) is fetched for Bitbucket inline threads. The group_bb_threads algorithm needs the full parent-id topology — including deleted/pending comments — to resolve reply chains. Comments on page 2+ are absent from the topology, so any live reply whose intermediate-parent was on page 2 is silently dropped, and entire threads opened after the first 100 comments disappear. view_pr has the same single-page cap, but at least there group_bb_threads sees a consistent slice. Consider adding a simple pagination loop (two or three pages max, like GitLab's five-page cap in fetch_mr_discussions).

    nit · src-tauri/src/forge/gitlab.rs review_threads (line 2245) 

    } else {
    (position.old_path.clone(), 0, "new")  // ← side should be "old"
}

    The fourth branch fires when only old_path is present and old_line is None — but maps the side as "new". This means a file-level (not line-level) comment on an old-side-only path will be bucketed on the wrong side of a split diff and won't anchor correctly. Should be "old".

    Edge Cases

    should-fix · src/features/diff/DiffSurface.tsx RenderedDiff extendData builder 

    for (const a of lineAnchors) {
  const target = a.side === "old" ? oldFile : newFile;
  target[String(a.line)] = { data: { render: a.render } };  // last-write wins
}

    If two DiffLineAnchor entries share the same side+line, the second silently overwrites the first. PrFilesPane pre-groups so this never fires today, but the interface (DiffLineAnchor) documents no such uniqueness constraint. Any future caller of GitDiffView that doesn't pre-group would silently lose anchors. At minimum, document the invariant on the interface; ideally, guard or merge inside extendData.

    Readability / Nits

    nit · src/features/pulls/ReviewThreads.tsx (line 884) 

    <p className="truncate font-mono text-xs text-muted-foreground">
  {path}
</p>

    Per the house "clipped text needs a tooltip" rule, a statically-truncated string should carry title={path}. The reviewer can't see the full path of a long file without expanding the thread.

    nit · src/features/pulls/ReviewThreads.tsx and src/features/pulls/RemotePrView.tsx
    SUBMIT_HINT = formatBinding("mod+enter") is defined identically in both files. Move to a shared location (e.g., @/lib/hotkeys/binding or a small ui-constants module) to avoid drift.

     

  • Anonymous

    Anonymous - 2026-07-05

    Originally posted by: theBGuy

    AI review (sonnet) · automated

    The "changes since last review" bundle three orthogonal bug fixes — trashing an open repo, narrowing a "not found" swallow in branch deletion, and the matching UI-side close-before-trash/reopen-on-failure logic — all on top of the unchanged inline-review-threads feature. The three new fixes are logically correct. The previous review findings on the thread feature itself are unaddressed by these changes.

    Resolved since last review

    None. The "changes since" diff is entirely about the repo-removal flow, not the review-thread feature. All previous findings carry forward; each is re-verified below.

    Correctness

    should-fix · src/features/pulls/RemotePrViewParts.tsx PrFilesPane · lineAnchors useMemo — still present
    onReply, onResolve, and apply are all fresh literals on every RemotePrView render. The new suggestionApply object is constructed inline (not memoized):

    const suggestionApply: SuggestionApply = {
  ...
  onApply: (a) => applySuggestion.mutateAsync({ ...a, stageWhenClean: true }),
};

    Every poll tick or repoStatus update creates new references, busting lineAnchors and the downstream extendData memo. Wrap the four callbacks and suggestionApply in useCallback/useMemo in RemotePrView.

    should-fix · src-tauri/src/forge/bitbucket.rs review_threads — could not verify
    The previous review flagged a single-page (100-comment) cap. The review_threads body is truncated in both the main diff and the "changes since" diff. If pagination was not added, threads opened after the first 100 comments remain invisible and their reply chains are silently broken.

    should-fix · src-tauri/src/github/pr.rs · gh_pr_merge call-site propagation — could not verify
    Copilot flagged (marked outdated by them) that gh_delete_remote_head_branch(...).await? at the call site in gh_pr_merge causes a successfully-merged PR to appear failed when branch deletion fails. The current diff correctly narrows the inner function's "not found" swallow (removing the too-broad || lower.contains("not found") arm), but the call-site ? is not visible in the diff. Warrants a manual check: if the caller still propagates the delete error with ?, a permission failure on deletion will show the user a toast error despite a clean merge.

    Edge Cases

    should-fix · src/features/diff/DiffSurface.tsx extendData builder — still present
    Two DiffLineAnchor entries on the same side+line silently overwrite each other (last-write wins). PrFilesPane pre-groups so this never fires today, but DiffLineAnchor documents no uniqueness contract. Any future caller that doesn't pre-group loses anchors silently. At minimum document the invariant on the interface.

    Nits

    nit · src/features/pulls/RemotePrViewParts.tsx · DiffThreadAnchor stale-closure toggle 

    setExpanded((prev) => ({ ...prev, [t.id]: !isExpanded(t) }))

    isExpanded(t) reads the render-closure expanded, not prev. Under rapid batched clicks the second toggle reads stale state and may flip the wrong direction. Fix: !(prev[t.id] ?? !t.isResolved).

    nit · src/features/pulls/ReviewThreads.tsx · truncated path — still present
    The {path} element with className="truncate …" lacks title={path}. Per the house rule, statically truncated user content needs a title for full-text visibility on hover.

    nit · ReviewThreads.tsx + RemotePrView.tsx — still present
    SUBMIT_HINT = formatBinding("mod+enter") is now defined identically in both files (the RemotePrView.tsx definition was introduced in this change). Move to a shared constant.

    nit · src-tauri/src/forge/gitlab.rs · side "old"/"new" branch — could not verify
    Previously flagged: the fallback branch when only old_path is present and old_line is None maps the side as "new" instead of "old". The gitlab.rs diff is truncated; cannot confirm whether this was fixed.

     

  • Anonymous

    Anonymous - 2026-07-05
     

  • Anonymous

    Anonymous - 2026-07-05

    Originally posted by: theBGuy

    AI review (sonnet) · automated

    This PR ships file-anchored review threads for GitHub/GitLab/Bitbucket — grouped in the Conversation tab and anchored in the diff — plus local apply-suggestion and AI Generate on the Edit PR dialog. The design is sound, the security controls in replace_file_lines are thorough, and most of the previous review's findings are resolved. Two real issues remain.

    Correctness

    should-fix — RemotePrView.tsx, line 1067 — "No activity yet." can appear simultaneously with review threads for GitLab and Bitbucket PRs that have only inline thread activity (no general notes, no reviews). The empty-state guard checks pr.reviews.length === 0 && pr.comments.length === 0 but is unaware of reviewThreads.data. For those providers pr.reviews is always [] (they emit no review entries), so a GitLab/Bitbucket MR with only diff-note threads shows a thread block above an incorrect "No activity yet." line.

    Fix: add && !reviewThreads.data?.length to the condition (optional-chain handles loading, empty array, and undefined cleanly).

    should-fix — src-tauri/src/github/pr.rs, gh_pr_review_threads — the GraphQL query fetches reviewThreads(first:100) with no cursor/pagination. A PR with more than 100 review threads (not uncommon on large Copilot reviews or code-heavy PRs with many comments) silently delivers only the first 100. This is a brand-new feature establishing a hard cap on its first version; adding a pageInfo { endCursor hasNextPage } loop (or at least a bounded 2–3-page follow) would keep parity with the Bitbucket and GitLab pagination story already present in this PR.

    Edge cases

    should-fix — src/features/pulls/ReviewThreads.tsx, parseHunk — the guard if (raw === "" && out.length === 0) continue; only skips a blank line at the start of a hunk body. A blank line later in the hunk (e.g. a genuine blank content line that GitHub renders without a leading space) reaches the else branch, is treated as a context line, and increments newNo. This would cause recoverOriginals to compute a startLine that is off by one per such blank line, making replace_file_lines reject the suggestion with a "lines no longer match" error rather than applying it. A conservative fix is to treat lines with no leading marker as context (i.e. add a " " prefix before storing in text, or skip blank raw lines explicitly).

    Nit

    nit — changelog.d/added-inline-review-threads.md, line 8 — the sentence "Previously they were invisible (GitHub) or shown as context-free flat comments (GitLab/Bitbucket)." starts mid-line immediately after a word-wrapped line, producing a broken visual in editors (though Markdown normalises it at render). Reflow to start that sentence on a fresh line.

    Resolved since last review

    • GitLab fallback arm of gl_thread_anchor now correctly returns "old" for the old-path-only case (was returning "new"), and the fix is covered by a new four-arm unit test. ✓
    • review_id field added to ReviewThreadOut, allowing review-body copy-markdown to include owned threads. ✓
    • GitHub review entries now populate id: r.id instead of String::new(), enabling the review_id lookup. ✓
    • quoteReply refs-in-render fix applied consistently across RemotePrView, DiscussionView, RemoteIssueView, and useLocalConversation. ✓
    • Bitbucket view_pr now filters out inline comments (they surface via review_threads) so they no longer appear context-free in the flat conversation list. ✓
     

  • Anonymous

    Anonymous - 2026-07-06

    Ticket changed by: theBGuy

    • status: open --> closed
     

Log in to post a comment.

MongoDB Logo MongoDB