Fix: Clear draggingTimeout on unmount to prevent timer leak#323
Fix: Clear draggingTimeout on unmount to prevent timer leak#323upendra512 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
Add useEffect cleanup to clear draggingTimeout when component unmounts. Previously, the timeout could fire after unmount, causing timer memory leak and potential errors from accessing stale references. Fixes AOSSIE-Org#309 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
WalkthroughA memory leak fix in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip You can disable poems in the walkthrough.Disable the |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 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.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d45dd8c2-7110-4c67-8de8-b7aa78fb21ee
📒 Files selected for processing (1)
src/hooks/useTrackpadGesture.ts
| useEffect(() => { | ||
| return () => { | ||
| if (draggingTimeout.current) { | ||
| clearTimeout(draggingTimeout.current) | ||
| } | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
🧹 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.
| 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.
2 participants
Description
Fixes Issue #309 - draggingTimeout not cleared on unmount in useTrackpadGesture
Changes
useEffectcleanup function to cleardraggingTimeouton component unmountTesting
Technical Details
The
draggingTimeoutwas set inhandleTouchEnd(line 243) but never cleaned up when the component unmounts. This could cause:The fix adds a standard React cleanup pattern with
useEffectto ensure the timeout is cleared on unmount.Related Issue
Closes #309
Summary by CodeRabbit