PRs: keep merged pull requests after their lane is deleted - #988
Merged
Conversation
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: <lane>` 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 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds soft-detached pull-request records with preserved lane provenance and merge metadata. Active lookups exclude detached rows. Desktop, iOS, and CLI views display terminal PR history and grouped results. ChangesDetached pull-request history
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arul28
changed the title
The pull_request event produced no GitHub Actions check suite for d56fbb4 — railway, cursor, graphite, vercel, mintlify and greptile all registered one, Actions alone did not, and close/reopen did not fire it either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx (1)
1081-1096: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPeriod grouping assumes an order the callers do not guarantee. Both grouping helpers key a row on the merge timestamp, while both list callers sort by update time or by a user-selected sort option. When the two orders disagree, one period emits several headers and the period id repeats, which also breaks list-row identity.
apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx#L1081-L1096: sortfilteredItemsbyprListGroupTimestampwhenfilterismergedorclosed, and reuse that flag forbuildPrListRows.apps/desktop/src/renderer/components/prs/shared/prListGrouping.ts#L99-L130: keep the documented newest-first contract, and make the period id unique per occurrence so a repeated period cannot collide on the React key.apps/ios/ADE/Views/PRs/PrsRootScreen.swift#L834-L851: sort the merged and closedrepoItemsbyprListGroupDatebefore callingprListPeriodGroups, soForEachreceives uniquePrListPeriodGroupids.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx` around lines 1081 - 1096, Update apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx lines 1081-1096 to reuse a merged/closed grouping flag, sort filteredItems by prListGroupTimestamp for those filters, and pass the flag to buildPrListRows. In apps/desktop/src/renderer/components/prs/shared/prListGrouping.ts lines 99-130, preserve the newest-first contract and generate unique period ids for repeated occurrences. In apps/ios/ADE/Views/PRs/PrsRootScreen.swift lines 834-851, sort merged and closed repoItems by prListGroupDate before calling prListPeriodGroups so ForEach receives unique group ids.apps/desktop/src/main/services/lanes/laneService.ts (1)
4494-4517: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winExclude already-detached rows from the stale-PR lookup.
In
switchBranch(Lines 4499-4507) andresolveBranchDrift(Lines 4693-4701), thestalePrRowsquery selects bylane_id,project_id, andhead_branch <> targetBranchRef. It does not filterdetached_at is null.Before this change, a matched row was hard-deleted, so it could never reappear in a later switch. Now the matched rows are soft-detached and keep the same
lane_idand the same (now permanently stale)head_branch, so every later branch switch on the same lane re-selects them again. Each re-selection rerunsdetachPullRequestRowsByIds:countLaneProvenance, the commit/file-count UPDATE, the snapshot-nulling UPDATE, and thepr_group_membersDELETE all execute again against rows that are already historical. This is idempotent, so it is not a correctness bug, but the set of reprocessed rows grows without bound as a lane keeps switching branches.Add
and detached_at is nullto both queries so they select only rows that still need to be detached.⚡ Proposed fix for both occurrences
const stalePrRows = db.all<{ id: string }>( ` select id from pull_requests where lane_id = ? and project_id = ? and head_branch <> ? + and detached_at is null `, [row.id, projectId, targetBranchRef], );Also applies to: 4688-4711
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 4494 - 4517, Update the stalePrRows queries in both switchBranch and resolveBranchDrift to add a detached_at is null predicate, so already-detached pull request rows are excluded from subsequent stale-row processing while active stale rows continue through detachPullRequestRowsByIds.
🧹 Nitpick comments (5)
apps/ios/ADE/Services/Database.swift (1)
3016-3021: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe new columns are mirrored but never read.
The schema now carries
merged_by_login,merged_by_avatar_url,merge_method,commit_count,changed_files, and thedetached_*columns.fetchPullRequestsLockedandfetchPullRequestListItemsLockeddo not select them, andreplacePullRequestHydrationLockeddoes not write them. SoPullRequestListItem.mergedBy,mergeMethod,commitCount,changedFiles, anddetachedstay nil for a mapped PR.PrDetailScreen.shippedFactsthen shows merge facts only when aGitHubPrListItemis present.The PR description records this as follow-up work, so the mirroring here is the correct scope. Do you want me to extend the SELECT list, the row struct, and the upsert so the mapped row supplies these facts?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Services/Database.swift` around lines 3016 - 3021, The new pull-request columns are added to the schema but are not persisted or loaded, leaving mapped merge and detached facts nil. Extend fetchPullRequestsLocked and fetchPullRequestListItemsLocked to select the mirrored fields, add them to the corresponding row struct, and update replacePullRequestHydrationLocked to write them while preserving existing GitHubPrListItem mappings.apps/ios/ADE/Views/PRs/PrsRootScreen.swift (1)
842-842: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMemoize the period groups.
prListPeriodGroups(repoItems)runs on everybodypass. This file already memoizes the filter, sort, and count derivations ingithubDerivedfor that reason. Compute the groups inrecomputeGitHubDerivedand store them in state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Views/PRs/PrsRootScreen.swift` at line 842, Update the GitHub-derived state flow by computing prListPeriodGroups(repoItems) inside recomputeGitHubDerived and storing the result in githubDerived alongside the existing filter, sort, and count values. Change the ForEach in PRsRootScreen body to read the memoized groups from githubDerived instead of invoking prListPeriodGroups(repoItems) during rendering.apps/ios/ADE/Views/PRs/PrHelpers.swift (1)
1142-1142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the
ISO8601DateFormatterand align formatter safety with the file's existing pattern.Line 1142 allocates a new
ISO8601DateFormatterfor every grouped row.prListPeriodGroupscalls this once per item, so a long merged history pays repeated allocation on the render path. Store one formatter next toprDayMonthFormatter.This file already guards
prIsoFormatterwithprDateFormatterLock, becauseDateFormatteris not thread-safe. The new global formatters have no such guard. IfprListGroupLabelcan run off the main actor, use the same lock. Under Swift 6 strict concurrency, a globalletof a non-Sendableclass also needsnonisolated(unsafe)or an isolation annotation. Please confirm the package's Swift language mode.♻️ Proposed change
- let key = ISO8601DateFormatter().string(from: itemWeek.start).prefix(10) + let key = prWeekKeyFormatter.string(from: itemWeek.start).prefix(10)let prWeekKeyFormatter = ISO8601DateFormatter()Also applies to: 1187-1197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Views/PRs/PrHelpers.swift` at line 1142, Hoist a shared ISO8601DateFormatter beside prDayMonthFormatter and reuse it in prListPeriodGroups and the related prListGroupLabel path instead of allocating one per item. Align access with the existing prDateFormatterLock when these helpers may run off the main actor, and apply the package’s Swift 6 concurrency requirement (such as nonisolated(unsafe) or the established isolation annotation) to the shared formatter.apps/desktop/src/main/services/state/kvDb.ts (1)
2400-2421: 🗄️ Data Integrity & Integration | 🔵 TrivialConfirm the iOS mirror schema is updated before this ships.
pull_requestsis a CRR-replicated table. This migration adds nine new columns (detached_at,detached_lane_name,detached_lane_color,detached_provenance,merged_by_login,merged_by_avatar_url,merge_method,commit_count,changed_files) to it. An earlier comment in this same file, forterminal_sessions.settle_override, documents the established failure mode for this exact situation: a missing iOS half does not fail on desktop, it surfaces as changeset-apply errors on the phone. The PR objectives state that the iOS mirror's SELECT/upsert code does not yet include these new columns. Confirm thatapps/ios/ADE/Resources/DatabaseBootstrap.sqlandDatabase.swift'sensureColumnmigrations add matching columns before this ships, and thatapps/desktop/src/shared/types/prs.tsis updated to carry the new fields across the IPC boundary.Separately,
detached_atrows are never purged (per the PR design, they persist indefinitely to keep merged-PR history). Consider whether the existingidx_pull_requests_project_idindex is sufficient for project-wide merged-view queries as this table grows, or whether a composite index on(project_id, detached_at)or(project_id, state)would help.Based on the coding guideline "Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes" and the PR objectives noting "Local iOS mirror SELECT/upsert code does not yet include the new columns."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/state/kvDb.ts` around lines 2400 - 2421, Update the iOS mirror schema and synchronization paths for all nine new pull_requests fields: add matching columns in DatabaseBootstrap.sql, include them in Database.swift ensureColumn migrations and SELECT/upsert logic, and extend apps/desktop/src/shared/types/prs.ts so the fields cross the IPC boundary. Also assess the existing idx_pull_requests_project_id against detached merged-view query patterns and add an appropriate composite index if needed.Source: Coding guidelines
apps/desktop/src/main/services/prs/prService.test.ts (1)
6498-6652: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood regression coverage for the detach/reattach identity behavior.
The new suite exercises the three important cases: update-not-reinsert by identity, staying detached when the lane is gone or moved to another branch, and reclaiming a detached row when a lane returns to the same branch. This directly backs the
upsertRowreattach logic.None of these tests exercise
linkToLane's manual re-link path, which is where the missingallowRepoPrAdoptiongap was found (see theprService.tscomment on Line 6465). Consider adding a case that callslinkToLaneagainst a detached row on the same branch to lock in the fix once applied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.test.ts` around lines 6498 - 6652, Add regression coverage for the manual relink path by calling linkToLane with a detached PR row whose lane is on the same branch. Assert the operation succeeds and preserves the expected adoption behavior controlled by allowRepoPrAdoption, complementing the existing getStatus reattach tests. Use the existing detachedRow, database setup, and service builders in the detached PR suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 2512-2542: The reattachment path in upsertRow must not clear
detached provenance for terminal PRs selected by bestByLane. Require the
incoming summary state to be open or draft before treating an existing detached
row as reattachesToLiveLane, while preserving the existing lane and branch
checks for in-flight PRs.
In `@apps/ios/ADE/Views/PRs/PrRowCard.swift`:
- Around line 130-136: Update accessibilitySummary and the related live-state
text around the visible row summary logic to include branch-pair, CI, and
review-state details only when !data.isTerminal, while preserving provenance,
lane, merge, and cleanup text as appropriate. Update warnMessage so it returns
nil for terminal rows, preventing PrWarnBanner from displaying stale CI or
workflow data.
---
Outside diff comments:
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 4494-4517: Update the stalePrRows queries in both switchBranch and
resolveBranchDrift to add a detached_at is null predicate, so already-detached
pull request rows are excluded from subsequent stale-row processing while active
stale rows continue through detachPullRequestRowsByIds.
In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx`:
- Around line 1081-1096: Update
apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx lines 1081-1096 to
reuse a merged/closed grouping flag, sort filteredItems by prListGroupTimestamp
for those filters, and pass the flag to buildPrListRows. In
apps/desktop/src/renderer/components/prs/shared/prListGrouping.ts lines 99-130,
preserve the newest-first contract and generate unique period ids for repeated
occurrences. In apps/ios/ADE/Views/PRs/PrsRootScreen.swift lines 834-851, sort
merged and closed repoItems by prListGroupDate before calling prListPeriodGroups
so ForEach receives unique group ids.
---
Nitpick comments:
In `@apps/desktop/src/main/services/prs/prService.test.ts`:
- Around line 6498-6652: Add regression coverage for the manual relink path by
calling linkToLane with a detached PR row whose lane is on the same branch.
Assert the operation succeeds and preserves the expected adoption behavior
controlled by allowRepoPrAdoption, complementing the existing getStatus reattach
tests. Use the existing detachedRow, database setup, and service builders in the
detached PR suite.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 2400-2421: Update the iOS mirror schema and synchronization paths
for all nine new pull_requests fields: add matching columns in
DatabaseBootstrap.sql, include them in Database.swift ensureColumn migrations
and SELECT/upsert logic, and extend apps/desktop/src/shared/types/prs.ts so the
fields cross the IPC boundary. Also assess the existing
idx_pull_requests_project_id against detached merged-view query patterns and add
an appropriate composite index if needed.
In `@apps/ios/ADE/Services/Database.swift`:
- Around line 3016-3021: The new pull-request columns are added to the schema
but are not persisted or loaded, leaving mapped merge and detached facts nil.
Extend fetchPullRequestsLocked and fetchPullRequestListItemsLocked to select the
mirrored fields, add them to the corresponding row struct, and update
replacePullRequestHydrationLocked to write them while preserving existing
GitHubPrListItem mappings.
In `@apps/ios/ADE/Views/PRs/PrHelpers.swift`:
- Line 1142: Hoist a shared ISO8601DateFormatter beside prDayMonthFormatter and
reuse it in prListPeriodGroups and the related prListGroupLabel path instead of
allocating one per item. Align access with the existing prDateFormatterLock when
these helpers may run off the main actor, and apply the package’s Swift 6
concurrency requirement (such as nonisolated(unsafe) or the established
isolation annotation) to the shared formatter.
In `@apps/ios/ADE/Views/PRs/PrsRootScreen.swift`:
- Line 842: Update the GitHub-derived state flow by computing
prListPeriodGroups(repoItems) inside recomputeGitHubDerived and storing the
result in githubDerived alongside the existing filter, sort, and count values.
Change the ForEach in PRsRootScreen body to read the memoized groups from
githubDerived instead of invoking prListPeriodGroups(repoItems) during
rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03a0865c-b3e5-4670-9c3e-7fca156b2aed
⛔ Files ignored due to path filters (3)
docs/features/lanes/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (32)
apps/ade-cli/src/cli.tsapps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.tsapps/ade-cli/src/tuiClient/rightPaneFormatters.tsapps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/lanes/autoRebaseService.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/prs/pullRequestRowCleanup.test.tsapps/desktop/src/main/services/prs/pullRequestRowCleanup.tsapps/desktop/src/main/services/review/reviewContextBuilder.test.tsapps/desktop/src/main/services/review/reviewContextBuilder.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/PrDetailMergeRail.test.tsxapps/desktop/src/renderer/components/prs/shared/PrDetailMergeRail.tsxapps/desktop/src/renderer/components/prs/shared/prListGrouping.test.tsapps/desktop/src/renderer/components/prs/shared/prListGrouping.tsapps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/shared/types/prs.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Views/PRs/PrDetailOverviewTab.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swiftapps/ios/ADE/Views/PRs/PrHelpers.swiftapps/ios/ADE/Views/PRs/PrListRowModifier.swiftapps/ios/ADE/Views/PRs/PrRowCard.swiftapps/ios/ADE/Views/PRs/PrsRootScreen.swift
Comment on lines
+2512
to
+2542
| // 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; | ||
| })(), | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect candidate selection in backfillLanePrRowsFromGithubPulls and confirm
# whether closed/merged PRs can win as "best" candidates for a lane's branch.
rg -n "bestByLane" -B5 -A40 apps/desktop/src/main/services/prs/prService.tsRepository: arul28/ADE
Length of output: 3362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Outline prService relevant symbols =="
ast-grep outline apps/desktop/src/main/services/prs/prService.ts --match upsertRow --view expanded || true
ast-grep outline apps/desktop/src/main/services/prs/prService.ts --match getRowById --view expanded || true
ast-grep outline apps/desktop/src/main/services/prs/prService.ts --match getRowForRepoPr --view expanded || true
echo
echo "== Section around reattachment block =="
sed -n '2480,2635p' apps/desktop/src/main/services/prs/prService.ts | cat -n
echo
echo "== Section around upsertRow implementation =="
sed -n '2000,2250p' apps/desktop/src/main/services/prs/prService.ts | cat -nRepository: arul28/ADE
Length of output: 19808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate relevant declarations and query macros =="
rg -n "const getRowForRepoPr|function getRowForRepoPr|getRowForRepoPr|LIVE_PR_ROWS|isActivePrState|rowMatchesLaneBranchForDisplay|projectionMatchesLaneBranchForDisplay" apps/desktop/src/main/services/prs/prService.ts
echo
echo "== getRowForRepoPr definition and nearby rows =="
rg -n -B8 -A80 "const getRowForRepoPr|function getRowForRepoPr" apps/desktop/src/main/services/prs/prService.ts | cat -n
echo
echo "== Macros around LIVE_PR_ROWS and candidate sort helpers =="
rg -n -B8 -A25 "LIVE_PR_ROWS|isActivePrState|rowMatchesLaneBranchForDisplay|projectionMatchesLaneBranchForDisplay|compareLanePrDisplayCandidates" apps/desktop/src/main/services/prs/prService.ts | cat -nRepository: arul28/ADE
Length of output: 27995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Exact upsert update block tail =="
sed -n '2620,2665p' apps/desktop/src/main/services/prs/prService.ts | cat -n
echo
echo "== Read-only deterministic model of reattachment conditions from source =="
python3 - <<'PY'
from pathlib import Path
import re
src = Path("apps/desktop/src/main/services/prs/prService.ts").read_text()
m_best = re.search(
r"const\s+bestByLane\s*=\s*new\s+Map<\w+>\s*\(\)\s*;\s*"
r"for\s*\(\s*const\s+rawPr\s+of\s+rawPulls\s*\)\s*\{"
r"(?P<body>.*?)for\s*\(\s*const\s+\{ rawPr[\s\S]+?\n\s*\}\s+of\s+bestByLane\.values\(\)\s*\)",
src,
)
if m_best:
print("bestByLane_candidate_order_found")
body = m_best.group("body")
print("open_draft_can_be_preferred", 'isOpen && !prev.isOpen' in body)
print("newest_can_be_preferred", 'isOpen === prev.isOpen && prNumber > prev.prNumber' in body)
print("no_state_guard_in_bestByLane_loop", '"merged"' not in body and '"closed"' not in body and 'state === "open"' not in body)
else:
print("bestByLane_candidate_order_not_found")
m_reattach = re.search(
r"const\s+reattachesToLiveLane\s*=.*?\(\)&&(\s*;\s*|\))",
src,
)
cond = m_reattach.group(0) if m_reattach else ""
print("reattachesToLiveLane_checks_branch_match", 'laneBranch === prBranch' in cond)
print("reattachesToLiveLane_checks_summary_state", bool(re.search(r"summary\.state|summary\.state\s*===|isOpen|draft|closed|merged", m_reattach.group(0) if m_reattach else "", re.S)))
PYRepository: arul28/ADE
Length of output: 1793
Require an in-flight PR state before clearing detached provenance.
bestByLane selects closed/merged PRs when no open/draft PR exists on a branch, and upsertRow(..., { allowRepoPrAdoption: true }) adopts that row if the branch matches. A reused branch name can therefore clear detached_at, detached_lane_name, and detached_provenance for a detached merged/closed PR. Limit reclamation to incoming open/draft PRs, or add an explicit guard that does not clear provenance for terminal PR states.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/prs/prService.ts` around lines 2512 - 2542,
The reattachment path in upsertRow must not clear detached provenance for
terminal PRs selected by bestByLane. Require the incoming summary state to be
open or draft before treating an existing detached row as reattachesToLiveLane,
while preserving the existing lane and branch checks for in-flight PRs.
Comment on lines
+130
to
+136
| } else if let provenance = data.provenanceLabel { | ||
| parts.append(provenance) | ||
| } else if let lane = data.laneLabel { | ||
| parts.append("Lane \(lane)") | ||
| } | ||
| if let facts = data.mergeFacts { parts.append("Merged \(facts)") } | ||
| if data.needsBranchCleanup { parts.append("Remote branch still exists") } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Suppress stale live-state text for terminal rows.
A merged or closed row can still announce its branch pair, CI state, and review state in accessibilitySummary. It can also render PrWarnBanner with stale CI or workflow data because warnMessage does not exclude terminal.
Gate these live-work signals with !data.isTerminal. Set warnMessage to nil when terminal is true.
Proposed fix
- if let head = data.headBranch, let base = data.baseBranch { parts.append("\(head) into \(base)") }
- if let ci = data.ciIndicator { parts.append(ci.title) }
- if let review = data.reviewIndicator { parts.append(review.label) }
+ if !data.isTerminal, let head = data.headBranch, let base = data.baseBranch {
+ parts.append("\(head) into \(base)")
+ }
+ if !data.isTerminal, let ci = data.ciIndicator { parts.append(ci.title) }
+ if !data.isTerminal, let review = data.reviewIndicator { parts.append(review.label) }
...
- self.warnMessage = unmapped
+ self.warnMessage = unmapped || terminal
? nilAlso applies to: 454-497
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Views/PRs/PrRowCard.swift` around lines 130 - 136, Update
accessibilitySummary and the related live-state text around the visible row
summary logic to include branch-pair, CI, and review-state details only when
!data.isTerminal, while preserving provenance, lane, merge, and cleanup text as
appropriate. Update warnMessage so it returns nil for terminal rows, preventing
PrWarnBanner from displaying stale CI or workflow data.
…pping # Conflicts: # docs/features/pull-requests/README.md
An unknown column in a cr-sqlite changeset raises, nacks the whole batch, and stalls replication for that device until an app update ships — so the column mirroring is a sync liveness property, not a feature detail. Covers both a current install and, more importantly, a phone that installed before these columns existed: legacy schema, real bootstrap migration on relaunch, then a desktop changeset carrying all 11 new columns. Includes merge_conflicts and behind_base_by, which desktop has written for some time without the phone declaring them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 1, 2026
* refactor(prs): apply the quality findings, and make /quality require it 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 <noreply@anthropic.com> * fix(prs): close quality follow-up gaps * ship: iteration 1 — resolve all quality findings * ship: iteration 2 — address final review findings --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 1, 2026
ADE rendered "CI passed · 3 jobs" for PR #988, where zero CI jobs ran. Three third-party apps reported `success` — CodeRabbit while rate-limited, Vercel while cancelled by an ignored-build step, and a comment bot — and GitHub Actions registered no check suite at all. The rollup asked "did anything succeed?" rather than "was this code verified?", so absence rendered as success. That is the failure mode a CI indicator exists to prevent: strictly worse than showing nothing, because it asserts a fact that is false. The derivation moves to shared/prChecksRollup.ts, with three rules: 1. State mapping delegates to prPipelineState, so `skipped`/`neutral` can no longer masquerade as success and the rollup can no longer disagree with the per-job rows rendered beneath it. 2. A green requires a CI producer — GitHub Actions or a legacy commit status (Buildkite/CircleCI/Jenkins). Preview, review and comment apps still render as rows but cannot carry a green on their own. 3. Required contexts that never reported hold the rollup back. Sourced in three tiers because credentials differ in what GitHub will show them: `/rules/branches/{branch}` (rulesets, read access only, works for all five credential sources), classic branch protection (admin-only), and `mergeStateStatus === blocked` as corroboration. Unreadable means unknown, never "none required". Both ingestion paths inherit this: ingestGithubWebhook uses its payload only to resolve which PR changed, then re-derives through computeStatus. New `not_run` state, distinct from `none`: `none` is a repo with no CI and stays quiet, `not_run` means something was expected and nothing verified the commit. It renders as a hollow dashed ring — an empty slot, not an alarm — on desktop, iOS, the Work-chat card, Lanes, the graph, and the TUI. The sweep found four more places `not_run` would have gone green: * adeRpcServer summarizePrChecks initialised `overall = "passing"`, so zero checks reported passing. The same bug, in the CLI's own rollup. * ChatPrPane returned an emerald "3/3 checks" from row counts alone. * ChatGitToolbar bucketed `skipped` into the passed counter. * getPrEdgeColor painted a not_run PR green whenever it had an approval. Display-only by design. No merge button, action or automation is gated — including iOS's merge gate, where only the "All checks green" sentence changes and the tone that feeds canMerge is deliberately left alone. Regression tests at the prChecksRollup layer so every surface inherits them, using the real #988 payload as a fixture. Fixes ADE-135 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 2, 2026
ADE rendered "CI passed · 3 jobs" for PR #988, where zero CI jobs ran. Three third-party apps reported `success` — CodeRabbit while rate-limited, Vercel while cancelled by an ignored-build step, and a comment bot — and GitHub Actions registered no check suite at all. The rollup asked "did anything succeed?" rather than "was this code verified?", so absence rendered as success. That is the failure mode a CI indicator exists to prevent: strictly worse than showing nothing, because it asserts a fact that is false. The derivation moves to shared/prChecksRollup.ts, with three rules: 1. State mapping delegates to prPipelineState, so `skipped`/`neutral` can no longer masquerade as success and the rollup can no longer disagree with the per-job rows rendered beneath it. 2. A green requires a CI producer — GitHub Actions or a legacy commit status (Buildkite/CircleCI/Jenkins). Preview, review and comment apps still render as rows but cannot carry a green on their own. 3. Required contexts that never reported hold the rollup back. Sourced in three tiers because credentials differ in what GitHub will show them: `/rules/branches/{branch}` (rulesets, read access only, works for all five credential sources), classic branch protection (admin-only), and `mergeStateStatus === blocked` as corroboration. Unreadable means unknown, never "none required". Both ingestion paths inherit this: ingestGithubWebhook uses its payload only to resolve which PR changed, then re-derives through computeStatus. New `not_run` state, distinct from `none`: `none` is a repo with no CI and stays quiet, `not_run` means something was expected and nothing verified the commit. It renders as a hollow dashed ring — an empty slot, not an alarm — on desktop, iOS, the Work-chat card, Lanes, the graph, and the TUI. The sweep found four more places `not_run` would have gone green: * adeRpcServer summarizePrChecks initialised `overall = "passing"`, so zero checks reported passing. The same bug, in the CLI's own rollup. * ChatPrPane returned an emerald "3/3 checks" from row counts alone. * ChatGitToolbar bucketed `skipped` into the passed counter. * getPrEdgeColor painted a not_run PR green whenever it had an approval. Display-only by design. No merge button, action or automation is gated — including iOS's merge gate, where only the "All checks green" sentence changes and the tone that feeds canMerge is deliberately left alone. Regression tests at the prChecksRollup layer so every surface inherits them, using the real #988 payload as a fixture. Fixes ADE-135 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 2, 2026
Follow-up work on the same ticket, from /quality (36 findings), /test (parity + regression coverage), and the ship-loop revalidation. The original fix corrected the rollup but left the same disease on other surfaces. Fourteen more places rendered a pass from producer-blind row counts; each is now gated on the canonical rollup: * the merge checklist, desktop and iOS, which said "All 3 checks passed" for PR #988's exact payload — the ticket's own bug on the surface a reader trusts most; * adeRpcServer.summarizePrChecks, which initialised its verdict to "passing" so *zero* checks reported green, on the surface an agent reads before deciding to merge; * PrChecksCard's header, PrDetailPane's pill, PrChecksTab's strip, the ade code drawer and right pane, ChatPrPane, ChatGitToolbar, rightPaneFormatters, LaneNode, getPrEdgeColor, iOS's stat strip and group cards, and PrLaneSummary. Two design corrections to the original fix: * The CI-producer rule was an Actions-only allowlist. CircleCI, Buildkite and Azure Pipelines report through the Checks API with their own app slugs, so that rule marked every such repo permanently unverified — the same bug pointing the other way. It is now a denylist of known non-CI apps, plus a branch-protection override: if every required context reported and passed, the commit was verified whoever ran it. The override is ordered after the in-flight check so it cannot claim a pass while a job is still running. * The grace-window clock read PR `updated_at`, which GitHub bumps on every comment — including the comments posted by the very bots whose presence is the finding. It now reads the earliest check `started_at`, which a comment cannot move. Also: a failed /check-runs fetch preserves the previous rollup instead of persisting a false not_run (bestEffort returns [] on a 403, which is indistinguishable from "no checks"); check runs carrying a terminal conclusion before their status flips no longer stick at pending forever; and pr_get_checks returns the persisted verdict rather than a row tally that cannot see required contexts. Tests: regression coverage at the prChecksRollup and requiredChecks layers so every surface inherits it, plus named tests for the merge checklist, the agent-facing RPC, the TUI's bare-array path, the fetch-failure preservation, and lane summaries. The real #988 payload is committed as a fixture. Docs, iOS, the ade CLI and the TUI are updated in lockstep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 2, 2026
CodeRabbit review on #1004. Eleven findings; ten applied, one refuted. The substantive one: a `not_run` rollup can coexist with pending or failing THIRD-PARTY rows, and several surfaces evaluated those raw counts first. A commit nothing verified would report "2 pending checks" or a failing count instead of the honest answer — the same producer-blind claim in a different tense. The canonical verdict now takes precedence in the merge checklist, the merge blockers, and the ade code right pane. Also: * A stale commit whose CI was entirely skipped, with a required context that never reported, stayed `pending` forever: the guard admitted any producer rather than an actual pass. * `fetchCombinedStatus` sat unwrapped in the same Promise.all as the guarded check-runs fetch, so a 403 there still aborted the whole refresh instead of preserving the last-known rollup. * A supplied canonical `checksStatus` is authoritative in the TUI formatter; the row fallback speaks only when no verdict was sent. * iOS bootstrap SQL declares the two ADE-135 columns, so a fresh install has them before cr-sqlite delivers a changeset carrying them. * Fixture and naming corrections: browserMock rows carry real app slugs (an absent slug is CI-eligible by design, which silently defeated the fixture), the denylist test is named for the rule it asserts, the lane summary test is named for the path it actually exercises, and the iOS #988 preview clone no longer points at #559's URL. Refuted: the mock row store's positional read was reported as broken by the new upsert params. It targets a different UPDATE (the projection write, unchanged here) and was already inert for the other one. Its matcher is now specific so the two cannot be confused again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 2, 2026
…#1004) * fix(prs): stop rendering "CI passed" when nothing verified the commit ADE rendered "CI passed · 3 jobs" for PR #988, where zero CI jobs ran. Three third-party apps reported `success` — CodeRabbit while rate-limited, Vercel while cancelled by an ignored-build step, and a comment bot — and GitHub Actions registered no check suite at all. The rollup asked "did anything succeed?" rather than "was this code verified?", so absence rendered as success. That is the failure mode a CI indicator exists to prevent: strictly worse than showing nothing, because it asserts a fact that is false. The derivation moves to shared/prChecksRollup.ts, with three rules: 1. State mapping delegates to prPipelineState, so `skipped`/`neutral` can no longer masquerade as success and the rollup can no longer disagree with the per-job rows rendered beneath it. 2. A green requires a CI producer — GitHub Actions or a legacy commit status (Buildkite/CircleCI/Jenkins). Preview, review and comment apps still render as rows but cannot carry a green on their own. 3. Required contexts that never reported hold the rollup back. Sourced in three tiers because credentials differ in what GitHub will show them: `/rules/branches/{branch}` (rulesets, read access only, works for all five credential sources), classic branch protection (admin-only), and `mergeStateStatus === blocked` as corroboration. Unreadable means unknown, never "none required". Both ingestion paths inherit this: ingestGithubWebhook uses its payload only to resolve which PR changed, then re-derives through computeStatus. New `not_run` state, distinct from `none`: `none` is a repo with no CI and stays quiet, `not_run` means something was expected and nothing verified the commit. It renders as a hollow dashed ring — an empty slot, not an alarm — on desktop, iOS, the Work-chat card, Lanes, the graph, and the TUI. The sweep found four more places `not_run` would have gone green: * adeRpcServer summarizePrChecks initialised `overall = "passing"`, so zero checks reported passing. The same bug, in the CLI's own rollup. * ChatPrPane returned an emerald "3/3 checks" from row counts alone. * ChatGitToolbar bucketed `skipped` into the passed counter. * getPrEdgeColor painted a not_run PR green whenever it had an approval. Display-only by design. No merge button, action or automation is gated — including iOS's merge gate, where only the "All checks green" sentence changes and the tone that feeds canMerge is deliberately left alone. Regression tests at the prChecksRollup layer so every surface inherits them, using the real #988 payload as a fixture. Fixes ADE-135 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(prs): harden the checks rollup across every surface, and prove it Follow-up work on the same ticket, from /quality (36 findings), /test (parity + regression coverage), and the ship-loop revalidation. The original fix corrected the rollup but left the same disease on other surfaces. Fourteen more places rendered a pass from producer-blind row counts; each is now gated on the canonical rollup: * the merge checklist, desktop and iOS, which said "All 3 checks passed" for PR #988's exact payload — the ticket's own bug on the surface a reader trusts most; * adeRpcServer.summarizePrChecks, which initialised its verdict to "passing" so *zero* checks reported green, on the surface an agent reads before deciding to merge; * PrChecksCard's header, PrDetailPane's pill, PrChecksTab's strip, the ade code drawer and right pane, ChatPrPane, ChatGitToolbar, rightPaneFormatters, LaneNode, getPrEdgeColor, iOS's stat strip and group cards, and PrLaneSummary. Two design corrections to the original fix: * The CI-producer rule was an Actions-only allowlist. CircleCI, Buildkite and Azure Pipelines report through the Checks API with their own app slugs, so that rule marked every such repo permanently unverified — the same bug pointing the other way. It is now a denylist of known non-CI apps, plus a branch-protection override: if every required context reported and passed, the commit was verified whoever ran it. The override is ordered after the in-flight check so it cannot claim a pass while a job is still running. * The grace-window clock read PR `updated_at`, which GitHub bumps on every comment — including the comments posted by the very bots whose presence is the finding. It now reads the earliest check `started_at`, which a comment cannot move. Also: a failed /check-runs fetch preserves the previous rollup instead of persisting a false not_run (bestEffort returns [] on a 403, which is indistinguishable from "no checks"); check runs carrying a terminal conclusion before their status flips no longer stick at pending forever; and pr_get_checks returns the persisted verdict rather than a row tally that cannot see required contexts. Tests: regression coverage at the prChecksRollup and requiredChecks layers so every surface inherits it, plus named tests for the merge checklist, the agent-facing RPC, the TUI's bare-array path, the fetch-failure preservation, and lane summaries. The real #988 payload is committed as a fixture. Docs, iOS, the ade CLI and the TUI are updated in lockstep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(prs): address review — canonical rollup outranks raw check rows CodeRabbit review on #1004. Eleven findings; ten applied, one refuted. The substantive one: a `not_run` rollup can coexist with pending or failing THIRD-PARTY rows, and several surfaces evaluated those raw counts first. A commit nothing verified would report "2 pending checks" or a failing count instead of the honest answer — the same producer-blind claim in a different tense. The canonical verdict now takes precedence in the merge checklist, the merge blockers, and the ade code right pane. Also: * A stale commit whose CI was entirely skipped, with a required context that never reported, stayed `pending` forever: the guard admitted any producer rather than an actual pass. * `fetchCombinedStatus` sat unwrapped in the same Promise.all as the guarded check-runs fetch, so a 403 there still aborted the whole refresh instead of preserving the last-known rollup. * A supplied canonical `checksStatus` is authoritative in the TUI formatter; the row fallback speaks only when no verdict was sent. * iOS bootstrap SQL declares the two ADE-135 columns, so a fresh install has them before cr-sqlite delivers a changeset carrying them. * Fixture and naming corrections: browserMock rows carry real app slugs (an absent slug is CI-eligible by design, which silently defeated the fixture), the denylist test is named for the rule it asserts, the lane summary test is named for the path it actually exercises, and the iOS #988 preview clone no longer points at #559's URL. Refuted: the mock row store's positional read was reported as broken by the new upsert params. It targets a different UPDATE (the projection write, unchanged here) and was already inert for the other one. Its matcher is now specific so the two cannot be confused again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In the PRs tab's Merged bucket, nearly every row showed an amber
unmappedbadge.The cause wasn't cosmetic: deleting a lane hard-deleted its
pull_requestsrow, so the normal "merge → delete the lane and branch" flow erased ADE's record that the PR was ever ADE's. Each row also silently lost its CI outcome, review result and diff stats (all derived from the linked row), and the detail pane fell back to a synthetic stub whose only affordance — "map to a lane" — could never fire on a merged PR.So the merged tab was ADE's own shipped-work history, rendered as anonymous GitHub rows with a warning on each.
Approach
Lane deletion, branch switch and rename now soft-detach instead of deleting.
detached_at+ the lane's name, colour, and a frozen count of its chats/artifacts/checkpoints. Those counts cannot be recomputed later — the lane's sessions and artifacts are hard-deleted with it, and there is no commits table — so they're captured before the cascade runs.lane_idis deliberately left dangling. It isNOT NULLon a cr-sqlite CRR inPHONE_CRITICAL_CRR_TABLES, and cr-sqlite cannot alter nullability without a full table rebuild. CRR already strips FKs, so the dangling id is inert and doubles as a provenance key.files/checks/comments/reviews) after lifting commit and file counts onto the row.pull_request_snapshotswas 33% of a real 26 MB db.detached_at is nullgates every lane-scoped "what is this lane working on" read. Project-wide reads deliberately keep detached rows — that's how the merged view recovers its provenance. A detached row owns nothing, so it can't block re-mapping, and a lane reclaims one only when it still exists and still tracks the PR's head branch.Also
land()and on the poller's merge transition, with no extra GitHub calls. Null for PRs merged before this shipped; the UI degrades rather than showing blanks.was: <lane>provenance chip, merge facts replacing the branch pair, sticky day/week period headers with per-period totals.pr.laneIdnow survives.Pre-existing bug fixed in passing
iOS declared only 4 of
pull_requests' columns. Desktop has writtenmerge_conflictsandbehind_base_bysince the merge-conflict work (prService.ts:2446UPDATE,:2506INSERT), and the phone never declared either. An unknown column in a cr-sqlite changeset throws and nacks the whole batch, freezing replication for that device. Both are now mirrored alongside the 9 new ones.Testing
BUILD SUCCEEDED; 1115 tests, no new failures (6 pre-existing, verified against a clean tree — one is flaky: 0/2/2 failures across three identical runs, nondeterministic JSON key ordering)/qualitycaught: duplicate-PK insert on detached rows, permanently-broken re-mapping, and a first fix whose gate was too looseNot done
Database.swift's local-mirror SELECT/upsert still don't carry the new columns — hand-numbered positional indices, no coverage, and no benefit, sinceGitHubPrListItemdecodes from the host RPC rather than the mirror.PullRequestListItem.detachedstays nil on iOS and degrades cleanly.Needs a real two-device check before merge: that a
pull_requestschangeset carrying the new columns applies cleanly desktop→phone.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes