feat(studio): expose timeline treegrid semantics - #2713

R1 adversarial review — treegrid semantics

The semantic contract is right and thoroughly tested. Roles compose validly (treegrid > rowgroup > row > rowheader / gridcell), aria-rowindex sequence is asserted end-to-end, aria-expanded polarity is correct (attribute omitted when expandable=false, so SRs won't misannounce leaves as "collapsed"). Identity module extraction to timelineNavigationIdentity.ts gives stable JSON-encoded ids that survive reorder (element-identity keying — good), and useTimelineLogicalRows cleanly memoizes the physical-track lookup. sr-only level-2 rows preserve the logical tree even when property lanes visually collapse into the sticky header.

Adversarial pass — items to consider as follow-ups, not blockers on a semantics-only slice:

• [nit] aria-multiselectable="true" without any aria-selected on rows. Per WAI-ARIA APG treegrid, SRs derive selectability presence from the attribute; without any row having aria-selected="false|true", JAWS/NVDA won't announce a coherent selection state model. Wire this when the selection-plumbing PR in this stack lands.
• [nit] No aria-colcount / aria-colindex. The layout is a stable 2-column shape (rowheader + gridcell); APG recommends both.
• [nit] Roving tabindex + key bindings not in this PR. Understood as scope carve-out (title says "expose ... semantics"), and timelineKeyboardNavigation.ts only picked up an identity-import rename here. Worth confirming the next-slice PR wires Arrow* / Home / End / * per APG on top of these semantics.
• [info] aria-owns on sr-only property rowheaders reparents visible widgets out of the level-1 rowheader into the level-2 row. Internally consistent, but atypical vs APG examples — a screen-reader user reaching the caret only via child property rows may be surprised. Documenting this in the design note would help future maintainers.

CI: Player perf / preview-regression / regression all SUCCESS at 3869327; UNSTABLE only because Graphite mergeability_check is IN_PROGRESS. No code smell in the diff.

— Review by Via

Reviewed at 3869327c.

Wires the treegrid semantics onto the model from #2712. Structural split: identity generators move out of timelineKeyboardNavigation.ts into timelineNavigationIdentity.ts (natural boundary; keeps the model pure and the identity conventions reusable). useTimelineLogicalRows hooks the model into React with logicalRowsByTrack lookup for per-track slicing. TimelineTrackRow gets logicalRow/propertyRows props, becomes an outer role="rowgroup" + inner role="row" + N sr-only property rows that use aria-owns to reach visible DOM in TimelinePropertyLanes / TimelineTrackHeader. role="list" → role="treegrid" on the outer container; aria-posinset/setsize → aria-rowindex/rowcount.

Two inline: (1) 🟠 per-instance id collision on the new timelineLogicalRowCellId — the two-timelines-on-one-page invariant that used to be enforced via lanesIdPrefix no longer covers the new aria-owns targets; (2) 🟡 the useMemo cache depends on 7 refs from the store.

Verified end-to-end:

• ✅ aria-rowcount computed from full logicalRows.length (5 in the 3-track/2-property fixture). ARIA-conformant.
• ✅ aria-rowindex uses logicalIndex + 1 (1-based) consistently across visible track row and sr-only property rows.
• ✅ aria-level is 1 for tracks, 2 for property rows — matches the model's level: 1 | 2 type constraint.
• ✅ aria-expanded={logicalRow.expandable ? logicalRow.expanded : undefined} — property rows and empty tracks get NO attribute (undefined skips in React 18+), tracks with lanes get true/false. Matches WAI-ARIA treegrid convention.
• ✅ role="rowgroup" is valid inside role="treegrid" per ARIA 1.2 (semantically comparable to <tbody>).
• ✅ role="rowheader" on the sticky track header + role="gridcell" on the track content. First-cell-as-row-header pattern is standard treegrid.
• ✅ New expandable field on TimelineLogicalRow — true iff lanes.length > 0; property rows and empty tracks are false. The expanded field implicitly requires expandable=true via the lanes.length > 0 gate.
• ✅ Test at Timeline.test.ts:263-269 asserts aria-owns attribute values match the referenced cells' ids — but the test doesn't validate both nodes mount together (see gap below).

PR-body promise audit:

• ✅ "Treegrid with stable row and cell identities" — treegrid role in place, ids stable across renders (JSON.stringify of stable inputs).
• ✅ "Screen readers get the same hierarchy sighted users see" — rowgroup > row > rowheader/gridcell hierarchy matches the visible sticky-header + content layout.
• ✅ "Complete logical row count stays independent of the mounted viewport" — aria-rowcount uses logicalRows.length, not the virtualized subset. Timeline.virtualization.test.tsx:206 asserts aria-rowcount=1000 on a 1000-row fixture with only ≤16 rendered. Good.
• ⚠ "One identity module owns track, property, clip, keyframe, and easing focus IDs" — mostly true, but timelineLogicalRowCellId uses a different format (${rowId}:${cell}) from the other five (JSON.stringify([...])). Inconsistent — the cell-id format doesn't survive a copy-paste consumer that expects JSON-arrays. Minor.

Nits (body-only):

• Ponytail comment loss at TimelineLanes.tsx:104 — the removed comment "Per-INSTANCE, so two timelines on one page (a mini-timeline in a modal beside the main one) cannot both mint …" named a specific failure mode; the new comment "One stable id namespace per timeline instance links sticky headers to canvas lanes" doesn't. Per [[project_hf_ponytail_comment_idiom]], this is exactly the kind of load-bearing why worth marking // ponytail: two timelines on one page…. Related to the inline concern about timelineLogicalRowCellId.
• No integration test that aria-owns targets mount concurrently — Timeline.test.ts:263-269 checks aria-owns attribute values match id strings, but doesn't call document.getElementById(aria-owns-value) to verify the target exists in the tree. A virtualized-out row could have a live aria-owns pointing at unmounted DOM without any test catching it. Cheap fix: assert both nodes present.
• Sr-only row content text at TimelineTrackRow.tsx:74 ({keyframeCount} keyframes, {easeCount} ease controls) is not localized. If HF supports non-English UI in the future, this string needs to route through the i18n boundary. Not this PR's problem, but worth naming.
• useTimelineLogicalRows prop shape uses Pick<TimelineLaneBaseProps, …> — good for source-of-truth typing, but couples this hook to changes in TimelineLaneBaseProps. If HF later renames a field, the hook silently updates too. Acceptable trade-off for the current single-consumer, worth naming if the hook picks up a second consumer.

Ready from my side once the id-collision concern lands, leaving as COMMENTED.

— Review by Rames D Jusso

Delta-reviewed 3869327c..0c5a3e17.

Nothing else surfaced in the delta. The body-nit observations from R1 (no test that aria-owns targets mount concurrently, sr-only i18n, Pick<TimelineLaneBaseProps> coupling) remain soft flags for the wiring PR — though the new TimelineLanes.test.tsx regression does verify aria-owns targets exist in the same host tree, so the first body nit is partially closed.

Ready from my side, leaving as COMMENTED.

— Review by Rames D Jusso

R2 delta re-review at 0c5a3e1 (vs R1 3869327).

Verified:

1. Per-instance namespace scoping — FIXED. TimelineLanes.tsx mints lanesIdPrefix = \timeline-lanes${useId().replaceAll(":", "")}`(React'suseId()guarantees per-instance uniqueness across concurrent mounts), then composeslanesId = `${lanesIdPrefix}-track-${row}`. Every logical-cell ID flows through timelineLogicalRowCellId(lanesId, rowId, cell)intimelineNavigationIdentity.ts, which prepends lanesIdbefore the stable["timeline","cell",…]JSON.aria-ownson bothrowheaderandgridcellinTimelineTrackRow.tsx` uses that helper, so no cell ID escapes the instance namespace.

2. Failure-mode comment — RESTORED. TimelineLanes.tsx carries // ponytail: One per-instance namespace prevents aria-controls and aria-owns from resolving into a second timeline that renders the same logical rows. above the lanesIdPrefix mint, documenting exactly the failure mode the fix prevents.

3. Two-timeline regression — DRIVES REAL COLLISION. New test "mints lane ids that do not collide with a second TimelineLanes on the page" mounts two independent renderLanes() roots, then asserts: (a) firstIds ∩ secondIds = ∅ for all aria-controls; (b) same for [data-property-group][id] cell IDs; (c) every aria-owns in each instance resolves ONLY into that instance's own cell-id set; (d) IDs remain valid CSS-id selectors. Not presence-only — dropping the useId() prefix would make both instances produce identical IDs and (a)-(c) would all fail.

Prior nits status:

• Nit-1 (aria-selected on rows): DEFERRED — belongs in selection-plumbing slice (grep confirms none added).
• Nit-2 (aria-colcount/aria-colindex): SKIPPED in this PR — worth a follow-up, non-blocking.
• Nit-3 (roving tabindex + Arrow/Home/End/*): DEFERRED — logical-navigation model is added in timelineKeyboardNavigation.ts (313L) but no tabIndex/onKeyDown on the treegrid yet; that's the wiring slice.
• Info (aria-owns reparenting): still present, now correctly per-instance scoped.

Nice work on the collision test — the aria-owns integrity assertion (per-instance ownership) is a bonus and would catch a partial-scoping mutation the ID-set check alone wouldn't.

— Review by Via

Delta re-verify at 415548272 against prior R2 head 0c5a3e170: genuine code changes, not pure rebase noise — and every prior-R2 finding is preserved plus one prior R1 nit is now addressed.

Prior findings preserved

• TimelineLanes still mints lanesIdPrefix from useId() (colon-stripped) and every cell id flows through timelineLogicalRowCellId(lanesId, rowId, cell) in timelineNavigationIdentity.ts.
• Failure-mode comment ponytail: One per-instance namespace prevents aria-controls and aria-owns from resolving into a second timeline... restored.
• Two-timeline regression test mounts two renderLanes() roots and asserts: no aria-controls intersection, no cell-id intersection, aria-owns resolves only into own cell-set, ids remain valid CSS selectors.

R1 nit progress at this HEAD

• aria-colcount={2} on the treegrid and aria-colindex on every rowheader/gridcell — with regression-test coverage. Previously logged as SKIPPED; now landed.
• aria-selected and roving tabindex/Arrow/Home/End/* still deferred to the selection-plumbing slice / #2715 as scoped.

Family F invariants preserved

• Element-identity keying el.key ?? el.id via getTimelineElementIdentity.
• No non-component exports from .tsx (identity module is .ts, hook is .ts).
• No useEffect state-syncing — useTimelineLogicalRows is useMemo on stable input refs.
• Test upgrades trade innerHTML presence checks for semantic visibleText + ariaLabels — assertions guard meaning, not markup.

CI: required regression SUCCESS at HEAD; base is stack branch codex/studio-timeline-f-logical-navigation-v2, so the other 7 required checks are correctly NOT_RUN and will fire on retarget. Graphite / mergeability_check still IN_PROGRESS is expected.

— Review by Via

Reviewed at 15ef84ca1 — post-retarget rebase onto main.

Delta from last-reviewed 415548272:

All 13 scope files byte-identical between 415548272 and 15ef84ca1. git patch-id --stable on the diffs matches: 8510b7a20ce142f9a08a35eb1f105ee252a8aedf.

Pure rebase, no content changes. Prior R1 resolutions (treegrid ARIA hardening with aria-colcount === "2" + aria-multiselectable absence + aria-colindex verification in TimelineLanes.test.tsx:270-284; CSS.escape comment on stableId in timelineNavigationIdentity.ts:8; extended docstring in useTimelineLogicalRows.ts:17) all stand. Nothing new to review. LGTM from my side — safe to re-stamp.

— Review by Rames D Jusso

Delta re-verify at retargeted HEAD 15ef84ca1 (base now main directly, post-#2712 merge).

Patch-equivalence: byte-for-byte identical to prior stack-branch stamp at 415548272. Verified via diff <(gh api compare/d247c9056...415548272 --jq patches) <(gh api compare/main...15ef84ca1 --jq patches) — no drift. Same 13 files, same additions/deletions per file, same commit-message header ("feat(studio): expose timeline treegrid semantics"). Clean rebase.

Prior R2 findings state (all preserved by construction):

• Per-instance lanesIdPrefix from useId() on TimelineLanes.
• Every cell ID via timelineLogicalRowCellId(lanesId, rowId, cell).
• "Ponytail" failure-mode comment on mint site.
• Two-timeline collision / aria-owns / CSS-id-legal regression test.
• Extracted useTimelineLogicalRows hook.
• ID identity split into timelineNavigationIdentity.ts.
• TimelineTrackRow rowgroup>row with rowheader/gridcell colindex.
• aria-colcount + aria-colindex with test coverage.
• expandable flag.

Family F invariants (el.key ?? el.id, deterministic tie-breaks, deletion fallback chain, no non-component exports from .tsx, no useEffect state-syncing): preserved (patch identical).

Required-checks rollup (HF ruleset 14211637, 8 required):

• Build: IN_PROGRESS
• Render on windows-latest: IN_PROGRESS
• Semantic PR title: SUCCESS
• Test: IN_PROGRESS
• Test: runtime contract: SUCCESS
• Tests on windows-latest: IN_PROGRESS
• Typecheck: IN_PROGRESS
• regression: SUCCESS

3/8 SUCCESS, 5/8 IN_PROGRESS, 0/8 FAILURE. mergeStateStatus: BLOCKED (CI still running).

APPROVE — patch-equivalent to prior stamp, no drift, no regressions. Land-ready pending the 5 in-flight required checks turning green.

— Review by Via