fix(studio): block element selection while composition is loading#695
Conversation
Prevent users from selecting elements in the preview while the composition is still loading (showing "Loading composition" overlay). Selection and hover highlighting are suppressed until the player fires the ready event. Also reverts motion panel and manual drag editing defaults to false — these were accidentally set to true during the PR #693 merge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vanceingalls
left a comment
There was a problem hiding this comment.
Post-merge advisory review (this PR is already merged into next). Findings flagged for follow-up, not gating.
Verdict — Approve, but this PR exposes a process gap worth flagging.
The actual behavioral change is correct: suppress preview selection/hover while composition is loading. The revert of the Motion + manual-drag defaults to false is also correct (they were never supposed to be on by default).
Follow-ups
-
The defaults revert is the second time these flags flipped in 24 hours. PR #693 set them to
true, this PR sets them back tofalse. Reason: that's two product-state changes forSTUDIO_MOTION_PANEL_ENABLEDandSTUDIO_PREVIEW_MANUAL_EDITING_ENABLEDin a single day's stack. Worth a process question: should alpha feature-flag defaults require an explicit dedicated PR (not buried in unrelated commits)? The "accidentally set during merge conflict resolution" pattern is hard to catch in review. -
onCompositionLoadingChangeis plumbed throughNLELayout→App.tsxvia a new prop anduseEffect. This effect fires whenevercompositionLoadingflips. Good. But there's no test for the prop callback firing inNLELayout.test.ts. Reason: silent regression risk — if a future PR removes theuseEffector changes whensetCompositionLoadingfires, selection-blocking will silently stop working and the loading-overlay-but-still-selectable bug returns. -
Selection-blocking is gated on
compositionLoadingfromNLELayout, but PR #707 commit 2 expands this to also includeshowAssetOverlay. That's a follow-up gap: this PR's protection only covers composition-readiness, not asset-readiness. Worth confirming the PR #707 fix made it all the way (verified in the Player.tsx diff:onCompositionLoadingChange?.(showCompositionOverlay || showAssetOverlay)— yes it did).
Important
- The PR body is 3 lines. For a fix that ships a behavior change plus a defaults revert, the body should explicitly call out the second commit. Reason: makes the audit trail clearer for the auditor (or for Vai 6 weeks later) trying to understand why defaults flipped.
Nits
- "472 studio tests pass" — would be even more useful with a count delta vs. main (i.e. "added 0 tests").
Praise
- The fix is correctly scoped: prop callback up, two handler guards in App.tsx. No spurious refactoring.
- Catching the accidental defaults flip from #693 within 2 hours is exactly the right responsiveness.
— Vai
Summary
compositionLoadingstate is forwarded fromNLELayout→App.tsxvia a newonCompositionLoadingChangeprop.false— these were accidentally set totrueduring the PR fix: alpha preview e2e fixes — exports, init templates, EPIPE crash #693 merge conflict resolution.Test plan
🤖 Generated with Claude Code