Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/studio/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ export function StudioApp() {
} = useInspectorState(
panelLayout.rightPanelTab,
panelLayout.rightInspectorPanes,
panelLayout.rightCollapsed,
panelLayout.effectiveRightCollapsed,
isPlaying,
domEditSession.domEditSelection,
gestureState === "recording",
Expand Down Expand Up @@ -512,7 +512,7 @@ export function StudioApp() {
/>
}
right={
panelLayout.rightCollapsed ? null : (
panelLayout.effectiveRightCollapsed ? null : (
<StudioRightPanel
designPanelActive={designPanelActive}
activeBlockParams={activeBlockParams}
Expand Down
28 changes: 28 additions & 0 deletions packages/studio/src/components/StudioHeader.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { describe, expect, it } from "vitest";
import { shouldOpenInspector } from "./StudioHeader";

describe("shouldOpenInspector", () => {
it("opens when the panel is hidden", () => {
expect(shouldOpenInspector(true, false)).toBe(true);
});

it("opens when a non-inspector tab is showing", () => {
expect(shouldOpenInspector(false, false)).toBe(true);
});

it("closes when the inspector is genuinely on screen", () => {
expect(shouldOpenInspector(false, true)).toBe(false);
});

it("opens when the window railed the panel away", () => {
// The regression this guards: the button used to branch on the raw
// rightCollapsed intent, which is still `false` while the window has the
// panel railed. That took the close branch, wrote rightCollapsed=true, and
// since that value is synced into the shareable Studio URL, a click that
// did nothing visible rewrote the link.
const userIntentIsOpen = false;
const windowRailedItAway = true;
expect(shouldOpenInspector(windowRailedItAway, true)).toBe(true);
expect(shouldOpenInspector(userIntentIsOpen, true)).toBe(false);
});
});
23 changes: 21 additions & 2 deletions packages/studio/src/components/StudioHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,21 @@ export function ViewModeToggle() {
);
}

/**
* Does the header's Inspector button open the panel, or close it?
*
* Takes the EFFECTIVE collapse state, so a panel the window has railed away
* counts as closed even though the user's stored intent still says open. The
* argument name is the guard: passing raw intent here is the bug this exists
* to keep out.
*/
export function shouldOpenInspector(
effectiveRightCollapsed: boolean,
inspectorPanelActive: boolean,
): boolean {
return effectiveRightCollapsed || !inspectorPanelActive;
}

// fallow-ignore-next-line complexity
export function StudioHeader({
captureFrameHref,
Expand All @@ -208,7 +223,11 @@ export function StudioHeader({
onExport,
}: StudioHeaderProps) {
const { projectId, editHistory, handleUndo, handleRedo, renderQueue } = useStudioShellContext();
const { rightCollapsed, setRightCollapsed, setRightPanelTab } = usePanelLayoutContext();
// effectiveRightCollapsed, not the raw intent: in the auto-railed state the
// intent is still "open" while the panel is hidden, so branching on intent
// made this button write rightCollapsed=true — and that value is synced into
// the shareable Studio URL, so a dead click would rewrite a link.
const { effectiveRightCollapsed, setRightCollapsed, setRightPanelTab } = usePanelLayoutContext();
const isRendering = renderQueue.isRendering;

return (
Expand Down Expand Up @@ -328,7 +347,7 @@ export function StudioHeader({
<button
type="button"
onClick={() => {
if (rightCollapsed || !inspectorPanelActive) {
if (shouldOpenInspector(effectiveRightCollapsed, inspectorPanelActive)) {
trackStudioEvent("panel_toggle", { panel: "inspector", collapsed: false });
setRightPanelTab("design");
setRightCollapsed(false);
Expand Down
4 changes: 2 additions & 2 deletions packages/studio/src/components/StudioLeftSidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function StudioLeftSidebar({
onAddCompositionToTimeline,
}: StudioLeftSidebarProps) {
const {
leftCollapsed,
effectiveLeftCollapsed,
leftWidth,
adjustPanelWidth,
toggleLeftSidebar,
Expand Down Expand Up @@ -71,7 +71,7 @@ export function StudioLeftSidebar({
[renderQueue, waitForPendingDomEditSaves],
);

if (leftCollapsed) {
if (effectiveLeftCollapsed) {
return (
<div className="mr-0.5 flex w-10 flex-shrink-0 flex-col items-center rounded-lg border border-neutral-800/50 bg-neutral-950 pt-1">
<button
Expand Down
23 changes: 16 additions & 7 deletions packages/studio/src/components/nle/NLEContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { useTimelinePlayer, usePlayerStore } from "../../player";
import type { TimelineElement } from "../../player";
import type { CompositionLevel } from "./CompositionBreadcrumb";
import { useCompositionStack } from "./useCompositionStack";
import { MIN_TIMELINE_H, MIN_PREVIEW_H } from "./TimelineResizeDivider";
import { MIN_TIMELINE_H, fitTimelineHeight } from "../../utils/fitPanels";
import { setCompositionSourceMap } from "../editor/domEditingDom";
import { ensureMotionPathPluginLoaded } from "../../utils/gsapSoftReload";
import { readStudioUiPreferences, writeStudioUiPreferences } from "../../utils/studioUiPreferences";
Expand Down Expand Up @@ -273,13 +273,22 @@ export function NLEProvider({
}, []);
const containerRef = useRef<HTMLDivElement>(null);
// A height persisted on a tall window can exceed this window's container and
// collapse the flex-1 preview to 0px — clamp once the container is measurable
// (the drag/keyboard paths already clamp; the restore path must too).
// collapse the flex-1 preview to 0px. Observing the container rather than
// clamping once at mount is what makes a window RESIZED after load behave the
// same as one loaded at that size: dragging 760 -> 520 tall used to leave the
// timeline at its stored 429px and the preview at 47px.
useEffect(() => {
const containerH = containerRef.current?.getBoundingClientRect().height;
if (!containerH) return;
const max = containerH - MIN_PREVIEW_H;
setTimelineH((prev) => (prev > max ? Math.max(MIN_TIMELINE_H, max) : prev));
const element = containerRef.current;
if (!element || typeof ResizeObserver === "undefined") return;
const reconcile = () => {
const containerH = element.getBoundingClientRect().height;
if (!containerH) return;
setTimelineH((prev) => fitTimelineHeight(containerH, prev));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

};
reconcile();
const observer = new ResizeObserver(reconcile);
observer.observe(element);
return () => observer.disconnect();
}, []);

const hasLoadedOnceRef = useRef(false);
Expand Down
15 changes: 4 additions & 11 deletions packages/studio/src/components/nle/TimelineResizeDivider.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { useCallback, useRef } from "react";

export const MIN_TIMELINE_H = 100;
export const MIN_PREVIEW_H = 120;
import { MIN_PREVIEW_H, MIN_TIMELINE_H, fitTimelineHeight } from "../../utils/fitPanels";

/**
* Horizontal drag/keyboard-resizable divider between the preview and the
Expand Down Expand Up @@ -41,12 +39,7 @@ export function TimelineResizeDivider({
if (!isDragging.current || !containerRef.current) return;
const rect = containerRef.current.getBoundingClientRect();
const mouseY = e.clientY - rect.top;
const containerH = rect.height;
const newTimelineH = Math.max(
MIN_TIMELINE_H,
Math.min(containerH - MIN_PREVIEW_H, containerH - mouseY),
);
setTimelineH(newTimelineH);
setTimelineH(fitTimelineHeight(rect.height, rect.height - mouseY));
},
[disabled, containerRef, setTimelineH],
);
Expand All @@ -61,10 +54,10 @@ export function TimelineResizeDivider({
if (disabled) return;
if (e.key !== "ArrowUp" && e.key !== "ArrowDown") return;
e.preventDefault();
const containerH = containerRef.current?.getBoundingClientRect().height ?? Infinity;
const containerH = containerRef.current?.getBoundingClientRect().height ?? 0;
const delta = e.key === "ArrowUp" ? 16 : -16;
setTimelineH((prev) => {
const next = Math.max(MIN_TIMELINE_H, Math.min(containerH - MIN_PREVIEW_H, prev + delta));
const next = fitTimelineHeight(containerH, prev + delta);
persistTimelineH(next);
return next;
});
Expand Down
9 changes: 6 additions & 3 deletions packages/studio/src/contexts/PanelLayoutContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ export function PanelLayoutProvider({
rightWidth,
adjustPanelWidth,
leftCollapsed,
setLeftCollapsed,
rightCollapsed,
setRightCollapsed,
effectiveLeftCollapsed,
effectiveRightCollapsed,
rightPanelTab,
setRightPanelTab,
rightInspectorPanes,
Expand All @@ -41,9 +42,10 @@ export function PanelLayoutProvider({
rightWidth,
adjustPanelWidth,
leftCollapsed,
setLeftCollapsed,
rightCollapsed,
setRightCollapsed,
effectiveLeftCollapsed,
effectiveRightCollapsed,
rightPanelTab,
setRightPanelTab,
rightInspectorPanes,
Expand All @@ -59,9 +61,10 @@ export function PanelLayoutProvider({
rightWidth,
adjustPanelWidth,
leftCollapsed,
setLeftCollapsed,
rightCollapsed,
setRightCollapsed,
effectiveLeftCollapsed,
effectiveRightCollapsed,
rightPanelTab,
setRightPanelTab,
rightInspectorPanes,
Expand Down
117 changes: 117 additions & 0 deletions packages/studio/src/hooks/usePanelLayout.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ function renderPanelLayout() {
return renderPanelLayoutWith(usePanelLayout);
}

function resizeWindowTo(width: number) {
Object.defineProperty(window, "innerWidth", { configurable: true, value: width });
window.dispatchEvent(new Event("resize"));
}

describe("usePanelLayout — right inspector panes", () => {
it("opens Design with the intended viewport-scaled panel widths", () => {
const harness = renderPanelLayout();
Expand Down Expand Up @@ -160,6 +165,118 @@ describe("usePanelLayout — right inspector panes", () => {
harness.unmount();
});

it("caps a panel relative to the window instead of at a flat 600px", () => {
resizeWindowTo(700);
const harness = renderPanelLayout();
// The old flat cap let the inspector claim 600 of a 700px window.
expect(harness.getState().rightWidth).toBeLessThanOrEqual(280);
harness.unmount();
});

it("rails both panels once the window cannot fit them", () => {
resizeWindowTo(560);
const harness = renderPanelLayout();
expect(harness.getState()).toMatchObject({
effectiveLeftCollapsed: true,
effectiveRightCollapsed: true,
leftCollapsed: false,
rightCollapsed: false,
});
harness.unmount();
});

it("auto-collapse never writes the user's persisted or URL-synced intent", () => {
const harness = renderPanelLayout();
act(() => resizeWindowTo(560));

expect(harness.getState().effectiveLeftCollapsed).toBe(true);
// localStorage carries leftCollapsed; the shareable URL carries rightCollapsed.
// A ten-second window drag must rewrite neither.
expect(readStudioUiPreferences().leftCollapsed).toBeUndefined();
expect(harness.getState().leftCollapsed).toBe(false);
expect(harness.getState().rightCollapsed).toBe(false);
harness.unmount();
});

it("returns the user's own width when the window grows back", () => {
const harness = renderPanelLayout();
const wide = harness.getState().leftWidth;

act(() => resizeWindowTo(560));
expect(harness.getState().leftWidth).toBeLessThan(wide);

act(() => resizeWindowTo(1496));
expect(harness.getState().leftWidth).toBe(wide);
harness.unmount();
});

it("keeps an explicitly collapsed sidebar collapsed after a narrow trip", () => {
const harness = renderPanelLayout();
act(() => harness.getState().toggleLeftSidebar());
expect(readStudioUiPreferences().leftCollapsed).toBe(true);

act(() => resizeWindowTo(560));
act(() => resizeWindowTo(1496));

expect(harness.getState().effectiveLeftCollapsed).toBe(true);
harness.unmount();
});

it("lets the user reopen a panel the window auto-collapsed", () => {
const harness = renderPanelLayout();
act(() => resizeWindowTo(560));
expect(harness.getState().effectiveRightCollapsed).toBe(true);

// Without this the header Inspector button would be dead below 700px.
act(() => harness.getState().setRightCollapsed(false));
expect(harness.getState().effectiveRightCollapsed).toBe(false);
harness.unmount();
});

it("opens the sidebar when the rail's own button is clicked", () => {
const harness = renderPanelLayout();
act(() => resizeWindowTo(560));
expect(harness.getState().effectiveLeftCollapsed).toBe(true);

// Regression: the toggle used to flip stored INTENT, which was already
// false here, so the click persisted leftCollapsed=true and the rail stayed
// railed — a dead button that silently saved a collapse nobody asked for.
act(() => harness.getState().toggleLeftSidebar());

expect(harness.getState().effectiveLeftCollapsed).toBe(false);
expect(harness.getState().leftCollapsed).toBe(false);
expect(readStudioUiPreferences().leftCollapsed).toBe(false);
// And it gets a real width: rendering an expanded sidebar at the 42px rail
// width would squash its own content. Only a real-UI click caught this.
expect(harness.getState().leftWidth).toBeGreaterThanOrEqual(200);
harness.unmount();
});

it("closes the sidebar again on the next click", () => {
const harness = renderPanelLayout();
act(() => resizeWindowTo(560));
act(() => harness.getState().toggleLeftSidebar());
act(() => harness.getState().toggleLeftSidebar());

expect(harness.getState().effectiveLeftCollapsed).toBe(true);
expect(readStudioUiPreferences().leftCollapsed).toBe(true);
harness.unmount();
});

it("forgets that reopen once the window is wide again", () => {
const harness = renderPanelLayout();
act(() => resizeWindowTo(560));
act(() => harness.getState().setRightCollapsed(false));
expect(harness.getState().effectiveRightCollapsed).toBe(false);

// Widening past the threshold clears the override, so a later narrow trip
// rails again rather than staying open forever off one old click.
act(() => resizeWindowTo(1496));
act(() => resizeWindowTo(560));
expect(harness.getState().effectiveRightCollapsed).toBe(true);
harness.unmount();
});

it("setRightPanelTab is flat-aware: exclusivity holds for callers other than a direct in-panel tab click", async () => {
vi.resetModules();
vi.doMock("../components/editor/manualEditingAvailability", async () => {
Expand Down
Loading
Loading