From 0b0eb05a4c512801cb41b5a9964cda26d55f3d7d Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Fri, 31 Jul 2026 21:22:50 -0400 Subject: [PATCH 1/4] refactor(prs): apply the quality findings, and make /quality require it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four findings from the PR #988 review were verified as real and then left in the code. Applying them: - The two `deriveGithubSnapshot*` mappers capture nothing from the prService factory — they are pure row->DTO functions and now live at module scope beside `rowToSummary`, where they can be tested without constructing a service. - The sticky period header is derived during render from the range being drawn. It previously round-tripped through a ref and a state-setting effect, re-rendering the whole list every scroll frame to compute something already in hand. - `detachRows` takes an id list instead of a SQL predicate string plus a params array that four separate statements had to agree with positionally. - The GitHub PR row renderer moves to its own module. None of it depends on GitHubTab's state, so the tab is left as a coordinator: 2468 -> 1960 lines. The skill changes matter more than the refactor. /quality's synthesis step said "apply the safe fixes ... gate the rest", which reads as licence to verify a finding and then defer it — so a run could surface real problems, fix none of them, and still report success. It now says fix every verified finding at any severity, and narrows the gate to the only two things that genuinely need the author: a product decision, or a behavior change the branch was not asked to make. "Structural", "large" and "out of scope" are named as non-reasons. /ship gained the matching precondition: a non-empty quality gate blocks the merge, rather than being mentioned afterwards. Co-Authored-By: Claude Opus 5 --- .agents/skills/quality/SKILL.md | 59 +- .agents/skills/ship/SKILL.md | 19 +- .agents/skills/test/SKILL.md | 2 +- .../src/main/services/prs/prService.ts | 88 ++- .../services/prs/pullRequestRowCleanup.ts | 55 +- .../components/prs/shared/GitHubTabPrRow.tsx | 531 ++++++++++++++++ .../components/prs/tabs/GitHubTab.tsx | 573 ++---------------- .../components/prs/tabs/githubPrBranch.ts | 4 + 8 files changed, 705 insertions(+), 626 deletions(-) create mode 100644 apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx create mode 100644 apps/desktop/src/renderer/components/prs/tabs/githubPrBranch.ts diff --git a/.agents/skills/quality/SKILL.md b/.agents/skills/quality/SKILL.md index 7f0b125c2..facc3f158 100644 --- a/.agents/skills/quality/SKILL.md +++ b/.agents/skills/quality/SKILL.md @@ -4,8 +4,10 @@ description: >- Make the code correct, clean, and current. A thermo dual-review: a correctness/security track and a maintainability/code-judo track run in parallel, then a synthesis step dedupes, severity-ranks (Blocker/High/Medium/ - Low), verifies each finding against the real code, auto-applies the safe - behavior-preserving fixes, re-reviews, and gates the rest. Grounded in ADE's + Low), verifies each finding against the real code, and FIXES EVERY VERIFIED + FINDING at any severity — re-reviewing until clean. Only findings needing a + product decision, or a behavior change this branch was not asked to make, + reach the merge-blocking gate. Grounded in ADE's own bug classes (runtime-backed null services, daemon action-domain wiring, cr-sqlite CRR, IPC contract drift, fast-tier loading). --- @@ -107,15 +109,34 @@ are handled by the synthesis step below, not a separate phase. 5. **Sweep the bug class.** When an accepted finding is a repeated pattern, scan the diff scope for sibling instances and fix them together — stop at touched surfaces and owner boundaries; no refactor beyond the class. -6. **Apply** the fixes that are **unambiguous and behavior-preserving** — safe - correctness fixes and Track B judo moves. Every applied change must be - verifiable by reading the diff; do not change behavior. +6. **Apply every finding you accepted in step 4 — all of them, whatever the + severity.** Verified means valid; valid means fix it. Medium and Low are not a + backlog, and "behavior-preserving" describes *how* you apply a fix, not which + findings earn one. This is the entire point of the skill: a run that surfaces + real problems and leaves them in the code has cost the user tokens and + returned nothing. + + Fix correctness findings and Track B judo moves alike. If a fix is genuinely + large (a multi-file extraction, a schema migration), it is still yours to do — + do it here, in this run, not "as a follow-up". 7. **Re-review until clean.** If step 6 changed code, re-run Track A on the *new* diff. New accepted findings → verify (4), apply (6), re-check. Stop when a pass yields no new accepted findings (cap 2 extra passes; anything still open goes to the gate). Catches fix-induced regressions before `/test` or `/ship`. -8. **Gate** — do NOT auto-apply Blockers or judgment-call findings. Surface them - in the report for the author and for `/ship` to gate the merge on. +8. **Gate — the narrow exception, not the escape hatch.** Only two kinds of + accepted finding may go to the gate unfixed: + - it needs a **product decision you cannot make** (which of two valid + behaviors the user wants), or + - the fix is **not behavior-preserving** and changing behavior is not what + this branch was asked to do. + + "Structural", "large", "risky", "pre-existing", "out of scope for this PR", + and "worth doing deliberately" are **not** gate reasons — those are fixes you + owe. If you gate a finding, the report must say which of the two reasons + applies and what decision you need. Anything in the Gate table blocks the + merge until the author resolves it; `/ship` treats a non-empty gate as a stop. + + A finding you neither fixed nor gated is a bug in your run. 9. **Reconcile (optional)** — if a PR exists, *after* the independent audit, read the PR discussion and review-bot comments (`gh pr view --comments`, or the `ade-pr-workflows` skill). ADE's review bots are `@copilot` (first push) and @@ -126,8 +147,9 @@ are handled by the synthesis step below, not a separate phase. ## Completion -Output a summary. The **Gate** section is what `/test` and `/ship` consume — list -every Blocker and High finding that was NOT auto-fixed. +Output a summary. The **Gate** section is what `/test` and `/ship` consume, and a +non-empty gate blocks the merge. List only findings you could not fix for one of +the two permitted reasons — not findings you chose to defer. ```markdown ## Quality Summary @@ -137,11 +159,18 @@ every Blocker and High finding that was NOT auto-fixed. - Auto-applied: [count] (safe correctness fixes + structural judo moves) - Re-review passes: [n] -### Gate (not auto-fixed — for /test regression targets and /ship merge gate) -| Severity | file:line | Finding | Why not auto-fixed | -|---|---|---|---| -| Blocker | ... | ... | needs human judgment / product intent | -| High | ... | ... | ... | +### Gate (MERGE-BLOCKING — every row needs an author decision) +Only two reasons belong here: a product decision you cannot make, or a fix that +is not behavior-preserving on a branch that was not asked to change behavior. +Empty is the expected outcome. "Structural / large / out of scope" is not a +gate reason — those get fixed above. -Next: /test (turn each Blocker/High above into a named regression test). +| Severity | file:line | Finding | Which gate reason | Decision needed | +|---|---|---|---|---| +| Blocker | ... | ... | product decision | which behavior do you want? | + +Next: /test (turn each Blocker/High into a named regression test). + +**Before you print this:** every accepted finding is either in "Auto-applied" or +in this table. If one is in neither, go back to step 6 and fix it. ``` diff --git a/.agents/skills/ship/SKILL.md b/.agents/skills/ship/SKILL.md index 14d11fb49..21e14fe00 100644 --- a/.agents/skills/ship/SKILL.md +++ b/.agents/skills/ship/SKILL.md @@ -38,6 +38,23 @@ The playbook's Phase 0 is **commit → push → open PR** only. Test generation the local-CI gate are NOT part of ship — that's `/test` (and optionally `/finalize`) before you reach this skill. +## Precondition: the `/quality` gate must be empty + +Before Phase 0, and again before the Phase 3c merge, check for an open +`/quality` gate. A non-empty gate **blocks the merge** — every row in it is a +finding that was verified as real and left unfixed, and by `/quality`'s contract +the only two things that may be there are a product decision the author owes, or +a behavior change this branch was not asked to make. Both need the author. + +- Gate rows exist → do not merge. Surface them, state the decision needed, and + stop with `blocked`. Do not merge and mention them afterwards. +- If `/quality` was never run on this lane, say so plainly rather than assuming + the gate is empty. + +Severity is irrelevant here: a Medium in the gate blocks exactly as hard as a +Blocker, because presence in the gate means it needed a human, not that it was +minor. + --- ## Execution Mode: Autonomous @@ -184,7 +201,7 @@ self-resume signal. Either: |--------|---------| | `done-clean` | PR merged on main | | `done-max` | 5 normal + 1 force-finalize exhausted, merge genuinely blocked | -| `blocked` | Unrecoverable conflict, gate failure, API error, or force-finalize CI failed | +| `blocked` | Unrecoverable conflict, gate failure, API error, force-finalize CI failed, or a non-empty `/quality` gate awaiting an author decision | Always print the final summary (PR, branch, iterations, status, reason, per-iteration log, unaddressed items) on exit. Do NOT schedule a wake when diff --git a/.agents/skills/test/SKILL.md b/.agents/skills/test/SKILL.md index 2c8aceb25..fb13b3235 100644 --- a/.agents/skills/test/SKILL.md +++ b/.agents/skills/test/SKILL.md @@ -17,7 +17,7 @@ The suite has bloated for three reasons. You exist to fight all three: Every run does three passes in this order: **PRUNE → CONSOLIDATE → ADD**. You may finish at any pass — adding is optional. -**Consume the `/quality` gate.** If `/quality` ran on this lane, take its Summary's **Gate** section — every Blocker/High it surfaced but did not auto-fix. Each is a named regression-test target for the ADD pass: a test that fails on the bug and passes once it's fixed. A finding isn't "handled" until a test pins it. No gate available → derive the same targets from the diff. +**Consume the `/quality` gate.** If `/quality` ran on this lane, take its Summary's **Gate** section — every finding it surfaced but did not fix, at any severity (a Medium in the gate got there because it needed a human, not because it was minor). Each is a named regression-test target for the ADD pass: a test that fails on the bug and passes once it's fixed. A finding isn't "handled" until a test pins it. No gate available → derive the same targets from the diff. **Run the way CI would.** After the suite work, run only the affected shards, never the full suite (that's `/finalize`'s local gate and `/ship`'s remote CI). Verify every new/edited test file matches a vitest workspace glob so CI actually picks it up. diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 07d9a6726..89ddba8ff 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -1078,6 +1078,46 @@ function normalizeMergeMethod(value: unknown): MergeMethod | null { return value === "squash" || value === "merge" || value === "rebase" ? value : null; } +/** + * Resolve the lane columns of a list row. + * + * A detached row reports NO live lane (`linkedLaneId`/`linkedLaneName` stay null) and + * carries `detached` instead, so the renderer shows it as history rather than as a + * mapping it could act on. A missing lane row yields a null name, never the raw lane + * UUID — a bare id in the list is worse than no chip at all. + */ +function deriveGithubSnapshotLaneLink( + linked: PullRequestRow | null, + laneById: Map | undefined, +): Pick { + if (!linked) return { linkedLaneId: null, linkedLaneName: null, detached: null }; + const detached = rowDetachedLane(linked); + if (detached) return { linkedLaneId: null, linkedLaneName: null, detached }; + return { + linkedLaneId: linked.lane_id, + linkedLaneName: laneById?.get(linked.lane_id)?.name ?? null, + detached: null, + }; +} + +/** Merge outcome + size, so a merged row can describe itself without a GitHub call. */ +function deriveGithubSnapshotMergeFacts( + linked: PullRequestRow | null, +): Pick< + GitHubPrListItem, + "mergedAt" | "mergedBy" | "mergeMethod" | "additions" | "deletions" | "commitCount" | "changedFiles" +> { + return { + mergedAt: linked?.merged_at ?? null, + mergedBy: linked ? rowMergedBy(linked) : null, + mergeMethod: normalizeMergeMethod(linked?.merge_method), + additions: normalizeCount(linked?.additions), + deletions: normalizeCount(linked?.deletions), + commitCount: normalizeCount(linked?.commit_count), + changedFiles: normalizeCount(linked?.changed_files), + }; +} + function rowMergedBy(row: PullRequestRow): PrMergedBy | null { const login = String(row.merged_by_login ?? "").trim(); if (!login) return null; @@ -8625,54 +8665,6 @@ export function createPrService({ }; }; - /** - * Resolve the lane columns of a list row. - * - * A detached row reports NO live lane (`linkedLaneId`/`linkedLaneName` stay null) and - * carries `detached` instead, so the renderer shows it as history rather than as a - * mapping it could act on. Previously the lane name fell back to the raw lane UUID - * when the lane row was missing, which surfaced a bare id in the list. - */ - const deriveGithubSnapshotLaneLink = ( - linked: PullRequestRow | null, - laneById: Map | undefined, - ): Pick => { - if (!linked) return { linkedLaneId: null, linkedLaneName: null, detached: null }; - const detached = rowDetachedLane(linked); - if (detached) return { linkedLaneId: null, linkedLaneName: null, detached }; - const laneName = laneById?.get(linked.lane_id)?.name ?? null; - return { linkedLaneId: linked.lane_id, linkedLaneName: laneName, detached: null }; - }; - - /** Merge outcome + size, so a merged row can describe itself without a GitHub call. */ - const deriveGithubSnapshotMergeFacts = ( - linked: PullRequestRow | null, - ): Pick< - GitHubPrListItem, - "mergedAt" | "mergedBy" | "mergeMethod" | "additions" | "deletions" | "commitCount" | "changedFiles" - > => { - if (!linked) { - return { - mergedAt: null, - mergedBy: null, - mergeMethod: null, - additions: null, - deletions: null, - commitCount: null, - changedFiles: null, - }; - } - return { - mergedAt: linked.merged_at ?? null, - mergedBy: rowMergedBy(linked), - mergeMethod: normalizeMergeMethod(linked.merge_method), - additions: normalizeCount(linked.additions), - deletions: normalizeCount(linked.deletions), - commitCount: normalizeCount(linked.commit_count), - changedFiles: normalizeCount(linked.changed_files), - }; - }; - /** * Persist how a PR shipped. Written once at merge time and again by the poller when * it observes the merge; `coalesce` keeps whichever arrived first rather than letting diff --git a/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts b/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts index be47478c9..cb3ae181a 100644 --- a/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts +++ b/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts @@ -4,13 +4,17 @@ type DbLike = { /** * Reads are only needed by the detach path, which must count before rows vanish. - * Signature mirrors `AdeDb.get` so the concrete db satisfies it structurally. + * Signature mirrors `AdeDb.get`/`AdeDb.all` so the concrete db satisfies it structurally. */ type ReadableDbLike = DbLike & { get = Record>( sql: string, - params?: any[], + params?: unknown[], ): T | null; + all = Record>( + sql: string, + params?: unknown[], + ): T[]; }; /** @@ -114,21 +118,29 @@ export function deletePullRequestRowsByIds(db: DbLike, projectId: string, prIds: * than the retained row costs. `commit_count` / `changed_files` are lifted onto the row * first so the merged view survives the purge. */ +/** + * Apply the detach mutation to an explicit set of PR ids. + * + * Taking ids rather than a SQL predicate keeps one parameter list for all four + * statements below; an earlier version threaded a predicate string plus a params array + * that every statement had to agree with positionally, which is a bug waiting to happen + * for no benefit. + */ function detachRows( db: ReadableDbLike, args: { projectId: string; - /** SQL predicate over `pull_requests`, e.g. `lane_id = ? and project_id = ?`. */ - predicate: string; - predicateParams: unknown[]; + prIds: string[]; laneName: string | null; laneColor: string | null; detachedAt: string; provenance: DetachedLaneProvenance; }, ): void { - const { projectId, predicate, predicateParams, laneName, laneColor, detachedAt, provenance } = args; - const prSelect = `select id from pull_requests where ${predicate}`; + const { projectId, prIds, laneName, laneColor, detachedAt, provenance } = args; + if (prIds.length === 0) return; + const placeholders = prIds.map(() => "?").join(", "); + const scope = [projectId, ...prIds]; // Lift counts off the snapshot before nulling it, so the merged row can still say // "12 commits · 9 files" once the JSON is gone. @@ -147,9 +159,9 @@ function detachRows( from pull_request_snapshots s where s.pr_id = pull_requests.id and json_valid(s.files_json)) ) - where ${predicate} + where project_id = ? and id in (${placeholders}) `, - predicateParams, + scope, ); // `detached_at is null` keeps the first detach authoritative: re-detaching an already @@ -161,9 +173,9 @@ function detachRows( detached_lane_name = ?, detached_lane_color = ?, detached_provenance = ? - where ${predicate} and detached_at is null + where project_id = ? and id in (${placeholders}) and detached_at is null `, - [detachedAt, laneName, laneColor, JSON.stringify(provenance), ...predicateParams], + [detachedAt, laneName, laneColor, JSON.stringify(provenance), ...scope], ); // Keep detail/status/commits (small, and what the merged view reads); drop the bulky @@ -175,13 +187,13 @@ function detachRows( checks_json = null, comments_json = null, reviews_json = null - where pr_id in (${prSelect}) + where pr_id in (${placeholders}) `, - predicateParams, + prIds, ); // Group membership is lane-scoped work-in-progress, not history — it goes. - db.run(`delete from pr_group_members where pr_id in (${prSelect})`, predicateParams); + db.run(`delete from pr_group_members where pr_id in (${placeholders})`, prIds); pruneEmptyPrGroups(db, projectId); } @@ -190,15 +202,19 @@ export function detachPullRequestRowsForLane( args: DetachPullRequestRowsArgs, ): void { const { projectId, laneId, laneName, laneColor, detachedAt } = args; + const prIds = db + .all<{ id: string }>("select id from pull_requests where lane_id = ? and project_id = ?", [laneId, projectId]) + .map((row) => String(row.id)); detachRows(db, { projectId, - predicate: "lane_id = ? and project_id = ?", - predicateParams: [laneId, projectId], + prIds, laneName, laneColor, detachedAt, provenance: countLaneProvenance(db, projectId, laneId), }); + // Lane-scoped membership survives the id lookup above (a group member can outlive its + // PR row), so clear it explicitly and prune once. db.run("delete from pr_group_members where lane_id = ?", [laneId]); pruneEmptyPrGroups(db, projectId); } @@ -214,17 +230,12 @@ export function detachPullRequestRowsByIds( args: DetachPullRequestRowsArgs & { prIds: string[] }, ): void { const { projectId, laneId, laneName, laneColor, detachedAt } = args; - const ids = uniqueIds(args.prIds); - if (ids.length === 0) return; - const placeholders = ids.map(() => "?").join(", "); detachRows(db, { projectId, - predicate: `project_id = ? and id in (${placeholders})`, - predicateParams: [projectId, ...ids], + prIds: uniqueIds(args.prIds), laneName, laneColor, detachedAt, provenance: countLaneProvenance(db, projectId, laneId), }); } - diff --git a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx new file mode 100644 index 000000000..dd40a6c72 --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx @@ -0,0 +1,531 @@ +import React from "react"; +import { ArrowSquareOut, ChatText, CheckCircle, GitBranch, XCircle } from "@phosphor-icons/react"; + +import type { GitHubPrListItem, PrSummary } from "../../../../shared/types/prs"; +import { COLORS, MONO_FONT, SANS_FONT, inlineBadge } from "../../lanes/laneDesignTokens"; +import { LaneAccentDot } from "../../lanes/LaneAccentDot"; +import { useAppStore } from "../../../state/appStore"; +import { isTerminalPrState } from "../../../lib/prState"; +import { formatTimeAgoCompact } from "./prFormatters"; +import { PrCiRunningIndicator } from "./prVisuals"; +import { GitHubStackBadge } from "./GitHubStackBadge"; +import { formatPrListGroupDiff, type PrListGroupHeader as PrListGroupHeaderModel } from "./prListGrouping"; + +/** + * Presentation for one row of the GitHub PR list, and the period header that groups + * them. Split out of `GitHubTab.tsx` because none of it depends on that component's + * state — `useLaneColorById` reads the store directly — so the tab is left as a + * coordinator rather than a coordinator plus 450 lines of row markup. + */ + +import { branchNameFromRef } from "../tabs/githubPrBranch"; + +/* -- Color-coded state badge with distinct colors per state -- */ +function stateColor(state: string): { bg: string; border: string; text: string } { + switch (state) { + case "open": + return { bg: "rgba(59,130,246,0.10)", border: "rgba(59,130,246,0.20)", text: "#60A5FA" }; + case "draft": + return { bg: "rgba(245,158,11,0.10)", border: "rgba(245,158,11,0.20)", text: "#FBBF24" }; + case "merged": + return { bg: "rgba(34,197,94,0.10)", border: "rgba(34,197,94,0.20)", text: "#4ADE80" }; + default: + return { bg: "rgba(161,161,170,0.08)", border: "rgba(161,161,170,0.15)", text: "#A1A1AA" }; + } +} + +function stateBadgeStyle(item: GitHubPrListItem): React.CSSProperties { + const c = stateColor(item.state); + return { + display: "inline-flex", + alignItems: "center", + padding: "2px 7px", + fontSize: 10, + fontWeight: 600, + fontFamily: SANS_FONT, + color: c.text, + background: c.bg, + border: `1px solid ${c.border}`, + borderRadius: 5, + textTransform: "capitalize", + }; +} + +/* -- CI status dot color -- */ +function ciDotColor(linkedPr: PrSummary | null): { color: string; title: string } | null { + if (!linkedPr) return null; + switch (linkedPr.checksStatus) { + case "passing": + return { color: COLORS.success, title: "CI passing" }; + case "failing": + return { color: COLORS.danger, title: "CI failing" }; + case "pending": + return { color: COLORS.warning, title: "CI pending" }; + default: + return null; + } +} + +/* -- Review status indicator -- */ +function reviewIndicator(linkedPr: PrSummary | null): { color: string; label: string } | null { + if (!linkedPr) return null; + switch (linkedPr.reviewStatus) { + case "approved": + return { color: COLORS.success, label: "Approved" }; + case "changes_requested": + return { color: COLORS.danger, label: "Changes" }; + case "requested": + return { color: COLORS.warning, label: "Review required" }; + default: + return null; + } +} + +/* -- adeKind badge with distinctive styling -- */ +const ADE_KIND_STYLES: Record = { + integration: { + color: "#FBBF24", + background: "linear-gradient(135deg, rgba(245,158,11,0.14) 0%, rgba(217,119,6,0.06) 100%)", + border: "1px solid rgba(245,158,11,0.22)", + }, +}; + +function AdeKindBadge({ kind }: { kind: GitHubPrListItem["adeKind"] }): React.ReactElement | null { + if (!kind || kind === "single") return null; + const style = ADE_KIND_STYLES[kind]; + if (!style) return null; + return {kind}; +} + +function adeKindBadgeStyle(style: { color: string; background: string; border: string }): React.CSSProperties { + return { + display: "inline-flex", + alignItems: "center", + padding: "2px 7px", + fontSize: 10, + fontWeight: 600, + fontFamily: SANS_FONT, + color: style.color, + background: style.background, + border: style.border, + borderRadius: 5, + }; +} + +/* -- Label text color from hex background (luminance-aware) -- */ +function labelTextColor(hexColor: string): string { + const hex = hexColor.replace("#", ""); + const r = parseInt(hex.substring(0, 2), 16) || 0; + const g = parseInt(hex.substring(2, 4), 16) || 0; + const b = parseInt(hex.substring(4, 6), 16) || 0; + const luminance = (0.299 * r + 0.587 * g + 0.114 * b) / 255; + return luminance > 0.5 ? "#1a1a2e" : "#f0f0f0"; +} + + +/** + * Whether the "unmapped" badge would actually lead somewhere. Selecting the row offers + * one of two actions depending on whether a lane already tracks the head branch — map + * to it, or create one — so the presence of a usable local branch is the real gate. + * Fork PRs have no local branch to work with, and a terminal PR cannot be mapped at all. + * + * Drives the amber-vs-neutral choice, so the warning colour is only ever spent on rows + * the user can do something about. + */ +function isPrRowMappable(item: GitHubPrListItem): boolean { + if (item.linkedPrId || item.scope !== "repo") return false; + if (item.state !== "open" && item.state !== "draft") return false; + return Boolean(branchNameFromRef(item.headBranch)); +} + +/** + * The lane column of a PR row. + * + * Three states, deliberately distinct: + * - **mapped** — the lane chip, in the lane's colour. + * - **detached** — `was: ` plus the activity frozen when the lane was deleted. + * This is history, so it is dim and carries no call to action. + * - **no lane** — nothing at all in terminal buckets (absence already reads as "no + * lane"), and a neutral chip in Open. It only turns amber when there is genuinely + * something to do, so amber keeps meaning "act on this". + */ +function PrRowLaneChip({ + item, + linkedLaneColor, + mappable, +}: { + item: GitHubPrListItem; + linkedLaneColor: string | null; + mappable: boolean; +}) { + if (item.linkedLaneName || item.linkedLaneId) { + return ( + + {linkedLaneColor ? : null} + {item.linkedLaneName ?? item.linkedLaneId} + + ); + } + + if (item.detached) return ; + + // Terminal PRs have no mapping story worth telling — the lane is gone and mapping one + // now would do nothing. Showing a badge here is what made Merged a wall of warnings. + if (isTerminalPrState(item.state)) return null; + + const actionable = mappable; + return ( + + unmapped + + ); +} + +/** + * `arul · squash → main` — how a terminal PR shipped, folded onto the meta line in + * place of the branch row. Every part is optional: PRs merged before ADE recorded + * merge metadata simply show less, rather than showing placeholders. + */ +function PrRowMergeFacts({ item }: { item: GitHubPrListItem }) { + const parts = [ + item.mergedBy?.login ?? null, + item.mergeMethod, + item.baseBranch ? `→ ${item.baseBranch}` : null, + ].filter(Boolean) as string[]; + if (parts.length === 0) return null; + return ( + + {parts.join(" · ")} + + ); +} + +function PrRowDiffStat({ additions, deletions }: { additions: number | null; deletions: number | null }) { + if (additions == null && deletions == null) return null; + return ( + + +{additions ?? 0} + -{deletions ?? 0} + + ); +} + +/** `was: · 3 chats · 2 proof` — what ADE knows that GitHub cannot show. */ +function PrRowGhostLaneChip({ detached }: { detached: NonNullable }) { + const counts = [ + detached.chats > 0 ? `${detached.chats} chat${detached.chats === 1 ? "" : "s"}` : null, + detached.artifacts > 0 ? `${detached.artifacts} proof` : null, + ].filter(Boolean) as string[]; + const name = detached.laneName?.trim(); + const detachedAgo = formatTimeAgoCompact(detached.at); + if (!name && counts.length === 0) return null; + return ( + + {detached.laneColor ? : null} + {name ? was: {name} : null} + {counts.length > 0 ? · {counts.join(" · ")} : null} + + ); +} + +/* ---- PR row (shared between list and virtualizer) ---- */ +function useLaneColorById(laneId: string | null | undefined): string | null { + return useAppStore((s) => { + if (!laneId) return null; + return s.lanes.find((l) => l.id === laneId)?.color ?? null; + }); +} + +export function GitHubTabPrRow({ + item, + selected, + linkedPr, + onSelect, +}: { + item: GitHubPrListItem; + selected: boolean; + linkedPr: PrSummary | null; + onSelect: (item: GitHubPrListItem) => void; +}) { + const sc = stateColor(item.state); + // A merged PR is a record, not a queue item: CI outcome, "review required" and the + // state badge are all answered by the fact that it merged. Dropping them is what lets + // the row collapse to two lines and stops the list reading as a wall of signals. + const terminal = isTerminalPrState(item.state); + const ci = terminal ? null : ciDotColor(linkedPr); + const ciRunning = linkedPr?.checksStatus === "pending"; + const review = terminal ? null : reviewIndicator(linkedPr); + // Open rows are about how long something has been waiting; merged rows are about + // when it shipped. + const ago = formatTimeAgoCompact(terminal ? (item.mergedAt ?? item.updatedAt) : item.createdAt); + const labels = item.labels ?? []; + const visibleLabels = labels.slice(0, 4); + const overflowCount = labels.length - 4; + const rowLinkedLaneColor = useLaneColorById(item.linkedLaneId ?? null); + const mappable = isPrRowMappable(item); + return ( + + ); +} + +/** + * Period header for the merged/closed log. Announced as a heading so screen readers get + * the same structure sighted users do, instead of an undifferentiated pile of buttons. + */ +export function PrListGroupHeaderRow({ header }: { header: PrListGroupHeaderModel }) { + const diff = formatPrListGroupDiff(header.additions, header.deletions); + return ( +
+ {header.label} + + {header.count} {header.outcome}{diff ? ` · ${diff}` : ""} + +
+ ); +} diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx index 683732073..1471b631a 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx @@ -33,6 +33,8 @@ import { GitHubStackBadge } from "../shared/GitHubStackBadge"; import { GitHubStackInspector } from "../shared/GitHubStackInspector"; import { getGitHubSnapshotCoalesced } from "../../../lib/prReadCache"; import { isTerminalPrState } from "../../../lib/prState"; +import { GitHubTabPrRow, PrListGroupHeaderRow } from "../shared/GitHubTabPrRow"; +import { branchNameFromRef } from "./githubPrBranch"; import { buildPrListRows, formatPrListGroupDiff, @@ -41,6 +43,22 @@ import { type PrListRow, } from "../shared/prListGrouping"; +/* -- Filter button styles -- */ +const FILTER_COLORS: Record = { + open: { + active: { bg: "linear-gradient(135deg, rgba(59,130,246,0.16) 0%, rgba(37,99,235,0.08) 100%)", border: "rgba(59,130,246,0.30)", text: "#60A5FA", shadow: "0 0 10px rgba(59,130,246,0.12)" }, + inactive: { text: "#60A5FA" }, + }, + closed: { + active: { bg: "linear-gradient(135deg, rgba(161,161,170,0.14) 0%, rgba(113,113,122,0.06) 100%)", border: "rgba(161,161,170,0.25)", text: "#A1A1AA", shadow: "0 0 10px rgba(161,161,170,0.08)" }, + inactive: { text: "#71717A" }, + }, + merged: { + active: { bg: "linear-gradient(135deg, rgba(34,197,94,0.14) 0%, rgba(22,163,74,0.06) 100%)", border: "rgba(34,197,94,0.28)", text: "#4ADE80", shadow: "0 0 10px rgba(34,197,94,0.10)" }, + inactive: { text: "#4ADE80" }, + }, +}; + const VIRTUALIZE_AT = 50; const LINKED_HYDRATION_LIMIT = 8; const GITHUB_TAB_REVISIT_CACHE_TTL_MS = 60_000; @@ -393,149 +411,7 @@ function countGitHubItemsByState(items: GitHubPrListItem[]): GitHubFilterCounts }; } -/* -- Color-coded state badge with distinct colors per state -- */ -function stateColor(state: string): { bg: string; border: string; text: string } { - switch (state) { - case "open": - return { bg: "rgba(59,130,246,0.10)", border: "rgba(59,130,246,0.20)", text: "#60A5FA" }; - case "draft": - return { bg: "rgba(245,158,11,0.10)", border: "rgba(245,158,11,0.20)", text: "#FBBF24" }; - case "merged": - return { bg: "rgba(34,197,94,0.10)", border: "rgba(34,197,94,0.20)", text: "#4ADE80" }; - default: - return { bg: "rgba(161,161,170,0.08)", border: "rgba(161,161,170,0.15)", text: "#A1A1AA" }; - } -} - -function stateBadgeStyle(item: GitHubPrListItem): React.CSSProperties { - const c = stateColor(item.state); - return { - display: "inline-flex", - alignItems: "center", - padding: "2px 7px", - fontSize: 10, - fontWeight: 600, - fontFamily: SANS_FONT, - color: c.text, - background: c.bg, - border: `1px solid ${c.border}`, - borderRadius: 5, - textTransform: "capitalize", - }; -} - -/* -- CI status dot color -- */ -function ciDotColor(linkedPr: PrSummary | null): { color: string; title: string } | null { - if (!linkedPr) return null; - switch (linkedPr.checksStatus) { - case "passing": - return { color: COLORS.success, title: "CI passing" }; - case "failing": - return { color: COLORS.danger, title: "CI failing" }; - case "pending": - return { color: COLORS.warning, title: "CI pending" }; - default: - return null; - } -} - -/* -- Review status indicator -- */ -function reviewIndicator(linkedPr: PrSummary | null): { color: string; label: string } | null { - if (!linkedPr) return null; - switch (linkedPr.reviewStatus) { - case "approved": - return { color: COLORS.success, label: "Approved" }; - case "changes_requested": - return { color: COLORS.danger, label: "Changes" }; - case "requested": - return { color: COLORS.warning, label: "Review required" }; - default: - return null; - } -} - -/* -- adeKind badge with distinctive styling -- */ -const ADE_KIND_STYLES: Record = { - integration: { - color: "#FBBF24", - background: "linear-gradient(135deg, rgba(245,158,11,0.14) 0%, rgba(217,119,6,0.06) 100%)", - border: "1px solid rgba(245,158,11,0.22)", - }, -}; - -function AdeKindBadge({ kind }: { kind: GitHubPrListItem["adeKind"] }): React.ReactElement | null { - if (!kind || kind === "single") return null; - const style = ADE_KIND_STYLES[kind]; - if (!style) return null; - return {kind}; -} - -function adeKindBadgeStyle(style: { color: string; background: string; border: string }): React.CSSProperties { - return { - display: "inline-flex", - alignItems: "center", - padding: "2px 7px", - fontSize: 10, - fontWeight: 600, - fontFamily: SANS_FONT, - color: style.color, - background: style.background, - border: style.border, - borderRadius: 5, - }; -} - -/* -- Label text color from hex background (luminance-aware) -- */ -function labelTextColor(hexColor: string): string { - const hex = hexColor.replace("#", ""); - const r = parseInt(hex.substring(0, 2), 16) || 0; - const g = parseInt(hex.substring(2, 4), 16) || 0; - const b = parseInt(hex.substring(4, 6), 16) || 0; - const luminance = (0.299 * r + 0.587 * g + 0.114 * b) / 255; - return luminance > 0.5 ? "#1a1a2e" : "#f0f0f0"; -} - -/* -- Filter button styles -- */ -const FILTER_COLORS: Record = { - open: { - active: { bg: "linear-gradient(135deg, rgba(59,130,246,0.16) 0%, rgba(37,99,235,0.08) 100%)", border: "rgba(59,130,246,0.30)", text: "#60A5FA", shadow: "0 0 10px rgba(59,130,246,0.12)" }, - inactive: { text: "#60A5FA" }, - }, - closed: { - active: { bg: "linear-gradient(135deg, rgba(161,161,170,0.14) 0%, rgba(113,113,122,0.06) 100%)", border: "rgba(161,161,170,0.25)", text: "#A1A1AA", shadow: "0 0 10px rgba(161,161,170,0.08)" }, - inactive: { text: "#71717A" }, - }, - merged: { - active: { bg: "linear-gradient(135deg, rgba(34,197,94,0.14) 0%, rgba(22,163,74,0.06) 100%)", border: "rgba(34,197,94,0.28)", text: "#4ADE80", shadow: "0 0 10px rgba(34,197,94,0.10)" }, - inactive: { text: "#4ADE80" }, - }, -}; - -function useLaneColorById(laneId: string | null | undefined): string | null { - return useAppStore((s) => { - if (!laneId) return null; - return s.lanes.find((l) => l.id === laneId)?.color ?? null; - }); -} - -/** - * Whether the "unmapped" badge would actually lead somewhere. Selecting the row offers - * one of two actions depending on whether a lane already tracks the head branch — map - * to it, or create one — so the presence of a usable local branch is the real gate. - * Fork PRs have no local branch to work with, and a terminal PR cannot be mapped at all. - * - * Drives the amber-vs-neutral choice, so the warning colour is only ever spent on rows - * the user can do something about. - */ -function isPrRowMappable(item: GitHubPrListItem): boolean { - if (item.linkedPrId || item.scope !== "repo") return false; - if (item.state !== "open" && item.state !== "draft") return false; - return Boolean(branchNameFromRef(item.headBranch)); -} -function branchNameFromRef(ref: string | null | undefined): string { - return String(ref ?? "").replace(/^refs\/heads\//, "").trim(); -} function canCreateLaneFromPrBranch(item: GitHubPrListItem, lanes: LaneSummary[]): boolean { if (item.linkedPrId || item.scope !== "repo") return false; @@ -1977,358 +1853,6 @@ function PrBucketTransitionBanner({ ); } -/** - * The lane column of a PR row. - * - * Three states, deliberately distinct: - * - **mapped** — the lane chip, in the lane's colour. - * - **detached** — `was: ` plus the activity frozen when the lane was deleted. - * This is history, so it is dim and carries no call to action. - * - **no lane** — nothing at all in terminal buckets (absence already reads as "no - * lane"), and a neutral chip in Open. It only turns amber when there is genuinely - * something to do, so amber keeps meaning "act on this". - */ -function PrRowLaneChip({ - item, - linkedLaneColor, - mappable, -}: { - item: GitHubPrListItem; - linkedLaneColor: string | null; - mappable: boolean; -}) { - if (item.linkedLaneName || item.linkedLaneId) { - return ( - - {linkedLaneColor ? : null} - {item.linkedLaneName ?? item.linkedLaneId} - - ); - } - - if (item.detached) return ; - - // Terminal PRs have no mapping story worth telling — the lane is gone and mapping one - // now would do nothing. Showing a badge here is what made Merged a wall of warnings. - if (isTerminalPrState(item.state)) return null; - - const actionable = mappable; - return ( - - unmapped - - ); -} - -/** - * `arul · squash → main` — how a terminal PR shipped, folded onto the meta line in - * place of the branch row. Every part is optional: PRs merged before ADE recorded - * merge metadata simply show less, rather than showing placeholders. - */ -function PrRowMergeFacts({ item }: { item: GitHubPrListItem }) { - const parts = [ - item.mergedBy?.login ?? null, - item.mergeMethod, - item.baseBranch ? `→ ${item.baseBranch}` : null, - ].filter(Boolean) as string[]; - if (parts.length === 0) return null; - return ( - - {parts.join(" · ")} - - ); -} - -function PrRowDiffStat({ additions, deletions }: { additions: number | null; deletions: number | null }) { - if (additions == null && deletions == null) return null; - return ( - - +{additions ?? 0} - -{deletions ?? 0} - - ); -} - -/** `was: · 3 chats · 2 proof` — what ADE knows that GitHub cannot show. */ -function PrRowGhostLaneChip({ detached }: { detached: NonNullable }) { - const counts = [ - detached.chats > 0 ? `${detached.chats} chat${detached.chats === 1 ? "" : "s"}` : null, - detached.artifacts > 0 ? `${detached.artifacts} proof` : null, - ].filter(Boolean) as string[]; - const name = detached.laneName?.trim(); - const detachedAgo = formatTimeAgoCompact(detached.at); - if (!name && counts.length === 0) return null; - return ( - - {detached.laneColor ? : null} - {name ? was: {name} : null} - {counts.length > 0 ? · {counts.join(" · ")} : null} - - ); -} - -/* ---- PR row (shared between list and virtualizer) ---- */ -function GitHubTabPrRow({ - item, - selected, - linkedPr, - onSelect, -}: { - item: GitHubPrListItem; - selected: boolean; - linkedPr: PrSummary | null; - onSelect: (item: GitHubPrListItem) => void; -}) { - const sc = stateColor(item.state); - // A merged PR is a record, not a queue item: CI outcome, "review required" and the - // state badge are all answered by the fact that it merged. Dropping them is what lets - // the row collapse to two lines and stops the list reading as a wall of signals. - const terminal = isTerminalPrState(item.state); - const ci = terminal ? null : ciDotColor(linkedPr); - const ciRunning = linkedPr?.checksStatus === "pending"; - const review = terminal ? null : reviewIndicator(linkedPr); - // Open rows are about how long something has been waiting; merged rows are about - // when it shipped. - const ago = formatTimeAgoCompact(terminal ? (item.mergedAt ?? item.updatedAt) : item.createdAt); - const labels = item.labels ?? []; - const visibleLabels = labels.slice(0, 4); - const overflowCount = labels.length - 4; - const rowLinkedLaneColor = useLaneColorById(item.linkedLaneId ?? null); - const mappable = isPrRowMappable(item); - return ( - - ); -} - /* ---- Virtual list for GitHub PR sidebar (activated above VIRTUALIZE_AT) ---- */ function GitHubTabVirtualList({ parentRef, @@ -2346,12 +1870,6 @@ function GitHubTabVirtualList({ onHydrationItemsChange: (items: GitHubPrListItem[]) => void; }) { const headerIndices = React.useMemo(() => prListHeaderIndices(rows), [rows]); - // The header governing the current scroll position. Kept in a ref as well as state - // so `rangeExtractor` (called during measurement) can read it without re-subscribing. - const activeHeaderRef = React.useRef(headerIndices[0] ?? null); - const [activeHeaderIndex, setActiveHeaderIndex] = React.useState( - headerIndices[0] ?? null, - ); const virtualizer = useVirtualizer({ count: rows.length, @@ -2361,10 +1879,9 @@ function GitHubTabVirtualList({ overscan: 6, rangeExtractor: React.useCallback( (range: { startIndex: number; endIndex: number; overscan: number; count: number }) => { - // Pin the header for the topmost visible row so the period stays legible while - // scrolling deep into history. - const pinned = headerIndices.filter((index) => index <= range.startIndex).pop() ?? null; - activeHeaderRef.current = pinned; + // Keep the governing header rendered even once it has scrolled out of range, + // so it can be pinned to the top of the viewport. + const pinned = activeHeaderFor(headerIndices, range.startIndex); const start = Math.max(0, range.startIndex - range.overscan); const end = Math.min(range.count - 1, range.endIndex + range.overscan); const indices = new Set(); @@ -2377,10 +1894,10 @@ function GitHubTabVirtualList({ }); const virtualItems = virtualizer.getVirtualItems(); - - React.useEffect(() => { - if (activeHeaderRef.current !== activeHeaderIndex) setActiveHeaderIndex(activeHeaderRef.current); - }, [activeHeaderIndex, virtualItems]); + // Derived during render from the range we are about to draw. An earlier version + // round-tripped this through a ref and a state-setting effect, which re-rendered the + // whole list on every scroll frame to compute something already in hand. + const activeHeaderIndex = activeHeaderFor(headerIndices, virtualItems[0]?.index ?? 0); React.useEffect(() => { // Only PR rows may be hydrated — a header has nothing to fetch. @@ -2431,35 +1948,13 @@ function GitHubTabVirtualList({ ); } -/** - * Period header for the merged/closed log. Announced as a heading so screen readers get - * the same structure sighted users do, instead of an undifferentiated pile of buttons. - */ -function PrListGroupHeaderRow({ header }: { header: PrListGroupHeaderModel }) { - const diff = formatPrListGroupDiff(header.additions, header.deletions); - return ( -
- {header.label} - - {header.count} {header.outcome}{diff ? ` · ${diff}` : ""} - -
- ); +/** The last header at or above `startIndex` — the one governing that scroll position. */ +function activeHeaderFor(headerIndices: number[], startIndex: number): number | null { + let active: number | null = null; + for (const index of headerIndices) { + if (index > startIndex) break; + active = index; + } + return active; } + diff --git a/apps/desktop/src/renderer/components/prs/tabs/githubPrBranch.ts b/apps/desktop/src/renderer/components/prs/tabs/githubPrBranch.ts new file mode 100644 index 000000000..46d5ec015 --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/tabs/githubPrBranch.ts @@ -0,0 +1,4 @@ +/** Strip the `refs/heads/` prefix so lane refs and PR head branches compare directly. */ +export function branchNameFromRef(ref: string | null | undefined): string { + return String(ref ?? "").replace(/^refs\/heads\//, "").trim(); +} From ca6d7c61d22696870e6e3078f8ea087d39c59eef Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sat, 1 Aug 2026 01:08:23 -0400 Subject: [PATCH 2/4] fix(prs): close quality follow-up gaps --- .agents/skills/quality/SKILL.md | 8 ++++++-- .agents/skills/ship/SKILL.md | 4 ++-- .agents/skills/test/SKILL.md | 12 ++++++++++-- .../main/services/prs/pullRequestRowCleanup.ts | 9 ++------- .../components/prs/shared/GitHubTabPrRow.tsx | 3 +-- .../renderer/components/prs/tabs/GitHubTab.tsx | 12 ++---------- docs/playbooks/ship-lane.md | 16 ++++++++++++---- 7 files changed, 35 insertions(+), 29 deletions(-) diff --git a/.agents/skills/quality/SKILL.md b/.agents/skills/quality/SKILL.md index facc3f158..76319fa2c 100644 --- a/.agents/skills/quality/SKILL.md +++ b/.agents/skills/quality/SKILL.md @@ -121,8 +121,9 @@ are handled by the synthesis step below, not a separate phase. do it here, in this run, not "as a follow-up". 7. **Re-review until clean.** If step 6 changed code, re-run Track A on the *new* diff. New accepted findings → verify (4), apply (6), re-check. Stop when a - pass yields no new accepted findings (cap 2 extra passes; anything still open - goes to the gate). Catches fix-induced regressions before `/test` or `/ship`. + pass yields no new accepted findings. A re-review count is never a reason to + defer a verified finding or move it to the gate. This catches fix-induced + regressions before `/test` or `/ship`. 8. **Gate — the narrow exception, not the escape hatch.** Only two kinds of accepted finding may go to the gate unfixed: - it needs a **product decision you cannot make** (which of two valid @@ -165,6 +166,9 @@ is not behavior-preserving on a branch that was not asked to change behavior. Empty is the expected outcome. "Structural / large / out of scope" is not a gate reason — those get fixed above. +When empty, print `- Empty.` and omit the table. Never leave an example or +placeholder row that another skill could mistake for a live gate. + | Severity | file:line | Finding | Which gate reason | Decision needed | |---|---|---|---|---| | Blocker | ... | ... | product decision | which behavior do you want? | diff --git a/.agents/skills/ship/SKILL.md b/.agents/skills/ship/SKILL.md index 21e14fe00..42ee06c1c 100644 --- a/.agents/skills/ship/SKILL.md +++ b/.agents/skills/ship/SKILL.md @@ -48,8 +48,8 @@ a behavior change this branch was not asked to make. Both need the author. - Gate rows exist → do not merge. Surface them, state the decision needed, and stop with `blocked`. Do not merge and mention them afterwards. -- If `/quality` was never run on this lane, say so plainly rather than assuming - the gate is empty. +- If `/quality` was never run on this lane, or its final gate result is not + available in the lane handoff, stop with `blocked`; unknown is not empty. Severity is irrelevant here: a Medium in the gate blocks exactly as hard as a Blocker, because presence in the gate means it needed a human, not that it was diff --git a/.agents/skills/test/SKILL.md b/.agents/skills/test/SKILL.md index fb13b3235..369f72bf5 100644 --- a/.agents/skills/test/SKILL.md +++ b/.agents/skills/test/SKILL.md @@ -1,6 +1,6 @@ --- name: test -description: 'Prove the new code works: enforce the logging/PostHog ground truth, prune dead tests, consolidate fragments, add only tests that prove new contracts, turn each /quality Blocker/High finding into a named regression test, then run CI-mirrored shards. Also keeps docs/mobile/CLI/TUI parity in lockstep.' +description: 'Prove the new code works: enforce the logging/PostHog ground truth, prune dead tests, consolidate fragments, add only tests that prove new contracts, turn accepted /quality correctness findings into named regression tests, then run CI-mirrored shards. Also keeps docs/mobile/CLI/TUI parity in lockstep.' --- # /test — Test Suite Steward @@ -17,7 +17,15 @@ The suite has bloated for three reasons. You exist to fight all three: Every run does three passes in this order: **PRUNE → CONSOLIDATE → ADD**. You may finish at any pass — adding is optional. -**Consume the `/quality` gate.** If `/quality` ran on this lane, take its Summary's **Gate** section — every finding it surfaced but did not fix, at any severity (a Medium in the gate got there because it needed a human, not because it was minor). Each is a named regression-test target for the ADD pass: a test that fails on the bug and passes once it's fixed. A finding isn't "handled" until a test pins it. No gate available → derive the same targets from the diff. +**Consume the `/quality` result.** A non-empty `/quality` gate blocks this skill: +the branch still contains a verified finding awaiting an author decision, and +committing a knowingly failing test is not a substitute for fixing it. Resume +after the decision and the corresponding `/quality` fix. From the completed +quality summary, turn accepted correctness findings into named regression-test +targets when a test can pin the public contract. Structural maintainability +findings do not require artificial tests when existing coverage already proves +the behavior-preserving move. No quality result available → derive the same +targets from the diff and state that quality evidence was unavailable. **Run the way CI would.** After the suite work, run only the affected shards, never the full suite (that's `/finalize`'s local gate and `/ship`'s remote CI). Verify every new/edited test file matches a vitest workspace glob so CI actually picks it up. diff --git a/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts b/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts index cb3ae181a..3f4f1de72 100644 --- a/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts +++ b/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts @@ -117,14 +117,9 @@ export function deletePullRequestRowsByIds(db: DbLike, projectId: string, prIds: * Storage does not grow: the heavy snapshot columns are nulled here, which frees more * than the retained row costs. `commit_count` / `changed_files` are lifted onto the row * first so the merged view survives the purge. - */ -/** - * Apply the detach mutation to an explicit set of PR ids. * - * Taking ids rather than a SQL predicate keeps one parameter list for all four - * statements below; an earlier version threaded a predicate string plus a params array - * that every statement had to agree with positionally, which is a bug waiting to happen - * for no benefit. + * The mutation takes explicit PR ids rather than a SQL predicate, keeping one parameter + * list for all four statements below instead of making them agree positionally. */ function detachRows( db: ReadableDbLike, diff --git a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx index dd40a6c72..ba5619517 100644 --- a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx @@ -10,6 +10,7 @@ import { formatTimeAgoCompact } from "./prFormatters"; import { PrCiRunningIndicator } from "./prVisuals"; import { GitHubStackBadge } from "./GitHubStackBadge"; import { formatPrListGroupDiff, type PrListGroupHeader as PrListGroupHeaderModel } from "./prListGrouping"; +import { branchNameFromRef } from "../tabs/githubPrBranch"; /** * Presentation for one row of the GitHub PR list, and the period header that groups @@ -18,8 +19,6 @@ import { formatPrListGroupDiff, type PrListGroupHeader as PrListGroupHeaderModel * coordinator rather than a coordinator plus 450 lines of row markup. */ -import { branchNameFromRef } from "../tabs/githubPrBranch"; - /* -- Color-coded state badge with distinct colors per state -- */ function stateColor(state: string): { bg: string; border: string; text: string } { switch (state) { diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx index 1471b631a..878360efc 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx @@ -1,5 +1,5 @@ import React from "react"; -import { ArrowSquareOut, ChatText, CheckCircle, CircleNotch, GitBranch, GitMerge, GithubLogo, Warning, XCircle } from "@phosphor-icons/react"; +import { CircleNotch, GitBranch, GitMerge, GithubLogo, Warning, XCircle } from "@phosphor-icons/react"; import { useNavigate } from "react-router-dom"; import { Group, Panel } from "react-resizable-panels"; import { useVirtualizer } from "@tanstack/react-virtual"; @@ -19,17 +19,13 @@ import type { import { syntheticGithubPrId } from "../../../../shared/types/prs"; import { EmptyState } from "../../ui/EmptyState"; import { ResizeGutter } from "../../ui/ResizeGutter"; -import { COLORS, LABEL_STYLE, MONO_FONT, SANS_FONT, cardStyle, inlineBadge, outlineButton, primaryButton } from "../../lanes/laneDesignTokens"; -import { LaneAccentDot } from "../../lanes/LaneAccentDot"; +import { COLORS, LABEL_STYLE, MONO_FONT, SANS_FONT, cardStyle, outlineButton, primaryButton } from "../../lanes/laneDesignTokens"; import { selectActiveProjectRoot, useAppStore, useAppStoreApi } from "../../../state/appStore"; import { PrDetailPane, type UnmappedAffordance } from "../detail/PrDetailPane"; -import { formatTimeAgoCompact } from "../shared/prFormatters"; -import { PrCiRunningIndicator } from "../shared/prVisuals"; import { usePrs } from "../state/PrsContext"; import type { PrDetailRouteTab } from "../prsRouteState"; import { GitHubRepoSyncBar } from "../shared/GitHubRepoSyncBar"; import { GitHubPrSearchInput } from "../shared/GitHubPrSearchInput"; -import { GitHubStackBadge } from "../shared/GitHubStackBadge"; import { GitHubStackInspector } from "../shared/GitHubStackInspector"; import { getGitHubSnapshotCoalesced } from "../../../lib/prReadCache"; import { isTerminalPrState } from "../../../lib/prState"; @@ -37,9 +33,7 @@ import { GitHubTabPrRow, PrListGroupHeaderRow } from "../shared/GitHubTabPrRow"; import { branchNameFromRef } from "./githubPrBranch"; import { buildPrListRows, - formatPrListGroupDiff, prListHeaderIndices, - type PrListGroupHeader as PrListGroupHeaderModel, type PrListRow, } from "../shared/prListGrouping"; @@ -629,7 +623,6 @@ export function GitHubTab({ const appStore = useAppStoreApi(); const { prs, - mergeContextByPrId, detailStatus, detailChecks, detailReviews, @@ -1957,4 +1950,3 @@ function activeHeaderFor(headerIndices: number[], startIndex: number): number | } return active; } - diff --git a/docs/playbooks/ship-lane.md b/docs/playbooks/ship-lane.md index e9ada0f8d..763db73be 100644 --- a/docs/playbooks/ship-lane.md +++ b/docs/playbooks/ship-lane.md @@ -143,13 +143,16 @@ Ship is a **pure merge loop** — it does NOT run `/quality`, `/test`, or reaching ship. Ship assumes the lane is already reviewed, tested, and (optionally) finalized; it never generates tests or simplifies code itself. -Sanity-check only here: +Preconditions here: - `git status` must be clean of foreign changes. If uncommitted changes belong to this lane, commit them with `ship: checkpoint before ship`. If they're unrelated, exit `blocked` with `exitReason: "dirty-working-tree"`. -- If the lane was never run through `/quality` or `/test`, that's the author's - call — ship does not block on it, but note it in the first iteration summary. +- The lane must have completed `/quality` and `/test`. Read the final quality + result from the current conversation or lane handoff. A non-empty gate, a + missing result, or an ambiguous placeholder row blocks ship; set + `exitReason: "quality-gate-nonempty"` or `"quality-result-missing"` rather + than assuming unknown means clean. Ship does not run either skill itself. ### 0.3 Commit + push + create PR @@ -477,6 +480,11 @@ Post bot pings (Phase 4), update state (Phase 5), and schedule the next wake. Do Runs when Phase 2 routes here (everything terminal, no fix work, not behind, not already merged). The point of this playbook is "PR-to-merge", not "PR-to-green" — once green, the lane lands. +Before resolving merge style, re-read the lane's final `/quality` result. The +gate must still be explicitly empty. If it is non-empty or unavailable, exit +`blocked` with `quality-gate-nonempty` or `quality-result-missing`; never merge +and disclose deferred findings afterwards. + ### 3c.1 Resolve repo merge style ```bash @@ -679,7 +687,7 @@ The cadence is a hint, not a live polling budget. Prefer longer sleeps over freq | --- | --- | --- | | `done-clean` | PR merged on `main` (Phase 3c succeeded, possibly after Phase 3d force-finalize) | clear state file; print summary | | `done-max` | 5 normal iterations + 1 force-finalize iteration exhausted AND Phase 3c could not merge (policy block + no admin + no auto-merge) | leave state file; post PR handoff comment to human | -| `blocked` | Unrecoverable conflict, gate failure, API error, or `force-finalize-ci-failed` (iteration 6 could not turn CI green) | leave state file; post PR comment with reason | +| `blocked` | Unrecoverable conflict, missing/non-empty quality gate, API error, or `force-finalize-ci-failed` (iteration 6 could not turn CI green) | leave state file; post PR comment with reason | ## Summary output (always print on exit) From 595a24fb6601cde972d6bedbba9df8a4aedf471f Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sat, 1 Aug 2026 02:23:06 -0400 Subject: [PATCH 3/4] =?UTF-8?q?ship:=20iteration=201=20=E2=80=94=20resolve?= =?UTF-8?q?=20all=20quality=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .agents/skills/quality/SKILL.md | 29 +- .agents/skills/ship/SKILL.md | 38 +- .agents/skills/test/SKILL.md | 31 +- .../main/services/prs/prRowMetadata.test.ts | 81 ++ .../src/main/services/prs/prRowMetadata.ts | 102 ++ .../src/main/services/prs/prService.ts | 91 +- .../prs/pullRequestRowCleanup.test.ts | 90 +- .../services/prs/pullRequestRowCleanup.ts | 68 +- .../components/prs/shared/GitHubTabPrRow.tsx | 136 +- .../components/prs/tabs/GitHubTab.test.tsx | 1044 +------------- .../prs/tabs/GitHubTab.testFixtures.ts | 232 ++++ .../prs/tabs/GitHubTab.testHarness.tsx | 131 ++ .../components/prs/tabs/GitHubTab.tsx | 1225 ++--------------- .../prs/tabs/GitHubTabCreateLaneDialog.tsx | 282 ++++ .../prs/tabs/GitHubTabRowsAndMapping.test.tsx | 739 ++++++++++ .../components/prs/tabs/GitHubTabView.tsx | 521 +++++++ .../components/prs/tabs/githubTabModel.ts | 231 ++++ .../prs/tabs/useGitHubTabListModel.ts | 117 ++ docs/playbooks/ship-lane.md | 172 ++- 19 files changed, 3034 insertions(+), 2326 deletions(-) create mode 100644 apps/desktop/src/main/services/prs/prRowMetadata.test.ts create mode 100644 apps/desktop/src/main/services/prs/prRowMetadata.ts create mode 100644 apps/desktop/src/renderer/components/prs/tabs/GitHubTab.testFixtures.ts create mode 100644 apps/desktop/src/renderer/components/prs/tabs/GitHubTab.testHarness.tsx create mode 100644 apps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsx create mode 100644 apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx create mode 100644 apps/desktop/src/renderer/components/prs/tabs/GitHubTabView.tsx create mode 100644 apps/desktop/src/renderer/components/prs/tabs/githubTabModel.ts create mode 100644 apps/desktop/src/renderer/components/prs/tabs/useGitHubTabListModel.ts diff --git a/.agents/skills/quality/SKILL.md b/.agents/skills/quality/SKILL.md index 76319fa2c..32792b7cb 100644 --- a/.agents/skills/quality/SKILL.md +++ b/.agents/skills/quality/SKILL.md @@ -119,11 +119,13 @@ are handled by the synthesis step below, not a separate phase. Fix correctness findings and Track B judo moves alike. If a fix is genuinely large (a multi-file extraction, a schema migration), it is still yours to do — do it here, in this run, not "as a follow-up". -7. **Re-review until clean.** If step 6 changed code, re-run Track A on the *new* - diff. New accepted findings → verify (4), apply (6), re-check. Stop when a - pass yields no new accepted findings. A re-review count is never a reason to - defer a verified finding or move it to the gate. This catches fix-induced - regressions before `/test` or `/ship`. +7. **Re-review until clean.** If step 6 changed code, re-run **both mandatory + tracks, A and B,** on the *new* diff. New accepted findings → verify (4), + apply (6), re-check with both tracks again. Stop only when the same pass + yields no new accepted findings from either track. A re-review count is never + a reason to defer a verified finding or move it to the gate. This catches + fix-induced correctness regressions and maintainability debt before `/test` + or `/ship`. 8. **Gate — the narrow exception, not the escape hatch.** Only two kinds of accepted finding may go to the gate unfixed: - it needs a **product decision you cannot make** (which of two valid @@ -166,15 +168,18 @@ is not behavior-preserving on a branch that was not asked to change behavior. Empty is the expected outcome. "Structural / large / out of scope" is not a gate reason — those get fixed above. -When empty, print `- Empty.` and omit the table. Never leave an example or -placeholder row that another skill could mistake for a live gate. +When empty, print exactly: -| Severity | file:line | Finding | Which gate reason | Decision needed | -|---|---|---|---|---| -| Blocker | ... | ... | product decision | which behavior do you want? | +- Empty. -Next: /test (turn each Blocker/High into a named regression test). +Do not print a table. When non-empty, replace `- Empty.` with a table containing +only real findings and these columns: Severity, file:line, Finding, Which gate +reason, Decision needed. Never leave an example or placeholder row that another +skill could mistake for a live gate. + +Next: /test (itemize every accepted correctness finding and give each a named +regression test or explicit alternate verification). **Before you print this:** every accepted finding is either in "Auto-applied" or -in this table. If one is in neither, go back to step 6 and fix it. +the Gate section. If one is in neither, go back to step 6 and fix it. ``` diff --git a/.agents/skills/ship/SKILL.md b/.agents/skills/ship/SKILL.md index 42ee06c1c..85773672b 100644 --- a/.agents/skills/ship/SKILL.md +++ b/.agents/skills/ship/SKILL.md @@ -4,8 +4,10 @@ description: >- Autonomous PR-to-merge loop. Polls CI and review bots, fixes failures, rebases only on real conflicts, and lands the PR on main. Soft cap of 5 normal iterations plus one force-finalize iteration that bypasses review and fixes - only CI. Pure loop — it does NOT run /quality or /test; run those first. Full - phase logic lives in docs/playbooks/ship-lane.md. + only CI. Pure loop — it does not replace the baseline /quality or /test runs; + run those first. It does revalidate quality after any ship-loop mutation so + the final result is bound to the exact commit merged. Full phase logic lives + in docs/playbooks/ship-lane.md. --- # Ship Skill — Autonomous Merge Loop @@ -38,18 +40,31 @@ The playbook's Phase 0 is **commit → push → open PR** only. Test generation the local-CI gate are NOT part of ship — that's `/test` (and optionally `/finalize`) before you reach this skill. -## Precondition: the `/quality` gate must be empty +## Precondition: `/quality` must be empty and bound to the merge commit -Before Phase 0, and again before the Phase 3c merge, check for an open -`/quality` gate. A non-empty gate **blocks the merge** — every row in it is a -finding that was verified as real and left unfixed, and by `/quality`'s contract -the only two things that may be there are a product decision the author owes, or -a behavior change this branch was not asked to make. Both need the author. +Before Phase 0, require a completed `/quality` result with an empty gate. Before +Phase 3c, also require `qualityValidatedSha` in the ship state to equal both +`git rev-parse HEAD` and the PR's current `headRefOid`. The result is valid only +for that exact commit; green CI on a later commit does not preserve it. + +A non-empty gate **blocks the merge** — every row in it is a finding that was +verified as real and left unfixed, and by `/quality`'s contract the only two +things that may be there are a product decision the author owes, or a behavior +change this branch was not asked to make. Both need the author. - Gate rows exist → do not merge. Surface them, state the decision needed, and stop with `blocked`. Do not merge and mention them afterwards. - If `/quality` was never run on this lane, or its final gate result is not available in the lane handoff, stop with `blocked`; unknown is not empty. +- Any rebase, conflict resolution, Phase 3b edit, or force-finalize edit clears + `qualityValidatedSha`. Run the playbook's single canonical **Commit-bound + quality revalidation** procedure before pushing that mutation. +- Never enter Phase 3c with a missing or mismatched binding. Revalidate first; + do not merge and disclose stale quality evidence afterwards. +- Bind every normal or admin merge attempt with + `--match-head-commit "$QUALITY_VALIDATED_SHA"`. Persistent auto-merge is not + allowed because a later push can replace the validated head while it remains + armed. Severity is irrelevant here: a Medium in the gate blocks exactly as hard as a Blocker, because presence in the gate means it needed a human, not that it was @@ -126,8 +141,11 @@ fix-iteration re-pushes → `@codex review`. For a >250-file diff, also ping expected review signals to settle before fixing. This is the playbook's Phase 4 rule — defer to it for exact bodies. -**Merge needs admin.** `main` is ruleset-guarded — `gh pr merge --squash` will -show BLOCKED. Retry with `gh pr merge --admin --squash`; the ruleset's +**Merge needs admin.** `main` is ruleset-guarded — +`gh pr merge --squash --match-head-commit "$QUALITY_VALIDATED_SHA"` will show +BLOCKED. Retry with +`gh pr merge --admin --squash --match-head-commit "$QUALITY_VALIDATED_SHA"`; +the ruleset's non-linear-history rule can still reject `--admin`, in which case fall back to a local merge + admin-bypass push (per AGENTS.md). Do NOT pass `--delete-branch` (it fails from a worktree); delete the head ref server-side via diff --git a/.agents/skills/test/SKILL.md b/.agents/skills/test/SKILL.md index 369f72bf5..1c805b341 100644 --- a/.agents/skills/test/SKILL.md +++ b/.agents/skills/test/SKILL.md @@ -20,12 +20,24 @@ Every run does three passes in this order: **PRUNE → CONSOLIDATE → ADD**. Yo **Consume the `/quality` result.** A non-empty `/quality` gate blocks this skill: the branch still contains a verified finding awaiting an author decision, and committing a knowingly failing test is not a substitute for fixing it. Resume -after the decision and the corresponding `/quality` fix. From the completed -quality summary, turn accepted correctness findings into named regression-test -targets when a test can pin the public contract. Structural maintainability -findings do not require artificial tests when existing coverage already proves -the behavior-preserving move. No quality result available → derive the same -targets from the diff and state that quality evidence was unavailable. +after the decision and the corresponding `/quality` fix. + +Build a correctness inventory from the completed quality summary. Itemize +**every accepted correctness finding** by a stable finding name and original +`file:line`; aggregate counts such as "5 findings covered" are not sufficient. +For each item, provide exactly one of: + +- a named regression test that pins the public contract and would fail on the + pre-fix behavior, or +- an explicit alternate verification: the exact command/check, the observed + evidence, and why a regression test is not appropriate. + +Existing coverage counts only when you name the specific test and confirm that +it exercises the finding's failure mode. Structural maintainability findings do +not require artificial tests when existing coverage already proves the +behavior-preserving move. No quality result available → derive the same +itemized correctness inventory from the diff and state that quality evidence +was unavailable. **Run the way CI would.** After the suite work, run only the affected shards, never the full suite (that's `/finalize`'s local gate and `/ship`'s remote CI). Verify every new/edited test file matches a vitest workspace glob so CI actually picks it up. @@ -579,6 +591,11 @@ Added: - — - Or "none — feature was visual / fully covered by consolidation" +Quality correctness findings: +- (`file:line`) — regression: ` :: ` +- (`file:line`) — alternate verification: `` → ``; no regression test because +- Or "none — /quality accepted no correctness findings" + Parity: - Logging/PostHog: — privacy + cost gate PASS / blocked - Docs: — validation PASS / blocked @@ -612,3 +629,5 @@ Mark **completed** only if all of: 5. Every new test file matches a vitest workspace glob. 6. The summary is the *only* thing you output. 7. `docs/logging.md` exists, was read, and every analytics-applicable change is covered or has an explicit not-applicable rationale. +8. Every accepted correctness finding from `/quality` appears individually in + the summary with a named regression test or explicit alternate verification. diff --git a/apps/desktop/src/main/services/prs/prRowMetadata.test.ts b/apps/desktop/src/main/services/prs/prRowMetadata.test.ts new file mode 100644 index 000000000..e3e712375 --- /dev/null +++ b/apps/desktop/src/main/services/prs/prRowMetadata.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "vitest"; +import { + deriveGithubSnapshotLaneLink, + deriveGithubSnapshotMergeFacts, + type PullRequestRowMetadata, +} from "./prRowMetadata"; + +const LANE_ID = "lane-42"; + +function makePrRowMetadata( + overrides: Partial = {}, +): PullRequestRowMetadata { + return { + lane_id: LANE_ID, + merged_at: null, + additions: null, + deletions: null, + ...overrides, + }; +} + +describe("PR row metadata mappers", () => { + it("separates a live lane mapping from frozen detached-lane provenance", () => { + const laneById = new Map([[LANE_ID, { name: "my-feature" }]]); + + expect(deriveGithubSnapshotLaneLink(makePrRowMetadata(), laneById)).toEqual({ + linkedLaneId: LANE_ID, + linkedLaneName: "my-feature", + detached: null, + }); + + expect(deriveGithubSnapshotLaneLink(makePrRowMetadata({ + detached_at: "2026-07-30T00:00:00Z", + detached_lane_name: "retired-lane", + detached_lane_color: "#4ADE80", + detached_provenance: JSON.stringify({ chats: 3, artifacts: 2, checkpoints: 5 }), + }), laneById)).toEqual({ + linkedLaneId: null, + linkedLaneName: null, + detached: { + at: "2026-07-30T00:00:00Z", + laneName: "retired-lane", + laneColor: "#4ADE80", + chats: 3, + artifacts: 2, + checkpoints: 5, + }, + }); + }); + + it("normalizes persisted merge facts for a GitHub list row", () => { + expect(deriveGithubSnapshotMergeFacts(makePrRowMetadata({ + merged_at: "2026-07-29T00:00:00Z", + merged_by_login: " octocat ", + merged_by_avatar_url: "https://example.com/octocat.png", + merge_method: "squash", + additions: 12, + deletions: 4, + commit_count: 3, + changed_files: 2, + }))).toEqual({ + mergedAt: "2026-07-29T00:00:00Z", + mergedBy: { login: "octocat", avatarUrl: "https://example.com/octocat.png" }, + mergeMethod: "squash", + additions: 12, + deletions: 4, + commitCount: 3, + changedFiles: 2, + }); + + expect(deriveGithubSnapshotMergeFacts(makePrRowMetadata({ + merge_method: "unsupported", + additions: -1, + commit_count: Number.NaN, + }))).toMatchObject({ + mergeMethod: null, + additions: null, + commitCount: null, + }); + }); +}); diff --git a/apps/desktop/src/main/services/prs/prRowMetadata.ts b/apps/desktop/src/main/services/prs/prRowMetadata.ts new file mode 100644 index 000000000..056aecdb3 --- /dev/null +++ b/apps/desktop/src/main/services/prs/prRowMetadata.ts @@ -0,0 +1,102 @@ +import type { + GitHubPrListItem, + MergeMethod, + PrDetachedLane, + PrMergedBy, +} from "../../../shared/types"; + +/** Persisted PR-row fields used to build list and summary metadata. */ +export type PullRequestRowMetadata = { + lane_id: string; + detached_at?: string | null; + detached_lane_name?: string | null; + detached_lane_color?: string | null; + detached_provenance?: string | null; + merged_at?: string | null; + merged_by_login?: string | null; + merged_by_avatar_url?: string | null; + merge_method?: string | null; + additions?: number | null; + deletions?: number | null; + commit_count?: number | null; + changed_files?: number | null; +}; + +/** Normalize a persisted non-negative count, returning null for absent or invalid data. */ +export function normalizeCount(value: unknown): number | null { + if (value == null) return null; + const parsed = Number(value); + return Number.isFinite(parsed) && parsed >= 0 ? parsed : null; +} + +/** Narrow persisted merge-method data to the supported public union. */ +export function normalizeMergeMethod(value: unknown): MergeMethod | null { + return value === "squash" || value === "merge" || value === "rebase" ? value : null; +} + +/** Rehydrate the GitHub actor that merged a persisted PR row. */ +export function rowMergedBy(row: PullRequestRowMetadata): PrMergedBy | null { + const login = String(row.merged_by_login ?? "").trim(); + if (!login) return null; + return { login, avatarUrl: row.merged_by_avatar_url ?? null }; +} + +/** + * Rehydrate the lane provenance frozen at detach time. Returns null for live rows. + * Malformed provenance still yields a usable lane record with zeroed counts. + */ +export function rowDetachedLane(row: PullRequestRowMetadata): PrDetachedLane | null { + const at = String(row.detached_at ?? "").trim(); + if (!at) return null; + let counts: { chats?: unknown; artifacts?: unknown; checkpoints?: unknown } = {}; + try { + const parsed = JSON.parse(String(row.detached_provenance ?? "{}")); + if (parsed && typeof parsed === "object") counts = parsed as typeof counts; + } catch { + /* a corrupt blob must not hide the lane name */ + } + return { + at, + laneName: row.detached_lane_name ?? null, + laneColor: row.detached_lane_color ?? null, + chats: normalizeCount(counts.chats) ?? 0, + artifacts: normalizeCount(counts.artifacts) ?? 0, + checkpoints: normalizeCount(counts.checkpoints) ?? 0, + }; +} + +/** + * Resolve the lane columns of a GitHub list row. Detached rows carry frozen + * provenance instead of exposing a live lane mapping. + */ +export function deriveGithubSnapshotLaneLink( + linked: PullRequestRowMetadata | null, + laneById: ReadonlyMap | undefined, +): Pick { + if (!linked) return { linkedLaneId: null, linkedLaneName: null, detached: null }; + const detached = rowDetachedLane(linked); + if (detached) return { linkedLaneId: null, linkedLaneName: null, detached }; + return { + linkedLaneId: linked.lane_id, + linkedLaneName: laneById?.get(linked.lane_id)?.name ?? null, + detached: null, + }; +} + +/** Build the merge outcome and size fields shown by a GitHub list row. */ +export function deriveGithubSnapshotMergeFacts( + linked: PullRequestRowMetadata | null, +): Pick< + GitHubPrListItem, + "mergedAt" | "mergedBy" | "mergeMethod" | "additions" | "deletions" | "commitCount" | "changedFiles" +> { + return { + mergedAt: linked?.merged_at ?? null, + mergedBy: linked ? rowMergedBy(linked) : null, + mergeMethod: normalizeMergeMethod(linked?.merge_method), + additions: normalizeCount(linked?.additions), + deletions: normalizeCount(linked?.deletions), + commitCount: normalizeCount(linked?.commit_count), + changedFiles: normalizeCount(linked?.changed_files), + }; +} diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 89ddba8ff..7c2ace028 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -56,8 +56,6 @@ import type { PrCommit, PrConflictAnalysis, PrCreationStrategy, - PrDetachedLane, - PrMergedBy, PrEventPayload, PrGroupMemberRole, PrHealth, @@ -161,6 +159,14 @@ import { spawn } from "node:child_process"; import { runGit, runGitMergeTree, runGitOrThrow } from "../git/git"; import { shouldAttemptAdminMergeForRestError } from "./resolverUtils"; import { deletePullRequestRowsByIds } from "./pullRequestRowCleanup"; +import { + deriveGithubSnapshotLaneLink, + deriveGithubSnapshotMergeFacts, + normalizeCount, + normalizeMergeMethod, + rowDetachedLane, + rowMergedBy, +} from "./prRowMetadata"; import { createGithubStackStore } from "./githubStackStore"; import { extractFirstJsonObject } from "../ai/utils"; import { buildIntegrationPreflight } from "./integrationPlanning"; @@ -1068,87 +1074,6 @@ function rowToSummary(row: PullRequestRow): PrSummary { }; } -function normalizeCount(value: unknown): number | null { - if (value == null) return null; - const parsed = Number(value); - return Number.isFinite(parsed) && parsed >= 0 ? parsed : null; -} - -function normalizeMergeMethod(value: unknown): MergeMethod | null { - return value === "squash" || value === "merge" || value === "rebase" ? value : null; -} - -/** - * Resolve the lane columns of a list row. - * - * A detached row reports NO live lane (`linkedLaneId`/`linkedLaneName` stay null) and - * carries `detached` instead, so the renderer shows it as history rather than as a - * mapping it could act on. A missing lane row yields a null name, never the raw lane - * UUID — a bare id in the list is worse than no chip at all. - */ -function deriveGithubSnapshotLaneLink( - linked: PullRequestRow | null, - laneById: Map | undefined, -): Pick { - if (!linked) return { linkedLaneId: null, linkedLaneName: null, detached: null }; - const detached = rowDetachedLane(linked); - if (detached) return { linkedLaneId: null, linkedLaneName: null, detached }; - return { - linkedLaneId: linked.lane_id, - linkedLaneName: laneById?.get(linked.lane_id)?.name ?? null, - detached: null, - }; -} - -/** Merge outcome + size, so a merged row can describe itself without a GitHub call. */ -function deriveGithubSnapshotMergeFacts( - linked: PullRequestRow | null, -): Pick< - GitHubPrListItem, - "mergedAt" | "mergedBy" | "mergeMethod" | "additions" | "deletions" | "commitCount" | "changedFiles" -> { - return { - mergedAt: linked?.merged_at ?? null, - mergedBy: linked ? rowMergedBy(linked) : null, - mergeMethod: normalizeMergeMethod(linked?.merge_method), - additions: normalizeCount(linked?.additions), - deletions: normalizeCount(linked?.deletions), - commitCount: normalizeCount(linked?.commit_count), - changedFiles: normalizeCount(linked?.changed_files), - }; -} - -function rowMergedBy(row: PullRequestRow): PrMergedBy | null { - const login = String(row.merged_by_login ?? "").trim(); - if (!login) return null; - return { login, avatarUrl: row.merged_by_avatar_url ?? null }; -} - -/** - * Rehydrate the lane provenance frozen at detach time. Returns null for live rows. - * A malformed or missing provenance blob still yields a usable record — the lane name - * is the part the UI leads with, and zeroed counts are simply not rendered. - */ -function rowDetachedLane(row: PullRequestRow): PrDetachedLane | null { - const at = String(row.detached_at ?? "").trim(); - if (!at) return null; - let counts: { chats?: unknown; artifacts?: unknown; checkpoints?: unknown } = {}; - try { - const parsed = JSON.parse(String(row.detached_provenance ?? "{}")); - if (parsed && typeof parsed === "object") counts = parsed as typeof counts; - } catch { - /* a corrupt blob must not hide the lane name */ - } - return { - at, - laneName: row.detached_lane_name ?? null, - laneColor: row.detached_lane_color ?? null, - chats: normalizeCount(counts.chats) ?? 0, - artifacts: normalizeCount(counts.artifacts) ?? 0, - checkpoints: normalizeCount(counts.checkpoints) ?? 0, - }; -} - const BACKGROUND_REFRESH_MAX_PRS = 4; const REFRESH_CONCURRENCY = 4; const TARGETED_LANE_PR_BRANCH_LOOKUP_CONCURRENCY = 4; diff --git a/apps/desktop/src/main/services/prs/pullRequestRowCleanup.test.ts b/apps/desktop/src/main/services/prs/pullRequestRowCleanup.test.ts index fa3e519c9..b726bbc28 100644 --- a/apps/desktop/src/main/services/prs/pullRequestRowCleanup.test.ts +++ b/apps/desktop/src/main/services/prs/pullRequestRowCleanup.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { openKvDb } from "../state/kvDb"; import { countLaneProvenance, + deletePullRequestRowsByIds, detachPullRequestRowsByIds, detachPullRequestRowsForLane, } from "./pullRequestRowCleanup"; @@ -14,14 +15,21 @@ function createLogger() { } const PROJECT_ID = "proj-1"; +const OTHER_PROJECT_ID = "proj-2"; const LANE_ID = "lane-1"; const DETACHED_AT = "2026-07-31T12:00:00.000Z"; +type PrOverrides = { + projectId?: string; + laneId?: string; + state?: string; +}; + describe("pullRequestRowCleanup detach", () => { let dir: string; let db: Awaited>; - const insertPr = (id: string, overrides: Record = {}) => { + const insertPr = (id: string, overrides: PrOverrides = {}) => { db.run( `insert into pull_requests (id, project_id, lane_id, repo_owner, repo_name, github_pr_number, github_url, @@ -30,9 +38,9 @@ describe("pullRequestRowCleanup detach", () => { ?, 'main', 'ade/feature', 412, 88, ?, ?)`, [ id, - PROJECT_ID, - (overrides.lane_id as string) ?? LANE_ID, - (overrides.state as string) ?? "merged", + overrides.projectId ?? PROJECT_ID, + overrides.laneId ?? LANE_ID, + overrides.state ?? "merged", DETACHED_AT, DETACHED_AT, ], @@ -175,6 +183,80 @@ describe("pullRequestRowCleanup detach", () => { ).toBeNull(); }); + it("does not purge snapshots or group membership for ids owned by another project", () => { + insertPr("pr-local"); + insertPr("pr-foreign", { projectId: OTHER_PROJECT_ID, laneId: "lane-foreign" }); + insertSnapshot("pr-local"); + insertSnapshot("pr-foreign"); + db.run( + "insert into pr_groups(id, project_id, group_type, created_at) values (?, ?, 'integration', ?)", + ["group-local", PROJECT_ID, DETACHED_AT], + ); + db.run( + "insert into pr_groups(id, project_id, group_type, created_at) values (?, ?, 'integration', ?)", + ["group-foreign", OTHER_PROJECT_ID, DETACHED_AT], + ); + db.run( + `insert into pr_group_members(id, group_id, pr_id, lane_id, position, role) + values ('member-local', 'group-local', 'pr-local', ?, 0, 'source')`, + [LANE_ID], + ); + db.run( + `insert into pr_group_members(id, group_id, pr_id, lane_id, position, role) + values ('member-foreign', 'group-foreign', 'pr-foreign', 'lane-foreign', 0, 'source')`, + ); + + detachPullRequestRowsByIds(db, { + projectId: PROJECT_ID, + laneId: LANE_ID, + laneName: "local-lane", + laneColor: null, + detachedAt: DETACHED_AT, + prIds: ["pr-local", "pr-foreign"], + }); + + expect( + db.get<{ checks_json: string | null }>("select checks_json from pull_request_snapshots where pr_id = ?", [ + "pr-local", + ])?.checks_json, + ).toBeNull(); + expect( + db.get<{ checks_json: string | null }>("select checks_json from pull_request_snapshots where pr_id = ?", [ + "pr-foreign", + ])?.checks_json, + ).not.toBeNull(); + expect(db.get("select id from pr_group_members where id = 'member-local'")).toBeNull(); + expect(db.get("select id from pr_group_members where id = 'member-foreign'")).not.toBeNull(); + expect( + db.get<{ detached_at: string | null }>("select detached_at from pull_requests where id = 'pr-foreign'") + ?.detached_at, + ).toBeNull(); + }); + + it("keeps another project's child rows when hard-deleting a mixed id list", () => { + insertPr("pr-local"); + insertPr("pr-foreign", { projectId: OTHER_PROJECT_ID, laneId: "lane-foreign" }); + insertSnapshot("pr-local"); + insertSnapshot("pr-foreign"); + db.run( + "insert into pull_request_ai_summaries(pr_id, head_sha, summary_json, generated_at) values (?, 'head', '{}', ?)", + ["pr-local", DETACHED_AT], + ); + db.run( + "insert into pull_request_ai_summaries(pr_id, head_sha, summary_json, generated_at) values (?, 'head', '{}', ?)", + ["pr-foreign", DETACHED_AT], + ); + + deletePullRequestRowsByIds(db, PROJECT_ID, ["pr-local", "pr-foreign"]); + + expect(db.get("select id from pull_requests where id = 'pr-local'")).toBeNull(); + expect(db.get("select pr_id from pull_request_snapshots where pr_id = 'pr-local'")).toBeNull(); + expect(db.get("select pr_id from pull_request_ai_summaries where pr_id = 'pr-local'")).toBeNull(); + expect(db.get("select id from pull_requests where id = 'pr-foreign'")).not.toBeNull(); + expect(db.get("select pr_id from pull_request_snapshots where pr_id = 'pr-foreign'")).not.toBeNull(); + expect(db.get("select pr_id from pull_request_ai_summaries where pr_id = 'pr-foreign'")).not.toBeNull(); + }); + it("counts zero provenance for a lane with no recorded activity", () => { expect(countLaneProvenance(db, PROJECT_ID, LANE_ID)).toEqual({ chats: 0, diff --git a/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts b/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts index 3f4f1de72..4f3634cce 100644 --- a/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts +++ b/apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts @@ -73,6 +73,19 @@ function uniqueIds(ids: string[]): string[] { return [...new Set(ids.map((id) => id.trim()).filter(Boolean))]; } +function projectPrScope(projectId: string, prIds: string[]): { + params: unknown[]; + selectSql: string; +} | null { + const ids = uniqueIds(prIds); + if (ids.length === 0) return null; + const placeholders = ids.map(() => "?").join(", "); + return { + params: [projectId, ...ids], + selectSql: `select id from pull_requests where project_id = ? and id in (${placeholders})`, + }; +} + function pruneEmptyPrGroups(db: DbLike, projectId: string): void { db.run( ` @@ -92,14 +105,25 @@ function pruneEmptyPrGroups(db: DbLike, projectId: string): void { } export function deletePullRequestRowsByIds(db: DbLike, projectId: string, prIds: string[]): void { - const ids = uniqueIds(prIds); - if (ids.length === 0) return; - const placeholders = ids.map(() => "?").join(", "); + const scope = projectPrScope(projectId, prIds); + if (!scope) return; - db.run(`delete from pr_group_members where pr_id in (${placeholders})`, ids); - db.run(`delete from pull_request_ai_summaries where pr_id in (${placeholders})`, ids); - db.run(`delete from pull_request_snapshots where pr_id in (${placeholders})`, ids); - db.run(`delete from pull_requests where project_id = ? and id in (${placeholders})`, [projectId, ...ids]); + db.run( + `delete from pr_group_members + where pr_id in (${scope.selectSql})`, + scope.params, + ); + db.run( + `delete from pull_request_ai_summaries + where pr_id in (${scope.selectSql})`, + scope.params, + ); + db.run( + `delete from pull_request_snapshots + where pr_id in (${scope.selectSql})`, + scope.params, + ); + db.run(`delete from pull_requests where id in (${scope.selectSql})`, scope.params); pruneEmptyPrGroups(db, projectId); } @@ -133,9 +157,8 @@ function detachRows( }, ): void { const { projectId, prIds, laneName, laneColor, detachedAt, provenance } = args; - if (prIds.length === 0) return; - const placeholders = prIds.map(() => "?").join(", "); - const scope = [projectId, ...prIds]; + const scope = projectPrScope(projectId, prIds); + if (!scope) return; // Lift counts off the snapshot before nulling it, so the merged row can still say // "12 commits · 9 files" once the JSON is gone. @@ -154,9 +177,9 @@ function detachRows( from pull_request_snapshots s where s.pr_id = pull_requests.id and json_valid(s.files_json)) ) - where project_id = ? and id in (${placeholders}) + where id in (${scope.selectSql}) `, - scope, + scope.params, ); // `detached_at is null` keeps the first detach authoritative: re-detaching an already @@ -168,9 +191,9 @@ function detachRows( detached_lane_name = ?, detached_lane_color = ?, detached_provenance = ? - where project_id = ? and id in (${placeholders}) and detached_at is null + where id in (${scope.selectSql}) and detached_at is null `, - [detachedAt, laneName, laneColor, JSON.stringify(provenance), ...scope], + [detachedAt, laneName, laneColor, JSON.stringify(provenance), ...scope.params], ); // Keep detail/status/commits (small, and what the merged view reads); drop the bulky @@ -182,13 +205,17 @@ function detachRows( checks_json = null, comments_json = null, reviews_json = null - where pr_id in (${placeholders}) + where pr_id in (${scope.selectSql}) `, - prIds, + scope.params, ); // Group membership is lane-scoped work-in-progress, not history — it goes. - db.run(`delete from pr_group_members where pr_id in (${placeholders})`, prIds); + db.run( + `delete from pr_group_members + where pr_id in (${scope.selectSql})`, + scope.params, + ); pruneEmptyPrGroups(db, projectId); } @@ -210,7 +237,12 @@ export function detachPullRequestRowsForLane( }); // Lane-scoped membership survives the id lookup above (a group member can outlive its // PR row), so clear it explicitly and prune once. - db.run("delete from pr_group_members where lane_id = ?", [laneId]); + db.run( + `delete from pr_group_members + where lane_id = ? + and group_id in (select id from pr_groups where project_id = ?)`, + [laneId, projectId], + ); pruneEmptyPrGroups(db, projectId); } diff --git a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx index ba5619517..9f83e7194 100644 --- a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx @@ -50,16 +50,26 @@ function stateBadgeStyle(item: GitHubPrListItem): React.CSSProperties { }; } -/* -- CI status dot color -- */ -function ciDotColor(linkedPr: PrSummary | null): { color: string; title: string } | null { - if (!linkedPr) return null; - switch (linkedPr.checksStatus) { +function PrRowCiStatus({ status }: { status: PrSummary["checksStatus"] | null }) { + switch (status) { case "passing": - return { color: COLORS.success, title: "CI passing" }; + return ( + + + + ); case "failing": - return { color: COLORS.danger, title: "CI failing" }; + return ( + + + + ); case "pending": - return { color: COLORS.warning, title: "CI pending" }; + return ( + + + + ); default: return null; } @@ -157,7 +167,7 @@ function PrRowLaneChip({ linkedLaneColor: string | null; mappable: boolean; }) { - if (item.linkedLaneName || item.linkedLaneId) { + if (item.linkedLaneName) { return ( {linkedLaneColor ? : null} - {item.linkedLaneName ?? item.linkedLaneId} + {item.linkedLaneName} ); } if (item.detached) return ; + // A linked id is internal identity, not user-facing copy. If the lane metadata is + // temporarily unavailable, omit the chip instead of leaking a UUID or claiming the + // PR is unmapped. + if (item.linkedLaneId) return null; + // Terminal PRs have no mapping story worth telling — the lane is gone and mapping one // now would do nothing. Showing a badge here is what made Merged a wall of warnings. if (isTerminalPrState(item.state)) return null; @@ -284,8 +299,6 @@ export function GitHubTabPrRow({ // state badge are all answered by the fact that it merged. Dropping them is what lets // the row collapse to two lines and stops the list reading as a wall of signals. const terminal = isTerminalPrState(item.state); - const ci = terminal ? null : ciDotColor(linkedPr); - const ciRunning = linkedPr?.checksStatus === "pending"; const review = terminal ? null : reviewIndicator(linkedPr); // Open rows are about how long something has been waiting; merged rows are about // when it shipped. @@ -296,29 +309,30 @@ export function GitHubTabPrRow({ const rowLinkedLaneColor = useLaneColorById(item.linkedLaneId ?? null); const mappable = isPrRowMappable(item); return ( - + + + ); } diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx index 3b8a08d50..d48db9dee 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx @@ -2,319 +2,51 @@ import React from "react"; import { MemoryRouter } from "react-router-dom"; -import { act, cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; +import { act, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import type { CreateLaneFromPrBranchPreflightResult, GitHubPrSnapshot, LaneSummary, MergeMethod, PrWithConflicts } from "../../../../shared/types"; - -vi.mock("react-resizable-panels", () => ({ - Group: ({ children }: { children: React.ReactNode }) =>
{children}
, - Panel: ({ children, ...props }: React.HTMLAttributes & { id?: string }) => ( -
- {children} -
- ), - Separator: (props: React.HTMLAttributes) =>
, -})); - -const mockUsePrs = vi.fn(); - -vi.mock("../state/PrsContext", () => ({ - usePrs: () => mockUsePrs(), -})); - -type MockUnmappedAffordance = { - linkableLanes: Array<{ id: string; name: string }>; - selectedLaneId: string; - onSelectLane: (laneId: string) => void; - onLink: () => void; - linkBusy: boolean; - canCreateLane: boolean; - onCreateLane: () => void; - scope: "repo" | "external"; -}; - -vi.mock("../detail/PrDetailPane", () => ({ - PrDetailPane: ({ - pr, - onUnmap, - unmapped, - unmappedAffordance, - }: { - pr: { id: string }; - onUnmap?: () => void; - unmapped?: boolean; - unmappedAffordance?: MockUnmappedAffordance | null; - }) => ( -
- {pr.id} - {onUnmap ? : null} - {unmappedAffordance ? ( -
- {unmappedAffordance.canCreateLane ? ( - - ) : null} - {unmappedAffordance.linkableLanes.length > 0 ? ( - <> - - - - ) : null} -
- ) : null} -
- ), -})); +import type { GitHubPrSnapshot, LaneSummary, MergeMethod } from "../../../../shared/types"; -import { GitHubTab } from "./GitHubTab"; - -function makeGitHubPr(overrides: Partial = {}): GitHubPrSnapshot["repoPullRequests"][number] { +vi.mock("react-resizable-panels", async () => { + const harness = await import("./GitHubTab.testHarness"); return { - id: "repo-open", - scope: "repo", - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 101, - githubUrl: "https://github.com/ade-dev/ade/pull/101", - title: "Open PR", - state: "open", - isDraft: false, - baseBranch: "main", - headBranch: "feature/open", - author: "octocat", - createdAt: "2026-03-13T11:00:00.000Z", - updatedAt: "2026-03-13T11:30:00.000Z", - linkedPrId: "pr-open", - linkedGroupId: null, - linkedLaneId: "lane-open", - linkedLaneName: "lane-open", - adeKind: "single", - workflowDisplayState: null, - cleanupState: null, - labels: [], - isBot: false, - commentCount: 0, - ...overrides, + Group: harness.MockPanelGroup, + Panel: harness.MockPanel, + Separator: harness.MockSeparator, }; -} +}); -function makeLaneSummary(overrides: Partial = {}): LaneSummary { - return { - id: "lane-open", - name: "lane-open", - description: null, - laneType: "worktree", - baseRef: "main", - branchRef: "refs/heads/feature/open", - worktreePath: "/tmp/lane-open", - attachedRootPath: null, - parentLaneId: null, - childCount: 0, - stackDepth: 0, - parentStatus: null, - isEditProtected: false, - status: { dirty: false, ahead: 0, behind: 0, remoteBehind: -1, rebaseInProgress: false }, - color: null, - icon: null, - tags: [], - folder: null, - createdAt: "2026-03-13T10:00:00.000Z", - archivedAt: null, - ...overrides, - }; -} - -const snapshot: GitHubPrSnapshot = { - repo: { owner: "ade-dev", name: "ade" }, - viewerLogin: "octocat", - syncedAt: "2026-03-13T12:00:00.000Z", - repoPullRequests: [ - makeGitHubPr(), - makeGitHubPr({ - id: "repo-merged", - githubPrNumber: 102, - githubUrl: "https://github.com/ade-dev/ade/pull/102", - title: "Merged PR", - state: "merged", - headBranch: "feature/merged", - createdAt: "2026-03-13T09:00:00.000Z", - updatedAt: "2026-03-13T10:00:00.000Z", - linkedPrId: "pr-merged", - linkedLaneId: "lane-merged", - linkedLaneName: "lane-merged", - }), - ], - externalPullRequests: [], -}; - -type Deferred = { - promise: Promise; - resolve: (value: T) => void; - reject: (error: unknown) => void; -}; - -function createDeferred(): Deferred { - let resolve!: (value: T) => void; - let reject!: (error: unknown) => void; - const promise = new Promise((res, rej) => { - resolve = res; - reject = rej; - }); - return { promise, resolve, reject }; -} - -function makePreflightResult(args: { - githubPrNumber: number; - title: string; - headBranch: string; - remoteBranch: string; -}): CreateLaneFromPrBranchPreflightResult { - return { - preflight: { - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: args.githubPrNumber, - githubUrl: `https://github.com/ade-dev/ade/pull/${args.githubPrNumber}`, - title: args.title, - headBranch: args.headBranch, - headRepoOwner: "ade-dev", - headRepoName: "ade", - headSha: "head-sha", - remoteBranch: args.remoteBranch, - importBranchRef: args.remoteBranch, - targetLaneName: args.title, - baseBranch: "main", - canCreate: true, - status: "ready", - blockingConflict: null, - blockingConflicts: [], - }, - lane: null, - pr: null, - }; -} +vi.mock("../state/PrsContext", async () => { + const { mockUsePrs } = await import("./GitHubTab.testHarness"); + return { usePrs: () => mockUsePrs() }; +}); -describe("GitHubTab", () => { - beforeEach(() => { - mockUsePrs.mockReturnValue({ - prs: [ - { id: "pr-open", state: "open", checksStatus: "pending", reviewStatus: "requested", additions: 12, deletions: 3 }, - { id: "pr-merged", state: "merged", checksStatus: "passing", reviewStatus: "approved", additions: 5, deletions: 1 }, - ] satisfies Partial[], - mergeContextByPrId: {}, - detailStatus: null, - detailChecks: [], - detailReviews: [], - detailComments: [], - detailBusy: false, - loading: false, - setViewerLogin: vi.fn(), - }); +vi.mock("../detail/PrDetailPane", async () => { + const { MockPrDetailPane } = await import("./GitHubTab.testHarness"); + return { PrDetailPane: MockPrDetailPane }; +}); - Object.assign(window, { - ade: { - prs: { - getGitHubSnapshot: vi.fn().mockResolvedValue(snapshot), - onEvent: vi.fn(() => () => {}), - linkToLane: vi.fn(), - preflightCreateLaneFromPrBranch: vi.fn().mockResolvedValue({ - preflight: { - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "Unlinked PR", - headBranch: "feature/open", - headRepoOwner: "ade-dev", - headRepoName: "ade", - remoteBranch: "origin/feature/open", - importBranchRef: "origin/feature/open", - targetLaneName: "Unlinked PR", - baseBranch: "main", - canCreate: true, - status: "ready", - blockingConflict: null, - blockingConflicts: [], - }, - lane: null, - pr: null, - }), - createLaneFromPrBranch: vi.fn().mockResolvedValue({ - preflight: { - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "Unlinked PR", - headBranch: "feature/open", - headRepoOwner: "ade-dev", - headRepoName: "ade", - remoteBranch: "origin/feature/open", - importBranchRef: "origin/feature/open", - targetLaneName: "Unlinked PR", - baseBranch: "main", - canCreate: true, - status: "ready", - blockingConflict: null, - blockingConflicts: [], - }, - lane: { id: "lane-created", name: "Unlinked PR" }, - pr: { id: "pr-created", laneId: "lane-created" }, - }), - addGitHubStackPullRequests: vi.fn().mockResolvedValue(null), - unstackGitHubStack: vi.fn().mockResolvedValue(null), - delete: vi.fn().mockResolvedValue(undefined), - }, - github: { - getStatus: vi.fn().mockResolvedValue({ - tokenStored: true, - patTokenStored: true, - tokenDecryptionFailed: false, - storageScope: "app", - authSource: "pat", - tokenType: "classic", - repo: { owner: "ade-dev", name: "ade" }, - hasOrigin: true, - userLogin: "octocat", - scopes: [], - ghCliPath: null, - ghAuthError: null, - checkedAt: "2026-03-13T12:00:00.000Z", - repoAccessOk: null, - repoAccessError: null, - connected: true, - }), - }, - app: { - openExternal: vi.fn(), - }, - lanes: { - list: vi.fn().mockResolvedValue([]), - }, - }, - }); +import { GitHubTab } from "./GitHubTab"; +import { + cleanupGitHubTabTest, + mockUsePrs, + renderGitHubTab, + setupGitHubTabTest, +} from "./GitHubTab.testHarness"; +import { + createDeferred, + makeGitHubPr, + makePrsContext, + snapshot, +} from "./GitHubTab.testFixtures"; + +describe("GitHubTab snapshot lifecycle", () => { + beforeEach(() => { + setupGitHubTabTest(); }); afterEach(() => { - cleanup(); - vi.useRealTimers(); + cleanupGitHubTabTest(); }); function renderTab(overrides: Partial<{ @@ -323,20 +55,21 @@ describe("GitHubTab", () => { onRefreshAll: ReturnType; lanes: LaneSummary[]; }> = {}) { - const onSelectPr = overrides.onSelectPr ?? vi.fn(); - const onRefreshAll = overrides.onRefreshAll ?? vi.fn().mockResolvedValue(undefined); - render( + return renderGitHubTab(GitHubTab, overrides); + } + + function renderTabEl(selectedPrId: string) { + return ( - , + ); - return { onSelectPr, onRefreshAll }; } it("shows and manages the selected GitHub stack from the cached snapshot", async () => { @@ -433,41 +166,13 @@ describe("GitHubTab", () => { expect(screen.queryByTestId("pr-detail-pane")).toBeNull(); }); - function prsContext(prs: Array & { id: string }>) { - return { - prs, - mergeContextByPrId: {}, - detailStatus: null, - detailChecks: [], - detailReviews: [], - detailComments: [], - detailBusy: false, - loading: false, - setViewerLogin: vi.fn(), - }; - } - - function renderTabEl(selectedPrId: string) { - return ( - - - - ); - } - it("follows a selected PR into the merged bucket when its linked state transitions", async () => { (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue({ ...snapshot, repoPullRequests: [makeGitHubPr()], externalPullRequests: [], }); - mockUsePrs.mockReturnValue(prsContext([ + mockUsePrs.mockReturnValue(makePrsContext([ { id: "pr-open", state: "open", repoOwner: "ade-dev", repoName: "ade", githubPrNumber: 101 }, ])); @@ -480,7 +185,7 @@ describe("GitHubTab", () => { expect((screen.getByRole("button", { name: /^open/i }) as HTMLButtonElement).style.fontWeight).toBe("600"); // The linked ADE PR transitions open -> merged. - mockUsePrs.mockReturnValue(prsContext([ + mockUsePrs.mockReturnValue(makePrsContext([ { id: "pr-open", state: "merged", repoOwner: "ade-dev", repoName: "ade", githubPrNumber: 101 }, ])); view.rerender(renderTabEl("pr-open")); @@ -503,7 +208,7 @@ describe("GitHubTab", () => { return () => {}; }); - mockUsePrs.mockReturnValue(prsContext([ + mockUsePrs.mockReturnValue(makePrsContext([ { id: "pr-open", state: "open", repoOwner: "ade-dev", repoName: "ade", githubPrNumber: 101 }, ])); @@ -515,7 +220,7 @@ describe("GitHubTab", () => { expect(screen.getByRole("button", { name: /#101 Open PR/i })).toBeTruthy(); // The PR merges AND drops out of the open-only snapshot. - mockUsePrs.mockReturnValue(prsContext([ + mockUsePrs.mockReturnValue(makePrsContext([ { id: "pr-open", state: "merged", repoOwner: "ade-dev", repoName: "ade", githubPrNumber: 101 }, ])); getSnap.mockResolvedValue({ ...openOnly, repoPullRequests: [] }); @@ -680,27 +385,6 @@ describe("GitHubTab", () => { expect(screen.getByRole("button", { name: /connect github/i })).toBeTruthy(); }); - it("requires confirmation before unmapping a GitHub PR from its lane", async () => { - const user = userEvent.setup(); - const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(false); - try { - renderTab(); - - await waitFor(() => { - expect(screen.getByText("Open PR")).toBeTruthy(); - }); - await user.click(screen.getByRole("button", { name: /#101 Open PR/i })); - await waitFor(() => { - expect(screen.getByTestId("pr-detail-pane").textContent).toContain("pr-open"); - }); - await user.click(screen.getByRole("button", { name: /unmap from lane/i })); - - expect(confirmSpy).toHaveBeenCalledWith(expect.stringContaining("Unmap PR #101")); - expect(window.ade.prs.delete).not.toHaveBeenCalled(); - } finally { - confirmSpy.mockRestore(); - } - }); it("renders a cached ADE detail shell while a linked PR hydrates", async () => { mockUsePrs.mockReturnValue({ @@ -729,13 +413,6 @@ describe("GitHubTab", () => { }); }); - it("shows a running CI indicator for PR cards with pending checks", async () => { - renderTab(); - - await waitFor(() => { - expect(screen.getAllByLabelText("CI running").length).toBeGreaterThan(0); - }); - }); it("does not force-refresh the GitHub snapshot when linked PRs hydrate after mount", async () => { const emptyContext = { @@ -1156,629 +833,4 @@ describe("GitHubTab", () => { expect(screen.getByText("Stale open snapshot")).toBeTruthy(); }); }); - - it("shows linked and unmapped PRs together under the status tabs", async () => { - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - ...snapshot.repoPullRequests, - makeGitHubPr({ - id: "repo-unlinked", - githubPrNumber: 200, - title: "Unlinked PR", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); - renderTab(); - - await waitFor(() => { - expect(screen.getByText("Open PR")).not.toBeNull(); - expect(screen.getByText("Unlinked PR")).not.toBeNull(); - }); - }); - - it("marks unlinked PRs as unmapped", async () => { - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - ...snapshot.repoPullRequests, - makeGitHubPr({ - id: "repo-unlinked", - githubPrNumber: 200, - title: "Unlinked PR", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); - renderTab(); - - await waitFor(() => { - expect(screen.getByText("Unlinked PR")).not.toBeNull(); - }); - expect(screen.getAllByText("unmapped").length).toBeGreaterThan(0); - }); - - it("does not mark unlinked PRs as unmapped in the merged bucket", async () => { - const user = userEvent.setup(); - const snapshotWithMergedUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - ...snapshot.repoPullRequests, - makeGitHubPr({ - id: "repo-merged-unlinked", - githubPrNumber: 201, - title: "Merged after lane deleted", - state: "merged", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-12T12:00:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithMergedUnlinked); - renderTab(); - - await user.click(await screen.findByRole("button", { name: /merged/i })); - await waitFor(() => { - expect(screen.getByText("Merged after lane deleted")).not.toBeNull(); - }); - // Mapping is a live-work concept: on a merged PR the lane is gone and mapping one - // would do nothing, so the badge must not appear. - expect(screen.queryByText("unmapped")).toBeNull(); - }); - - it("shows frozen lane provenance on a detached merged PR", async () => { - const user = userEvent.setup(); - const snapshotWithDetached: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - ...snapshot.repoPullRequests, - makeGitHubPr({ - id: "repo-detached", - githubPrNumber: 202, - title: "Shipped from a deleted lane", - state: "merged", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-11T12:00:00.000Z", - detached: { - at: "2026-03-12T09:00:00.000Z", - laneName: "auto-naming", - laneColor: "#4ADE80", - chats: 3, - artifacts: 2, - checkpoints: 5, - }, - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithDetached); - renderTab(); - - await user.click(await screen.findByRole("button", { name: /merged/i })); - await waitFor(() => { - expect(screen.getByText("Shipped from a deleted lane")).not.toBeNull(); - }); - expect(screen.getByText("was: auto-naming")).not.toBeNull(); - expect(screen.getByText("· 3 chats · 2 proof")).not.toBeNull(); - expect(screen.queryByText("unmapped")).toBeNull(); - }); - - it("shows merge facts instead of CI and review signals on a merged row", async () => { - const user = userEvent.setup(); - const snapshotWithMerged: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - ...snapshot.repoPullRequests, - makeGitHubPr({ - id: "repo-merged-facts", - githubPrNumber: 203, - title: "Merged with facts", - state: "merged", - baseBranch: "main", - mergedAt: "2026-03-12T10:00:00.000Z", - mergedBy: { login: "arul", avatarUrl: null }, - mergeMethod: "squash", - createdAt: "2026-03-10T12:00:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithMerged); - renderTab(); - - await user.click(await screen.findByRole("button", { name: /merged/i })); - await waitFor(() => { - expect(screen.getByText("Merged with facts")).not.toBeNull(); - }); - expect(screen.getByText("arul · squash · → main")).not.toBeNull(); - }); - - it("renders the full PR detail pane (with create/map affordance) for a selected unmapped PR", async () => { - const user = userEvent.setup(); - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "repo-unlinked", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "Unlinked PR", - headBranch: "feature/no-lane", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:05:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); - // No lane owns the PR head branch → the "Create lane from PR branch" action - // is offered (and there is no matching lane to map to). - renderTab({ lanes: [] }); - - await user.click(await screen.findByText("Unlinked PR")); - - // The full detail pane renders (not the legacy read-only gate), keyed by a - // stable synthetic id derived from the GitHub coordinates. - const pane = await screen.findByTestId("pr-detail-pane"); - expect(pane.getAttribute("data-unmapped")).toBe("true"); - expect(pane.textContent).toContain("gh:ade-dev/ade#200"); - - // The create/map affordance is present (no read-only gate). - const affordance = within(pane).getByTestId("pr-unmapped-affordance"); - expect(within(affordance).getByRole("button", { name: /create lane from pr branch/i })).toBeTruthy(); - }); - - it("maps an unmapped PR to a lane via the in-pane affordance", async () => { - const user = userEvent.setup(); - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "repo-unlinked", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "Unlinked PR", - headBranch: "feature/lane-match", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:05:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); - renderTab({ - lanes: [makeLaneSummary({ id: "lane-match", name: "Matching lane", branchRef: "refs/heads/feature/lane-match" })], - }); - - await user.click(await screen.findByText("Unlinked PR")); - const pane = await screen.findByTestId("pr-detail-pane"); - const affordance = within(pane).getByTestId("pr-unmapped-affordance"); - - await user.selectOptions(within(affordance).getByLabelText("Select lane to map"), "lane-match"); - await user.click(within(affordance).getByRole("button", { name: /^map$/i })); - - await waitFor(() => { - expect(window.ade.prs.linkToLane).toHaveBeenCalledWith({ - laneId: "lane-match", - prUrlOrNumber: "https://github.com/ade-dev/ade/pull/200", - }); - }); - }); - - it("opens a preflight dialog for an unmapped PR branch", async () => { - const user = userEvent.setup(); - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "repo-unlinked", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "Unlinked PR", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:05:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); - renderTab(); - - await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); - - expect(window.ade.prs.preflightCreateLaneFromPrBranch).toHaveBeenCalledWith({ - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 200, - }); - const dialog = await screen.findByRole("dialog", { name: /create lane from pr branch/i }); - expect(within(dialog).getByText(/#200 Unlinked PR/)).toBeTruthy(); - expect(within(dialog).getAllByText("origin/feature/open").length).toBeGreaterThan(0); - expect(within(dialog).getAllByText("Unlinked PR").length).toBeGreaterThan(0); - expect(within(dialog).getAllByText("main").length).toBeGreaterThan(0); - }); - - it("ignores stale create-lane preflight results from a previous PR", async () => { - const user = userEvent.setup(); - const firstPreflight = createDeferred(); - const secondPreflight = createDeferred(); - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "repo-unlinked-first", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "First PR", - headBranch: "feature/first", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:10:00.000Z", - }), - makeGitHubPr({ - id: "repo-unlinked-second", - githubPrNumber: 201, - githubUrl: "https://github.com/ade-dev/ade/pull/201", - title: "Second PR", - headBranch: "feature/second", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:05:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); - (window.ade.prs.preflightCreateLaneFromPrBranch as ReturnType) - .mockImplementation((args: { githubPrNumber: number }) => - args.githubPrNumber === 200 ? firstPreflight.promise : secondPreflight.promise); - renderTab(); - - await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); - expect(window.ade.prs.preflightCreateLaneFromPrBranch).toHaveBeenCalledWith({ - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 200, - }); - await user.click(screen.getByRole("button", { name: /cancel/i })); - await user.click(await screen.findByText("Second PR")); - await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); - expect(window.ade.prs.preflightCreateLaneFromPrBranch).toHaveBeenCalledWith({ - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 201, - }); - - await act(async () => { - firstPreflight.resolve(makePreflightResult({ - githubPrNumber: 200, - title: "First PR", - headBranch: "feature/first", - remoteBranch: "origin/feature/first", - })); - await firstPreflight.promise; - }); - expect(screen.queryByText(/#200 First PR/)).toBeNull(); - expect(screen.getByText(/checking branch ownership/i)).toBeTruthy(); - - await act(async () => { - secondPreflight.resolve(makePreflightResult({ - githubPrNumber: 201, - title: "Second PR", - headBranch: "feature/second", - remoteBranch: "origin/feature/second", - })); - await secondPreflight.promise; - }); - - expect(await screen.findByText(/#201 Second PR/)).toBeTruthy(); - expect(screen.queryByText(/#200 First PR/)).toBeNull(); - const secondDialog = await screen.findByRole("dialog", { name: /create lane from pr branch/i }); - expect(within(secondDialog).getAllByText("origin/feature/second").length).toBeGreaterThan(0); - }); - - it("shows blocking preflight conflicts before creating a lane", async () => { - const user = userEvent.setup(); - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "repo-unlinked", - githubPrNumber: 200, - title: "Unlinked PR", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:05:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); - (window.ade.prs.preflightCreateLaneFromPrBranch as ReturnType).mockResolvedValueOnce({ - preflight: { - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "Unlinked PR", - headBranch: "feature/open", - headRepoOwner: "ade-dev", - headRepoName: "ade", - remoteBranch: "origin/feature/open", - importBranchRef: "origin/feature/open", - targetLaneName: "Unlinked PR", - baseBranch: "main", - canCreate: false, - status: "blocked", - blockingConflict: { - code: "branch_owned", - message: "Branch 'feature/open' is already owned by lane 'Existing lane'.", - laneId: "lane-existing", - laneName: "Existing lane", - }, - blockingConflicts: [], - }, - lane: null, - pr: null, - }); - renderTab(); - - await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); - - expect(await screen.findByText(/already owned by lane 'Existing lane'/i)).toBeTruthy(); - expect(screen.getByRole("button", { name: /^create lane$/i })).toHaveProperty("disabled", true); - expect(window.ade.prs.createLaneFromPrBranch).not.toHaveBeenCalled(); - }); - - it("does not let an archived branch match hide the create-lane action", async () => { - const user = userEvent.setup(); - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "repo-unlinked", - githubPrNumber: 200, - title: "Unlinked PR", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:05:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); - (window.ade.prs.preflightCreateLaneFromPrBranch as ReturnType).mockResolvedValueOnce({ - preflight: { - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "Unlinked PR", - headBranch: "feature/open", - headRepoOwner: "ade-dev", - headRepoName: "ade", - remoteBranch: "origin/feature/open", - importBranchRef: "origin/feature/open", - targetLaneName: "Unlinked PR", - baseBranch: "main", - canCreate: false, - status: "blocked", - blockingConflict: { - code: "branch_owned", - message: "Branch 'feature/open' is already owned by archived lane 'Archived lane'.", - laneId: "lane-archived", - laneName: "Archived lane", - }, - blockingConflicts: [], - }, - lane: null, - pr: null, - }); - - renderTab({ - lanes: [ - makeLaneSummary({ - id: "lane-archived", - name: "Archived lane", - branchRef: "refs/heads/feature/open", - archivedAt: "2026-03-12T12:00:00.000Z", - }), - ], - }); - - expect(await screen.findByRole("button", { name: /create lane from pr branch/i })).toBeTruthy(); - expect(screen.queryByRole("option", { name: "Archived lane" })).toBeNull(); - - await user.click(screen.getByRole("button", { name: /create lane from pr branch/i })); - - expect(await screen.findByText(/already owned by archived lane 'Archived lane'/i)).toBeTruthy(); - expect(screen.getByRole("button", { name: /^create lane$/i })).toHaveProperty("disabled", true); - }); - - it("creates a lane from an unmapped PR branch, refreshes lanes, and selects the mapped PR", async () => { - const user = userEvent.setup(); - const onSelectPr = vi.fn(); - const onRefreshAll = vi.fn().mockResolvedValue(undefined); - const forcedSnapshot = createDeferred(); - const snapshotWithUnlinked: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "repo-unlinked", - githubPrNumber: 200, - githubUrl: "https://github.com/ade-dev/ade/pull/200", - title: "Unlinked PR", - linkedPrId: null, - linkedLaneId: null, - linkedLaneName: null, - adeKind: null, - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:05:00.000Z", - }), - ], - externalPullRequests: [], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType) - .mockResolvedValueOnce(snapshotWithUnlinked) - .mockReturnValueOnce(forcedSnapshot.promise); - renderTab({ onSelectPr, onRefreshAll }); - - await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); - await user.click(await screen.findByRole("button", { name: /^create lane$/i })); - - await waitFor(() => { - expect(window.ade.prs.createLaneFromPrBranch).toHaveBeenCalledWith({ - repoOwner: "ade-dev", - repoName: "ade", - githubPrNumber: 200, - }); - }); - await waitFor(() => { - expect(onSelectPr).toHaveBeenCalledWith("pr-created"); - }); - expect(onRefreshAll).toHaveBeenCalledWith({ prId: "pr-created" }); - expect(window.ade.lanes.list).toHaveBeenCalledWith({ - includeArchived: false, - includeStatus: false, - }); - expect(window.ade.prs.getGitHubSnapshot).toHaveBeenCalledWith({ force: true }); - await act(async () => { - forcedSnapshot.resolve(snapshotWithUnlinked); - await forcedSnapshot.promise; - }); - }); - - it("renders bot badge when isBot is true", async () => { - const snapshotWithBot: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "bot-pr", - githubPrNumber: 300, - title: "Bot PR", - author: "dependabot[bot]", - isBot: true, - createdAt: "2026-03-13T12:00:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithBot); - renderTab(); - - await waitFor(() => { - expect(screen.getByText("bot")).not.toBeNull(); - }); - }); - - it("renders labels when present", async () => { - const snapshotWithLabels: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "labeled-pr", - githubPrNumber: 400, - title: "Labeled PR", - labels: [ - { name: "bug", color: "d73a4a", description: null }, - { name: "enhancement", color: "a2eeef", description: null }, - ], - createdAt: "2026-03-13T12:00:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithLabels); - renderTab(); - - await waitFor(() => { - expect(screen.getByText("bug")).not.toBeNull(); - expect(screen.getByText("enhancement")).not.toBeNull(); - }); - }); - - it("renders comment count when greater than zero", async () => { - const snapshotWithComments: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "commented-pr", - githubPrNumber: 500, - title: "Commented PR", - commentCount: 42, - createdAt: "2026-03-13T12:00:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithComments); - renderTab(); - - await waitFor(() => { - expect(screen.getByText("42")).not.toBeNull(); - }); - }); - - it("sorts PRs by updatedAt descending", async () => { - const snapshotOrdered: GitHubPrSnapshot = { - ...snapshot, - repoPullRequests: [ - makeGitHubPr({ - id: "pr-old", - githubPrNumber: 50, - title: "Old PR", - createdAt: "2026-03-13T12:00:00.000Z", - updatedAt: "2026-03-13T12:10:00.000Z", - }), - makeGitHubPr({ - id: "pr-new", - githubPrNumber: 150, - title: "New PR", - createdAt: "2026-03-13T08:00:00.000Z", - updatedAt: "2026-03-13T12:30:00.000Z", - }), - ], - }; - (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotOrdered); - renderTab(); - - await waitFor(() => { - const buttons = screen.getAllByRole("button").filter((btn) => - btn.textContent?.includes("PR") && (btn.textContent?.includes("Old") || btn.textContent?.includes("New")), - ); - expect(buttons.length).toBe(2); - expect(buttons[0]!.textContent).toContain("New PR"); - expect(buttons[1]!.textContent).toContain("Old PR"); - }); - }); }); diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.testFixtures.ts b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.testFixtures.ts new file mode 100644 index 000000000..be4ac5fbf --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.testFixtures.ts @@ -0,0 +1,232 @@ +import { vi } from "vitest"; +import type { + CreateLaneFromPrBranchPreflightResult, + GitHubPrSnapshot, + LaneSummary, + PrWithConflicts, +} from "../../../../shared/types"; + +export function makeGitHubPr(overrides: Partial = {}): GitHubPrSnapshot["repoPullRequests"][number] { + return { + id: "repo-open", + scope: "repo", + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 101, + githubUrl: "https://github.com/ade-dev/ade/pull/101", + title: "Open PR", + state: "open", + isDraft: false, + baseBranch: "main", + headBranch: "feature/open", + author: "octocat", + createdAt: "2026-03-13T11:00:00.000Z", + updatedAt: "2026-03-13T11:30:00.000Z", + linkedPrId: "pr-open", + linkedGroupId: null, + linkedLaneId: "lane-open", + linkedLaneName: "lane-open", + adeKind: "single", + workflowDisplayState: null, + cleanupState: null, + labels: [], + isBot: false, + commentCount: 0, + ...overrides, + }; +} + +export function makeLaneSummary(overrides: Partial = {}): LaneSummary { + return { + id: "lane-open", + name: "lane-open", + description: null, + laneType: "worktree", + baseRef: "main", + branchRef: "refs/heads/feature/open", + worktreePath: "/tmp/lane-open", + attachedRootPath: null, + parentLaneId: null, + childCount: 0, + stackDepth: 0, + parentStatus: null, + isEditProtected: false, + status: { dirty: false, ahead: 0, behind: 0, remoteBehind: -1, rebaseInProgress: false }, + color: null, + icon: null, + tags: [], + folder: null, + createdAt: "2026-03-13T10:00:00.000Z", + archivedAt: null, + ...overrides, + }; +} + +export const snapshot: GitHubPrSnapshot = { + repo: { owner: "ade-dev", name: "ade" }, + viewerLogin: "octocat", + syncedAt: "2026-03-13T12:00:00.000Z", + repoPullRequests: [ + makeGitHubPr(), + makeGitHubPr({ + id: "repo-merged", + githubPrNumber: 102, + githubUrl: "https://github.com/ade-dev/ade/pull/102", + title: "Merged PR", + state: "merged", + headBranch: "feature/merged", + createdAt: "2026-03-13T09:00:00.000Z", + updatedAt: "2026-03-13T10:00:00.000Z", + linkedPrId: "pr-merged", + linkedLaneId: "lane-merged", + linkedLaneName: "lane-merged", + }), + ], + externalPullRequests: [], +}; + +type Deferred = { + promise: Promise; + resolve: (value: T) => void; + reject: (error: unknown) => void; +}; + +export function createDeferred(): Deferred { + let resolve!: (value: T) => void; + let reject!: (error: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +export function makePreflightResult(args: { + githubPrNumber: number; + title: string; + headBranch: string; + remoteBranch: string; +}): CreateLaneFromPrBranchPreflightResult { + return { + preflight: { + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: args.githubPrNumber, + githubUrl: `https://github.com/ade-dev/ade/pull/${args.githubPrNumber}`, + title: args.title, + headBranch: args.headBranch, + headRepoOwner: "ade-dev", + headRepoName: "ade", + headSha: "head-sha", + remoteBranch: args.remoteBranch, + importBranchRef: args.remoteBranch, + targetLaneName: args.title, + baseBranch: "main", + canCreate: true, + status: "ready", + blockingConflict: null, + blockingConflicts: [], + }, + lane: null, + pr: null, + }; +} + +export function makePrsContext(prs: Array & { id: string }>) { + return { + prs, + mergeContextByPrId: {}, + detailStatus: null, + detailChecks: [], + detailReviews: [], + detailComments: [], + detailBusy: false, + loading: false, + setViewerLogin: vi.fn(), + }; +} + +export function installGitHubTabWindowMocks(): void { + Object.assign(window, { + ade: { + prs: { + getGitHubSnapshot: vi.fn().mockResolvedValue(snapshot), + onEvent: vi.fn(() => () => {}), + linkToLane: vi.fn(), + preflightCreateLaneFromPrBranch: vi.fn().mockResolvedValue({ + preflight: { + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "Unlinked PR", + headBranch: "feature/open", + headRepoOwner: "ade-dev", + headRepoName: "ade", + remoteBranch: "origin/feature/open", + importBranchRef: "origin/feature/open", + targetLaneName: "Unlinked PR", + baseBranch: "main", + canCreate: true, + status: "ready", + blockingConflict: null, + blockingConflicts: [], + }, + lane: null, + pr: null, + }), + createLaneFromPrBranch: vi.fn().mockResolvedValue({ + preflight: { + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "Unlinked PR", + headBranch: "feature/open", + headRepoOwner: "ade-dev", + headRepoName: "ade", + remoteBranch: "origin/feature/open", + importBranchRef: "origin/feature/open", + targetLaneName: "Unlinked PR", + baseBranch: "main", + canCreate: true, + status: "ready", + blockingConflict: null, + blockingConflicts: [], + }, + lane: { id: "lane-created", name: "Unlinked PR" }, + pr: { id: "pr-created", laneId: "lane-created" }, + }), + addGitHubStackPullRequests: vi.fn().mockResolvedValue(null), + unstackGitHubStack: vi.fn().mockResolvedValue(null), + delete: vi.fn().mockResolvedValue(undefined), + }, + github: { + getStatus: vi.fn().mockResolvedValue({ + tokenStored: true, + patTokenStored: true, + tokenDecryptionFailed: false, + storageScope: "app", + authSource: "pat", + tokenType: "classic", + repo: { owner: "ade-dev", name: "ade" }, + hasOrigin: true, + userLogin: "octocat", + scopes: [], + ghCliPath: null, + ghAuthError: null, + checkedAt: "2026-03-13T12:00:00.000Z", + repoAccessOk: null, + repoAccessError: null, + connected: true, + }), + }, + app: { + openExternal: vi.fn(), + }, + lanes: { + list: vi.fn().mockResolvedValue([]), + }, + }, + }); +} diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.testHarness.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.testHarness.tsx new file mode 100644 index 000000000..70334e03b --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.testHarness.tsx @@ -0,0 +1,131 @@ +import React from "react"; +import { MemoryRouter } from "react-router-dom"; +import { cleanup, render } from "@testing-library/react"; +import { vi } from "vitest"; +import type { LaneSummary, MergeMethod } from "../../../../shared/types"; +import type { GitHubTabProps } from "./GitHubTab"; +import { installGitHubTabWindowMocks, makePrsContext } from "./GitHubTab.testFixtures"; + +export const mockUsePrs = vi.fn(); + +export function MockPanelGroup({ children }: { children: React.ReactNode }) { + return
{children}
; +} + +export function MockPanel({ + children, + id, + defaultSize: _defaultSize, + minSize: _minSize, + maxSize: _maxSize, + ...props +}: React.HTMLAttributes & { + id?: string; + defaultSize?: unknown; + minSize?: unknown; + maxSize?: unknown; +}) { + return
{children}
; +} + +export function MockSeparator(props: React.HTMLAttributes) { + return
; +} + +type MockUnmappedAffordance = { + linkableLanes: Array<{ id: string; name: string }>; + selectedLaneId: string; + onSelectLane: (laneId: string) => void; + onLink: () => void; + linkBusy: boolean; + canCreateLane: boolean; + onCreateLane: () => void; + scope: "repo" | "external"; +}; + +export function MockPrDetailPane({ + pr, + onUnmap, + unmapped, + unmappedAffordance, +}: { + pr: { id: string }; + onUnmap?: () => void; + unmapped?: boolean; + unmappedAffordance?: MockUnmappedAffordance | null; +}) { + return ( +
+ {pr.id} + {onUnmap ? : null} + {unmappedAffordance ? ( +
+ {unmappedAffordance.canCreateLane ? ( + + ) : null} + {unmappedAffordance.linkableLanes.length > 0 ? ( + <> + + + + ) : null} +
+ ) : null} +
+ ); +} + +export function setupGitHubTabTest(): void { + mockUsePrs.mockReturnValue(makePrsContext([ + { id: "pr-open", state: "open", checksStatus: "pending", reviewStatus: "requested", additions: 12, deletions: 3 }, + { id: "pr-merged", state: "merged", checksStatus: "passing", reviewStatus: "approved", additions: 5, deletions: 1 }, + ])); + installGitHubTabWindowMocks(); +} + +export function cleanupGitHubTabTest(): void { + cleanup(); + vi.useRealTimers(); +} + +export function renderGitHubTab( + Component: React.ComponentType, + overrides: Partial<{ + selectedPrId: string | null; + onSelectPr: ReturnType; + onRefreshAll: ReturnType; + lanes: LaneSummary[]; + }> = {}, +) { + const onSelectPr = overrides.onSelectPr ?? vi.fn(); + const onRefreshAll = overrides.onRefreshAll ?? vi.fn().mockResolvedValue(undefined); + render( + + + , + ); + return { onSelectPr, onRefreshAll }; +} diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx index 878360efc..a7535bb08 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx @@ -1,13 +1,7 @@ import React from "react"; -import { CircleNotch, GitBranch, GitMerge, GithubLogo, Warning, XCircle } from "@phosphor-icons/react"; import { useNavigate } from "react-router-dom"; -import { Group, Panel } from "react-resizable-panels"; -import { useVirtualizer } from "@tanstack/react-virtual"; import type { - CreateLaneFromPrBranchArgs, - CreateLaneFromPrBranchPreflight, CreateLaneFromPrBranchPreflightResult, - CreateLaneFromPrBranchResult, GitHubPrListItem, GitHubPrSnapshot, LaneSummary, @@ -16,81 +10,54 @@ import type { PrSummary, PrWithConflicts, } from "../../../../shared/types"; -import { syntheticGithubPrId } from "../../../../shared/types/prs"; -import { EmptyState } from "../../ui/EmptyState"; -import { ResizeGutter } from "../../ui/ResizeGutter"; -import { COLORS, LABEL_STYLE, MONO_FONT, SANS_FONT, cardStyle, outlineButton, primaryButton } from "../../lanes/laneDesignTokens"; import { selectActiveProjectRoot, useAppStore, useAppStoreApi } from "../../../state/appStore"; -import { PrDetailPane, type UnmappedAffordance } from "../detail/PrDetailPane"; +import type { UnmappedAffordance } from "../detail/PrDetailPane"; import { usePrs } from "../state/PrsContext"; import type { PrDetailRouteTab } from "../prsRouteState"; -import { GitHubRepoSyncBar } from "../shared/GitHubRepoSyncBar"; -import { GitHubPrSearchInput } from "../shared/GitHubPrSearchInput"; -import { GitHubStackInspector } from "../shared/GitHubStackInspector"; import { getGitHubSnapshotCoalesced } from "../../../lib/prReadCache"; -import { isTerminalPrState } from "../../../lib/prState"; -import { GitHubTabPrRow, PrListGroupHeaderRow } from "../shared/GitHubTabPrRow"; -import { branchNameFromRef } from "./githubPrBranch"; import { - buildPrListRows, - prListHeaderIndices, - type PrListRow, -} from "../shared/prListGrouping"; - -/* -- Filter button styles -- */ -const FILTER_COLORS: Record = { - open: { - active: { bg: "linear-gradient(135deg, rgba(59,130,246,0.16) 0%, rgba(37,99,235,0.08) 100%)", border: "rgba(59,130,246,0.30)", text: "#60A5FA", shadow: "0 0 10px rgba(59,130,246,0.12)" }, - inactive: { text: "#60A5FA" }, - }, - closed: { - active: { bg: "linear-gradient(135deg, rgba(161,161,170,0.14) 0%, rgba(113,113,122,0.06) 100%)", border: "rgba(161,161,170,0.25)", text: "#A1A1AA", shadow: "0 0 10px rgba(161,161,170,0.08)" }, - inactive: { text: "#71717A" }, - }, - merged: { - active: { bg: "linear-gradient(135deg, rgba(34,197,94,0.14) 0%, rgba(22,163,74,0.06) 100%)", border: "rgba(34,197,94,0.28)", text: "#4ADE80", shadow: "0 0 10px rgba(34,197,94,0.10)" }, - inactive: { text: "#4ADE80" }, - }, -}; - -const VIRTUALIZE_AT = 50; -const LINKED_HYDRATION_LIMIT = 8; -const GITHUB_TAB_REVISIT_CACHE_TTL_MS = 60_000; -const GITHUB_TAB_SNAPSHOT_FRESH_MS = 30_000; -const GITHUB_TAB_HOT_REFRESH_DELAY_MS = 30_000; -const GITHUB_TAB_HISTORY_INITIAL_PAGE_LIMIT = 2; -const GITHUB_TAB_HISTORY_PAGE_INCREMENT = 2; -const GITHUB_TAB_HISTORY_MAX_PAGE_LIMIT = 10; -const GITHUB_TAB_CACHE_DISABLED = import.meta.env.MODE === "test"; -const GITHUB_PR_LIST_WIDTH_KEY = "ade.prs.githubListWidth"; -const GITHUB_PR_LIST_MIN_PX = 260; -const GITHUB_PR_LIST_MAX_PX = 560; -const GITHUB_PR_LIST_DEFAULT_PX = 380; - -function readPersistedGithubPrListPx(): number { - try { - const raw = localStorage.getItem(GITHUB_PR_LIST_WIDTH_KEY); - if (raw) { - const value = Number(raw); - if (Number.isFinite(value) && value >= GITHUB_PR_LIST_MIN_PX && value <= GITHUB_PR_LIST_MAX_PX) { - return value; - } - } - } catch { - /* ignore */ - } - return GITHUB_PR_LIST_DEFAULT_PX; -} - -function persistGithubPrListPx(px: number): void { - try { - localStorage.setItem(GITHUB_PR_LIST_WIDTH_KEY, String(Math.round(px))); - } catch { - /* ignore */ - } -} + GITHUB_TAB_HISTORY_INITIAL_PAGE_LIMIT, + GITHUB_TAB_HISTORY_MAX_PAGE_LIMIT, + GITHUB_TAB_HISTORY_PAGE_INCREMENT, + GITHUB_TAB_HOT_REFRESH_DELAY_MS, + GITHUB_TAB_REVISIT_CACHE_TTL_MS, + GITHUB_TAB_SNAPSHOT_FRESH_MS, + LINKED_HYDRATION_LIMIT, + buildSyntheticUnmappedPr, + bucketForState, + formatGitHubSnapshotError, + initialGitHubFilterSelections, + matchesFilter, + normalizeGitHubFilter, + normalizeHistoryPageLimit, + readGitHubTabWarmCache, + snapshotRequestKey, + snapshotRequestSatisfies, + syntheticUnmappedPrId, + writeGitHubTabWarmCache, + type GitHubFilter, + type GitHubFilterSelectionMap, + type GitHubSnapshotRequestKey, + type GitHubTabWarmCache, +} from "./githubTabModel"; +import { + CreateLaneFromPrBranchDialog, + canCreateLaneFromPrBranch, + createLaneFromPrBranchApi, + createLaneFromPrBranchArgs, + createLaneFromPrBranchRequestKey, + createLaneMappedLaneId, + createLaneMappedLaneName, + createLaneMappedPrId, + formatActionError, + patchSnapshotWithMappedPr, + upsertLaneSummary, +} from "./GitHubTabCreateLaneDialog"; +import { GitHubTabView } from "./GitHubTabView"; +import { branchNameFromRef } from "./githubPrBranch"; +import { useGitHubTabListModel } from "./useGitHubTabListModel"; -type GitHubTabProps = { +export type GitHubTabProps = { lanes: LaneSummary[]; mergeMethod: MergeMethod; selectedPrId: string | null; @@ -112,501 +79,6 @@ export type GitHubHeaderChromeState = { onSearchQueryChange: (value: string) => void; }; -type GitHubFilter = "open" | "closed" | "merged"; -type GitHubFilterSelectionMap = Partial>; - -type GitHubTabWarmCache = { - projectRoot: string; - snapshot: GitHubPrSnapshot | null; - filter: GitHubFilter; - selectedItemId: string | null; - selectedItemIdsByFilter?: GitHubFilterSelectionMap; - searchQuery: string; - externalHistoryLoaded: boolean; - cachedAt: number; -}; - -type GitHubSnapshotRequestKey = { - includeExternalClosed: boolean; - historyPageLimit: number; -}; - -type CreateLaneFromPrBranchApi = { - preflightCreateLaneFromPrBranch: ( - args: CreateLaneFromPrBranchArgs, - ) => Promise; - createLaneFromPrBranch: ( - args: CreateLaneFromPrBranchArgs, - ) => Promise; -}; - -let githubTabWarmCache: GitHubTabWarmCache | null = null; - -function normalizeGitHubFilter(value: unknown): GitHubFilter { - return value === "open" || value === "closed" || value === "merged" ? value : "open"; -} - -function initialGitHubFilterSelections(cache: GitHubTabWarmCache | null): GitHubFilterSelectionMap { - const selections: GitHubFilterSelectionMap = { ...(cache?.selectedItemIdsByFilter ?? {}) }; - if (cache?.selectedItemId) { - selections[normalizeGitHubFilter(cache.filter)] = cache.selectedItemId; - } - return selections; -} - -function normalizeHistoryPageLimit(value: unknown): number { - const numeric = Number(value); - if (!Number.isFinite(numeric) || numeric <= 0) { - return GITHUB_TAB_HISTORY_INITIAL_PAGE_LIMIT; - } - return Math.min( - GITHUB_TAB_HISTORY_MAX_PAGE_LIMIT, - Math.max(GITHUB_TAB_HISTORY_INITIAL_PAGE_LIMIT, Math.floor(numeric)), - ); -} - -function snapshotRequestKey(options?: { - includeExternalClosed?: boolean; - historyPageLimit?: number; -}): GitHubSnapshotRequestKey { - const includeExternalClosed = options?.includeExternalClosed === true; - return { - includeExternalClosed, - historyPageLimit: includeExternalClosed ? normalizeHistoryPageLimit(options?.historyPageLimit) : 0, - }; -} - -function snapshotRequestSatisfies( - current: GitHubSnapshotRequestKey | null, - requested: GitHubSnapshotRequestKey, -): boolean { - if (!current) return false; - if (!requested.includeExternalClosed) return true; - return current.includeExternalClosed && current.historyPageLimit >= requested.historyPageLimit; -} - -function readGitHubTabWarmCache(projectRoot: string | null): GitHubTabWarmCache | null { - if (GITHUB_TAB_CACHE_DISABLED) return null; - if (!projectRoot) return null; - if (githubTabWarmCache?.projectRoot !== projectRoot) return null; - const filter = normalizeGitHubFilter((githubTabWarmCache as { filter?: unknown }).filter); - return filter === githubTabWarmCache.filter ? githubTabWarmCache : { ...githubTabWarmCache, filter }; -} - -function writeGitHubTabWarmCache(cache: GitHubTabWarmCache): void { - if (GITHUB_TAB_CACHE_DISABLED) return; - if (!cache.projectRoot) return; - githubTabWarmCache = cache; -} - -function formatGitHubSnapshotError(err: unknown): string { - const raw = err instanceof Error ? err.message : String(err ?? ""); - const message = raw - .replace(/^Error invoking remote method '[^']+':\s*/i, "") - .replace(/^Error:\s*/i, "") - .trim(); - if (/github (token|auth) missing/i.test(message)) { - return "Connect GitHub in Settings with gh auth or a PAT to sync pull requests."; - } - return message || "Unable to sync pull requests."; -} - -function formatActionError(err: unknown): string { - const raw = err instanceof Error ? err.message : String(err ?? ""); - return raw - .replace(/^Error invoking remote method '[^']+':\s*/i, "") - .replace(/^Error:\s*/i, "") - .trim() || "Action failed."; -} - -function createLaneFromPrBranchApi(): CreateLaneFromPrBranchApi { - return window.ade.prs as typeof window.ade.prs & CreateLaneFromPrBranchApi; -} - -function createLaneFromPrBranchArgs(item: GitHubPrListItem): CreateLaneFromPrBranchArgs { - return { - repoOwner: item.repoOwner, - repoName: item.repoName, - githubPrNumber: item.githubPrNumber, - }; -} - -function createLaneFromPrBranchRequestKey(item: GitHubPrListItem): string { - return `${item.repoOwner}/${item.repoName}#${Number(item.githubPrNumber)}`; -} - -function preflightText(value: unknown): string | null { - if (value == null) return null; - if (typeof value === "string") { - const trimmed = value.trim(); - return trimmed || null; - } - if (typeof value === "number" || typeof value === "boolean") return String(value); - if (typeof value === "object") { - const record = value as Record; - const message = preflightText(record.message) ?? preflightText(record.reason) ?? preflightText(record.summary); - if (message) return message; - try { - return JSON.stringify(value); - } catch { - return "Conflict details unavailable."; - } - } - return String(value); -} - -function preflightPrNumber(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): number { - return Number(preflight?.githubPrNumber ?? item.githubPrNumber); -} - -function preflightTitle(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): string { - return preflightText(preflight?.title) ?? item.title; -} - -function preflightRemoteBranch(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): string { - return preflightText(preflight?.remoteBranch) ?? preflightText(preflight?.headBranch) ?? item.headBranch ?? "---"; -} - -function preflightImportRef(preflight: CreateLaneFromPrBranchPreflight | null): string | null { - return preflightText(preflight?.importBranchRef); -} - -function preflightTargetLaneName(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): string { - const remoteBranch = preflightRemoteBranch(preflight, item); - const fallback = branchNameFromRef(remoteBranch); - return preflightText(preflight?.targetLaneName) ?? (fallback || "New lane"); -} - -function preflightBaseBranch(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): string { - return preflightText(preflight?.baseBranch) ?? item.baseBranch ?? "---"; -} - -function preflightBlockingConflict(preflight: CreateLaneFromPrBranchPreflight | null): string | null { - return preflightText(preflight?.blockingConflict); -} - -function createLaneMappedPrId(result: CreateLaneFromPrBranchResult): string | null { - return preflightText(result.pr?.id); -} - -function createLaneMappedLaneId(result: CreateLaneFromPrBranchResult): string | null { - return preflightText(result.pr?.laneId) ?? preflightText(result.lane?.id); -} - -function createLaneMappedLaneName(result: CreateLaneFromPrBranchResult): string | null { - return preflightText(result.lane?.name); -} - -function upsertLaneSummary(lanes: LaneSummary[], lane: LaneSummary): LaneSummary[] { - const index = lanes.findIndex((entry) => entry.id === lane.id); - if (index === -1) return [lane, ...lanes]; - const next = lanes.slice(); - next[index] = lane; - return next; -} - -function isKnownPrState(value: unknown): value is PrSummary["state"] { - return value === "draft" || value === "open" || value === "merged" || value === "closed"; -} - -function reconcileLinkedPrState(item: GitHubPrListItem, linkedPr: PrSummary | null | undefined): GitHubPrListItem { - if (!isKnownPrState(linkedPr?.state)) return item; - if (!isTerminalPrState(linkedPr.state) || isTerminalPrState(item.state)) return item; - return { - ...item, - state: linkedPr.state, - isDraft: false, - title: linkedPr.title || item.title, - updatedAt: linkedPr.updatedAt || item.updatedAt, - }; -} - -function matchesFilter(item: GitHubPrListItem, filter: GitHubFilter): boolean { - if (filter === "open") return item.state === "open" || item.state === "draft"; - return item.state === filter; -} - -/** The filter bucket a PR row belongs in based on its (reconciled) state. */ -function bucketForState(state: GitHubPrListItem["state"]): GitHubFilter { - return state === "merged" ? "merged" : state === "closed" ? "closed" : "open"; -} - -/** Stable repo/owner/number coordinate key, shared by the terminal-row overlay. */ -function githubCoordKey(item: { repoOwner: string; repoName: string; githubPrNumber: number }): string { - return `${item.repoOwner}/${item.repoName}#${Number(item.githubPrNumber)}`; -} - -/** - * Build a terminal-bucket overlay row for a linked PR that has dropped out of an - * open-only GitHub snapshot. Reuses the last-seen row (its lane info, labels, and - * — crucially — its `id`, so the current selection stays attached) and stamps the - * authoritative terminal state from the linked ADE PR. - */ -function buildOverlayRowFromLastSeen(lastSeen: GitHubPrListItem, linkedPr: PrSummary): GitHubPrListItem { - return { - ...lastSeen, - state: linkedPr.state, - isDraft: false, - title: linkedPr.title || lastSeen.title, - updatedAt: linkedPr.updatedAt || lastSeen.updatedAt, - linkedPrId: linkedPr.id, - }; -} - -/** - * Terminal-bucket overlay rows: linked ADE PRs that have gone merged/closed but - * have dropped out of the open-only GitHub snapshot. We keep their last-seen row - * visible under the terminal bucket until a full-history fetch reintroduces the - * authoritative row (at which point it is `present` and produces no overlay). - * Reopened PRs (linked state back to open) are non-terminal, so they produce no - * overlay and naturally drop. Rows never previously displayed are skipped — we - * have no lane/label info to synthesize from. - */ -function computeTerminalOverlayItems( - reconciledItems: GitHubPrListItem[], - prsById: Map, - lastSeenByCoord: Map, -): GitHubPrListItem[] { - if (prsById.size === 0) return []; - const presentCoords = new Set(reconciledItems.map((item) => githubCoordKey(item))); - const overlays: GitHubPrListItem[] = []; - const usedLinkedIds = new Set(); - for (const pr of prsById.values()) { - if (!isTerminalPrState(pr.state)) continue; - if (usedLinkedIds.has(pr.id)) continue; - const key = githubCoordKey(pr); - if (presentCoords.has(key)) continue; - const lastSeen = lastSeenByCoord.get(key); - if (!lastSeen) continue; - usedLinkedIds.add(pr.id); - overlays.push(buildOverlayRowFromLastSeen(lastSeen, pr)); - } - return overlays; -} - -function mergeGitHubListItems(snapshot: GitHubPrSnapshot): GitHubPrListItem[] { - const combined = [...snapshot.repoPullRequests, ...snapshot.externalPullRequests]; - const seen = new Set(); - return combined.filter((item) => { - const key = `${item.scope}:${item.repoOwner}/${item.repoName}#${item.githubPrNumber}`; - if (seen.has(key)) return false; - seen.add(key); - return true; - }); -} - -type GitHubFilterCounts = Record; - -function countGitHubItemsByState(items: GitHubPrListItem[]): GitHubFilterCounts { - return { - open: items.filter((item) => item.state === "open" || item.state === "draft").length, - closed: items.filter((item) => item.state === "closed").length, - merged: items.filter((item) => item.state === "merged").length, - }; -} - - - -function canCreateLaneFromPrBranch(item: GitHubPrListItem, lanes: LaneSummary[]): boolean { - if (item.linkedPrId || item.scope !== "repo") return false; - if (item.state !== "open" && item.state !== "draft") return false; - const headBranch = branchNameFromRef(item.headBranch); - if (!headBranch) return false; - return !lanes.some((lane) => !lane.archivedAt && branchNameFromRef(lane.branchRef) === headBranch); -} - -function sameGitHubPr(left: GitHubPrListItem, right: GitHubPrListItem): boolean { - return left.repoOwner === right.repoOwner - && left.repoName === right.repoName - && Number(left.githubPrNumber) === Number(right.githubPrNumber); -} - -/** - * Stable synthetic PR id for an unmapped GitHub PR (no ADE lane / DB row). - * Must be deterministic across renders — it keys both per-id effects in the - * pane and the React `key`. - */ -function syntheticUnmappedPrId(item: GitHubPrListItem): string { - return syntheticGithubPrId(item); -} - -/** - * Build a referentially-stable synthetic `PrWithConflicts` for an unmapped - * GitHub PR so it can flow through the full `PrDetailPane`. `laneId` is empty - * (no lane) and the id is derived deterministically from the GitHub item. - */ -function buildSyntheticUnmappedPr(item: GitHubPrListItem, projectId: string): PrWithConflicts { - return { - id: syntheticUnmappedPrId(item), - laneId: "", - projectId, - repoOwner: item.repoOwner, - repoName: item.repoName, - githubPrNumber: item.githubPrNumber, - githubUrl: item.githubUrl, - githubNodeId: null, - title: item.title, - state: item.state, - baseBranch: item.baseBranch ?? "", - headBranch: item.headBranch ?? "", - checksStatus: "none", - reviewStatus: "none", - additions: 0, - deletions: 0, - lastSyncedAt: null, - createdAt: item.createdAt, - updatedAt: item.updatedAt, - stack: item.stack ?? null, - conflictAnalysis: null, - }; -} - -function patchSnapshotWithMappedPr( - snapshot: GitHubPrSnapshot, - item: GitHubPrListItem, - args: { - mappedPrId: string; - laneId: string | null; - laneName: string | null; - }, -): GitHubPrSnapshot { - const patchItems = (items: GitHubPrListItem[]) => items.map((candidate) => { - if (candidate.id !== item.id && !sameGitHubPr(candidate, item)) return candidate; - return { - ...candidate, - linkedPrId: args.mappedPrId, - linkedLaneId: args.laneId ?? candidate.linkedLaneId, - linkedLaneName: args.laneName ?? candidate.linkedLaneName, - adeKind: candidate.adeKind ?? "single", - }; - }); - return { - ...snapshot, - repoPullRequests: patchItems(snapshot.repoPullRequests), - externalPullRequests: patchItems(snapshot.externalPullRequests), - }; -} - -function CreateLaneFromPrBranchDialog({ - item, - preflight, - loading, - busy, - error, - onCancel, - onConfirm, -}: { - item: GitHubPrListItem; - preflight: CreateLaneFromPrBranchPreflight | null; - loading: boolean; - busy: boolean; - error: string | null; - onCancel: () => void; - onConfirm: () => void; -}) { - const blockingConflict = preflightBlockingConflict(preflight); - const canConfirm = Boolean(preflight?.canCreate) && !loading && !busy; - const sourceBranch = preflightRemoteBranch(preflight, item); - const importRef = preflightImportRef(preflight); - const rows = [ - ["PR", `#${preflightPrNumber(preflight, item)} ${preflightTitle(preflight, item)}`], - ["Source branch", sourceBranch], - ...(importRef && importRef !== sourceBranch ? [["Import ref", importRef] as const] : []), - ["Target lane", preflightTargetLaneName(preflight, item)], - ["Base branch", preflightBaseBranch(preflight, item)], - ] as const; - - return ( -
-
-
-
- Create lane from PR branch -
-
-
- {loading ? ( -
- Checking branch ownership and PR head availability... -
- ) : ( -
- {rows.map(([label, value]) => ( -
-
{label}
-
- {value} -
-
- ))} -
- )} - {blockingConflict ? ( -
- - {blockingConflict} -
- ) : null} - {error ? ( -
- {error} -
- ) : null} -
-
- - -
-
-
- ); -} - export function GitHubTab({ lanes, mergeMethod, @@ -687,7 +159,6 @@ export function GitHubTab({ const externalHistoryLoadedRef = React.useRef(externalHistoryLoaded); const projectRootRef = React.useRef(projectRoot); const listRef = React.useRef(null); - const defaultListPx = React.useMemo(() => readPersistedGithubPrListPx(), []); snapshotRef.current = snapshot; filterRef.current = filter; externalHistoryLoadedRef.current = externalHistoryLoaded; @@ -903,87 +374,22 @@ export function GitHubTab({ }); }, [currentHistoryPageLimit, loadSnapshot, prs, prsContextLoading, startHotRefreshWindow]); - const matchesSearch = React.useCallback((item: GitHubPrListItem) => { - if (!searchQuery.trim()) return true; - const q = searchQuery.trim().toLowerCase(); - return ( - item.title.toLowerCase().includes(q) || - (item.author?.toLowerCase().includes(q) ?? false) || - (item.headBranch?.toLowerCase().includes(q) ?? false) || - String(item.githubPrNumber).includes(q) - ); - }, [searchQuery]); - - const allItems = React.useMemo( - () => (snapshot ? mergeGitHubListItems(snapshot) : []), - [snapshot], - ); - const reconciledItems = React.useMemo( - () => allItems.map((item) => - reconcileLinkedPrState(item, item.linkedPrId ? prsByIdMap.get(item.linkedPrId) : null) - ), - [allItems, prsByIdMap], - ); - - // Remember every row we have actually shown so the terminal-row overlay can - // resurrect one that later drops out of an open-only snapshot. The map only - // grows within a session (PR counts are small), and is read during render by - // the overlay memo below — recorded after commit so the overlay memo still - // sees the prior row on the render where a PR first disappears. - React.useEffect(() => { - const map = lastSeenRowByCoordRef.current; - for (const item of allItems) { - map.set(githubCoordKey(item), item); - } - }, [allItems]); - - const overlayItems = React.useMemo( - () => computeTerminalOverlayItems(reconciledItems, prsByIdMap, lastSeenRowByCoordRef.current), - [reconciledItems, prsByIdMap], - ); - - const displayedItems = React.useMemo( - () => (overlayItems.length === 0 ? reconciledItems : [...reconciledItems, ...overlayItems]), - [reconciledItems, overlayItems], - ); - - const filteredItems = React.useMemo( - () => displayedItems - .filter((item) => matchesFilter(item, filter) && matchesSearch(item)) - .sort((a, b) => - new Date(b.updatedAt || b.createdAt).getTime() - new Date(a.updatedAt || a.createdAt).getTime(), - ), - [displayedItems, filter, matchesSearch], - ); - const hydrationItems = filteredItems.length > VIRTUALIZE_AT ? renderedHydrationItems : filteredItems; - - // Period headers only in the terminal buckets. `filteredItems` stays a pure item - // array so selection, hydration and sorting keep operating on rows alone. - const listRows = React.useMemo( - () => buildPrListRows(filteredItems, { grouped: filter === "merged" || filter === "closed" }), - [filteredItems, filter], - ); - - const filterCounts = React.useMemo(() => { - const listedCounts = countGitHubItemsByState(displayedItems); - const snapshotCounts = snapshot?.history?.repoPullRequestCounts; - // Snapshot totals were computed server-side from the raw snapshot, so a - // row that reconciliation moved between states (stale open → merged) - // must move in the badge totals too — otherwise the item changes tabs - // while the counts still bucket it under its stale state. - const rawCounts = countGitHubItemsByState(allItems); - const withReconcileDelta = (base: number | null | undefined, key: keyof GitHubFilterCounts, fallback: number): number => - base == null ? fallback : Math.max(0, base + listedCounts[key] - rawCounts[key]); - return { - open: withReconcileDelta(snapshotCounts?.open, "open", listedCounts.open), - closed: withReconcileDelta(snapshotCounts?.closed, "closed", listedCounts.closed), - merged: withReconcileDelta(snapshotCounts?.merged, "merged", listedCounts.merged), - }; - }, [allItems, displayedItems, snapshot?.history?.repoPullRequestCounts]); - const canLoadOlderHistory = - filter !== "open" - && Boolean(snapshot?.history?.repoPullRequestsMayHaveMore) - && currentHistoryPageLimit() < GITHUB_TAB_HISTORY_MAX_PAGE_LIMIT; + const { + displayedItems, + filteredItems, + hydrationItems, + listRows, + filterCounts, + canLoadOlderHistory, + } = useGitHubTabListModel({ + snapshot, + searchQuery, + prsByIdMap, + filter, + renderedHydrationItems, + lastSeenRowByCoordRef, + currentHistoryPageLimit, + }); const showListLoadingIndicator = loading || syncing || loadingFilter !== null; React.useEffect(() => { @@ -1493,296 +899,80 @@ export function GitHubTab({ selectedItem, ]); - if (error && !snapshot) { - return ( - - - - ); - } + const detailPaneProps = selectedItem && selectedDisplayPr ? { + pr: selectedDisplayPr, + status: selectedLinkedPr ? detailStatus : null, + checks: selectedLinkedPr ? detailChecks : [], + reviews: selectedLinkedPr ? detailReviews : [], + comments: selectedLinkedPr ? detailComments : [], + snapshotHydration: selectedLinkedPr + ? (detailSnapshot?.prId === selectedDisplayPr.id + ? detailSnapshot + : detailSnapshotsByPrId[selectedDisplayPr.id] ?? null) + : null, + snapshotHydrationOwnedByContext: Boolean(selectedLinkedPr), + liveDetailReady: Boolean(selectedLinkedPr) && detailLiveDataPrId === selectedDisplayPr.id, + detailBusy, + lanes, + mergeMethod, + onRefresh: handleSync, + onNavigate: navigate, + onOpenRebaseTab, + initialDetailTab: selectedDetailTab, + onDetailTabChange, + onUnmap: selectedItem.linkedPrId ? () => handleUnlink(selectedItem) : undefined, + unmapBusy: Boolean(selectedItem.linkedPrId) && unlinkingPrId === selectedItem.linkedPrId, + unmapped: !selectedItem.linkedPrId, + githubCoords: selectedItem.linkedPrId ? null : selectedGithubCoords, + unmappedAffordance: selectedItem.linkedPrId ? null : unmappedAffordance, + } : null; return ( -
- {/* Search / sync chrome only renders inline when it hasn't been hoisted to - the shared PRs header. The Open/Merged/Closed tabs now live at the top of - the list column (below) so the detail pane can rise to sit level with them. */} - {!relocateHeaderChrome ? ( -
- - { - void handleSync(); - }} - /> -
- ) : null} - - {error ? ( -
- {error} -
- ) : null} - -
- - persistGithubPrListPx(size.inPixels)} - className="min-h-0 min-w-0" - style={{ overflow: "hidden", borderRight: "1px solid rgba(255,255,255,0.06)" }} - > -
- {/* Filter tabs (Open / Merged / Closed) — fixed header capping the - list column. The detail pane to the right rises to sit level with - these; the list min width keeps the right edge of "Closed" aligned - with the list/detail divider. */} -
- {(["open", "merged", "closed"] as GitHubFilter[]).map((state) => { - const active = filter === state; - const fc = FILTER_COLORS[state]; - const count = filterCounts[state]; - const icon = state === "merged" ? : null; - const tabLoading = active && (loading || syncing || loadingFilter === state || loadingOlderHistory); - return ( - - ); - })} -
- {showListLoadingIndicator ? ( - - - - ) : null} -
-
- {filteredItems.length === 0 ? ( -
- -
- ) : filteredItems.length > VIRTUALIZE_AT ? ( - - ) : ( - listRows.map((row) => ( - row.kind === "header" ? ( - - ) : ( - - ) - )) - )} - {canLoadOlderHistory ? ( -
- -
- ) : null} -
-
- - - - {selectedItem && selectedDisplayPr ? ( -
- {selectedBucketMismatch ? ( -
- handleFilterChange(bucketForState(selectedItem.state))} - /> -
- ) : null} - {selectedStack ? ( - item.repoOwner === selectedStack.repoOwner - && item.repoName === selectedStack.repoName, - )} - selectedPrNumber={selectedItem.githubPrNumber} - syncing={syncing} - onSelectPr={handleSelectItem} - onOpenGitHub={() => { - void window.ade.app.openExternal(selectedItem.githubUrl); - }} - onSync={() => { - void handleSync(); - }} - onAddPullRequests={handleAddStackPullRequests} - onUnstack={handleUnstack} - /> - ) : null} -
- handleUnlink(selectedItem) : undefined} - unmapBusy={Boolean(selectedItem.linkedPrId) && unlinkingPrId === selectedItem.linkedPrId} - unmapped={!selectedItem.linkedPrId} - githubCoords={selectedItem.linkedPrId ? null : selectedGithubCoords} - unmappedAffordance={selectedItem.linkedPrId ? null : unmappedAffordance} - /> -
-
- ) : ( -
- -
- )} - - -
+ <> + { void handleSync(); }, + error, + onConnectGitHub: () => navigate("/settings?tab=general#github-connection"), + }} + list={{ + parentRef: listRef, + filter, + filterCounts, + loading, + loadingFilter, + loadingOlderHistory, + showLoadingIndicator: showListLoadingIndicator, + hasSnapshot: Boolean(snapshot), + filteredItems, + rows: listRows, + selectedItemId, + prsByIdMap, + canLoadOlderHistory, + onFilterChange: handleFilterChange, + onSelect: handleSelectItem, + onHydrationItemsChange: handleHydrationItemsChange, + onLoadOlderHistory: () => { void handleLoadOlderHistory(); }, + }} + detail={{ + selectedItem, + selectedBucketMismatch, + selectedStack, + displayedItems, + paneProps: detailPaneProps, + onSelect: handleSelectItem, + onSync: () => { void handleSync(); }, + onAddStackPullRequests: handleAddStackPullRequests, + onUnstack: handleUnstack, + onFilterChange: handleFilterChange, + }} + /> {createLaneItem ? ( ) : null} -
- ); -} - -/** - * Slim banner shown at the top of the detail pane when the selected PR's state - * has moved out of the active filter's bucket (transiently, or after a manual - * filter change). Matches the neighboring lane/rebase banner idiom — no new - * colors or gradients. - */ -function PrBucketTransitionBanner({ - state, - onShow, -}: { - state: GitHubPrListItem["state"]; - onShow: () => void; -}) { - const isMerged = state === "merged"; - const label = isMerged ? "Merged" : "Closed"; - const accent = isMerged ? COLORS.success : COLORS.danger; - return ( -
-
-
- {isMerged ? ( - - ) : ( - - )} - - This PR is now {label} - -
- -
-
- ); -} - -/* ---- Virtual list for GitHub PR sidebar (activated above VIRTUALIZE_AT) ---- */ -function GitHubTabVirtualList({ - parentRef, - rows, - selectedItemId, - prsByIdMap, - onSelect, - onHydrationItemsChange, -}: { - parentRef: React.RefObject; - rows: PrListRow[]; - selectedItemId: string | null; - prsByIdMap: Map; - onSelect: (item: GitHubPrListItem) => void; - onHydrationItemsChange: (items: GitHubPrListItem[]) => void; -}) { - const headerIndices = React.useMemo(() => prListHeaderIndices(rows), [rows]); - - const virtualizer = useVirtualizer({ - count: rows.length, - getScrollElement: () => parentRef.current, - // Headers are much shorter than rows; a bad estimate here makes the scrollbar jump. - estimateSize: (index) => (rows[index]?.kind === "header" ? 30 : 108), - overscan: 6, - rangeExtractor: React.useCallback( - (range: { startIndex: number; endIndex: number; overscan: number; count: number }) => { - // Keep the governing header rendered even once it has scrolled out of range, - // so it can be pinned to the top of the viewport. - const pinned = activeHeaderFor(headerIndices, range.startIndex); - const start = Math.max(0, range.startIndex - range.overscan); - const end = Math.min(range.count - 1, range.endIndex + range.overscan); - const indices = new Set(); - if (pinned != null) indices.add(pinned); - for (let index = start; index <= end; index += 1) indices.add(index); - return [...indices].sort((a, b) => a - b); - }, - [headerIndices], - ), - }); - - const virtualItems = virtualizer.getVirtualItems(); - // Derived during render from the range we are about to draw. An earlier version - // round-tripped this through a ref and a state-setting effect, which re-rendered the - // whole list on every scroll frame to compute something already in hand. - const activeHeaderIndex = activeHeaderFor(headerIndices, virtualItems[0]?.index ?? 0); - - React.useEffect(() => { - // Only PR rows may be hydrated — a header has nothing to fetch. - onHydrationItemsChange( - virtualItems - .map((virtualRow) => rows[virtualRow.index]) - .filter((row): row is Extract => row?.kind === "item") - .map((row) => row.item), - ); - }, [rows, onHydrationItemsChange, virtualItems]); - - return ( -
- {virtualItems.map((virtualRow) => { - const row = rows[virtualRow.index]!; - const pinned = row.kind === "header" && virtualRow.index === activeHeaderIndex; - return ( -
- {row.kind === "header" ? ( - - ) : ( - - )} -
- ); - })} -
+ ); } - -/** The last header at or above `startIndex` — the one governing that scroll position. */ -function activeHeaderFor(headerIndices: number[], startIndex: number): number | null { - let active: number | null = null; - for (const index of headerIndices) { - if (index > startIndex) break; - active = index; - } - return active; -} diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsx new file mode 100644 index 000000000..143f57da4 --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsx @@ -0,0 +1,282 @@ +import { GitBranch, Warning } from "@phosphor-icons/react"; +import type { + CreateLaneFromPrBranchArgs, + CreateLaneFromPrBranchPreflight, + CreateLaneFromPrBranchPreflightResult, + CreateLaneFromPrBranchResult, + GitHubPrListItem, + GitHubPrSnapshot, + LaneSummary, +} from "../../../../shared/types"; +import { + COLORS, + LABEL_STYLE, + MONO_FONT, + SANS_FONT, + outlineButton, + primaryButton, +} from "../../lanes/laneDesignTokens"; +import { branchNameFromRef } from "./githubPrBranch"; + +type CreateLaneFromPrBranchApi = { + preflightCreateLaneFromPrBranch: ( + args: CreateLaneFromPrBranchArgs, + ) => Promise; + createLaneFromPrBranch: ( + args: CreateLaneFromPrBranchArgs, + ) => Promise; +}; + +export function formatActionError(err: unknown): string { + const raw = err instanceof Error ? err.message : String(err ?? ""); + return raw + .replace(/^Error invoking remote method '[^']+':\s*/i, "") + .replace(/^Error:\s*/i, "") + .trim() || "Action failed."; +} + +export function createLaneFromPrBranchApi(): CreateLaneFromPrBranchApi { + return window.ade.prs as typeof window.ade.prs & CreateLaneFromPrBranchApi; +} + +export function createLaneFromPrBranchArgs(item: GitHubPrListItem): CreateLaneFromPrBranchArgs { + return { + repoOwner: item.repoOwner, + repoName: item.repoName, + githubPrNumber: item.githubPrNumber, + }; +} + +export function createLaneFromPrBranchRequestKey(item: GitHubPrListItem): string { + return `${item.repoOwner}/${item.repoName}#${Number(item.githubPrNumber)}`; +} + +function preflightText(value: unknown): string | null { + if (value == null) return null; + if (typeof value === "string") { + const trimmed = value.trim(); + return trimmed || null; + } + if (typeof value === "number" || typeof value === "boolean") return String(value); + if (typeof value === "object") { + const record = value as Record; + const message = preflightText(record.message) ?? preflightText(record.reason) ?? preflightText(record.summary); + if (message) return message; + try { + return JSON.stringify(value); + } catch { + return "Conflict details unavailable."; + } + } + return String(value); +} + +function preflightPrNumber(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): number { + return Number(preflight?.githubPrNumber ?? item.githubPrNumber); +} + +function preflightTitle(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): string { + return preflightText(preflight?.title) ?? item.title; +} + +function preflightRemoteBranch(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): string { + return preflightText(preflight?.remoteBranch) ?? preflightText(preflight?.headBranch) ?? item.headBranch ?? "---"; +} + +function preflightImportRef(preflight: CreateLaneFromPrBranchPreflight | null): string | null { + return preflightText(preflight?.importBranchRef); +} + +function preflightTargetLaneName(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): string { + const remoteBranch = preflightRemoteBranch(preflight, item); + const fallback = branchNameFromRef(remoteBranch); + return preflightText(preflight?.targetLaneName) ?? (fallback || "New lane"); +} + +function preflightBaseBranch(preflight: CreateLaneFromPrBranchPreflight | null, item: GitHubPrListItem): string { + return preflightText(preflight?.baseBranch) ?? item.baseBranch ?? "---"; +} + +function preflightBlockingConflict(preflight: CreateLaneFromPrBranchPreflight | null): string | null { + return preflightText(preflight?.blockingConflict); +} + +export function createLaneMappedPrId(result: CreateLaneFromPrBranchResult): string | null { + return preflightText(result.pr?.id); +} + +export function createLaneMappedLaneId(result: CreateLaneFromPrBranchResult): string | null { + return preflightText(result.pr?.laneId) ?? preflightText(result.lane?.id); +} + +export function createLaneMappedLaneName(result: CreateLaneFromPrBranchResult): string | null { + return preflightText(result.lane?.name); +} + +export function upsertLaneSummary(lanes: LaneSummary[], lane: LaneSummary): LaneSummary[] { + const index = lanes.findIndex((entry) => entry.id === lane.id); + if (index === -1) return [lane, ...lanes]; + const next = lanes.slice(); + next[index] = lane; + return next; +} + +export function canCreateLaneFromPrBranch(item: GitHubPrListItem, lanes: LaneSummary[]): boolean { + if (item.linkedPrId || item.scope !== "repo") return false; + if (item.state !== "open" && item.state !== "draft") return false; + const headBranch = branchNameFromRef(item.headBranch); + if (!headBranch) return false; + return !lanes.some((lane) => !lane.archivedAt && branchNameFromRef(lane.branchRef) === headBranch); +} + +function sameGitHubPr(left: GitHubPrListItem, right: GitHubPrListItem): boolean { + return left.repoOwner === right.repoOwner + && left.repoName === right.repoName + && Number(left.githubPrNumber) === Number(right.githubPrNumber); +} + +export function patchSnapshotWithMappedPr( + snapshot: GitHubPrSnapshot, + item: GitHubPrListItem, + args: { + mappedPrId: string; + laneId: string | null; + laneName: string | null; + }, +): GitHubPrSnapshot { + const patchItems = (items: GitHubPrListItem[]) => items.map((candidate) => { + if (candidate.id !== item.id && !sameGitHubPr(candidate, item)) return candidate; + return { + ...candidate, + linkedPrId: args.mappedPrId, + linkedLaneId: args.laneId ?? candidate.linkedLaneId, + linkedLaneName: args.laneName ?? candidate.linkedLaneName, + adeKind: candidate.adeKind ?? "single", + }; + }); + return { + ...snapshot, + repoPullRequests: patchItems(snapshot.repoPullRequests), + externalPullRequests: patchItems(snapshot.externalPullRequests), + }; +} + +export function CreateLaneFromPrBranchDialog({ + item, + preflight, + loading, + busy, + error, + onCancel, + onConfirm, +}: { + item: GitHubPrListItem; + preflight: CreateLaneFromPrBranchPreflight | null; + loading: boolean; + busy: boolean; + error: string | null; + onCancel: () => void; + onConfirm: () => void; +}) { + const blockingConflict = preflightBlockingConflict(preflight); + const canConfirm = Boolean(preflight?.canCreate) && !loading && !busy; + const sourceBranch = preflightRemoteBranch(preflight, item); + const importRef = preflightImportRef(preflight); + const rows = [ + ["PR", `#${preflightPrNumber(preflight, item)} ${preflightTitle(preflight, item)}`], + ["Source branch", sourceBranch], + ...(importRef && importRef !== sourceBranch ? [["Import ref", importRef] as const] : []), + ["Target lane", preflightTargetLaneName(preflight, item)], + ["Base branch", preflightBaseBranch(preflight, item)], + ] as const; + + return ( +
+
+
+
+ Create lane from PR branch +
+
+
+ {loading ? ( +
+ Checking branch ownership and PR head availability... +
+ ) : ( +
+ {rows.map(([label, value]) => ( +
+
{label}
+
+ {value} +
+
+ ))} +
+ )} + {blockingConflict ? ( +
+ + {blockingConflict} +
+ ) : null} + {error ? ( +
+ {error} +
+ ) : null} +
+
+ + +
+
+
+ ); +} diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx new file mode 100644 index 000000000..e4b228ff2 --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx @@ -0,0 +1,739 @@ +// @vitest-environment jsdom + +import React from "react"; +import { act, render, screen, waitFor, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { + CreateLaneFromPrBranchPreflightResult, + GitHubPrSnapshot, +} from "../../../../shared/types"; + +vi.mock("react-resizable-panels", async () => { + const harness = await import("./GitHubTab.testHarness"); + return { + Group: harness.MockPanelGroup, + Panel: harness.MockPanel, + Separator: harness.MockSeparator, + }; +}); + +vi.mock("../state/PrsContext", async () => { + const { mockUsePrs } = await import("./GitHubTab.testHarness"); + return { usePrs: () => mockUsePrs() }; +}); + +vi.mock("../detail/PrDetailPane", async () => { + const { MockPrDetailPane } = await import("./GitHubTab.testHarness"); + return { PrDetailPane: MockPrDetailPane }; +}); + +import { GitHubTabPrRow } from "../shared/GitHubTabPrRow"; +import { GitHubTab } from "./GitHubTab"; +import { + cleanupGitHubTabTest, + renderGitHubTab, + setupGitHubTabTest, +} from "./GitHubTab.testHarness"; +import { + createDeferred, + makeGitHubPr, + makeLaneSummary, + makePreflightResult, + snapshot, +} from "./GitHubTab.testFixtures"; + +describe("GitHubTab rows and mapping", () => { + beforeEach(() => { + setupGitHubTabTest(); + }); + + afterEach(() => { + cleanupGitHubTabTest(); + }); + + function renderTab(overrides: Parameters[1] = {}) { + return renderGitHubTab(GitHubTab, overrides); + } + + it("keeps the GitHub action outside the row button and does not expose a lane id as its label", async () => { + const user = userEvent.setup(); + const onSelect = vi.fn(); + const item = makeGitHubPr({ + linkedLaneId: "lane-internal-uuid", + linkedLaneName: null, + }); + const { container } = render( + , + ); + + expect(screen.queryByText("lane-internal-uuid")).toBeNull(); + expect(screen.queryByText("unmapped")).toBeNull(); + + const rowButton = container.querySelector('[data-tour="prs.listRow"]'); + const githubButton = screen.getByRole("button", { name: "View on GitHub" }); + expect(rowButton).not.toBeNull(); + expect(rowButton?.contains(githubButton)).toBe(false); + + await user.click(rowButton!); + expect(onSelect).toHaveBeenCalledWith(item); + await user.click(githubButton); + expect(window.ade.app.openExternal).toHaveBeenCalledWith(item.githubUrl); + expect(onSelect).toHaveBeenCalledTimes(1); + }); + + it("shows a running CI indicator for PR cards with pending checks", async () => { + renderTab(); + + await waitFor(() => { + expect(screen.getAllByLabelText("CI running").length).toBeGreaterThan(0); + }); + }); + + it("shows linked and unmapped PRs together under the status tabs", async () => { + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + ...snapshot.repoPullRequests, + makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + title: "Unlinked PR", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); + renderTab(); + + await waitFor(() => { + expect(screen.getByText("Open PR")).not.toBeNull(); + expect(screen.getByText("Unlinked PR")).not.toBeNull(); + }); + }); + + it("marks unlinked PRs as unmapped", async () => { + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + ...snapshot.repoPullRequests, + makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + title: "Unlinked PR", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); + renderTab(); + + await waitFor(() => { + expect(screen.getByText("Unlinked PR")).not.toBeNull(); + }); + expect(screen.getAllByText("unmapped").length).toBeGreaterThan(0); + }); + + it("does not mark unlinked PRs as unmapped in the merged bucket", async () => { + const user = userEvent.setup(); + const snapshotWithMergedUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + ...snapshot.repoPullRequests, + makeGitHubPr({ + id: "repo-merged-unlinked", + githubPrNumber: 201, + title: "Merged after lane deleted", + state: "merged", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-12T12:00:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithMergedUnlinked); + renderTab(); + + await user.click(await screen.findByRole("button", { name: /merged/i })); + await waitFor(() => { + expect(screen.getByText("Merged after lane deleted")).not.toBeNull(); + }); + // Mapping is a live-work concept: on a merged PR the lane is gone and mapping one + // would do nothing, so the badge must not appear. + expect(screen.queryByText("unmapped")).toBeNull(); + }); + + it("shows frozen lane provenance on a detached merged PR", async () => { + const user = userEvent.setup(); + const snapshotWithDetached: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + ...snapshot.repoPullRequests, + makeGitHubPr({ + id: "repo-detached", + githubPrNumber: 202, + title: "Shipped from a deleted lane", + state: "merged", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-11T12:00:00.000Z", + detached: { + at: "2026-03-12T09:00:00.000Z", + laneName: "auto-naming", + laneColor: "#4ADE80", + chats: 3, + artifacts: 2, + checkpoints: 5, + }, + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithDetached); + renderTab(); + + await user.click(await screen.findByRole("button", { name: /merged/i })); + await waitFor(() => { + expect(screen.getByText("Shipped from a deleted lane")).not.toBeNull(); + }); + expect(screen.getByText("was: auto-naming")).not.toBeNull(); + expect(screen.getByText("· 3 chats · 2 proof")).not.toBeNull(); + expect(screen.queryByText("unmapped")).toBeNull(); + }); + + it("shows merge facts instead of CI and review signals on a merged row", async () => { + const user = userEvent.setup(); + const snapshotWithMerged: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + ...snapshot.repoPullRequests, + makeGitHubPr({ + id: "repo-merged-facts", + githubPrNumber: 203, + title: "Merged with facts", + state: "merged", + baseBranch: "main", + mergedAt: "2026-03-12T10:00:00.000Z", + mergedBy: { login: "arul", avatarUrl: null }, + mergeMethod: "squash", + createdAt: "2026-03-10T12:00:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithMerged); + renderTab(); + + await user.click(await screen.findByRole("button", { name: /merged/i })); + await waitFor(() => { + expect(screen.getByText("Merged with facts")).not.toBeNull(); + }); + expect(screen.getByText("arul · squash · → main")).not.toBeNull(); + }); + + it("renders bot badge when isBot is true", async () => { + const snapshotWithBot: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "bot-pr", + githubPrNumber: 300, + title: "Bot PR", + author: "dependabot[bot]", + isBot: true, + createdAt: "2026-03-13T12:00:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithBot); + renderTab(); + + await waitFor(() => { + expect(screen.getByText("bot")).not.toBeNull(); + }); + }); + + it("renders labels when present", async () => { + const snapshotWithLabels: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "labeled-pr", + githubPrNumber: 400, + title: "Labeled PR", + labels: [ + { name: "bug", color: "d73a4a", description: null }, + { name: "enhancement", color: "a2eeef", description: null }, + ], + createdAt: "2026-03-13T12:00:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithLabels); + renderTab(); + + await waitFor(() => { + expect(screen.getByText("bug")).not.toBeNull(); + expect(screen.getByText("enhancement")).not.toBeNull(); + }); + }); + + it("renders comment count when greater than zero", async () => { + const snapshotWithComments: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "commented-pr", + githubPrNumber: 500, + title: "Commented PR", + commentCount: 42, + createdAt: "2026-03-13T12:00:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithComments); + renderTab(); + + await waitFor(() => { + expect(screen.getByText("42")).not.toBeNull(); + }); + }); + + it("sorts PRs by updatedAt descending", async () => { + const snapshotOrdered: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "pr-old", + githubPrNumber: 50, + title: "Old PR", + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:10:00.000Z", + }), + makeGitHubPr({ + id: "pr-new", + githubPrNumber: 150, + title: "New PR", + createdAt: "2026-03-13T08:00:00.000Z", + updatedAt: "2026-03-13T12:30:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotOrdered); + renderTab(); + + await waitFor(() => { + const buttons = screen.getAllByRole("button").filter((btn) => + btn.textContent?.includes("PR") && (btn.textContent?.includes("Old") || btn.textContent?.includes("New")), + ); + expect(buttons.length).toBe(2); + expect(buttons[0]!.textContent).toContain("New PR"); + expect(buttons[1]!.textContent).toContain("Old PR"); + }); + }); + + it("requires confirmation before unmapping a GitHub PR from its lane", async () => { + const user = userEvent.setup(); + const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(false); + try { + renderTab(); + + await waitFor(() => { + expect(screen.getByText("Open PR")).toBeTruthy(); + }); + await user.click(screen.getByRole("button", { name: /#101 Open PR/i })); + await waitFor(() => { + expect(screen.getByTestId("pr-detail-pane").textContent).toContain("pr-open"); + }); + await user.click(screen.getByRole("button", { name: /unmap from lane/i })); + + expect(confirmSpy).toHaveBeenCalledWith(expect.stringContaining("Unmap PR #101")); + expect(window.ade.prs.delete).not.toHaveBeenCalled(); + } finally { + confirmSpy.mockRestore(); + } + }); + + it("renders the full PR detail pane (with create/map affordance) for a selected unmapped PR", async () => { + const user = userEvent.setup(); + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "Unlinked PR", + headBranch: "feature/no-lane", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:05:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); + // No lane owns the PR head branch → the "Create lane from PR branch" action + // is offered (and there is no matching lane to map to). + renderTab({ lanes: [] }); + + await user.click(await screen.findByText("Unlinked PR")); + + // The full detail pane renders (not the legacy read-only gate), keyed by a + // stable synthetic id derived from the GitHub coordinates. + const pane = await screen.findByTestId("pr-detail-pane"); + expect(pane.getAttribute("data-unmapped")).toBe("true"); + expect(pane.textContent).toContain("gh:ade-dev/ade#200"); + + // The create/map affordance is present (no read-only gate). + const affordance = within(pane).getByTestId("pr-unmapped-affordance"); + expect(within(affordance).getByRole("button", { name: /create lane from pr branch/i })).toBeTruthy(); + }); + + it("maps an unmapped PR to a lane via the in-pane affordance", async () => { + const user = userEvent.setup(); + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "Unlinked PR", + headBranch: "feature/lane-match", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:05:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); + renderTab({ + lanes: [makeLaneSummary({ id: "lane-match", name: "Matching lane", branchRef: "refs/heads/feature/lane-match" })], + }); + + await user.click(await screen.findByText("Unlinked PR")); + const pane = await screen.findByTestId("pr-detail-pane"); + const affordance = within(pane).getByTestId("pr-unmapped-affordance"); + + await user.selectOptions(within(affordance).getByLabelText("Select lane to map"), "lane-match"); + await user.click(within(affordance).getByRole("button", { name: /^map$/i })); + + await waitFor(() => { + expect(window.ade.prs.linkToLane).toHaveBeenCalledWith({ + laneId: "lane-match", + prUrlOrNumber: "https://github.com/ade-dev/ade/pull/200", + }); + }); + }); + + it("opens a preflight dialog for an unmapped PR branch", async () => { + const user = userEvent.setup(); + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "Unlinked PR", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:05:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); + renderTab(); + + await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); + + expect(window.ade.prs.preflightCreateLaneFromPrBranch).toHaveBeenCalledWith({ + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 200, + }); + const dialog = await screen.findByRole("dialog", { name: /create lane from pr branch/i }); + expect(within(dialog).getByText(/#200 Unlinked PR/)).toBeTruthy(); + expect(within(dialog).getAllByText("origin/feature/open").length).toBeGreaterThan(0); + expect(within(dialog).getAllByText("Unlinked PR").length).toBeGreaterThan(0); + expect(within(dialog).getAllByText("main").length).toBeGreaterThan(0); + }); + + it("ignores stale create-lane preflight results from a previous PR", async () => { + const user = userEvent.setup(); + const firstPreflight = createDeferred(); + const secondPreflight = createDeferred(); + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "repo-unlinked-first", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "First PR", + headBranch: "feature/first", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:10:00.000Z", + }), + makeGitHubPr({ + id: "repo-unlinked-second", + githubPrNumber: 201, + githubUrl: "https://github.com/ade-dev/ade/pull/201", + title: "Second PR", + headBranch: "feature/second", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:05:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); + (window.ade.prs.preflightCreateLaneFromPrBranch as ReturnType) + .mockImplementation((args: { githubPrNumber: number }) => + args.githubPrNumber === 200 ? firstPreflight.promise : secondPreflight.promise); + renderTab(); + + await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); + expect(window.ade.prs.preflightCreateLaneFromPrBranch).toHaveBeenCalledWith({ + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 200, + }); + await user.click(screen.getByRole("button", { name: /cancel/i })); + await user.click(await screen.findByText("Second PR")); + await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); + expect(window.ade.prs.preflightCreateLaneFromPrBranch).toHaveBeenCalledWith({ + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 201, + }); + + await act(async () => { + firstPreflight.resolve(makePreflightResult({ + githubPrNumber: 200, + title: "First PR", + headBranch: "feature/first", + remoteBranch: "origin/feature/first", + })); + await firstPreflight.promise; + }); + expect(screen.queryByText(/#200 First PR/)).toBeNull(); + expect(screen.getByText(/checking branch ownership/i)).toBeTruthy(); + + await act(async () => { + secondPreflight.resolve(makePreflightResult({ + githubPrNumber: 201, + title: "Second PR", + headBranch: "feature/second", + remoteBranch: "origin/feature/second", + })); + await secondPreflight.promise; + }); + + expect(await screen.findByText(/#201 Second PR/)).toBeTruthy(); + expect(screen.queryByText(/#200 First PR/)).toBeNull(); + const secondDialog = await screen.findByRole("dialog", { name: /create lane from pr branch/i }); + expect(within(secondDialog).getAllByText("origin/feature/second").length).toBeGreaterThan(0); + }); + + it("shows blocking preflight conflicts before creating a lane", async () => { + const user = userEvent.setup(); + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + title: "Unlinked PR", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:05:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); + (window.ade.prs.preflightCreateLaneFromPrBranch as ReturnType).mockResolvedValueOnce({ + preflight: { + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "Unlinked PR", + headBranch: "feature/open", + headRepoOwner: "ade-dev", + headRepoName: "ade", + remoteBranch: "origin/feature/open", + importBranchRef: "origin/feature/open", + targetLaneName: "Unlinked PR", + baseBranch: "main", + canCreate: false, + status: "blocked", + blockingConflict: { + code: "branch_owned", + message: "Branch 'feature/open' is already owned by lane 'Existing lane'.", + laneId: "lane-existing", + laneName: "Existing lane", + }, + blockingConflicts: [], + }, + lane: null, + pr: null, + }); + renderTab(); + + await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); + + expect(await screen.findByText(/already owned by lane 'Existing lane'/i)).toBeTruthy(); + expect(screen.getByRole("button", { name: /^create lane$/i })).toHaveProperty("disabled", true); + expect(window.ade.prs.createLaneFromPrBranch).not.toHaveBeenCalled(); + }); + + it("does not let an archived branch match hide the create-lane action", async () => { + const user = userEvent.setup(); + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + title: "Unlinked PR", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:05:00.000Z", + }), + ], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); + (window.ade.prs.preflightCreateLaneFromPrBranch as ReturnType).mockResolvedValueOnce({ + preflight: { + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "Unlinked PR", + headBranch: "feature/open", + headRepoOwner: "ade-dev", + headRepoName: "ade", + remoteBranch: "origin/feature/open", + importBranchRef: "origin/feature/open", + targetLaneName: "Unlinked PR", + baseBranch: "main", + canCreate: false, + status: "blocked", + blockingConflict: { + code: "branch_owned", + message: "Branch 'feature/open' is already owned by archived lane 'Archived lane'.", + laneId: "lane-archived", + laneName: "Archived lane", + }, + blockingConflicts: [], + }, + lane: null, + pr: null, + }); + + renderTab({ + lanes: [ + makeLaneSummary({ + id: "lane-archived", + name: "Archived lane", + branchRef: "refs/heads/feature/open", + archivedAt: "2026-03-12T12:00:00.000Z", + }), + ], + }); + + expect(await screen.findByRole("button", { name: /create lane from pr branch/i })).toBeTruthy(); + expect(screen.queryByRole("option", { name: "Archived lane" })).toBeNull(); + + await user.click(screen.getByRole("button", { name: /create lane from pr branch/i })); + + expect(await screen.findByText(/already owned by archived lane 'Archived lane'/i)).toBeTruthy(); + expect(screen.getByRole("button", { name: /^create lane$/i })).toHaveProperty("disabled", true); + }); + + it("creates a lane from an unmapped PR branch, refreshes lanes, and selects the mapped PR", async () => { + const user = userEvent.setup(); + const onSelectPr = vi.fn(); + const onRefreshAll = vi.fn().mockResolvedValue(undefined); + const forcedSnapshot = createDeferred(); + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [ + makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + githubUrl: "https://github.com/ade-dev/ade/pull/200", + title: "Unlinked PR", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + createdAt: "2026-03-13T12:00:00.000Z", + updatedAt: "2026-03-13T12:05:00.000Z", + }), + ], + externalPullRequests: [], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType) + .mockResolvedValueOnce(snapshotWithUnlinked) + .mockReturnValueOnce(forcedSnapshot.promise); + renderTab({ onSelectPr, onRefreshAll }); + + await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); + await user.click(await screen.findByRole("button", { name: /^create lane$/i })); + + await waitFor(() => { + expect(window.ade.prs.createLaneFromPrBranch).toHaveBeenCalledWith({ + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 200, + }); + }); + await waitFor(() => { + expect(onSelectPr).toHaveBeenCalledWith("pr-created"); + }); + expect(onRefreshAll).toHaveBeenCalledWith({ prId: "pr-created" }); + expect(window.ade.lanes.list).toHaveBeenCalledWith({ + includeArchived: false, + includeStatus: false, + }); + expect(window.ade.prs.getGitHubSnapshot).toHaveBeenCalledWith({ force: true }); + await act(async () => { + forcedSnapshot.resolve(snapshotWithUnlinked); + await forcedSnapshot.promise; + }); + }); +}); diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabView.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabView.tsx new file mode 100644 index 000000000..e90a7d85e --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabView.tsx @@ -0,0 +1,521 @@ +import React from "react"; +import { CircleNotch, GitMerge, GithubLogo, XCircle } from "@phosphor-icons/react"; +import { useVirtualizer } from "@tanstack/react-virtual"; +import { Group, Panel } from "react-resizable-panels"; +import type { + GitHubPrListItem, + GitHubPrStack, + PrSummary, +} from "../../../../shared/types"; +import { EmptyState } from "../../ui/EmptyState"; +import { ResizeGutter } from "../../ui/ResizeGutter"; +import { + COLORS, + MONO_FONT, + SANS_FONT, + cardStyle, + outlineButton, + primaryButton, +} from "../../lanes/laneDesignTokens"; +import { PrDetailPane } from "../detail/PrDetailPane"; +import { GitHubPrSearchInput } from "../shared/GitHubPrSearchInput"; +import { GitHubRepoSyncBar } from "../shared/GitHubRepoSyncBar"; +import { GitHubStackInspector } from "../shared/GitHubStackInspector"; +import { GitHubTabPrRow, PrListGroupHeaderRow } from "../shared/GitHubTabPrRow"; +import { + prListHeaderIndices, + type PrListRow, +} from "../shared/prListGrouping"; +import { + GITHUB_TAB_VIRTUALIZE_AT, + bucketForState, + type GitHubFilter, + type GitHubFilterCounts, +} from "./githubTabModel"; + +const FILTER_ACCENTS: Record = { + open: "#60A5FA", + closed: "#A1A1AA", + merged: "#4ADE80", +}; + +const GITHUB_PR_LIST_WIDTH_KEY = "ade.prs.githubListWidth"; +const GITHUB_PR_LIST_MIN_PX = 260; +const GITHUB_PR_LIST_MAX_PX = 560; +const GITHUB_PR_LIST_DEFAULT_PX = 380; + +function readPersistedGithubPrListPx(): number { + try { + const raw = localStorage.getItem(GITHUB_PR_LIST_WIDTH_KEY); + if (raw) { + const value = Number(raw); + if (Number.isFinite(value) && value >= GITHUB_PR_LIST_MIN_PX && value <= GITHUB_PR_LIST_MAX_PX) { + return value; + } + } + } catch { + /* ignore */ + } + return GITHUB_PR_LIST_DEFAULT_PX; +} + +function persistGithubPrListPx(px: number): void { + try { + localStorage.setItem(GITHUB_PR_LIST_WIDTH_KEY, String(Math.round(px))); + } catch { + /* ignore */ + } +} + +type GitHubTabViewChrome = { + relocated: boolean; + searchQuery: string; + onSearchQueryChange: (value: string) => void; + repoLabel: string; + syncing: boolean; + syncedAt: string | null; + onSync: () => void; + error: string | null; + onConnectGitHub: () => void; +}; + +type GitHubTabViewList = { + parentRef: React.RefObject; + filter: GitHubFilter; + filterCounts: GitHubFilterCounts; + loading: boolean; + loadingFilter: GitHubFilter | null; + loadingOlderHistory: boolean; + showLoadingIndicator: boolean; + hasSnapshot: boolean; + filteredItems: GitHubPrListItem[]; + rows: PrListRow[]; + selectedItemId: string | null; + prsByIdMap: Map; + canLoadOlderHistory: boolean; + onFilterChange: (filter: GitHubFilter) => void; + onSelect: (item: GitHubPrListItem) => void; + onHydrationItemsChange: (items: GitHubPrListItem[]) => void; + onLoadOlderHistory: () => void; +}; + +type GitHubTabViewDetail = { + selectedItem: GitHubPrListItem | null; + selectedBucketMismatch: boolean; + selectedStack: GitHubPrStack | null; + displayedItems: GitHubPrListItem[]; + paneProps: React.ComponentProps | null; + onSelect: (item: GitHubPrListItem) => void; + onSync: () => void; + onAddStackPullRequests: (pullRequests: number[]) => Promise; + onUnstack: () => Promise; + onFilterChange: (filter: GitHubFilter) => void; +}; + +export type GitHubTabViewProps = { + chrome: GitHubTabViewChrome; + list: GitHubTabViewList; + detail: GitHubTabViewDetail; +}; + +export function GitHubTabView({ chrome, list, detail }: GitHubTabViewProps) { + const selectedItem = detail.selectedItem; + const selectedStack = detail.selectedStack; + const defaultListPx = React.useMemo(() => readPersistedGithubPrListPx(), []); + + if (chrome.error && !list.hasSnapshot) { + return ( + + + + ); + } + + return ( +
+ {!chrome.relocated ? ( +
+ + +
+ ) : null} + + {chrome.error ? ( +
+ {chrome.error} +
+ ) : null} + +
+ + persistGithubPrListPx(size.inPixels)} + className="min-h-0 min-w-0" + style={{ overflow: "hidden", borderRight: "1px solid rgba(255,255,255,0.06)" }} + > +
+
+ {(["open", "merged", "closed"] as GitHubFilter[]).map((state) => { + const active = list.filter === state; + const accent = FILTER_ACCENTS[state]; + const count = list.filterCounts[state]; + const icon = state === "merged" ? : null; + const tabLoading = active && ( + list.loading + || chrome.syncing + || list.loadingFilter === state + || list.loadingOlderHistory + ); + return ( + + ); + })} +
+ {list.showLoadingIndicator ? ( + + + + ) : null} +
+
+ {list.filteredItems.length === 0 ? ( +
+ +
+ ) : list.filteredItems.length > GITHUB_TAB_VIRTUALIZE_AT ? ( + + ) : ( + list.rows.map((row) => ( + row.kind === "header" ? ( + + ) : ( + + ) + )) + )} + {list.canLoadOlderHistory ? ( +
+ +
+ ) : null} +
+
+ + + + {selectedItem && detail.paneProps ? ( +
+ {detail.selectedBucketMismatch ? ( +
+ detail.onFilterChange(bucketForState(selectedItem.state))} + /> +
+ ) : null} + {selectedStack ? ( + item.repoOwner === selectedStack.repoOwner + && item.repoName === selectedStack.repoName, + )} + selectedPrNumber={selectedItem.githubPrNumber} + syncing={chrome.syncing} + onSelectPr={detail.onSelect} + onOpenGitHub={() => { + void window.ade.app.openExternal(selectedItem.githubUrl); + }} + onSync={detail.onSync} + onAddPullRequests={detail.onAddStackPullRequests} + onUnstack={detail.onUnstack} + /> + ) : null} +
+ +
+
+ ) : ( +
+ +
+ )} + + +
+
+ ); +} + +function PrBucketTransitionBanner({ + state, + onShow, +}: { + state: GitHubPrListItem["state"]; + onShow: () => void; +}) { + const isMerged = state === "merged"; + const label = isMerged ? "Merged" : "Closed"; + const accent = isMerged ? COLORS.success : COLORS.danger; + return ( +
+
+
+ {isMerged ? ( + + ) : ( + + )} + + This PR is now {label} + +
+ +
+
+ ); +} + +function GitHubTabVirtualList({ + parentRef, + rows, + selectedItemId, + prsByIdMap, + onSelect, + onHydrationItemsChange, +}: { + parentRef: React.RefObject; + rows: PrListRow[]; + selectedItemId: string | null; + prsByIdMap: Map; + onSelect: (item: GitHubPrListItem) => void; + onHydrationItemsChange: (items: GitHubPrListItem[]) => void; +}) { + const headerIndices = React.useMemo(() => prListHeaderIndices(rows), [rows]); + + const virtualizer = useVirtualizer({ + count: rows.length, + getScrollElement: () => parentRef.current, + estimateSize: (index) => (rows[index]?.kind === "header" ? 30 : 108), + overscan: 6, + rangeExtractor: React.useCallback( + (range: { startIndex: number; endIndex: number; overscan: number; count: number }) => { + const pinned = activeHeaderFor(headerIndices, range.startIndex); + const start = Math.max(0, range.startIndex - range.overscan); + const end = Math.min(range.count - 1, range.endIndex + range.overscan); + const indices = new Set(); + if (pinned != null) indices.add(pinned); + for (let index = start; index <= end; index += 1) indices.add(index); + return [...indices].sort((a, b) => a - b); + }, + [headerIndices], + ), + }); + + const virtualItems = virtualizer.getVirtualItems(); + const activeHeaderIndex = activeHeaderFor(headerIndices, virtualizer.range?.startIndex ?? 0); + + React.useEffect(() => { + onHydrationItemsChange( + virtualItems + .map((virtualRow) => rows[virtualRow.index]) + .filter((row): row is Extract => row?.kind === "item") + .map((row) => row.item), + ); + }, [rows, onHydrationItemsChange, virtualItems]); + + return ( +
+ {virtualItems.map((virtualRow) => { + const row = rows[virtualRow.index]!; + const pinned = row.kind === "header" && virtualRow.index === activeHeaderIndex; + return ( +
+ {row.kind === "header" ? ( + + ) : ( + + )} +
+ ); + })} +
+ ); +} + +function activeHeaderFor(headerIndices: number[], startIndex: number): number | null { + let active: number | null = null; + for (const index of headerIndices) { + if (index > startIndex) break; + active = index; + } + return active; +} diff --git a/apps/desktop/src/renderer/components/prs/tabs/githubTabModel.ts b/apps/desktop/src/renderer/components/prs/tabs/githubTabModel.ts new file mode 100644 index 000000000..52164585b --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/tabs/githubTabModel.ts @@ -0,0 +1,231 @@ +import type { + GitHubPrListItem, + GitHubPrSnapshot, + PrSummary, + PrWithConflicts, +} from "../../../../shared/types"; +import { syntheticGithubPrId } from "../../../../shared/types/prs"; +import { isTerminalPrState } from "../../../lib/prState"; + +export const LINKED_HYDRATION_LIMIT = 8; +export const GITHUB_TAB_VIRTUALIZE_AT = 50; +export const GITHUB_TAB_REVISIT_CACHE_TTL_MS = 60_000; +export const GITHUB_TAB_SNAPSHOT_FRESH_MS = 30_000; +export const GITHUB_TAB_HOT_REFRESH_DELAY_MS = 30_000; +export const GITHUB_TAB_HISTORY_INITIAL_PAGE_LIMIT = 2; +export const GITHUB_TAB_HISTORY_PAGE_INCREMENT = 2; +export const GITHUB_TAB_HISTORY_MAX_PAGE_LIMIT = 10; + +const GITHUB_TAB_CACHE_DISABLED = import.meta.env.MODE === "test"; + +export type GitHubFilter = "open" | "closed" | "merged"; +export type GitHubFilterSelectionMap = Partial>; + +export type GitHubTabWarmCache = { + projectRoot: string; + snapshot: GitHubPrSnapshot | null; + filter: GitHubFilter; + selectedItemId: string | null; + selectedItemIdsByFilter?: GitHubFilterSelectionMap; + searchQuery: string; + externalHistoryLoaded: boolean; + cachedAt: number; +}; + +export type GitHubSnapshotRequestKey = { + includeExternalClosed: boolean; + historyPageLimit: number; +}; + +export type GitHubFilterCounts = Record; + +let githubTabWarmCache: GitHubTabWarmCache | null = null; + +export function normalizeGitHubFilter(value: unknown): GitHubFilter { + return value === "open" || value === "closed" || value === "merged" ? value : "open"; +} + +export function initialGitHubFilterSelections(cache: GitHubTabWarmCache | null): GitHubFilterSelectionMap { + const selections: GitHubFilterSelectionMap = { ...(cache?.selectedItemIdsByFilter ?? {}) }; + if (cache?.selectedItemId) { + selections[normalizeGitHubFilter(cache.filter)] = cache.selectedItemId; + } + return selections; +} + +export function normalizeHistoryPageLimit(value: unknown): number { + const numeric = Number(value); + if (!Number.isFinite(numeric) || numeric <= 0) { + return GITHUB_TAB_HISTORY_INITIAL_PAGE_LIMIT; + } + return Math.min( + GITHUB_TAB_HISTORY_MAX_PAGE_LIMIT, + Math.max(GITHUB_TAB_HISTORY_INITIAL_PAGE_LIMIT, Math.floor(numeric)), + ); +} + +export function snapshotRequestKey(options?: { + includeExternalClosed?: boolean; + historyPageLimit?: number; +}): GitHubSnapshotRequestKey { + const includeExternalClosed = options?.includeExternalClosed === true; + return { + includeExternalClosed, + historyPageLimit: includeExternalClosed ? normalizeHistoryPageLimit(options?.historyPageLimit) : 0, + }; +} + +export function snapshotRequestSatisfies( + current: GitHubSnapshotRequestKey | null, + requested: GitHubSnapshotRequestKey, +): boolean { + if (!current) return false; + if (!requested.includeExternalClosed) return true; + return current.includeExternalClosed && current.historyPageLimit >= requested.historyPageLimit; +} + +export function readGitHubTabWarmCache(projectRoot: string | null): GitHubTabWarmCache | null { + if (GITHUB_TAB_CACHE_DISABLED) return null; + if (!projectRoot) return null; + if (githubTabWarmCache?.projectRoot !== projectRoot) return null; + const filter = normalizeGitHubFilter((githubTabWarmCache as { filter?: unknown }).filter); + return filter === githubTabWarmCache.filter ? githubTabWarmCache : { ...githubTabWarmCache, filter }; +} + +export function writeGitHubTabWarmCache(cache: GitHubTabWarmCache): void { + if (GITHUB_TAB_CACHE_DISABLED) return; + if (!cache.projectRoot) return; + githubTabWarmCache = cache; +} + +export function formatGitHubSnapshotError(err: unknown): string { + const raw = err instanceof Error ? err.message : String(err ?? ""); + const message = raw + .replace(/^Error invoking remote method '[^']+':\s*/i, "") + .replace(/^Error:\s*/i, "") + .trim(); + if (/github (token|auth) missing/i.test(message)) { + return "Connect GitHub in Settings with gh auth or a PAT to sync pull requests."; + } + return message || "Unable to sync pull requests."; +} + +function isKnownPrState(value: unknown): value is PrSummary["state"] { + return value === "draft" || value === "open" || value === "merged" || value === "closed"; +} + +export function reconcileLinkedPrState( + item: GitHubPrListItem, + linkedPr: PrSummary | null | undefined, +): GitHubPrListItem { + if (!isKnownPrState(linkedPr?.state)) return item; + if (!isTerminalPrState(linkedPr.state) || isTerminalPrState(item.state)) return item; + return { + ...item, + state: linkedPr.state, + isDraft: false, + title: linkedPr.title || item.title, + updatedAt: linkedPr.updatedAt || item.updatedAt, + }; +} + +export function matchesFilter(item: GitHubPrListItem, filter: GitHubFilter): boolean { + if (filter === "open") return item.state === "open" || item.state === "draft"; + return item.state === filter; +} + +export function bucketForState(state: GitHubPrListItem["state"]): GitHubFilter { + if (state === "merged") return "merged"; + if (state === "closed") return "closed"; + return "open"; +} + +export function githubCoordKey(item: { + repoOwner: string; + repoName: string; + githubPrNumber: number; +}): string { + return `${item.repoOwner}/${item.repoName}#${Number(item.githubPrNumber)}`; +} + +function buildOverlayRowFromLastSeen(lastSeen: GitHubPrListItem, linkedPr: PrSummary): GitHubPrListItem { + return { + ...lastSeen, + state: linkedPr.state, + isDraft: false, + title: linkedPr.title || lastSeen.title, + updatedAt: linkedPr.updatedAt || lastSeen.updatedAt, + linkedPrId: linkedPr.id, + }; +} + +export function computeTerminalOverlayItems( + reconciledItems: GitHubPrListItem[], + prsById: Map, + lastSeenByCoord: Map, +): GitHubPrListItem[] { + if (prsById.size === 0) return []; + const presentCoords = new Set(reconciledItems.map((item) => githubCoordKey(item))); + const overlays: GitHubPrListItem[] = []; + const usedLinkedIds = new Set(); + for (const pr of prsById.values()) { + if (!isTerminalPrState(pr.state)) continue; + if (usedLinkedIds.has(pr.id)) continue; + const key = githubCoordKey(pr); + if (presentCoords.has(key)) continue; + const lastSeen = lastSeenByCoord.get(key); + if (!lastSeen) continue; + usedLinkedIds.add(pr.id); + overlays.push(buildOverlayRowFromLastSeen(lastSeen, pr)); + } + return overlays; +} + +export function mergeGitHubListItems(snapshot: GitHubPrSnapshot): GitHubPrListItem[] { + const combined = [...snapshot.repoPullRequests, ...snapshot.externalPullRequests]; + const seen = new Set(); + return combined.filter((item) => { + const key = `${item.scope}:${item.repoOwner}/${item.repoName}#${item.githubPrNumber}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + }); +} + +export function countGitHubItemsByState(items: GitHubPrListItem[]): GitHubFilterCounts { + return { + open: items.filter((item) => item.state === "open" || item.state === "draft").length, + closed: items.filter((item) => item.state === "closed").length, + merged: items.filter((item) => item.state === "merged").length, + }; +} + +export function syntheticUnmappedPrId(item: GitHubPrListItem): string { + return syntheticGithubPrId(item); +} + +export function buildSyntheticUnmappedPr(item: GitHubPrListItem, projectId: string): PrWithConflicts { + return { + id: syntheticUnmappedPrId(item), + laneId: "", + projectId, + repoOwner: item.repoOwner, + repoName: item.repoName, + githubPrNumber: item.githubPrNumber, + githubUrl: item.githubUrl, + githubNodeId: null, + title: item.title, + state: item.state, + baseBranch: item.baseBranch ?? "", + headBranch: item.headBranch ?? "", + checksStatus: "none", + reviewStatus: "none", + additions: 0, + deletions: 0, + lastSyncedAt: null, + createdAt: item.createdAt, + updatedAt: item.updatedAt, + stack: item.stack ?? null, + conflictAnalysis: null, + }; +} diff --git a/apps/desktop/src/renderer/components/prs/tabs/useGitHubTabListModel.ts b/apps/desktop/src/renderer/components/prs/tabs/useGitHubTabListModel.ts new file mode 100644 index 000000000..dfcf67ce6 --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/tabs/useGitHubTabListModel.ts @@ -0,0 +1,117 @@ +import React from "react"; +import type { + GitHubPrListItem, + GitHubPrSnapshot, + PrSummary, +} from "../../../../shared/types"; +import { buildPrListRows } from "../shared/prListGrouping"; +import { + GITHUB_TAB_HISTORY_MAX_PAGE_LIMIT, + GITHUB_TAB_VIRTUALIZE_AT, + computeTerminalOverlayItems, + countGitHubItemsByState, + githubCoordKey, + matchesFilter, + mergeGitHubListItems, + reconcileLinkedPrState, + type GitHubFilter, + type GitHubFilterCounts, +} from "./githubTabModel"; + +export function useGitHubTabListModel({ + snapshot, + searchQuery, + prsByIdMap, + filter, + renderedHydrationItems, + lastSeenRowByCoordRef, + currentHistoryPageLimit, +}: { + snapshot: GitHubPrSnapshot | null; + searchQuery: string; + prsByIdMap: Map; + filter: GitHubFilter; + renderedHydrationItems: GitHubPrListItem[]; + lastSeenRowByCoordRef: React.MutableRefObject>; + currentHistoryPageLimit: () => number; +}) { + const matchesSearch = React.useCallback((item: GitHubPrListItem) => { + if (!searchQuery.trim()) return true; + const q = searchQuery.trim().toLowerCase(); + return ( + item.title.toLowerCase().includes(q) + || (item.author?.toLowerCase().includes(q) ?? false) + || (item.headBranch?.toLowerCase().includes(q) ?? false) + || String(item.githubPrNumber).includes(q) + ); + }, [searchQuery]); + + const allItems = React.useMemo( + () => (snapshot ? mergeGitHubListItems(snapshot) : []), + [snapshot], + ); + const reconciledItems = React.useMemo( + () => allItems.map((item) => + reconcileLinkedPrState(item, item.linkedPrId ? prsByIdMap.get(item.linkedPrId) : null) + ), + [allItems, prsByIdMap], + ); + + React.useEffect(() => { + const map = lastSeenRowByCoordRef.current; + for (const item of allItems) { + map.set(githubCoordKey(item), item); + } + }, [allItems, lastSeenRowByCoordRef]); + + const overlayItems = React.useMemo( + () => computeTerminalOverlayItems(reconciledItems, prsByIdMap, lastSeenRowByCoordRef.current), + [lastSeenRowByCoordRef, reconciledItems, prsByIdMap], + ); + const displayedItems = React.useMemo( + () => (overlayItems.length === 0 ? reconciledItems : [...reconciledItems, ...overlayItems]), + [reconciledItems, overlayItems], + ); + const filteredItems = React.useMemo( + () => displayedItems + .filter((item) => matchesFilter(item, filter) && matchesSearch(item)) + .sort((a, b) => + new Date(b.updatedAt || b.createdAt).getTime() - new Date(a.updatedAt || a.createdAt).getTime() + ), + [displayedItems, filter, matchesSearch], + ); + const hydrationItems = filteredItems.length > GITHUB_TAB_VIRTUALIZE_AT + ? renderedHydrationItems + : filteredItems; + const listRows = React.useMemo( + () => buildPrListRows(filteredItems, { grouped: filter === "merged" || filter === "closed" }), + [filteredItems, filter], + ); + const filterCounts = React.useMemo(() => { + const listedCounts = countGitHubItemsByState(displayedItems); + const snapshotCounts = snapshot?.history?.repoPullRequestCounts; + const rawCounts = countGitHubItemsByState(allItems); + const withReconcileDelta = ( + base: number | null | undefined, + key: keyof GitHubFilterCounts, + fallback: number, + ): number => base == null ? fallback : Math.max(0, base + listedCounts[key] - rawCounts[key]); + return { + open: withReconcileDelta(snapshotCounts?.open, "open", listedCounts.open), + closed: withReconcileDelta(snapshotCounts?.closed, "closed", listedCounts.closed), + merged: withReconcileDelta(snapshotCounts?.merged, "merged", listedCounts.merged), + }; + }, [allItems, displayedItems, snapshot?.history?.repoPullRequestCounts]); + const canLoadOlderHistory = filter !== "open" + && Boolean(snapshot?.history?.repoPullRequestsMayHaveMore) + && currentHistoryPageLimit() < GITHUB_TAB_HISTORY_MAX_PAGE_LIMIT; + + return { + displayedItems, + filteredItems, + hydrationItems, + listRows, + filterCounts, + canLoadOlderHistory, + }; +} diff --git a/docs/playbooks/ship-lane.md b/docs/playbooks/ship-lane.md index 763db73be..ba2818499 100644 --- a/docs/playbooks/ship-lane.md +++ b/docs/playbooks/ship-lane.md @@ -15,7 +15,7 @@ Run this playbook once per lane, when the code on the branch is done (or nearly ## Execution contract - **Autonomous.** Do not pause for user confirmation mid-loop. -- **Bounded with a force-finalize escape hatch.** Soft cap: 5 normal iterations of fix-and-poll. Exit earlier if clean or blocked. **At the cap, the loop must land the lane**: if the PR is not merged after iteration 5, run **one** additional force-finalize iteration (Phase 3d) that ignores all open review comments, fixes only CI failures so every required check goes green, then routes through Phase 3c (auto-merge). Only if iteration 6 cannot make CI green, or Phase 3c is genuinely blocked (base-branch policy + no admin rights + no auto-merge enabled), do you stop and leave a handoff comment for a human. The playbook's exit contract is "PR merged into `main`, or merge genuinely impossible" — never "PR green and parked". +- **Bounded with a force-finalize escape hatch.** Soft cap: 5 normal iterations of fix-and-poll. Exit earlier if clean or blocked. **At the cap, the loop must land the lane**: if the PR is not merged after iteration 5, run **one** additional force-finalize iteration (Phase 3d) that ignores all open review comments, fixes only CI failures so every required check goes green, then routes through Phase 3c. Only if iteration 6 cannot make CI green, or Phase 3c is genuinely blocked by base-branch policy with no authorized direct/admin path, do you stop and leave a handoff comment for a human. The playbook's exit contract is "PR merged into `main`, or merge genuinely impossible" — never "PR green and parked". - **Rebase budget rebate.** A rebase, merge-from-main, or conflict-resolution pass moves the current iteration count down by 2 before the next cap check, with a floor of 0. Example: if the lane is on iteration 4 and must rebase because `main` moved, record the rebase and continue as iteration 2. - **Scoped checks.** Never run the full test suite between iterations. For CI, fix and rerun only the failing test file(s) or failing check target. For review-only changes, rerun only directly affected existing tests, plus the narrow package typecheck/lint when the touched surface needs it. - **One push per iteration. Wait for BOTH signals before fixing anything.** Never push a CI-only fix while review bots are still running, and never push a review-only fix while CI is still running. Both signals must be **terminal** before the iteration commits — that is, every required check has a final conclusion AND every review bot with current-head start evidence has posted or settled. This is not just an efficiency rule: **review-comment fixes routinely introduce new CI failures**, so applying them on a partial signal means the next push fails and you've thrown away the prior CI cycle. Wait for both, then dispatch ci-fix-agent and review-fix-agent in parallel with full knowledge of both, and combine their edits into one commit. If only one signal has landed when you wake, do not iterate — reschedule and sleep. @@ -80,9 +80,8 @@ These are operational mistakes this playbook explicitly guards against: UTC timestamps. Normalize before comparing in `jq`, otherwise old comments can look new and trigger duplicate review-fix work. 3. **`done-clean` still merges.** A green PR with resolved review - comments should route through Phase 3c auto-merge. Only mark - `done-max` when normal merge, admin merge, and auto-merge are all - genuinely blocked. + comments should route through Phase 3c. Only mark `done-max` when normal and + authorized admin merge paths are genuinely blocked. 4. **Do not pass `--delete-branch` to `gh pr merge`.** It can try to checkout the base branch locally and fail when `main` is already checked out by another worktree. Delete the remote head ref after a @@ -106,6 +105,7 @@ Path: `.ade/shipLane/.json` (sanitize by replacing `/` with `_ "prNumber": 1234, "iteration": 2, "lastPushSha": "abc123...", + "qualityValidatedSha": "abc123...", "addressedCommentIds": [987654, 987655], "status": "running", "startedAt": "2026-04-23T14:30:00Z", @@ -118,6 +118,48 @@ Path: `.ade/shipLane/.json` (sanitize by replacing `/` with `_ The `iteration` value is the active turn budget counter, not a raw count of pushes. Normal fix iterations increment it by 1. Rebase/merge/conflict recovery decrements it by 2 first, then the current pass records its result. Never let it go below 0. +## Commit-bound quality revalidation + +This is the single canonical procedure for every ship-loop mutation. Phase 0, +post-rebase, Phase 3b, and force-finalize supply only their commit message, diff +base, narrow test targets, and push command; they do not restate this algorithm. + +1. Clear `qualityValidatedSha` before editing. +2. Run both `/quality` tracks on the final combined diff, fix every accepted + finding, and repeat both tracks until the same pass is clean. +3. Build the validation scope from the union of committed, unstaged, staged, + and untracked changes, so quality-created files cannot evade narrow tests: + + ```bash + CHANGED=$( + { + git diff --name-only "${QUALITY_DIFF_BASE:-HEAD}...HEAD" + git diff --name-only + git diff --cached --name-only + git ls-files --others --exclude-standard + } | sort -u + ) + ``` + + Run only the directly affected tests/checks required by Fix discipline. +4. Stage the clean reviewed tree, capture its identity, commit once, and prove + the commit contains exactly that tree: + + ```bash + git add -A + QUALITY_VALIDATED_TREE=$(git write-tree) + git diff --cached --quiet || git commit -m "$QUALITY_COMMIT_MESSAGE" + test "$(git rev-parse HEAD^{tree})" = "$QUALITY_VALIDATED_TREE" + QUALITY_VALIDATED_SHA=$(git rev-parse HEAD) + ``` + +5. Push with the phase-appropriate command, then verify the PR's `headRefOid` + equals `QUALITY_VALIDATED_SHA` before writing it to ship state. Any later edit + or head mismatch clears the binding and restarts this procedure. + +The exact-SHA merge check in Phase 3c is the second half of this contract; a +green CI result never substitutes for it. + --- ## Phase 0 — Setup (first invocation only) @@ -137,11 +179,12 @@ If a PR exists for the current branch, skip to 0.4 (bot pings) with `prNumber` c ### 0.2 Pre-push expectation (no existing PR) -Ship is a **pure merge loop** — it does NOT run `/quality`, `/test`, or -`/finalize`. Those are separate steps in the dev loop +Ship is a **pure merge loop** — it does not replace the baseline `/quality`, +`/test`, or `/finalize` runs. Those are separate steps in the dev loop (`/context → work → /quality → /test → /ship`) that the author runs *before* reaching ship. Ship assumes the lane is already reviewed, tested, and -(optionally) finalized; it never generates tests or simplifies code itself. +(optionally) finalized; its only quality work is revalidation after a ship-loop +mutation so the result stays bound to the exact commit being merged. Preconditions here: @@ -152,18 +195,32 @@ Preconditions here: result from the current conversation or lane handoff. A non-empty gate, a missing result, or an ambiguous placeholder row blocks ship; set `exitReason: "quality-gate-nonempty"` or `"quality-result-missing"` rather - than assuming unknown means clean. Ship does not run either skill itself. + than assuming unknown means clean. Ship does not replace either baseline + skill run. +- The quality result must be bound to the exact commit that will merge by the + canonical Commit-bound quality revalidation procedure above. Do not infer a + binding from a green gate summary that predates the commit. ### 0.3 Commit + push + create PR -Commit and push first — both paths below need the remote branch to exist: +Create the checkpoint commit first: ```bash git add -A git diff --cached --quiet || git commit -m "ship: prepare lane for review" -git push -u origin "$CURRENT_BRANCH" ``` +Run the canonical Commit-bound quality revalidation procedure with the initial +checkpoint as `QUALITY_DIFF_BASE`, the commit message below, and +`git push -u origin "$CURRENT_BRANCH"` as the push command: + +```bash +QUALITY_COMMIT_MESSAGE="ship: apply initial quality revalidation" +``` + +If anything changes after the clean pass, discard both captured values and +revalidate again. + **PR creation: prefer the ADE CLI.** Opening the PR via `ade` registers it in ADE's PR tracking (lane ↔ PR link, check/comment inventory, review-thread state). `gh pr create` is the fallback, not the default. Falling back too eagerly defeats the purpose — do it only after you've genuinely confirmed the ADE path is broken. ### Discovery protocol (for the agent — not a script) @@ -204,12 +261,16 @@ only when the diff touches more than 250 files. ### 0.5 Write initial state +After PR creation, verify its `headRefOid` equals `QUALITY_VALIDATED_SHA`. If it +does not, leave `qualityValidatedSha` null and revalidate the actual PR head. + ```json { "branch": "", "prNumber": , "iteration": 0, "lastPushSha": "", + "qualityValidatedSha": "", "addressedCommentIds": [], "status": "running", "startedAt": "", @@ -328,10 +389,11 @@ Pure logic on the poll summary: | Condition | Action | | --- | --- | -| `merged == true` | Exit `done-clean`; clear state file. | +| `merged == true` AND `pollHeadSha == qualityValidatedSha` | Exit `done-clean`; clear state file. | +| `merged == true` AND the SHAs differ | Exit `blocked` with `merged-unvalidated-head`; never report stale quality as clean. | | `behindMain == true` | Go to Phase 3a (rebase), apply the rebase budget rebate, then schedule/poll according to Phase 5. | | `ciRunning == true` OR `reviewBotsRunning == true` | Do NOT iterate on a partial signal. Go to Phase 5 (schedule next wake). This applies even if the other signal already shows failures/comments — pushing a fix now means the next CI+review cycle races the fix and you likely re-push for the other half. | -| `ciFailed` empty, `newComments` empty, `ciRunning == false`, `reviewBotsRunning == false` | Go to **Phase 3c (auto-merge)**. Done-clean does not mean "stop and leave for human" — it means everything is green, and the lane should land on `main`. | +| `ciFailed` empty, `newComments` empty, `ciRunning == false`, `reviewBotsRunning == false` | Go to **Phase 3c**. Done-clean does not mean "stop and leave for human" — it means everything is green, and the lane should land on `main`. | | Otherwise (both signals terminal, fix work exists) | Go to Phase 3b (fix). Fix CI failures and review comments **in the same iteration / same push**. | --- @@ -361,28 +423,37 @@ Resolve merge conflicts the same way. If the merge is **still** unrecoverable, e ### Post-resolution validation -Before pushing, run tests scoped to touched files only: +The rebase or merge invalidates the old quality binding. Run the canonical +Commit-bound quality revalidation procedure with `ORIG_HEAD` as +`QUALITY_DIFF_BASE`, the post-rebase commit message, and +`git push --force-with-lease` as the push command. Use its `CHANGED` union for +the scoped checks below: ```bash -# Touched since rebase started -CHANGED=$(git diff --name-only ORIG_HEAD HEAD | grep -E '\.(ts|tsx)$') - # Run colocated test files that exist -for f in $CHANGED; do - TEST="${f%.ts}.test.ts" - [ -f "$TEST" ] && echo "$TEST" -done | sort -u | xargs -r -I{} sh -c 'cd apps/desktop && npx vitest run {}' +for f in $(echo "$CHANGED" | grep -E '\.(ts|tsx)$'); do + case "$f" in + *.test.ts|*.test.tsx) echo "$f" ;; + *.tsx) + TEST="${f%.tsx}.test.tsx" + [ -f "$TEST" ] && echo "$TEST" + ;; + *.ts) + TEST="${f%.ts}.test.ts" + [ -f "$TEST" ] && echo "$TEST" + ;; + esac +done | sort -u | xargs -r -n1 sh -c ' + TEST_FILE=${1#apps/desktop/} + cd apps/desktop && npx vitest run "$TEST_FILE" +' sh # If typescript touched, typecheck the package echo "$CHANGED" | grep -q "^apps/desktop/" && (cd apps/desktop && npx tsc --noEmit -p .) echo "$CHANGED" | grep -q "^apps/ade-cli/" && (cd apps/ade-cli && npm run typecheck) ``` -Then: - -```bash -git push --force-with-lease -``` +Use `QUALITY_COMMIT_MESSAGE="ship: apply post-rebase quality revalidation"`. Before bookkeeping, apply the **rebase budget rebate**: @@ -459,6 +530,10 @@ git diff --stat The lead reviews the combined diff. If anything is surprising (unrelated files touched, enormous diffs), the lead can revert specific hunks with `git checkout -- ` before committing. +The old quality binding is now invalid. Run the canonical Commit-bound quality +revalidation procedure on the final combined tree. This is revalidation of the +ship-loop delta, not permission to skip the original full `/quality` run. + Re-run only the narrow checks that matter: - For CI fixes, rerun the failing test file(s) or exact failing check target. @@ -468,20 +543,22 @@ Re-run only the narrow checks that matter: Commit with a message that lists what was addressed: ```bash -git commit -m "ship: iteration $N — fix $CI_JOBS, address #$COMMENT_IDS" -git push +QUALITY_COMMIT_MESSAGE="ship: iteration $N — fix $CI_JOBS, address #$COMMENT_IDS" ``` Post bot pings (Phase 4), update state (Phase 5), and schedule the next wake. Do not restart Phase 1 immediately after a push; give CI and review bots time to run while the agent is asleep. --- -## Phase 3c — Auto-merge +## Phase 3c — Merge Runs when Phase 2 routes here (everything terminal, no fix work, not behind, not already merged). The point of this playbook is "PR-to-merge", not "PR-to-green" — once green, the lane lands. Before resolving merge style, re-read the lane's final `/quality` result. The -gate must still be explicitly empty. If it is non-empty or unavailable, exit +gate must still be explicitly empty, and `qualityValidatedSha` must equal both +`git rev-parse HEAD` and the PR's current `headRefOid`. A missing or mismatched +binding means a later commit invalidated the result: return to quality +revalidation before merging. If the gate is non-empty or unavailable, exit `blocked` with `quality-gate-nonempty` or `quality-result-missing`; never merge and disclose deferred findings afterwards. @@ -496,7 +573,7 @@ Prefer the dominant style of the recent `main` history (look for `(#NNN)` suffix ### 3c.2 Attempt the merge ```bash -gh pr merge "$PR_NUMBER" --squash +gh pr merge "$PR_NUMBER" --squash --match-head-commit "$QUALITY_VALIDATED_SHA" ``` (Substitute `--merge` or `--rebase` per 3c.1.) @@ -506,16 +583,14 @@ gh pr merge "$PR_NUMBER" --squash If `gh pr merge` fails with `the base branch policy prohibits the merge` (typical when the repo requires a CODEOWNER review that the loop can't produce on its own), retry with admin override **only if the running user has admin rights on the repo**: ```bash -gh pr merge "$PR_NUMBER" --squash --admin -``` - -If the user is not a repo admin, do NOT use `--admin`. Fall back to: - -```bash -gh pr merge "$PR_NUMBER" --squash --auto +gh pr merge "$PR_NUMBER" --squash --admin --match-head-commit "$QUALITY_VALIDATED_SHA" ``` -`--auto` queues GitHub's native auto-merge; the PR will land on its own once the missing requirement is satisfied. If `--auto` is also rejected (the repo doesn't have auto-merge enabled), exit `blocked` with `exitReason: "merge-policy-blocked-no-auto"` and post a PR comment explaining what reviewer is needed. +If the user is not a repo admin, do NOT use `--admin`. Exit `blocked` with +`exitReason: "merge-policy-blocked-no-authorized-path"` and post a PR comment +explaining what reviewer or admin action is needed. Do not arm persistent +auto-merge: a later push can replace the validated head while auto-merge remains +enabled, which violates the exact-commit quality contract. ### 3c.4 Branch deletion @@ -574,11 +649,14 @@ Same input shape as Phase 3b.3, but with explicit constraints: ### 3d.4 Lead commit + push +Force-finalize may bypass review comments, but it may not bypass commit-bound +quality. Run the canonical Commit-bound quality revalidation procedure with the +commit message below and a normal push: + ```bash git status git diff --stat -git commit -m "ship: iteration 6 (force-finalize, review skipped) — fix $CI_JOBS" -git push +QUALITY_COMMIT_MESSAGE="ship: iteration 6 (force-finalize, review skipped) — fix $CI_JOBS" ``` Post the Phase 4 bot ping (a force-finalize push is a re-push → `@codex review`). Update state: @@ -588,6 +666,7 @@ Post the Phase 4 bot ping (a force-finalize push is a re-push → `@codex review "iteration": 6, "forceFinalize": true, "lastPushSha": "", + "qualityValidatedSha": "", "addressedCommentIds": [, ], "lastPolledAt": "", "status": "running" @@ -600,9 +679,10 @@ Schedule the next wake at the normal post-push cadence (270s if CI hasn't starte When the next wake polls: -- `merged == true` → `done-clean`, exit. +- `merged == true` and the merged head matches `qualityValidatedSha` → `done-clean`, exit. +- `merged == true` and the SHAs differ → `blocked` with `merged-unvalidated-head`. - `behindMain == true` → run Phase 3a (rebase) once, push, schedule next wake. Do NOT count it as a new iteration; force-finalize already ran. -- CI terminal AND green → route **immediately** through Phase 3c (auto-merge). Do NOT wait on review bots; review is intentionally bypassed in this phase. +- CI terminal AND green → route **immediately** through Phase 3c. Do NOT wait on review bots; review is intentionally bypassed in this phase. - CI terminal AND any required check still failing → exit `blocked`, `exitReason: "force-finalize-ci-failed"`, post a PR comment listing the failing job names + links. Do not start a seventh iteration. - CI still running → sleep on the normal cadence; do not act on a partial signal. @@ -645,6 +725,7 @@ These are separate comments (not a single body) so each bot handler parses its o { "iteration": , "lastPushSha": "", + "qualityValidatedSha": "", "addressedCommentIds": [, ], "lastPolledAt": "", "status": "running" @@ -653,9 +734,10 @@ These are separate comments (not a single body) so each bot handler parses its o ### 5.2 Decide exit vs next wake -- `merged == true` (observed during this iteration) → set `status: done-clean`, exit. +- `merged == true` with `pollHeadSha == qualityValidatedSha` → set `status: done-clean`, exit. +- `merged == true` with a mismatched SHA → set `status: blocked`, `exitReason: "merged-unvalidated-head"`, exit. - `iteration >= 5` AND `forceFinalize` unset/false AND not merged → run Phase 3d (force-finalize) on the next wake's fix turn. Do not exit; the cap is not a stop sign, it's a "land it now" trigger that switches the loop into review-ignoring CI-only mode. -- `forceFinalize == true` AND CI green AND not merged → route immediately through Phase 3c (auto-merge). Only if Phase 3c can't merge (policy blocked + no admin + auto-merge disabled) do you set `status: done-max` and leave a handoff comment. +- `forceFinalize == true` AND CI green AND not merged → route immediately through Phase 3c. Only if Phase 3c has no authorized direct/admin path do you set `status: done-max` and leave a handoff comment. - `forceFinalize == true` AND CI still red after the iteration-6 push → set `status: blocked`, `exitReason: "force-finalize-ci-failed"`, exit. Do not run a seventh iteration. - Otherwise → schedule next wake. @@ -686,7 +768,7 @@ The cadence is a hint, not a live polling budget. Prefer longer sleeps over freq | status | meaning | next action | | --- | --- | --- | | `done-clean` | PR merged on `main` (Phase 3c succeeded, possibly after Phase 3d force-finalize) | clear state file; print summary | -| `done-max` | 5 normal iterations + 1 force-finalize iteration exhausted AND Phase 3c could not merge (policy block + no admin + no auto-merge) | leave state file; post PR handoff comment to human | +| `done-max` | 5 normal iterations + 1 force-finalize iteration exhausted AND Phase 3c has no authorized direct/admin merge path | leave state file; post PR handoff comment to human | | `blocked` | Unrecoverable conflict, missing/non-empty quality gate, API error, or `force-finalize-ci-failed` (iteration 6 could not turn CI green) | leave state file; post PR comment with reason | ## Summary output (always print on exit) From 811766a84a3481c35b640741c5ba4c04426590f1 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sat, 1 Aug 2026 02:50:12 -0400 Subject: [PATCH 4/4] =?UTF-8?q?ship:=20iteration=202=20=E2=80=94=20address?= =?UTF-8?q?=20final=20review=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .agents/skills/ship/SKILL.md | 55 +++++---- AGENTS.md | 8 +- .../main/services/prs/prRowMetadata.test.ts | 9 ++ .../src/main/services/prs/prRowMetadata.ts | 6 +- .../prs/tabs/GitHubTabCreateLaneDialog.tsx | 84 +++++++++----- .../prs/tabs/GitHubTabRowsAndMapping.test.tsx | 56 ++++++++- docs/playbooks/ship-lane.md | 109 +++++++++++++----- 7 files changed, 238 insertions(+), 89 deletions(-) diff --git a/.agents/skills/ship/SKILL.md b/.agents/skills/ship/SKILL.md index 85773672b..05b2be90e 100644 --- a/.agents/skills/ship/SKILL.md +++ b/.agents/skills/ship/SKILL.md @@ -6,7 +6,8 @@ description: >- iterations plus one force-finalize iteration that bypasses review and fixes only CI. Pure loop — it does not replace the baseline /quality or /test runs; run those first. It does revalidate quality after any ship-loop mutation so - the final result is bound to the exact commit merged. Full phase logic lives + the final result is bound to the exact reviewed PR head and content tree. + Full phase logic lives in docs/playbooks/ship-lane.md. --- @@ -36,16 +37,18 @@ runtime-neutral entrypoint and the ADE-specific deltas below. If re-invoked by a scheduled wake, read the state file first; if `status == running`, skip Phase 0 and go to Phase 1. -The playbook's Phase 0 is **commit → push → open PR** only. Test generation and -the local-CI gate are NOT part of ship — that's `/test` (and optionally -`/finalize`) before you reach this skill. +The playbook's Phase 0 is **checkpoint → commit-bound quality revalidation → +push → open PR**. Baseline test generation and the local-CI gate are NOT part +of ship — that's `/test` (and optionally `/finalize`) before you reach this +skill. -## Precondition: `/quality` must be empty and bound to the merge commit +## Precondition: `/quality` must be empty and bound to the final tree Before Phase 0, require a completed `/quality` result with an empty gate. Before -Phase 3c, also require `qualityValidatedSha` in the ship state to equal both -`git rev-parse HEAD` and the PR's current `headRefOid`. The result is valid only -for that exact commit; green CI on a later commit does not preserve it. +Phase 3c, run the playbook's single canonical **Validate the current quality +binding** procedure. It binds the reviewed head, content tree, and base so +GitHub's squash/merge/rebase result has the reviewed tree. Green CI on a later +head or base does not preserve this binding. A non-empty gate **blocks the merge** — every row in it is a finding that was verified as real and left unfixed, and by `/quality`'s contract the only two @@ -56,8 +59,9 @@ change this branch was not asked to make. Both need the author. stop with `blocked`. Do not merge and mention them afterwards. - If `/quality` was never run on this lane, or its final gate result is not available in the lane handoff, stop with `blocked`; unknown is not empty. -- Any rebase, conflict resolution, Phase 3b edit, or force-finalize edit clears - `qualityValidatedSha`. Run the playbook's single canonical **Commit-bound +- Any base movement, rebase, conflict resolution, Phase 3b edit, or + force-finalize edit clears all three quality binding fields. Run the + playbook's single canonical **Commit-bound quality revalidation** procedure before pushing that mutation. - Never enter Phase 3c with a missing or mismatched binding. Revalidate first; do not merge and disclose stale quality evidence afterwards. @@ -65,6 +69,10 @@ change this branch was not asked to make. Both need the author. `--match-head-commit "$QUALITY_VALIDATED_SHA"`. Persistent auto-merge is not allowed because a later push can replace the validated head while it remains armed. +- GitHub creates a new commit for squash/merge/rebase. The validation claim is + deliberately about its exact content tree, not its not-yet-created commit + OID. After merge, run the playbook's canonical **Confirm the validated merge + result** procedure; a mismatch is never `done-clean`. Severity is irrelevant here: a Medium in the gate blocks exactly as hard as a Blocker, because presence in the gate means it needed a human, not that it was @@ -127,11 +135,11 @@ terminal-neutral, and continue. Record it under `inactiveReviewBots`, never If branch protection requires an absent check, Phase 3c will surface that as a merge-policy block. -**Rebase only on real conflicts.** `behindMain` alone does NOT trigger a rebase. -Only rebase/merge `main` when there is an actual conflict (`mergeStateStatus` -shows the PR is dirty/conflicting). If the branch is merely behind but cleanly -mergeable, skip the rebase and let the merge handle it — needless rebases burn -iterations and CI. +**Rebase only on real conflicts or a stale quality base.** `behindMain` alone +does not normally trigger a rebase. The one safety exception is base movement +after quality validation: the final tree is no longer the reviewed head tree, +so rebase and rerun the canonical quality procedure even when GitHub reports a +clean merge. Otherwise, skip needless rebases. **Bot pings by iteration.** Never ping GitHub Copilot and never treat Copilot as an expected review signal; quota exhaustion otherwise leaves the loop waiting @@ -145,10 +153,11 @@ rule — defer to it for exact bodies. `gh pr merge --squash --match-head-commit "$QUALITY_VALIDATED_SHA"` will show BLOCKED. Retry with `gh pr merge --admin --squash --match-head-commit "$QUALITY_VALIDATED_SHA"`; -the ruleset's -non-linear-history rule can still reject `--admin`, in which case fall back to a -local merge + admin-bypass push (per AGENTS.md). Do NOT pass `--delete-branch` -(it fails from a worktree); delete the head ref server-side via +the ruleset's non-linear-history rule can still reject `--admin`. Do not fall +back to a locally-created commit: it would not be the reviewed and CI-tested PR +merge result. Exit blocked if both direct `gh` paths fail. After a successful +merge, run **Confirm the validated merge result**. Do NOT +pass `--delete-branch` (it fails from a worktree); delete the head ref server-side via `gh api -X DELETE "repos/{owner}/{repo}/git/refs/heads/"`. **Fix discipline (every fix agent must follow):** (1) Fix CI and review together @@ -195,14 +204,16 @@ self-resume signal. Either: ## The loop (summary — full detail in the playbook) - **Phase 0 (first run):** safety rails (clean tree, GitHub origin, refuse - `main`) → commit → push → open PR (`ade`, gh fallback) → write state → + `main`) → checkpoint → canonical commit-bound quality revalidation → push → + open PR (`ade`, gh fallback) → verify the provisional binding → write state → schedule first wake. - **Phase 1 — Poll:** wait for CI terminal and every bot that actually started to become terminal. After one 12-minute grace window, classify bots with zero evidence as inactive/terminal-neutral. Return a structured summary (merged / conflicting / ciFailed / newComments). Don't fix on a partial signal. -- **Phase 2 — Decide:** merged → `done-clean`. Real conflict → Phase 3a rebase - (rebate). CI or bots running → reschedule. Both terminal, no work → 3c merge. +- **Phase 2 — Decide:** merged → run **Confirm the validated merge result** and + only then set `done-clean`. Real conflict → Phase 3a rebase (rebate). CI or + bots running → reschedule. Both terminal, no work → 3c merge. Both terminal, work exists, `iter < 5` → 3b fix. `iter >= 5`, not merged → 3d force-finalize. - **Phase 3a Rebase / 3b Fix / 3c Merge / 3d Force-finalize** — per the playbook. diff --git a/AGENTS.md b/AGENTS.md index 75eee514d..efa0e0507 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,15 +17,15 @@ Day-to-day work follows a five-stage loop, each stage an agent-folder skill unde `/context` → work → `/quality` → `/test` → `/ship` - **/context** — session primer: detects the lane's area and loads only the matching docs + perf skill (never a broad dump). -- **/quality** — dual-track review (correctness/security + maintainability/code-judo); auto-fixes the safe findings, gates Blockers. The loop's bug-finding and cleanup engine. -- **/test** — test steward: prune/consolidate/add + docs/mobile/CLI/TUI parity + CI-mirrored shards; turns each `/quality` gate finding into a named regression test. -- **/ship** — pure autonomous PR→merge loop (poll → fix → rebase → merge). Does NOT run quality/test — run those first. Wraps `docs/playbooks/ship-lane.md`. +- **/quality** — dual-track review (correctness/security + maintainability/code-judo); fixes every verified finding at every severity. It gates only a product decision the agent cannot make or a behavior change the branch was not authorized to make. +- **/test** — test steward: prune/consolidate/add + docs/mobile/CLI/TUI parity + CI-mirrored shards; records a named regression test or exact alternate verification for every accepted correctness finding. +- **/ship** — autonomous PR→merge loop (poll → fix → rebase → merge). Run baseline `/quality` and `/test` first; after any ship-loop mutation, ship reruns commit-bound `/quality` revalidation before pushing or merging. Wraps `docs/playbooks/ship-lane.md`. Utilities (run when relevant, not part of the core loop): **/audit** (targeted bug hunt), **/finalize** (optional pre-push local-CI gate), **/optimize** (perf profiling), **/release** (cut a release). ## Playbooks -- `docs/playbooks/ship-lane.md` — autonomous PR-to-merge driver (poll → fix → rebase → merge; `/quality` and `/test` run *before* it, not inside it). Any agent CLI can follow it directly; Claude Code invokes it via the `/ship` skill. +- `docs/playbooks/ship-lane.md` — autonomous PR-to-merge driver (poll → fix → rebase → merge). Baseline `/quality` and `/test` run before it; mutation-specific commit-bound quality revalidation runs inside it. Any agent CLI can follow it directly; Claude Code invokes it via the `/ship` skill. ## Working norms diff --git a/apps/desktop/src/main/services/prs/prRowMetadata.test.ts b/apps/desktop/src/main/services/prs/prRowMetadata.test.ts index e3e712375..3038be3a5 100644 --- a/apps/desktop/src/main/services/prs/prRowMetadata.test.ts +++ b/apps/desktop/src/main/services/prs/prRowMetadata.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { deriveGithubSnapshotLaneLink, deriveGithubSnapshotMergeFacts, + normalizeCount, type PullRequestRowMetadata, } from "./prRowMetadata"; @@ -20,6 +21,14 @@ function makePrRowMetadata( } describe("PR row metadata mappers", () => { + it("accepts only non-negative integer count values", () => { + expect(normalizeCount(0)).toBe(0); + expect(normalizeCount(12)).toBe(12); + expect(normalizeCount("")).toBeNull(); + expect(normalizeCount(false)).toBeNull(); + expect(normalizeCount(1.5)).toBeNull(); + }); + it("separates a live lane mapping from frozen detached-lane provenance", () => { const laneById = new Map([[LANE_ID, { name: "my-feature" }]]); diff --git a/apps/desktop/src/main/services/prs/prRowMetadata.ts b/apps/desktop/src/main/services/prs/prRowMetadata.ts index 056aecdb3..7c0a5e75a 100644 --- a/apps/desktop/src/main/services/prs/prRowMetadata.ts +++ b/apps/desktop/src/main/services/prs/prRowMetadata.ts @@ -24,9 +24,9 @@ export type PullRequestRowMetadata = { /** Normalize a persisted non-negative count, returning null for absent or invalid data. */ export function normalizeCount(value: unknown): number | null { - if (value == null) return null; - const parsed = Number(value); - return Number.isFinite(parsed) && parsed >= 0 ? parsed : null; + return typeof value === "number" && Number.isSafeInteger(value) && value >= 0 + ? value + : null; } /** Narrow persisted merge-method data to the supported public union. */ diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsx index 143f57da4..33b49de43 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsx @@ -1,4 +1,6 @@ import { GitBranch, Warning } from "@phosphor-icons/react"; +import * as Dialog from "@radix-ui/react-dialog"; +import { useEffect, useRef } from "react"; import type { CreateLaneFromPrBranchArgs, CreateLaneFromPrBranchPreflight, @@ -178,6 +180,10 @@ export function CreateLaneFromPrBranchDialog({ onCancel: () => void; onConfirm: () => void; }) { + const returnFocusRef = useRef( + document.activeElement instanceof HTMLElement ? document.activeElement : null, + ); + const cancelRef = useRef(null); const blockingConflict = preflightBlockingConflict(preflight); const canConfirm = Boolean(preflight?.canCreate) && !loading && !busy; const sourceBranch = preflightRemoteBranch(preflight, item); @@ -190,38 +196,48 @@ export function CreateLaneFromPrBranchDialog({ ["Base branch", preflightBaseBranch(preflight, item)], ] as const; + useEffect(() => { + if (busy) cancelRef.current?.focus(); + }, [busy]); + return ( -
-
{ if (!open && !busy) onCancel(); }}> + + + { + event.preventDefault(); + returnFocusRef.current?.focus(); + }} + onEscapeKeyDown={(event) => { if (busy) event.preventDefault(); }} + style={{ + position: "fixed", + left: "50%", + top: "50%", + transform: "translate(-50%, -50%)", + zIndex: 101, width: "min(560px, 100%)", + maxWidth: "calc(100vw - 40px)", borderRadius: 12, border: `1px solid ${COLORS.border}`, background: COLORS.cardBgSolid, boxShadow: "0 24px 80px rgba(0,0,0,0.45)", overflow: "hidden", }} - > + >
-
+ Create lane from PR branch -
+
{loading ? ( @@ -264,9 +280,22 @@ export function CreateLaneFromPrBranchDialog({ ) : null}
- + + +
-
-
+ + + ); } diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx index e4b228ff2..a4d3acdb0 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx @@ -5,6 +5,7 @@ import { act, render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { + CreateLaneFromPrBranchResult, CreateLaneFromPrBranchPreflightResult, GitHubPrSnapshot, } from "../../../../shared/types"; @@ -464,7 +465,8 @@ describe("GitHubTab rows and mapping", () => { (window.ade.prs.getGitHubSnapshot as ReturnType).mockResolvedValue(snapshotWithUnlinked); renderTab(); - await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); + const trigger = await screen.findByRole("button", { name: /create lane from pr branch/i }); + await user.click(trigger); expect(window.ade.prs.preflightCreateLaneFromPrBranch).toHaveBeenCalledWith({ repoOwner: "ade-dev", @@ -472,10 +474,62 @@ describe("GitHubTab rows and mapping", () => { githubPrNumber: 200, }); const dialog = await screen.findByRole("dialog", { name: /create lane from pr branch/i }); + const cancel = within(dialog).getByRole("button", { name: /cancel/i }); + const confirm = within(dialog).getByRole("button", { name: /create lane/i }); + expect(document.activeElement).toBe(cancel); + await user.tab({ shift: true }); + expect(document.activeElement).toBe(confirm); + await user.tab(); + expect(document.activeElement).toBe(cancel); expect(within(dialog).getByText(/#200 Unlinked PR/)).toBeTruthy(); expect(within(dialog).getAllByText("origin/feature/open").length).toBeGreaterThan(0); expect(within(dialog).getAllByText("Unlinked PR").length).toBeGreaterThan(0); expect(within(dialog).getAllByText("main").length).toBeGreaterThan(0); + await user.keyboard("{Escape}"); + expect(screen.queryByRole("dialog", { name: /create lane from pr branch/i })).toBeNull(); + expect(document.activeElement).toBe(trigger); + }); + + it("keeps focus in the dialog while creation is busy and restores an action after failure", async () => { + const user = userEvent.setup(); + const createResult = createDeferred(); + const snapshotWithUnlinked: GitHubPrSnapshot = { + ...snapshot, + repoPullRequests: [makeGitHubPr({ + id: "repo-unlinked", + githubPrNumber: 200, + title: "Unlinked PR", + linkedPrId: null, + linkedLaneId: null, + linkedLaneName: null, + adeKind: null, + })], + }; + (window.ade.prs.getGitHubSnapshot as ReturnType) + .mockResolvedValue(snapshotWithUnlinked); + (window.ade.prs.createLaneFromPrBranch as ReturnType) + .mockReturnValueOnce(createResult.promise); + renderTab(); + + await user.click(await screen.findByRole("button", { name: /create lane from pr branch/i })); + const dialog = await screen.findByRole("dialog", { name: /create lane from pr branch/i }); + await user.click(within(dialog).getByRole("button", { name: /^create lane$/i })); + + await user.keyboard("{Escape}"); + expect(screen.getByRole("dialog", { name: /create lane from pr branch/i })).toBe(dialog); + await user.tab(); + expect(dialog.contains(document.activeElement)).toBe(true); + + await act(async () => { + createResult.reject(new Error("creation failed")); + await Promise.resolve(); + }); + expect(await within(dialog).findByText("creation failed")).toBeTruthy(); + await user.tab(); + const activeControl = document.activeElement; + expect(dialog.contains(activeControl)).toBe(true); + expect(activeControl).toBeInstanceOf(HTMLButtonElement); + expect((activeControl as HTMLButtonElement).disabled).toBe(false); }); it("ignores stale create-lane preflight results from a previous PR", async () => { diff --git a/docs/playbooks/ship-lane.md b/docs/playbooks/ship-lane.md index ba2818499..df75e1aae 100644 --- a/docs/playbooks/ship-lane.md +++ b/docs/playbooks/ship-lane.md @@ -106,6 +106,8 @@ Path: `.ade/shipLane/.json` (sanitize by replacing `/` with `_ "iteration": 2, "lastPushSha": "abc123...", "qualityValidatedSha": "abc123...", + "qualityValidatedTree": "def456...", + "qualityValidatedBaseSha": "789abc...", "addressedCommentIds": [987654, 987655], "status": "running", "startedAt": "2026-04-23T14:30:00Z", @@ -124,7 +126,13 @@ This is the single canonical procedure for every ship-loop mutation. Phase 0, post-rebase, Phase 3b, and force-finalize supply only their commit message, diff base, narrow test targets, and push command; they do not restate this algorithm. -1. Clear `qualityValidatedSha` before editing. +1. Clear `qualityValidatedSha`, `qualityValidatedTree`, and + `qualityValidatedBaseSha` before editing. Fetch the intended base, set + `QUALITY_VALIDATED_BASE_SHA` to that fetched commit, and require it to be an + ancestor of the candidate head. If it is not, preserve the work, route + through Phase 3a, and restart this procedure after the rebase; do not bind a + behind-base tree. This works before PR creation; Phase 0 uses `main` as the + intended base. 2. Run both `/quality` tracks on the final combined diff, fix every accepted finding, and repeat both tracks until the same pass is clean. 3. Build the validation scope from the union of committed, unstaged, staged, @@ -153,12 +161,37 @@ base, narrow test targets, and push command; they do not restate this algorithm. QUALITY_VALIDATED_SHA=$(git rev-parse HEAD) ``` -5. Push with the phase-appropriate command, then verify the PR's `headRefOid` - equals `QUALITY_VALIDATED_SHA` before writing it to ship state. Any later edit - or head mismatch clears the binding and restarts this procedure. - -The exact-SHA merge check in Phase 3c is the second half of this contract; a -green CI result never substitutes for it. +5. Push with the phase-appropriate command and verify the remote branch head + equals `QUALITY_VALIDATED_SHA`. If a PR already exists, also require its + `headRefOid` and `baseRefOid` to equal `QUALITY_VALIDATED_SHA` and + `QUALITY_VALIDATED_BASE_SHA`. Before the PR exists, retain the fetched + intended base as a provisional binding; Phase 0.5 performs the PR-field + verification immediately after creation. In both modes, require the base to + be an ancestor of the head and `HEAD^{tree}` to still equal + `QUALITY_VALIDATED_TREE` before writing all three values to ship state. Any + later edit, head mismatch, or base movement clears the binding and restarts + this procedure. + +The exact head/base/tree checks in Phase 3c are the second half of this +contract; a green CI result never substitutes for them. GitHub creates the +final squash/merge/rebase commit, so this procedure binds the reviewed content +tree rather than claiming to know or validate that future commit OID. + +### Validate the current quality binding + +This is the only pre-merge binding check. Read the three values from ship state, +then require the local head SHA/tree and the PR head/base SHAs to match them, +require the base to be an ancestor of HEAD, and require the quality gate to be +explicitly empty. Any mismatch clears the binding and routes back through the +canonical revalidation procedure (via Phase 3a first when the base moved). + +### Confirm the validated merge result + +This is the only clean-merge confirmation. Require the PR's final `headRefOid` +to equal `qualityValidatedSha`, resolve `mergeCommit.oid`, read that commit's +tree through the GitHub API, and require it to equal `qualityValidatedTree`. +Only then may state become `done-clean`. A head mismatch is +`merged-unvalidated-head`; a tree mismatch is `merged-unvalidated-tree`. --- @@ -184,7 +217,7 @@ Ship is a **pure merge loop** — it does not replace the baseline `/quality`, (`/context → work → /quality → /test → /ship`) that the author runs *before* reaching ship. Ship assumes the lane is already reviewed, tested, and (optionally) finalized; its only quality work is revalidation after a ship-loop -mutation so the result stays bound to the exact commit being merged. +mutation so the result stays bound to the exact reviewed head and content tree. Preconditions here: @@ -197,7 +230,8 @@ Preconditions here: `exitReason: "quality-gate-nonempty"` or `"quality-result-missing"` rather than assuming unknown means clean. Ship does not replace either baseline skill run. -- The quality result must be bound to the exact commit that will merge by the +- The quality result must be bound to the exact PR head, base, and content tree + that will merge by the canonical Commit-bound quality revalidation procedure above. Do not infer a binding from a green gate summary that predates the commit. @@ -210,15 +244,21 @@ git add -A git diff --cached --quiet || git commit -m "ship: prepare lane for review" ``` -Run the canonical Commit-bound quality revalidation procedure with the initial -checkpoint as `QUALITY_DIFF_BASE`, the commit message below, and -`git push -u origin "$CURRENT_BRANCH"` as the push command: +Fetch `origin/main`, bind the intended base, and set `QUALITY_DIFF_BASE` to the +feature merge-base before running the canonical Commit-bound quality +revalidation procedure. This includes every committed feature file rather than +only edits made after the checkpoint: ```bash +git fetch origin main +QUALITY_VALIDATED_BASE_SHA=$(git rev-parse origin/main) +QUALITY_DIFF_BASE=$(git merge-base HEAD "$QUALITY_VALIDATED_BASE_SHA") QUALITY_COMMIT_MESSAGE="ship: apply initial quality revalidation" +# Push command for canonical step 5: +git push -u origin "$CURRENT_BRANCH" ``` -If anything changes after the clean pass, discard both captured values and +If anything changes after the clean pass, discard all three binding values and revalidate again. **PR creation: prefer the ADE CLI.** Opening the PR via `ade` registers it in ADE's PR tracking (lane ↔ PR link, check/comment inventory, review-thread state). `gh pr create` is the fallback, not the default. Falling back too eagerly defeats the purpose — do it only after you've genuinely confirmed the ADE path is broken. @@ -261,8 +301,9 @@ only when the diff touches more than 250 files. ### 0.5 Write initial state -After PR creation, verify its `headRefOid` equals `QUALITY_VALIDATED_SHA`. If it -does not, leave `qualityValidatedSha` null and revalidate the actual PR head. +After PR creation, verify its `headRefOid` and `baseRefOid` match the quality +binding. If either differs, clear all quality binding fields and revalidate the +actual PR head and base. ```json { @@ -271,6 +312,8 @@ does not, leave `qualityValidatedSha` null and revalidate the actual PR head. "iteration": 0, "lastPushSha": "", "qualityValidatedSha": "", + "qualityValidatedTree": "", + "qualityValidatedBaseSha": "", "addressedCommentIds": [], "status": "running", "startedAt": "", @@ -389,8 +432,7 @@ Pure logic on the poll summary: | Condition | Action | | --- | --- | -| `merged == true` AND `pollHeadSha == qualityValidatedSha` | Exit `done-clean`; clear state file. | -| `merged == true` AND the SHAs differ | Exit `blocked` with `merged-unvalidated-head`; never report stale quality as clean. | +| `merged == true` | Run **Confirm the validated merge result**; exit `done-clean` only when it succeeds. | | `behindMain == true` | Go to Phase 3a (rebase), apply the rebase budget rebate, then schedule/poll according to Phase 5. | | `ciRunning == true` OR `reviewBotsRunning == true` | Do NOT iterate on a partial signal. Go to Phase 5 (schedule next wake). This applies even if the other signal already shows failures/comments — pushing a fix now means the next CI+review cycle races the fix and you likely re-push for the other half. | | `ciFailed` empty, `newComments` empty, `ciRunning == false`, `reviewBotsRunning == false` | Go to **Phase 3c**. Done-clean does not mean "stop and leave for human" — it means everything is green, and the lane should land on `main`. | @@ -554,11 +596,10 @@ Post bot pings (Phase 4), update state (Phase 5), and schedule the next wake. Do Runs when Phase 2 routes here (everything terminal, no fix work, not behind, not already merged). The point of this playbook is "PR-to-merge", not "PR-to-green" — once green, the lane lands. -Before resolving merge style, re-read the lane's final `/quality` result. The -gate must still be explicitly empty, and `qualityValidatedSha` must equal both -`git rev-parse HEAD` and the PR's current `headRefOid`. A missing or mismatched -binding means a later commit invalidated the result: return to quality -revalidation before merging. If the gate is non-empty or unavailable, exit +Before resolving merge style, run the canonical **Validate the current quality +binding** procedure. A missing or mismatched binding means a later head or base +invalidated the result: rebase when needed, then return to quality revalidation +before merging. If the gate is non-empty or unavailable, exit `blocked` with `quality-gate-nonempty` or `quality-result-missing`; never merge and disclose deferred findings afterwards. @@ -590,7 +631,11 @@ If the user is not a repo admin, do NOT use `--admin`. Exit `blocked` with `exitReason: "merge-policy-blocked-no-authorized-path"` and post a PR comment explaining what reviewer or admin action is needed. Do not arm persistent auto-merge: a later push can replace the validated head while auto-merge remains -enabled, which violates the exact-commit quality contract. +enabled, which violates the exact head/base/tree quality contract. + +If the admin path is rejected too, exit with the same blocked reason. Do not +create and push a local fallback commit: that commit would bypass the reviewed +PR merge result and its CI evidence. ### 3c.4 Branch deletion @@ -604,11 +649,9 @@ Or simply skip deletion and rely on the repo's "Automatically delete head branch ### 3c.5 Confirm + finalize -```bash -gh pr view "$PR_NUMBER" --json state,mergedAt,mergeCommit -``` - -If `state == MERGED`, exit `done-clean`, clear `.ade/shipLane/.json`, and print the summary. Do NOT schedule another wake-up. +Run the canonical **Confirm the validated merge result** procedure. Only when it +succeeds may the loop clear `.ade/shipLane/.json` and print the summary. +Do NOT schedule another wake-up. --- @@ -667,6 +710,8 @@ Post the Phase 4 bot ping (a force-finalize push is a re-push → `@codex review "forceFinalize": true, "lastPushSha": "", "qualityValidatedSha": "", + "qualityValidatedTree": "", + "qualityValidatedBaseSha": "", "addressedCommentIds": [, ], "lastPolledAt": "", "status": "running" @@ -679,8 +724,7 @@ Schedule the next wake at the normal post-push cadence (270s if CI hasn't starte When the next wake polls: -- `merged == true` and the merged head matches `qualityValidatedSha` → `done-clean`, exit. -- `merged == true` and the SHAs differ → `blocked` with `merged-unvalidated-head`. +- `merged == true` → run **Confirm the validated merge result**; exit `done-clean` only when it succeeds. - `behindMain == true` → run Phase 3a (rebase) once, push, schedule next wake. Do NOT count it as a new iteration; force-finalize already ran. - CI terminal AND green → route **immediately** through Phase 3c. Do NOT wait on review bots; review is intentionally bypassed in this phase. - CI terminal AND any required check still failing → exit `blocked`, `exitReason: "force-finalize-ci-failed"`, post a PR comment listing the failing job names + links. Do not start a seventh iteration. @@ -726,6 +770,8 @@ These are separate comments (not a single body) so each bot handler parses its o "iteration": , "lastPushSha": "", "qualityValidatedSha": "", + "qualityValidatedTree": "", + "qualityValidatedBaseSha": "", "addressedCommentIds": [, ], "lastPolledAt": "", "status": "running" @@ -734,8 +780,7 @@ These are separate comments (not a single body) so each bot handler parses its o ### 5.2 Decide exit vs next wake -- `merged == true` with `pollHeadSha == qualityValidatedSha` → set `status: done-clean`, exit. -- `merged == true` with a mismatched SHA → set `status: blocked`, `exitReason: "merged-unvalidated-head"`, exit. +- `merged == true` → run **Confirm the validated merge result**; set `done-clean` only when it succeeds. - `iteration >= 5` AND `forceFinalize` unset/false AND not merged → run Phase 3d (force-finalize) on the next wake's fix turn. Do not exit; the cap is not a stop sign, it's a "land it now" trigger that switches the loop into review-ignoring CI-only mode. - `forceFinalize == true` AND CI green AND not merged → route immediately through Phase 3c. Only if Phase 3c has no authorized direct/admin path do you set `status: done-max` and leave a handoff comment. - `forceFinalize == true` AND CI still red after the iteration-6 push → set `status: blocked`, `exitReason: "force-finalize-ci-failed"`, exit. Do not run a seventh iteration.