fix(studio): cap the timeline FX popover to its gap, and scroll the list inside (#3413)

* fix(studio): cap the timeline FX popover to its gap, and scroll the list inside

The popover grew to whatever the preset list needed, so on a short window it ran
off the top or the bottom of the viewport and took its footer ('+ effect' /
'Open rack') with it — nothing scrolled, so the presets past the edge were
simply unreachable.

It now caps to the space on whichever side it opens toward, and the preset list
scrolls inside that while the footer stays put. `min-h-0` on the scroller is
load-bearing: a flex child defaults to min-height:auto and would refuse to
shrink, pushing the footer out instead of scrolling.

`spaceAbove` is named for the cap's benefit; it equals `anchorRect.top`, so the
flip condition is unchanged.

Four tests cover it, because this shipped once before with none: the downward
cap, the upward cap, the usable-minimum clamp, and the footer being a sibling of
the scroller rather than inside it. Verified they fail without the cap.

* fix(studio): slide the FX popover in-bounds instead of hanging it off the edge

Review found the minimum defeating the viewport cap: `Math.max(MIN_POPOVER_HEIGHT,
available)` kept the box 160px tall even when the chosen gap was smaller, so the
box extended past the edge it opened away from. At 200px of viewport with the
anchor at 100..120 it flipped up to `bottom: 104px` and spanned y = -64..96 —
every preset still reachable, but through a ~57px window with the top third of
the dialog off-screen. Reachable at high browser zoom, not only in a synthetic
short window: `available` drops under 160 once the gap is under ~172px, which
400% zoom on a 1080p display produces on both sides.

Shrinking to the gap would undo the floor on purpose (a 20px gap gives a 20px
popover — the vanishing popover in a new costume), so honour the floor and clamp
the resulting box into the viewport the way `left` already is. Two parts:

- Cap the floor by the window itself (`innerHeight - 2 * VIEWPORT_MARGIN`). The
  minimum is a floor against a tight gap, not against a tight window; below
  176px of viewport, physical space has to win.
- Inset the `top` / `bottom` offset to `innerHeight - height - VIEWPORT_MARGIN`,
  so a floor larger than the gap slides the box back in rather than off the top.

The tight case now lands at `bottom: 32px` with `maxHeight: 160px` — the box at
y = 8..40, one margin on each side. The two ordinary cases are unchanged
(34/726 down, 72/688 up), which the existing tests pin.

Tests: two added — both edges in-bounds when the minimum exceeds the gap, and
the floor yielding when the whole window is shorter than it. Both fail on the
previous arithmetic (104px vs 32px, 160px vs 104px). The three pre-existing
geometry tests now pin `window.innerHeight` through one shared helper instead of
inheriting happy-dom's 768 default, so their expected numbers are derivable from
the test and immune to a dependency bump.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

packages/studio/src/components/editor/TimelineFxPopover.test.tsx

@@ -10,6 +10,28 @@ import { TimelineFxPopover } from "./TimelineFxPopover.js";
 const EMPTY_CHAIN: HfAudioFxChain = { version: 1, nodes: [] };
 const RECT = { left: 0, top: 0, right: 0, bottom: 0 } as DOMRect;
 
+function rect(top: number, bottom: number): DOMRect {
+  return { left: 0, top, right: 0, bottom } as DOMRect;
+}
+
+/** Pin the viewport height: every expected number below is derived from it, and
+ *  happy-dom's 768 default is not something this file should silently inherit. */
+function withViewportHeight<T>(value: number, run: () => T): T {
+  const previous = window.innerHeight;
+  Object.defineProperty(window, "innerHeight", { value, configurable: true });
+  try {
+    return run();
+  } finally {
+    Object.defineProperty(window, "innerHeight", { value: previous, configurable: true });
+  }
+}
+
+function dialogOf(host: HTMLElement): HTMLElement {
+  const el = host.querySelector('[role="dialog"]');
+  if (!el) throw new Error("no dialog");
+  return el as HTMLElement;
+}
+
 function byTextButton(host: HTMLElement, text: string): HTMLButtonElement | undefined {
   return Array.from(host.querySelectorAll("button")).find((b) => b.textContent?.includes(text));
 }
@@ -102,6 +124,73 @@ describe("TimelineFxPopover", () => {
     expect(onClose).not.toHaveBeenCalled();
   });
 
+  // These guard the regression that shipped once already: an uncapped
+  // popover grew past the gap it opened into, ran under the timeline chrome and
+  // took its footer with it, and nothing scrolled.
+  it("caps its height to the space below when it opens downward", () => {
+    // spaceBelow 738 > spaceAbove 10, so it opens down: 738 - margin(8) - gap(4).
+    const dialog = withViewportHeight(768, () =>
+      dialogOf(mount({ anchorRect: rect(10, 30) }).host),
+    );
+    expect(dialog.style.top).toBe("34px");
+    expect(dialog.style.maxHeight).toBe("726px");
+  });
+
+  it("caps its height to the space above when it flips upward", () => {
+    // spaceBelow 48 < 260 and spaceAbove 700 is larger, so it flips up.
+    const dialog = withViewportHeight(768, () =>
+      dialogOf(mount({ anchorRect: rect(700, 720) }).host),
+    );
+    expect(dialog.style.bottom).toBe("72px");
+    expect(dialog.style.maxHeight).toBe("688px");
+  });
+
+  it("never caps below a usable minimum, however tight the gap", () => {
+    // Both gaps are tiny (80 below / 100 above); the cap must not collapse.
+    const dialog = withViewportHeight(200, () =>
+      dialogOf(mount({ anchorRect: rect(100, 120) }).host),
+    );
+    expect(dialog.style.maxHeight).toBe("160px");
+  });
+
+  it("keeps both edges in the viewport when the minimum exceeds the gap", () => {
+    // The case the minimum used to lose: at 200px of viewport (roughly 400% zoom
+    // on a laptop) neither gap can hold 160px, so honouring the floor has to
+    // slide the box in rather than hang its top edge off-screen.
+    const dialog = withViewportHeight(200, () =>
+      dialogOf(mount({ anchorRect: rect(100, 120) }).host),
+    );
+    // bottom:32 + maxHeight:160 puts the box at y = 8..40 — a margin on each side.
+    expect(dialog.style.bottom).toBe("32px");
+    const bottom = Number.parseFloat(dialog.style.bottom);
+    const height = Number.parseFloat(dialog.style.maxHeight);
+    expect(bottom).toBeGreaterThanOrEqual(8);
+    expect(200 - bottom - height).toBeGreaterThanOrEqual(8);
+  });
+
+  it("shrinks below the minimum only when the whole window is shorter", () => {
+    // A floor against a tight gap is not a floor against a tight window: 120px of
+    // viewport cannot hold 160px, and hanging off the edge is worse than short.
+    const dialog = withViewportHeight(120, () =>
+      dialogOf(mount({ anchorRect: rect(60, 80) }).host),
+    );
+    expect(dialog.style.maxHeight).toBe("104px");
+    expect(dialog.style.bottom).toBe("8px");
+  });
+
+  it("scrolls the preset list and leaves the footer outside the scroller", () => {
+    const { host } = mount();
+    const dialog = dialogOf(host);
+    const scroller = dialog.querySelector(".overflow-y-auto");
+    expect(scroller).toBeTruthy();
+    // The footer must be a SIBLING of the scroller, not inside it — otherwise it
+    // scrolls away instead of staying put, which is the original bug.
+    const footer = byTextButton(host, "Open rack");
+    expect(footer).toBeDefined();
+    expect(scroller?.contains(footer as Node)).toBe(false);
+    expect(dialog.contains(footer as Node)).toBe(true);
+  });
+
   it("the footer opens the rack and closes the popover", () => {
     const { host, onOpenRack, onClose } = mount();
     const openRack = byTextButton(host, "Open rack");

packages/studio/src/components/editor/TimelineFxPopover.tsx

@@ -17,6 +17,8 @@ import { useFxAudition } from "./useFxAudition.js";
 
 const POPOVER_WIDTH = 260;
 const VIEWPORT_MARGIN = 8;
+/** Below this the popover is useless anyway; it scrolls instead of vanishing. */
+const MIN_POPOVER_HEIGHT = 160;
 
 function clampedStyle(anchorRect: DOMRect): CSSProperties {
   const left = Math.min(
@@ -24,14 +26,41 @@ function clampedStyle(anchorRect: DOMRect): CSSProperties {
     Math.max(VIEWPORT_MARGIN, window.innerWidth - POPOVER_WIDTH - VIEWPORT_MARGIN),
   );
   const spaceBelow = window.innerHeight - anchorRect.bottom;
-  const openUpward = spaceBelow < 260 && anchorRect.top > spaceBelow;
+  // Named, because the height cap below needs the same quantity the flip does.
+  // (`spaceAbove === anchorRect.top` for a viewport-relative rect, so the flip
+  // condition itself is unchanged — this is a rename, not a behaviour fix.)
+  const spaceAbove = anchorRect.top;
+  const openUpward = spaceBelow < 260 && spaceAbove > spaceBelow;
+  // Flipping direction alone is not enough: the preset list is taller than either
+  // gap on a short window, so the popover ran off the top or the bottom and its
+  // footer ("+ effect" / "Open rack") went with it. Cap to whatever the chosen
+  // side actually has and let the list scroll inside that.
+  const available = (openUpward ? spaceAbove : spaceBelow) - VIEWPORT_MARGIN - 4;
+  // The minimum is a floor against a tight GAP, not against a tight window: keep
+  // a usable list when the gap is smaller than 160px, but never ask for more
+  // height than the viewport itself can hold.
+  const height = Math.min(
+    Math.max(MIN_POPOVER_HEIGHT, available),
+    window.innerHeight - VIEWPORT_MARGIN * 2,
+  );
+  // A floor larger than the gap would hang the box off the edge it opened away
+  // from — reachable at high browser zoom, where both gaps fall under ~172px.
+  // Slide it back in-bounds the way `left` is already clamped, rather than
+  // shrinking below the floor: the offset that keeps BOTH edges inside is
+  // `innerHeight - height - VIEWPORT_MARGIN`, from either side.
+  const inset = (desired: number) =>
+    Math.max(
+      VIEWPORT_MARGIN,
+      Math.min(desired, Math.max(VIEWPORT_MARGIN, window.innerHeight - height - VIEWPORT_MARGIN)),
+    );
   return {
     position: "fixed",
     left,
     width: POPOVER_WIDTH,
+    maxHeight: height,
     ...(openUpward
-      ? { bottom: window.innerHeight - anchorRect.top + 4 }
-      : { top: anchorRect.bottom + 4 }),
+      ? { bottom: inset(window.innerHeight - anchorRect.top + 4) }
+      : { top: inset(anchorRect.bottom + 4) }),
   };
 }
 
@@ -96,22 +125,27 @@ export function TimelineFxPopover({
       ref={rootRef}
       role="dialog"
       aria-label="Effects"
-      className="z-50 rounded-md border border-white/10 bg-[#1b1b1f] p-2 shadow-xl"
+      className="z-50 flex flex-col overflow-hidden rounded-md border border-white/10 bg-[#1b1b1f] p-2 shadow-xl"
       style={clampedStyle(anchorRect)}
       onKeyDown={onKeyDown}
       onPointerDown={(event) => event.stopPropagation()}
     >
-      <FxPresetMenu
-        trackKind={trackKind}
-        onPick={applyPreset}
-        onAudition={
-          onChainPreview
-            ? (id) =>
-                audition(id ? (base) => applyPresetToChain(base, id, trackKind) ?? base : null)
-            : undefined
-        }
-      />
-      <div className="mt-2 flex items-center justify-between border-t border-white/10 pt-2 text-[10px] text-white/55">
+      {/* The list scrolls; the footer below stays put. `min-h-0` is load-bearing
+          — a flex child defaults to min-height:auto and would refuse to shrink,
+          pushing the footer out of the popover instead of scrolling. */}
+      <div className="min-h-0 flex-1 overflow-y-auto">
+        <FxPresetMenu
+          trackKind={trackKind}
+          onPick={applyPreset}
+          onAudition={
+            onChainPreview
+              ? (id) =>
+                  audition(id ? (base) => applyPresetToChain(base, id, trackKind) ?? base : null)
+              : undefined
+          }
+        />
+      </div>
+      <div className="mt-2 flex shrink-0 items-center justify-between border-t border-white/10 pt-2 text-[10px] text-white/55">
         <button
           type="button"
           className="hover:text-white"