Skip to content

Commit f73b739

Browse files
committed
fix(studio): stop the grouping dialog opening off the bottom of the window
Reported as "the grouping button did nothing — I clicked it and nothing happened". The dialog WAS opening. It positioned itself raw at `top: anchorRect.bottom + 4` with no flip and no clamp, and the timeline lives at the bottom of the studio window, so it opened past the viewport edge. It was the only floating surface in this feature with no viewport handling at all — the FX popover next to it has had a clamp since it was written. The clamp moves to `components/editor/floatingPanelPosition.ts` and both surfaces use it, so the next one cannot drift the same way. Second silent path on the same click, fixed too: the timeline offers the grouping pointer when `clipCount > 1`, but the write bails out returning [] when fewer than two clips RESOLVE against the expanded-rows list. That bail is correct for the carve picker (nothing to group) and invisible for a button someone just pressed, so a request for 2+ clips that resolves to fewer now says so instead of doing nothing quietly. `useAudioGroupCarveAssignment` moves to its own module in the process — it is not track-visibility work, and that file was at the 600-line studio ceiling. Two tests, with a realistic bottom-of-window anchor. The existing group-pointer test passed throughout: happy-dom reports an all-zero rect for an unlaid-out button, which lands the dialog at top:4 — on screen, and nothing like the app.
1 parent 26f3bf9 commit f73b739

7 files changed

Lines changed: 199 additions & 96 deletions

File tree

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

Lines changed: 9 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -8,45 +8,20 @@
88
* clip) — nothing here serializes a chain of its own.
99
*/
1010

11-
import { useEffect, useRef, type CSSProperties, type KeyboardEvent } from "react";
11+
import { useEffect, useRef, type KeyboardEvent } from "react";
1212
import type { HfAudioFxChain } from "@hyperframes/core/audio-fx";
1313
import type { HfAudioNameKind } from "@hyperframes/core/audio-carve";
1414
import { FxPresetMenu } from "./propertyPanelFxPresetMenu.js";
1515
import { applyPresetToChain } from "./useApplyAudioFxPreset.js";
1616
import { useFxAudition } from "./useFxAudition.js";
17+
import { floatingPanelStyle } from "./floatingPanelPosition.js";
1718

1819
const POPOVER_WIDTH = 260;
19-
const VIEWPORT_MARGIN = 8;
20+
/** Below this much room underneath, the popover flips above the anchor. */
21+
const POPOVER_PREFERRED_HEIGHT = 260;
2022
/** Below this the popover is useless anyway; it scrolls instead of vanishing. */
2123
const MIN_POPOVER_HEIGHT = 160;
2224

