From 671cde74610a44793a9c2e338b34dfd3d750a5a5 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 21:49:51 +0100 Subject: [PATCH] fix(player): drive playback through the audio engine, not the silent multitrack engineMode() returns "chunked" unless the user has set the audioEngine flag to "0", and on that path audioEngine owns the clock while the multitrack is mounted with url: null for visuals only. transport.js handled this everywhere via `audioEngine ?? multitrack`; main.js imported only `multitrack` and called it bare. So on the default configuration: - The footer scrub bar did nothing. It is a full-size cursor: pointer overlay, and clicking or dragging anywhere on it moved neither the playhead nor the audio. - [ and ] seeked the silent multitrack, so nothing happened. - I and O read multitrack.getCurrentTime(), which is pinned at 0 there, so "set loop in at playhead" always wrote 0 no matter where the playhead was, and "set loop out" always wrote max(0, loopStart + 0.5). Space and ruler clicks were unaffected because they live in transport.js. Rather than patching five call sites, transport.js exports the accessor it was already using internally, plus setPlayheadTime. Seeking now goes through setPlayheadTime, which also updates the playhead marker, footer times and presence playhead -- none of which the old multitrack.setTime call did, so the scrub bar would have left the marker stale even had it worked. Same anti-drift shape as apply_ffmpeg_path in #506. The keydown guard excluded HTMLInputElement but not HTMLTextAreaElement, so Space in the settings log viewer started playback instead of scrolling. Fixed alongside, since the guard was being edited anyway. The test is structural rather than DOM-driven: the bug was not a wrong value from a function, it was reaching for the wrong object, so it asserts main.js holds no bare multitrack playback calls. All 8 checks fail against the previous code. Refs #515 --- static/js/main.js | 36 ++++++++++------ static/js/transport.js | 13 +++++- tests/js/transport-clock.test.mjs | 72 +++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 15 deletions(-) create mode 100644 tests/js/transport-clock.test.mjs diff --git a/static/js/main.js b/static/js/main.js index 4009ce6c..80e6f331 100644 --- a/static/js/main.js +++ b/static/js/main.js @@ -1,5 +1,5 @@ import { - playBtn, loopBtn, multitrack, totalDuration, loopEnabled, loopStart, loopEnd, + playBtn, loopBtn, totalDuration, loopEnabled, loopStart, loopEnd, setLoopStart, setLoopEnd, selectedStems, saveSelectedStems, stemSelectionReady, currentJobId, vocalSplitMode, vocalSplitModeReady, setVocalSplitMode, setAutoSectionsResetFn, @@ -10,7 +10,7 @@ import { wireJobForm, showError } from "./job.js"; import { initSearch } from "./search.js"; import { wireTransportButtons } from "./transport.js"; import { wireBeatGridUi } from "./beatgridUi.js"; -import { togglePlayPause, updateLoopRegionVisual, toggleMetronome } from "./transport.js"; +import { togglePlayPause, updateLoopRegionVisual, toggleMetronome, transport, setPlayheadTime } from "./transport.js"; import { wireStemListControls, wireMixerToolbar } from "./mixer.js"; import { initCatalog, collectDiagnostics } from "./catalog.js"; import { initNotifications, notifyFailure, dismissFailuresByJobId } from "./notifications.js"; @@ -486,10 +486,14 @@ function wireFooterControls() { const scrub = document.getElementById("footer-scrub"); if (scrub) { function seekToX(clientX) { - if (!multitrack || !totalDuration) return; + // setPlayheadTime, not multitrack.setTime: on the default chunked engine + // the multitrack is silent and this whole bar did nothing. It also moves + // the playhead marker, footer times and presence playhead, which the old + // call never did (#515). + if (!totalDuration) return; const rect = scrub.getBoundingClientRect(); const frac = Math.max(0, Math.min(1, (clientX - rect.left) / rect.width)); - multitrack.setTime(frac * totalDuration); + setPlayheadTime(frac * totalDuration); } let _scrubbing = false; scrub.addEventListener("mousedown", (e) => { @@ -645,32 +649,36 @@ function wireAppShellControls() { // ─── Keyboard shortcuts ─── document.addEventListener("keydown", (e) => { - if (!multitrack) return; - if (e.target instanceof HTMLInputElement) return; + if (!transport()) return; + // Textareas were not excluded, so Space in the log viewer started playback + // instead of scrolling. + if (e.target instanceof HTMLInputElement || e.target instanceof HTMLTextAreaElement) return; if (e.code === "Space") { e.preventDefault(); togglePlayPause(); } else if (e.code === "BracketLeft") { e.preventDefault(); - multitrack.setTime(Math.max(0, multitrack.getCurrentTime() - 5)); + // setPlayheadTime clamps to [0, totalDuration] itself, so the Math.max / + // Math.min the multitrack version needed are gone with it. + setPlayheadTime(transport().getCurrentTime() - 5); } else if (e.code === "BracketRight") { e.preventDefault(); - multitrack.setTime( - Math.min(multitrack.getDuration(), multitrack.getCurrentTime() + 5), - ); + setPlayheadTime(transport().getCurrentTime() + 5); } else if (e.code === "KeyL") { e.preventDefault(); loopBtn.click(); } else if (e.code === "KeyK") { e.preventDefault(); toggleMetronome(); - } else if (e.code === "KeyI" && loopEnabled && multitrack) { + } else if (e.code === "KeyI" && loopEnabled) { e.preventDefault(); - setLoopStart(Math.min(multitrack.getCurrentTime(), loopEnd - 0.5)); + // multitrack.getCurrentTime() is pinned at 0 on the engine path, so "set + // loop in at playhead" always wrote 0 regardless of where the playhead was. + setLoopStart(Math.min(transport().getCurrentTime(), loopEnd - 0.5)); updateLoopRegionVisual(); - } else if (e.code === "KeyO" && loopEnabled && multitrack) { + } else if (e.code === "KeyO" && loopEnabled) { e.preventDefault(); - setLoopEnd(Math.max(multitrack.getCurrentTime(), loopStart + 0.5)); + setLoopEnd(Math.max(transport().getCurrentTime(), loopStart + 0.5)); updateLoopRegionVisual(); } }); diff --git a/static/js/transport.js b/static/js/transport.js index 7b6e92fc..826456e8 100644 --- a/static/js/transport.js +++ b/static/js/transport.js @@ -70,7 +70,18 @@ function timeFromClientX(clientX) { return frac * totalDuration; } -function setPlayheadTime(sec) { +/// The clock that actually owns playback. +/// +/// engineMode() defaults to "chunked", where audioEngine drives audio and the +/// multitrack is mounted with url: null for visuals only -- so operating on +/// `multitrack` directly moves nothing and reads 0. Everything in this module +/// already went through `audioEngine ?? multitrack`; exporting it stops other +/// modules re-deriving it and drifting (#515). +export function transport() { + return audioEngine ?? multitrack; +} + +export function setPlayheadTime(sec) { const tx = audioEngine ?? multitrack; if (!tx || !totalDuration) return; const next = Math.max(0, Math.min(totalDuration, sec)); diff --git a/tests/js/transport-clock.test.mjs b/tests/js/transport-clock.test.mjs new file mode 100644 index 00000000..61bf6f06 --- /dev/null +++ b/tests/js/transport-clock.test.mjs @@ -0,0 +1,72 @@ +// main.js drove `multitrack` directly while audioEngine owns the clock (#515). +// +// engineMode() defaults to "chunked", where the multitrack is mounted with +// url: null for visuals only. So the footer scrub bar did nothing at all, and +// "set loop in at playhead" always wrote 0 because multitrack.getCurrentTime() +// is pinned there. +// +// A structural check rather than a DOM one: the bug was not a wrong value from +// a function, it was reaching for the wrong object. Anything that reintroduces +// a bare multitrack playback call in main.js fails here. + +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +const root = join(dirname(fileURLToPath(import.meta.url)), '..', '..'); +const mainSrc = readFileSync(join(root, 'static/js/main.js'), 'utf8'); +const transportSrc = readFileSync(join(root, 'static/js/transport.js'), 'utf8'); + +let passed = 0; +let failed = 0; + +function check(name, condition, detail = '') { + if (condition) { + passed++; + console.log(`PASS ${name}`); + } else { + failed++; + console.log(`FAIL ${name}${detail ? ` -- ${detail}` : ''}`); + } +} + +// Strip comments so the explanatory ones below don't count as usage. +const code = mainSrc.replace(/\/\/.*$/gm, '').replace(/\/\*[\s\S]*?\*\//g, ''); + +for (const call of ['multitrack.setTime', 'multitrack.getCurrentTime', 'multitrack.getDuration']) { + check( + `main.js does not call ${call}`, + !code.includes(call), + 'the multitrack is silent on the default chunked engine', + ); +} + +check( + 'transport.js exports the accessor', + /export function transport\(\)/.test(transportSrc), +); + +check( + 'the accessor prefers the audio engine', + /return audioEngine \?\? multitrack;/.test(transportSrc), +); + +check( + 'transport.js exports setPlayheadTime', + /export function setPlayheadTime\(/.test(transportSrc), +); + +check( + 'main.js imports both rather than re-deriving them', + /import \{[^}]*\btransport\b[^}]*\} from "\.\/transport\.js"/.test(mainSrc) && + /import \{[^}]*\bsetPlayheadTime\b[^}]*\} from "\.\/transport\.js"/.test(mainSrc), +); + +check( + 'the keyboard guard excludes textareas', + code.includes('HTMLTextAreaElement'), + 'Space in the log viewer used to start playback instead of scrolling', +); + +console.log(`\n${passed}/${passed + failed} checks passed`); +process.exit(failed === 0 ? 0 : 1);