Skip to content

Fix: Clear draggingTimeout on unmount to prevent timer leak #323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
upendra512 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: Clear draggingTimeout on unmount to prevent timer leak #323

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/hooks/useTrackpadGesture.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useRef, useState } from "react"
import { useEffect, useRef, useState } from "react"
import {
PINCH_THRESHOLD,
TOUCH_MOVE_THRESHOLD,
Expand Down Expand Up @@ -254,6 +254,14 @@ export const useTrackpadGesture = (
}
}

useEffect(() => {
return () => {
if (draggingTimeout.current) {
clearTimeout(draggingTimeout.current)
}
}
}, [])
Comment on lines +257 to +263
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Cleanup implementation is correct and addresses the memory leak.

The empty dependency array ensures the cleanup runs only on unmount, and the conditional check before clearTimeout is appropriate since draggingTimeout.current may be null.

One minor suggestion for consistency with lines 137-138: you could also null out the ref after clearing, though it's not strictly necessary since the component is unmounting.

,

♻️ Optional: Null out ref for consistency 
 useEffect(() => {
   return () => {
     if (draggingTimeout.current) {
       clearTimeout(draggingTimeout.current)
+      draggingTimeout.current = null
     }
   }
 }, [])
📝 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
useEffect(() => {
return () => {
if (draggingTimeout.current) {
clearTimeout(draggingTimeout.current)
}
}
}, [])
useEffect(() => {
return () => {
if (draggingTimeout.current) {
clearTimeout(draggingTimeout.current)
draggingTimeout.current = null
}
}
}, [])
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useTrackpadGesture.ts` around lines 257 - 263, The cleanup clears
draggingTimeout.current on unmount correctly; for consistency with the pattern
used around lines 137-138, after calling clearTimeout(draggingTimeout.current)
in the useEffect cleanup, also set draggingTimeout.current = null so the ref is
explicitly cleared before unmount; locate the useEffect cleanup where
draggingTimeout.current is conditionally cleared and add the null assignment
immediately after the clearTimeout call.


return {
isTracking,
handlers: {
Expand Down
Loading