fix: company-input-v2 — explicit selection only, no clear icon, "Use "<typed>"" row#289
fix: company-input-v2 — explicit selection only, no clear icon, "Use "<typed>"" row#289JpMaxMan wants to merge 2 commits into
Conversation
…"<typed>"" row
Root cause of the wrong-company bug: `autoSelect` committed the currently
*highlighted* option on blur, so merely mousing over a suggestion and then
tabbing away silently populated the wrong company.
- Remove `autoSelect`. Selection is now explicit (click / Enter) only.
- New onBlur: tab/click-away commits exactly what was typed — resolved to an
existing company only on an exact (case-insensitive) name match, else a
free-text { id: 0, name }. Never commits a merely-highlighted option.
- `disableClearable`: remove the MUI clear (x) icon; the field commits free
text, so an explicit clear affordance isn't wanted (delete text to empty).
- Append a synthetic 'Use "<typed>"' row so users can explicitly commit their
text; skipped when it already matches a listed company. Commits a clean
{ id: 0, name }, dropping the display-only marker.
- Predictive typeahead unchanged (freeSolo + server-side queryRegistrationCompanies).
Tests: +5 (blur keeps typed text / never a highlighted option; clear icon never
renders even with a real value; "Use" row commits clean free text; no redundant
"Use" row on exact match). 25/25 green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCompanyInputV2 commits typed text on blur, resolves case-insensitive company matches, supports explicit ChangesCompanyInputV2 selection behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 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.
Inline comments:
In `@src/components/inputs/company-input-v2.js`:
- Around line 113-139: Update the onBlur handler to clear the committed company
when the trimmed inputValue is empty, by calling fireChange with null or the
component’s established clear value. Preserve the existing exact-match and
free-text behavior for non-empty input, avoid duplicate changes when already
cleared, and add a regression test covering delete-all-text followed by blur.
🪄 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: ecfeb9fa-e11a-4a27-bcea-861a0f48f74e
📒 Files selected for processing (2)
src/components/inputs/__tests__/company-input-v2.test.jssrc/components/inputs/company-input-v2.js
| freeSolo | ||
| // No clear (x) icon: the field commits free text, so an explicit clear | ||
| // affordance isn't wanted; users empty it by deleting the text. | ||
| disableClearable | ||
| includeInputInList | ||
| filterSelectedOptions | ||
| value={normalizedValue} | ||
| onBlur={() => { if (onBlur) onBlur(name) }} | ||
| // NOTE: `autoSelect` is intentionally NOT set. With it, blurring the field | ||
| // committed the currently *highlighted* option — so merely mousing over a | ||
| // suggestion and then tabbing away silently populated the wrong company. | ||
| // Selection is now explicit (click / Enter) only; see onBlur for the | ||
| // "keep what was typed" fallback. | ||
| onBlur={() => { | ||
| // On blur with no explicit selection, take the typed text as-is: | ||
| // resolve to an existing company only on an exact (case-insensitive) | ||
| // name match, otherwise commit it as a free-text { id: 0, name }. Never | ||
| // commit a merely-highlighted option. Skip when the text already matches | ||
| // the committed value (e.g. right after an explicit selection). | ||
| const typed = inputValue.trim(); | ||
| const currentName = isCompanyObject(normalizedValue) | ||
| ? normalizedValue.name | ||
| : (typeof normalizedValue === "string" ? normalizedValue : ""); | ||
| if (typed && typed.toLowerCase() !== currentName.trim().toLowerCase()) { | ||
| fireChange(findExistingByName(options, typed) || { id: 0, name: typed }); | ||
| } | ||
| if (onBlur) onBlur(name); | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Allow blur-to-empty to clear the selected company
When the user deletes all text and blurs, typed === "" skips fireChange, so the previously selected company stays committed while the field appears empty. With disableClearable, there’s no other UI path to clear the selection. Send null (or the component’s clear value) when the trimmed input is empty, and add a regression test for delete-all-text-then-blur.
🤖 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/inputs/company-input-v2.js` around lines 113 - 139, Update the
onBlur handler to clear the committed company when the trimmed inputValue is
empty, by calling fireChange with null or the component’s established clear
value. Preserve the existing exact-match and free-text behavior for non-empty
input, avoid duplicate changes when already cleared, and add a regression test
covering delete-all-text followed by blur.
…241) Follow-up to removing autoSelect. autoSelect (added in #241) committed the input's DOM value on blur, which is how it captured iOS-Chrome browser autofill (autofill writes to the DOM without firing onInputChange). The initial onBlur here read React input state, which would be stale for autofill and reintroduce the #241 required-field-validation bug. - onBlur now reads event.target.value (the DOM value), not React state, so typed AND autofilled values propagate on blur — while still committing the field text, never a highlighted option (the autoSelect-on-hover bug fix stands). - Test: commits a browser-autofilled value on blur even when onInputChange never fired. 26/26 green. Needs a Chrome-iOS device re-test of the #241 autofill path before merge.
Re:
|
1 participant
The bug
CompanyInputV2used MUI'sautoSelect, which commits the currently highlighted option on blur. Users would mouse over a suggestion, then tab away intending to keep their typed text — and the moused-over company got saved instead (wrong company populated).Changes (
src/components/inputs/company-input-v2.js)autoSelect— selection is now explicit (click / Enter) only. This is the root-cause fix.onBlur— tab/click-away commits exactly what was typed: resolved to an existing company only on an exact (case-insensitive) name match, otherwise a free-text{ id: 0, name }. Never commits a merely-highlighted option.disableClearable— removes the MUI clear (x) icon (the field commits free text, so an explicit clear affordance isn't wanted; delete the text to empty).Use "<typed>"row —filterOptionsappends a synthetic option so users can explicitly commit their typed text; skipped when it already matches a listed company. Commits a clean{ id: 0, name }(display-only marker stripped).renderOptionshows theUse "…"label;getOptionLabelstill returns the plain name.freeSolo+ server-sidequeryRegistrationCompanies).This affects all
CompanyInputV2consumers, not just the requesting screen: no more clear (x) icon, and blur no longer auto-selects a highlighted option (it keeps the typed text). Both are intentional — flagging for reviewers.Note: the Formik variant
CompanyInputMUI(mui/formik-inputs/company-input-mui.js) is a separate implementation and is not touched here; if a Formik form needs the same behavior we'll do it in a follow-up.Tests
+5 tests; the file's suite is 25/25 green:
Use "…"row commits clean free text; no redundantUserow on an exact match.Verified by running jest directly (
node_modules/.bin/jest) — CI's Node 18 avoids a localnode-sass/Node-25 build issue unrelated to this change.package.jsonversion intentionally not bumped (done separately at release).Summary by CodeRabbit
New Features
Bug Fixes