diff --git a/packages/core/package.json b/packages/core/package.json index 06ac920aa8..29f6a2a87d 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -549,7 +549,7 @@ }, "scripts": { "build": "bun run build:hyperframes-runtime && bun run build:position-edits-render && bun run build:audio-fx-runtime && tsc && tsx scripts/rewrite-esm-extensions.ts", - "test": "bun run check:position-edits-render && vitest run", + "test": "bun run check:position-edits-render && bun run build:audio-fx-runtime && vitest run", "test:watch": "vitest", "test:coverage": "vitest run --coverage", "test:runtime-coverage": "vitest run --coverage src/runtime", diff --git a/packages/core/src/audio/audioFxGraph.test.ts b/packages/core/src/audio/audioFxGraph.test.ts index 9c54f12bc9..1a8d235196 100644 --- a/packages/core/src/audio/audioFxGraph.test.ts +++ b/packages/core/src/audio/audioFxGraph.test.ts @@ -438,3 +438,54 @@ describe("automatable parameters", () => { expect(onePole.automation?.frequency).toBeUndefined(); }); }); + +describe("phaser automation targets", () => { + /** + * `in_gain` and `out_gain` trim the signal entering and leaving the effect, + * which the builder drives through inTrim/outTrim while pinning wet and dry + * to 1. The automation map used to aim both lanes at wet/dry — so an envelope + * modulated a constant, the trim it was supposed to move stayed frozen, and + * "fade the phaser out" left the dry leg playing at full level. + * + * Asserted by VALUE rather than by node identity: the trims are internal, and + * the only honest question is whether the param a lane would drive is the one + * the knob sets. + */ + it("drives the trims a lane is named for, not the pinned wet/dry pair", () => { + const handle = buildFxNode(ctx() as unknown as BaseAudioContext, "phaser", { + ...defaultAudioFxParams("phaser"), + in_gain: 0.25, + out_gain: 0.5, + }); + expect(handle.automation?.in_gain?.[0]?.param.value).toBeCloseTo(0.25, 6); + expect(handle.automation?.out_gain?.[0]?.param.value).toBeCloseTo(0.5, 6); + }); +}); + +describe("chain update keeps ids with their effects", () => { + const band = (id: string, frequency: number) => ({ + type: "peaking", + id, + enabled: true, + params: { ...defaultAudioFxParams("peaking"), frequency }, + }); + + /** + * Reordering two effects of the same type leaves the shape string identical, + * so the chain updates in place rather than rebuilding — correct for the + * audio, since the params move with the position. The ids have to move too: + * a lane addresses its effect by id, and an id captured at build time names + * whichever effect used to occupy that slot. The scheduler would then drive + * `fx.n2.frequency` into the band that is now n1 — the exact swap that + * HfAudioFxNode.id documents itself as preventing, and the one the voiceover + * carve's all-peaking chains make easy to hit. + */ + it("moves an id with its slot when same-type effects are reordered", () => { + const chain: HfAudioFxChain = { version: 1, nodes: [band("n1", 200), band("n2", 4000)] }; + const handle = buildFxChain(ctx() as unknown as BaseAudioContext, chain); + + const swapped: HfAudioFxChain = { version: 1, nodes: [band("n2", 4000), band("n1", 200)] }; + expect(handle.update(swapped)).toBe(true); + expect(handle.nodes.map((n) => n.id)).toEqual(["n2", "n1"]); + }); +}); diff --git a/packages/core/src/audio/audioFxGraph.ts b/packages/core/src/audio/audioFxGraph.ts index a366b823e7..09f3c91a7a 100644 --- a/packages/core/src/audio/audioFxGraph.ts +++ b/packages/core/src/audio/audioFxGraph.ts @@ -360,8 +360,13 @@ const allpassPhaser: Builder = (ctx, p) => { // frequency at once — not one knob, one param — so they stay unautomated. automation: { speed: [{ param: lfo.frequency }], - in_gain: [{ param: dry.gain }], - out_gain: [{ param: wet.gain }], + // The trims, not wet/dry. apply() drives inTrim/outTrim from these knobs + // and pins wet and dry to 1 — so a lane aimed at wet/dry modulated a + // constant and left the trim frozen, and the next values-only edit slammed + // it back over the running envelope. The comment above records that this + // wiring was already moved once; the automation map was missed. + in_gain: [{ param: inTrim.gain }], + out_gain: [{ param: outTrim.gain }], }, dispose: () => { try { @@ -510,7 +515,17 @@ export function buildFxChain(ctx: BaseAudioContext, chain: HfAudioFxChain): FxCh if (shapeOf(next) !== shape) return false; const active = next.nodes.filter((node) => node.enabled !== false); active.forEach((node, i) => { - handles[i]?.handle.update(normalizeAudioFxParams(node.type, node.params)); + const held = handles[i]; + if (!held) return; + held.handle.update(normalizeAudioFxParams(node.type, node.params)); + // The id follows the position, because the params just did. Reordering + // two effects of the same type leaves the shape identical, so the graph + // is updated in place — but a lane addresses its effect BY id, and an id + // captured at build time then names whichever effect used to be here. + // The scheduler would drive `fx.n2.frequency` into the band that is now + // n1: exactly what HfAudioFxNode.id documents itself as preventing. + if (node.id === undefined) delete held.id; + else held.id = node.id; }); shape = shapeOf(next); return true; diff --git a/packages/core/src/runtime/media.test.ts b/packages/core/src/runtime/media.test.ts index c6fa207ed8..bf697161b8 100644 --- a/packages/core/src/runtime/media.test.ts +++ b/packages/core/src/runtime/media.test.ts @@ -365,6 +365,37 @@ describe("syncRuntimeMedia", () => { expect(only).toBeCloseTo(0.55, 5); }); + /** + * The render bakes the lane at CLIP-LOCAL time: prepareAudioTrack already + * cut the wav with `-ss mediaStart`, so its t=0 is the clip's start, and + * normaliseEnvelope subtracts trackStart. Preview used to sample at MEDIA + * time — mediaStart included, scaled by playbackRate, wrapped on a loop — so + * the same envelope played somewhere else than it rendered. + */ + it("samples the lane at clip-local time, the way the render bakes it", () => { + const trimmed = (t: number) => { + const clip = createMockClip({ start: 0, end: 10, volume: 0.55, mediaStart: 30 }); + Object.defineProperty(clip.el, "readyState", { value: 4, writable: true }); + clip.el.setAttribute("data-automation", DUCK); + let seen = -1; + syncRuntimeMedia({ + clips: [clip], + timeSeconds: t, + playing: true, + playbackRate: 1, + onElementVolume: (_el, v) => { + seen = v; + }, + }); + return seen; + }; + // `data-media-start="30"` on a clip whose lane holds 0.8 until t=2 then + // ducks to 0.1 by t=3. At media time the playhead is already 30 s past the + // last point, so preview held 0.1 from the first frame and never ducked. + expect(trimmed(1)).toBeCloseTo(0.8, 5); + expect(trimmed(5)).toBeCloseTo(0.1, 5); + }); + it("supersedes keyframes probed from the timeline", () => { // Both present: the lane is the explicit one, and `lint` warns about it. const clip = createMockClip({ start: 0, end: 10, volume: 0.55 }); diff --git a/packages/core/src/runtime/media.ts b/packages/core/src/runtime/media.ts index 8dabe8c37b..a6aa4c2183 100644 --- a/packages/core/src/runtime/media.ts +++ b/packages/core/src/runtime/media.ts @@ -274,7 +274,15 @@ export function syncRuntimeMedia(params: { // An explicit volume lane owns the fader. It is checked before the probed // keyframes because the two would otherwise fight, and it is the one the // author drew — `lint` warns when a track carries both. - const laneGain = elementVolumeLaneGain(el, relTime); + // Clip-local, NOT `relTime`. A lane's `t` is "seconds from the start of + // the clip" (see HfAutomationPoint), and the render honours that: the wav + // is already cut with `-ss mediaStart`, so its t=0 IS the clip's start. + // `relTime` is MEDIA time — it carries mediaStart, scales by playbackRate + // and wraps on a loop — so feeding it here played the envelope at a + // different position than it renders, or ran it off the end entirely on a + // trimmed clip. The FX lanes on this same feature use clip-local elapsed; + // there is one time base, and this is it. + const laneGain = elementVolumeLaneGain(el, params.timeSeconds - clip.start); if (laneGain !== null) { authorVolume = clampVolume(laneGain); } else if (clip.volumeKeyframes && clip.volumeKeyframes.length > 0) { diff --git a/packages/core/src/runtime/webAudioTransport.test.ts b/packages/core/src/runtime/webAudioTransport.test.ts index 15f1825a13..f1597ccc8b 100644 --- a/packages/core/src/runtime/webAudioTransport.test.ts +++ b/packages/core/src/runtime/webAudioTransport.test.ts @@ -388,6 +388,19 @@ describe("WebAudioTransport", () => { expect(transport.isActive()).toBe(false); }); + it("disposes the FX graph when a clip ends naturally", async () => { + // stopAll() disposes by walking _activeSources, and the splice above had + // already removed this entry — so the handle, its MutationObserver and any + // running LFO survived the clip for the rest of the session. + const { transport, mock, gen } = setupTransport(100); + await transport.schedulePlayback(mockEl, mockBuffer, 0, 0, 0, 1, gen); + + mock.sourceNode._fireEnded(); + + expect(mock.sourceNode.disconnect).toHaveBeenCalled(); + expect(mock.gainNode.disconnect).toHaveBeenCalled(); + }); + it("registers onended listener on the sourceNode", async () => { const { transport, mock, gen } = setupTransport(100); diff --git a/packages/core/src/runtime/webAudioTransport.ts b/packages/core/src/runtime/webAudioTransport.ts index fedcdca0bf..2ce3ea4bba 100644 --- a/packages/core/src/runtime/webAudioTransport.ts +++ b/packages/core/src/runtime/webAudioTransport.ts @@ -243,6 +243,21 @@ export class WebAudioTransport { if (idx !== -1) { this._activeSources.splice(idx, 1); el.muted = priorMuted; + // The graph goes with it. Splicing alone left the FX handle alive and + // then UNREACHABLE — stopAll() disposes by walking this array, which + // the splice just emptied of this entry. Every clip that finished + // naturally leaked its MutationObserver for the session, and each one + // still answered later `data-fx-chain` edits by rebuilding a whole + // graph (impulse response, chorus/phaser oscillators started and never + // stopped) around a dead source. Not disposed when idx is -1: stopAll() + // has already done it, and `stop()` is what fired this event. + try { + sourceNode.disconnect(); + fx?.dispose(); + gainNode.disconnect(); + } catch { + // Already torn down. + } if (this._activeSources.length === 0) this._paused = true; } }); diff --git a/packages/core/stubs/audio-fx-runtime-entry.ts b/packages/core/stubs/audio-fx-runtime-entry.ts index 81f8e5f035..d6a78b9439 100644 --- a/packages/core/stubs/audio-fx-runtime-entry.ts +++ b/packages/core/stubs/audio-fx-runtime-entry.ts @@ -81,6 +81,11 @@ async function render( const chain: HfAudioFxChain = parseAudioFxChain(chainJson); const channels = Math.max(1, planes.length); const frames = planes[0]?.length ?? 0; + // Nothing to process, and `new OfflineAudioContext(ch, 0, rate)` throws — an + // error the render treats as fatal. applyAudioFxChain screens empty tracks + // out before they reach the browser; this is the same guard at the point the + // constructor would actually blow up. + if (frames === 0) return planes; const parsedAutomation = automationJson ? resolveAutomation(parseAutomation(automationJson), chain) : null; diff --git a/packages/engine/src/services/audioFxRender.test.ts b/packages/engine/src/services/audioFxRender.test.ts index c3ce166047..8d9e1cf85b 100644 --- a/packages/engine/src/services/audioFxRender.test.ts +++ b/packages/engine/src/services/audioFxRender.test.ts @@ -279,3 +279,25 @@ describe.skipIf(!HAS_BROWSER)("browser render", () => { expect(readWav(outPath).samples.length).toBeGreaterThan(0); }, 180_000); }); + +/** + * ffmpeg exits 0 and writes a structurally valid but EMPTY wav whenever a + * clip's trim starts past the end of its source (`-ss 10 -t 5` on a 2 s file). + * That track used to reach `new OfflineAudioContext(ch, 0, rate)`, which throws + * — and the error travels past the mixer's per-track failure collector, so one + * mis-set `data-media-start` took the whole render down. + */ +describe("an empty track", () => { + it("is handed back untouched rather than failing the render", async () => { + const input = join(dir, "empty.wav"); + writeWav(input, new Float32Array(0), SR); + const output = join(dir, "out.wav"); + + // Returns the input path, the same contract as a chain with nothing enabled + // — and without paying for a browser to decide it. + await expect( + applyAudioFxChain(input, chainOf("peaking"), output, { trackId: "t" }), + ).resolves.toBe(input); + expect(existsSync(output)).toBe(false); + }); +}); diff --git a/packages/engine/src/services/audioFxRender.ts b/packages/engine/src/services/audioFxRender.ts index 80ce1cd8ef..b09fb1f853 100644 --- a/packages/engine/src/services/audioFxRender.ts +++ b/packages/engine/src/services/audioFxRender.ts @@ -205,18 +205,30 @@ export async function applyAudioFxChain( const { samples, sampleRate, channels } = readWav(inputWav); const planes = deinterleave(samples, channels); + // An empty track has nothing to process — and an OfflineAudioContext of zero + // length throws, which is fatal for the WHOLE render rather than this track: + // the error travels past the mixer's per-track failure collector. ffmpeg + // writes an empty but structurally valid WAV whenever a clip's trim starts + // past the end of its source, so one mis-set `data-media-start` used to take + // the render down. Guarded here as well as in the runtime so an empty track + // never costs a browser. + if ((planes[0]?.length ?? 0) === 0) return inputWav; - // Audio processing needs no GPU or special capture mode; a plain sandboxed - // browser is enough, and the lease pool reuses one across tracks. - const lease = await acquireBrowser([ - "--no-sandbox", - "--autoplay-policy=no-user-gesture-required", - ]); + // Both resources are taken INSIDE the try that releases them. The lease used + // to be acquired above it, with the mkdtemp between — so a failure there + // (a full disk, a read-only tmpdir) leaked a pooled browser, and a pool with + // no leases left hangs every later render rather than failing one. const hostDir = mkdtempSync(join(tmpdir(), "hf-fx-host-")); + let lease: Awaited> | null = null; try { + // Audio processing needs no GPU or special capture mode; a plain sandboxed + // browser is enough, and the lease pool reuses one across tracks. + // Checked before the lease, not after: an already-cancelled track has no + // reason to take a browser out of the pool just to hand it straight back. if (options.signal?.aborted) { throw new AudioFxRenderError(`Audio FX cancelled for track ${options.trackId}`); } + lease = await acquireBrowser(["--no-sandbox", "--autoplay-policy=no-user-gesture-required"]); const page = await lease.browser.newPage(); try { // AudioWorklet is only exposed in a secure context, and about:blank is @@ -302,7 +314,7 @@ export async function applyAudioFxChain( ); } finally { rmSync(hostDir, { recursive: true, force: true }); - await lease.release().catch(() => undefined); + await lease?.release().catch(() => undefined); } } diff --git a/packages/engine/src/services/audioMixer.ts b/packages/engine/src/services/audioMixer.ts index 369562e328..8a2dff3135 100644 --- a/packages/engine/src/services/audioMixer.ts +++ b/packages/engine/src/services/audioMixer.ts @@ -765,6 +765,12 @@ export async function processCompositionAudio( // be able to abort the in-flight ffmpeg runs before the finally-block removes // workDir out from under them. Chained off the caller's signal so external // cancellation still behaves as before. + // + // Every child that can outlive a sibling's failure has to be given THIS + // signal, not the caller's: the trim, the video extract and the download all + // took `signal`, so `internalController.abort()` cancelled nothing and the + // `rmSync(workDir)` on the next line ran while their ffmpeg children were + // still writing into it. const internalController = new AbortController(); const effectiveSignal = internalController.signal; if (signal) { @@ -795,7 +801,7 @@ export async function processCompositionAudio( if (isHttpUrl(srcPath)) { try { - srcPath = await downloadToTemp(srcPath, workDir); + srcPath = await downloadToTemp(srcPath, workDir, undefined, effectiveSignal); } catch (err: unknown) { failures.push( downloadFailure(err instanceof Error ? err.message : String(err), element.id), @@ -842,7 +848,7 @@ export async function processCompositionAudio( startTime: element.mediaStart, duration: element.end - element.start, }, - signal, + effectiveSignal, config, ); if (!extractResult.success) { @@ -868,7 +874,7 @@ export async function processCompositionAudio( trimmedPath, element.mediaStart, element.end - element.start, - signal, + effectiveSignal, config, ); if (!prepResult.success) { diff --git a/packages/lint/src/rules/media.test.ts b/packages/lint/src/rules/media.test.ts index f2574aa178..8554bad69d 100644 --- a/packages/lint/src/rules/media.test.ts +++ b/packages/lint/src/rules/media.test.ts @@ -457,6 +457,27 @@ describe("audio_volume_double_automation", () => { } }); + it("does not blame the wrong element in a chained timeline", async () => { + // A chain has no semicolon until its very end, so a run that could cross `)` + // reached the `volume` in a LATER call and reported the element from an + // earlier one. Acting on the fixHint would have deleted #bgm's only real + // automation to fix a tween that is on #vo. + const res = await lintHyperframeHtml( + withScript( + LANE, + `gsap.timeline().to("#bgm", { duration: 0.6, x: 10 }).to("#vo", { volume: 1 });`, + ), + ); + expect(res.findings.some((f) => f.code === "audio_volume_double_automation")).toBe(false); + }); + + it("still catches a real tween further down the same call", async () => { + const res = await lintHyperframeHtml( + withScript(LANE, `gsap.timeline().to("#bgm", { duration: 0.6, ease: "none", volume: 0 });`), + ); + expect(res.findings.some((f) => f.code === "audio_volume_double_automation")).toBe(true); + }); + it("ignores a lane that automates something other than volume", async () => { const res = await lintHyperframeHtml( withScript( diff --git a/packages/lint/src/rules/media.ts b/packages/lint/src/rules/media.ts index 9efca4f6fe..32c10d8620 100644 --- a/packages/lint/src/rules/media.ts +++ b/packages/lint/src/rules/media.ts @@ -625,7 +625,14 @@ function findVolumeDoubleAutomationFindings(ctx: LintContext): HyperframeLintFin // the same call the runtime's own probe would pick up, and the rule only // warns, so a miss costs nothing. const escaped = escapeRegExp(id); - const tweened = new RegExp(`#${escaped}(?![\\w-])[^;]{0,200}?\\bvolume\\s*:`, "s").test(script); + // `[^;)]`, not `[^;]`: a chained timeline has no semicolon until the end of + // the whole chain, so a run that could cross `)` matched `volume` in a LATER + // `.to()` call and named the wrong element — and this rule's fixHint tells + // the author to delete their lane. Refusing to cross the closing paren keeps + // the match inside the call the selector belongs to. It costs a false + // negative when some other value in the same object is a call result, which + // is the safe direction for a warning that already only guesses. + const tweened = new RegExp(`#${escaped}(?![\\w-])[^;)]{0,200}?\\bvolume\\s*:`).test(script); if (!tweened) continue; findings.push({ code: "audio_volume_double_automation", diff --git a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx index 9c508668d7..16986e6ee8 100644 --- a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx +++ b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx @@ -1456,3 +1456,66 @@ describe("AudioFxGroup carve against a deleted voice", () => { ).toEqual(["fx.k1.frequency"]); }); }); + +describe("AudioFxGroup while the carve is measuring", () => { + /** + * `analyse` captures the chain and the automation before its fetch and decode, + * then rewrites the whole `data-fx-chain` from that snapshot. An effect added + * — or a knob committed — during those seconds landed first and was silently + * discarded when the analysis returned. Only the Analyse control was gated; + * every other control in the rack stayed live throughout. + */ + it("locks the rack, so an edit cannot be made against a snapshot that is moving", async () => { + // A fetch that never settles holds the panel in its analysing state, which + // is exactly the window the race lives in. + const hang = vi.fn(() => new Promise(() => {})); + vi.stubGlobal("fetch", hang); + // happy-dom has no Web Audio, and without a constructor `analyse` returns + // before it ever reaches the decode — closing the window under test. + vi.stubGlobal( + "OfflineAudioContext", + class { + decodeAudioData() { + return new Promise(() => {}); + } + }, + ); + try { + const bed = document.createElement("audio"); + bed.id = "bed"; + bed.setAttribute("src", "bed.wav"); + document.body.append(bed); + const voice = document.createElement("audio"); + voice.id = "narration"; + voice.setAttribute("src", "vo.wav"); + document.body.append(voice); + + const host = document.createElement("div"); + document.body.append(host); + await act(async () => { + createRoot(host).render( + , + ); + }); + + // The bed carves itself against its one candidate, which starts the + // decode — and the whole rack goes read-only until it lands. + expect(hang).toHaveBeenCalled(); + const controls = Array.from(host.querySelectorAll(".hf-fx-slider")); + expect(controls.length).toBeGreaterThan(0); + expect(controls.every((c) => c.disabled)).toBe(true); + } finally { + vi.unstubAllGlobals(); + } + }); +}); diff --git a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx index 33b9f162ce..c900214f09 100644 --- a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx +++ b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx @@ -609,6 +609,14 @@ export function AudioFxGroup({ return ( { + document.body.innerHTML = ""; +}); + +/** A frequency knob: wide range, so a clamp to the minimum is unmistakable. */ +const FREQUENCY: HfAudioFxNumberParam = { + kind: "number", + key: "frequency", + label: "Frequency", + min: 20, + max: 20000, + step: 1, + default: 1000, + unit: "Hz", +}; + +/** React tracks its own value on the node, so a plain assignment is ignored. */ +function setInputValue(input: HTMLInputElement, text: string): void { + const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, "value")?.set; + setter?.call(input, text); + input.dispatchEvent(new Event("input", { bubbles: true })); +} + +function mount(value: number) { + const onChange = vi.fn(); + const onCommit = vi.fn(); + const host = document.createElement("div"); + document.body.append(host); + let root!: Root; + act(() => { + root = createRoot(host); + root.render( + , + ); + }); + const number = () => host.querySelector(".hf-fx-number")!; + const slider = () => host.querySelector(".hf-fx-slider")!; + const type = (text: string) => { + act(() => { + number().focus(); + setInputValue(number(), text); + }); + }; + /** A new value arrives through the prop — a carve, or an undo. */ + const receive = (next: number) => { + act(() => { + root.render( + , + ); + }); + }; + return { host, number, slider, onChange, onCommit, type, receive }; +} + +describe("FxParamRow number field", () => { + it("shows what is being typed instead of snapping back to the stored value", () => { + // Every keystroke is clamped and written live, and the live write does not + // refresh the prop — so a field bound to the committed value put the old + // number straight back. Typing 5000 wrote 20 on the first keystroke and the + // knob could not be typed into at all, only dragged. + const { number, type } = mount(1000); + type("5"); + expect(number().value).toBe("5"); + type("5000"); + expect(number().value).toBe("5000"); + }); + + it("does not write the parameter minimum while the field is empty", () => { + // `Number("") === 0` passes Number.isFinite, so select-all + Delete before + // retyping used to clamp to the minimum and live-write it — 20 Hz here. + const { onChange, type } = mount(1000); + type(""); + expect(onChange).not.toHaveBeenCalled(); + type("800"); + expect(onChange).toHaveBeenLastCalledWith("frequency", 800); + }); +}); + +describe("FxParamRow commit", () => { + it("stays quiet when a gesture changed nothing", () => { + // commit() hangs off pointerup, keyup and blur, all reachable without an + // edit. Firing then costs a source patch, a selection resync, a preview + // reload and an audio restart for a gesture that moved nothing. + const { slider, number, onCommit } = mount(1000); + act(() => slider().dispatchEvent(new Event("pointerup", { bubbles: true }))); + act(() => number().dispatchEvent(new Event("blur", { bubbles: true }))); + expect(onCommit).not.toHaveBeenCalled(); + }); + + it("does not re-persist a stale value over a change that arrived meanwhile", () => { + // The scenario: open an EQ row, run the carve (or press undo), which + // rewrites the chain so this row's value becomes 400. Then click the slider + // thumb and release without moving it. `latest` was seeded at mount and only + // written by an edit, so it still held 1000 — and the release wrote it back, + // undoing the carve with no gesture that looks like an edit. + const { slider, onCommit, type, receive } = mount(1000); + // An edit happened earlier in this row's life, so `latest` holds 1000. + type("1000"); + act(() => slider().dispatchEvent(new Event("pointerup", { bubbles: true }))); + onCommit.mockClear(); + + receive(400); + act(() => slider().dispatchEvent(new Event("pointerup", { bubbles: true }))); + expect(onCommit).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/studio/src/components/editor/propertyPanelFxControls.tsx b/packages/studio/src/components/editor/propertyPanelFxControls.tsx index 4fd3254bdb..63d7bfd6da 100644 --- a/packages/studio/src/components/editor/propertyPanelFxControls.tsx +++ b/packages/studio/src/components/editor/propertyPanelFxControls.tsx @@ -132,6 +132,26 @@ export function FxParamRow({ * write applied on the way down. */ const [pending, setPending] = useState(null); + /** + * What the number field is showing while it has focus. + * + * The field cannot be bound to the committed value: every keystroke is + * clamped into range and written live, and the live write does not refresh + * the prop — so React put the old number straight back and typing `5000` into + * a 20..20000 knob wrote 20 on the first keystroke and never got further. Held + * as TEXT, so a half-typed "-" or "" is a state the field can be in rather + * than a number to clamp and persist. + */ + const [typing, setTyping] = useState(null); + /** + * Did this gesture actually change anything? + * + * `commit()` hangs off pointerup, keyup and blur, all of which fire without + * an edit — clicking a slider thumb without moving it, or tabbing through the + * field. Committing then re-persisted whatever `latest` happened to hold and + * silently reverted any change that had arrived meanwhile. + */ + const edited = useRef(false); useEffect(() => { if (!dragging) setLocal(value); }, [value, dragging]); @@ -146,6 +166,7 @@ export function FxParamRow({ const p = param as HfAudioFxNumberParam; const next = Math.min(p.max, Math.max(p.min, raw)); latest.current = next; + edited.current = true; setLocal(next); onChange(param.key, next); }, @@ -154,6 +175,12 @@ export function FxParamRow({ const commit = useCallback(() => { setDragging(false); + // Nothing was edited, so there is nothing to persist. Without this a bare + // focus/blur — or a click on the slider thumb that never moved — fired a + // full persisting write: source patch, selection resync, preview reload and + // an audio restart, for a gesture that changed no value. + if (!edited.current) return; + edited.current = false; if (typeof latest.current === "number") setPending(latest.current); onCommit?.(param.key, latest.current); }, [onCommit, param.key]); @@ -228,13 +255,25 @@ export function FxParamRow({ min={param.min} max={param.max} step={param.step} - value={display(param, current)} + value={typing ?? display(param, current)} disabled={locked} + onFocus={() => setTyping(display(param, current))} onChange={(e) => { - const next = Number(e.target.value); + const text = e.target.value; + setTyping(text); + // An empty field, a lone "-", or a trailing "." are all states on the + // way to a number, not numbers. `Number("")` is 0, which passes + // Number.isFinite — so clearing the field to retype used to clamp to + // the parameter MINIMUM and write it live: 20 Hz on a cutoff, -40 dB + // on a gain. + if (text.trim() === "") return; + const next = Number(text); if (Number.isFinite(next)) handleNumber(next); }} - onBlur={commit} + onBlur={() => { + commit(); + setTyping(null); + }} onKeyDown={(e) => { if (e.key === "Enter") commit(); }}