Skip to content

fix(github): append and edit review comments in an existing pending review - #909

Merged
matt2e merged 2 commits into
mainfrom
pending-code-review
Aug 6, 2026
Merged

fix(github): append and edit review comments in an existing pending review#909
matt2e merged 2 commits into
mainfrom
pending-code-review

Conversation

@matt2e

@matt2e matt2e commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Clicking GitHub on a review comment failed with a 422 (user_id can only have one pending review per pull request) whenever the user already had a draft review open on the PR, and the comment was lost. POST /pulls/{n}/comments creates a submitted one-comment review, so GitHub has to implicitly open a second pending review and refuses.

Posting

REST has no endpoint that appends to an existing pending review, so the inline path now checks for the viewer's draft up front and, when there is one, adds the comment with the GraphQL addPullRequestReviewThread mutation. Behaviour with no draft open is unchanged — the comment still lands immediately as a standalone submitted comment. The 422 is also retried through the append path, covering the race where a review is started between the lookup and the POST. A failed lookup is non-fatal and degrades to today's behaviour rather than blocking the post.

The comment ID comes from fullDatabaseId (a BigInt scalar, hence a JSON string), keeping the REST ID so the stored #discussion_r{id} anchor still resolves.

Editing

The REST review-comment endpoints do not list comments belonging to a draft review, and whether PATCH /pulls/comments/{id} reaches one anyway is undocumented. The edit path now works either way: PATCH is still tried first, so a published comment costs no extra round trip; a 404 on a review comment is treated as "possibly still a draft" and retried through GraphQL. Since GraphQL takes a node ID and only the REST database ID is persisted, the viewer's pending review is scanned (first 20 pending reviews, first 100 comments) for the matching fullDatabaseId. Not finding it — including when the draft was discarded, leaving the stored ID dangling — reports the original REST failure, which is the more useful error.

UI

A comment that joined a draft review is invisible to everyone else until the review is submitted, so GitHubCommentResult gains a non-persisted pending flag and the diff modal shows a toast instead of a silent checkmark.

Also in github.rs

  • Add a shared graphql_request helper that surfaces errors[].message. GraphQL fails with HTTP 200 and an errors array, which neither existing call site checked, so failures deserialized into useless missing-field errors. update_pull_request is retrofitted onto it, replacing its hand-rolled quote escaping with proper JSON variables.
  • Paginate fetch_pr_diff_lines. It read only the first default-size page of /pulls/{n}/files, so on a PR with more than 30 changed files, later files looked entirely outside the diff and an in-diff comment got a hard 422 instead of the inline post.
  • Delete the unreachable sync_review_to_github and its helpers. It was never registered as a Tauri command, and its delete-then-recreate strategy is strictly worse than appending: it destroys the draft if the recreate fails and renumbers every comment.

Testing

