Menu ▾ ▴

#93 feat(pulls,automations,settings): expand draft controls and rerun automation

closed
nobody
enhancement (85)
2026-07-20
2026-07-20
Anonymous
No

Originally created by: theBGuy
Originally owned by: theBGuy

Expand pull-request draft workflows across GitHub, GitLab, and Bitbucket while making stopped automated reviews recoverable in-app. The changes reduce repetitive setup for draft PRs and preserve failed or cancelled automation runs so users can retry the exact review mode.

Pull-request draft workflows

  • Adds createPrsAsDraft to src/lib/settings/api.ts and exposes the Create pull requests as drafts toggle in src/features/settings/GeneralSection.tsx.
  • Initializes the Create PR dialog’s draft checkbox from the setting in src/features/pulls/CreatePrDialog.tsx, while retaining per-PR override support.
  • Extends forge_pr_set_draft in src-tauri/src/forge/mod.rs to support both draft directions across GitHub, GitLab, and Bitbucket.
  • Adds GitLab draft conversion through set_mr_draft in src-tauri/src/forge/gitlab.rs.
  • Adds GitHub gh pr ready --undo support through gh_pr_set_ready in src-tauri/src/github/pr.rs, including fork-aware lens handling.
  • Updates provider capability metadata in src-tauri/src/forge/model.rs, src/lib/git/types.ts, and src/lib/git/queries.ts for the shared draft toggle.
  • Consolidates footer and command-palette actions in src/features/pulls/RemotePrView.tsx and src/lib/hotkeys/registry.ts for Ready for review and Convert to draft.
  • Updates the GitDesktop MCP set_pull_request_draft tool in src-tauri/src/mcp_server/write_forge.rs and its API wrapper in src/lib/git/api.ts.
  • Adds optimistic draft-state updates and rollback handling in useSetPrDraft within src/lib/git/queries.ts.

Automation reruns

  • Keeps cancelled and failed automated review rows in a Stopped group with Re-run and Dismiss actions in src/features/activity/ActivityDock.tsx.
  • Stores the originating automation event and mode on review entries in src/lib/stores/reviews.ts, limits retained stopped rows, and preserves manual run behavior.
  • Adds mode-scoped reruns in src/lib/automations/runner.ts so retrying a stopped row re-fires only that automation mode.
  • Clears dismissed-head state before rerunning in src/lib/automations/dismissals.ts, preventing cancelled runs from being skipped by the existing watermark.
  • Adds failed-automation inbox notifications and improves error extraction in src/lib/automations/runner.ts.
  • Adds a provider-diff fallback in src/lib/automations/runner.ts when a remote pull-request head is not available locally.

Documentation and release notes

  • Documents draft controls, default draft creation, and stopped automation reruns in README.md.
  • Updates user guidance in src/features/help/content.ts.
  • Adds release notes in changelog.d/added-automation-rerun.md, changelog.d/added-default-draft-setting.md, and changelog.d/changed-draft-toggle-all-providers.md.

