fix(dashboard): the board's fan-out trait index never reached the memo - #2993
Merged
gsxdsm merged 1 commit intoJul 31, 2026
Merged
Conversation
Folding my #2989 into this branch rather than running two PRs over the same seam. The Board half of the parent fix is inert. `useBlockerFanout` memoizes on `[tasks, options.staleHighFanoutAgeThresholdMs]`, and `columnFlagsByTaskId` is not 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, so 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, never arriving — the first failure shape from the learnings doc, and invisible here because this repo has no `react-hooks/exhaustive-deps` rule (a disable directive for it also fails CI, so the list is maintained by hand). Adds the Board-level regression test that caught it. It drives the REAL Board with a mocked `Column` capturing the map it was handed, rather than calling `computeBlockerFanoutMap` directly — the existing suites do the latter and pass a populated map from the first call, so they exercise the pure function and never the memo, which is where the value has to survive an async arrival. MEASURED - before this commit, on this branch: `expected +0 to be 2` - after: 2 passed - useBlockerFanout + ExecutorStatusBar + the new Board suite: 87 tests green - census and fnxc gates green; lint and tsc clean Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…named board (#2996) ## How this was found By generalizing the memo-dependency defect in the blocker fan-out (#2993) into a sweep for memoized hooks that read a lane value absent from their dependency list — rather than treating that one as a one-off. **13 raw hits, 12 benign** (refs, or values reached through a covered object). This is the one that's a live defect. ## The defect `wantsLiveTimeIndicator` decides whether a card subscribes to the shared time ticker. It reads `isWipColumn`, `isReviewColumn` and `taskColumnFlags` — all derived from the `taskColumnFlags` **prop** — while its dependency array listed only `task.*` fields. Those flags arrive **after first paint**: the board resolves workflow traits asynchronously. So: 1. first computation runs with flags `undefined`; 2. role helpers fall back to legacy ids — `isWipColumnRole(undefined, "building")` is **false**; 3. the card declines the ticker; 4. flags arrive, but `task.column` hasn't changed, so nothing in the dep array changed; 5. the memo never recomputes. **No live elapsed time, for the life of the mount.** ## Why it survived On a legacy board the fallback already answers `true` on the very first paint (`column === "in-progress"`), so the memo's initial value is correct and the stale list costs nothing. The defect is **renamed-board-only**. This repo also has no `react-hooks/exhaustive-deps` rule, so the entire class is invisible to lint — and a disable directive for that rule fails CI, so these lists are maintained by hand. ## Measured The test was written **first** and was red for the right reason before any fix — the control and the negative passed, and only the renamed case failed: | stage | result | |---|---| | before the fix | `expected false to be true` (renamed case only) | | after | 3 passed | | `TaskCard.test` + `cli-states` + `oversight` + new suite | **456 tests green** | | gates | census + FNXC green; lint and `tsc` clean | The assertion is on `useLiveTimeTicker(enabled)` — `enabled` *is* `wantsLiveTimeIndicator`, so it observes the subscription itself rather than a proxy for it. ## The negative case is the one that matters Recomputing must not degrade into "every card subscribes". A card in the renamed **complete** lane must stay off the shared ticker, or the fix trades one stalled indicator for sixty cards waking a backgrounded tab — the exact cost the shared-ticker refactor documented at this site (it replaced 60 per-card `setInterval`s precisely because mobile browsers discard a page that never goes idle). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…after a memo has answered (#2998) ## The shape Three defects this session, all the same, none visible to any instrument here: A lane value resolved **asynchronously** (the board fetches workflow traits after first paint) is read inside a `useMemo`/`useCallback` whose dependency list omits it. The first computation runs with the flags `undefined`, the role helpers correctly fall back to legacy ids, and on a **renamed** board that answer is wrong. When the flags arrive nothing in the dep list changed, so the memo never recomputes. | defect | severity | |---|---| | blocker fan-out trait index (#2993) | permanent — empty index for the mount | | card live elapsed-time indicator (#2996) | permanent — never subscribes | | near-duplicate chip (#2997) | bounded — self-heals on the next task refresh | A legacy board hides all three: there the fallback already answers correctly on the first paint, so the stale list costs nothing. **Every instance is renamed-board-only**, which is why they accumulated — and this repo has no `react-hooks/exhaustive-deps` rule, so the class is invisible to lint. ## Two properties decide severity, both readable off the dep list 1. **Does any dependency refresh quickly?** `allTasks`, a live clock, a task identity — any of them rebuilds the closure on the next update, making the wrong answer a bounded window. The chip keys on `allTasks` and recovers; the indicator keys on `task.column`, which never changes, so it never does. 2. **Is the value covered transitively?** A dependency that itself lists the flags gets a new identity when they arrive, and that propagates. ## A gate was built and rejected — the part worth writing down The scanner reports **19 sites; two were real.** Property 2 is why: transitive coverage is invisible to any purely syntactic check and would need a real dependency graph. `TaskCard`'s context-menu memo omits all three role flags and is **nonetheless correct** — it depends on `taskActionMenuModel.actions`, and that model lists `taskColumnFlags`, so the whole chain recomputes. I checked that before filing it, which is the only reason this PR isn't a bug report about missing Archive/Revert menu entries. Freezing 19 would have baselined mostly noise and trained everyone to skip the report — the exact failure this document already records for `sortTasksForDisplayColumn`, where an annotation saying "ignore these" hid a real defect for days. **A good investigative tool is not automatically a good ratchet**, and the next person deserves to know the turn was considered rather than missed. The triage that does work is cheap: run the scan, then ask the two questions above. Nine of nineteen survive question 1; hand-checking those is an afternoon, not a project. Docs only — no code, no baselines. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…thout asking (#3011) The **sixth** instance of the async-memo shape #2998 documents — and the only one that **loses work** rather than mis-rendering. `handleDrop` gates the "Preserve Progress?" confirmation on the lane's role: ```js const shouldPrompt = hasStepProgress && isPreImplementationColumnRole(columnFlags, column); if (shouldPrompt) { const keepProgress = await confirm({ … }); … } ``` …but its `useCallback` deps were `[addToast, allTasks, column, confirm, onMoveTask, tasks, t]` — no `columnFlags`. The board resolves workflow traits after first paint, so the DOM keeps the closure built during the pre-load render, where `columnFlags` is `undefined` and the helper falls back to `LEGACY_PRE_IMPLEMENTATION_COLUMN_IDS`. A renamed intake lane is not a member. **Result: a card with completed steps dropped into that lane moves with `shouldPrompt === false`.** The user is never offered "Keep Progress", and the steps are reset silently. ## How this was found It is the last unverified candidate from the derivation-aware scan I posted on #2998, where I explicitly declined to file it as a bug without checking. Checking it is what turned it from a scanner hit into this. ## Severity, stated honestly `allTasks` and `tasks` are in the dep list and change identity on any task-list refresh, so the stale closure is rebuilt within seconds on a busy board. The exposure is the quiet gap right after the traits land — **bounded**, like the near-duplicate chip (#2997), not permanent like the ticker (#2996) whose only refreshing dependency fired at local midnight. Bounded still matters here because the cost is not a wrong pixel: it is completed steps discarded without a prompt, and the window is exactly when someone has just opened a board and starts dragging. ## Verification | state | result | |---|---| | clean | 2/2 pass | | revert `columnFlags` from the deps | **1 failed / 1 passed** | The paired negative asserts a non-pre-implementation lane still moves **without** prompting — a fix that prompts everywhere turns the dialog into noise that gets clicked through, costing the same progress it protects. The observable is `confirm`, not `onMoveTask`: whether the user was *asked* is the contract, and asserting on the move alone passes either way. `tsc -p tsconfig.app.json` 0 errors in the new file, lint clean, FNXC gate exit 0, 4/4 across both `Column` flags-arrival suites. ## Running tally of this shape ticker (#2996) · near-duplicate chip (#2997) · fan-out trait index (#2993) · merge signature (#3001) · lifecycle dates (#3007) · this. Six, in two components plus the fan-out path. #3001 called the merge signature "the last live site"; it was the last of *that* sweep's nine candidates, and two more have surfaced since from a different scan. Worth knowing before anyone declares the class closed again. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.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 Board half of #2990 is inert
useBlockerFanoutmemoizes on:columnFlagsByTaskIdisn't in the list.Boardbuilds that index fromboardWorkflows, 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 > 0guard 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-depsrule, and a disable directive for it fails CI, so the dep list is maintained by hand.Measured
expected +0 to be 2useBlockerFanout+ExecutorStatusBar+ new Board suiteCensus and FNXC gates green; lint and
tscclean.The test is the point
It drives the real
Boardwith a mockedColumnthat captures the map it was handed. The existing suites callcomputeBlockerFanoutMapdirectly 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
undefinedrather 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, themergeOrchestrationarm, 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
ExecutorColumnFlagsPickdroppinghumanReviewproduced a wrong answer. It doesn't — the sole supplier (App.tsx:553) passes the wholecolumn.flagsobject, so the trait is there at runtime. Latent type-safety at most; your type is fine as-is.