From 94ecaa3ca1afe59c5fb5a036ac0dd757479c47a8 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 20 Aug 2026 16:11:35 -0700 Subject: [PATCH] fix(studio): align group lane labels with their curves, and three small cleanups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Group lane labels drifted off the curves they name.** The labels iterated raw `elementAutomationLanes` while the curves and the reserved height both use `groupAutomationLanes` — deduped by property and filtered for resolvability — and the old `if (!parts) return null` consumed an index without drawing a row. So one unresolvable target slid every later label one row off its own curve. Both sides read the same source now, and the label takes its name and parameter from the group entry rather than re-deriving them (the second of the two duplicate `automationLaneLabelParts` calls the review counted). **`useEffectiveTimelineDuration` restated `getEffectiveTimelineDuration`** with weaker guards: no non-finite check on the stored duration or the result, so an element carrying NaN timing returned NaN and every downstream width became NaN with it. It delegates now. **`createStableContext` warns on a genuine name collision.** Two modules asking for one name silently share ONE context, so a provider's value is read by the other's consumers and the symptom appears far from either file. Told apart from an HMR re-evaluation by the default value: a re-evaluated module registers the same default, a collision does not. **Documented that `groupNormalizeOptionUnsupported` is live, not dead regex.** A review pass claimed the `amix` normalize fallback could never match; ffmpeg 8.1.1 emits `Error applying option 'X' to filter 'amix': Option not found`, which the second test matches. Verified against the binary, and the comment now says so with the one wording that would miss (pre-4.4 libavfilter, which predates `normalize` existing). **Left alone deliberately:** the leftover wrapper `
` in `PlayerControls`. It is genuinely redundant — `PreviewPane` supplies its own flex wrapper — but removing it is a pure-cosmetic JSX re-indent of a 300-line component with no test that would catch a mistake, which is a bad trade against the rest of this batch. Noted for whoever is next in that file. studio 1356 player tests, engine grouping 11. fallow clean. --- packages/engine/src/services/audioMixer.ts | 8 +++++++ .../src/hooks/useEffectiveTimelineDuration.ts | 16 +++++++------- .../components/TimelineGroupLaneLabels.tsx | 21 ++++++++++--------- packages/studio/src/utils/hmrStableContext.ts | 17 +++++++++++++++ 4 files changed, 45 insertions(+), 17 deletions(-) diff --git a/packages/engine/src/services/audioMixer.ts b/packages/engine/src/services/audioMixer.ts index b333bfe91..656e64d48 100644 --- a/packages/engine/src/services/audioMixer.ts +++ b/packages/engine/src/services/audioMixer.ts @@ -889,6 +889,14 @@ async function mixAudioTracks( }; } +/** + * Verified against the real binary, because the wording is the whole contract: + * ffmpeg 8.1.1 emits `Error applying option 'X' to filter 'amix': Option not + * found`, which "option not found" matches. Only pre-4.4 libavfilter's + * `Option 'normalize' not found` phrasing sits outside the first test — and + * that build predates `normalize` existing at all, so it would fail for the + * right reason anyway. A review pass read this as dead and it is not. + */ function groupNormalizeOptionUnsupported(stderr: string): boolean { return ( /normalize/i.test(stderr) && diff --git a/packages/studio/src/hooks/useEffectiveTimelineDuration.ts b/packages/studio/src/hooks/useEffectiveTimelineDuration.ts index b89fd3b82..43d4c8a70 100644 --- a/packages/studio/src/hooks/useEffectiveTimelineDuration.ts +++ b/packages/studio/src/hooks/useEffectiveTimelineDuration.ts @@ -1,5 +1,6 @@ import { useMemo } from "react"; import type { TimelineElement } from "../player/store/timelineElement"; +import { getEffectiveTimelineDuration } from "../player/components/timelineViewModel"; /** * The stored `duration` lags a moment behind an edit that pushes an element @@ -10,11 +11,12 @@ export function useEffectiveTimelineDuration( timelineDuration: number, timelineElements: readonly TimelineElement[], ): number { - return useMemo(() => { - const maxEnd = - timelineElements.length > 0 - ? Math.max(...timelineElements.map((el) => el.start + el.duration)) - : 0; - return Math.max(timelineDuration, maxEnd); - }, [timelineDuration, timelineElements]); + // Delegates to `getEffectiveTimelineDuration` rather than restating the + // arithmetic: that one guards a non-finite stored duration and a non-finite + // result (an element with NaN timing), which this copy did not — it would + // return NaN and every downstream width became NaN with it. + return useMemo( + () => getEffectiveTimelineDuration(timelineDuration, timelineElements), + [timelineDuration, timelineElements], + ); } diff --git a/packages/studio/src/player/components/TimelineGroupLaneLabels.tsx b/packages/studio/src/player/components/TimelineGroupLaneLabels.tsx index 62e1b8847..6eb77512e 100644 --- a/packages/studio/src/player/components/TimelineGroupLaneLabels.tsx +++ b/packages/studio/src/player/components/TimelineGroupLaneLabels.tsx @@ -13,11 +13,7 @@ */ import { sampleAutomationLane } from "@hyperframes/core/audio-automation"; -import { - automationLaneLabelParts, - elementAutomationLanes, - elementFxChain, -} from "./automationLaneData"; +import { groupAutomationLanes } from "./automationLaneData"; import { AUTOMATION_LANE_H } from "./automationLaneHeight"; import type { TimelineElement } from "../store/playerStore"; import { useLivePlayheadTime } from "../../hooks/useLivePlayheadTime"; @@ -43,13 +39,18 @@ export function TimelineGroupLaneLabels({ // on seek, so the readout sat frozen while the curve was audibly working, // which is precisely the failure this number exists to prevent. const currentTime = useLivePlayheadTime(); - const chain = elementFxChain(groupElement); - const lanes = elementAutomationLanes(groupElement); + // The SAME source the curves and the reserved height use + // (`TimelineGroupRow`), not raw `elementAutomationLanes`. Raw lanes are + // neither deduped by property nor filtered for resolvability, and the old + // `if (!parts) return null` consumed an index without drawing — so one + // unresolvable target slid every later label one row off the curve it names. + const laneGroups = groupAutomationLanes([groupElement]); return ( <> - {lanes.map((lane, index) => { - const parts = automationLaneLabelParts(lane.target, chain); - if (!parts) return null; + {laneGroups.map((laneGroup, index) => { + const lane = laneGroup.entries[0]?.lane; + if (!lane) return null; + const parts = { name: laneGroup.name, param: laneGroup.param }; // A group's clock is composition time (§1.3), so the playhead needs no // clip-local rebase here — unlike a clip's lane. const value = sampleAutomationLane(lane, currentTime); diff --git a/packages/studio/src/utils/hmrStableContext.ts b/packages/studio/src/utils/hmrStableContext.ts index 68407e745..1c0ea4e2c 100644 --- a/packages/studio/src/utils/hmrStableContext.ts +++ b/packages/studio/src/utils/hmrStableContext.ts @@ -24,6 +24,10 @@ const REGISTRY = "__hfStudioContexts"; type Registry = Map>; +/** Default registered per name, so a genuine collision can be told from an HMR + * re-evaluation (which re-registers the same default). */ +const seenNames = new Map(); + function registry(): Registry { const host = globalThis as unknown as Record; const existing = host[REGISTRY]; @@ -39,6 +43,19 @@ function registry(): Registry { */ export function createStableContext(name: string, defaultValue: T): Context { const store = registry(); + // A collision is silent and its symptom is remote: two modules asking for the + // same name share ONE context, so one provider's value is read by the other's + // consumers and the bug surfaces as a wrong value far from either file. The + // convention is module path + export name; this makes breaking it loud. + if (store.has(name) && seenNames.get(name) !== defaultValue) { + // Not on the HMR path: a re-evaluated module hands back the SAME default it + // registered, which is how a hot reload keeps its context alive. + console.warn( + `[hmrStableContext] "${name}" was registered twice with different defaults — ` + + "two contexts are sharing one identity. Use module path + export name.", + ); + } + seenNames.set(name, defaultValue); const existing = store.get(name); if (existing) return existing as Context; const created = createContext(defaultValue);