fix: add debounced prop and ux to mui search input component#209
fix: add debounced prop and ux to mui search input component#209
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSearchInput adds an optional Changes
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>
1 participant
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