⚡ Bolt: Fix debouncing in TalksFilterBar to prevent redundant route transitions#139
⚡ Bolt: Fix debouncing in TalksFilterBar to prevent redundant route transitions#139
Conversation
The previous implementation of `handleSearchChange` returned a cleanup function (`return () => clearTimeout(timeoutId)`), which React ignores for standard event handlers. This meant the timeout was never cleared, and every keystroke queued up a separate, redundant route transition after 300ms. This commit fixes the issue by storing the `timeoutId` in a `useRef`, explicitly clearing the previous timeout on each keystroke, and adding a proper cleanup effect on component unmount to prevent memory leaks. 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.
|
📝 WalkthroughWalkthroughThe TalksFilterBar component's debounce timeout management was refactored to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
Review Summary by QodoFix debouncing in TalksFilterBar to prevent redundant transitions
WalkthroughsDescription• Fixed broken debounce in TalksFilterBar search handler • Replaced incorrect cleanup function pattern with useRef • Added proper cleanup effect to prevent memory leaks • Eliminates redundant route transitions on rapid typing Diagramflowchart LR
A["Event Handler<br/>handleSearchChange"] -->|"stores timeout in"| B["timeoutRef<br/>useRef"]
B -->|"clears on each<br/>keystroke"| C["Previous Timeout<br/>Cleared"]
C -->|"prevents"| D["Redundant Route<br/>Transitions"]
B -->|"cleanup on<br/>unmount"| E["Memory Leak<br/>Prevention"]
File Changes1. components/layout/TalksFilterBar.tsx
|
Code Review by Qodo
1. Debounce not canceled on track
|
![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 adds debouncing to the search input in TalksFilterBar using useRef. Feedback suggests refactoring the implementation to use a useEffect hook instead to prevent potential stale closure issues with selectedTrack and updateFilters, which would also allow for the removal of the useRef import and the timeoutRef variable.
| // ⚡ Bolt: Clear timeout on unmount to prevent memory leaks | ||
| useEffect(() => { | ||
| return () => { | ||
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | ||
| }; | ||
| }, []); | ||
|
|
||
| const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newQuery = e.target.value; | ||
| setSearchQuery(newQuery); | ||
|
|
||
| // ⚡ Bolt: Clear previous timeout before setting a new one | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } | ||
|
|
||
| // Debounce the URL update for search | ||
| const timeoutId = setTimeout(() => { | ||
| timeoutRef.current = setTimeout(() => { | ||
| updateFilters(selectedTrack, newQuery); | ||
| }, 300); | ||
| return () => clearTimeout(timeoutId); | ||
| }; |
There was a problem hiding this comment.
The current debouncing implementation using useRef and manual clearTimeout in the event handler is prone to stale closure issues. The setTimeout callback captures the selectedTrack and updateFilters function from the render cycle when the user started typing. If the user changes the track selection while a search is debouncing, the eventual call to updateFilters will use the old track value and old searchParams, effectively reverting the track selection.
A more robust approach is to use a useEffect to handle the debounced URL update. This ensures that the latest state is always used and that pending updates are correctly cancelled if the state or URL changes in the meantime.
// ⚡ Bolt: Debounce URL updates when searchQuery changes
useEffect(() => {
const currentQuery = searchParams.get("q") || "";
if (searchQuery === currentQuery) return;
const timer = setTimeout(() => {
updateFilters(selectedTrack, searchQuery);
}, 300);
return () => clearTimeout(timer);
}, [searchQuery, selectedTrack, updateFilters, searchParams]);
const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => {
setSearchQuery(e.target.value);
};
|
|
||
| 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.
Since useRef is no longer needed for the debouncing logic, it can be removed from the React imports.
| import { useCallback, useEffect, useRef, useState, useTransition } from "react"; | |
| import { useCallback, useEffect, useState, useTransition } from "react"; |
| // ⚡ Bolt: Store timeout ID to properly debounce and prevent redundant API/route calls | ||
| const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); |
There was a problem hiding this comment.
The timeoutRef and its associated comment are no longer necessary if the debouncing logic is moved to a useEffect hook, which is a more robust way to handle side effects in React.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| updateFilters(newTrack, searchQuery); | ||
| }; | ||
|
|
||
| // ⚡ Bolt: Clear timeout on unmount to prevent memory leaks | ||
| useEffect(() => { | ||
| return () => { | ||
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | ||
| }; | ||
| }, []); | ||
|
|
||
| const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newQuery = e.target.value; | ||
| setSearchQuery(newQuery); | ||
|
|
||
| // ⚡ Bolt: Clear previous timeout before setting a new one | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } | ||
|
|
||
| // Debounce the URL update for search | ||
| const timeoutId = setTimeout(() => { | ||
| timeoutRef.current = setTimeout(() => { | ||
| updateFilters(selectedTrack, newQuery); | ||
| }, 300); |
There was a problem hiding this comment.
1. Debounce not canceled on track 🐞 Bug ≡ Correctness
A pending debounced search timeout is not cleared when a track is clicked, so the timer can fire later and push a URL built with stale selectedTrack/searchParams, potentially undoing the user's track selection. This can cause incorrect filters and unexpected route transitions when a user types then changes track within the debounce window.
Agent Prompt
### Issue description
`TalksFilterBar` debounces search updates via `timeoutRef`, but `handleTrackClick` doesn’t cancel a pending search timeout. If the user types and then changes track within 300ms, the old timeout can fire and push a URL built with stale `selectedTrack`/`searchParams`, potentially undoing the track selection.
### Issue Context
- `handleSearchChange` schedules `setTimeout(() => updateFilters(selectedTrack, newQuery), 300)`.
- Only search changes clear the timeout; track clicks and URL param changes do not.
- Querystring updates typically re-render without unmounting, so unmount cleanup won’t help.
### Fix Focus Areas
- components/layout/TalksFilterBar.tsx[57-83]
### Recommended fix
- In `handleTrackClick`, clear any pending `timeoutRef.current` (and set it to `null`) before calling `updateFilters`.
- Optionally also clear any pending timeout when `searchParams` changes (inside the effect that syncs state from the URL) to avoid stale timers firing after back/forward or other URL-driven updates.
- Consider adding a unit test: type into search (don’t advance timers), click a track, then advance timers and assert the track isn’t reverted by a later push.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/layout/TalksFilterBar.tsx (1)
57-61:⚠️ Potential issue | 🟠 MajorClear pending search debounce when track changes to avoid stale URL overwrite.
At Line 60 you push filters immediately on track click, but a previously scheduled timeout from Lines 80-82 can still fire and call
updateFilterswith staleselectedTrackcaptured earlier (Line 81). This can revert the selected track after the user clicks a pill.💡 Proposed fix
const handleTrackClick = (track: string) => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + timeoutRef.current = null; + } const newTrack = track === selectedTrack ? "" : track; setSelectedTrack(newTrack); updateFilters(newTrack, searchQuery); }; @@ - timeoutRef.current = setTimeout(() => { + timeoutRef.current = setTimeout(() => { updateFilters(selectedTrack, newQuery); + timeoutRef.current = null; }, 300);Also applies to: 74-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/layout/TalksFilterBar.tsx` around lines 57 - 61, The click handler (handleTrackClick) immediately calls updateFilters but doesn't cancel a pending search debounce, so a previously scheduled timeout can later call updateFilters with stale selectedTrack; fix by storing the search debounce ID in a ref/variable (e.g., searchDebounceTimeout or searchDebounceRef) and call clearTimeout(...) at the start of handleTrackClick before setSelectedTrack/updateFilters, and do the same in the search-change debounce logic (the handler that schedules the timeout) so any pending timeout is cleared whenever the track or search is changed.
🧹 Nitpick comments (1)
components/layout/TalksFilterBar.tsx (1)
25-25: Prefer concise “why-only” comments and drop inline branding markers.The added comments are mostly describing what the code does; the code is already clear enough without these lines.
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."
Also applies to: 63-64, 74-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/layout/TalksFilterBar.tsx` at line 25, Remove the inline explanatory comments (e.g., "// ⚡ Bolt: Store timeout ID to properly debounce and prevent redundant API/route calls" and the similar comments at the other noted locations) and either delete them or replace them with concise "why-only" comments that explain non-obvious rationale (no implementation-level descriptions) and strip any branding markers like "⚡ Bolt". Target the comments around the TalksFilterBar component where timeout/debounce behavior is implemented so the code remains self-documenting while retaining any brief rationale for unusual decisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/layout/TalksFilterBar.tsx`:
- Around line 57-61: The click handler (handleTrackClick) immediately calls
updateFilters but doesn't cancel a pending search debounce, so a previously
scheduled timeout can later call updateFilters with stale selectedTrack; fix by
storing the search debounce ID in a ref/variable (e.g., searchDebounceTimeout or
searchDebounceRef) and call clearTimeout(...) at the start of handleTrackClick
before setSelectedTrack/updateFilters, and do the same in the search-change
debounce logic (the handler that schedules the timeout) so any pending timeout
is cleared whenever the track or search is changed.
---
Nitpick comments:
In `@components/layout/TalksFilterBar.tsx`:
- Line 25: Remove the inline explanatory comments (e.g., "// ⚡ Bolt: Store
timeout ID to properly debounce and prevent redundant API/route calls" and the
similar comments at the other noted locations) and either delete them or replace
them with concise "why-only" comments that explain non-obvious rationale (no
implementation-level descriptions) and strip any branding markers like "⚡ Bolt".
Target the comments around the TalksFilterBar component where timeout/debounce
behavior is implemented so the code remains self-documenting while retaining any
brief rationale for unusual decisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ca3bebf-187a-4e6e-bb7f-70383a02e6e8
📒 Files selected for processing (1)
components/layout/TalksFilterBar.tsx
1 participant
💡 What: Fixed a broken debounce implementation in
TalksFilterBar.tsxby using auseRefto store and correctly clear the timeout ID. Added a cleanupuseEffectto clear timeouts on unmount.🎯 Why: The previous code attempted to return a cleanup function from the
onChangeevent handler, which React ignores. This resulted in the timeout never being cleared, meaning every single keystroke fired off a redundant route transition and URL update after 300ms.📊 Impact: Prevents redundant route transitions and URL state updates when users type quickly in the "Search talks..." input. Reduces main thread blocking and unnecessary React renders, leading to a smoother typing experience.
🔬 Measurement: Type rapidly in the talks search input. Previously, this would queue up N route changes (one for every keystroke). Now, only one route change will occur 300ms after you stop typing. Verified via Playwright script
verify_cuj.py.PR created automatically by Jules for task 14204785367317422303 started by @anyulled
Summary by CodeRabbit