[BUG] : fixed Toast Timer Memory Leak in useToast.jsx - #1154
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 |
github-actions
Bot
added
assignee-match
bug
📝 WalkthroughWalkthroughThe ChangesToast timer cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
🎉 PR Ready for Mentor ReviewHey @hrshjswniii! 👋 Your PR passed all checks and is now in the GSSoC review queue. Note 🔗 Closing: #1140 · 📐 17 lines across 2 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 |
![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
🧹 Nitpick comments (1)
client/src/components/useToast.test.js (1)
4-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd behavioral coverage for timer cleanup.
This test passes even if timer references are never initialized, removed, or reset. Add hook tests covering multiple toasts, timer-driven dismissal, final-toast cleanup, and unmount cleanup using the project’s hook test harness and fake timers.
🤖 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 `@client/src/components/useToast.test.js` around lines 4 - 7, Expand the useToast hook tests beyond export validation to cover multiple toasts, fake-timer dismissal, cleanup after the final toast, and cleanup on unmount. Use the project’s existing hook test harness and fake-timer utilities, and assert timer references are initialized, cleared, and reset through the useToast behavior.
🤖 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 `@client/src/components/useToast.jsx`:
- Around line 22-29: Move the timers.current reset out of the setToasts
functional updater in the toast removal flow. In the timeout/removal logic, only
clear the ref when committed toast state is empty or no active timers remain, so
batched updates cannot erase timer tracking for newly added toasts; preserve
filtering by toast.id and cleanup of the removed timer.
---
Nitpick comments:
In `@client/src/components/useToast.test.js`:
- Around line 4-7: Expand the useToast hook tests beyond export validation to
cover multiple toasts, fake-timer dismissal, cleanup after the final toast, and
cleanup on unmount. Use the project’s existing hook test harness and fake-timer
utilities, and assert timer references are initialized, cleared, and reset
through the useToast behavior.
🪄 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: 7e699dd1-9720-4691-90dd-7306adfe2a1e
📒 Files selected for processing (2)
client/src/components/useToast.jsxclient/src/components/useToast.test.js
| delete timers.current[id]; | ||
| setToasts((previous) => { | ||
| const nextToasts = previous.filter((toast) => toast.id !== id); | ||
| if (nextToasts.length === 0) { | ||
| timers.current = {}; | ||
| } | ||
| return nextToasts; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file and surrounding implementation.
if [ -f client/src/components/useToast.jsx ]; then
echo "== file exists =="
realpath client/src/components/useToast.jsx
wc -l client/src/components/useToast.jsx
echo "== full file =="
cat -n client/src/components/useToast.jsx
else
echo "client/src/components/useToast.jsx not found"
fd -a 'useToast.jsx|useToast.js|Toast.*' .
fi
echo "== usages of useToast / timers =="
rg -n "useToast|timers\.current|setTimeout|clearTimeout|delete timers" client/src || trueRepository: itzzavdhesh/VoiceForge
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== list matching files =="
find client src components 2>/dev/null | grep -E '(^|/)useToast\.jsx$|(^|/)useToast\.js$|Toast.*' || true
echo "== inspect relevant file =="
if [ -f client/src/components/useToast.jsx ]; then
wc -l client/src/components/useToast.jsx
cat -n client/src/components/useToast.jsx
else
echo "client/src/components/useToast.jsx not found"
fi
echo "== usages of useToast / timers / toast cleanup =="
grep -RIn -E "useToast|timers\.current|setTimeout|clearTimeout|delete timers" client src 2>/dev/null || trueRepository: itzzavdhesh/VoiceForge
Length of output: 5183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call site context in Call.jsx =="
sed -n '1,45p' client/src/pages/Call.jsx 2>/dev/null || true
echo "== Call.jsx call/export occurrences =="
grep -n "showToast|useToast|ToastContainer" client/src/pages/Call.jsx 2>/dev/null || true
echo "== other call sites for useToast context =="
sed -n '1,80p' client/src/pages/Settings.jsx 2>/dev/null || true
grep -n "showToast|useToast|ToastContainer" client/src/pages/Settings.jsx 2>/dev/null || true
sed -n '150,220p' client/src/components/VoiceForge.jsx 2>/dev/null || true
grep -n "showToast|useToast|ToastContainer" client/src/components/VoiceForge.jsx 2>/dev/null || true
echo "== package/react version hints =="
for f in package.json client/package.json package-lock.json yarn.lock pnpm-lock.yaml; do
[ -f "$f" ] && { echo "-- $f"; grep -E '"react"|react@|versions|dependencies|devDependencies' "$f" | head -80 || true; }
doneRepository: itzzavdhesh/VoiceForge
Length of output: 9896
🌐 Web query:
React updates apply to state inside React event handlers batched synchronous functional component state updater may be deferred queueing updates
💡 Result:
In React, state updates within event handlers are batched to optimize performance, meaning React waits until all code in the event handler has executed before processing state updates and triggering a re-render [1][2]. This prevents unnecessary intermediate re-renders and ensures the UI remains consistent [1]. Key points regarding this behavior include: State Updater Functions and Queueing: When you call a state setter function, React does not immediately update the state variable in the current render [1][2]. Instead, it queues the update [1]. If you pass an updater function (e.g., setNumber(n => n + 1)), React adds that function to a queue [1][2]. During the subsequent re-render, React processes this queue by applying the updater functions in order to the pending state to calculate the final state value [1][2]. Batching and Asynchronous Code: - Synchronous Code: Multiple state updates within the synchronous portion of an event handler are batched together [1][3][4]. - Asynchronous Code: React's automatic batching is tied to the event loop tick [5]. If state updates occur after an asynchronous boundary (such as after an await keyword, within a Promise callback, or in a setTimeout), they may be treated as separate updates and fall outside the initial batch, potentially causing additional re-renders [3][4][5]. While React 18+ introduced automatic batching for updates outside of event handlers (like in Promises or timeouts), asynchronous operations that break the synchronous execution flow can still prevent related updates from being batched together if they occur in different ticks of the event loop [4][5][6]. Forcing Synchronous Updates: If you need to force an update to be applied immediately (for instance, to access the updated DOM), you can use the flushSync API from react-dom [2][7][8]. This forces React to process the updates synchronously and re-render the component before proceeding with subsequent code [2][7].
Citations:
- 1: https://react.dev/learn/queueing-a-series-of-state-updates
- 2: https://react.dev/reference/react/useState
- 3: Bug: SetState Calls not being batched inside Promise calls/micro Task react/react#30605
- 4: https://stackoverflow.com/questions/56581542/react-wont-batch-setstate-calls-inside-event-handler-if-theres-an-await-call-b
- 5: https://stackoverflow.com/questions/79464505/how-react-will-schedules-batch-updates-if-we-have-async-code-in-event-handler-fu
- 6: https://stackoverflow.com/questions/53048495/does-react-batch-state-update-functions-when-using-hooks
- 7: Batching makes it difficult to perform imperative actions like focus react/react#18402
- 8: Bug: is the current
useSyncExternalStorebatching & flushing behaviour intended? react/react#25191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Read-only semantic probe: model React's updater queue vs a ref mutation embedded
# in the updater. The goal is to show that clearing timers.current in the updater
# can remove slots populated by later-to-run queueing code before cleanup can.
class TimerRef:
def __init__(self):
self.current = {}
class StateQueue:
def __init__(self, initial=None):
self.pending = []
self.current = initial
self.ref = TimerRef()
def schedule(self, updater):
self.pending.append(updater)
def process(self):
value = self.current
while self.pending:
updater = self.pending.pop(0)
value = updater(value)
return value
def current_model():
q = StateQueue([])
q.ref.current[1] = "timer1"
q.schedule(lambda previous: [*(i for i in previous if i != 1)])
q.ref.current[2] = "timer2"
q.schedule(lambda previous: [*(i for i in previous if i != 2)][:0] and [] and setattr(q.ref, "current", {}))
result = q.process()
return result, q.ref.current
def fixed_model():
q = StateQueue([])
q.ref.current[1] = "timer1"
q.schedule(lambda previous: [*(i for i in previous if i != 1)])
q.ref.current[2] = "timer2"
# Defer ref cleanup until after scheduled updaters are queued/run.
q.schedule(lambda previous: (setattr(q.ref, "current", {}), nextToasts := [*(i for i in previous if i != 2)] and nextToasts)[1])
result = q.process()
return result, q.ref.current
print("current_model_toast_ids", current_model()[0])
print("current_model_timers", current_model()[1])
print("fixed_model_timers", fixed_model()[1])
PYRepository: itzzavdhesh/VoiceForge
Length of output: 257
Do not reset timers.current while batching toast state updates.
React queues functional updaters before committing state. If the timeout queues its update, but a newer toast is added before that updater runs and timers.current is empty, the queued update can clear the newer toast from state while its timer remains in the ref, preventing unmount cleanup from clearing it. Move the ref reset outside the updater and guard it with committed state or an active-timer count.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 20-29: Avoid using the initial state variable in setState
Context: setTimeout(() => {
delete timers.current[id];
setToasts((previous) => {
const nextToasts = previous.filter((toast) => toast.id !== id);
if (nextToasts.length === 0) {
timers.current = {};
}
return nextToasts;
});
}, duration)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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 `@client/src/components/useToast.jsx` around lines 22 - 29, Move the
timers.current reset out of the setToasts functional updater in the toast
removal flow. In the timeout/removal logic, only clear the ref when committed
toast state is empty or no active timers remain, so batched updates cannot erase
timer tracking for newly added toasts; preserve filtering by toast.id and
cleanup of the removed timer.
![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 2 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/components/useToast.jsx">
<violation number="1" location="client/src/components/useToast.jsx:25">
P1: The `timers.current = {}` inside the `setToasts` functional updater creates a race condition that can lose newly-created timer references. If a new toast is shown (creating a new timer entry in `timers.current`) between when `delete timers.current[id]` runs synchronously and when React executes this functional updater, the `nextToasts.length === 0` branch will reset `timers.current = {}` and silently drop the new timer — that toast will never auto-dismiss.
**How it happens (example):**
1. Only toast A exists. Timer A fires → `delete timers.current[aId]` runs → `timers.current` is now `{}`.
2. Before React processes the setToasts updater, an event handler shows toast B → `timers.current[bId] = timerB` is set.
3. The setToasts updater runs with `previous = [{id: aId}]` → filters out aId → `nextToasts` is `[]` → `timers.current = {}` wipes timerB.
4. Toast B is in state but its timer is gone — it persists until the page reloads.
**Recommendation:** Remove the `timers.current = {}` branch inside the updater. The `delete timers.current[id]` on line 22 already handles per-timer cleanup. If the goal is to fully reset `timers.current` when all toasts are dismissed, do it outside the updater (or skip it entirely — unused ref entries are negligible memory).</violation>
</file>
<file name="client/src/components/useToast.test.js">
<violation number="1" location="client/src/components/useToast.test.js:1">
P3: The test file only checks the export type of `useToast` without testing any actual hook behavior. The PR description states these tests "ensure the hook initializes and manages state correctly", but the current assertions don't verify state initialization, toast lifecycle (show → auto-dismiss), timer cleanup, or the memory-leak fix that this PR is meant to address. Adding at least one integration test using `renderHook` from `@testing-library/react` would validate that toasts render and auto-dismiss, making the test coverage match the PR's stated purpose.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| delete timers.current[id]; | ||
| setToasts((previous) => { | ||
| const nextToasts = previous.filter((toast) => toast.id !== id); | ||
| if (nextToasts.length === 0) { |
There was a problem hiding this comment.
P1: The timers.current = {} inside the setToasts functional updater creates a race condition that can lose newly-created timer references. If a new toast is shown (creating a new timer entry in timers.current) between when delete timers.current[id] runs synchronously and when React executes this functional updater, the nextToasts.length === 0 branch will reset timers.current = {} and silently drop the new timer — that toast will never auto-dismiss.
How it happens (example):
- Only toast A exists. Timer A fires →
delete timers.current[aId]runs →timers.currentis now{}. - Before React processes the setToasts updater, an event handler shows toast B →
timers.current[bId] = timerBis set. - The setToasts updater runs with
previous = [{id: aId}]→ filters out aId →nextToastsis[]→timers.current = {}wipes timerB. - Toast B is in state but its timer is gone — it persists until the page reloads.
Recommendation: Remove the timers.current = {} branch inside the updater. The delete timers.current[id] on line 22 already handles per-timer cleanup. If the goal is to fully reset timers.current when all toasts are dismissed, do it outside the updater (or skip it entirely — unused ref entries are negligible memory).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/useToast.jsx, line 25:
<comment>The `timers.current = {}` inside the `setToasts` functional updater creates a race condition that can lose newly-created timer references. If a new toast is shown (creating a new timer entry in `timers.current`) between when `delete timers.current[id]` runs synchronously and when React executes this functional updater, the `nextToasts.length === 0` branch will reset `timers.current = {}` and silently drop the new timer — that toast will never auto-dismiss.
**How it happens (example):**
1. Only toast A exists. Timer A fires → `delete timers.current[aId]` runs → `timers.current` is now `{}`.
2. Before React processes the setToasts updater, an event handler shows toast B → `timers.current[bId] = timerB` is set.
3. The setToasts updater runs with `previous = [{id: aId}]` → filters out aId → `nextToasts` is `[]` → `timers.current = {}` wipes timerB.
4. Toast B is in state but its timer is gone — it persists until the page reloads.
**Recommendation:** Remove the `timers.current = {}` branch inside the updater. The `delete timers.current[id]` on line 22 already handles per-timer cleanup. If the goal is to fully reset `timers.current` when all toasts are dismissed, do it outside the updater (or skip it entirely — unused ref entries are negligible memory).</comment>
<file context>
@@ -18,8 +19,14 @@ export function useToast(duration = 2200) {
delete timers.current[id];
+ setToasts((previous) => {
+ const nextToasts = previous.filter((toast) => toast.id !== id);
+ if (nextToasts.length === 0) {
+ timers.current = {};
+ }
</file context>
| @@ -0,0 +1,8 @@ | |||
| import { describe, it, expect } from "vitest"; | |||
There was a problem hiding this comment.
P3: The test file only checks the export type of useToast without testing any actual hook behavior. The PR description states these tests "ensure the hook initializes and manages state correctly", but the current assertions don't verify state initialization, toast lifecycle (show → auto-dismiss), timer cleanup, or the memory-leak fix that this PR is meant to address. Adding at least one integration test using renderHook from @testing-library/react would validate that toasts render and auto-dismiss, making the test coverage match the PR's stated purpose.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/useToast.test.js, line 1:
<comment>The test file only checks the export type of `useToast` without testing any actual hook behavior. The PR description states these tests "ensure the hook initializes and manages state correctly", but the current assertions don't verify state initialization, toast lifecycle (show → auto-dismiss), timer cleanup, or the memory-leak fix that this PR is meant to address. Adding at least one integration test using `renderHook` from `@testing-library/react` would validate that toasts render and auto-dismiss, making the test coverage match the PR's stated purpose.</comment>
<file context>
@@ -0,0 +1,8 @@
+import { describe, it, expect } from "vitest";
+import { useToast } from "./useToast";
+
</file context>
Itzzavdheshh
added
type:accessibility
🎊 PR Merged SuccessfullyHey @hrshjswniii! 👋 Congratulations and thank you for your contribution to VoiceForge! Note 🔗 Linked issue(s): #1140 · ✅ 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
type:feature
4 participants
🚀 Program
GSSoC
📝 Description
This PR resolves a memory leak in
useToast.jsxwhere dictionary keys and closure scopes intimers.currentpersisted indefinitely over long application sessions.Key fixes:
showToastto evaluatenextToasts.length. When all active toasts dismiss (nextToasts.length === 0),timers.currentis reset to an empty object{}.useEffectunmount cleanup handler to clear running timeouts and resettimers.current = {}.useToast.test.jsto ensure the hook initializes and manages state correctly.🔗 Related Issue
Closes #1140
🔄 Type of Change
🧪 How to Test
http://localhost:5173in a browser.timers.currentholds no orphaned references.npm run test --workspace clientand verify all tests pass.✅ Checklist
fix: prevent memory leak in useToast)Summary by cubic
Fixes a memory leak in
useToastby clearing timers on unmount and resetting the timers map when the last toast dismisses. Prevents retained references and memory growth during long sessions.timers.current = {}; remove per-toast timers on dismiss.timers.currentto drop stale closures. AddeduseToast.test.jsfor basic export coverage.Written for commit ff131f1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests