feat(studio): coordinate logical timeline focus - #2714

R1 review — coordinate logical timeline focus @ b75399c782

Verdict: APPROVE. Family F next slice is coherent: the reveal/focus split is folded into a single coordinator keyed by a logical-model ID, DOM nodes advertise their target via data-timeline-focus-id, and the deletion-fallback chain (resolveTimelineFocusFallback) is reused instead of duplicated. The scope guard (projectId + sessionEpoch) inside isCurrentRequest and beginTimelineSession clearing focus in createTimelineResetState() close the project/session-change axis. Deferred parity confirmed: useTimelineRevealClip.{ts,test.tsx} deleted, no remaining callers of requestClipReveal at HEAD, and both sidebar entry points (AssetCard, AudioRow) migrated.

Axis 1 — Focus stability across virtualization unmount/remount

• Focus is stored in the logical model (timelineFocus.id), not in document.activeElement. Remount consults logicalRows + data-timeline-focus-id — no reliance on DOM identity across unmount.
• Effect runs after commit (useEffect, not layout); reveal-then-focus ordering preserved (scrollToTarget before focusElement in useTimelineFocusCoordinator.ts:130-146).
• Roving-tabindex invariant holds: TimelineTrackRow.tsx:53, TimelineClip.tsx:93, and TimelineTrackHeader.tsx:225 all set tabIndex={-1} — no candidate advertises tabIndex={0}, so .focus({preventScroll: true}) is the only path in.
• Between reveal-scroll and focus, if virtualization unmounts the target, focusElement returns false and appliedRef stays unset → the effect fires again next render. Self-healing.

Axis 2 — Reveal/focus handoff via the logical model

• Model-first resolve → locateTimelineLogicalTarget on the current row set; if absent, resolveTimelineFocusFallback(previousRows, nextRows, id) walks item-backward → forward → surviving-row → parent → row-index. Result is rewritten via requestTimelineFocus(fallback.id) (useTimelineFocusCoordinator.ts:114-117); the two-tick loop terminates once resolution matches the new request id.
• Offscreen target: coordinator computes scroll geometry from the model (element index → rowGeometry.getRowTop) without needing the target in the DOM. Scroll is issued, mount follows on the next render, focus lands then.
• Scroll direct-assign (no behavior: 'smooth') — no focus-vs-settle race.

Axis 3 — Regression tests

• useTimelineFocusCoordinator.test.tsx covers: pin-then-mount, latest-request-wins, stale-scope, and parent-row fallback on delete — all four assert behavior, not internals.
• Timeline.virtualization.test.tsx:288-296 drives the full store flow (requestTimelineFocus(timelineClipFocusId(...))) and asserts document.activeElement === focusedClip after remount, i.e. real focus lands post-virtualization, not just a flag.
• playerStore.test.ts exercises project rebinding via beginTimelineSession("project-b") and asserts timelineFocus === null — session-change tear-down verified.

Nit

• focusElement (useTimelineFocusCoordinator.ts:33-45) enumerates every [data-timeline-focus-id] under the scroll container and .find()s by dataset value. Fine at current row counts, but for large timelines querySelector with CSS.escape on the attribute value would drop it from O(n) to O(1) — same pattern the old useTimelineRevealClip used. Optional follow-up, not blocking.

— Review by Via

Reviewed at b75399c7.

Substantial refactor — 666+/546- across 24 files replacing the old useTimelineRevealClip clip-only reveal path (180 lines + 139-line test) with a single useTimelineFocusCoordinator that unifies logical target resolution, deterministic fallback, two-axis reveal, and DOM focus. Store slice reshapes accordingly (clipRevealRequest → timelineFocus with projectId/sessionEpoch/nonce), sidebar callsites migrated (requestClipReveal(id) → requestTimelineFocus(timelineClipFocusId(id))), and TimelineTrackRow / TimelineTrackHeader / TimelineClip all gain data-timeline-focus-id for the coordinator's DOM lookup. Model-first design: mounting is a consequence of the returned pins, not a precondition to focus.

Three inline concerns, all 🟡 — none blocking.

Verified end-to-end:

• ✅ Wire symmetry: sidebar's requestTimelineFocus(timelineClipFocusId(clipKey)) uses the SAME identity generator that TimelineClip.tsx:93 uses to emit data-timeline-focus-id. focusElement finds the clip via that attribute. No two-side-of-the-wire drift.
• ✅ Stale-scope defense: TimelineFocusRequest carries projectId + sessionEpoch. isCurrentRequest guard filters mid-flight requests against the current scope. Test at useTimelineFocusCoordinator.test.tsx:115-127 proves external DOM targets in wrong scope are never focused.
• ✅ Nonce idempotency: clearTimelineFocus(nonce) is a compare-and-clear (s.timelineFocus?.nonce === nonce ? {timelineFocus: null} : s). A stale clear over a newer request is a no-op. Test at playerStore.test.ts:521-524 proves.
• ✅ Fallback rewrite: when locate misses and fallback resolves to a different id, coordinator dispatches requestTimelineFocus(fallbackId) which bumps nonce; next render's resolvedRef.current?.nonce !== request.nonce triggers reset. One hop, converges. Test at :129-137 verifies parent-row fallback when a clip disappears.
• ✅ Cross-instance safety: container.querySelectorAll scoped to scrollRef.current. data-timeline-focus-id values may collide across instances (same clip id), but the query never leaves its own container. No aria-owns-style DOM-invariant violation (that's #2713's concern, already resolved).
• ✅ Two-axis reveal: computeRevealScroll handles sticky-header offsets (stickyLeft: contentOrigin, stickyTop: RULER_H). For row-only targets, allowHorizontal: allowHorizontal && left !== null correctly disables horizontal scroll.
• ✅ logicalRow.elementId added to the row model with cardinality-correct: null for track rows, activeId for property rows. Coordinator's pinnedElementId derivation from a row target uses resolution.row.elementId ?? undefined — correct for both.
• ✅ Model consolidation: useTimelineLogicalRows was duplicated across Timeline.tsx and TimelineLanes.tsx; now single-source at Timeline (via useTimelineLogicalFocus) and passed down as a prop. logicalRowsByTrack derived locally in TimelineLanes via a targeted useMemo([logicalRows]). No double-computation.

