Skip to content

feat(studio): glue API coexistence layer for the NLE swap#2205

Open
ukimsanov wants to merge 1 commit into
studio-dnd/pr13-nle-shell-componentsfrom
studio-dnd/pr14-glue-api-coexistence
Open

feat(studio): glue API coexistence layer for the NLE swap#2205
ukimsanov wants to merge 1 commit into
studio-dnd/pr13-nle-shell-componentsfrom
studio-dnd/pr14-glue-api-coexistence

Conversation

@ukimsanov

@ukimsanov ukimsanov commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

What

extends 21 glue files so the OLD timeline/canvas engine and the NEW NLE components type-check side by side: playerStore (multi-select setters, zoom pin, snap toggle, non-reactive scale scratch), drag-state types gain optional NLE fields, timelineLayout/timelineAssetDrop/timelineEditingHelpers/ timelineEditing/timelineElementHelpers/studioHelpers/assetHelpers gain the NLE exports, DomEditOverlay + gestures + AssetContextMenu + Timeline props gain optional callbacks/params, contexts gain *Optional hooks, and TimelineEditCallbacks.onMoveElements becomes a bivariant method accepting both engines' change shapes. patchDocumentRootDuration's test rides along.

Why

this is the keystone that dissolves the old "welded glue" problem — every symbol the NLE components need is ADDED next to what the old engine still uses, so the engine components and the swaps can land as separate reviewable PRs.

How

15 authored intermediate files (main content + additive symbols; no behavior changes — new fields optional, new callbacks unused until wired) plus 6 files whose final content is already purely additive. New exports without consumers yet carry TEMP(studio-dnd) ignoreExports entries, removed by the app-shell swap.

Test plan

tsc --noEmit in studio + studio-server (verifies BOTH engines compile); bunx vitest run (full suite green incl. the 6 new patchDocumentRootDuration tests); fallow audit clean.


Stack position 14/25 — studio NLE overhaul (CapCut-parity timeline + canvas). Graphite manages bases; merge from #2192 upward. Temporary TEMP(studio-dnd) fallow entries (unwired-component windows) are all removed by #2213 (app-shell swap), whose tree is byte-identical to the fully-verified integration branch.

ukimsanov commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

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.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Stack Review — NLE Overhaul (25 PRs, ~15,600 additions, ~10,100 deletions)

