feat(memory-saver): Add plugin that reloads the page to reclaim leaked memory - #4617
feat(memory-saver): Add plugin that reloads the page to reclaim leaked memory#4617danielchalmers wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates Memory Saver’s shuffle-aware reload gating by caching ChangesMemory Saver Plugin
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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/plugins/memory-saver/index.ts`:
- Around line 85-86: Update the reload flow around lastRequestAt and
ipc.send(RELOAD_CHANNEL) so the cooldown starts when the renderer actually
performs the reload rather than when it is requested. Either acknowledge
reload() completion back to the backend before updating lastRequestAt, or
conservatively add GRACE_MS to the next eligible-time calculation.
- Around line 102-107: Update the video-data handling around getVideoData so the
pending state is cleared and graceTimeout is cancelled and reset before
returning when videoId is missing. Preserve the existing cleanup behavior for
valid video data and ensure later IPC requests are not blocked by a stale
pending state.
- Around line 149-175: Store the RELOAD_CHANNEL callback registered in the
plugin startup flow as a renderer-state listener, rather than an inline
anonymous function. In stop(), remove that stored callback with
ipc.off(RELOAD_CHANNEL, reloadListener) and clear the stored reference so
disabling and re-enabling does not retain stale 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b35f4d8-c8ce-487f-bb8b-67acfb32e448
📒 Files selected for processing (2)
src/i18n/resources/en.jsonsrc/plugins/memory-saver/index.ts
| const { video_id: videoId, list } = this.api?.getVideoData() ?? {}; | ||
| if (!videoId) return; | ||
|
|
||
| this.pending = false; | ||
| if (this.graceTimeout) clearTimeout(this.graceTimeout); | ||
| this.graceTimeout = null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear pending state before returning for missing video data.
If video_id is unavailable, this returns with pending still true and the grace timer retained. Later IPC requests are ignored at line 150 until a dataloaded event happens.
Proposed fix
const { video_id: videoId, list } = this.api?.getVideoData() ?? {};
- if (!videoId) return;
-
this.pending = false;
if (this.graceTimeout) clearTimeout(this.graceTimeout);
this.graceTimeout = null;
+ if (!videoId) return;📝 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.
| const { video_id: videoId, list } = this.api?.getVideoData() ?? {}; | |
| if (!videoId) return; | |
| this.pending = false; | |
| if (this.graceTimeout) clearTimeout(this.graceTimeout); | |
| this.graceTimeout = null; | |
| const { video_id: videoId, list } = this.api?.getVideoData() ?? {}; | |
| this.pending = false; | |
| if (this.graceTimeout) clearTimeout(this.graceTimeout); | |
| this.graceTimeout = null; | |
| if (!videoId) return; |
🤖 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/plugins/memory-saver/index.ts` around lines 102 - 107, Update the
video-data handling around getVideoData so the pending state is cleared and
graceTimeout is cancelled and reset before returning when videoId is missing.
Preserve the existing cleanup behavior for valid video data and ensure later IPC
requests are not blocked by a stale pending state.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/plugins/memory-saver/index.ts (1)
106-126: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClear the interval when the window is destroyed.
The cooldown fix here (
COOLDOWN_MS + GRACE_MS) correctly resolves the previously flagged anchoring issue. One small leftover: whenwindow.isDestroyed()is true, the interval keeps ticking indefinitely (harmless early-return) instead of being cleared. Consider clearing it here too, rather than relying on a futurestart()call to replace it.♻️ Optional cleanup
this.interval = setInterval(async () => { - if (window.isDestroyed()) return; + if (window.isDestroyed()) { + clearInterval(this.interval); + return; + }🤖 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/plugins/memory-saver/index.ts` around lines 106 - 126, Update the interval callback in start so that when window.isDestroyed() is true, it clears this.interval before returning; preserve the existing early-return behavior for live windows.
🤖 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 `@src/plugins/memory-saver/index.ts`:
- Around line 106-126: Update the interval callback in start so that when
window.isDestroyed() is true, it clears this.interval before returning; preserve
the existing early-return behavior for live windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bdb29f5f-8512-4b9f-bf7f-caa94c7cfda9
📒 Files selected for processing (2)
src/i18n/resources/en.jsonsrc/plugins/memory-saver/index.ts
|
How much of this PR was actually done by you |
The vast bulk of this was Fable. I ran it to investigate the problem for my own use and wanted to post the results in case the findings would help anyone else as it took a few hours to get through. Would like to see if this is on the right track so I can help plan/pivot/diagnose further if it's a direction we want to take so I can become more familiar with the project as a whole. Is there a repo rule against that? I can close it if so and keep it forked instead, no problem. Will update the description in the meantime. |
…d memory The web player itself leaks memory over long sessions, so the app gets progressively slower and eventually runs out of memory. Measured on a real playlist, the renderer accumulates ~130 detached DOM nodes, ~33 event listeners and ~0.45 MB of heap per track change, which works out to roughly 370k nodes and over 1 GB across a week of continuous playback. Reloading the page reclaims all of it. This plugin watches the renderer's working set from the backend and, once it is over a configurable threshold, reloads at the least disruptive moment: only while the user is away from the keyboard, and only in a gap in playback, keeping the current song, position and shuffle state intact.
27c816e to
65433f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugins/memory-saver/index.ts (1)
161-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: DOM listeners aren't defensively cleared on re-entry like the IPC channel is.
Line 164 defensively clears the IPC listener because "this can run again on plugin re-enable," but the
pause/videodatachangelisteners added at lines 182-185 aren't similarly guarded. In practice,stop()already removes them on disable, and any duplicatereload()call is cut short by the immediatelocation.replace, so impact is minimal — but for consistency with the IPC defensive pattern, consider removing before re-adding.🤖 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/plugins/memory-saver/index.ts` around lines 161 - 186, Update onPlayerApiReady to defensively remove the existing video pause listener and document videodatachange listener before adding them again, matching the IPC listener cleanup pattern and preventing duplicate handlers on re-entry.
🤖 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/plugins/memory-saver/index.ts`:
- Around line 171-180: In the RELOAD_CHANNEL handler and the pending reload
flow, add a 30-minute force-reload timer so pending requests eventually invoke
reload() even without a pause or track change. Store the timer in
forceReloadTimeout, avoid duplicate timers while pending, and clear it in stop()
alongside the existing cleanup.
---
Nitpick comments:
In `@src/plugins/memory-saver/index.ts`:
- Around line 161-186: Update onPlayerApiReady to defensively remove the
existing video pause listener and document videodatachange listener before
adding them again, matching the IPC listener cleanup pattern and preventing
duplicate handlers on re-entry.
🪄 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 Plus
Run ID: 68fd5eb4-16b0-4565-89bb-741f58f1e8be
📒 Files selected for processing (2)
src/i18n/resources/en.jsonsrc/plugins/memory-saver/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/i18n/resources/en.json
Reading the raw store misses plugin config defaults; isEnabled() is the API every other cross-plugin check in the repo uses. Resolved once per reload request so the reload gate itself stays synchronous. Also drops trailing periods from comments to match the surrounding code.
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/plugins/memory-saver/index.ts`:
- Around line 165-176: Update the RELOAD_CHANNEL handler and stop() lifecycle in
the memory-saver plugin to use a generation token that invalidates in-flight
reload requests during teardown. Capture the token when handling a request, then
after await window.mainConfig.plugins.isEnabled('resume-shuffle') resolves,
verify the token still matches the active lifecycle before assigning
resumeShuffleEnabled, setting pending, or calling tryReload; increment the token
during stop() so stale handlers cannot affect a later enabled instance.
🪄 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 Plus
Run ID: 6fc16a5c-3a88-4b4d-951f-3e16efd59970
📒 Files selected for processing (1)
src/plugins/memory-saver/index.ts
…fle plugin The reload gate referenced a plugin that only exists in another open PR, which complicates reviewing this one on its own. Shuffle safety stays: the plugin simply never reloads while shuffle is on. A small follow-up can allow those reloads again once a way to restore the shuffle exists.
2 participants
Adds an opt-in plugin that reloads the page when the renderer's memory crosses a threshold, working around the web player's memory leak behind the long-session slowdown and crash reports (#4450, #1722, #1158, #797, #322, #295).
🤖 Claude measured over the DevTools protocol against the real app:
Performance.getMetricssampled after every track change on a 50-track playlist, with GC forced before each sample so growth reflects actual retention.The rate is unchanged with our code completely absent, so the leak is upstream and can't be fixed here. The retained nodes are detached (the attached DOM grew by 6 elements while total nodes grew ~1,500), which is why the UI degrades rather than just growing. Over a week of continuous playback that's roughly 370k nodes and over 1 GB. A reload reclaims all of it: in one test, 83 to 73 MB of heap and 43.8k to 41.6k nodes.
Behavior
powerMonitor.getSystemIdleTime()), since a reload lands on the song page. The threshold isthresholdMBin config.json (default 1200).Summary by CodeRabbit