refactor(leaderboard): split recompute-all-skills.ts, 210 LOC over the 200 LOC limit #27

Activity

1. mrgoonie

   added

   refactorInternal restructuring, no behavior changeInternal restructuring, no behavior change

   on Sep 15, 2026
2. clark-cant commented on Oct 31, 2026

   

   clark-cant

   on Oct 31, 2026

   More actions

   Verified on main at c95ab77. The LOC violation is real and the numbers in the report still hold, but the extraction seam is described slightly wrong and the proposed split would land a third copy of code that already exists twice.

   Confirmed

   • wc -l apps/web/app/lib/leaderboard/recompute-all-skills.ts → 210 (the N+1 rework did not bring it under).
   • CLAUDE.md:164 — "Max 200 LOC per file — split into focused modules if exceeded."
   • Nothing else in lib/leaderboard/ is close: recompute-skill-scores.ts 140, leaderboard-scoring.ts 84, recompute-state.ts 49, and routes/api.admin.recompute.ts 131.

   Correction: the seam is 131-174, not 139-176

   for (const skill of batch) opens at line 131 and the loop closes at 174; line 139 is the computeBayesianRating(...) call inside the body. Ending the extraction at 176 also pulls newOffset / isComplete out of the orchestrator, and those belong with the state block at 179-195 — which must stay inside recomputeBatch for #26, where the 409 claim and the run_id fence have to wrap the write at 191-195.

   Also missing from the plan: interface SkillAggregates is declared inside recomputeBatch (105-112). It has to move to the new module and be exported, otherwise the helper cannot type its argument.

   The larger point: this computation already exists twice

   recompute-skill-scores.ts repeats it almost line for line — the same global-stats query (22-30 vs 58-66), the same max-trending subquery (89-101 vs 72-84), the same compute block (104-128 vs 139-163, including the Math.max(..., 1) guards) and the same four-column UPDATE (131-139 vs 165-173). Extracting only the loop turns two copies into three.

   So extract the math, not the loop:

   • new apps/web/app/lib/leaderboard/recompute-score-update.ts exporting SkillAggregates and a pure buildScoreUpdate({ skill, aggregates, globalStats, maxTrendingRaw }) that returns bayesian_rating / composite_score / trending_score / favorite_count;
   • recomputeBatch keeps the db.update and the state writes → 210 becomes roughly 165;
   • optionally point recomputeSkillScores at the same builder later (140 → ~100). Keep that out of this PR: 5 routes import that module and 4 test files vi.mock("~/lib/leaderboard/recompute-skill-scores"), so its export path must not move.

   Why it is worth doing now

   #26 adds a 409 in-flight guard plus run_id fencing, roughly 15-20 lines in this same file. Split first and it lands ~185; split after and you are refactoring a file that just grew.

   On the open question — new helper vs moving the aggregate fetch: the fetch block (102-128) is the second-best candidate and gets close to the limit on its own (~183), but it de-duplicates nothing, so the drift above survives. The pure builder is the better answer.

   Tests: neither module has a test file today (find apps/web -name "*recompute*" returns only the four sources). A pure buildScoreUpdate is unit-testable with no D1 or KV binding, because every normalization input arrives as a plain number.

   No work in flight: no branch carries this (branch list has no recompute/refactor branch), no open or closed PR references #27, and this is the first comment on the issue.

   Verdict: keep open. Mechanical and behavior-preserving, but do it as a dedupe — shared pure builder, orchestration stays put — rather than the loop-only split as filed.

   ---

   Generated by the github-maintain skill at 2026-10-31
3. mrgoonie

   added

   akadmin-wipAKAdmin is implementing this right now — do not start parallel workAKAdmin is implementing this right now — do not start parallel work

   on Oct 31, 2026
4. mrgoonie commented on Oct 31, 2026

   

   mrgoonie

   on Oct 31, 2026

   ContributorAuthor

   More actions

   Claiming this one now.

   Root cause is not only the LOC limit: the Bayesian rating / composite / trending math lived twice, in recompute-all-skills.ts and recompute-skill-scores.ts. Splitting just the loop would have left two copies that must agree on every future scoring change.

   So the fix extracts one pure buildScoreUpdate() and points both callers at it:

   • recompute-all-skills.ts 210 → 166 lines
   • recompute-skill-scores.ts 140 → 117 lines
   • new recompute-score-update.ts + 8 tests

   Verified before pushing: full vitest run 2893 passed / 3 skipped, and pnpm typecheck clean across all 4 packages.

   PR: #170