feat(tasks): filter the project task list by status, search it by PR/issue number, and bulk-edit a selection - #920
Conversation
…us facet The Tasks table's search box only ever matched title, branch and workflow, so a task known by its tracker number — the way it is named in a PR body or a standup — could only be found by remembering its title. `referenceNeedle` recognizes `open-mercato#909`, a bare `909`, `pr 909` and `issue 42`, and `filterRuns` answers them against `taskReferences`: the same list the row's own chip is built from, so search can never surface a number the table would not show. Alongside it, the pure model the status filter needs: `TaskListFilters`, `filterTaskList` (facet AND search), `statusFacetOptions` with counts computed the way the global page computes them, and the toggle/count helpers the Clear affordance reads. The facet's vocabulary is the status PILL's word (`deriveAttention().label`), not `RunRecord.status`, so it can offer `scheduled` and `monitoring` — sub-states the record spells as failed/running — and can never disagree with the column beside it.
… supports `lib/task-selection.ts` is the pure half of multi-edit: toggling a row, the tri-state header checkbox, and which of archive / restore / mark-read / mark-unread a given selection can actually carry out. Two rules it exists to enforce. A selection is always read against the rows CURRENTLY ON SCREEN, so an id that outlived its row — filtered away, archived, patched out by an SSE tick — is inert rather than able to act. And an action offers itself only for the rows it would really change, using the SAME gates the single-row actions use: a `review` run is not archivable by checkbox any more than it is by the "Archive finished" broom, and only a `canBeUnread` row can be put back to unread.
…ion of rows The Tasks table gets the two things it was missing next to the global page: a Status facet — the same searchable multi-select pill, with per-option row counts — and a selection column whose bulk bar can archive, restore, mark read and mark unread any number of rows at once. The facet reads the status PILL's word, so it can offer `scheduled` and `monitoring`, and it ANDs with the search box; Clear counts and undoes both. An empty result now blames the filter when a filter is what emptied it, instead of reporting an empty archive. Multi-edit is deliberately conservative. Select-all covers the FILTERED list and never the rows a filter is hiding; each action is offered only for the rows it would really change (a `review` run is no more archivable by checkbox than by the broom); the fan-out is `allSettled` over the per-run endpoints, so one refused write neither cancels the batch nor gets reported as a success — the toast says "Archived 1 of 2 tasks — 1 failed: …" and the list is refetched from the server rather than trusted from the cache.
Review finding (minor, user-facing copy): `Marked read 2 tasks.` is a log line, not a sentence. English puts the object in the middle of "marked … read", so the verb table now carries a trailing complement and the toast says "Marked 2 tasks read." — and "Marked 0 of 1 task read — 1 failed" on the partial path.
…ist-filters-bulk-edit
🧪 Verification, and two things a maintainer needs from meGate, re-run in full on the head commit One environment caveat worth knowing if you run the gate yourself: six server tests ( 📸 The one real gap: no live-browser verification. This is a UI change, and 🏷️ Labels — I could not apply them (read-only access to this repository). Suggested set, with the reasoning so you can disagree with it cheaply:
|
|
|
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Code Review: feat(tasks): filter the project task list by status, search it by PR/issue number, and bulk-edit a selection
🎯 Summary
This PR brings the per-project Tasks list (/p/:projectId/) up to the bar the global /tasks page already set, in three additive pieces. packages/web/src/lib/tasks-table.ts grows the pure filter model: referenceNeedle() recognizes #909 / 909 / pr 909 / issue 42, filterRuns() answers those against taskReferences() — the same list the row's own chip is built from, so search cannot surface a number the table would not show — and TaskListFilters / filterTaskList() / statusFacetOptions() add a status facet whose vocabulary is the status pill's word (deriveAttention().label) rather than RunRecord.status, which is what lets it offer scheduled and monitoring and is what stops it from disagreeing with the column beside it. The new packages/web/src/lib/task-selection.ts is the pure selection model — toggling, the tri-state selectionSummary, per-action gating in bulkActionTargets, and the receipt wording. packages/web/src/routes/tasks-overview.tsx wires both into the header (Status pill + Clear), a selection column on the table and the <md cards, a bulk action bar, a filter-aware empty state, and a route-level Promise.allSettled fan-out over the existing per-run endpoints.
I reviewed all seven changed files against the current head bea6fe58 in an isolated worktree, and ran the full validation gate locally. Three things stand out as genuinely well done rather than merely adequate. First, the selection is never trusted on its own: selectionSummary() intersects the id set with the rows currently on screen (task-selection.ts:82-90), so an id that outlived its row is inert rather than dangerous — and the component deliberately does not prune the set in an effect, which would have been a second source of truth racing the first. Second, bulkActionTargets() reuses the same gates the single-row actions use (FINISHED_STATUSES, isUnread, canBeUnread), and FINISHED_STATUSES was exported rather than restated, so the "Archive finished" broom and the bulk bar cannot drift apart about what finished means — a review run still cannot be swept away by a checkbox, which is exactly right. Third, the fan-out uses allSettled rather than all (tasks-overview.tsx:1246), so one refused write does not cancel the rest and the receipt can honestly say "3 of 5" instead of claiming work that never happened.
There is no server, contract, or persisted-state change: every action already had a per-run route, and the client helpers used (archiveRun, markRunSeen, markRunUnseen) are the queryScope()-based ones, which is the correct choice for a page that already stands inside /p/:projectId — the explicit-project twins (archiveProjectRun, setProjectRunRead) exist for the global page and are correctly not used here. Test coverage is unusually thorough for a UI change, including a route-level test that exercises a partial-failure batch through a mocked fetch. My findings are all minor or nit; none of them blocks.
Verdict
✅ approve — there are no blockers and no majors. The full validation gate is green on the head commit, no protected surface from BACKWARD_COMPATIBILITY.md is touched, the change is additive and cockpit-only, and every behavior it adds carries tests. The four items below are minors and nits the author can pick up at their discretion; the one that is worth acting on is the partial-failure receipt losing the handle on the rows that failed.
🧪 Validation Gate
Run in an isolated worktree at bea6fe58 with TMPDIR=/tmp (see the note under the table).
| Command | Status | Notes |
|---|---|---|
npm run typecheck |
✅ PASS | All four workspaces clean — contract, api-client, server, web. |
npm test |
✅ PASS | 6221 tests passed across 327 files, none skipped, in 100.8s. This includes design-guardian.test.ts, so the new violet/10 / violet/40 surfaces are token-compliant. |
npm run test:unit |
✅ PASS | 36 node:test cases, 0 failures. |
npm run build |
✅ PASS | Vite build succeeded and check:pack ok — 475 files, 85 under web/dist (shell + assets present). |
npm run test:package |
✅ PASS | 15 packaged-CLI E2E cases, 0 failures. |
I can confirm the environment caveat the author raised: the six server tests that mkdtemp a "not a git repo" fixture do fail when TMPDIR points inside the repository, and they pass on this commit with TMPDIR=/tmp. That is a pre-existing property of those tests and has nothing to do with this diff.
CI has not run on this pull request in this repository. There are zero GitHub Actions check-runs on head bea6fe58 here; the only reported check is license/cla, which is pending because the CLA is not signed (CLA assistant). Signing it is a hard prerequisite for merge and is the likely reason the workflows have not been dispatched for this fork branch. This approval therefore covers the code on the strength of the local gate above, not a green run in this repository, and I am not promising a CI follow-up comment — a CLA signature settles on a human's action, not on a timer, so there is nothing here for an agent to usefully wait on. The green CI the PR description cites is the run on the author's mirror PR (sheeerth/cezar#3), which is the same diff but not this repository's checks.
Findings
🔹 Minor
packages/web/src/routes/tasks-overview.tsx:177-182 — a partially failed batch leaves the reader with no handle on the rows that failed. runBulkAction calls onBulkAction(action, targets) and then setSelected(NO_SELECTION) synchronously, so the selection is cleared before the mutation resolves and the bar unmounts on the same render. When the fan-out comes back partial, the toast says Archived 3 of 5 tasks — 2 failed: run is locked (task-selection.ts:160-164) — a count and the first reason, but not which two. The reader is then left re-finding those rows by eye in a list where they are no longer distinguished by anything except still being there. The mitigation is real but weak: the failed rows do stay in the Active list, so the state is discoverable, just not pointed at. The cheapest fix that keeps the intent (the bar should not linger over rows that are leaving the view) is to have the route report the failed ids back and re-seed the selection with them on a partial failure — the selection becomes exactly "the ones that did not go through", which is also what you want to retry. Failing that, listing the failed task titles in the toast body would at least name them.
packages/web/src/lib/tasks-table.ts:213-221 — the doc comment on statusFacetOptions promises something the function does not do. It says options "come from every status PRESENT in the list, so a facet can never offer a value that could only ever empty the table". But present is computed from the unfiltered runs (line 232) while counts is computed from filterTaskList(runs, { ...filters, statuses: [] }) (line 228), which honors the search box. So with a query typed, the facet does offer options whose count is 0, and ticking one empties the table. The behavior is intentional and is pinned by a test (tasks-table.test.ts:290-306 asserts ['needs you', 0] and ['running', 0] under the query gamma) — the very next sentence in the same comment defends showing a 0 count on purpose. It is the first sentence that overstates, and in a module whose comments are this load-bearing, a promise the code does not keep is the kind of thing the next reader will trust. Reword it to say options come from every status present in the list before the search narrows it, so unticking always brings something back, and a 0 count is the facet telling you in advance that the search has already excluded that status.
💅 Nit
packages/web/src/lib/task-selection.ts:160-164 — the receipt is a sentence on success and not quite one on failure. The success branch ends with a full stop (Archived 3 tasks.), the partial branch does not (Archived 3 of 5 tasks — 2 failed: run is locked). The module's own comment on BULK_DONE_VERB argues, correctly, that this string is shown to a person and should read as a sentence; the failure path is the one where the reader is most likely to be reading carefully. Both branches are pinned by tests (task-selection.test.ts:118-131), so this is one character plus three test-string updates.
packages/web/src/routes/tasks-overview.test.tsx:1176-1184 — the bulkPending guard is asserted by injection rather than exercised. The test renders with bulkPending: true as a prop and checks the button is disabled. In the running app that state is reachable only by a narrow path: runBulkAction empties the selection, the bar unmounts, and pending can matter again only if the reader re-ticks rows while the first batch is still in flight. The assertion as written proves the prop is wired, not that the race it names is actually prevented. A test that ticks rows, fires the action, and re-ticks while the fetch is still unresolved would cover the real path; as it stands the test's name ("waits rather than letting a second batch race the first") claims more than it checks.
packages/web/src/routes/tasks-overview.tsx:526-527 — title duplicates aria-label on every tick box. SelectionCheckbox sets both to the same string, and some screen-reader/browser pairings announce the accessible name and then the tooltip, so a row reads as "Select Done one, Select Done one". Since the desktop row and the <md card are both mounted (hidden by CSS, not unmounted), that is now four announcements of the same name per run. Dropping title where it merely repeats aria-label is enough; keep it on the bulk-bar buttons, where it carries the distinct "why this is disabled" text that aria-label does not.
packages/web/src/routes/tasks-overview.tsx:160-169 — the filter pipeline runs three times per render. filterTaskList(inView, filters) for visible, again inside statusFacetOptions(inView, filters), and a third time as filterTaskList(all, filters) for the compare strips — each pass calling deriveAttention() per run, and, when the query parses as a reference needle, taskReferences() per run on top. Nothing is memoized, so every keystroke in the search box pays all three. The previous code already called filterRuns twice, so this is a widening rather than a new problem, and at the list sizes this page actually sees it is not worth a useMemo for its own sake. Worth knowing it is there if the project list ever grows past a few hundred rows.
💥 Breaking Changes
- No exported/public symbol removed or renamed without a deprecation path.
FINISHED_STATUSESmoves from module-private to exported (tasks-table.ts:26), which is purely additive; nothing was removed. - No function signature changed in a breaking way.
filterRuns(runs, query)keeps its signature; its behavior widens to also match tracker references, which strictly adds matches rather than removing any.TasksEmptyState's prop change fromquerytofiltersis module-private and not exported. - No required type field removed or narrowed.
TaskListFiltersandStatusFacetOptionare new; nothing existing was narrowed. - No HTTP route URL removed or renamed; no method changed for an existing operation. The bulk fan-out reuses
POST /runs/:id/archive,/readand/unreadexactly as the single-row actions already did. - No field removed or retyped in an existing response shape. No server code is touched.
- No event or message name renamed or removed; no payload field removed.
- No CLI command or flag renamed or removed; no machine-parsed output format changed.
- No database table or column renamed or removed; no column type narrowed. No
.ai/cezar/state shape is touched, so oldruns.jsonfiles keep parsing unchanged. - No config key renamed and no default changed silently. The two new
TasksOverviewprops (onBulkAction,bulkPending) are optional with defaults, so a direct render needs no stub. - Where a contract had to change: not applicable — no contract changed. Nothing in
BACKWARD_COMPATIBILITY.md's nine protected surfaces is in this diff.
The table does gain a leading selection column, which shifts the column indices a DOM-order query would see. The diff handles that correctly by tagging the new cells data-column-id="select" and excluding them from the column assertions (tasks-overview.test.tsx:92-95, :345, :361) rather than by renumbering expectations, so the existing assertions still mean what they meant.
🧪 Test Coverage
The PR adds 40 cases across three files, and they cover the behavior rather than the implementation.
lib/tasks-table.test.ts covers needle parsing (#909, bare 909, pr 909, issue 42, and the kind-narrowing that stops pr 42 from surfacing an issue), the facet's status vocabulary, and the two-sided counting rule — that counts narrow with the search box but not with the facet's own ticks, which is the property that makes unticking a promise the facet can keep (:290-306). It also carries a drift guard (:308-332) pinning every label deriveAttention can currently emit to a place in STATUS_FILTER_ORDER; I checked this against lib/attention.ts and it is honest — the two ladder entries the test omits, needs permission and unseen, are wired to hard-coded false today by design, so they are unreachable rather than untested.
lib/task-selection.test.ts covers toggling without mutation, the tri-state header, per-action gating (including that a review row is selected but not archivable), select-all under a filter leaving hidden picks alone, and the receipt wording in all three shapes — full success, partial, and total failure.
routes/tasks-overview.test.tsx is the strongest part: it drives the real Radix + cmdk popover in jsdom rather than stubbing the facet, and it covers the ORing of statuses, the AND with the search box, the Clear count, the filter-aware empty state under data changing beneath a live filter, reference search end to end, the checkbox-does-not-navigate rule on both the row and the card, and a route-level bulk archive where one run is refused with a 409 and the batch still finishes.
The gaps are two, and neither is large. The bulkPending race is asserted by prop injection rather than exercised, as noted in the nits above. More materially, there is no real-browser verification of this change, which the author flags honestly in the PR description and in a comment: npm run test:e2e could not provision the agent-browser provider in their environment and exited TEST_E2E_STATUS=skipped, which is not a pass. This repository's own CODE_REVIEW.md names real-browser E2E as the QA layer for user-facing changes, and this is a user-facing change with new interactive controls (a popover, a tri-state checkbox whose indeterminate is set through a ref, a bar that appears and disappears). That gap is why I am applying needs-qa rather than skip-qa, and I have posted diff-derived manual-QA instructions in a separate comment. It is a QA-gate matter, not a code-review finding, so it does not affect this verdict.
pat-lewczuk
added
merge-queue
|
🤖
|
🧪 Manual QA instructions (
|
|
🤖 Full Labels: Two things still hold the merge, neither of them a code-review finding: the CLA is unsigned ( autofix: skipped (not my PR — re-run with --autofix to fix it here). Nothing needed fixing in any case: the verdict is approve, and the remaining items are the author's call. |
4 participants
Tracking plan: .ai/runs/2026-08-23-task-list-filters-bulk-edit.md
Status: complete
🎯 Goal
What Changed
packages/web/src/lib/tasks-table.ts— the pure filter model.referenceNeedle()recognizes#909, a bare909,pr 909andissue 42;filterRuns()answers them againsttaskReferences(), the same list the row's own chip is built from, so search can never surface a number the table would not show. Alongside it:TaskListFilters,filterTaskList()(facet AND search),statusFacetOptions()(options from the statuses present, counts computed the way the global page computes them), plustoggleStatusFilter/activeFilterCount/hasActiveTaskFiltersfor the Clear affordance. The facet's vocabulary is the status pill's word (deriveAttention().label), notRunRecord.status, so it can offerscheduledandmonitoringand can never disagree with the column beside it.packages/web/src/lib/task-selection.ts(new) — the pure selection model:toggleSelected,toggleAllVisible, the tri-stateselectionSummary(which intersects the selected ids with the rows currently on screen, so a stale pick is inert),bulkActionTargets(per-action gating on the same rules the single-row actions use), andbulkResultMessagefor an honest receipt.packages/web/src/routes/tasks-overview.tsx— the Status pill and Clear in the header, a selection column on the table (tri-state header box) and on the<mdcards, the bulk action bar, a filter-aware empty state, and the route-level fan-out:Promise.allSettledover the existing per-run endpoints, one invalidation, and a toast that can say "Archived 1 of 2 tasks — 1 failed: …".No server, contract or persisted-state change: every action already had a per-run route (
POST /runs/:id/archive,/read,/unread).🧪 Tests
npm run typecheck✅ ·npm test✅ 6221 passed / 327 files ·npm run test:unit✅ 36 ·npm run build✅ (incl.check:pack) ·npm run test:package✅ 15.lib/tasks-table.test.ts(reference needle parsing, reference search, status vocabulary, facet counts, the status-order drift guard),lib/task-selection.test.ts(toggling, stale picks, per-action gating, select-all under a filter, receipt wording) androutes/tasks-overview.test.tsx(filtering through the real facet popover, Clear, empty-state wording, selection mechanics, action gating, checkbox-does-not-navigate on both row and card, and a route-level bulk archive where one run is refused).om-auto-review-pr --autofixreturned approve — no blockers, no majors; one minor (the bulk receipt's wording) was fixed ina4ab6fecand the gate re-run green on that commit. The repository's own CI workflow (Unit, build, E2E, and package) passed on this head.npm run test:e2ecould not provision theagent-browserprovider here and exitedTEST_E2E_STATUS=skipped, which is not a pass. This is a UI change, so that gap is real — see the run-summary comment.TMPDIRpoints inside the repository (theymkdtempa "not a git repo" fixture, which then resolves to this repo). The gate above was run withTMPDIR=/tmp; the failures are unrelated to this change and reproduce on a clean checkout.💥 Breaking Changes
data-column-id="select"and are excluded from the column assertions).📋 Progress
See the Progress section in the tracking plan.
Opened from the fork branch
sheeerth:feat/task-list-filters-bulk-edit. The same branch also carries the automation's own run artifacts — execution plan, code-review report and run summary — on sheeerth/cezar#3; nothing there is required to review this PR, the diff is identical.