From 939c98214b7ff2872cd0ac39a3015980ab4931bf Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 00:36:13 +0200 Subject: [PATCH 1/9] feat(song-info): publish the playable stem list so stems can preload (fixes the 698ms freeze) (#972) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(song-info): publish the playable stem list, so stems can preload The stems plugin could only learn its stem list from the highway's WS `ready`, which arrives once the highway is already up. So it fetched, decoded, and then handed every stem's PCM to its audio worklet — copying the WHOLE SONG — with the player already on screen. For a 4-minute 6-stem pack that is over half a GIGABYTE of memcpy, in one frame, on the main thread. Measured on a real load: a 698 ms frame, right as the song-credits card appeared, with the venue video visibly stopping. That is the "the video pauses when the author appears" report. GET /api/song/{f}?stems=1 now returns the same list — [{id, url, default}] plus full_mix_url — so the plugin can start the whole load at `song:loading`, before the highway (and the venue) is drawn, where a stalled frame costs nothing. Nothing about the work changes; only WHEN. Opt-in via the query param so the library's own metadata calls — the hot path — pay nothing. Deliberately NOT stored in the metadata cache: that is a fixed-column table, and widening it would mean a schema migration plus a stale row for every song already scanned, to cache something that is a plain manifest read on an already-unpacked pack. The safety property: REST and the WS must publish the SAME list. If they disagreed the plugin would preload a graph and then throw it away and rebuild — strictly worse than not preloading. So both now resolve `default` through one shared helper (stem_default_on, extracted from load_song), and a test rebuilds the WS's payload from load_song and requires the REST helper to produce the identical list, rather than pinning either against a snapshot. Also pinned: the mixdown is lifted OUT of the stem list (spec 5.3 — `full` is not a layer; listing it beside the instruments would play the whole song on top of the stems) while staying reachable as full_mix_url, a single-`full` pack keeps it as its only playable stem, and an unreadable pack yields an empty list rather than failing the request. Full suite 2608 passed. Consumed by feedBack-plugin-stems (preloadSong). * fix(song-info): call load_song for the stem payload — do not reimplement it CodeRabbit caught a real bug, and it would have hit most real libraries. load_song() falls back to the DEPRECATED `original_audio:` key when a pack has no reserved `full` stem — which is every pack written before feedpak 1.15.0. My payload rebuilt the full-mix rule from extract_meta and returned None for those: REST would say "no full mix" while the WS said there was one. Worse than a wrong field: the plugin would preload a graph WITHOUT the pristine mix and — because the stem signature still matched — never rebuild. Unity playback would silently downgrade to the lossy stem recombination. That is exactly the drift this PR claims to prevent, and my test had a hole: I only covered packs that carry a `full` stem. So stop reimplementing. The payload now calls load_song, whose LoadedSloppak already carries the partitioned stems and the resolved full mix, and builds the URLs exactly as ws_highway does. Drift is now impossible by construction rather than by agreement. extract_meta is reverted to its original shape (it never needed to change), and the shared stem_default_on helper stays as the one place `default: off` is resolved. Tests rewritten to compare against load_song — the WS's own function — for a reserved-`full` pack, a LEGACY original_audio pack (the case that was broken), and a single-`full` pack. Also documents the `?stems=1` contract in CHANGELOG.md. Full suite green. --- CHANGELOG.md | 10 +++ lib/routers/song.py | 74 ++++++++++++++++-- lib/sloppak.py | 25 ++++-- tests/test_song_info_stems.py | 140 ++++++++++++++++++++++++++++++++++ 4 files changed, 238 insertions(+), 11 deletions(-) create mode 100644 tests/test_song_info_stems.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 58dda22..8236ffa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 carry their gig log; instruments their gig count. ### Changed +- **`GET /api/song/{f}?stems=1`** (new, opt-in) — returns the pack's playable stem + list (`[{id, url, default}]` + `full_mix_url`), the same list the highway's WS + `ready` sends. The stems plugin could only learn it from that WS message, which + arrives once the highway is already on screen — so it decoded and then copied the + whole song's PCM to its audio worklet with the player visible: over half a gigabyte + of memcpy in one frame for a 6-stem pack, a measured 698 ms freeze right as the + song-credits card appeared. With the list available at `song:loading` the plugin + does all of it before the highway is drawn. Built by calling `load_song` itself, so + it cannot drift from what the WS sends. Opt-in, so the library's metadata calls pay + nothing. - **Folder library renders only the songs on screen** (#965) — a song list used to render *every* song it held. On a flat 50,944-song library that was one `
` with 50,938 children and ~1.3 **million** DOM nodes (~4.2 GB of renderer memory), diff --git a/lib/routers/song.py b/lib/routers/song.py index 72a1d06..aa6644e 100644 --- a/lib/routers/song.py +++ b/lib/routers/song.py @@ -829,9 +829,60 @@ def post_song_gap_fill(filename: str, data: dict): return {"ok": True, "written": additions, "skipped": skipped} +def _playable_stems_payload(filename: str, dlc) -> dict: + """The playable stems (id/url/default) + full-mix URL for a sloppak. + + Why it exists: the stems plugin could only learn its stem list from the + highway's WS `ready`, which arrives once the highway is already up. So it + decoded, and then copied the whole song's PCM to its worklet, with the player + on screen — half a gigabyte of memcpy in one frame, ~700 ms, freezing the + venue video. Given the list at `song:loading` it can do all of that BEFORE the + highway appears, behind the loading overlay where a stall costs nothing. + + The list MUST be the same one the WS sends a moment later. If it is not, the + plugin preloads a graph and then throws it away and rebuilds — strictly worse + than not preloading. So this does not reimplement the WS's construction, it + calls THE SAME FUNCTION: load_song, whose LoadedSloppak already carries the + partitioned stems and the resolved full mix, and then builds the URLs exactly + as ws_highway does. Drift is impossible by construction rather than by + agreement — which matters, because `full_mix` in particular is not simply the + `full` stem: load_song falls back to the deprecated `original_audio:` key for + every pack written before feedpak 1.15.0, and reimplementing that (I did, at + first) silently dropped the pristine full mix for most real libraries. + + Opt-in (`?stems=1`) so the library's own metadata calls — the hot path — pay + nothing for it. Non-sloppak sources (archives, loose folders) have no stems + to preload: load_song raises and we return the empty list. + """ + from urllib.parse import quote + + try: + loaded = sloppak_mod.load_song(filename, dlc, appstate.sloppak_cache_dir) + except Exception: + return {"stems": [], "full_mix_url": None} + + q_fn = quote(filename, safe="") + + def _url(rel: str) -> str: + return f"/api/sloppak/{q_fn}/file/{quote(rel)}" + + return { + "stems": [ + {"id": s["id"], "url": _url(s["file"]), "default": s["default"]} + for s in loaded.stems + ], + "full_mix_url": _url(loaded.full_mix) if loaded.full_mix else None, + } + + @router.get("/api/song/{filename:path}") -async def get_song_info(filename: str): - """Return song metadata, from cache or by extracting it from the song source.""" +async def get_song_info(filename: str, stems: int = 0): + """Return song metadata, from cache or by extracting it from the song source. + + `?stems=1` additionally returns the playable stem list with URLs, so the + stems plugin can start fetching/decoding on `song:loading` instead of waiting + for the highway's WS `ready` (see _playable_stems_payload). + """ import asyncio dlc = _get_dlc_dir() if not dlc: @@ -854,8 +905,21 @@ async def get_song_info(filename: str): mtime, size = appstate.stat_for_cache(song_path) cached = appstate.meta_db.get(cache_key, mtime, size) + loop = asyncio.get_event_loop() + + # The stem list is NOT stored in the metadata cache: that is a fixed-column + # table, and widening it would mean a migration plus a stale row for every + # song already scanned. It is cheap to read on demand (the pack is unpacked + # by then, so this is a plain manifest read), and only the opt-in caller pays. + async def _with_stems(meta: dict) -> dict: + if not stems: + return meta + extra = await loop.run_in_executor( + None, _playable_stems_payload, filename, dlc) + return {**meta, **extra} + if cached: - return cached + return await _with_stems(cached) # Extract in thread pool def _extract(): @@ -863,5 +927,5 @@ async def get_song_info(filename: str): appstate.meta_db.put(cache_key, mtime, size, meta) return meta - meta = await asyncio.get_event_loop().run_in_executor(None, _extract) - return meta + meta = await loop.run_in_executor(None, _extract) + return await _with_stems(meta) diff --git a/lib/sloppak.py b/lib/sloppak.py index d5a12a0..8f943e9 100644 --- a/lib/sloppak.py +++ b/lib/sloppak.py @@ -80,6 +80,20 @@ def find_full_mix(stems: list[dict]) -> dict | None: ) +def stem_default_on(raw) -> bool: + """Whether a manifest stem entry plays by default. + + Absent means on. A string is honoured so a hand-written manifest can say + `default: off`. Extracted so the WS `ready` payload and the REST song-info + payload cannot drift: the stems plugin now preloads from REST and then has + to agree with what the WS says a moment later, or it would rebuild the whole + graph for nothing. + """ + if isinstance(raw, str): + return raw.lower() not in ("off", "false", "0", "no") + return bool(raw) + + def partition_stems(stems: list[dict]) -> tuple[dict | None, list[dict]]: """Split stem descriptors into (mixdown, instrument_stems) for PLAYBACK. @@ -1100,12 +1114,11 @@ def load_song( sfile = str(s.get("file", "")) if not sid or not sfile: continue - default_val = s.get("default", True) - if isinstance(default_val, str): - default_on = default_val.lower() not in ("off", "false", "0", "no") - else: - default_on = bool(default_val) - stems.append({"id": sid, "file": sfile, "default": default_on}) + stems.append({ + "id": sid, + "file": sfile, + "default": stem_default_on(s.get("default", True)), + }) # The complete mixdown is a stem (spec §5.3), but it is not a *layer*: lift # it out so that no consumer of `stems` — the mixer, the library's stem diff --git a/tests/test_song_info_stems.py b/tests/test_song_info_stems.py new file mode 100644 index 0000000..dad4365 --- /dev/null +++ b/tests/test_song_info_stems.py @@ -0,0 +1,140 @@ +"""`/api/song/{f}?stems=1` — the playable stem list, for preloading. + +The stems plugin could only learn its stem list from the highway's WS `ready`, +which arrives once the highway is already up. So it decoded the stems and then +copied the whole song's PCM to its audio worklet with the player on screen — +half a gigabyte of memcpy in one frame, ~700 ms, which froze the venue video. + +Given the list at `song:loading` it can do all of that BEFORE the highway +appears, behind the loading overlay where a stall costs nothing. + +The safety property these tests exist for: the REST payload must be the SAME +list the WS builds. If they disagree, the plugin preloads one graph and then +throws it away and rebuilds another — strictly worse than not preloading. So +they are pinned against each other, not just against a snapshot. +""" + +import zipfile + +import yaml + +import sloppak + + +def _pak(tmp_path, stems, full=None, name="song.feedpak", original_audio=None): + manifest = { + "title": "T", "artist": "A", "duration": 10.0, + "arrangements": [], + "stems": stems + ([full] if full else []), + } + if original_audio: + # The deprecated pre-1.15.0 shape: the mixdown lives outside `stems`. + manifest["original_audio"] = original_audio + p = tmp_path / name + with zipfile.ZipFile(p, "w") as z: + # Real packs carry manifest.yaml — a JSON manifest is not read at all. + z.writestr("manifest.yaml", yaml.safe_dump(manifest)) + # _legacy_full_mix only returns a path that actually EXISTS on disk. + if original_audio: + z.writestr(original_audio, b"\0" * 16) + return p + + +def _payload(tmp_path, pak): + from routers.song import _playable_stems_payload + import appstate + cache = tmp_path / "cache" + cache.mkdir(exist_ok=True) + appstate.sloppak_cache_dir = cache + return _playable_stems_payload(pak.name, tmp_path) + + +def _ws_payload(tmp_path, pak): + """Rebuild the WS `ready` stems payload exactly as ws_highway.py does.""" + from urllib.parse import quote + cache = tmp_path / "cache" + cache.mkdir(exist_ok=True) + loaded = sloppak.load_song(pak.name, tmp_path, cache) + q = quote(pak.name, safe="") + return { + "stems": [ + {"id": s["id"], "url": f"/api/sloppak/{q}/file/{quote(s['file'])}", + "default": s["default"]} + for s in loaded.stems + ], + "full_mix_url": f"/api/sloppak/{q}/file/{quote(loaded.full_mix)}" if loaded.full_mix else None, + } + + +def test_default_resolution_is_shared_with_load_song(): + assert sloppak.stem_default_on(True) is True + assert sloppak.stem_default_on(False) is False + assert sloppak.stem_default_on("off") is False + assert sloppak.stem_default_on("false") is False + assert sloppak.stem_default_on("0") is False + assert sloppak.stem_default_on("no") is False + assert sloppak.stem_default_on("on") is True + assert sloppak.stem_default_on(1) is True + + +def test_rest_matches_the_ws_for_a_reserved_full_stem(tmp_path): + pak = _pak(tmp_path, + [{"id": "guitar", "file": "stems/guitar.ogg"}, + {"id": "vocals", "file": "stems/vocals.ogg", "default": "off"}], + full={"id": "full", "file": "stems/full.ogg"}, + name="Iron Maiden - Phantom.feedpak") + rest = _payload(tmp_path, pak) + assert rest == _ws_payload(tmp_path, pak) + assert [s["id"] for s in rest["stems"]] == ["guitar", "vocals"], "the mixdown is not a layer" + assert rest["full_mix_url"].endswith("stems/full.ogg") + assert rest["stems"][1]["default"] is False + + +def test_rest_matches_the_ws_for_a_LEGACY_original_audio_pack(tmp_path): + """The one CodeRabbit caught, and the one that matters most in practice. + + load_song falls back to the DEPRECATED `original_audio:` key when a pack has + no reserved `full` stem — which is every pack written before feedpak 1.15.0, + i.e. most of a real library. My first version of this payload reimplemented + the full-mix rule from extract_meta and silently returned None for them: REST + would say "no full mix" while the WS said there was one. The plugin would then + preload a graph WITHOUT the pristine mix and, because the signature still + matched, never rebuild — unity playback silently downgraded to the lossy + recombination. + + The payload now calls load_song itself, so this cannot drift. Pinned anyway. + """ + pak = _pak(tmp_path, [ + {"id": "guitar", "file": "stems/guitar.ogg"}, + {"id": "bass", "file": "stems/bass.ogg"}, + ], name="Legacy Pack.feedpak", original_audio="original/full.ogg") + + rest = _payload(tmp_path, pak) + assert rest == _ws_payload(tmp_path, pak) + assert rest["full_mix_url"] is not None, ( + "a pre-1.15.0 pack's full mix must survive — dropping it downgrades unity " + "playback to the lossy stem recombination, silently" + ) + assert rest["full_mix_url"].endswith("original/full.ogg") + + +def test_rest_matches_the_ws_for_a_single_full_pack(tmp_path): + # Its ONE stem IS the mixdown: nothing to be pristine against, so `full` stays + # the sole playable stem and no separate mixdown is surfaced. + pak = _pak(tmp_path, [{"id": "full", "file": "stems/full.ogg"}], name="Single.feedpak") + rest = _payload(tmp_path, pak) + assert rest == _ws_payload(tmp_path, pak) + assert [s["id"] for s in rest["stems"]] == ["full"] + assert rest["full_mix_url"] is None + + +def test_a_broken_pack_yields_an_empty_list_not_an_error(tmp_path): + # Preloading is an optimisation: an unreadable pack must fall back to the + # normal WS-driven path, never break the song-info request. + from routers.song import _playable_stems_payload + import appstate + cache = tmp_path / "cache" + cache.mkdir() + appstate.sloppak_cache_dir = cache + (tmp_path / "bad.feedpak").write_bytes(b"not a zip") + assert _playable_stems_payload("bad.feedpak", tmp_path) == {"stems": [], "full_mix_url": None} From 917d81c2d2b319b541a53d63b9e24322d02336b6 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 00:36:16 +0200 Subject: [PATCH 2/9] fix(highway): a SUPERSEDED renderer init is not a FAILED one (#970) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting a gig dropped the player onto the fallback 2D highway with no venue. startGig() calls setViz('venue'), which installs the 3D renderer — whose init is async — and then immediately starts its play queue. playSong() re-initialises that same renderer a tick later. A renderer mints a fresh readyPromise per init() and rejects the previous one with "superseded"; highway.js only checked that the RENDERER OBJECT was unchanged, which it is. So it treated a healthy, re-initialising renderer as a failed one, tore it down, and reverted to 2D: renderer async init failure: Error: superseded viz picker: reverted to default renderer (async-init-failure) The guard now also checks the PROMISE identity: a rejection from an init cycle the renderer has already moved on from is ignored. The renderer-identity guard stays (a rejection for a renderer since REPLACED is also not ours), and a genuine failure of the CURRENT cycle still reverts — both init() call sites go through _setRenderer, which re-wires the handler every time, so the new cycle is always watched. Reproduced and fixed against the real build: before: vizSelection=default viz-picker=default venue=inactive viz:reverted after: vizSelection=venue viz-picker=venue venue=ACTIVE (no revert) Also widens the paused-frame throttle's opt-out. The throttle fires whenever the CHART CLOCK is stalled — not only on a pause, but through a count-in and the credits/author overlay too. Its opt-out only asked "is a crowd video rolling", but the venue scene animates on a clock of its own with no pack at all (backdrop breathe, parallax, haze drift, warmth pulse — Math.sin(t) in the draw loop), so that motion was still being throttled. It now claims frames for both sources; a plain 3D highway with no venue reads motion mode 'off' and keeps the #654 GPU saving. HONEST CAVEAT on that second part: I could not get the throttle to fire in a reproduction. A control run on the shipped code showed 100 draws/sec while paused, not the ~10/sec a firing throttle would give — so the change is defensible on its own terms (a stalled clock is genuinely not a static picture) but it does NOT have a demonstrated symptom behind it. The viz fix above does. Tests: the superseded guard, and that the throttle opt-out covers both motion sources. All fail against the pre-fix source. eslint 0 errors; JS 1207/1207. --- plugins/highway_3d/screen.js | 22 +++++++++-- static/highway.js | 16 ++++++++ tests/js/highway_pause_throttle.test.js | 49 ++++++++++++++++++++++--- 3 files changed, 78 insertions(+), 9 deletions(-) diff --git a/plugins/highway_3d/screen.js b/plugins/highway_3d/screen.js index 577726f..3bc2ff8 100644 --- a/plugins/highway_3d/screen.js +++ b/plugins/highway_3d/screen.js @@ -15398,15 +15398,29 @@ // highway throttled the whole room. Pausing the song dropped the // venue, the crowd and the stage to 10 fps. // - // Only claim continuous frames while a crowd video is actually - // rolling: with no venue pack (the common case) the paused scene IS - // static and the throttle should still save the GPU. + // Two independent sources of motion, and BOTH must keep their frames: + // + // • a crowd video rolling on its own clock (career venue pack), and + // • the venue scene's own fake-depth motion — the backdrop breathes, + // the haze drifts, warmth pulses, the shimmer moves. That is + // Math.sin(t) in the draw loop (see _venueApplyFakeDepthMotion), + // so it only moves while we are actually given frames, and it runs + // with NO pack at all. + // + // The throttle fires whenever the CHART CLOCK is stalled — which is + // not just a pause. A count-in and the credits/author overlay stall it + // exactly the same way, so the venue was stuttering there too. + // + // With no venue at all (plain 3D highway) the paused scene really is a + // still picture: motion mode reads 'off', we claim nothing, and the + // throttle still saves the GPU as #654 intended. needsContinuousFrames() { if (!_isReady || _ctxLost) return false; for (const v of _venueCrowdVideos) { if (v && !v.paused && !v.ended && v.readyState >= 2) return true; } - return false; + // 'off' also covers prefers-reduced-motion and "no venue scene". + try { return _venueEffectiveMotionMode() !== 'off'; } catch (_) { return false; } }, draw(bundle) { diff --git a/static/highway.js b/static/highway.js index cc74ae4..cb8811c 100644 --- a/static/highway.js +++ b/static/highway.js @@ -986,6 +986,22 @@ function createHighway() { // inline arrow function. function _handleAsyncInitFailure(e) { if (hwState._renderer !== _installedRenderer) return; + // ...and ignore a rejection from a SUPERSEDED init cycle. + // + // A renderer mints a fresh readyPromise on every init(), and + // rejects the previous one ("superseded") when a newer init + // starts. The renderer object is unchanged, so the identity + // check above does not catch it — and we would tear down a + // perfectly healthy renderer that is merely re-initialising. + // + // This is exactly what starting a gig did: setViz('venue') + // installed the 3D renderer, then the queue's playSong() + // re-initialised it a tick later; init #1's promise rejected, + // and the gig dropped to the fallback 2D highway with the + // venue gone. A superseded init is not a failed init — the + // NEW cycle owns the outcome, and its own promise is what we + // must judge. + if (_installedRenderer.readyPromise !== rp) return; console.error('renderer async init failure:', e); _destroyCurrentIfInited(); hwState._renderer = _defaultRenderer; diff --git a/tests/js/highway_pause_throttle.test.js b/tests/js/highway_pause_throttle.test.js index bc5ffbc..57baf51 100644 --- a/tests/js/highway_pause_throttle.test.js +++ b/tests/js/highway_pause_throttle.test.js @@ -114,13 +114,52 @@ test('the capability probe fails closed (absent / non-function / throwing)', () 'only an explicit true opts out — a truthy accident must not disable the throttle'); }); -test('3D highway claims continuous frames only while a crowd video is rolling', () => { +test('3D highway claims continuous frames for BOTH sources of venue motion', () => { const h3d = fs.readFileSync( path.join(__dirname, '..', '..', 'plugins', 'highway_3d', 'screen.js'), 'utf8'); const fn = extractBlock(h3d, 'needsContinuousFrames()'); + // (1) a crowd video rolling on its own clock (career venue pack) assert.match(fn, /_venueCrowdVideos/, 'must key off the actual crowd video elements'); - assert.match(fn, /\.paused/, 'a paused video is a still frame — throttle should still apply'); - // With no venue pack (the common case) the paused scene really is static and - // the GPU saving must survive: the method has to be able to return false. - assert.match(fn, /return false;/, 'must fall through to false with no live video'); + assert.match(fn, /\.paused/, 'a paused video is a still frame'); + // (2) the venue scene's OWN fake-depth motion — backdrop breathe, haze drift, + // warmth pulse, shimmer. Math.sin(t) in the draw loop, so it only moves while + // we get frames, and it runs with NO pack at all. Missing this meant the venue + // still stuttered on pause / count-in / credits whenever no video was rolling. + assert.match(fn, /_venueEffectiveMotionMode\s*\(\s*\)\s*!==\s*'off'/, + 'the venue scene animates without any video — it must claim frames too'); + // ...and with no venue at all the paused scene IS static: the #654 GPU saving + // must survive, so the method has to be able to return false. + assert.match(fn, /return false;/, 'must fall through to false on a plain 3D highway'); +}); + +// ── a SUPERSEDED init is not a FAILED init ────────────────────────────────── +// +// Starting a gig dropped the player onto the fallback 2D highway with no venue. +// +// setViz('venue') installs the 3D renderer, whose init is async; the gig then +// immediately starts its play queue, and playSong() re-initialises that same +// renderer a tick later. A renderer mints a fresh readyPromise per init() and +// rejects the previous one with "superseded" — but highway.js only checked that +// the RENDERER object was unchanged, which it is. So it treated a healthy +// re-initialising renderer as a failed one, tore it down, and reverted to 2D: +// +// renderer async init failure: Error: superseded +// viz picker: reverted to default renderer (async-init-failure) +// +// Reproduced and fixed against the real build (venue stays selected, scene +// active, no viz:reverted). + +test('a superseded readyPromise must not revert the viz to 2D', () => { + const src = highwaySources(); + const fn = extractBlock(src, 'function _handleAsyncInitFailure(e)'); + assert.match(fn, /readyPromise\s*!==\s*rp[\s\S]{0,40}return/, + 'a rejection from a STALE readyPromise (the renderer has since re-init\'d) must be ' + + 'ignored — otherwise a re-initialising renderer is torn down as if it had failed'); + // The renderer-identity check must survive too: a rejection belonging to a + // renderer that has since been REPLACED is also not our problem. + assert.match(fn, /hwState\._renderer\s*!==\s*_installedRenderer[\s\S]{0,20}return/, + 'the renderer-identity guard must remain'); + // ...and a genuine failure of the CURRENT init cycle must still revert. + assert.match(fn, /_emitVizReverted\s*\(\s*'async-init-failure'\s*\)/, + 'a real async-init failure must still fall back to the default renderer'); }); From 1702afa3793ee249625256410e4d53c269cb7d29 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 00:36:20 +0200 Subject: [PATCH 3/9] feat(career): extract the whole setlist before the gig starts (no more waiting between songs) (#971) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(career): extract the whole setlist before the gig starts A feedpak is a zip, and the first play of one pays for its extraction into sloppak_cache. Inside a set that cost landed BETWEEN songs: the player finished a number and then sat there waiting for the next one to unpack, mid-gig. A setlist is a known list up front, so unpack it all while the poster is still on screen. New POST /gigs/prepare walks the set through resolve_source_dir; the poster's Play button shows "Preparing set…" while it runs. Best-effort by design, at every level: - a corrupt pak in the set does not sink the prepare (it is reported in `failed`; the play itself surfaces the error exactly as it does outside a gig — slow beats blocked) - a host without the library resolvers degrades to a no-op rather than 500 - a failed request just falls through to the old lazy extraction Ordering matters and is pinned: the set is unpacked BEFORE the stage is borrowed (venue/viz overwritten) and before the queue starts, so a proposal cancelled while unpacking leaves nothing half-applied to unwind. Tests unpack REAL zips rather than mocking the extractor: every song of the set lands on disk before the first note, a re-prepare does not duplicate the unpack, one bad pak still leaves the good one prepared, and no-library / empty-setlist degrade cleanly. 18/18. NB the other half of the gig report — the per-song results popup interrupting the set (and worse, claimAutoExit'ing so the queue would not advance until it was dismissed) — is fixed in the note_detect plugin repo, which is not part of this checkout. * fix(career): bound the prepare request; validate the setlist (PR #971 review) Both CodeRabbit findings were right. 1. A HUNG PREPARE COULD BLOCK THE GIG FOREVER. `await fetch(...)` only rejects on a network ERROR. A server that accepts the connection and then never answers hangs indefinitely — and the gig would never start. That makes this optimisation the exact thing the PR promises it can never be: the reason you cannot play. The request is now bounded by an AbortController (PREPARE_TIMEOUT_MS, generous because unpacking a setlist is real work — but a CEILING, not a wait). Past it we start the gig and let the first play extract lazily, as it always did. The Play button is restored in a `finally`, so a timeout cannot strand the poster on "Preparing set…" with Play disabled — which would have been the same bug wearing a different hat. 2. THE `songs` BODY WAS UNVALIDATED. A str is iterable: "abc" would have prepared three one-character "songs". And the endpoint unpacks zips, so an arbitrary caller could ask for unbounded work. Now list-only, string entries, blanks dropped, capped at MAX_GIG_SONGS. Tests: the fetch is abortable and the button is re-enabled on EVERY path including the abort; non-list bodies, non-string/blank entries, and an oversized setlist. 50 career tests, JS 5/5, eslint clean. * fix(career): path-traversal guard on prepare; a cap test that actually tests the cap CodeRabbit again, and the first one is a real hole I put there. 1. PATH TRAVERSAL. sloppak.resolve_source_dir() does a bare `dlc_root / filename` with NO containment guard — so `../../x` walks straight out of the library, and my new endpoint handed it attacker-supplied filenames. Every filename now goes through _resolve_dlc_path first, the same check every other filename-bound handler applies. Pinned: `..`, backslash traversal, an absolute POSIX path and a Windows drive path are all refused, and nothing outside the library is unpacked. 2. THE CAP TEST WAS VACUOUS. It asserted `prepared == 0` against a fixture with no library — where the endpoint exits before extraction — so it passed whether or not MAX_GIG_SONGS existed. It now runs against a real library and asserts the endpoint CONSIDERED at most MAX_GIG_SONGS of the 82 it was handed. Verified to fail when the cap is removed. Same class of mistake as the notedetect gigBlock: a test that passes for the wrong reason. Worth saying out loud since it is twice in one day. 3. E702 — semicolon-joined statements in the new tests, split. 51 career tests; full suite green. --- plugins/career/routes.py | 61 ++++++++++ plugins/career/screen.js | 50 +++++++- tests/js/gig_song_end.test.js | 98 ++++++++++++++++ tests/plugins/career/test_routes.py | 173 ++++++++++++++++++++++++++++ 4 files changed, 380 insertions(+), 2 deletions(-) create mode 100644 tests/js/gig_song_end.test.js diff --git a/plugins/career/routes.py b/plugins/career/routes.py index 90be728..7f8b423 100644 --- a/plugins/career/routes.py +++ b/plugins/career/routes.py @@ -46,6 +46,8 @@ from pathlib import Path from fastapi import Body, HTTPException from fastapi.responses import FileResponse +import sloppak +from dlc_paths import _resolve_dlc_path from progression import instrument_for_arrangement PLUGIN_ID = "career" @@ -53,6 +55,9 @@ VENUE_ID_RE = re.compile(r"^[a-z0-9_-]{1,40}$") PACK_FILENAME_RE = re.compile(r"^[a-z0-9_-]{1,64}\.(mp4|webm|mp3|json)$") REQUIRED_LOOPS = ("bored", "neutral", "engaged", "ecstatic") DOWNLOAD_CHUNK = 1024 * 256 +# A setlist is a handful of songs; this endpoint unpacks zips, so cap the work an +# arbitrary caller can ask for. +MAX_GIG_SONGS = 32 _lock = threading.Lock() _state = { @@ -734,6 +739,62 @@ def setup(app, context): "snapshot": snapshot}) return {"ok": True} + @app.post(f"/api/plugins/{PLUGIN_ID}/gigs/prepare") + def prepare_gig(body: dict = Body(...)): + """Unpack every song of the set BEFORE the gig starts. + + A feedpak is a zip: the first play of one pays for its extraction into + sloppak_cache. Inside a set that cost landed BETWEEN songs — the player + finished a number and then sat waiting for the next one to unpack, mid- + gig. A set is a known list up front, so extract it all while the player + is still looking at the poster. + + Idempotent and cheap on a warm cache: resolve_source_dir() returns the + already-unpacked dir without rewriting it. Best-effort per song — one + bad feedpak must not block the set from starting (the play itself will + surface the error, exactly as it does outside a gig). + """ + raw = (body or {}).get("songs") + # A str is iterable: without the list check, "abc" would prepare three + # one-character "songs". Cap the count too — this endpoint unpacks zips, + # so an oversized list is real work, and a setlist is a handful of songs. + if not isinstance(raw, list): + return {"ok": True, "prepared": 0, "failed": []} + files = [f for f in raw if isinstance(f, str) and f.strip()][:MAX_GIG_SONGS] + if not files: + return {"ok": True, "prepared": 0, "failed": []} + + # .get, not []: a host that doesn't hand us the resolvers (or has no + # library configured) must degrade to "extract lazily, as before" — this + # is an optimisation, and it is never allowed to be the thing that stops + # a gig from starting. + get_dlc = context.get("get_dlc_dir") + get_cache = context.get("get_sloppak_cache_dir") + dlc_root = get_dlc() if callable(get_dlc) else None + cache_root = get_cache() if callable(get_cache) else None + if dlc_root is None or cache_root is None: + return {"ok": False, "prepared": 0, "failed": files, "error": "no library"} + + root = Path(dlc_root) + prepared, failed = 0, [] + for fn in files: + # CONTAINMENT FIRST. resolve_source_dir() does a bare + # `dlc_root / filename` with no guard, so a crafted `../..` would + # walk straight out of the library. Every other filename-bound + # handler validates through _resolve_dlc_path; so does this one. + safe = _resolve_dlc_path(root, fn) + if safe is None: + _state["log"].warning("career: gig pre-extract rejected unsafe path %r", fn) + failed.append(fn) + continue + try: + sloppak.resolve_source_dir(fn, root, Path(cache_root)) + prepared += 1 + except Exception as exc: # noqa: BLE001 — one bad pak can't sink the set + _state["log"].warning("career: gig pre-extract failed for %s: %s", fn, exc) + failed.append(fn) + return {"ok": True, "prepared": prepared, "failed": failed} + @app.post(f"/api/plugins/{PLUGIN_ID}/gigs/propose") def propose_gig(body: dict = Body(...)): inst = str((body or {}).get("instrument") or "") diff --git a/plugins/career/screen.js b/plugins/career/screen.js index c981625..7600735 100644 --- a/plugins/career/screen.js +++ b/plugins/career/screen.js @@ -12,6 +12,10 @@ 'use strict'; const API = '/api/plugins/career'; + // Unpacking a setlist is real work (zips, possibly on a slow/network drive), + // so this is generous — but it is a CEILING, not a wait. Past it we start the + // gig and let the first play extract lazily, as it always did. + const PREPARE_TIMEOUT_MS = 60000; const VENUE_OVERRIDE_KEY = 'feedBack-career-venue'; const NO_VENUE = '__none__'; const PREV_VIZ_KEY = 'feedBack-career-prev-viz'; @@ -1123,10 +1127,52 @@ sfx('page'); } - function startGig() { + // Unpack the whole set before the first note. + // + // A feedpak is a zip, and the first play of one pays for its extraction. In + // a set that cost landed BETWEEN songs: the player finished a number and + // then sat there waiting for the next one to unpack, mid-gig. The setlist is + // known up front, so warm it all while the poster is still on screen. + // + // Best-effort by design: a library that won't pre-extract must not stop the + // gig from starting — the play itself surfaces the error the same way it + // does outside a gig. Slow is better than blocked. + async function prepareGigSongs(prop, btn) { + const label = btn && btn.textContent; + if (btn) { btn.disabled = true; btn.textContent = 'Preparing set…'; } + // A bare `await fetch(...)` only rejects on a network ERROR — a server + // that accepts the connection and then never answers hangs forever, and + // the gig would never start. That would make this optimisation the very + // thing it promises never to be: the reason you cannot play. Give up + // waiting and let the first play extract lazily, exactly as before. + const ctrl = new AbortController(); + const timer = setTimeout(() => ctrl.abort(), PREPARE_TIMEOUT_MS); + try { + await fetch(`${API}/gigs/prepare`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ songs: prop.songs.map((s) => s.filename) }), + signal: ctrl.signal, + }); + } catch (_) { + // abort, offline, non-2xx — all the same: start the gig anyway. + } finally { + clearTimeout(timer); + if (btn) { btn.disabled = false; if (label) btn.textContent = label; } + } + } + + async function startGig(btn) { const prop = _ppGigProposal; const q = window.feedBack && window.feedBack.playQueue; if (!prop || !q || typeof q.start !== 'function' || typeof window.playSong !== 'function') return; + + // Extract the setlist BEFORE the stage is borrowed and the queue starts, + // so a failure here leaves nothing half-applied to unwind. + await prepareGigSongs(prop, btn); + // The poster's Play could have been cancelled while we were unpacking. + if (_ppGigProposal !== prop) return; + // The gig BORROWS the stage: stash whatever venue/viz the user had so // the set ending gives it back (unlike "Play here", which is an // explicit persistent choice on the venue card). @@ -1424,7 +1470,7 @@ } const gigBtn = e.target.closest('[data-pp-gig]'); if (gigBtn) { bookGig(gigBtn.dataset.ppGig); return; } - if (e.target.closest('[data-pp-gig-play]')) { startGig(); return; } + if (e.target.closest('[data-pp-gig-play]')) { startGig(e.target.closest('[data-pp-gig-play]')); return; } if (e.target.closest('[data-pp-gig-reroll]')) { if (_ppGigProposal) bookGig(_ppGigProposal.genre_key); return; diff --git a/tests/js/gig_song_end.test.js b/tests/js/gig_song_end.test.js new file mode 100644 index 0000000..40ab479 --- /dev/null +++ b/tests/js/gig_song_end.test.js @@ -0,0 +1,98 @@ +// A gig is a SET, not a run of unrelated songs. +// +// Reported from a live gig: the player finished the first song and had to sit +// through the per-song results popup before the next one would start, and then +// wait again while that song was extracted from its feedpak zip. +// +// This file covers the CORE half — career pre-extracts the whole setlist before +// the first note. The other half (note_detect must not show its per-song summary +// inside a gig) lives in the note_detect plugin repo, which is not part of this +// checkout: plugins/*/ is gitignored here and note_detect ships from +// feedBack-plugin-notedetect. A test reading it from core would pass on a dev +// box (where the plugin happens to be bundled) and fail in CI, which is worse +// than no test. +// +// The pre-extraction is tested for REAL behaviour — actually unpacking zips — in +// tests/plugins/career/test_routes.py. These are the wiring guards around it. + +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..', '..'); +const CAREER = fs.readFileSync(path.join(ROOT, 'plugins', 'career', 'screen.js'), 'utf8'); +const CAREER_ROUTES = fs.readFileSync(path.join(ROOT, 'plugins', 'career', 'routes.py'), '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('startGig extracts the whole setlist before starting the queue', () => { + const fn = extractBlock(CAREER, 'async function startGig('); + const prepIdx = fn.search(/await\s+prepareGigSongs\s*\(/); + const startIdx = fn.search(/q\.start\s*\(/); + assert.ok(prepIdx !== -1, 'startGig must pre-extract the set'); + assert.ok(startIdx !== -1, 'q.start not found'); + assert.ok(prepIdx < startIdx, + 'the set must be unpacked BEFORE the queue starts — otherwise the player ' + + 'waits between songs, which is the bug'); +}); + +test('the stage is only borrowed once the set is ready', () => { + const fn = extractBlock(CAREER, 'async function startGig('); + const prepIdx = fn.search(/await\s+prepareGigSongs\s*\(/); + const stageIdx = fn.search(/VENUE_OVERRIDE_KEY/); + assert.ok(prepIdx < stageIdx, + 'a gig cancelled while unpacking must not leave the venue/viz overwritten'); + assert.match(fn, /_ppGigProposal\s*!==\s*prop/, + 'a proposal dismissed while unpacking must not then start a gig'); +}); + +test('pre-extraction never blocks the gig from starting', () => { + const fn = extractBlock(CAREER, 'async function prepareGigSongs('); + assert.match(fn, /catch\s*\(/, + 'a failed prepare must fall through to the old lazy extraction, not abort the gig'); +}); + +test('the prepare route degrades instead of failing', () => { + assert.match(CAREER_ROUTES, /def prepare_gig/, 'prepare route missing'); + assert.match(CAREER_ROUTES, /context\.get\(\s*["']get_dlc_dir["']\s*\)/, + 'a host without the library resolvers must degrade, not 500 — pre-extraction ' + + 'is an optimisation and can never be why a gig will not start'); +}); + +// ── the prepare must never be able to BLOCK the gig (CodeRabbit, #971) ────── +// +// A bare `await fetch(...)` only rejects on a network error. A server that +// accepts the connection and then never answers hangs forever — and the gig +// would never start. That would make this optimisation the exact thing it +// promises never to be: the reason you cannot play. + +test('the prepare fetch is bounded — a hung server cannot block the gig', () => { + const fn = extractBlock(CAREER, 'async function prepareGigSongs('); + assert.match(fn, /AbortController/, 'the request must be abortable'); + assert.match(fn, /setTimeout\([\s\S]{0,40}abort\s*\(\s*\)/, + 'a hung request must be aborted, not awaited forever'); + assert.match(fn, /signal:\s*ctrl\.signal/, 'the signal must actually be passed to fetch'); + assert.match(fn, /clearTimeout/, 'the timer must be cleared on the happy path'); + assert.match(CAREER, /const\s+PREPARE_TIMEOUT_MS\s*=\s*\d+/, 'the ceiling must be named'); + // The button must be restored however we leave — otherwise a timeout strands + // the poster on "Preparing set…" with Play disabled: unplayable. + assert.match(fn, /finally\s*\{[\s\S]{0,220}btn\.disabled\s*=\s*false/, + 'the Play button must be re-enabled on EVERY path, including the abort'); +}); diff --git a/tests/plugins/career/test_routes.py b/tests/plugins/career/test_routes.py index e1ed626..1830f7a 100644 --- a/tests/plugins/career/test_routes.py +++ b/tests/plugins/career/test_routes.py @@ -174,3 +174,176 @@ def test_double_download_409s(client, monkeypatch): career_routes._state["downloads"]["bar"] = {"status": "running"} assert client.post("/api/plugins/career/packs/bar/download").status_code == 409 assert client.delete("/api/plugins/career/packs/bar").status_code == 409 + + +# ── gig pre-extraction (the wait between songs) ───────────────────────────── +# +# A feedpak is a zip: the first play of one pays for its extraction into +# sloppak_cache. Inside a set that cost landed BETWEEN songs — the player +# finished a number and then sat waiting for the next one to unpack, mid-gig. +# The setlist is known up front, so extract it all while the poster is up. + +def _career_client_with_library(tmp_path, meta_db, dlc, cache): + from fastapi import FastAPI + from fastapi.testclient import TestClient + import routes as career_routes + app = FastAPI() + career_routes.setup(app, { + "config_dir": str(tmp_path), + "meta_db": meta_db, + "get_dlc_dir": lambda: dlc, + "get_sloppak_cache_dir": lambda: cache, + }) + return TestClient(app) + + +def _write_feedpak(dlc, name, title="T"): + """A minimal but REAL feedpak zip, so resolve_source_dir genuinely unpacks.""" + import json as _json + import zipfile as _zip + p = dlc / name + with _zip.ZipFile(p, "w") as z: + z.writestr("manifest.json", _json.dumps({"title": title, "artist": "A", "arrangements": []})) + return p + + +def test_gig_prepare_extracts_every_song_up_front(tmp_path, meta_db): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + for n in ("one.feedpak", "two.feedpak", "three.feedpak"): + _write_feedpak(dlc, n) + + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + before = list(cache.iterdir()) + assert before == [], "nothing unpacked yet" + + res = client.post("/api/plugins/career/gigs/prepare", + json={"songs": ["one.feedpak", "two.feedpak", "three.feedpak"]}) + assert res.status_code == 200 + body = res.json() + assert body["ok"] is True + assert body["prepared"] == 3, body + assert body["failed"] == [] + # The point of the whole exercise: the set is on disk BEFORE the first note. + assert len(list(cache.iterdir())) == 3, "every song of the set must be unpacked" + + +def test_gig_prepare_is_idempotent_on_a_warm_cache(tmp_path, meta_db): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _write_feedpak(dlc, "one.feedpak") + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + + first = client.post("/api/plugins/career/gigs/prepare", json={"songs": ["one.feedpak"]}).json() + second = client.post("/api/plugins/career/gigs/prepare", json={"songs": ["one.feedpak"]}).json() + assert first["prepared"] == second["prepared"] == 1 + assert len(list(cache.iterdir())) == 1, "a re-prepare must not duplicate the unpack" + + +def test_one_bad_feedpak_does_not_stop_the_set(tmp_path, meta_db): + # A corrupt pak in the setlist must not block the gig: the play itself will + # surface the error exactly as it does outside a gig. Slow beats blocked. + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _write_feedpak(dlc, "good.feedpak") + (dlc / "bad.feedpak").write_bytes(b"not a zip at all") + + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": ["good.feedpak", "bad.feedpak"]}).json() + assert body["ok"] is True, "a bad pak must not fail the whole prepare" + assert body["prepared"] == 1 + assert body["failed"] == ["bad.feedpak"] + + +def test_gig_prepare_degrades_without_a_library(tmp_path, meta_db, client): + # The stock fixture's context has no dlc/cache resolvers. That must be a + # graceful no-op, not a 500 — pre-extraction is an optimisation and can + # never be the reason a gig won't start. + res = client.post("/api/plugins/career/gigs/prepare", json={"songs": ["x.feedpak"]}) + assert res.status_code == 200 + assert res.json()["prepared"] == 0 + + +def test_gig_prepare_empty_setlist(tmp_path, meta_db, client): + res = client.post("/api/plugins/career/gigs/prepare", json={"songs": []}) + assert res.status_code == 200 + assert res.json() == {"ok": True, "prepared": 0, "failed": []} + + +def test_prepare_rejects_a_non_list_songs_value(tmp_path, meta_db, client): + # A str is iterable: without the list check, "abc" would prepare three + # one-character "songs". + for bad in ("abc", 42, {"a": 1}, None): + res = client.post("/api/plugins/career/gigs/prepare", json={"songs": bad}) + assert res.status_code == 200 + assert res.json()["prepared"] == 0 + + +def test_prepare_ignores_non_string_and_blank_entries(tmp_path, meta_db): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _write_feedpak(dlc, "good.feedpak") + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": ["good.feedpak", "", " ", 7, None, {"x": 1}]}).json() + assert body["prepared"] == 1 + assert body["failed"] == [] + + +def test_prepare_caps_the_setlist(tmp_path, meta_db): + # This endpoint unpacks zips — an arbitrary caller must not be able to ask for + # unbounded work. + # + # The first version of this test asserted `prepared == 0` against a fixture + # with NO library: the endpoint exits before extraction there, so it passed + # whether or not the cap existed. Give it a real library, ask for far more than + # the cap, and assert the endpoint only ever considered MAX_GIG_SONGS of them. + import routes as career_routes + assert career_routes.MAX_GIG_SONGS <= 64 + + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + + n = career_routes.MAX_GIG_SONGS + 50 + # None of these exist, so every song the endpoint LOOKS AT lands in `failed`. + # That makes `failed` an exact count of how many it considered. + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": [f"missing{i}.feedpak" for i in range(n)]}).json() + assert body["prepared"] == 0 + assert len(body["failed"]) == career_routes.MAX_GIG_SONGS, ( + f"the endpoint must consider at most MAX_GIG_SONGS " + f"({career_routes.MAX_GIG_SONGS}), not all {n}" + ) + + +def test_prepare_refuses_to_escape_the_library(tmp_path, meta_db): + # resolve_source_dir() does a bare `dlc_root / filename` with no containment + # guard, so a crafted path would walk straight out of the library. Every + # filename must go through _resolve_dlc_path first. + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + (tmp_path / "outside.feedpak").write_bytes(b"secret") + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + + for evil in ("../outside.feedpak", "..\\outside.feedpak", + "a/../../outside.feedpak", "/etc/passwd", "C:/Windows/x.feedpak"): + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": [evil]}).json() + assert body["prepared"] == 0, f"{evil!r} must never be prepared" + assert body["failed"] == [evil] + # Nothing outside the library may have been unpacked. + assert list(cache.iterdir()) == [] From 365cec1d29e476b36fd923476538f7bb47b312fd Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 10:50:55 +0200 Subject: [PATCH 4/9] =?UTF-8?q?fix(career):=20gig=20song=20selection=20?= =?UTF-8?q?=E2=80=94=20full-genre=20pool,=20working=20re-roll,=20and=20the?= =?UTF-8?q?=20venue=20pack=20loads=20(#976)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(career): a gig's song pool is the whole genre, and re-roll varies it Two tester reports, one root: the gig song pool was built from only two sets — songs played ON THIS PASSPORT'S INSTRUMENT, and songs never played AT ALL (`filename NOT IN song_stats`). A song played on a DIFFERENT instrument's arrangement is in neither: it has a stats row (so the "unplayed" filler skipped it), and its played bucket is that other instrument's, not this passport's. It could never be gigged. - "Metalcore says 137 songs only shows 1 in the gig list" — a library of metalcore all played on another instrument. Reproduced: a guitar passport with 137 bass-played metalcore songs got a 404, zero songs. The "1" the tester saw was whatever handful happened to be on-instrument or truly unplayed. - "Passport re-roll does not change songs" — a set drawn from that filler was the library's first N in table ORDER, every call. Re-roll re-proposes, so it returned the identical set. Reproduced: 3 proposals, byte-identical. _unplayed_genre_songs -> _fill_genre_songs: the pool is now every library song of the genre the set hasn't already picked (a stats row on some other instrument has no bearing on whether a song can be in THIS gig), and it is shuffled so re-roll actually re-rolls. Both reproduced against the real propose logic before the fix and pinned as regression tests (both fail on the pre-fix routes.py). Full career suite green. * fix(career): load the gig's venue pack when the gig starts Tester: "Venue doesn't load when starting song from passport. Loads standard particles." crowd.setManifest(venue) — the call that actually loads a venue's crowd/stage pack — is reached ONLY through pushCrowdManifest, and pushCrowdManifest is called ONLY from refresh(), the career tab's own reload. A gig navigates AWAY from the career tab to the player, so refresh() never runs during it. startGig set the venue override and nulled _appliedManifestVenue but never re-pushed, so the venue visualization turned on (3D highway) while its pack never loaded — the song played over the bare highway backdrop, or over whatever venue a previous refresh() had left applied. startGig now pushes the crowd manifest for the gig venue right after setting the override, using the career state the booking screen already fetched. This is a call-graph fact, not a guess (pushCrowdManifest has exactly one other caller and startGig is not it), but it is fixed by static analysis — I could not reproduce the user-visible symptom locally because this instance happened to have a manifest already applied from a prior refresh. On-device confirmation on a real passport gig is still owed. Guard test: startGig must push the manifest after setting the override (fails on the pre-fix source). Career suite green. --- plugins/career/routes.py | 46 ++++++++++++++++---------- plugins/career/screen.js | 14 ++++++++ tests/js/career_plugin.test.js | 27 +++++++++++++++ tests/plugins/career/test_passports.py | 27 +++++++++++++++ 4 files changed, 97 insertions(+), 17 deletions(-) diff --git a/plugins/career/routes.py b/plugins/career/routes.py index 7f8b423..7ec4a17 100644 --- a/plugins/career/routes.py +++ b/plugins/career/routes.py @@ -522,27 +522,39 @@ def _current_venue(): return best -def _unplayed_genre_songs(gkey, exclude, limit): - """Library songs of a genre with no stats yet — a young passport's gig - still gets a full set (playing them is how stubs start). - ponytail: full stat-less scan + python-side genre match (a few ms at 7k - songs, single-user); push the match into SQL if propose ever feels slow.""" +def _fill_genre_songs(gkey, exclude, limit): + """Library songs of a genre to round out a gig — ANY song of the genre the + set hasn't already picked. + + Was `_unplayed_genre_songs`, restricted to `filename NOT IN song_stats`. + That restriction created a hole: a song you'd played on a DIFFERENT + instrument's arrangement has a stats row, so it was excluded here — and it + lives in the played bucket for THAT instrument, not this passport's, so it + was excluded there too. It could never be gigged. A player with 137 metalcore + songs, all played on another instrument, got a 404 (reproduced). The player's + library is the pool; whether a song has stats on some other instrument has no + bearing on whether it can be in THIS gig. + + Shuffled, so re-roll actually changes the set. The old version returned the + library's first N in table order every time, so re-roll was a no-op for any + set drawn from the filler (reproduced). + + ponytail: full genre scan + python-side match + shuffle (a few ms at 7k + songs, single-user); push into SQL if propose ever feels slow. + """ db = _state["meta_db"] if db is None: return [] rows = db.conn.execute( - f"SELECT filename, title, artist, {_genre_expr(db)} AS g FROM songs " - "WHERE filename NOT IN (SELECT filename FROM song_stats)" + f"SELECT filename, title, artist, {_genre_expr(db)} AS g FROM songs" ).fetchall() - out = [] - for filename, title, artist, genre in rows: - if _genre_key(genre) != gkey or filename in exclude: - continue - out.append({"filename": filename, "title": title or filename, - "artist": artist or ""}) - if len(out) >= limit: - break - return out + pool = [ + {"filename": filename, "title": title or filename, "artist": artist or ""} + for filename, title, artist, genre in rows + if _genre_key(genre) == gkey and filename not in exclude + ] + random.shuffle(pool) # re-roll must vary; free per call + return pool[:limit] def _validate_pack_dir(pack_dir: Path): @@ -836,7 +848,7 @@ def setup(app, context): picks.append(s) if len(picks) < size: exclude = {s["filename"] for s in picks} - picks.extend(_unplayed_genre_songs(gkey, exclude, size - len(picks))) + picks.extend(_fill_genre_songs(gkey, exclude, size - len(picks))) if not picks: raise HTTPException(404, "No songs of this genre in the library.") venue = _current_venue() diff --git a/plugins/career/screen.js b/plugins/career/screen.js index 7600735..4e47db1 100644 --- a/plugins/career/screen.js +++ b/plugins/career/screen.js @@ -1192,7 +1192,21 @@ if (typeof window.setViz === 'function') window.setViz('venue'); } catch (_) { /* viz optional — restore stays intact */ } } + // Push the gig's venue pack to the crowd layer NOW. + // + // crowd.setManifest(venue) is reached only through pushCrowdManifest, + // and pushCrowdManifest is called only from refresh() — the career + // tab's own reload. A gig navigates AWAY from the career tab to the + // player, so refresh() never runs during it, and setting the override + // above does nothing on its own. The result the testers saw: the venue + // visualization turns on (3D highway) but its crowd/stage pack never + // loads, so the song plays over the bare highway backdrop ("standard + // particles"), or over whatever venue a previous refresh() happened to + // leave applied. We just changed the override to this gig's venue, so + // re-push for it. _state is the career state the booking screen already + // fetched; guard for the rare null. _appliedManifestVenue = null; + if (_state) pushCrowdManifest(_state); _ppGigRun = { songs: prop.songs, venue_id: prop.venue_id, diff --git a/tests/js/career_plugin.test.js b/tests/js/career_plugin.test.js index f038727..d7f0405 100644 --- a/tests/js/career_plugin.test.js +++ b/tests/js/career_plugin.test.js @@ -116,3 +116,30 @@ test('career screen pushes the crowd manifest with a base URL', () => { // Degrades without the crowd layer (PR1 not merged / older desktop). assert.match(src, /typeof crowd\.setManifest !== 'function'\) return/); }); + +// feedBack#… (tester): "Venue doesn't load when starting song from passport. +// Loads standard particles." crowd.setManifest(venue) is reached ONLY through +// pushCrowdManifest, and pushCrowdManifest is called ONLY from refresh() (the +// career tab's own reload). A gig navigates away from that tab, so refresh() +// never runs during it — the venue viz turns on but its crowd/stage pack never +// loads. startGig must push the manifest itself after setting the override. +test('startGig pushes the crowd manifest for the gig venue', () => { + const fs = require('node:fs'); + const path = require('node:path'); + const src = fs.readFileSync( + path.join(__dirname, '..', '..', 'plugins', 'career', 'screen.js'), 'utf8'); + const start = src.indexOf('async function startGig('); + assert.ok(start !== -1, 'startGig not found'); + const open = src.indexOf('{', src.indexOf(')', start)); + let depth = 1, i = open + 1; + while (i < src.length && depth > 0) { const ch = src[i]; if (ch === '{') depth++; else if (ch === '}') depth--; i++; } + const fn = src.slice(start, i); + // The override is set, then the manifest must be (re)pushed for it. + const overrideIdx = fn.search(/VENUE_OVERRIDE_KEY,\s*prop\.venue_id/); + const pushIdx = fn.search(/pushCrowdManifest\s*\(/); + assert.ok(overrideIdx !== -1, 'startGig must set the venue override'); + assert.ok(pushIdx !== -1, + 'startGig must push the crowd manifest — refresh() (its only other caller) ' + + 'never runs during a gig, so the venue pack would never load'); + assert.ok(overrideIdx < pushIdx, 'the manifest must be pushed AFTER the override is set to the gig venue'); +}); diff --git a/tests/plugins/career/test_passports.py b/tests/plugins/career/test_passports.py index 8687565..5b09a23 100644 --- a/tests/plugins/career/test_passports.py +++ b/tests/plugins/career/test_passports.py @@ -445,3 +445,30 @@ def test_gold_intake_rejects_junk(client, meta_db): res = client.post("/api/plugins/career/drill-state", json={"byNode": {}, "goldImprov": blob}) assert res.status_code == 413 + + +def test_gig_includes_songs_played_on_another_instrument(client, meta_db): + # feedBack#… (tester): "Metalcore says 137 songs, only shows 1 in the gig list". + # A song played on a DIFFERENT instrument's arrangement has a stats row, so it + # was excluded from the unplayed filler — and its played bucket is that other + # instrument's, not this passport's — so it fell into a gap and could never be + # gigged. A guitar passport with a library of bass-played metalcore got a 404. + for i in range(137): + meta_db.add(f"mc{i}.feedpak", 0, 0.80, genre="Metalcore", arrangements=BASS) + res = client.post("/api/plugins/career/gigs/propose", + json={"instrument": "guitar", "genre": "Metalcore", "size": 4}) + assert res.status_code == 200, "a full library of the genre must never 404" + assert len(res.json()["songs"]) == 4, "the gig must fill from the library, not the gap" + + +def test_gig_reroll_changes_the_set(client, meta_db): + # feedBack#… (tester): "Passport re-roll does not change songs". A set drawn + # from the filler used to be the library's first N in table order, every time. + for i in range(40): + meta_db.add_song_only(f"un{i}.feedpak", genre="Metalcore") + sets = set() + for _ in range(5): + r = client.post("/api/plugins/career/gigs/propose", + json={"instrument": "guitar", "genre": "Metalcore", "size": 4}) + sets.add(tuple(sorted(s["filename"] for s in r.json()["songs"]))) + assert len(sets) > 1, "re-roll must be able to produce a different set" From e14ef64224dc1045f5668a33f817655b7cf22dbd Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 10:50:59 +0200 Subject: [PATCH 5/9] fix(playback): the song queue must survive a playSong wrapper that drops options (#977) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tester: "Passports does not advance in the song queue." The play queue tells playSong "don't clear the queue I'm driving" by passing options.fromQueue. But window.playSong is wrapped by a CHAIN of plugins — nam_tone, midi_amp, fretboard, invert_highway, tabview — and each wrapper forwards only (filename, arrangement), silently dropping the options object. So fromQueue never reached playSong: it cleared the queue the instant its first song started, and a gig/album/playlist never advanced. Reproduced on the real build via a queue.start + a hooked clear(): the queue went inactive with 0 remaining immediately after start, and the clear stack ran through nam_tone -> midi_amp -> invert_highway -> fretboard -> session.js. Fixing six plugin wrappers is whack-a-mole and the next plugin re-breaks it. Fix it at the source instead: the queue raises an out-of-band flag (_consumeInternalPlay, one-shot) beside the wrapper chain, not through it, and playSong's clear-guard honours it. options.fromQueue stays as the in-band path. The flag is consumed on read so a later MANUAL play still abandons the queue. Verified on the real build: the gig queue stays active after start and advances on song:ended (Iron Maiden -> Blind Guardian), and a manual play still clears. Tests drive the real clear-guard against the queue for: a dropped-options wrapper (the bug), the one-shot manual-play-still-clears invariant, and the in-band fromQueue path on its own. All 3 fail on the pre-fix source. JS 1211/1211. --- static/app.js | 19 +++++++++- static/js/session.js | 15 ++++++-- tests/js/play_queue_peek.test.js | 60 ++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/static/app.js b/static/app.js index 7ea343f..3a72d42 100644 --- a/static/app.js +++ b/static/app.js @@ -1334,12 +1334,25 @@ if (window.feedBack) window.feedBack.closeCurrentSong = closeCurrentSong; // leaving the player still leaves — and abandons the queue. window.feedBack.playQueue = (function () { let list = [], idx = -1, source = '', arrangements = null; + // Set true by _play() right before it drives playSong, consumed once by + // playSong's clear-guard. The primary "don't clear the queue I'm driving" + // signal is options.fromQueue, but a chain of plugin playSong wrappers + // (nam_tone, midi_amp, fretboard, invert_highway, tabview, ...) forward only + // (filename, arrangement) and silently drop the options object — so the flag + // never arrived and the queue cleared itself the instant its first song + // started (a gig/album/playlist never advanced). This flag rides beside the + // wrapper chain, not through it. + let _internalPlay = false; const active = () => idx >= 0 && idx < list.length; const hasNext = () => active() && idx < list.length - 1; function clear() { list = []; idx = -1; source = ''; arrangements = null; } function _play(i) { const fn = list[i]; - // fromQueue keeps the queue from clearing itself; playSong decodeURIs. + // fromQueue is the in-band signal; _internalPlay is the out-of-band one + // that survives wrapper chains dropping the options arg. Both set; either + // suffices. playSong runs its clear-guard synchronously at entry, and the + // wrapper chain reaches it synchronously, so the flag is still set then. + _internalPlay = true; window.playSong(encodeURIComponent(fn), arrangements ? arrangements[i] : undefined, { fromQueue: true }); } function start(files, opts) { @@ -1371,6 +1384,10 @@ window.feedBack.playQueue = (function () { } return { start: start, advance: advance, hasNext: hasNext, active: active, clear: clear, + // One-shot: true iff _play just kicked off this playSong. Consumed on + // read so a later MANUAL play still clears the queue. playSong calls this + // instead of trusting options.fromQueue to survive the wrapper chain. + _consumeInternalPlay: function () { const v = _internalPlay; _internalPlay = false; return v; }, source: function () { return source; }, remaining: function () { return active() ? list.length - idx - 1 : 0; }, // What's coming, for consumers that RENDER the queue (a results diff --git a/static/js/session.js b/static/js/session.js index 5d6daa2..4e56e0b 100644 --- a/static/js/session.js +++ b/static/js/session.js @@ -638,9 +638,18 @@ export let artAbortController = null; export async function playSong(filename, arrangement, options) { console.log('playSong called:', filename); // A manual (non-queue) play abandons any active play-queue, so a stale queue - // can't hijack the next song's end. The queue passes fromQueue to keep itself. - if ((!options || !options.fromQueue) && window.feedBack && window.feedBack.playQueue) { - window.feedBack.playQueue.clear(); + // can't hijack the next song's end. The queue signals a play it is DRIVING + // two ways: options.fromQueue (in-band) and _consumeInternalPlay() (out-of- + // band). The out-of-band one exists because plugin playSong wrappers forward + // only (filename, arrangement) and drop the options object — with just the + // in-band flag, the queue cleared itself the instant its first song played + // and a gig never advanced. Consume the flag whether or not we go on to clear, + // so it can't leak into a later manual play. + const _pq = window.feedBack && window.feedBack.playQueue; + const _queueDriven = (options && options.fromQueue) + || (_pq && typeof _pq._consumeInternalPlay === 'function' && _pq._consumeInternalPlay()); + if (!_queueDriven && _pq) { + _pq.clear(); } if (!options || options.bridge !== false) { _recordPlaybackBridge('playback.window-play-song', 'window.playSong', 'legacy playSong entry point used'); diff --git a/tests/js/play_queue_peek.test.js b/tests/js/play_queue_peek.test.js index 8f0cbbc..57d1e66 100644 --- a/tests/js/play_queue_peek.test.js +++ b/tests/js/play_queue_peek.test.js @@ -49,3 +49,63 @@ test('peekNext is null after clear', () => { q.clear(); assert.strictEqual(q.peekNext(), null); }); + +// A gig/album/playlist queue must survive a playSong wrapper that drops the +// options object. +// +// The queue tells playSong "don't clear the queue I'm driving" via +// options.fromQueue. But a chain of plugin playSong wrappers (nam_tone, +// midi_amp, fretboard, invert_highway, tabview, ...) forward only +// (filename, arrangement) and silently drop the 3rd arg. With just the in-band +// flag, playSong cleared the queue the instant its first song started, so a gig +// never advanced (feedBack#… tester: "Passports does not advance in the song +// queue"). The queue now also raises an out-of-band flag, _consumeInternalPlay(), +// which playSong honours regardless of the wrapper chain. + +// The real clear-guard from session.js, driven against the queue. +function clearGuard(win, options) { + const pq = win.feedBack && win.feedBack.playQueue; + const queueDriven = (options && options.fromQueue) + || (pq && typeof pq._consumeInternalPlay === 'function' && pq._consumeInternalPlay()); + if (!queueDriven && pq) pq.clear(); +} + +test('the queue survives a playSong that drops the options arg', () => { + const { q } = makeQueue(); + // Rebind the queue's window.playSong to a wrapper that forwards ONLY + // (filename, arrangement) — exactly the plugin bug — and runs the real guard. + const win = { feedBack: { playQueue: q } }; + // Reach the same window the IIFE closed over: re-drive through the guard by + // calling start and simulating what _play's playSong does. + // We can't rebind the closed-over window, so instead assert the out-of-band + // signal directly: _play sets it, and the guard consumes it. + q.start(['a.sloppak', 'b.sloppak', 'c.sloppak'], { source: 'gig' }); + // After start()->_play, the internal flag was set; the guard (which the real + // playSong runs) must see it as queue-driven and NOT clear. + win.feedBack.playQueue = q; + clearGuard(win, undefined /* wrapper dropped options */); + assert.strictEqual(q.active(), true, 'a dropped options arg must not clear the queue'); + assert.strictEqual(q.remaining(), 2, 'the queue must still have its remaining tracks'); +}); + +test('_consumeInternalPlay is one-shot — a later MANUAL play still clears', () => { + const { q } = makeQueue(); + q.start(['a.sloppak', 'b.sloppak'], { source: 'album' }); + const win = { feedBack: { playQueue: q } }; + // First guard call (the queue's own play) consumes the flag → no clear. + clearGuard(win, undefined); + assert.strictEqual(q.active(), true); + // A subsequent MANUAL play (no fromQueue, flag already consumed) must clear. + clearGuard(win, undefined); + assert.strictEqual(q.active(), false, 'a manual play after the queue play must abandon the queue'); +}); + +test('fromQueue in options still works on its own (in-band path)', () => { + const { q } = makeQueue(); + q.start(['a.sloppak', 'b.sloppak'], { source: 'gig' }); + // consume the internal flag first so ONLY options.fromQueue is under test + q._consumeInternalPlay(); + const win = { feedBack: { playQueue: q } }; + clearGuard(win, { fromQueue: true }); + assert.strictEqual(q.active(), true, 'options.fromQueue alone must still keep the queue'); +}); From 2f2a095e4cd864ef4ca76db3ad7dfff3d04356b5 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 12:39:25 +0200 Subject: [PATCH 6/9] fix(venue): fly in once per set, not before every song (#978) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tester, mid-gig: "the second song in the gig started when the first one ended. But it showed the flyover intro again." The flyover is arriving at the venue, and you arrive once. #968 stopped it replaying on an arrangement SWITCH (same filename), but a gig's song 2 is a genuinely different file, so it took the full-teardown path and played the arrival flyover again — the camera flew in from the back of the room before every track of the set. The play queue now answers isContinuation(): false for the first song of a set (or a standalone play — an arrival), true for song 2..N. onSongLoaded carries the room over to the new song's loop on a continuation, and only a real arrival plays the intro. Verified on the built AppImage: isContinuation goes false (song 1) -> true (song 2) across an advance, and song 2 no longer flies in. Also confirmed NOT a bug, same session: "didn't show the author for the second song." The credits card shows on a queue advance whenever the song carries authors — reproduced with a song that has them as the advanced-to track. The tester's song 2 simply had no `authors:` metadata (most auto-converted feedpaks don't). No code change. Tests: isContinuation across start/advance/clear, and that onSongLoaded gates the flyover on the continuation check. Both fail on pre-fix source. JS 1214/1214. --- static/app.js | 5 +++++ static/v3/venue-crowd.js | 22 +++++++++++++++++++++- tests/js/play_queue_peek.test.js | 17 +++++++++++++++++ tests/js/venue_scope.test.js | 19 +++++++++++++++++++ 4 files changed, 62 insertions(+), 1 deletion(-) diff --git a/static/app.js b/static/app.js index 3a72d42..8d60608 100644 --- a/static/app.js +++ b/static/app.js @@ -1384,6 +1384,11 @@ window.feedBack.playQueue = (function () { } return { start: start, advance: advance, hasNext: hasNext, active: active, clear: clear, + // True when the current song is a queue ADVANCE (song 2..N of a set), + // false for its first song or a standalone play. The venue uses this to + // fly in once on arrival at the set, then continue the room between + // songs instead of replaying the arrival flyover every track. + isContinuation: function () { return active() && idx > 0; }, // One-shot: true iff _play just kicked off this playSong. Consumed on // read so a later MANUAL play still clears the queue. playSong calls this // instead of trusting options.fromQueue to survive the wrapper chain. diff --git a/static/v3/venue-crowd.js b/static/v3/venue-crowd.js index fbb2c5d..a0a8506 100644 --- a/static/v3/venue-crowd.js +++ b/static/v3/venue-crowd.js @@ -529,7 +529,27 @@ _loadingLoop = null; _fadingLoop = null; if (_venueActive && _manifest) { - if (!playIntro()) showLoop(machine.current, FADE_MS); + // The flyover is ARRIVING at the venue, and you arrive once. Songs + // 2..N of a set (a gig / album / playlist) are a NEW song but the + // SAME arrival — the camera should not fly in from the back of the + // room before every track (tester: "it showed the flyover intro + // again" on a gig's second song). Continue the room to the new song's + // loop; only a first-song / standalone arrival flies in. + if (_isSetContinuation()) showLoop(machine.current, FADE_MS); + else if (!playIntro()) showLoop(machine.current, FADE_MS); + } + } + + // Is this song load a continuation of a play queue (a set already in + // progress), rather than an arrival? True for song 2..N of a gig/album/ + // playlist. The queue owns the answer; treat any error / absent queue as + // "not a continuation" so a standalone play still flies in. + function _isSetContinuation() { + try { + const q = window.feedBack && window.feedBack.playQueue; + return !!(q && typeof q.isContinuation === 'function' && q.isContinuation()); + } catch (_) { + return false; } } diff --git a/tests/js/play_queue_peek.test.js b/tests/js/play_queue_peek.test.js index 57d1e66..0abe2ca 100644 --- a/tests/js/play_queue_peek.test.js +++ b/tests/js/play_queue_peek.test.js @@ -109,3 +109,20 @@ test('fromQueue in options still works on its own (in-band path)', () => { clearGuard(win, { fromQueue: true }); assert.strictEqual(q.active(), true, 'options.fromQueue alone must still keep the queue'); }); + +// isContinuation(): true for song 2..N of a set, false for the first song / a +// standalone play. The venue uses it to fly in once on arrival, then carry the +// room between songs instead of replaying the arrival flyover every track +// (tester: "it showed the flyover intro again" on a gig's second song). +test('isContinuation is false on the first song, true after advancing', () => { + const { q } = makeQueue(); + assert.strictEqual(q.isContinuation(), false, 'idle queue is not a continuation'); + q.start(['a.sloppak', 'b.sloppak', 'c.sloppak'], { source: 'gig' }); + assert.strictEqual(q.isContinuation(), false, 'the FIRST song of a set is an arrival, not a continuation'); + q.advance(); + assert.strictEqual(q.isContinuation(), true, 'song 2 is a continuation — no re-flyover'); + q.advance(); + assert.strictEqual(q.isContinuation(), true, 'song 3 too'); + q.clear(); + assert.strictEqual(q.isContinuation(), false, 'a cleared queue is not a continuation'); +}); diff --git a/tests/js/venue_scope.test.js b/tests/js/venue_scope.test.js index af9564b..79f9ba3 100644 --- a/tests/js/venue_scope.test.js +++ b/tests/js/venue_scope.test.js @@ -117,3 +117,22 @@ test('a throwing document does not take the venue down with it', () => { global.document = prev; } }); + +// The arrival flyover must NOT replay for songs 2..N of a set. onSongLoaded +// consults the play queue: a continuation (gig/album/playlist song 2+) carries +// the room over with a loop crossfade, only an arrival plays the intro. +test('a set continuation carries the room over instead of re-flying-in', () => { + const fs = require('node:fs'); + const path = require('node:path'); + const src = fs.readFileSync(path.join(__dirname, '..', '..', 'static', 'v3', 'venue-crowd.js'), 'utf8'); + const start = src.indexOf('function onSongLoaded('); + const open = src.indexOf('{', src.indexOf(')', start)); + let depth = 1, i = open + 1; + while (i < src.length && depth > 0) { const ch = src[i]; if (ch === '{') depth++; else if (ch === '}') depth--; i++; } + const fn = src.slice(start, i); + const contIdx = fn.search(/_isSetContinuation\s*\(\s*\)/); + const introIdx = fn.search(/playIntro\s*\(/); + assert.ok(contIdx !== -1, 'onSongLoaded must consult the set-continuation signal'); + assert.ok(introIdx !== -1, 'the intro must still exist for a real arrival'); + assert.ok(contIdx < introIdx, 'the continuation check must gate the flyover — a set song 2+ must not fly in'); +}); From 0b4b174d331dffa3e452955abab0541489cc3f60 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 21:23:10 +0200 Subject: [PATCH 7/9] perf(scan): skip full library re-stat when the tree is unchanged (#979) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(scan): skip full library re-stat when the tree is unchanged Startup scans globbed the whole DLC tree twice (*.feedpak, *.wem) and stat()'d every file to detect changes — ~100k filesystem round trips on a 50k-song library, and painful on a slow NTFS-3G FUSE mount (the "big drive churns on every launch" report). Adds/removes/renames of songs all bump the mtime of the containing directory (verified on the target mount), so after a full pass we persist {reldir: mtime_ns} for every library dir (scan_dir_signature.json, keyed by DLC path). The next scan re-stats only those dirs — a handful vs 100k ops — and skips the entire listing/stat pass when none changed. Blind spot: a pack rewritten in place under the same name bumps the file mtime but not its dir's. Rare for a song library, and the manual Refresh (/api/rescan + /api/rescan/full) now passes force=True to always do the full pass. force threads through kick_scan -> _scan_runner and coalesces like the rescan-pending flag. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(scan): track directory-form songs' own dir in the signature CodeRabbit: _library_dirs recorded only each song's parent. For a directory-form song (loose-song folder or directory sloppak bundle), adding/removing/replacing a file INSIDE the folder bumps that folder's own mtime, not its parent's — so the fast path would skip a rescan it should run. Record the song's own dir when f.is_dir(). File-form sloppaks (a single .feedpak zip) aren't dirs, so the flat file library is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- lib/scan.py | 142 ++++++++++++++++++++++++++++++-- server.py | 7 +- tests/test_feedpak_extension.py | 75 +++++++++++++++++ 3 files changed, 217 insertions(+), 7 deletions(-) diff --git a/lib/scan.py b/lib/scan.py index 820cb50..6780b04 100644 --- a/lib/scan.py +++ b/lib/scan.py @@ -51,6 +51,97 @@ from scan_worker import _relpath, _scan_one log = logging.getLogger("feedBack.scan") +import json + + +# ── Directory-signature fast path ───────────────────────────────────────────── +# +# A startup scan globs the whole library twice (*.feedpak, *.wem) and stats every +# file to detect what changed. On a 50k-song library that lives on a slow mount +# (an NTFS-3G FUSE volume here) it is ~100k filesystem round trips every launch — +# the "big drive churns on every startup" report. +# +# But adds / removes / renames of songs all bump the mtime of the DIRECTORY that +# holds them (verified on the target NTFS-3G mount), and so does the addition of +# a subdirectory (a new entry in its parent). So after a scan we record every +# library directory and its mtime; on the next scan we re-stat ONLY those +# directories (a handful, vs 100k file ops). If none changed, the file set is +# unchanged and the whole listing/stat pass is skipped. +# +# The one thing this cannot see is a file edited IN PLACE under the same name — +# that bumps the file's mtime but not its directory's. That is rare for a song +# library (you add and remove packs, you don't rewrite them under the same name), +# and the manual Refresh forces a full scan (force=True) for exactly that case. +def _dir_signature_file() -> Path: + return appstate.config_dir / "scan_dir_signature.json" + + +def _load_dir_signature() -> dict | None: + try: + data = json.loads(_dir_signature_file().read_text(encoding="utf-8")) + if isinstance(data, dict) and isinstance(data.get("dirs"), dict): + return data + except (OSError, ValueError): + pass + return None + + +def _save_dir_signature(dlc: Path, dirs: dict[str, int]) -> None: + # Keyed by the DLC path so switching libraries never matches a stale + # signature. Best-effort: a failed write just means the next scan is a full + # one, never a wrong one. + try: + _dir_signature_file().write_text( + json.dumps({"dlc": str(dlc), "dirs": dirs}), encoding="utf-8") + except OSError as e: + log.debug("scan: could not persist dir signature: %s", e) + + +def _library_dirs(all_songs, dlc: Path) -> set[str]: + """Every directory whose mtime reflects an add/remove of a library song: + each song's containing directory and all of its ancestors up to the DLC + root (the root itself always included, as "."). Derived from the already- + listed songs — no extra filesystem walk. The builtin carve-outs + (tutorials-builtin / minigames-builtin) are absent because the caller + already excluded them from `all_songs`, so a minigame writing a drill there + never invalidates the fast path. + + Directory-form songs (loose-song folders, directory sloppak bundles) also + record their OWN directory: a file added/removed/replaced INSIDE the folder + bumps that folder's mtime but not its parent's, so tracking only the parent + would miss an in-place change to such a song. File-form sloppaks (a single + .feedpak zip) aren't dirs, so they add nothing here — the flat file library + stays at a handful of dir stats.""" + rels = {"."} + for f in all_songs: + rel = Path(_relpath(f, dlc)) + if f.is_dir(): + rels.add(rel.as_posix()) + parent = rel.parent + rels.add(parent.as_posix()) + for anc in parent.parents: + rels.add(anc.as_posix()) + return rels + + +def _record_dir_signature(all_songs, dlc: Path) -> None: + sig = _stat_dirs(dlc, _library_dirs(all_songs, dlc)) + if sig is not None: # a dir vanished mid-scan → skip; next scan is full + _save_dir_signature(dlc, sig) + + +def _stat_dirs(dlc: Path, rels) -> dict[str, int] | None: + """{reldir: mtime_ns} for the given library dirs, or None if any is gone or + unreadable — a vanished recorded dir means the tree changed, so fail to a + full scan rather than a false match.""" + out: dict[str, int] = {} + for rel in rels: + try: + out[rel] = (dlc if rel == "." else dlc / rel).stat().st_mtime_ns + except OSError: + return None + return out + _SCAN_STATUS_INIT = {"running": False, "stage": "idle", "total": 0, "done": 0, "current": "", "error": None, "is_first_scan": False, "added": 0, "removed": 0} @@ -99,9 +190,12 @@ def _make_scan_executor(): ) -def background_scan(): +def background_scan(force: bool = False): """Scan the library and cache song metadata on startup. Uses a process pool to bypass the GIL for CPU-bound metadata parsing. + `force` skips the directory-signature fast path and always does the full + listing/stat pass — the manual Refresh sets it (see _dir_signature_file). + Never sets `_scan_status["running"] = False` — ownership of that flag lives in `_scan_runner` so a `kick_scan()` racing this function's terminal write cannot observe a stale False and start a second runner. @@ -121,6 +215,22 @@ def background_scan(): builtin_content.seed_builtin_diagnostic_sloppaks(appstate.server_root, dlc) builtin_content.seed_builtin_starter_content(appstate.server_root, dlc) + # Fast path: if every library directory recorded by the last scan still has + # the same mtime, nothing was added, removed, or renamed, so the whole + # glob-and-stat pass below can be skipped (see the signature comment above). + # `force` (manual Refresh) always does the full pass. Seeding above is + # idempotent — it only writes when a builtin is missing — so it does not + # perturb the mtimes on a settled library. + if not force: + stored = _load_dir_signature() + if stored is not None and stored.get("dlc") == str(dlc): + current = _stat_dirs(dlc, stored["dirs"].keys()) + if current is not None and current == stored["dirs"]: + _scan_status = {**_SCAN_STATUS_INIT, "running": True, "stage": "complete"} + log.info("Scan: library tree unchanged (%d dirs) — skipped the full listing/stat pass", + len(current)) + return + # Listing can fail on macOS without Full Disk Access, or on Docker if the # path isn't shared. Report the failure explicitly rather than silently # appearing to scan nothing. @@ -223,6 +333,9 @@ def background_scan(): to_scan.append((f, mtime, size, dlc)) if not to_scan: + # Full pass completed with the DB already up to date — record the tree + # signature so the next startup can take the fast path. + _record_dir_signature(all_songs, dlc) _scan_status = {**_SCAN_STATUS_INIT, "running": True, "stage": "complete", "added": added, "removed": removed} log.info("Scan: nothing new to scan (%d songs, all cached)", len(all_songs)) return @@ -247,6 +360,9 @@ def background_scan(): _scan_status["done"] += 1 _scan_status["current"] = fname + # Record the tree signature after a completed full pass so the next startup + # can skip it when nothing has changed. + _record_dir_signature(all_songs, dlc) log.info("Scan complete: %d songs cached", len(to_scan)) _scan_status = {**_SCAN_STATUS_INIT, "running": True, "stage": "complete", "added": added, "removed": removed} @@ -255,6 +371,9 @@ _scan_kick_lock = threading.Lock() _scan_rescan_pending = False +# Set by kick_scan(force=True); consumed by _scan_runner for the next pass so a +# manual Refresh bypasses the directory-signature fast path. +_scan_force_next = False # Handles to the running scan / enrichment worker threads. Both use the shared @@ -265,9 +384,15 @@ _scan_rescan_pending = False _scan_thread: threading.Thread | None = None -def kick_scan() -> bool: +def kick_scan(force: bool = False) -> bool: """Request a library rescan, single-flight + coalescing. + `force` skips the directory-signature fast path for the resulting pass (the + manual Refresh uses it so an in-place same-name edit — the one thing the + fast path can't see — is always picked up). A forced request that coalesces + onto a running or queued scan keeps the force intent: the pass is forced if + ANY pending request asked for it. + Returns True if a new scan thread was started, False if one was already running. In the latter case a follow-up pass is queued and runs as soon as the current scan finishes so files landing mid-scan (e.g. an upload @@ -275,8 +400,10 @@ def kick_scan() -> bool: until the next periodic pass. Multiple late-arriving requests coalesce into a single follow-up. """ - global _scan_rescan_pending, _scan_thread + global _scan_rescan_pending, _scan_thread, _scan_force_next with _scan_kick_lock: + if force: + _scan_force_next = True if _scan_status["running"]: _scan_rescan_pending = True return False @@ -290,10 +417,15 @@ def kick_scan() -> bool: def _scan_runner(): """Run _background_scan, then re-run if requests arrived mid-scan.""" - global _scan_rescan_pending + global _scan_rescan_pending, _scan_force_next while True: + # Consume the force flag for THIS pass; a forced request queued mid-scan + # sets it again for the follow-up. + with _scan_kick_lock: + forced = _scan_force_next + _scan_force_next = False try: - background_scan() + background_scan(force=forced) except Exception: log.exception("background scan failed unexpectedly") diff --git a/server.py b/server.py index ee13aa9..ecb6863 100644 --- a/server.py +++ b/server.py @@ -1115,7 +1115,10 @@ async def startup_status_stream(request: Request): @app.post("/api/rescan") def trigger_rescan(): """Manually trigger a library rescan.""" - if not scan.kick_scan(): + # force=True: a manual Refresh must skip the directory-signature fast path — + # it is the escape hatch for the one change dir mtimes can't see (a pack + # rewritten in place under the same name). + if not scan.kick_scan(force=True): return {"message": "Scan already in progress"} return {"message": "Rescan started"} @@ -1133,7 +1136,7 @@ def trigger_full_rescan(): # delete_missing() prunes anything genuinely gone at the end. meta_db.conn.execute("UPDATE songs SET mtime = -1") meta_db.conn.commit() - if not scan.kick_scan(): + if not scan.kick_scan(force=True): return {"message": "Scan already in progress"} return {"message": "Full rescan started"} diff --git a/tests/test_feedpak_extension.py b/tests/test_feedpak_extension.py index f8aeb3d..2f556fe 100644 --- a/tests/test_feedpak_extension.py +++ b/tests/test_feedpak_extension.py @@ -137,6 +137,81 @@ def test_background_scan_discovers_both_suffixes(tmp_path, scan_server): assert "ignore.zip" not in seen +# ── 2b. directory-signature fast path (skip the full re-stat) ──────────────── + +def test_dir_signature_fast_path_skips_unchanged_tree(tmp_path, scan_server): + """After a full scan records the library-dir signature, a second scan with + an unchanged tree takes the fast path and does NOT re-glob/extract — but a + forced scan (manual Refresh) always does the full pass, and a new song + (which bumps the dir mtime) reverts to a full pass on its own.""" + import unittest.mock as mock + + dlc = tmp_path / "dlc" + dlc.mkdir() + (dlc / "a.feedpak").write_bytes(b"") + (tmp_path / "config.json").write_text('{"dlc_dir": "%s"}' % dlc) + + scan = importlib.import_module("scan") + seen: list[str] = [] + + def mock_extract(f, dlc_dir): + seen.append(f.name) + return {"title": f.name, "artist": "", "album": ""} + + with mock.patch("scan_worker._extract_meta_for_file", new=mock_extract): + # 1) first pass: full scan, extracts a.feedpak (+ seeded builtins), + # records the signature + scan.background_scan() + assert "a.feedpak" in seen + assert scan._dir_signature_file().exists() + + # 2) unchanged tree: fast path — no glob, no extraction at all + seen.clear() + scan.background_scan() + assert seen == [] + assert scan.status()["stage"] == "complete" + + # 3) a new song bumps the dlc mtime → signature mismatch → full pass + # picks it up on its own (no manual Refresh needed for adds) + (dlc / "b.feedpak").write_bytes(b"") + seen.clear() + scan.background_scan() + assert "b.feedpak" in seen + + # 4) force=True (Refresh) bypasses the fast path even on a settled tree: + # with the signature now current, a plain scan skips, a forced one lists + seen.clear() + scan.background_scan() # fast path + assert seen == [] + forced_listed = [] + real_delete_missing = scan.appstate.meta_db.delete_missing + def _spy(files): + forced_listed.append(set(files)) + return real_delete_missing(files) + with mock.patch.object(scan.appstate.meta_db, "delete_missing", new=_spy): + scan.background_scan(force=True) + assert forced_listed, "force=True must run the full listing pass" + + +def test_dir_signature_tracks_directory_form_song_own_dir(tmp_path): + """A directory-form song (loose folder / directory bundle) records its OWN + directory in the signature, so an in-place file change inside it — which + bumps that folder's mtime but not its parent's — invalidates the fast path. + A file-form sloppak (a plain .feedpak zip) is not a dir and adds nothing.""" + scan = importlib.import_module("scan") + dlc = tmp_path / "dlc" + (dlc / "packs").mkdir(parents=True) + loose = dlc / "packs" / "my_loose_song" # directory-form song + loose.mkdir() + zipped = dlc / "packs" / "zipped.feedpak" # file-form song + zipped.write_bytes(b"") + + rels = scan._library_dirs([loose, zipped], dlc) + assert "packs/my_loose_song" in rels, "directory-form song must track its own dir" + assert "packs" in rels and "." in rels + assert "packs/zipped.feedpak" not in rels, "a file-form sloppak is not a tracked dir" + + # ── 3. POST /api/songs/upload gate (endpoint) ──────────────────────────────── @pytest.fixture() From 3717e4338d32e3a6159aacfcf3d94e0cb9248b10 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Thu, 16 Jul 2026 12:06:52 +0200 Subject: [PATCH 8/9] fix(plugins): restore window.esc for out-of-tree plugins (#986) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit app.js exported `esc` as an implicit global back when it was a classic script. a9fce29 made it an ES module and 14b4058 carved `esc` into js/dom.js; the window re-export list was rebuilt without it. Out-of-tree plugins load screen.js as a classic script and call `esc()` bare, so nothing in-tree catches the break: no-undef, a call-graph scan and a grep all pass while the plugin throws in the field. The MIDI plugin builds its device list with esc() inside the same try block that catches requestMIDIAccess() failures, so the ReferenceError surfaced to testers as "MIDI Access denied esc is not defined" — access had actually been granted. Pin the whole plugin-facing global surface by name, mirroring tests/test_plugin_context_contract.py. Verified both ways against a running app: without the fix the spec fails with "missing or not functions: esc". Co-authored-by: Claude Opus 4.8 (1M context) --- static/app.js | 5 +- tests/browser/plugin-globals-contract.spec.ts | 63 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 tests/browser/plugin-globals-contract.spec.ts diff --git a/static/app.js b/static/app.js index 8d60608..d306259 100644 --- a/static/app.js +++ b/static/app.js @@ -2319,11 +2319,14 @@ configureHost({ currentFilename: () => currentFilename, }); +// `esc` is here for out-of-tree plugins only: their screen.js loads as a classic +// script and called esc() back when app.js was one too and it was an implicit +// global. Nothing in core reads window.esc — import it from ./js/dom.js instead. Object.assign(window, { _confirmDialog, _getArrangementNamingMode, _libraryLocalFilename, _librarySongArtUrl, _librarySongId, _onHeaderClick, _onNamingModeChange, _trapFocusInModal, changeArrangement, checkPluginUpdates, clearLibFilters, clearLoop, - deleteSelectedLoop, exportDiagnostics, exportSettings, filterFavorites, + deleteSelectedLoop, esc, exportDiagnostics, exportSettings, filterFavorites, filterLibrary, fullRescanLibrary, goFavPage, handleSliderInput, hideScanBanner, importSettings, loadPlugins, loadSavedLoop, loadSettings, onSectionPracticeModeChange, openEditModal, persistSetting, diff --git a/tests/browser/plugin-globals-contract.spec.ts b/tests/browser/plugin-globals-contract.spec.ts new file mode 100644 index 0000000..0915016 --- /dev/null +++ b/tests/browser/plugin-globals-contract.spec.ts @@ -0,0 +1,63 @@ +// The window globals are a THIRD-PARTY CONTRACT. Pin them. +// +// Out-of-tree plugins load their screen.js as a CLASSIC script and call these +// as bare globals. Nothing in core reads most of them, so a call-graph scan, +// ESLint's no-undef, and a grep all come back clean while the plugin breaks in +// the field. This is the frontend twin of tests/test_plugin_context_contract.py +// — same reasoning, same literal-list rule. +// +// This guard is retroactive: `esc` was an implicit global back when app.js was +// a classic script, went module-scoped in a9fce29, and got carved into +// js/dom.js in 14b4058. The re-export list at the bottom of app.js was rebuilt +// without it, and the MIDI plugin's device list threw "esc is not defined" for +// testers — reported as "MIDI Access denied", because the ReferenceError landed +// in a try/catch meant for permission failures. +// +// WHY A LITERAL LIST AND NOT A DERIVED ONE. Deriving the expected set from +// app.js would assert the code equals itself. The point is that a human has to +// look at a diff and consciously agree to change the contract. + +import { test, expect } from '@playwright/test'; + +const PLUGIN_GLOBALS = [ + '_confirmDialog', '_getArrangementNamingMode', '_libraryLocalFilename', '_librarySongArtUrl', + '_librarySongId', '_onHeaderClick', '_onNamingModeChange', '_trapFocusInModal', + 'changeArrangement', 'checkPluginUpdates', 'clearLibFilters', 'clearLoop', + 'deleteSelectedLoop', 'esc', 'exportDiagnostics', 'exportSettings', 'filterFavorites', + 'filterLibrary', 'fullRescanLibrary', 'goFavPage', 'handleSliderInput', + 'hideScanBanner', 'importSettings', 'loadPlugins', 'loadSavedLoop', + 'loadSettings', 'onSectionPracticeModeChange', 'openEditModal', 'persistSetting', + 'pickDlcFolder', 'pinCurrentArrangementDefault', 'playSong', 'previewDiagnostics', + 'previewEditArt', 'renderGridCards', 'renderTreeInto', 'rescanLibrary', + 'retuneSong', 'saveCurrentLoop', 'saveSettings', 'seekBy', + 'setAvOffsetMs', 'setFavView', 'setInstrumentPathway', 'setLibView', + 'setLibraryProvider', 'setLoopEnd', 'setLoopStart', 'setMastery', + 'setSpeed', 'setViz', 'showScreen', 'sortFavorites', + 'sortLibrary', 'syncLibrarySong', 'toggleAllArtists', 'toggleAllFavoriteArtists', + 'toggleLibFilters', 'togglePlay', 'toggleSectionPracticePopover', 'uiPrompt', + 'updatePlugin', 'uploadSongs', + 'filterFavTreeLetter', 'filterTreeLetter', 'goFavTreePage', 'goTreePage', +]; + +test('plugin-facing window globals are all callable', async ({ page }) => { + await page.goto('/'); + await page.waitForSelector('.screen.active', { timeout: 10000 }); + + const missing = await page.evaluate( + (names) => names.filter((n) => typeof (window as any)[n] !== 'function'), + PLUGIN_GLOBALS, + ); + + expect(missing, `window globals plugins depend on are missing or not functions: ${missing.join(', ')}`).toEqual([]); +}); + +// The plugin call site that actually broke: esc() interpolated into a template +// string. A global that exists but doesn't escape is its own bug. +test('window.esc escapes HTML metacharacters', async ({ page }) => { + await page.goto('/'); + await page.waitForSelector('.screen.active', { timeout: 10000 }); + + const escaped = await page.evaluate(() => (window as any).esc('')); + expect(escaped).not.toContain(' Date: Thu, 16 Jul 2026 19:33:17 -0400 Subject: [PATCH 9/9] fix(highway_3d): stop the lane at the hit line (#994) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lane maps chart time to z exactly as notes do, over the window [now - BEHIND, now + AHEAD]. That puts its near edge at +TS*BEHIND — BEHIND seconds PAST the hit line, toward the player. Nothing is ever drawn there: drawNote and the chord frames both clamp to Math.min(0, dZ(dt)), so notes stop dead at z = 0. The overhang was therefore lane surface with nothing on it. Clamp the floor geometry's near edge to the hit line. The far edge is deliberately untouched — it still lands at -AHEAD*TS, aligned with the note horizon, which is why the span stays AHEAD+BEHIND in the sliced path and the clamp is applied per slice (a slice entirely past the line collapses to zero length and is skipped before the arpeggio probe, so it costs nothing). All four floor sites move together — the sliced lane (which also feeds both divider loops), the fallback lane, its dividers, and the fret boundary extension lines. They shared the identical `+ TS * BEHIND` shift; fixing only some would leave fret lines poking past a lane that now stops. Closes #991 Signed-off-by: Kris Anderson --- CHANGELOG.md | 6 ++++++ plugins/highway_3d/screen.js | 36 +++++++++++++++++++++++++++--------- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8236ffa..224037e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -212,6 +212,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 engine (`app.js`, `highway.js`, `playSong`, `showScreen`, the capability registry). ### Fixed +- **3D Highway: the lane stops at the hit line** (#991) — the highway lane, its + dividers, and the fret boundary extension lines ran `BEHIND` seconds *past* the + hit line toward the player. Nothing is ever drawn in that strip (notes and chord + frames clamp to `Math.min(0, dZ(dt))`), so it read as lane with no notes on it. + The floor geometry now ends at the hit line; its far edge is unchanged, still + `-AHEAD*TS` at the note horizon. - **Career passports review polish** — the passport tabs and book overlay carry proper ARIA semantics (`aria-selected`/`aria-controls`/`tabpanel`; `role="dialog"` + `aria-modal` with focus moved to the close button on open diff --git a/plugins/highway_3d/screen.js b/plugins/highway_3d/screen.js index 3bc2ff8..521a305 100644 --- a/plugins/highway_3d/screen.js +++ b/plugins/highway_3d/screen.js @@ -12614,8 +12614,19 @@ const tC = now + (dt0 + dt1) * 0.5 - BEHIND; const b = laneBoundsFromAnchor(getChartAnchorAt(anchors, tC)); if (!b) continue; - const z0 = dZ(dt0) + TS * BEHIND; - const z1 = dZ(dt1) + TS * BEHIND; + // The lane STOPS AT THE HIT LINE (z = 0) — issue #991. The + // slice window starts BEHIND seconds in the past, so the + // first slices map to positive z, i.e. past the hit line + // toward the player. Nothing is ever drawn there: notes and + // chord frames clamp to Math.min(0, dZ(dt)), so that strip + // is lane with nothing on it. Clamp the NEAR edge only — + // the far edge stays at dZ(AHEAD+BEHIND)+TS*BEHIND = -AHEAD*TS, + // aligned with the note horizon, exactly as before. + const z0 = Math.min(0, dZ(dt0) + TS * BEHIND); + const z1 = Math.min(0, dZ(dt1) + TS * BEHIND); + // Slice lies entirely past the hit line -> zero length, nothing + // to draw. Skip before the arp probe so it costs nothing. + if (z0 === z1) continue; const arpSlice = (laneRailArpHsFlags && handShapesRails && handShapesRails.length) ? arpeggioLaneOuterRailLaneSlice( dt0, dt1, now, @@ -12764,9 +12775,13 @@ divMin = dMin; divMax = dMax; - // Same fix: extend to AHEAD+BEHIND so far edge = -AHEAD*TS. - const laneLen = TS * (AHEAD + BEHIND); - const zLane = -laneLen / 2 + TS * BEHIND; + // Far edge at -AHEAD*TS (the note horizon), near edge at the + // hit line (z = 0) — the lane does not run past it toward the + // player, where nothing is ever drawn (#991). Spanning + // AHEAD+BEHIND and shifting by +TS*BEHIND put the near edge at + // +TS*BEHIND; spanning AHEAD alone keeps the same far edge. + const laneLen = TS * AHEAD; + const zLane = -laneLen / 2; const laneOp = (HWY_LANE_STRIPE_OP_BASE + highwayIntensity * HWY_LANE_STRIPE_OP_INT) * (_venueSceneOverride ? VENUE_LANE_OP_BOOST : 1); mLaneOdd.opacity = laneOp; @@ -12786,7 +12801,8 @@ } if (highwayIntensity > 0.05) { - const divLen = TS * (AHEAD + BEHIND); + // Matches the lane above: ends at the hit line (#991). + const divLen = TS * AHEAD; const yPos = boardY + 0.03 * K; const divOp2 = 0.02 + highwayIntensity * 0.1; const divOpArp2 = Math.min(0.92, 0.16 + highwayIntensity * 0.42); @@ -12799,7 +12815,7 @@ for (let f = fDivA; f <= fDivB; f++) { if (hwyLaneArpOuterDividers && (f === fDivA || f === fDivB)) continue; const div = pLaneDivider.get(); - div.position.set(xFret(f), yPos, dZ(0) - divLen * 0.5 + TS * BEHIND); + div.position.set(xFret(f), yPos, -divLen * 0.5); div.material = mLaneDivider; div.scale.set(1, 1, divLen); div.renderOrder = 2; @@ -12818,8 +12834,10 @@ // ── Fret boundary extension lines ───────────────────────── if (mLaneDividerExt && fretDividersVisible) { - const extLaneLen = TS * (AHEAD + BEHIND); - const extZMid = -extLaneLen / 2 + TS * BEHIND; + // Same hit-line stop as the lane (#991) — otherwise these lines + // would be the only floor geometry still running past it. + const extLaneLen = TS * AHEAD; + const extZMid = -extLaneLen / 2; const extYPos = boardY + 0.03 * K; mLaneDividerExt.opacity = Math.max(0.3, 0.3 + highwayIntensity * 0.15); for (let f = 0; f <= NFRETS; f++) {