mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-09-12 02:18:32 +00:00
fix(vocal-cal): cancel fallback timer on Skip via _activeCleanup (Creed finding)
Root cause: clicking Calibrate in the vocalCalibration-absent path queued
an 1800ms setTimeout but never stored the timer id. If the user clicked
Skip before the timer fired, advance('vocals', false) resolved the wizard;
the stale timer then fired advance('vocals', true), mutating the completed
array after resolution and (with a multi-instrument queue) double-
incrementing idx so the next instrument was dropped from both lists.
Fix: capture the setTimeout return value as _timerId and assign:
_activeCleanup = () => clearTimeout(_timerId)
advance() already drains _activeCleanup on every exit path (Skip,
Calibrate, and any future button), so no further call sites needed.
The _advancing flag + calBtn.disabled remain as the double-click guard;
this fix covers the orthogonal Skip-before-timer race.
Test 3c (new): Calibrate then Skip before timer — asserts clearTimeout
called, vocals in skipped only (not completed), stale timer no-op.
Fails without the fix. Harness updated to expose clearTimeout to vm ctx.
Gates: JS 1141/1141, pytest 60/60.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H2bM5jSbMskpdxm2CmuQVj
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
47e85d61c7
commit
d2847ab153
@@ -200,7 +200,12 @@
|
|||||||
calBtn.disabled = true;
|
calBtn.disabled = true;
|
||||||
const body = host.querySelector('[data-is-body]');
|
const body = host.querySelector('[data-is-body]');
|
||||||
if (body) body.innerHTML = '<p class="text-sm text-fb-textDim">Vocal calibration will be available once the Vocal Highway plugin is enabled. Continuing…</p>';
|
if (body) body.innerHTML = '<p class="text-sm text-fb-textDim">Vocal calibration will be available once the Vocal Highway plugin is enabled. Continuing…</p>';
|
||||||
setTimeout(() => advance(inst, true), 1800);
|
// Store timer id so advance() (via _activeCleanup) can cancel it
|
||||||
|
// if the user hits Skip before the 1800ms fires — prevents the
|
||||||
|
// stale timer from double-advancing the wizard and dropping the
|
||||||
|
// next instrument from both completed and skipped.
|
||||||
|
const _timerId = setTimeout(() => advance(inst, true), 1800);
|
||||||
|
_activeCleanup = () => clearTimeout(_timerId);
|
||||||
}
|
}
|
||||||
} else if (hasDetector) {
|
} else if (hasDetector) {
|
||||||
// Hide our own full-screen overlay while note_detect's
|
// Hide our own full-screen overlay while note_detect's
|
||||||
|
|||||||
@@ -96,6 +96,8 @@ function loadScreenJs(windowOverrides) {
|
|||||||
const timeoutImpl = (windowOverrides && windowOverrides.setTimeout)
|
const timeoutImpl = (windowOverrides && windowOverrides.setTimeout)
|
||||||
|| function(fn, _ms) { fn(); };
|
|| function(fn, _ms) { fn(); };
|
||||||
|
|
||||||
|
const clearTimeoutImpl = (windowOverrides && windowOverrides.clearTimeout) || function(_id) {};
|
||||||
|
|
||||||
const ctx = {
|
const ctx = {
|
||||||
window: Object.assign({
|
window: Object.assign({
|
||||||
feedBack: {
|
feedBack: {
|
||||||
@@ -117,6 +119,7 @@ function loadScreenJs(windowOverrides) {
|
|||||||
})(),
|
})(),
|
||||||
fetch() { return Promise.resolve({ ok: true }); },
|
fetch() { return Promise.resolve({ ok: true }); },
|
||||||
setTimeout: timeoutImpl,
|
setTimeout: timeoutImpl,
|
||||||
|
clearTimeout: clearTimeoutImpl,
|
||||||
console,
|
console,
|
||||||
};
|
};
|
||||||
vm.createContext(ctx);
|
vm.createContext(ctx);
|
||||||
@@ -237,6 +240,62 @@ test('double-click on fallback Calibrate does not double-advance the wizard', as
|
|||||||
assert.equal(result.completed[0], 'vocals');
|
assert.equal(result.completed[0], 'vocals');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ── 3c. Calibrate-then-Skip stale-timer bug (Creed finding) ──────────────────
|
||||||
|
// Failure input: vocalCalibration absent, ['vocals'] queue, user clicks
|
||||||
|
// Calibrate (queues 1800ms timer) then clicks Skip before timer fires.
|
||||||
|
// Without fix: Skip calls advance('vocals', false) → wizard resolves, then
|
||||||
|
// the stale timer fires advance('vocals', true) → completed array mutated
|
||||||
|
// AFTER promise settled → result.completed gains 'vocals' retroactively;
|
||||||
|
// with a 2-instrument queue, idx is also double-incremented, dropping the
|
||||||
|
// next instrument from both lists.
|
||||||
|
// Fix: _activeCleanup = () => clearTimeout(_timerId) — advance() drains it.
|
||||||
|
|
||||||
|
test('Skip before fallback timer fires cancels the timer — no stale advance', async () => {
|
||||||
|
const timers = []; // collect scheduled timers without auto-firing
|
||||||
|
const cancelled = [];
|
||||||
|
const w = loadScreenJs({
|
||||||
|
feedBack: { capabilities: null, midiInput: null, vocalCalibration: undefined },
|
||||||
|
setTimeout(fn, _ms) { const id = timers.length; timers.push(fn); return id; },
|
||||||
|
clearTimeout(id) { cancelled.push(id); timers[id] = null; },
|
||||||
|
});
|
||||||
|
|
||||||
|
const hostEl = makeEl('div', {});
|
||||||
|
const mountPromise = w.feedBackInputSetup.mount(hostEl, { instruments: ['vocals'] });
|
||||||
|
await new Promise((r) => setImmediate(r));
|
||||||
|
|
||||||
|
const calBtn = hostEl._namedChildren && hostEl._namedChildren['is-cal'];
|
||||||
|
assert.ok(calBtn, 'Calibrate button must exist');
|
||||||
|
|
||||||
|
// Step 1: click Calibrate — queues the 1800ms timer
|
||||||
|
calBtn.click();
|
||||||
|
assert.equal(timers.length, 1, 'one timer must be queued after Calibrate');
|
||||||
|
|
||||||
|
// Step 2: click Skip BEFORE the timer fires
|
||||||
|
const skipBtn = hostEl._namedChildren && hostEl._namedChildren['is-skip'];
|
||||||
|
assert.ok(skipBtn, 'Skip button must exist');
|
||||||
|
skipBtn.click();
|
||||||
|
|
||||||
|
// The promise resolves via Skip's advance('vocals', false)
|
||||||
|
const result = await mountPromise;
|
||||||
|
|
||||||
|
// vocals must be in skipped, NOT completed
|
||||||
|
assert.equal(result.skipped.length, 1, 'vocals must be skipped');
|
||||||
|
assert.equal(result.skipped[0], 'vocals');
|
||||||
|
assert.equal(result.completed.length, 0, 'completed must be empty after skip');
|
||||||
|
|
||||||
|
// The timer must have been cancelled — input that FAILS without the fix:
|
||||||
|
// if clearTimeout was NOT called, firing the stale timer now would mutate
|
||||||
|
// the completed array retrospectively.
|
||||||
|
assert.ok(cancelled.length > 0, 'clearTimeout must be called — stale timer must be cancelled');
|
||||||
|
|
||||||
|
// Verify: firing the (now-cancelled) timer is a no-op (timers[0] nulled out)
|
||||||
|
if (timers[0] !== null) {
|
||||||
|
// If the fix is missing, this would push 'vocals' into completed
|
||||||
|
timers[0]();
|
||||||
|
assert.equal(result.completed.length, 0, 'stale timer must not mutate completed after skip');
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
// ── 4. vocalCalibration facade present → launch() called, not noteDetect ──────
|
// ── 4. vocalCalibration facade present → launch() called, not noteDetect ──────
|
||||||
|
|
||||||
test('renderAudioPanel for vocals calls vocalCalibration.launch when facade present', async () => {
|
test('renderAudioPanel for vocals calls vocalCalibration.launch when facade present', async () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user