feat(studio): add timeline keyboard controls - #2715

Verified against 204a84f01d (F #2715 tip). One focused commit adds useTimelineKeyboardActor, threads a rovingTargetId through row/clip/keyframe/ease surfaces, and moves the collapsed-clip diamond lane out of the TimelineClip <button> into a sibling wrapper. Focused + Studio suites pass per the description.

Axis 1 — key binding correctness. Arrow/Home/End/Page keys route through the pre-existing resolveTimelineNavigationTarget; ArrowLeft/Right traverse the item strip within a row (clips/keyframes/ease) and ArrowUp/Down/Page traverse rows — an intentional two-axis adaptation of APG treegrid, not a bug. Enter/Space toggle only when located.target.kind === "row" and expandable — verified against the test (row toggles, clip does not, preventDefault gated accordingly). Ctrl/Meta+Home/End escape row-boundary correctly. ContextMenu/Shift+F10 both synthesize a bubbling contextmenu MouseEvent at the target's center; the existing scoped onContextMenu handlers catch it. No * (asterisk) sibling-expand, but that pattern was never part of Miguel's advertised scope.

Axis 2 — roving tabindex invariant. Every focus target now takes tabIndex as a prop derived from rovingTargetId, computed from the store's focusedTargetId with a logicalRows[0]?.id fallback. Prop-driven — no imperative DOM writes. Virtualization test asserts [data-timeline-focus-id][tabindex="0"] count === 1 on a 1000-row model. Coordinator (from prior slices) scrolls the focused target back into the mounted window, so a virtualized-out focusedTargetId doesn't drop the invariant in practice.

Axis 3 — frame-aware seek. Out of scope for this PR — the actor is focus-only. Test asserts requestedSeekTime stays null after every keyboard action; keyboard nav does not perturb the playhead. Good.

Axis 4 — regression tests. useTimelineKeyboardActor.test.tsx fires real KeyboardEvent via dispatchEvent (not userEvent.keyboard, but the actual event carries key, modifiers, and defaultPrevented). Assertions are on output — usePlayerStore.getState().timelineFocus?.id, defaultPrevented, onToggleRow mock, requestedSeekTime. Not presence-only. TimelineClip.test.tsx asserts the new native-button semantics (type === "button", tabIndex === -1, aria-pressed). TimelineClipDiamonds.test.tsx covers time-based label, boundary markers, Shift+click additive selection, and detail-0 keyboard activation on the diamond. All meaningful.

Axis 5 — global shortcut interop. Handler scoped to <div role="treegrid">'s React onKeyDown — no window listener, no synthetic-vs-native ordering hazard. Focus scope is enforced by eventTarget() returning null unless an ancestor [data-timeline-focus-id] is inside event.currentTarget — good.

Other lenses.

• Element-identity keying el.key ?? el.id preserved throughout — timelineClipFocusId(elementKey) on both writer (TimelineClip.tsx, TimelineLanes.tsx) and reader (actor via locateTimelineLogicalTarget). Consistent.
• elementId: null → elementId: activeId in buildTimelineLogicalRows is the fix that makes onToggleRow's if (row.elementId) toggleClipExpandedTracked(row.elementId) actually fire for track rows. Prior slices left this as a silent no-op.
• No useEffect state-syncing — actor uses useCallback only.
• Fast Refresh clean — .tsx files export only components, hook module is .ts.
• <div> → <button> on TimelineClip correctly relocates TimelineClipDiamonds to a sibling wrapper (TimelineCompactDiamonds) to avoid nested-interactive HTML. Absolute positioning is preserved by keeping the wrapper in the same offset parent as <TimelineClip> and mirroring the same left/top/width/height math.
• Nit: aria-multiselectable="true" was removed from the treegrid, and the virtualization test now asserts its absence. Timeline still supports multi-clip selection via shift-click/marquee, so this deviates from APG's "if multiselect, aria-multiselectable=true" guidance. Worth explaining in the PR body why single-selection was chosen at the treegrid boundary (probably because roving tabindex + selection state are decoupled, and AT users interact with aria-pressed on individual rows/clips).
• Nit: three invariant comments trimmed — elementKey unique per clip; do NOT fold in map index (TimelineLanes), Passenger of a live multi-drag … (TimelineLanes), Interactive controls own their clicks; scrubbing would preventDefault and eat them (Timeline.tsx). These encode learned lessons ("splice/reorder remounts" is a real trap); re-restoring at least the clipKey comment is cheap insurance.
• Nit: useTimelineKeyboardActor.onKeyDown carries // fallow-ignore-next-line complexity. Four key classes in one function is defensible ("intentionally share one actor" is even in the comment above resolveTimelineNavigationTarget), but a follow-up could split by kind.

CI: preview-regression, regression, player-perf, Preflight all SUCCESS at this SHA; Graphite / mergeability_check still IN_PROGRESS. Family F #2711–#2714 already stamped, docs tip #3031 remains draft.

Verdict: APPROVE. No correctness bugs; roving invariant, focus-scope, and test discipline are all solid.

— Review by Via

Reviewed at fc6a7b52 (announced as 204a84f0; restacked to pick up the current #2714 merge base — every file in #2715's own scope is byte-identical between the two heads, verified via git diff --stat 204a84f01d fc6a7b5221 -- <all 12 scope files> returning empty). Every finding, line anchor, and PR-body-audit item below applies at HEAD.

Wires the logical model from #2714's focus coordinator into a single keyboard actor at useTimelineKeyboardActor.ts (111 lines) attached to the treegrid's onFocus/onKeyDown. Arrow keys route through the pure resolveTimelineNavigationTarget (extended with Home/End/PageUp/PageDown), Enter/Space toggles disclosure on expandable rows, and Ctrl-modifier variants of Home/End jump to the timeline boundary. Roving tab stop is expressed as a single rovingTargetId derived from focusedTargetId; every TimelineClip, TimelineTrackRow, PropertyGroupHeaderRow, TimelineDiamondLane keyframe/ease button reads it against its own focus id and sets tabIndex={0} or {-1}.

Three inline concerns, all 🟡 — none blocking. Design intent + spec-deviation naming.

Verified end-to-end:

• ✅ Roving invariant on virtualized timelines: Timeline.virtualization.test.tsx:213 asserts exactly one [data-timeline-focus-id][tabindex="0"] across a 1000-row/16-mounted-row treegrid. The pin from #2714's pinnedRowKeys keeps the focused row mounted; the pin from pinnedClipIdentities keeps the focused clip mounted. Together they guarantee the roving target has a DOM element even when scrolled off.
• ✅ Focus-target dispatch through eventTarget: event.target.closest("[data-timeline-focus-id]") + event.currentTarget.contains(target) — bubbles from any focus id inside the treegrid, rejects targets in unrelated portals or aria-owns subtrees. Correctly handles the click-on-caret-inside-row case (caret has no focus id → walks up to the row's focus id → requestTimelineFocus(row.id)).
• ✅ Redundant-focus guard: useTimelineKeyboardActor.ts:68 — if (id && id !== focusedTargetId) requestTimelineFocus(id). By the time the browser's focus event bubbles up (post-focusElement() in the coordinator's useEffect), React has already re-rendered with the new focusedTargetId, so the closure sees the current value and no redundant nonce bump fires.
• ✅ Enter/Space semantics preserve native activation: actor's guard at :97-102 gates disclosure to kind === "row" && expandable. Clip / keyframe / ease activation falls through to the button's own onClick, which correctly fires from Enter/Space with e.detail === 0 (verified by useTimelineKeyboardActor.test.tsx:152).
• ✅ ContextMenu key + Shift+F10 dispatch: synthesized MouseEvent("contextmenu", ...) at the target's getBoundingClientRect() center. Bubbles through the target's own onContextMenu handler → opens ClipContextMenu / KeyframeContextMenu at clientX/Y. Tested at :157-166.
• ✅ Store slice reshape: requestTimelineFocus(id) bumps timelineFocusNonce and stamps a fresh TimelineFocusRequest{id, projectId, sessionEpoch, nonce}. Coordinator's compare-and-clear (s.timelineFocus?.nonce === nonce ? { timelineFocus: null } : s) prevents stale clears over newer requests. Tested at playerStore.test.ts:495-524.
• ✅ Nearest-target tie-breaking on vertical navigation: resolveTimelineNavigationTarget:293 picks the item with minimum |time - target.time|, tie-breaking by earlier time then stable identity (localeCompare). Deterministic — tested at timelineKeyboardNavigation.test.ts:208-225.
• ✅ Multi-timeline isolation from #2713: no id-scoping regression here. data-timeline-focus-id values from a second timeline instance are inside its own scrollRef container, and the eventTarget contains check bounds each actor to its own tree.

PR-body promise audit:

• ✅ "One keyboard actor handles arrow navigation, Home, End, paging, disclosure, and context-menu commands" — useTimelineKeyboardActor.ts is the sole handler; single onKeyDown covers all six key classes.
• ✅ "Clips, keyframes, and easing controls expose stable focus identities" — timelineClipFocusId, timelineKeyframeFocusId, timelineEaseFocusId route through timelineNavigationIdentity.ts.
• ✅ "Time-based labels" — clip aria-label (${label}, ${startLabel} to ${endLabel} seconds) at TimelineClip.tsx:108, keyframe aria-label uses keyframeTimeLabel(clipStart, clipDuration, percentage) at TimelineClipDiamonds.tsx:346.
• ✅ "Selection state" — aria-pressed={isSelected} on clip; aria-pressed={isKfSelected} on keyframe (TimelineClipDiamonds.tsx:347); aria-current="time" on the diamond nearest the playhead.
• ✅ "Focus coordinator handles reveal and remounting" — no coordinator changes in this PR; the wire from #2714 is preserved (requestTimelineFocus(id) → store → useTimelineFocusCoordinator → scrollToTarget + focusElement).
• ✅ "One focused commit" — one commit 204a84f0, no rebase noise.

Test plan claim: "177 focused timeline and navigation tests" and "3,351 tests with 1 file skipped and 18 planned todos" — CI is green on regression, preview-regression, player-perf, preview-parity, player-perf.

Nits (body-only):

• Enter/Space test coverage on clip could assert one native effect. useTimelineKeyboardActor.test.tsx:152 asserts defaultPrevented === false on Enter over a clip, but doesn't verify that the native onClick actually fires. Adding a const click = vi.fn(); target.addEventListener("click", click); key(target, "Enter"); expect(click).toHaveBeenCalled(); would make the invariant "the actor lets native activation through" concrete rather than inferred. Non-blocking — the actor's early-return is straightforward — but the invariant is load-bearing (it's how clips/keyframes get activated at all) and the test currently only proves half of it.
• PageDown at the last row → no-op (returns target). Tested via PageDown with no pageSize at timelineKeyboardNavigation.test.ts:190 (?.id).toBe(current)), but this exercises the "invalid page size → no-op" path (line 286-287), NOT the "already at last row, valid page size → no-op" path (line 292's destinationIndex === rowIndex). The two return target short-circuits are semantically different and worth a distinct test.
• useTimelineKeyboardActor.ts:65-71 onFocus deps [focusedTargetId]. The useCallback re-creates the callback whenever focusedTargetId changes — every keystroke that moves focus creates a new callback and re-attaches React's synthetic listener. Not a real perf concern (React reuses the listener slot), but the invariant "the closure captures the current focusedTargetId" is what makes the redundant-focus guard correct. Worth a ponytail: naming this — the guard's correctness depends on the closure being current, and a future refactor that inlines the callback or drops the dep would silently regress into the double-nonce-bump case.
• viewportPageSize allocates a Set<number> and filters logicalRows on every keystroke (useTimelineKeyboardActor.ts:36-37). Cheap at 100 rows; scales linearly with row count. For a 1000-row timeline that's 1000 predicate calls per PageUp/PageDown keystroke — still fast but names a smell. Consider memoizing per (logicalRows, viewport.scrollTop, viewport.clientHeight) if this shows up in the perf pass.
• The disclosure caret in LayerDisclosureRow.tsx:47 (tabIndex={-1}) has no data-timeline-focus-id, but the row shell above it does. Focus flow works (caret click → focus bubbles → onFocus → closest walks up to the row → requestTimelineFocus(row.id)), but if a future maintainer adds data-timeline-focus-id to the caret they'd bifurcate the row's focus identity across two DOM nodes. A comment on the caret (ponytail: no focus id — focus routing intentionally routes to the enclosing row shell) would prevent that regression.

Ready from where I sit — this is a solid keyboard model, deviates from WAI-ARIA APG in named ways, and the inline concerns are about naming those deviations rather than reversing them. Leaving as COMMENTED.

— Review by Rames D Jusso

Delta-verified at be24c5412 against R1 at fc6a7b5221. 5 files touched, one focused commit. Every R1 concern addressed cleanly; the fixes are load-bearing rather than cosmetic.

Findings resolved (all three):

• ✅ APG treegrid divergence (was useTimelineKeyboardActor.ts:84 in R1). The actor's onKeyDown now short-circuits BEFORE resolveTimelineNavigationTarget when the target is a kind === "row" with expand/collapse semantics: ArrowRight on expandable && !expanded calls onToggleRow(located.target) (expand); ArrowLeft on expandable && expanded does the same (collapse). Falls through to the model otherwise, so ArrowRight on an already-expanded row still lands on the first item and ArrowLeft on a collapsed non-expandable row still walks to the parent — the model itself now handles that via if (!row.parentId) return row; return locateTimelineLogicalTarget(rows, row.parentId)?.target ?? row; in the itemIndex < 0 branch of timelineKeyboardNavigation.ts:274-276. Ctrl+End boundary now returns boundaryRow directly (timelineKeyboardNavigation.ts:272), matching the APG spec's "Control+End → last row." Coverage: useTimelineKeyboardActor.test.tsx:188-198 (arrow-driven expand + Enter/Space parity), timelineKeyboardNavigation.test.ts:210-217 (Ctrl+End = row not last item), timelineKeyboardNavigation.test.ts:220-226 (property row ArrowLeft → parent). The Enter/Space path stays intact as an equivalent — good, that's a valid APG alternative too.

• ✅ viewportPageSize focus-anchoring (was useTimelineKeyboardActor.ts:38 in R1). Signature now takes focusedTrackKey: number; when the focused physical row is resolvable via geometry.getRowIndex(focusedTrackKey), pageTop = geometry.getRowTop(focusedRow) anchors the visibility window to the focused target's neighborhood, falling back to viewport.scrollTop only when the focused key isn't in the geometry. Call-site at useTimelineKeyboardActor.ts:107 correctly passes located.row.physicalTrackKey (the focused row's key, not the closest ancestor's), so property rows page based on their track's density. Regression coverage in useTimelineKeyboardActor.test.tsx:174-207 uses a deliberately non-uniform [104, 48, 48] layout with clientHeight=47 and off-screen focus on track-3 — the old viewport-derived formula would page by (rows visible at scrollTop=0) = 3 (track-1 + 2 property rows), landing on property-1; the new focus-anchored formula pages by (rows near focus) = 1, landing on track-2. The test asserts track-2, so it discriminates the two behaviors correctly.

• ✅ Native header controls / roving scope (was TimelineTrackHeader.tsx:234 in R1). The clean fix lives in eventTarget at useTimelineKeyboardActor.ts:20-27: event.target.closest("button, input, select, textarea, a[href], [contenteditable], [data-timeline-focus-id]") now finds the nearest interactive ancestor and only accepts it when its dataset.timelineFocusId is set. So clicks/keydowns on a native <button> inside a row shell (visibility eye, keyframe chevrons, add/remove-keyframe) resolve to the button first, see no focus id, and return null — the actor no-ops, browser handles focus/activation natively. Row shells (which own the focus id) still resolve. Coverage: useTimelineKeyboardActor.test.tsx:136-142 (data-native-control count + tabIndex === 0 invariant across 3 native buttons alongside the roving row) + :224-229 (focusing a native control leaves timelineFocus null; Enter on that control doesn't preventDefault, so native activation flows). Elegant — no per-callsite tabIndex bookkeeping needed.

Also verified:

• Timeline.tsx diff is a pure destructure → struct-access refactor ({ logicalRows, focusedTargetId, pinnedElementId, ... } = useTimelineLogicalFocus(...) → const timelineFocus = useTimelineLogicalFocus(...) + timelineFocus.foo at each access site). 31/37 insertions/deletions with -w still showing 74 lines — the whitespace-insensitive count reflects the reshape, not a semantic change.
• No changes to the store slice, coordinator, pinnedRowKeys/pinnedClipIdentities, or any of R1's ✅ items — all end-to-end invariants from R1 apply unchanged.
• CI at HEAD: regression, Player perf, and preview-regression preflight all green; Preview parity still IN_PROGRESS at review time; Graphite / mergeability_check IN_PROGRESS.

Micro-nit (non-blocking, body-only):

• viewportPageSize clamps the last row calculation via Math.max(0, viewport.clientHeight - 0.001) (useTimelineKeyboardActor.ts:44). The -0.001 is a rounding guard for clientHeight landing exactly on a row-height boundary (avoids overshooting into the next row). Correct fix — worth a one-line comment naming the invariant so a future maintainer doesn't "clean up" the magic number.

Vance's approval at 204a84f01d still carries per the reviewDecision at HEAD. Ready from where I sit — leaving as COMMENTED.

— Review by Rames D Jusso

R2 delta re-review at be24c541. Verified against the three inline threads opened by Rames on 2026-08-04 and their resolutions on the same head.

FIX 1 — APG disclosure + parent + Ctrl+End [FIXED]
useTimelineKeyboardActor.ts:93-102 gates the toggle on expandable, so ArrowRight on collapsed→expand and ArrowLeft on expanded→collapse both fire before the navigation model runs. Falling through to resolveTimelineNavigationTarget correctly handles ArrowRight-on-expanded → first item ("first cell" in APG terms), ArrowLeft-from-level-2 → parent row (timelineKeyboardNavigation.ts:275-280), and timelineBoundary on End → rows.at(-1) (last logical row, timelineKeyboardNavigation.ts:269-274). expandable: lanes.length > 0 in buildTimelineLogicalRows (line 206) rules out the "expandable but no children yet" state — no spurious no-op expand.

FIX 2 — Focus-anchored paging [FIXED]
viewportPageSize now anchors first at geometry.getRowIndex(focusedTrackKey) and computes pageTop = getRowTop(focusedRow) instead of viewport.scrollTop (useTimelineKeyboardActor.ts:35-48). Off-screen focus no longer inherits the visible viewport's density. The 104/48/48 non-uniform regression at useTimelineKeyboardActor.test.tsx:174-208 proves PageUp moves by the focused neighborhood. Adversarial: mid-scroll unmount degrades gracefully — getRowIndex returning -1 falls back to getRowFromY(scrollTop).

FIX 3 — Native header Tab access [FIXED]
eventTarget puts button, input, ... ahead in the closest() selector, requires dataset.timelineFocusId, so header buttons (no data-timeline-focus-id) resolve to null and onKeyDown bails without preventDefault (useTimelineKeyboardActor.ts:19-27, 87-89). Row wrappers keep tabIndex gated by rovingTargetId so exactly one row is a roving stop. The regression at useTimelineKeyboardActor.test.tsx:237-244 verifies focusing a native control does not steal timelineFocus and Enter is not preventDefaulted.

R1 nits residual

• aria-multiselectable: still skipped (accepted in R1)
• invariant comments: three fallow-ignore comments carry equivalent locality — useTimelineKeyboardActor.ts:84, timelineKeyboardNavigation.ts:257,305
• fallow-ignore-next-line complexity on onKeyDown: present at useTimelineKeyboardActor.ts:85
• elementId: null → activeId: wired at timelineKeyboardNavigation.ts:205,220
• TimelineClip is a <button>: TimelineClip.tsx:97

Family F invariants

• Element-identity keying el.key ?? el.id: timelineKeyboardNavigation.ts:85-87
• Roving-tabindex single stop: rovingTargetId propagation intact
• Deletion fallback chain (item-backward → forward → surviving-row → parent → row-index): resolveTimelineFocusFallback at timelineKeyboardNavigation.ts:307-334
• No non-component exports from .tsx: helpers in TimelineTrackHeader.tsx are module-local

CI at head: all completed checks SUCCESS/SKIPPED; Preview parity + Graphite mergeability IN_PROGRESS. Focused suite 33/33 confirmed.

No new findings. Approving.

— Review by Via

Delta re-verify at 16d27c28c vs prior R2 head be24c5412. All three prior review threads resolved. Not a byte-clean rebase — genuine code delta on top of a moved base (5 behind), but all changes are semantics-preserving or strictly additive:

• useTimelineKeyboardActor.ts: viewportPageSize refactored from rows.filter(visibleTracks.has) to a useMemo-hoisted logicalRowCountByTrack: Map<physicalTrackKey, count> with a first..last loop. focusedRow = geometry.getRowIndex(focusedTrackKey) anchor preserved — focus-anchored paging invariant intact.
• useTimelineKeyboardActor.test.tsx: Enter-delegation test strengthened — now dispatches an explicit click() and asserts a native handler fires (previously only defaultPrevented === false).
• TimelineLanes.tsx: added aria-colcount={2} on the grid and aria-colindex={2} on the lanes column (treegrid completeness improvement); added inline comments codifying Family F element-identity keying ("Never fold in the map index") and multi-drag passenger invariant.
• Timeline.tsx / LayerDisclosureRow.tsx: additive invariant-documenting comments only.

Prior R2 findings still hold: actor gates expandable && expanded, ArrowLeft parent traversal at itemIndex < 0, rows.at(-1) for Ctrl+End, dataset.timelineFocusId Tab-bail gate, rovingTargetId-gated row tabIndex. Family F invariants preserved: element-identity keying (now commented), Fast-Refresh compliance (actor is .ts, tests strictly additive), no useEffect state-sync introduced, roving-tabindex-count=1 test unchanged.

Base is stack branch codex/studio-timeline-f-focus-coordination-v2 — the 8-check required-ruleset doesn't gate here; regression workflow SUCCESS, all completed checks green. mergeStateStatus: UNSTABLE reflects Graphite mergeability check still in progress. Not yet land-ready for main (needs retarget when the stack lands).

— Review by Via