Fix video-seek, leak, and correctness issues found in codebase review

CLAUDE.md

@@ -34,6 +34,8 @@ Physics for educators: `doc/model.md`. Architecture: `doc/implementation-notes.m
 
 - `ModelViewTransformFactory.buildModelViewTransform()` maps real-world units to video pixels from coordinate-system position/rotation and calibration endpoints. Moving axes or calibration **re-expresses** digitized track points so they stay pinned on the video.
 - Velocity and acceleration are **finite-difference estimates** from position series — noise amplifies on differentiation.
+- **Time sync is one-way and centralized.** `VideoPlayerNode` links `currentTimeProperty` → `videoElement.currentTime` (guarded by the `scrubbing` flag and a half-frame deadband). Writers of `currentTimeProperty` should *not* seek the element themselves; the video follows the model automatically.
+- **`TrackPoint.time` is authoritative, `frame` is derived.** Changing the frame rate renumbers `currentFrameProperty`, so `TrackingModel.retimeTrackPoints()` re-derives every stored point's index from its timestamp (wired in `TrackLabModel`). Lowering the rate can collide two points onto one index; first wins.
 - Up to 4 concurrent tracks (labels A–Z available; symbols are not reused) with independent colors for graphs and table rows.
 
 ## Accessibility
@@ -67,7 +69,12 @@ Actual specs:
 
 - `tests/track-lab/model/KinematicsComputer.test.ts`
 - `tests/track-lab/model/TrackingModel.test.ts`
-- `tests/memory-leak.test.ts`
+- `tests/track-lab/model/TrackRetiming.test.ts` — frame indices re-derived from timestamps on frame-rate change
+- `tests/track-lab/model/ModelViewTransformFactory.test.ts` — includes the retransform round-trip (points stay pinned to the same video pixel)
+- `tests/track-lab/model/TrackExporter.test.ts`
+- `tests/track-lab/model/VideoPlaybackModel.test.ts`
+- `tests/track-lab/graph/GraphDataManager.test.ts` — tick spacing stays on the 1-2-5 sequence
+- `tests/memory-leak.test.ts` — WeakRef dispose regression, plus listener-count regressions for `PlaybackControlsNode` (an HSlider that is rebuilt but never inserted must still be disposed, or it retains listeners on `currentTimeProperty`)
 
 Run `npm test`. CI runs the suite when a `test` script is present.
 
@@ -89,5 +96,5 @@ npm run generate-svg-icon   # bouncing-ball icon SVG
 Extra `src/` root files beyond the standard set, each justified by cross-screen use:
 `TrackLabIcons.ts`, `TrackLabButton.ts`, `webcam.ts`, and the `src/tracking/` folder (OpenCV Web Worker).
 
-- **OpenCV WASM** requires COOP/COEP headers (configured in Vite dev + production). Sample videos in `public/videos/`.
+- **OpenCV WASM** requires COOP/COEP headers (configured in Vite dev + production). Sample videos live in top-level `videos/`, served at `/videos/` by the `serveVideos()` Vite plugin (Range-request support for seeking) and copied to `dist/videos/` on build — they are not under `public/`.
 - **Wall-clock timers (allowed exception)** — webcam/video code uses raw `setTimeout`/`setInterval` rather than `stepTimer`: camera-init timeout in `webcam.ts`, FPS-sampling interval in `WebcamPanel.ts`, source-switch debounce in `VideoSourceControlNode.ts`. These track real hardware time, independent of the sim clock.

README.md

@@ -10,7 +10,7 @@ export.
 - Draggable coordinate system and calibration tool for real-world units
 - OpenCV auto-tracking and manual frame-by-frame digitizing with magnifier
 - Kinematics graph, data table, measuring tape, and angle tool
-- English and French UI, projector color profile, and installable PWA
+- English, French, and Spanish UI, projector color profile, and installable PWA
 - OpenCV.js WASM tracking in a Web Worker (requires COOP/COEP headers)
 
 ## Quick Start

public/opencv-worker.js

@@ -112,7 +112,15 @@ self.onmessage = async (event) => {
 
         if (templateMat) templateMat.delete();
         const roi = new cv.Rect(clampedX, clampedY, roiW, roiH);
-        templateMat = blurred.roi(roi).clone();
+        // roi() returns a new Mat header sharing `blurred`'s buffer; it must be
+        // deleted explicitly.  Only the clone owns memory that outlives this
+        // message, and WASM heap allocations are never reclaimed by the JS GC.
+        const roiMat = blurred.roi(roi);
+        try {
+          templateMat = roiMat.clone();
+        } finally {
+          roiMat.delete();
+        }
 
         self.postMessage({
           id,
@@ -129,7 +137,10 @@ self.onmessage = async (event) => {
       }
     } else if (type === 'track') {
       if (!cv || !templateMat) {
-        self.postMessage({ id, type: 'track-result', x: null, y: null, confidence: 0 });
+        // confidence 0 is below any threshold, so the main thread discards this
+        // result.  x/y are still numbers so the payload matches the declared
+        // WorkerResponse shape in OpenCVTracker.ts.
+        self.postMessage({ id, type: 'track-result', x: 0, y: 0, confidence: 0 });
         return;
       }
 

src/i18n/StringManager.ts

@@ -36,11 +36,18 @@ function applyStringTest<T>(obj: T): T {
 }
 
 // ── Compile-time key-parity check ─────────────────────────────────────────────
-// satisfies errors immediately if either locale file is missing keys from the other.
+// satisfies errors immediately if any locale file is missing keys from another.
+// Every shipped locale is checked against English in both directions; omitting
+// one lets it drift silently, which matters because the language picker
+// (supportsDynamicLocale) exposes all of them at runtime.
 // biome-ignore lint/complexity/noVoid: intentional compile-time type assertion
 void (stringsEn satisfies typeof stringsFr);
 // biome-ignore lint/complexity/noVoid: intentional compile-time type assertion
 void (stringsFr satisfies typeof stringsEn);
+// biome-ignore lint/complexity/noVoid: intentional compile-time type assertion
+void (stringsEn satisfies typeof stringsEs);
+// biome-ignore lint/complexity/noVoid: intentional compile-time type assertion
+void (stringsEs satisfies typeof stringsEn);
 
 // ── Build the reactive string property tree ───────────────────────────────────
 const stringProperties = LocalizedString.getNestedStringProperties({

src/track-lab/graph/GraphDataManager.ts

@@ -20,6 +20,9 @@ export interface GridVisualizationConfig {
 }
 
 export default class GraphDataManager {
+  /** Upper bound on ticks per axis; spacing escalates until the count fits. */
+  private static readonly MAX_TICKS = 20;
+
   // ── Circular buffer ──────────────────────────────────────────────────────
   // Fixed-size ring buffer so that oldest-point eviction is O(1) rather than
   // O(n) (Array.shift reindexes every remaining element after removal).
@@ -316,7 +319,17 @@ export default class GraphDataManager {
       spacing = 10 * magnitude;
     }
 
-    return Math.max(spacing, rangeLength / 20);
+    // Cap the tick count by stepping up through the 1-2-5 sequence rather than
+    // clamping to rangeLength / MAX_TICKS, which would hand back an arbitrary
+    // non-round spacing (visible at targetTicks = 15, where the nice value can
+    // fall just under the cap).
+    while (rangeLength / spacing > GraphDataManager.MAX_TICKS) {
+      const decade = 10 ** Math.floor(Math.log10(spacing));
+      const step = Math.round(spacing / decade);
+      spacing = step < 2 ? 2 * decade : step < 5 ? 5 * decade : 10 * decade;
+    }
+
+    return spacing;
   }
 
   /**

src/track-lab/model/TrackLabModel.ts

@@ -45,6 +45,14 @@ export class TrackLabModel {
     this.overlayTools.modelViewTransformProperty.lazyLink((newMvt, oldMvt) => {
       this.tracking.retransformTrackPoints(oldMvt, newMvt);
     });
+
+    // currentFrameProperty is derived as round(time × fps), so changing the
+    // frame rate renumbers the *current* frame.  Already-digitized points store
+    // a frame index too, so they must be renumbered from their timestamps in
+    // the same step or they stop lining up with the frame the UI is on.
+    this.playback.frameRateProperty.lazyLink((frameRate) => {
+      this.tracking.retimeTrackPoints(frameRate);
+    });
   }
 
   /**

src/track-lab/model/TrackingModel.ts

@@ -287,6 +287,59 @@ export class TrackingModel {
     }));
   }
 
+  /**
+   * Re-derive every stored point's frame index from its recorded timestamp
+   * using `frameRate`.  Called by TrackLabModel when the frame rate changes.
+   *
+   * `time` is the authoritative quantity — it is captured from the video
+   * element — while `frame` is a derived index that must stay consistent with
+   * `VideoPlaybackModel.currentFrameProperty`, which recomputes as
+   * `round(time × fps)`.  Without this, changing the frame rate after
+   * digitizing leaves stored points labelled with indices from the old rate,
+   * and everything keyed on `frame` (the erase target, the current-frame ring,
+   * the table row highlight) silently addresses the wrong point.
+   *
+   * Lowering the frame rate can map two points onto the same index.  Frames are
+   * unique keys within a track, so a collision keeps the earlier point,
+   * matching addPointToTrack()'s first-wins policy.
+   */
+  public retimeTrackPoints(frameRate: number): void {
+    if (!(frameRate > 0)) {
+      return;
+    }
+    const tracks = this.tracksProperty.value;
+    if (tracks.length === 0) {
+      return;
+    }
+
+    this.tracksProperty.value = tracks.map((track) => {
+      const seen = new Set<number>();
+      const points: TrackPoint[] = [];
+      // Points are already sorted by ascending frame, and frame is monotonic in
+      // time, so this walks the track in time order — the first point to claim
+      // an index is the earliest one.
+      for (const pt of track.points) {
+        const frame = Math.round(pt.time * frameRate);
+        if (seen.has(frame)) {
+          continue;
+        }
+        seen.add(frame);
+        points.push({ ...pt, frame });
+      }
+      return { ...track, points };
+    });
+
+    // The stash holds a point carrying an index from the previous frame rate.
+    // Restoring it would place it at the wrong frame, so re-derive it too.
+    const deleted = this.lastDeletedPointProperty.value;
+    if (deleted) {
+      this.lastDeletedPointProperty.value = {
+        trackId: deleted.trackId,
+        point: { ...deleted.point, frame: Math.round(deleted.point.time * frameRate) },
+      };
+    }
+  }
+
   // ── Tracker facade ──────────────────────────────────────────────────────
   // Views interact with the tracker exclusively through these methods so that
   // the tracker implementation stays encapsulated inside the model layer.

src/track-lab/view/DataTableNode.ts

@@ -146,8 +146,17 @@ export class DataTableNode extends Node {
           const link = document.createElement("a");
           link.href = url;
           link.download = `export${this.exportCounter}.csv`;
+          // The anchor must be in the document for the synthetic click to start
+          // a download in Firefox, and the object URL must outlive the click:
+          // revoking it synchronously cancels the download in some browsers.
+          // Wall-clock setTimeout is deliberate here — this is browser download
+          // plumbing, not simulation time, so it must not follow the sim clock.
+          document.body.appendChild(link);
           link.click();
-          URL.revokeObjectURL(url);
+          setTimeout(() => {
+            link.remove();
+            URL.revokeObjectURL(url);
+          }, 0);
           this.exportCounter++;
         },
       },
@@ -196,16 +205,6 @@ export class DataTableNode extends Node {
       );
     };
 
-    const runRebuild = () => {
-      tableRenderer.rebuild(
-        tracking.tracksProperty.value,
-        unitProperty.value,
-        getTableColors(),
-        getLabels(),
-        getA11yLabels(),
-      );
-    };
-
     const tracksListener = () => runUpdate();
     tracking.tracksProperty.link(tracksListener);
 
@@ -215,13 +214,37 @@ export class DataTableNode extends Node {
     const currentFrameListener = (frame: number) => tableRenderer.setCurrentFrame(frame);
     playback.currentFrameProperty.link(currentFrameListener);
 
-    // Color-theme and locale changes require a full rebuild because cell colours
-    // and label strings are baked into the DOM.
-    const tableHeaderBgListener = () => runRebuild();
-    TrackLabColors.tableHeaderBackgroundProperty.lazyLink(tableHeaderBgListener);
-
-    const frameStringListener = () => runRebuild();
-    dataTableStrings.frameStringProperty.lazyLink(frameStringListener);
+    // Colour-theme and locale changes are baked into the DOM as CSS strings and
+    // text nodes, so they need a rebuild.  Every contributing property is
+    // observed rather than one representative of each group: switching colour
+    // profile or locale updates these properties one at a time, so a single
+    // representative can fire while its siblings still hold their old values.
+    // TableRenderer.update() compares the full colour/label snapshot and
+    // rebuilds when it differs, so the last notification wins with final values.
+    const themeProperties = [
+      TrackLabColors.tableHeaderBackgroundProperty,
+      TrackLabColors.tableHeaderTextProperty,
+      TrackLabColors.tableRowOddProperty,
+      TrackLabColors.tableRowEvenProperty,
+      TrackLabColors.tableGridStrokeProperty,
+      TrackLabColors.tableEmptyTextProperty,
+      TrackLabColors.tableSymbolShadowProperty,
+      TrackLabColors.tableBackgroundProperty,
+      TrackLabColors.tableCurrentRowProperty,
+    ];
+    const localeProperties = [
+      dataTableStrings.frameStringProperty,
+      dataTableStrings.timeSecondsStringProperty,
+      dataTableStrings.noDataStringProperty,
+      a11yStrings.dataTableStringProperty,
+    ];
+    const themeOrLocaleListener = () => runUpdate();
+    for (const property of themeProperties) {
+      property.lazyLink(themeOrLocaleListener);
+    }
+    for (const property of localeProperties) {
+      property.lazyLink(themeOrLocaleListener);
+    }
 
     const videoLoadedListener = (loaded: boolean) => {
       this.visible = loaded;
@@ -420,8 +443,12 @@ export class DataTableNode extends Node {
       tracking.tracksProperty.unlink(tracksListener);
       unitProperty.unlink(unitListener);
       playback.currentFrameProperty.unlink(currentFrameListener);
-      TrackLabColors.tableHeaderBackgroundProperty.unlink(tableHeaderBgListener);
-      dataTableStrings.frameStringProperty.unlink(frameStringListener);
+      for (const property of themeProperties) {
+        property.unlink(themeOrLocaleListener);
+      }
+      for (const property of localeProperties) {
+        property.unlink(themeOrLocaleListener);
+      }
       videoLoadedProperty.unlink(videoLoadedListener);
       exportButton.dispose();
       panel.localBoundsProperty.unlink(localBoundsListener);

src/track-lab/view/KinematicsGraphNode.ts

@@ -6,7 +6,7 @@
  * The velocity and acceleration groups can be hidden via user preferences.
  */
 
-import { Property, type TReadOnlyProperty } from "scenerystack/axon";
+import { DerivedProperty, Property, type TReadOnlyProperty } from "scenerystack/axon";
 import { Node, Text, VBox } from "scenerystack/scenery";
 import { PhetFont } from "scenerystack/scenery-phet";
 import { Checkbox } from "scenerystack/sun";
@@ -35,7 +35,12 @@ export class KinematicsGraphNode extends Node {
   private readonly tracking: TrackingModel;
   private readonly selectedTracksProperty: Property<Set<string>>;
   private readonly trackCheckboxPanel: Node;
-  private readonly trackCheckboxes: Map<string, { checkbox: Checkbox; property: Property<boolean> }> = new Map();
+  private readonly trackCheckboxes: Map<
+    string,
+    { checkbox: Checkbox; property: Property<boolean>; nameProperty: TReadOnlyProperty<string> }
+  > = new Map();
+  /** VBox wrapping the current checkboxes; disposed and replaced on each rebuild. */
+  private checkboxContainer: VBox | null = null;
   private readonly disposeKinematicsGraph: () => void;
 
   public constructor(
@@ -108,9 +113,10 @@ export class KinematicsGraphNode extends Node {
     // Update checkboxes when tracks change (link fires immediately, building initial checkboxes)
     const tracksListener = (tracks: readonly Track[]) => {
       // Dispose old checkboxes
-      for (const [, { checkbox, property }] of this.trackCheckboxes) {
+      for (const [, { checkbox, property, nameProperty }] of this.trackCheckboxes) {
         checkbox.dispose();
         property.dispose();
+        nameProperty.dispose();
       }
       this.trackCheckboxes.clear();
 
@@ -191,11 +197,14 @@ export class KinematicsGraphNode extends Node {
       preferencesModel.showAccelerationInGraphProperty.unlink(accelerationPrefListener);
       videoLoadedProperty.unlink(videoLoadedListener);
       this.graph.localBoundsProperty.unlink(graphBoundsListener);
-      for (const [, { checkbox, property }] of this.trackCheckboxes) {
+      for (const [, { checkbox, property, nameProperty }] of this.trackCheckboxes) {
         checkbox.dispose();
         property.dispose();
+        nameProperty.dispose();
       }
       this.trackCheckboxes.clear();
+      this.checkboxContainer?.dispose();
+      this.checkboxContainer = null;
       this.selectedTracksProperty.dispose();
       this.graph.dispose();
     };
@@ -213,8 +222,13 @@ export class KinematicsGraphNode extends Node {
   private rebuildTrackCheckboxes(): void {
     const tracks = this.tracking.tracksProperty.value;
 
+    // The previous container is detached here and disposed either way; leaving
+    // it to be garbage collected would leak a VBox on every track change.
+    this.trackCheckboxPanel.children = [];
+    this.checkboxContainer?.dispose();
+    this.checkboxContainer = null;
+
     if (tracks.length === 0) {
-      this.trackCheckboxPanel.children = [];
       return;
     }
 
@@ -241,18 +255,23 @@ export class KinematicsGraphNode extends Node {
         fill: getTrackColor(track.colorIndex),
       });
 
-      // Create checkbox with accessibility label
+      // Create checkbox with accessibility label.  Derived rather than a
+      // `.value` snapshot so the name re-translates when the user switches
+      // language (the sim enables supportsDynamicLocale).
       const kinematicsGraphStrings = StringManager.getInstance().getKinematicsGraph();
+      const nameProperty = new DerivedProperty([kinematicsGraphStrings.trackItemStringProperty], (pattern) =>
+        pattern.split("{{symbol}}").join(track.symbol),
+      );
       const checkbox = new Checkbox(checkboxProperty, label, {
         checkboxColor: TrackLabColors.checkboxColorProperty,
         checkboxColorBackground: TrackLabColors.checkboxColorBackgroundProperty,
         spacing: 4,
-        accessibleName: kinematicsGraphStrings.trackItemStringProperty.value.split("{{symbol}}").join(track.symbol),
+        accessibleName: nameProperty,
       });
       checkbox.addInputListener({ down: () => checkbox.focus() });
 
       // Store for later disposal
-      this.trackCheckboxes.set(track.id, { checkbox, property: checkboxProperty });
+      this.trackCheckboxes.set(track.id, { checkbox, property: checkboxProperty, nameProperty });
 
       checkboxNodes.push(checkbox);
     }
@@ -264,6 +283,7 @@ export class KinematicsGraphNode extends Node {
       align: "left",
     });
 
+    this.checkboxContainer = checkboxContainer;
     this.trackCheckboxPanel.children = [checkboxContainer];
     this.updateCheckboxPositions();
   }