PR-body promise audit:

• ✅ "One coordinator resolves the logical target, chooses a stable fallback when needed" — useTimelineFocusCoordinator + resolveTimelineFocusFallback.
• ✅ "Pins the relevant row and clip" — focusedRowKey → useTimelineRowVirtualization.pinnedRowKeys; pinnedElementId → useTimelineClipRenderWindow.pinnedClipIdentities.
• ✅ "Reveals it on both axes" — scrollToTarget via computeRevealScroll, allowHorizontal respected.
• ✅ "Applies DOM focus once the target mounts" — focusElement with data-reveal-highlight marker, .focus({preventScroll: true}), activeElement re-check for verify-then-highlight semantics.
• ✅ "Replaces the older clip-only reveal path" — useTimelineRevealClip.ts + .test.tsx fully deleted.

Nits (body-only):

• Coverage gap — depth-2+ fallback chains. Test at useTimelineFocusCoordinator.test.tsx:129-137 covers "focused clip disappears → fallback to parent row." Doesn't cover "focused clip disappears AND parent row also disappears" (e.g., track removed while a clip was focused). The underlying resolveTimelineFocusFallback chain handles this per #2712's tests, but the coordinator's wiring of requestTimelineFocus(fallbackId) isn't exercised for the deeper case.
• resolvedRef.current.id = fallback.id mutation during render at useTimelineFocusCoordinator.ts:144 — legal per React docs (refs aren't state), and stable under StrictMode double-invocation because the nonce !== request.nonce check is the memoization gate. Non-obvious pattern — worth a ponytail: marker per [[project_hf_ponytail_comment_idiom]] naming why the ref is reassigned mid-render (memo cache keyed on request nonce, not state).
• useTimelineLogicalRows still carries 7 store deps — a concern I raised on #2713 R1 (🟡) still stands, though it's out-of-scope for this PR. The wiring PR that closes the focus loop end-to-end (or a follow-up perf pass) should verify each upstream selector returns a stable ref; unmemoized Map/Set/Array construction on any of those seven selectors would rebuild the model every render and cascade through the coordinator's useLayoutEffect.
• data-timeline-focus-id attribute contract — a new DOM attribute contract now exists (coordinator-side query key). If the team ever adds another focus-adjacent mechanism, they'll want to check for name collision. Consider a packages/studio/src/player/components/README.md (or CLAUDE.md entry) naming the attribute as reserved for the focus coordinator.
• focusElement remove-highlight blur listener at :62-64 uses {once: true}. If the element is programmatically re-focused later (e.g., user tabs away and back), the highlight already disappeared on the first blur — no restore. Semantically correct for "one-shot reveal highlight," worth naming.

Clean refactor. Ready from my side, leaving as COMMENTED.

— Review by Rames D Jusso

Delta re-verify at 21b2c9149 — approving (was already approved at b75399c782, this stamp confirms the delta).

Genuine code drift (not rebase noise) — Miguel addressed rames-d-jusso's 3 nits:

• useTimelineFocusCoordinator.ts — resolution wrapped in useMemo keyed on [logicalRows, projectId, request, sessionEpoch], so the apply-focus effect no longer fires on every render; focusElement swapped from O(N) .find() to O(1) container.querySelector with CSS.escape(targetId) (this is the exact improvement I flagged as a nit in R1).
• useTimelineRowVirtualization.ts — removed the native focusin/focusout useEffect (which stuck to the dead scroll node on remount); now returns timelineFocusProps = { onFocus, onBlur } via useCallback, spread onto the scroll container in Timeline.tsx so React's synthetic-event system handles remounts.
• TimelineLanes.tsx — dropped aria-multiselectable="true", added aria-colcount={2} / aria-colindex={2} (single-select widget correctly labeled).

Prior R1 findings state — all still hold:

• Focus lives in timelineFocus.id; TimelineTrackRow / TimelineClip / PropertyGroupHeaderRow still tabIndex={-1}; deletion fallback chain preserved (now inside useMemo, semantics unchanged); reveal-then-focus ordering and direct scrollLeft assign intact; useTimelineRevealClip.{ts,test.tsx} remain deleted with no callers.

Test coverage strengthened:

• New "does not retry an unchanged unmounted target on an unrelated render" (spies scroll.querySelector to prove memo elides redundant work).
• New "persists the next surviving row when both a clip and its parent track disappear" (deletion fallback → parent-row → surviving-row).
• New Timeline.virtualization.test.tsx case "keeps focus pinning active after the scroll viewport remounts" — directly exercises the fix for the stale-listener regression.

Family F invariants preserved: element-identity keying, deterministic tie-breaks, deletion fallback, no non-component exports from .tsx, no useEffect state-syncing (the removed effect was DOM-subscription; its replacement is pure synthetic events).

Memo safety: mutation of resolvedRef.current inside useMemo is idempotent under Strict-Mode double-invocation (nonce-keyed early-continue), and previousRowsRef.current is still populated in the useLayoutEffect that runs after commit — fallback chain still sees the prior-render rows.

CI: base is stack branch codex/studio-timeline-f-treegrid-v2 — 7 required checks NOT_RUN (path/base filter, expected); regression SUCCESS. Land-ready via Graphite retarget to main.

— Review by Via