fix(studio): keep the preview alive when the window is tight - #3091
Conversation
Studio stored its three panel dimensions as absolute pixels with hard floors and reconciled them against the window only at mount. The preview is the only flexible region, so it absorbed every squeeze. Measured before: at a 760px window the preview was 192px wide; at 560px it was 2px wide on a fresh load; dragging a 1100px window from 760 to 520 tall left it 47px tall, showing only the transport bar. Add fitPanels, one pure owner of the 'who yields' decision for both axes, and route every caller through it: - the preview gets a 360x200 floor before any panel gets its preference - panel caps are relative to the window, replacing a flat 600px inspector cap that was no constraint at all on a small screen - a window resize re-reconciles both axes instead of only mount - below 860 the sidebar renders as its existing rail, below 700 the inspector collapses too; thresholds are derived from the minimums Auto-collapse is derived render state, never a write to user intent: leftCollapsed stays in localStorage and rightCollapsed stays in the shareable URL untouched, and an explicit reopen overrides the rail so the Inspector button still works on a narrow window. Measured after: 760 -> 433px, 560 -> 516px, 1100x520 -> 497x196. Windows at or above 1280px are unchanged.
vanceingalls
left a comment
There was a problem hiding this comment.
R1 adversarial pass — approve. Well-designed, thoughtfully-tested fix; the extraction is clean, the invariants are documented, and the tests encode the load-bearing "auto-collapse never writes user intent" contract explicitly.
Head SHA reviewed: 1407a3b01b57acf742717aca414dea63bb5c9e14
What I checked (adversarial lenses):
- useEffect for state syncing — one candidate,
packages/studio/src/hooks/usePanelLayout.ts:81-86(override reset). Details below. Non-blocking. - Fast Refresh — component files export only components —
fitPanels.tsis.tsand hook files are.ts, soreact-refresh/only-export-componentsdoesn't apply. Clean. - List virtualization — no new lists.
- Element-identity keying — no key changes.
- WebGL teardown — not touched.
- DOM retainer leaks — new
ResizeObserveratpackages/studio/src/components/nle/NLEContext.tsx:281-289disconnects on cleanup; newresizelistener atusePanelLayout.ts:70-74removes on cleanup; no.bind(); no closure retainers. - Mid-drag reads live state —
handlePanelResizeStartreadsfittedRef.current[side](rendered width), which is exactly what avoids the seam-jump-mid-drag class of bug. Correct. - Idempotence under ResizeObserver —
fitPanelWidthsandfitTimelineHeightboth have explicit idempotence tests. RO cannot oscillate. - Editor-UI parity lens set — preview floor is symmetric across mount / resize / drag / keyboard / preference restore because all five paths route through the same helper. That's the whole point of the refactor.
- Ambient setup lost in extraction — the four prior clamp sites (getInitialPanelWidths, mount effect in NLEContext, drag handler, keyboard handler) all now route through
fitPanels; nothing dropped. Keyboard handler's fallback for missing container height correctly flipped from?? Infinityto?? 0sofitTimelineHeightshort-circuits to the preferred value. - Cancellation / signal chain — no debounce/throttle, no rAF. Resize is a plain synchronous setState. Fine.
- Aggregate silence: novelty vs abort — auto-collapse deliberately doesn't emit
panel_toggle. Consistent with the design (auto-collapse is derived render state, not intent), and I agree with the choice. If you ever want to measure "how often is the shell railing users?" that's a distinct new event, not a repurposing ofpanel_toggle. - Semantic tokens / hardcoded colors / native elements — no new className or JSX-element additions in the diff.
i18n/ mobile / images — n/a; this is layout math.
Findings
-
nit — state-sync useEffect (
usePanelLayout.ts:81-86). The override-reset effect (if (fitted.autoCollapseLeft || fitted.autoCollapseRight) return; setAutoCollapseOverride(...)) matches the pattern CLAUDE.md flags as ""no useEffect for state syncing"" — it's setState-in-response-to-derived-state, not external-system sync. It's guarded against re-fires so it can't loop, and the tests cover the state machine it produces. The natural alternative is to reset inside the resize handler when the new width crosses back above both rail thresholds (that's an event handler, not an effect). Not blocking; the effect is contained and correct as-is. -
nit —
setLeftCollapsed/setRightCollapsedreturn-value asymmetry (usePanelLayout.ts:230-235).setRightCollapsedis wrapped bysetRightCollapsedWithOverride, butsetLeftCollapsedis returned raw. Today the only left-side un-collapse path istoggleLeftSidebar(which handles the override internally), so this is dormant. But a future caller doingsetLeftCollapsed(false)from the exposed context below the 860px threshold would open the panel for a paint and then get immediately railed again — exactly the bug ""an explicit reopen beats the rail"" is designed to prevent on the right. Either mirror the wrapper or drop the raw exposure. Follow-up, not blocking. -
nit — double
setPreferredinsidecommitPanelWidth(usePanelLayout.ts:127-133). TwosetPreferred(side, ...)calls in one event tick — the first is immediately overwritten. Batched, so functionally identical to one call, but a reader has to convince themselves of that. Could beconst settled = fitPanelWidths(readViewportWidth(), { ...preferredRef.current, [side]: Math.max(0, Math.round(width)) })[side]; setPreferred(side, settled);— one write, same result. Style. -
nit —
RAIL_Wused for budget vs. actual rendered rail width.fitPanelWidthscomputes preview budget assuming the collapsed sidebar occupiesRAIL_W = 40, but the actual rail element inStudioLeftSidebar.tsxismr-0.5 ... w-10 ... border(≈ 42-44px accounting for margin + border). Off-by-a-few-pixels; not visible at any of the screenshotted breakpoints. Style.
CI: all completed checks green as of head SHA; two Windows-render jobs and CI-Build / Typecheck / CLI-smoke were mid-run when I looked — no failures observed. The prior push of the same head has full green history.
Tests: the ""projected preview widths"" table in fitPanels.test.ts is exactly the frozen-literal audit the plan asks for — I reproduced the arithmetic for 1680 / 860 / 700 / 699 / 560 and it lines up. The usePanelLayout.test.ts case that asserts readStudioUiPreferences().leftCollapsed === undefined after a 560px squeeze is the load-bearing contract test; keep it.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 1407a3b01.
The bug reproduction, screenshots, derived-threshold math, and the "auto-collapse is derived render state, never a write to intent" framing are all excellent — this closes the actual preview-collapses-to-2px UX with a clean, DOM-free helper and idempotent reconciliation.
Two blockers on the same shape, one on each axis, both violate the PR body's own design contract:
- Left rail:
toggleLeftSidebaris dead when clicked from the auto-rail state, and silently flips persisted intent to collapsed. Inline atusePanelLayout.ts:143. - Right header:
StudioHeader.tsx:331's Inspector-button branch reads rawrightCollapsedinstead ofeffectiveRightCollapsed, so clicking the button in the auto-rail-with-design-tab-active state silently setsrightCollapsed=trueand, since it's URL-synced, rewrites a shareable link. Inline atusePanelLayout.ts:231(fix lives in the untouched caller file, described inline).
Both are shaped like "a click made because the width auto-rail was in effect ends up writing the user's persisted or URL-synced intent" — the exact anti-pattern the PR body flags as the reason to have effective* at all.
One asymmetry concern: timeline height doesn't restore its preferred value after a shrink-then-grow the way panel widths do. Inline at NLEContext.tsx:286 with a suggested refactor (preferredTimelineH state + derived timelineH, symmetric with widths).
What lands cleanly:
- The derived thresholds (
200 + 280 + 360 + 6 = 846 → 860,40 + 280 + 360 + 6 = 686 → 700) with the "change a minimum and move the matching threshold with it" invariant is the right shape — no magic numbers in the reconciliation logic, and the boundary-pair tests (fitPanels.test.ts:74/80— 860/859 and 700/699) pin it exactly. - Frozen literals in
defaultPanelWidthsregression tests (:20-27) — the "wide windows untouched" promise stays a real guardrail rather than collapsing into a tautology after the hook was rewritten to delegate here. The rationale comment ("asserting against the live hook would be circular and would silently stop guarding the promise") is worth its ink. - Idempotence of
fitPanelWidths(:119-125) andfitTimelineHeight(:151-154) means the newResizeObserverinNLEContext.tsxcannot oscillate — a genuine risk with observer-driven state, closed by construction rather than by a debounce. leftFloor/rightFloorsplit (:100-101+ test:87-93) prevents an expanded sidebar from being driven down to rail width by the squeeze fallback — this is a subtle correctness point that would silently break if forgotten in a refactor.commitPanelWidthreadswindow.innerWidthdirectly viareadViewportWidth()(:124) instead of the stateviewportWidth, correctly picking up a mid-drag resize that hasn't landed in state yet. Comment could name why; the code is right.fittedRef(:86-87) means drag handles anchor from the rendered width, so the seam doesn't jump when the panel is currently narrower than its stored preference — clean fix for the "start-drag snap" that would otherwise appear.- The
ResizeObservercontainer isEditorShell.tsx:248's outer flex parent (flex flex-col flex-1 min-h-0), whose height is externally-driven by App shell rather than bytimelineHitself — sosetTimelineH(…)can't retrigger the observer through layout. No infinite loop. - Test coverage is well-focused:
fitPanels.test.tspins the projected preview widths at 10 viewport sizes against literal expected values, and the vertical case at:135pins the exact 477-tall / 429-preferred numbers from the bug report.
The two blockers are the same generic anti-pattern in two places, and each closes with either a small caller-side change (right) or a small hook-side branch (left). I'd want them fixed — or the auto-collapse scoped narrower to avoid the click paths — before this merges. Detail and repro traces on the inline threads.
| const next = !collapsed; | ||
| writeStudioUiPreferences({ leftCollapsed: next }); | ||
| trackStudioEvent("panel_toggle", { panel: "left_sidebar", collapsed: next }); | ||
| if (!next) setAutoCollapseOverride((prev) => ({ ...prev, left: true })); |
There was a problem hiding this comment.
🔴 Blocker — toggleLeftSidebar is dead in the auto-rail state, and silently flips persisted intent to collapsed.
Setup: user at 700px window, has never manually collapsed the left sidebar. State: leftCollapsed=false, fitted.autoCollapseLeft=true, leftCollapsedByWidth=true, effectiveLeftCollapsed=true → StudioLeftSidebar renders the rail with the "Show sidebar" chevron.
User clicks the chevron (StudioLeftSidebar.tsx:79 onClick={toggleLeftSidebar}). Trace with prev=false:
next = !false = true
writeStudioUiPreferences({ leftCollapsed: true }) // persisted intent flips silently
if (!next) … // !true = false, override NEVER set
return true // leftCollapsed becomes true
New state: leftCollapsed=true, autoCollapseOverride.left=false, fitted.autoCollapseLeft=true. effectiveLeftCollapsed = true || (true && !false) = true — still railed. The button labelled "Show sidebar" produced no visible change AND stealth-wrote the user's saved intent to leftCollapsed: true in localStorage. On next Studio load, sidebar opens collapsed (rail even at 1600px) — a preference the user never chose.
This violates the PR-body contract verbatim: "a width-driven collapse [must not] permanently flip a saved preference." The toggle-from-rail path is exactly that — a click made because of the width-driven rail, silently rewriting intent.
Missing coverage: usePanelLayout.test.ts:213 ("keeps an explicitly collapsed sidebar collapsed after a narrow trip") only exercises the wide→collapse direction. :225 covers the right panel's setRightCollapsed(false) direct-set (which routes through setRightCollapsedWithOverride — that's fine), but no test invokes toggleLeftSidebar when effectiveLeftCollapsed && !leftCollapsed. A one-liner would fail today:
it("un-rails an auto-collapsed sidebar via the rail chevron without silently flipping intent", () => {
const harness = renderPanelLayout();
act(() => resizeWindowTo(560));
expect(harness.getState().leftCollapsed).toBe(false); // not user-collapsed
expect(harness.getState().effectiveLeftCollapsed).toBe(true); // rail'd by width
act(() => harness.getState().toggleLeftSidebar()); // user clicks chevron
expect(harness.getState().effectiveLeftCollapsed).toBe(false); // rail should be gone
expect(harness.getState().leftCollapsed).toBe(false); // intent unchanged
expect(readStudioUiPreferences().leftCollapsed).toBeUndefined(); // not persisted
});Suggested fix: teach toggleLeftSidebar to branch on the auto-rail state, symmetric with setRightCollapsedWithOverride. Concretely:
const toggleLeftSidebar = useCallback(() => {
// If the sidebar is only railed by width, an un-rail click is intent-neutral;
// clear the auto-collapse override without touching leftCollapsed at all.
const fittedNow = fittedRef.current;
if (fittedNow.autoCollapseLeft && !leftCollapsedRef.current) {
setAutoCollapseOverride((prev) => ({ ...prev, left: true }));
return;
}
setLeftCollapsed((collapsed) => { /* current body */ });
}, []);(Needs a leftCollapsedRef mirroring preferredRef so the callback can read the freshest value without adding it as a dep — the pattern is already used elsewhere in the hook.)
— Review by Rames D Jusso
| setRightCollapsed: setRightCollapsedWithOverride, | ||
| /** What the shell actually renders: intent OR the window forcing a rail. */ | ||
| effectiveLeftCollapsed: leftCollapsed || leftCollapsedByWidth, | ||
| effectiveRightCollapsed: rightCollapsed || rightCollapsedByWidth, |
There was a problem hiding this comment.
🔴 Blocker — the header Inspector button has the same shape of bug on the right axis. It's a caller in packages/studio/src/components/StudioHeader.tsx (not in this diff), but it consumes what you export here, and the un-rail semantics only work when the caller reads effectiveRightCollapsed rather than the raw rightCollapsed for the branch decision.
StudioHeader.tsx:331 reads rightCollapsed from context and does:
if (rightCollapsed || !inspectorPanelActive) {
setRightCollapsed(false); // wrapper — correctly sets override
return;
}
setRightCollapsed(true); // caller's "close" branchScenario that breaks: 700px window, rightCollapsed=false (user never manually collapsed), effectiveRightCollapsed=true (rail'd), inspector tab is the default "design" → inspectorPanelActive=true. User clicks the header Inspector button expecting to reveal the inspector.
- Condition
rightCollapsed || !inspectorPanelActive=false || false= false → falls into the CLOSE branch. setRightCollapsed(true)—setRightCollapsedWithOverridesets user intent to closed.effectiveRightCollapsed = true || …= true — panel still hidden.- Because
rightCollapsedis URL-synced (viauseStudioUrlState, per the PR body's own design note), the shareable URL now carriesrightCollapsed=1. If the user was in the middle of preparing a link to share — the exact ten-second window drag scenario the PR body calls out — the flip is silent and remote.
The wrapper you added here is right; the caller's check is what's stale. Two ways to close it:
- Export the
effective*flags and haveStudioHeader.tsx:331useeffectiveRightCollapsed || !inspectorPanelActivefor the branch. This is the smallest change and matches the design note verbatim. - Fold the semantics into the hook: return an
expandRightInspector()action that unconditionally setsrightCollapsed=false+ override, and have the header call that in the OPEN branch. This scales better if more callers (dom-select auto-open, block install, caption edit) grow the same pattern; they already all passfalseunconditionally so they'd be trivial to migrate.
Same missing-coverage story as the left toggle: usePanelLayout.test.ts:225 covers setRightCollapsed(false) when the panel is auto-collapsed, but nothing covers the header caller's actual condition wiring. A caller-level test in a new StudioHeader.test.tsx (or a hook-level test that pretends to be the header — if (rightCollapsed || !inspectorPanelActive) setRightCollapsed(false); else setRightCollapsed(true); — from the rail'd default-design state) would fail today.
— Review by Rames D Jusso
| const reconcile = () => { | ||
| const containerH = element.getBoundingClientRect().height; | ||
| if (!containerH) return; | ||
| setTimelineH((prev) => fitTimelineHeight(containerH, prev)); |
There was a problem hiding this comment.
🟡 Concern — timeline height is asymmetric with panel widths: shrink-then-grow forgets the user's preferred height.
Widths are stored as preferredWidths state, fitPanelWidths reads them fresh on every render, and the fitted output is never persisted-back — so a temporary squeeze narrows the rendered value while the preferred survives, and the width returns when the window grows (asserted at usePanelLayout.test.ts:201). Heights don't have that: timelineH state is the preferred value, and the ResizeObserver here overwrites prev with the fitted result via setTimelineH((prev) => fitTimelineHeight(containerH, prev)).
Repro: user loads Studio at 760 tall, timeline preference 429. Drags window to 520 tall → fitTimelineHeight(400, 429) = 200, timelineH = 200. Drags back to 760 → fitTimelineHeight(640, 200) = 200, timeline stays at 200. The user's real preference (429) is now unrecoverable within this session without manual drag. localStorage still says 429 (persistTimelineH isn't called from reconcile, correct), so a reload restores it — but any within-session narrow trip loses it.
The PR-body contract for widths — "a real preference survives a temporary squeeze and returns when the window grows" — doesn't hold for the height axis. Given how deliberately the width axis was structured to guarantee this, the divergence feels accidental rather than intended.
Suggested shape, mirroring the width axis:
const [preferredTimelineH, setPreferredTimelineH] = useState(/* from prefs, as today */);
const [containerH, setContainerH] = useState(0);
const timelineH = fitTimelineHeight(containerH, preferredTimelineH); // derived per render
useEffect(() => {
const element = containerRef.current;
if (!element || typeof ResizeObserver === "undefined") return;
const observer = new ResizeObserver(() => {
setContainerH(element.getBoundingClientRect().height);
});
observer.observe(element);
setContainerH(element.getBoundingClientRect().height);
return () => observer.disconnect();
}, []);Then TimelineResizeDivider's drag/keyboard paths set preferredTimelineH (rather than timelineH), and persistTimelineH writes preferredTimelineH. Same idempotence guarantee as fitPanelWidths — the observer never triggers a state change unless the container height itself changes.
Non-blocking; the current shape is a strict improvement over the mount-only clamp, and no user-visible break is worse than the pre-PR 47px preview. Naming it because the design contract diverges and it's a small refactor to close.
— Review by Rames D Jusso
Review found two ways the auto-collapse contract leaked into stored intent, both of them dead clicks that wrote state the user never asked for. - The rail's Show sidebar button toggled stored leftCollapsed, which was already false while the window held the rail. The click flipped it to true, persisted that, and left the rail railed. The toggle now reads the effective state, so one click opens the sidebar and the preference is untouched. - The header Inspector button branched on the raw rightCollapsed for the same reason, taking the close branch and writing rightCollapsed=true. That value is synced into the shareable Studio URL, so a click that did nothing visible rewrote the link. The branch is now a named helper that takes the effective state. Reopening a railed panel also has to give it a real width: fitPanelWidths now takes the reopened sides, otherwise an expanded sidebar rendered at the 42px rail and squashed its own content. Also from review: RAIL_W is the measured 42px footprint (40 box + 2px margin) rather than 40; the override reset moved out of an effect watching derived state and into the resize handler that owns the width; commitPanelWidth writes once instead of twice; setLeftCollapsed is no longer exported, since toggleLeftSidebar is the only safe writer. Verified in the browser at 760 and 640: the sidebar opens on one click with leftCollapsed still false, and the inspector opens with rc unchanged at 0 in the URL.
What
Studio's preview pane no longer collapses when the window is tight. Panel sizes are now reconciled against the window on every resize, with the preview holding a 360 x 200 floor that panels yield to.
Measured on a real project, preview pane rect before and after:
Half of a laptop screen, 760 x 760. Panels held 560px of a 760px window; the video got a 192px strip.
A third of the screen, 560 x 760. There was no video at all, on a fresh load.
Dragged shorter after loading, 1100 x 520. The timeline kept its saved 429px height and the video collapsed to a transport bar.
Full screen, 1440 x 760, the control. Identical in both runs, which is the no-regression case.
Why
The three panel dimensions (left width, right width, timeline height) were stored as absolute pixels with hard floors, and reconciled against the window only at mount. The preview is
flex-1 min-w-0, so it was the only region with no floor to defend itself and absorbed every squeeze.Two independent expressions of the same bug:
600, which is no constraint at all on a small window.useEffectwith[]deps. Its own comment named the failure it guarded against; it just stopped guarding after mount. Loading at 760 tall and dragging to 520 left the timeline at its saved 429px and the preview at 47px, showing only the transport bar.Split-screen and half-window layouts are common while working alongside an editor or docs, and in both cases the video — the thing being edited — was the first casualty.
How
One pure, DOM-free helper (
packages/studio/src/utils/fitPanels.ts) owns the "who yields" decision for both axes. Every caller routes through it: mount, resize, drag, keyboard nudge, preference restore. It previously lived as four separate rules in four files.Reconciliation order, each step only running if the previous left the preview under its floor:
Thresholds are derived, not chosen.
200 + 280 + 360 + 6 = 846, so 860 is the first width where everything genuinely fits;40 + 280 + 360 + 6 = 686, so 700 is the second. Changing a minimum moves the matching threshold with it.Two design decisions worth flagging for review:
leftCollapsedis persisted to localStorage andrightCollapsedis synced into the shareable Studio URL. If a width-driven collapse wrote either, a ten-second window drag would permanently flip a saved preference and rewrite a link the user might be about to share. The hook now exposeseffectiveLeftCollapsed/effectiveRightCollapsedfor rendering while persistence and URL sync keep consuming intent.Reconciled widths are never persisted — only an explicit drag or keyboard nudge writes a preference, so a real preference survives a temporary squeeze and returns when the window grows.
Windows at or above 1280px are byte-identical to before. The existing default upper bounds (384 / 424) are untouched; only the floors drop and the caps become relative. An earlier draft dropped the 424 bound and silently made the preview smaller at 1680px, so the no-regression assertions below use frozen literals rather than calling the (now-modified) default function, which would have been circular.
Test plan
Automated — 46 new assertions across two suites, full Studio suite green (3533 passed, 0 failed):
fitPanels.test.ts— the projected preview width at ten window sizes; boundary pairs at 860/859 and 700/699; the 40% cap; preview-floor-beats-cap; a viewport narrower than the floor alone; an unmeasured (0-width) container; idempotence, so aResizeObservercannot oscillate; and the vertical axis including the exact 477-tall / 429-preferred case from the bug.usePanelLayout.test.ts— window-relative cap on a 700px window; both rails at 560; auto-collapse leaves localStorage and the URL-synced intent untouched; the user's width returns when the window grows back; an explicitly collapsed sidebar stays collapsed after a narrow round trip; an explicit reopen overrides the rail.Manual — the screenshots above, captured from the same project at four window sizes with the preview rect read via
getBoundingClientRect()in each. The before column was captured from an unmodified checkout so the only variable is this commit.Not covered by this PR
Deliberately out of scope, visible in the 560px screenshot and landing separately:
Exportfalls off the right edge and the project name does not truncateAlso not in scope: any phone or tablet layout (Studio is a desktop NLE), and saved workspace presets.