fix(studio): three defects a browser found that no amount of reading did

First time any of this stack has been looked at rather than reasoned
about. Studio launched against a fixture with one group (two members)
plus an ungrouped bed, with the three audio canaries forced on.

The group element drew a phantom CLIP row. `<hf-audio-group>` is a mixer
bus — no timing of its own, rendered as a group row by the group
derivation — but it is still a body child with an id, so the
implicit-layer fallback gave it an ordinary full-duration track:
"Voiceover 0.0s-12.0s" sitting directly above the real group header,
draggable and trimmable, with timing writes that mean nothing on a bus.
Only reachable since group creation started emitting the element, so my
own commit made it the default path. Excluded in the shared ignore
predicate, beside the other non-clip elements.

Group writes never reached the store. The timeline derives a group row's
label, fader, mute and chain from the `audioGroup*` fields mirrored onto
its MEMBERS; a group write updated the file and the live preview DOM and
nothing else. Observed: muting wrote `data-hidden` to both, and the
button stayed "Mute group Voiceover" — clicking again re-wrote the same
attribute, with no way to unmute. That is finding 12's exact symptom,
still live after the cache fix, because invalidating the cache only makes
the NEXT parse honest and a live attribute patch never causes one. Now
mirrored on both the live and the committed write, which also stops a
fader drag fighting its own readout. Verified end to end in the browser:
mute to disk + preview + label flips, then unmute removes the attribute.

The bus fader offered 0..2 while every consumer clamps to [0,1]. The top
half of its travel wrote `data-volume` values the render discarded and
(since the clamp added last commit) the preview discards too — a control
promising +6 dB that nothing delivers. Ceiling lowered to unity. Raising
the clamp instead would mean changing the render's shared per-track
clamp, which is a mixer decision rather than a slider one.

Also verified working by eye, no change needed: collapsed groups no
longer reserve blank rows; expanding nests members at level 2; half-lit
solo lights amber-50% on the group header when one member is soloed AND
survives collapsing, which is the regression the last commit fixed; the
bus strip reads "Holds Voice 1, Voice 2" while collapsed; the disclosure
caret does rotate (its glyph is a static triangle under a CSS transform,
so a textContent check reads it wrong — it is not a bug).

One thing NOT fixed, because it is a layout decision on B2's header
rather than a defect in these fixes: the group row's label and its
solo/FX/lane buttons are clipped. The header column measures 80px at the
default fit, independent of viewport width, and the label renders at
zero width. A normal track survives this because its clips carry the name
on the bar; a group row has no clip bar, so the gutter is the only place
its name exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Vance Ingalls
2026-08-20 16:39:39 -07:00
co-authored by Claude Opus 5
parent 7393095be8
commit ff4965037c
8 changed files with 252 additions and 28 deletions
@@ -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<string>() },
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<typeof useSetAudioGroupAttribute> | 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);
});
});
@@ -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";
@@ -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", () => {
@@ -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]"
@@ -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> = {}): 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. `<hf-audio-group>` 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("<hf-audio-group> is not a timeline layer", () => {
it("gets no implicit row of its own", () => {
const doc = makeDoc(`
<div data-composition-id="root">
<audio id="voice-1" data-start="0" data-duration="6" data-audio-group="voiceover"></audio>
<hf-audio-group id="voiceover" data-label="Voiceover"></hf-audio-group>
</div>
`);
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(`<hf-audio-group id="vo"></hf-audio-group><div id="panel"></div>`);
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(`
@@ -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 `<hf-audio-group>` 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(
[
@@ -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;
@@ -86,3 +86,33 @@ export interface TimelineElement {
expandedParentStart?: number;
expandedHostKey?: string;
}
/**
* The fields `updateElement` may write.
*
* Deliberately a narrow allow-list rather than `Partial<TimelineElement>`: 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"
>
>;