fix: stop monkey-patching window.showScreen — listen to the screen events instead#38
Merged
Merged
Conversation
…ents instead 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>
📝 WalkthroughWalkthroughThe stems plugin adds feedBack readiness-event fallbacks, replaces the ChangesEvent-bus lifecycle wiring
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/main.js`:
- Around line 1015-1021: Update wireLifecycleListeners() to resolve the
available event bus before registering lifecycle handlers, matching the
bus-resolution approach used by wireScreenListener(). Use that resolved bus for
every subscription instead of requiring window.slopsmith.on, while preserving
the existing lifecycle event handlers and readiness fallback listeners.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1015
to
1021
| // `feedBack:capabilities:ready` is what capabilities.js actually dispatches | ||
| // (capabilities.js:1536). The slopsmith: name is the PRE-DMCA event and nothing | ||
| // emits it any more — so this fallback has been DEAD since the rename, and the | ||
| // lifecycle listeners silently never wired when the bus was late. Keep the old | ||
| // alias too, harmlessly, for an older capabilities build. | ||
| window.addEventListener('feedBack:capabilities:ready', wireLifecycleListeners, { once: true }); | ||
| window.addEventListener('slopsmith:capabilities:ready', wireLifecycleListeners, { once: true }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the resolved bus for lifecycle subscriptions.
This retry wakes on feedBack, but wireLifecycleListeners() still requires and calls window.slopsmith.on. On a feedBack-only host, lifecycle handlers never subscribe, so stem initialization and teardown on playback events stop working. Mirror wireScreenListener() by resolving one bus and using it for every subscription.
Proposed fix
const wireLifecycleListeners = () => {
- if (!(window.slopsmith && typeof window.slopsmith.on === 'function')) {
+ const bus = window.feedBack || window.slopsmith;
+ if (!(bus && typeof bus.on === 'function')) {
window.addEventListener('feedBack:capabilities:ready', wireLifecycleListeners, { once: true });
window.addEventListener('slopsmith:capabilities:ready', wireLifecycleListeners, { once: true });
return;
}
- window.slopsmith.on('song:loading', onLoading);
- window.slopsmith.on('playback:loading', onLoading);
- window.slopsmith.on('playback:ready', onReady);
- window.slopsmith.on('song:loaded', onReady);
- window.slopsmith.on('song:ready', onReady);
- window.slopsmith.on('playback:stopped', onStopped);
- window.slopsmith.on('playback:ended', onFinished);
+ bus.on('song:loading', onLoading);
+ bus.on('playback:loading', onLoading);
+ bus.on('playback:ready', onReady);
+ bus.on('song:loaded', onReady);
+ bus.on('song:ready', onReady);
+ bus.on('playback:stopped', onStopped);
+ bus.on('playback:ended', onFinished);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // `feedBack:capabilities:ready` is what capabilities.js actually dispatches | |
| // (capabilities.js:1536). The slopsmith: name is the PRE-DMCA event and nothing | |
| // emits it any more — so this fallback has been DEAD since the rename, and the | |
| // lifecycle listeners silently never wired when the bus was late. Keep the old | |
| // alias too, harmlessly, for an older capabilities build. | |
| window.addEventListener('feedBack:capabilities:ready', wireLifecycleListeners, { once: true }); | |
| window.addEventListener('slopsmith:capabilities:ready', wireLifecycleListeners, { once: true }); | |
| // `feedBack:capabilities:ready` is what capabilities.js actually dispatches | |
| // (capabilities.js:1536). The slopsmith: name is the PRE-DMCA event and nothing | |
| // emits it any more — so this fallback has been DEAD since the rename, and the | |
| // lifecycle listeners silently never wired when the bus was late. Keep the old | |
| // alias too, harmlessly, for an older capabilities build. | |
| const bus = window.feedBack || window.slopsmith; | |
| if (!(bus && typeof bus.on === 'function')) { | |
| window.addEventListener('feedBack:capabilities:ready', wireLifecycleListeners, { once: true }); | |
| window.addEventListener('slopsmith:capabilities:ready', wireLifecycleListeners, { once: true }); | |
| return; | |
| } | |
| bus.on('song:loading', onLoading); | |
| bus.on('playback:loading', onLoading); | |
| bus.on('playback:ready', onReady); | |
| bus.on('song:loaded', onReady); | |
| bus.on('song:ready', onReady); | |
| bus.on('playback:stopped', onStopped); | |
| bus.on('playback:ended', onFinished); |
🤖 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 `@src/main.js` around lines 1015 - 1021, Update wireLifecycleListeners() to
resolve the available event bus before registering lifecycle handlers, matching
the bus-resolution approach used by wireScreenListener(). Use that resolved bus
for every subscription instead of requiring window.slopsmith.on, while
preserving the existing lifecycle event handlers and readiness fallback
listeners.
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.
Refs got-feedBack/feedBack#924.
This plugin wrapped
window.showScreento tear down the stems graph when leaving the player. It was the third wrapper on that one global — and it shipped a bug.What it broke
Each captures whatever happens to be there at the time — and plugins load asynchronously, so the chain links up in whatever order the race settles. 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 ofshowScreen, 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 emitsscreen:changingbefore it does anything.2. Both events, because of cross-repo ordering.
screen:changingis new — today's released host doesn't emit it. Listening only to the new one would meanteardown()never runs on the current host; only to the old one reintroduces the late-teardown problem once the host updates. So: both. Safe by construction — the wrapper this replaces calledteardown()on every non-player navigation, so it's already idempotent.3. Either bus.
window.slopsmithis the legacy alias of the same object (core:window.slopsmith = window.feedBack), and the rest of this file reads it. Reading onlywindow.feedBackwould attach nothing on a build exposing 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). Theslopsmith:name is the pre-DMCA event, and nothing has emitted it since the rename — so every one of those fallbacks has been dead, andwireLifecycleListeners/installCapabilityParticipantsilently 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.
🤖 Generated with Claude Code
Summary by CodeRabbit