Fix all three critical issues from CODE_REVIEW.md - #15
Merged
Conversation
1. OpenCV ROI dimension validation (OpenCVTracker.ts) Extract roiW/roiH to local variables and throw an explicit Error if either is <= 0. Previously, a degenerate selection box could produce zero or negative dimensions that crash the WASM module with an unrecoverable error requiring a page reload. 2. Duplicate-frame point replacement (SimModel.ts) addPointToTrack() now replaces an existing point at the same frame rather than silently discarding the new coordinates. The old behaviour caused re-digitized points (e.g. correcting a misclick) to appear to succeed in the view while the data table retained the stale value. 3. dispose() on three view components (PlaybackControlsNode, CoordinateSystemNode, CalibrationToolNode) Each component now captures its Axon property listeners by name and unlinks them — along with disposing local DerivedProperty instances — in a private dispose closure called from an override dispose(). This matches the pattern already used by VideoPlayerNode and establishes a consistent cleanup contract across all view components. https://claude.ai/code/session_0166NkhC8JkCQWkiRvir4o2B
1. Move coord-origin clamping to model layer (SimModel.ts) The re-entrancy-flag approach in the view was fragile and in the wrong layer. The clamping lazyLink now lives in SimModel.constructor() where any writer of coordOriginProperty benefits automatically. The isClamping flag is eliminated: the condition `if (clampedX !== pos.x || ...)` already prevents re-entry because a clamped value clamps to itself. Removes VIDEO_BOUNDS, Bounds2, and the four VIDEO_* layout imports that were only needed for clamping from CoordinateSystemNode. 2. Refactor verbose kinematics computation (SimModel.ts) The ~130-line computeTrackKinematics() with three near-identical forward/backward/central difference blocks is replaced by a single 16-line finiteDifference() helper. Two passes (velocity then acceleration) each call finiteDifference(), halving the line count and eliminating all duplication. 3. Add per-track kinematics caching (SimModel.ts) trackKinematicsProperty previously recomputed kinematics for every track on any track change. A kinematicsCache Map now stores the last computed TrackKinematics per track ID keyed by the points array reference; only tracks with a changed points array are recomputed. Since addPointToTrack() always creates a new array, reference equality detects modifications exactly. The cache is cleared on reset(). 4. Fix auto-tracker race condition (AutoTrackerNode.ts) If the user deletes the active track while WASM is loading, the initFromVideo().then() callback now detects that the track no longer exists and disposes the tracker rather than leaving a ready-but-idle tracker running while silently recording no data. 5. Document X/Y axis drag direction (GraphInteractionHandler.ts) Added explanatory comments to both the Y-axis drag (+deltaY) and X-axis drag (-deltaX) code paths explaining why the signs differ and confirming both produce the same "content follows drag" UX. 6. Add dispose() to WebcamPanel (WebcamPanel.ts) WebcamPanel had no dispose() method; if the component was torn down while recording, the setInterval timer continued running indefinitely. dispose() now calls cleanup() (which calls stopTimer()) before super. https://claude.ai/code/session_0166NkhC8JkCQWkiRvir4o2B
11. Frame rounding inconsistency (PlaybackControlsNode.ts)
frameCountTextProperty now uses Math.round(time * frameRate) instead of
Math.round(time / frameDuration), matching AutoTrackerNode.ts and
avoiding cascading IEEE 754 error at non-integer frame rates like 29.97.
The DerivedProperty dependency is updated from frameDurationProperty to
frameRateProperty accordingly.
12. Array.shift() trail → O(1) circular buffer (AutoTrackerNode.ts)
The trail was a plain array whose oldest element was removed with shift()
(O(n)) at 30 Hz. Replaced with a fixed-size ring buffer: a pre-allocated
MAX_TRAIL-element array plus head/size counters. Push and eviction are
O(1); iteration visits elements oldest-to-newest via the tail formula
(head - size + i + MAX_TRAIL) % MAX_TRAIL.
13. Dual property links → Multilink (CalibrationToolNode.ts)
Two separate calibPoint1Property.link(updateGeometry) /
calibPoint2Property.link(updateGeometry) calls are replaced by a single
Multilink.multilink([prop1, prop2], updateGeometry). Disposal is now
one calibMultilink.dispose() call instead of two unlink() calls.
14. Magic VIDEO_CENTER_Y constant (TrackLabConstants.ts)
Added module-private LAYOUT_WIDTH = 1024 and LAYOUT_HEIGHT = 618
constants. VIDEO_PLAYER_Y_OFFSET is moved before the center constants
so they can reference it. VIDEO_CENTER_X and VIDEO_CENTER_Y are now
expressed as LAYOUT_WIDTH/2 and LAYOUT_HEIGHT/2 + VIDEO_PLAYER_Y_OFFSET,
making the derivation self-documenting.
15. Cross-origin errors silently swallowed (OpenCVTracker.ts)
The bare catch block in track() now binds the error as `e` and logs a
console.warn so developers can diagnose cross-origin CORS failures
without crashing or silently losing frames.
16. No i18n key-parity validation (StringManager.ts)
Two module-level assignments (_enMatchesFr / _frMatchesEn) act as
compile-time structural type checks. TypeScript will report a type
error here if either language file diverges from the other's key
structure, catching missing translations before the app is run.
https://claude.ai/code/session_0166NkhC8JkCQWkiRvir4o2B
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OpenCV ROI dimension validation (OpenCVTracker.ts)
Extract roiW/roiH to local variables and throw an explicit Error if
either is <= 0. Previously, a degenerate selection box could produce
zero or negative dimensions that crash the WASM module with an
unrecoverable error requiring a page reload.
Duplicate-frame point replacement (SimModel.ts)
addPointToTrack() now replaces an existing point at the same frame
rather than silently discarding the new coordinates. The old behaviour
caused re-digitized points (e.g. correcting a misclick) to appear to
succeed in the view while the data table retained the stale value.
dispose() on three view components (PlaybackControlsNode,
CoordinateSystemNode, CalibrationToolNode)
Each component now captures its Axon property listeners by name and
unlinks them — along with disposing local DerivedProperty instances —
in a private dispose closure called from an override dispose(). This
matches the pattern already used by VideoPlayerNode and establishes a
consistent cleanup contract across all view components.
https://claude.ai/code/session_0166NkhC8JkCQWkiRvir4o2B