feat(web): add redispatch action to board card for failed/cancelled tasks #1188
No reviewers
Labels
No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
charles/agent-hooks!1188
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "dev/0"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
failure,cancelled, orinterruptednow show aRotateCcwicon button directly on the card face — no side panel required to redispatch.getLatestTaskForIssuetotask-store.tsand populates the newlast_task_id/last_task_statusfields onidle_assignedBoardCardobjects when the prior run wasn't a success.redispatchMutationin the/boardroute and threadsonRedispatchthroughBoard→BoardCardView→ the newRedispatchButtonsub-component.Test plan
/boardwith an agent column that has an idle card whose last task failed/was cancelled — aRotateCcwicon should appear in the card's bottom-right corner."Last run failed — re-dispatch"/"Last run was cancelled — re-dispatch"/"Last run was interrupted — re-dispatch")."Re-dispatched → task <shortId>"appears and the board refreshes (card transitions to queued/running).success(or cards with no prior task) show no icon — visual noise is avoided.just qapasses (typecheck + lint + format).Merge the DB-native TaskStatus and the derived TaskUiStatus into a single unified TaskStatus union that covers all 14 distinct values: DB-native: queued | running | paused | success | failure | cancelled | interrupted UI-enriched: planning | executing | waiting | reviewing | done | error | abandoned New module: packages/shared/src/task-status.ts - deriveTaskStatus() (replaces deriveTaskUiStatus) - deriveTaskStatusReason() (replaces deriveTaskUiStatusReason) Kept backward-compat shim: task-ui-status.ts re-exports the new functions under their old names so out-of-tree code doesn't break. Updated consumers: - StatusPill now accepts the full TaskStatus union and maps DB-native values to their UI-equivalent icon/label/colour at render time - history.ts handler switched to deriveTaskStatus / deriveTaskStatusReason - board.ts BoardCard.ui_status typed as TaskStatus - Web api/tasks.ts, status-pill.test.tsx, board-card.tsx all migrated from TaskUiStatus to TaskStatus Co-authored-by: Cursor <cursoragent@cursor.com>6ceec1e0ed66d95af612Code Review — PR #1188
CI is green. However there are two real bugs to fix and a merge conflict to resolve before this can land.
❌ Not mergeable
mergeable: false— almost certainly a conflict with PR #1187, which also touches theTaskStatustype system and the shared package. Rebase ontomainonce #1187's fate is settled (or after it lands).🐛 Bug 1 —
RedispatchButtonhas no pending/disabled guardredispatchMutation.isPendingis never threaded down toRedispatchButton, so a user who double-clicks (or clicks while the network is slow) fires multiplePOST /task/:id/redispatchrequests for the samelast_task_id. Each one creates a new task row.Fix: pass
isPendingthrough and disable the button:Alternatively, track a
pendingTaskIdstring in the route so only the clicked card is locked — but at minimum disable re-dispatch while any mutation is pending.🐛 Bug 2 —
handleRedispatchuseCallbackdeps cause a board-wide re-render on every renderuseMutationreturns a new object reference on every render;mutateis the stable part. BecausehandleRedispatchre-creates every render,onRedispatchin the board'suseMemodep array ([..., onRedispatch]) fires a full card list re-render on every parent render — even with no state change.Fix:
⚠️ Note — N+1 DB queries during board projection
getLatestTaskForIssueissues one SQLite query peridle_assignedcard per board refresh. For a board with 20 idle cards that's 20 extra round-trips per poll. This won't hurt in practice today but it's worth tracking: a singleSELECT … WHERE (repo, issue_number) IN (…)over the collected idle pairs would be O(1) queries instead.Not a blocker, but worth opening a follow-up issue.
⚠️ Note —
TaskUiStatusshim silently widens the typeOld
TaskUiStatuswas a 7-member union (UI-enriched values only). It is now the full 14-memberTaskStatusunion. Anyswitch/exhaustive check onTaskUiStatuswithout adefaultbranch that was previously exhaustive will silently miss"queued","running","success", etc. at runtime.This is an acceptable migration trade-off (and the comment marks it deprecated), but worth auditing for exhaustive switches before the shim is removed.
✅ What's good
last_task_id/last_task_statuson the board projection) is correct — the card face gets exactly what it needs without a side-panel round-trip.getLastTaskis optional inBoardProjectionInput— tests that don't need the affordance don't need to stub it. Clean.e.stopPropagation()on bothonClickandonPointerDowncorrectly prevents the card's click-to-open-panel from firing.TimestampLabelconsolidation is excellent — removes at least 5 inlineformatRelative/formatTimestampfunctions and gives every timestamp a semantic<time dateTime="…">element with a11y and devtools benefits. Tests are thorough.fmtDateTimeusesgetLocale()— i18n-compliant, consistent with the rest of the format helpers.deriveTaskStatusis idempotent — passing an already-enriched value back through returns the correct result, which makes the unifiedTaskStatustype safe to use at any layer.data-testid="board-card-redispatch"on the button — easy to target in e2e tests.isExecutingfix inStatusPill—status === "running" || status === "executing"is the right guard for the pulse animation.Summary: Fix the double-fire bug (Bug 1), fix the
useCallbackdep (Bug 2), then rebase. After that this is ready to merge.Code Review — PR #1188
CI passes ✅ The architecture is sound (server projection → shared type → client wiring) and the feature works end-to-end, but three issues need addressing before merge.
🔴 N+1 DB queries in
board-projection.tsgetLastTask(repo, issue.number)is called inside the per-card loop for everyidle_assignedcard:This fires one synchronous SQLite query per idle card. With 20 idle cards that's 20 round-trips per board refresh. Please add a batched variant — e.g.
getLatestTasksForIssues(pairs: { repo: string; issueNumber: number }[])using a singleWHERE (repo, issue_number) IN (...)— collect allidle_assignedcards first, fetch once before the loop, then look up from aMap.🟡
RedispatchButtonhas no loading / disabled stateThe button in
board-card.tsxhas nodisabledor spinner during the pending mutation.isPendingfromredispatchMutationis not threaded down to the button, so rapid clicks can fire multiple concurrent redispatch requests before the first resolves.Minimal fix: thread
isPendingthroughBoardCardViewProps(e.g.redispatchPending?: boolean) →RedispatchButton, adddisabled={redispatchPending}and a spin class on the icon.🟡
"running"is a dead arm ingetLatestTaskForIssue's return typeAn
idle_assignedcard can't have a concurrently-running task, and the projection immediately discards anything that isn'tfailure | cancelled | interrupted. The"running"union arm is never exercised at the call site and slightly misleads readers about what the function guarantees. Drop it —TerminalStatusalone is the correct return.Minor —
handleRedispatchcallback stabilityTanStack Query guarantees
.mutateis referentially stable across renders. Depending on the fullredispatchMutationobject causeshandleRedispatch(and therefore the memoisedBoardrender fn) to be recreated every render. Use[redispatchMutation.mutate]as the dependency instead.Review — APPROVED ✅
CI green. The feature is well-structured: batch pre-fetch in the cache layer, pure projection, clean component decomposition, and the server endpoint was already in place so nothing to wire server-side.
Two findings — neither is a blocker.
1. Dead export:
getLatestTaskForIssue(singular)task-store.tsadds both a single-issue variant and the batch variant, but only the batchgetLatestTasksForIssuesis imported (byboard-cache.ts).getLatestTaskForIssueis exported from the module but has no callers anywhere in the codebase. Either drop it now or add a// future: used by Xcomment so the next reader doesn't wonder.2.
aborted_no_skillis fetched but silently droppedgetLatestTasksForIssuesqueries for allTERMINAL_STATUSES(which includes"aborted_no_skill"), but the projection filter only gates onfailure | cancelled | interrupted:So an issue whose last terminal task was
aborted_no_skillcauses a DB row to be fetched and held inlastTaskByIssuebut never consumed. Omittingaborted_no_skillfrom the button is the right call (no skill configured → redispatching won't help), but it's worth either narrowing the query'sinArrayto the three actionable statuses, or leaving a comment on the filter explaining whyaborted_no_skillis intentionally excluded.Minor note (no action required)
redispatchPendingis a single global boolean, so clicking Re-dispatch on card A disables Re-dispatch on every other card until the round-trip completes. Safe and simple for now; worth revisiting if operators routinely need to queue multiple redispatches in quick succession.Good work overall — the batch pre-fetch approach is exactly right to avoid N+1 on board rebuild.
APPROVED and CI is green, but the branch has a merge conflict against
main— auto-merge failed. Please rebase ontomainand push; I'll merge once the conflict is resolved.fb917392578fa77dcbc4