refactor(studio): extract timeline render contracts - #2699
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
73d1bb5 to
ecf76d7
Compare
5e02427 to
c327ee5
Compare
ecf76d7 to
a69af1c
Compare
c327ee5 to
d9d7106
Compare
a69af1c to
832f7b3
Compare
d9d7106 to
a224fdc
Compare
832f7b3 to
57aa9f0
Compare
a224fdc to
a289615
Compare
57aa9f0 to
6f4ff16
Compare
a289615 to
da1c1be
Compare
6f4ff16 to
c41fac2
Compare
da1c1be to
2913f87
Compare
c41fac2 to
42579a9
Compare
4089ed6 to
29b22a9
Compare
29b22a9 to
4edbc70
Compare
b4d9414 to
4ddc5bd
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — grade B+ — rubric CORRECT
Role in stack: D4 refactor — pre-virtualization render-contract extraction. Stacks cleanly onto D3.
Root-cause fit: The extraction gives D5-D8 the seams they need: ticks (useTimelineTicks/timelineRulerGeometry), selection lifecycle (useTimelineSelectionLifecycle), shift modifier, preview-element derivation (getTimelinePreviewElement), duration & keyframe predicates (timelineViewModel), and the player-sync equality check (timelinePlayerSync). Row-height / thumbnail-request / context-menu-target contracts are still baked into the orchestrator — D4 explicitly scopes to ruler/selection/modifier/view-model, deferring is fine so long as D5 exposes row measurement without a re-extraction round.
Claims verified (file:line at HEAD 4edbc701):
Timeline.tsx+21/-71:selectedElement/selectedElementRef/shiftHeld-effect/tick-useMemo/getPreviewElementbodies removed; only local consumers, no dangling refs.timelineLayout.ts-118/+2: tick logic moved out; re-exportsformatTimelineTickLabel/generateTicksfrom ruler module — no import-path breakage.- New files are
.ts(hooks/helpers only, no JSX) — Fast Refresh compliant. - Named exports only,
import typeon pure-type imports. - No
any, no@ts-ignore, no disabled tests, no new TODOs. Pre-existingeslint-disable react-hooks/exhaustive-depsmoved verbatim. - Bit-identical output:
hasKeyframedTimelineClips,getEffectiveTimelineDuration,getTimelinePreviewElement, tick math,timelineElementsChangedline-for-line equivalent (identifier renames only).
Adversarial findings:
- Nit — element-identity duplication: the
element.key ?? element.idfallback is inlined ingetTimelinePreviewElementanduseTimelineSelectionLifecycle. D5-D8 will need the same fallback for keying — worth a one-linertimelineElementIdentity(el)helper intimelineViewModel.tsnow. - Nit — narrowed union:
useTimelineTicksacceptstimeDisplayMode: "time" | "frame"inline instead of importing the store type; drifts silently if playerStore extends the mode. - Nit — doc erosion:
timelineRulerGeometry.tsdrops the "Nice NLE steps" / "frame-labels-must-be-whole-frames" explanatory comments — the load-bearing why for these two functions. - Nit — indirection:
useTimelineTicksimportsgenerateTicksfrom./timelineLayout(which re-exports), not from source./timelineRulerGeometry.
CI state: Required checks green.
Suggested next step: Land as-is. Address the element-identity helper in D5 where row/clip windows will re-introduce the key ?? id fallback, and consider a follow-up to restore the tick-generation docstrings.
— Review by Via
4ddc5bd to
6958e61
Compare
4edbc70 to
2b89498
Compare
|
Addressed at
|
|
Reviewed the exact #2699 delta (6958e61..2b89498), not the rest of the stack.\n\nThe refactor is clean: ruler geometry, view-model derivation, selection lifecycle, Shift tracking, tick generation, and player-sync decisions move behind narrow modules without changing their algorithms. The current head also resolves the prior review drift: it reuses getTimelineElementIdentity, shares TimelineTimeDisplayMode, restores the semantic comments, and imports ruler geometry directly instead of through the Timeline re-export. Window listeners in the new Shift hook are paired with cleanup, and I found no new import cycle or production side effect.\n\nCoverage remains primarily behavioral through the existing Timeline/player tests rather than unit tests for every extracted pure helper; that is acceptable for this mechanical delta. Exact-head lightweight checks are green/skipped as expected, with Graphite mergeability pending on the downstack chain. It must wait for #2697s blocker, but this delta itself is approvable.\n\nGitHub will not let the shared author identity submit a formal approval, so recording the verdict as a PR comment.\n\nVerdict: APPROVE\nReasoning: The render-contract extraction preserves behavior, improves ownership boundaries, and introduces no lifecycle or dependency regression.\n\n— Magi |
2b89498 to
f5cd679
Compare
9b9e15e to
8c1dd61
Compare
f5cd679 to
60e3c4f
Compare
8c1dd61 to
dd56d5b
Compare
60e3c4f to
3f74418
Compare
3f74418 to
f8cd5e9
Compare
dd56d5b to
6b92049
Compare

What
Extract timeline rendering contracts and orchestration helpers needed by viewport virtualization.
Why
The original Timeline component mixed view-model construction, lane contracts, tick generation, selection lifecycle, modifier state, and player synchronization. Virtualization needs those decisions to have explicit owners without introducing parallel geometry or state paths.
How
This is D4 of the Family D draft review sequence. It targets current main after Family C landed; its Family D patch was replayed without semantic changes. It supersedes legacy PRs #2635 and #2636, which remain open for audit until landing.
Test plan
Validated on the exact Family D tip through Timeline and timeline-layout coverage, the full Studio suite, both package typechecks, formatting, lint, file-size checks, and the strict 50,000-element Chromium gate.
2026-07-29 rebase verification
No merge was performed; this PR remains a draft.