Reviewed the entire stack in four tiers: math/model layer (#2192#2201), keystone API coexistence (#2205), UI components (#2202#2210, #2214), and swap/cleanup (#2211#2216). No blockers across the stack.


Keystone: #2205 — glue API coexistence (reviewed hardest)

This PR extends the existing studio APIs to support both the legacy engine and the new NLE simultaneously. It's the hinge between the "build the new" and "swap out the old" phases.

SSOT analysis — strong:

  • computeOverlayRootScale extracted from inline code in toOverlayRect — now shared between toOverlayRect and elementCornerOverlayPoints. Single source for iframe→overlay mapping.
  • restampManualOffsetDragGestureBase and applyManualOffsetCommitValue extracted from applyManualOffsetDragCommit — enables reuse for the new nudge path without duplicating the re-stamp logic.
  • furthestClipEndFromDocument extracted from readTimelineDurationFromDocument — the latter now delegates. Single computation of "furthest clip end." Correctly excludes the root element (prevents feedback loop where root duration is counted as clip content).
  • patchDocumentRootDuration correctly finds the TOP-LEVEL root (no ancestor composition) — 6 well-targeted tests.
  • Union type on onMoveElements (TimelineGroupMoveChange | TimelineGroupMoveEdit) allows both engines to coexist on the same callback without forking.

Edge case coverage:

  • orientedOverlayRect falls back to plain AABB when corners can't be measured — safe degradation, pixel-identical to legacy at angle=0.
  • readElementRotationDegrees guards NaN/Infinity on matrix components AND the output.
  • resolveReloadSeekTime uses ?? (not ||) so explicit seek-to-0 isn't treated as missing.
  • setTimelineScale uses non-reactive mutation to avoid subscriber re-renders — documented as scratch state.

Coexistence hygiene: All TEMP fallow exemptions are marked "removed by studio-dnd/pr22" (#2213). The code-duplication ignores are clearly labeled as coexistence-window clones. #2213 confirms all ~30 exemptions are cleaned up.

One noted SSOT gap: readTimelineElementZIndex contains its own parseZ inline function and the comment says "Mirrors canvasContextMenuZOrder.parseZIndex semantics." Two z-index parsers — documented dependency, different packages. Acceptable.


Tier 1: Math/Model Layer (#2192#2201) — all clean

Heavily unit-tested pure functions (2000+ test lines for ~1200 implementation lines). Each module owns one concern:

  • #2192 — log-scale slider, zoom-pin, beat-source. Round-trip tests within 1%.
  • #2193 — reload seek-restore. Double-seek guard trick for GSAP re-eval is clever.
  • #2194 — batch files route. shift-positions-batch folds N shifts into one write.
  • #2195 — collision/placement. Half-open intervals, prefer-up search, insert-row detection. 292 lines of tests.
  • #2196 — snapping. Priority dedup (playhead > clip > beat), clamped-snap-drop.
  • #2197 — multi-drag preview. Rigid formation, leftmost-at-0 constraint, -0 avoidance.
  • #2198 — lane↔z stacking sync. Tie-aware cascade, multi-edit lower-first.
  • #2199 — lane-zone model. Constrained packing. The z-to-lane round-trip convergence test is the kind of invariant test that prevents whole classes of regressions.
  • #2200 — asset-click + nudge gate. Reference-counted claim/release, idempotent release.
  • #2201 — gesture characterization. Center-anchored resize convergence proof + razor-split undo proof.

One nit: overlapsInTime (with epsilon) is copy-pasted identically between timelineStackingSync.ts (#2198) and timelineZones.ts (#2199). Should be a shared import. The exact-vs-epsilon distinction from timeRangesOverlap (#2195) is intentional and defensible.

Tier 2: UI Components (#2202#2210, #2214) — all clean

Bottom-up build: pure math → state/context → drag engine → interaction hooks → chrome → shell → leaf UI.

  • #2202 — z-order menu. Tie-aware DOM-order renumbering. Cross-realm instanceof fix.
  • #2203 — canvas nudge. 1px / Shift 10px, composition-px not screen-px.
  • #2204 — NLE shell + context. Nit: NLEContext has 25+ fields — future decomposition candidate.
  • #2206 — clip-drag engine. Three-phase persist (patch-in-memory, atomic write, preview sync). Rollback on failure. Nit: fire-and-forget error path in persistMoveEdits should document that callers can't observe failure.
  • #2207 — timeline hooks + lanes. 40+ prop surface on TimelineLanes — consider grouping.
  • #2208 — canvas chrome. Rotation-aware corner handles. Radial resize well-tested. Nit: siblingZIndexEntry silently drops untargetable elements — z patches can be partial.
  • #2209 — NLE shell assembly. Sub-comp coordinate rebasing (toLocalElement) applied consistently across all edit paths.
  • #2210 — asset card. Click-vs-drag gate via pointer threshold.
  • #2214 — clip thumbnails. computeThumbnailStrip + resolveMediaPreviewUrl now shared between image and video. SSOT win.

Tier 3: Swap + Cleanup (#2211#2216)

  • #2211 — canvas glue swap. AABB → OBB resize, nudge, context menu. All old consumers migrated.
  • #2212 — timeline glue swap. Stacking-layer → flat-track model. Geometry hook extraction, multi-drag preview, sticky ruler.
  • #2213 — app-shell swap + old engine deletion (-5522 lines). All ~30 TEMP fallow exemptions cleaned up. Nit: The old 963-line useTimelineEditing.test.tsx integration suite is deleted without same-scope replacement — the new paths (batched move, multi-delete, content-driven duration) should get integration tests in a follow-up.
  • #2215 — visual refresh. Pure CSS. preview-regression CI failure is expected (visual diff from intentional style changes).
  • #2216 — assets/blocks panel. Full-bleed media now selectable. Block drag-to-timeline.

Cross-Stack Architecture

The stack is disciplined:

  1. Pure math first, UI second, swap last. Every decision function is testable in isolation before it's wired into React.
  2. Coexistence window is explicit. Fallow exemptions, union types, optional fields — all labeled TEMP with cleanup PR reference.
  3. Swap is one-pass. #2213 removes the old engine and all exemptions in one commit. No lingering dead code.
  4. 2000+ lines of math tests including the z-to-lane convergence proof (#2199) and the resize oscillation proof (#2201).

Two things to watch post-merge:

  1. Integration test coverage for the new batched-move / multi-delete / content-driven-duration paths (gap from #2213's test deletion).
  2. The duplicated overlapsInTime + epsilon constant between stacking sync and zone model — worth extracting to a shared module.

CI: 0 failures across all 25 PRs (one preview-regression on #2215 is expected from the visual refresh).

Ship the stack.

— Miga

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1 Batch-D keystone deep-dive — #2205 glue API coexistence

Reviewed as the load-bearing hinge of the 25-PR NLE stack. Verified the 8 keystone invariants against SHA a2f4dd0, cross-referenced against #2211/#2212/#2213 removal paths. Full-file reads (not just diffs) on every callback and context-touching module.

Invariant disposition

1. Bivariance soundness on onMoveElements — verified within window.
Enumerated all 7 hits of the identifier. At packages/studio/src/player/components/timelineCallbacks.ts:47-50 the callback is intentionally declared with method syntax (onMoveElements?(...)) so its parameter (Array<TimelineGroupMoveChange | TimelineGroupMoveEdit>) checks bivariantly — the escape hatch that lets the two engines' shape assignments both type-check. This is a documented TypeScript soundness hole; at #2205 it is bounded and safe because:

  • The one producerpackages/studio/src/components/StudioPreviewArea.tsx:70,246 — wires handleTimelineGroupMove: (changes: TimelineGroupMoveChange[]) => … (legacy-only) into the bivariant slot. Method-syntax accepts.
  • The one legacy consumeruseTimelineClipDrag.ts:107 — declares its own local prop as TimelineGroupMoveChange[] only, and the legacy engine only ever invokes with that shape. So at #2205 no TimelineGroupMoveEdit-shape value ever reaches handleTimelineGroupMove at runtime.
  • Pass-through sites (Timeline.tsx:74,225, useResolvedTimelineEditCallbacks.ts, TimelineEditContext.tsx) don't discriminate — they forward whichever union member arrives.
  • The NLE producer side lives in #2206's timelineClipDragCommit.ts:19,59 and would pass TimelineMoveEdit[] shape — but the engine is unwired at #2205, so the path is not reachable. The hazard is bounded to the swap window.

Deferred-hazard resolution audit (out of #2205 scope but relevant): verified via gh api /pulls/2213/files that #2213 deletes packages/studio/src/components/StudioPreviewArea.tsx in its entirety (additions: 0, deletions: 500). So the legacy handleTimelineGroupMove narrow-shape consumer never sees TimelineGroupMoveEdit — it evaporates with the whole file when the NLE producer starts firing. This is the cleanest possible resolution of the bivariance-window: full substitution rather than incremental narrow-to-union widening. Well designed.

2. *Optional context hooks — verified.
Two flavors, both internally coherent:

  • StudioContext.tsx:64-66 and DomEditContext.tsx:106-108 return Value | null (with docstrings). Callers gate correctly (e.g. useTimelineStackingSync.ts:23-26 reads via ?. and computes zSyncEnabled = Boolean(handleDomZIndexReorderCommit && zSyncPreviewIframeRef) before engaging any downstream behavior).
  • TimelineEditContext.tsx:16-18 returns TimelineEditCallbacks (an empty object, not | null) when outside a provider. This is technically a tighter pattern than the other two — every callback in the bag is undefined on empty, and consumers must gate on the individual callback presence anyway. Non-Optional variants still throw as expected (useTimelineEditContext, useStudioShellContext, useDomEditActionsContext).

3. Multi-select ↔ single-select coexistence — verified.
playerStore.setSelectedElementId (line 465-480) canonically collapses selectedElementIds to Set([id]) on a genuine single-select transition. setSelectionAnchor (line 484-490) preserves the group when id is already a member (DOM echo path). Multi-select mutations (addSelectedElementId/toggleSelectedElementId/setSelectedElementIds) route through resolveElementSelection which keeps selectedElementId a valid anchor into selectedElementIds. Consumer discipline confirmed at TimelineLanes.tsx:412-417 — plain click clears multi then applies single. Legacy single-select subscribers get the anchor; NLE multi-select subscribers get the full set.

4. patchDocumentRootDuration.test.ts — pins new behavior; helper is unwired at #2205.
The helper is new at #2205 (gh api /pulls/2205/files shows +125/-0 on timelineEditingHelpers.ts, and patchDocumentRootDuration + patchIframeRootDuration both appear only in the + half of the diff). It has no callers at #2205 head — the callers land at the swap PRs (patchIframeRootDuration is called by #2206's timelineElementsMove.ts:120 and eventually by move-commit paths post-swap). The 6-test suite is a co-landed contract for a to-be-consumed helper. Not a red flag under coexistence framing.

5. Non-reactive timelinePps / timelineFitPps scratch — accepted with a watch-note.
playerStore.setTimelineScale (line 426-433) does state.timelinePps = pps without calling zustand's set(...), deliberately bypassing the subscriber notify path. Docstring is explicit ("Non-reactive publish"). Zero call sites at #2205 head — writers land in #2207 (useTimelineGeometry.ts:112 is the one writer; grep across #2205 files confirms no reader/writer). Readers land later (imperative getState() reads via pinTimelineZoom, per docstring). Verdict: safe at #2205, but a standing footgun: any future usePlayerStore((s) => s.timelinePps) selector call is a silent bug (subscription with no notifier). If this pattern proliferates, worth an ESLint guard or type NonReactive<T> brand. Follow-up watch-note for the #2207/#2212 reviewers: verify only imperative .getState() reads are used on these fields.

Additional concern I noticed on #2207's writer at useTimelineGeometry.ts:112 — the usePlayerStore.getState().setTimelineScale(pps, fitPps) call runs in the render body, not in a useEffect. Under React 18 concurrent-mode replay (Suspense retry, transitions), the render body can run twice. Because setTimelineScale is idempotent (same input → same field values), this is currently safe, but a useEffect(() => { setTimelineScale(pps, fitPps); }, [pps, fitPps]) would be strictly correct. Non-blocking; flagging for the #2207 review batch.

6. TimelineGroupCommitOptions in both branches — verified symmetric.
Both branches of the union share the same options?: TimelineGroupCommitOptions parameter position (timelineCallbacks.ts:47-50). Legacy path (useTimelineGroupEditing.ts:151-152) accepts options? and reads options?.coalesceKey, options?.beforeTiming. NLE path (#2206's timelineClipDragCommit.ts:59) calls onMoveElements(edits) without options — passes undefined. No commit-history-tag ever leaks between branches (there's no shape-branching read on options); the options object is either present-with-tag or absent.

7. Fallow TEMP(studio-dnd) entries — verified as subset of #2213 removals.
Extracted the 37 symbol strings added to .fallowrc.jsonc at #2205 (fallow keys inside TEMP-tagged blocks). Cross-set against the 45 symbol strings removed by #2213 from the same file. comm -23 = empty — every symbol #2205 adds is deleted by #2213. Superset holds. Same for the 17 file-path strings.

8. domEditOverlayGeometry.ts +228/-19 — verified as SSOT extraction, not a silent geometry swap.
Deep-read the patch. The 19 deletions correspond to inline scale/rect calculation code inside toOverlayRect (and one epsilon comparison in the equality check). The 228 additions extract three helpers: findOverlayRootElement(doc), resolveRootDimensions(root), and computeOverlayRootScale(overlayEl, iframe, doc) — plus updated toOverlayRect and elementCornerOverlayPoints bodies that call the new helper via const scale = computeOverlayRootScale(...). Semantics preserved: still prefers data-width/data-height (declared) over getBoundingClientRect (measured) to avoid GSAP-transform misalignment, still falls back to measured rect on missing declared dims, still returns null when unmeasurable. Miga's review flagged this positively as a shared source; my file-line read confirms no math changed. The undocumented-in-body finding is a doc gap, not a code gap.

Cross-batch consumer inventory

onMoveElements producers/consumers post-swap (traced through #2211/#2212/#2213 file lists):

  • #2206 adds NLE producer via timelineClipDragCommit.ts — invokes onMoveElements(edits: TimelineMoveEdit[]).
  • #2207 adds useTimelineEditPinning.ts — a pinning proxy that preserves the bivariant Parameters<typeof onMoveElements> at the type level.
  • #2213 deletes the entire StudioPreviewArea.tsx (the legacy narrow-shape consumer) — resolves the runtime hazard cleanly.

*Optional context consumers post-#2205:

  • #2207 useTimelineStackingSync.ts — reads both useDomEditActionsContextOptional() and useStudioShellContextOptional(); gates on handleDomZIndexReorderCommit presence before engaging z-sync. Correct.
  • #2207 useTimelineEditPinning.ts — reads via usePlayerStore selector (s.pinTimelineZoom); doesn't touch the null-return contexts. N/A.

Overall

🟢 with two watch-notes:

  1. Non-reactive timelinePps/timelineFitPps field is a standing footgun for downstream reviewers — enforce imperative .getState() reads only.
  2. useTimelineGeometry (in #2207) calls the non-reactive setter during render body; move to useEffect for React-18-concurrent strictness.

The bivariance escape hatch is the load-bearing mechanism of this PR. It's a documented TS soundness hole, but the design bounds the hazard cleanly: the one narrow-shape callee (handleTimelineGroupMove inside StudioPreviewArea.tsx) is deleted wholesale at #2213 rather than gradually rewritten, and no TimelineGroupMoveEdit-shape value can reach it while the file lives. Solid engineering.

R1 by Via

@ukimsanov ukimsanov force-pushed the studio-dnd/pr13-nle-shell-components branch from ba130cb to d8c95b9 Compare July 11, 2026 02:56
@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from a2f4dd0 to 7d93612 Compare July 11, 2026 02:56
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Re the invariant-5 watch-note (non-reactive timelinePps/timelineFitPps): convention acknowledged and holds across the stack — the single reader (ships in #2213, playerStore zoom-pin) uses an imperative get() read; zero reactive selector subscriptions to either field exist anywhere. An ESLint guard (or a branded NonReactive<T> type) is logged as a post-merge follow-up alongside the shared-epsilon extraction and the integration-test suite both reviews requested.

Resubmit summary for the whole stack (13 files, +154/−29): the #2201 blocker remediation (aspirational tests moved to #2211, rewired through a new named production helper), the files-route hardening (#2194: batch no-op parity + Array.isArray guard, with tests), doc/comment fixes (#2192 tie-break docstring, #2198 epsilon divergence note, persist fire-and-forget note), test additions (#2196 clip-edge>beat tie-break, #2199 zone-identity invariant, #2200 nudge-gate reset), and #2207's useEffect + virtualization-TODO. Deferred as agreed: shared epsilon module, NLEContext/TimelineLanes decomposition, typed error callback, integration suite.

@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from 7d93612 to dbb7d62 Compare July 11, 2026 08:18
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Fix wave landed — this force-push carries the consolidated response to the /code-review max findings (15 reported, 12 confirmed) plus the Slack-thread reviews that never hit GitHub. Every branch re-passed the full local gate set (tsc, full vitest at the known 18-failure baseline, fallow audit --base origin/main --fail-on-issues, repo-wide format:check, per-branch filesize, builds at #2213/#2216) and the tip is byte-identical to the integration tree the wave was verified on.

Correctness fixes (confirmed findings):

  • Stale multi-select set (data loss): setSelectedElementId now collapses the multi-select set at the store level, which covers every single-select site (canvas click, keyframe diamond, sidebar asset/audio clicks) in one place; the two additive flows (marquee, cmd-click) were audited and reordered primary-then-set. Canvas delete clears the set; Escape also clears the primary. Regression tests at the store and hook level.
  • Selection race: the monotonic seq guard removed with the legacy engine is restored in handleTimelineElementSelect; a stale async selection can no longer clobber a newer one (race test included). The rightPanelTab === "variables" preservation is also restored.
  • Group move capability bypass: marquee members are filtered through getTimelineEditCapabilities().canMove before edits are built — a locked clip in a marquee is never patched (test included).
  • Lane-drag dual-persist race: the z-reorder now waits for the move persist to resolve, is skipped and rolled back if it fails, and both records share a per-gesture coalesceKey so the gesture is one undo entry (editHistory merges consecutive same-key entries).
  • Duration rollback + soft-reload clobber: optimistic setDuration/patchIframeRootDuration revert on failed persist; multi-file moves only soft-reload the active composition and fall back to a full reload when any other file changed.
  • Stale store zIndex: z-edits now sync the store via updateElement (key resolved from the DOM node), so normalizeToZones and stacking sync read consistent z.
  • Razor split on expanded sub-comps: split times are rebased to the child's local coordinates at the shared executeSplit seam (mirrors TimelinePane.handleSplitElement); root-level splits are byte-identical (tests cover both + mixed razor-all).
  • Empty-zone drop (feat(studio): timeline collision and placement model #2195): the idx === -1 branch signals an insert instead of colliding into an occupied track 0 — the audio-on-visual-only repro is encoded as a test.
  • Stacking cascade (feat(studio): timeline z-stacking sync model #2198): liftAbove cascades transitively over the overlap graph; the M/N/E repro is encoded. trySingleZ now consults the DOM tie-break (paintsAbove) before cascading, so an authored z that already paints correctly isn't rewritten.
  • Z-resolution miss: a failed element resolution no longer fabricates z=0 — the neighbor is excluded from computeStackingPatches. (Implemented as NaN + non-finite filter rather than null: the readZIndex: (el) => number contract is shared by three consumers and widening it would ripple; runtime behavior is identical.)
  • Renumber ordering (feat(studio): canvas context menu and z-order actions (unwired) #2202): overlap-scoped renumbers stay within the scoped clips' original z-band so untouched pairs never invert; the scoped path now has real coverage (mocked getBoundingClientRect — jsdom rects are 0×0).
  • Cross-project preview bleed + raw URLs: assetPreviewStore clears on project change; asset/audio/composition preview URLs route through the shared encoders (spaces/parens filenames covered by tests).

Restored feature: multi-select group resize (dropped with the legacy hook) is rewired through resolveTimelineGroupResize into the new preview/commit path with an all-or-nothing capability gate. Placement note: the group-resize work and the coalesce plumbing land in #2213's diff (the old and new engine APIs can't type-check side by side in the #2205#2212 coexistence window), which is consistent with #2211#2213 merging as one unit — a group with a locked member degrades to single-clip. Single-clip resize is byte-identical (parity tests). Known follow-up: group resize persists per-member (no batch resize handler exists yet), so it isn't a single undo entry — this joins the deferred integration-suite item.

Perf (no behavior change): the selection-chrome RAF takes the AABB path unless the element is actually rotated; geometry measurement computes the root scale/matrix once; the resize pointermove no longer forces synchronous reflows; snap targets and the audio-track set are built once per drag gesture and reused by the auto-scroll loop.

Test hygiene: the two vacuous tests flagged in review now assert their real invariants (no z-patches without stacking deps; no no-op patches from renumbering).

Deferred (unchanged from the earlier summary): shared epsilon/overlap module, NLEContext/TimelineLanes decomposition, typed error callback on DragCommitDeps, ESLint guard for non-reactive store fields, virtualization — and the batched-move/multi-delete/content-driven-duration integration suite, which stays the top follow-up.

Merge note: #2211, #2212, #2213 merge as one unit (seam evidence in the review thread); the PR bodies carry the same note.

@ukimsanov ukimsanov marked this pull request as ready for review July 11, 2026 09:06

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — keystone deep dive, two structural NEW findings

Confirmed 6 of 8 prior claims cleanly (SSOT computeOverlayRootScale, root-excluded furthestClipEndFromDocument, NaN guards, safe AABB fallback, 37 TEMP fallow entries matching Via's count exactly + superset-invariant with #2213). Two structural NEW findings the first pass missed:

NEW #1DomEditOverlay dead prop surface = the upstream root cause of #2211's phantom z-menu

DomEditOverlay.tsx:90,97 declares onDeleteSelection? and onApplyZIndex? in the props interface (importing ZOrderPatch at :29), but neither is:

  • destructured at :101-122,
  • stored in any useRef,
  • forwarded to createDomEditOverlayGestureHandlers, or
  • referenced anywhere in the component body.

Search of the branch shows zero references beyond the interface declaration. So any caller who passes these callbacks today gets a silent no-op — which is exactly the manifestation Abhai flagged at #2211 (context menu writes to iframe DOM but nothing persists; Delete silently no-ops).

Impact: bounded by the wholesale deletion at #2213, but it's a real footgun for any reviewer reading the props contract in isolation. And fixing it here removes the footgun for any future caller that passes these props — cheaper than healing only at #2213.

Fix: destructure + forward both props into the gesture handler bag (~10 LOC).

NEW #2 — triple data-composition-id...data-duration regex fragility

Same regex appears in 3 places across this PR:

  • rootDuration.ts:4-6 (extendRootDurationInSource)
  • timelineAssetDrop.ts:145 (setCompositionDurationToContent)
  • timelineAssetDrop.ts:157 (extendCompositionDurationIfNeeded)

All three: /(<[^>]*data-composition-id="[^"]*"[^>]*data-duration=")([^"]*)(")/

Two failure modes, both silent (return source on no-match):

  1. Attribute order dependency: requires data-composition-id BEFORE data-duration in source order. A hand-authored composition with the order swapped silently never extends duration.
  2. Double-quotes only: resolveTimelineAssetCompositionSize (same file, :194-195) accepts both quote styles via (["'])([^"']+)\1. A single-quoted composition (unusual but legal HTML5) reads w/h correctly but never extends duration.

Additional divergence: extendRootDurationInSource selects the FIRST [data-composition-id] tag by source text order, while patchDocumentRootDuration (timelineEditingHelpers.ts:622-624) selects by ancestor-closest. For pathological multi-root docs the two functions patch different roots — LIVE DOM and PERSISTED SOURCE end up disagreeing.

Fix: replace all 3 regexes with a single DOM-based patchSourceRootDuration that mirrors patchDocumentRootDuration's ancestry-aware root selection and uses DOMParser/serializer. DOMParser is already imported nearby in the same file (furthestClipEndFromSource). ~40 LOC eliminates the silent-fail class across the whole coexistence window.

Additional nits

  • playerStore direct mutation (RDJ's finding stands + goes further): state.timelinePps = pps bypasses set(). Beyond DevTools: useSyncExternalStore snapshots use referential equality on the whole state object, so mutation is invisible to any consumer that reads this via useSyncExternalStore. Documented as non-reactive scratch, but nothing enforces that at the type level.

  • onMoveElements union bivariance safety is purely social: no runtime type-guard, no discriminant. Coexistence holds only if the stack merges strictly in order. One out-of-order merge silently breaks group moves.

  • 15+ new pure exports, 1 tested: applyTimelineAutoScrollStep, resolveTimelineAutoScrollLoopAction, resolveTimelineDragEscape, getTimelineRowTop, getTimelineRowFromY, getTimelineFitPps, getTimelineDisplayContentWidth, truncateMiddle, formatDuration, readTimelineElementZIndex, furthestClipEndFromSource, setCompositionDurationToContent, extendCompositionDurationIfNeeded, fitTimelineAssetGeometry, resolveTimelineAssetCompositionSize, applyManualOffsetNudgeDraft/Commit. All are pure with clear invariants — trivially unit-testable. Their consumers land later in the stack, but the coexistence layer's own invariants are undefended. Matches Via's #2201 model-bound-tests concern.

None are #2205's own merge blockers if you accept the coexistence framing. But NEW #1 (~10 LOC) is worth fixing here because it kills the root cause of the #2211 phantom-menu bug at the boundary. NEW #2 (~40 LOC) is worth fixing here because it closes a silent-fail class for hand-authored HTML that survives the whole coexistence window.

— Rames Jusso

@ukimsanov ukimsanov force-pushed the studio-dnd/pr13-nle-shell-components branch from 0ab5336 to 5d3b082 Compare July 11, 2026 10:49
@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from dbb7d62 to 0a4df4f Compare July 11, 2026 10:49
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Second fix wave landed — covering Ular's hands-on feel-test on a real project plus the Slack re-review round (Abhai + Rames). All 25 branches re-passed the full local gate set (tsc, full suite at the 18-failure baseline, verbatim fallow with zero suppressions, format:check, per-branch filesize, builds, exact tip parity).

From the re-review — independently verified before acting:

From the hands-on session (fixed on a real project that reproduced them):

  • Timeline drops now land on the aimed lane — the display sorts lanes by global z, but the commit only patched z against time-overlapping neighbours, silently discarding the drop intent (root cause of "new track lands somewhere else" and "other clips shuffle"). A drop-intent z nudge on the dragged clip only; neighbours' authored z untouched.
  • Razor split, then undo, works — the split was the only server-write path that never resynced the in-memory SDK document, so its next serialize reverted the split and tripped the "File changed outside Studio" guard.
  • Undo/redo of style/timing edits restores through the soft path (no more black iframe flash); structural restores keep the full reload.
  • Canvas "move to back" no longer rejects id-less elements (?? null → omit; the patch falls through to hfId/selector — the validator was right to reject null).
  • The transport total can no longer be dragged below the authored root duration by a shorter clip manifest (the 0:44/0:40 case); time readouts are zero-padded (00:44) everywhere; playback no longer scrolls the timeline to chase the playhead (deliberate UX decision — scrubbing auto-scroll unchanged).

Not acted on (with reasons): Rames's "#2205 dead prop surface" — the props are forwarded at the tip; the finding describes the coexistence intermediate. "#2209 multi-select resize dropped" — restored in the previous wave. The triple duration-regex → DOMParser refactor and the per-member group-resize undo remain on the deferred list, alongside the integration-test suite.

@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from 0a4df4f to 4675cb9 Compare July 11, 2026 12:51
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Third wave landed — hands-on regressions fixed + a lane-model change (product decision by Ular).

The previous wave's hands-on testing surfaced two regressions the unit gates missed, plus the root cause of the long-standing drag jank. All were reproduced live in a browser against a real project before fixing, and re-verified the same way after (file diffs as ground truth).

Regressions fixed:

  • Marquee collapse — the stale-selection data-loss fix collapsed the marquee's own set when the gesture's async inspector-open notify resolved late. setSelectedElementId gains an explicit preserveSet opt-out, used only when the late-resolving primary is already a member of the live selection; a fresh click on a non-member still collapses (both directions regression-tested).
  • Phantom inserts on horizontal drags — the insert gesture armed across ~64% of each lane's height (INSERT_BAND = 0.32), so plain time-moves became lane-changes, triggering the z machinery and preview churn. The band is now geometry-exact: only the visible gutter between clip bodies.

Lane model change (the root cause, user-approved): display lanes were derived from global z-rank and re-persisted onto every clip on each edit — so one clip's horizontal move could rewrite other clips' track indexes and mint z values (file-diff-proven). The timeline now uses stable track lanes: a clip's lane is its data-track-index, ordered per kind-zone; z-index is canvas paint order only (the deliberate vertical-move stacking sync remains the one place z updates). The commit has three outcomes — pure time-move (writes only the dragged clip's start), lane change (writes only the dragged clip), and gap-insert (the one sanctioned multi-clip write: a minimal +1 renumber). resolveDropIntentZ is deleted. timelineZones tests were rewritten to pin the new contract, including the live repro: a horizontal move among overlapping neighbors diffs exactly one element.

Insert UX (user-specified): the insert indicator only triggers in the gap band between rows; inserting above the top row works; a drop whose aim is occupied creates an adjacent track (pointer-nearest gap) instead of snapping back to origin.

Verification: full per-branch gates (tsc, suites at the known 18-failure baseline, fallow with zero suppressions, format, filesize, builds, exact tip parity) plus pointer-driven browser verification on a real project: marquee/collapse both directions, horizontal-drag one-element file-diff invariant, zero z-order requests, zero reload fetches (the edit blink is gone), no-op drags write nothing.

@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from 4675cb9 to 6be93e9 Compare July 11, 2026 21:08
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Round-3 items landed. Owning the miss first: "all 25 re-gated green" was wrong — the gate script printed per-head vitest counts without asserting them. It now fails any branch whose count deviates from the 18-failure baseline (tolerating only the documented happy-dom GC flake by name), and this submit ran fully green under that assertion.

@ukimsanov ukimsanov force-pushed the studio-dnd/pr13-nle-shell-components branch from 41d9a4b to 37f90ba Compare July 11, 2026 22:38
@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from 6be93e9 to 2a55fa3 Compare July 11, 2026 22:38
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Restacked onto current main (v0.7.53, 1dfb016f3) so the merge train starts clean — main is now contained in every head's ancestry, so nothing from its 78 commits can be lost.

Two conflict sites total, both resolved semantically (full per-file log in the stack's conflict report):

Post-restack verification: full per-branch gates on all 25 heads — tsc, complete suites at the known baseline with the per-head failure-count assertion, fallow (zero suppressions), format, filesize, builds — all green, including main's newer suites (graded-element, sdkCutover, variable-promote) running against this stack's engine.

Standing: #2206#2213 as one ordered unit; no stamps pending Abhay Zala per channel rule. Merge-ready from our side.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current head 2a55fa3 still has actionable structural issues from prior review: DomEditOverlay declares onDeleteSelection/onApplyZIndex but never forwards them (silent no-op boundary), and three duration-extension regexes duplicate fragile attribute-order/double-quote assumptions and can diverge from DOM-root selection. Resolve or explicitly supersede these before approval.

@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Two corrections on the final-sweep readout (verified at the current heads):

@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from 2a55fa3 to 4c1399b Compare July 11, 2026 23:26
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Duration-regex triple: closed. One shared DOMParser-based reader (readRootCompositionDuration) + a byte-preserving attribute splice for writes (patchRootCompositionDuration) replaces all three regex sites (extendRootDurationInSource, extendCompositionDurationIfNeeded, setCompositionDurationToContent). Handles swapped attribute order, single quotes, and whitespace — the silent-no-op cases — with tests for each; the write path splices only the data-duration value (full-document re-serialization measurably reformats unrelated markup, so it was rejected). Lands at #2205 (helper + glue variant) and #2213 (final + tests). Full 25-head gates green under the count assertion; resubmitted. That closes the last deferred code item from the review rounds.

@ukimsanov ukimsanov force-pushed the studio-dnd/pr13-nle-shell-components branch from 37f90ba to 790de69 Compare July 12, 2026 01:03
@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from 4c1399b to fc1973d Compare July 12, 2026 01:03
What: extends 21 glue files so the OLD timeline/canvas engine and the NEW
NLE components type-check side by side: playerStore (multi-select setters,
zoom pin, snap toggle, non-reactive scale scratch), drag-state types gain
optional NLE fields, timelineLayout/timelineAssetDrop/timelineEditingHelpers/
timelineEditing/timelineElementHelpers/studioHelpers/assetHelpers gain the
NLE exports, DomEditOverlay + gestures + AssetContextMenu + Timeline props
gain optional callbacks/params, contexts gain *Optional hooks, and
TimelineEditCallbacks.onMoveElements becomes a bivariant method accepting
both engines' change shapes. patchDocumentRootDuration's test rides along.

Why: this is the keystone that dissolves the old "welded glue" problem —
every symbol the NLE components need is ADDED next to what the old engine
still uses, so the engine components and the swaps can land as separate
reviewable PRs.

How: 15 authored intermediate files (main content + additive symbols; no
behavior changes — new fields optional, new callbacks unused until wired)
plus 6 files whose final content is already purely additive. New exports
without consumers yet carry TEMP(studio-dnd) ignoreExports entries, removed
by the app-shell swap.

Test plan: tsc --noEmit in studio + studio-server (verifies BOTH engines
compile); bunx vitest run (full suite green incl. the 6 new
patchDocumentRootDuration tests); fallow audit clean.
@ukimsanov ukimsanov force-pushed the studio-dnd/pr13-nle-shell-components branch from 790de69 to 6a03b3c Compare July 12, 2026 01:22
@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from fc1973d to ee487d0 Compare July 12, 2026 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants