fix: add debounced prop and ux to mui search input component - #209
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSearchInput gains an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SearchInput
participant DebounceWrapper
participant Parent_onSearch as Parent onSearch
User->>SearchInput: type chars / press Enter
Note right of SearchInput: updates local searchTerm
alt debounced = true
SearchInput->>DebounceWrapper: invoke debounced(searchTerm)
DebounceWrapper-->>SearchInput: schedule call after DEBOUNCE_WAIT
rect rgba(200,200,255,0.5)
DebounceWrapper->>Parent_onSearch: call with latest searchTerm (after wait)
end
else debounced = false
SearchInput->>Parent_onSearch: call immediately (onChange or Enter)
end
User->>SearchInput: clear input
SearchInput->>DebounceWrapper: cancel pending (if any)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/search-input.js (1)
58-75:⚠️ Potential issue | 🟡 MinorTwo issues with adornment logic.
Duplicate search icons: With the new
startAdornmentalways showing a SearchIcon (lines 58-62), the fallback SearchIcon inendAdornment(lines 72-74) creates duplicate icons when no term exists. The end SearchIcon should be removed.Clear button uses prop instead of state: The condition
term ? ...references the prop, not the localsearchTermstate. If the user types something but the parent hasn't updated thetermprop yet, the clear button won't appear. UsesearchTermfor immediate feedback.🐛 Proposed fix
startAdornment: ( <InputAdornment position="start"> <SearchIcon sx={{ color: "#0000008F" }} /> </InputAdornment> ), - endAdornment: term ? ( + endAdornment: searchTerm ? ( <IconButton size="small" onClick={handleClear} sx={{ position: "absolute", right: 0 }} > <ClearIcon sx={{ color: "#0000008F" }} /> </IconButton> - ) : ( - <SearchIcon - sx={{ mr: 1, color: "#0000008F", position: "absolute", right: 0 }} - /> - ) + ) : null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/search-input.js` around lines 58 - 75, The adornment logic currently renders a SearchIcon in both startAdornment and in the endAdornment fallback and uses the prop term to decide showing the clear button; to fix this remove the fallback SearchIcon from endAdornment so only the startAdornment SearchIcon is rendered, and change the clear-button condition to use the local searchTerm state (instead of the prop term) so the IconButton with onClick={handleClear} appears immediately when the user types; update the JSX that defines startAdornment and endAdornment (InputAdornment, SearchIcon, IconButton, ClearIcon, handleClear, term -> searchTerm) accordingly.
🧹 Nitpick comments (1)
src/components/mui/search-input.js (1)
45-49: Consider allowing Enter key to trigger immediate search even in debounced mode.Currently, pressing Enter does nothing when
debouncedis true. Users commonly expect Enter to immediately submit their search query. Consider canceling the pending debounce and triggering the search immediately on Enter.💡 Optional enhancement
const handleKeyDown = (ev) => { - if (!debounced && ev.key === "Enter") { + if (ev.key === "Enter") { + onSearchDebounced?.cancel(); onSearch(searchTerm); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/search-input.js` around lines 45 - 49, handle the Enter key in handleKeyDown so it triggers an immediate search even when debounced is true: detect ev.key === "Enter", clear/cancel any pending debounce timer or cancel the debounced call (e.g., clearTimeout on a timeoutId or call debounced.cancel if using lodash/debounce), then call onSearch(searchTerm) immediately; reference handleKeyDown, debounced, onSearch, and searchTerm so the fix cancels the pending debounce and runs the search right away.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/search-input.js`:
- Around line 33-36: Replace useCallback with useMemo when creating the
debounced function: the current onSearchDebounced is memoizing the result of
debounce((value) => onSearch(value), DEBOUNCE_WAIT) so wrap that expression in
useMemo and include onSearch and debounced in the dependency array (and
debounce/DEBOUNCE_WAIT if not stable) so a stable debounced function instance is
returned; ensure the import for useMemo is added and that when debounced is
false you return null (same behavior) to preserve existing logic.
---
Outside diff comments:
In `@src/components/mui/search-input.js`:
- Around line 58-75: The adornment logic currently renders a SearchIcon in both
startAdornment and in the endAdornment fallback and uses the prop term to decide
showing the clear button; to fix this remove the fallback SearchIcon from
endAdornment so only the startAdornment SearchIcon is rendered, and change the
clear-button condition to use the local searchTerm state (instead of the prop
term) so the IconButton with onClick={handleClear} appears immediately when the
user types; update the JSX that defines startAdornment and endAdornment
(InputAdornment, SearchIcon, IconButton, ClearIcon, handleClear, term ->
searchTerm) accordingly.
---
Nitpick comments:
In `@src/components/mui/search-input.js`:
- Around line 45-49: handle the Enter key in handleKeyDown so it triggers an
immediate search even when debounced is true: detect ev.key === "Enter",
clear/cancel any pending debounce timer or cancel the debounced call (e.g.,
clearTimeout on a timeoutId or call debounced.cancel if using lodash/debounce),
then call onSearch(searchTerm) immediately; reference handleKeyDown, debounced,
onSearch, and searchTerm so the fix cancels the pending debounce and runs the
search right away.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36d2f051-31fb-4dac-9a9d-5cab289c16a9
📒 Files selected for processing (1)
src/components/mui/search-input.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
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/components/mui/search-input.js (1)
58-75:⚠️ Potential issue | 🟠 MajorUse
searchTermfor end-adornment state and remove the duplicate search icon.Line 63 currently keys off
term, which can be stale in the new non-debounced flow (typing updates local state but not parent state yet). Also, with the new start adornment (Lines 58-62), the fallback right-side search icon (Lines 71-74) creates duplicate icons.♻️ Proposed fix
- endAdornment: term ? ( + endAdornment: searchTerm ? ( <IconButton size="small" onClick={handleClear} sx={{ position: "absolute", right: 0 }} > <ClearIcon sx={{ color: "#0000008F" }} /> </IconButton> - ) : ( - <SearchIcon - sx={{ mr: 1, color: "#0000008F", position: "absolute", right: 0 }} - /> - ) + ) : null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/search-input.js` around lines 58 - 75, The endAdornment currently checks the stale prop/local variable term and renders a duplicate SearchIcon on the right; change the condition to use the controlled value searchTerm (the up-to-date state/prop) when deciding to show the Clear button, and remove the right-side fallback SearchIcon since you already render a startAdornment SearchIcon; update the endAdornment logic around IconButton and handleClear to rely on searchTerm and render null (or nothing) when searchTerm is empty so no duplicate icon appears.
🧹 Nitpick comments (1)
src/components/mui/__tests__/search-input.test.js (1)
63-96: Add a regression test for clearing with a pending debounced call.Given the new debounced flow, we should explicitly assert that clear does not allow a previously typed value to fire later.
➕ Suggested test case
+ test("clearing input cancels pending debounced search", async () => { + const onSearch = jest.fn(); + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + render(<SearchInput term="initial" onSearch={onSearch} debounced />); + + const input = screen.getByPlaceholderText("Search..."); + await user.clear(input); + await user.type(input, "something"); + await user.click(screen.getByRole("button")); + + act(() => jest.advanceTimersByTime(DEBOUNCE_WAIT)); + expect(onSearch).toHaveBeenCalledWith(""); + expect(onSearch).not.toHaveBeenCalledWith("something"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/__tests__/search-input.test.js` around lines 63 - 96, Add a regression test in the "debounced prop" suite for SearchInput that ensures clearing cancels a pending debounced call: render SearchInput with debounced and a jest.fn() onSearch, use userEvent.setup({ advanceTimers: jest.advanceTimersByTime }) to type a value, then trigger the component's clear action (e.g., click the clear button or invoke the clear control exposed by SearchInput), advance timers with act(() => jest.advanceTimersByTime(DEBOUNCE_WAIT)) and assert onSearch was not called; keep the test alongside the existing tests and reuse DEBOUNCE_WAIT, SearchInput, and onSearch identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/search-input.js`:
- Around line 40-43: handleClear can allow a previously scheduled trailing call
from handleChange to fire after the input is cleared; fix by cancelling any
pending debounced work before clearing. In the component, call the debounced
function's cancel method (e.g., debounced.cancel()) if present at the start of
handleClear, then proceed to setSearchTerm('') and emit any immediate clear
behavior (e.g., call onSearch or onSearchDebounced with an empty string as
needed), guarding for the existence of debounced and onSearchDebounced.
---
Outside diff comments:
In `@src/components/mui/search-input.js`:
- Around line 58-75: The endAdornment currently checks the stale prop/local
variable term and renders a duplicate SearchIcon on the right; change the
condition to use the controlled value searchTerm (the up-to-date state/prop)
when deciding to show the Clear button, and remove the right-side fallback
SearchIcon since you already render a startAdornment SearchIcon; update the
endAdornment logic around IconButton and handleClear to rely on searchTerm and
render null (or nothing) when searchTerm is empty so no duplicate icon appears.
---
Nitpick comments:
In `@src/components/mui/__tests__/search-input.test.js`:
- Around line 63-96: Add a regression test in the "debounced prop" suite for
SearchInput that ensures clearing cancels a pending debounced call: render
SearchInput with debounced and a jest.fn() onSearch, use userEvent.setup({
advanceTimers: jest.advanceTimersByTime }) to type a value, then trigger the
component's clear action (e.g., click the clear button or invoke the clear
control exposed by SearchInput), advance timers with act(() =>
jest.advanceTimersByTime(DEBOUNCE_WAIT)) and assert onSearch was not called;
keep the test alongside the existing tests and reuse DEBOUNCE_WAIT, SearchInput,
and onSearch identifiers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0184f1f4-9eaf-4b46-b783-a52629fbeb79
📒 Files selected for processing (2)
src/components/mui/__tests__/search-input.test.jssrc/components/mui/search-input.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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)
src/components/mui/search-input.js (1)
59-76:⚠️ Potential issue | 🟠 MajorDuplicate search icons when
termis falsy.The
startAdornmentalways renders aSearchIcon(lines 59-63), and whentermis falsy, theendAdornmentalso renders aSearchIcon(lines 73-75). This results in two search icons displayed simultaneously when the input is empty.🐛 Proposed fix: Remove the redundant endAdornment SearchIcon
endAdornment: term ? ( <IconButton size="small" onClick={handleClear} sx={{ position: "absolute", right: 0 }} > <ClearIcon sx={{ color: "#0000008F" }} /> </IconButton> - ) : ( - <SearchIcon - sx={{ mr: 1, color: "#0000008F", position: "absolute", right: 0 }} - /> - ) + ) : null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/search-input.js` around lines 59 - 76, The UI renders duplicate SearchIcon components because startAdornment always renders a SearchIcon and endAdornment renders another SearchIcon when term is falsy; update the endAdornment logic in the component (the JSX that sets startAdornment and endAdornment) to remove the redundant SearchIcon branch so endAdornment only renders the IconButton with ClearIcon when term is truthy (using term and handleClear) and otherwise renders nothing; keep the startAdornment InputAdornment with SearchIcon intact and ensure IconButton/clear flow remains tied to handleClear and ClearIcon.
🧹 Nitpick comments (1)
src/components/mui/search-input.js (1)
28-37: Consider reordering declarations for clarity.
handleClearreferencesonSearchDebouncedbefore it's declared. This works at runtime (the closure is only evaluated when the click handler fires), but it harms readability and can confuse static analysis tools or future maintainers.♻️ Suggested reordering
+ const onSearchDebounced = useMemo( + () => debounced ? debounce((value) => onSearch(value), DEBOUNCE_WAIT) : null, + [onSearch, debounced] + ); + const handleClear = () => { onSearchDebounced?.cancel(); setSearchTerm(""); onSearch(""); }; - - const onSearchDebounced = useMemo( - () => debounced ? debounce((value) => onSearch(value), DEBOUNCE_WAIT) : null, - [onSearch, debounced] - );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/search-input.js` around lines 28 - 37, Move the onSearchDebounced declaration (the useMemo that returns debounce((value) => onSearch(value), DEBOUNCE_WAIT) or null) above the handleClear function so handleClear no longer references onSearchDebounced before it's declared; update handleClear to keep its current logic (onSearchDebounced?.cancel(), setSearchTerm(""), onSearch("")) but ensure onSearchDebounced is defined earlier and that the dependency array on the useMemo remains [onSearch, debounced].
🤖 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 `@src/components/mui/search-input.js`:
- Around line 59-76: The UI renders duplicate SearchIcon components because
startAdornment always renders a SearchIcon and endAdornment renders another
SearchIcon when term is falsy; update the endAdornment logic in the component
(the JSX that sets startAdornment and endAdornment) to remove the redundant
SearchIcon branch so endAdornment only renders the IconButton with ClearIcon
when term is truthy (using term and handleClear) and otherwise renders nothing;
keep the startAdornment InputAdornment with SearchIcon intact and ensure
IconButton/clear flow remains tied to handleClear and ClearIcon.
---
Nitpick comments:
In `@src/components/mui/search-input.js`:
- Around line 28-37: Move the onSearchDebounced declaration (the useMemo that
returns debounce((value) => onSearch(value), DEBOUNCE_WAIT) or null) above the
handleClear function so handleClear no longer references onSearchDebounced
before it's declared; update handleClear to keep its current logic
(onSearchDebounced?.cancel(), setSearchTerm(""), onSearch("")) but ensure
onSearchDebounced is defined earlier and that the dependency array on the
useMemo remains [onSearch, debounced].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7d95058-bab2-4ca5-a9a3-453a7069873f
📒 Files selected for processing (1)
src/components/mui/search-input.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
| }; | ||
|
|
||
| const handleClear = () => { | ||
| onSearchDebounced?.cancel(); |
There was a problem hiding this comment.
@tomrndom handleClear references onSearchDebounced before it is declared
| <SearchIcon sx={{ color: "#0000008F" }} /> | ||
| </InputAdornment> | ||
| ), | ||
| endAdornment: term && ( |
There was a problem hiding this comment.
@tomrndom
Clear button hidden during debounced typing
in debounced mode the parent doesn't learn about the new value for 500ms. The user types, there is text in the box, but no clear affordance until the debounce flushes , the affordance the icon refactor was supposed to improve. Pre-existing pattern, but debouncing amplifies it materiall.
Suggested fix
Use the local searchTerm (or searchTerm || term) to drive the clear-button visibility.
| <SearchIcon | ||
| sx={{ mr: 1, color: "#0000008F", position: "absolute", right: 0 }} | ||
| /> | ||
| startAdornment: ( |
There was a problem hiding this comment.
@tomrndom
Visual/contract change for ALL existing consumers (regardless of debounced)
keep the legacy single-icon layout when debounced is not set
| onSearch(""); | ||
| }; | ||
|
|
||
| const onSearchDebounced = useMemo( |
There was a problem hiding this comment.
@tomrndom
debounce silently broken when parent passes inline onSearch.
useMemo recomputes whenever onSearch changes identity. The cleanup effect then cancel()s the previous debounced instance. A debounced function cancelled before its timer fires loses its pending invocation forever. That is the entire bug.
Why the existing tests miss it
render(<SearchInput term="" onSearch={onSearch} debounced />);In a test, onSearch is a stable jest.fn() reference and there are no parent re-renders. So useMemo never recomputes, the debounced function is never cancelled, and the timer fires normally. The test passes. Production breaks.
Proposed fix
Replace the current memoization with a ref-stable wrapper so the debounced function is created once per debounced value and is not destroyed by parent re-renders, while still calling the latest
onSearch.
…arent re render Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
| }, [onSearch]); | ||
|
|
||
| const onSearchDebouncedRef = useRef( | ||
| debounced |
There was a problem hiding this comment.
we don't need this condition here, onSearchDebouncedRef is only called when debounced is true
| <ClearIcon fontSize="small" sx={{ color: "#0000008F" }} /> | ||
| </IconButton> | ||
| ) : ( | ||
| <SearchIcon |
There was a problem hiding this comment.
so if no searchTerm and not debounced we have two searchIcons at start AND end adornments ? Doesn't look right
…ebouncedRef Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
3 participants
ref: https://app.clickup.com/t/86b84rhkb
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests