Skip to content

Conversation

@shaharkazaz
Copy link

@shaharkazaz shaharkazaz commented Oct 26, 2025

User description

While researching different design system implementations, I stumbled upon the following bug:
There is a race condition in the Combobox/Search component where typing text and immediately clearing the input would cause the text to reappear with "No results found" message.

Screen.Recording.2025-10-26.at.6.45.30.mov

Reproduction steps:

  1. Open Combobox in Storybook (story/components-combobox--overview)
  2. Type text quickly (e.g., "test")
  3. Immediately click the clear button (before 400ms debounce completes)
  4. Bug: Text reappears with "No results found" instead of staying cleared

Root cause: The useDebounceEvent hook didn't cancel pending debounced callbacks when clearValue() was called, allowing stale values to fire after clearing.

Solution

  • Store debounced function reference in useDebounceEvent hook via useRef
  • Cancel pending callbacks in clearValue() before clearing the value
  • Add cleanup effect to cancel on component unmount
  • Follow TDD approach: Test first (failed), then fix (passed)

Changes

Modified Files

  • packages/core/src/hooks/useDebounceEvent/index.ts - Added cancel logic
  • packages/core/src/hooks/__tests__/useDebounceEvent.test.ts - Added race condition test
  • packages/core/src/components/Combobox/__tests__/Combobox.interactions.js - Added integration test

Tests

  • ✅ Unit test: should cancel pending debounced onChange when clearValue is called
  • ✅ Integration test: onImmediateClearBeforeDebounceCompletesTest
  • ✅ All existing tests pass (12/12 in useDebounceEvent)
  • ✅ Manual testing in Storybook

Future Improvement Suggestion

Currently, the test waits an arbitrary 500ms for the debounce to complete:

  await new Promise(resolve => setTimeout(resolve, 500));

Suggestion: Export SEARCH_DEFAULT_DEBOUNCE_RATE constant from a consumer component (e.g combobox) for use in tests:

export const COMBOBOX_SEARCH_DEBOUNCE_RATE = 400; (or depend on `SEARCH_DEBOUNCE_RATE`)

This makes the relationship explicit and prevents tests from breaking if the default changes.


PR Type

Bug fix, Tests


Description

  • Fix race condition in Search/Combobox where typing and immediately clearing input caused text to reappear

  • Store debounced function reference to enable cancellation of pending callbacks

  • Cancel pending debounced calls when clearValue() is invoked

  • Add cleanup effect to cancel debounced callbacks on component unmount

  • Add unit and integration tests to verify race condition is resolved


Diagram Walkthrough

flowchart LR
  A["User types text"] -->|schedules debounce| B["Pending callback"]
  C["User clears input"] -->|cancel pending| B
  B -->|stale value fires| D["Bug: text reappears"]
  C -->|fixed: cancel works| E["Text stays cleared"]
Loading

File Walkthrough

Relevant files
Bug fix
index.ts
Store debounced function reference and add cancellation logic

packages/core/src/hooks/useDebounceEvent/index.ts

  • Added debouncedFnRef to store reference to debounced function for
    cancellation
  • Store debounced function in ref when creating it in useMemo
  • Added cleanup effect to cancel pending debounced calls on unmount
  • Call debouncedFnRef.current?.cancel() in clearValue() before clearing
    state
+11/-1   
Tests
useDebounceEvent.test.ts
Add unit test for debounce cancellation on clear                 

packages/core/src/hooks/tests/useDebounceEvent.test.ts

  • Added new test case for race condition scenario
  • Test verifies that typing text and immediately clearing cancels
    pending debounced callback
  • Ensures onChange is only called once with empty string, not with stale
    value
  • Uses vi.runOnlyPendingTimers() to verify no additional callbacks fire
+22/-0   
Combobox.interactions.js
Add integration test for immediate clear before debounce 

packages/core/src/components/Combobox/tests/Combobox.interactions.js

  • Added new integration test onImmediateClearBeforeDebounceCompletesTest
  • Test simulates typing without waiting for debounce, then immediately
    clearing
  • Waits 500ms for debounce period to complete and verifies input remains
    empty
  • Verifies all options remain visible (no stale filter applied)
  • Added test to default test suite
+25/-1   

Fixes race condition where typing and immediately clearing the search input would cause the text to reappear due to a pending debounced callback firing with a stale value.

Changes:
 - Store debounced function reference in useDebounceEvent hook
 - Cancel pending debounced calls in clearValue()
 - Add cleanup to cancel on unmount
 - Add unit test for race condition scenario
 - Add integration test in Combobox interactions
@qodo-merge-for-open-source
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
 Summary findings: - ⚠️ Title violates Conventional Commits
⚡ Recommended focus areas for review

Possible Memory Leak

The debounced function reference is updated when dependencies change, but the previous debounced instance is not cancelled before being replaced. Consider cancelling the existing debounced function in the memo effect cleanup or before assigning a new one to avoid a pending callback firing after props change.

  const debouncedFn = debounce(onChange, delay);
  debouncedFnRef.current = debouncedFn;
  return debouncedFn;
}, [onChange, delay]);

useEffect(() => {
  return () => {
    debouncedFnRef.current?.cancel();
  };
}, []);
Stale Closure Risk

The debounced function is created from 'onChange' and stored in a ref; if 'onChange' or 'delay' change frequently, queued calls from the previous instance might fire with outdated closures. Validate whether you need to cancel when 'onChange'/'delay' change and recreate the debounced function with a cleanup to prevent stale calls.

  const debouncedFn = debounce(onChange, delay);
  debouncedFnRef.current = debouncedFn;
  return debouncedFn;
}, [onChange, delay]);

useEffect(() => {
  return () => {
    debouncedFnRef.current?.cancel();
  };
}, []);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant