feat(studio): model logical timeline navigation - #2712
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ebadea0 to
5b22e39
Compare
f191fe9 to
2d0f39e
Compare
5b22e39 to
816894f
Compare
2d0f39e to
6970bd9
Compare
816894f to
1a4e7c0
Compare
6970bd9 to
974a0f9
Compare
1a4e7c0 to
7b2f56e
Compare
974a0f9 to
0224171
Compare
7b2f56e to
b74d0f7
Compare
0224171 to
56c56ca
Compare
b74d0f7 to
2b50771
Compare
6079dcd to
ed633c9
Compare
2b50771 to
d0891eb
Compare
ed633c9 to
0b635c5
Compare
0b635c5 to
61e5b35
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
R1 adversarial review — APPROVE at 61e5b35c495729de91d78f4bcf3ac57b9a82a19c
Pure logical model over timeline tracks/clips/property-lanes/keyframes/eases. +581/-0 = two new files, 263 test lines vs 318 code (~82% ratio). Model has no React, no DOM, no subscriptions — trivially free of useEffect anti-patterns and retainer leaks.
Verified end-to-end (not presence-only):
- Row projection with continuous
logicalIndex,level: 1|2, parent linkage — asserted structurally. - Keyframe + ease ordering across two colliding position animations — six-entry sequence checked exactly (
[10 z, 12.5 ease-a, 15 a, 15 z, 17.5 ease-z, 20 z]); the same-time gatecurrent.time > previous.timecorrectly suppresses ease between coincident keyframes. - Sole expanded-lane owner:
resolveTrackKeyframeClippicks the selected clip on a shared track (asserted "visual" only, not "position"). - Horizontal + Home/End; PageUp/PageDown with caller-supplied
pageSize(missing/invalid → no-op, tested). - Equal-distance vertical tie-break: distance → time asc → id lex —
earlier-awins overearlier-z/laterat abs-5, exercising all three levels. - Deletion fallback chain prev → next → parent → containing row — three separate tests plus a null-for-unknown-id case.
Adversarial passes:
- Element-identity keying —
elementId = element.key ?? element.idconsistently at every sort/ID/lookup site (per memory pattern). - Virtualization independence — model is built from
tracks+displayTrackOrder, never DOM. This is the PR's stated invariant and it holds. - Immutable inputs —
[...row.items].sort(),[...unique.entries()].sort(); no readonly mutation. - Row-index clamp —
Math.max(0, Math.min(rows.length - 1, ...))+destinationIndex === rowIndex → targetprevents infinite loops at boundaries. - Stable IDs —
JSON.stringify(["timeline", kind, ...parts])namespaced by kind, no cross-collision (track_1vsproperty_1_positionvsclip_active). - Shortcut conflict lens — N/A: this PR does not bind keys; it resolves navigation targets given a key name. Backspace/undo/delete conflicts belong to the future binding PR.
- ARIA framing — no aria emitted here; consumer #2713 reads
logicalIndex/level/parentId/expandedto producearia-rowindex/aria-level/aria-expanded(verified in #2713's diff). Contract is clean.
Two follow-up notes (non-blocking):
level: 1 | 2bakes in exactly two levels. Fine today; if nested groups arrive, this needs widening.TimelineLogicalRow.expanded: boolean— #2713 referenceslogicalRow.expandable; that field is added in #2713's delta. Not this PR's concern, just flagging the interface will grow.
CI: 15/17 SUCCESS, 2 SKIPPED, Graphite mergeability_check IN_PROGRESS. mergeStateStatus: UNSTABLE from the in-progress check only.
— Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 61e5b35c.
Pure logical navigation model for the timeline: track/property rows, clip items, keyframe/ease items, all under a treegrid mental model. 318 lines source + 263 lines test. 11 focused tests exercise projection, sorting, keyframe/ease interleaving, navigation across all key classes (except Up/PageUp — see nit), and focus-preservation fallback. Type-safe (level: 1 | 2 caps hierarchy depth to what HF actually models), sort determinism explicit at three-key tie-breaking, fallow-ignore-next-line complexity opt-outs justified by docstrings above each large actor.
One concern inline about the orphan surface at merge — the two nav-actor functions have no runtime consumer even at the stack tip.
Nits / observations (body-only):
- Ease-control attribution between different animations.
propertyItems(:100-141in this PR's file) emits an ease control between adjacent composite keyframes, attributed tocurrent.target(the incoming animation). The test attimelineKeyboardNavigation.test.ts:117-124locks inease/12.5/a-animationbetweenkeyframe/10/z-animationandkeyframe/15/a-animation— semantically defensible for a composite timeline where the user edits a shared time axis, but the ease at 12.5 doesn't map to a-animation's own tween segment (a-animation has no keyframe at time 10). Consider a docstring noting the composite-sequence semantics so a future reader doesn't misread it as "the incoming ease inside a-animation's tween." propertyItemsguards ease emission oncurrent.target.animationId !== undefined(:117in this PR's file) but not on whetherprevious.target.animationId === current.target.animationId. The current test explicitly requires cross-animation eases; if that's the intended behavior, fine, but worth naming in a comment.- Test coverage gaps — the horizontal + vertical nav suite exercises
ArrowRight/ArrowDown/Home/End/PageDownbut not the up-direction symmetry (ArrowUp/PageUpexplicit assertions). Direction is captured indirection = key === "ArrowUp" || key === "PageUp" ? -1 : 1— the code path is uniform, but a targeted test would harden against future asymmetric changes. Also missing:ArrowLeftfrom row itself (should stay per:541);ArrowRightfrom last item (should stay per:546). stableIdusesJSON.stringify— produces ids like["timeline","clip","hero"]with literal brackets/quotes. Legal HTMLid(no whitespace, no U+0022 issue because it's the ATTRIBUTE value not a delimiter), but they're not usable in CSS#selectorsyntax without escaping.document.getElementByIdandaria-ownsreference them fine. Worth noting so no one grafts a CSS selector on top later.
Ready from my side, leaving as COMMENTED.
61e5b35 to
2981a48
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
R2 delta re-review at 2981a48 (full rewrite since R1 61e5b35).
Nit-1 [SKIPPED]: level: 1|2 still baked-in — non-blocker; widen when nesting arrives.
Nit-2 [FIXED via documentation]: Both navigation exports (resolveTimelineNavigationTarget, resolveTimelineFocusFallback) now carry JSDoc naming their downstream consumers (useTimelineKeyboardActor, useTimelineFocusCoordinator). The expanded: boolean shape wasn't unified with #2713's expandable, but future consumers now see the coupling contract on the export surface — acceptable per the flag semantics.
No regressions in F1 correctness:
- Element-identity keying preserved (
element.key ?? element.id). - Deterministic tie-breaks intact (time → id.localeCompare).
- Deletion fallback chain unchanged (previous item backward → forward → surviving row → parent → row-index) with
fallow-ignore-next-line complexitynoting the invariant. - Same-time-keyframe gate preserved (
current.time > previous.time && animationId !== undefinedskips ease). - PageUp/PageDown correctly no-ops when
pageSizeunset/invalid — viewport-actor contract respected.
CI: Detect changes/Preflight/regression/player-perf all SUCCESS; Preview parity + Graphite mergeability still IN_PROGRESS (non-blocking for content review).
— Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Delta-reviewed 61e5b35c..2981a48fb.
| # | R1 concern | R2 verdict | Anchor |
|---|---|---|---|
| I1 | resolveTimelineNavigationTarget + resolveTimelineFocusFallback have zero runtime consumers even at #2713's tip — orphan-at-merge |
🟢 RESOLVED | timelineKeyboardNavigation.ts:248-252,292-296 — both exports carry JSDoc naming the downstream hook that consumes them: useTimelineKeyboardActor and useTimelineFocusCoordinator respectively. Same treatment as the .fallowrc.jsonc naming Miguel used on #2990's externalConflictStorage exports; converts orphan-at-merge from silent shipping risk to an auditable trail across the workstream |
Nothing else surfaced in the delta. The two body-nit observations from R1 (ease-control cross-animation attribution, missing Up/PageUp explicit assertions, stableId CSS-selector caveat) remain as soft flags for the wiring PR to close over.
Ready from my side, leaving as COMMENTED.
2981a48 to
d247c90
Compare
1be5a1f to
fc416c8
Compare
|
Review follow-ups folded into exact head |
vanceingalls
left a comment
There was a problem hiding this comment.
Re-verify at rebased head d247c9056 (prior R2 at 2981a48fb).
Delta since prior stamp (PR files only):
timelineKeyboardNavigation.ts: two new inline comments —stableIdCSS.escape callout for consumers, and aponytail:note documenting that the composite property lane owns adjacency for the ease-segment loop. No behavior change.timelineKeyboardNavigation.test.ts: three additive assertions — ArrowLeft/Right no-op at row-list edges, ArrowUp/PageUp from a property target lands on the active track row, PageDown withpageSize: 1at the last row is a stable no-op.
Family F invariants preserved: elementId() still element.key ?? element.id (line 87); clip sort still cascades start → end → id for deterministic tie-breaks; deletion-fallback chain intact; .ts module so no component/hook export surface. Rames' orphan-consumers thread (line 248) resolved at 2981a48 via JSDoc on both nav exports naming useTimelineKeyboardActor/useTimelineFocusCoordinator.
CI: regression SUCCESS on the stacked base (the only master-required check that fires at this base); mergeStateStatus: UNSTABLE is Graphite's mergeability_check still in-progress (non-required), same profile as prior two stamps. Miguel confirmed threads resolved.
— Review by Via
The base branch was changed.
d247c90 to
51fd4ac
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 51fd4acbd — post-retarget rebase onto main.
Delta from last-reviewed d247c9056 (my prior COMMENTED at 2981a48f):
Scope-file blob SHAs are byte-identical between d247c9056 and 51fd4acbd:
packages/studio/src/player/components/timelineKeyboardNavigation.ts→b4eac8f2packages/studio/src/player/components/timelineKeyboardNavigation.test.ts→7495a675
git patch-id --stable on both diffs → 35e51576c5c730f6dbd19a926d6712af32b6b922 (match).
Pure rebase, no content changes. My prior findings resolutions all stand (ponytail comment on composite-property lane; CSS.escape guidance on stableId; 5 boundary tests). Nothing new to review. LGTM from my side — leaving as a comment, safe to re-stamp.
vanceingalls
left a comment
There was a problem hiding this comment.
Delta re-verify at exact head 51fd4acbd.
Retarget: base is now main directly (previously stack branch); #2711 merged so its content is already in main. Patch is now the pure addition of the two navigation files.
Patch-equivalence vs prior stamp d247c9056: verified. Both files (timelineKeyboardNavigation.ts, timelineKeyboardNavigation.test.ts) are byte-identical to the prior stamped revision — SHA-content diff is empty. The retarget is pure rebase noise; the PR's own diff against main is +330 / +276 / -0, as expected once #2711 is subtracted.
Prior findings state: all preserved.
- Downstream-consumer contracts on both nav exports intact.
- Rames orphan-consumers thread stays resolved.
- Prior R2 delta additions (doc comment in
timelineKeyboardNavigation.tsat the fallback chain, 3 additive test assertions) are part of the byte-identical content.
Family F invariants: preserved.
- Element-identity keying:
element.key ?? element.id(line 87). - Deterministic tie-breaks:
localeCompareonelementId/ row id (lines 96, 141, 246). - Deletion fallback chain:
resolveTimelineFocusFallbackpresent with invariant doc comment (line 300).
Required-check state (HF ruleset 14211637, at head 51fd4acbd):
Build— not yet scheduled (CI workflow queued/pending)Render on windows-latest— IN_PROGRESSSemantic PR title— not yet reportedTest— not yet scheduled (CI workflow queued/pending)Test: runtime contract— not yet scheduled (CI workflow queued/pending)Tests on windows-latest— IN_PROGRESSTypecheck— not yet scheduled (CI workflow queued/pending)regression— SUCCESS
Approving on content: patch-equivalent to prior stamp, no drift, invariants intact. Do not merge until all 8 required checks show SUCCESS — five are still pending/in-progress on this exact head.
— Review by Via

What
Add a pure logical navigation model for timeline tracks, clips, property lanes, keyframes, and easing controls.
Why
Keyboard movement must follow the complete editor structure rather than the subset of DOM nodes currently mounted by timeline virtualization.
How
The model assigns stable identities, orders items by authored time, moves horizontally within a row, and moves vertically to the nearest target at the same time. Home, End, Page Up, and Page Down use the same deterministic traversal contract.
This PR targets #2711 and remains one focused commit.
Test plan
The exact PR delta passes its 11 focused navigation-model tests, Studio typecheck, Fallow, formatting, and exact-head workflows.