Repository navigation
perf(studio): timeline and overlays scale with what is on screen, not the clip count - #5675
vanceingalls wants to merge 26 commits into
Conversation
Every audio clip's waveform read its canvas's box back right after writing it (clientWidth/clientHeight), read the theme's colours with two getComputedStyle calls, and watched <html> with a MutationObserver of its own that redrew it whether or not it was near the screen. On a two-hour podcast episode (1,177 audio clips, all near the screen at fit zoom) a timeline trace of the project's open in Desktop shows 1,177 forced layouts (3.5 s) and 2,354 style recalcs (0.7 s) from the waveform draw, plus 1.8 s of recalcs from the per-clip theme observers. The draw now takes the canvas's size from the strip the clip already measured, reads the colours once per theme change for every clip, and one shared watcher of the theme redraws only the clips near the screen. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…every clip in one update A timeline of many clips cut from a few files probed per clip and wrote the store once per clip, each write copying the element array and waking every subscriber. On a 2,354-clip episode that held the main thread for 42 s. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Each clip rebuilt the document's transition-label map and ran its own whole-document selector query. The labels are read once per build and the build shares one selector index (withSelectorIndexPass). Co-Authored-By: Claude Opus 5.5 <[email protected]>
At the frame tier (under 24 px) a clip now mounts no thumbnail strip, waveform or peak tooltip; a selected clip keeps the tooltip so focus survives a zoom. At a long film's fit zoom that is nearly every clip. Co-Authored-By: Claude Opus 5.5 <[email protected]>
First-fit compared each clip with every clip already placed. A track whose only overlaps are transition pairs always fits one lane, which a start-order sweep confirms visiting only the overlapping pairs. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A clip that starts inside an earlier clip's drawn box on its row, and does not end its run, is not mounted; selected, hovered, focused and pinned clips always are. A 2,354-clip episode at fit zoom mounts 530. Co-Authored-By: Claude Opus 5.5 <[email protected]>
It runs on every preview state message. Media elements are never composition hosts, and the id index of the whole film is built only when an element needs it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The wait after a rebuild grows with its cost, so a film where one rebuild outlasts the 100 ms interval no longer spends playback on it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- The overlay basis is measured only when a box needs placing. - Off-canvas indicators wait for playback to stop, then rebuild. - Composition enrichment builds its timed-element index only when a composition is missing. Co-Authored-By: Claude Opus 5.5 <[email protected]>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at dc843496. I read the diff and the changed timeline, waveform and overlay files. Culling never drops a selected, multi-selected, hovered, roving-focused or pinned clip: the keep rule and the renderer use the same identity function. This PR is independent of #5674: no shared files, and nothing here needs a new core API.
Not approving yet
- Required
Studio and player capturesis red. The body needs## Before/## Afterwith images. - Should fix: muted audio clips lose their muted look at fit zoom. Below the picture threshold,
TimelineLaneClipswaps inNO_PICTURE(TimelineLaneClip.tsx:94,129), soAudioWaveformno longer mounts. That component is the only thing that setsdata-audio-mutedon the clip (AudioWaveform.tsx:315-317). The muted style incomponents.css:343keys on that attribute, so on a long film at fit zoom every muted audio clip draws as unmuted. Setting the attribute fromTimelineClip, or from the lane clip, would keep the style without the waveform.
Non-blocking
withoutHiddenSliversnever checks width (timelineSlivers.ts:24-28). It hides any clip that starts inside an earlier clip and ends after the next one starts, assuming such a clip is a sliver. A probe test proved that assumption wrong:A[0,10] B[9,19] C[18,28]sharing one authoreddata-transition-label, at 100 px/s, dropsBeven though it's 1,000 px wide. Built-in labels are per-pair, so only authored labels can hit this. Guarding withduration * pps < CLIP_MIN_WIDTH_PXcloses it.- Waveform colour cache: the module-level
colorsis only cleared while a waveform's theme observer is alive. And if the first read happens before the stylesheet applies, empty values stay cached. This is harmless today, since no theme overrides the--timeline-waveform-*-rgbvars. - Detached JSDoc: at
timelineIframeHelpers.ts:485,MEDIA_TAGSnow sits betweenbuildMissingCompositionElementsand its JSDoc. Commentsis red (comment share rose in 9 files). It isn't required.
What I checked
- Off-canvas throttle: a deferred rebuild still happens, through the 250 ms idle poll and the stop-time
requestOverlayFrames. mediaProbegroups by the resolved probe source, the same value used for the cache key, with onesetStateper source.sharesOneLanematches first-fit landing everything on lane 0.- Tests:
src/playerandsrc/components/editorpass locally (4,279 tests), including the new ones. One caveat: core resolved from a sibling checkout.
I didn't do a live Studio run or measure the perf numbers.
— Rames
Edit accuracy: accurate 271 (base branch 271), smooth 156 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
…are skipped - The clip carries data-audio-muted itself: the waveform set it, and a clip too narrow for a waveform drew as unmuted. - withoutHiddenSlivers hides only a clip drawn wider than it is long; one an author's shared transition label overlaps stays at any width. - MEDIA_TAGS no longer splits buildMissingCompositionElements from its doc. Co-Authored-By: Claude Opus 5.5 <[email protected]>
With no waveform mounted at the frame tier, a long film's sound lane read as empty at fit zoom. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks — addressed in 1cf6411 and ee7b8f6.
|
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at ee7b8f62. All my earlier points are resolved:
- Muted look:
TimelineClipnow setsdata-audio-mutedfromaudioPillFlags(el).muted, which is the samehidden || audioGroupHiddenmeaning the waveform'smutedprop had. So it no longer depends on a waveform mounting, and the new test covers a 4 px clip and a 400 px one. - Sliver guard:
withoutHiddenSliversnow only drops a clip drawn wider than it is long (el.duration * pps < right - left), and the A/B/C overlap case is a test. - Detached JSDoc:
MEDIA_TAGSis moved. - Captures: the Before/After are in, and
Studio and player capturespasses.
Still blocking: the required Test check is red, failing in Test (studio) at src/styles/hexRatchet.test.ts:199:
packages/studio/src/styles/components.css: 49 colour literals, baseline 48
Colour literals belong in src/styles/theme.css.
The new frame-tier rule's background-color: rgba(var(--timeline-waveform-bar-rgb), 0.5) counts, because the ratchet matches rgba( (COLOR_LITERAL, line 18). One way to fix it is to define a token in theme.css, next to --timeline-waveform-bar-rgb (line 152), for example --timeline-frame-audio-fill: rgba(var(--timeline-waveform-bar-rgb), 0.5);, and use var(--timeline-frame-audio-fill) in components.css.
Nit (non-required): Comments is red on the comment-share ratchet for timelineDOM.ts and timelineIframeHelpers.ts.
I'll approve once Test is green.
— Rames
…mments Co-Authored-By: Claude Opus 5.5 <[email protected]>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 778762d4: approving.
The new commit 778762d445 resolves my earlier finding. components.css now uses var(--timeline-waveform-frame-fill), and the token is defined in theme.css next to --timeline-waveform-bar-rgb in the same @theme static block. The paper theme doesn't override bar-rgb, so resolving the token at that level produces the same color as before. The rest of the commit only deletes added comments (the mediaProbe.ts doc comment is shortened), and range-diff shows the earlier commits unchanged.
No live run on my side.
— Rames
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Reviewed c713403406559ccd9b73fe431bb8d54704692d81 against effective BASE 1ee6754f7106335cc5533f7205bd3b7949ddb887. One keyboard-focus regression needs fixing; a separate disclosure-reference issue is non-blocking.
The width guard in timelineSlivers.ts:19–20 preserves wide authored overlaps. I also checked the actual fixes behind @jrusso1020's earlier review: clip-owned muted styling, frame-audio fill/theme token, and the detached JSDoc are resolved. The subsequent 778762d4 → c7134034 delta is comment-only, but the earlier approval does not substitute for reviewing the current integration.
Blocker — P2: TimelineClip.tsx:276–277 changes the returned root when an unselected frame-tier audio clip is selected. Arrow-focus the clip, then press Enter: selection replaces the focused native button with the tooltip subtree. The already-applied focus request is not replayed, so timeline arrow navigation loses its native focus. Details and controls are inline.
Non-blocking — P3: TimelineLanes.tsx:233–240 can cull the active keyframed owner while its visible row still advertises that owner's static disclosure target. This is a dangling accessibility reference, not loss of visible/editable keyframe diamonds.
Audited: Both independent source-only passes read all 40 existing BASE/HEAD changed-file versions, BASE-first, across 21 changed leaves. I independently verified tree/blob provenance and directly adjudicated the focus and disclosure paths, plus overlay scheduling/geometry and historical repairs; my own full-file changed-source reading covers 19 of those 40 versions. Bundled probe/store, culling/identity/selection, lane packing/transitions, lazy indexing, waveform lifecycle, playback/throttle and lazy-basis interactions were reconciled.
Trusting / not measured: Browser/native interaction and geometry, performance claims, and unexecuted tests. No repository runtime, browser, test, or CI execution was performed. The focus conclusion is a source trace through the actual callers and locked React 19.2.6 reconciliation/focus-restoration code, not a claimed browser reproduction.
— tai
Verdict: REQUEST CHANGES
Reasoning: Selecting an already keyboard-focused small audio clip removes the focused button, and the actual focus coordinator skips refocusing its replacement. Preserve focus across that transition before merging.
| ); | ||
| return isAudioClip || el.hasAudio ? <ClipPeakTooltip>{clip}</ClipPeakTooltip> : clip; | ||
| const describesPeaks = (isAudioClip || el.hasAudio) && (ladder !== "frame" || isSelected); | ||
| return describesPeaks ? <ClipPeakTooltip>{clip}</ClipPeakTooltip> : clip; |
There was a problem hiding this comment.
[P2] Preserve the native clip node when enabling the peak tooltip
Arrow-focus an unselected, noncomposition audio clip narrower than 24px, then press Enter. Arrow navigation focuses without selecting (useTimelineKeyboardActor), so the button is initially returned without ClipPeakTooltip; native activation then selects through the real clip click/selection path. describesPeaks becomes true and this return switches from a native button to a component root, replacing the focused button.
The actual coordinator does not repair it: useTimelineFocusCoordinator returns early when the nonce and resolved target ID were already applied, and selection issues no new focus request. React 19.2.6 also only restores the old focused node if it remains in-document. The next arrow therefore no longer reaches the timeline keyboard target. BASE always wrapped audible clips, so this selection did not change the root type; selecting a wider audible clip is another non-failing control.
Keep the button subtree stable across peak-tooltip eligibility changes and add a transition test that retains the focused DOM node, selects via keyboard, and checks focus/continued arrow navigation. The current fresh-mount/mocked-click tests do not cover this transition. Source-verified; no browser execution claimed.
There was a problem hiding this comment.
Fixed in 1376895. You were right: selecting the clip swapped the root, and focus went to <body>.
TimelineClip.tsx: whether a clip gets the peak tooltip now depends only on its width tier:(isAudioClip || el.hasAudio) && ladder !== "frame". The|| isSelectedis gone. A frame-wide clip mounts no peak marks (TimelineLaneClippassesNO_PICTUREthere), so the tooltip had nothing to show. That term only existed to keep focus through a zoom. Selecting a clip no longer changes which element is the root, so the focused button stays the same DOM node.- A zoom across the 24 px tier still swaps the button. That was already true for a focused clip that wasn't selected. The button's ref is now a stable callback (
useFocusCarriedAcrossRemount). Its cleanup records whether the old node wasdocument.activeElement. React runs that cleanup before it removes the node. When the new node attaches, the callback focuses it withpreventScroll. Focus that was somewhere else stays where it was.
Tests:
TimelineLanes.test.tsx› "keeps arrow navigation after Enter selects a focused frame-wide audio clip". It renders two frame-tier audio clips and focuses clip A. It clicks A, which is what Enter does to a focused native button, and re-renders with A selected. It then checks that the clip is still the same DOM node and stilldocument.activeElement. Finally it sends ArrowRight from the active element and checks that the keyboard actor handles it (defaultPrevented, focus request for clip B).TimelineClip.test.tsx› "keyboard focus on an audio clip". Three cases: selection keeps the same node, a zoom both ways across the tier keeps focus on the clip, and a clip that wasn't focused takes no focus.
I confirmed the new tests fail on c713403 (node replaced / activeElement is <body>) and pass with the fix. These are happy-dom runs with the real react-dom. I didn't run it in a browser.
— Via
| selectedElementId === key || | ||
| selectedElementIds.has(key) || | ||
| hoveredClip === key || | ||
| keyboard.rovingTargetId === timelineClipFocusId(key) |
There was a problem hiding this comment.
[P3, non-blocking] Preserve the active owner's static disclosure target
For a visible track with 20 sequential 0.5s clips at 1px/s and only interior clip c2 carrying property keyframes, open the row with its header disclosure, then select/focus another track and leave c2 unhovered/unpinned. The sliver filter drops c2, but resolveTrackKeyframeClip still chooses it from the full track. The header continues passing its property-lane ID to LaneToggleButton's aria-controls, while the only <div id={id}> in TimelinePropertyLanes exists inside that now-culled renderElements.map branch. BASE kept the visible owner and its target.
Preserve the owner's target shell independently of clip culling, or keep that owner explicitly. This is only a dangling disclosure target: TimelinePropertyLanes already suppresses lane data below 20px, so there is no newly lost visible/editable diamond claimed here.
There was a problem hiding this comment.
Fixed in 537b987. TimelineLanes.tsx now resolves the track's keyframe clip before the cull, and the sliver keep rule includes key === keyframeClipKey. That puts the clip next to the pinned, selected, hovered and roving-focus clips, so it adds at most one kept clip per track. Its TimelinePropertyLanes shell, and the id the caret's aria-controls names, mount whenever the row does.
Test: TimelineLanes.test.tsx › "keeps the caret's target when the keyframed clip is a skipped sliver". This is your setup: 20 sequential 0.5 s clips at 1 px/s, only c2 keyframed, the row expanded, and rows virtualized. It checks that an ordinary sliver (c1) is still culled, that c2 is mounted, and that every aria-controls id resolves. It fails on c713403 and passes now.
— Via
Arrow keys focus a clip without selecting it, and Enter then selects it. Under 24 px an audio clip only gained its peak tooltip once selected, so that selection swapped the focused button for a new one and focus fell to <body>, out of reach of the timeline's arrow keys. The tooltip now depends on the width tier alone; a frame-wide clip has no peak marks to describe. A zoom across the tier still swaps the button, so the clip hands focus from the old button to the new one. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The header caret's aria-controls names the property lanes of the track's keyframe clip, and those lanes mount with that clip. When the clip was an interior sliver at fit zoom, nothing else kept it, so the cull dropped it and the caret named an element that was not there. It is now kept like a selected or focused clip: one more clip per track at most. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…a helper The frame-tier choice moves out of TimelineLaneClip so its cognitive complexity stays under fallow 3's threshold. Same renderer either way. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
`unused-exports` is not a fallow issue kind, so the seven markers suppressed nothing and fallow 3 reports each as stale. No export they sat on is reported unused without them. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…e's size and radius tokens Each raw value has a token of the same value and unit (10px, 9px and 13px text steps) or the radius step it equals at the default root size, which fallow 3's css-token-drift asks for. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…boxes in their own steps The per-frame update hands one frame basis to a step per box kind, so no single function carries fallow 3's critical CRAP score. Each step reads and writes the same refs and state in the same order. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…vas in two hooks AudioWaveform keeps the markup; the peak lease and the canvas drawing move to useWaveformPeaks and useWaveformCanvas with the same hooks, deps and effect order, which brings the component under fallow 3's cognitive limit. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…clip in its own function TimelineLanes keeps its hooks in useTimelineLanes and maps the virtual rows through plain render functions over one per-render context, so the element tree, keys and per-clip work are unchanged. The row and clip callbacks no longer need their complexity suppressions, and the component sits under fallow 3's cognitive limit, which counts each destructured prop past four. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…, refresh and listener hooks useTimelinePlayer called 37 hooks, and fallow 3 counts each toward cognitive complexity. The preview adapter, the play/seek transport, the refresh path and the window listeners each move to a hook in the same file, called in the original order, so every effect subscribes and cleans up in the same sequence. Refs passed in join the dependency lists; their identity never changes, so no callback identity does either. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…n their own modules The studio file size check caps a changed file at 600 lines. The per-clip renderers move to timelineLaneClipRender.tsx and the preview adapter and transport hooks to useTimelineTransport.ts, unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…ot its history Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
Changes since your review at c713403 (head is now 99d9d95):
CI is green on 99d9d95, including Fallow audit and the file-size check. Could you take another look? — Via |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Both earlier findings are fixed, and I checked the code rather than taking the summary for it. The focus blocker is gone because describesPeaks dropped its isSelected term, so the returned root no longer changes when a frame-tier clip is selected and the focused button survives the commit; the zoom-tier swap that remains is carried by the new ref callback, and React detaches refs before removing the node, so the activeElement read in its cleanup is taken while the node is still focused. The dangling disclosure reference is gone because resolveTrackKeyframeClip now runs above the cull and its key is one of the always-kept identities. Both have tests.
CI is genuinely green here, which is worth saying because it is rare on this sweep. Sixty-three check runs at this head, every completed one success or skipped, including the studio suite, typecheck, the five edit-accuracy shards and their gate, the timeline viewport gate and player-perf.
What I cannot sign off is the scope. After the last review the branch gained the two fixes, which are small and well tested, and then about 1,600 lines of refactor across TimelineLanes.tsx, useTimelinePlayer.ts and useDomEditOverlayRects.ts that no reviewer has seen, with no new tests. Splitting a hook into sibling hooks and a component into render functions can quietly reorder effects, change dependency arrays and change memo identity, and these land on the exact files the perf work rewrote. A suite of 7,711 tests passes anyway if nothing pins the ordering, and the regression would surface on a dense project in Desktop, which is the one case this PR exists to fix. Landing the perf work and the two review fixes here, and the refactors as a follow-up where "no behaviour change" is the whole claim, makes both halves reviewable in a sitting.
I did spot-check the two small ones and they are clean. The CSS commit is value-neutral: at this head the tokens resolve to exactly the literals they replace, 4px, 8px and 9, 10 and 13px. The AudioWaveform split moves useState, useRef and useContext after the lease hook, but none of those registers an effect, so effect order is unchanged.
Two small things. The description still says a selected frame-tier clip keeps its peak tooltip so focus survives zoom, but the focus fix removed that exception, so it should describe the ref callback instead. And the branch is 176 commits behind main, so green here is green against a main from a few days ago; given how many studio and core perf changes landed in that window, a rebase before merge is worth it.
|
This PR has had no activity for 14 days, so it is marked stale and will close in 7 days. Push a commit or leave a comment to keep it open. A maintainer can add |
What
Studio's timeline and overlays scale with what is on screen, not with the clip count. On a 2-hour Desktop podcast
episode (2,354 media clips) opening the project held the main thread 42 s and the window was replaced by Desktop's
hang watchdog; after this PR (with the runtime PR) the longest task is 1.1–1.6 s.
Why
Measured in Desktop over CDP with local builds swapped in:
probeMissingSourceDurationsapplied each clip with its ownsetState, copying the element array and waking every subscriberAudioWaveformforced layout per clip; a MutationObserver per clipcreateTimelineElementFromManifestCliprebuilt transition labels per clip, no selector passpackTrackLanesfirst-fit, quadratic per trackHow
withoutHiddenSlivers: a clip starting inside an earlier clip's drawn box, not ending its run, is not mounted;selected, hovered, roving-focused and pinned clips always are. The episode mounts 530 clips.
sharesOneLane: a track with no overlaps except transition pairs packs in one start-order sweep.withSelectorIndexPassshared.data-audio-muteditself (a frame-wide muted clip greyed only via its waveform);withoutHiddenSlivershides only clips drawn wider than they are long; frame-wide audio fills in the bar colour.Before
Desktop, the 2-hour episode at fit zoom, published Studio 0.8.158, 90 s after opening (preview still black): 2,376 clips
mounted, 45,715 nodes, 666 MB heap.
After
Same episode and zoom with this PR, 30 s after opening: 530 clips mounted, 10,333 nodes, 140 MB heap. The lanes read
the same; frame-wide audio clips fill in the waveform colour instead of mounting a waveform each.
Related work
Runtime half: #5674 (independent).
Test plan
adaptive throttle, playback hold).
🤖 Generated with Claude Code