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) => ( - - ))} -
- + + + + ))} +
)}
);