fix(studio): keep layer selection seek within clip range#792
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
First review at 039c2b44. CI clean (all required green: Build, CLI smoke, Test, Typecheck, Lint, Format). No prior reviews.
Audited
packages/studio/src/components/editor/LayersPanel.tsx:1-176(modified — theseekToLayercall site)packages/studio/src/utils/studioHelpers.ts:158-172(the newresolveTimelineSelectionSeekTimehelper)packages/studio/src/utils/studioHelpers.test.ts:1-20(the new test suite)
Context — three converging PRs on the same bug
This PR overlaps with #793 (Jefsky, "fix(studio): keep layer selection seek within clip range") which has the same title and addresses the same defect. The two PRs are NOT byte-equivalent:
- #792 (this one): modifies the existing
LayersPanel.tsx, extracts the clamping logic intoresolveTimelineSelectionSeekTimeinstudioHelpers.ts, adds unit tests for the helper. - #793: adds
LayersPanel.tsxas a new file (status: added, 295 lines) — which will fail to merge againstmainsince the file already exists. No helper extraction, no tests.
Recommendation: prefer this one (#792). Close #793 as superseded. Same call-out applies to #795 (Jefsky) which bundles three unrelated fixes including a duplicate LayersPanel.tsx new-file.
Strengths
- Helper extraction is the right shape.
resolveTimelineSelectionSeekTime(currentTime, { start, duration })(studioHelpers.ts:158-172) takes a small, well-typed input and returns the clamped time. The component just consumes the return value — no business logic leaked into JSX. Easy to test, easy to re-use if another panel grows the same need. - NaN handling is explicit. The helper falls back to clip start when
currentTimeis NaN (studioHelpers.ts:165-167, test atstudioHelpers.test.ts:17-19). That's the right default — a NaN currentTime can happen during initial player mount before the first tick, and clamping-to-NaN would propagate torequestSeekand break the player. currentTimeadded to theuseCallbackdeps array (LayersPanel.tsx:157). The closure now captures the current time correctly without staleness — this is the kind of fix that often gets forgotten in seek-handling code (the same class of bug as the stale-closure debounced save in #623).- Tests pin the three clamp branches plus the NaN fallback. Four cases, each pinning a distinct behavior (
inside-range,before,after,NaN). No flakytoBeCloseToshape — exact integer assertions. ✓
Important — NaN-fallback returns clip start, but caller still gates on nextTime != null
LayersPanel.tsx:152 reads if (nextTime != null) usePlayerStore.getState().requestSeek(nextTime);. The helper at studioHelpers.ts:160-170 returns number in every branch (never null or undefined) — the type signature is number, not number | null. The != null check is defensive against a return shape the helper doesn't produce. Either tighten the helper signature to number and drop the guard, or document the future case where a null return could mean "don't seek". Current state is dead-code defensive.
Nit
studioHelpers.test.ts:1-20— nodescribeblock for the NaN test boundary case. Could split intodescribe("inside the clip range")/describe("before the clip")/describe("after the clip")/describe("invalid input")for readability, butit.eachor namedit()calls in a single block is the team pattern in this file, so this is taste.
Verdict
Verdict: APPROVE
Reasoning: Correct fix, well-extracted helper, NaN guarded, tests pin all four branches, CI green. Tighten the dead-code != null guard at the call site as a follow-up. Supersedes #793 and the LayersPanel portion of #795 — both should be closed.
— Vai
What
Keep Layers panel selection seek within the selected clip range.
Why
Clicking a layer always jumped to the clip midpoint. That could move the preview even when the current time was already inside the clip.
How
Resolve the next seek time by clamping the current time into the clip range. This keeps the current time when it is already valid, and only seeks when the current time is outside the clip.
Test plan
How was this tested?