⚡ Bolt: Correct talks search debounce logic to prevent excess URL updates#133
⚡ Bolt: Correct talks search debounce logic to prevent excess URL updates#133
Conversation
…updates Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA React misconception regarding cleanup functions in event handlers is documented and corrected. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a debouncing bug in the TalksFilterBar component by utilizing useRef for timeout management and updates the project's learning documentation in .jules/bolt.md. Feedback suggests further improvements to make the component more robust, including adding a useEffect cleanup to prevent memory leaks on unmount, resolving a race condition when track filters are changed during a pending search, and extracting the debounce delay into a named constant.
|
|
||
| import { usePathname, useRouter, useSearchParams } from "next/navigation"; | ||
| import { useCallback, useEffect, useState, useTransition } from "react"; | ||
| import { useCallback, useEffect, useRef, useState, useTransition } from "react"; |
There was a problem hiding this comment.
The introduction of useRef for manual timeout management is a good fix for the debouncing logic. However, this manual approach requires careful handling of the component lifecycle. A timeout scheduled with setTimeout will execute even if the component unmounts, which can lead to React warnings about state updates on unmounted components or memory leaks.
To prevent this, you should clean up the pending timeout in a useEffect hook. This ensures that if the component is unmounted, the timeout is cleared.
useEffect(() => {
// Return a cleanup function from a useEffect with an empty dependency array
return () => {
if (debounceRef.current) {
clearTimeout(debounceRef.current);
}
};
}, []); // This effect runs only once on mount and its cleanup on unmount.Please add this effect to the component to make it more robust.
| debounceRef.current = setTimeout(() => { | ||
| updateFilters(selectedTrack, newQuery); | ||
| }, 300); |
There was a problem hiding this comment.
This implementation correctly debounces the search input. However, it introduces a potential race condition. The setTimeout callback captures the selectedTrack value at the time of the last keystroke. If the user changes the track filter after typing but before the 300ms debounce timeout fires, this callback will execute with the old, stale selectedTrack value, incorrectly reverting the track filter in the URL.
For example:
- User is on the "Java" track.
- User types in the search box. A timeout is scheduled to update the URL with
track=Java. - Before the timeout fires, the user clicks the "Cloud" track. The URL is immediately updated to
track=Cloud. - The original timeout fires and updates the URL back to
track=Java, which is not what the user expects.
A simple way to fix this is to cancel the pending search timeout whenever a track is changed. This would require a small modification to the handleTrackClick function:
const handleTrackClick = (track: string) => {
if (debounceRef.current) {
clearTimeout(debounceRef.current);
}
// ... rest of the function
};Since handleTrackClick is outside the changed lines in this PR, I'm pointing this out for your consideration to make the component more robust. You could also add a test case to cover this scenario.
| const timeoutId = setTimeout(() => { | ||
| debounceRef.current = setTimeout(() => { | ||
| updateFilters(selectedTrack, newQuery); | ||
| }, 300); |
There was a problem hiding this comment.
The debounce delay 300 is a magic number. Consider extracting it into a named constant to improve readability and maintainability. This makes it easier to understand the purpose of the value and to change it consistently if needed.
For example:
const DEBOUNCE_DELAY_MS = 300;
// ...
debounceRef.current = setTimeout(() => {
updateFilters(selectedTrack, newQuery);
}, DEBOUNCE_DELAY_MS);
![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)
components/layout/TalksFilterBar.tsx (1)
65-71: Drop “what” comments and keep only rationale comments.Line 65 and Line 70 comments restate obvious behavior; please remove or rewrite them to capture only non-obvious intent.
As per coding guidelines, "Code must be self-documenting. Only explain why non-obvious decisions were made in comments. DO NOT add inline comments explaining what code does."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/layout/TalksFilterBar.tsx` around lines 65 - 71, In TalksFilterBar, remove the inline "what" comments that restate obvious actions around debounceRef.current and setTimeout (the comments on/around the clearTimeout and debounce assignment) and replace them only with a short rationale if needed (e.g., explain why we debounce URL updates for search or why we clear previous timeout to avoid races), so keep comments that describe non-obvious intent or trade-offs but delete comments that merely describe what clearTimeout or setTimeout do.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/layout/TalksFilterBar.tsx`:
- Around line 14-15: A pending debounce timer in handleSearchChange can fire
after a user clicks a track and overwrite the newly-selected track; update
handleTrackClick to clear any pending timeout by checking and calling
clearTimeout(debounceRef.current) and setting debounceRef.current = null before
calling updateFilters, modify the setTimeout callback in handleSearchChange to
set debounceRef.current = null after it runs, and add a useEffect cleanup that
clears debounceRef.current (if present) on unmount to prevent timers firing
after the component is destroyed; refer to debounceRef, handleSearchChange,
handleTrackClick, updateFilters and the new useEffect cleanup for where to apply
these changes.
---
Nitpick comments:
In `@components/layout/TalksFilterBar.tsx`:
- Around line 65-71: In TalksFilterBar, remove the inline "what" comments that
restate obvious actions around debounceRef.current and setTimeout (the comments
on/around the clearTimeout and debounce assignment) and replace them only with a
short rationale if needed (e.g., explain why we debounce URL updates for search
or why we clear previous timeout to avoid races), so keep comments that describe
non-obvious intent or trade-offs but delete comments that merely describe what
clearTimeout or setTimeout do.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff1b1978-33d4-4404-aac0-edfbe4c34d3b
⛔ Files ignored due to path filters (1)
__tests__/snapshots/sections/home10/__snapshots__/Section1.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
.jules/bolt.mdcomponents/layout/TalksFilterBar.tsx
| const debounceRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect debounce lifecycle in TalksFilterBar:"
rg -n "handleTrackClick|handleSearchChange|clearTimeout\\(debounceRef.current\\)|setTimeout\\(" components/layout/TalksFilterBar.tsx -C2
echo
echo "Inspect tests for 'type then click track before debounce expires':"
rg -n "fireEvent\\.change|fireEvent\\.click|advanceTimersByTime" __tests__/components/layout/TalksFilterBar.test.tsx -C3Repository: anyulled/devbcn-nextjs
Length of output: 2332
🏁 Script executed:
cat -n components/layout/TalksFilterBar.tsx | head -100Repository: anyulled/devbcn-nextjs
Length of output: 3907
🏁 Script executed:
cat -n components/layout/TalksFilterBar.tsx | tail -80Repository: anyulled/devbcn-nextjs
Length of output: 3394
🏁 Script executed:
cat -n __tests__/components/layout/TalksFilterBar.test.tsxRepository: anyulled/devbcn-nextjs
Length of output: 2468
Clear pending debounce before non-debounced track clicks to prevent stale URL rewrites.
A pending timeout from handleSearchChange can fire after a track click and push stale filters using the captured selectedTrack from the timeout's closure. If a user types a search query and then clicks a track before the 300ms debounce expires, the pending timeout will fire with the original track value, causing two consecutive router updates and reverting the just-selected track.
Additionally, the component lacks cleanup for the debounce timeout on unmount, which allows the timer to fire after the component is destroyed.
The following fixes are required:
- Clear
debounceRefinhandleTrackClickbefore callingupdateFilters - Null
debounceRef.currentafter the setTimeout callback executes - Add a
useEffectcleanup function to clear any pending timeout on unmount
Proposed fix
@@
const handleTrackClick = (track: string) => {
+ if (debounceRef.current) {
+ clearTimeout(debounceRef.current);
+ debounceRef.current = null;
+ }
const newTrack = track === selectedTrack ? "" : track;
setSelectedTrack(newTrack);
updateFilters(newTrack, searchQuery);
};
@@
const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => {
@@
debounceRef.current = setTimeout(() => {
updateFilters(selectedTrack, newQuery);
+ debounceRef.current = null;
}, 300);
};
+
+ useEffect(() => {
+ return () => {
+ if (debounceRef.current) {
+ clearTimeout(debounceRef.current);
+ }
+ };
+ }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/layout/TalksFilterBar.tsx` around lines 14 - 15, A pending
debounce timer in handleSearchChange can fire after a user clicks a track and
overwrite the newly-selected track; update handleTrackClick to clear any pending
timeout by checking and calling clearTimeout(debounceRef.current) and setting
debounceRef.current = null before calling updateFilters, modify the setTimeout
callback in handleSearchChange to set debounceRef.current = null after it runs,
and add a useEffect cleanup that clears debounceRef.current (if present) on
unmount to prevent timers firing after the component is destroyed; refer to
debounceRef, handleSearchChange, handleTrackClick, updateFilters and the new
useEffect cleanup for where to apply these changes.
1 participant
💡 What: Corrected debounce logic for search input by using
useRefto store and clear previous timeouts.🎯 Why: The previous implementation returned the cleanup function from an event handler (which React ignores) and did not track the timeout ID across renders. This resulted in the router updating state for every single key press (N times) rather than just once after typing stopped, causing unnecessary re-renders and potential performance issues with React transitions.
📊 Impact: Prevents N router pushes when typing N characters quickly. Significantly reduces layout thrashing during searches.
🔬 Measurement: Typing fast into the talks search bar no longer creates multiple staggered UI updates or URL changes.
PR created automatically by Jules for task 15001196132622939092 started by @anyulled
Summary by CodeRabbit
Bug Fixes
Documentation