fix(dashboard): blocker fan-out badges read legacy lane ids on a renamed board - #2989
fix(dashboard): blocker fan-out badges read legacy lane ids on a renamed board#2989gsxdsm wants to merge 2 commits into
Conversation
…med board
The card "blocking N tasks" count came back ZERO on any board whose lanes are renamed,
while real cards sat blocked behind the blocker.
`computeBlockerFanoutMap` in core takes four lane options. The dashboard wrapper
(`hooks/useBlockerFanout.ts`) declared and forwarded exactly one —
`staleHighFanoutAgeThresholdMs` — so core fell back to `holdColumn: "todo"`,
LEGACY_TERMINAL_COLUMNS and BLOCKER_ESCALATION_COLUMNS for every dashboard caller. None of
those match a renamed board, so:
- `activeTodoCount` counts cards in a lane called `todo` that does not exist -> the badge
undercounts, usually to zero;
- terminal detection misses the renamed complete lane, so finished blockers read as active;
- `shouldEscalate` is false for EVERY blocker, so a stale blocker holding up many cards
never escalates. Core's own note calls this the worse half: no escalation looks exactly
like nothing needing escalation.
PER-TASK CLASSIFIERS, NOT COLUMN SETS. Core documents `classify`/`escalationClassify` as
"the only correct option on a multi-workflow board": a column id means something only
relative to its OWN workflow, so any board-wide union marks a shared id with two workflows'
roles at once. Passing resolved SETS here would have reproduced the union read this
program's learnings doc lists as its fourth failure shape.
Board resolves each card against its own workflow via the `taskContextMenuColumnsByTaskId`
metadata it already builds — the same accessor shape ListView uses. A card whose workflow is
known but whose column that workflow no longer declares gets ABSENT flags rather than a
neighbour's traits; knowing the workflow and finding no such column is an answer.
SAFE FOR UNCONVERTED BOARDS. Absent flags degrade byte-identically to the previous defaults,
verified against core: terminal {done, archived}, hold "todo", escalation
{in-progress, in-review}. The role helpers fall back to exactly those.
The hook call moved below the per-task metadata memo it now depends on; `blockerFanoutMap`
is only consumed in JSX far below, so nothing between the two positions reads it.
MEASURED
- new test drives the REAL Board, not the wrapper: testing the wrapper would prove it
forwards what it is given and say nothing about whether anything gives it, which is the
producer/consumer split recorded as this program's fifth failure shape. The defect WAS
the producer.
- reverting the two classifier props: renamed case fails `expected +0 to be 2`, control
still passes
- neighbouring suites green: useBlockerFanout + Board + board-no-legacy-flash, 111 tests
- all five gates green; lint and tsc clean
NOT DONE: `TaskDetailModal` and `ExecutorStatusBar` call the same wrapper and have the same
gap. Neither has per-task workflow metadata in scope, so wiring them is a separate change
rather than a mechanical repeat — noted in the PR body.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughBlocker fan-out classification now uses workflow column metadata for hold, terminal, work-in-progress, and review roles. ChangesBlocker fan-out classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant WorkflowColumnMetadata
participant Board
participant useBlockerFanout
participant computeBlockerFanoutMap
WorkflowColumnMetadata->>Board: provides task column flags
Board->>useBlockerFanout: supplies classification callbacks
useBlockerFanout->>computeBlockerFanoutMap: forwards callbacks
computeBlockerFanoutMap-->>Board: returns fan-out counts
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Verified — and this is the call site my #2974 census audit flagged, so it's a satisfying confirmation that the ratchet's list produces real bugs rather than paperwork. Mutation: removing the two classifiers const blockerFanoutMap = useBlockerFanout(tasks, {
staleHighFanoutAgeThresholdMs: staleHighFanoutBlockerAgeThresholdMs,
// classify / escalationClassify removed
});→ 1 failed / 1 passed. The new coverage pins the forwarding, not merely the presence of a hook. One note for anyone probing this file laterMy first mutation attempt stripped the lane keys from () => computeBlockerFanoutMap(tasks, options)so the keys I deleted there were type-level only and changed no runtime behaviour. The lanes are supplied by the caller. I nearly filed that as "the tests don't catch it," which would have been the third time today a harness — not the instrument — produced the wrong answer, exactly as #2983 describes. The tell was the same one that PR recommends: a mutation that should obviously bite and doesn't means check the mutation first. Cross-reference
|
…renamed board (#2991) > **Stacked on #2989** — that PR adds the `classify`/`escalationClassify` passthrough this one uses. Review/merge #2989 first; the base will retarget to `main` automatically. ## The defect `ExecutorStatusBar` computes its **own** fan-out map to find the blocker holding up the most work. It passed only `staleHighFanoutAgeThresholdMs`, so core fell back to `holdColumn: "todo"` — a lane a renamed board doesn't have. `overlapBlockedTodoCount` stayed at zero, the high-fan-out threshold (`>= 5`) never tripped, and the warning **simply never appeared** while cards sat blocked behind one card. The flags were in scope the whole time: this component already receives `columnFlagsByTaskId` and hands it to `useExecutorStats`. It just never reached the fan-out. ## One type fix rides along, and it isn't cosmetic `ExecutorColumnFlags` was a `Pick` that dropped `humanReview`, while its supplier in `App.tsx` builds the map from `workflow.columns.find(...).flags` — the whole object. So the trait is present at **runtime** and only the type discarded it. That was harmless while these flags answered complete/archived/wip questions. It stops being harmless for the review role: `isReviewColumnRole` reads `mergeBlocker || humanReview`, so a lane hosting a human review **without** blocking merges would have classified as not-review. That's a wrong answer rather than a degradation — the "supplying the wrong flags" shape this program keeps re-finding — so the `Pick` now declares what the data already carries. I'd rather flag this than let it pass as a one-word type tweak: it's the kind of narrowing that reads as tidy and answers incorrectly. ## Measured | check | result | |---|---| | removing the two classifiers | renamed case fails; control **and** negative both pass | | `ExecutorStatusBar` suite + both new fan-out suites | **76 tests green** | | gates | all five green; lint and `tsc` clean | The negative case is the one worth keeping: cards already in the renamed WIP lane must not count as held, or the fix trades a silent zero for a wrong number — which invites no scrutiny at all. ## Still not done: TaskDetailModal The third caller stays unfixed, and for a different reason than this one. It holds `detailColumnFlags` for the **open task only**, while the fan-out needs a role answer for every **dependent**. So unlike `ExecutorStatusBar`, where the data was merely unthreaded, here it is genuinely absent — wiring it means deciding where per-task workflow metadata comes from in a modal, which is a design call rather than a mechanical repeat. That leaves the surface enumeration at 2 of 3 closed, stated rather than implied. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Heads-up: #2990 fixes this same defect independently, and the two conflict. Both also touch The one substantive question I checked, since #2990 raises it explicitly: what happens before the workflow resolves. #2990 guards an empty flags map so it won't fabricate answers. This PR gets there per-card instead — // packages/core/src/column-roles.ts:106
return flags ? flags.complete === true : columnId === LEGACY_COMPLETE_COLUMN_ID;So an unresolved card takes the legacy answer individually rather than the whole board dropping to legacy. Finer-grained, and I'd call it the better degradation — but it leans on a fallback two files away, which is worth a line in This PR's board-level test is the stronger artifact (mutation-verified: dropping the two classifiers fails 1 of 2, exercising the surface the operator sees). #2990's escalation analysis is the more thorough write-up. Whichever lands, that reasoning is worth keeping. |
|
Superseded by #2990, which reached the same diagnosis independently and goes further ( The two things here that #2990 did not have are carried over in #2993 against its branch:
Withdrawing one claim I made on the stacked #2991: the Closing to keep one PR over this seam. |
#2993) > Against #2990's branch. This is me folding #2989 into yours rather than running two PRs over the same seam — see [my comment there](#2990 (comment)). ## The Board half of #2990 is inert `useBlockerFanout` memoizes on: ```ts [tasks, options.staleHighFanoutAgeThresholdMs] ``` `columnFlagsByTaskId` isn't in the list. `Board` builds that index from `boardWorkflows`, which is **null until an async fetch resolves** — so the first computation runs against an empty map and takes the documented legacy fallback. When the index populates, neither dependency has changed, the memo never recomputes, and the pre-load answer survives for the life of the mount. The `flagsByTaskId.size > 0` guard is right. It only ever ran against the empty map. Threaded end to end, correctly typed, and never arriving — the first failure shape from the learnings doc, in the one place lint can't see it: **this repo has no `react-hooks/exhaustive-deps` rule**, and a disable directive for it fails CI, so the dep list is maintained by hand. ## Measured | state | result | |---|---| | this branch before the commit | `expected +0 to be 2` | | after | 2 passed | | `useBlockerFanout` + `ExecutorStatusBar` + new Board suite | **87 tests green** | Census and FNXC gates green; lint and `tsc` clean. ## The test is the point It drives the **real `Board`** with a mocked `Column` that captures the map it was handed. The existing suites call `computeBlockerFanoutMap` directly and pass a populated map from the first call — so they exercise the pure function and never the memo, which is exactly where the value has to survive an async arrival. That's why a correct implementation and green tests coexisted with a board that read legacy lanes. It also asserts the tree actually rendered before trusting the captured map: a Board that throws mid-render yields `undefined` rather than a wrong number, and I'd rather that fail loudly than read as a passing zero. ## What I'm dropping #2989 and its stacked #2991 are superseded by yours — I'll close #2989 once you've taken what you want. Yours is broader (`reviewColumns`, the `mergeOrchestration` arm, scheduler parity, the census baseline), so this carries over only the two things it didn't have: the memo dep and the producer-level test. Also withdrawn: I claimed on #2991 that the `ExecutorColumnFlags` `Pick` dropping `humanReview` produced a wrong answer. It doesn't — the sole supplier (`App.tsx:553`) passes the whole `column.flags` object, so the trait is there at runtime. Latent type-safety at most; your type is fine as-is. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The defect
The card "blocking N tasks" count came back zero on any board whose lanes are renamed, while real cards sat blocked behind the blocker.
computeBlockerFanoutMapin core takes four lane options. The dashboard wrapper declared and forwarded exactly one —staleHighFanoutAgeThresholdMs— so core fell back toholdColumn: "todo",LEGACY_TERMINAL_COLUMNSandBLOCKER_ESCALATION_COLUMNSfor every dashboard caller. None of those match a renamed board:activeTodoCountcounts a lane calledtodothat doesn't existshouldEscalateis false for every blockerCore's own note calls that last one the worse half: no escalation looks exactly like nothing needing escalation.
Diagnosed by @gsxdsm on #2981; this is the fix.
Per-task classifiers, not column sets
Core documents
classify/escalationClassifyas "the only correct option on a multi-workflow board" — a column id means something only relative to its own workflow, so any board-wide union marks a shared id with two workflows' roles at once. Passing resolved sets here would have reproduced the union read that this program's learnings doc lists as its fourth failure shape.Boardresolves each card against its own workflow using thetaskContextMenuColumnsByTaskIdmetadata it already builds — the same accessor shapeListViewuses. A card whose workflow is known but whose column that workflow no longer declares gets absent flags rather than a neighbour's traits: knowing the workflow and finding no such column is an answer.Safe for unconverted boards
Absent flags degrade byte-identically to the previous defaults. Verified against core rather than assumed:
done∪archivedLEGACY_TERMINAL_COLUMNS = {done, archived}todoholdColumn = "todo"in-progress∪in-reviewBLOCKER_ESCALATION_COLUMNSThe hook call moved below the per-task metadata memo it now depends on;
blockerFanoutMapis only consumed in JSX far below, so nothing between the two positions reads it.Measured
The test drives the real
Board, not the wrapper. Testing the wrapper would prove it forwards what it's given and say nothing about whether anything gives it — the producer/consumer split recorded as this program's fifth failure shape, where a converted consumer with an unconverted producer passed every instrument. The defect here was the producer.expected +0 to be 2; control still passesuseBlockerFanout+Board+board-no-legacy-flash— 111 tests greentsccleanThe cases are differential — identical task graphs under two vocabularies whose roles match and only the ids differ.
draftingcollides with no legacy id, so a surviving"todo"can't pass by luck. The test also asserts the tree actually rendered before trusting the map, since a Board that throws mid-render producesundefinedrather than a wrong number.Not done, deliberately
TaskDetailModalandExecutorStatusBarcall the same wrapper and have the same gap. Neither has per-task workflow metadata in scope, so wiring them means deciding where that comes from in each — a separate change rather than a mechanical repeat, and I'd rather not guess at it here. Board is the surface that renders the badge on every card, so it's the one that matters most.Summary by CodeRabbit
Bug Fixes
Tests