fix(dashboard): the time indicator was gated on a hardcoded legacy lane set - #3014
Merged
Conversation
…ne set CORRECTION TO #2996, and the reason this exists. That PR fixed the SUBSCRIPTION — `wantsLiveTimeIndicator` kept a pre-load answer, so the card never joined the shared ticker — and I described it as making renamed-lane cards "show their live elapsed-time indicator". It made them eligible to. They still rendered nothing, because a second gate rejects them first, and I did not look past the seam I had just fixed. `TIME_INDICATOR_COLUMNS` is `{in-progress, in-review, done}`. Both the `timeIndicator` memo and the `chipFarRight` layout test `task.column` against it directly, so a card in a renamed WIP, review or completion lane returns null whatever its resolved traits say. WHY NO CHECK SAW IT: the census counts COMPARISONS against legacy ids, and this is a Set literal — a DEFINITION. Nothing in the backlog ever pointed here, the same blind spot that hid `BLOCKER_ESCALATION_COLUMNS` until someone read the code. The legacy set stays as the no-flags fallback, marked DELIBERATE-LITERAL, so a card whose traits have not resolved — first paint, or a lane its workflow no longer declares — behaves exactly as before. MEASURED - test red first for the right reason: control and negative passed, only the renamed case failed, `expected false to be true` - after: 3 passed - reverting the memo gate to the raw set fails that case again - five TaskCard suites: 418 tests green - all five gates green; lint and tsc clean The negative case: resolving traits must not put a live timer on every lane. A card in the renamed INTAKE lane has not started, so it stays out — otherwise the fix trades a missing indicator for a running clock on work that has not begun. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Verified — 3/3, and reverting This one matters more than its diff suggests, and the self-correction is the reason:
That is the most useful sentence in this batch. #2996 was correct, well-tested, and mutation-verified — I checked it myself and signed off — and the feature it claimed to restore still didn't work. A passing test at the seam you fixed says nothing about the seam downstream of it. Both of us verified the subscription and neither of us asked whether anything rendered. Worth pairing with #3012's severity correction, because they are the same failure at different scopes: we were reasoning about the mechanism we had just touched rather than what the user sees. The dep-list heuristic sorted by identifier name; this one verified by hook eligibility. Both stopped one layer short of the observable.
Keeping the legacy set as the no-flags fallback is right, and the One process note, on my own probeMy first mutation targeted |
This was referenced Jul 31, 2026
Contributor
|
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 (3)
📝 WalkthroughWalkthroughTaskCard now uses resolved workflow column roles to control elapsed-time indicators. It retains legacy column IDs as a fallback and adds regression tests for legacy, renamed work-in-progress, and intake lanes. ChangesRenamed lane time indicators
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
✨ Finishing Touches📝 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 |
This was referenced Jul 31, 2026
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
… entry was wrong (#3018) It hid two real defects — and it explicitly told the next reader not to re-probe them. ## What the entry did Counted **declarations** (48, then 49) and concluded the population was benign because each one is a fallback vocabulary, a builtin column list, or an already-converted seam. All true of the declarations. **The declaration isn't where the defect lives.** ## Measure the use, not the declaration A collection used as a **membership gate against a column**. Nine exist, and two were live user-visible defects sitting inside a population this doc had marked clean: | site | defect | |---|---| | `TIME_INDICATOR_COLUMNS.has(task.column)` — `TaskCard` | elapsed-time indicator never rendered on a renamed board (#3014) | | `PLANNER_ACTIVITY_COLUMN_IDS.has(task.column)` — `useTasks` | planning border and pulsing badge never appeared (#3017) | The other seven are genuinely fine, and the reasons are kept because they're the shapes worth recognising: the no-flags fallback *inside* a role helper, a seam that seeds the legacy pair then unions resolved lanes, a marked `DELIBERATE-LITERAL` fallback chain, and a plugin with no trait source at all. ## The tell One question separates the two groups: **does a flags path exist in this file at all?** Both defects had none — the gate was the only decision, with nothing to degrade from. Every benign case had a resolved path sitting right next to the literal. ## Why this is worth its own PR A "do not re-probe" note that is wrong is **worse than no note**: it converts one person's incomplete measurement into everybody's blind spot. That's the same failure this document already records for `sortTasksForDisplayColumn`, one level up — there an annotation told readers to skip a *row*, here it told them to skip a *population*. I wrote the original entry, and I'd read past it twice myself before #3014 forced the re-measurement. 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
…e lane (#3017) ## How this was found — by re-testing a claim of mine The learnings doc records "named legacy-id collections" as **measured and clean**: 48 declarations, all fallback vocabularies, builtin column lists, or already-converted seams. #3014 disproved that conclusion — `TIME_INDICATOR_COLUMNS` was in that population and was a live defect. So I re-measured over the shape that actually matters: **collections used as a membership gate against a column.** Nine exist. | site | verdict | |---|---| | `columnRoles.ts` ×2, `useSessionFiles.ts` | the no-flags fallback *inside* the role helpers — correct by design | | `branch-group-ops.ts` | seeds the legacy pair then unions resolved lanes — already converted | | `DocumentsView.tsx` | marked `DELIBERATE-LITERAL` fallback chain | | `TaskCard.tsx` ×2 | fixed in #3014 | | `plugins/…/reconciler.ts` | plugin with no trait source — same class as #3003 | | **`useTasks.ts`** | **no flags path anywhere in the file** | ## The defect `useTasks` stamps `recentAgentActivityAt` only for cards in `{triage, todo}`. The note at that set argues over-stamping is harmless because every consumer re-checks for an intake lane before showing anything. That's true, and it **only protects against false positives**. On a board whose intake and hold lanes are renamed, the pair matches nothing — so no stamp is ever written, and a correct downstream role check has nothing to filter. The planning border and pulsing badge never appear while the planner is actively working the card. ## The supplier ships with the seam An optional resolver with no caller is the first failure shape in the learnings doc, and my own gate would flag it — so `App` supplies it in the same commit. `useBoardWorkflows` moved above `useTasks` to make that expressible; it depends on `projectId` alone, nothing about tasks, so reading it first is safe. Remote rows deliberately get **no** flags — they belong to another store, and local board-workflow metadata must never be applied to their ids. That's the rule the footer index already follows. ## Measured | check | result | |---|---| | `useTasks` suite | 124 → **126**, all green | | reverting the gate to the legacy pair | fails exactly the renamed case; the negative (renamed WIP is not planning) still passes | | `App.test` + `useTasks` together | **269 green** | | gates | all five green; lint and `tsc` clean | ## One observation I could not reproduce The `App`+`useTasks` pair failed once, on a single unnamed test, and passed on **four** subsequent runs including three consecutive. The captured output showed jsdom URL-parse noise from `MissionManager` fetches rather than an assertion failure, and the same pair is green on unmodified `main`. I'm not quarantining another file's test on one unreproducible observation, but recording it rather than letting a green rerun bury it — if it resurfaces in CI, this is the prior sighting. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 31, 2026
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…amed board (#3022) Closes #3020 — which I filed **instead of** fixing, on a rationale that turned out to be wrong. I said the plugin had no scaffolding for faking `CePipelineStore` + `taskStore` together. It does: `_harness.ts` already builds a real `PluginContext` over a live PostgreSQL layer. The gap was **two missing readers on its task-store stub**, not missing infrastructure. I checked the harness only after filing. ## The defect `TERMINAL_COLUMNS` is `{in-review, done}`, and the reconciler advances a pipeline only when **every** current-stage board task is in that set. On a board whose review and completion lanes are renamed that's false for every task, permanently: - the pipeline never advances a stage - it never creates its outbound task - it sits `running` indefinitely Nothing errors, so it reads as work that hasn't finished. Unlike the display defects in this family (#3014, #3017), the CE flow actually **stops**. ## Shape The decision is extracted to an exported `isStageTerminalColumn` because it *is* the whole decision. Left private it could only be reached through a pipeline-state + links + board-tasks fixture, and the half that needed proving is that a renamed board resolves to its own lanes through this store. It uses `resolveReviewColumns` rather than re-deriving the union — that helper is the documented review **set** (`mergeOrchestration ∪ mergeBlocker ∪ humanReview`), so a board splitting those across a merge lane and a human lane is covered without this site drifting from it. ## Two things my first attempt got wrong **The fixture spelled traits in camelCase** — `{ trait: "humanReview" }`. Trait **ids** are kebab-case (`human-review`, `merge-blocker`, `wip`); the camelCase names are the resolved **flags**. Those columns therefore resolved to *no roles at all*, silently, because an unknown trait isn't an error. `complete` is spelled identically in both vocabularies, which is exactly what made the first run look like *"complete works, review is broken"* rather than *"the fixture is wrong"* — I nearly went debugging the production union. **The harness extension is additive** and inert until a test seeds it, so all 24 existing plugin suites see the previous shape. ## Measured | check | result | |---|---| | new suite | **4/4** | | reverting to the literal-only gate | fails **exactly 2** — the renamed-terminal case, and a board declaring a NON-terminal column named `done` — while the legacy control and the WIP/intake negative still pass | | plugin suite | **24 files, 184 tests green** | | `tsc` + all five gates | clean | That second row is the one that matters: the `done`-without-`complete` board is the only shape where a real resolution and a legacy fallback disagree, so it's what separates the fix from a lucky agreement. --------- 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.
This is a correction to #2996, and that's why it exists
#2996 fixed the subscription:
wantsLiveTimeIndicatorkept a pre-load answer, so the card never joined the shared ticker. I described it as making renamed-lane cards "show their live elapsed-time indicator."It made them eligible to. They still rendered nothing, because a second gate rejects them first — and I didn't look past the seam I'd just fixed.
Both the
timeIndicatormemo and thechipFarRightlayout testtask.columnagainst that set directly, so a card in a renamed WIP, review or completion lane returnsnullwhatever its resolved traits say.Why no check saw it
The census counts comparisons against legacy ids. This is a
Setliteral — a definition. Nothing in the backlog ever pointed here, which is the same blind spot that hidBLOCKER_ESCALATION_COLUMNSuntil someone read the code rather than the report.The fix
The gate becomes a role question, with the legacy set kept as the no-flags fallback and marked
DELIBERATE-LITERAL. A card whose traits haven't resolved — first paint, or a lane its workflow no longer declares — behaves exactly as before.Measured
expected false to be true)TaskCardsuitestsccleanThe negative case
Resolving traits must not put a live timer on every lane. A card in the renamed intake lane hasn't started, so it stays out — otherwise the fix trades a missing indicator for a running clock on work that hasn't begun.
Worth noting for the pattern
Two of my last four findings came from re-examining my own merged work rather than from new code: this one, and the
boundedheuristic correction in #3012. Fixing one seam and declaring the symptom gone is its own failure mode — the user-visible behaviour needed both halves, and I only checked the half I'd touched.Summary by CodeRabbit