Skip to content

⚡ Bolt: Correct talks search debounce logic to prevent excess URL updates #133

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

Closed
anyulled wants to merge 1 commit into from
Closed

⚡ Bolt: Correct talks search debounce logic to prevent excess URL updates #133

Show file tree
Hide file tree
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
5 changes: 5 additions & 0 deletions .jules/bolt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,8 @@

**Learning:** When attempting to optimize an O(N^2) array spread operation (`[...existing, talk]`) inside a grouping loop in `groupTalksByTrack`, the purely functional/immutable constraint specified by the team (and the lack of `Map.groupBy` support in Node 20.x Jest environments) means that we must fall back to immutable reductions.
**Action:** When constraints require strict immutability without mutation of objects, use `reduce` with object and array spreads (e.g., `{ ...acc, [key]: [...(acc[key] || []), item] }`) even if it introduces O(N^2) overhead for large arrays. Avoid using `push()` or modifying accumulators directly. Always run Prettier/formatting checks before merge to resolve CI failures.

## 2026-03-29 - React Event Handler Cleanup Illusion

**Learning:** Returning a cleanup function (like `() => clearTimeout(id)`) from a React event handler (e.g., `onChange`) does absolutely nothing. React ignores the return value of event handlers. In `TalksFilterBar`, this caused debouncing to fail completely, creating N timeouts for N keystrokes and leading to excessive state updates.
**Action:** When debouncing or throttling inside event handlers without a custom hook, always store the timeout ID in a `useRef` and explicitly call `clearTimeout(ref.current)` before setting the new timeout.
2 changes: 1 addition & 1 deletion __tests__/snapshots/sections/home10/__snapshots__/Section1.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ exports[`Section1 Component matches snapshot 1`] = `
alt=""
src="/assets/img/icons/calender1.svg"
/>
15th, 16th, & 17th January “2025”
15th, 16th, & 17th January “2025”
</a>
</li>
</ul>
Expand Down
12 changes: 9 additions & 3 deletions components/layout/TalksFilterBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"use client";

import { usePathname, useRouter, useSearchParams } from "next/navigation";
import { useCallback, useEffect, useState, useTransition } from "react";
import { useCallback, useEffect, useRef, useState, useTransition } from "react";
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 29, 2026

Choose a reason for hiding this comment

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

high

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.

import { motion } from "framer-motion";

interface TalksFilterBarProps {
Expand All @@ -11,6 +11,7 @@ interface TalksFilterBarProps {

export default function TalksFilterBar({ tracks, year: _year }: TalksFilterBarProps) {
const router = useRouter();
const debounceRef = useRef<ReturnType<typeof setTimeout> | null>(null);

Comment on lines +14 to 15
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 29, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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 -C3

Repository: anyulled/devbcn-nextjs

Length of output: 2332

🏁 Script executed:

cat -n components/layout/TalksFilterBar.tsx | head -100

Repository: anyulled/devbcn-nextjs

Length of output: 3907

🏁 Script executed:

cat -n components/layout/TalksFilterBar.tsx | tail -80

Repository: anyulled/devbcn-nextjs

Length of output: 3394

🏁 Script executed:

cat -n __tests__/components/layout/TalksFilterBar.test.tsx

Repository: 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 debounceRef in handleTrackClick before calling updateFilters
  • Null debounceRef.current after the setTimeout callback executes
  • Add a useEffect cleanup 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.

const formatTrackName = (track: string) => {
return track.split("(")[0].trim();
Expand Down Expand Up @@ -60,11 +61,16 @@ export default function TalksFilterBar({ tracks, year: _year }: TalksFilterBarPr
const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newQuery = e.target.value;
setSearchQuery(newQuery);

// Clear the previous timeout if it exists
if (debounceRef.current) {
clearTimeout(debounceRef.current);
}

// Debounce the URL update for search
const timeoutId = setTimeout(() => {
debounceRef.current = setTimeout(() => {
updateFilters(selectedTrack, newQuery);
}, 300);
Comment on lines +71 to 73
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 29, 2026

Choose a reason for hiding this comment

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

high

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:

  1. User is on the "Java" track.
  2. User types in the search box. A timeout is scheduled to update the URL with track=Java.
  3. Before the timeout fires, the user clicks the "Cloud" track. The URL is immediately updated to track=Cloud.
  4. 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.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 29, 2026

Choose a reason for hiding this comment

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

medium

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);

return () => clearTimeout(timeoutId);
};

return (
Expand Down
Loading