fix(company-input-v2): dropdown UX cleanup, clear-icon, canonical-case fixes (v5 port of #246)#250
Open
gcutrini wants to merge 3 commits into
Open
fix(company-input-v2): dropdown UX cleanup, clear-icon, canonical-case fixes (v5 port of #246)#250gcutrini wants to merge 3 commits into
gcutrini wants to merge 3 commits into
Conversation
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.
…e fixes - Drops the 'no match / use typed value' row. Dropdown only shows real company suggestions; typing a non-match commits the text as a free-text entry on blur without an extra row. - On blur, if the typed text matches an existing company case-insensitively, picks the canonical option (e.g. typing 'tipit' lands on 'Tipit'). - When blur beats the API response, the free-text commit is replaced with the canonical match once results arrive (cancellation flag prevents this firing after a later clear). - Treats empty strings and empty-name value objects as no selection so the clear (x) icon does not appear on hover of an apparently empty field. - Stabilises the parent onChange via useEventCallback so inline arrow consumers do not re-run the effect every render. - De-dupes the blur-committed value against options on the same id, and resolves renderOption labels for string-valued options.
…t, and empty-name clear icon - 18 unit tests for the new helpers (isCompanyObject, isExistingCompany, isNewCompany, findExistingByName, normalizeCompanyValue). - 3 integration tests through the real component: blur canonical-case swap, post-API replacement when blur beats the response, clear icon hidden for empty-name value.
📝 WalkthroughWalkthroughA new ChangesCompany Input Normalization with Event Callback Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 2
🤖 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/__tests__/company-input-v2.test.js`:
- Around line 208-231: The test fails because it renders with initialValue
{id:0,name:"tipit"} so typing the same text doesn't trigger
queryRegistrationCompanies; change the scenario to start empty, set
queryRegistrationCompanies.mockImplementation((_summitId, _input, cb) => {
resolveQuery = cb; }) before render, renderControlled with no initialValue (or
empty string), use fireEvent.change(input, { target: { value: "tipit" } }) then
fireEvent.blur(input) to commit the free-text, then call act(() => {
resolveQuery([{ id: 1, name: "Tipit" }]); }) and finally assert that onChange
received the promoted canonical object (use the existing onChange mock and
promoted extraction).
In `@src/components/inputs/company-input-v2.js`:
- Around line 123-145: The handler currently keeps whitespace-only strings as
truthy values; update the onChange block in company-input-v2.js so that after
computing trimmed = tmpValue.trim() you set tmpValue to null when trimmed === ""
and otherwise set tmpValue = findExistingByName(options, trimmed) || { id: 0,
name: trimmed }; keep using setOptions and onChange but ensure they receive null
for trimmed-empty input (so setOptions uses the existing falsy branch and the
emitted onChange.target.value is null, matching normalizeCompanyValue). Refer to
tmpValue, findExistingByName, setOptions, onChange and normalizeCompanyValue
when making the change.
🪄 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: 451b0c4e-ac6d-410b-923a-7303ceb7ac6f
📒 Files selected for processing (4)
src/components/inputs/__tests__/company-input-v2.test.jssrc/components/inputs/company-input-v2.jssrc/utils/use-event-callback.jswebpack.common.js
Comment on lines
+208
to
+231
| it("auto-replaces a free-text commit with the canonical match when the API response arrives after blur", () => { | ||
| // Withhold the API callback to simulate the network being slower than blur. | ||
| let resolveQuery; | ||
| queryRegistrationCompanies.mockImplementation((_summitId, _input, cb) => { | ||
| resolveQuery = cb; | ||
| }); | ||
|
|
||
| const onChange = jest.fn(); | ||
| // Start with the free-text commit already in place (what happens when | ||
| // blur fires before the response). | ||
| renderControlled({ initialValue: { id: 0, name: "tipit" }, onChange }); | ||
| const input = screen.getByRole("combobox"); | ||
|
|
||
| // Type to populate inputValue so the effect kicks in. | ||
| fireEvent.change(input, { target: { value: "tipit" } }); | ||
|
|
||
| // Now the response arrives with the canonical option. | ||
| act(() => { resolveQuery([{ id: 1, name: "Tipit" }]); }); | ||
|
|
||
| // The component should have called onChange with the canonical option. | ||
| const promoted = onChange.mock.calls | ||
| .map((c) => c[0].target.value) | ||
| .find((v) => v && typeof v === "object" && v.id === 1); | ||
| expect(promoted).toEqual({ id: 1, name: "Tipit" }); |
There was a problem hiding this comment.
This integration test does not reliably start the mocked query.
The component is rendered with value={{ id: 0, name: "tipit" }}, then the test changes the input to the same "tipit" string. In CI that path never invokes queryRegistrationCompanies, so resolveQuery stays unset and Line 225 crashes exactly as the pipeline shows. Drive the real sequence instead: start empty, type "tipit", blur to commit free-text, then resolve the queued callback.
More stable test shape
- it("auto-replaces a free-text commit with the canonical match when the API response arrives after blur", () => {
+ it("auto-replaces a free-text commit with the canonical match when the API response arrives after blur", async () => {
// Withhold the API callback to simulate the network being slower than blur.
let resolveQuery;
queryRegistrationCompanies.mockImplementation((_summitId, _input, cb) => {
resolveQuery = cb;
});
const onChange = jest.fn();
- // Start with the free-text commit already in place (what happens when
- // blur fires before the response).
- renderControlled({ initialValue: { id: 0, name: "tipit" }, onChange });
+ renderControlled({ onChange });
const input = screen.getByRole("combobox");
- // Type to populate inputValue so the effect kicks in.
+ // Type to start the query, then blur before the response arrives.
fireEvent.change(input, { target: { value: "tipit" } });
+ fireEvent.blur(input);
+ await waitFor(() => expect(queryRegistrationCompanies).toHaveBeenCalled());
// Now the response arrives with the canonical option.
act(() => { resolveQuery([{ id: 1, name: "Tipit" }]); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("auto-replaces a free-text commit with the canonical match when the API response arrives after blur", () => { | |
| // Withhold the API callback to simulate the network being slower than blur. | |
| let resolveQuery; | |
| queryRegistrationCompanies.mockImplementation((_summitId, _input, cb) => { | |
| resolveQuery = cb; | |
| }); | |
| const onChange = jest.fn(); | |
| // Start with the free-text commit already in place (what happens when | |
| // blur fires before the response). | |
| renderControlled({ initialValue: { id: 0, name: "tipit" }, onChange }); | |
| const input = screen.getByRole("combobox"); | |
| // Type to populate inputValue so the effect kicks in. | |
| fireEvent.change(input, { target: { value: "tipit" } }); | |
| // Now the response arrives with the canonical option. | |
| act(() => { resolveQuery([{ id: 1, name: "Tipit" }]); }); | |
| // The component should have called onChange with the canonical option. | |
| const promoted = onChange.mock.calls | |
| .map((c) => c[0].target.value) | |
| .find((v) => v && typeof v === "object" && v.id === 1); | |
| expect(promoted).toEqual({ id: 1, name: "Tipit" }); | |
| it("auto-replaces a free-text commit with the canonical match when the API response arrives after blur", async () => { | |
| // Withhold the API callback to simulate the network being slower than blur. | |
| let resolveQuery; | |
| queryRegistrationCompanies.mockImplementation((_summitId, _input, cb) => { | |
| resolveQuery = cb; | |
| }); | |
| const onChange = jest.fn(); | |
| renderControlled({ onChange }); | |
| const input = screen.getByRole("combobox"); | |
| // Type to start the query, then blur before the response arrives. | |
| fireEvent.change(input, { target: { value: "tipit" } }); | |
| fireEvent.blur(input); | |
| await waitFor(() => expect(queryRegistrationCompanies).toHaveBeenCalled()); | |
| // Now the response arrives with the canonical option. | |
| act(() => { resolveQuery([{ id: 1, name: "Tipit" }]); }); | |
| // The component should have called onChange with the canonical option. | |
| const promoted = onChange.mock.calls | |
| .map((c) => c[0].target.value) | |
| .find((v) => v && typeof v === "object" && v.id === 1); | |
| expect(promoted).toEqual({ id: 1, name: "Tipit" }); |
🧰 Tools
🪛 GitHub Actions: jest / 0_build.txt
[error] 225-225: Test failed with TypeError: resolveQuery is not a function. Error occurs at: act(() => { resolveQuery([{ id: 1, name: "Tipit" }]); })
🪛 GitHub Actions: jest / build
[error] 225-225: Test failed with TypeError: resolveQuery is not a function (auto-replaces a free-text commit with the canonical match when the API response arrives after 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/__tests__/company-input-v2.test.js` around lines 208 -
231, The test fails because it renders with initialValue {id:0,name:"tipit"} so
typing the same text doesn't trigger queryRegistrationCompanies; change the
scenario to start empty, set
queryRegistrationCompanies.mockImplementation((_summitId, _input, cb) => {
resolveQuery = cb; }) before render, renderControlled with no initialValue (or
empty string), use fireEvent.change(input, { target: { value: "tipit" } }) then
fireEvent.blur(input) to commit the free-text, then call act(() => {
resolveQuery([{ id: 1, name: "Tipit" }]); }) and finally assert that onChange
received the promoted canonical object (use the existing onChange mock and
promoted extraction).
Comment on lines
123
to
+145
| onChange={(_, newValue) => { | ||
| let tmpValue = newValue?.inputValue || newValue; | ||
| // if new option is selected ... | ||
| if (newValue && typeof newValue === "object" && newValue.inputValue) { | ||
| tmpValue = { | ||
| id: 0, | ||
| name: newValue.inputValue | ||
| }; | ||
| } | ||
| // autoSelect commits the raw typed/autofilled string on blur; normalize to {id, name}. | ||
| let tmpValue = newValue; | ||
| // autoSelect commits the raw typed/autofilled string on blur. If the | ||
| // string matches an existing company case-insensitively, pick that | ||
| // option (so typing "tipit" tabs out to "Tipit"). Otherwise commit | ||
| // the typed value as a free-text {id: 0, name} entry. | ||
| if (typeof tmpValue === "string" && tmpValue.trim()) { | ||
| tmpValue = { id: 0, name: tmpValue.trim() }; | ||
| const trimmed = tmpValue.trim(); | ||
| tmpValue = findExistingByName(options, trimmed) || { id: 0, name: trimmed }; | ||
| } | ||
| setOptions(tmpValue ? [tmpValue, ...options] : options); | ||
| let ev = { | ||
| // Prepend the committed value but drop any existing entry with | ||
| // the same id; otherwise resolving to an existing company would | ||
| // produce a duplicate row when the dropdown next opens. | ||
| setOptions(tmpValue | ||
| ? [tmpValue, ...options.filter((o) => o?.id !== tmpValue?.id)] | ||
| : options); | ||
| onChange({ | ||
| target: { | ||
| id: name, | ||
| value: tmpValue, | ||
| type: "companyinput" | ||
| } | ||
| }; | ||
| onChange(ev); | ||
| }); |
There was a problem hiding this comment.
Normalize whitespace-only free-text to null.
A blur on " " skips the tmpValue.trim() branch, so this handler still emits a truthy whitespace string to the parent and re-inserts it into options, even though normalizeCompanyValue later treats that as empty. Converting trimmed-empty strings to null here keeps the onChange contract consistent.
Proposed fix
onChange={(_, newValue) => {
let tmpValue = newValue;
+ if (typeof tmpValue === "string") {
+ const trimmed = tmpValue.trim();
+ tmpValue = trimmed
+ ? findExistingByName(options, trimmed) || { id: 0, name: trimmed }
+ : null;
+ }
- // autoSelect commits the raw typed/autofilled string on blur. If the
- // string matches an existing company case-insensitively, pick that
- // option (so typing "tipit" tabs out to "Tipit"). Otherwise commit
- // the typed value as a free-text {id: 0, name} entry.
- if (typeof tmpValue === "string" && tmpValue.trim()) {
- const trimmed = tmpValue.trim();
- tmpValue = findExistingByName(options, trimmed) || { id: 0, name: trimmed };
- }
// Prepend the committed value but drop any existing entry with
// the same id; otherwise resolving to an existing company would
// produce a duplicate row when the dropdown next opens.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onChange={(_, newValue) => { | |
| let tmpValue = newValue?.inputValue || newValue; | |
| // if new option is selected ... | |
| if (newValue && typeof newValue === "object" && newValue.inputValue) { | |
| tmpValue = { | |
| id: 0, | |
| name: newValue.inputValue | |
| }; | |
| } | |
| // autoSelect commits the raw typed/autofilled string on blur; normalize to {id, name}. | |
| let tmpValue = newValue; | |
| // autoSelect commits the raw typed/autofilled string on blur. If the | |
| // string matches an existing company case-insensitively, pick that | |
| // option (so typing "tipit" tabs out to "Tipit"). Otherwise commit | |
| // the typed value as a free-text {id: 0, name} entry. | |
| if (typeof tmpValue === "string" && tmpValue.trim()) { | |
| tmpValue = { id: 0, name: tmpValue.trim() }; | |
| const trimmed = tmpValue.trim(); | |
| tmpValue = findExistingByName(options, trimmed) || { id: 0, name: trimmed }; | |
| } | |
| setOptions(tmpValue ? [tmpValue, ...options] : options); | |
| let ev = { | |
| // Prepend the committed value but drop any existing entry with | |
| // the same id; otherwise resolving to an existing company would | |
| // produce a duplicate row when the dropdown next opens. | |
| setOptions(tmpValue | |
| ? [tmpValue, ...options.filter((o) => o?.id !== tmpValue?.id)] | |
| : options); | |
| onChange({ | |
| target: { | |
| id: name, | |
| value: tmpValue, | |
| type: "companyinput" | |
| } | |
| }; | |
| onChange(ev); | |
| }); | |
| onChange={(_, newValue) => { | |
| let tmpValue = newValue; | |
| if (typeof tmpValue === "string") { | |
| const trimmed = tmpValue.trim(); | |
| tmpValue = trimmed | |
| ? findExistingByName(options, trimmed) || { id: 0, name: trimmed } | |
| : null; | |
| } | |
| // Prepend the committed value but drop any existing entry with | |
| // the same id; otherwise resolving to an existing company would | |
| // produce a duplicate row when the dropdown next opens. | |
| setOptions(tmpValue | |
| ? [tmpValue, ...options.filter((o) => o?.id !== tmpValue?.id)] | |
| : options); | |
| onChange({ | |
| target: { | |
| id: name, | |
| value: tmpValue, | |
| type: "companyinput" | |
| } | |
| }); |
🤖 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 123 - 145, The
handler currently keeps whitespace-only strings as truthy values; update the
onChange block in company-input-v2.js so that after computing trimmed =
tmpValue.trim() you set tmpValue to null when trimmed === "" and otherwise set
tmpValue = findExistingByName(options, trimmed) || { id: 0, name: trimmed };
keep using setOptions and onChange but ensure they receive null for
trimmed-empty input (so setOptions uses the existing falsy branch and the
emitted onChange.target.value is null, matching normalizeCompanyValue). Refer to
tmpValue, findExistingByName, setOptions, onChange and normalizeCompanyValue
when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b9rnr4r
Port of #246 to main (v5). Functionally identical to the v4.x version, re-indented to match the v5 codebase style and reusing the @testing-library/react@12 that v5 already has installed (no new deps).
21 tests, all passing.
Summary by CodeRabbit
Bug Fixes
Tests