Unit tests cover the 422 conflict matcher against the real response body, the mutation input for single-line vs multi-line spans, awkward comment bodies surviving as variables, fullDatabaseId parsing, GraphQL error extraction, Link header parsing, and the node-ID scan (match, another author's draft, empty/absent cases).

Verified: just fmt-check lint typecheck test test-frontend (653 Rust, 611 frontend).

🤖 Generated with Claude Code

matt2e and others added 2 commits August 6, 2026 13:14
Clicking GitHub on a review comment failed with a 422 ("user_id can only
have one pending review per pull request") whenever the user already had a
draft review open on the PR, and the comment was lost. `POST /pulls/{n}/
comments` creates a *submitted* one-comment review, so GitHub has to
implicitly open a second pending review and refuses.

REST has no endpoint that appends to an existing pending review, so the
inline path now checks for the viewer's draft up front and, when there is
one, adds the comment with the GraphQL `addPullRequestReviewThread`
mutation. Behaviour with no draft open is unchanged: the comment still
lands immediately as a standalone submitted comment. The 422 is also
retried through the append path, covering the race where a review is
started between the lookup and the POST. A failed lookup is non-fatal — it
degrades to today's behaviour rather than blocking the post.

The comment ID comes from `fullDatabaseId` (a BigInt scalar, hence a JSON
string); GraphQL does not expose `databaseId` on a review comment. Keeping
the REST ID means later edits still PATCH in place and the stored
`#discussion_r{id}` anchor still resolves.

Since a comment added to a draft is invisible to everyone else until the
review is submitted, `GitHubCommentResult` gains a non-persisted `pending`
flag so the diff modal can say so instead of showing a silent checkmark.

Along the way, in the same file:

- Add a shared `graphql_request` helper that surfaces `errors[].message`.
  GraphQL fails with HTTP 200 and an `errors` array, which neither existing
  call site checked, so failures deserialized into useless missing-field
  errors. `update_pull_request` is retrofitted onto it, which also replaces
  its hand-rolled quote escaping with proper JSON variables.
- Paginate `fetch_pr_diff_lines`. It read only the first default-size page
  of `/pulls/{n}/files`, so on a PR with more than 30 changed files, later
  files looked entirely outside the diff and an in-diff comment got a hard
  422 instead of the inline post.
- Delete the unreachable `sync_review_to_github` and its
  `find_pending_review`/`delete_pending_review`/`get_current_user` helpers.
  It was never registered as a Tauri command, and its delete-then-recreate
  strategy is strictly worse than appending: it destroys the draft if the
  recreate fails and renumbers every comment.

Unit tests cover the 422 conflict matcher against the real response body,
the mutation input for single-line vs multi-line spans, awkward comment
bodies surviving as variables, `fullDatabaseId` parsing, GraphQL error
extraction, and `Link` header parsing.

Verified: `just fmt-check lint typecheck test test-frontend` (648 Rust,
611 frontend). Editing a comment that is still pending exercises
`PATCH /pulls/comments/{id}` and needs the live check from the plan's
manual verification steps.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Review of a851cb9 flagged that "later edits still PATCH in place" was
unverified: the REST review-comment endpoints do not list comments belonging
to a draft review, and whether `PATCH /pulls/comments/{id}` reaches one
anyway is undocumented. If it does not, editing a comment that landed in a
pending review fails until the review is submitted.

Rather than pin the behaviour down by hand on a live PR, the edit path now
works either way. `PATCH` is still tried first, so a published comment costs
no extra round trip; a 404 on a review comment is then treated as "possibly
still a draft" and retried through GraphQL, which can address draft comments.
GraphQL takes a node ID and all that is persisted is the REST database ID,
and there is no lookup by database ID, so the viewer's pending review is
scanned (first 20 pending reviews, first 100 comments) for the comment whose
`fullDatabaseId` matches. Not finding it — including when the user discarded
the draft, leaving the stored ID dangling — reports the original REST
failure, which is the more useful error, so a broken lookup cannot mask it.

An edit that goes down the GraphQL path reports `pending: true`, since it
only happens for a comment that is still a draft, and the diff modal's
message is now worded for an edit rather than a fresh post.

The `fullDatabaseId`/`url` parsing shared with `add_comment_to_pending_review`
moves into `parse_pending_comment_result`.

The review's second point — that the pending toast is one-shot, with no
lasting signal that a draft is still unsubmitted — is left as it was, per its
own suggestion to ship the toast and see whether it confuses anyone before
persisting the flag or reflecting draft state on refresh.

Unit tests cover the node-ID scan matching on the REST ID, skipping another
author's draft, and the empty/absent cases, plus the shared result parsing.

Verified: `just fmt-check lint typecheck test test-frontend` (653 Rust, 611
frontend).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
@matt2e
matt2e requested review from baxen and wesbillman as code owners August 6, 2026 04:51

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4cb762e76

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

client,
token,
MUTATION,
serde_json::json!({ "input": build_add_thread_input(gh_comment, review_node_id) }),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Recheck the PR head before adding to a pending review

When this pending-review path is used, the earlier current_head_sha check in post_single_comment_to_github is no longer tied to the actual write: addPullRequestReviewThread only receives path/line through this input, while the REST path still sends commit_id. If another client pushes to the PR after the head check but before this mutation, GitHub resolves the line against the newer head and the app records a successful comment that can be attached to the wrong line. Please revalidate the PR head immediately around this mutation and fail or delete the draft comment if it changed.

Useful? React with 👍 / 👎.

@matt2e
matt2e merged commit de32363 into main Aug 6, 2026
4 checks passed
@matt2e
matt2e deleted the pending-code-review branch August 6, 2026 05:23
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