Skip to content

feat: pull request review and conversation actions (A5a) - #666

Merged
Tryanks merged 8 commits into
mainfrom
feat/pr-actions-review
Oct 9, 2026
Merged

Tryanks merged 8 commits into
mainfrom
feat/pr-actions-review

Conversation

@Tryanks

@Tryanks Tryanks commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Summary

client ── RunPullRequestAction{key, action} ──▶ runtime ── linked? ──▶ services::PullRequestReads (one cache owner)
          (never retained for redelivery)        │                     ├─ drop conversation, replies, labels, reviewers
                                                 │                     ├─ pre-reads: node id (REST, cached per PR);
                                                 │                     │   subject kind for edit / reaction (not for the PR's own id); fresh head (review)
                                                 │                     ├─ the write, interactive, sent once (labels/reviewers: in order → Applied | Partial)
                                                 │                     └─ drop the same four again, whatever the outcome
          ◀── PullRequestAction(Applied | Rejected(reason) | Partial | Uncertain)
                                                 └─ not Rejected → sync asked for an immediate read
client ── EditPullRequestReviewDraft{AddComment|EditComment|RemoveComment|SetBody|MoveToHead|Discard}
          ──▶ runtime: account read → anchor checked against the diff
                       → in the host queue: a draft with comments at another head refuses AddComment (pull_request_head_changed)
                       → SessionMeta.pull_request_reviews[(key, account)] (persisted, every client sees it; unlinking keeps it)
UI shows / submits only the draft whose account = conversation.account; an unanswered review clears on the first
conversation read that began after the submission landed.

Writes: comment (addComment), review (one POST pulls/N/reviews), reply and resolve/unresolve (mutations sent directly, as upstream), reactions (8), edit comment / title / body (unset fields not sent), SetLabels{add, remove} (one POST, then one DELETE per label), SetReviewers{add, remove} (one POST, one DELETE; users + already-requested teams).
The review's REST body carries commit_id (the draft's head, so a moved head is refused rather than re-anchored by GitHub) and start_line/start_side for multi-line comments: a deliberate addition over upstream, which sends neither.
UI (A5 spec §3, §4, §7): one editor (PrEditor, inline on desktop, bottom sheet on phone), composer, quote reply, Files line comments → pending cards → review bar → Review sheet (stale-head notice + Move to the new head, unplaced comments, uncertain notice), thread reply/resolve footer, reactions with a chip-shaped add control (hover / focus-visible on desktop), edit comment/description, title edit (Pencil on desktop, ⋯ menu on phone), MetaBlock + staged label/reviewer pickers, result toasts. "Add review comment" is disabled on a stale draft ("Move to the new head first"); Undo after removing a pending comment is offered only for a placed one.

Part of #641

Evidence

  • After: desktop light, Review sheet over a pending comment in Files (author-only verdicts):
    desktop
    phone light, Conversation with the chip-shaped add-reaction controls and no header pencil (title edit is in the ⋯ menu):
    phone

Tests (through the real GitHubApi against the in-process HTTP fixture):

  • conversation_writes_send_their_payloads_once_and_the_conversation_is_read_again: variables per mutation, title-only edit omits body, one REST node-id read, conversation dropped after a write
  • a_subject_of_another_pull_request_is_refused_before_any_mutation: foreign comment id → ForeignSubject, zero mutations
  • labels_are_added_in_one_request_and_removed_one_per_request_until_one_fails: one SetLabels → one POST, per-label DELETE (encoded), Partial, nothing after the failure, and both the candidates and the conversation are read again
  • reviewers_are_offered_without_the_author_and_requested_by_kind: one SetReviewers → POST of the additions, DELETE of the removals by kind; a refused DELETE → Partial
  • a_review_is_one_submission_at_the_head_read_fresh_and_a_moved_head_sends_nothing: exact REST body; moved head → StaleHead, no POST; unplaced comment never sent
  • a_write_left_unanswered_is_uncertain_and_never_sent_again: dropped connection and 502 → Uncertain, exactly one request each
  • runtime a_review_draft_is_the_hosts_across_a_restart_and_a_moved_head_keeps_it_until_moved: edits via the pipe, off-diff line refused, draft survives a store close/reopen, stale submission keeps it whole, a comment read at the new head is refused with pull_request_head_changed and the draft is unchanged, MoveToHead keeps the unchanged line and unplaces the changed one, submit clears it, sync asked
  • runtime a_review_draft_is_its_accounts_and_an_unanswered_submission_waits_for_a_later_read: a 502 review → draft marked uncertain → a conversation read clears it; under another account the conversation's account finds no draft, a submission sends nothing (Invalid), a new draft is kept beside the first, unlinking keeps both
  • client a_pull_request_action_in_flight_fails_on_disconnect_and_is_not_resent
  • conversation read test extended: author-only verdicts, triage label right, reviewer states

