fix: add clean up storage event listeners on Settings unmount to prevent memory leaks - #1179
Conversation
|
@hrshjswniii is attempting to deploy a commit to the itzzavdhesh's projects Team on Vercel. A member of the Team first needs to authorize it. |
✍️ DCO Sign-off NeededHey @hrshjswniii! 👋 One or more commits in this PR are missing a Warning
How to fix: For the latest commit: git commit --amend --signoff
git push --force-with-leaseFor multiple commits, replace git rebase --signoff HEAD~N
git push --force-with-leaseThis comment will update automatically after you push. 🤖 VoiceForge Automation · Updates automatically on edits |
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
github-actions
Bot
added
assignee-match
bug
🎉 PR Ready for Mentor ReviewHey @hrshjswniii! 👋 Your PR passed all checks and is now in the GSSoC review queue. Note 🔗 Closing: #1118 · 📐 42 lines across 3 file(s) · 📬 Already requested or no eligible reviewer found @sabeenaviklar @Anushreebasics @itsdakshjain @snehkris @1754riya @Mrigakshi-Rathore @Itzzavdheshh @Nitya-003 @4f4d @lovestaco, this PR is ready for your review — please confirm scope, check behavior and tests, then approve or request changes. Important This is not an approval. Please wait for mentor feedback before expecting a merge. If changes are requested, push them to this same branch and keep the PR focused on the linked issue. 🤖 VoiceForge Automation · Updates automatically on edits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/src/hooks/useVoiceClone.test.js">
<violation number="1" location="client/src/hooks/useVoiceClone.test.js:9">
P3: The `subscribeProfileChanges` function handles two event sources: `voiceforge:profileChanged` custom events and browser `storage` events (filtered by `e.key === ACTIVE_KEY || !e.key`). The test only covers the custom event path. The storage-event filtering logic — which has a non-trivial condition — is untested. Consider adding a test case that dispatches a `StorageEvent` for the `ACTIVE_KEY` and asserts the callback fires, and another for an unrelated key to verify filtering.</violation>
<violation number="2" location="client/src/hooks/useVoiceClone.test.js:20">
P2: Cleanup verification missing after unsubscribe — the test name says 'cleans up profile change event listeners' but never verifies the cleanup succeeded. After calling unsubscribe(), dispatch the event again and assert callback was NOT called (toHaveBeenCalledTimes(1)). Without this, a broken no-op unsubscribe would still pass.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| expect(callback).toHaveBeenCalledTimes(1); | ||
| } | ||
|
|
||
| unsubscribe(); |
There was a problem hiding this comment.
P2: Cleanup verification missing after unsubscribe — the test name says 'cleans up profile change event listeners' but never verifies the cleanup succeeded. After calling unsubscribe(), dispatch the event again and assert callback was NOT called (toHaveBeenCalledTimes(1)). Without this, a broken no-op unsubscribe would still pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/hooks/useVoiceClone.test.js, line 20:
<comment>Cleanup verification missing after unsubscribe — the test name says 'cleans up profile change event listeners' but never verifies the cleanup succeeded. After calling unsubscribe(), dispatch the event again and assert callback was NOT called (toHaveBeenCalledTimes(1)). Without this, a broken no-op unsubscribe would still pass.</comment>
<file context>
@@ -0,0 +1,22 @@
+ expect(callback).toHaveBeenCalledTimes(1);
+ }
+
+ unsubscribe();
+ });
+});
</file context>
| expect(typeof useVoiceClone).toBe("function"); | ||
| }); | ||
|
|
||
| it("subscribes and cleans up profile change event listeners", () => { |
There was a problem hiding this comment.
P3: The subscribeProfileChanges function handles two event sources: voiceforge:profileChanged custom events and browser storage events (filtered by e.key === ACTIVE_KEY || !e.key). The test only covers the custom event path. The storage-event filtering logic — which has a non-trivial condition — is untested. Consider adding a test case that dispatches a StorageEvent for the ACTIVE_KEY and asserts the callback fires, and another for an unrelated key to verify filtering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/hooks/useVoiceClone.test.js, line 9:
<comment>The `subscribeProfileChanges` function handles two event sources: `voiceforge:profileChanged` custom events and browser `storage` events (filtered by `e.key === ACTIVE_KEY || !e.key`). The test only covers the custom event path. The storage-event filtering logic — which has a non-trivial condition — is untested. Consider adding a test case that dispatches a `StorageEvent` for the `ACTIVE_KEY` and asserts the callback fires, and another for an unrelated key to verify filtering.</comment>
<file context>
@@ -0,0 +1,22 @@
+ expect(typeof useVoiceClone).toBe("function");
+ });
+
+ it("subscribes and cleans up profile change event listeners", () => {
+ const callback = vi.fn();
+ const unsubscribe = subscribeProfileChanges(callback);
</file context>
Nitya-003
left a comment
Nitya-003
left a comment
There was a problem hiding this comment.
@hrshjswniii Resolve the comments by bot.
github-actions
Bot
added
mentor:Nitya-003
🔄 Changes RequestedHey @hrshjswniii! 👋 A mentor has reviewed your PR and requested some changes. Warning Please review the feedback above, update this same branch, and keep the PR focused on the linked issue. Once you push your updates, the review flow will continue automatically on this same PR. 🤖 VoiceForge Automation · Updates automatically on edits |
itsdakshjain
left a comment
itsdakshjain
left a comment
There was a problem hiding this comment.
Please address the feedback from the Cubic AI bot regarding the test file (useVoiceClone.test.js). Specifically, make sure to add assertions verifying that the cleanup function successfully removes event listeners after unsubscribing, and add test cases covering the storage event key-filtering logic.
sabeenaviklar
left a comment
sabeenaviklar
left a comment
There was a problem hiding this comment.
Hi @hrshjswniii
Good work on the PR!
A few changes are required before the PR can be merged.
Verify Unsubscribe in Tests: The added test confirms that the callback works but doesn't verify the cleanup. Please dispatch an event after calling the unsubscribe cleanup function and assert that the callback is not triggered again.
Test Storage Filtering: Since your hook checks for a specific active profile key in storage events, please add a test to verify that the storage listener responds to the correct key and ignores unrelated key events.
itsdakshjain
left a comment
itsdakshjain
left a comment
There was a problem hiding this comment.
Are you still working on it? @hrshjswniii @itzzavdhesh
🎊 PR Merged SuccessfullyHey @hrshjswniii! 👋 Congratulations and thank you for your contribution to VoiceForge! Note 🔗 Linked issue(s): #1118 · ✅ Marked as merged and complete Maintainers may still handle final cleanup, release notes, or follow-up tracking after the merge. 🤖 VoiceForge Automation · Updates automatically on edits |
itzzavdhesh
added
enhancement
5 participants
🚀 Program
GSSoC
📝 Description
This PR resolves memory leaks and duplicate event handler execution caused by uncleaned storage event listeners in
Settings.jsx.Key fixes:
subscribeProfileChanges): ExportedsubscribeProfileChanges(callback)inuseVoiceClone.jslistening tovoiceforge:profileChangedcustom events and browserstorageevents. Returns an explicit cleanup function removing both listeners on component unmount.Settings.jsx): Refactored profile loading inSettings.jsxto returnsubscribeProfileChanges(loadProfiles)inuseEffect, guaranteeing all event listeners are cleanly unbound upon unmounting.useVoiceClone.test.js): Added Vitest test assertions verifying profile subscription registration and unmount cleanup.🔗 Related Issue
Closes #1118
🔄 Type of Change
🧪 How to Test
http://localhost:5173in a browser.npm run test --workspace clientand verify all tests pass.✅ Checklist
fix: clean up storage event listeners on Settings unmount to prevent memory leaks)Summary by cubic
Prevents memory leaks and duplicate profile updates by cleaning up
storageandvoiceforge:profileChangedlisteners whenSettingsunmounts. AddssubscribeProfileChangesinuseVoiceClone.jsand tests to verify cleanup.subscribeProfileChanges(callback)to listen tovoiceforge:profileChangedandstorage, returning an unsubscribe.Settings.jsxto use it inuseEffect, removing listeners on unmount.useVoiceClone.test.jsto assert subscription and cleanup.Written for commit f5eda2b. Summary will update on new commits.