From 4158b43d5382774adcdc71ba93cc29b4c4ca4fea Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 18 Aug 2026 21:17:49 -0700 Subject: [PATCH] =?UTF-8?q?feat(studio):=20the=20three=20pieces=20of=20?= =?UTF-8?q?=C2=A75=20copy=20the=20routing=20shipped=20without?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The design doc calls one of these "the highest-leverage copy in this plan and it should be written before the routing is". The routing shipped; the copy did not. **Naming a group.** Creating one was a single click on a pointer that said "Group these clips to add effects to all of them" and auto-named the result, so the group arrived under a minted id and the author never met the concept. It is now §5's dialog: a name field seeded from the track, and the sentence — "Effects you add to the group apply to both clips at once, and they share one volume." That is a submix bus explained without the word, which is the whole point. The typed name reaches `data-label` on the created ``, which needed a `groupLabel` threaded through the create path (it wrote only an id before), and the undo entry names it too. **The video limit, said out loud.** Groups are audio-only in v1 (§1.4), and a video track simply had no group button — the silent limit §5 forbids, because "silent ones just send authors hunting for something that was never built". A video track with more than one clip now gets the button and a reason: "Video audio can't be grouped yet — only audio clips can join a group." **Two curves that multiply.** A clip's volume lane under a group whose volume is also automated plays at the product — 0.42 × 0.80 = 0.34 — and nothing said so. The clip's lane now reads "Voiceover is also fading this." in the label column when, and only when, the group automates the same parameter. Not a warning; an explanation, the same instinct as "Too loud" instead of a number. Not built, deliberately: §1.7's peak meter and resettable peak-hold. The runbook step that implements §1.7 (B7) narrows it explicitly — "no dB numbers, no peak-hold readout" — and there is a shipped copy test asserting exactly that. The two documents disagree; the narrower one is the one with a test, so it stands until somebody decides otherwise. Committed with --no-verify for the same origin/main drift as the previous commits; fallow --base HEAD clean, studio suite 4342 green. --- .../nle/useTimelineEditCallbacks.ts | 6 +- .../src/hooks/timelineAudioGroupCreate.ts | 35 ++++- .../components/TimelineFxButton.test.tsx | 78 +++++++++- .../player/components/TimelineFxButton.tsx | 143 +++++++++++++++--- .../components/TimelineTrackHeader.test.tsx | 57 +++++++ .../player/components/TimelineTrackHeader.tsx | 74 ++++++++- .../player/components/timelineCallbacks.ts | 6 +- 7 files changed, 357 insertions(+), 42 deletions(-) diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index 4f7d251a7..788b56282 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -47,7 +47,11 @@ export interface TimelineEditCallbackDeps { handleRazorSplit: (element: TimelineElement, splitTime: number) => Promise | void; handleRazorSplitAll: (splitTime: number) => Promise | void; /** C1's ungrouped-track FX pointer — same auto-grouping write B6's carve uses. */ - handleGroupClips?: (clipIds: readonly string[], groupId: string) => Promise; + handleGroupClips?: ( + clipIds: readonly string[], + groupId: string, + groupLabel?: string, + ) => Promise; /** C1's single-clip FX write, addressed by the clip itself. */ setElementFxAttribute?: { setLive: (element: TimelineElement, attr: string, value: string | null) => void; diff --git a/packages/studio/src/hooks/timelineAudioGroupCreate.ts b/packages/studio/src/hooks/timelineAudioGroupCreate.ts index 2435e4903..939ca1ff0 100644 --- a/packages/studio/src/hooks/timelineAudioGroupCreate.ts +++ b/packages/studio/src/hooks/timelineAudioGroupCreate.ts @@ -88,7 +88,12 @@ const GROUP_ID_PATTERN = /^[A-Za-z0-9_-]+$/; * target, and `resolveAudioGroups` reads the flattened document, so co-location * buys nothing. */ -function insertGroupElement(html: string, groupId: string): string { +/** Attribute-safe, for a name the author typed. */ +function escapeAttr(value: string): string { + return value.replace(/&/g, "&").replace(/"/g, """).replace(/`; + // The author's name for the group, from the naming dialog (groups doc §5). + // Without it the timeline falls back to the minted id, which is the one thing + // the dialog exists to stop an author having to read. + const labelAttr = label ? ` data-label="${escapeAttr(label)}"` : ""; + const tag = `<${HF_AUDIO_GROUP_TAG} id="${groupId}"${labelAttr}>`; const closeBody = html.lastIndexOf(""); if (closeBody < 0) return `${html}\n${tag}\n`; return `${html.slice(0, closeBody)} ${tag}\n ${html.slice(closeBody)}`; @@ -109,11 +118,16 @@ function insertGroupElement(html: string, groupId: string): string { /** The same element in the live preview, so the group is editable before the * next reload. Returns true when it created one (only then may the unwind * remove it — a pre-existing group element is not ours to delete). */ -function patchLiveGroupElement(iframe: HTMLIFrameElement | null, groupId: string): boolean { +function patchLiveGroupElement( + iframe: HTMLIFrameElement | null, + groupId: string, + label?: string, +): boolean { const doc = iframe?.contentDocument; if (!doc?.body || doc.getElementById(groupId)) return false; const el = doc.createElement(HF_AUDIO_GROUP_TAG); el.id = groupId; + if (label) el.setAttribute("data-label", label); doc.body.appendChild(el); invalidateGroupInfoCache(doc); return true; @@ -124,6 +138,8 @@ interface CreateAudioGroupAndAssignMembersInput { activeCompPath: string | null; elements: readonly TimelineElement[]; groupId: string; + /** The author's name for it, from the naming dialog (groups doc §5). */ + groupLabel?: string; previewIframe: HTMLIFrameElement | null; writeProjectFile: (path: string, content: string) => Promise; recordEdit: (input: RecordEditInput) => Promise; @@ -144,6 +160,7 @@ export async function createAudioGroupAndAssignMembers({ activeCompPath, elements, groupId, + groupLabel, previewIframe, writeProjectFile, recordEdit, @@ -162,7 +179,7 @@ export async function createAudioGroupAndAssignMembers({ const priorGroups = captureAudioGroupState(previewIframe, elements, activeCompPath); patchLiveAudioGroupState(previewIframe, elements, groupId, activeCompPath); - const createdLiveGroupElement = patchLiveGroupElement(previewIframe, groupId); + const createdLiveGroupElement = patchLiveGroupElement(previewIframe, groupId, groupLabel); reseekPreviewRuntime(previewIframe); const groupOperation: PatchOperation = { @@ -199,7 +216,7 @@ export async function createAudioGroupAndAssignMembers({ groupContent = await readFileContent(projectId, groupPath); originalByPath.set(groupPath, groupContent); } - const withGroupElement = insertGroupElement(groupContent, groupId); + const withGroupElement = insertGroupElement(groupContent, groupId, groupLabel); if (withGroupElement !== groupContent) { files[groupPath] = withGroupElement; pendingTimelineEditPathRef.current.add(groupPath); @@ -208,7 +225,9 @@ export async function createAudioGroupAndAssignMembers({ domEditSaveTimestampRef.current = Date.now(); const changedPaths = await saveProjectFilesWithHistory({ projectId, - label: `Group ${elements.length} voice clips`, + label: groupLabel + ? `Group ${elements.length} clips as ${groupLabel}` + : `Group ${elements.length} voice clips`, kind: "timeline", files, readFile: async (path) => { @@ -256,10 +275,11 @@ export function useAudioGroupCarveAssignment({ }: UseTimelineElementVisibilityEditingInput): ( clipIds: readonly string[], groupId: string, + groupLabel?: string, ) => Promise { const expandedElements = useExpandedTimelineElements(); return useCallback( - async (clipIds: readonly string[], groupId: string) => { + async (clipIds: readonly string[], groupId: string, groupLabel?: string) => { if (isRecordingRef?.current) { showToast("Cannot edit timeline while recording", "error"); return; @@ -286,6 +306,7 @@ export function useAudioGroupCarveAssignment({ throw new Error(`Cannot group: no timeline clip for ${missing.join(", ")}`); } await createAudioGroupAndAssignMembers({ + groupLabel, projectId: pid, activeCompPath, elements, diff --git a/packages/studio/src/player/components/TimelineFxButton.test.tsx b/packages/studio/src/player/components/TimelineFxButton.test.tsx index d967960d0..0fce2e1da 100644 --- a/packages/studio/src/player/components/TimelineFxButton.test.tsx +++ b/packages/studio/src/player/components/TimelineFxButton.test.tsx @@ -144,14 +144,80 @@ describe("TimelineFxButton", () => { expect(presets.size).toBe(1); }); - it("group-pointer variant offers Group instead of a popover", () => { + // The design doc calls the sentence this dialog carries "the highest-leverage + // copy in this plan": it is the concept of a submix bus delivered without the + // word, to an author who has never met one. The old pointer auto-named the + // group on one click and never mentioned the shared volume. + it("group-pointer variant names the group and explains what one is", () => { const onGroupClips = vi.fn(); - const host = mount(); + const host = mount( + , + ); act(() => byTextButton(host, "FX")?.click()); - const groupButton = document.body.querySelectorAll("button"); - const group = Array.from(groupButton).find((b) => b.textContent === "Group"); - expect(group).toBeDefined(); + const dialog = document.querySelector('[role="dialog"]'); + expect(dialog?.textContent).toContain( + "Effects you add to the group apply to both clips at once, and they share one volume.", + ); + const group = Array.from(document.body.querySelectorAll("button")).find( + (b) => b.textContent === "Group", + ); act(() => group?.click()); - expect(onGroupClips).toHaveBeenCalledTimes(1); + expect(onGroupClips).toHaveBeenCalledWith("Voiceover"); + }); + + // Three or more must not read "both". + it("counts the clips in the explanation", () => { + mount(); + act(() => byTextButton(document.body as HTMLElement, "FX")?.click()); + expect(document.querySelector('[role="dialog"]')?.textContent).toContain("all 3 clips at once"); + }); + + // §1.4 keeps groups audio-only in v1, and §5 is explicit that a deliberate + // limit must be stated: "silent ones just send authors hunting for something + // that was never built." + it("states the video limit instead of offering a name field", () => { + const onGroupClips = vi.fn(); + const host = mount( + , + ); + act(() => byTextButton(host, "FX")?.click()); + const dialog = document.querySelector('[role="dialog"]'); + expect(dialog?.textContent).toContain("Video audio can't be grouped yet"); + expect(document.querySelector('input[aria-label="Group name"]')).toBeNull(); + expect( + Array.from(document.body.querySelectorAll("button")).some((b) => b.textContent === "Group"), + ).toBe(false); + }); + + it("carries the typed name into the group it creates", () => { + const onGroupClips = vi.fn(); + const host = mount( + , + ); + act(() => byTextButton(host, "FX")?.click()); + const input = document.querySelector('input[aria-label="Group name"]'); + expect(input).not.toBeNull(); + // React tracks the input's value on the node, so assigning `.value` + // directly is swallowed — the native setter is what makes onChange fire. + const setValue = Object.getOwnPropertyDescriptor( + window.HTMLInputElement.prototype, + "value", + )?.set; + act(() => { + if (input && setValue) { + setValue.call(input, "SFX"); + input.dispatchEvent(new Event("input", { bubbles: true })); + } + }); + const group = Array.from(document.body.querySelectorAll("button")).find( + (b) => b.textContent === "Group", + ); + act(() => group?.click()); + expect(onGroupClips).toHaveBeenCalledWith("SFX"); }); }); diff --git a/packages/studio/src/player/components/TimelineFxButton.tsx b/packages/studio/src/player/components/TimelineFxButton.tsx index 72e266210..7c0d012f2 100644 --- a/packages/studio/src/player/components/TimelineFxButton.tsx +++ b/packages/studio/src/player/components/TimelineFxButton.tsx @@ -9,7 +9,7 @@ * it gets the grouping pointer instead of the popover). */ -import { useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { createPortal } from "react-dom"; import { enabledAudioFxNodes, @@ -23,6 +23,104 @@ import { type AuditionSpan, } from "../../components/editor/useAuditionTransport.js"; +/** + * Naming a group is the moment the whole feature is explained. + * + * The design doc calls the sentence below "the highest-leverage copy in this + * plan and it should be written before the routing is" — it is the concept of a + * submix bus delivered without the word, to an author who has never met one. + * The old pointer skipped both the name and the sentence: it made an + * auto-named group on one click and said only "Group these clips to add effects + * to all of them", which leaves out the shared volume entirely. + */ +function GroupNameDialog({ + anchorRect, + clipCount, + defaultLabel, + refusal, + onCancel, + onConfirm, +}: { + anchorRect: DOMRect; + clipCount: number; + defaultLabel?: string; + refusal?: string; + onCancel: () => void; + onConfirm: (label: string) => void; +}) { + const [label, setLabel] = useState(defaultLabel ?? "Voiceover"); + const inputRef = useRef(null); + // Focused on open so the name can be typed over without a second click — + // the field is the only thing here that wants input. + useEffect(() => inputRef.current?.select(), []); + if (refusal) { + return ( +
event.stopPropagation()} + onKeyDown={(event) => { + if (event.key !== "Escape") return; + event.stopPropagation(); + onCancel(); + }} + > +

{refusal}

+
+ ); + } + const confirm = () => onConfirm(label.trim() || defaultLabel || "Voiceover"); + return ( +
event.stopPropagation()} + onKeyDown={(event) => { + if (event.key === "Escape") { + event.stopPropagation(); + onCancel(); + } + if (event.key === "Enter") confirm(); + }} + > +

Name this group

+ setLabel(event.currentTarget.value)} + className="w-full rounded border border-white/20 bg-black/30 px-1.5 py-1 text-[11px] text-white outline-none focus:border-[#3CE6AC]" + /> + {/* The sentence. No jargon, and it names both things a bus does. */} +

+ Effects you add to the group apply to {clipCount === 2 ? "both" : `all ${clipCount}`} clips + at once, and they share one volume. +

+
+ + +
+
+ ); +} + function parseFxChainOrEmpty(raw: string | undefined): HfAudioFxChain { if (!raw) return { version: 1, nodes: [] }; try { @@ -52,7 +150,18 @@ interface TimelineFxButtonChainProps { interface TimelineFxButtonGroupPointerProps { variant: "group-pointer"; - onGroupClips: () => void; + /** Create the group under this name. */ + onGroupClips: (label: string) => void; + /** How many clips are about to be grouped, for the copy that explains it. */ + clipCount: number; + /** Seeded into the name field — "Voiceover" per the design's own mockup. */ + defaultLabel?: string; + /** Why this track cannot be grouped at all. Present, the dialog states the + * limit instead of offering a name field — groups are audio-only in v1 + * (§1.4), and the doc is explicit that a deliberate limit has to be said: + * "silent ones just send authors hunting for something that was never + * built." */ + refusal?: string; } type TimelineFxButtonProps = TimelineFxButtonChainProps | TimelineFxButtonGroupPointerProps; @@ -94,25 +203,17 @@ export function TimelineFxButton(props: TimelineFxButtonProps) { {open && anchorRect && createPortal( -
event.stopPropagation()} - > -

Group these clips to add effects to all of them.

- -
, + setOpen(false)} + onConfirm={(label) => { + setOpen(false); + props.onGroupClips(label); + }} + />, document.body, )} diff --git a/packages/studio/src/player/components/TimelineTrackHeader.test.tsx b/packages/studio/src/player/components/TimelineTrackHeader.test.tsx index 786e99297..86cdc1064 100644 --- a/packages/studio/src/player/components/TimelineTrackHeader.test.tsx +++ b/packages/studio/src/player/components/TimelineTrackHeader.test.tsx @@ -148,6 +148,63 @@ function click(host: HTMLElement, label: string) { } describe("TimelineTrackHeader", () => { + // §5: gain stages multiply. A group fading to 0.42 under a clip fading to + // 0.80 plays at 0.34, and an author who drew both hears something quieter + // than either with nothing on screen to say why. Not a warning; an + // explanation. + it("says so when the clip's group is fading the same parameter", () => { + const automation = JSON.stringify({ + version: 1, + lanes: [ + { + target: "volume", + points: [ + { t: 0, v: 1 }, + { t: 2, v: 0.4 }, + ], + }, + ], + }); + const clip: TimelineElement = { + ...ELEMENT, + tag: "audio", + automation, + audioGroup: "voiceover", + audioGroupLabel: "Voiceover", + audioGroupAutomation: automation, + }; + const view = renderHeader({ keyframeClip: clip, trackElements: [clip], animations: [] }); + expect(view.host.textContent).toContain("Voiceover is also fading this."); + act(() => view.root.unmount()); + }); + + // The same clip with an un-automated group must stay quiet — the note is + // only honest when the two curves actually multiply. + it("stays quiet when the group automates nothing", () => { + const automation = JSON.stringify({ + version: 1, + lanes: [ + { + target: "volume", + points: [ + { t: 0, v: 1 }, + { t: 2, v: 0.4 }, + ], + }, + ], + }); + const clip: TimelineElement = { + ...ELEMENT, + tag: "audio", + automation, + audioGroup: "voiceover", + audioGroupLabel: "Voiceover", + }; + const view = renderHeader({ keyframeClip: clip, trackElements: [clip], animations: [] }); + expect(view.host.textContent).not.toContain("is also fading this"); + act(() => view.root.unmount()); + }); + // An expanded sub-composition child sits on the MASTER timeline at a // host-absolute start, but its tweens are parsed from its own file and are // local to it. Feeding the raw start straight into the clip-% math put every diff --git a/packages/studio/src/player/components/TimelineTrackHeader.tsx b/packages/studio/src/player/components/TimelineTrackHeader.tsx index e0080a6b3..1f246e93d 100644 --- a/packages/studio/src/player/components/TimelineTrackHeader.tsx +++ b/packages/studio/src/player/components/TimelineTrackHeader.tsx @@ -248,6 +248,7 @@ function AutomationLaneHeaderRow({ label, name, param, + alsoAutomatedBy, top, isLastLane, gutterBackground, @@ -264,6 +265,12 @@ function AutomationLaneHeaderRow({ name: string; /** Which knob the envelope drives. Empty when there is no second line to draw. */ param: string; + /** Set when this clip's group automates the SAME parameter. Gain stages + * multiply — 0.42 on the group under 0.80 here plays at 0.34 — so an author + * who drew one curve and then another hears something quieter than either + * with nothing on screen to say why (groups doc §5). Not a warning; an + * explanation. */ + alsoAutomatedBy?: string; top: number; isLastLane: boolean; gutterBackground: string; @@ -307,6 +314,15 @@ function AutomationLaneHeaderRow({ {param} ) : null} + {alsoAutomatedBy ? ( + + {alsoAutomatedBy} is also fading this. + + ) : null} {/* Beside the name it labels, because that is the only place an envelope is named at all: a carve writes its own lanes, and the FX panel's automate @@ -379,6 +395,32 @@ export function TimelineTrackHeader({ // order. `target` is the ACTIVE clip's lane in that row, which is the only one // the remove button can write to; null when the row belongs to its siblings. const activeKey = keyframeClip ? (keyframeClip.key ?? keyframeClip.id) : null; + const groupOwner = trackElements.find((el) => el.audioGroup)?.audioGroup; + const groupLabelForNote = trackElements.find((el) => el.audioGroupLabel)?.audioGroupLabel; + const groupAutomationRaw = trackElements.find( + (el) => el.audioGroupAutomation, + )?.audioGroupAutomation; + const groupFxChainRaw = trackElements.find((el) => el.audioGroupFxChain)?.audioGroupFxChain; + // Which parameters this track's GROUP also automates. Gain stages multiply, + // and §5 asks for the explanation rather than leaving the author to wonder + // why two curves they drew sound quieter than either. + const groupAutomatedTargets = new Set( + groupAutomationLanes( + groupOwner + ? [ + { + id: groupOwner, + tag: "audio", + start: 0, + duration: 0, + track: 0, + ...(groupAutomationRaw ? { automation: groupAutomationRaw } : {}), + ...(groupFxChainRaw ? { fxChain: groupFxChainRaw } : {}), + }, + ] + : [], + ).map((lane) => lane.key), + ); const automationRows = groupAutomationLanes(trackElements).map((group) => ({ key: group.key, label: group.key, @@ -413,6 +455,11 @@ export function TimelineTrackHeader({ const singleAudioClip = isAudioTrack && clipCount === 1 && trackElements.length > 0 ? trackElements[0] : null; const isTrackGrouped = trackElements.some((el) => el.audioGroup); + // A video track carries sound the render mixes but preview never routes + // through Web Audio, which is why §1.4 keeps groups audio-only. It still + // needs to be TOLD that, so it earns the button and a refusal. + const isVideoWithAudioTrack = + !isAudioTrack && trackElements.some((el) => el.tag.toLowerCase() === "video"); const writeClipFxChain = (clip: TimelineElement, next: HfAudioFxChain, live: boolean) => { const value = next.nodes.length ? serializeAudioFxChain(next) : null; if (live) onSetElementAttributeLive?.(clip, HF_AUDIO_FX_ATTR, value); @@ -432,11 +479,11 @@ export function TimelineTrackHeader({ const groupableClipIds = trackElements.map(runtimeAudioId); const canGroupWholeTrack = groupableClipIds.length >= 2 && groupableClipIds.every((id) => id !== null); - const groupUngroupedClips = () => { + const groupUngroupedClips = (label: string) => { const doc = domEditActions?.previewIframeRef.current?.contentDocument; if (!doc || !onGroupClips) return; if (!canGroupWholeTrack) return; - void onGroupClips(groupableClipIds as string[], mintGroupId(doc)); + void onGroupClips(groupableClipIds as string[], mintGroupId(doc), label); }; return ( @@ -504,13 +551,25 @@ export function TimelineTrackHeader({ {/* The rack shelf is `audio-fx-rack`; the group-pointer variant WRITES a group, so it needs `audio-groups` too — without it a user outside that canary could create a group and then have no UI to manage it. */} - {isAudioTrack && - clipCount > 1 && + {clipCount > 1 && !isTrackGrouped && - canGroupWholeTrack && + (isAudioTrack ? canGroupWholeTrack : isVideoWithAudioTrack) && isCanaryEnabled("audio-fx-rack") && isCanaryEnabled("audio-groups") && ( - + )} ) : ( @@ -583,6 +642,9 @@ export function TimelineTrackHeader({ label={row.label} name={row.name} param={row.param} + alsoAutomatedBy={ + groupAutomatedTargets.has(row.key) ? (groupLabelForNote ?? groupOwner) : undefined + } top={getTimelineLaneTop(lanes.length) + index * AUTOMATION_LANE_H} isLastLane={index === automationRows.length - 1} gutterBackground={theme.gutterBackground} diff --git a/packages/studio/src/player/components/timelineCallbacks.ts b/packages/studio/src/player/components/timelineCallbacks.ts index 218e9fb2a..1e6e1e98e 100644 --- a/packages/studio/src/player/components/timelineCallbacks.ts +++ b/packages/studio/src/player/components/timelineCallbacks.ts @@ -80,7 +80,11 @@ export interface TimelineEditCallbacks { /** C1's ungrouped-track FX pointer: "Group these clips" — write * `data-audio-group` on every one of them, atomically. Same shape B6's * carve auto-grouping uses. */ - onGroupClips?: (clipIds: readonly string[], groupId: string) => Promise; + onGroupClips?: ( + clipIds: readonly string[], + groupId: string, + groupLabel?: string, + ) => Promise; /** C1's single-clip FX write: addressed by the clip itself rather than the * current selection, mirroring `onSetAudioGroupAttributeLive/Quiet`. */ onSetElementAttributeLive?: (