feat(studio): instrument the audio FX rack, including work an agent did - #3229
Conversation
terencecho
left a comment
There was a problem hiding this comment.
Verdict: LGTM_WITH_NITS
Empirical claim verification (@ head 9f028bf)
- One-module telemetry (
audioFxTelemetry.ts): verified —packages/studio/src/components/editor/audioFxTelemetry.ts:1-234centralises theaudio_fx_*name-space and every callsite (FxSection, FxNodeRow, FxPresetMenu, AudioFxGroup) imports from it. - "Nothing user-authored leaves the browser": verified for the provenance path —
chainShapeataudioFxTelemetry.ts:71-92emits only counts / booleans, andaudioFxTelemetry.test.ts:73-76asserts the serialised payload contains neither"n1"nor"3000". Note the rule as stated in the PR body is scoped to provenance;trackParamCommitted(audioFxTelemetry.ts:145-152) does ship numeric knob values — this matches the "a key without a value would just tell you somebody touched frequency" argument and is consistent with the module's own docstring. - Agent-runtime plumbing: verified end-to-end. CLI publishes at
packages/cli/src/server/telemetryIdentity.ts:160-179(gated ontelemetryShouldTrack()); Studio reads viapackages/studio/src/telemetry/agentRuntime.ts:29-42(memoised, empty-string coerced to null); attaches on every event viastudioTelemetry.ts:46and on the system meta viasystem.ts:54. - Shelf audition dedup:
propertyPanelFxPresetMenu.tsx:75-82—useRef(new Set)resets on unmount, one report per preset per shelf-open.
OLD-assumption counterfactuals
resolveAgentRuntime empty string → nullfails under OLD "returnrawverbatim" — would emit""and"none"as two rows.memoization testfails under OLD non-cached lookup — a mid-sessionwindow.__HF_CLI_AGENT_RUNTIMEoverwrite would split a session.escapes </script>fails under OLD naïve string concat inbuildCliIdentityScript.chainShape counts distinct presetsfails under OLD "count preset nodes" — three telephone/voice-clean nodes across two presets would read as 3.authored_outside=false when panelEdits>0fails under OLD "flag every observation" — every panel edit would double-log as agent work.stays silent when telemetry off, even under an agentfails under OLD unconditional publish — leaves a marker in the page of an opted-out user.
Cross-PR coupling
HfAudioFxNodenot extended: no changes topackages/core/src/audioFx.ts. Attribution reads existingfromPreset/fromCarve/fromEq/fromLevellerflags — the parse/serialize allow-list concern does not apply.- Attempt/result pairing: N/A — every event is a committed action, not a two-phase funnel.
preset_auditioned → preset_appliedis a sequential hover-then-decide analytic, not attempt/result. - Retryable-persist black-out: N/A — every emit is BEFORE the persist / mutate call (
propertyPanelFxSection.tsx:322-329, 389-393, 471-473, 549, 588, 804-805;AudioFxGroupleveller at:685, :795). No throw-swallowed retries. - Write-key:
phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpxinstudioTelemetry.ts:6— pre-existing, unchanged.
Attribution reliability
Tier-1 spoofable (a user can export CLAUDECODE=1) but low-consequence — mislabels a single self-driven session. Tier-2 (authored_outside = panelEdits === 0) also fires on external edits from any source (another tab, collab, git-pull HMR) — treat "authored_outside" as "not-this-panel", not literally "an agent".
Nits
AudioFxGroupchain-fingerprint atpropertyPanelAudioFxGroup.tsx:224-228omitsfromEq/fromLeveller/enabledfrom the shape hash — an external bypass-toggle or leveller swap could go unreported. MatchnodeOrigin's discriminants for symmetry.- Same block:
useEffecthas no dep array — runs on every render. Guarded, but wasteful.
CI
Preflight (lint + format) in preview-regression: FAILURE. Given wa-20* stack tip and recurring oxfmt --check drift (parent memory), highly likely pre-existing stack drift, not this PR. Regression / player-perf / WIP all green. Would call LGTM_PENDING_CI if the Preflight log confirms non-3229 files; on the assumption it's the standard stack condition, LGTM stands.
— Review by tai (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Review: feat(studio): instrument the audio FX rack, including work an agent did — #3229
Verdict: LGTM
Comprehensive telemetry tip — 16 event types under audio_fx_* covering the full rack lifecycle: discover (audition), adopt (apply/add), configure (param/profile/amount), organize (move/bypass/automate), remove, and attribute (chain_observed + agent_runtime).
Privacy is clean. Module header states "Nothing user-authored leaves the browser." Implementation follows through: FxEventProperties is typed as scalar-only (no objects/arrays), chainShape() reduces to origin counts and preset counts (never node contents), $ip: null on every batch. The privacy test explicitly verifies param values and node IDs don't appear in serialized shapes.
Prior-slice integration is correct. presetAmount tracked on wet/dry commit, profiles on one-knob commit with surface: "knob" | "details" distinction (directly answers whether the abstraction gets used), copy layer used for display only (no copy text leaks into events), family classification from tints, node origin flags check all four provenance markers.
chain_observed attribution is clever. authored_outside: panelEdits === 0 discriminates agent-written chains (HTML edits bypass the panel) from panel-written chains. The panelEdits counter resets after each observation, so interleaved agent + human edits are correctly discriminated.
Two minor findings (non-blocking):
-
trackParamAutomatedis the only event function missingFxTrackContext— it won't emittrack_kind, making it a gap in any dashboard that groups rack events by material type. Every other event function follows the pattern. -
nodeOriginwithfromEqis implemented but not tested (four other branches are). One-line test case would close it.
agentRuntime memoization, trackPresetAuditioned dedup via useRef(new Set()), chainShape comparison short-circuit — all correct for performance. The XSS test for encodeInlineScriptValue with </script> payload is a nice touch.
Review by Miga
🤖 Generated with Claude Code
8f0736d to
a1c6078
Compare
7443ac2 to
86d8d5d
Compare
a1c6078 to
631f6d9
Compare
86d8d5d to
eca1927
Compare
199aed7 to
d426aa6
Compare
eca1927 to
f33ed99
Compare
d426aa6 to
2aa26d0
Compare
f33ed99 to
9d69919
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/studio/src/components/editor/propertyPanelAudioFxGroup.tsx # packages/studio/src/components/editor/propertyPanelFxNodeRow.tsx # packages/studio/src/components/editor/propertyPanelFxPresetMenu.tsx # packages/studio/src/components/editor/propertyPanelFxSection.tsx # packages/studio/src/player/components/TimelineTrackRow.tsx # plans/audio-automation-lanes/SPEC.md # plans/audio-fx-presets.md # plans/audio-fx-ux/README.md # skills-manifest.json # skills/hyperframes-audio/SKILL.md
Fallow flagged 9 complexity findings and 2 file-size violations after the telemetry stack landed. Extracts FxPresetRun, FxAddMenu, FxRackChain, FxNodeOpenBody, FxNodeParams, and useFxAudition/useFxCarve/useFxLevelling/ useFxChainObserved out of propertyPanelFxSection.tsx and propertyPanelAudioFxGroup.tsx, splits propertyPanelFxNodeRow.tsx's open-face rendering into its own component, and dedupes a clone in studioTelemetry.ts. Pure structural move — no behavior change; full test suite still green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Nothing in the audio FX feature reported anything, so every question about it — which presets get used, whether the named jobs beat a bare
peaking, whether anybody touches the one-knob profiles — was unanswerable.Dashboard: Audio FX rack — usage (1986431, project 356858). 12 HogQL tiles. Every query verified to execute; the reach tile already returns its real denominator (~25–30k studio installs/week). The
audio_fxtiles stay empty until a build carrying these events ships.The events
studio:audio_fx_*through the existingtrackStudioEventtransport, defined in one module (audioFxTelemetry.ts) rather than scattered across six components, because the shapes have to agree for a dashboard to join them.Presets applied / removed / amount / automated / auditioned; nodes added (with
via: job|effect|eq), removed, bypassed, moved; parameters committed (withsurface: knob|details); profile knobs; carve changes; the leveller; the Tone EQ.track_kindrides on all of them, so every answer can be read per material.Two rules the module states and enforces:
Agent attribution
The harder half, in two tiers.
1. The CLI already fingerprints twelve agent runtimes from its own environment, and Studio cannot see any of it. So
buildCliIdentityScriptnow publishes the resolved category aswindow.__HF_CLI_AGENT_RUNTIME, and both studio session-property builders attach it to every event — not just audio ones, since "person or agent" is worth asking of any feature. Gated on telemetry being on, unlike the canary decisions: those exist so Studio does not enrol, this only labels an event, and with telemetry off there is no event to label.2. An agent asked to fix a mix does not drive the panel — it edits the composition HTML or runs
scripts/carve.mjs, and none of the events above will ever fire. So the panel watches the chain's own shape and reportsaudio_fx_chain_observedwhen it changes, carrying how many edits this session had made. A chain that moved while that counter stood still moved because something outside the studio moved it. The write path is wrapped once rather than counted at ten callsites — the count is only meaningful if it is exhaustive.agent_runtimeis encoded as"none"rather than omitted, on both transports: most sessions are people, that is a finding, and this project's telemetry has already produced one wrong conclusion from comparing a populated sentinel against an absence.Verified
claude_codein an agent-driven shell.3,739 studio tests, 311 CLI telemetry/server tests, lint and tsc clean.
Not verified, and why
No live PostHog event from this checkout.
browserTelemetryAllowed()is false under Vite dev, and the CLI'sisDevMode()is true whenever it runs from.tssource. Those guards exist to stop developers polluting production telemetry, and I did not defeat them to produce a demo event. First real data arrives when a released build runs this code.Left out deliberately
scripts/carve.mjsis not instrumented. Its output is already identifiable by thefromCarvetag on the nodes it writes, so tier 2 covers it without giving a script context its own policy, id and transport to get wrong.render_completewould be a good cross-check — separate change.🤖 Generated with Claude Code