Skip to content

feat(studio): NLE shell assembly (unwired)#2209

Open
ukimsanov wants to merge 1 commit into
studio-dnd/pr17-canvas-chrome-componentsfrom
studio-dnd/pr18-nle-shell-assembly
Open

feat(studio): NLE shell assembly (unwired)#2209
ukimsanov wants to merge 1 commit into
studio-dnd/pr17-canvas-chrome-componentsfrom
studio-dnd/pr18-nle-shell-assembly

Conversation

@ukimsanov

@ukimsanov ukimsanov commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

What

EditorShell (the full editor layout replacing NLELayout + StudioPreviewArea), TimelinePane (timeline host with sub-comp rebasing) and useTimelineEditCallbacks (the callback bag bridging store edits to the timeline), all unwired.

Why

the shell that App swaps to in the final step; reviewing it standalone keeps that swap PR small.

How

new files against the coexistence layer; TEMP(studio-dnd) entries until App mounts EditorShell in the app-shell swap.

Test plan

tsc --noEmit; bunx vitest run (suite unchanged); fallow audit clean.


Stack position 18/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.

Reviewed as part of the 25-PR NLE overhaul stack. Full stack review on #2205 (the keystone). No blockers on this PR. — 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 C (unwired chrome / assembly)

Verdict: 🟢 approve

Reviewed at fdda4e9c. Focus per Phase-2 recon: EditorShell is the mount target of #2213; verify no live-tree consumer, no global mount-time side effects owned by this PR, expanded-clip callbacks preserved, fallow entries tracked.

Unwired assertion — 🟢

Grepped packages/studio/src/**/*.{ts,tsx} at this SHA:

  • EditorShell — the only grep hit outside EditorShell.tsx itself is a doc-comment in packages/studio/src/utils/assetPreviewStore.ts:9 referencing "prop-drilling through the multi-layer EditorShell". No import line, no JSX consumer. Package barrel (packages/studio/src/index.ts) does not re-export EditorShell.
  • TimelinePane — consumed only by sibling EditorShell.tsx:3,228.
  • useTimelineEditCallbacks — consumed only by EditorShell.tsx:6-8,99.

Since EditorShell is unmounted at this SHA and not exported from the package barrel, none of its embedded providers (NLEProvider, TimelineEditProvider) fire their effects. The single useTimelinePlayer() call inside NLEProvider is dormant; the live NLELayout remains the sole player instance until #2213 swaps the mount.

Extraction correctness (expanded-clip locality) — 🟢

