[mrgoonie]

What

recompute-all-skills.ts was 210 lines, over the repo's 200 LOC limit (issue #27). Splitting the loop alone would not fix the real problem: the Bayesian rating / composite score / trending score math existed twice, once here and once in recompute-skill-scores.ts, and both copies had to agree on every future scoring change.

This extracts the math into one pure function, buildScoreUpdate() in apps/web/app/lib/leaderboard/recompute-score-update.ts, and points both callers at it.

Changes

• New recompute-score-update.ts (113 lines) — buildScoreUpdate({ skill, aggregates, globalStats, maxTrendingRaw }) returns exactly the four columns both callers write: bayesian_rating, composite_score, trending_score, favorite_count.
• New recompute-score-update.test.ts (157 lines, 8 cases) — guard paths, Bayesian shrinkage toward the global mean, neutral 0.5 success rate, null rating columns.
• recompute-all-skills.ts 210 → 166 lines — local SkillAggregates moved to the new module, per-skill loop body collapsed to a single buildScoreUpdate + db.update.
• recompute-skill-scores.ts 140 → 117 lines — its separate reads folded into SkillAggregates and passed to the same builder. Export name and path unchanged, so the 5 routes and 4 test mocks that vi.mock this module are untouched.

Behaviour

No change. The Math.max(..., 1) guards are kept, so a zero-max trending window and null rating columns still produce finite numbers instead of NaN. Because the math is now one function, the two paths can no longer drift apart.

Verification

• npx vitest run — 157 files, 2893 passed, 3 skipped
• pnpm typecheck — Done for skill-tree, contracts, cli, apps/web

Closes #27

[clark-cant]

Re-review — 370a7a8 (2026-10-31)

Read all four files at the head plus leaderboard-scoring.ts and scoring-utils.ts, and checked every claim in the description.

Verified

• LOC is exactly as advertised: recompute-all-skills.ts 166, recompute-skill-scores.ts 117, recompute-score-update.ts 113, test 157. The 210 -> under-200 target for #27 is met, and it leaves room for the 15-20 line guard #26 will add.
• The math now has one production call site. grep -rn "computeBayesianRating|computeCompositeScore|computeTrendingScore" apps/web returns only recompute-score-update.ts and the definitions in leaderboard-scoring.ts; nothing else consumes the primitives directly, so the "both copies must agree" failure mode is gone rather than merely reduced.
• Behaviour parity holds line by line: same ?? 0 coercions, same Math.max(maxNetVotes, 1) / Math.max(maxFavorites, 1), same total > 0 ? successes / total : 0.5 rule (now successRateFrom, and NaN-safe because !(total_usage > 0) catches it), same recent_ratings / recent_usage inputs, same four written columns. The single path folds its own reads into SkillAggregates with total_usage = successData.total, so the two paths cannot drift.
• isComplete = batch.length < batchSize is untouched, which is correct for this PR's scope.

Nits, none blocking

1. SkillAggregates.skill_id is required by the builder input but never read by buildScoreUpdate(). It is the map key in the bulk path and a value the single path has to invent. The pure builder's contract should not carry a query-shape field, so aggregates?: Omit<SkillAggregates, "skill_id"> (or making it optional) keeps SkillAggregates as the row type without implying the builder needs an identity.
2. The de-duplication stops at the math. Both modules still carry an identical global-stats select (coalesce(avg(avg_rating), 0) plus four coalesce(max(...), 1)) and an identical max-trending subquery over ratings / usage_stats, roughly 30 lines each. That is exactly the kind of copy that drifts next — window length, coalesce defaults — so a follow-up loadGlobalStats(db) / loadMaxTrendingRaw(db) helper would finish the job. Not this PR: it would push the diff back over the LOC goal it is fixing.
3. No parity test. The 8 builder cases pin the math, but nothing asserts that the two entry points write the same four columns for the same skill. A mocked-db test per entry point comparing the .set() payload would have caught a wrong aggregate mapping — for example recent_usage passed into the ratings slot — which is cheap insurance for a refactor whose entire claim is "no behaviour change".

Cross-PR

No file overlap with #169 (api.admin.recompute.ts plus new lib/admin/batch-limit.ts), so either merge order is clean. Note that #169 only bounds the route parameter, so after both land recomputeBatch still has no empty-batch guard — that residual is tracked in #28 and is not a defect in this PR.

Verdict

Sound, behaviour-preserving, and it closes #27's LOC problem without the third copy that a loop-only split would have created. Not merging: reviewDecision is empty and only an owner approval clears that. Merging on approval is fine as-is.

Posted by the github-maintain skill at 2026-10-31T13:08:00Z