-
Notifications
You must be signed in to change notification settings - Fork 0
Fix: Improve useDebounce with advanced call control and unmount cleanup #124
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.
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
Conversation
WalkthroughThe PR renames the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/use-debounce.ts (1)
32-32: Critical:immediateis undefined and will cause a ReferenceError.The dependency array references
immediate, but this parameter was removed from the function signature. This will throw aReferenceErrorat runtime when the component mounts.Apply this diff to fix the dependency array:
- }, [value, delay, immediate]); // Re-run effect if value, delay, or immediate changes + }, [value, delay]); // Re-run effect if value or delay changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/__tests__/useDebounce.test.tsx(7 hunks)src/hooks/use-debounce.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/useDebounce.test.tsx (1)
src/hooks/use-debounce.ts (1)
useDebouncedValue(15-35)
🔇 Additional comments (2)
src/hooks/use-debounce.ts (1)
3-35: Inconsistency between PR objectives and implementation.The PR description states: "adds configurable call behavior" with
leadingandtrailingoptions for debounce control. However, the implementation only debounces a value with a simple delay—there are noleading/trailingoptions implemented.Either update the PR description to accurately reflect the current implementation, or implement the advertised features.
src/__tests__/useDebounce.test.tsx (1)
7-143: LGTM! Test coverage is comprehensive.The tests properly cover:
- Initial value return
- Debouncing behavior with timer assertions
- Multiple rapid value changes (only last value persists)
- Default delay when undefined
- Null and undefined value handling
- Timer cleanup on unmount
- Zero and negative delay edge cases
Note: These tests will fail until the critical bug (
immediateundefined reference) in the hook implementation is fixed.
| @@ -1,36 +1,22 @@ | |||
| import { useEffect, useState, useRef } from 'react'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import: useRef.
useRef is imported but not used anywhere in the hook.
-import { useEffect, useState, useRef } from 'react';
+import { useEffect, useState } from 'react';📝 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.
| import { useEffect, useState, useRef } from 'react'; | |
| import { useEffect, useState } from 'react'; |
🤖 Prompt for AI Agents
In src/hooks/use-debounce.ts around line 1, the React import includes useRef
which is not used; remove useRef from the import list (i.e. import only the
hooks actually used such as useEffect and useState) to eliminate the unused
import warning and keep imports minimal.
|
that cleanup on unmount is key—nice touch! 🎉 merging this now. |
Overview
This PR refactors the
useDebouncehook to provide more flexible control over function execution, including support for leading and trailing call behaviors, and ensures proper cleanup on component unmount.Changes
useDebounceinsrc/hooks/use-debounce.tsto debounce a function rather than a value.leadingandtrailingoptions to control when the debounced function is invoked (immediately or after the delay).Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.