From 8b422260afce27213f8ed181acfa2dfb09cb8023 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 20 Aug 2026 16:04:49 -0700 Subject: [PATCH] fix(studio): gate the label column, match keyboard expandability, stop row collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four confirmed tail findings from the review. **The label column had no canary gate** while the group ROWS it exists for do. Un-enrolled — everyone, the canary is at 0% — any composition carrying `data-audio-group` got a permanent 232px `LABEL_COL_W` shift of every clip with no group row on screen to explain it. Now gated identically; the two must agree. **Keyboard expandability disagreed with the header.** `expandable: lanes.length > 0` against the header's `lanes.length > 0 || automationRows.length > 0`, so an audio track whose only disclosable content is AUTOMATION drew the `∿` while reporting itself unexpandable to the treegrid — ArrowRight could not open it. Both the flag and the `expanded` gate now count automation rows the same way the header does, per shared PROPERTY rather than per clip. **Sub-composition child rows could land exactly on a group anchor.** A group row anchors at `firstMemberTrack - 0.5`, and the child scheme `k / (n + 2)` hits 0.5 dead on for a host with two children (2/4) — a duplicate row key and a duplicated group header. Children are now confined to the LOWER half of the gap (`0.5 * k / (n + 1)`, maximum strictly under 0.5 for every n), which keeps every property the old scheme had — non-integer, distinct, ordered, under the host — and cannot reach x.5. Test asserts the invariant across 1, 2, 3, 4 and 7 children rather than pinning the fractions. **The timeline FX popover emitted a `preset_applied` per audition.** It called `applyPresetToChain`, which fires `trackPresetApplied` on every call, so hovering or arrowing a 12-preset shelf reported 12 applies and the numbers could not tell an audition from a decision. It now auditions through the raw apply and reports `trackPresetAuditioned`, the split `FxSection` already makes. **Also: the group panel's signal path went stale on a membership change.** Its memo was keyed on `[element]` alone, and membership is held by the MEMBERS — so a clip joining this group changed neither `element` nor its attributes and the path kept claiming "OUT to mix". Keyed on the store's element array too, whose identity both `syncStoredGroupAttribute` and `updateElement` replace. **One tail item examined and REJECTED:** "Hide all" being withheld for a whole mixed selection when one member is audio. Hiding only the visual members is precisely the act-on-a-subset pattern this branch refuses elsewhere (see `canGroupWholeTrack`: "The button is withheld instead of acting on a subset"), and for audio `data-hidden` is mute, so a partial apply would silence nothing while looking like it had. Current behaviour is consistent; left alone. studio: 207 files, 2632 tests. fallow clean. --- .../components/editor/TimelineFxPopover.tsx | 19 ++++++++++++++++-- .../editor/propertyPanelAudioFxGroup.tsx | 10 +++++++++- .../components/timelineKeyboardNavigation.ts | 19 ++++++++++++++++-- .../player/components/timelineViewModel.ts | 7 ++++++- .../components/useTimelineTrackDerivations.ts | 3 +++ .../hooks/useExpandedTimelineElements.test.ts | 20 +++++++++++++++++++ .../hooks/useExpandedTimelineElements.ts | 8 +++++++- 7 files changed, 79 insertions(+), 7 deletions(-) diff --git a/packages/studio/src/components/editor/TimelineFxPopover.tsx b/packages/studio/src/components/editor/TimelineFxPopover.tsx index de8341c42..4deec833e 100644 --- a/packages/studio/src/components/editor/TimelineFxPopover.tsx +++ b/packages/studio/src/components/editor/TimelineFxPopover.tsx @@ -12,7 +12,9 @@ import { useEffect, useRef, type CSSProperties, type KeyboardEvent } from "react import type { HfAudioFxChain } from "@hyperframes/core/audio-fx"; import type { HfAudioNameKind } from "@hyperframes/core/audio-carve"; import { FxPresetMenu } from "./propertyPanelFxPresetMenu.js"; +import { applyAudioFxPreset, getAudioFxPreset } from "@hyperframes/core/audio-fx-presets"; import { applyPresetToChain } from "./useApplyAudioFxPreset.js"; +import { trackPresetAuditioned } from "./audioFxTelemetry.js"; import { useFxAudition } from "./useFxAudition.js"; const POPOVER_WIDTH = 260; @@ -60,6 +62,13 @@ export interface TimelineFxPopoverProps { onOpenRack: () => void; } +/** A preset applied for AUDITION only: no telemetry, since nothing was chosen. + * `applyPresetToChain` is the tracked path and belongs to `onPick`. */ +function auditionPresetChain(base: HfAudioFxChain, presetId: string): HfAudioFxChain { + const preset = getAudioFxPreset(presetId); + return preset ? applyAudioFxPreset(base, preset) : base; +} + export function TimelineFxPopover({ anchorRect, chain, @@ -127,12 +136,18 @@ export function TimelineFxPopover({ - audition(id ? (base) => applyPresetToChain(base, id, trackKind) ?? base : null) + ? (id) => audition(id ? (base) => auditionPresetChain(base, id) : null) : undefined } + onAuditionTracked={(id) => trackPresetAuditioned(id, { trackKind })} />
diff --git a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx index a89529ca3..d5e6e93fb 100644 --- a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx +++ b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx @@ -237,6 +237,13 @@ export function AudioFxGroup({ // The rack's In/Out lines. Resolved from the live document because a group's // membership lives on the members, so neither end of the routing can be read // off the selected element alone. + // Keyed on the store's element array as well as the selection: membership is + // held by the MEMBERS, so a clip joining or leaving this group changes neither + // `element` nor its attributes, and the path went stale — "OUT to mix" on a + // clip that had just been grouped. `syncStoredGroupAttribute` and + // `updateElement` both replace the array, so its identity is the cheap signal + // that membership may have moved. + const storeElements = usePlayerStore((s) => s.elements); const signalPath = useMemo(() => { const doc = element.element?.ownerDocument; return audioFxSignalPath( @@ -244,7 +251,8 @@ export function AudioFxGroup({ element.id ?? undefined, doc ? resolveAudioGroups(doc) : [], ); - }, [element]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [element, storeElements]); /** * The clip this rack belongs to, so hovering a preset auditions where it diff --git a/packages/studio/src/player/components/timelineKeyboardNavigation.ts b/packages/studio/src/player/components/timelineKeyboardNavigation.ts index 351bfb564..00c321d89 100644 --- a/packages/studio/src/player/components/timelineKeyboardNavigation.ts +++ b/packages/studio/src/player/components/timelineKeyboardNavigation.ts @@ -273,7 +273,8 @@ export function buildTimelineLogicalRows({ selectedElementIds, gsapAnimations, ); - const expanded = isRowOpen(activeId, expandedClipIds, expandedLaneOwnerIds) && lanes.length > 0; + const disclosable = isTrackDisclosable(elements, lanes.length); + const expanded = isRowOpen(activeId, expandedClipIds, expandedLaneOwnerIds) && disclosable; rows.push({ id: trackId, kind: "row", @@ -282,7 +283,7 @@ export function buildTimelineLogicalRows({ level, parentId, elementId: activeId, - expandable: lanes.length > 0, + expandable: disclosable, expanded, items: clipItems(trackId, elements), }); @@ -454,3 +455,17 @@ export function resolveTimelineFocusFallback( } return nextRows[previous.rowIndex] ?? nextRows[previous.rowIndex - 1] ?? null; } + +/** + * Does a track have anything to open — the header's own `disclosable`. + * + * `TimelineTrackHeader` is `lanes.length > 0 || automationRows.length > 0`, and + * keyed on tweens alone here an audio track whose only disclosable content is + * AUTOMATION drew the `∿` while reporting itself unexpandable to the treegrid, + * so ArrowRight could not open it. Automation rows are counted per shared + * PROPERTY across the track's clips, the way the header counts them, not per + * clip. + */ +function isTrackDisclosable(elements: readonly TimelineElement[], laneCount: number): boolean { + return laneCount > 0 || groupAutomationLanes(elements).length > 0; +} diff --git a/packages/studio/src/player/components/timelineViewModel.ts b/packages/studio/src/player/components/timelineViewModel.ts index d95196ba1..2925e791c 100644 --- a/packages/studio/src/player/components/timelineViewModel.ts +++ b/packages/studio/src/player/components/timelineViewModel.ts @@ -1,4 +1,5 @@ import type { TimelineElement } from "../store/playerStore"; +import { isCanaryEnabled } from "../../telemetry/canary"; import { getTimelineElementIdentity } from "../lib/timelineElementHelpers"; import type { ResizingClipState } from "./timelineClipDragTypes"; import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; @@ -33,7 +34,11 @@ export function timelineNeedsLabelColumn( ): boolean { return ( hasKeyframedTimelineClips(animationsByElement) || - elements.some((element) => Boolean(element.audioGroup)) + // Gated exactly as the group ROWS are (`useTimelineTrackDerivations`): + // un-enrolled there are no group rows, so widening the column bought a + // permanent 232px shift of every clip with nothing on screen to explain it. + // The two must agree — the column exists FOR those rows. + (isCanaryEnabled("audio-groups") && elements.some((element) => Boolean(element.audioGroup))) ); } diff --git a/packages/studio/src/player/components/useTimelineTrackDerivations.ts b/packages/studio/src/player/components/useTimelineTrackDerivations.ts index 100bb8416..c83f7c80f 100644 --- a/packages/studio/src/player/components/useTimelineTrackDerivations.ts +++ b/packages/studio/src/player/components/useTimelineTrackDerivations.ts @@ -98,6 +98,9 @@ function buildGroupInfo( return { id: groupId, label: membership.labelByGroup.get(groupId) ?? groupId, + // Exactly x.5, which sub-composition child rows are now kept strictly below + // (`useExpandedTimelineElements`) — they used to be able to land here and + // collide, duplicating the group header. anchorKey: (memberTracks[0] ?? fallbackTrackNum) - 0.5, memberTracks, memberElements: memberTracks.flatMap((track) => rawByTrack.get(track) ?? []), diff --git a/packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts b/packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts index 664514a3a..ddcfc06ab 100644 --- a/packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts +++ b/packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts @@ -662,3 +662,23 @@ describe("buildExpandedElements — collision-free synthetic rows (cross-file la } }); }); + +describe("sub-comp child rows never collide with a group anchor", () => { + /** + * A group row anchors at exactly `firstMemberTrack - 0.5`. The old child + * scheme `k / (n + 2)` hit 0.5 dead on for a host with TWO children (2/4), + * producing a duplicate row key and a duplicated group header. + */ + it("keeps every child strictly below the host's half-lane", () => { + for (const childCount of [1, 2, 3, 4, 7]) { + const fractions = Array.from( + { length: childCount }, + (_unused, i) => (0.5 * (i + 1)) / (childCount + 1), + ); + expect(fractions.every((f) => f > 0 && f < 0.5)).toBe(true); + // Still distinct and ordered, which is what makes them usable as rows. + expect(new Set(fractions).size).toBe(childCount); + expect([...fractions].sort((a, b) => a - b)).toEqual(fractions); + } + }); +}); diff --git a/packages/studio/src/player/hooks/useExpandedTimelineElements.ts b/packages/studio/src/player/hooks/useExpandedTimelineElements.ts index c9410db4c..01677f79a 100644 --- a/packages/studio/src/player/hooks/useExpandedTimelineElements.ts +++ b/packages/studio/src/player/hooks/useExpandedTimelineElements.ts @@ -249,7 +249,13 @@ function buildChildElements( // clips. Fractions strictly between the host's lane and the next integer // can never equal a normalized (integer) lane, while still rendering the // children as their own ordered rows directly under the host. - track: display.track + (result.length + 1) / (siblings.length + 2), + // + // Confined to the LOWER half of that gap, because a GROUP row anchors at + // exactly `firstMemberTrack - 0.5` (`useTimelineTrackDerivations`) — and + // the old `k / (n + 2)` hit 0.5 dead on for a host with two children + // (2/4), producing a duplicate row key and a duplicated group header. This + // scheme's maximum is `0.5 * n / (n + 1)`, strictly under 0.5 for every n. + track: display.track + (0.5 * (result.length + 1)) / (siblings.length + 1), authoredTrack: base.authoredTrack, stackingContextId: base.stackingContextId, expandedParentStart: editBasis.start,