From 6de75fd4b4bae789c3004bd42ef837c36d5e584f Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 18 Aug 2026 12:33:07 -0700 Subject: [PATCH] fix(studio): make sub-composition audio groups actually work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Browser-verified the one surface that had never been run: a group and its members declared entirely inside a sub-composition. Both fixes written for that case were broken, and both of their tests passed — because I wrote fixtures that matched my assumption instead of the DOM. Membership never arrived. `hostElementState` inherits from the flat store twin, and a sub-comp that declares its own group keeps those members OUT of the flat store — the store held three elements (panel, sub-comp host, bed) and neither voice. So there was nothing to inherit from and no group row appeared at all. Membership now rides `DomClipChild`, captured during the DOM walk that is the only place holding the child's live element, with the flat twin still preferred when it exists. Routing never worked either. `getTimelineElementSourceFile` stops at the nearest `[data-composition-id]`, which for an inlined sub-composition is its own ROOT element — that carries the composition id but not the file. The file sits on the HOST above it: hf-audio-group#voiceover (no composition attrs) section#voices-root data-composition-id="voices" <- stopped here div#voices-host data-composition-file="...voices.html" <- file is here body data-composition-id="" My unit fixture put the file on the sub-comp root, so the test passed while the studio still threw "Unable to patch element in index.html" on every mute, fader move and FX preset. The resolver climbs composition ancestors until one names a file, and returns undefined for a root-level group so the caller still falls back to activeCompPath. The new tests use the ancestor shape copied from a live preview, and both fixes were mutation-checked. Verified end to end in the studio: the group row appears with its members nested, mute writes `data-hidden` into compositions/voices.html and not index.html, unmute removes it, and the fader writes data-volume="0.35" to the same file. Co-Authored-By: Claude Opus 5 (1M context) --- .../hooks/timelineAudioGroupVolume.test.ts | 47 +++++++++++++- .../src/hooks/timelineAudioGroupVolume.ts | 34 +++++++++- .../hooks/useExpandedTimelineElements.test.ts | 63 +++++++++++++++++++ .../hooks/useExpandedTimelineElements.ts | 42 ++++++++++--- .../player/hooks/useTimelineSyncCallbacks.ts | 25 ++++++++ .../studio/src/player/store/playerStore.ts | 11 ++++ 6 files changed, 208 insertions(+), 14 deletions(-) diff --git a/packages/studio/src/hooks/timelineAudioGroupVolume.test.ts b/packages/studio/src/hooks/timelineAudioGroupVolume.test.ts index 8fdc07d8b..ede1506bd 100644 --- a/packages/studio/src/hooks/timelineAudioGroupVolume.test.ts +++ b/packages/studio/src/hooks/timelineAudioGroupVolume.test.ts @@ -16,7 +16,7 @@ import { afterEach, describe, expect, it } from "vitest"; import { usePlayerStore, type TimelineElement } from "../player"; -import { useSetAudioGroupAttribute } from "./timelineAudioGroupVolume"; +import { resolveGroupSourceFile, useSetAudioGroupAttribute } from "./timelineAudioGroupVolume"; afterEach(() => { usePlayerStore.getState().reset(); @@ -128,3 +128,48 @@ describe("group attribute writes reach the store", () => { expect(byId.get("sfx")?.audioGroupVolume).toBe(1); }); }); + +/** + * The ancestor shape here is COPIED FROM A LIVE PREVIEW, not imagined. + * + * The first attempt at this fix used `getTimelineElementSourceFile` and a + * fixture in which the sub-composition root carried `data-composition-file`. + * The test passed; the studio still threw "Unable to patch element in + * index.html", because the runtime inlines a sub-comp as its own root element + * that carries only the composition ID — the FILE is on the host above it. + */ +describe("resolveGroupSourceFile", () => { + function livePreviewShape(): Document { + const doc = document.implementation.createHTMLDocument("preview"); + doc.body.setAttribute("data-composition-id", "subcomp-group-qa"); + doc.body.innerHTML = ` +
+
+ +
+
+ + `; + return doc; + } + + it("climbs past the sub-comp root to the host that names the file", () => { + const doc = livePreviewShape(); + expect(resolveGroupSourceFile(doc.getElementById("voiceover"))).toBe( + "compositions/voices.html", + ); + }); + + // A group in the root composition: body names an id but no file, so the + // caller falls back to activeCompPath. Returning body's id here would route + // every root-composition group write at a path that does not exist. + it("returns undefined for a group in the root composition", () => { + const doc = livePreviewShape(); + expect(resolveGroupSourceFile(doc.getElementById("root-group"))).toBeUndefined(); + }); + + it("is safe on a detached or missing element", () => { + expect(resolveGroupSourceFile(null)).toBeUndefined(); + expect(resolveGroupSourceFile(document.createElement("hf-audio-group"))).toBeUndefined(); + }); +}); diff --git a/packages/studio/src/hooks/timelineAudioGroupVolume.ts b/packages/studio/src/hooks/timelineAudioGroupVolume.ts index c9596252b..dc76d86f4 100644 --- a/packages/studio/src/hooks/timelineAudioGroupVolume.ts +++ b/packages/studio/src/hooks/timelineAudioGroupVolume.ts @@ -3,7 +3,6 @@ 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/timelineGroupInfo"; -import { getTimelineElementSourceFile } from "../player/lib/timelineElementHelpers"; import { buildPatchTarget, persistElementAttribute, @@ -83,6 +82,36 @@ function syncStoredGroupAttribute(groupId: string, attr: string, value: string | })); } +/** + * The composition FILE that contains a group element, walking up through + * composition ancestors until one names a file. + * + * `getTimelineElementSourceFile` stops at the nearest `[data-composition-id]`, + * which for an inlined sub-composition is its own ROOT element — that carries + * the composition id but not the file. The file is on the HOST one level above + * it. Measured on a live preview: + * + * hf-audio-group#voiceover (no composition attrs) + * section#voices-root data-composition-id="voices" <- stops here + * div#voices-host data-composition-file="…/voices.html" <- file is here + * body data-composition-id="" + * + * Returns undefined for a group in the root composition (body names an id but + * no file), which is exactly when the caller should fall back to activeCompPath. + */ +export function resolveGroupSourceFile(groupEl: Element | null): string | undefined { + let node: Element | null = groupEl?.parentElement ?? null; + while (node) { + const owner: Element | null = node.closest("[data-composition-id]"); + if (!owner) return undefined; + const file = + owner.getAttribute("data-composition-file") ?? owner.getAttribute("data-composition-src"); + if (file) return file; + node = owner.parentElement; + } + return undefined; +} + interface SetAudioGroupAttributeInput { projectId: string; activeCompPath: string | null; @@ -125,8 +154,7 @@ async function setAudioGroupAttribute({ // FX preset throws "Unable to patch element in index.html". Every sibling // timeline writer already routes `element.sourceFile || activeCompPath`. const groupEl = previewIframe?.contentDocument?.getElementById(groupId) ?? null; - const targetPath = - (groupEl ? getTimelineElementSourceFile(groupEl) : undefined) || activeCompPath || "index.html"; + const targetPath = resolveGroupSourceFile(groupEl) || activeCompPath || "index.html"; const patchTarget = buildPatchTarget({ domId: groupId }); if (!patchTarget) return []; diff --git a/packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts b/packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts index 4b7a6f600..664514a3a 100644 --- a/packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts +++ b/packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts @@ -598,4 +598,67 @@ describe("buildExpandedElements — collision-free synthetic rows (cross-file la // Distinct ordered rows per child. expect(children[0].track).not.toBe(children[1].track); }); + + /** + * A sub-composition that declares BOTH a group and its members keeps those + * members out of the flat store entirely — the store holds only the host. + * So "inherit membership from the flat twin" had nothing to inherit from, + * and the group produced no timeline row at all, for exactly the case group + * support was extended to cover. Verified against a real studio session + * before this test was written: the flat store held three elements (the + * panel, the sub-comp host and an ungrouped bed) and neither voice. + */ + it("takes group membership from the DOM child when there is no flat store twin", () => { + const elements = [ + el({ id: "voices-host", start: 0, duration: 12, compositionSrc: "voices.html" }), + ]; + const manifest = [ + clip({ id: "voices-host", start: 0, duration: 12, compositionSrc: "voices.html" }), + ]; + const parentMap = new Map([ + ["voice-1", "voices-host"], + ["voice-2", "voices-host"], + ]); + const domClipChildren = [ + { + id: "voice-1", + parentId: "voices-host", + hostId: "voices-host", + label: "voice-1", + stackingContextId: "css:0", + audioGroup: "voiceover", + audioGroupLabel: "Voiceover", + audioGroupVolume: 0.8, + audioGroupHidden: false, + }, + { + id: "voice-2", + parentId: "voices-host", + hostId: "voices-host", + label: "voice-2", + stackingContextId: "css:0", + audioGroup: "voiceover", + audioGroupLabel: "Voiceover", + audioGroupVolume: 0.8, + audioGroupHidden: false, + }, + ]; + + const out = buildExpandedElements( + elements, + manifest, + parentMap, + "voices-host", + "voices-host", + domClipChildren, + ); + + const voices = out.filter((e) => e.domId?.startsWith("voice-")); + expect(voices).toHaveLength(2); + for (const voice of voices) { + expect(voice.audioGroup).toBe("voiceover"); + expect(voice.audioGroupLabel).toBe("Voiceover"); + expect(voice.audioGroupVolume).toBeCloseTo(0.8, 6); + } + }); }); diff --git a/packages/studio/src/player/hooks/useExpandedTimelineElements.ts b/packages/studio/src/player/hooks/useExpandedTimelineElements.ts index b5561fcb4..14e0a6ea0 100644 --- a/packages/studio/src/player/hooks/useExpandedTimelineElements.ts +++ b/packages/studio/src/player/hooks/useExpandedTimelineElements.ts @@ -146,6 +146,31 @@ interface DisplayBounds { * could never be shown again (not even after a reload, since the attribute is in * the source). */ +/** + * Audio-group membership for an expanded child, from whichever source has it. + * + * The flat store twin when there is one; otherwise the `DomClipChild` record, + * which carried it off the live element during the DOM walk. That fallback is + * the ONLY source for a sub-composition that declares both a group and its + * members: those members never enter the flat store, so "inherit from the flat + * twin" silently produced no membership and therefore no group row — for + * exactly the case group support was extended to cover. + */ +function childGroupState( + flat: TimelineElement | undefined, + domChild: DomClipChild | undefined, +): Partial { + const source = flat?.audioGroup ? flat : domChild?.audioGroup ? domChild : null; + if (!source) return {}; + return { + audioGroup: source.audioGroup, + audioGroupLabel: source.audioGroupLabel, + audioGroupVolume: source.audioGroupVolume, + audioGroupHidden: source.audioGroupHidden, + audioGroupFxChain: source.audioGroupFxChain, + }; +} + function hostElementState(flat: TimelineElement | undefined): Partial { if (!flat) return {}; return { @@ -159,16 +184,6 @@ function hostElementState(flat: TimelineElement | undefined): Partial, ): TimelineElement[] { const result: TimelineElement[] = []; for (const child of siblings) { @@ -210,6 +226,10 @@ function buildChildElements( result.push({ ...base, ...hostElementState(elements.find((element) => element.key === key)), + ...childGroupState( + elements.find((element) => element.key === key), + domId ? domChildrenById.get(domId) : undefined, + ), key, start: clamped.start, duration: clamped.duration, @@ -312,6 +332,7 @@ export function buildExpandedElements( }; const parentKey = topLevelElement.key ?? topLevelElement.id; + const domChildrenById = new Map(domClipChildren.map((child) => [child.id, child])); const expanded = buildChildElements( siblings, { @@ -322,6 +343,7 @@ export function buildExpandedElements( editBasis, parentKey, elements, + domChildrenById, ); if (expanded.length === 0) return filterToTopLevel(elements, parentMap); diff --git a/packages/studio/src/player/hooks/useTimelineSyncCallbacks.ts b/packages/studio/src/player/hooks/useTimelineSyncCallbacks.ts index c4526e260..9b21562c3 100644 --- a/packages/studio/src/player/hooks/useTimelineSyncCallbacks.ts +++ b/packages/studio/src/player/hooks/useTimelineSyncCallbacks.ts @@ -12,6 +12,8 @@ import { useCallback } from "react"; import { liveTime, usePlayerStore } from "../store/playerStore"; import type { TimelineElement, DomClipChild } from "../store/playerStore"; import { resolveCssStackingContextId } from "@hyperframes/core/runtime/stacking-context"; +import { HF_AUDIO_GROUP_ATTR } from "@hyperframes/core/audio-groups"; +import { groupInfoFor } from "../lib/timelineGroupInfo"; import type { PlaybackAdapter, ClipManifestClip, IframeWindow } from "../lib/playbackTypes"; import { parseTimelineFromDOM, @@ -86,6 +88,28 @@ export function resolveReloadSeekTime(input: { return Math.min(target, input.duration); } +/** + * A sub-comp child's audio-group membership, read off its live element. + * + * Captured during the DOM walk because that walk holds the only reference to + * the element. A sub-composition that declares both a group and its members + * keeps those members out of the flat store entirely, so an expanded child has + * no flat twin to inherit membership from later — without this, a group defined + * inside a sub-composition produced no group row at all. + */ +function readChildAudioGroupState(child: Element): Partial { + const audioGroup = child.getAttribute(HF_AUDIO_GROUP_ATTR); + if (!audioGroup) return {}; + const info = groupInfoFor(child.ownerDocument, audioGroup); + return { + audioGroup, + audioGroupLabel: info.label, + audioGroupVolume: info.volume, + audioGroupHidden: info.hidden, + ...(info.fxChain ? { audioGroupFxChain: info.fxChain } : {}), + }; +} + /** Reject non-finite, non-positive, and absurdly large (loop-inflated) values. */ function sanitizeDurationSeconds(value: number): number { return Number.isFinite(value) && value > 0 && value < 7200 ? value : 0; @@ -201,6 +225,7 @@ export function useTimelineSyncCallbacks({ hostId, label: isGroup ? child.getAttribute("data-hf-group") || child.id : child.id, stackingContextId: resolveCssStackingContextId(child), + ...readChildAudioGroupState(child), }); parentMap.set(child.id, parentId); if (isGroup) collect(child, child.id); diff --git a/packages/studio/src/player/store/playerStore.ts b/packages/studio/src/player/store/playerStore.ts index 2e2bad769..8e23092cb 100644 --- a/packages/studio/src/player/store/playerStore.ts +++ b/packages/studio/src/player/store/playerStore.ts @@ -234,6 +234,17 @@ export interface DomClipChild { hostId: string; label: string; stackingContextId: string; + /** + * The child's audio-group state, read off its live element during the DOM + * walk — the only place that sees it. A sub-composition can declare a group + * and its members entirely within itself, and those members never reach the + * flat store, so an expanded child has no twin to inherit membership from. + */ + audioGroup?: string; + audioGroupLabel?: string; + audioGroupVolume?: number; + audioGroupHidden?: boolean; + audioGroupFxChain?: string; } interface BeatHistoryEntry {