feat: Desktop workflow enhancements (audio-only, playback recovery, virtual desktop, tray hover mini-player)#4428
Open
EmreSoyak wants to merge 2 commits intopear-devs:masterfrom
Open
Conversation
…ver mini-player Custom feature pack by EmreSoyak: - Virtual Desktop Awareness: Window moves to current virtual desktop instead of yanking you back (Options > Tray toggle) - Audio-Only Plugin: Forces audio-only streaming, saves ~300MB RAM - Playback Recovery Plugin: Auto-recovers from stuck/stalled playback with progressive strategies (seek, seek-forward, skip) - Tray Hover Mini-Player: Hover the tray icon to see album art, song info, and prev/play-pause/next controls - Toast suppression when hover popup is active - Deferred tray event handlers (fixes plugin-before-tray init order) - DevTools gated behind OPEN_DEVTOOLS env var See EMRE-FEATURES.md for full documentation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewrites EMRE-FEATURES.md with focus on user problems and benefits rather than implementation notes. Each feature section now explains the pain point it solves, how the solution works, and what files are involved. Updated hover popup docs to reflect the final implementation (cursor polling, title-based IPC). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change introduces four opt-in features for a desktop YouTube Music app: Audio-Only Mode (a plugin forcing audio-only playback with lowest quality), Playback Recovery (a watchdog monitoring video health and triggering recovery actions), Virtual Desktop Awareness (enabling the tray window to move to the current desktop), and a Tray Hover Mini-Player (popup with playback controls shown on tray hover). Supporting changes include configuration defaults, UI localization, HTML assets, menu integration, and tray event handler infrastructure for deferred plugin registration. Changes
Sequence DiagramssequenceDiagram
participant User as User
participant Tray as System Tray
participant Polling as Mouse Polling
participant Popup as Popup Window
participant Renderer as Renderer
participant PlayerAPI as Player API
User->>Tray: Hover over tray
Polling->>Polling: 150ms interval
Polling->>Popup: Cursor on popup?
Polling->>Tray: Cursor on tray?
alt Cursor detected on popup or tray
Popup->>Popup: Show (fade in via CSS)
Renderer->>Popup: Inject song info
Popup->>Popup: Display title, artist, art
else Cursor left
Popup->>Popup: 250ms delay then hide (fade out)
Polling->>Polling: Resume tracking
end
User->>Popup: Click prev/play-pause/next button
Popup->>Renderer: Update document.title: act:<action>:<counter>
Renderer->>PlayerAPI: Invoke playPause/previous/next
sequenceDiagram
participant Renderer as Renderer Process
participant Watchdog as Watchdog Loop (3s)
participant MediaEvents as Media Events
participant RecoveryLogic as Recovery Logic
participant PlayerAPI as Player API
Renderer->>Renderer: Initialize plugin
MediaEvents->>Renderer: Listen: error, stalled, waiting, timeupdate
Watchdog->>Renderer: Every 3 seconds
Watchdog->>Renderer: Check: readyState=0, frozen time, buffer empty
alt Stall/error detected
Watchdog->>RecoveryLogic: Trigger attemptRecovery()
RecoveryLogic->>RecoveryLogic: Increment consecutiveFailures
alt Under maxRetries
alt Early attempts
RecoveryLogic->>PlayerAPI: Seek to current time
PlayerAPI->>Renderer: Resume playback
else Later attempts
RecoveryLogic->>PlayerAPI: Seek forward / Resume
PlayerAPI->>Renderer: Force buffer reload
end
else Max retries exceeded
RecoveryLogic->>PlayerAPI: nextVideo() - skip track
RecoveryLogic->>RecoveryLogic: Reset failure counters
end
else Playback healthy
MediaEvents->>RecoveryLogic: timeupdate received
RecoveryLogic->>RecoveryLogic: Clear failure counters
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/notifications/index.ts (1)
25-37:⚠️ Potential issue | 🟠 Major
hoverControlsdefault should be off to stay opt-in.Setting
hoverControls: trueenables the new hover UI by default for users already using notifications. That conflicts with the “opt-in/default off” rollout intent.Suggested fix
export const defaultConfig: NotificationsPluginConfig = { @@ - hoverControls: true, + hoverControls: false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/notifications/index.ts` around lines 25 - 37, The defaultConfig currently enables the new hover UI by setting hoverControls: true; change the default to hoverControls: false in the exported defaultConfig (type NotificationsPluginConfig) so the hover controls remain opt-in for users; update the hoverControls property value in the defaultConfig object to false and run tests/lint to confirm no other references rely on the true default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/hover-popup.html`:
- Around line 98-109: Replace hardcoded UI copy in the hover popup by wiring it
to the app i18n system: remove the literal "No song playing" in the element with
id="title" and the literal title attributes on buttons with ids "prev", "pp",
and "next", and instead set their text/title via your i18n lookup (e.g., using
the same i18n keys pattern used elsewhere) when the popup is created/updated;
ensure you add unique i18n keys like "player.noSong", "player.prevTooltip",
"player.playPauseTooltip", and "player.nextTooltip" to the locale files and
update the popup initialization code to populate `#title.textContent` and the
buttons' title attributes from those keys.
In `@EMRE-FEATURES.md`:
- Around line 101-105: The fenced code block containing the
win.setVisibleOnAllWorkspaces(...) and win.show() example lacks a language tag
and triggers markdownlint MD040; update the triple-backtick fence to include a
language (e.g., "ts") so it reads ```ts before the first line and keep the
closing ``` at the end, ensuring the block exactly surrounds the
win.setVisibleOnAllWorkspaces(true), win.show(),
win.setVisibleOnAllWorkspaces(false) lines.
In `@src/index.ts`:
- Around line 791-798: When trayMoveToCurrentDesktop is enabled the code calls
showOnCurrentDesktop(mainWindow) but doesn't focus the window; update the
second-instance handling so that after calling showOnCurrentDesktop(mainWindow)
you ensure the window is visible and call mainWindow.focus() (similar to the
else branch). Locate the branch that checks
config.get('options.trayMoveToCurrentDesktop') and modify the flow around
showOnCurrentDesktop to mirror the existing behavior that calls
mainWindow.show() and mainWindow.focus() when needed.
In `@src/plugins/audio-only/index.ts`:
- Around line 76-83: The setAudioOnly() method currently sets the
'playback-mode' on the ytmusic-player once but doesn't re-enable the enforcement
performed by lockPlaybackMode() when the player is reacquired; update
setAudioOnly() to call lockPlaybackMode() (or re-run whatever enforcement logic
lockPlaybackMode uses) after finding the player so that every time
setAudioOnly() reacquires the ytmusic-player it both sets the attribute and
re-locks the playback mode; reference the setAudioOnly(), lockPlaybackMode(),
and start() functions and ensure the enforcement is idempotent if called
multiple times.
In `@src/plugins/notifications/hover-popup.ts`:
- Around line 48-50: The popup can finish loading after doShow() ran, so when
popup.webContents emits 'did-finish-load' set popupReady = true and then re-run
the missed first-render steps: call pushSongInfo(...) and window.showPopup() if
there is a pending display (e.g. a stored lastSong/pendingShow flag used by
doShow()). Update the did-finish-load handler to check for that pending state
(or non-null lastSong info) and invoke pushSongInfo and window.showPopup so the
first hover/song change is rendered immediately.
In `@src/plugins/notifications/interactive.ts`:
- Around line 286-289: The toast-suppression check is currently only applied at
one call site, so other callers of sendNotification(...) can still show toasts
while the hover popup is visible; move the visibility check into
sendNotification by having sendNotification call isHoverPopupVisible() at its
start and return early (no toast) when the hover popup is visible, and remove
any duplicate caller-side checks (such as the guard around the call shown) so
the suppression logic is enforced centrally in sendNotification.
In `@src/plugins/notifications/main.ts`:
- Around line 65-67: The hoverControls setting is only applied at startup
because setupHoverPopup(context.window) is never toggled after initial load;
update onConfigChange to detect changes to config.hoverControls and call
setupHoverPopup(context.window) when it becomes true and a matching teardown
function (e.g., teardownHoverPopup or removeHoverPopupListeners) when it becomes
false. Ensure you keep any state needed (listener handles, popup element
references) in module-scope variables so teardownHoverPopup can remove event
listeners and DOM nodes cleanly, and adjust setupHoverPopup to be idempotent if
called multiple times.
In `@src/plugins/playback-recovery/index.ts`:
- Around line 101-145: hookMediaEvents() currently attaches anonymous listeners
and timers that aren't removable; change it to register named handler functions
(e.g., onTimeUpdate, onError, onStalled, onWaiting) and store references and any
timer IDs on the video element (use the existing (video as any).__pbRecovery or
create (video as any).__pbRecoveryHandlers to hold {handlers, timers}). Update
attemptRecovery/getVideo usage if needed, and implement stop() to iterate
attached videos (or use getVideo()/stored references) and call
removeEventListener for each named handler, clearTimeout for stored timer IDs,
and delete the __pbRecovery marker so recovery truly stops. Ensure you reference
hookMediaEvents, stop, getVideo, and attemptRecovery when making the changes.
- Around line 194-205: The watchdog treats a slow-starting track as
"dead-playback" because lastGoodTimestamp defaults to 0; before computing
elapsed in the readyState===0 && !video.paused branch in playback-recovery,
initialize a baseline by setting this.lastGoodTimestamp = now if
this.lastGoodTimestamp === 0 (or otherwise ensure lastGoodTimestamp is no older
than now - stallTimeoutMs) so the first check doesn't immediately exceed
this.config?.stallTimeoutMs; update the logic around lastGoodTimestamp,
stallTimeoutMs, and the attemptRecovery('dead-playback') call to only trigger
recovery after a real period of no progress.
---
Outside diff comments:
In `@src/plugins/notifications/index.ts`:
- Around line 25-37: The defaultConfig currently enables the new hover UI by
setting hoverControls: true; change the default to hoverControls: false in the
exported defaultConfig (type NotificationsPluginConfig) so the hover controls
remain opt-in for users; update the hoverControls property value in the
defaultConfig object to false and run tests/lint to confirm no other references
rely on the true default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f61ac91-955e-4840-a4b8-0e9952c9ec88
📒 Files selected for processing (15)
EMRE-FEATURES.mdassets/hover-popup.htmlsrc/config/defaults.tssrc/i18n/resources/en.jsonsrc/index.tssrc/menu.tssrc/plugins/audio-only/index.tssrc/plugins/notifications/hover-popup.tssrc/plugins/notifications/index.tssrc/plugins/notifications/interactive.tssrc/plugins/notifications/main.tssrc/plugins/notifications/menu.tssrc/plugins/playback-recovery/index.tssrc/tray.tssrc/window-utils.ts
Comment on lines
+98
to
+109
| <div id="title">No song playing</div> | ||
| <div id="artist"></div> | ||
| </div> | ||
| <div id="controls"> | ||
| <button class="btn" id="prev" title="Previous"> | ||
| <svg viewBox="0 0 24 24"><path d="M6 6h2v12H6zM18 6v12l-8.5-6z"/></svg> | ||
| </button> | ||
| <button class="btn pp" id="pp" title="Play/Pause"> | ||
| <svg viewBox="0 0 24 24" id="ppSvg"><path d="M8 5v14l11-7z"/></svg> | ||
| </button> | ||
| <button class="btn" id="next" title="Next"> | ||
| <svg viewBox="0 0 24 24"><path d="M6 18l8.5-6L6 6v12zM16 6v12h2V6h-2z"/></svg> |
There was a problem hiding this comment.
Move the popup copy into i18n.
No song playing plus the three button tooltips are hardcoded here, so this mini-player will stay English even when the rest of the app is translated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/hover-popup.html` around lines 98 - 109, Replace hardcoded UI copy in
the hover popup by wiring it to the app i18n system: remove the literal "No song
playing" in the element with id="title" and the literal title attributes on
buttons with ids "prev", "pp", and "next", and instead set their text/title via
your i18n lookup (e.g., using the same i18n keys pattern used elsewhere) when
the popup is created/updated; ensure you add unique i18n keys like
"player.noSong", "player.prevTooltip", "player.playPauseTooltip", and
"player.nextTooltip" to the locale files and update the popup initialization
code to populate `#title.textContent` and the buttons' title attributes from those
keys.
Comment on lines
+101
to
+105
| ``` | ||
| win.setVisibleOnAllWorkspaces(true) // Pin to all desktops (appears on current) | ||
| win.show() // Show and focus | ||
| win.setVisibleOnAllWorkspaces(false) // Unpin (stays on current desktop) | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
This block is currently tripping markdownlint (MD040).
Suggested fix
-```
+```ts
win.setVisibleOnAllWorkspaces(true) // Pin to all desktops (appears on current)
win.show() // Show and focus
win.setVisibleOnAllWorkspaces(false) // Unpin (stays on current desktop)</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @EMRE-FEATURES.md around lines 101 - 105, The fenced code block containing
the win.setVisibleOnAllWorkspaces(...) and win.show() example lacks a language
tag and triggers markdownlint MD040; update the triple-backtick fence to include
a language (e.g., "ts") so it reads ts before the first line and keep the closing at the end, ensuring the block exactly surrounds the
win.setVisibleOnAllWorkspaces(true), win.show(),
win.setVisibleOnAllWorkspaces(false) lines.
</details>
<!-- fingerprinting:phantom:medusa:grasshopper:65f23a06-22db-4af4-b272-5df95138973f -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+791
to
798
| if (config.get('options.trayMoveToCurrentDesktop')) { | ||
| showOnCurrentDesktop(mainWindow); | ||
| } else { | ||
| if (!mainWindow.isVisible()) { | ||
| mainWindow.show(); | ||
| } | ||
| mainWindow.focus(); | ||
| } |
There was a problem hiding this comment.
Second-instance flow loses focus when desktop-move is enabled.
In the trayMoveToCurrentDesktop branch, the window is shown but not focused. That changes expected second-instance behavior (foregrounding existing window).
Suggested fix
if (config.get('options.trayMoveToCurrentDesktop')) {
showOnCurrentDesktop(mainWindow);
+ mainWindow.focus();
} else {
if (!mainWindow.isVisible()) {
mainWindow.show();
}
mainWindow.focus();
}📝 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
| if (config.get('options.trayMoveToCurrentDesktop')) { | |
| showOnCurrentDesktop(mainWindow); | |
| } else { | |
| if (!mainWindow.isVisible()) { | |
| mainWindow.show(); | |
| } | |
| mainWindow.focus(); | |
| } | |
| if (config.get('options.trayMoveToCurrentDesktop')) { | |
| showOnCurrentDesktop(mainWindow); | |
| mainWindow.focus(); | |
| } else { | |
| if (!mainWindow.isVisible()) { | |
| mainWindow.show(); | |
| } | |
| mainWindow.focus(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 791 - 798, When trayMoveToCurrentDesktop is
enabled the code calls showOnCurrentDesktop(mainWindow) but doesn't focus the
window; update the second-instance handling so that after calling
showOnCurrentDesktop(mainWindow) you ensure the window is visible and call
mainWindow.focus() (similar to the else branch). Locate the branch that checks
config.get('options.trayMoveToCurrentDesktop') and modify the flow around
showOnCurrentDesktop to mirror the existing behavior that calls
mainWindow.show() and mainWindow.focus() when needed.
Comment on lines
+76
to
+83
| setAudioOnly() { | ||
| // 1. Set playback-mode attribute | ||
| const player = document.querySelector<HTMLElement>('ytmusic-player'); | ||
| if (player) { | ||
| player.setAttribute('playback-mode', 'ATV_PREFERRED'); | ||
| player.style.margin = 'auto 0px'; | ||
| } | ||
|
|
There was a problem hiding this comment.
Re-lock playback-mode whenever you reacquire the player.
lockPlaybackMode() only runs during start(). If ytmusic-player is missing then, or YouTube remounts it later, subsequent setAudioOnly() calls only set the attribute once and stop enforcing it, so the player can drift back to video mode.
Suggested fix
setAudioOnly() {
// 1. Set playback-mode attribute
const player = document.querySelector<HTMLElement>('ytmusic-player');
if (player) {
player.setAttribute('playback-mode', 'ATV_PREFERRED');
player.style.margin = 'auto 0px';
+ this.playbackModeObserver?.disconnect();
+ this.lockPlaybackMode(player);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/audio-only/index.ts` around lines 76 - 83, The setAudioOnly()
method currently sets the 'playback-mode' on the ytmusic-player once but doesn't
re-enable the enforcement performed by lockPlaybackMode() when the player is
reacquired; update setAudioOnly() to call lockPlaybackMode() (or re-run whatever
enforcement logic lockPlaybackMode uses) after finding the player so that every
time setAudioOnly() reacquires the ytmusic-player it both sets the attribute and
re-locks the playback mode; reference the setAudioOnly(), lockPlaybackMode(),
and start() functions and ensure the enforcement is idempotent if called
multiple times.
Comment on lines
+48
to
+50
| popup.webContents.on('did-finish-load', () => { | ||
| popupReady = true; | ||
| }); |
There was a problem hiding this comment.
Complete the first render after did-finish-load.
If the first hover happens before the popup finishes loading, doShow() has already shown the window but skipped pushSongInfo() and window.showPopup(). Nothing replays that after popupReady flips, so the first popup can stay blank/invisible until the next hover or song change.
Suggested fix
popup.webContents.on('did-finish-load', () => {
popupReady = true;
+ if (_isVisible) {
+ pushSongInfo();
+ popup.webContents.executeJavaScript('window.showPopup()').catch(() => {});
+ }
});📝 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
| popup.webContents.on('did-finish-load', () => { | |
| popupReady = true; | |
| }); | |
| popup.webContents.on('did-finish-load', () => { | |
| popupReady = true; | |
| if (_isVisible) { | |
| pushSongInfo(); | |
| popup.webContents.executeJavaScript('window.showPopup()').catch(() => {}); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/notifications/hover-popup.ts` around lines 48 - 50, The popup can
finish loading after doShow() ran, so when popup.webContents emits
'did-finish-load' set popupReady = true and then re-run the missed first-render
steps: call pushSongInfo(...) and window.showPopup() if there is a pending
display (e.g. a stored lastSong/pendingShow flag used by doShow()). Update the
did-finish-load handler to check for that pending state (or non-null lastSong
info) and invoke pushSongInfo and window.showPopup so the first hover/song
change is rendered immediately.
Comment on lines
+286
to
+289
| // Don't show toast when hover popup is already visible | ||
| if (!isHoverPopupVisible()) { | ||
| sendNotification(songInfo); | ||
| } |
There was a problem hiding this comment.
Toast suppression is only partial right now.
This guard covers one call path, but other sendNotification(...) callers can still emit toasts while the hover popup is visible. Move the visibility check into sendNotification to enforce the rule consistently.
Suggested fix
const sendNotification = (songInfo: SongInfo) => {
+ if (isHoverPopupVisible()) {
+ return;
+ }
const iconSrc = notificationImage(songInfo, config());
@@
- // Don't show toast when hover popup is already visible
- if (!isHoverPopupVisible()) {
- sendNotification(songInfo);
- }
+ sendNotification(songInfo);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/notifications/interactive.ts` around lines 286 - 289, The
toast-suppression check is currently only applied at one call site, so other
callers of sendNotification(...) can still show toasts while the hover popup is
visible; move the visibility check into sendNotification by having
sendNotification call isHoverPopupVisible() at its start and return early (no
toast) when the hover popup is visible, and remove any duplicate caller-side
checks (such as the guard around the call shown) so the suppression logic is
enforced centrally in sendNotification.
Comment on lines
+65
to
+67
| if (config.hoverControls) { | ||
| setupHoverPopup(context.window); | ||
| } |
There was a problem hiding this comment.
hoverControls toggle is not applied dynamically.
setupHoverPopup is only invoked during initial load. After config changes, onConfigChange updates data only, so enabling/disabling hoverControls from the menu won’t actually start/stop the popup behavior in-session.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/notifications/main.ts` around lines 65 - 67, The hoverControls
setting is only applied at startup because setupHoverPopup(context.window) is
never toggled after initial load; update onConfigChange to detect changes to
config.hoverControls and call setupHoverPopup(context.window) when it becomes
true and a matching teardown function (e.g., teardownHoverPopup or
removeHoverPopupListeners) when it becomes false. Ensure you keep any state
needed (listener handles, popup element references) in module-scope variables so
teardownHoverPopup can remove event listeners and DOM nodes cleanly, and adjust
setupHoverPopup to be idempotent if called multiple times.
Comment on lines
+101
to
+145
| hookMediaEvents() { | ||
| const attachTo = (video: HTMLVideoElement) => { | ||
| if ((video as any).__pbRecovery) return; | ||
| (video as any).__pbRecovery = true; | ||
|
|
||
| // Track healthy playback progress | ||
| video.addEventListener('timeupdate', () => { | ||
| if (video.currentTime > 0 && !video.paused) { | ||
| this.lastGoodTime = video.currentTime; | ||
| this.lastGoodTimestamp = Date.now(); | ||
| this.consecutiveFailures = 0; | ||
| this.recovering = false; | ||
| } | ||
| }); | ||
|
|
||
| // On error, attempt immediate recovery | ||
| video.addEventListener('error', () => { | ||
| const code = video.error?.code ?? 0; | ||
| const msg = video.error?.message ?? ''; | ||
| this.log(`Media error: code=${code} msg="${msg}"`); | ||
| this.attemptRecovery('media-error'); | ||
| }); | ||
|
|
||
| // On stall, start a short timer — if it doesn't resolve, recover | ||
| video.addEventListener('stalled', () => { | ||
| this.log('Stream stalled, waiting for recovery...'); | ||
| setTimeout(() => { | ||
| const v = this.getVideo(); | ||
| if (v && !v.paused && v.readyState < 3) { | ||
| this.log('Stall did not resolve'); | ||
| this.attemptRecovery('stall-timeout'); | ||
| } | ||
| }, this.config?.stallTimeoutMs ?? 8000); | ||
| }); | ||
|
|
||
| // On waiting (buffering), monitor if it persists too long | ||
| video.addEventListener('waiting', () => { | ||
| setTimeout(() => { | ||
| const v = this.getVideo(); | ||
| if (v && !v.paused && v.readyState < 2) { | ||
| this.log('Buffering did not resolve'); | ||
| this.attemptRecovery('buffer-timeout'); | ||
| } | ||
| }, this.config?.stallTimeoutMs ?? 8000); | ||
| }); |
There was a problem hiding this comment.
stop() doesn't actually stop the recovery hooks.
These listeners are attached with anonymous callbacks and never removed, so toggling the plugin off leaves timeupdate / error / stalled / waiting handlers and their follow-up timers alive. Since restartNeeded is false, recovery can keep mutating playback after the plugin is disabled.
🧰 Tools
🪛 ESLint
[error] 103-103: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 104-104: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/playback-recovery/index.ts` around lines 101 - 145,
hookMediaEvents() currently attaches anonymous listeners and timers that aren't
removable; change it to register named handler functions (e.g., onTimeUpdate,
onError, onStalled, onWaiting) and store references and any timer IDs on the
video element (use the existing (video as any).__pbRecovery or create (video as
any).__pbRecoveryHandlers to hold {handlers, timers}). Update
attemptRecovery/getVideo usage if needed, and implement stop() to iterate
attached videos (or use getVideo()/stored references) and call
removeEventListener for each named handler, clearTimeout for stored timer IDs,
and delete the __pbRecovery marker so recovery truly stops. Ensure you reference
hookMediaEvents, stop, getVideo, and attemptRecovery when making the changes.
Comment on lines
+194
to
+205
| const now = Date.now(); | ||
|
|
||
| // Case 1: readyState 0 while "playing" — completely stuck | ||
| if (video.readyState === 0 && !video.paused) { | ||
| const elapsed = now - this.lastGoodTimestamp; | ||
| if (elapsed > (this.config?.stallTimeoutMs ?? 8000)) { | ||
| this.log( | ||
| `Dead playback: readyState=0, no progress for ${elapsed}ms`, | ||
| ); | ||
| this.attemptRecovery('dead-playback'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Initialize the dead-playback baseline before comparing against the stall timeout.
lastGoodTimestamp starts at 0, so now - this.lastGoodTimestamp is immediately greater than stallTimeoutMs. A slow-starting track can therefore be treated as dead playback on the very first watchdog pass.
Suggested fix
const now = Date.now();
+ if (this.lastGoodTimestamp === 0) {
+ this.lastGoodTimestamp = now;
+ }
// Case 1: readyState 0 while "playing" — completely stuck
if (video.readyState === 0 && !video.paused) {
const elapsed = now - this.lastGoodTimestamp;📝 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
| const now = Date.now(); | |
| // Case 1: readyState 0 while "playing" — completely stuck | |
| if (video.readyState === 0 && !video.paused) { | |
| const elapsed = now - this.lastGoodTimestamp; | |
| if (elapsed > (this.config?.stallTimeoutMs ?? 8000)) { | |
| this.log( | |
| `Dead playback: readyState=0, no progress for ${elapsed}ms`, | |
| ); | |
| this.attemptRecovery('dead-playback'); | |
| return; | |
| } | |
| const now = Date.now(); | |
| if (this.lastGoodTimestamp === 0) { | |
| this.lastGoodTimestamp = now; | |
| } | |
| // Case 1: readyState 0 while "playing" — completely stuck | |
| if (video.readyState === 0 && !video.paused) { | |
| const elapsed = now - this.lastGoodTimestamp; | |
| if (elapsed > (this.config?.stallTimeoutMs ?? 8000)) { | |
| this.log( | |
| `Dead playback: readyState=0, no progress for ${elapsed}ms`, | |
| ); | |
| this.attemptRecovery('dead-playback'); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/playback-recovery/index.ts` around lines 194 - 205, The watchdog
treats a slow-starting track as "dead-playback" because lastGoodTimestamp
defaults to 0; before computing elapsed in the readyState===0 && !video.paused
branch in playback-recovery, initialize a baseline by setting
this.lastGoodTimestamp = now if this.lastGoodTimestamp === 0 (or otherwise
ensure lastGoodTimestamp is no older than now - stallTimeoutMs) so the first
check doesn't immediately exceed this.config?.stallTimeoutMs; update the logic
around lastGoodTimestamp, stallTimeoutMs, and the
attemptRecovery('dead-playback') call to only trigger recovery after a real
period of no progress.
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.
Summary
Four opt-in features for power users who run YouTube Music as a background audio companion — particularly those who use virtual desktops, minimize to tray, and want quick playback control without opening the full window.
Every feature defaults to off and is toggled from the existing settings/plugin menu. No existing behavior is changed unless the user explicitly enables a feature.
Features
Audio-Only Mode (plugin): Forces audio-only streaming, eliminates video decoding, saves ~300 MB RAM. Uses
playback-mode="ATV_PREFERRED"+ quality lock + MutationObserver to prevent YouTube from switching back.Playback Recovery (plugin): Watchdog that detects stuck/stalled/dead playback every 3 seconds and applies progressive recovery (seek → seek-forward → skip to next). Handles media errors, stream stalls, buffer exhaustion, and video element recreation.
Virtual Desktop Awareness (core setting, Options > Tray): Clicking the tray icon moves the window to your current virtual desktop instead of yanking you back to the original one. Uses
setVisibleOnAllWorkspacespin/unpin technique. Works on Windows, macOS Spaces, and Linux workspaces.Tray Hover Mini-Player (notification plugin extension): Hover the tray icon to see a floating mini-player with album art, song info, and prev/play-pause/next buttons. Stays visible while your mouse is on it. Suppresses toast notifications while visible to avoid stacking.
Infrastructure fix
Plugins load before the tray is created, so
setTrayOnClick/setTrayOnDoubleClick/setTrayOnMouseMovecalls from plugins were silently dropped. Fixed by queuing handlers and applying them after tray creation. This also fixes the same latent bug for the existingtrayControlsfeature in the notification plugin.Design decisions
EMRE-FEATURES.mdFiles changed
src/plugins/audio-only/index.ts(new)src/plugins/playback-recovery/index.ts(new)src/window-utils.ts(new),src/tray.ts,src/index.ts,src/config/defaults.ts,src/menu.tssrc/plugins/notifications/hover-popup.ts(new),assets/hover-popup.html(new),src/plugins/notifications/{index,main,menu,interactive}.ts,src/tray.tssrc/i18n/resources/en.jsonEMRE-FEATURES.md(new)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit