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

CLAUDE.md

@@ -53,9 +53,14 @@ single-screen previews stay relay-free via `/?test=1&scenario=…` (see README)
   remaps in `server/index.js`, so update the Dockerfile + CSP when changing them.
 - **Series, not single runs:** a session is N runs (host picks 3/5/7 in the lobby, like the
   difficulty switch — `SET_RUNS`/`RUNS_UPDATE`). The SkiEngine stays run-only (no series
-  concept); the series tally lives in `display/main.js` (`seriesScores`, `runIndex`). Points
-  per run come from the pure `seriesPoints()` in `protocol.js` (linear N..1, DNF 0 — tested in
-  `tests/series.test.js`); cumulative score + champion are folded in `endRun`/`buildSeriesRows`.
+  concept); the series tally lives in `display/SeriesTally.js` (run index/length/over-flag,
+  per-player banked scores, points folding + standings-row derivation) — a dependency-free,
+  THREE-free class (injected `seriesPoints`) so the Node tests load it directly
+  (`tests/seriesTally.test.js`). `main.js` owns the surrounding lifecycle/IO (intermission
+  timer, broadcasts, DOM). Points per run come from the pure `seriesPoints()` in `protocol.js`
+  (linear N..1, DNF 0 — tested in `tests/series.test.js`); cumulative score + champion are
+  folded in `endRun`/`tally.buildRows` (the "Run X of N" header wording is shared with the
+  phone board via `shared/seriesFormat.js`).
   Between runs the display auto-advances after an `INTERMISSION` countdown. `?runs=N`/`?intermission=N`
   pin them for previews/tests (any N≥1; `runs=1` = the old single-run flow — E2E helpers use it).
 - Slope layout is plain data in `public/shared/slopes.js`; `SlopeBuilder.js` turns it into a

public/controller/SwipeInput.js

@@ -79,17 +79,26 @@ export class SwipeInput {
 
     this._down = this._down.bind(this);
     this._up = this._up.bind(this);
-    this._bindKeys();
+    this._onKeyDown = this._onKeyDown.bind(this);
+    this._onKeyUp = this._onKeyUp.bind(this);
   }
 
   start() {
+    if (typeof window !== 'undefined') {
+      window.addEventListener('keydown', this._onKeyDown);
+      window.addEventListener('keyup', this._onKeyUp);
+    }
     if (!this.surface) return;
     this.surface.addEventListener('pointerdown', this._down);
     this.surface.addEventListener('pointerup', this._up);
     this.surface.addEventListener('pointercancel', this._up);
     this.surface.addEventListener('pointerleave', this._up);
   }
   stop() {
+    if (typeof window !== 'undefined') {
+      window.removeEventListener('keydown', this._onKeyDown);
+      window.removeEventListener('keyup', this._onKeyUp);
+    }
     if (this.surface) {
       this.surface.removeEventListener('pointerdown', this._down);
       this.surface.removeEventListener('pointerup', this._up);
@@ -184,45 +193,43 @@ export class SwipeInput {
   // ArrowDown = front flip, Q = spin-left, E = spin-right. (ArrowLeft/Right + A/D are carve,
   // owned by TiltInput, so they're left alone here.) Each trick key maps to the
   // same gesture angle a real flick would produce.
-  _bindKeys() {
-    if (typeof window === 'undefined') return;
-    // Never steal keys from a text field (the name input): these are
-    // window-level listeners, live from construction, and their preventDefault
-    // would otherwise swallow "s"/space/arrows while typing a name — and fire
-    // phantom brake/flick callbacks (buzz, HUD flashes) on every keystroke.
-    const typing = (e) => {
-      const t = e.target;
-      return !!(t && (t.tagName === 'INPUT' || t.tagName === 'TEXTAREA' || t.isContentEditable));
-    };
-    window.addEventListener('keydown', (e) => {
-      if (typing(e)) return;
-      const k = e.key.toLowerCase();
-      if (k === 's') {
-        // keyboard brake is unambiguous (no flick to disambiguate) — confirm at once
-        if (!this._keyBrake) { this._keyBrake = true; this._braking = true; this._brakeConfirmed = true; this.onBrakeStart(); }
-        e.preventDefault();
-      } else if (k === 'arrowup' || k === ' ') {
-        if (!this._keyUp) { this._keyUp = true; this._fireFlick(Math.PI / 2); }   // back flip (air); nothing on the snow
-        e.preventDefault();
-      } else if (k === 'arrowdown') {
-        if (!this._keyDown) { this._keyDown = true; this._fireFlick(-Math.PI / 2); } // front flip
-        e.preventDefault();
-      } else if (k === 'q') {
-        if (!this._keyLeft) { this._keyLeft = true; this._fireFlick(Math.PI); }   // spin left
-        e.preventDefault();
-      } else if (k === 'e') {
-        if (!this._keyRight) { this._keyRight = true; this._fireFlick(0); }       // spin right
-        e.preventDefault();
-      }
-    });
-    window.addEventListener('keyup', (e) => {
-      if (typing(e)) return;
-      const k = e.key.toLowerCase();
-      if (k === 's') { this._keyBrake = false; this._endBrake(); e.preventDefault(); }
-      else if (k === 'arrowup' || k === ' ') { this._keyUp = false; e.preventDefault(); }
-      else if (k === 'arrowdown') { this._keyDown = false; e.preventDefault(); }
-      else if (k === 'q') { this._keyLeft = false; e.preventDefault(); }
-      else if (k === 'e') { this._keyRight = false; e.preventDefault(); }
-    });
+  // Never steal keys from a text field (the name input): these are
+  // window-level listeners (added in start(), removed in stop() — symmetric
+  // with the pointer listeners), and their preventDefault would otherwise
+  // swallow "s"/space/arrows while typing a name — and fire phantom
+  // brake/flick callbacks (buzz, HUD flashes) on every keystroke.
+  _typing(e) {
+    const t = e.target;
+    return !!(t && (t.tagName === 'INPUT' || t.tagName === 'TEXTAREA' || t.isContentEditable));
+  }
+  _onKeyDown(e) {
+    if (this._typing(e)) return;
+    const k = e.key.toLowerCase();
+    if (k === 's') {
+      // keyboard brake is unambiguous (no flick to disambiguate) — confirm at once
+      if (!this._keyBrake) { this._keyBrake = true; this._braking = true; this._brakeConfirmed = true; this.onBrakeStart(); }
+      e.preventDefault();
+    } else if (k === 'arrowup' || k === ' ') {
+      if (!this._keyUp) { this._keyUp = true; this._fireFlick(Math.PI / 2); }   // back flip (air); nothing on the snow
+      e.preventDefault();
+    } else if (k === 'arrowdown') {
+      if (!this._keyDown) { this._keyDown = true; this._fireFlick(-Math.PI / 2); } // front flip
+      e.preventDefault();
+    } else if (k === 'q') {
+      if (!this._keyLeft) { this._keyLeft = true; this._fireFlick(Math.PI); }   // spin left
+      e.preventDefault();
+    } else if (k === 'e') {
+      if (!this._keyRight) { this._keyRight = true; this._fireFlick(0); }       // spin right
+      e.preventDefault();
+    }
+  }
+  _onKeyUp(e) {
+    if (this._typing(e)) return;
+    const k = e.key.toLowerCase();
+    if (k === 's') { this._keyBrake = false; this._endBrake(); e.preventDefault(); }
+    else if (k === 'arrowup' || k === ' ') { this._keyUp = false; e.preventDefault(); }
+    else if (k === 'arrowdown') { this._keyDown = false; e.preventDefault(); }
+    else if (k === 'q') { this._keyLeft = false; e.preventDefault(); }
+    else if (k === 'e') { this._keyRight = false; e.preventDefault(); }
   }
 }

public/controller/TiltInput.js

@@ -70,7 +70,8 @@ export class TiltInput {
     this._timer = null;
 
     this._onOrient = this._onOrient.bind(this);
-    this._bindKeys();
+    this._onKeyDown = this._onKeyDown.bind(this);
+    this._onKeyUp = this._onKeyUp.bind(this);
     this._initSurface();
   }
 
@@ -111,10 +112,18 @@ export class TiltInput {
 
   start() {
     if (this._timer) return;
+    if (typeof window !== 'undefined') {
+      window.addEventListener('keydown', this._onKeyDown);
+      window.addEventListener('keyup', this._onKeyUp);
+    }
     const interval = 1000 / SEND_HZ;
     this._timer = setInterval(() => this._tick(), interval);
   }
   stop() {
+    if (typeof window !== 'undefined') {
+      window.removeEventListener('keydown', this._onKeyDown);
+      window.removeEventListener('keyup', this._onKeyUp);
+    }
     clearInterval(this._timer); this._timer = null;
   }
 
@@ -156,22 +165,21 @@ export class TiltInput {
   }
 
   // --- keyboard fallback / testing (works over plain HTTP — no sensors) ---
-  _bindKeys() {
-    if (typeof window === 'undefined') return;
-    const set = (e, down) => {
-      // Never steal keys from a text field (the name input) — the
-      // preventDefault below would swallow "a"/"d"/arrows while typing a name.
-      const t = e.target;
-      if (t && (t.tagName === 'INPUT' || t.tagName === 'TEXTAREA' || t.isContentEditable)) return;
-      const k = e.key.toLowerCase();
-      if (k === 'arrowleft' || k === 'a') { this._keyL = down; e.preventDefault(); }
-      else if (k === 'arrowright' || k === 'd') { this._keyR = down; e.preventDefault(); }
-      else return;
-      this._key = (this._keyR ? 1 : 0) - (this._keyL ? 1 : 0);
-    };
-    window.addEventListener('keydown', (e) => set(e, true));
-    window.addEventListener('keyup', (e) => set(e, false));
+  // Listeners are added in start() and removed in stop() (symmetric with the
+  // deviceorientation listener), so re-instantiation never leaks handlers.
+  _setKey(e, down) {
+    // Never steal keys from a text field (the name input) — the
+    // preventDefault below would swallow "a"/"d"/arrows while typing a name.
+    const t = e.target;
+    if (t && (t.tagName === 'INPUT' || t.tagName === 'TEXTAREA' || t.isContentEditable)) return;
+    const k = e.key.toLowerCase();
+    if (k === 'arrowleft' || k === 'a') { this._keyL = down; e.preventDefault(); }
+    else if (k === 'arrowright' || k === 'd') { this._keyR = down; e.preventDefault(); }
+    else return;
+    this._key = (this._keyR ? 1 : 0) - (this._keyL ? 1 : 0);
   }
+  _onKeyDown(e) { this._setKey(e, true); }
+  _onKeyUp(e) { this._setKey(e, false); }
 
   // Carve is via tilt; the control surface just needs to not scroll/zoom under
   // the player's thumb while they ski (the swipe gestures share this surface).

public/controller/ui.js

@@ -1,6 +1,7 @@
 // Small controller-UI helpers shared by the live phone (main.js) and the gallery
 // preview (TestHarness.js) so the two can't drift. No globals, no relay — pure DOM.
 import { buildLevelSeg as buildSeg, paintLevelSeg } from '../shared/levelSeg.js';
+import { runTag } from '../shared/seriesFormat.js';
 
 // Latency chip (bottom-right). halfMs is one-way (RTT/2); halfMs < 0 means the
 // PONG is overdue (no signal). viaFastlane lights the bolt when the reading came
@@ -66,7 +67,7 @@ export function renderResultsBoard(rows, { over, seriesOver, runIndex, runTotal,
 
   // Header tag + champion banner (overall board only).
   const tag = document.getElementById('result-runtag');
-  if (tag) tag.textContent = runTotal ? (seriesOver ? `Final standings · ${runTotal} runs` : `Run ${runIndex} of ${runTotal}`) : '';
+  if (tag) tag.textContent = runTotal ? runTag(runIndex, runTotal, seriesOver) : '';
   const champEl = document.getElementById('result-champ');
   if (champEl) {
     const winners = seriesOver ? rows.filter((r) => r.champion) : [];

public/display/SeriesTally.js

@@ -0,0 +1,117 @@
+// SeriesTally — the cumulative scoring across a SERIES of runs, lifted out of
+// display/main.js so the trickiest run-to-run state lives in one testable unit.
+//
+// A session is `runsTotal` head-to-head runs with points accumulating to an overall
+// champion. This owns: the current run index (1-based; 0 in the lobby), the series
+// length, the over-flag, the per-player banked scores, and the points/folding/row
+// derivation. It does NOT touch the DOM, the net, or the engine — main.js keeps that
+// lifecycle/IO (the intermission timer, broadcasts, rendering). `seriesPoints` is
+// injected (the pure place-→points rule from shared/protocol.js) so this module is
+// dependency-free / THREE-free and the Node tests (tests/seriesTally.test.js) load
+// it directly — the same discipline the SkiEngine follows.
+//
+// Scores are keyed by playerId so a player leaving/reconnecting (and CPU-id reuse)
+// doesn't drop their tally; the LIVE run's points are layered on top of the banked
+// totals by buildRows each render, so the final run is never double-counted (it is
+// folded only at advance time, and the final run isn't folded at all).
+export class SeriesTally {
+  constructor(seriesPoints, runsTotal) {
+    this._seriesPoints = seriesPoints;
+    this.runsTotal = runsTotal;     // series length (host's lobby pick; survives reset)
+    this.runIndex = 0;              // current run, 1-based; 0 in the lobby
+    this.seriesOver = false;        // the final run is in the books → overall board
+    this.scores = new Map();        // playerId -> { name, colorIndex, ai, points } through COMPLETED runs
+  }
+
+  // Wipe the tally for a fresh series (keeps the host's runsTotal pick).
+  reset() {
+    this.scores = new Map();
+    this.runIndex = 0;
+    this.seriesOver = false;
+  }
+
+  // Step into the next run.
+  startNextRun() { this.runIndex += 1; }
+
+  // Mark the just-ended run: the series is over once the final run is in the books.
+  // Returns the new seriesOver so the caller can branch on it.
+  endCurrentRun() {
+    this.seriesOver = this.runIndex >= this.runsTotal;
+    return this.seriesOver;
+  }
+
+  // Host's lobby pick for the series length.
+  setRunsTotal(n) { this.runsTotal = n; }
+
+  // Cross-device reconnect: carry a player's banked points onto their new slot.
+  rekey(oldId, newId) {
+    if (this.scores.has(oldId)) {
+      this.scores.set(newId, this.scores.get(oldId));
+      this.scores.delete(oldId);
+    }
+  }
+
+  // This run's series points for every finisher, keyed by playerId. `res` is the
+  // engine getResults() `results` array (1-based rank, finished flag).
+  _pointsFor(res) {
+    const m = new Map();
+    for (const r of res) m.set(r.playerId, this._seriesPoints(r.rank, res.length, r.finished));
+    return m;
+  }
+
+  // Bank the just-finished run's points into the cumulative tally. Called BEFORE the
+  // run's session is torn down (it reads the live field + results). Not called for the
+  // final run — the overall board derives totals as banked(prior runs) + final-run
+  // points, so the last run is never double-counted.
+  fold(field, resultsObj) {
+    const pts = this._pointsFor((resultsObj && resultsObj.results) || []);
+    for (const p of field) {
+      const cur = this.scores.get(p.peerIndex) || { points: 0 };
+      this.scores.set(p.peerIndex, {
+        name: p.name, colorIndex: p.colorIndex, ai: !!p.ai,
+        points: cur.points + (pts.get(p.peerIndex) || 0),
+      });
+    }
+  }
+
+  // The ordered standings rows for the CURRENT run, each carrying this-run `points`
+  // and the cumulative `score` (banked total through prior runs + this run's points).
+  // Shared by the phone broadcast and the big-screen board so they can't drift. Per
+  // run the rows stay in finish order; once the series is over they're sorted by total
+  // score (the overall ranking), `place` renumbered, and the leader(s) flagged
+  // champion. `resultsObj` is the engine's getResults() output; `field` names/colours
+  // the rows (both passed in so a no-relay TestHarness with its own session drives
+  // this exact path).
+  buildRows(resultsObj, field) {
+    const res = (resultsObj && resultsObj.results) || [];
+    const byId = new Map(field.map((p) => [p.peerIndex, p]));
+    const pts = this._pointsFor(res);
+    const rows = res.map((r) => {
+      const p = byId.get(r.playerId) || {};
+      const point = pts.get(r.playerId) || 0;
+      const prior = (this.scores.get(r.playerId) || { points: 0 }).points;
+      return {
+        playerId: r.playerId, name: p.name || 'Skier',
+        colorIndex: p.colorIndex || 0, ai: !!p.ai,
+        finished: r.finished, time: r.time, dnf: !!r.dnf,
+        place: r.rank, points: point, score: prior + point,
+      };
+    });
+    if (this.seriesOver) {
+      // Overall ranking: most points first, this run's finish order breaks ties.
+      rows.sort((a, b) => b.score - a.score || a.place - b.place);
+      const top = rows.length ? rows[0].score : 0;
+      rows.forEach((row, i) => { row.place = i + 1; row.champion = top > 0 && row.score === top; });
+    }
+    return rows;
+  }
+
+  // Stage series state for a no-relay preview (the gallery 'results' scenario) so the
+  // live render path shows a mid-series or final board. Never used in live play.
+  stagePreview({ index = 1, total = this.runsTotal, over = false, scores = null } = {}) {
+    this.runIndex = index;
+    this.runsTotal = total;
+    this.seriesOver = over;
+    if (scores) this.scores = new Map(Object.entries(scores).map(([id, points]) => [id, { points }]));
+  }
+}

public/display/SkiTrails.js

@@ -93,8 +93,12 @@ export class SkiTrails {
     t.lastAir = false;
     t.count++;
 
-    t.posAttr.needsUpdate = true;
-    t.nrmAttr.needsUpdate = true;
+    // Only this one slot (4 verts = VPP*3 = 12 array elements at offset vb) changed,
+    // so scope the GPU upload to that range instead of re-uploading the whole buffer.
+    // start/count are in array-INDEX units; three's WebGLAttributes auto-clears
+    // updateRanges after each upload, so we re-add the range every recorded point.
+    t.posAttr.addUpdateRange(vb, VPP * 3); t.posAttr.needsUpdate = true;
+    t.nrmAttr.addUpdateRange(vb, VPP * 3); t.nrmAttr.needsUpdate = true;
     this._rebuildIndex(t);
   }
 
@@ -122,6 +126,8 @@ export class SkiTrails {
       idx[n++] = a + 2; idx[n++] = b + 3; idx[n++] = b + 2;
     }
     t.geom.setDrawRange(0, n);
+    // Index is deliberately uploaded in full (no updateRange): the ring-buffer wrap
+    // seam makes incremental index updates unsafe to do hastily — separate concern.
     t.idxAttr.needsUpdate = true;
   }
 

public/display/SlopeBuilder.js

@@ -6,7 +6,7 @@
 // Returns { centerline, length, slopeWidth, groundY, ramps, obstacles, def }.
 import * as THREE from 'three';
 import { Centerline } from './Centerline.js';
-import { generateSlope } from '../shared/slopes.js';
+import { generateSlope, obstacleRadius } from '../shared/slopes.js';
 
 const DEG = Math.PI / 180;
 const STEP = 2.0; // arclength between centerline samples (Catmull-Rom smooths between)
@@ -107,10 +107,10 @@ export function buildSlope(def) {
   const groundY = minY - 0.4;
 
   const ramps = (def.ramps || []).map((r) => ({
-    s: r.at * length, lat: r.lat || 0, radius: r.radius || 1.5, width: r.width || 2.4,
+    s: r.at * length, lat: r.lat || 0, width: r.width || 2.4,
   }));
   const obstacles = (def.obstacles || []).map((o) => ({
-    s: o.at * length, lat: o.lat || 0, radius: o.radius || (o.kind === 'rock' ? 0.85 : 0.7), kind: o.kind || 'tree',
+    s: o.at * length, lat: o.lat || 0, radius: o.radius || obstacleRadius(o.kind), kind: o.kind || 'tree',
   }));
   // The finish-gate posts (drawn by the renderer's FinishGate) are solid obstacles
   // too: clipping one at the line wipes you out like a tree/rock. At the OUTER piste

public/display/SlopeScenery.js

@@ -5,7 +5,7 @@
 // no renderer state. SceneRenderer owns cameras/lighting/skiers and calls these
 // from setTrack.
 import * as THREE from 'three';
-import { mulberry32 } from '../shared/slopes.js';
+import { mulberry32, obstacleRadius } from '../shared/slopes.js';
 import { hitSL, SKI_HALF } from './engine/SkiEngine.js';
 
 const _up = new THREE.Vector3(0, 1, 0);
@@ -468,7 +468,7 @@ export function addObstacle(group, cl, o, hitboxDebug) {
   g.quaternion.setFromUnitVectors(_up, up);
   group.add(g);
   if (hitboxDebug) {
-    group.add(debugCircle(f, o.lat || 0, o.radius || (o.kind === 'rock' ? 0.7 : 0.8), 0xff2244));
+    group.add(debugCircle(f, o.lat || 0, o.radius || obstacleRadius(o.kind), 0xff2244));
   }
 }