Mutation checks: removing the conversation drop after a label write, the stale-head refusal in the host queue, or the post-submission read clearing each fails its test (run locally); earlier: the fresh head read, the omit-unset-field logic, the redelivery exclusion.

Tests changed by the review corrections:

  • count("PullRequestSubject") == 5 dropped (step 2: a request count nothing depends on; the thread scope pre-read it counted is gone, upstream sends reply/resolve directly).
  • the ReplyToThread-with-a-comment-id → Invalid case dropped from the foreign-subject test (step 2: the contract was the removed pre-read; GitHub refuses a non-thread id itself, and the foreign-subject contract stays covered by the reaction and edit cases).
  • labels / reviewers / unanswered tests moved from AddLabels/RemoveLabels/RequestReviewers to the one-intent actions (step 3: the intent changed by request; the per-request contracts are kept and the labels test now also asserts the conversation re-read).

Checks on the final commit: cargo fmt --all --check ok; cargo clippy --workspace --all-targets --locked -- -D warnings ok; cargo nextest run --workspace --locked 1068 passed, 13 skipped; cargo machete clean.

Live (disposable private repo in the gh account, PR closed and branch deleted afterwards): comment, review with one inline comment (REST body confirmed via gh api), reply, resolve, reactions on a comment and the PR, foreign subject refused, comment edit, title-only edit (body unchanged on GitHub), labels add 2 / remove with one missing → Partial, reviewer request → GitHub's refusal (a personal repo has no other collaborator and the author cannot be requested, so the reviewer-request success path was not reachable live; it is covered only by the fixture test), all through the services; and through the app: pending comment → restart → still there → submitted; resolve; comment from the phone; label picker add+remove; head pushed under a pending review → "Review not submitted: #1 changed", draft kept → Move to the new head → unchanged line placed, changed line unplaced. After the corrections (read-only against the same closed PR, phone geometry): add-reaction chips, ⋯ → Edit title opens its sheet, Files ⋯ → Review… opens the Review sheet without a trigger.
Gaps: mobile/Web builds left to CI; the corrections' writes (SetLabels/SetReviewers, account binding) were not re-run against GitHub, only through the fixture; desktop hover reveal of the head-row add chip was not driven live.

Merge Danger

Door: two-way
Blast Radius: pull-requests
Wire changes (noted above PROTOCOL_VERSION, no bump): new commands and response, typed reaction content, SetLabels/SetReviewers actions, new conversation fields (serde-defaulted). Thread metadata gains pull_request_reviews (skipped when empty), each draft carrying its account. Writes go to GitHub as the host's account, single attempt.

…rom a host-owned draft, reply, resolve, react, edit, labels, reviewers
…scope, labels, stale head, no retry, the host's review draft
…ments against the diff, and read the account's rights, labels and reviewers
…view draft in Files, review sheet, thread reply and resolve, reactions, edits, labels and reviewers
…draft, and make every pull request write one intent that drops every read it may change

A new line comment on a draft at an older head is refused (pull_request_head_changed)
instead of dropped. Drafts carry the account the conversation names and are shown and
submitted only under it; unlinking keeps them. An unanswered review clears on a
conversation read that began after it. Labels and reviewers are one SetLabels /
SetReviewers intent answered Applied or Partial. Reply and resolve send without a
scope read; a reaction on the pull request itself skips the subject read.
… placed pending comment, chip-shaped reaction add revealed on hover, title edit from the phone menu, and the summary editor made when the Review sheet opens
@Tryanks
Tryanks marked this pull request as ready for review October 9, 2026 05:15
@Tryanks
Tryanks merged commit 90f2d3f into main Oct 9, 2026
7 checks passed
@Tryanks
Tryanks deleted the feat/pr-actions-review branch October 9, 2026 05:15
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