GitDesktop / Tickets / #146 feat(pulls,github,mcp): manage PR stacks and retarget base branches

Anonymous - 2026-08-05

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with Cloudflare Pages

Latest commit: c659371

Status: ✅ Deploy successful!

Preview URL: https://d8c9015b.gitdesktop.pages.dev

Branch Preview URL: https://feat-stacked-prs-phase-2.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

Context for reviewers — deliberate calls and probe-verified facts, numbered for reference:

What this is. Wave 2 of [#142]: the stack write side on GitHub (create / add / dissolve via a contextual offer + inline preview in the PR view, three palette commands, three MCP remote-write tools), base-branch retargeting in the Edit dialog on all three forges, and a hardened list stack join (tri-state + pagination).

Unstack is dissolve-only by API design — live-probed: POST /stacks/{n}/unstack returns 204 and dissolves the whole stack; the pull_requests request body is completely ignored (probed with a middle member, the top member, a PR not in the stack, and a nonexistent number — every variant dissolved everything). There is no partial-remove primitive, so the app ships Dissolve behind a destructive confirm; per-PR remove is deferred with a home (backlog: Stacked-PRs Phase 3).

PATCH-base on a stacked PR hard-fails server-side — live-probed 422 with the detail nested in errors[]: "Cannot change the base branch because the pull request is part of a stack." The Edit dialog pre-gates known-stacked GitHub PRs (select disabled + a visible dissolve-first explanation); the stackUnknown arm lets the server arbitrate. Because gh discards errors[] detail to a bare "Validation Failed (HTTP 422)" on stderr, every gh-api write in this PR routes through one helper that recovers the detail from the response body. (Probe note: the create/add validation messages ride top-level message and already survive gh's stderr — the helper is concept-level hardening, not a bug fix.)

Chain detection mirrors the GitLab inference rules rule-for-rule (unique parent; shared-base and branching ambiguity poison the whole connected component; cycles yield nothing) with GitHub's write rules on top: append on top only (insert-at-bottom probed 422), native stacks only, minimum 2 to create. The attach check is the stack's top member — merged members still count toward size, so a stack whose top merged offers nothing (fail-closed). A race surfaces the server's own message verbatim in the offer's error line.

The list stack join is fail-open for decoration, fail-closed for offers. Stack data decorates a list that must render regardless (Wave-1 contract), but a failed join now marks rows stackUnknown and the offer refuses to render — a failed probe can no longer mint a false "create stack" preview over rows that are actually stacked. A join timeout therefore suppresses offers entirely; deliberate — the preview must never assert what it can't know.

The stacks read paginates now because GET /stacks retains merge-dissolved stacks as open:false forever (live-probed) — a long-lived repo's first page fills with residue. Still bounded by STACKS_TIMEOUT, past which the list renders without decoration — which, per item 5, now reads as unknown rather than "no stacks".

The GitHub loop was dogfooded live against a real repo: create → Stack section swap, add with the position-offset preview (offset confirmed on screen), dissolve → offer re-detection, and a forge-confirmed base retarget.

The GitLab and Bitbucket retarget arms were live-verified on scratch repos through this PR's own MCP path — a GitLab MR target-branch flip and a Bitbucket PR destination flip, title/description preserved by the fetch-to-preserve logic both times.

Not live-verified: merge-queue interactions (enqueued outcomes, merge_action) — merge queues are org-repo-only and the fixtures are personal repos. Unchanged posture from [#142], same backlog home.

base rides the wire as an explicit null when unset (the repo's optional-invoke-arg idiom — IPC serialization drops undefined keys; serde reads null as None). All three arms send their base / target_branch / destination field only when set; unset is unit-tested byte-identical to today's requests on all three.

stack-chains.ts ships without unit tests — the repo has no frontend test runner (standing posture). The Rust twin (infer_mr_stacks) is unit-tested, and the TS carries comments kept diffable against the Rust rules.

Pre-existing, not this PR: three clippy --all-targets lints in #[cfg(test)] code (generate.rs:2901/2927 MutexGuard-held-across-await; read_git.rs:431) — measured on two separate runs during this build, invisible to the repo's standard clippy gate (no --all-targets), on record in the backlog.

Accepted limitation: the edit dialog's base picker offers local branches (the create-PR picker's composition, reused deliberately). A remote-only branch isn't offered (the current base always stays selectable); a never-pushed local branch fails server-side with a legible error per item 3.

Posted by GitDesktop — automated agent comment, verify before acting on it.

Related

Tickets: ~~#142~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Adds the GitHub stack write side (create / add / dissolve via a contextual offer, three palette actions, three MCP tools), base-branch retargeting on all three forges, and turns the list's stack join into a tri-state (stackUnknown) with a paginated --slurp read. The design is sound and unusually well fenced — fail-closed offers, a bounded decoration read, -F used only on formatted u64s, and all four doc surfaces (README, capabilities.ts, help guide, changelog.d/) carried in the same change. Nothing blocking; one should-fix in the chain detector.

Correctness

should-fix — src/lib/git/stack-chains.ts, detectStackOffer (the sources map, ~line 80): parent links are matched on branch name alone, and the rows it consumes carry no cross-repo marker (PR_LIST_FIELDS in src-tauri/src/github/pr.rs:1640 requests only headRefName/baseRefName), so a fork PR is indistinguishable from a same-repo one. Concrete case, common in any public repo: PR [#10] arrives from a contributor's fork with head branch main into base main; your PR [#12] is head feature-x, base main. sources["main"] = [10], so [#12] gets exactly one parent candidate (not ambiguous), [#10] is parentless (its own base main resolves only to itself and is filtered), and the component [10, 12] is a clean unstacked chain → a "create stack" offer whose preview names a stranger's fork PR as the stack bottom. confirmStackOffer (RemotePrView.tsx:518) then sends stackOffer.members to forge_stack_create → gh_stack_create, which the forge rejects because [#10]'s head branch doesn't exist in this repo — after a preview that asserted a chain that was never there. Fix: add isCrossRepository (a gh pr list --json field, as is headRepositoryOwner) to PR_LIST_FIELDS, add #[serde(default)] pub cross_repository: bool to PrInfo (github/pr.rs:534) — which obliges setting it false in every literal construction site: from_bb_pr (forge/bitbucket.rs), from_glab_mr (forge/gitlab.rs), rest_pull_to_pr_info (github/pr.rs), and the two test builders (pr_row in pr.rs tests, the gitlab test info() helper) — mirror it as crossRepository?: boolean on PrInfo in src/lib/git/types.ts, and in detectStackOffer drop those rows from rows alongside the existing state filter (a PR whose head lives in another repository can never be a stack member), extending the doc comment's rule list with that exclusion.

Edge cases

nit — src/features/pulls/RemotePrView.tsx:516,545 (stackWriteError / cancelStackOffer): the create/add mutation error is cleared only by Cancel or by the PR-change effect at :557, while StackOffer resets expanded on its own offerKey change (StackSection.tsx, ~line 300). A failed create, then a chain change under the same PR (another PR opened on top), collapses the preview but keeps the error — re-expanding shows the previous chain's forge message above a different preview. Concrete fix: export a stackOfferKey(offer) helper from stack-chains.ts, use it both for StackOffer's lastKey and in the parent's existing reset effect (useEffect(..., [number, offerKeyOrEmpty])), so the two identities can't drift.

nit — src/features/pulls/RemotePrView.tsx:500: detection sees at most 100 open PRs, so on a repo with more, a chain member outside that page turns what should be an "add to stack #N" into a "create stack" over the visible tail; consider suppressing the offer when offerList.data.length === 100 (a possibly-truncated page), matching the fail-closed posture the stackUnknown path already takes.

Readability

nit — src/lib/error-summary.ts:50: stripGitPrefix now strips gh: too; rename to stripToolPrefix (both call sites are in firstMeaningfulLine, lines 59 and 61) so the name matches the generalized doc comment.

nit — src/features/pulls/RemotePrView.tsx:2680+ (BaseBranchField) re-implements SelectField (src/components/form/fields.tsx:173) verbatim, including the sizeToContent popup sizing (alignItemWithTrigger={false}, w-auto min-w-(--anchor-width) max-w-[28rem]); extracting SelectField's presentational core into a form-agnostic component both use would keep the two from drifting.

nit — src-tauri/src/forge/bitbucket.rs: edit_pr(… target: Option<&str>) calls build_edit_body(… base: Option<&str>) for the same value that every other arm calls base; rename the parameter to base and update the edit_pr doc line that says "when target is given".

Recorded decisions (acknowledged, not re-flagged)

Notes 5–6 (a timed-out paginated stacks probe now yields no badges rather than a partial first page), note 11 (stack-chains.ts untested — no frontend runner; the Rust twin infer_mr_stacks carries the tested rules), and note 13 (edit-dialog base picker offers local branches only) all remain recorded calls; nothing in the diff contradicts them.

Posted by GitDesktop — AI output, verify before acting on it.

Related

Tickets: ~~#10~~
Tickets: ~~#12~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

No genuinely exploitable security vulnerabilities in these changes: the new gh/glab writes go through argv (no shell) with values carried as -f key=value raw fields (the project's established guard against gh's @-magic file read), the one new -F use interpolates only formatted u64s, Bitbucket's destination rides a serde_json::json! body, all three new MCP tools call ensure_remote_write() consistently with the existing destructive-annotated forge tools (and the permission copy was updated to disclose them), and the new frontend surfaces render forge-supplied strings as escaped JSX text with the destructive dissolve behind a confirm.

Posted by GitDesktop — AI output, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

Re the AI review — all five items land in the next push, plus one addition its findings prompted:

Fork PRs: isCrossRepository joins the list projection with a serde alias so the wire stays crossRepository; detectStackOffer excludes cross-repo rows outright, and both directions of the alias are pinned by a test (a silent name mismatch would have marked every fork PR same-repo). Class boundary, on record: GitLab's inference shares the name-collision hazard but its MR list carries no project ids — display-only badges there, deferred with a home in the backlog.

Truncation: accepted with a severity correction — it's worse than reported. A stack bottom's base can be any branch, so the visible tail of a truncated page forms a valid partial stack that succeeds silently rather than being rejected. detectStackOffer now fails closed at the page limit via a shared exported constant (the guard and the list fetch read the same value).

stripToolPrefix renamed as suggested.

SelectControl extracted from SelectField's presentational core; SelectField is now a thin form-bound wrapper and the edit dialog's base picker uses the same component — no more parallel markup to drift.

Bitbucket param renamed to base (the URL local is api_base now).

New, prompted by (1)+(2): the two new refusal conditions are now named in the user docs (help guide + README) — fork pull requests never join a stack, and on a repository whose open-PR list fills the page the offer stays quiet rather than guess from a truncated view. The stackUnknown arm stays covered in spirit ("a chain it can't see whole") rather than by name — it's an internal probe failure, not a property of the repo. The changelog fragment deliberately keeps benefit-level wording.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Re-reviewed against the current diff: every previous finding is addressed, and the fix hunks themselves are clean apart from one accessibility gap in the offer preview and a small type-duplication nit. Nothing blocking.

Resolved since last review

Fork PRs poisoning chain detection — PR_LIST_FIELDS now requests isCrossRepository (github/pr.rs:1646), PrInfo.cross_repository carries the serde alias plus camelCase wire name (:543), every literal construction site sets it (from_bb_pr, from_glab_mr, rest_pull_to_pr_info, both test builders), types.ts:1489 mirrors it, and detectStackOffer drops those rows alongside the state filter with the doc's rule list extended. Both alias directions and the projection are pinned by new tests.

Offer-identity drift — offerIdentity is exported from stack-chains.ts:44 and drives both StackOffer's lastKey (StackSection.tsx:325) and the parent's write-reset key (RemotePrView.tsx:565-572), so the two identities can't diverge.

Truncated-page offer — STACK_OFFER_PAGE_LIMIT is the single source for both the usePrList limit (RemotePrView.tsx:498) and the fail-closed guard (stack-chains.ts:100), with the reasoning corrected to the stronger "a partial stack would succeed silently" case.

stripGitPrefix naming — renamed to stripToolPrefix with both call sites (error-summary.ts:59,61) and the doc comment updated.

BaseBranchField duplicating SelectField — SelectControl extracted; BaseBranchField now calls it with sizeToContent, and I verified the trigger keeps className="w-full" (fields.tsx:217) and the popup keeps alignItemWithTrigger={false} + w-auto min-w-(--anchor-width) max-w-[28rem], so the picker is visually unchanged. The now-dead useId / Label / Select* imports are gone from RemotePrView.tsx with no remaining uses, and isNativeStack has exactly one definition (stack-chains.ts:14) with every importer updated.

Bitbucket target parameter — renamed to base, the URL local renamed api_base, the edit_pr doc line reworded, and the sole call site (forge/mod.rs:1577) updated.

Accessibility

should-fix — src/features/pulls/StackSection.tsx:384-409 (StackOffer preview): the preview scroller is max-h-48 overflow-y-auto (12rem) over plain div rows (~29px each: py-1.5 + text-xs + border), so a chain of 7+ members overflows with no focusable descendant anywhere inside. Its sibling at :203 gets away with the same cap because its rows are <button>s wired to listKeyboardNav, so focus scrolls it; here a keyboard-only user cannot reach the hidden members — and this preview is the pre-write safety check the whole offer is built around (the auto-focused Confirm at :416 is the very next stop). Chromium 127+ makes scrollers focusable by default, so this reproduces specifically on the macOS WKWebView build. Fix: put tabIndex={0} on the scroller at :387 plus an accessible name, e.g. role="group" with aria-label={offer.kind === "create" ? "Pull requests to stack, bottom to top" : \Pull requests to add to stack #${offer.stackNumber}`}— DOM order puts the new tab stop before Confirm, so the expand-timeconfirmRef.current?.focus()still wins the initial focus. Knock-on: the comment at:384-386` currently asserts "nothing here is selectable, so it takes no keyboard nav" — reword it to say the container is focusable purely so its overflow is scrollable by keyboard, while the rows stay non-interactive.

Readability

nit — src/components/form/fields.tsx:256-262: SelectField's prop type is a hand-copied duplicate of SelectControl's, so a prop added to the control is silently missing from the bound wrapper; derive it instead — }: Omit<ComponentProps<typeof SelectControl>, "value" | "onValueChange">) { — adding type ComponentProps to the existing react type import.

Copilot's only remaining comment (stale stack-write error surviving an offer change, StackSection.tsx:335) is fixed in the current diff by the shared offerIdentity key; no need to act on it.

The author's reply records that a reset during a genuinely in-flight write clears the pending flag without aborting the request, and that the settled write still reports — that remains a recorded decision and I'm not re-flagging it. Documentation is complete across all four surfaces (README's GitLab/MCP/stacked-PR paragraphs, capabilities.ts, the help guide's GitHub/GitLab/Bitbucket/MCP sections, and changelog.d/added-stack-management.md); site/src/pages/index.astro has no stacks FeatureRow to go stale.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

Re the re-review — both remaining items land in the next push:

Preview scroller a11y: the offer preview's scroll container is now a labeled tab stop (tabIndex={0}, role="group", an offer-specific aria-label naming the create/add intent) so a 7+-member chain is keyboard-reachable on WebKit too; the rows stay non-interactive, DOM order is unchanged so the expand-time focus still lands on Confirm, and the comment above it now states the real constraint (focusable purely for keyboard-scrollable overflow — this preview is the pre-write safety check). Matches the existing focusable-scroller pattern in PrTasksSection.

SelectField prop type derived via Omit<ComponentProps<typeof SelectControl>, "value" | "onValueChange"> — a prop added to the control can no longer go silently missing from the bound wrapper.

One honesty note carried forward: the WebKit focusability difference is applied as documented behavior — the dev target here is Chromium-based, so the macOS keyboard pass stays on the live-check list.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Both items from the last round are fixed, and the fix hunks are clean; the only thing left is a one-line styling consistency nit on the new tab stop.

Accessibility

nit — src/features/pulls/StackSection.tsx:397-398: the new focusable scroller carries no focus styling (className="mt-1.5 max-h-48 overflow-y-auto border"), so it falls back to the UA ring, unlike the repo's three other focusable container divs which all pin it explicitly — BitbucketDefaultReviewersSection.tsx:73, CollaboratorsSection.tsx:158,218, GitLabMembersSection.tsx:142, each outline-none focus-visible:ring-1 focus-visible:ring-ring. Match them: className="mt-1.5 max-h-48 overflow-y-auto rounded-md border outline-none focus-visible:ring-1 focus-visible:ring-ring" (drop rounded-md if the square border is intended here).

Resolved since last review

Offer-preview scroller unreachable by keyboard — StackSection.tsx:389-398 now wraps the rows in role="group" + tabIndex={0} with an offer-specific aria-label (create/add branches narrow cleanly off offer.kind, so offer.stackNumber is well-typed in the add arm), the rows stay non-interactive, DOM order is unchanged so the expand-time confirmRef.current?.focus() still wins, and the comment above it now states the real constraint instead of the now-false "takes no keyboard nav".

SelectField prop-type duplication — fields.tsx:255 derives from Omit<ComponentProps<typeof SelectControl>, "value" | "onValueChange"> with type ComponentProps added to the existing react type import; the destructured names still match the derived keys exactly, useId/ReactNode remain used by the other fields, and SelectField passing field.state.value into SelectControl's required value: string preserves the old value || null behavior (the falsy-to-null coercion now lives in SelectControl).

Nothing further to raise — the rest of the diff is unchanged since the last round and re-verified as reported there.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

Re the styling nit — accepted with a correction on the record: the three files cited as precedent don't exist in this tree (grep: no BitbucketDefaultReviewersSection.tsx, CollaboratorsSection.tsx, or GitLabMembersSection.tsx). The repo's actual idiom — on the very tabIndex={0} precedent this scroller was matched to, PrTasksSection.tsx:101, and its interactive siblings — is outline-none focus-visible:ring-1 focus-visible:ring-ring/50 (with the /50 opacity, not bare ring-ring). Applied in that form, square border kept to match this file's own sibling list. One line; rides the closing push.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

The only change since the last round is the one-line focus-ring class on the offer preview's scroller, which resolves the sole outstanding nit; the fix hunk introduces no collateral.

Resolved since last review

Offer-preview scroller focus styling — StackSection.tsx:398 now carries outline-none focus-visible:ring-1 focus-visible:ring-ring/50. The /50 opacity is the right call for this file: every focusable element in src/features/pulls/ uses it (PrTasksSection.tsx:101, ReviewThreads.tsx:632,843, LinkedIssuesField.tsx:218,395). One correction for the record only — the three repo-settings files cited as precedent last round do exist in this tree (BitbucketDefaultReviewersSection.tsx:73, CollaboratorsSection.tsx:158,218, GitLabMembersSection.tsx:142, all bare ring-ring); that doesn't change the fix, which correctly matches the closer features/pulls idiom. No other class was touched, DOM order and the role="group" / aria-label / tabIndex triple are unchanged, so the expand-time confirmRef.current?.focus() still wins.

Nothing further to raise — the rest of the diff is unchanged from the previous round and re-verified as reported there (spot-checked usePrList(repo, enabled, state, limit, lens) against the new offer call site's argument order, and presentError / onError / defaultBranch are all in scope at their new use sites in RemotePrView.tsx).

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

Correction to my previous comment, for the record: the reviewer is right and I was wrong — all three cited files exist at src/features/repo-settings/ (BitbucketDefaultReviewersSection.tsx:73, CollaboratorsSection.tsx:158,218, GitLabMembersSection.tsx:142, bare ring-ring). My "no such files" claim came from a wrong path guess plus a truncated file-list grep — my error, not a reviewer fabrication. The applied fix is unchanged and unaffected: ring-ring/50 per this file's own family (PrTasksSection, ReviewThreads, LinkedIssuesField), which the re-review confirms as the right call here.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

feat(pulls,github,mcp): manage PR stacks and retarget base branches

Milestone

Searches

Help

#146 feat(pulls,github,mcp): manage PR stacks and retarget base branches

Stack writes (GitHub)

Stack join becomes tri-state

Base-branch retargeting

MCP surface

Frontend

Documentation

Discussion

Deploying gitdesktop with Cloudflare Pages

Related

Correctness

Edge cases

Readability

Recorded decisions (acknowledged, not re-flagged)

Related

Resolved since last review

Accessibility

Readability

Accessibility

Resolved since last review

Resolved since last review

Latest commit:	`c659371`
Status:	✅ Deploy successful!
Preview URL:	https://d8c9015b.gitdesktop.pages.dev
Branch Preview URL:	https://feat-stacked-prs-phase-2.gitdesktop.pages.dev