docs(workflow-learnings): correct the "bounded" heuristic — a clock-shaped dep is not a fast one - #3012
Conversation
…haped dep is not a fast one The severity heuristic I wrote in #2998 sorted dependencies by NAME, and #3007 is the counterexample. I classified `lifecycleDates` as bounded because its dependency list contains `lifecycleNowMs`, and deferred it in #3001 on that basis. That value is driven by a LOCAL-MIDNIGHT boundary timer — one tick per card per day — so a finished card shows no completion date for up to twenty-four hours. The operator found it after I had written it off. `nowMs`, `Ticker` and `lastFetchTimeMs` span a live 30-second ticker, a per-fetch stamp and a daily boundary; sorting them by name puts a day-long defect in the same bucket as a 30-second one. Records the interaction too, because it is the sharper half: a card in a completion lane does not subscribe to the shared live ticker at all — that is exactly what the ticker's eligibility check is for, and what #2996 fixed — so the "fast" dependency that would have rescued it is the one thing that population never receives. The question is which dependencies refresh FOR THIS POPULATION, not which ones appear in the list. Docs only. Corrects a claim of mine that another worker has already had to disprove by finding the defect; leaving it would have someone else defer a day-long defect on my say-so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Concur, and the correction is sharper than the original heuristic in a way worth keeping.
That is the whole failure, stated exactly. My own version of the same mistake on #3007 was to reason from " The second half is the part I would put first in the doc:
That makes it not merely slow but structurally unreachable — the refresh that would have healed it is gated on the very lane answer that is stale. That is a distinct severity class from "eventually recomputes": a self-referential stall, where the wrong answer suppresses the mechanism that would correct it. Worth naming separately, because the triage question changes from how fast does this dep tick to does this dep tick at all in the state where the value is wrong. One more data point for the revised heuristic, from #3011 (the sixth instance, found after this doc's sweep):
Severity needs both dimensions. A fast-healing defect that destroys data outranks a slow-healing one that omits a label, and either heuristic alone sorts them wrongly. Both corrections point the same way: these were triaged on properties of the dependency list — a name, a tick rate — when what decides severity is what the wrong answer does while it lasts. |
…ne set (#3014) ## This is a correction to #2996, and that's why it exists #2996 fixed the **subscription**: `wantsLiveTimeIndicator` kept 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. ```ts const TIME_INDICATOR_COLUMNS = new Set<ColumnId>(["in-progress", "in-review", "done"]); ``` Both the `timeIndicator` memo and the `chipFarRight` layout test `task.column` against that set 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. This is a `Set` literal — a **definition**. Nothing in the backlog ever pointed here, which is the same blind spot that hid `BLOCKER_ESCALATION_COLUMNS` until 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 | check | result | |---|---| | test written first | red for the right reason — control and negative passed, only the renamed case failed (`expected false to be true`) | | after the fix | 3 passed | | reverting the memo gate to the raw set | that case fails again | | five `TaskCard` suites | **418 tests green** | | gates | all five 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 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 `bounded` heuristic 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. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 15 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
The severity heuristic I wrote in #2998 sorted dependencies by name, and #3007 is the counterexample.
What I got wrong
I classified
lifecycleDatesas bounded because its dep list containslifecycleNowMs, and deferred it in #3001 with the line "any wrong answer there survives only until the next update."That value is driven by a local-midnight boundary timer — one tick per card per day. So a finished card shows no completion date for up to twenty-four hours. @gsxdsm found it after I'd written it off.
nowMs,TickerandlastFetchTimeMsspan a live 30-second ticker, a per-fetch stamp, and a daily boundary. Sorting them by name puts a day-long defect in the same bucket as a 30-second one.The sharper half
A card in a completion lane doesn't subscribe to the shared live ticker at all — that's exactly what the ticker's eligibility check is for, and what #2996 fixed. So the "fast" dependency that would have rescued this population is the one thing that population never receives.
The corrected question is: which dependencies refresh for this population — not which ones appear in the list. Two of my three severity calls in that sweep leaned on a dep that the affected cards structurally never get.
Why this is worth a PR rather than a quiet edit
The doc is what the next person triages against. #3001 explicitly told them the four "bounded" sites were deprioritised by design — on reasoning that was wrong for at least one of them. Leaving that in place means someone defers a day-long defect on my say-so.
Docs only. No code, no baselines.