fix(core): task-duration stats were computed from an empty set on a renamed board - #2870
Conversation
…enamed board aggregateProductivityAnalytics filtered its duration query on "column" = 'done'. On a renamed board that matches nothing, so the whole distribution — median, p90, average, total — is computed from an EMPTY row set and reports zeros while the project ships work. Nothing errors. Third instance of the same shape, same fix as #2864 and #2866: resolve per PROJECT via resolveProjectColumnsForRoles, bind an IN list, thread the store from the single Command Center caller so the parameter has a supplier immediately. MEASURED: reverted, only the renamed case fails; the default-vocabulary control, the WIP-lane negative, and the omitted-store legacy case stay green. With the fix, 4 passed. The negative asserts medianMs is null as well as completedTasks being 0, because this fix's failure mode is worse than the bug: resolving too many lanes would put unfinished work into the distribution and produce a plausible-but-wrong median, which invites no scrutiny at all. Test-shape note: TaskDurationSummary exposes completedTasks, not count. The first version asserted a field that does not exist and failed with "expected undefined to be 1" on every case including the controls — a fixture error that looks exactly like a product failure until you read it. The sync SQLite arm keeps its literal: it throws in backend mode and has no production caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 2 minutes 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 (5)
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 |
Greptile SummaryFixes PostgreSQL productivity duration analytics for renamed workflow lanes.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/core/src/productivity-analytics.ts | Resolves complete-lane identifiers through the supplied project store and binds them into the PostgreSQL duration query while retaining the legacy fallback. |
| packages/dashboard/src/routes/register-command-center-routes.ts | Supplies the already project-scoped store to productivity analytics so lane resolution and database access share the same project. |
| packages/core/src/tests/postgres/productivity-analytics-renamed-lanes.pg.test.ts | Adds positive and negative PostgreSQL coverage demonstrating that renamed complete lanes are included while renamed WIP lanes remain excluded. |
| scripts/lib/sql-column-literals-baseline.json | Reduces the expected productivity-analytics SQL lane-literal count after replacing the PostgreSQL literal. |
| .changeset/productivity-analytics-renamed-lanes.md | Documents the renamed-lane analytics correction as a patch release. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Request[Command Center productivity request] --> Store[Project-scoped task store]
Store --> Resolver[Resolve project complete lanes]
Resolver --> Query[Parameterized PostgreSQL IN filter]
Query --> Stats[Duration summary and trend]
Reviews (2): Last reviewed commit: "chore(gate): re-record the SQL baseline ..." | Re-trigger Greptile
This branch removes legacy comparisons, and the gate (#2841, merged after this was opened) fails on a LOWERED count so a migrated slot cannot be silently regrown into. Without this the branch turns main red on merge, which is what #2864 did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Added a baseline re-record for the SQL column-literal gate (#2841), which merged after this PR was opened. That gate fails on a lowered count — deliberately, so a migrated site cannot leave an unused allowance behind for the surface to regrow into. This branch removes legacy comparisons, so without the re-record it turns Baseline entries are per file and this PR only touches its own, so it can merge in any order relative to the other analytics conversions. The coupling was mine to spot when I opened five PRs against a gate that did not exist yet, and I did not. |
**`pnpm test:gate` and both `pretest` hooks fail on `main` right now.** Merge this first. ## What happened #2841 (the SQL gate) merged, then #2864 merged. #2864 removed three legacy comparisons from `team-analytics.ts`, but its baseline entry still allows six — and this gate **fails on a lowered count by design**, so a migrated slot cannot be silently regrown into later. Baseline 30 → 28. ## This is my sequencing error The four analytics conversions were branched and reviewed **before** the gate existed, so none of them carries a baseline update. The gate then landed first, which means **each of them breaks `main` as it merges**. I opened all five without thinking about the order they would land in. The three still open — #2866, #2870, #2871 — will each do this again. I am adding baseline updates to them next so they land clean. ## Note on the downward check The "count went down" failure looks like pedantry until it fires. It exists so a migrated site cannot leave an unused allowance behind for the surface to regrow into — the same rot as an allow-list entry for a deleted function. The real cost is that a conversion and its gate have to land in a known order, which is a coupling I created and did not plan for. ## Verification `pnpm test:gate` green with the re-recorded baseline · lint 0 · `node scripts/check-sql-column-literals.mjs` exit 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
) Fourth and last of the lane-bound analytics sites from #2839, after #2864, #2866 and #2870. ## The defect `aggregateGithubIssueAnalytics` and its GitLab twin filtered their resolved-issue query on `"column" = 'done'`. On a renamed board that matches nothing, so `fixed` is **zero**, the resolved-issue list is empty, and `net` reports every filed issue as still outstanding — while the team closes issues all week. Nothing errors. Same fix as the previous three: resolve per **project** via `resolveProjectColumnsForRoles`, bind an `IN` list, thread the store from each Command Center caller so the parameters have suppliers immediately. ## Both providers in one change, deliberately These two files are **copies** — same query, only the provider literal differs — and a copy is exactly what gets half-fixed. Converting one and not the other type-checks, passes that provider's test, and leaves the second silently broken with no signal anywhere. The suite runs every case against both, so the pair cannot drift. ## Measured Reverted, exactly the two renamed cases fail — **one per provider** — while both default-vocabulary controls, both WIP-lane negatives, and both omitted-store legacy cases stay green: ``` ✓ github: default vocabulary counts a resolved issue × github: renamed vocabulary counts a resolved issue ✓ github: renamed vocabulary does NOT count an issue still in the WIP lane ✓ github: without a lane store, the legacy id still answers ✓ gitlab: default vocabulary counts a resolved issue × gitlab: renamed vocabulary counts a resolved issue ✓ gitlab: renamed vocabulary does NOT count an issue still in the WIP lane ✓ gitlab: without a lane store, the legacy id still answers Tests 2 failed | 6 passed (8) ``` That the failures are symmetric is itself the check on the copy-paste risk. ## Scope The sync SQLite arms keep their literals: they throw in backend mode and have no production caller, the same dead-arm conclusion as `cleanupStaleMergeQueueRowsImpl` on #2839. ## Verification `pnpm test:gate` green · Command Center + GitLab issue analytics suites 10/10 · `tsc` core 0, dashboard 0 · lint 0 · changeset included. --- **This closes the lane-bound half of #2839.** All 14 sites the hand-review identified as genuinely vocabulary-bound are now converted across four PRs. What remains there is the 11 `!= 'archived'` exclusions, which are probably correct as literals — archiving writes `task.column = 'archived'` unconditionally as a state rather than a lane — plus one dead SQLite arm. Those need per-site judgment, not conversion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third of the 14 lane-bound SQL sites from #2839, after #2864 and #2866. Independent of both.
The defect
aggregateProductivityAnalyticsfiltered its duration query on"column" = 'done'. On a renamed board that matches nothing, so the entire task-duration distribution — median, p90, average, total — is computed from an empty row set and reports zeros while the project ships work. Nothing errors.Same shape and fix as the previous two: resolve per project via
resolveProjectColumnsForRoles, bind anINlist, thread the store from the single Command Center caller so the parameter has a supplier immediately rather than becoming an inert seam.Measured
Reverted, only the renamed case flips:
The negative asserts the median, not just the count
This fix's failure mode is worse than the bug it fixes. Resolving too many lanes would pull unfinished work into the distribution and produce a plausible-but-wrong median — a number nobody questions — where the bug produces an obvious zero. So the WIP-lane case asserts
medianMsis null as well ascompletedTasksbeing 0.A fixture error worth naming
My first version asserted
taskDuration.count.TaskDurationSummaryexposescompletedTasks. Every case failed withexpected undefined to be 1— including the controls — which reads exactly like a broken product until you notice the control is failing too. A control that fails is a fixture bug, not a finding; that asymmetry is the fastest way to tell them apart.Scope
The sync SQLite arm keeps its literal: it throws in backend mode and has no production caller, the same dead-arm conclusion as
cleanupStaleMergeQueueRowsImplon #2839.Verification
pnpm test:gategreen · both Command Center analytics suites 8/8 ·tsccore 0, dashboard 0 · lint 0 · changeset included.🤖 Generated with Claude Code