fix(studio): restore golden-branch timeline behaviors dropped by the stack rebuild#2347
fix(studio): restore golden-branch timeline behaviors dropped by the stack rebuild#2347ukimsanov wants to merge 4 commits into
Conversation
…stack rebuild The Studio stack rebuild (#2291) landed the remaining NLE layers but dropped or regressed several final-wave behaviors from the reviewed studio-dnd stack, and never repaired the stale timelineZones.ts that #2279 introduced. Restores: - TimelineRuler: sticky under vertical scroll, full-height gridlines removed (beat lines only), frame-number tick labels via a persisted timeDisplayMode store preference (PlayerControls toggle now store-backed) - timelineZones: stable track lanes — lane = authored data-track-index ascending; z is paint order only (replaces the stale z-driven lane pack, which broke track insert-band commits that contractually depend on it) - persistTimelineBatchEdit: a batch member whose patch is a no-op (attributes already at target values, e.g. in a track-insert renumber) is skipped instead of aborting and rolling back the whole batch — this alone made new-track creation (incl. the top insert band) fail silently - useTimelineStackingSync: unresolvable clips read as NaN again so timelineStackingSync's Number.isFinite exclusion contract holds (z=0 fabrications skewed stacking boundaries) - timelineAssetDrop: drops land on the drop track (no overlap bump to max-track+1), data-hf-id stamped, audio gets data-volume - timing edits: soft-reload the server's rewritten GSAP script instead of a full iframe remount (no all-clips flash on move/resize); full reload only when no scriptText or the soft path can't apply, and one full reload when a group edit touches non-active files (new hooks/timelineTimingSync.ts) - duration: content-driven grow-AND-shrink on move/resize/delete, synced optimistically to the store and the live root data-duration at release (was a grow-only ratchet; shrink never updated the readout) New UX: sidebar asset click opens a compact non-modal preview over the canvas (dismiss on outside click, Escape, playback, or seek), and clicking an already-added asset reveals its clip in the timeline (smooth minimal scroll to its time and lane; vertical-only in fit zoom). Verified by pointer-driving a real project: sticky ruler + gridline removal, no iframe remount on move/resize (marker survives, GSAP tween positions rewritten in place), duration readout 40->37->40 on shrink/stretch, and top-insert-band track creation renumbering lanes correctly on disk.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const response = await fetch( | ||
| `/api/projects/${projectId}/files/${encodeURIComponent(targetPath)}`, | ||
| ); |
| const res = await fetch( | ||
| `/api/projects/${projectId}/gsap-mutations/${encodeURIComponent(filePath)}`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| type: "shift-positions", | ||
| targetSelector: `#${elementId}`, | ||
| delta, | ||
| }), | ||
| }, | ||
| ); |
| const res = await fetch( | ||
| `/api/projects/${projectId}/gsap-mutations/${encodeURIComponent(filePath)}`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| type: "scale-positions", | ||
| targetSelector: `#${elementId}`, | ||
| oldStart, | ||
| oldDuration, | ||
| newStart, | ||
| newDuration, | ||
| }), | ||
| }, | ||
| ); |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Review: restore golden timeline behaviors
Reviewed the diff three ways: PR vs main, PR vs golden (studio-dnd/consolidated-tip), and semantic verification of the files that were reinterpreted rather than copied. Method: for each of the six restore claims I checked that the regression on main is real, that the restored code matches golden, and (for the non-identical files) that the port preserves golden's semantics against main's post-#2291 surrounding code.
Overall this is sound and mergeable for its stated scope. Every regression claim I sampled is truthful, the six exact-restore files are byte-identical to golden with their dependency contracts intact, and the load-bearing soft-reload ("blink") fix is faithful to golden, including the authored-opacity restore path. Requesting changes for three small, user-visible fixes plus a documentation correction; the rest are follow-ups.
Fidelity summary
| Claim | Regression real | Restore faithful |
|---|---|---|
| #1 Gridlines + sticky ruler | yes | byte-identical to golden |
#2 Stale timelineZones (z to track lanes) |
yes | byte-identical; consumer contract holds |
| #3 Batch no-op silent fail | yes | faithful, but shares golden's latent flaw |
| #4 Canvas blink (soft-reload) | yes | core logic byte-identical; one new edge risk |
| #5 Duration grow-and-shrink | yes | server path correct; two gaps |
| #6 Stacking-sync NaN contract | yes | traced end-to-end, no || 0 coercion |
| Asset-drop restore + reveal/preview UX | net-new | three UX bugs |
The NaN exclusion chain (#6) was traced Timeline.tsx to timelineClipDragCommit.ts to computeStackingPatches with no coercion, so restoring readClipZIndex to return NaN correctly activates the pre-existing Number.isFinite filter. The soft/full reload gating (#4) is byte-identical to golden, and the mixed-file group edit does exactly one full reload when a change targets a non-active comp.
Please fix before merge
1. Stuck preview overlay when revealing an already-added asset
components/sidebar/AssetCard.tsx:151-158 (and the mirror in AudioRow.tsx): the used reveal branch does setSelectedElementId + requestClipReveal + return and never calls clearPreviewAsset. Repro: open the preview overlay on a not-yet-added asset A, then click a different, already-added asset B in the sidebar. A stays stuck open while the timeline scrolls to reveal B underneath it. The only in-scope callers of clearPreviewAsset are the overlay's own dismiss handlers and project-switch, so nothing clears it here. Add a clearPreviewAsset() to the reveal branch.
2. Duration readout is not rolled back on a failed write
Golden captures previousDuration and reverts both the store and the live root in its catch (timelineElementsMove.ts:312-314). The PR sets the duration optimistically via syncPreviewContentDuration at gesture release (useTimelineGroupEditing.ts:222,311) but has no previousDuration capture and no rollback. A failed save leaves the store and live root data-duration advertising a length that was never persisted, until the next reload. This is on the default (server) path, not gated by any flag. Restore golden's rollback.
3. Dismiss-on-playback is edge-triggered only
components/nle/AssetPreviewOverlay.tsx:102-108 evaluates shouldDismissAssetPreview only against future usePlayerStore.subscribe mutations. Playback drives currentTime outside Zustand in the RAF loop and only writes the store at the end of the range or on loop wrap, so a preview opened while playback is already running does not dismiss until then, contrary to the "dismiss on playback" contract. Check the current store state once at subscribe time (or gate opening the preview on !isPlaying).
Worth fixing while this code is open
4. Batch no-op skip cannot distinguish a legitimate no-op from a mistargeted member
hooks/timelineEditingHelpers.ts:328 treats patched === current as a skip. But applyPatchByTarget (via findTagByTarget in utils/sourcePatcher.ts) returns the string unchanged both when the element is already at the target value (legitimate no-op) and when the target is not found at all (a real error). So a wrong domId/hfId/selector is silently dropped instead of surfacing. This is faithful to golden (timelineElementsMove.ts has the same ambiguity), so it is not a regression this PR introduces, and it is why the primary claim (track-insert renumber no longer aborts) is correctly fixed. But the single-element path throws in exactly this case, so the two paths now disagree. Cheap root-cause fix: resolve the target with findTagByTarget first and throw on null, only treating byte-identical output as a no-op when the target actually resolved.
5. New silent stale-preview surface introduced by the history-fold extraction
hooks/timelineTimingSync.ts: foldGsapMutationIntoHistory runs the GSAP server rewrite, then does a readFileContent/recordEdit fold step afterward. If that post-mutation step throws, it propagates to finishTimelineTimingFallback's catch, which calls onGsapError (console only) and returns, discarding the already-rewritten scriptText with no syncTimingEditPreview and no reloadPreview. The preview is left showing the pre-rewrite GSAP positions with no recovery. Golden's inline path had no fold step between the mutation and the sync, so this failure mode is new to the extraction. Sync the preview in a finally, or catch the fold step separately so a history-record failure never suppresses the reload.
6. timelineRevealScroll has no guard for a degenerate viewport
player/components/timelineRevealScroll.ts:60: windowSize = windowEnd - windowStart is never guarded against <= 0. When the scroll container is smaller than stickyStart + 2 * padding (reachable at the minimum timeline height combined with the caption-mode footer), the oversized-clip branch still fires and returns a target that places the clip top at stickyStart + padding, off-screen in a viewport that small. The reveal silently scrolls but the clip stays hidden. No test covers windowSize <= 0. Add a guard and a test.
Non-blocking
- The description's claim that
timelineClipDragCommit.tsis "already golden" onmainis incorrect: its blob equalsmain's, not golden's (it is missing golden'sbeginTimelineOptimisticGesturerace guard, a separate feature). This is functionally harmless because thenormalizeToZonescall site is identical and only reads.track/.key, but the description misrepresents a dependency; please correct it. player/components/PlayerControls.tsxbundles an undisclosed cosmetic change (removal of the transport-barborderTop) not covered by any claim. Harmless, but call it out or drop it.- The SDK fast path (
sdkTimingPersist) writes clip attributes but not rootdata-duration, so a shrink routed through it would leave the persisted root duration stale. This is currently dormant (STUDIO_SDK_CUTOVER_ENABLEDdefaults false, and the caller falls through to the server path when it returns false), but it is a real gap to close before that flag flips on.
Duration edge cases (deleting the furthest clip, empty timeline, clip at time 0) are all handled correctly on the server path: setCompositionDurationToContent rejects a content end <= 0 and keeps the prior duration, and the start < 0 exclusion correctly counts a clip at start 0.
…e z/lane pipeline
Vertical clip moves committed in the store but never survived: two persist
bugs plus a runtime renumber all fought the stable-track-lanes model.
- timelineMoveAdapter deliberately stripped the track from lane-reorder
persists ('z-only reorder path' — the old z-driven lane model). Lane =
authored data-track-index now: lane-reorder and track-insert both persist
the track; plain timing moves omit it to stay SDK-fast-path eligible.
- Display lanes and file tracks are different coordinate spaces:
normalizeToZones packs sparse authored tracks (1,2,... or gaps, or DOM-index
fallbacks) onto contiguous display lanes, and lane edits persisted the LANE
number — silently re-targeting the wrong row in any non-0-contiguous file.
Elements now record their authoredTrack when remapped; a lane change
persists the target lane's authored track (store stays in lane space).
- The runtime split same-track clips of different kinds (video vs caption
div) onto separate renumbered tracks at discovery, so authored indices
never round-tripped ('drop onto an existing track' bounced back). Removed:
data-track-index is honored verbatim (render never reads it); kind-based
row presentation belongs in the display layer if ever wanted.
Adversarial review fixes on the same pipeline:
- runtime: parseInt(attr) || fallback dropped authored track 0 for GSAP and
overlay clips (parseAuthoredTrack helper honors 0)
- single-clip move fallback persisted only data-start — lane changes snapped
back on reload (now passes the track to the patch builder)
- lane-change z-sync candidate ignored a multi-selection's time shift, so
patches were computed against stale overlap sets
- track insert around a locked clip persisted a colliding renumber (the next
normalize merged lanes); the insert is now refused with a warning
- computeStackingPatches compared leaf z across CSS stacking contexts, where
ancestor z decides paint order; the sync now partitions by
stackingContextId and never patches across contexts
Timeline geometry (user-reported):
- fit zoom leaves 20% trailing headroom (FIT_ZOOM_HEADROOM in
timelineLayout.ts; single fit-pps source, so ruler/lanes/playhead/drag all
inherit it)
- playhead line center now sits exactly on GUTTER + t*pps at every zoom
(wrapper had shrink-wrapped to the 9px diamond, off-centering the line);
ruler ticks center on their timestamp
- ruler: frame-mode steps snap to whole frames (no duplicate labels), hour
steps added for far zoom-out, tick positions computed as exact multiples
(no float drift)
|
Second commit ( Vertical lane moves didn't persist — three independent causes, all fixed:
Review findings fixed: Timeline geometry: 20% fit-zoom trailing headroom (single shared constant), playhead line center exactly on Known follow-up (deliberately not in this PR): ~1,000 lines of dead old-model code (the z-driven vertical-drag pipeline: |
|
@ukimsanov rebase main |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Follow-up review: commit 2 (fefbde4) z/lane pipeline, single-source-of-truth pass
Thanks for the second round. The vertical-lane persistence fixes and the display-lane vs authored-track split are the right architecture, and the adversarial pass caught real bugs. Reviewing this commit specifically through a single-source-of-truth lens (one owner per decision, one authoritative source per field, no stale plumbing) surfaces that the fix is incomplete at two construction boundaries, ships one mechanism that is inert in production, and leaves two z-models coexisting. Findings are ranked; each was traced to file and line.
The common root cause
createTimelineElementFromManifestClip (packages/studio/src/player/lib/timelineDOM.ts:115) is the single translation boundary from the runtime manifest to a studio TimelineElement. It sets track: clip.track but drops two authoritative fields that the runtime computes: authoredTrack and stackingContextId. Findings 1, 2, and 4 all trace back here. Carrying both fields through this factory is the single-source-of-truth fix; the drag helpers should read them from a complete element rather than reconstruct them from display state.
Ranked findings
1. Critical: expanded sub-composition drags still persist a display lane into data-track-index.
The factory above never sets authoredTrack. buildChildElements (packages/studio/src/player/hooks/useExpandedTimelineElements.ts, around line 166) then overwrites track with display.track + offset, permanently discarding any authored value. authoredTrackForLane (packages/studio/src/player/components/timelineClipDragCommit.ts:169-175) only searches the top-level elements array, which does not contain expanded children, so dragging an expanded child onto an empty display lane returns the display-lane integer as the authored track. That is the exact coordinate-space bug this commit set out to fix, still reachable for sub-composition children on a sparse file.
2. High: the new stackingContextId partitioning is unwired in production.
stackingContextId is declared on the store type (packages/studio/src/player/store/playerStore.ts:44) and the wire type, computed by the runtime (packages/core/src/runtime/timeline.ts:404,513,568), and consumed by timelineStackingSync (packages/studio/src/player/components/timelineStackingSync.ts:301). But createTimelineElementFromManifestClip never copies it, and every runtime clip routes through that factory, so every production clip resolves to null (root context) and the per-context partitioning is a no-op. The new stacking tests inject stackingContextId directly, so they pass while production never exercises the wiring.
3. High: optimistic store updates never refresh authoredTrack.
A second drag that lands before the reload completes computes the authored track from stale store data.
4. High: spill-lane drops have no distinct authored identity, and the guiding comment is wrong.
authoredTrackForLane's docstring states overlap sub-lane spills are "never a lane-move target," but Timeline.tsx's trackOrder (grouped by el.track from packTrackLanes) includes every display sub-lane, including spill lanes, as legal drop targets. A drag onto a spilled lane is reachable; the persisted track then collapses on reload. Please fix the comment and the handling together.
5. Medium: the non-atomic single-element move path drops vertical-only moves.
packages/studio/src/hooks/useTimelineEditing.ts:144-161: applyTimelineStackingReorder runs, then if (!startChanged) return reorderDone returns before the file-track persist below it. A pure lane change (no time change) routed through the onMoveElement-only fallback writes nothing to the file.
6. Medium: two z-models coexist (stale plumbing).
The old z-reorder vocabulary (timelineLayerDrag.ts resolveTimelineLayerZIndexChanges, applyTimelineStackingReorder, TimelineMoveInput.stackingElement, plus their tests) is dead by dataflow: the drag-commit path passes onMoveElement a StartTrack with no stackingReorder, so applyTimelineStackingReorder (useTimelineEditing.ts:152) always early-returns on a null intent, and the only producer branch (timelineEditing.ts:132) is unreachable because timelineClipDragPreview.ts never passes stackingElement. No current drag double-writes z-index and data-track-index, so this is not a live bug. But the model is still typed, still executed during preview (it computes a stackingReorder that is then discarded at commit), and still test-covered, and the description's "zero production callers" is imprecise: applyTimelineStackingReorder has a live call site, just fed undefined. Deferring the deletion is reasonable, but please track it as stale plumbing rather than describe it as already dead.
7. Low: stackingContextId equality is recomputed in three places inside computeStackingPatches rather than through one canonical contextKey helper.
Test-coverage gap (the tell)
Every new test injects authoredTrack and stackingContextId directly rather than constructing elements through createTimelineElementFromManifestClip, which is why findings 1 and 2 shipped green. A single pipeline test from sparse authored tracks through expansion, drag commit, and file patch would have caught both. Please add coverage that crosses the real factory boundary.
Requesting changes for findings 1 and 2 (correctness and a dead feature); 3 through 5 are worth fixing in the same pass, and 6 through 7 are cleanup that can follow.
…ents An adversarial review of the canvas context-menu z-order pipeline (Bring to Front / Forward / Backward / Send to Back) found the resolver math sound but the glue between the menu and the commit hook broken: - The menu optimistically wrote style.zIndex AND position: relative to the live elements BEFORE the commit hook ran. The hook decides whether to persist position by checking getComputedStyle(el).position === 'static' — always false after the pre-apply — so the position patch was never persisted on the menu path and the reorder silently reverted at the post-commit reload for any nested/static element (root clips survive only because the runtime forces position:absolute). The same pre-apply made the failure rollback capture the already-mutated values, restoring the broken state on persist errors. The menu no longer pre-applies; the hook owns the live writes (it already applied both synchronously) and now sees true priors. Siblings without a persistable identity still get their z applied live-only so a renumber stays visually coherent. - The commit hook's entry.key store-sync plumbing had zero production callers; the store zIndex went stale until full reload. All three callers (canvas menu via PreviewOverlays, timeline lane z-sync, LayersPanel) now derive and pass the timeline store key (new deriveTimelineStoreKey helper). - patchElementBatch discarded the server's per-patch matched[]; unresolvable siblings persisted partially and silently. Unmatched targets now warn and report save-failure telemetry (z-reorder-unmatched) without rolling back the matched subset. - template/noscript elements counted as painting siblings, so renumber fallbacks wrote z-index/position into <template> tags in the source file. Excluded from the sibling family. - The default undo coalesce key merged DISTINCT z actions within 300ms into one undo entry; the action kind is now part of the key (LayersPanel drags keep coalescing within a drag; explicit lane-move gesture keys untouched). - rectsIntersect comment claimed touching rects intersect; the strict inequalities say otherwise — comment fixed.
|
Third commit ( Resolver verdict: sound. Tie-aware (equal z → DOM order, matching CSS), minimal-churn fast path, band-constrained renumber fallback, no negative z, no unbounded growth. Live-verified: bring-to-front = one atomic single-element write; menu items disable correctly at the extremes from fresh state. Glue defects fixed:
Deferred to the dead-code cleanup follow-up: multi-file partial-persist orphaning (unreachable from the menu), unifying LayersPanel's stamp-everything Gates: all pre-commit hooks green; studio suite 2058 passing (same 18 pre-existing environmental failures). |
Two legibility fixes for the canvas z-order menu, from user feel-testing: - z-only commits no longer remount the preview iframe. The commit hook already applies the inline z (+ injected position) to the live elements and updates the store synchronously; the post-commit reloadPreview() was a redundant full remount that read as a canvas 'blink' on every action. commitDomEditPatchBatches gains skipReload, engaged only when provably safe: every op is an inline-style patch AND the server reports every patch matched — anything else falls back to the reload so the preview reconverges with disk. The file-watcher's own reload stays suppressed by the existing domEditSaveTimestampRef window, so the skip is real. - Bring Forward / Send Backward step over the next VISIBLY overlapping sibling. The nearest z-neighbor in a composition is often invisible at the current frame (runtime hides time-inactive clips with inline visibility/display; GSAP parks elements at opacity 0), so the step crossed something the user couldn't see — 'enabled but nothing happens'. The forward/backward set now filters on element-level computed visibility (display/visibility/opacity, injectable for tests); enable/disable shares the resolver so the menu is honest: actions disable when no visible neighbor exists. Front/back keep the full painting family. - The neighbor that was stepped over gets a 600ms accent flash, drawn in the studio overlay layer (never in the iframe DOM), so the action shows its work.
|
Fourth commit ( |
3 participants
What
Restores six timeline behaviors from the reviewed studio-dnd stack (
studio-dnd/consolidated-tip) that the Studio rebuild (#2291) dropped or regressed, fixes the staletimelineZones.tsthat #2279 introduced and #2291 never repaired, and adds two small sidebar-asset UX improvements.Why
A golden-branch-vs-main comparison after the Studio cutover found five verified regressions behind user-visible symptoms, plus one new bug found by pointer-driving the result:
086fdabda) but not itsTimelineRuler.tsx/PlayerControls.tsxhunks: main kept the pre-refresh ruler that draws full-height gridlines (visible in the gaps above/below opaque tracks) and scrolls away vertically.timelineZones.ts— main still ran feat(studio): timeline collision and placement model #2279's z-driven lane pack, while its owntimelineClipDragCommit.tsis the golden code whose insert-band commits contractually depend on the golden track-drivennormalizeToZones(lane = authoreddata-track-index; z = paint order only).persistTimelineBatchEdittreated any no-op batch member (attributes already at target values, which a track-insert renumber legitimately produces) as a fatal targeting error, aborting and rolling back the whole insert. The golden batch patcher skips no-op members (timelineElementsMove.ts:116).scriptTextand calledreloadPreview()(a full iframe remount) unconditionally; golden soft-reloads the script in place and full-reloads only when it can't.readClipZIndexwas reverted to fabricate0for unresolvable clips whiletimelineStackingSyncstill implements the goldenNumber.isFiniteexclusion contract, skewing z-boundary math.How
TimelineRuler.tsxadopted from golden (stickytop-0, beat-lines-only background, frame labels);timeDisplayModelifted intoplayerStore(persisted via studio UI preferences);PlayerControlstoggle store-backed.timelineZones.ts+ test replaced with the golden stable-track-lanes version;timelineClipDragCommit.test.tsupdated surgically (main-only optimistic-revision coverage kept).persistTimelineBatchEditskips no-op members and returns early when nothing changed; regression tests added.hooks/timelineTimingSync.ts(extracted to stay under the 600-line gate):GsapMutationStatuscarriesscriptText,syncTimingEditPreviewsoft-reloads viaapplySoftReload, group edits soft-reload only when every change targets the active comp (one full reload otherwise). The server already returnedscriptText; the client had discarded it.setCompositionDurationToContent(furthestClipEndFromSource(...))(grow-or-shrink);syncPreviewContentDurationupdates the store + live rootdata-durationsynchronously at release; delete shrinks too. Main's optimistic-revision rollback, operation tags, PostHog commit events, andrunGestureTransactioncanvas paths are untouched.timelineAssetDrop.tsrestored to golden: drops land on the drop track (no overlap bump to max-track+1),data-hf-idstamped, audio getsdata-volume.timelineRevealScrollmath +clipRevealRequeststore channel; vertical-only in fit zoom).Test plan
telemetry/*+SnapToolbarfail identically on pristine main (Node-26 localStorage environment issue)tsc --noEmitclean; oxlint/oxfmt clean; full workspace build green; all pre-commit gates (filesize, fallow, lint, format, typecheck) pass