The expanded-sub-comp local-coordinate wrappers previously in NLELayout (toLocalElement, handleMoveElement, handleResizeElement, handleSplitElement, handleDeleteElement plus trackStudioExpandedClipEdit telemetry) are relocated into TimelinePane.tsx (grep at #2209 head confirms expandedParentStart, toLocalElement, trackStudioExpandedClipEdit all present at lines 71–120+). The Escape-to-pop-composition keyboard handler is preserved in EditorShellBody:195. Semantics match NLELayout at the callsites.

Mount-time side effects — 🟢

EditorShell.tsx body has no useEffect / useLayoutEffect / mount subscription of its own — it's pure composition. Every side effect at mount comes from providers it wraps, and those providers are the same ones NLELayout already wraps in the live path. No new global listener (no window.addEventListener, no document.fullscreenchange subscription — the fullscreen store hook lived only in NLELayout and is intentionally not carried into this shell). Activating the mount via #2213 will not introduce a net-new subscription surface, only relocate the existing one.

Fallow lifecycle — 🟢

Added to .fallowrc.jsonc:

  • unusedExports block: EditorShell.tsx, nle/TimelinePane.tsx, nle/useTimelineEditCallbacks.ts — 3 entries.
  • duplication ignore block: StudioPreviewArea.tsx, nle/useTimelineEditCallbacks.ts, EditorShell.tsx, nle/PreviewOverlays.tsx — 4 entries.

Note: StudioPreviewArea.tsx in the duplication ignore block is a live file, not one of this PR's additions. It's ignored here because the coexistence-window makes it structurally duplicate against the new EditorShell. That entry needs to be removed alongside the block at #2213 when StudioPreviewArea presumably slims to just mount EditorShell (or the duplication naturally resolves as the new shell takes over). Flagging so Phase-3 can confirm #2213 removes this entry and not just the newly-added ones.

Notes for Phase 3 cross-check

🟡 The StudioPreviewArea.tsx duplication ignore entry (line 458 in .fallowrc.jsonc per patch context) is coupled to the outcome of the #2213 swap, not just deletion of new files. Phase-3 should verify #2213's fallow-delta drops this entry, or the studio-dnd/pr22 cleanup does — otherwise a stale "coexistence-window" ignore lingers on a live file that no longer duplicates.

R1 by Via

@ukimsanov ukimsanov force-pushed the studio-dnd/pr17-canvas-chrome-components branch from 2c33b51 to 6e29370 Compare July 11, 2026 02:56
@ukimsanov ukimsanov force-pushed the studio-dnd/pr18-nle-shell-assembly branch from fdda4e9 to 29d6c83 Compare July 11, 2026 02:56
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Closing the loop on the fallow-entry lifecycle note: confirmed — #2213 deletes StudioPreviewArea.tsx outright and its checkout of the final .fallowrc.jsonc removes the duplication-ignore entry along with every other TEMP(studio-dnd) block (the gates assert zero TEMP residue at that position).

@ukimsanov ukimsanov force-pushed the studio-dnd/pr18-nle-shell-assembly branch from 29d6c83 to ddb8f80 Compare July 11, 2026 08:18
@ukimsanov ukimsanov force-pushed the studio-dnd/pr17-canvas-chrome-components branch from 6e29370 to d1e68f9 Compare July 11, 2026 08:18
@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 — multi-select resize contract regression + telemetry gap

Two NEW findings from source-level trace:

#1 — Multi-select resize dropped in the swap contract (real regression vs legacy)

The callback bag returned from useTimelineEditCallbacks.ts:83-98 exposes:

  • onMoveElement, onMoveElements — batch move ✓
  • onResizeElement — single resize ✓
  • onSplitElement, onRazorSplit*, onToggleTrackHidden, onBlockedEditAttempt

But NO onResizeElements, onPreviewMoveElements, onPreviewResizeElements. Meanwhile TimelineEditContext.tsx + legacy StudioPreviewArea.tsx wire all four:

  • onResizeElements: handleTimelineGroupResize
  • onPreviewMoveElements: handleTimelineGroupMovePreview
  • onPreviewResizeElements: handleTimelineGroupResizePreview

Compounding: even if useTimelineEditCallbacks were extended to declare these deps, TimelinePane.tsx:47-49 destructures only {onMoveElement, onMoveElements, onResizeElement, onSplitElement} from useTimelineEditContext(). So a fix that only touches the callback bag lands with the wiring still dead — two-place hazard.

This isn't just an atomic-undo tax (Abhai's #2212 reframing gets that right). It's a feature contract regression: multi-select resize + preview optimism don't exist in the new NLE at this PR's shape. Every other edit shape has both singular AND plural forms in the callback surface; resize alone is missing both plural + preview.

Fix: (a) declare handleTimelineElementsResize + the two preview handlers in TimelineEditCallbackDeps, (b) return them from useTimelineEditCallbacks, (c) extend TimelinePane.tsx's destructure + <Timeline .../> prop-forwarding to include the three new plural/preview names.

#2 — Batch-move telemetry gap in TimelinePane

TimelinePane.tsx:80-97 handleMoveElements rebases each expanded sub-comp child by basis but never calls trackStudioExpandedClipEdit, unlike its 4 siblings in the same file:

  • handleMoveElement (:65) — calls it ✓
  • handleResizeElement (:107) — calls it ✓
  • handleDeleteElement (:118) — calls it ✓
  • handleSplitElement (:129) — calls it ✓
  • handleMoveElements (:80-97) — silent ✗

Every other edit on an expanded child emits the telemetry; only the batched-move variant is silent. Pure asymmetric miss.

— Rames Jusso

@ukimsanov ukimsanov force-pushed the studio-dnd/pr18-nle-shell-assembly branch from ddb8f80 to 49c25f9 Compare July 11, 2026 10:49
@ukimsanov ukimsanov force-pushed the studio-dnd/pr17-canvas-chrome-components branch from d1e68f9 to 645a3cc Compare July 11, 2026 10:49
What: EditorShell (the full editor layout replacing NLELayout +
StudioPreviewArea), TimelinePane (timeline host with sub-comp rebasing) and
useTimelineEditCallbacks (the callback bag bridging store edits to the
timeline), all unwired.

Why: the shell that App swaps to in the final step; reviewing it standalone
keeps that swap PR small.

How: new files against the coexistence layer; TEMP(studio-dnd) entries
until App mounts EditorShell in the app-shell swap.

Test plan: tsc --noEmit; bunx vitest run (suite unchanged); fallow audit
clean.
@ukimsanov ukimsanov force-pushed the studio-dnd/pr17-canvas-chrome-components branch from 645a3cc to dd1310c Compare July 11, 2026 12:51
@ukimsanov ukimsanov force-pushed the studio-dnd/pr18-nle-shell-assembly branch from 49c25f9 to 6fec739 Compare July 11, 2026 12:51
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.

4 participants