23-
function clampedStyle(anchorRect: DOMRect): CSSProperties {
24-
const left = Math.min(
25-
Math.max(anchorRect.left, VIEWPORT_MARGIN),
26-
Math.max(VIEWPORT_MARGIN, window.innerWidth - POPOVER_WIDTH - VIEWPORT_MARGIN),
27-
);
28-
const spaceBelow = window.innerHeight - anchorRect.bottom;
29-
// Named, because the height cap below needs the same quantity the flip does.
30-
// (`spaceAbove === anchorRect.top` for a viewport-relative rect, so the flip
31-
// condition itself is unchanged — this is a rename, not a behaviour fix.)
32-
const spaceAbove = anchorRect.top;
33-
const openUpward = spaceBelow < 260 && spaceAbove > spaceBelow;
34-
// Flipping direction alone is not enough: the preset list is taller than either
35-
// gap on a short window, so the popover ran off the top or the bottom and its
36-
// footer ("+ effect" / "Open rack") went with it. Cap to whatever the chosen
37-
// side actually has and let the list scroll inside that.
38-
const available = (openUpward ? spaceAbove : spaceBelow) - VIEWPORT_MARGIN - 4;
39-
return {
40-
position: "fixed",
41-
left,
42-
width: POPOVER_WIDTH,
43-
maxHeight: Math.max(MIN_POPOVER_HEIGHT, available),
44-
...(openUpward
45-
? { bottom: window.innerHeight - anchorRect.top + 4 }
46-
: { top: anchorRect.bottom + 4 }),
47-
};
48-
}
49-
5025
export interface TimelineFxPopoverProps {
5126
anchorRect: DOMRect;
5227
chain: HfAudioFxChain;
@@ -109,7 +84,11 @@ export function TimelineFxPopover({
10984
role="dialog"
11085
aria-label="Effects"
11186
className="z-[200] flex flex-col overflow-hidden rounded-md border border-white/10 bg-[#1b1b1f] p-2 shadow-xl"
112-
style={clampedStyle(anchorRect)}
87+
style={floatingPanelStyle(anchorRect, {
88+
width: POPOVER_WIDTH,
89+
preferredHeight: POPOVER_PREFERRED_HEIGHT,
90+
minHeight: MIN_POPOVER_HEIGHT,
91+
})}
11392
onKeyDown={onKeyDown}
11493
onPointerDown={(event) => event.stopPropagation()}
11594
>
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/**
2+
* Viewport-aware placement for a timeline surface portaled to `document.body`.
3+
*
4+
* Shared because the timeline's floating surfaces sit at the BOTTOM of the
5+
* studio window, so the naive `top: anchorRect.bottom + 4` puts them off the
6+
* bottom edge — a click that appears to do nothing. The FX popover grew this
7+
* handling; the group-creation dialog was still positioning itself raw. One
8+
* implementation, so the next surface cannot drift again.
9+
*/
10+
11+
import type { CSSProperties } from "react";
12+
13+
/** Keep this much clear of every viewport edge. */
14+
const VIEWPORT_MARGIN = 8;
15+
/** The gap between the anchor and the surface it opens. */
16+
const ANCHOR_GAP = 4;
17+
18+
export interface FloatingPanelPlacement {
19+
/** Fixed width of the surface, in px — needed to clamp the right edge. */
20+
width: number;
21+
/**
22+
* How tall the surface wants to be. Below this much room underneath, it
23+
* flips above the anchor (when there is genuinely more room there).
24+
*/
25+
preferredHeight: number;
26+
/** Never cap shorter than this; the surface scrolls instead of vanishing. */
27+
minHeight: number;
28+
}
29+
30+
/**
31+
* Placement for `anchorRect`, as inline style. Flips above the anchor when
32+
* below is too tight, clamps horizontally into the viewport, and caps the
33+
* height to whichever side it chose so the surface cannot run off-screen.
34+
*/
35+
export function floatingPanelStyle(
36+
anchorRect: DOMRect,
37+
{ width, preferredHeight, minHeight }: FloatingPanelPlacement,
38+
): CSSProperties {
39+
const left = Math.min(
40+
Math.max(anchorRect.left, VIEWPORT_MARGIN),
41+
Math.max(VIEWPORT_MARGIN, window.innerWidth - width - VIEWPORT_MARGIN),
42+
);
43+
const spaceBelow = window.innerHeight - anchorRect.bottom;
44+
// The space actually available above. Equal to `anchorRect.top` for a
45+
// viewport-relative rect; named because the height cap needs it too.
46+
const spaceAbove = anchorRect.top;
47+
const openUpward = spaceBelow < preferredHeight && spaceAbove > spaceBelow;
48+
const available = (openUpward ? spaceAbove : spaceBelow) - VIEWPORT_MARGIN - ANCHOR_GAP;
49+
return {
50+
position: "fixed",
51+
left,
52+
width,
53+
maxHeight: Math.max(minHeight, available),
54+
...(openUpward
55+
? { bottom: window.innerHeight - anchorRect.top + ANCHOR_GAP }
56+
: { top: anchorRect.bottom + ANCHOR_GAP }),
57+
};
58+
}

packages/studio/src/hooks/timelineTrackVisibility.ts

Lines changed: 0 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -532,67 +532,3 @@ export function useTimelineElementVisibilityEditing({
532532
],
533533
);
534534
}
535-
536-
/**
537-
* The write behind B6's auto-group: pick two or more voice clips in the carve
538-
* picker and they land in a group instead of naming each other by id. Same
539-
* expanded-rows resolution as element-visibility, for the same reason — a
540-
* nested sub-composition child has no entry in the raw store list.
541-
*/
542-
export function useAudioGroupCarveAssignment({
543-
projectIdRef,
544-
activeCompPath,
545-
showToast,
546-
writeProjectFile,
547-
recordEdit,
548-
domEditSaveTimestampRef,
549-
previewIframeRef,
550-
pendingTimelineEditPathRef,
551-
isRecordingRef,
552-
}: UseTimelineElementVisibilityEditingInput): (
553-
clipIds: readonly string[],
554-
groupId: string,
555-
) => Promise<void> {
556-
const expandedElements = useExpandedTimelineElements();
557-
return useCallback(
558-
async (clipIds: readonly string[], groupId: string) => {
559-
if (isRecordingRef?.current) {
560-
showToast("Cannot edit timeline while recording", "error");
561-
return;
562-
}
563-
const pid = projectIdRef.current;
564-
if (!pid) return;
565-
const keys = new Set(clipIds);
566-
const elements = expandedElements.filter((item) => keys.has(item.key ?? item.id));
567-
try {
568-
await createAudioGroupAndAssignMembers({
569-
projectId: pid,
570-
activeCompPath,
571-
elements,
572-
groupId,
573-
previewIframe: previewIframeRef.current,
574-
writeProjectFile,
575-
recordEdit,
576-
domEditSaveTimestampRef,
577-
pendingTimelineEditPathRef,
578-
});
579-
} catch (error) {
580-
console.error("[Timeline] Failed to group voice clips", error);
581-
const message = error instanceof Error ? error.message : "Failed to group voice clips";
582-
showToast(message);
583-
}
584-
},
585-
[
586-
activeCompPath,
587-
expandedElements,
588-
previewIframeRef,
589-
writeProjectFile,
590-
recordEdit,
591-
domEditSaveTimestampRef,
592-
pendingTimelineEditPathRef,
593-
isRecordingRef,
594-
showToast,
595-
projectIdRef,
596-
],
597-
);
598-
}
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
import { useCallback } from "react";
2+
import { useExpandedTimelineElements } from "../player/hooks/useExpandedTimelineElements";
3+
import {
4+
createAudioGroupAndAssignMembers,
5+
type UseTimelineElementVisibilityEditingInput,
6+
} from "./timelineTrackVisibility";
7+
8+
/**
9+
* The write behind B6's auto-group and behind C1's grouping pointer in a track
10+
* header: pick two or more voice clips (or press the pointer on an ungrouped
11+
* audio track) and they land in a group instead of naming each other by id.
12+
*
13+
* Its own module because this is not track-visibility work, and that file is at
14+
* the studio 600-line ceiling. Same
15+
* expanded-rows resolution as element-visibility, for the same reason — a
16+
* nested sub-composition child has no entry in the raw store list.
17+
*/
18+
export function useAudioGroupCarveAssignment({
19+
projectIdRef,
20+
activeCompPath,
21+
showToast,
22+
writeProjectFile,
23+
recordEdit,
24+
domEditSaveTimestampRef,
25+
previewIframeRef,
26+
pendingTimelineEditPathRef,
27+
isRecordingRef,
28+
}: UseTimelineElementVisibilityEditingInput): (
29+
clipIds: readonly string[],
30+
groupId: string,
31+
) => Promise<void> {
32+
const expandedElements = useExpandedTimelineElements();
33+
return useCallback(
34+
async (clipIds: readonly string[], groupId: string) => {
35+
if (isRecordingRef?.current) {
36+
showToast("Cannot edit timeline while recording", "error");
37+
return;
38+
}
39+
const pid = projectIdRef.current;
40+
if (!pid) return;
41+
const keys = new Set(clipIds);
42+
const elements = expandedElements.filter((item) => keys.has(item.key ?? item.id));
43+
// `createAudioGroupAndAssignMembers` returns [] for fewer than two
44+
// members, which is right for the carve picker (nothing to group) but
45+
// silent for a button the author just pressed: the timeline offers the
46+
// grouping pointer on `clipCount > 1`, so a request that resolves to
47+
// fewer than two clips is an id-space mismatch, not a no-op. Say so
48+
// rather than letting the click look like it did nothing.
49+
if (clipIds.length > 1 && elements.length < 2) {
50+
console.error("[Timeline] Grouping resolved too few clips", {
51+
requested: clipIds,
52+
resolved: elements.map((item) => item.key ?? item.id),
53+
});
54+
showToast("Could not group these clips — try expanding the track first", "error");
55+
return;
56+
}
57+
try {
58+
await createAudioGroupAndAssignMembers({
59+
projectId: pid,
60+
activeCompPath,
61+
elements,
62+
groupId,
63+
previewIframe: previewIframeRef.current,
64+
writeProjectFile,
65+
recordEdit,
66+
domEditSaveTimestampRef,
67+
pendingTimelineEditPathRef,
68+
});
69+
} catch (error) {
70+
console.error("[Timeline] Failed to group voice clips", error);
71+
const message = error instanceof Error ? error.message : "Failed to group voice clips";
72+
showToast(message);
73+
}
74+
},
75+
[
76+
activeCompPath,
77+
expandedElements,
78+
previewIframeRef,
79+
writeProjectFile,
80+
recordEdit,
81+
domEditSaveTimestampRef,
82+
pendingTimelineEditPathRef,
83+
isRecordingRef,
84+
showToast,
85+
projectIdRef,
86+
],
87+
);
88+
}

packages/studio/src/hooks/useTimelineEditing.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,10 @@ import { useSetAudioGroupAttribute } from "./timelineAudioGroupVolume";
2525
import { useTimelineDeleteOps } from "./useTimelineDeleteOps";
2626
import { useSetElementAttribute } from "./timelineElementFxAttribute";
2727
import {
28-
useAudioGroupCarveAssignment,
2928
useTimelineElementVisibilityEditing,
3029
useTimelineTrackVisibilityEditing,
3130
} from "./timelineTrackVisibility";
31+
import { useAudioGroupCarveAssignment } from "./useAudioGroupCarveAssignment";
3232
import { useTimelineGroupEditing } from "./useTimelineGroupEditing";
3333
import { serializeZLaneGesture } from "../components/nle/zLaneGesture";
3434
import { cutoverCommittedOrThrow, sdkTimingPersist } from "../utils/sdkCutover";

packages/studio/src/player/components/TimelineFxButton.test.tsx

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,36 @@ describe("TimelineFxButton", () => {
6868
expect(document.querySelector('[role="dialog"]')).toBeTruthy();
6969
});
7070

71+
// The reported symptom: clicking FX on an ungrouped audio track "did
72+
// nothing". The dialog WAS opening — it was positioned at
73+
// `anchorRect.bottom + 4` with no flip, and the timeline lives at the bottom
74+
// of the studio window, so it opened past the viewport edge. The old test
75+
// passed because happy-dom reports an all-zero rect for an unlaid-out button,
76+
// which lands the dialog at top:4 — on screen, and nothing like the real app.
77+
it("flips the group dialog above the anchor when it sits at the bottom of the window", () => {
78+
const host = mount(<TimelineFxButton variant="group-pointer" onGroupClips={vi.fn()} />);
79+
const fx = byTextButton(host, "FX");
80+
// A track header near the bottom edge of the (1024x768) window.
81+
fx!.getBoundingClientRect = () => ({ left: 300, top: 760, right: 320, bottom: 776 }) as DOMRect;
82+
act(() => fx?.click());
83+
const dialog = document.querySelector('[role="dialog"]') as HTMLElement;
84+
expect(dialog).toBeTruthy();
85+
// Anchored from the bottom, not pushed off the edge with `top`.
86+
expect(dialog.style.bottom).toBe("12px");
87+
expect(dialog.style.top).toBe("");
88+
});
89+
90+
it("keeps the group dialog inside the right edge of the window", () => {
91+
const host = mount(<TimelineFxButton variant="group-pointer" onGroupClips={vi.fn()} />);
92+
const fx = byTextButton(host, "FX");
93+
// Anchor hard against the right edge: 224px wide + 8px margin must fit.
94+
fx!.getBoundingClientRect = () =>
95+
({ left: 1010, top: 100, right: 1024, bottom: 116 }) as DOMRect;
96+
act(() => fx?.click());
97+
const dialog = document.querySelector('[role="dialog"]') as HTMLElement;
98+
expect(dialog.style.left).toBe("792px");
99+
});
100+
71101
it("group-pointer variant offers Group instead of a popover", () => {
72102
const onGroupClips = vi.fn();
73103
const host = mount(<TimelineFxButton variant="group-pointer" onGroupClips={onGroupClips} />);

packages/studio/src/player/components/TimelineFxButton.tsx

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ import {
1818
} from "@hyperframes/core/audio-fx";
1919
import type { HfAudioNameKind } from "@hyperframes/core/audio-carve";
2020
import { TimelineFxPopover } from "../../components/editor/TimelineFxPopover.js";
21+
import { floatingPanelStyle } from "../../components/editor/floatingPanelPosition.js";
22+
23+
/** `w-56`, and roughly the height of its one paragraph plus the button. */
24+
const GROUP_DIALOG_WIDTH = 224;
25+
const GROUP_DIALOG_HEIGHT = 120;
2126

2227
function parseFxChainOrEmpty(raw: string | undefined): HfAudioFxChain {
2328
if (!raw) return { version: 1, nodes: [] };
@@ -80,7 +85,14 @@ export function TimelineFxButton(props: TimelineFxButtonProps) {
8085
role="dialog"
8186
aria-label="Group these clips to add effects"
8287
className="z-[200] w-56 rounded-md border border-white/10 bg-[#1b1b1f] p-2.5 text-[11px] text-white/75 shadow-xl"
83-
style={{ position: "fixed", left: anchorRect.left, top: anchorRect.bottom + 4 }}
88+
// Was positioning raw at `anchorRect.bottom + 4`. The timeline
89+
// sits at the BOTTOM of the window, so this dialog opened past
90+
// the viewport edge and the click read as doing nothing at all.
91+
style={floatingPanelStyle(anchorRect, {
92+
width: GROUP_DIALOG_WIDTH,
93+
preferredHeight: GROUP_DIALOG_HEIGHT,
94+
minHeight: GROUP_DIALOG_HEIGHT,
95+
})}
8496
onPointerDown={(event) => event.stopPropagation()}
8597
>
8698
<p>Group these clips to add effects to all of them.</p>

0 commit comments

Comments
 (0)