fix(nav): the library sometimes showed the legacy screen — map 'home' inside showScreen#923
Merged
Merged
Conversation
… inside showScreen
Testers: "randomly, when moving to the library from another menu option, the library shows the
old interface — never when a song ends."
━━━ WHAT WAS ACTUALLY HAPPENING ━━━
#home is the PRE-V3 library screen. The v3 shell replaced it with #v3-songs, and the mapping DID
exist — but only inside WRAPPERS on window.showScreen, and only for callers that go through
`window`. THREE independent parties monkey-patch it, each capturing whatever happens to be there
at the time:
app.js publishes the raw function
-> shell.js wraps it, adding the home -> v3-songs mapping
-> the stems plugin wraps it AGAIN (src/main.js:1029), capturing the current value
Plugins load ASYNCHRONOUSLY. The chain links up in whatever order the race settles, and any
capture taken before shell.js installs — or any re-assignment after it — silently drops the
mapping. Hence "randomly".
AND THE INTERNAL CALLERS NEVER TOUCHED window.showScreen AT ALL. closeCurrentSong and the
Esc-from-settings shortcut call the IMPORTED showScreen, which no wrapper ever sees. Reproduced
in a browser: the unwrapped function with 'home' lands on the dead legacy screen EVERY time.
"Never when a song ends" is the tell, and it is what identified the mechanism: closeCurrentSong
resolves its target through _resolvePlayerOrigin(), which ALREADY applies this mapping. That one
path was fine — which is exactly why the bug looked random rather than total.
PRE-EXISTING, not a regression from the module carve: the onclick="showScreen('home')" links and
the wrapper-only mapping both date to 2026-06-22.
━━━ THE FIX ━━━
The guard lives inside showScreen now: ONE place, in the function every caller routes through,
instead of a chain of monkey-patches that must each remember. Wrapper order stops mattering, and
the module-internal callers are covered for the first time.
Verified in a browser: the raw, unwrapped showScreen('home') — which reproduced as #home — now
lands on #v3-songs, and cannot be undone by any wrapper order.
━━━ AND A [P1] I INTRODUCED, WHICH CODEX CAUGHT ━━━
My first cut mapped BOTH 'home' and 'v3-home', copied straight from _resolvePlayerOrigin.
That is correct THERE and wrong HERE. _resolvePlayerOrigin computes where to RETURN TO after a
song, and landing on the Songs list from the dashboard is the right behaviour. But #v3-home is
the v3 DASHBOARD — a real screen that the shell's Home nav, the onboarding tour and the dashboard
re-render listener all target. Redirecting it would have made Home unreachable.
A LEGACY ALIAS IS NOT THE SAME THING AS A RETURN TARGET. Only 'home' is mapped now, and a test
pins that: re-adding 'v3-home' to the guard fails it.
4 tests, bite-tested both ways.
node 1049, pytest 2425, ESLint 0, Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthrough
ChangesLegacy home routing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/js/show_screen_legacy_home.test.js (1)
32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd runtime coverage for the redirect contract.
These assertions inspect source text but never execute
showScreen; a broken DOM transition could still pass. Add a focused behavior test for both “#v3-songspresent” and “absent” cases, and verify thatv3-homeremains unchanged. At minimum, make the regex match the entire guard condition rather than separate nearby tokens.Also applies to: 44-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/js/show_screen_legacy_home.test.js` around lines 32 - 42, Extend the tests around showScreen with runtime coverage for both present and absent `#v3-songs` cases, executing the function and asserting the redirect behavior while confirming v3-home remains unchanged. Replace or supplement the source-text checks so the regex validates the complete guard condition rather than matching nearby tokens independently; retain bodyOf only where it remains necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/js/show_screen_legacy_home.test.js`:
- Around line 32-42: Extend the tests around showScreen with runtime coverage
for both present and absent `#v3-songs` cases, executing the function and
asserting the redirect behavior while confirming v3-home remains unchanged.
Replace or supplement the source-text checks so the regex validates the complete
guard condition rather than matching nearby tokens independently; retain bodyOf
only where it remains necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49359fa6-aa1b-4638-bf72-8388e89acf51
📒 Files selected for processing (2)
static/js/session.jstests/js/show_screen_legacy_home.test.js
This was referenced Jul 12, 2026
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
…ging, make the shell listen (#924) (#925) * fix(nav): the library sometimes showed the legacy screen — map 'home' inside showScreen Testers: "randomly, when moving to the library from another menu option, the library shows the old interface — never when a song ends." ━━━ WHAT WAS ACTUALLY HAPPENING ━━━ #home is the PRE-V3 library screen. The v3 shell replaced it with #v3-songs, and the mapping DID exist — but only inside WRAPPERS on window.showScreen, and only for callers that go through `window`. THREE independent parties monkey-patch it, each capturing whatever happens to be there at the time: app.js publishes the raw function -> shell.js wraps it, adding the home -> v3-songs mapping -> the stems plugin wraps it AGAIN (src/main.js:1029), capturing the current value Plugins load ASYNCHRONOUSLY. The chain links up in whatever order the race settles, and any capture taken before shell.js installs — or any re-assignment after it — silently drops the mapping. Hence "randomly". AND THE INTERNAL CALLERS NEVER TOUCHED window.showScreen AT ALL. closeCurrentSong and the Esc-from-settings shortcut call the IMPORTED showScreen, which no wrapper ever sees. Reproduced in a browser: the unwrapped function with 'home' lands on the dead legacy screen EVERY time. "Never when a song ends" is the tell, and it is what identified the mechanism: closeCurrentSong resolves its target through _resolvePlayerOrigin(), which ALREADY applies this mapping. That one path was fine — which is exactly why the bug looked random rather than total. PRE-EXISTING, not a regression from the module carve: the onclick="showScreen('home')" links and the wrapper-only mapping both date to 2026-06-22. ━━━ THE FIX ━━━ The guard lives inside showScreen now: ONE place, in the function every caller routes through, instead of a chain of monkey-patches that must each remember. Wrapper order stops mattering, and the module-internal callers are covered for the first time. Verified in a browser: the raw, unwrapped showScreen('home') — which reproduced as #home — now lands on #v3-songs, and cannot be undone by any wrapper order. ━━━ AND A [P1] I INTRODUCED, WHICH CODEX CAUGHT ━━━ My first cut mapped BOTH 'home' and 'v3-home', copied straight from _resolvePlayerOrigin. That is correct THERE and wrong HERE. _resolvePlayerOrigin computes where to RETURN TO after a song, and landing on the Songs list from the dashboard is the right behaviour. But #v3-home is the v3 DASHBOARD — a real screen that the shell's Home nav, the onboarding tour and the dashboard re-render listener all target. Redirecting it would have made Home unreachable. A LEGACY ALIAS IS NOT THE SAME THING AS A RETURN TARGET. Only 'home' is mapped now, and a test pins that: re-adding 'v3-home' to the guard fails it. 4 tests, bite-tested both ways. node 1049, pytest 2425, ESLint 0, Codex 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(nav): nobody may monkey-patch window.showScreen — add screen:changing, make the shell listen (#924) window.showScreen was wrapped by THREE independent parties, each capturing whatever happened to be there at the time: app.js publishes the raw function -> static/v3/shell.js wrapped it (to call syncActive, and to map home -> v3-songs) -> the stems plugin wrapped it AGAIN (to tear down on leaving the player) Plugins load ASYNCHRONOUSLY, so the chain linked up in whatever order the race settled. A capture taken before shell.js installed silently dropped the mapping it carried — and the library opened on the dead legacy #home screen. Testers saw that as "randomly, the library shows the old interface" (#923). #923 fixed the symptom by moving the mapping inside showScreen. This removes the CAUSE: neither wrapper ever needed to be one. ━━━ TWO EVENTS, AND THE DISTINCTION IS THE WHOLE POINT ━━━ screen:changing emitted BEFORE anything happens. "I am leaving `from`." Teardown/cancel here. screen:changed emitted after the DOM and data settle. "I am on `id`." Now carries `from`. screen:changing is new, and it exists because Codex caught me collapsing the two. The stems plugin tore down its audio graph BEFORE showScreen did anything; screen:changed fires at the very END, after core awaits library and provider loads — so moving the plugin onto it would have delayed teardown behind a slow fetch, or skipped it entirely if that fetch threw, and stems would keep playing on a non-player screen. A test pins the ordering: screen:changing must precede the first await. shell.js is a plain screen:changed listener now, like app.js, audio-mixer.js and tour-engine.js already were. window.showScreen is an unwrapped function again, and tests/js/ no_showscreen_monkeypatch.test.js fails CI if anything in static/ ever assigns to it again — so the hazard is structurally impossible rather than merely avoided. ━━━ AND A FALLBACK THAT COULD NEVER FIRE ━━━ My retry-if-the-bus-is-late path listened for `slopsmith:capabilities:ready`. Core dispatches `feedBack:capabilities:ready` (capabilities.js:1536) — the slopsmith: name is the PRE-DMCA event and nothing has emitted it since the rename. Codex caught it. A guard that cannot fire is worse than no guard: it reads as protection and is decoration. (The same dead-event bug turned out to be sitting in THREE of the stems plugin's fallbacks, where it has silently disabled its lifecycle wiring whenever the bus was late. Fixed in feedback-plugin-stems#38.) VERIFIED. A/B against origin/main: the nav highlight and topbar title follow IDENTICALLY with shell.js as a listener; screen:changing -> screen:changed fire in order with the right {id, from}; window.showScreen is unwrapped; and showScreen('home') still lands on v3-songs. node 1053, pytest 2425, ESLint 0, Codex 0. Closes #924 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
to got-feedBack/feedBack-plugin-stems
that referenced
this pull request
Jul 12, 2026
…ents instead (#38) This wrapped window.showScreen to tear down the stems graph when leaving the player. It was the THIRD wrapper on that one global, and it shipped a bug. Core publishes window.showScreen; the v3 shell wrapped it (carrying the legacy home -> v3-songs mapping); this wrapped it again. Each captured whatever happened to be there AT THE TIME — and plugins load ASYNCHRONOUSLY, so the chain linked up in whatever order the race settled. When this wrapper won, the shell's mapping was silently dropped and THE LIBRARY OPENED ON THE DEAD LEGACY SCREEN. Testers reported it as "randomly, the library shows the old interface" (got-feedBack/feedBack#923, #924). Core already emits screen events for exactly this. Listening costs nothing, cannot clobber another plugin, and cannot be clobbered by one. ━━━ THREE THINGS CODEX CAUGHT, EACH A SILENT NO-OP ━━━ 1. TIMING. My first cut listened to screen:changed — which fires at the END of showScreen, after core awaits library and provider loads. The old wrapper ran BEFORE navigation began. Late teardown means the stems graph keeps playing on a non-player screen, and a slow or failing fetch skips it entirely. Core now emits screen:changing before it does anything, and that is the event this wants. 2. BOTH EVENTS, because of cross-repo ordering. screen:changing is NEW — today's released host does not emit it. Listening only to the new one would mean teardown() NEVER runs on the current host. Listening only to the old one reintroduces the late-teardown problem once the host updates. So: both. It is safe by construction — the wrapper this replaces called teardown() on EVERY non-player navigation, so it is already idempotent. 3. EITHER BUS. window.slopsmith is the legacy alias of the SAME object (core: `window.slopsmith = window.feedBack`), and the rest of this file reads it. Reading only window.feedBack would attach nothing on a build that exposes just the old name. ━━━ AND A PRE-EXISTING BUG, FOUND ON THE WAY ━━━ All THREE of this file's "bus is late" fallbacks listened for `slopsmith:capabilities:ready`. Core dispatches `feedBack:capabilities:ready` (capabilities.js:1536). The slopsmith: name is the PRE-DMCA event and NOTHING has emitted it since the rename — so every one of those fallbacks has been DEAD, and wireLifecycleListeners / installCapabilityParticipant silently never wired whenever the bus was late. All three now listen for the event that is actually dispatched (keeping the old alias too, harmlessly). 47/47, Codex 0. Refs got-feedBack/feedBack#924 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was actually happening
#homeis the pre-v3 library screen. The v3 shell replaced it with#v3-songs, and the mapping did exist — but only inside wrappers onwindow.showScreen, and only for callers that go throughwindow.Three independent parties monkey-patch it, each capturing whatever happens to be there at the time:
Plugins load asynchronously. The chain links up in whatever order the race settles — and any capture taken before shell.js installs, or any re-assignment after it, silently drops the mapping.
Hence "randomly."
And the internal callers never touched
window.showScreenat all.closeCurrentSongand the Esc-from-settings shortcut call the importedshowScreen, which no wrapper ever sees. Reproduced in a browser: the unwrapped function with'home'lands on the dead legacy screen every time."Never when a song ends" is the tell — and it's what identified the mechanism.
closeCurrentSongresolves its target through_resolvePlayerOrigin(), which already applies this mapping. That one path was fine, which is exactly why the bug looked random rather than total.Pre-existing, not a regression from the module carve: the
onclick="showScreen('home')"links and the wrapper-only mapping both date to 2026-06-22.The fix
The guard lives inside
showScreennow: one place, in the function every caller routes through, instead of a chain of monkey-patches that must each remember. Wrapper order stops mattering, and the module-internal callers are covered for the first time.Verified in a browser: the raw, unwrapped
showScreen('home')— which reproduced as#home— now lands on#v3-songs, and cannot be undone by any wrapper order.And a [P1] I introduced, which Codex caught
My first cut mapped both
'home'and'v3-home', copied straight from_resolvePlayerOrigin.That's correct there and wrong here.
_resolvePlayerOrigincomputes where to return to after a song, and landing on the Songs list from the dashboard is right. But#v3-homeis the v3 dashboard — a real screen that the shell's Home nav, the onboarding tour, and the dashboard re-render listener all target. Redirecting it would have made Home unreachable.Only
'home'is mapped now, and a test pins it: re-adding'v3-home'to the guard fails.4 tests, bite-tested both ways.
node 1049 · pytest 2425 · ESLint 0 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests