fix(discover): preserve keyword filter input focus after selection#2962
Conversation
📝 WalkthroughWalkthroughKeywordSelector converted from uncontrolled to controlled: internal state now stores react-select ChangesSelector / KeywordSelector
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Selector/index.tsx (1)
301-330:⚠️ Potential issue | 🟡 MinorAsync effect can race and clobber the user's latest selection.
Now that
valueis controlled, every user pick propagates to the parent (URL/filter state) and comes back as a newdefaultValue, which re-fires this effect and re-fetches each keyword by id. There are two practical concerns:
- Stale-response race:
loadDefaultKeywordshas no cancellation. IfdefaultValuechanges faster than the network (e.g., user picks a second keyword before the previous fetch resolves), an older response can resolve last and overwriteselectedValuewith stale data.- Redundant work: every selection triggers
setSelectedValue(value)(immediate) followed shortly bysetSelectedValue(fetched)from the effect — an N+1 round-trip per user pick, which can also cause a transient flicker of the chip(s).Consider guarding the effect with a cancellation flag and short-circuiting when
defaultValuealready matches the currentselectedValue(so user-driven updates don't re-fetch themselves):♻️ Suggested refactor
useEffect(() => { + let cancelled = false; const loadDefaultKeywords = async (): Promise<void> => { if (!defaultValue) { - setSelectedValue(null); + if (!cancelled) setSelectedValue(null); return; } + // Skip refetch when defaultValue already reflects the current selection + const currentIds = (Array.isArray(selectedValue) + ? selectedValue + : selectedValue + ? [selectedValue] + : [] + ) + .map((v) => String(v.value)) + .join(','); + if (currentIds === defaultValue) return; + const keywords = await Promise.all( defaultValue.split(',').map(async (keywordId) => { const keyword = await axios.get<Keyword | null>( `/api/v1/keyword/${keywordId}` ); return keyword.data; }) ); const validKeywords: Keyword[] = keywords.filter( (keyword): keyword is Keyword => keyword !== null ); const nextValue = validKeywords.map((keyword) => ({ label: keyword.name, value: keyword.id, })); - setSelectedValue(isMulti ? nextValue : (nextValue[0] ?? null)); + if (!cancelled) { + setSelectedValue(isMulti ? nextValue : (nextValue[0] ?? null)); + } }; loadDefaultKeywords(); + return () => { + cancelled = true; + }; }, [defaultValue, isMulti]);If you adopt the early-return guard, add
selectedValueto the dep array (or read it via a ref to avoid re-running the effect).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Selector/index.tsx` around lines 301 - 330, The effect's async loader (loadDefaultKeywords) can race and clobber user selection; modify useEffect to (1) bail out early when the incoming defaultValue already matches the current selectedValue (compare string form of selectedValue vs defaultValue) to avoid redundant refetches, (2) add a local cancelled flag or AbortController inside loadDefaultKeywords and check it before calling setSelectedValue so stale responses never overwrite state, and (3) either include selectedValue in the effect deps or read selectedValue from a ref inside the effect to avoid unnecessary re-runs; target the loadDefaultKeywords, setSelectedValue, defaultValue, selectedValue and isMulti symbols when making these changes.
🧹 Nitpick comments (2)
src/components/Selector/index.tsx (2)
348-368: Fix LGTM — controlledvaluecorrectly resolves the focus-loss bug.Dropping the
key={...defaultDataValue}pattern (which forced a remount on every default change and stole focus) in favor of the controlledvalue={selectedValue}is the right approach for#2830. Mirroring this in the sibling selectors (CompanySelector,GenreSelector,StatusSelector,UserSelector), which still rely on the dynamickey+defaultValuepattern, would be a nice follow-up if those filters can also be appended in-place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Selector/index.tsx` around lines 348 - 368, Sibling selectors (CompanySelector, GenreSelector, StatusSelector, UserSelector) still use the remount-causing pattern (key={...defaultDataValue} + defaultValue) and should mirror the fix applied in Selector/index.tsx: remove the dynamic key and defaultValue usage, make each selector fully controlled by wiring its value prop to component state (e.g., selectedValue) and call the existing onChange handler to update state, keep other props (isMulti, isDisabled, loadOptions, placeholder, noOptionsMessage) intact; locate the components by their component names and replace the key/defaultValue pattern with value={selectedValue} and the same onChange logic used in Selector's AsyncSelect to prevent focus loss on updates.
297-299: Remove the redundant| nullin the state type.
SingleValue<SingleVal>fromreact-selectalready expands toSingleVal | null, making the trailing| nullredundant:Suggested change
- const [selectedValue, setSelectedValue] = useState< - MultiValue<SingleVal> | SingleValue<SingleVal> | null - >(null); + const [selectedValue, setSelectedValue] = useState< + MultiValue<SingleVal> | SingleValue<SingleVal> + >(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Selector/index.tsx` around lines 297 - 299, The state type for selectedValue is using an unnecessary union with null; update the useState generic so selectedValue is typed as MultiValue<SingleVal> | SingleValue<SingleVal> (remove the trailing | null) and keep the same identifiers (selectedValue, setSelectedValue, useState, MultiValue<SingleVal>, SingleValue<SingleVal>) so the nullability provided by SingleValue<SingleVal> is relied on instead of repeating | null.
🤖 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/Selector/index.tsx`:
- Around line 301-330: The effect's async loader (loadDefaultKeywords) can race
and clobber user selection; modify useEffect to (1) bail out early when the
incoming defaultValue already matches the current selectedValue (compare string
form of selectedValue vs defaultValue) to avoid redundant refetches, (2) add a
local cancelled flag or AbortController inside loadDefaultKeywords and check it
before calling setSelectedValue so stale responses never overwrite state, and
(3) either include selectedValue in the effect deps or read selectedValue from a
ref inside the effect to avoid unnecessary re-runs; target the
loadDefaultKeywords, setSelectedValue, defaultValue, selectedValue and isMulti
symbols when making these changes.
---
Nitpick comments:
In `@src/components/Selector/index.tsx`:
- Around line 348-368: Sibling selectors (CompanySelector, GenreSelector,
StatusSelector, UserSelector) still use the remount-causing pattern
(key={...defaultDataValue} + defaultValue) and should mirror the fix applied in
Selector/index.tsx: remove the dynamic key and defaultValue usage, make each
selector fully controlled by wiring its value prop to component state (e.g.,
selectedValue) and call the existing onChange handler to update state, keep
other props (isMulti, isDisabled, loadOptions, placeholder, noOptionsMessage)
intact; locate the components by their component names and replace the
key/defaultValue pattern with value={selectedValue} and the same onChange logic
used in Selector's AsyncSelect to prevent focus loss on updates.
- Around line 297-299: The state type for selectedValue is using an unnecessary
union with null; update the useState generic so selectedValue is typed as
MultiValue<SingleVal> | SingleValue<SingleVal> (remove the trailing | null) and
keep the same identifiers (selectedValue, setSelectedValue, useState,
MultiValue<SingleVal>, SingleValue<SingleVal>) so the nullability provided by
SingleValue<SingleVal> is relied on instead of repeating | null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09df69ad-1c36-4829-ad59-dd8b140f8f4a
📒 Files selected for processing (1)
src/components/Selector/index.tsx
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/Selector/index.tsx (1)
301-330:⚠️ Potential issue | 🟠 MajorPotential race condition: stale
loadDefaultKeywordsresolution can clobber the user's just-added selection.After this change, every user selection round-trips through the parent (URL params / form state) and pushes a new
defaultValueback into the component, which re-fires this effect and starts a freshaxios.getper keyword id. There is no cancellation, so when the user adds keywords in quick succession (the exact UX this PR enables — typing, hitting enter, repeating), the in-flight requests can resolve out of order: an earlier invocation finishing last will callsetSelectedValuewith a smaller/older array and silently drop the most recently added entry. Additionally, if/api/v1/keyword/:idreturnsnullfor a freshly-selected keyword, thevalidKeywordsfilter strips it and the selection visibly reverts.Guard the async work with a cancellation flag (or
AbortController) so only the latest invocation can update state.🔒 Suggested fix: ignore stale resolutions
useEffect(() => { + let cancelled = false; const loadDefaultKeywords = async (): Promise<void> => { if (!defaultValue) { - setSelectedValue(null); + if (!cancelled) setSelectedValue(null); return; } const keywords = await Promise.all( defaultValue.split(',').map(async (keywordId) => { const keyword = await axios.get<Keyword | null>( `/api/v1/keyword/${keywordId}` ); return keyword.data; }) ); + if (cancelled) return; + const validKeywords: Keyword[] = keywords.filter( (keyword): keyword is Keyword => keyword !== null ); const nextValue = validKeywords.map((keyword) => ({ label: keyword.name, value: keyword.id, })); setSelectedValue(isMulti ? nextValue : (nextValue[0] ?? null)); }; loadDefaultKeywords(); + return () => { + cancelled = true; + }; }, [defaultValue, isMulti]);As a follow-up, consider short‑circuiting the refetch when
selectedValuealready matchesdefaultValue(the values were just produced byloadKeywordOptionsand don't need re-fetching) — that would also avoid the “API returns null → selection disappears” edge case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Selector/index.tsx` around lines 301 - 330, The effect that defines loadDefaultKeywords can clobber newer user selections because in-flight axios.get calls resolve out of order; modify loadDefaultKeywords (and its enclosing useEffect) to use a cancellation mechanism (e.g., create an AbortController or a local `let cancelled = false`) tied to the effect cleanup so only the latest invocation updates state: pass the controller.signal to axios.get (or check cancelled after each await), and in the useEffect cleanup call controller.abort() or set cancelled = true to prevent stale resolutions from calling setSelectedValue; keep the existing mapping/validation logic (defaultValue.split, validKeywords, nextValue) but gate the final setSelectedValue(isMulti ? nextValue : (nextValue[0] ?? null)) behind the cancellation check.
🤖 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/Selector/index.tsx`:
- Around line 301-330: The effect that defines loadDefaultKeywords can clobber
newer user selections because in-flight axios.get calls resolve out of order;
modify loadDefaultKeywords (and its enclosing useEffect) to use a cancellation
mechanism (e.g., create an AbortController or a local `let cancelled = false`)
tied to the effect cleanup so only the latest invocation updates state: pass the
controller.signal to axios.get (or check cancelled after each await), and in the
useEffect cleanup call controller.abort() or set cancelled = true to prevent
stale resolutions from calling setSelectedValue; keep the existing
mapping/validation logic (defaultValue.split, validKeywords, nextValue) but gate
the final setSelectedValue(isMulti ? nextValue : (nextValue[0] ?? null)) behind
the cancellation check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ae36a47-70ef-4b59-83bc-c15fd2855c1c
📒 Files selected for processing (1)
src/components/Selector/index.tsx
Co-authored-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
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/Selector/index.tsx (1)
301-330:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against stale async writes in keyword default loading.
When
defaultValuechanges rapidly, an older in-flight request can resolve last and overwrite a newer selection. Please gatesetSelectedValuewith effect cleanup/cancellation.Suggested fix
useEffect(() => { + let cancelled = false; + const loadDefaultKeywords = async (): Promise<void> => { if (!defaultValue) { - setSelectedValue(null); + if (!cancelled) { + setSelectedValue(null); + } return; } const keywords = await Promise.all( defaultValue.split(',').map(async (keywordId) => { const keyword = await axios.get<Keyword | null>( `/api/v1/keyword/${keywordId}` ); return keyword.data; }) ); const validKeywords: Keyword[] = keywords.filter( (keyword): keyword is Keyword => keyword !== null ); const nextValue = validKeywords.map((keyword) => ({ label: keyword.name, value: keyword.id, })); - setSelectedValue(isMulti ? nextValue : (nextValue[0] ?? null)); + if (!cancelled) { + setSelectedValue(isMulti ? nextValue : (nextValue[0] ?? null)); + } }; loadDefaultKeywords(); + return () => { + cancelled = true; + }; }, [defaultValue, isMulti]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Selector/index.tsx` around lines 301 - 330, The async loadDefaultKeywords effect can write stale results when defaultValue changes; modify it to cancel/outdate prior requests by using an in-effect token (e.g., a boolean `isCurrent` or AbortController) and check that token before calling setSelectedValue. Specifically, inside the useEffect that defines loadDefaultKeywords, create a local `isCurrent = true` (or an AbortController) before starting async work, and in the cleanup return set `isCurrent = false` (or call controller.abort()); then, before calling setSelectedValue(...) only proceed if `isCurrent` is still true (or the request wasn't aborted). Keep the rest of loadDefaultKeywords (axios.get, mapping to nextValue) the same and ensure the effect still depends on defaultValue and isMulti.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/Selector/index.tsx`:
- Around line 301-330: The async loadDefaultKeywords effect can write stale
results when defaultValue changes; modify it to cancel/outdate prior requests by
using an in-effect token (e.g., a boolean `isCurrent` or AbortController) and
check that token before calling setSelectedValue. Specifically, inside the
useEffect that defines loadDefaultKeywords, create a local `isCurrent = true`
(or an AbortController) before starting async work, and in the cleanup return
set `isCurrent = false` (or call controller.abort()); then, before calling
setSelectedValue(...) only proceed if `isCurrent` is still true (or the request
wasn't aborted). Keep the rest of loadDefaultKeywords (axios.get, mapping to
nextValue) the same and ensure the effect still depends on defaultValue and
isMulti.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccd16b77-ce49-4c31-9844-413a9e1df0bd
📒 Files selected for processing (1)
src/components/Selector/index.tsx
4 participants
Description
The PR fixes an annoying issue in Discover where the keyword and exclude-keyword filters would lose focus every time you selected a value. This happened because the selector component was being remounted on each update.
How Has This Been Tested?
Typed a keyword, hit enter, repeated without loosing input focus.
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit