Code-review follow-ups: obstacle radius, SeriesTally extraction, trail perf, test coverage

[tim4724]

Follow-ups from a thorough code/architecture review. Correctness, coverage, performance, and maintainability — no behavior change in the live game (verified end-to-end).

Changes

Area	What	Why
fix(slopes)	One obstacleRadius(kind) source of truth (shared/slopes.js), consumed by builder + scenery; engine fallback made kind-agnostic; RAMP_HALF_S named; dead ramp.radius dropped	The per-kind default was duplicated across 3 files and inverted (builder rock=0.85/tree=0.7 vs engine rock=0.7/tree=0.8). Latent footgun — live path unaffected since the builder always stamps a radius.
test(centerline)	First direct tests for the Catmull-Rom spline	Centerline.js had zero coverage — engine tests use a stub, so the real interpolation + divide-by-zero nudges never ran under test.
perf(trails)	Scope per-point trail GPU uploads to the changed slot (addUpdateRange)	Was re-uploading two full ~48KB ring buffers per recorded point per skier — the dominant trail bandwidth cost on weak hardware. Index left full-upload (wrap seam).
fix(controller)	Bind keyboard listeners in start()/stop(), not the constructor	stop() only removed pointer/orientation listeners — keyboard listeners leaked on re-instantiation.
refactor(display)	Extract SeriesTally from the 1170-line main.js (dependency-free, unit-tested); share runTag; cap showSoundHint poll	The trickiest run-to-run state now lives in one testable unit (it previously had no unit coverage, only e2e).

Verification

• 171/171 unit tests (incl. new seriesTally + centerline + obstacleRadius tests)
• 15/15 e2e over the real relay — the gold-standard test of the series orchestration the refactor touched
• Browser-verified the final + mid-series boards and live ski trails (no NaN, no console errors, no visual artifacts)
• Adversarial diff review (3 reviewers + completeness critic): clean

Deliberately out of scope

Hostile-peer defenses don't apply to a local couch party game (the relay-stamped from is the real trust boundary), so these were skipped by design: reclaim-token auth, message-size cap, PAUSE debounce, and the style-src CSP nit. Also skipped: car→Skier rename (churn) and the MODULE_TYPELESS test warning (risky fix).

🤖 Generated with Claude Code

[github-actions]

Preview

• Display: https://powder-chore-review-followups.couch-games.com
• Controller (phone): https://powder-chore-review-followups.couch-games.com/controller/index.html
• Display gallery: https://powder-chore-review-followups.couch-games.com/gallery.html
• Phone gallery: https://powder-chore-review-followups.couch-games.com/gallery-controller.html
• Slope gallery: https://powder-chore-review-followups.couch-games.com/gallery-slopes.html

[claude]

Claude finished @tim4724's task in 6m 17s —— View job

---

Code Review

• Read changed files and diff
• Review obstacle radius unification (slopes.js, SlopeBuilder.js, SlopeScenery.js, SkiEngine.js)
• Review SeriesTally extraction (SeriesTally.js, main.js, seriesFormat.js)
• Review trail performance fix (SkiTrails.js)
• Review controller keyboard listener fix (SwipeInput.js, TiltInput.js, ui.js)
• Review new tests (centerline.test.js, seriesTally.test.js, slopes.test.js)

---

No correctness bugs found. The logic across all five change areas is sound. Notes below are test-quality observations only.

fix(slopes) — obstacle radius unification

obstacleRadius(kind) in slopes.js:50 is the right place for this. The inverted-default bug (builder had rock=0.85/tree=0.7, engine had rock=0.7/tree=0.8, scenery debug had yet another) is cleanly resolved. The SkiEngine's new kind-agnostic fallback (0.8) is correct since the engine is supposed to receive a builder-stamped radius and only falls back for hand-built test tracks. RAMP_HALF_S naming is a good catch.

perf(trails) — scoped GPU uploads

The addUpdateRange(vb, VPP * 3) call in SkiTrails.js:100 is correct. Confirmed from three.module.js:203:

gl.bufferSubData(bufferType, range.start * array.BYTES_PER_ELEMENT, array, range.start, range.count);

The vendored Three.js uses element-index units for {start, count}, which matches vb = slot * VPP * 3 (already an element index) and VPP * 3 = 12 elements per slot. The comment in the code matches the implementation. Index stays full-upload — the ring-seam concern noted in the comment is real.

fix(controller) — keyboard listener lifetime

The start()/stop() symmetry in both SwipeInput.js and TiltInput.js is correct. The bound method pattern (binding in the constructor, registering in start(), removing in stop()) matches the pointer/orientation listener pattern already used. The _typing() guard is preserved.

refactor(display) — SeriesTally extraction

The refactor is clean. Key correctness checks:

• fold/buildRows split: fold() is called in advanceToNextRun before teardown, never on the final run. buildRows layers the live run's points on top each render. The guard if (!raceEnded || tally.seriesOver) return in advanceToNextRun correctly prevents folding the final run. ✓
• endCurrentRun() sets seriesOver before broadcastStandings(true) reads tally.seriesOver. ✓
• rekey() is a direct port of the old inline reassignment. ✓
• stagePreview() stores { points } — consistent with how buildRows reads .points from the scores map. ✓
• Co-champion tie-breaking in buildRows: rows.sort((a, b) => b.score - a.score || a.place - b.place) uses the final run's finish rank as tiebreaker (the place field holds r.rank pre-sort). ✓

Tests

centerline.test.js — Good coverage of the real Catmull-Rom math: boundary clamping, finite/unit-tangent sweep, divide-by-zero nudge at duplicate points, two-point degenerate case.

seriesTally.test.js — Solid. Covers: lobby state, run advancement, fold+overlay non-double-count, DNF, co-champions on tied scores, rekey, reset, stagePreview.

slopes.test.js:40-46 — one mild gap: The obstacleRadius test checks the raw generateSlope() output (not the built slope). Since generated defs carry no explicit radius, o.radius || obstacleRadius(o.kind) reduces to obstacleRadius(o.kind) trivially. The two hard-coded value assertions on lines 35-36 are the actually meaningful part. A builder-level test (calling buildSlope() and checking the resolved .radius on resulting obstacles) would more directly validate the "single source of truth" claim — but that requires Three.js and is heavier to set up. Acceptable given the e2e coverage.

Fix this →

---

Overall: clean, well-motivated changes with strong test coverage. The co-champion sort, the trail ring-buffer math, and the fold/buildRows split are all correct. Ready to merge.