mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-10 22:20:14 +00:00
fix(studio,core,engine): close the defects a max-effort review found in the fixes
A review of the five fix commits found eleven real defects, including a regression one of them introduced. Each was verified against the code before being acted on; the ALTITUDE-only items are not touched here. REGRESSION, from "group rows survive a collapse". Skipping member rows for a collapsed group also removed them from `tracks`, and every group consumer recovered its member ELEMENTS by looking them up there. Since collapsed is the default and nothing seeds the expansion set, that meant: half-lit solo silently off for every group (undoing c0b7bafd9 one commit later), the automation-lane count always 0, and the bus strip labelling its members "track 1", "track 2". Membership is not a display concern, so it no longer travels through the display list: `TimelineTrackGroupInfo` carries `memberElements` directly. Group bus. `reanchor` wrote `fader.gain.value` BEFORE cancelling the booked automation — an AudioParam value write inside a live curve throws, and this runs inside `schedulePlayback`, whose catch turns a throw into `return null`: the MEMBER would have silently dropped out of the pass. Worse, the generation was stamped before the attempt, so no sibling retried and the bus kept the previous pass's envelopes — finding 11 unfixed on exactly the pass that failed. Now: clear first, stamp only on success, and isolate the call. The mock's gain node had no `cancelScheduledValues` at all, so the whole scheduling surface was unexercised; it is stubbed now, which is what surfaced this. `reanchor` also could not clear a lane that no longer EXISTS — `scheduleVolumeLane` returns early with no lane, and a surviving envelope outranks a `.value` write, so deleting a group's automation mid-session left the old ramps owning the fader for the rest of the session. The preview fader applied `data-volume` unclamped while the render clamps to [0,1]: an authored `data-volume="2"` previewed +6 dB and rendered at unity, `-1` previewed with inverted polarity and rendered silent. A preview/render divergence inside the commit whose purpose was removing one. Pitch shift. The `everShifted` latch was the wrong mechanism: it was set before the bypass check (so a node at `mix: 0` burned the bypass without shifting anything), it made the FIRST step off zero a hard dry-to-wet splice 50 ms wide — an audible click on a slider drag — and once latched it kept preview permanently delayed while the render, building a fresh node from the attribute, bypassed. Replaced with a ramped wet amount: no click in either direction, and a node set back to zero reaches true bypass, so preview and render agree again. Silent no-ops. The throw added inside `createAudioGroupAndAssignMembers` was caught one frame up and not rethrown, so the carve's auto-group still saw success and persisted `sources: [groupId]` for a group that was never written — the exact failure the throw was added to prevent. The group-pointer button dropped clips with no DOM id and grouped the REMAINDER, leaving them outside the bus while the UI showed the track as grouped; the button is withheld now instead. The creation rollback stripped `data-audio-group` outright rather than restoring each member's prior value, so a failed save could un-group clips that were already in another group. `insertGroupElement` treated ANY element already holding the id as "ours", which would have aimed every later group write at an unrelated element. `setAudioMuteHidden` rescheduled Web Audio mid-play without `stopAll()`. Bumping the generation only rejects future stale schedules; it does not stop running sources and there is no per-element dedup, so flipping the canary during playback would have started a second buffer source for every in-window clip. `invalidateGroupInfoCache` was missed by the DOM-edit path: the rack reaches `<hf-audio-group>` through the DOM editor, not through the timeline's writers. Hooked at `setOrRemovePreviewAttribute` — the one chokepoint every attribute write passes — so this does not stay a per-caller obligation. Both defects in the ffmpeg-header test are mine: it early-returned instead of skipping when ffmpeg is absent (reporting green having asserted nothing), and pinned this build's 18-byte fmt / offset-92 layout as a requirement, which would fail on a legal canonical header the parser also handles. Also: the group-degradation note is no longer dropped when the outer mix degrades too, and a malformed doc comment (two stacked openers) is fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
07203f59d1
commit
7393095be8
@@ -28,21 +28,47 @@ import {
|
||||
type UseTimelineElementVisibilityEditingInput,
|
||||
} from "./timelineTrackVisibility";
|
||||
|
||||
/**
|
||||
* Assign (or restore) `data-audio-group` across a set of members.
|
||||
*
|
||||
* `restore` carries each member's PRIOR value so the unwind can put back a
|
||||
* membership that already existed, rather than removing the attribute outright.
|
||||
* `setElementsHidden`, which this mirrors, gets away with a plain `!hidden`
|
||||
* because hidden is boolean; group membership is an arbitrary id, and the carve
|
||||
* path does not check whether a clip is already grouped — so a failed save
|
||||
* could silently un-group clips that belonged to another group before it.
|
||||
*/
|
||||
function patchLiveAudioGroupState(
|
||||
iframe: HTMLIFrameElement | null,
|
||||
elements: readonly TimelineElement[],
|
||||
groupId: string | null,
|
||||
activeCompPath: string | null,
|
||||
restore?: ReadonlyMap<TimelineElement, string | null>,
|
||||
): void {
|
||||
for (const element of elements) {
|
||||
const target = findTimelineElementInIframe(iframe, element, activeCompPath);
|
||||
if (!target) continue;
|
||||
if (groupId) target.setAttribute(HF_AUDIO_GROUP_ATTR, groupId);
|
||||
const next = restore ? (restore.get(element) ?? null) : groupId;
|
||||
if (next) target.setAttribute(HF_AUDIO_GROUP_ATTR, next);
|
||||
else target.removeAttribute(HF_AUDIO_GROUP_ATTR);
|
||||
}
|
||||
invalidateGroupInfoCache(iframe?.contentDocument);
|
||||
}
|
||||
|
||||
/** Each member's `data-audio-group` before this write, for the unwind. */
|
||||
function captureAudioGroupState(
|
||||
iframe: HTMLIFrameElement | null,
|
||||
elements: readonly TimelineElement[],
|
||||
activeCompPath: string | null,
|
||||
): Map<TimelineElement, string | null> {
|
||||
const prior = new Map<TimelineElement, string | null>();
|
||||
for (const element of elements) {
|
||||
const target = findTimelineElementInIframe(iframe, element, activeCompPath);
|
||||
prior.set(element, target?.getAttribute(HF_AUDIO_GROUP_ATTR) ?? null);
|
||||
}
|
||||
return prior;
|
||||
}
|
||||
|
||||
/** Group ids are interpolated into markup and into a render-side filename, so
|
||||
* they stay in the character set an HTML id and a path can both carry. */
|
||||
const GROUP_ID_PATTERN = /^[A-Za-z0-9_-]+$/;
|
||||
@@ -63,7 +89,17 @@ const GROUP_ID_PATTERN = /^[A-Za-z0-9_-]+$/;
|
||||
* buys nothing.
|
||||
*/
|
||||
function insertGroupElement(html: string, groupId: string): string {
|
||||
if (readTagSnippetByTarget(html, { id: groupId }) !== undefined) return html;
|
||||
const existing = readTagSnippetByTarget(html, { id: groupId });
|
||||
if (existing !== undefined) {
|
||||
// Only OUR tag counts as "already there". The id was minted against the
|
||||
// live preview document, which does not contain markup that is on disk but
|
||||
// not rendered (inside a `<template>`, or an unloaded sub-composition) — so
|
||||
// an unrelated element can already own it. Writing nothing there would aim
|
||||
// every later group write (`buildPatchTarget({ domId })`) at that element,
|
||||
// stamping data-volume / data-hidden / data-fx-chain onto it.
|
||||
if (new RegExp(`^<\\s*${HF_AUDIO_GROUP_TAG}\\b`, "i").test(existing)) return html;
|
||||
throw new Error(`Cannot create audio group: id ${groupId} is already used in this file`);
|
||||
}
|
||||
const tag = `<${HF_AUDIO_GROUP_TAG} id="${groupId}"></${HF_AUDIO_GROUP_TAG}>`;
|
||||
const closeBody = html.lastIndexOf("</body>");
|
||||
if (closeBody < 0) return `${html}\n${tag}\n`;
|
||||
@@ -124,6 +160,7 @@ export async function createAudioGroupAndAssignMembers({
|
||||
throw new Error(`Invalid audio group id ${JSON.stringify(groupId)}`);
|
||||
}
|
||||
|
||||
const priorGroups = captureAudioGroupState(previewIframe, elements, activeCompPath);
|
||||
patchLiveAudioGroupState(previewIframe, elements, groupId, activeCompPath);
|
||||
const createdLiveGroupElement = patchLiveGroupElement(previewIframe, groupId);
|
||||
reseekPreviewRuntime(previewIframe);
|
||||
@@ -191,7 +228,7 @@ export async function createAudioGroupAndAssignMembers({
|
||||
// Mirrors setElementsHidden's failure path: the optimistic live patch
|
||||
// already ran, so a save failure has to be unwound or the preview shows a
|
||||
// grouping that never made it to disk.
|
||||
patchLiveAudioGroupState(previewIframe, elements, null, activeCompPath);
|
||||
patchLiveAudioGroupState(previewIframe, elements, null, activeCompPath, priorGroups);
|
||||
if (createdLiveGroupElement) {
|
||||
previewIframe?.contentDocument?.getElementById(groupId)?.remove();
|
||||
}
|
||||
@@ -263,6 +300,12 @@ export function useAudioGroupCarveAssignment({
|
||||
console.error("[Timeline] Failed to group voice clips", error);
|
||||
const message = error instanceof Error ? error.message : "Failed to group voice clips";
|
||||
showToast(message);
|
||||
// Rethrown, not just reported: the carve's auto-group chains
|
||||
// `.then(() => ({ ...next, sources: [groupId] }))` off this promise, so
|
||||
// swallowing here let it persist a carve pointing at a group that was
|
||||
// never written — the exact silent no-op the throw inside
|
||||
// `createAudioGroupAndAssignMembers` exists to prevent.
|
||||
throw error;
|
||||
}
|
||||
},
|
||||
[
|
||||
|
||||
@@ -115,8 +115,11 @@ describe("useAudioGroupCarveAssignment", () => {
|
||||
act(() => root.unmount());
|
||||
});
|
||||
|
||||
// Loud, not silent: the caller persists the group id once this resolves.
|
||||
it("toasts instead of silently writing nothing when an id resolves to no clip", async () => {
|
||||
// Loud AND rejecting: the carve chains `.then(() => ({...next, sources:
|
||||
// [groupId]}))` off this promise, so a resolved-but-failed call let it
|
||||
// persist a carve aimed at a group that was never written. Toasting alone
|
||||
// was not enough — the promise has to carry the failure too.
|
||||
it("rejects, and toasts, when an id resolves to no clip", async () => {
|
||||
stubProjectFiles(new Map([["index.html", FILE]]));
|
||||
usePlayerStore.getState().setElements([audio({ domId: "voice-1" })]);
|
||||
|
||||
@@ -126,7 +129,7 @@ describe("useAudioGroupCarveAssignment", () => {
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
await assign(["voice-1", "voice-gone"], "voiceover");
|
||||
await expect(assign(["voice-1", "voice-gone"], "voiceover")).rejects.toThrow("voice-gone");
|
||||
});
|
||||
|
||||
expect(writes.size).toBe(0);
|
||||
|
||||
@@ -9,6 +9,8 @@ import type { PersistDomEditOperations } from "./domEditCommitTypes";
|
||||
import { reportDomEditPersistFailure } from "./domEditPersistFailure";
|
||||
import { bumpDomEditCommitMapVersion, runDomEditCommit } from "./domEditCommitRunner";
|
||||
import { syncStoredAutomationFromPreview } from "../player/lib/automationStoreSync";
|
||||
import { HF_AUDIO_GROUP_TAG } from "@hyperframes/core/audio-groups";
|
||||
import { invalidateGroupInfoCache } from "../player/lib/timelineDOM";
|
||||
|
||||
// ── Types ──
|
||||
|
||||
@@ -63,6 +65,17 @@ function setOrRemovePreviewAttribute(
|
||||
} else {
|
||||
el.setAttribute(fullAttr, value);
|
||||
}
|
||||
// Every DOM-edit attribute write funnels through here, which is the only
|
||||
// place that can catch a group edit made from the rack rather than from the
|
||||
// group header — `openGroupFxRack` hands the `<hf-audio-group>` to the DOM
|
||||
// editor, and that path never went near the timeline's own writers. The group
|
||||
// scan is cached against the preview Document, and group edits are live
|
||||
// patches so that document is never replaced; a stale entry is re-read on
|
||||
// every manifest tick, so the header's preset button then builds on the old
|
||||
// chain and discards the rack's edit.
|
||||
if (el.tagName.toLowerCase() === HF_AUDIO_GROUP_TAG) {
|
||||
invalidateGroupInfoCache(el.ownerDocument);
|
||||
}
|
||||
}
|
||||
|
||||
function findPreviewAttributeElement(
|
||||
|
||||
Reference in New Issue
Block a user