Discussion

  • Anonymous

    Anonymous - 2026-07-20
     

  • Anonymous

    Anonymous - 2026-07-20

    Originally posted by: theBGuy

    🗒️ Notes for reviewers

    Recorded decisions for this three-feature batch (stopped-automation re-run · all-provider draft toggle · default-draft setting), so deliberate choices read as such:

    1. Stopped rows are session-scoped by design (zustand; no cross-restart persistence). Cross-restart recovery stays on the incidental paths (catch-up poller / pr-sync watermarks). User-accepted v1 scope.
    2. cancelReview's auto: arm now patches to cancelled instead of removing — deliberately inverting the old comment's rationale: the stopped row (with Re-run/Dismiss) IS the cancel feedback now. The runner's cancel arms intentionally do NOT settle; control cleanup is split (cancelReview deletes the control; fail/settle guard on own-control identity).
    3. Stopped cap = 8, oldest-by-seq evicted, enforced on every stopped transition; running rows are excluded by the phase filter, so eviction can't touch a live run.
    4. rerun closure presence is the dock discriminator — manual panel runs also reach cancelled/error phases but never carry rerun, so they stay out of the Stopped group by construction.
    5. Re-run replays the retained event scoped to that row's mode only (run(event, only)), clearing that (target, mode) dismissed-head watermark first; claim files (released on stop, kept on deliver) make re-fires per-mode safe. A rule disabled since the stop toasts instead of no-oping silently.
    6. Re-run ignores the PR's CURRENT draft state deliberately — it's an explicit manual re-fire; honoring the click wins over draft-suppression parity (spec-review adjudicated as intended v1).
    7. Remote pr-open diff: local-first with forgePrDiff fallback — remote pr-sync already used the forge diff ("head may not be local"); pr-open kept the local branch diff, which broke for catch-up/ready-flip events on unfetched heads (observed live: git diff main...<head> fatals when the head was never fetched). Fallback validated end-to-end on a ref-only fixture — both automated reviews delivered through it.
    8. errorMessage(e) in the runner's failure path — the prior raw interpolation rendered Tauri rejections as "[object Object]" (observed live); the normalized message also feeds the stopped row's tooltip.
    9. Failed (not canceled) runs push a review-failed inbox notification gated on notifications.automations — parity with manual runs' notifyReviewDone. Canceled runs deliberately don't (user-initiated; the stopped row is the feedback).
    10. GitHub's draft toggle rides gh pr ready [--undo], not the HTTP forge — mr_draft_toggle is deliberately false for GitHub (per-provider Implemented convention); the footer pair's GitHub gate reduces to the old canWrite, keeping the Ready path zero-diff (same variant/copy/position, fireReadyReview preserved on button + palette).
    11. gh_pr_ready stays registered though the frontend no longer imports it — it's the shared impl behind the forge arm's GitHub dispatch, not dead code.
    12. useSetPrDraft is optimistic (field-scoped isDraft patch + rollback, mirroring useSetPrAssignees) with an explicit repo-wide invalidate on settle — reproducing useRepoMutation's default deliberately, since draft state feeds the merge gate and PR lists, broader than the assignees mirror's PR-scoped invalidate.
    13. Palette actions gate on the footer's full busy (not just setDraft.isPending) — the busy declaration was hoisted above the palette block (TDZ), expression unchanged.
    14. First PR-detail-scoped palette actions (defaultBinding null = palette-only) — a deliberate class extension; dock row actions (Re-run/Dismiss) stay unregistered like the existing dock Cancel precedent.
    15. createPrsAsDraft seeds at defaultValues + the seedOnOpen reset (fresh read per open via useEffectEvent); it deliberately does NOT retro-flip an already-open dialog, and ?? false keeps pre-field settings stores byte-identical.
    16. MCP set_pull_request_draft: description/doc text only — tool count stays 119; the forge call site gained the None lens arg with the new signature.
    17. Docs: the marketing site was deliberately skipped (no existing capability entry covers draft state; avoided churning freshly-rewritten site copy); README / in-app guide / changelog fragments updated with code-verified claims.

    Validation run before this PR: pnpm build green · cargo test 787 passed + clippy clean · biome check-only clean on all touched files · live E2E on scratch PRs (draft both directions via palette AND footer verified against gh pr view, Stopped/Re-run/Dismiss exercised on real failures, inbox parity, dialog draft seeding). Not live-tested: the dock cancel arm (fixture reviews completed too fast to cancel) and the GitLab glab arm (both flags CLI-verified; no GitLab scratch repo).

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

     

  • Anonymous

    Anonymous - 2026-07-20

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    This PR adds three coherent features — re-runnable stopped automations, an all-provider draft toggle, and a default-draft setting — and the implementation is careful and well-documented; the Rust arms, optimistic mutation, and gating all line up correctly. I found one should-fix in the Re-run flow and a minor doc inaccuracy; nothing else blocking.

    Correctness

    • should-fix — src/features/activity/ActivityDock.tsx StoppedTaskRow (Re-run onClick) + src/lib/automations/runner.ts rerunAutomation/run claim gate. The Re-run handler calls resetReview(task.key) (synchronously removing the row) before firing task.rerun?.(), and the re-run is async and can silently do nothing. Concrete cases where run(event, only) returns attempted === 0 even though the rule is still enabled:
    • Fast click after a dock Cancel: cancelReview synchronously patches the row to "cancelled" and exposes Re-run immediately, but the cancelled run's releaseClaim() only runs once the abort/subprocess-kill unwinds generateReviewText (100ms–1s+ for a CLI provider). Clicking Re-run in that window makes claim_automation_run return false (the original claim is still held) → if (!won) continue → attempted === 0.
    • Cross-instance / already-covered: a sibling worktree instance claims+reviews the same head (keeps its claim), or a concurrent successful review persists at the same head so the pr-sync sameSha(prior.headSha, headSha) gate skips.

    In all of these the row is already gone (via resetReview) and the user gets the misleading toast Automated review for this pull request is turned off. — with no way to retry. Fix: don't remove the stopped row up-front; only remove it once run() confirms it registered a new run (e.g. have rerunAutomation resolve a "started" signal and remove/keep the row based on it), and narrow the attempted === 0 toast so a held-claim/already-covered outcome isn't reported as "turned off."

    Docs

    • nit — src/features/help/content.ts (Activity & notifications section). The new sentence says the failed-automated-run inbox row matches "a manual run (both gated on the automations notification preference)." The manual path (notifyReviewDone in stores/reviews.ts) gates its inbox row on notifications.reviews, not automations — only the automated run is gated on automations. Reword to avoid claiming the manual run rides the automations pref. (The changelog fragment's "when automation notifications are on" is accurate.)

    Acknowledged deliberate decisions (not re-flagged)

    • Marketing-site update skipped (note [#17]) — recorded as intentional; README, help, and changelog fragments are updated.
    • gh_pr_ready kept registered though the frontend no longer imports it (note [#11]) — recorded; note that the forge GitHub arm actually calls gh_pr_set_ready directly, so gh_pr_ready is now only reachable via its Tauri registration, which matches the stated backward-compat intent.

    The optimistic useSetPrDraft (key ["repo", repo, "pr", lens, number] matches usePrDetails), the !draft→gh pr ready [--undo] mapping, the mr_draft_toggle flag wiring across all constructors/tests, the busy hoist (no duplicate declaration), and the removal of useReadyPr/ghPrReady with no dangling callers all check out.

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

     

    Related

    Tickets: #11
    Tickets: #17

  • Anonymous

    Anonymous - 2026-07-20

    Originally posted by: theBGuy

    🤖 GitDesktop AI security audit · opus · automated

    I verified the only injection-capable sinks in this diff:

    • set_mr_draft / gh_pr_set_ready shell glab/gh via Command::new(...).args(&[...]) (confirmed in glab.rs/runner.rs) — argument arrays, no shell interpolation. Their dynamic inputs are number: u64 (digits only) and fixed literal flags (--draft/--ready/--undo), so no argument injection is possible.
    • lens flows into gh_lens_slug → lens_remote, which strictly whitelists None/"origin"/"upstream" and rejects anything else with InvalidArgument before any spawn, so the new Option<String> lens parameter cannot smuggle attacker-controlled tokens into the command.
    • The MCP set_pull_request_draft tool still calls ensure_remote_write() and passes None for the lens, so authorization is unchanged.

    The rest of the diff is frontend state management (Zustand review store, react-query optimistic patch, settings field, UI rows, help/README/changelog text) with no unsafe sinks (no dangerouslySetInnerHTML, no secrets, no new network host control). The automation runner embedding diffs into review prompts is the product working as designed.

    No genuine security issues in these changes.

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

     

  • Anonymous

    Anonymous - 2026-07-20

    Originally posted by: theBGuy

    Round-1 disposition (fixes verified locally, landing in the next push):

    • AI review should-fix (Re-run row-removal race) — FIXED. The dock's Re-run no longer removes the stopped row up front; the runner removes it only at the instant the replacement run registers (run(event, only, replacesKey)), so a re-fire that can't start keeps the row as a retry target. run now returns {matched, attempted} and rerunAutomation distinguishes the three outcomes: rule genuinely gone → "turned off" toast; rule applies but a claim/watermark blocked it (your fast-click-after-Cancel and already-covered cases) → a retryable "another run already covers this head… the row is kept" toast; registered → no toast, the fresh Running row is the feedback. Verified against both concrete races in the finding: the claim gate sits upstream of the removal, so a Re-run racing the canceled run's releaseClaim unwind keeps the row; double-click is safe (second fire loses the claim, remove no-ops on the absent key).
    • Copilot's inline (silent .catch(() => undefined)) — FIXED by the same change. The whole-body swallow is gone: a throw before/outside the loop (e.g. loadAutomations I/O) now surfaces Couldn't re-run the …: <message> and the row stays; only the best-effort clearDismissedHead keeps its own catch. Replied + resolved on the thread.
    • AI review docs nit — FIXED. The help sentence now attributes the automations notification pref to the automated run's inbox row only ("…lands a review failed row in the inbox (gated on the automations notification preference), matching manual-run failures") — the manual path rides the reviews pref, as the review noted.
    • Security audit — clean, no action (sink verification matched the implementation: args-array spawns, digits-only number, lens whitelist, unchanged ensure_remote_write).

    Verification after the fixes: pnpm build green, biome check clean on the three touched files.

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

     

  • Anonymous

    Anonymous - 2026-07-20

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    I've verified the key claims against the actual code. Here's my review.

    The change cleanly adds three features — re-runnable stopped automations, an all-provider draft toggle, and a default-draft setting. The Rust dispatch arms, the optimistic useSetPrDraft mutation, the capability flag wiring, and the re-run flow all line up correctly. The revision since the last review addressed the one real should-fix well. No blocking issues; this looks sound to merge.

    Resolved since last review

    • Re-run row-removal race (prior should-fix) — resolved. StoppedTaskRow's Re-run now calls only task.rerun?.() (no up-front resetReview), and the stale row is removed inside run() at line ~348 after the replacement registerAutomationRun succeeds and past the claim gate (if (!won) continue at line 297 sits before attempted++ at line 323). run returns {matched, attempted}, and rerunAutomation now distinguishes rule-gone (matched 0 → "turned off"), blocked-but-retryable (matched>0/attempted 0 → "row kept, try again"), and started (no toast). The fast-click-after-Cancel and already-covered cases from the prior finding now keep the row. Verified end-to-end in the current diff.
    • Whole-body .catch(() => undefined) in rerunAutomation (Copilot) — resolved. The swallow is gone; a throw before/outside the per-rule loop now surfaces Couldn't re-run the …: <message> and keeps the row, while only the best-effort clearDismissedHead retains its own scoped catch.
    • Help sentence attributing the automations pref to the manual run (prior docs nit) — resolved. Reworded to "(gated on the automations notification preference), matching manual-run failures," which no longer claims the manual inbox row rides the automations pref (it rides notifications.reviews via notifyReviewDone).

    Verified correct

    • useSetPrDraft's optimistic key ["repo", repo, "pr", lens, args.number] matches prDetailsOptions' queryKey (queries.ts:950); the field-scoped rollback and repo-wide onSettled invalidate mirror useSetPrAssignees with the documented broader-invalidate deviation, and both footer buttons + palette twins pass onError so failures still toast.
    • The review-failed / tone: "danger" push notification uses a valid NotificationTone, and its target.kind = event.target.type is "remote" | "local" as the NotificationTarget requires.
    • Every terminal path in run() is accounted for after removing the old finally { settle() }: success/skip → settle, failure → fail, both cancel arms intentionally leave the "cancelled" row; no continue after registration leaves an orphaned "running" row.
    • The GitHub footer gate reduces to the old canWrite && pr.isDraft (since canToggleDraft is false for GitHub), and backend dispatch is provider-detected server-side, so a momentarily-undefined provider on GitLab/Bitbucket can't misroute through gh pr ready.
    • Rust: !draft → ready mapping into gh_pr_set_ready (--undo when !ready), the GitLab --draft|--ready flags, and the mr_draft_toggle flag (github false / gitlab+bitbucket true) with matching tests.

    Acknowledged deliberate decisions

    • Marketing-site update skipped (note [#17]) — recorded as intentional; README, in-app guide, and changelog fragments are updated with code-matching copy.
    • gh_pr_ready kept registered though the frontend dropped ghPrReady (note [#11]) — recorded; it's now only reachable via its Tauri registration, the forge arm calling gh_pr_set_ready directly.
    • Repo-wide onSettled invalidate and stopped-cap eviction at 8 (notes [#12], [#3]) — recorded and consistent with the code.

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

     

    Related

    Tickets: #11
    Tickets: #12
    Tickets: #17
    Tickets: #3

  • Anonymous

    Anonymous - 2026-07-20

    Ticket changed by: theBGuy

    • status: open --> closed
     

Log in to post a comment.

MongoDB Logo MongoDB