fix(company-input-v2): dropdown UX cleanup, clear-icon, canonical-case fixes#246
Conversation
Drops the synthetic no-match dropdown row and the i18n keys backing it, restores the explicit 1em font size, and makes autoSelect commit the real API-matched option when the typed text matches case-insensitively (typing "tipit" tabs out to "Tipit"). Unrecognized values still commit as free-text. Adds unit tests for the new helpers.
Normalizes the incoming value (empty string, empty-name object) to null via a useMemo'd helper so MUI's Autocomplete no longer treats it as a selection and keeps the clear (x) icon visible on hover of an empty field. Adds unit tests for normalizeCompanyValue.
…the response If the user types and blurs faster than the debounced API call, the free-text value is already committed by the time the response arrives. The effect now checks for a case-insensitive existing match and replaces the free-text entry with the canonical option. Adds the isNewCompany helper and its tests.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| */ | ||
|
|
||
| import { |
There was a problem hiding this comment.
@gcutrini
Missing coverage:
- Blur triggers canonical-case swap through the real component (the core PR feature , tested nowhere).
- Post-API response swap when blur precedes the response (the race-condition fix , tested nowhere).
- Clear icon hidden when value is an empty-name object (the third headline fix , tested nowhere).
| return undefined; | ||
| } | ||
|
|
||
| queryRegistrationCompanies(summitId, inputValue, (results) => { |
There was a problem hiding this comment.
@gcutrini
Stale-closure race: late API callback fires onChange after user clears the field
No cleanup in the effect; the in-flight queryRegistrationCompanies callback closes over the old normalizedValue ({id:0, name:"tipit"}), so if the user clears the field before the response arrives, the callback still fires isNewCompany(normalizedValue) as true and calls onChange with the canonical company , overwriting the user's deliberate clear action.
suggested fix
Add a cancellation flag and return a cleanup function:
React.useEffect(() => {
if (inputValue === "") { ... return undefined; }
let cancelled = false; // ← flag
queryRegistrationCompanies(summitId, inputValue, (results) => {
if (cancelled) return; // ← bail if effect was re-run or component unmounted
...
if (isNewCompany(normalizedValue)) {
const match = findExistingByName(results, normalizedValue.name);
if (match) {
onChange({ target: { id: name, value: match, ... } });
}
}
}, options2Show);
return () => { cancelled = true; }; // ← cleanup: runs before next effect or on unmount
}, [normalizedValue, inputValue, summitId, options2Show, onChange, name]);
| }, options2Show); | ||
| return undefined; | ||
| }, [value, inputValue, summitId, options2Show]); | ||
| }, [normalizedValue, inputValue, summitId, options2Show, onChange, name]); |
There was a problem hiding this comment.
@gcutrini if u add the onChange on a deps array this will trigger an infinite loop on the effect
generally speaking most of the clients pass and inline arrow function and that arrow function object is a new one on each re render
so remove the onChange deps to avoid the infinite loop or use a a ref to hold the latest version of the onChange callback
Latest-ref wrapper that returns a stable-identity callback always invoking the latest version of the wrapped function. Lets effects call prop callbacks without listing them as deps. Equivalent to React's still- experimental useEffectEvent; replace when the native hook stabilizes.
The debounced queryRegistrationCompanies callback closes over the value of normalizedValue captured at effect run time. Without cleanup, a late response could fire onChange with the previous typed value after the user had already cleared the field, clobbering their action.
Consumers commonly pass onChange as an inline arrow function, which gets a new identity on every parent render. Listing it in the effect's deps would re-run the effect (and the debounced API call) every render. Wrap onChange via useEventCallback so the effect can call it safely without tracking its identity.
…pty-name clear icon Adds three integration tests through the real component: - on blur, typing a case-insensitive match commits the canonical option - when blur beats the API response, the free-text commit is replaced with the canonical match once results arrive - the clear icon is hidden when value is an empty-name object Pins @testing-library/react@^11 for React 16 compatibility.
|
@smarcet — pushed the changes:
Ready for another look. |
| <Typography | ||
| variant="body2" | ||
| sx={{ fontSize: "1em", color: "text.secondary", padding: "5px 0" }} | ||
| > |
There was a problem hiding this comment.
[LOW] renderOption renders blank for string-valued options
option.name is undefined when option is a string. normalizeCompanyValue preserves non-empty strings, so the options array can contain plain string entries (e.g. when the consumer passes value="Tipit"). MUI calls renderOption for those entries and <Typography> renders nothing.
getOptionLabel already handles this correctly — renderOption needs the same guard:
renderOption={(props, option) => {
const { key, ...optionProps } = props;
const label = typeof option === "string" ? option : option.name;
return (
<li key={key} {...optionProps}>
<Typography variant="body2" sx={{ fontSize: "1em", color: "text.secondary", padding: "5px 0" }}>
{label}
</Typography>
</li>
);
}}
| const trimmed = tmpValue.trim(); | ||
| tmpValue = findExistingByName(options, trimmed) || { id: 0, name: trimmed }; | ||
| } | ||
| setOptions(tmpValue ? [tmpValue, ...options] : options); |
There was a problem hiding this comment.
[LOW] Blur-committed company is duplicated in the options list
When findExistingByName resolves tmpValue to an existing company, that company is already present in options (it came from the prior API call). Prepending it unconditionally produces a duplicate entry the next time the dropdown opens.
// before: may duplicate an existing entry
setOptions(tmpValue ? [tmpValue, ...options] : options);
// after: filter out the same id before prepending
setOptions(tmpValue
? [tmpValue, ...options.filter(o => o?.id !== tmpValue?.id)]
: options
);This is visual-only and resolves itself on the next onInputChange, but it is noticeable when the user re-opens the dropdown immediately after blur.
… string options - The blur path could prepend a canonical company that was already in options, leaving a visible duplicate the next time the dropdown opens. Filter out the same id before prepending. - renderOption mirrored getOptionLabel's handling of string options incompletely, rendering an empty row when the consumer passed value as a plain string. Resolve the label the same way getOptionLabel does.
2 participants
ref: https://app.clickup.com/t/86b9rnr4r
Summary
Reworks the CompanyInputV2 field UX (after #241 enabled autoSelect on blur).
96abac9 show plain options, pick canonical case on blur. Removes the extra "no match / use typed value" row from the dropdown, and makes blur commit the canonical company when the typed text matches one regardless of case (e.g. typing "tipit" picks "Tipit").
243150c match canonical case post-API when blur beats the response. If the user types and blurs before the search request comes back, the typed text gets accepted as-is. Once the response arrives, if it included a matching company, the field swaps to that company automatically.
e3759dc hide clear icon when value is effectively empty. Treats blank values (empty string, value object with no name) as no selection so MUI stops showing the clear (x) icon on hover of a field that looks empty.