fix(studio): audition from the playhead while paused, and let an author out of a menu - #3190
Conversation
373cdbb to
e021065
Compare
83f09a3 to
7eb2dcb
Compare
terencecho
left a comment
There was a problem hiding this comment.
Verdict: LGTM_PENDING_CI — the two features and the transport-request plumbing are correct and well-tested; the only red CI is unrelated .md drift under plans/, skills/hyperframes-audio/, and README.md (9 files), which cascades into preview-regression via the skipped Preflight.
Verified claims (@ 7eb2dcb)
- "Hovering writes to the running graph, silent while paused":
propertyPanelFxSection.tsx:234-244—audition()writes viaonChainPreview(preview channel). - "Hovering now plays from the playhead":
propertyPanelAudioFxGroup.tsx:598-610— capturesstore.currentTimeon(true), requestsrequestPlayback(true); on(false)requestsrequestPlayback(false, returnTo).useTimelinePlayer.ts:386-396subscribes and callspause()thenseek(returnTo). - "Transport the author started is left alone, in both directions":
propertyPanelAudioFxGroup.tsx:600-609— guardstore.isPlaying || auditionReturn.current !== nullon enter; on leave, no-ops ifauditionReturn.current === null. Correct in both directions. - "Order matters":
propertyPanelFxSection.tsx:239-244—onChainPreview(...)beforeonAuditionTransport?.(true);onAuditionTransport?.(false)before the revertonChainPreview(auditionBase.current). Justified in comment. - "Buttons stay and close what they opened, one closes the other":
propertyPanelFxSection.tsx:843-885— buttons rendered unconditionally with togglingonClickandaria-expanded. Opening add closes preset (setPicking(false)) and vice versa. - "Escape closes whichever is open":
propertyPanelFxSection.tsx:533-543—closeMenusgated onevent.key !== "Escape" || (!adding && !picking);stopPropagation()only fires when there's a menu to close. - Nonce mechanism:
playerStore.ts:355-359—nonce: (prev.nonce ?? 0) + 1per request; consumed vianonce !== prev.playbackRequest?.nonceatuseTimelinePlayer.ts:390.
OLD-assumption counterfactuals (each new test)
plays from the playhead, then puts it back— OLD:audition()never calledonAuditionTransport, soplaybackRequeststaysnull.expect(...playing).toBe(true)fails.leaves a transport the author started alone— OLD (no isPlaying guard):noncebumps on enter/leave despite already-playing.toBe(before)fails.starts playback for paused author/stops it on the way out/stops on apply/stops on unmount— each fails without the correspondingonAuditionTransport?.(false)inapplyPreset/addEffect/addJob/addEqand thetransportRef.current?.(false)in the unmount effect (propertyPanelFxSection.tsx:274).- Menu tests (
closes with same button,add menu same way,one closes the other,closes on Escape,puts the chain back on close-while-auditioning) — all fail under the pre-diff{adding || picking ? null : (…buttons)}conditional and the missingonKeyDown={closeMenus}. leaves Escape alone when no menu is open— guards against a naive fix that wouldstopPropagation()unconditionally.wave only when hovering can be heard— OLD: nohf-fx-preset-waveDOM at all.
Cross-PR coupling
auditionBasesemantics unchanged; newtransportRefmirrors the existingpreviewRefpattern (propertyPanelFxSection.tsx:270-282) — same defense against inline-callback teardown storms.- Every exit path from an audition (
applyPreset,addJob,addEffect,addEq, unmount teardown, menu-toggle-close, Escape) now callsonAuditionTransport?.(false)— full enumeration verified. - No new
HfAudioFxNodefield.audioFx.tsuntouched; parse/serialize allow-list not affected. - Leveller
decodedcache and per-shelfauditionEntercounter are on separate paths inpropertyPanelAudioFxGroup.tsx; the transport hook is orthogonal.
Nits (non-blocking)
- Accessibility: after Escape closes a menu, focus lands nowhere — the section
divis not focusable. Consider returning focus to the just-closed trigger ("Presets"/"Add effect") for keyboard-only authors. aria-expandedis set on the buttons;aria-haspopup="menu"would sharpen the screen-reader read.auditionTransportis a fresh arrow every AudioFxGroup render →FxSection'sauditioncallback also re-creates every render. The group already re-renders on playhead ticks so this is not a new cost, but a stable ref (useCallbackwith empty deps + reading store on invocation) would be cheap defense.
CI
- Preflight fails on
oxfmt --checkfor 9 unrelated.mdfiles (README +plans/…+skills/hyperframes-audio/…) — stack-wide drift, none touched by this PR. preview-regressionFAILURE is a cascade:PREVIEW_PARITY_RESULT=skippedbecause Preflight failed. Not a real signal from this PR.- Real signals:
player-perf,regression, WIP all green.Preview paritystep itself was SKIPPED, not run.
— Review by tai (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Review: fix(studio): audition from the playhead while paused, and let an author out of a menu — #3190
Verdict: LGTM
Playhead-while-paused fix is correct. Reads store.currentTime at audition start, stores in auditionReturn.current, passes back as returnTo when stopping. The consumer processes via pause() then seek(returnTo). The nonce-based request detection handles consecutive same-direction requests and rapid hover/leave sequences. The "already playing" guard correctly leaves the author's transport alone (tested).
Audition ordering is deliberate: start writes the chain FIRST then starts transport (first audible frame is the preset); stop halts transport FIRST then reverts chain (last audible frame is the preset, not the snap-back).
Menu dismiss — three gestures: toggle button click (Presets→Close), Escape key (with stopPropagation to prevent panel's own Escape from deselecting the clip), mutual exclusivity (opening one menu closes the other). No click-outside, consistent with the toggle-button pattern.
No regressions to hover-audition from #3192. The FxPresetMenu enter/leave wiring is unchanged. The unmount cleanup is strengthened to also stop transport.
Tests cover: start-from-paused + return, already-playing no-op, close button (both menus), Escape (both menus), mutual exclusivity, cleanup on close with active audition. Wave indicator gets conditional rendering test and prefers-reduced-motion: reduce support.
Nit: playbackRequest: null is missing from createTimelineResetState() — the other request-type state (requestedSeekTime, clipRevealRequest) is reset there. Low practical risk but inconsistent.
Review by Miga
🤖 Generated with Claude Code
7eb2dcb to
3972215
Compare
e021065 to
6a61908
Compare
3972215 to
0a6bf88
Compare
6a61908 to
98fb1e0
Compare
0a6bf88 to
788bf68
Compare
98fb1e0 to
f42d061
Compare
788bf68 to
890c0fd
Compare
f42d061 to
beb004b
Compare
# Conflicts: # packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx # packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx # packages/studio/src/components/editor/propertyPanelFxSection.tsx # skills-manifest.json # skills/hyperframes-audio/SKILL.md # skills/hyperframes-audio/references/attributes.md # skills/hyperframes-audio/scripts/carve.mjs
# Conflicts: # packages/core/src/audio/audioFxGraph.ts # packages/core/src/audioFx.ts # packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx # packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx # packages/studio/src/components/editor/propertyPanelFxPresetMenu.tsx # packages/studio/src/components/editor/propertyPanelFxSection.test.tsx # packages/studio/src/components/editor/propertyPanelFxSection.tsx # plans/audio-automation-lanes/SPEC.md # plans/audio-fx-presets.md # plans/audio-fx-ux/README.md # plans/automation-lanes-shared-rows.md # plans/automation-time-selection-design.md
Auditioning while paused. Hovering a preset writes it to the running graph, which is silent while the transport is paused — so the whole affordance only worked mid-playback. Hovering now plays from the playhead and leaving stops and returns it exactly where it was. A transport the author started is left alone in both directions.
Getting out of a menu. Opening the preset shelf or add menu hid both buttons, and the only thing that set them back was picking something. The buttons now stay and close what they opened; Escape closes whichever is open.
Also two audition bugs found by review rather than by tests: a teardown keyed on an inline callback reverted the audition ~30 times a second during playback, and sliding from one shelf entry to its neighbour left a measurement in flight.
🤖 Generated with Claude Code