Claude/refactor mvc violations r887u

src/screen-name/model/TrackingModel.ts

@@ -78,6 +78,11 @@ export class TrackingModel {
   // ── OpenCV Tracker (computational service) ────────────────────────────
   private readonly tracker = new OpenCVTracker(VIDEO_WIDTH, VIDEO_HEIGHT);
 
+  // Monotonically-increasing counter used to detect stale async initTracker
+  // results.  resetTracker() increments it; initTracker() captures the value
+  // before awaiting and returns false (stale) if the counter changed.
+  private initVersion = 0;
+
   // ── Track mutation methods ────────────────────────────────────────────
 
   /**
@@ -117,18 +122,18 @@ export class TrackingModel {
 
   /**
    * Record a digitized position for `frame` on the track identified by `id`.
+   * If a point for `frame` already exists on the track, the call is a no-op
+   * (deduplication policy: first recorded position wins).
    */
   public addPointToTrack(id: string, frame: number, time: number, x: number, y: number): void {
     const tracks = this.tracksProperty.value.map((track) => {
       if (track.id !== id) {
         return track;
       }
 
-      const existingIndex = track.points.findIndex((p) => p.frame === frame);
-      if (existingIndex !== -1) {
-        const updatedPoints = [...track.points];
-        updatedPoints[existingIndex] = { frame, time, x, y };
-        return { ...track, points: updatedPoints };
+      // Skip if this frame is already recorded on the track.
+      if (track.points.some((p) => p.frame === frame)) {
+        return track;
       }
 
       const point: TrackPoint = { frame, time, x, y };
@@ -183,6 +188,7 @@ export class TrackingModel {
 
   /** Reset tracking state. Cancels any in-flight operation and clears the template. */
   public resetTracker(): void {
+    this.initVersion++;
     this.tracker.dispose();
   }
 
@@ -196,10 +202,28 @@ export class TrackingModel {
 
   /**
    * Capture the tracking template from the current video frame within `region`.
-   * Resolves when the worker has processed the template and is ready to track.
+   * Returns true when the worker is ready to track, or false if this call was
+   * superseded by a newer initTracker call (stale — the view should discard
+   * the result silently).  Throws only for genuine errors (CORS, worker crash).
    */
-  public async initTracker(video: HTMLVideoElement, region: TrackerRegion): Promise<void> {
-    await this.tracker.initFromVideo(video, region);
+  public async initTracker(video: HTMLVideoElement, region: TrackerRegion): Promise<boolean> {
+    const captured = ++this.initVersion;
+    try {
+      await this.tracker.initFromVideo(video, region);
+    } catch (err) {
+      // If the tracker was reset mid-flight (initVersion changed), this error
+      // is a deliberate cancellation, not a real failure.
+      if (this.initVersion !== captured) {
+        return false;
+      }
+      throw err;
+    }
+    if (this.initVersion !== captured) {
+      // A newer drag started while the worker was initialising; discard.
+      this.tracker.dispose();
+      return false;
+    }
+    return true;
   }
 
   /**
@@ -215,6 +239,7 @@ export class TrackingModel {
     this.tracksProperty.value = [];
     this.activeTrackIdProperty.value = null;
     this.nextSymbolCode = TRACK_SYMBOL_FIRST_CODE;
+    this.initVersion = 0;
     this.tracker.dispose();
   }
 }

src/screen-name/model/VideoPlaybackModel.ts

@@ -56,6 +56,15 @@ export class VideoPlaybackModel {
     (fps) => 1 / fps,
   );
 
+  // Derived current frame index: Math.round(currentTime * frameRate)
+  // Multiply by frame rate directly rather than dividing by frameDuration
+  // (1/fps) to avoid cascading floating-point error at non-integer fps values
+  // like 29.97, which could cause adjacent timestamps to map to the same frame.
+  public readonly currentFrameProperty: TReadOnlyProperty<number> = new DerivedProperty(
+    [this.currentTimeProperty, this.frameRateProperty],
+    (time, fps) => Math.round(time * fps),
+  );
+
   // ── Actual display dimensions of the loaded video ─────────────────────
   // Updated in VideoPlayerNode once loadedmetadata fires.  Starts at the
   // max dimensions; overlays and the OpenCV canvas react to changes.
@@ -90,6 +99,35 @@ export class VideoPlaybackModel {
     range: new Range(0.5, 1.5),
   });
 
+  /**
+   * Convert a continuous time value to a discrete frame index.
+   * Multiply by frame rate directly rather than dividing by frameDuration
+   * (1/fps) to avoid cascading floating-point error at non-integer fps values
+   * like 29.97.
+   */
+  public timeToFrame(time: number): number {
+    return Math.round(time * this.frameRateProperty.value);
+  }
+
+  /**
+   * Pause playback and advance or retreat by exactly one frame in the given
+   * direction (+1 = forward, -1 = backward). Updates currentTimeProperty;
+   * the view is responsible for syncing videoElement.currentTime afterward.
+   */
+  public seekByFrames(direction: number): void {
+    this.isPlayingProperty.value = false;
+    const duration = this.durationProperty.value;
+    if (!(duration > 0)) {
+      return;
+    }
+    const raw = this.currentTimeProperty.value + direction * this.frameDurationProperty.value;
+    const clamped = Math.max(0, Math.min(raw, duration));
+    if (!Number.isFinite(clamped)) {
+      return;
+    }
+    this.currentTimeProperty.value = clamped;
+  }
+
   public reset(): void {
     this.isPlayingProperty.reset();
     this.currentTimeProperty.reset();

src/screen-name/view/AutoTrackerNode.ts

@@ -21,7 +21,8 @@ import type { TrackingModel } from "../model/TrackingModel.js";
 type AutoTrackerNodeOptions = {
   tracking: TrackingModel;
   videoDimensionsProperty: TReadOnlyProperty<Dimension2>;
-  frameRateProperty: TReadOnlyProperty<number>;
+  /** Converts a continuous time value to a discrete frame index. */
+  timeToFrame: (time: number) => number;
   modelViewTransformProperty: TReadOnlyProperty<Transform3>;
 };
 
@@ -63,8 +64,6 @@ export class AutoTrackerNode extends Node {
   private readonly trailBuf: Array<{ x: number; y: number }> = new Array(MAX_TRAIL);
   private trailHead = 0; // index of the slot where the NEXT write will land
   private trailSize = 0; // number of valid entries (0 … MAX_TRAIL)
-  /** Frames already recorded to the active track; cleared on track change or reset. */
-  private readonly recordedFrames = new Set<number>();
 
   private readonly hintText: Text;
   private readonly errorText: Text;
@@ -79,11 +78,6 @@ export class AutoTrackerNode extends Node {
   private pendingFrameId = 0;
   /** True while an async track() call is in flight; prevents concurrent tracking calls. */
   private trackInProgress = false;
-  // Monotonically increasing counter — each new initFromVideo call captures the
-  // current value and only applies results if the counter hasn't changed by the
-  // time the async initialisation completes, preventing stale results from a
-  // previous drag from overwriting a more recent one.
-  private initVersion = 0;
 
   private readonly disposeAutoTrackerNode: () => void;
 
@@ -100,7 +94,7 @@ export class AutoTrackerNode extends Node {
   ) {
     super({ visible: false });
 
-    const { tracking, videoDimensionsProperty, frameRateProperty, modelViewTransformProperty } = options;
+    const { tracking, videoDimensionsProperty, timeToFrame, modelViewTransformProperty } = options;
     this.tracking = tracking;
 
     const autoTrackerStrings = StringManager.getInstance().getAutoTracker();
@@ -179,8 +173,8 @@ export class AutoTrackerNode extends Node {
       start: (event) => {
         this.trailHead = 0;
         this.trailSize = 0;
-        // Bump version so any in-flight initTracker call is discarded when it resolves.
-        this.initVersion++;
+        // resetTracker() increments the model's version counter so any in-flight
+        // initTracker call will be discarded when it resolves.
         this.tracking.resetTracker();
         this.setCrosshairVisible(false);
         this.trailPath.shape = null;
@@ -231,16 +225,14 @@ export class AutoTrackerNode extends Node {
           }
 
           // initTracker is async (loads WASM on first call); tracking begins
-          // automatically once isTrackerReady becomes true.
-          // Capture the current version so stale results from a previous drag
-          // (still awaiting WASM load) are discarded if a new drag has started.
-          const capturedVersion = this.initVersion;
+          // automatically once isTrackerReady becomes true.  Staleness detection
+          // (new drag starting before this one resolves) is handled inside the
+          // model: initTracker() returns false when superseded.
           this.tracking
             .initTracker(videoElement, region)
-            .then(() => {
-              if (this.initVersion !== capturedVersion) {
-                // A newer drag has already started; discard this result.
-                this.tracking.resetTracker();
+            .then((ready) => {
+              if (!ready) {
+                // Superseded by a newer drag — silently discard.
                 return;
               }
               // Guard against the race condition where the user removes the
@@ -256,11 +248,6 @@ export class AutoTrackerNode extends Node {
               }
             })
             .catch((err: unknown) => {
-              // A version mismatch means the operation was cancelled (reset or new drag
-              // started) – not a real failure.  Silently discard.
-              if (this.initVersion !== capturedVersion) {
-                return;
-              }
               // biome-ignore lint/suspicious/noConsole: error logging for tracker init failure
               console.error("AutoTracker: failed to initialise OpenCV tracker:", err);
               const message =
@@ -322,20 +309,13 @@ export class AutoTrackerNode extends Node {
       const activeId = tracking.activeTrackIdProperty.value;
       if (activeId) {
         const time = videoElement.currentTime;
-        // Multiply by frame rate directly rather than dividing by frameDuration
-        // (1/fps) to avoid cascading floating-point error at non-integer fps values
-        // like 29.97, which could cause two adjacent timestamps to map to the same
-        // frame or skip a frame entirely.
-        const frame = Math.round(time * frameRateProperty.value);
-
-        // O(1) duplicate-frame check via Set (vs O(n) linear scan).
-        if (!this.recordedFrames.has(frame)) {
-          // Convert video-local pixel coords directly to model coords.
-          // The MVT operates in video-local space, matching these coordinates.
-          const modelPt = modelViewTransformProperty.value.inversePosition2(new Vector2(pt.x, pt.y));
-          tracking.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y);
-          this.recordedFrames.add(frame);
-        }
+        const frame = timeToFrame(time);
+        // Convert video-local pixel coords directly to model coords.
+        // The MVT operates in video-local space, matching these coordinates.
+        // Deduplication (skip if frame already recorded) is enforced inside
+        // TrackingModel.addPointToTrack().
+        const modelPt = modelViewTransformProperty.value.inversePosition2(new Vector2(pt.x, pt.y));
+        tracking.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y);
       }
     };
 
@@ -347,12 +327,6 @@ export class AutoTrackerNode extends Node {
     videoElement.addEventListener("timeupdate", onFrame);
     videoElement.addEventListener("seeked", onFrame);
 
-    // Clear the recorded-frames set whenever the user switches to a different
-    // track so frames from the previous track don't suppress recording on the
-    // new one.
-    const clearRecordedFrames = () => this.recordedFrames.clear();
-    tracking.activeTrackIdProperty.lazyLink(clearRecordedFrames);
-
     // ── Show/hide based on combined "video loaded && autoTracking" ────────
     const autoTrackingShownListener = (shown: boolean) => {
       if (!shown) {
@@ -370,7 +344,6 @@ export class AutoTrackerNode extends Node {
       videoElement.removeEventListener("timeupdate", onFrame);
       videoElement.removeEventListener("seeked", onFrame);
       this.cancelPendingFrame();
-      tracking.activeTrackIdProperty.unlink(clearRecordedFrames);
       autoTrackingShownProperty.unlink(autoTrackingShownListener);
       videoDimensionsProperty.unlink(videoDimensionsListener);
       this.tracking.resetTracker();
@@ -416,13 +389,11 @@ export class AutoTrackerNode extends Node {
   public reset(): void {
     this.cancelPendingFrame();
     this.trackInProgress = false;
-    // Bump version before disposing so any in-flight initTracker rejection is
-    // treated as a cancellation rather than a real error (mirrors DragListener.start).
-    this.initVersion++;
+    // resetTracker() increments the model's version counter so any in-flight
+    // initTracker rejection is treated as a cancellation rather than a real error.
     this.tracking.resetTracker();
     this.trailHead = 0;
     this.trailSize = 0;
-    this.recordedFrames.clear();
     this.selecting = false;
     this.selectionRect.visible = false;
     this.trailPath.shape = null;

src/screen-name/view/DigitizingOverlayNode.ts

@@ -309,8 +309,7 @@ export class DigitizingOverlayNode extends Node {
     const trackPaths = new Map<string, Path>(); // track id → Path
 
     const rebuildMarks = () => {
-      const frameDuration = playback.frameDurationProperty.value;
-      const currentFrame = Math.round(playback.currentTimeProperty.value / frameDuration);
+      const currentFrame = playback.currentFrameProperty.value;
       const mvt = modelViewTransformProperty.value;
       const tracks = tracking.tracksProperty.value;
       const activeTrackIds = new Set(tracks.map((t) => t.id));

src/screen-name/view/PlaybackControlsNode.ts

@@ -232,18 +232,15 @@ export class PlaybackControlsNode extends HBox {
         playback.frameRateProperty,
         playback.totalFrameCountProperty,
       ],
-      (time: number, duration: number, frameRate: number, totalFrameCount: number) => {
+      (time: number, duration: number, _frameRate: number, totalFrameCount: number) => {
         if (duration <= 0) {
           return "0/0";
         }
-        // Multiply by frame rate directly rather than dividing by frameDuration
-        // (1/fps) to avoid cascading floating-point error at non-integer fps
-        // values like 29.97, matching the approach used in AutoTrackerNode.
-        const current = Math.round(time * frameRate);
+        const current = playback.timeToFrame(time);
         if (!Number.isFinite(duration)) {
           return `${current}/?`;
         }
-        const total = totalFrameCount > 0 ? totalFrameCount : Math.round(duration * frameRate);
+        const total = totalFrameCount > 0 ? totalFrameCount : playback.timeToFrame(duration);
         return `${current}/${total}`;
       },
     );

src/screen-name/view/VideoPlayerNode.ts

@@ -132,7 +132,7 @@ export class VideoPlayerNode extends Node {
     const autoTrackerNode = new AutoTrackerNode(this.videoElement, autoTrackingShownProperty, {
       tracking: model.tracking,
       videoDimensionsProperty: model.playback.videoDimensionsProperty,
-      frameRateProperty: model.playback.frameRateProperty,
+      timeToFrame: (time: number) => model.playback.timeToFrame(time),
       modelViewTransformProperty: model.overlayTools.modelViewTransformProperty,
     });
 
@@ -429,19 +429,9 @@ export class VideoPlayerNode extends Node {
   }
 
   private seekByFrames(direction: number): void {
-    this.playback.isPlayingProperty.value = false;
-    const duration = this.videoElement.duration;
-    if (!(duration > 0)) {
-      return;
-    }
-    const frameDuration = this.playback.frameDurationProperty.value;
-    const raw = this.videoElement.currentTime + direction * frameDuration;
-    const clamped = Math.max(0, Math.min(raw, duration));
-    if (!Number.isFinite(clamped)) {
-      return;
-    }
-    this.videoElement.currentTime = clamped;
-    this.playback.currentTimeProperty.value = clamped;
+    this.playback.seekByFrames(direction);
+    // Sync the DOM element to the model's new time so the video frame updates immediately.
+    this.videoElement.currentTime = this.playback.currentTimeProperty.value;
   }
 
   private loadUrl(url: string): void {