feat(change-set): one delivery overview per issue, from the readiness collection - #218
feat(change-set): one delivery overview per issue, from the readiness collection#218SoloJiang wants to merge 9 commits into
Conversation
…ollection The state of an issue is spread across its Directions, worktrees, Evidence rows and Sessions, so answering "what will be written, why those repos, in what order, how much is trustworthy, what is left" means opening a Session and assembling it by hand. `readiness` already computes every one of those facts on its way to a verdict — including the Git signature probe of each lane's registered worktrees — and then discards whatever the verdict did not consume. This adds a projection that takes them from that SAME collection instead of re-deriving them: two derivations are two answers that can disagree, which is the drift `readiness` exists to prevent, and a second probe would pay for the same `git` calls twice on the path that paints the first screen. - readiness: carry the probe samples on `LaneFacts` as `checkouts`, an `Option<Vec<LaneCheckout>>`. `None` = this collection never probed (the same paths where `reconciliation` is Unknown for that reason); `Some([])` = probed, nothing registered yet. Collapsing those two would leave a reader unable to tell "we did not look" from "there is nothing there". - change_set: join the discarded facts with the rows that say WHERE each lane writes — repo, selection reason, declared base/branch, dependency order, evidence trust — and reuse the verdict's own per-lane answer verbatim. - Pair verdicts to facts by an ordered cursor walk, not a `direction_id` map: the unbound-PR row, the issue-wide ask row and every unmaterialized proposed lane all carry `direction_id == 0`, so keying on it would hand several distinct lanes one shared verdict. - Judge revision-anchored Evidence against the head SHAs the collection already sampled, so the summary reports real freshness instead of a blanket `unknown`; a lane that was never probed still fails closed. - Expose it as `issue_change_set` rather than widening `issue_readiness`, which the board surfaces poll per thread. Verified: cargo test --lib change_set (10 passed), cargo test --lib readiness (80 passed), git diff --check. Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
The backend overview needed a screen. This adds a third issue tab beside the lead chat and the board, showing every active lane as one row: the repo it writes and why, the declared base/branch beside what the probe observed, the lane's own readiness chip, its checks/upstream/reconciliation state, its evidence trust, and its PRs. - Lanes are grouped into dependency waves — step 1 is everything waiting on nothing, step N everything whose upstreams landed earlier — so "in what order" is readable without reconstructing it lane by lane. An edge pointing at a lane the verdict excluded is ignored rather than treated as unsatisfiable, and a dependency cycle is emitted as one honest block instead of being dropped or given an order the edges do not support. - All multi-way state is derived ONCE in `changeSetView.ts` as a discriminated value and mapped exhaustively; nothing re-tests booleans per call site. In particular `not_probed` and `none_registered` stay separate all the way to the screen, and the checkout state comes from the backend `reconciliation` rather than from a frontend re-comparison of the branches. - The declared-branch highlight is explicitly a display pointer at the row worth reading, not a second verdict; an unsampled row is neither agreeing nor disagreeing and is left unmarked. - `repo_id` joins `repo_name` on the lane DTO so a row can open its Session directly: two repos may share a display name and only the id is unique. - The panel re-reads on the board's existing readiness refresh key rather than inventing a second invalidation rule, and follows the house contract that a refresh never presents a prior verdict as current evidence. Verified: pnpm build, pnpm test (201 passed), npx tsc --noEmit, git diff --check. Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
…llapse `laneWaves` tracked pending lanes in a Map keyed by `direction_id`. The unbound-PR row, the issue-wide ask row and every proposed lane not yet materialized ALL carry `direction_id === 0`, so an issue with more than one of them kept only the last: the others were silently dropped from the panel while the header still counted them. The React keys had the same flaw one layer up. This is the defect the backend join was already written to avoid; the frontend layout had reintroduced it. Lanes are now tracked by POSITION throughout, with `direction_id` used only to resolve dependency edges — where `0` names no lane and the backend already drops those edges. Tests: three new cases covering several lanes sharing id 0, virtual lanes mixed with real dependent lanes, and a cycle among them; every lane must appear exactly once. Verified: pnpm test (15 in changeSetView, 204 total), npx tsc --noEmit. Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
…ontend with the chip Opening the Change Set tab made the board issue TWO collections for the same thread: its existing `issue_readiness` poll and the panel's own `issue_change_set`. Both run the same collection and the same bounded Git signature probe of every registered worktree, so they contend — and a probe that loses its budget reports `unknown`. The user-visible result was the panel claiming "checkout could not be read" and downgrading a lane's evidence to unknown while the chip beside it, from the other collection, said the lane was fine. That is exactly the disagreement `change_set`'s "one collection, two projections" contract exists to rule out; the contract held per collection and I had broken it by running two. Measured on the running app before the fix: 0 of 12 lane probes failed with the reads sequential, 1 of 12 with a Change Set read racing a readiness poll. After: 7 consecutive tab round-trips, both lanes "checkout matches", zero spurious unknowns. - New `issueDelivery.ts`: ONE stored delivery read, produced by whichever command the open tab called for, feeding both the chip and the panel. - The open tab joins the refresh key, so switching to the Change Set re-reads and switching away stops paying for the heavier command. - `ChangeSetPanel` no longer fetches — it is now a pure function of that read, the same split `ReadinessChip` and `EvidenceBody` already use. - A ready read that carries no change set (the readiness command answered) renders as loading, never as empty: "this issue writes nothing" is a different and wrong claim. Verified: pnpm build, pnpm test (206 passed), npx tsc --noEmit, git diff --check, and on the running Tauri/WebView surface in both locales. Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
…tatus token Four defects an adversarial review surfaced, all of them the same shape: a value presented as an answer it had not earned. - `evidence_by_direction` bucketed by `direction_id`, so every virtual lane — which all carry `0` — was handed the issue's own evidence as if it had produced it. That is the collision the verdict join was written to avoid, reintroduced one field later. Issue-level rows are now kept apart and reported as `issue_evidence`; a virtual lane reports none, because it has none. - `newest_observed_at` took the first row of an id-ordered scan, but `append_evidence` refreshes `observed_at` in place on a dedupe hit without moving the id. Id order is not observation order, so a re-confirmed fact was reported as older than it is. It now compares the timestamps, and an unparsable one never displaces a comparable one. - The evidence scan bound is per ISSUE while the summaries are per LANE, so a busy lane can push a quiet one's rows past the cut and leave it reading as having no evidence at all. Hitting the bound is now reported (`evidence_scan_truncated`) instead of being passed off as a complete count; the doc comment claiming truncation only drops the least interesting rows was simply wrong. - The lane row printed the store's raw `direction_status` token, putting an untranslated backend string on screen — visible as "queued" in the Chinese UI. It now goes through one discriminated `LaneStatusView` mapped exhaustively to i18n keys, with an unrecognized token rendering as "status unknown" rather than raw. Also: a blank declared branch is no longer treated as something a checkout can differ from. `reconciliation_for` returns Unknown for an empty declared branch rather than judging it, so highlighting a row as "differs" was the frontend inventing a comparison the backend had declined to make. Verified: cargo test --lib change_set (13 passed) and readiness (80 passed), pnpm build, pnpm test (208 passed), git diff --check, and on the running Tauri/WebView surface in both locales — the status now reads "排队中" where it previously showed "queued". Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 097a4932e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ready.length === 0) { | ||
| // Every lane left is in or behind a cycle. Emit them as one wave. | ||
| waves.push({ lanes: [...pending].map((position) => lanes[position]) }); | ||
| break; |
There was a problem hiding this comment.
Separate cycle members from lanes blocked behind the cycle
When a dependency cycle has downstream consumers (for example, A depends on B, B depends on A, and C depends on A), this branch emits A, B, and C in the same wave. The panel defines a wave as lanes that can proceed together, so C is incorrectly presented as parallel with its still-blocked upstream; moreover, if an independent wave preceded these lanes, the ordinary wave header falsely says the whole block waits on that preceding step. Represent the cycle as a distinct state and continue ordering its downstream lanes separately rather than dumping every pending lane into one normal wave.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| export interface IssueChangeSet { | ||
| readiness: IssueReadiness; | ||
| reasons: ReadinessReason[]; | ||
| active_lane_count: number; | ||
| lanes: ChangeSetLane[]; | ||
| } |
There was a problem hiding this comment.
Surface truncated evidence scans before claiming no evidence
When an issue has more than 200 evidence rows, the backend intentionally returns evidence_scan_truncated: true because an older lane can receive a zero summary even though it has evidence beyond the scan limit. This frontend DTO omits that flag, so ChangeSetPanel unconditionally renders “no evidence recorded” for such a lane, turning an incomplete scan into a false assertion. Preserve the truncation field and render an incomplete/unknown indication instead of the empty-state label.
Useful? React with 👍 / 👎.
| <ReadinessChip state={{ kind: "ready", dto: lane }} className="max-w-[20rem]" /> | ||
| <span className="ml-auto shrink-0 text-[10.5px] text-ink-faint"> | ||
| {t(STATUS_KEYS[laneStatusView(lane.direction_status)])} |
There was a problem hiding this comment.
Distinguish virtual lanes before rendering lifecycle status
Whenever the collector adds an unbound-PR row, an issue-wide ask, or an unmaterialized proposed lane, readiness assigns the synthetic direction_status value working only to make its verdict fail closed; it is not a real lifecycle state. Rendering that token through STATUS_KEYS labels every such virtual row as “building,” even though an issue ask or unbound PR has no worker building anything. Derive a status view that accounts for direction_id === 0 instead of presenting the readiness sentinel as stored lane status.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| {lane.pull_requests.length > 0 && ( | ||
| <span className="inline-flex items-center gap-1"> | ||
| <GitPullRequest size={11} aria-hidden="true" /> | ||
| {t("changeSet.pullRequests", { count: lane.pull_requests.length })} | ||
| </span> |
There was a problem hiding this comment.
Show which pull request is blocking the lane
When a lane has multiple tracked PRs, especially when only one has failing CI, requested changes, unresolved threads, or a conflict, this renders only “N pull requests.” The serialized facts contain the axes but omit the persisted PR number, URL, title, and repository identity, and none of the axes are rendered, so the user cannot determine which PR produced the lane verdict or what action to take without leaving the overview. Project a display identity from the stored PR row and render each PR's relevant host state rather than only its count.
Useful? React with 👍 / 👎.
| // A virtual lane (unbound PR row, issue-wide ask) has no direction to open, | ||
| // and a lane whose write repo was never resolved has nothing to open it in. | ||
| const openable = lane.direction_id !== 0 && lane.repo_id !== 0; |
There was a problem hiding this comment.
Disable the lane opener when its repository is unresolved
When a direction retains a dangling repo_id, the backend explicitly tolerates that state by returning an empty repo_name while preserving the nonzero ID. This predicate therefore still renders the opener, and clicking it calls viewDirection with a repository that does not exist, leading to an unusable session view instead of keeping the degraded Change Set visible. Treat the failed repository resolution as non-openable, for example by returning repo_id: 0 or checking the resolved repository state here.
Useful? React with 👍 / 👎.
…support All from Codex on #218; all confirmed against the code. **A cycle is not a step.** The layout dumped every remaining lane into one wave when it got stuck, so a lane merely blocked BEHIND a deadlock was presented as running in parallel with its own blocked upstream — and the ordinary "waits on the step above" header described an order that does not exist. Only lanes actually ON the cycle are now emitted, as their own `cycle` kind with its own header; downstream consumers keep their real position in later waves. **A readiness sentinel is not a lifecycle.** `virtual_lane_facts` sets `direction_status: "working"` purely so a virtual lane's verdict fails closed. Rendering that token labelled every unbound-PR row and issue-wide ask as "building", asserting work nobody is doing. Lanes now carry an explicit `materialized` flag — reported rather than inferred from `direction_id == 0`, since a proposed lane whose direction was deleted keeps a non-zero id — and an unmaterialized lane renders as "not started". **An id whose row is gone is not an openable id.** A direction can retain a dangling `repo_id`; the backend tolerated it by returning an empty name while still handing out the id, so the opener stayed enabled and would navigate to a repository that does not exist. The repo is now resolved once and both id and name come from that one answer, with unresolved reported as `0`. **A count is not an answer.** A lane with several PRs rendered only "N pull requests", so a reader could not tell which one was red. `PullRequestFacts` carries axes and a primary key because that is all a VERDICT needs; the stored row's display identity (number, title, host slug) is now joined back on, and each host axis renders through a lookup with an explicit fallback. **An incomplete scan is not an empty one.** `evidence_scan_truncated` reached the Rust DTO but never the frontend, so a lane whose rows all fell past the per-issue scan bound was reported as "no evidence recorded" — an incomplete read turned into a false assertion. Evidence is now one discriminated `LaneEvidenceView`, and "none" is only claimed when the scan supports it. Verified: cargo test --lib change_set (14 passed) and readiness (80 passed), pnpm build, pnpm test (212 passed), npx tsc --noEmit, git diff --check. Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92775bb0a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {t("changeSet.laneCount", { count: state.changeSet.active_lane_count })} | ||
| </span> | ||
| </header> | ||
| {state.waves.map((wave, index) => ( |
There was a problem hiding this comment.
Whenever evidence is recorded with direction_id == 0—for example, host evidence for an unbound PR—the backend stores it only in issue_evidence and deliberately leaves virtual-lane summaries empty. This ready branch renders only state.waves, never state.changeSet.issue_evidence, so those records disappear from the overview; if there are no lanes, the panel instead reports the entire Change Set as empty. Render an issue-level evidence summary independently of the lane waves.
Useful? React with 👍 / 👎.
| <span>{axis(CI_KEYS, pr.ci.state)}</span> | ||
| <span>{axis(REVIEW_KEYS, pr.review.state)}</span> | ||
| <span>{axis(THREADS_KEYS, pr.threads.state)}</span> | ||
| <span>{axis(CONFLICT_KEYS, pr.conflict.state)}</span> |
There was a problem hiding this comment.
Render the lifecycle of each pull request
When a lane has multiple PRs and one is closed without merging, readiness produces the lane-level pr_closed_unmerged reason, but each row here renders only CI, review, thread, and conflict axes; the closed PR can therefore look completely green and the user still cannot identify which row caused the verdict. Fresh evidence after the prior identity finding is that the updated DTO now carries pr.lifecycle, but this renderer never consumes it; map that lifecycle exhaustively per row.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| let rows = repo::list_evidence(db, thread_id, None, EVIDENCE_SCAN_LIMIT).await?; | ||
| let truncated = rows.len() as u64 >= EVIDENCE_SCAN_LIMIT; |
There was a problem hiding this comment.
Detect evidence truncation using an extra row
When an issue has exactly 200 evidence rows, this query returns all of them, yet >= EVIDENCE_SCAN_LIMIT marks the scan as truncated. The frontend consequently labels every zero-count lane as unscanned even though the scan was complete. Fetch one row beyond the display limit and set the flag only when that sentinel row exists, then summarize only the first 200.
Useful? React with 👍 / 👎.
| let directions = repo::list_directions(db, thread_id).await?; | ||
| let directions_by_id: HashMap<i32, &crate::store::entities::direction::Model> = | ||
| directions.iter().map(|row| (row.id, row)).collect(); |
There was a problem hiding this comment.
Project scope from the same direction snapshot
When this collection overlaps proposal confirmation or materialization, readiness can build a virtual fact for a direction that did not exist at collection start, while this second direction read sees the newly created row. The projection then marks that lane materialized, attaches its new repo, branch, and dependencies, but retains the virtual fact's sentinel lifecycle, absent checkout sample, and verdict; deletions or scope changes during the long-running check collection produce the inverse mismatch. Carry the direction snapshot used by collect_facts_and_readiness into the projection, or version-check and retry rather than combining two generations.
Useful? React with 👍 / 👎.
| const read = | ||
| deliveryView === "changeSet" | ||
| ? api.issueChangeSet(request.threadId).then(deliveryFromChangeSet) | ||
| : api.issueReadiness(request.threadId).then(deliveryFromReadiness); |
There was a problem hiding this comment.
Prevent delivery refreshes from overlapping
When the user switches between the Change Set and another issue tab before the current invoke finishes—or when a collection lasts into the next 60-second poll—the effect cleanup only suppresses the old response; it does not stop the backend command. This branch immediately starts the other collection, so issue_readiness and issue_change_set can probe the same worktrees concurrently, recreating the contention and unknown results that the new single-read design explicitly aims to prevent. Serialize or reuse the in-flight collection, or add real backend cancellation rather than merely discarding its response.
Useful? React with 👍 / 👎.
…hat was collected Five more from Codex on #218. Two are structural and matter more than the count suggests. **The projection read a SECOND generation.** `collect_facts_and_readiness` read the directions, then `project` read them again. A proposal confirmed or a lane deleted between the two would produce a row half from each: a lane marked `materialized` with a freshly created direction's repo, branch and dependencies, still carrying the virtual fact's sentinel lifecycle, absent checkout sample and verdict. This module's whole premise is that two derivations can disagree — and it was doing exactly that with its own inputs. The collection now returns the direction snapshot it consumed (`CollectedIssue`), and the projection takes it rather than re-reading. **Serializing the reads was not enough to stop the overlap.** Cancelling an effect only discards the RESPONSE; the backend collection keeps running. A tab switch mid-flight, or a poll tick landing on a long collection, still put two collections on the same worktrees — reviving the probe contention the single-read design exists to remove. Reads now chain: each waits for the previous to settle before issuing. This does not claim to cancel the backend, it prevents the second start. **Truncation was off by one.** An issue with exactly 200 rows was scanned completely, yet `>= LIMIT` called it truncated, which made every quiet lane read "not fully scanned" when the counts were exact. The scan now reads one row past the bound purely as a sentinel and summarizes only the first 200. **A closed PR could look entirely green.** `pr_closed_unmerged` drives the lane verdict on its own, but the row rendered only CI, review, threads and conflict — never the lifecycle the DTO already carried. It is now shown first, and a closed one is marked. **Issue-level evidence had no home.** Rows recorded against the issue rather than a lane are deliberately kept out of the lane summaries, so without rendering them they simply vanished — and an issue with no lanes reported as empty while still holding records. The empty state now carries the change set, and both states render an issue-level line. Verified: cargo test --lib change_set (14 passed) and readiness (80 passed), pnpm build, pnpm test (213 passed), npx tsc --noEmit, git diff --check. Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cfb5fec9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const read = deliveryReadChainRef.current.catch(() => {}).then(issue); | ||
| deliveryReadChainRef.current = read.catch(() => {}); |
There was a problem hiding this comment.
Drop obsolete delivery reads before invoking them
When a collection exceeds the 60-second polling interval, or the user switches tabs/issues repeatedly while one is running, every invalidated effect still appends its captured issue callback to this chain; cleanup suppresses only its response. The newest visible request must wait for all obsolete collections, and if refreshes arrive faster than collections finish the queue grows without bound, leaving the panel loading or increasingly stale. Coalesce queued requests or check cancellation before invoking the next backend command.
Useful? React with 👍 / 👎.
| const { fresh, stale, unknown } = changeSet.issue_evidence; | ||
| if (fresh + stale + unknown === 0) return null; |
There was a problem hiding this comment.
Handle truncation in issue-level evidence
When an issue has more than 200 evidence rows and its direction_id == 0 rows fall beyond the scan window, issue_evidence is all zero while evidence_scan_truncated is true, so this return hides the issue-level evidence entirely. Unlike laneEvidenceView, this path never distinguishes a complete zero from an incomplete scan; derive an exhaustive issue-evidence view from both the counts and truncation flag and render an incomplete indication.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| let depends_on = match direction { | ||
| Some(direction) => upstream_direction_ids(db, direction.id).await?, | ||
| None => Vec::new(), |
There was a problem hiding this comment.
Project dependency edges from the collected snapshot
When proposal confirmation or re-proposal updates dependency edges during a long readiness collection, the lane verdict uses the edge state read by collect_lane, but this later query can read the replacement edges and display a different ordering beside that verdict. Fresh evidence after the earlier direction-snapshot fix is that dependency edges are still fetched separately here, and reused direction rows can have their edges changed without changing that snapshot; carry the collected edges in LaneFacts or version-check and retry.
Useful? React with 👍 / 👎.
| return ( | ||
| <article className="flex flex-col gap-2 rounded-[var(--radius-lg)] border border-border bg-surface px-3 py-2.5"> | ||
| <div className="flex min-w-0 flex-wrap items-center gap-2"> | ||
| <span className="min-w-0 truncate text-[12.5px] font-semibold text-ink">{lane.name}</span> |
There was a problem hiding this comment.
Translate synthetic lane names before rendering them
When the UI is using Chinese and readiness adds an issue-wide ask or unbound-PR contributor, the backend supplies the fixed English names issue ask and unbound PR, and this renders them verbatim among otherwise translated Change Set labels. These are application-owned states rather than user-provided lane names, so expose a virtual-lane discriminator or translation key and map it through the locale files before display.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
…ply the truncation rule to issue evidence Both are regressions from the previous commit. **The serialization chain had no coalescing.** Fixing the overlap by chaining reads meant every invalidated effect still appended its command to the queue — cleanup suppressed the RESPONSE but not the call. Rapid tab or issue switching, or a collection outliving the 60-second poll, would queue one obsolete backend command per refresh, and the newest visible request would wait behind all of them; the panel stays loading while the queue grows. Cancellation is now re-checked at the FRONT of each queued turn, so an obsolete read becomes a no-op rather than a collection nobody is waiting for. **Issue-level evidence ignored the truncation flag** the lane rows had just been taught to respect. All-zero counts from a truncated scan were rendered as nothing at all — hiding the row asserts "there is none" just as strongly as printing it would. It now takes the same three-way view. Verified: pnpm build, pnpm test (214 passed), npx tsc --noEmit, git diff --check. Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 691cfbfc98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut rows = repo::list_evidence(db, thread_id, None, EVIDENCE_SCAN_LIMIT + 1).await?; | ||
| let truncated = rows.len() as u64 > EVIDENCE_SCAN_LIMIT; | ||
| rows.truncate(EVIDENCE_SCAN_LIMIT as usize); |
There was a problem hiding this comment.
Preserve refreshed evidence in bounded scans
When an issue has more than 200 evidence rows and an older row is re-confirmed, append_evidence updates that row's observed_at without changing its ID, while list_evidence still orders the bounded page by descending ID (src-tauri/src/store/repo.rs:8618-8640,8690-8693). This scan can therefore omit a newly refreshed fact; if that lane has any other row inside the window, the UI renders the resulting nonzero counts without the truncation warning and can show stale/unknown evidence despite a fresh observation. Select the bounded window by observation recency, or otherwise include refreshed rows before summarizing.
Useful? React with 👍 / 👎.
| <span>{t("changeSet.wave", { index: index + 1 })}</span> | ||
| {index > 0 && <ArrowRight size={11} aria-hidden="true" />} | ||
| {index > 0 && ( | ||
| <span className="normal-case tracking-normal font-normal">{t("changeSet.waveWaits")}</span> |
There was a problem hiding this comment.
Number executable waves independently of cycles
When a cycle precedes a downstream executable wave, index still counts the cycle even though the cycle header explicitly has no step number. For example, an A↔B cycle followed by C renders C as “step 2” and says it waits on the step above even though no step 1 exists; with an independent first wave it skips from step 1 to step 3. Compute the displayed ordinal from preceding parallel waves rather than the raw array position.
Useful? React with 👍 / 👎.
| /// `reconciliation_for` reduced away. | ||
| fn lane_checkout(probed: &ProbedWorktree) -> LaneCheckout { | ||
| LaneCheckout { | ||
| repo_name: probed.repo.clone(), |
There was a problem hiding this comment.
Localize the missing-repository checkout label
When a worktree's repository row cannot be resolved, probe_worktrees_for_direction supplies the English sentinel repo {id} (readiness.rs:1800-1803), and this new projection exposes it as repo_name for direct rendering in the Change Set. In the Chinese locale that degraded checkout row therefore contains an untranslated application-owned label; carry a missing-repository discriminator or ID and format the fallback through the locale files instead.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
| let resolved_repo = match direction { | ||
| Some(direction) => resolve_repo(db, direction.repo_id).await, | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
Preserve proposed lane scope before materialization
While a proposal is still pending or partially approved, its unmaterialized lanes already carry a repository, reason, base branch, and declared dependencies, but this projection treats every lane without a direction row as having no scope. Those lanes consequently render without the repo/rationale and are placed as independent waves even when the proposal says they wait on another lane, defeating the overview precisely before confirmation. Carry the collected proposal snapshot into LaneFacts or the projection instead of deriving all scope exclusively from materialized directions.
Useful? React with 👍 / 👎.
A cycle wave renders without a step number, because it is not a step anyone can take — but it still consumed an array index, so the executable wave after an A<->B cycle read "step 2, waits on the step above" when no step 1 had been shown, and an independent first wave made the numbering jump 1 -> 3. The ordinal is now counted over the parallel waves only. Verified: pnpm build, pnpm test (215 passed), npx tsc --noEmit, git diff --check. Refs #175 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
|
CI note — The failure is Evidence it is not from this branch:
#216 is the fix for exactly this, by routing spawn failures to I have not re-run the job — the flake is real and worth landing #216 for rather than papering over with a retry. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0d72e5de7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const remaining of blockers.values()) { | ||
| for (const position of stuck) { | ||
| remaining.delete(position); |
There was a problem hiding this comment.
Keep dependents of a cycle blocked
When A and B form a cycle and C depends on A, deleting the cycle positions from every remaining blocker set makes the next iteration emit C as a normal parallel wave; the panel consequently renders C as executable “Step 1” with no waiting warning, even though its upstream cycle remains unresolved. Fresh evidence after the prior cycle finding is that the revised grouping now separates C from the cycle but explicitly promotes it after removing the cyclic blocker; preserve a distinct blocked-after-cycle state instead.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| let pull_request_rows = repo::list_pull_requests_for_thread(db, thread_id) | ||
| .await | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Propagate pull-request lookup failures
If list_pull_requests_for_thread fails because the database is locked, corrupt, or otherwise unavailable, unwrap_or_default() silently turns the failure into an empty result. The command then succeeds while every tracked PR is rendered as “record missing,” which is materially different from being unable to read the records; propagate the error so the panel reports a load failure instead.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| if repo_id == 0 { | ||
| return None; | ||
| } | ||
| let row = repo::get_repo(db, repo_id).await.ok().flatten()?; |
There was a problem hiding this comment.
Distinguish repository lookup errors from missing rows
When get_repo returns an actual database error, .ok() converts it into the same None used for a dangling repo_id. The resulting successful response falsely reports the repository as unresolved and disables the lane opener, masking an operational failure as missing data; return Result<Option<_>> and propagate Err while still tolerating a successful lookup that returns no row.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
Closes #175.
An issue's state is spread across its Directions, worktrees, Evidence rows and Sessions, so answering what will be written, why those repos, in what order, how much is trustworthy, what is left means opening a Session and assembling it by hand. This assembles it once.
One collection, two projections
readinessalready computes every one of those facts on its way to a verdict — including the Git signature probe of each lane's registered worktrees — and then discards whatever the verdict did not consume. The Change Set takes them from that same collection rather than re-deriving them: two derivations are two answers that can disagree, which is the driftreadinessexists to prevent. The verdict shown is the verdictissue_readinesswould give, by construction rather than by agreement.Concretely,
collect_with_check_executionsplits intocollect_facts_and_readiness, which returns(IssueReadinessDto, Vec<LaneFacts>), andLaneFactsgainscheckouts: Option<Vec<LaneCheckout>>carrying the probe samples thatreconciliation_forreduced to a single word.NoneandSome([])are deliberately distinct:Nonemeans this collection never probed (the same paths wherereconciliationis Unknown for that reason),Some([])means it probed and no worktree is registered yet.reconciliationreadsUnknownfor both, so without the distinction a reader cannot tell we did not look from there is nothing there yet — and those need different next actions. The distinction survives to the screen.What the panel shows
A third issue tab beside the lead chat and the board. Each active lane is one row: the repo it writes and why, the declared base/branch beside what the probe observed, the lane's own readiness chip, checks/upstream/reconciliation, evidence trust, and its PRs. Lanes are grouped into dependency waves — step 1 is everything waiting on nothing, step N everything whose upstreams landed earlier.
An edge pointing at a lane the verdict excluded is ignored rather than treated as unsatisfiable, and a dependency cycle is emitted as one honest block rather than dropped or given an order the edges do not support.
Three defects of one shape, caught during the work
Each is an identity collapsed onto something that is not an identity.
The verdict join. Virtual lanes — the unbound-PR row, the issue-wide ask row, every unmaterialized proposed lane — all carry
direction_id == 0.issue_readinessbuilds its lane list by filtering the same facts in order, so its rows are an ordered subsequence and an ordered cursor walk pairs them exactly; adirection_idmap would hand several distinct lanes one shared verdict.The wave layout reintroduced that same flaw in the frontend, keying pending lanes by
direction_id, so an issue with more than one virtual lane kept only the last and silently dropped the rest while the header still counted them. Lanes are now tracked by position throughout.The evidence join had it a third time, one field later: bucketing by
direction_idhanded every virtual lane the issue's own evidence as if it had produced it. Issue-level rows are now reported separately asissue_evidence.Two more from review
newest_observed_attook the first row of an id-ordered scan, butappend_evidencerefreshesobserved_atin place on a dedupe hit without moving the id — so a re-confirmed fact was reported as older than it is. It now compares timestamps, and an unparsable one never displaces a comparable one.The evidence scan bound is per issue while the summaries are per lane, so a busy lane can push a quiet one's rows past the cut and leave it reading as having no evidence at all. Hitting the bound is now reported (
evidence_scan_truncated) instead of being passed off as a complete count.A design mistake worth recording
Making
issue_change_seta separate command — to keep the polled readiness path light — backfired. The board then ran two collections against one thread, their Git probes contended, and a probe that lost its budget reportedunknown: the panel claimed "checkout could not be read" while the chip beside it, fed by the other collection, said the lane was fine. That is exactly the disagreement this module's contract claims is impossible; the contract held per collection and was broken by running two.Measured on the running app: 0 of 12 lane probes failed with the reads sequential, 1 of 12 with a Change Set read racing a readiness poll. The board now issues one command per refresh — the richer one when the tab is open — and both surfaces read that single result. After the fix: 7 consecutive tab round-trips, both lanes
checkout matches, zero spurious unknowns.Constraints
All multi-way state is derived once as a discriminated value and mapped exhaustively; nothing re-tests booleans per call site. The checkout state comes from the backend
reconciliation, never from a frontend re-comparison — and a blank declared branch is not something a checkout can differ from, sincereconciliation_fordeclines to judge that case at all. The lane status goes through oneLaneStatusViewmapped to i18n keys, with an unrecognized token rendering as "status unknown" rather than raw: it previously leaked the store'squeuedtoken untranslated into the Chinese UI.Verification
cargo test --lib change_set— 13 passedcargo test --lib readiness— 80 passedcargo test --test readiness— 49 passed;m2_git,m2_worktree,lead_repo_state,delete_late_writes,worktree_deletealso greenpnpm build,pnpm test— 208 passed,npx tsc --noEmitclean,git diff --checkcleanapi-service→web-clientjoin dependency), in both localesThree
--libtests fail in this sandbox and are unrelated:checkpoint::mid_restore_failure_rolls_everything_backneeds a chmod-undeletable file, which is impossible as uid 0 (verified: root deletes through a read-only directory here), and twoproc_registryprocess-group reaping tests. Neither module referencesreadinessoutside doc comments.Not done here
evidence_scan_truncatedis reported but no UI reads it yet — surfacing it is a small follow-up.🤖 Generated with Claude Code
https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
Generated by Claude Code