feat(repos): one shared repo card that can run actions - #911
Open
matt2e wants to merge 18 commits into
Open
Conversation
The repos grid, the home repos row, and the pinned repos in the projects sidebar each had their own card markup, so they drifted: the grid and home cards led with the badge short name, and only the sidebar card carried the repo actions. Collapse all three onto a single RepoCard: - The card title is now the full repo/subpath, rendered by the shared RepoLabel (muted prefix, badge-hue emphasis on the distinguishing segment) instead of the short name. RepoLabel gains a `wrap` prop so a long path flows over as many lines as it needs rather than truncating. - Under the title sits the action row previously exclusive to the sidebar: new project, run (cloned) or clone (not cloned), pin/unpin, and the more menu with "Open in…" and "Copy Path". Unpin moves out of the more menu into a one-click pin toggle on every surface, replacing the grid's floating corner toggle. - The sidebar's pinned repos are the same card in `reorderable` mode, so they keep drag-to-reorder while picking up the grid's tint, border and layout. The more menu now resolves the clone path and opener apps when it first opens instead of on mount, so a grid of N repos no longer fires N get_repo_clone_path calls up front. SidebarPinnedRepo is deleted, and the grid's duplicated card CSS along with it; ReposListView and ProjectsList only own their own layout (grid cell stretch, 200px scroll-row width). Verified with `just typecheck`, `just fmt-check`, `just test-frontend`, and a `VITE_REPOS_UI_ENABLED=true` production build. The repos UI stays behind that flag, so this is dark by default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up feedback on the shared RepoCard surfaces: - The repos grid splits into "Pinned repos" and "All repos" (the unpinned rest) sections instead of one mixed grid, dropping the "Pinned" caption that sat under pinned cards along with its wrapper markup. - The more menu now carries the full action set — New Project, Run or Clone Repo, and Pin/Unpin Repo — ahead of the local-clone Open in… / Copy Path items, and renders on every card instead of only cloned ones (a not-yet-cloned repo previously had no menu at all, which would have left a not-yet-cloned sidebar repo with no way to unpin). - The sidebar's pinned cards hide the inline pin toggle via the new hidePinButton prop, so unpin there is only reachable through the more menu rather than as a one-click button next to the drag handle. Verified with `just typecheck`, `just fmt-check`, `just test-frontend`, and a `VITE_REPOS_UI_ENABLED=true` production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… preselected A repo card's + button (and its more menu's New Project item) dispatched a staged:new-project event carrying the card's repo and subpath, but on the repos grid view nothing listened for it — ReposListView replaces ProjectsList/ProjectHome, the only two components with listeners — so the button did nothing at all. On the surfaces that did listen, the handlers ignored the event detail, so the dialog opened blank instead of preselecting the card's repo. Wire the event through end to end: - newProjectEvent.ts defines the event's detail payload and a helper that converts it into the RepoSelection the repo config form already understands; RepoCard types its dispatch with it. - NewProjectForm accepts an initialRepo and applies it through the same RepoConfigForm.selectRepo path a pasted GitHub URL takes, so the subpath field, monorepo check, and branch picker all come up as if the user had picked the repo by hand. NewProjectModal passes it through. - ProjectsList and ProjectHome read the seed from the event and hand it to their modals; the top bar + button and bare ⌘N still open blank. - ReposListView registers its own listener and hosts its own NewProjectModal, navigating to the project after creation — fixing both the repo cards and ⌘N doing nothing on the repos grid. Verified with `just typecheck`, `just fmt-check`, `just test-frontend`, and a `VITE_REPOS_UI_ENABLED=true` production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… the rest The repo card's action row was four unlabelled 22px icon buttons packed against the left edge, so the primary action (start a project from this repo) read as just another glyph. Give it a label: the + button becomes an icon-plus-text "Add project" button, and the run/clone, pin and more buttons move into a .card-actions-secondary group with `margin-left: auto`, so they hug the card's right edge with the labelled button alone on the left. The buttons themselves are unchanged otherwise — same sizes, hover tints and handlers — and the aria-label comes off the + button now that its text names the action. Verified with `just typecheck`, `just fmt-check`, `just test-frontend`, and a `VITE_REPOS_UI_ENABLED=true` production build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Branch cards run actions through run_branch_action, which dereferences
its branch id into a worktree path; repo cards have no branch, so their
Run button is still a stub. Add the backend entry point they will use:
- run_repo_action(github_repo, subpath, action_id, provider) validates
the action against the (github_repo, subpath) context and executes it
in the repo's main local clone (clone path joined with the subpath),
erroring when the clone — or the subpath within it — doesn't exist on
disk. The execution routes under the synthetic scope id
repo:{github_repo}[:{subpath}] (repo_action_scope_id), which the
registry, event stream, and the running-actions / stop / output /
run-phase commands already treat as an opaque branch id, so all of
them work unchanged.
- Auto-commit is always stripped for repo runs: the executor would
commit into the working dir, which here is the user's default-branch
checkout rather than a disposable worktree.
- The run-detection wiring (regex matchers, autodetect poller,
run-phase events) moves out of run_branch_action_impl into a shared
wire_run_detection helper used by both commands instead of being
duplicated.
- The command is registered in the Tauri handler and the web-server
dispatch, and src/lib/commands.ts gains a runRepoAction wrapper plus
a repoActionScopeId helper mirroring the backend id format.
No UI changes yet — later sessions extract the shared action runner
from BranchCardActionsBar and wire RepoCard up to it.
Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and
`cargo test --lib --bins --tests` (553 passed, including the
web-dispatch parity test), plus `just typecheck`, prettier
formatting, and `just test-frontend` (481 passed).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…into shared modules BranchCardActionsBar owned the entire action-running experience in one ~1,040-line component, so the repo card (whose Run button is still a stub on top of run_repo_action) could only get the same behavior by duplicating it. Pull the reusable pieces out into the actions feature, parameterized per the plan note by exactly three things — an opaque scope id (branch id or synthetic repo scope id), a loadActions() callback, and a run(actionId) callback: - actionRunner.svelte.ts: the ActionRunner state machine. Owns the configured action list, the live running-execution set (hydrated via get_running_branch_actions, updated by the action_status and run-phase event subscriptions), stopping/fade-out bookkeeping, and the output modal state, plus the run / stop / show-output / run-again handlers and the grouped-action deriveds. - RunningActionPills.svelte and PrimaryRunActionButton.svelte: the secondary running-pill row and the primary run button (circular or endpoint pill with copy-URL), moved verbatim with their styles and transitions, driven by a runner prop. The endpoint URL rewriting stays branch-side and comes in via a getEndpointCopyUrl prop; a show prop keeps the setting-up gate inside the same if-block so the slide transition behavior is unchanged. - actionMenu.ts + ActionsSubmenu.svelte: the "Actions" submenu builder (menu item types, per-type icons, the Format & Check collapsing) as a pure function, and the DropdownMenu.Sub that renders it. - actionGroups.ts: the five pure grouping helpers move over from branchCardHelpers.ts (BranchCardActionsBar was their only consumer). - altKey.svelte.ts: shared refcounted Alt-key tracking for the alt-click-to-stop affordance, previously component-local listeners. BranchCardActionsBar becomes the first consumer — scoped to branch.id, listProjectActions, and runBranchAction — with zero behavior change, keeping only its branch-specific concerns (endpoint rewriting, Open In menu, rename/rebase/squash/delete items, the output modal's branch id). RepoCard is deliberately untouched; a follow-up session wires it as the second consumer on top of run_repo_action. Verified with `just typecheck`, `just fmt-check`, `just test-frontend` (481 passed), and a `VITE_REPOS_UI_ENABLED=true` production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ction The repo card's Run button and its more menu's Run item were stubs that toasted "coming soon". Wire the card up as the second consumer of the shared action-runner modules, running through run_repo_action against the repo's main local clone: - The card owns an ActionRunner scoped to repoActionScopeId(repo, subpath), loading actions with listRepoActions and running them with runRepoAction (preferred-agent provider, same as branch runs). The stub Run button becomes RunningActionPills plus PrimaryRunActionButton — live status, alt-click-to-stop, endpoint pill with copy-URL — and the more menu's Run item becomes the shared ActionsSubmenu. Clicking a running pill opens ActionOutputModal with Run Again wired to runner.runAgain(). - ActionOutputModal's branchId prop becomes optional: repo-scoped executions have no branch to attach notes to, so the card omits it and the modal hides the save-selection-as-note affordance (and refuses the save path) when it's absent. BranchCardActionsBar still passes branch.id and is unchanged. - Detection normally runs during project setup, so a cloned repo never attached to a project has an empty action context. Such cards get an explicit Detect Actions affordance (Zap button in the run slot and a more-menu item) that mirrors the settings panel's flow: detect, then persist the suggestions that don't already exist, then broadcast project-actions-changed. Cards listen for that event and for repo-actions-detection (matched on repo+subpath) so detection kicked off on any surface updates every card showing the repo. - Clone gating stays: action lookups, the runner surfaces, and the Actions submenu only exist when hasLocalClone; not-yet-cloned cards keep the Clone button and menu item. Action state hydrates once cloning flips hasLocalClone. The card action rows gain flex-wrap so running pills wrap on narrow cards instead of overflowing. Verified with `just typecheck`, `just fmt-check`, `just test-frontend` (481 passed), and a `VITE_REPOS_UI_ENABLED=true` production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The action-runner surfaces RepoCard shares with the branch card (RunningActionPills and PrimaryRunActionButton) rendered with the branch card's fixed neutral tokens — elevated backgrounds, muted borders — which sat flat on the repo card's hue-tinted surface. Give both components a variant prop: 'default' keeps the elevated neutral look byte-for-byte (BranchCardActionsBar is untouched), while 'outline' renders clear backgrounds outlined in the host card's theme. The outline styles read the --accent / --card-border-hover / --card-bg-strong custom properties RepoCard already sets from its badge hue on the card root, so the pills, the circular run button, and the endpoint pill (including its copy-button divider and hover tint) all pick up the repo's color: - Resting outlines use the badge border-hover tone; pill hover strengthens the border to the accent and tints the fill with the card's strong tint, matching the card's other inline buttons. - Semantic status colors still win: completed/failed/stop states color the outline and icon with the same status tokens the default variant uses for text, and stopping keeps the hover-reset behavior on transparent. - RepoCard passes variant="outline" to both; the Actions submenu and output modal are overlays rather than on-card surfaces, so they keep the default theme everywhere. Verified with `just typecheck`, `just fmt-check`, `just test-frontend` (493 passed), and a `VITE_REPOS_UI_ENABLED=true` production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Every cloned repo's RepoCard hydrated its action state on its own: listRepoActions, then get_running_branch_actions, then a get_run_phase per running execution. All three repos surfaces render every card unvirtualized, so opening the repos view cost 2N+ IPC calls up front, a repo pinned in the sidebar while the grid was open paid twice, and each project-actions-changed broadcast (one per detection persist) made all N cards re-list. list_repo_actions also resolves its context via get_or_create_action_context, so merely rendering the grid inserted an action_contexts row for every cloned repo that never had one. Add two read-only bulk commands and coalesce the cards onto them: - list_all_repo_actions returns every context's actions from one SQL join, grouped per context (github_repo + subpath + actions) in Rust. Strictly read-only: a repo absent from the result has no context yet, which the frontend reads as an empty action list — the same UX today's get-or-create-then-empty produces, minus the write. Context creation stays with the paths that actually write. - get_all_running_actions returns every live execution — the registry filtered against the executor's liveness set, exactly as get_running_branch_actions does — each flattened alongside its run phase, which removes the per-execution get_run_phase round trips. Branch-scoped executions come back too; callers slice by scope id. - repoActionsBulk.ts is a zero-retention coalescer over the two commands. The first caller defers a microtask, then fires one command; every caller arriving before it resolves joins the same promise and picks its own slice from the keyed result (keyed by repoActionScopeId, so an empty subpath normalizes away). Nothing is retained past resolution, so there is no cache to invalidate; a card hydrating later (cloning flips hasLocalClone) just starts a fresh wave. - ActionRunner takes an optional loadRunning callback supplying snapshots with phases inline; without it hydration still calls get_running_branch_actions plus get_run_phase per execution, so BranchCardActionsBar is unchanged. RepoCard passes both bulk loaders. Opening the repos view with N cloned repos now costs 2 read-only calls instead of 2N+ (and no context-row inserts), each project-actions-changed broadcast costs 1 instead of N, and the same repo on multiple surfaces shares one wave. Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and `cargo test --lib --bins --tests` (573 passed, including the web-dispatch parity test plus the new store grouping and running-snapshot tests), then `just typecheck`, `just fmt-check`, `just test-frontend` (499 passed, including the coalescer and runner-hydration tests), and a `VITE_REPOS_UI_ENABLED=true` production build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
detect_repo_actions detected suggestions and left persisting them to the caller, but it cleared `detecting_actions` and emitted the `repo-actions-detection` `detecting: false` event as soon as the AI call returned — before the caller's create_repo_action loop had run. Every surface treats that event as "this context's action list is final", so for the whole persist loop the repo card's Detect Actions button re-enabled itself (its `detecting` flag is written straight from the event) and the backend's own in-progress guard was open too. A second click in that window — on the same card, on the same repo's card on another surface, or in the settings panel — started a second AI-backed detection whose dedupe read an action list the first run hadn't finished writing, so duplicates were possible. Cards also reloaded their action list off that early event, briefly showing the pre-detection list. Move the persist into the command, between detection and the flag drop: - persist_suggested_actions() is the one place suggestions become rows — skip commands the context already has, continue its sort order. The prerun-actions path in branches.rs, which already persisted in the backend, now calls it instead of carrying its own copy of that loop. - detect_repo_actions detects, persists, and returns the context's resulting actions, all while `detecting_actions` is set. The flag and the event that clears it now mean the list is complete, and the already-in-progress guard covers the writes, so a concurrent click is rejected instead of launching another detection. The clear-and-emit also runs when detection or the persist fails, so a failure can't leave every surface spinning on a run that is over. - RepoCard's handler shrinks to the one call plus runner.setActions() with what it returns; the settings panel adopts the returned list in place of its own create-per-suggestion loop. Both drop their project-actions-changed broadcast — repo cards and branch cards already reload on the detection event, which is now truthful. The Detect Actions button itself stays: it is the only way into detection for a repo that never went through project setup (the repos grid mints badges from search history, AI suggestions and pinning, and clone-from-card doesn't detect), and it doubles as the explanation for an otherwise-blank run slot. Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and `cargo test --lib --bins --tests` (574 passed, including the new persist-dedupe test), then `just typecheck`, `just fmt-check`, `just test-frontend` (499 passed), and a `VITE_REPOS_UI_ENABLED=true` production build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Six review findings on the repo-card action work, all on the edges the
happy paths skip:
- detect_repo_actions_impl treated a failed mark_action_context_detected
as fatal, but it had already emitted detecting:false — leaving the DB
flag set while every surface believed detection was over, so the
in-progress guard rejected every later detection for that context with
no UI path to clear it. The failure now logs loudly and falls back to
clearing just the flag; the detection result itself is what the caller
gets back.
- run_repo_action_impl validated the action's context with
get_or_create_action_context, so running an action whose context was
deleted (or passing a mismatched subpath) inserted a fresh empty
action_contexts row on the way to rejecting the run. The check moves
into validate_repo_action_context, a read-only lookup where a missing
context fails exactly like an unrelated one — consistent with
list_all_repo_actions, which is read-only for the same reason.
- ActionRunner.runAction and runAgain had drifted into two copies of the
same clear-stale / focus-existing / start prologue; they now share
startOrFocus(action, { keepModal }), which is the only thing that
differed between them.
- altKey's refcounted listeners kept `held` alive across mounts, so a
keyup swallowed by Alt+Tab left every action button app-wide stuck on
the stop icon until the user pressed and released Alt again. A window
blur handler now clears it.
- RepoCard set actionsLoaded even when the load failed. Since a failure
empties the list and one failed bulk wave rejects every card joined to
it, a transient IPC error flipped a whole surface to the empty-context
Detect affordance — whose recovery path is a real AI detection against
repos that already have actions. loadActions() now reports success, and
the card only marks the list loaded when a read resolved (or when
detection hands it one).
- ActionOutputModal registered its document selectionchange listener on
mount, and every card mounts a modal, so a grid of N repos put N
handlers on every selection change app-wide, each walking an unmounted
element. The listener now lives in an $effect gated on `open`.
Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and
`cargo test --lib --bins --tests` (670 passed, including the new
read-only context-validation test), then `just typecheck`,
`just fmt-check`, `just test-frontend` (620 passed, including the new
load-failure and run/run-again modal tests), and a
`VITE_REPOS_UI_ENABLED=true` production build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…dge it
Action detection runs inside a "detection window": set the SQLite
detecting_actions flag, broadcast repo-actions-detection {detecting:
true}, run AI detection, persist the suggestions, mark the context
detected (which drops the flag), broadcast {detecting: false}. Every
surface reads the closing broadcast as "this context's action list is
final", and the already-in-progress guard rejects detection while the
flag is set.
That window existed in three places and only one closed reliably —
commit 99482f1 hardened detect_repo_actions_impl, but both prerun paths
(run_prerun_actions_for_branch in branches.rs, run_prerun_actions_impl
in actions/commands.rs) still `?`-returned out of the persist and mark
steps, after the flag was set and detecting:true had gone out. Any
SQLite hiccup there — plausible with concurrent sessions on a locked DB
— left every surface spinning on a run that was over and the flag
durably set, so the repo card's Detect Actions button was rejected as
"already in progress" from then on, across restarts, with no UI path to
clear it. AI-detection failure was already tolerated on those paths, so
a first-touch branch setup only needed a store hiccup to wedge the repo.
Rather than paste the hardening into two more places, extract the whole
window into one helper:
- detect_and_persist_repo_actions() claims the flag, emits
detecting:true, detects, and closes the window on every exit —
detection failure, persist failure, mark failure alike.
- finish_detection_window() is the store-side half (persist, list, mark,
with the clear-just-the-flag fallback when the mark fails), split out
so the invariant is unit-testable without an AppHandle. The mark still
runs when detection failed, so prerun doesn't retry a bad context for
every branch.
- All three entry points route through it. run_prerun_actions_impl loses
the third verbatim copy of the persist loop, so persist_suggested_actions
really does have one caller now; branches.rs loses its hand-rolled
serde_json::json! event payload for the typed DetectingActionsEvent
(byte-identical, but only by discipline).
Two deliberate behavior changes, both on the prerun paths: a persist or
mark failure is now logged best-effort — matching how detection failure
is already treated there — and prerun gains the in-progress guard it
never had, so a branch created while a repo card's detection is in
flight proceeds with the current list instead of launching a second
concurrent AI detection whose dedupe reads a half-written list.
With all three callers on one helper, the guard also becomes atomic:
claim_action_context_detection() is the check-and-set in one UPDATE
(mirroring mark_branch_setup_complete), closing the window where two
racing callers could both pass a read-then-write check.
Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and
`cargo test --lib --bins --tests` (673 passed, including the new
window-always-closes and atomic-claim tests), then `just typecheck`,
`just fmt-check`, `just test-frontend` (620 passed), and a
`VITE_REPOS_UI_ENABLED=true` production build. No frontend changes: the
event payloads and command signatures are unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Action detection claims the `detecting_actions` flag on an `action_contexts` row for the length of its window, and every path through `detect_and_persist_repo_actions` closes that window before returning. The flag is only ever cleared by code inside the process that set it, so a hard kill during the AI call — tens of seconds, and it runs during first-touch branch setup — leaves it set in SQLite with nothing that ever clears it. Commit 414776b made all three entry points respect the claim, which also removed an accidental self-heal: the prerun paths used to ignore the flag and their unconditional mark zeroed it on the next branch creation. Now the orphaned row rejects Detect Actions on every surface and skips prerun detection for every branch on that repo, permanently, with nothing in the UI to explain it (the frontend's `detecting` state is purely event-driven and never reads the flag). A blanket `UPDATE action_contexts SET detecting_actions = 0` at startup would fix it if the database belonged to this process, but `data.db` is shared — nothing stops two Staged instances from opening it, which is exactly why `sessions` and `queued_session_messages` carry `owner_pid`. Clearing a live foreign claim would let a second instance launch the concurrent detection the claim exists to prevent. So record the owner and sweep only the dead ones, mirroring `recover_dead_sessions`: - Migration 0024 adds `action_contexts.detecting_pid` and zeroes any row arriving with the flag set. That backfill matters on its own: a row reaching the migration is orphaned by definition, so it heals every database already wedged by a shipped build, which no new runtime code would reach. - `claim_action_context_detection` takes the claiming pid and records it in the same atomic UPDATE (the WHERE clause is unchanged, so the claim stays one statement); `mark_action_context_detected` clears the pid back to NULL with the flag, so a stale pid can't outlive its window and read as a live claim after a pid-reuse coincidence. - `list_detecting_action_contexts` and `release_detection_claim` are the sweep's two primitives. The release is guarded on the pid the sweep read (`detecting_pid IS ?`), so a claim that changed hands in between matches zero rows instead of clobbering a live window. A dedicated query keeps `detecting_pid` off `ActionContext` and the six SELECT lists that build it — only the sweep reads it. - `recover_orphaned_detection_claims` loops over those primitives in the command layer and releases a window with no recorded owner, one carrying our own pid (at startup that can only be a dead process whose pid we inherited), or one whose pid is dead; a live foreign pid is left alone. `is_alive` is injected so it is testable without spawning processes; startup passes `session_runner::is_process_alive`, now `pub(crate)`. - Startup calls it in the `DbCompatibility::Ok` arm alongside the other recovery sweeps, logging the count. No event is emitted: no window exists at that point, no web client is connected, and the frontend's `detecting` state starts `false` regardless. `set_action_context_detecting(_, false)` becomes `clear_action_context_detection` — with the owner pid recorded at claim time, a bare "set detecting = true" is the wrong operation to keep exposed, and clearing was its only caller. This heals at the next launch. Stealing a dead owner's window inside `detect_and_persist_repo_actions` — "wedged until the next attempt" rather than "until the next app launch" — stays available on top of the recorded pid, but is not done here. Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and `cargo test --lib --bins --tests` (681 passed, including the new migration backfill, pid-recording/guarded-release store tests, and the four sweep cases), then `just typecheck`, `just fmt-check`, `just test-frontend` (620 passed), and a `VITE_REPOS_UI_ENABLED=true` production build. No frontend changes: command signatures, event payloads, and the `ActionContext` shape are all unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e031324a8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .claim_action_context_detection(&context.id, std::process::id()) | ||
| .map_err(|e| format!("Failed to set detection status: {e}"))?; | ||
| if !claimed { | ||
| return Err("Detection is already in progress for this repository".into()); |
There was a problem hiding this comment.
When this already-in-progress case is reached from the prerun paths (run_prerun_actions_impl and run_prerun_actions_for_branch), those callers catch the error, log it, and immediately list the context's actions. If a first-time branch setup starts while another branch setup or repo card is still detecting the same repo, the winning detection may not have persisted the new prerun actions yet, so this branch skips them entirely. Please distinguish this case for prerun and wait/reload after the active detection finishes instead of treating it like a best-effort detection failure.
Useful? React with 👍 / 👎.
…of skipping it Action detection runs inside a claimed window, and a second caller arriving mid-window is rejected with "Detection is already in progress for this repository". The repo card's Detect Actions button surfaces that rejection; both prerun paths caught it in the same arm as an AI-detection failure — log it, then list the context's actions and carry on. But the two mean opposite things. A failed detection says the actions aren't coming; the in-progress rejection says they're about to exist. The winner persists inside its window, so a racing branch listed the pre-detection (typically empty) list, filtered it for prerun actions, found none, and ran nothing. Prerun runs exactly once per branch, behind the atomic mark_branch_setup_complete claim, so that skip is permanent for that worktree: it never gets its dependency installs, with one log line as the only trace. The race is a realistic one — the window is an unbounded AI call, and both triggers are first-touch flows (click Detect Actions, then create a branch from that repo while the spinner runs; or create two branches in quick succession on a fresh repo). Commit 414776b made this tradeoff knowingly when it gave prerun the in-progress guard it never had, picking skip over the third option, wait. Waiting is strictly better here: had this caller won the claim, it would have awaited the same AI call itself. - DetectionError::InProgress vs ::Failed replaces the string the callers would otherwise have to match on; Display maps both back to the text the button already shows, so the frontend is untouched. - ensure_actions_detected() is the prerun paths' one way in, replacing the byte-identical detect-then-list blocks in run_prerun_actions_impl and run_prerun_actions_for_branch. Already detected → list. Claim won → detect best-effort, as today. Claim lost → wait, then list. No re-detection after the wait: the winner marks the context detected on every exit, failed detections included. - The wait polls SQLite rather than an in-process primitive. The claim holder can be another Staged instance — data.db is shared, which is the whole reason detecting_pid exists — so a Notify keyed by context id would miss it, and the repo-actions-detection broadcast is frontend-bound. One list_detecting_action_contexts call per tick answers both halves: whether the flag is still set, and who owns it. - The AI call has no timeout of its own, so the wait carries its own cap (5 minutes, polled every second); on expiry it logs and falls back to today's proceed-with-the-current-list. - Each tick also runs the startup sweep's orphan test, with one arm inverted: mid-session a claim carrying our own pid is live — another task in this process owns it — so it is waited on, not released. An ownerless or dead-pid claim is released (guarded on the pid just read) and the waiter claims it and detects the context itself. That is the "steal a dead owner's window, wedged until the next attempt rather than until the next launch" extension commit 1e03132 deferred. The Detect Actions button keeps its immediate rejection: blocking the click would freeze the button for the length of someone else's window, and its spinner is already driven by that window's detecting:true event. Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and `cargo test --lib --bins --tests` (686 passed, including the four new wait cases — window closes, dead owner taken over, cap reached, own pid treated as live), then `just typecheck`, `just fmt-check`, `just test-frontend` (620 passed), and a `VITE_REPOS_UI_ENABLED=true` production build. No frontend changes: command signatures, event payloads and error strings are all unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…UI_ENABLED flag The pinned-repos / repos-grid UI shipped dark behind the VITE_REPOS_UI_ENABLED build flag while it was built out across this branch. It's done now, so turn it on for everyone: - featureFlags.ts is deleted — reposUiEnabled was its only export. - Every gate unwraps to its enabled branch: App.svelte always routes navigation.showReposList to ReposListView (and keeps the sidebar mounted for it), showAllRepos() drops its early return, and ProjectsList / ProjectsSidebar always warm the home-repos cache on mount and render their repos row / All Repos + pinned section whenever there's data to show. Verified with `just typecheck`, `just fmt-check`, `just test-frontend` (620 passed), and a plain `pnpm build` with no env var set — the repos UI is now in the default production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Detection runs inside a claimed window, and a prerun caller that loses the claim waits it out. Should that wait find the window's owner gone — an ownerless claim, or a dead pid — it takes the window over and detects the context itself rather than waiting out the 5-minute cap for an owner that will never finish. That takeover was two statements: release_detection_claim in the wait loop, then a fresh claim_action_context_detection inside detect_and_persist_repo_actions. Between them the flag is unset, which is exactly the read-then-write gap claim_action_context_detection was collapsed into a single UPDATE to close, and it falls either way onto the silently skipped prerun the wait exists to prevent: - A second waiter ticking in the gap finds no claim, returns Closed, and takes the context's still-undetected (typically empty) action list for final — no prerun actions, setup skipped, permanently for that worktree. - Anyone claiming in the gap sends the taker-over back an InProgress rejection for a window it had just won, and that Err arm treats it as a detection failure: logged, then the same fall through to list(). Make the takeover atomic instead: - Store::take_over_detection_claim moves detecting_pid to the new owner with detecting_actions still 1, so the window is never observably closed and concurrent readers see one that was open the whole time. The expectation stays in the WHERE clause, as on the release, so a claim that changed hands between the wait's read and its write matches zero rows rather than being stolen from its new owner. - run_claimed_detection_window splits the window's body (emit detecting:true, detect, persist, close) out of detect_and_persist_repo_actions, which keeps only the claim. The wait's DetectionWait::Released becomes ::TookOver and its caller runs that body directly — there is no second claim left to be rejected, so the InProgress case can't arise there at all. release_detection_claim stays as the startup sweep's primitive: that one really does want the window closed rather than handed on. Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and `cargo test --lib --bins --tests` (689 passed, including the two new store tests for the swap and its owner guard, and a new wait test that a window which changed hands is left to its new owner), then `just typecheck`, `just fmt-check`, and `just test-frontend` (620 passed). No frontend changes: command signatures, event payloads and error strings are all unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… tick A prerun caller that loses the detection claim waits the window out by re-reading the claim every second for up to five minutes, and every tick ran the startup sweep's orphan test on what it read. That test ends in session_runner::is_process_alive, which shells out to `kill -0` and blocks on Command::status(), so one waiting caller could spawn 300 subprocesses on a tokio worker thread — one a second — to re-answer a question whose answer barely moves. The sweep's docstring justifies shelling out with "the loop only visits rows with the flag set, normally none"; that doesn't carry over to a loop designed to run for minutes. Only the claim read has to be per-tick — it is a SQLite query, and it is what the wait is actually waiting on. Put OwnerLiveness in front of the probe: a verdict stands for liveness_probe_interval (15s by default, so at most 20 probes across the cap instead of 300), and the cache is keyed on the pid, so a window that changed hands mid-wait is always probed afresh rather than judged by the previous owner's reading — reusing a dead verdict there would take a live owner's window over. Behavior is unchanged where it matters: the first tick still probes, so a window already orphaned when the wait starts is taken over immediately, which is the motivating case. An owner that dies mid-wait is taken over up to one probe interval later than before, immaterial against a five-minute cap. The probe was already skipped entirely when the claim carries our own pid, so this only ever bit across two Staged instances sharing data.db. Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and `cargo test --lib --bins --tests` (693 passed, including the two new OwnerLiveness cases and two wait cases — one probe across ~21 ticks, and a new owner probed rather than inheriting the old one's verdict; zeroing the probe interval turns the first into 21 probes), then `just typecheck`, `just fmt-check`, and `just test-frontend` (620 passed). No frontend changes: command signatures, event payloads and error strings are all unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…lock Prerun runs a branch's setup actions once, behind the atomic mark_branch_setup_complete claim. Before the first action starts, detection can wait out another caller's window for up to five minutes; each action then runs to completion in turn, and a dependency install alone can outlast that. Five entry points reach it, and only two — create_project and the Tauri add_project_repo — already did so from a spawned task, where a long wait genuinely is free. The other three awaited it inline, on a caller that can't afford to: - The Retry button. workspaceLifecycle.setupBranchWorktree holds the branch in pendingSetupBranches until setup_worktree_and_run_prerun resolves, and the card's "Setting up…" is derived from a worktreePath that only lands when it does — so the card claimed to be provisioning for the length of the whole prerun, with no label explaining the stall, over a worktree that had been on disk since the first few seconds. - MCP add_project_repo, which most clients time out well inside 300s. - The web dispatch of setup_worktree_and_run_prerun, an HTTP request with no timeout layer in front of it. The obvious fix — a shorter wait cap for the interactive callers — re-buys the bug the wait was added to fix: any cap short enough to keep Retry responsive loses the race routinely, and giving up means prerun proceeds with the pre-detection list and silently skips that worktree's setup for good. The wait isn't what makes these paths slow; awaiting prerun at all is. All three already discard prerun's result. So detach it, and keep the one generous DetectionWaitPolicy — 300s/1s, untouched — now only ever spent by something nobody is waiting on: - claim_and_run_prerun_actions() is the claim and the run in one place. The claim is one-shot, so a caller that takes it and then doesn't run strands that worktree forever; keeping the pair together is why run_prerun_actions_for_branch is now private, with this as its only caller. It returns a PrerunOutcome so the creation paths can still emit project-setup-progress on exactly the outcome they did before. - spawn_prerun_actions() is that, detached, for the two Retry paths: they return as soon as the worktree exists. - MCP spawns the whole tail — claim, prerun, then the auto-review — as one task so their order is preserved, making it structurally the Tauri add_project_repo command's spawned setup task. Its reply widens to say setup actions are still running, since the agent no longer learns that by waiting. - create_project and Tauri add_project_repo keep their existing spawned tasks and adopt the shared helper, which is what lets the runner go private. That collapses five copies of the claim-run-log block into one, and lib.rs's emit_setup_progress moves to branches.rs next to the event type it builds. - run_prerun_actions stays synchronous: its return value *is* the execution ids, so a caller asking to run prerun now is asking to wait. Its doc comment now states the exposure it keeps rather than growing a knob nothing selects. Deliberate consequences: Retry resolves at worktree-ready, so drainQueuedSessions fires while installs are still running — the primary creation path already behaves exactly this way, so this aligns Retry with it rather than introducing something new. The two Retry paths and MCP also start emitting running_setup_actions, which the branch card only reads while provisioning, so it stays invisible. Prerun failures were already log-only on all three. Verified with `cargo fmt --check`, `cargo clippy -- -D warnings`, and `cargo test --lib --bins --tests` (693 passed), then `just typecheck`, `just fmt-check`, `just test-frontend` (622 passed, including the two new retry-lifecycle cases), and `pnpm build`. The backend test the plan sketched — that the command returns before the branch is marked setup complete — isn't reachable: the crate has no Tauri AppHandle test harness, so the entry points can't be called from a unit test. Making the prerun runner private behind the one claim helper is the substitute, and it is compiler-enforced rather than asserted. No frontend changes: command signatures and event payloads are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.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.
The repos grid, the home repos row, and the pinned repos in the projects sidebar each had their own card markup, and only the sidebar one carried repo actions. This collapses all three onto one
RepoCardand gives that card the branch card's full action runner, so a repo can run its configured actions against its local clone without a project.All of it stays behind
VITE_REPOS_UI_ENABLED, so it's dark by default.Shared repo card
RepoCardfor all three surfaces, titled with the full repo/subpath viaRepoLabel(which gains awrapprop) instead of the badge short name. The sidebar renders it inreorderablemode, keeping drag-to-reorder.SidebarPinnedRepoand the grid's duplicated card CSS are deleted.get_repo_clone_pathcalls on mount.staged:new-projectevent is wired end to end:ReposListViewnow listens for it and hosts its own modal (previously the button and ⌘N did nothing on the grid), and the existing listeners preselect the card's repo through the sameRepoConfigForm.selectRepopath a pasted URL takes, instead of opening blank.Shared action runner
run_repo_actionexecutes an action in the repo's main clone (clone path + subpath), erroring when either is missing on disk, and always strips auto-commit since the working dir is the user's default-branch checkout rather than a disposable worktree. Executions route under the synthetic scope idrepo:{repo}[:{subpath}], which the registry, event stream, and the running-actions/stop/output/run-phase commands already treat as an opaque branch id. Run-detection wiring is shared withrun_branch_actionviawire_run_detection.BranchCardActionsBar's runner machinery is extracted intofeatures/actions/: theActionRunnerstate machine,RunningActionPills,PrimaryRunActionButton, the actions submenu builder, grouping helpers, and refcounted Alt-key tracking.BranchCardActionsBarbecomes the first consumer with no behavior change;RepoCardis the second, themed per host card.Detection-window fixes
Action detection sets a durable
detecting_actionsflag for the length of its run, and every surface treats the flag as "already in progress". Three bugs left it stuck permanently:?-returned out of the persist/mark steps after the flag was set. A single SQLite hiccup during first-touch branch setup wedged the repo across restarts with no UI path to clear it. All three now go through onedetect_and_persist_repo_actionshelper that closes the window on every exit.action_contexts.detecting_pid(mirroringsessions.owner_pid) and zeroes any row arriving with the flag set, healing databases already wedged by a shipped build. A startup sweep releases claims with no owner, our own pid, or a dead pid, and leaves live foreign claims alone so a second Staged instance on the shareddata.dbisn't clobbered.Verification
cargo fmt --check,cargo clippy -- -D warnings,cargo test --lib --bins --tests(681 passed),just typecheck,just fmt-check,just test-frontend(620 passed), and aVITE_REPOS_UI_ENABLED=trueproduction build.