From a4b3eb0b9e3fe69f89bccbed5f60726d88a200a9 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 20 Aug 2026 15:30:27 -0700 Subject: [PATCH] fix(studio): make the reveal actually fire, and give a hidden group a way back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings 8, 12, 13. **12 — the reveal was dead in BOTH halves,** which is why nobody caught it: each half hid the other. - *Not already selected.* `openClipFxRack` raises the request and then selects the clip asynchronously, so the selection landed AFTER and `setSelectedElementId` cleared `revealedAudioFxTarget` — the very request that caused the selection. The clear now spares a request whose `elementKey` is the element being selected; any other selection still drops it, because a request aimed elsewhere is stale. - *Already selected.* `FxSection` consumed with `useState(revealTarget ?? null)`, so `consumedReveal` initialised EQUAL to the request and the `!==` never fired — and a second click on the same lane was byte-identical, so also inert. It now consumes by nonce, initialised null, which is what `PropertyPanelFlat` already does for the same hazard and for the same reason. `propertyPanelAudioFxGroup` forwarded `automationTarget` and dropped the nonce; it forwards both now. **13 — an unescapable node id took the panel down instead of failing quietly.** `revealRowSelector` interpolated chain strings straight into `querySelector`. `parseAudioFxNode` accepts any non-empty string as an id and `parseAutomationTarget` only splits on `.`, so a hand- or LLM-authored chain can carry `a"]` — and the throw escaped a render-phase effect. Now escaped with the repo's own `escapeCssString` (which `findElementForTimelineElement` uses for exactly this) and wrapped, so a malformed selector returns false, which is the documented "row not mounted yet" contract. **8 — a hidden group had no way back.** The panel's visibility toggle is withheld for any audio selection, and the group header carries no visibility control now that mute and solo are gone — so a `data-hidden` bus was silent in preview (the bus's mute gain) and absent from the render (every member dropped), recoverable only by hand-editing the HTML. It is now offered while hidden, the same door-from-the-inside `TimelineTrackPlainHeader` keeps for an audio TRACK after the identical trap was diagnosed there on this branch. That fix needed a second one to work: `selectedElementHidden` derives from `timelineElements`, and after 0e86e64d2 a bus is not one — no timeline row, no `hidden` flag. `isSelectionHidden` falls back to the element's own attribute. `clearRevealedAudioFxTarget` is now called on unmount, closing a "left open" item in the PR body. The whole reveal consumption moved to `useAudioFxRevealSection.ts` because PropertyPanelFlat crossed 600 — the hook holds all three hazards (currency, nonce, retirement) instead of restating them at the call site. studio: 103 editor files, 1276 tests. fallow clean. --- .../components/editor/PropertyPanelFlat.tsx | 64 +++++++++---------- .../components/editor/audioFxRevealTarget.ts | 20 ++++-- .../editor/propertyPanelAudioFxGroup.tsx | 12 ++++ .../editor/propertyPanelFlatClosedGroup.tsx | 15 +++++ .../editor/propertyPanelFxSection.tsx | 7 +- .../editor/propertyPanelFxSectionTypes.ts | 9 +++ .../editor/useAudioFxRevealSection.ts | 64 +++++++++++++++++++ .../studio/src/player/store/playerStore.ts | 18 +++--- .../src/player/store/playerStoreSelection.ts | 21 ++++++ 9 files changed, 180 insertions(+), 50 deletions(-) create mode 100644 packages/studio/src/components/editor/useAudioFxRevealSection.ts create mode 100644 packages/studio/src/player/store/playerStoreSelection.ts diff --git a/packages/studio/src/components/editor/PropertyPanelFlat.tsx b/packages/studio/src/components/editor/PropertyPanelFlat.tsx index b48d7d189..53aceede9 100644 --- a/packages/studio/src/components/editor/PropertyPanelFlat.tsx +++ b/packages/studio/src/components/editor/PropertyPanelFlat.tsx @@ -10,7 +10,7 @@ import { audioFxSummary } from "./audioFxSummary"; import { HF_AUDIO_GROUP_TAG, resolveAudioGroups } from "@hyperframes/core/audio-groups"; import { PropertyPanelFlatHeader } from "./PropertyPanelFlatHeader"; import { PropertyPanelFlatFooter } from "./PropertyPanelFlatFooter"; -import { closedGroupHeader } from "./propertyPanelFlatClosedGroup"; +import { closedGroupHeader, isSelectionHidden } from "./propertyPanelFlatClosedGroup"; import { FlatGroupHeader } from "./propertyPanelFlatPrimitives"; import { FlatTextSection } from "./propertyPanelFlatTextSection"; import { FlatStyleSection } from "./propertyPanelFlatStyleSections"; @@ -19,16 +19,14 @@ import { FlatMotionSection } from "./propertyPanelFlatMotionSection"; import { isCanaryEnabled } from "../../telemetry/canary"; import { AudioFxGroup } from "./propertyPanelAudioFxGroup.js"; import { useVolumeAutomation } from "./useVolumeAutomation"; +import { useAudioFxRevealSection } from "./useAudioFxRevealSection"; import { FlatMediaSection } from "./propertyPanelFlatMediaSection"; import { deriveElementTiming } from "./propertyPanelFlatTimingDerivation"; import { createGsapLivePreview } from "./gsapLivePreview"; import { formatTextFieldPreview } from "./propertyPanelSections"; import { useColorGradingController } from "./useColorGradingController"; import { usePlayerStore } from "../../player"; -import { - isFocusedEaseRequestCurrent, - isRevealedAudioFxRequestCurrent, -} from "../../player/store/keyframeSlice"; +import { isFocusedEaseRequestCurrent } from "../../player/store/keyframeSlice"; import { FlatColorGradingAccessory, FlatColorGradingSection, @@ -166,15 +164,13 @@ export function PropertyPanelFlat({ // When the inline timeline ease button focuses a segment on this element, // force the Motion group open so its AnimationCard (which only mounts while // the group is expanded) can consume the focus and reveal the ease editor. - const { focusedEaseSegment, revealedAudioFxTarget, timelineProjectId, timelineSessionEpoch } = - usePlayerStore( - useShallow((state) => ({ - focusedEaseSegment: state.focusedEaseSegment, - revealedAudioFxTarget: state.revealedAudioFxTarget, - timelineProjectId: state.timelineProjectId, - timelineSessionEpoch: state.timelineSessionEpoch, - })), - ); + const { focusedEaseSegment, timelineProjectId, timelineSessionEpoch } = usePlayerStore( + useShallow((state) => ({ + focusedEaseSegment: state.focusedEaseSegment, + timelineProjectId: state.timelineProjectId, + timelineSessionEpoch: state.timelineSessionEpoch, + })), + ); // Identity of the element THIS panel actually renders (not the store's // selectedElementId, which flips synchronously on selection while the panel // still renders the previous element during async DOM-selection resolution): @@ -209,23 +205,14 @@ export function PropertyPanelFlat({ * module the request names — is not mounted while it is closed, so the click * selected the clip and then appeared to do nothing. */ - const revealNonceForThisPanel = - revealedAudioFxTarget !== null && - revealedAudioFxTarget.elementKey === element?.id && - isRevealedAudioFxRequestCurrent(revealedAudioFxTarget, { - timelineProjectId, - timelineSessionEpoch, - }) && - sections.audioFx - ? revealedAudioFxTarget.nonce - : null; - // Keyed on the request's NONCE, not the request object: clicking a lane - // selects the clip first, which REMOUNTS this panel — so a `!==` against the - // previous value would initialise to the already-set request and never fire. - // The nonce also makes a second click on the same lane a new request. - const [consumedRevealNonce, setConsumedRevealNonce] = useState(null); - if (revealNonceForThisPanel !== null && revealNonceForThisPanel !== consumedRevealNonce) { - setConsumedRevealNonce(revealNonceForThisPanel); + const hiddenNow = isSelectionHidden(selectedElementHidden, element); + + const reveal = useAudioFxRevealSection({ + elementId: element?.id, + hasAudioFxSection: Boolean(sections.audioFx), + }); + if (reveal.revealNonce !== null) { + reveal.consume(reveal.revealNonce); setOpenGroupId("audio-fx"); } @@ -536,7 +523,7 @@ export function PropertyPanelFlat({ name={element.label} meta={`${sourceLabel} · ${element.tagName}`} elementKind={elementKind} - hidden={selectedElementHidden} + hidden={hiddenNow} // Audio gets no hide control here. On an audio track "hidden" and // "muted" are not similar operations, they are the SAME operation // with two names (groups doc §2.1) — which is why the timeline's eye @@ -545,9 +532,18 @@ export function PropertyPanelFlat({ // set out to remove: "Two controls that silence a track, sitting // next to each other, differing only in a distinction the author // cannot see." An `` has no visual to hide at all. + // + // EXCEPT while it is already hidden — the same door-from-the-inside + // the timeline's eye keeps for an audio track + // (`TimelineTrackPlainHeader`). Withholding it unconditionally + // withheld the only way back: a `data-hidden` group is silent in + // preview (the bus's mute gain) and absent from the render (every + // member dropped), and the group header carries no visibility + // control of its own now that mute and solo are gone. Only + // hand-editing the HTML brought the audio back. onToggleHidden={ - selectedElementId && onToggleElementHidden && !audioSelection - ? () => void onToggleElementHidden(selectedElementId, !selectedElementHidden) + selectedElementId && onToggleElementHidden && (!audioSelection || hiddenNow) + ? () => void onToggleElementHidden(selectedElementId, !hiddenNow) : undefined } copied={clipboardCopied} diff --git a/packages/studio/src/components/editor/audioFxRevealTarget.ts b/packages/studio/src/components/editor/audioFxRevealTarget.ts index b58d096ca..764d7bf56 100644 --- a/packages/studio/src/components/editor/audioFxRevealTarget.ts +++ b/packages/studio/src/components/editor/audioFxRevealTarget.ts @@ -11,6 +11,7 @@ */ import { parseAutomationTarget } from "@hyperframes/core/audio-automation"; +import { escapeCssString } from "./domEditingDom"; import type { HfAudioFxChain } from "@hyperframes/core/audio-fx"; export type AudioFxRevealTarget = @@ -67,13 +68,13 @@ function revealRowSelector(where: AudioFxRevealTarget | null): string | null { if (!where) return null; switch (where.kind) { case "node": - return where.nodeId ? `[data-fx-node-id="${where.nodeId}"]` : null; + return where.nodeId ? `[data-fx-node-id="${escapeCssString(where.nodeId)}"]` : null; case "eq": - return `[data-fx-eq="${where.eqId}"]`; + return `[data-fx-eq="${escapeCssString(where.eqId)}"]`; case "carve": return ".hf-fx-carve-module"; case "preset": - return `[data-fx-preset="${where.runKey.replace(/-\d+$/, "")}"]`; + return `[data-fx-preset="${escapeCssString(where.runKey.replace(/-\d+$/, ""))}"]`; default: return null; } @@ -94,7 +95,18 @@ export function scrollRevealedRowIntoView( chain: HfAudioFxChain, ): boolean { const selector = revealRowSelector(audioFxRevealTarget(target, chain)); - const row = selector ? root?.querySelector(selector) : null; + // `querySelector` throws SyntaxError on a malformed selector, and this runs + // inside a render-phase effect — an unescapable id would take the whole + // property panel down instead of leaving the request pending, which is what + // returning false means. `parseAudioFxNode` accepts any non-empty string as + // an id and `parseAutomationTarget` only splits on `.`, so a hand- or + // LLM-authored chain can carry one. + let row: HTMLElement | null = null; + try { + row = selector ? (root?.querySelector(selector) ?? null) : null; + } catch { + return false; + } if (!row) return false; row.scrollIntoView({ block: "nearest", behavior: "smooth" }); return true; diff --git a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx index c36e2c2ed..a89529ca3 100644 --- a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx +++ b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx @@ -296,6 +296,18 @@ export function AudioFxGroup({ ? revealRequest.automationTarget : null } + // Forwarded, not dropped: the section consumes by nonce, so without it a + // request never fires and a second click on the same lane is inert. + revealNonce={ + revealRequest && + revealRequest.elementKey === element.id && + isRevealedAudioFxRequestCurrent(revealRequest, { + timelineProjectId, + timelineSessionEpoch, + }) + ? revealRequest.nonce + : null + } // Locked while the carve is measuring. `analyse` captures the chain and // the automation BEFORE its fetch and decode, then rewrites the whole // attribute from that snapshot — so an effect added, or a knob committed, diff --git a/packages/studio/src/components/editor/propertyPanelFlatClosedGroup.tsx b/packages/studio/src/components/editor/propertyPanelFlatClosedGroup.tsx index f0fdd04e0..92d7b8dc6 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatClosedGroup.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatClosedGroup.tsx @@ -29,3 +29,18 @@ export function closedGroupHeader( ); } + +/** + * Is the selection hidden RIGHT NOW. + * + * `selectedElementHidden` is derived from `timelineElements`, and an + * `` is not one — it is a mixer bus, and the runtime no longer + * stamps timing on it, so it has no timeline row to carry a `hidden` flag. Its + * `data-hidden` lives only on the element, so the attribute is the fallback. + */ +export function isSelectionHidden( + fromTimeline: boolean, + element: { dataAttributes?: Record } | null | undefined, +): boolean { + return fromTimeline || element?.dataAttributes?.["hidden"] != null; +} diff --git a/packages/studio/src/components/editor/propertyPanelFxSection.tsx b/packages/studio/src/components/editor/propertyPanelFxSection.tsx index 56951d148..c41fe3d8d 100644 --- a/packages/studio/src/components/editor/propertyPanelFxSection.tsx +++ b/packages/studio/src/components/editor/propertyPanelFxSection.tsx @@ -107,6 +107,7 @@ export function FxSection({ onAuditionTransport, signalPath, revealTarget, + revealNonce, }: FxSectionProps) { const presetAutomated = automatedPresets ?? new Set(); // Falls back to the persisting write when no preview handler is supplied, which @@ -330,11 +331,11 @@ export function FxSection({ * the request lands on, or the scroll below runs against a row that has not * mounted yet. */ - const [consumedReveal, setConsumedReveal] = useState(revealTarget ?? null); + const [consumedRevealNonce, setConsumedRevealNonce] = useState(null); const pendingRevealRef = useRef(null); const rootRef = useRef(null); - if ((revealTarget ?? null) !== consumedReveal) { - setConsumedReveal(revealTarget ?? null); + if (revealNonce != null && revealNonce !== consumedRevealNonce) { + setConsumedRevealNonce(revealNonce); const where = revealTarget ? audioFxRevealTarget(revealTarget, chain) : null; if (where) { // Each surface has its own open-state; the resolver says which one owns diff --git a/packages/studio/src/components/editor/propertyPanelFxSectionTypes.ts b/packages/studio/src/components/editor/propertyPanelFxSectionTypes.ts index 006a2474d..d33394774 100644 --- a/packages/studio/src/components/editor/propertyPanelFxSectionTypes.ts +++ b/packages/studio/src/components/editor/propertyPanelFxSectionTypes.ts @@ -15,6 +15,15 @@ export interface FxSectionProps { * row, an EQ module, a preset run, or the carve — and scrolls to it. */ revealTarget?: string | null; + /** + * The reveal request's nonce. Consumption keys on THIS, not on + * `revealTarget`: clicking a lane selects the clip first, which remounts this + * section, so a `!==` against the previous VALUE initialises to the + * already-set request and never fires — and a second click on the same lane + * would be byte-identical and inert. Same reason `PropertyPanelFlat` keys its + * own consumption on the nonce. + */ + revealNonce?: number | null; /** What the rack's `In`/`Out` lines name — see `audioFxSignalPath`. Absent * means an ungrouped clip, which is what those lines said before groups. */ signalPath?: AudioFxSignalPath; diff --git a/packages/studio/src/components/editor/useAudioFxRevealSection.ts b/packages/studio/src/components/editor/useAudioFxRevealSection.ts new file mode 100644 index 000000000..2c989c6ad --- /dev/null +++ b/packages/studio/src/components/editor/useAudioFxRevealSection.ts @@ -0,0 +1,64 @@ +/** + * Opening the Audio FX section for a lane's reveal request. + * + * Split out of `PropertyPanelFlat.tsx` to keep it under the studio's 600-line + * cap. All three of the request's hazards live here rather than being restated + * at the call site: it must be current, it is consumed by NONCE, and it is + * retired when the panel goes away. + */ + +import { useEffect, useState } from "react"; +import { usePlayerStore } from "../../player"; +import { isRevealedAudioFxRequestCurrent } from "../../player/store/keyframeSlice"; + +export interface AudioFxRevealSectionInput { + /** The element the panel is showing, or null. */ + elementId: string | null | undefined; + /** False when the panel does not render an Audio FX section at all. */ + hasAudioFxSection: boolean; +} + +/** + * The nonce this panel should act on, or null. + * + * Consumption is keyed on the NONCE, not the request object: clicking a lane + * selects the clip first, which REMOUNTS this panel, so a `!==` against the + * previous value would initialise to the already-set request and never fire. + * The nonce also makes a second click on the same lane a new request. + */ +export function useAudioFxRevealSection(input: AudioFxRevealSectionInput): { + /** Non-null exactly once per request: open the section on this commit. */ + revealNonce: number | null; + consume: (nonce: number) => void; +} { + const revealedAudioFxTarget = usePlayerStore((s) => s.revealedAudioFxTarget); + const timelineProjectId = usePlayerStore((s) => s.timelineProjectId); + const timelineSessionEpoch = usePlayerStore((s) => s.timelineSessionEpoch); + const clearRevealedAudioFxTarget = usePlayerStore((s) => s.clearRevealedAudioFxTarget); + const [consumed, setConsumed] = useState(null); + + // Retire the request once this panel is gone. Consumption is nonce-guarded so + // a stale request was already harmless — but it sat in the store until the + // next click, and a request nobody will ever consume is state every reader + // then has to reason about. + useEffect(() => { + if (consumed === null) return; + return () => clearRevealedAudioFxTarget(consumed); + }, [consumed, clearRevealedAudioFxTarget]); + + const forThisPanel = + revealedAudioFxTarget !== null && + revealedAudioFxTarget.elementKey === input.elementId && + isRevealedAudioFxRequestCurrent(revealedAudioFxTarget, { + timelineProjectId, + timelineSessionEpoch, + }) && + input.hasAudioFxSection + ? revealedAudioFxTarget.nonce + : null; + + return { + revealNonce: forThisPanel !== null && forThisPanel !== consumed ? forThisPanel : null, + consume: setConsumed, + }; +} diff --git a/packages/studio/src/player/store/playerStore.ts b/packages/studio/src/player/store/playerStore.ts index 354621db2..adf60e723 100644 --- a/packages/studio/src/player/store/playerStore.ts +++ b/packages/studio/src/player/store/playerStore.ts @@ -1,5 +1,6 @@ import { create } from "zustand"; import { attachPlayerStoreDevHandle } from "./playerStoreDevHandle"; +import { nextSelectionSet } from "./playerStoreSelection"; import type { MusicBeatAnalysis } from "@hyperframes/core/beats"; import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import type { BeatEditState } from "../../utils/beatEditing"; @@ -530,18 +531,17 @@ export const usePlayerStore = create((set, get) => ({ // echoes that must preserve a group go through setSelectionAnchor instead. setSelectedElementId: (id, options) => set((s) => { - const preserveSet = Boolean(options?.preserveSet && id && s.selectedElementIds.has(id)); - const selectedElementIds = preserveSet - ? new Set(s.selectedElementIds) - : options?.preserveSet - ? new Set() - : id - ? new Set([id]) - : new Set(); + const selectedElementIds = nextSelectionSet(s.selectedElementIds, id, options?.preserveSet); // Selecting a different element drops any active keyframe selection — otherwise // a stale activeKeyframePct from a prior diamond click would force the next drag // to "modify" a keyframe on the new element. A diamond click sets the pct AFTER // calling setSelectedElementId, so this never clobbers a genuine keyframe select. + // A reveal request survives the selection it is FOR. `openClipFxRack` + // raises the request and then selects the clip asynchronously, so the + // selection lands afterwards and used to clear the very request that + // caused it — the panel then read null and the section never opened. + // Any OTHER selection still drops it: a request aimed elsewhere is stale. + const revealSurvives = s.revealedAudioFxTarget?.elementKey === id; return id !== s.selectedElementId ? { selectedElementId: id, @@ -549,7 +549,7 @@ export const usePlayerStore = create((set, get) => ({ activeKeyframePct: null, motionPathArmed: false, focusedEaseSegment: null, - revealedAudioFxTarget: null, + ...(revealSurvives ? {} : { revealedAudioFxTarget: null }), } : { selectedElementId: id, selectedElementIds }; }), diff --git a/packages/studio/src/player/store/playerStoreSelection.ts b/packages/studio/src/player/store/playerStoreSelection.ts new file mode 100644 index 000000000..33817091b --- /dev/null +++ b/packages/studio/src/player/store/playerStoreSelection.ts @@ -0,0 +1,21 @@ +/** + * Selection-set arithmetic for the player store. + * + * Its own module so `playerStore.ts` stays under the studio's 600-line cap. + */ + +/** + * The id set a selection change leaves behind. + * + * `preserveSet` means "keep the multi-selection if this id is already in it" — + * a DOM→store echo re-announcing a member must not collapse the set — and + * anything else is a genuine single selection. + */ +export function nextSelectionSet( + current: ReadonlySet, + id: string | null, + preserveSet: boolean | undefined, +): Set { + if (preserveSet) return id && current.has(id) ? new Set(current) : new Set(); + return id ? new Set([id]) : new Set(); +}