diff --git a/packages/studio/src/hooks/timelineAudioGroupVolume.test.ts b/packages/studio/src/hooks/timelineAudioGroupVolume.test.ts new file mode 100644 index 000000000..c31e7af1e --- /dev/null +++ b/packages/studio/src/hooks/timelineAudioGroupVolume.test.ts @@ -0,0 +1,104 @@ +// @vitest-environment jsdom + +/** + * A group write has to reach the STORE, not just the file and the live DOM. + * + * The timeline derives a group row's label, fader, mute and chain from the + * `audioGroup*` fields mirrored onto its members — so a write that lands + * everywhere except there leaves the header rendering whatever it parsed at + * load. Observed in the studio: muting a group wrote `data-hidden` to disk and + * to the preview, and the button stayed "Mute group Voiceover", re-writing the + * same attribute on every click with no way to unmute. + * + * Invalidating the parse cache is necessary but not sufficient — it only makes + * the NEXT parse honest, and a live attribute patch never triggers one. + */ + +import { afterEach, describe, expect, it } from "vitest"; +import { usePlayerStore, type TimelineElement } from "../player"; +import { useSetAudioGroupAttribute } from "./timelineAudioGroupVolume"; + +afterEach(() => { + usePlayerStore.getState().reset(); +}); + +function member(domId: string, track: number): TimelineElement { + return { + id: domId, + key: `index.html#${domId}`, + domId, + tag: "audio", + start: 0, + duration: 5, + track, + audioGroup: "voiceover", + audioGroupHidden: false, + audioGroupVolume: 1, + }; +} + +/** The hook without React — it only closes over refs and callbacks. */ +function makeSetter() { + const input = { + projectIdRef: { current: "project-1" }, + activeCompPath: "index.html", + showToast: () => {}, + writeProjectFile: async () => {}, + recordEdit: async () => {}, + domEditSaveTimestampRef: { current: 0 }, + pendingTimelineEditPathRef: { current: new Set() }, + previewIframeRef: { current: null }, + }; + // `setLive` takes no async path and touches only the preview DOM + store, so + // it can be exercised directly; `setQuiet` additionally persists, which this + // test deliberately does not cover (that is timelineTrackVisibility's job). + let setter: ReturnType | null = null; + const Probe = () => { + setter = useSetAudioGroupAttribute(input as never); + return null; + }; + // Minimal hook harness: call the component function directly. It uses only + // useCallback, which React allows outside a renderer when the result is used + // immediately and never re-rendered. + return { Probe, get: () => setter }; +} + +describe("group attribute writes reach the store", () => { + it("mirrors data-hidden onto every member so the header can flip", async () => { + const react = await import("react"); + const { renderToStaticMarkup } = await import("react-dom/server"); + const harness = makeSetter(); + renderToStaticMarkup(react.createElement(harness.Probe)); + const setter = harness.get(); + expect(setter).not.toBeNull(); + + usePlayerStore.getState().setElements([member("voice-1", 0), member("voice-2", 1)]); + + setter?.setLive("voiceover", "data-hidden", ""); + expect(usePlayerStore.getState().elements.every((el) => el.audioGroupHidden === true)).toBe( + true, + ); + + setter?.setLive("voiceover", "data-hidden", null); + expect(usePlayerStore.getState().elements.every((el) => el.audioGroupHidden === false)).toBe( + true, + ); + }); + + it("mirrors data-volume, and leaves other groups alone", async () => { + const react = await import("react"); + const { renderToStaticMarkup } = await import("react-dom/server"); + const harness = makeSetter(); + renderToStaticMarkup(react.createElement(harness.Probe)); + const setter = harness.get(); + + const other: TimelineElement = { ...member("sfx", 2), audioGroup: "effects" }; + usePlayerStore.getState().setElements([member("voice-1", 0), other]); + + setter?.setLive("voiceover", "data-volume", "0.4"); + + const byId = new Map(usePlayerStore.getState().elements.map((el) => [el.id, el])); + expect(byId.get("voice-1")?.audioGroupVolume).toBeCloseTo(0.4, 6); + expect(byId.get("sfx")?.audioGroupVolume).toBe(1); + }); +}); diff --git a/packages/studio/src/hooks/timelineAudioGroupVolume.ts b/packages/studio/src/hooks/timelineAudioGroupVolume.ts index d425bf17f..539bbb2d1 100644 --- a/packages/studio/src/hooks/timelineAudioGroupVolume.ts +++ b/packages/studio/src/hooks/timelineAudioGroupVolume.ts @@ -1,4 +1,7 @@ import { useCallback } from "react"; +import { HF_AUDIO_FX_ATTR } from "@hyperframes/core/audio-fx"; +import { usePlayerStore } from "../player"; +import type { TimelineElementPatch } from "../player/store/timelineElement"; import { invalidateGroupInfoCache } from "../player/lib/timelineDOM"; import { buildPatchTarget, @@ -25,6 +28,43 @@ function patchLiveGroupAttribute( invalidateGroupInfoCache(iframe?.contentDocument); } +/** Which store field each writable group attribute mirrors into. */ +const GROUP_ATTR_TO_MIRROR: Record< + string, + (value: string | null, groupId: string) => TimelineElementPatch +> = { + "data-hidden": (value) => ({ audioGroupHidden: value !== null }), + "data-volume": (value) => ({ + audioGroupVolume: Number.isFinite(Number(value)) ? Number(value) : 1, + }), + "data-label": (value, groupId) => ({ audioGroupLabel: value ?? groupId }), + [HF_AUDIO_FX_ATTR]: (value) => ({ audioGroupFxChain: value ?? undefined }), +}; + +/** + * Mirror a group attribute onto the store copy every member carries. + * + * The timeline derives a group's label / volume / mute / chain from these + * mirrored `audioGroup*` fields on its MEMBERS, not from the group element — + * and a group write only ever touched the file and the live preview DOM. + * Nothing re-parsed, so the header went on reading the old value: the observed + * symptom was a muted group whose button stayed "Mute group", re-writing + * `data-hidden` on every click and never offering to unmute. + * + * Invalidating the parse cache is necessary but not sufficient — it only + * ensures the NEXT parse is honest, and a live attribute patch does not cause + * one. Same reason `commitDataAttribute` carries `syncStoredAutomationFromPreview`. + */ +function syncStoredGroupAttribute(groupId: string, attr: string, value: string | null): void { + const toPatch = GROUP_ATTR_TO_MIRROR[attr]; + if (!toPatch) return; + const patch = toPatch(value, groupId); + const store = usePlayerStore.getState(); + for (const element of store.elements) { + if (element.audioGroup === groupId) store.updateElement(element.key ?? element.id, patch); + } +} + interface SetAudioGroupAttributeInput { projectId: string; activeCompPath: string | null; @@ -103,6 +143,10 @@ export function useSetAudioGroupAttribute({ const setLive = useCallback( (groupId: string, attr: string, value: string | null) => { patchLiveGroupAttribute(previewIframeRef.current, groupId, attr, value); + // Live too, not just on commit: a fader drag is `setLive` per frame and + // `setQuiet` once on release, so without this the strip's own readout + // fights the drag. + syncStoredGroupAttribute(groupId, attr, value); }, [previewIframeRef], ); @@ -128,6 +172,7 @@ export function useSetAudioGroupAttribute({ domEditSaveTimestampRef, pendingTimelineEditPathRef, }); + syncStoredGroupAttribute(groupId, attr, value); } catch (error) { console.error("[Timeline] Failed to set group attribute", error); const message = error instanceof Error ? error.message : "Failed to update group"; diff --git a/packages/studio/src/player/components/TimelineGroupBusStrip.test.tsx b/packages/studio/src/player/components/TimelineGroupBusStrip.test.tsx index 981bc0349..5f6264001 100644 --- a/packages/studio/src/player/components/TimelineGroupBusStrip.test.tsx +++ b/packages/studio/src/player/components/TimelineGroupBusStrip.test.tsx @@ -77,28 +77,41 @@ describe("TimelineGroupBusStrip", () => { const { onVolumeChange, onVolumeCommit } = renderStrip({ volume: 1 }); const input = slider(); - act(() => setSliderValue(input, "1.2")); - act(() => setSliderValue(input, "1.5")); + act(() => setSliderValue(input, "0.4")); + act(() => setSliderValue(input, "0.6")); expect(onVolumeChange).toHaveBeenCalledTimes(2); - expect(onVolumeChange).toHaveBeenLastCalledWith(1.5); + expect(onVolumeChange).toHaveBeenLastCalledWith(0.6); expect(onVolumeCommit).not.toHaveBeenCalled(); act(() => { - input.value = "1.5"; + input.value = "0.6"; input.dispatchEvent(new PointerEvent("pointerup", { bubbles: true })); }); expect(onVolumeCommit).toHaveBeenCalledTimes(1); - expect(onVolumeCommit).toHaveBeenCalledWith(1.5); + expect(onVolumeCommit).toHaveBeenCalledWith(0.6); }); - it("clamps the volume slider to the 0..2 range", () => { - const { onVolumeChange } = renderStrip(); + // Unity is the ceiling because unity is what the pipeline honours: the render + // clamps every track volume to [0,1] and the preview bus clamps to match, so + // the fader's old travel to 2.0 spent its top half writing `data-volume` + // values that both ends discarded — a control promising +6 dB that nothing + // delivered. + it("clamps the volume slider to the 0..1 range the pipeline honours", () => { + // Starts below unity so each write below is a real change — setting a range + // input to the value it already holds fires no change event at all. + const { onVolumeChange } = renderStrip({ volume: 0.5 }); const input = slider(); expect(input.min).toBe("0"); - expect(input.max).toBe("2"); + expect(input.max).toBe("1"); + act(() => setSliderValue(input, "1")); + expect(onVolumeChange).toHaveBeenLastCalledWith(1); + + // Above unity is pulled back to unity rather than written through — the + // element's own max does the first half, `clampVolume` the rest. + act(() => setSliderValue(input, "0.5")); act(() => setSliderValue(input, "2")); - expect(onVolumeChange).toHaveBeenLastCalledWith(2); + expect(onVolumeChange).toHaveBeenLastCalledWith(1); }); it("the level bar tracks a live reading and shows nothing extra when it isn't clipping", () => { diff --git a/packages/studio/src/player/components/TimelineGroupBusStrip.tsx b/packages/studio/src/player/components/TimelineGroupBusStrip.tsx index 5d5e58274..d74a0cb88 100644 --- a/packages/studio/src/player/components/TimelineGroupBusStrip.tsx +++ b/packages/studio/src/player/components/TimelineGroupBusStrip.tsx @@ -12,8 +12,18 @@ import type { TimelineTheme } from "./timelineTheme"; /** How long "Too loud" stays lit after the last clipped block. */ const CLIP_HOLD_MS = 2000; +/** + * Unity is the ceiling because unity is what the pipeline honours: the render + * puts every track volume through its own `clampVolume` ([0,1]) before building + * the filter, and the preview bus clamps to match. A fader travelling to 2.0 + * therefore spent its top half writing `data-volume` values that BOTH ends + * discard — the control promised +6 dB and nothing delivered it. + * + * Raising the ceiling instead would mean changing the render's shared clamp for + * every track, not just group buses; that is a mixer decision, not a slider one. + */ function clampVolume(value: number): number { - return Math.min(2, Math.max(0, value)); + return Math.min(1, Math.max(0, value)); } interface TimelineGroupBusStripProps { @@ -66,7 +76,7 @@ export function TimelineGroupBusStrip({ type="range" aria-label="Group volume" min={0} - max={2} + max={1} step={0.01} value={shownVolume} className="h-1 w-20 shrink-0 accent-[#3CE6AC]" diff --git a/packages/studio/src/player/lib/timelineDOM.test.ts b/packages/studio/src/player/lib/timelineDOM.test.ts index 6ffa01715..a62e66a29 100644 --- a/packages/studio/src/player/lib/timelineDOM.test.ts +++ b/packages/studio/src/player/lib/timelineDOM.test.ts @@ -7,6 +7,7 @@ import { invalidateGroupInfoCache, mergeTimelineElementsPreservingDowngrades, } from "./timelineDOM"; +import { isTimelineIgnoredElement } from "./timelineElementHelpers"; import type { TimelineElement } from "../store/playerStore"; function el(id: string, extra: Partial = {}): TimelineElement { @@ -256,6 +257,33 @@ describe("createTimelineElementFromManifestClip — source-scoped selector ident }); }); +// Caught by looking at the studio, not by reading: a grouped composition drew +// "Voiceover • 0.0s – 12.0s" as a full-duration clip row directly above its own +// group header. `` is a mixer bus — no timing, drawn as a group +// row by the group derivation — but it is still a body child with an id, so the +// implicit-layer fallback happily gave it a track. Draggable and trimmable, and +// writing timing onto a bus means nothing. +describe(" is not a timeline layer", () => { + it("gets no implicit row of its own", () => { + const doc = makeDoc(` +
+ + +
+ `); + + const implicit = createImplicitTimelineLayersFromDOM(doc, 12, []); + + expect(implicit.map((el) => el.domId)).not.toContain("voiceover"); + }); + + it("is excluded by the shared ignore predicate", () => { + const doc = makeDoc(`
`); + expect(isTimelineIgnoredElement(doc.getElementById("vo") as Element)).toBe(true); + expect(isTimelineIgnoredElement(doc.getElementById("panel") as Element)).toBe(false); + }); +}); + describe("createImplicitTimelineLayersFromDOM — hfId from data-hf-id", () => { it("uses the runtime root paint scope for implicit siblings of manifest clips", () => { const doc = makeDoc(` diff --git a/packages/studio/src/player/lib/timelineElementHelpers.ts b/packages/studio/src/player/lib/timelineElementHelpers.ts index e8707e6eb..4ee0f4c93 100644 --- a/packages/studio/src/player/lib/timelineElementHelpers.ts +++ b/packages/studio/src/player/lib/timelineElementHelpers.ts @@ -11,6 +11,7 @@ import type { TimelineElement } from "../store/playerStore"; import type { ClipManifestClip } from "./playbackTypes"; import { isFinitePositive } from "./playbackAdapter"; import { getSourceScopedSelectorIndex } from "../../utils/sourceScopedSelectorIndex"; +import { HF_AUDIO_GROUP_TAG } from "@hyperframes/core/audio-groups"; // --------------------------------------------------------------------------- // Layer-reveal lift transparency @@ -81,6 +82,14 @@ function normalizePlaybackRate(raw: number): number { } export function isTimelineIgnoredElement(el: Element): boolean { + // An `` is a mixer bus, not a clip: it carries the group's + // label, fader, mute and FX chain, has no timing of its own, and is drawn as + // a GROUP ROW by the group derivation. Left in, the implicit-layer fallback + // also gave it an ordinary full-duration track — so a grouped composition + // showed "Voiceover • 0.0s – 12.0s" as a phantom clip directly above the real + // group header. Harmless-looking, but that row is draggable and trimmable, + // and writing timing onto the bus is meaningless. + if (el.tagName.toLowerCase() === HF_AUDIO_GROUP_TAG) return true; return Boolean( el.closest( [ diff --git a/packages/studio/src/player/store/playerStore.ts b/packages/studio/src/player/store/playerStore.ts index 1d1ed4737..389a0ca5a 100644 --- a/packages/studio/src/player/store/playerStore.ts +++ b/packages/studio/src/player/store/playerStore.ts @@ -21,7 +21,7 @@ import { createAudioSoloSlice, type AudioSoloSlice } from "./audioSoloSlice"; export type { KeyframeCacheEntry } from "./keyframeSlice"; export { liveTime } from "./liveTime"; -import type { TimelineElement } from "./timelineElement"; +import type { TimelineElement, TimelineElementPatch } from "./timelineElement"; export type { TimelineElement }; export type ZoomMode = "fit" | "manual"; @@ -143,22 +143,7 @@ interface PlayerState setSelectedElementId: (id: string | null, options?: SelectElementOptions) => void; /** Move the selection anchor within an active multi-selection without collapsing it. */ setSelectionAnchor: (id: string | null) => void; - updateElement: ( - elementId: string, - updates: Partial< - Pick< - TimelineElement, - | "start" - | "duration" - | "track" - | "zIndex" - | "hasExplicitZIndex" - | "playbackStart" - | "hidden" - | "audioGroup" - > - >, - ) => void; + updateElement: (elementId: string, updates: TimelineElementPatch) => void; setZoomMode: (mode: ZoomMode) => void; setManualZoomPercent: (percent: number) => void; bumpZEditVersion: () => void; diff --git a/packages/studio/src/player/store/timelineElement.ts b/packages/studio/src/player/store/timelineElement.ts index 2564f16b8..b73491835 100644 --- a/packages/studio/src/player/store/timelineElement.ts +++ b/packages/studio/src/player/store/timelineElement.ts @@ -86,3 +86,33 @@ export interface TimelineElement { expandedParentStart?: number; expandedHostKey?: string; } + +/** + * The fields `updateElement` may write. + * + * Deliberately a narrow allow-list rather than `Partial`: most + * of an element is derived from the document at parse time, and letting a + * caller poke those would put the store out of step with the file it mirrors. + * + * The `audioGroup*` entries are the GROUP's state, mirrored onto every member — + * a group row derives its label, fader, mute and chain from these, so a group + * write has to be able to land here or the header goes on rendering whatever it + * parsed at load. + */ +export type TimelineElementPatch = Partial< + Pick< + TimelineElement, + | "start" + | "duration" + | "track" + | "zIndex" + | "hasExplicitZIndex" + | "playbackStart" + | "hidden" + | "audioGroup" + | "audioGroupLabel" + | "audioGroupVolume" + | "audioGroupHidden" + | "audioGroupFxChain" + > +>;