From d56fbb420990a6cbd965adafcfb3824db67d9849 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:33:40 -0400 Subject: [PATCH 1/3] feat(prs): keep merged PRs after their lane is deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a lane hard-deleted its `pull_requests` row, so the normal "merge, then delete the lane and branch" flow erased ADE's record that the PR was ever ADE's. The Merged bucket rendered almost entirely as amber `unmapped` badges, and each row silently lost its CI outcome, review result and diff stats along with the row. Lane deletion, branch switch and rename now soft-detach instead: the row survives with `detached_at` plus the lane's name, colour and a frozen count of its chats, artifacts and checkpoints. Those counts cannot be recomputed later — the lane's sessions and artifacts are deleted with it — so they are captured before the cascade runs. `lane_id` is deliberately left dangling: it is NOT NULL on a cr-sqlite CRR that the phone treats as critical, and cr-sqlite cannot alter nullability without a table rebuild. Detaching also nulls the bulky snapshot JSON after lifting commit and file counts onto the row, so retaining history costs less storage than the delete it replaces. `detached_at is null` now gates every lane-scoped "what is this lane working on" read; project-wide reads keep detached rows, which is how the merged view recovers its provenance. A detached row owns nothing, so it can never block re-mapping, and a lane reclaims one only when it still exists and still tracks the PR's head branch. Merged PRs also now record how they shipped — who merged, by what method, commit and file counts — captured at merge and on the poller's transition, with no extra GitHub calls. The merged row is re-cut to match: no mapping badge in terminal buckets, neutral rather than amber in Open unless the badge is actionable, a `was: ` provenance chip, merge facts in place of the branch pair, and sticky day/week period headers with per-period totals. The PR detail pane drops the mapping controls that could never fire on a merged PR and gains a shipped summary in the merge rail. iOS mirrors all of it, including two columns (`merge_conflicts`, `behind_base_by`) that desktop already wrote but the phone never declared — an unrelated pre-existing gap in the same block, where an unknown column nacks the whole changeset and freezes replication. Co-Authored-By: Claude Opus 5 --- apps/ade-cli/src/cli.ts | 19 +- .../__tests__/rightPaneFormatters.test.ts | 14 + .../src/tuiClient/rightPaneFormatters.ts | 8 +- .../services/conflicts/conflictService.ts | 3 + .../main/services/lanes/autoRebaseService.ts | 2 + .../main/services/lanes/laneService.test.ts | 52 ++- .../src/main/services/lanes/laneService.ts | 55 ++- .../services/lanes/rebaseSuggestionService.ts | 3 + .../src/main/services/prs/prService.test.ts | 173 ++++++++- .../src/main/services/prs/prService.ts | 299 ++++++++++++++- .../prs/pullRequestRowCleanup.test.ts | 202 ++++++++++ .../services/prs/pullRequestRowCleanup.ts | 195 +++++++++- .../review/reviewContextBuilder.test.ts | 5 +- .../services/review/reviewContextBuilder.ts | 2 + apps/desktop/src/main/services/state/kvDb.ts | 22 ++ apps/desktop/src/renderer/browserMock.ts | 105 ++++++ .../components/prs/detail/PrDetailPane.tsx | 11 +- .../prs/shared/PrDetailMergeRail.test.tsx | 54 +++ .../prs/shared/PrDetailMergeRail.tsx | 65 ++++ .../prs/shared/prListGrouping.test.ts | 167 +++++++++ .../components/prs/shared/prListGrouping.ts | 157 ++++++++ .../components/prs/tabs/GitHubTab.test.tsx | 99 +++++ .../components/prs/tabs/GitHubTab.tsx | 346 +++++++++++++++--- apps/desktop/src/shared/types/prs.ts | 39 ++ apps/ios/ADE/Models/RemoteModels.swift | 36 ++ apps/ios/ADE/Services/Database.swift | 23 ++ .../ADE/Views/PRs/PrDetailOverviewTab.swift | 71 ++++ apps/ios/ADE/Views/PRs/PrDetailScreen.swift | 15 + apps/ios/ADE/Views/PRs/PrHelpers.swift | 121 ++++++ .../ios/ADE/Views/PRs/PrListRowModifier.swift | 35 ++ apps/ios/ADE/Views/PRs/PrRowCard.swift | 87 ++++- apps/ios/ADE/Views/PRs/PrsRootScreen.swift | 27 +- docs/features/lanes/README.md | 19 +- docs/features/pull-requests/README.md | 147 +++++++- .../sync-and-multi-device/ios-companion.md | 41 ++- 35 files changed, 2589 insertions(+), 130 deletions(-) create mode 100644 apps/desktop/src/main/services/prs/pullRequestRowCleanup.test.ts create mode 100644 apps/desktop/src/renderer/components/prs/shared/prListGrouping.test.ts create mode 100644 apps/desktop/src/renderer/components/prs/shared/prListGrouping.ts diff --git a/apps/ade-cli/src/cli.ts b/apps/ade-cli/src/cli.ts index 8e3452876..c88e5a793 100644 --- a/apps/ade-cli/src/cli.ts +++ b/apps/ade-cli/src/cli.ts @@ -17569,6 +17569,23 @@ function formatLaneDetail(value: unknown): string { ]); } +/** + * Lane cell for a PR row. + * + * PR rows are soft-detached rather than deleted when their lane is removed or moves to + * another branch, and their `lane_id` intentionally keeps pointing at the gone lane. So + * printing `laneId` alone would present dead history as a live mapping. Detached rows + * render as `was ` instead. + */ +function prLaneCell(pr: JsonObject): unknown { + const detached = isRecord(pr.detached) ? pr.detached : null; + if (detached) { + const name = asString(detached.laneName) ?? asString(pr.laneName); + return `was ${name ?? "deleted lane"}`; + } + return pr.laneId ?? pr.laneName; +} + function formatPrList(value: unknown): string { const prs = firstArray(value, ["prs", "pullRequests", "items", "results"]); return renderTable( @@ -17576,7 +17593,7 @@ function formatPrList(value: unknown): string { prs.map((pr) => [ pr.githubPrNumber ?? pr.number ?? pr.prNumber ?? pr.id, pr.state ?? pr.status, - pr.laneId ?? pr.laneName, + prLaneCell(pr), pr.headBranch ?? pr.headRefName ?? pr.branchRef ?? pr.branch, pr.title, ]), diff --git a/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts b/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts index cac1af640..ce9711e20 100644 --- a/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts +++ b/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts @@ -43,6 +43,20 @@ describe("rightPaneFormatters", () => { expect(body).not.toContain("\"title\""); }); + it("renders a detached PR's lane as history, not as a live mapping", () => { + const body = formatPrSummary({ + id: "pr-9", + number: 9, + title: "Merged work", + state: "merged", + laneId: "lane-deleted-uuid", + detached: { at: "2026-07-30T00:00:00Z", laneName: "prs-tab", laneColor: null }, + }); + + expect(body).toContain("was prs-tab"); + expect(body).not.toContain("lane-deleted-uuid"); + }); + it("formats PR create links from the new action envelope", () => { const body = formatPrSummary({ pr: { diff --git a/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts b/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts index e3087ba8a..1e7d48620 100644 --- a/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts +++ b/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts @@ -154,7 +154,13 @@ export function formatPrSummary(value: unknown): string { const draft = pickBoolean(pr, ["isDraft", "draft"]) === true ? " · draft" : ""; const title = pickString(pr, ["title", "name"]) ?? "Untitled PR"; const id = pickString(pr, ["id", "prId"]); - const lane = pickString(pr, ["laneName", "laneId"]); + // A PR whose lane was deleted (or moved to another branch) keeps its row and its now + // dangling `laneId`, so the raw id would read as a live mapping. Show it as history. + const detached = isRecord(pr.detached) ? pr.detached : null; + const laneValue = pickString(pr, ["laneName", "laneId"]); + const lane = detached + ? `was ${pickString(detached, ["laneName"]) ?? laneValue ?? "deleted lane"}` + : laneValue; const head = pickString(pr, ["headBranch", "headRefName", "branchRef", "branch"]); const base = pickString(pr, ["baseBranch", "baseRefName", "baseRef", "targetBranch"]); const githubUrl = pickString(root, ["githubUrl", "githubPrUrl"]) diff --git a/apps/desktop/src/main/services/conflicts/conflictService.ts b/apps/desktop/src/main/services/conflicts/conflictService.ts index 7deb5933a..8525eda0a 100644 --- a/apps/desktop/src/main/services/conflicts/conflictService.ts +++ b/apps/desktop/src/main/services/conflicts/conflictService.ts @@ -4380,6 +4380,9 @@ export function createConflictService({ from pull_requests where project_id = ? and state in ('open', 'draft') + -- A detached row belongs to a lane that was deleted or has moved to another + -- branch; it must not raise rebase needs against the live lane. + and detached_at is null order by updated_at desc, created_at desc `, [projectId], diff --git a/apps/desktop/src/main/services/lanes/autoRebaseService.ts b/apps/desktop/src/main/services/lanes/autoRebaseService.ts index c8fe678cc..33d27ded6 100644 --- a/apps/desktop/src/main/services/lanes/autoRebaseService.ts +++ b/apps/desktop/src/main/services/lanes/autoRebaseService.ts @@ -296,6 +296,8 @@ export function createAutoRebaseService(args: { from pull_requests where lane_id = ? and state in ('open', 'draft') + -- Detached rows describe a branch this lane no longer tracks. + and detached_at is null order by updated_at desc, created_at desc limit 1 `, diff --git a/apps/desktop/src/main/services/lanes/laneService.test.ts b/apps/desktop/src/main/services/lanes/laneService.test.ts index 73514e6f5..5603fc556 100644 --- a/apps/desktop/src/main/services/lanes/laneService.test.ts +++ b/apps/desktop/src/main/services/lanes/laneService.test.ts @@ -4098,10 +4098,37 @@ describe("laneService delete teardown + cancellation + streaming", () => { expect(count("conflict_proposals", "lane_id = ? or peer_lane_id = ?", ["lane-child", "lane-child"])).toBe(0); expect(count("files_workspaces", "lane_id = ?", ["lane-child"])).toBe(0); expect(count("file_directory_snapshots", "workspace_id = ?", ["workspace-child"])).toBe(0); - expect(count("pull_requests", "id = ?", ["pr-child"])).toBe(0); + // The PR row is DETACHED, not deleted: a merged PR outlives its lane, and deleting + // the row is what made merged PRs render as `unmapped` in the PRs tab. + expect(count("pull_requests", "id = ?", ["pr-child"])).toBe(1); + const detachedPr = db.get<{ + lane_id: string; + detached_at: string | null; + detached_lane_name: string | null; + detached_provenance: string | null; + }>( + "select lane_id, detached_at, detached_lane_name, detached_provenance from pull_requests where id = ?", + ["pr-child"], + ); + expect(detachedPr?.detached_at).toBeTruthy(); + // lane_id deliberately still points at the deleted lane — it is NOT NULL, CRR + // strips the FK, and it stays useful as a provenance key. + expect(detachedPr?.lane_id).toBe("lane-child"); + expect(detachedPr?.detached_lane_name).toBeTruthy(); + expect(JSON.parse(detachedPr?.detached_provenance ?? "{}")).toMatchObject({ + chats: expect.any(Number), + artifacts: expect.any(Number), + checkpoints: expect.any(Number), + }); expect(count("pr_group_members", "lane_id = ?", ["lane-child"])).toBe(0); - expect(count("pull_request_ai_summaries", "pr_id = ?", ["pr-child"])).toBe(0); - expect(count("pull_request_snapshots", "pr_id = ?", ["pr-child"])).toBe(0); + // The snapshot row survives with the bulky kinds purged, so storage does not grow. + expect(count("pull_request_snapshots", "pr_id = ?", ["pr-child"])).toBe(1); + expect( + db.get<{ files_json: string | null; checks_json: string | null }>( + "select files_json, checks_json, comments_json, reviews_json from pull_request_snapshots where pr_id = ?", + ["pr-child"], + ), + ).toEqual({ files_json: null, checks_json: null, comments_json: null, reviews_json: null }); expect(count("pr_auto_link_ignores", "lane_id = ?", ["lane-child"])).toBe(0); expect(count("review_runs", "id = ?", ["review-run-child"])).toBe(0); expect(count("review_reviewer_runs", "id = ?", ["reviewer-run-child"])).toBe(0); @@ -4746,13 +4773,22 @@ describe("laneService - branchSwitch", () => { ); expect(keep?.lane_id).toBe("lane-a"); - const stale = db.get<{ lane_id: string | null }>( - "select lane_id from pull_requests where id = ?", + // Stale rows are detached, not deleted — the PR happened on this lane even + // though the lane now tracks a different branch. + const stale = db.get<{ lane_id: string | null; detached_at: string | null; detached_lane_name: string | null }>( + "select lane_id, detached_at, detached_lane_name from pull_requests where id = ?", ["pr-stale"], ); - expect(stale).toBeNull(); - expect(db.get<{ count: number }>("select count(1) as count from pull_request_ai_summaries where pr_id = ?", ["pr-stale"])?.count).toBe(0); - expect(db.get<{ count: number }>("select count(1) as count from pull_request_snapshots where pr_id = ?", ["pr-stale"])?.count).toBe(0); + expect(stale?.detached_at).toBeTruthy(); + expect(stale?.lane_id).toBe("lane-a"); + // A detached row must not be treated as live: the kept row is the only one + // still claiming the lane's current branch. + expect( + db.get<{ count: number }>( + "select count(1) as count from pull_requests where lane_id = ? and detached_at is null", + ["lane-a"], + )?.count, + ).toBe(1); } finally { db.close(); fs.rmSync(repoRoot, { recursive: true, force: true }); diff --git a/apps/desktop/src/main/services/lanes/laneService.ts b/apps/desktop/src/main/services/lanes/laneService.ts index 38994d2fb..6d895c492 100644 --- a/apps/desktop/src/main/services/lanes/laneService.ts +++ b/apps/desktop/src/main/services/lanes/laneService.ts @@ -4,7 +4,7 @@ import { AsyncLocalStorage } from "node:async_hooks"; import { randomUUID } from "node:crypto"; import type { AdeDb } from "../state/kvDb"; import { getHeadSha, runGit, runGitOrThrow } from "../git/git"; -import { deletePullRequestRowsByIds, deletePullRequestRowsForLane } from "../prs/pullRequestRowCleanup"; +import { detachPullRequestRowsByIds, detachPullRequestRowsForLane } from "../prs/pullRequestRowCleanup"; import { isWithinDir, normalizeBranchName, resolvePathWithinRoot } from "../shared/utils"; import { fetchRemoteTrackingBranch } from "../shared/remoteTrackingBranch"; import { detectConflictKind } from "../git/gitConflictState"; @@ -2397,6 +2397,7 @@ export function createLaneService({ where pr.project_id = l.project_id and pr.lane_id = l.id and pr.state in ('open', 'draft') + and pr.detached_at is null ) `, [projectId, normalizedDefaultBaseRef], @@ -2419,6 +2420,7 @@ export function createLaneService({ where pr.project_id = lanes.project_id and pr.lane_id = lanes.id and pr.state in ('open', 'draft') + and pr.detached_at is null ) `, [normalizedDefaultBaseRef, projectId, normalizedDefaultBaseRef], @@ -3144,7 +3146,20 @@ export function createLaneService({ db.run("update integration_proposals set integration_lane_id = null where integration_lane_id = ? and project_id = ?", [laneId, projectId]); db.run("update integration_proposals set preferred_integration_lane_id = null where preferred_integration_lane_id = ? and project_id = ?", [laneId, projectId]); - deletePullRequestRowsForLane(db, projectId, laneId); + // Soft-detach rather than delete: the PR outlives the lane, and deleting the row is + // what made every merged PR show up as `unmapped` once its lane was cleaned up. + // Runs before the session/artifact/checkpoint deletes below, because it counts them. + const detachedLane = db.get<{ name: string | null; color: string | null }>( + "select name, color from lanes where id = ? and project_id = ?", + [laneId, projectId], + ); + detachPullRequestRowsForLane(db, { + projectId, + laneId, + laneName: detachedLane?.name ?? null, + laneColor: detachedLane?.color ?? null, + detachedAt: new Date().toISOString(), + }); db.run("delete from pr_auto_link_ignores where lane_id = ? and project_id = ?", [laneId, projectId]); db.run("delete from review_run_publications where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); @@ -4476,12 +4491,11 @@ export function createLaneService({ `, [targetBranchRef, baseRef, parentLaneId, row.id, projectId], ); - // Drop any PR rows still associated with this lane whose head_branch - // no longer matches the lane's current branch — those references are - // stale after a branch switch and must not bleed into PR lookups. - // pull_requests.lane_id is NOT NULL, so we DELETE (mirrors the explicit - // child-row cleanup used by the lane-delete path; CRR conversion can - // strip FK cascades). + // PR rows whose head_branch no longer matches the lane's current branch are + // stale references and must not bleed into PR lookups. Detach rather than + // delete: the PR still happened on this lane, and erasing it is what made + // merged PRs render as `unmapped`. detached_at takes them out of live lookups + // while keeping the history. const stalePrRows = db.all<{ id: string }>( ` select id from pull_requests @@ -4492,8 +4506,14 @@ export function createLaneService({ [row.id, projectId, targetBranchRef], ); if (stalePrRows.length > 0) { - const stalePrIds = stalePrRows.map((r) => r.id); - deletePullRequestRowsByIds(db, projectId, stalePrIds); + detachPullRequestRowsByIds(db, { + projectId, + laneId: row.id, + laneName: row.name ?? null, + laneColor: row.color ?? null, + detachedAt: new Date().toISOString(), + prIds: stalePrRows.map((r) => r.id), + }); } db.run("commit"); } catch (err) { @@ -4667,7 +4687,9 @@ export function createLaneService({ ); // Same rationale as switchBranch: PR rows whose head branch no longer // matches the lane are stale references and must not bleed into PR - // lookups now that the lane tracks a different branch. + // lookups now that the lane tracks a different branch. Detached, not deleted, + // so the merged view keeps the record. `row` is the pre-update snapshot, so + // `row.name` is the name these PRs were actually built under. const stalePrRows = db.all<{ id: string }>( ` select id from pull_requests @@ -4678,7 +4700,14 @@ export function createLaneService({ [row.id, projectId, targetBranchRef], ); if (stalePrRows.length > 0) { - deletePullRequestRowsByIds(db, projectId, stalePrRows.map((entry) => entry.id)); + detachPullRequestRowsByIds(db, { + projectId, + laneId: row.id, + laneName: row.name ?? null, + laneColor: row.color ?? null, + detachedAt: new Date().toISOString(), + prIds: stalePrRows.map((entry) => entry.id), + }); } db.run("commit"); } catch (err) { @@ -5517,7 +5546,7 @@ export function createLaneService({ if (remoteTemp.exitCode !== 2) return skipBranch("remote_state_unavailable"); } const pr = db.get<{ id: string }>( - "select id from pull_requests where project_id = ? and lane_id = ? limit 1", + "select id from pull_requests where project_id = ? and lane_id = ? and detached_at is null limit 1", [projectId, args.laneId], ); if (pr) return skipBranch("pull_request_exists"); diff --git a/apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts b/apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts index 780620901..57a16bcda 100644 --- a/apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts +++ b/apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts @@ -106,6 +106,9 @@ export function createRebaseSuggestionService(args: { select lane_id from pull_requests where project_id = ? + -- Detached rows keep a dangling lane_id, so they would otherwise mark a + -- lane as PR-backed long after its PR stopped belonging to it. + and detached_at is null `, [projectId] ); diff --git a/apps/desktop/src/main/services/prs/prService.test.ts b/apps/desktop/src/main/services/prs/prService.test.ts index fefe1d675..b68fc4c00 100644 --- a/apps/desktop/src/main/services/prs/prService.test.ts +++ b/apps/desktop/src/main/services/prs/prService.test.ts @@ -381,6 +381,13 @@ function preflightConflicts(preflight: any): unknown[] { function installPullRequestRowStore(db: ReturnType, initialRows: any[] = []) { const rows = [...initialRows]; + // Lane-scoped and ownership lookups carry `detached_at is null`; identity lookups + // (by row id) deliberately do not. Honouring that here keeps the store faithful to + // the real queries — without it a detached row looks live to every lookup and the + // detach behaviour cannot be exercised. + const liveOnly = (text: string) => text.includes("detached_at is null"); + const matchesLiveness = (row: any, text: string) => !liveOnly(text) || !row.detached_at; + db.get.mockImplementation((sql: string, params: unknown[] = []) => { const text = String(sql); if (!text.includes("from pull_requests")) return null; @@ -394,10 +401,13 @@ function installPullRequestRowStore(db: ReturnType, initialRo && String(row.repo_owner).toLowerCase() === String(owner).toLowerCase() && String(row.repo_name).toLowerCase() === String(name).toLowerCase() && Number(row.github_pr_number) === Number(prNumber) + && matchesLiveness(row, text) ) ?? null; } if (text.includes("where lane_id = ?")) { - return rows.find((row) => row.lane_id === params[0] && row.project_id === params[1]) ?? null; + return rows.find((row) => + row.lane_id === params[0] && row.project_id === params[1] && matchesLiveness(row, text) + ) ?? null; } return null; }); @@ -406,10 +416,12 @@ function installPullRequestRowStore(db: ReturnType, initialRo const text = String(sql); if (!text.includes("from pull_requests")) return []; if (text.includes("where lane_id = ?")) { - return rows.filter((row) => row.lane_id === params[0] && row.project_id === params[1]); + return rows.filter((row) => + row.lane_id === params[0] && row.project_id === params[1] && matchesLiveness(row, text) + ); } if (text.includes("where project_id = ?")) { - return rows.filter((row) => row.project_id === params[0]); + return rows.filter((row) => row.project_id === params[0] && matchesLiveness(row, text)); } return rows; }); @@ -6483,3 +6495,158 @@ describe("prService.reconcileOnFocus", () => { expect(reconcileEvents.map((e) => e.state)).toEqual(["running", "idle"]); }); }); + +describe("prService detached PR rows", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + /** + * A PR outlives its lane: deleting the lane detaches the row rather than deleting + * it, so merged history keeps its ADE identity. These tests pin the three ways that + * previously went wrong. + */ + function detachedRow(overrides?: Record) { + return makePrRow({ + id: "pr-detached", + github_pr_number: 90, + detached_at: "2026-07-30T00:00:00Z", + detached_lane_name: "auto-naming", + detached_lane_color: "#4ADE80", + detached_provenance: JSON.stringify({ chats: 3, artifacts: 2, checkpoints: 5 }), + ...overrides, + }); + } + + function githubServiceForPr90(row: { github_url: string; title: string }) { + return makeGithubService({ + apiRequest: vi.fn(async (args: { method?: string; path: string }) => { + if (args.path === "/repos/test-owner/test-repo/pulls/90") { + return { + data: makeGitHubPull({ + number: 90, + html_url: row.github_url, + title: row.title, + state: "closed", + merged: true, + merged_at: "2026-07-29T00:00:00Z", + head: { ref: "my-feature", sha: "head-sha" }, + base: { ref: "main", sha: "base-sha" }, + }), + }; + } + if (args.path.includes("/status")) return { data: { state: "success", statuses: [] } }; + if (args.path.includes("/check-runs")) return { data: { check_runs: [] } }; + if (args.path.includes("/reviews")) return { data: [] }; + if (args.path.includes("/compare/")) return { data: { behind_by: 0 } }; + return { data: {} }; + }), + }); + } + + it("updates a detached row instead of re-inserting its primary key", async () => { + // The lane-branch lookup is live-only, so it cannot see a detached row. Resolving + // by identity first is what keeps this an UPDATE — otherwise every merged PR whose + // lane was deleted would hit a duplicate-primary-key insert on open. + const row = detachedRow(); + const db = makeMockDb(); + installPullRequestRowStore(db, [row]); + const { service } = buildService({ db, githubService: githubServiceForPr90(row) }); + + await service.getStatus("pr-detached"); + + const sqlRun: string[] = db.run.mock.calls.map(([sql]: [unknown]) => String(sql)); + expect(sqlRun.some((sql: string) => sql.includes("update pull_requests"))).toBe(true); + expect(sqlRun.some((sql: string) => sql.includes("insert into pull_requests"))).toBe(false); + }); + + it("keeps a detached row detached when its lane no longer exists", async () => { + // A background refresh must never resurrect history. The lane is gone, so nothing + // can reclaim the row. + const row = detachedRow(); + const db = makeMockDb(); + installPullRequestRowStore(db, [row]); + const { service } = buildService({ + db, + githubService: githubServiceForPr90(row), + laneService: makeLaneService([]), + }); + + await service.getStatus("pr-detached"); + + const clearedDetach = db.run.mock.calls.some(([sql]: [unknown]) => + String(sql).includes("set detached_at = null"), + ); + expect(clearedDetach).toBe(false); + }); + + it("keeps a detached row detached when its lane moved to another branch", async () => { + // switchBranch/rename detach while the lane lives on. Lane existence alone is not + // enough to reclaim — the lane must still track this PR's head branch, or a poll + // would reattach a PR to a lane that has moved on. + const row = detachedRow(); + const db = makeMockDb(); + installPullRequestRowStore(db, [row]); + db.get.mockImplementation((sql: string, params: unknown[] = []) => { + const text = String(sql); + if (text.includes("from lanes")) return { branch_ref: "refs/heads/some-other-branch" }; + if (!text.includes("from pull_requests")) return null; + if (text.includes("where id = ?")) { + return params[0] === row.id ? row : null; + } + return null; + }); + const { service } = buildService({ db, githubService: githubServiceForPr90(row) }); + + await service.getStatus("pr-detached"); + + const clearedDetach = db.run.mock.calls.some(([sql]: [unknown]) => + String(sql).includes("set detached_at = null"), + ); + expect(clearedDetach).toBe(false); + }); + + it("reclaims a detached row when a lane on the same branch takes it back", async () => { + const row = detachedRow(); + const db = makeMockDb(); + installPullRequestRowStore(db, [row]); + db.get.mockImplementation((sql: string, params: unknown[] = []) => { + const text = String(sql); + if (text.includes("from lanes")) return { branch_ref: "refs/heads/my-feature" }; + if (!text.includes("from pull_requests")) return null; + if (text.includes("where id = ?")) { + return params[0] === row.id ? row : null; + } + return null; + }); + const { service } = buildService({ db, githubService: githubServiceForPr90(row) }); + + await service.getStatus("pr-detached"); + + const clearCall = db.run.mock.calls.find(([sql]: [unknown]) => + String(sql).includes("set detached_at = null"), + ); + expect(clearCall).toBeTruthy(); + // The dead lane's provenance goes with the marker — it describes a lane this PR + // no longer belongs to. + expect(String(clearCall?.[0])).toContain("detached_lane_name = null"); + expect(String(clearCall?.[0])).toContain("detached_provenance = null"); + }); + + it("exposes frozen lane provenance on the summary of a detached row", async () => { + const row = detachedRow(); + const db = makeMockDb(); + installPullRequestRowStore(db, [row]); + const { service } = buildService({ db }); + + const detached = service.listAll().find((entry: { id: string }) => entry.id === "pr-detached"); + + expect(detached?.detached).toMatchObject({ + laneName: "auto-naming", + laneColor: "#4ADE80", + chats: 3, + artifacts: 2, + }); + expect(detached?.laneId).toBe(LANE_ID); + }); +}); diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 8315372bf..07d9a6726 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -56,6 +56,8 @@ import type { PrCommit, PrConflictAnalysis, PrCreationStrategy, + PrDetachedLane, + PrMergedBy, PrEventPayload, PrGroupMemberRole, PrHealth, @@ -215,6 +217,17 @@ type PullRequestRow = { updated_at: string; merged_at: string | null; creation_strategy: string | null; + /** Set when the lane was deleted or moved to another branch. See pullRequestRowCleanup. */ + detached_at?: string | null; + detached_lane_name?: string | null; + detached_lane_color?: string | null; + /** JSON `{ chats, artifacts, checkpoints }` frozen at detach time. */ + detached_provenance?: string | null; + merged_by_login?: string | null; + merged_by_avatar_url?: string | null; + merge_method?: string | null; + commit_count?: number | null; + changed_files?: number | null; }; type PrAutoLinkIgnoreRow = { @@ -1046,7 +1059,53 @@ function rowToSummary(row: PullRequestRow): PrSummary { createdAt: row.created_at, updatedAt: row.updated_at, mergedAt: row.merged_at, - creationStrategy + creationStrategy, + detached: rowDetachedLane(row), + mergedBy: rowMergedBy(row), + mergeMethod: normalizeMergeMethod(row.merge_method), + commitCount: normalizeCount(row.commit_count), + changedFiles: normalizeCount(row.changed_files) + }; +} + +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; +} + +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, }; } @@ -1416,7 +1475,16 @@ export function createPrService({ const PR_COLUMNS = `id, lane_id, project_id, repo_owner, repo_name, github_pr_number, github_url, github_node_id, title, state, base_branch, head_branch, checks_status, review_status, additions, deletions, last_synced_at, - created_at, updated_at, merged_at, creation_strategy, merge_conflicts, behind_base_by, head_sha`; + created_at, updated_at, merged_at, creation_strategy, merge_conflicts, behind_base_by, head_sha, + detached_at, detached_lane_name, detached_lane_color, detached_provenance, + merged_by_login, merged_by_avatar_url, merge_method, commit_count, changed_files`; + /** + * Lane-scoped "what is this lane working on" lookups must ignore detached rows. + * A detached row is history: its lane was deleted, or the lane moved to another + * branch. It still belongs in project-wide reads (that is how the merged view gets + * its provenance back), just not in live lane state. + */ + const LIVE_PR_ROWS = "detached_at is null"; const GITHUB_PROJECTION_COLUMNS = `project_id, repo_owner, repo_name, github_pr_number, github_node_id, github_url, title, state, is_draft, base_branch, head_branch, head_repo_owner, head_repo_name, head_sha, base_sha, author, labels_json, @@ -1662,6 +1730,28 @@ export function createPrService({ [projectId, repoOwner, repoName, prNumber] ); + /** + * "Does a lane already own this PR?" — the question every mapping guard asks. + * + * Distinct from `getRowForRepoPr`, which answers "is there a row for these + * coordinates at all" and must still see detached rows so `upsertRow` updates them + * instead of inserting a duplicate primary key. A detached row owns nothing: its + * lane was deleted or moved on, so it must never block re-mapping. + */ + const getLiveRowForRepoPr = (repoOwner: string, repoName: string, prNumber: number): PullRequestRow | null => + db.get( + `select ${PR_COLUMNS} + from pull_requests + where project_id = ? + and lower(repo_owner) = lower(?) + and lower(repo_name) = lower(?) + and github_pr_number = ? + and ${LIVE_PR_ROWS} + order by updated_at desc + limit 1`, + [projectId, repoOwner, repoName, prNumber] + ); + const getRowByNumber = ( prNumber: number, repoOwner?: string, @@ -1759,6 +1849,7 @@ export function createPrService({ from pull_requests where lane_id = ? and project_id = ? + and ${LIVE_PR_ROWS} order by case when state in ('open', 'draft') then 0 else 1 end, updated_at desc, @@ -1894,6 +1985,10 @@ export function createPrService({ and lower(mapped.repo_owner) = lower(projection.repo_owner) and lower(mapped.repo_name) = lower(projection.repo_name) and mapped.github_pr_number = projection.github_pr_number + -- A detached row no longer speaks for this PR, so it must not + -- suppress the projection fallback — otherwise a lane recreated on + -- the same branch shows no PR at all. + and mapped.${LIVE_PR_ROWS} ) order by projection.updated_at desc, projection.created_at desc `, @@ -1929,7 +2024,7 @@ export function createPrService({ const lane = getLanePrLookupRow(laneId); if (!lane || lane.archived_at) return null; const mappedRows = db.all( - `select ${PR_COLUMNS} from pull_requests where lane_id = ? and project_id = ?`, + `select ${PR_COLUMNS} from pull_requests where lane_id = ? and project_id = ? and ${LIVE_PR_ROWS}`, [laneId, projectId], ); return selectLanePrDisplayCandidate({ @@ -1951,6 +2046,7 @@ export function createPrService({ where lane_id = ? and project_id = ? and state in ('open', 'draft') + and ${LIVE_PR_ROWS} order by updated_at desc, created_at desc `, [laneId, projectId], @@ -1976,6 +2072,7 @@ export function createPrService({ where lane_id = ? and project_id = ? and head_branch = ? + and ${LIVE_PR_ROWS} order by updated_at desc limit 1 `, @@ -2412,11 +2509,37 @@ export function createPrService({ // creation because a PR already exists for the head branch) is the only // legitimate use of the repo/PR-number fallback; it opts in via // `allowRepoPrAdoption: true`. - const existing = options?.allowRepoPrAdoption - ? getRowForLaneBranch(summary.laneId, summary.headBranch) - ?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber) - : getRowForLaneBranch(summary.laneId, summary.headBranch); + // Identity first. The lane-branch lookup is deliberately live-only (it answers + // "what is this lane working on"), so on its own it would miss a detached row and + // send us down the insert path with a primary key that already exists. + const existing = getRowById(summary.id) + ?? (options?.allowRepoPrAdoption + ? getRowForLaneBranch(summary.laneId, summary.headBranch) + ?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber) + : getRowForLaneBranch(summary.laneId, summary.headBranch)); if (existing) { + // A detached row is only reclaimed by a lane that still exists AND still tracks + // the PR's head branch. + // + // Lane existence alone is not enough: `switchBranch`/`rename` detach rows while + // the lane lives on, so a background refresh would otherwise reattach a PR to a + // lane that has moved to a different branch — reinstating exactly the stale + // reference the old DELETE prevented, and destroying the provenance snapshot on + // the way. The branch check also settles archived lanes correctly: one that still + // tracks the branch may reclaim its PR, one that moved on may not. + const reattachesToLiveLane = Boolean( + existing.detached_at + && (() => { + const lane = db.get<{ branch_ref: string | null }>( + "select branch_ref from lanes where id = ? and project_id = ?", + [summary.laneId, projectId], + ); + if (!lane) return false; + const laneBranch = normalizeBranchName(branchNameFromRef(lane.branch_ref ?? "")); + const prBranch = normalizeBranchName(branchNameFromRef(summary.headBranch ?? "")); + return Boolean(laneBranch) && laneBranch === prBranch; + })(), + ); if (existing.lane_id !== summary.laneId) { db.run(`delete from pr_group_members where pr_id = ?`, [existing.id]); db.run(`update integration_proposals set linked_pr_id = null where linked_pr_id = ?`, [existing.id]); @@ -2476,6 +2599,23 @@ export function createPrService({ projectId, ] ); + if (reattachesToLiveLane) { + // A lane that still exists has claimed this row back, so it is live work + // again: the detach marker goes, and with it the dead lane's provenance. + // Kept separate from the upsert so a plain refresh of a detached row — whose + // `lane_id` still names its deleted lane — can never resurrect it. + db.run( + ` + update pull_requests + set detached_at = null, + detached_lane_name = null, + detached_lane_color = null, + detached_provenance = null + where id = ? and project_id = ? + `, + [existing.id, projectId], + ); + } return existing.id; } @@ -2718,7 +2858,7 @@ export function createPrService({ if (!autoMapByBranchEnabled()) return null; // Guard #5: PR not already mapped to any lane. - if (getRowForRepoPr(repo.owner, repo.name, candidate.prNumber)) return null; + if (getLiveRowForRepoPr(repo.owner, repo.name, candidate.prNumber)) return null; // Guard #3: exactly one non-archived worktree lane whose head branch // matches the PR head branch. Zero or >1 → do nothing. @@ -2890,7 +3030,7 @@ export function createPrService({ if (ignoredAutoLinks.has(autoLinkIgnoreKey({ owner: repo.owner, repo: repo.name, prNumber, laneId: lane.id }))) { continue; } - const existingRepoRow = getRowForRepoPr(repo.owner, repo.name, prNumber); + const existingRepoRow = getLiveRowForRepoPr(repo.owner, repo.name, prNumber); if (existingRepoRow && existingRepoRow.lane_id !== lane.id) continue; const state = toPrState({ state: asString(rawPr?.state) || "open", @@ -3941,7 +4081,7 @@ export function createPrService({ blockingConflicts: block ? [block] : [], }); - const existingPr = getRowForRepoPr(repo.owner, repo.name, githubPrNumber); + const existingPr = getLiveRowForRepoPr(repo.owner, repo.name, githubPrNumber); if (existingPr) { const lane = (await laneService.list({ includeArchived: true, includeStatus: false })) .find((entry) => entry.id === existingPr.lane_id); @@ -4981,6 +5121,18 @@ export function createPrService({ }; upsertRow(refreshed); + // A merged PR should be able to describe how it shipped without another GitHub + // call. `pr` here is the single-PR payload, which carries merged_by/commits/ + // changed_files; this is the only place we see them for free. + if (status.state === "merged") { + recordMergeOutcome(summary.id, { + mergedByLogin: asString(pr?.merged_by?.login) || null, + mergedByAvatarUrl: asString(pr?.merged_by?.avatar_url) || null, + commitCount: normalizeCount(pr?.commits), + changedFiles: normalizeCount(pr?.changed_files), + }); + } + return status; }; @@ -6310,7 +6462,7 @@ export function createPrService({ // that default here so linked PRs participate in strategy-aware rebase // behavior (follow-up 3) instead of being treated as "unset". The // upsertRow path uses COALESCE so we never clobber an existing value. - const existingRow = getRowForRepoPr(repo.owner, repo.name, locator.number); + const existingRow = getLiveRowForRepoPr(repo.owner, repo.name, locator.number); if (existingRow && existingRow.lane_id !== lane.id) { const existingLane = (await laneService.list({ includeArchived: true, includeStatus: false })) .find((entry) => entry.id === existingRow.lane_id); @@ -6849,6 +7001,14 @@ export function createPrService({ const mergeCommitSha = asString(merge.data?.sha) || null; + // Record how this shipped. GitHub's merge response does not name the actor, but + // we merged as the authenticated viewer, so that is who merged it. Without this + // the merged view could never say more than "merged at