From 0d79facf1d385f31f3272c9cd530f55ebb1fa996 Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Wed, 15 Jul 2026 00:03:46 +0200 Subject: [PATCH 1/2] fix(gig): no per-song results popup inside a set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from a live career gig: the first song of the set ended and the end-of-song summary overlay popped up. It is wrong there twice over. 1. A gig is a SET. The career plugin already shows ONE card at the end of it, scoring every song played. A per-song card interrupts the set. 2. Worse, the overlay claimAutoExit's — it takes ownership of the host's post-song return. So the play queue would NOT advance to the next song until the player dismissed the popup. The set stalled after every song. The summary is skipped for gig songs, but the take is still credited: the XP submission keeps the exact gate a built summary would have applied (the `total < 5` check, now named _summaryWorthy and shared with showSummary so the two cannot drift into "XP with no summary" or the reverse). The detector is still torn down between songs. Narrow on purpose — only the 'gig' queue source. A playlist or album keeps its per-song summary, which is what people already expect there. A cleared or stale queue does not suppress an unrelated song's summary, and a throwing host falls back to SHOWING the summary: fail visible, not silent. Tests pin the ORDERING (the gig bail must come before showSummary — build the overlay and claimAutoExit stalls the queue, which IS the bug) and the narrowness. 4/4, all failing against the pre-fix source. Full suite 346/346. --- screen.js | 38 ++++++++++++- test/gig_summary_suppressed.test.js | 85 +++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 test/gig_summary_suppressed.test.js diff --git a/screen.js b/screen.js index dd2c2c8..b8596e6 100644 --- a/screen.js +++ b/screen.js @@ -14560,9 +14560,45 @@ function createNoteDetector(options = {}) { // also doubles the journal event). The user re-enables for the // next track the same way they already do today — the playSong // wrapper silent-disables on song-switch regardless. + // A take is only worth a summary (and the XP that rides with it) once the + // player has actually played something. Was inline in showSummary; the gig + // path below needs the same answer without building an overlay. + const _SUMMARY_MIN_JUDGMENTS = 5; + function _summaryWorthy() { + return (hits + misses) >= _SUMMARY_MIN_JUDGMENTS; + } + + // A gig is a SET: the career plugin shows ONE card at the end of it, scoring + // every song played. A per-song summary on top of that is wrong twice over — + // it interrupts the set with a popup after each song, and (worse) it + // claimAutoExit's, so the host's play queue would not advance to the next + // song until the player dismissed it. The set stalled on a popup every time. + // + // Narrow on purpose: only the 'gig' queue source. A playlist or album keeps + // its per-song summary, which is what people already expect there. + function _inGigSet() { + const q = window.feedBack && window.feedBack.playQueue; + if (!q || typeof q.active !== 'function' || typeof q.source !== 'function') return false; + try { return q.active() && q.source() === 'gig'; } catch (_) { return false; } + } + function _endOfSongOnEnded() { if (!isDefault) return; if (!enabled) return; + + // In a gig, skip the overlay — but still credit the take, exactly as a + // built summary would have (same _summaryWorthy() gate showSummary + // applies), and still tear the detector down between songs. + if (_inGigSet()) { + try { + if (_summaryWorthy()) _submitSongXp(); + } catch (e) { + console.warn('[note_detect] gig-song XP failed:', e && e.message ? e.message : e); + } + try { disable({ silent: true }); } catch (e) { /* teardown is best-effort */ } + return; + } + // showSummary() has its own `total < 5` guard, so a song that // ended before the user played anything meaningful is silently // skipped. When a training take is armed, _recOnEnded opens the @@ -17152,8 +17188,8 @@ function createNoteDetector(options = {}) { // (fewer than 5 judgments) — callers deferring the summary use this // to know whether there is actually an overlay to reveal later. function showSummary(opts) { + if (!_summaryWorthy()) return false; const total = hits + misses; - if (total < 5) return false; const existing = instanceRoot.querySelector('.nd-summary-overlay'); if (existing) { existing.remove(); _ndAutoExitRelease = null; } diff --git a/test/gig_summary_suppressed.test.js b/test/gig_summary_suppressed.test.js new file mode 100644 index 0000000..96cf775 --- /dev/null +++ b/test/gig_summary_suppressed.test.js @@ -0,0 +1,85 @@ +// No per-song results popup inside a gig. +// +// Reported from a live career gig: the first song of the set ended and the +// end-of-song summary overlay popped up. It is wrong there twice over. +// +// 1. A gig is a SET. The career plugin already shows ONE card at the end of it, +// scoring every song played. A per-song card interrupts the set. +// 2. Worse, the overlay claimAutoExit's — it takes ownership of the host's +// post-song return. So the play queue would NOT advance to the next song +// until the player dismissed the popup. The set stalled after every song. +// +// The summary is suppressed for gig songs, but the take is still credited: the +// XP submission keeps the exact gate a built summary would have applied. +// +// screen.js is one large closure with no seam to import, so these are +// source-shape guards (the same approach the repo's other structural tests use). +// What they lock down is ORDERING and NARROWNESS, which is where this can +// silently regress. + +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const SRC = fs.readFileSync(path.join(__dirname, '..', 'screen.js'), 'utf8'); + +function extractBlock(src, signature) { + const start = src.indexOf(signature); + assert.ok(start !== -1, `signature '${signature}' not found`); + const openBrace = src.indexOf('{', start); + let depth = 1; + let i = openBrace + 1; + while (i < src.length && depth > 0) { + const ch = src[i]; + if (ch === '{') depth++; + else if (ch === '}') depth--; + i++; + } + assert.ok(depth === 0, `unbalanced braces after '${signature}'`); + return src.slice(start, i); +} + +test('the gig check runs BEFORE the summary is built', () => { + const fn = extractBlock(SRC, 'function _endOfSongOnEnded()'); + const gigIdx = fn.search(/_inGigSet\s*\(\s*\)/); + const summaryIdx = fn.search(/showSummary\s*\(/); + assert.ok(gigIdx !== -1, '_endOfSongOnEnded must consult _inGigSet()'); + assert.ok(summaryIdx !== -1, 'showSummary call not found'); + assert.ok(gigIdx < summaryIdx, + 'the gig bail must come before showSummary — build it and claimAutoExit ' + + 'stalls the host queue, which is the bug'); + assert.match(fn, /_inGigSet\s*\(\s*\)\s*\)\s*\{[\s\S]{0,600}?\breturn;/, + 'the gig path must RETURN out of the handler, not fall through'); +}); + +test('a gig song still earns its XP and still tears the detector down', () => { + const fn = extractBlock(SRC, 'function _endOfSongOnEnded()'); + const gigBlock = fn.slice(fn.search(/if\s*\(\s*_inGigSet\s*\(\s*\)\s*\)/)); + assert.match(gigBlock, /_submitSongXp\s*\(\s*\)/, + "suppressing the overlay must not silently drop the take's XP"); + assert.match(gigBlock, /_summaryWorthy\s*\(\s*\)/, + 'XP must use the SAME gate a built summary would have applied'); + assert.match(gigBlock, /disable\s*\(\s*\{\s*silent:\s*true\s*\}\s*\)/, + 'the detector must still be torn down between songs of the set'); +}); + +test('_inGigSet is narrow: only the gig queue, and only while it is live', () => { + const fn = extractBlock(SRC, 'function _inGigSet()'); + assert.match(fn, /===\s*'gig'/, + "must key on the 'gig' queue source — a playlist or album keeps its per-song summary"); + assert.match(fn, /\.active\s*\(\s*\)/, + "a cleared or stale queue must not suppress an unrelated song's summary"); + assert.match(fn, /catch[\s\S]{0,40}return false/, + 'a throwing host must fall back to SHOWING the summary — fail visible, not silent'); +}); + +test('showSummary and the gig path share one judgment threshold', () => { + // Drift here would mean a take earns XP with no summary, or the reverse. + assert.match(SRC, /const\s+_SUMMARY_MIN_JUDGMENTS\s*=\s*\d+/, 'threshold must be named'); + const show = extractBlock(SRC, 'function showSummary(opts)'); + assert.match(show, /_summaryWorthy\s*\(\s*\)/, + 'showSummary must use the shared gate, not a second inline `total < 5`'); +}); From eb39907224c9694a02303b4d650fc8bf5c238573 Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Wed, 15 Jul 2026 00:19:54 +0200 Subject: [PATCH 2/2] fix(gig): honour the legacy playQueue alias; bound the test's gig branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both CodeRabbit findings were right. 1. _inGigSet() only read window.feedBack.playQueue, but showSummary's own Up-Next wiring falls back to window.slopsmith.playQueue. A host exposing only the legacy alias would look queue-less here — and the popup would come back in gigs, on exactly the builds least likely to be tested. Same lookup now. 2. The test's gigBlock was sliced from the `if (_inGigSet())` guard to the END of the function, so its assertions could have been satisfied by the NON-gig path further down — which also calls _submitSongXp() and disable({silent:true}). It would have passed with an EMPTY gig branch. That is a test passing for the wrong reason, which is worse than no test. The branch is now brace-matched and bounded, with a sanity check that the extraction cannot contain showSummary, plus a test guarding the extractor itself. 6/6. --- screen.js | 8 ++++- test/gig_summary_suppressed.test.js | 51 +++++++++++++++++++++++++++-- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/screen.js b/screen.js index b8596e6..85708b6 100644 --- a/screen.js +++ b/screen.js @@ -14577,7 +14577,13 @@ function createNoteDetector(options = {}) { // Narrow on purpose: only the 'gig' queue source. A playlist or album keeps // its per-song summary, which is what people already expect there. function _inGigSet() { - const q = window.feedBack && window.feedBack.playQueue; + // Same lookup showSummary's Up-Next wiring uses, including the legacy + // `slopsmith` alias: a host that exposes only that one would otherwise + // look queue-less here, and the popup would come back in gigs on exactly + // the builds least likely to be tested. + const q = (window.feedBack && window.feedBack.playQueue) + || (window.slopsmith && window.slopsmith.playQueue) + || null; if (!q || typeof q.active !== 'function' || typeof q.source !== 'function') return false; try { return q.active() && q.source() === 'gig'; } catch (_) { return false; } } diff --git a/test/gig_summary_suppressed.test.js b/test/gig_summary_suppressed.test.js index 96cf775..60a1dc1 100644 --- a/test/gig_summary_suppressed.test.js +++ b/test/gig_summary_suppressed.test.js @@ -55,9 +55,34 @@ test('the gig check runs BEFORE the summary is built', () => { 'the gig path must RETURN out of the handler, not fall through'); }); +// Extract ONLY the body of `if (_inGigSet()) { ... }`. +// +// The first version of this sliced from the `if` to the end of the function, so +// every assertion below could have been satisfied by the NON-gig path further +// down — which calls _submitSongXp() and disable({silent:true}) too. It would +// have passed even if the gig branch were empty. Bound it properly: a test that +// can pass for the wrong reason is worse than no test. +function gigBranch(src) { + const fn = extractBlock(src, 'function _endOfSongOnEnded()'); + const at = fn.search(/if\s*\(\s*_inGigSet\s*\(\s*\)\s*\)/); + assert.ok(at !== -1, 'gig guard not found'); + const open = fn.indexOf('{', at); + let depth = 1, i = open + 1; + while (i < fn.length && depth > 0) { + if (fn[i] === '{') depth++; + else if (fn[i] === '}') depth--; + i++; + } + assert.ok(depth === 0, 'unbalanced braces in the gig branch'); + const body = fn.slice(open + 1, i - 1); + // Sanity: the branch must not have swallowed the whole function. + assert.ok(!/showSummary\s*\(/.test(body), + 'the extracted branch must be the GIG path only — it must not contain showSummary'); + return body; +} + test('a gig song still earns its XP and still tears the detector down', () => { - const fn = extractBlock(SRC, 'function _endOfSongOnEnded()'); - const gigBlock = fn.slice(fn.search(/if\s*\(\s*_inGigSet\s*\(\s*\)\s*\)/)); + const gigBlock = gigBranch(SRC); assert.match(gigBlock, /_submitSongXp\s*\(\s*\)/, "suppressing the overlay must not silently drop the take's XP"); assert.match(gigBlock, /_summaryWorthy\s*\(\s*\)/, @@ -83,3 +108,25 @@ test('showSummary and the gig path share one judgment threshold', () => { assert.match(show, /_summaryWorthy\s*\(\s*\)/, 'showSummary must use the shared gate, not a second inline `total < 5`'); }); + +test('the legacy slopsmith.playQueue alias is honoured', () => { + const fn = extractBlock(SRC, 'function _inGigSet()'); + assert.match(fn, /window\.feedBack\s*&&\s*window\.feedBack\.playQueue/, + 'the modern bus first'); + assert.match(fn, /window\.slopsmith\s*&&\s*window\.slopsmith\.playQueue/, + 'a host exposing only the legacy alias would otherwise look queue-less, and the ' + + 'popup would come back in gigs on exactly the builds least likely to be tested ' + + '(showSummary already falls back this way)'); +}); + +test('the gig-branch extractor is genuinely bounded', () => { + // Guards the guard: if gigBranch() ever ran past the `if` block again, the + // assertions above could be satisfied by the non-gig path, which also calls + // _submitSongXp() and disable({silent:true}). + const body = gigBranch(SRC); + assert.ok(body.length > 0); + assert.ok(!/showSummary/.test(body)); + const whole = extractBlock(SRC, 'function _endOfSongOnEnded()'); + assert.ok(body.length < whole.length / 2, + 'the gig branch is a small early-return, not most of the function'); +});