mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix(studio): gate the label column, match keyboard expandability, stop row collisions
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.
This commit is contained in:
@@ -12,7 +12,9 @@ import { useEffect, useRef, type CSSProperties, type KeyboardEvent } from "react
|
|||||||
import type { HfAudioFxChain } from "@hyperframes/core/audio-fx";
|
import type { HfAudioFxChain } from "@hyperframes/core/audio-fx";
|
||||||
import type { HfAudioNameKind } from "@hyperframes/core/audio-carve";
|
import type { HfAudioNameKind } from "@hyperframes/core/audio-carve";
|
||||||
import { FxPresetMenu } from "./propertyPanelFxPresetMenu.js";
|
import { FxPresetMenu } from "./propertyPanelFxPresetMenu.js";
|
||||||
|
import { applyAudioFxPreset, getAudioFxPreset } from "@hyperframes/core/audio-fx-presets";
|
||||||
import { applyPresetToChain } from "./useApplyAudioFxPreset.js";
|
import { applyPresetToChain } from "./useApplyAudioFxPreset.js";
|
||||||
|
import { trackPresetAuditioned } from "./audioFxTelemetry.js";
|
||||||
import { useFxAudition } from "./useFxAudition.js";
|
import { useFxAudition } from "./useFxAudition.js";
|
||||||
|
|
||||||
const POPOVER_WIDTH = 260;
|
const POPOVER_WIDTH = 260;
|
||||||
@@ -60,6 +62,13 @@ export interface TimelineFxPopoverProps {
|
|||||||
onOpenRack: () => void;
|
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({
|
export function TimelineFxPopover({
|
||||||
anchorRect,
|
anchorRect,
|
||||||
chain,
|
chain,
|
||||||
@@ -127,12 +136,18 @@ export function TimelineFxPopover({
|
|||||||
<FxPresetMenu
|
<FxPresetMenu
|
||||||
trackKind={trackKind}
|
trackKind={trackKind}
|
||||||
onPick={applyPreset}
|
onPick={applyPreset}
|
||||||
|
// The RAW apply, not `applyPresetToChain` — that helper fires
|
||||||
|
// `trackPresetApplied` on every call, so auditioning a 12-preset shelf
|
||||||
|
// by hover or arrow key emitted 12 `preset_applied` events and the
|
||||||
|
// numbers could not tell an audition from a decision. `FxSection`
|
||||||
|
// makes exactly this split, with `onAuditionTracked` carrying the
|
||||||
|
// honest event.
|
||||||
onAudition={
|
onAudition={
|
||||||
onChainPreview
|
onChainPreview
|
||||||
? (id) =>
|
? (id) => audition(id ? (base) => auditionPresetChain(base, id) : null)
|
||||||
audition(id ? (base) => applyPresetToChain(base, id, trackKind) ?? base : null)
|
|
||||||
: undefined
|
: undefined
|
||||||
}
|
}
|
||||||
|
onAuditionTracked={(id) => trackPresetAuditioned(id, { trackKind })}
|
||||||
/>
|
/>
|
||||||
</div>
|
</div>
|
||||||
<div className="mt-2 flex shrink-0 items-center justify-between border-t border-white/10 pt-2 text-[10px] text-white/55">
|
<div className="mt-2 flex shrink-0 items-center justify-between border-t border-white/10 pt-2 text-[10px] text-white/55">
|
||||||
|
|||||||
@@ -237,6 +237,13 @@ export function AudioFxGroup({
|
|||||||
// The rack's In/Out lines. Resolved from the live document because a group's
|
// 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
|
// membership lives on the members, so neither end of the routing can be read
|
||||||
// off the selected element alone.
|
// 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 signalPath = useMemo(() => {
|
||||||
const doc = element.element?.ownerDocument;
|
const doc = element.element?.ownerDocument;
|
||||||
return audioFxSignalPath(
|
return audioFxSignalPath(
|
||||||
@@ -244,7 +251,8 @@ export function AudioFxGroup({
|
|||||||
element.id ?? undefined,
|
element.id ?? undefined,
|
||||||
doc ? resolveAudioGroups(doc) : [],
|
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
|
* The clip this rack belongs to, so hovering a preset auditions where it
|
||||||
|
|||||||
@@ -273,7 +273,8 @@ export function buildTimelineLogicalRows({
|
|||||||
selectedElementIds,
|
selectedElementIds,
|
||||||
gsapAnimations,
|
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({
|
rows.push({
|
||||||
id: trackId,
|
id: trackId,
|
||||||
kind: "row",
|
kind: "row",
|
||||||
@@ -282,7 +283,7 @@ export function buildTimelineLogicalRows({
|
|||||||
level,
|
level,
|
||||||
parentId,
|
parentId,
|
||||||
elementId: activeId,
|
elementId: activeId,
|
||||||
expandable: lanes.length > 0,
|
expandable: disclosable,
|
||||||
expanded,
|
expanded,
|
||||||
items: clipItems(trackId, elements),
|
items: clipItems(trackId, elements),
|
||||||
});
|
});
|
||||||
@@ -454,3 +455,17 @@ export function resolveTimelineFocusFallback(
|
|||||||
}
|
}
|
||||||
return nextRows[previous.rowIndex] ?? nextRows[previous.rowIndex - 1] ?? null;
|
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;
|
||||||
|
}
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import type { TimelineElement } from "../store/playerStore";
|
import type { TimelineElement } from "../store/playerStore";
|
||||||
|
import { isCanaryEnabled } from "../../telemetry/canary";
|
||||||
import { getTimelineElementIdentity } from "../lib/timelineElementHelpers";
|
import { getTimelineElementIdentity } from "../lib/timelineElementHelpers";
|
||||||
import type { ResizingClipState } from "./timelineClipDragTypes";
|
import type { ResizingClipState } from "./timelineClipDragTypes";
|
||||||
import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
|
import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
|
||||||
@@ -33,7 +34,11 @@ export function timelineNeedsLabelColumn(
|
|||||||
): boolean {
|
): boolean {
|
||||||
return (
|
return (
|
||||||
hasKeyframedTimelineClips(animationsByElement) ||
|
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)))
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -98,6 +98,9 @@ function buildGroupInfo(
|
|||||||
return {
|
return {
|
||||||
id: groupId,
|
id: groupId,
|
||||||
label: membership.labelByGroup.get(groupId) ?? 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,
|
anchorKey: (memberTracks[0] ?? fallbackTrackNum) - 0.5,
|
||||||
memberTracks,
|
memberTracks,
|
||||||
memberElements: memberTracks.flatMap((track) => rawByTrack.get(track) ?? []),
|
memberElements: memberTracks.flatMap((track) => rawByTrack.get(track) ?? []),
|
||||||
|
|||||||
@@ -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);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -249,7 +249,13 @@ function buildChildElements(
|
|||||||
// clips. Fractions strictly between the host's lane and the next integer
|
// clips. Fractions strictly between the host's lane and the next integer
|
||||||
// can never equal a normalized (integer) lane, while still rendering the
|
// can never equal a normalized (integer) lane, while still rendering the
|
||||||
// children as their own ordered rows directly under the host.
|
// 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,
|
authoredTrack: base.authoredTrack,
|
||||||
stackingContextId: base.stackingContextId,
|
stackingContextId: base.stackingContextId,
|
||||||
expandedParentStart: editBasis.start,
|
expandedParentStart: editBasis.start,
|
||||||
|
|||||||
Reference in New Issue
Block a user