BBC2-18 add bb pr review (-a / -r / -w) + surface approvals in bb pr view#17
Merged
Merged
Conversation
Four flat commands, each defaulting to the current branch's PR when no id is given: - bb pr approve / bb pr unapprove - bb pr request-changes / bb pr unrequest-changes Approve and request-changes are independent review states in Bitbucket — a reviewer can have an approval AND changes-requested from different users on the same PR simultaneously. The four commands wrap symmetric POST/DELETE pairs on /approve and /request-changes. Idempotency note: the spec doesn't document what re-approving an already-approved PR returns. We let any non-2xx propagate. The documented edge case — DELETE /approve returns 400 if the PR is already merged — surfaces as a clean PullRequestError; the command layer prints the message rather than a raw HTTP error. See docs/bb-notes.md → Approve / Unapprove / Request-changes for the endpoint details.
Two changes to make 'I approved it, now where did it go?' visible: - Backend PullRequestDetail now carries participants[] alongside reviewers[]. reviewers[] stays strictly role=REVIEWER (formal reviewers, as before). participants[] covers role=PARTICIPANT — including ad-hoc approvers on PRs with no assigned reviewers (the Snyk auto-PR case). Pure commenters show up with state=pending. - bb pr view gets a single APPROVALS entry on the top block with a one-line summary: 'N approved, M changes requested' aggregating both arrays. Minimal addition; the larger REVIEWERS vs PARTICIPANTS section split is deferred to its own ticket along with the full pr view redesign.
Replace the four flat commands (approve / unapprove / request-changes / unrequest-changes) with a single bb pr review taking mutually- exclusive flags: -a, --approve -r, --request-changes -w, --withdraw Matches gh's mental model of a single 'current review state' rather than Bitbucket's two-independent-toggles API. --approve and --request-changes implicitly clear the inverse state so the reviewer can flip cleanly; --withdraw clears whichever state is set. Backend functions unchanged — they remain the primitives the command layer composes. The command layer's secondary cleanup calls are best-effort (swallow PullRequestError) so a primary-action success isn't masked by an expected 'not currently in that state' failure on the cleanup DELETE.
b2l
changed the title
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.
Summary
Four flat subcommands for recording your own review state on a PR. Each defaults to the open PR for the current branch when no id is given.
bb pr approve [<id>]bb pr unapprove [<id>]bb pr request-changes [<id>]bb pr unrequest-changes [<id>]Approve and request-changes are independent states in Bitbucket — a reviewer can carry both simultaneously across users; the same user can carry one or the other (toggling).
Naming choice — call it out
The ticket left the naming of the request-changes inverse open. I went with
unrequest-changesfor symmetry withunapprove(consistentun-prefix). Ugly, but self-evident and matches what's already there. If you'd prefer e.g.withdraw-changesor a flag-basedbb pr request-changes --withdraw, easy to rename — say so before merge.Idempotency caveat
The ticket AC says re-running an action on an already-matching state shouldn't error. The Bitbucket spec doesn't document what re-approving / re-requesting actually returns, so I haven't coded a swallow. Smoke-test will reveal the actual behavior:
Same for the DELETE-on-never-matching cases.
The one documented error path that IS handled cleanly:
DELETE /approvereturns 400 when the PR is already merged. Test covers it.Implementation notes
postParticipantAction/deleteParticipantAction) — keeps the per-action code minimal while typing the discriminated path correctly.review.tsfile with a shared runner; the 4 exported run functions just bind the action and the success message. 4 separate files would have been 90% duplication.Test plan
bun src/index.ts pr approve <id>— UI shows your approval. Run again — confirm whether it errors or no-ops (this is the smoke-test gating idempotency).bun src/index.ts pr unapprove <id>— UI shows approval removed.bun src/index.ts pr request-changes <id>— UI shows changes-requested.bun src/index.ts pr unrequest-changes <id>— UI shows it cleared.bun src/index.ts pr unapprove <id>should fail with a clean message (not a stack trace).bun src/index.ts pr approve(no args) auto-detects.bun test(151 passing, 6 new).bun run lint.Out of scope (per ticket)
bb pr reviewcommand (could come later if the flat commands feel redundant).bb pr view.