From da1c2b710f952e9d0e70b7e6400489aac36d41bb Mon Sep 17 00:00:00 2001 From: LHagfoss Date: Sun, 15 Nov 2026 13:08:18 +0100 Subject: [PATCH] fix(desktop): merge the right pull request and ignore advisory checks The pull request panel keeps an opened pull per project and machine, so switching threads no longer leaves another project's #N with a live Merge button. The merge control reads the detail's check runs and leaves out the advisory portability jobs the engine's merge ignores. Merge is hidden on threads from another machine, the armed button changes colour and says what the second press does, and the panel drops its repeated heading, the header check mark and the Overview file list the Diff tab already has. --- apps/desktop/src/acp/mock.ts | 156 ++++++++------- apps/desktop/src/state/pullMerge.test.ts | 33 +++- apps/desktop/src/state/pullMerge.ts | 45 ++++- apps/desktop/src/ui/MergeButton.test.tsx | 70 +++++++ apps/desktop/src/ui/PullRequests.tsx | 229 ++++++++++++----------- 5 files changed, 345 insertions(+), 188 deletions(-) create mode 100644 apps/desktop/src/ui/MergeButton.test.tsx diff --git a/apps/desktop/src/acp/mock.ts b/apps/desktop/src/acp/mock.ts index a8c3234ba..31a3e8210 100644 --- a/apps/desktop/src/acp/mock.ts +++ b/apps/desktop/src/acp/mock.ts @@ -2337,6 +2337,84 @@ function configOptions(thread: MockThread, harnesses: HarnessSnapshot): Json[] { return options; } +/** The pull requests the mock project has; `/view` opens one of these by number. */ +const MOCK_PULLS = [ + { + number: 2788, + title: "feat(desktop): Tauri desktop app on every harness", + author: "LHagfoss", + state: "open", + draft: true, + review: "review_required", + checks: { passed: 5, failed: 0, pending: 1, skipped: 0 }, + headRef: "feat/desktop-tauri-harnesses", + baseRef: "main", + headSha: "58961fc5aa", + updatedAt: new Date(minutes(20)).toISOString(), + url: "https://github.com/LHagfoss/rustcode/pull/2788", + additions: 9120, + deletions: 12, + changedFiles: 64, + }, + { + number: 2781, + title: "fix(engine): keep the steer queue across a restart", + author: "LHagfoss", + state: "open", + draft: false, + review: "approved", + checks: { passed: 6, failed: 1, pending: 0, skipped: 0 }, + headRef: "fix/engine-steer-restart", + baseRef: "main", + headSha: "a1b2c3d4", + updatedAt: new Date(minutes(300)).toISOString(), + url: "https://github.com/LHagfoss/rustcode/pull/2781", + additions: 40, + deletions: 9, + changedFiles: 3, + }, + { + number: 2795, + title: "feat(desktop): add the login form", + author: "LHagfoss", + state: "open", + draft: false, + review: "changes_requested", + checks: { passed: 4, failed: 1, pending: 0, skipped: 0 }, + headRef: "feat/login-form", + baseRef: "main", + headSha: "58961fc5aa", + updatedAt: new Date(minutes(30)).toISOString(), + url: "https://github.com/LHagfoss/rustcode/pull/2795", + additions: 210, + deletions: 4, + changedFiles: 5, + }, +]; + +/** + * Each pull's check runs, matching its row's counts: #2781's one failure is an + * advisory job (Merge stays open), #2795's is a real one (Merge is blocked). + */ +const MOCK_CHECK_RUNS: Record = { + 2781: [ + { name: "desktop / check", status: "passed" }, + { name: "lint", status: "passed" }, + { name: "label", status: "passed" }, + { name: "Desktop UI", status: "passed" }, + { name: "rust / test", status: "passed" }, + { name: "clippy", status: "passed" }, + { name: "Windows and macOS portability coverage", status: "failed" }, + ], + 2795: [ + { name: "desktop / check", status: "passed" }, + { name: "lint", status: "passed" }, + { name: "label", status: "passed" }, + { name: "rust / test", status: "passed" }, + { name: "Desktop UI", status: "failed" }, + ], +}; + export class MockEngine { /** What this computer knows of the others (`fleet.json`), by host id. */ private textGenerators: JsonObject = { generators: [], selected: null }; @@ -3647,65 +3725,7 @@ export class MockEngine { case "_rustcode/environment/sync": return this.environmentSync(obj(params) ?? {}); case "_rustcode/pulls": - return { - list: { - provider: "github", - hasMore: false, - pulls: [ - { - number: 2788, - title: "feat(desktop): Tauri desktop app on every harness", - author: "LHagfoss", - state: "open", - draft: true, - review: "review_required", - checks: { passed: 5, failed: 0, pending: 1, skipped: 0 }, - headRef: "feat/desktop-tauri-harnesses", - baseRef: "main", - headSha: "58961fc5aa", - updatedAt: new Date(minutes(20)).toISOString(), - url: "https://github.com/LHagfoss/rustcode/pull/2788", - additions: 9120, - deletions: 12, - changedFiles: 64, - }, - { - number: 2781, - title: "fix(engine): keep the steer queue across a restart", - author: "LHagfoss", - state: "open", - draft: false, - review: "approved", - checks: { passed: 6, failed: 1, pending: 0, skipped: 0 }, - headRef: "fix/engine-steer-restart", - baseRef: "main", - headSha: "a1b2c3d4", - updatedAt: new Date(minutes(300)).toISOString(), - url: "https://github.com/LHagfoss/rustcode/pull/2781", - additions: 40, - deletions: 9, - changedFiles: 3, - }, - { - number: 2795, - title: "feat(desktop): add the login form", - author: "LHagfoss", - state: "open", - draft: false, - review: "changes_requested", - checks: { passed: 4, failed: 1, pending: 0, skipped: 0 }, - headRef: "feat/login-form", - baseRef: "main", - headSha: "58961fc5aa", - updatedAt: new Date(minutes(30)).toISOString(), - url: "https://github.com/LHagfoss/rustcode/pull/2795", - additions: 210, - deletions: 4, - changedFiles: 5, - }, - ], - }, - }; + return { list: { provider: "github", hasMore: false, pulls: MOCK_PULLS } }; case "_rustcode/project/read": { // The engine's `_rustcode/project/read`: a folder's git facts. const path = str(obj(params)?.path) || "/Users/me/code/rustcode"; @@ -3986,30 +4006,33 @@ export class MockEngine { checkout_branch: "main", } as unknown as Json; } - case "_rustcode/pulls/view": + case "_rustcode/pulls/view": { + // The row the list shows, so the detail agrees with it; a number the list + // does not have opens as an open pull with its checks still running. + const row = MOCK_PULLS.find((pull) => pull.number === params.number); return { pull: { - number: params.number as number, title: "feat(desktop): Tauri desktop app on every harness", author: "LHagfoss", state: "open", - // #2788 is the draft the list shows; any other opens ready to merge. - draft: params.number === 2788, + draft: false, checks: { passed: 5, failed: 0, pending: 1, skipped: 0 }, headRef: "feat/desktop-tauri-harnesses", baseRef: "main", headSha: "58961fc5aa", - url: `https://github.com/LHagfoss/rustcode/pull/${params.number}`, additions: 9120, deletions: 12, changedFiles: 64, + ...row, + number: params.number as number, + url: `https://github.com/LHagfoss/rustcode/pull/${params.number}`, body: "Adds `apps/desktop`: a Tauri shell around `rustcode --acp`.\n\n- Composer, approvals, harness pickers\n- Link chips and Smart UI", - checkRuns: [ + checkRuns: MOCK_CHECK_RUNS[params.number as number] ?? [ { name: "desktop / check", status: "passed" }, { name: "lint", status: "passed" }, { name: "label", status: "passed" }, { name: "Desktop UI", status: "passed" }, - { name: "rust / test", status: "pending" }, + { name: "rust / test", status: "passed" }, // A matrix job GitHub skipped before it expanded: the template stays in its name. { name: ["Desktop shell (", "$", "{{ matrix.name }})"].join(""), status: "pending" }, ], @@ -4145,6 +4168,7 @@ export class MockEngine { ], }, }; + } case "_rustcode/pulls/diff": return { diff: [ diff --git a/apps/desktop/src/state/pullMerge.test.ts b/apps/desktop/src/state/pullMerge.test.ts index 6eb612a1e..d66073aa0 100644 --- a/apps/desktop/src/state/pullMerge.test.ts +++ b/apps/desktop/src/state/pullMerge.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { checkName, checksHeadline, mergeControl } from "./pullMerge"; +import { advisoryCheck, checkName, checksHeadline, mergeControl } from "./pullMerge"; const checks = (passed = 0, failed = 0, pending = 0) => ({ passed, failed, pending, skipped: 0 }); @@ -40,5 +40,36 @@ describe("check names", () => { expect(checksHeadline([run("passed"), run("pending"), run("pending")])).toBe("2 checks running"); expect(checksHeadline([run("passed"), run("skipped")])).toBe("Checks passed"); expect(checksHeadline([])).toBe("No checks"); + expect(checksHeadline([run("passed"), { name: "portability coverage", status: "failed" }])).toBe( + "Checks passed", + ); + }); +}); + +describe("advisory checks", () => { + const run = (name: string, status: string) => ({ name, status }); + const open = { state: "open" as const, draft: false, checks: checks(1, 1) }; + + test("are named as the engine names them", () => { + expect(advisoryCheck("Windows and macOS portability coverage")).toBe(true); + expect(advisoryCheck("portability coverage (windows)")).toBe(true); + expect(advisoryCheck("cargo test --workspace")).toBe(false); + }); + + test("never hold a merge, failing or running", () => { + expect(mergeControl(open, [run("lint", "passed"), run("portability coverage", "failed")])).toEqual({ + kind: "merge", + }); + expect(mergeControl(open, [run("lint", "passed"), run("portability coverage", "pending")])).toEqual({ + kind: "merge", + }); + expect(mergeControl(open, [run("lint", "failed"), run("portability coverage", "failed")])).toEqual({ + kind: "blocked", + failed: 1, + }); + }); + + test("the list's counts stand in until the runs are read", () => { + expect(mergeControl(open)).toEqual({ kind: "blocked", failed: 1 }); }); }); diff --git a/apps/desktop/src/state/pullMerge.ts b/apps/desktop/src/state/pullMerge.ts index 764a0619a..a096386a7 100644 --- a/apps/desktop/src/state/pullMerge.ts +++ b/apps/desktop/src/state/pullMerge.ts @@ -24,13 +24,40 @@ export type MergeControl = | { kind: "merge_when_green"; pending: number } | { kind: "blocked"; failed: number }; -export function mergeControl(pull: Pick): MergeControl { +export function mergeControl( + pull: Pick, + runs?: readonly CheckRun[], +): MergeControl { if (pull.state !== "open" || pull.draft) return { kind: "none" }; - if (pull.checks.failed > 0) return { kind: "blocked", failed: pull.checks.failed }; - if (pull.checks.pending > 0) return { kind: "merge_when_green", pending: pull.checks.pending }; + // The detail's runs name each check, so the advisory ones the merge ignores + // can be left out; the list's counts cannot, and stand in until it is read. + const { failed, pending } = runs ? blockingCounts(runs) : pull.checks; + if (failed > 0) return { kind: "blocked", failed }; + if (pending > 0) return { kind: "merge_when_green", pending }; return { kind: "merge" }; } +/** + * The checks a merge never waits for, as the engine's `ADVISORY_CHECKS` + * (rustcode/engine/src/pull_requests.rs) and scripts/pr-merge.sh name them. + */ +export const ADVISORY_CHECKS = ["portability coverage"] as const; + +export function advisoryCheck(name: string): boolean { + const lower = name.toLowerCase(); + return ADVISORY_CHECKS.some((advisory) => lower.includes(advisory)); +} + +const settled = (status: string) => status === "passed" || status === "skipped"; + +/** Failing and running checks that hold a merge: every one but the advisory jobs. */ +export function blockingCounts(runs: readonly CheckRun[]): { failed: number; pending: number } { + const blocking = runs.filter((c) => !advisoryCheck(c.name)); + const failed = blocking.filter((c) => c.status === "failed").length; + const pending = blocking.filter((c) => c.status !== "failed" && !settled(c.status)).length; + return { failed, pending }; +} + /** * A check's name as people read it. A job GitHub skipped before it expanded its * matrix keeps the template in its name (`Desktop shell (${{ matrix.name }})`): @@ -49,10 +76,8 @@ type CheckRun = { name: string; status: string }; /** The checks' headline: what needs the user first, then that everything passed. */ export function checksHeadline(runs: readonly CheckRun[]): string { - const failed = runs.filter((c) => c.status === "failed").length; - const pending = runs.filter( - (c) => c.status !== "failed" && c.status !== "passed" && c.status !== "skipped", - ).length; + // Advisory jobs never hold a merge, so they never lead the headline either. + const { failed, pending } = blockingCounts(runs); if (failed) return `${failed} ${failed === 1 ? "check failed" : "checks failed"}`; if (pending) return `${pending} ${pending === 1 ? "check running" : "checks running"}`; return runs.length ? "Checks passed" : "No checks"; @@ -74,7 +99,11 @@ const subscribe = (listener: () => void) => { /** Whether a merge of this pull is waiting or running. */ export function useMerging(cwd: string, number: number): boolean { - useSyncExternalStore(subscribe, () => version); + useSyncExternalStore( + subscribe, + () => version, + () => version, + ); return running.has(keyOf(cwd, number)); } diff --git a/apps/desktop/src/ui/MergeButton.test.tsx b/apps/desktop/src/ui/MergeButton.test.tsx new file mode 100644 index 000000000..71be28004 --- /dev/null +++ b/apps/desktop/src/ui/MergeButton.test.tsx @@ -0,0 +1,70 @@ +import { describe, expect, test } from "bun:test"; +import { Tooltip } from "radix-ui"; +import { renderToStaticMarkup } from "react-dom/server"; +import type { PullRequest } from "../acp/rustcode"; +import { MergeButton } from "./PullRequests"; + +const PULL: PullRequest = { + number: 2781, + title: "fix(engine): keep the steer queue across a restart", + author: "LHagfoss", + state: "open", + draft: false, + checks: { passed: 6, failed: 0, pending: 0, skipped: 0 }, + headRef: "fix/engine-steer-restart", + baseRef: "main", + headSha: "a1b2c3d4", + updatedAt: 0, + additions: 40, + deletions: 9, + changedFiles: 3, +} as PullRequest; + +const run = (name: string, status: string) => ({ name, status }); + +function render(pull: Partial, runs?: { name: string; status: string }[]): string { + return renderToStaticMarkup( + + + , + ); +} + +// The attribute, not the `disabled:` styling every button's classes carry. +const enabled = (html: string) => html.includes("]* disabled=""/.test(html); + +describe("the merge button", () => { + test("merges once every check passed", () => { + const html = render({}); + expect(html).toContain("Merge"); + expect(html).not.toContain("Merge when checks pass"); + expect(enabled(html)).toBe(true); + }); + + test("waits for running checks", () => { + const html = render({ checks: { passed: 4, failed: 0, pending: 2, skipped: 0 } }); + expect(html).toContain("Merge when checks pass"); + expect(enabled(html)).toBe(true); + }); + + test("is disabled by a failing check, without repeating the count the Checks headline has", () => { + const html = render({}, [run("lint", "passed"), run("Desktop UI", "failed")]); + expect(html).toContain("Merge"); + expect(html).not.toContain("failed"); + expect(enabled(html)).toBe(false); + }); + + test("does not let an advisory job block it, as the engine's merge does not", () => { + const runs = [run("lint", "passed"), run("Windows and macOS portability coverage", "failed")]; + // The list counted the advisory failure; the detail's runs say what it was. + const html = render({ checks: { passed: 1, failed: 1, pending: 0, skipped: 0 } }, runs); + expect(enabled(html)).toBe(true); + expect(html).not.toContain("Merge when checks pass"); + }); + + test("offers nothing for a draft or a finished pull", () => { + expect(render({ draft: true })).toBe(""); + expect(render({ state: "merged" })).toBe(""); + expect(render({ state: "closed" })).toBe(""); + }); +}); diff --git a/apps/desktop/src/ui/PullRequests.tsx b/apps/desktop/src/ui/PullRequests.tsx index 2fc6fc4c7..f08bfa6f5 100644 --- a/apps/desktop/src/ui/PullRequests.tsx +++ b/apps/desktop/src/ui/PullRequests.tsx @@ -1,12 +1,13 @@ // Pull requests, after PullRequestsView.swift: the project's pull requests // (`_rustcode/pulls`), one in full (`/view`), and a review the user writes -// and submits (`/review`). Read-only otherwise: nothing here checks out, -// merges or comments on its own. Picked review feedback goes into the +// and submits (`/review`), and a merge the user confirms (state/pullMerge.ts). +// Nothing here checks out, merges or comments on its own. Picked review feedback goes into the // thread's composer as a draft; it is never sent from here. import { ArrowLeft, Bot, + ChevronDown, ChevronRight, CircleCheck, CircleDashed, @@ -15,7 +16,6 @@ import { ExternalLink, GitBranchPlus, GitMerge, - GitPullRequest, MessageSquare, MoreHorizontal, RefreshCw, @@ -37,6 +37,7 @@ import { openExternal } from "../lib/native"; import { count } from "../lib/plural"; import { type FeedbackItem, feedbackItems, feedbackLocation, keepSelected } from "../lib/reviewFeedback"; import { + advisoryCheck, cancelMerge, checkName, checksHeadline, @@ -337,12 +338,20 @@ const CONFIRM_MS = 4000; * finished pull. The first press arms it ("Confirm merge?"), the second merges; * a waiting merge shows here, with a way to stop it, until it ends. */ -function MergeButton({ cwd, pull }: { cwd: string; pull: PullRequest }) { +export function MergeButton({ + cwd, + pull, + runs, +}: { + cwd: string; + pull: PullRequest; + runs?: PullDetail["checkRuns"]; +}) { const merging = useMerging(cwd, pull.number); const [armed, setArmed] = useState(false); const timer = useRef>(undefined); useEffect(() => () => clearTimeout(timer.current), []); - const control = mergeControl(pull); + const control = mergeControl(pull, runs); if (merging) return ( @@ -356,17 +365,20 @@ function MergeButton({ cwd, pull }: { cwd: string; pull: PullRequest }) { ); if (control.kind === "none") return null; + // The failing count is the Checks headline's to say; the button only says it cannot merge. if (control.kind === "blocked") return ( - {count(control.failed, "check")} failed + Merge ); const label = control.kind === "merge" ? "Merge" : "Merge when checks pass"; + // Armed, the button changes colour and keeps saying what the second press does. + const confirm = control.kind === "merge" ? "Press again to merge" : "Press again to merge when green"; const press = () => { clearTimeout(timer.current); if (!armed) { @@ -383,8 +395,8 @@ function MergeButton({ cwd, pull }: { cwd: string; pull: PullRequest }) { control.kind === "merge_when_green" ? " once every check passes" : "" }`} > - setArmed(false)}> - {armed ? "Confirm merge?" : label} + setArmed(false)}> + {armed ? confirm : label} ); @@ -434,29 +446,25 @@ function ChecksSection({ runs }: { runs: PullDetail["checkRuns"] }) { ) : ( {checkName(c.name)} )} + {advisoryCheck(c.name) && ( + + advisory + + )} ))} ); } -function PullView({ - cwd, - host, - number, - onBack, -}: { - cwd: string; - host: string; - number: number; - onBack: () => void; -}) { +function PullView({ cwd, host, number }: { cwd: string; host: string; number: number }) { // The row the list already read draws the header at once; the body, the // files and the activity come from the detail read behind it. const { detail, summary, error, live } = usePullDetail(cwd, number); const [reviewing, setReviewing] = useState(false); const [working, setWorking] = useState(false); - const [tab, setTab] = useState<"overview" | "diff">("overview"); + const [tab, setTab] = useState<"details" | "diff">("details"); + const merges = !!summary && !host && mergeControl(summary, detail?.checkRuns).kind !== "none"; const askAgent = (pull: PullRequest) => { startNewThread(cwd, host || undefined); setDraft( @@ -467,13 +475,6 @@ function PullView({ }; return (
- {summary ? ( <>

@@ -481,7 +482,6 @@ function PullView({

{STATE_LABEL[pullState(summary)]} - {count(summary.changedFiles, "file")}{" "} @@ -491,12 +491,20 @@ function PullView({
- + {/* A thread on another machine names a path this engine does not have: it merges there. */} + {!host && } - - - + {merges ? ( + + + + ) : ( + // With no merge beside it, a bare "···" reads as nothing: it says what it holds. + + Actions + + )} {summary.url && ( @@ -542,7 +550,7 @@ function PullView({ value={tab} onChange={setTab} options={[ - { value: "overview", label: "Overview" }, + { value: "details", label: "Details" }, { value: "diff", label: `Diff · ${summary.changedFiles}` }, ]} /> @@ -553,7 +561,7 @@ function PullView({
)} - {tab === "overview" && detail && ( + {tab === "details" && detail && ( <> {detail.body && (
@@ -561,20 +569,6 @@ function PullView({
)} {detail.checkRuns.length > 0 && } - {detail.files.length > 0 && ( -
-

Files

- {detail.files.map((f) => ( -
- - {f.path} - - +{f.additions} - −{f.deletions} -
- ))} -
- )} )} @@ -708,7 +702,12 @@ export function PullsPanel() { const cwd = useApp((s) => (s.selectedId ? s.threads[s.selectedId]?.cwd : undefined) ?? s.projectCwd); const host = selectedId ? splitThreadKey(selectedId).host : ""; const [state, setState] = useState("open"); - const [viewing, setViewing] = useState(null); + // An open pull belongs to the project (and machine) it was opened in: another + // project's #N is a different pull request, so a switch goes back to the list. + const scope = `${host}\u0000${cwd ?? ""}`; + const [opened, setOpened] = useState<{ scope: string; number: number } | null>(null); + const viewing = opened?.scope === scope ? opened.number : null; + const setViewing = (number: number | null) => setOpened(number === null ? null : { scope, number }); // The panel paints the last answer for this state at once and refreshes // behind it; the skeleton is only for a first read with nothing saved. const { data: pulls, error } = usePullList(cwd, state, { limit: PULL_LIMIT }); @@ -724,10 +723,31 @@ export function PullsPanel() { return (
-
- - Pull requests - + {/* The panel's tab already says "Pull requests": the bar holds the state filter or the way back. */} +
+ {viewing !== null && cwd ? ( + + ) : ( + cwd && void readPullList(cwd, next, { limit: PULL_LIMIT })} + options={[ + { value: "open", label: "Open" }, + { value: "merged", label: "Merged" }, + { value: "closed", label: "Closed" }, + { value: "all", label: "All" }, + ]} + /> + )} @@ -736,69 +756,52 @@ export function PullsPanel() {
{viewing !== null && cwd ? ( - setViewing(null)} /> + ) : ( - <> -
- cwd && void readPullList(cwd, next, { limit: PULL_LIMIT })} - options={[ - { value: "open", label: "Open" }, - { value: "merged", label: "Merged" }, - { value: "closed", label: "Closed" }, - { value: "all", label: "All" }, - ]} - /> -
-
- {error && } - {!error && pulls?.length === 0 && ( -

No pull requests.

- )} - {!pulls && !error && } - {pulls?.map((pull) => ( - - ))} -
- + + + + ))} +
)}
);