fix(studio): reconnect property-panel audio controls - #3453
fix(studio): reconnect property-panel audio controls#3453vanceingalls wants to merge 23 commits into
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Review
Reviewed the diff (gh pr diff 3453) plus the head branch (wa-26j-studio-property-panel) directly to verify a couple of cross-file claims. Overall this is a clean, well-commented piece of the stack — the audio-vs-visual distinction in the property panel is handled thoughtfully and the reveal-request plumbing is careful about remount/nonce hazards. Two things below are worth fixing before merge; the rest are nits.
1. Duplicated, narrower audio-tag check risks reintroducing the bug this PR fixes
PropertyPanel.tsx and PropertyPanelFlat.tsx both add:
const selectedTag = element?.tagName?.toLowerCase();
const audioSelection = selectedTag === "audio" || selectedTag === HF_AUDIO_GROUP_TAG;But this same PR also wires up (and reuses, in StudioRightPanel.tsx / PropertyPanelEmptyState.tsx) the canonical isAudioDomElement() from utils/timelineInspector.ts, whose own docstring says: "Same tag set and same source-extension fallback, so the two cannot drift into disagreeing about what counts as audio." That canonical predicate recognizes audio, music, sfx, sound, narration tags plus a source-extension fallback (.mp3/.wav/.m4a/...) — verified directly on the head branch (utils/timelineInspector.ts lines 4-25).
The new audioSelection in PropertyPanel.tsx/PropertyPanelFlat.tsx only checks literal "audio" and the group tag, so it disagrees with the canonical predicate for music/sfx/sound/narration-tagged clips or any element classified as audio via the src-extension fallback. Concretely, for such a clip:
PropertyPanel.tsxwould still render the standalone "Hide element" control (sinceaudioSelectionis false) — reintroducing the exact "two controls that silence a track" problem the surrounding comments describe as the reason this change exists.PropertyPanelFlat.tsxwould leaveshowMotionEffectstrue, showing a tween editor (Motion) for an element with no transform/opacity/box — the same bug the new "Motion is for things that move" tests target, except those tests only covertagName: "audio"andhf-audio-group, not the other audio-family tags.
Suggest reusing isAudioDomElement(element.element) in both places (both already have element.element in scope) instead of hand-rolling the tag check, so there's one source of truth for "is this audio."
2. Dead file left behind: useAudioSoloBridge.ts
App.tsx drops the only call site of useAudioSoloBridge (and the hook file also exports useSoloBannerText). I checked the PR head branch directly — useAudioSoloBridge.ts still exists at packages/studio/src/hooks/useAudioSoloBridge.ts, and there are now zero remaining references to either export anywhere in the tree (confirmed via direct content fetch of App.tsx and PlayerControls.tsx on wa-26j-studio-property-panel, not just a search index). Since the PR comments elsewhere already say "now that mute and solo are gone," this looks like the file should be deleted in this PR rather than left as an orphaned module.
Minor / nits
packages/studio/package.json:"dev": "vite"→"dev": "bun --bun ./node_modules/.bin/vite --host 127.0.0.1". Unrelated to property-panel scope — worth confirming this is an intentional, team-wide dev-workflow change (requiresbuninstalled) rather than a personal-machine tweak that slipped into the commit.PropertyPanelFlat.tsx'saudioGroupLabelis recomputed via an unmemoized IIFE that callsresolveAudioGroups(doc)(a DOM scan) on every render. The near-identical computation inpropertyPanelAudioFxGroup.tsx(signalPath) is wrapped inuseMemokeyed on[element, storeElements]for exactly the reason documented there (membership lives on members, not the selected element). Not urgent sincePropertyPanelFlatdoesn't sit in the per-frame playhead render path today, but worth the same treatment for consistency.
Verified as fine (called out in case others wondered)
TimelineFxPopover.tsx's newauditionPresetChain(skipstrackKind) vs. the trackedapplyPresetToChain: checkeduseApplyAudioFxPreset.ts—trackKindis only used for telemetry there, both paths call the sameapplyAudioFxPresettransform, so audition preview and the applied result stay in sync. No divergence bug.FxSection/FxPresetMenualready definerevealTarget/revealNonce/signalPath/onAuditionTrackedon the head branch, so this PR's new usages aren't dangling references.
Nice touches
- Extracting
propertyPanelFlatClosedGroup.tsx,useAudioFxRevealSection.ts, anduseRemoveBackground.tsto respect the 600-line file cap while keeping each unit independently testable. - The reveal-request nonce handling correctly guards against remount timing and against reopening a section on the wrong element/session — good comments explaining why a
!==check alone wouldn't work. - Good regression tests for the carve-by-name-hint bug and for the new audio-aware Motion/Timing labeling.
— Miga
eef80e2 to
bdd9945
Compare
1e187cd to
37f8b3e
Compare
|
Addressed the property-panel review feedback:
Studio typechecks and builds successfully; the focused Studio set passes (178 tests). |
bdd9945 to
0c28c9f
Compare
37f8b3e to
82f2182
Compare
|
Follow-up CI fixes pushed: PropertyPanelFlat.tsx is now within the 600-line limit, and the solo-bridge deletion moved into this PR now carries its path-specific allowlist entry here. Both failing CI commands pass locally against the cumulative diff. |
bf27917 to
901824e
Compare
bb5442f to
e3b9db4
Compare
901824e to
17c1c88
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed the latest push (3 commits) against the two issues flagged previously — both are resolved.
Issue 1 (narrow audio-tag check): PropertyPanel.tsx and PropertyPanelFlat.tsx now both import isAudioDomElement from ../../utils/timelineInspector and use it to compute audioSelection, replacing the ad hoc tagName === "audio" || HF_AUDIO_GROUP_TAG check. This is consistent with the canonical predicate already wired up in StudioRightPanel.tsx / PropertyPanelEmptyState.tsx, so music/sfx/sound/narration tags and the source-extension fallback are now correctly recognized in these two panels as well. (Note: propertyPanelAudioFxGroup.tsx still checks tagName === HF_AUDIO_GROUP_TAG directly, but that's for a different purpose — deciding whether to expand audition spans to group members — not a stand-in for "is this audio," so no issue there.)
Issue 2 (dead useAudioSoloBridge.ts): File is now fully deleted (deleted file mode 100644), and it's been added to scripts/check-no-main-deletions.mjs's ALLOWED_DELETIONS map with a note explaining the removal ("removes the obsolete solo bridge after its last consumer leaves"). No dead code left behind.
Both previously flagged issues look properly addressed. Good to go from a correctness standpoint — pinging for a stamp separately.
miguel-heygen
left a comment
There was a problem hiding this comment.
Changed-head stamp at 17c1c882.
Audited: the two changed correctness seams, dead-module removal/allowlist, and current GitHub state.
Trusting: Miga's full property-panel review and broader line-cap extraction/test pass.
Both holds are closed. PropertyPanel.tsx and PropertyPanelFlat.tsx now share the canonical isAudioDomElement() predicate, so music, sfx, sound, narration, and audio-extension fallbacks cannot diverge from the rest of Studio. The zero-caller solo bridge is deleted and named explicitly in the exact deletion allowlist. The audio-group label scan was also memoized on the element/store replacement witnesses.
All reported checks for this stacked head are terminal with no failures.
Verdict: APPROVE
Reasoning: Audio classification is back on one source of truth, the obsolete solo bridge is removed with guarded deletion, and the current head is clean.
— Magi
e3b9db4 to
dd9342e
Compare
17c1c88 to
ca1fae8
Compare
The base branch was changed.
Part 10 of 12 replacing #3439. Base:
wa-26i-studio-fx-rack.Why
The property panel is the consuming surface for reveal requests, group visibility recovery, shared audition, and the final audio-section layout.
What
Review note
The
packages/studiodev-script change is intentional: it invokes Vite by its real binary path underbun --bun, keeping the development server on Bun instead of allowing the launcher shebang to select Node. It does not otherwise change the server command.Verification
fallow audit --base main --fail-on-issuesThe final stack tip preserves the verified #3439 replacement and includes the review fixes landed across the stack.
Stack: #3452 ← #3453 → #3454