From 1d377c4b4f61c46cf0ab9cb1521201d8ddea1e8d Mon Sep 17 00:00:00 2001 From: Gabriel Horacio Cutrini Date: Thu, 21 May 2026 14:48:52 -0300 Subject: [PATCH 1/3] feat(utils): add useEventCallback hook 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. --- src/utils/use-event-callback.js | 32 ++++++++++++++++++++++++++++++++ webpack.common.js | 1 + 2 files changed, 33 insertions(+) create mode 100644 src/utils/use-event-callback.js diff --git a/src/utils/use-event-callback.js b/src/utils/use-event-callback.js new file mode 100644 index 00000000..561b5974 --- /dev/null +++ b/src/utils/use-event-callback.js @@ -0,0 +1,32 @@ +/** + * Copyright 2026 OpenStack Foundation + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + **/ + +import React from "react"; + +// Returns a callback with a stable identity that always invokes the latest +// version of `fn`. Useful for calling props (like a parent's onChange) from +// inside an effect without listing them as deps, which would re-run the effect +// every time the consumer passes a new inline arrow function. +// +// Stable-identity wrapper over the latest-ref pattern; equivalent to React's +// still-experimental useEffectEvent. Replace with the native hook when it +// ships in a stable release. +const useEventCallback = (fn) => { + const ref = React.useRef(fn); + React.useLayoutEffect(() => { + ref.current = fn; + }); + return React.useCallback((...args) => ref.current(...args), []); +}; + +export default useEventCallback; diff --git a/webpack.common.js b/webpack.common.js index eae3e602..c852ecdc 100644 --- a/webpack.common.js +++ b/webpack.common.js @@ -160,6 +160,7 @@ module.exports = { 'i18n': './src/i18n/i18n.js', 'utils/questions-set': './src/utils/questions-set.js', 'utils/money': './src/utils/money.js', + 'utils/use-event-callback': './src/utils/use-event-callback.js', }, output: { path: path.resolve(__dirname, 'lib'), From 55316bf5e6dc575e836f05085d5db2eb56fc724b Mon Sep 17 00:00:00 2001 From: Gabriel Horacio Cutrini Date: Thu, 21 May 2026 14:49:13 -0300 Subject: [PATCH 2/3] fix(company-input-v2): dropdown UX cleanup, clear-icon, canonical-case 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. --- src/components/inputs/company-input-v2.js | 191 +++++++++++----------- 1 file changed, 96 insertions(+), 95 deletions(-) diff --git a/src/components/inputs/company-input-v2.js b/src/components/inputs/company-input-v2.js index e99338cc..e08d9586 100644 --- a/src/components/inputs/company-input-v2.js +++ b/src/components/inputs/company-input-v2.js @@ -13,31 +13,74 @@ import React from "react"; import PropTypes from "prop-types"; -import T from "i18n-react/dist/i18n-react"; import { TextField, Autocomplete, Typography } from "@mui/material"; import { queryRegistrationCompanies } from "../../utils/query-actions"; +import useEventCallback from "../../utils/use-event-callback"; + +// Any well-formed company object (has a name string). +export const isCompanyObject = (o) => + !!o && typeof o === "object" && typeof o.name === "string"; + +// A company already in the database (positive id assigned by the API). +export const isExistingCompany = (o) => isCompanyObject(o) && o.id > 0; + +// A company name the user typed that isn't in the database yet +// (id === 0 is the sentinel autoSelect uses for free-text values). +export const isNewCompany = (o) => isCompanyObject(o) && o.id === 0 && !!o.name.trim(); + +// Find an existing company in `candidates` whose name matches `name` +// case-insensitively. Returns null if `name` is empty or no match found. +export const findExistingByName = (candidates, name) => { + const trimmed = name?.trim().toLowerCase(); + if (!trimmed) return null; + return (candidates || []).find( + (c) => isExistingCompany(c) && c.name.toLowerCase() === trimmed + ) || null; +}; + +// Treat empty strings, null/undefined, and empty-name objects as no selection. +// MUI's Autocomplete renders the clear (x) icon whenever value is truthy, so +// without this an empty-name object would keep the clear icon visible on hover +// of an apparently empty field. +export const normalizeCompanyValue = (v) => { + if (!v) return null; + if (typeof v === "string") return v.trim() ? v : null; + if (typeof v === "object" && typeof v.name === "string" && v.name.trim()) return v; + return null; +}; const CompanyInputV2 = ({ summitId, isRequired, sx, onChange, id, name, label, value, error, helperText, onBlur, placeholder, options2Show, disableShrink, ...rest }) => { const [inputValue, setInputValue] = React.useState(""); const [options, setOptions] = React.useState([]); - const noCompanyMatchText = T.translate("request_modal.no_company_match"); - const createAccessRequestText = (companyName) => - T.translate("request_modal.create_company_access_request", { - companyName: `"${companyName}"` - }); + // Memoised so the effect below doesn't re-run on every render. + const normalizedValue = React.useMemo(() => normalizeCompanyValue(value), [value]); + + // Stable wrapper around the parent's onChange. Consumers commonly pass an + // inline arrow function (new identity each render), so depending on + // `onChange` directly in the effect below would re-run it every render and + // cause an infinite loop of network calls. + const fireChange = useEventCallback((nextValue) => { + onChange({ target: { id: name, value: nextValue, type: "companyinput" } }); + }); React.useEffect(() => { if (inputValue === "") { - setOptions(value ? [value] : []); + setOptions(normalizedValue ? [normalizedValue] : []); return undefined; } + // Guard against the in-flight callback firing after the user clears the + // field (or types something else): without this, a late response would + // call onChange with the previous typed value and clobber the clear. + let cancelled = false; queryRegistrationCompanies(summitId, inputValue, (results) => { + if (cancelled) return; + let newOptions = []; - if (value) { - newOptions = [value]; + if (normalizedValue) { + newOptions = [normalizedValue]; } if (results) { @@ -45,9 +88,20 @@ const CompanyInputV2 = ({ summitId, isRequired, sx, onChange, id, name, label, v } setOptions(newOptions); + + // If the user typed and blurred faster than the API responded, the + // free-text commit already happened. Once the response arrives, if + // there is a case-insensitive existing match, replace the free-text + // value with the canonical option. + if (isNewCompany(normalizedValue)) { + const match = findExistingByName(results, normalizedValue.name); + if (match) { + fireChange(match); + } + } }, options2Show); - return undefined; - }, [value, inputValue, summitId, options2Show]); + return () => { cancelled = true; }; + }, [normalizedValue, inputValue, summitId, options2Show, fireChange]); return ( { if (onBlur) onBlur(name) }} getOptionLabel={(option) => { - // Value selected with enter, right from the input - if (typeof option === "string") { - return option; - } - // Add "xxx" option created dynamically - if (option.inputValue) { - return option.inputValue; - } - // Regular option + if (typeof option === "string") return option; return option.name; }} 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); + }); }} onInputChange={(_, newInputValue) => { setInputValue(newInputValue); }} - filterOptions={(options, params) => { - const { inputValue } = params; - const trimmedInput = inputValue.trim(); - const filtered = [...options]; - - // Suggest the creation of a new value - const isExisting = options.some( - (option) => { - if (typeof option === "string") { - return option.toLowerCase() === trimmedInput.toLowerCase(); - } - - return option.name?.toLowerCase() === trimmedInput.toLowerCase(); - } - ); - - if (trimmedInput !== "" && !isExisting) { - filtered.push({ - inputValue: trimmedInput, - name: noCompanyMatchText - }); - } - - return filtered; - }} + // The API already filters server-side; disable MUI's client-side filtering + // so all returned matches stay visible regardless of substring match. + filterOptions={(opts) => opts} renderInput={(params) => ( { const { key, ...optionProps } = props; - const isCreateOption = Boolean(option.inputValue); - + // Mirror getOptionLabel: string options come through when the + // consumer passes value as a plain string. Without this guard + // those rows render empty. + const label = typeof option === "string" ? option : option?.name; return ( // eslint-disable-next-line react/jsx-props-no-spreading -
  • - {isCreateOption ? ( - <> - - {noCompanyMatchText} - - - {createAccessRequestText(option.inputValue)} - - - ) : ( - - {option.name} - - )} +
  • + + {label} +
    • ); }} From 706da6089d2982df1feed5c4edabf0dc24086081 Mon Sep 17 00:00:00 2001 From: Gabriel Horacio Cutrini Date: Thu, 21 May 2026 14:49:47 -0300 Subject: [PATCH 3/3] test(company-input-v2): cover helpers, blur swap, post-API replacement, 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. --- .../inputs/__tests__/company-input-v2.test.js | 247 ++++++++++++++++++ 1 file changed, 247 insertions(+) create mode 100644 src/components/inputs/__tests__/company-input-v2.test.js diff --git a/src/components/inputs/__tests__/company-input-v2.test.js b/src/components/inputs/__tests__/company-input-v2.test.js new file mode 100644 index 00000000..6cd0479a --- /dev/null +++ b/src/components/inputs/__tests__/company-input-v2.test.js @@ -0,0 +1,247 @@ +/** + * @jest-environment jsdom + * + * Copyright 2025 OpenStack Foundation + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + */ + +import React from "react"; +import { render, screen, fireEvent, waitFor, act } from "@testing-library/react"; +import "@testing-library/jest-dom"; + +import CompanyInputV2, { + isCompanyObject, + isExistingCompany, + isNewCompany, + findExistingByName, + normalizeCompanyValue +} from "../company-input-v2"; + +// Mock the API helper so tests can drive the callback synchronously. +jest.mock("../../../utils/query-actions", () => ({ + queryRegistrationCompanies: jest.fn() +})); +// eslint-disable-next-line import/first +import { queryRegistrationCompanies } from "../../../utils/query-actions"; + +describe("isCompanyObject", () => { + it("returns true for objects with a name string", () => { + expect(isCompanyObject({ name: "Tipit" })).toBe(true); + expect(isCompanyObject({ id: 5, name: "Tipit" })).toBe(true); + expect(isCompanyObject({ id: 0, name: "" })).toBe(true); + }); + + it("returns false for non-objects or objects missing a name string", () => { + expect(isCompanyObject(null)).toBe(false); + expect(isCompanyObject(undefined)).toBe(false); + expect(isCompanyObject("Tipit")).toBe(false); + expect(isCompanyObject(5)).toBe(false); + expect(isCompanyObject({})).toBe(false); + expect(isCompanyObject({ name: 5 })).toBe(false); + expect(isCompanyObject({ name: null })).toBe(false); + }); +}); + +describe("isExistingCompany", () => { + it("returns true when the object has a positive id and name string", () => { + expect(isExistingCompany({ id: 1, name: "Tipit" })).toBe(true); + expect(isExistingCompany({ id: 999, name: "Tipco" })).toBe(true); + }); + + it("returns false for free-text entries (id === 0)", () => { + expect(isExistingCompany({ id: 0, name: "Acme" })).toBe(false); + }); + + it("returns false for non-objects, missing/zero id, or missing name", () => { + expect(isExistingCompany(null)).toBe(false); + expect(isExistingCompany({ name: "Tipit" })).toBe(false); // no id + expect(isExistingCompany({ id: -1, name: "Tipit" })).toBe(false); + expect(isExistingCompany({ id: 5 })).toBe(false); // no name + }); +}); + +describe("isNewCompany", () => { + it("returns true for free-text entries with a non-empty name", () => { + expect(isNewCompany({ id: 0, name: "Acme" })).toBe(true); + expect(isNewCompany({ id: 0, name: " Acme " })).toBe(true); + }); + + it("returns false for existing companies (id > 0)", () => { + expect(isNewCompany({ id: 1, name: "Tipit" })).toBe(false); + }); + + it("returns false for empty or whitespace-only names", () => { + expect(isNewCompany({ id: 0, name: "" })).toBe(false); + expect(isNewCompany({ id: 0, name: " " })).toBe(false); + }); + + it("returns false for non-objects or missing name", () => { + expect(isNewCompany(null)).toBe(false); + expect(isNewCompany({ id: 0 })).toBe(false); + expect(isNewCompany("Acme")).toBe(false); + }); +}); + +describe("findExistingByName", () => { + const existing = [ + { id: 1, name: "Tipit" }, + { id: 2, name: "Tipco" }, + { id: 3, name: "ACME Corp" } + ]; + + it("returns the matching existing company when name matches case-insensitively", () => { + expect(findExistingByName(existing, "tipit")).toEqual({ id: 1, name: "Tipit" }); + expect(findExistingByName(existing, "TIPCO")).toEqual({ id: 2, name: "Tipco" }); + expect(findExistingByName(existing, " acme corp ")).toEqual({ id: 3, name: "ACME Corp" }); + }); + + it("returns null when no existing company matches", () => { + expect(findExistingByName(existing, "Nonexistent")).toBeNull(); + }); + + it("returns null for empty/missing inputs", () => { + expect(findExistingByName(existing, "")).toBeNull(); + expect(findExistingByName(existing, " ")).toBeNull(); + expect(findExistingByName(existing, undefined)).toBeNull(); + expect(findExistingByName(null, "Tipit")).toBeNull(); + }); + + it("ignores free-text entries (id === 0) when searching", () => { + const mixed = [ + { id: 0, name: "tipit" }, // free-text entry + { id: 1, name: "Tipit" } // real company + ]; + // Should pick the real one even though the free-text comes first + expect(findExistingByName(mixed, "tipit")).toEqual({ id: 1, name: "Tipit" }); + }); + + it("returns null when only free-text entries are present", () => { + const freeTextOnly = [{ id: 0, name: "Acme" }]; + expect(findExistingByName(freeTextOnly, "Acme")).toBeNull(); + }); +}); + +describe("normalizeCompanyValue", () => { + it("returns the value unchanged when it has a real name", () => { + const v = { id: 1, name: "Tipit" }; + expect(normalizeCompanyValue(v)).toBe(v); + expect(normalizeCompanyValue("Tipit")).toBe("Tipit"); + }); + + it("returns null for null/undefined", () => { + expect(normalizeCompanyValue(null)).toBeNull(); + expect(normalizeCompanyValue(undefined)).toBeNull(); + }); + + it("returns null for empty strings", () => { + expect(normalizeCompanyValue("")).toBeNull(); + expect(normalizeCompanyValue(" ")).toBeNull(); + }); + + it("returns null for objects with no name or empty name", () => { + expect(normalizeCompanyValue({})).toBeNull(); + expect(normalizeCompanyValue({ id: 5 })).toBeNull(); + expect(normalizeCompanyValue({ id: 0, name: "" })).toBeNull(); + expect(normalizeCompanyValue({ id: 0, name: " " })).toBeNull(); + }); +}); + +describe("CompanyInputV2 integration", () => { + beforeEach(() => { + queryRegistrationCompanies.mockReset(); + }); + + // Helper: render the component inside a controlled wrapper so we can react + // to onChange the same way a real form would (updating `value`). + const renderControlled = ({ initialValue = null, onChange } = {}) => { + let setValue; + const Wrapper = () => { + const [value, _setValue] = React.useState(initialValue); + setValue = _setValue; + return ( + { + if (onChange) onChange(ev); + // Mirror what a real form does: write the value back. + _setValue(ev.target.value); + }} + /> + ); + }; + const utils = render(); + return { ...utils, getValue: () => setValue }; + }; + + it("on blur commits the canonical existing match when the typed text matches case-insensitively", () => { + // Capture the API callback so we can resolve it manually. + let resolveQuery; + queryRegistrationCompanies.mockImplementation((_summitId, _input, cb) => { + resolveQuery = cb; + }); + + const onChange = jest.fn(); + renderControlled({ onChange }); + const input = screen.getByRole("combobox"); + + // Type "tipit": triggers onInputChange -> effect -> queryRegistrationCompanies + fireEvent.change(input, { target: { value: "tipit" } }); + // Resolve the API with the canonical company. + act(() => { resolveQuery([{ id: 1, name: "Tipit" }]); }); + + // Blur: autoSelect commits the typed string; our onChange handler maps + // it to the canonical option via findExistingByName. + fireEvent.blur(input); + + // Find the call where the canonical value landed. + const committed = onChange.mock.calls + .map((c) => c[0].target.value) + .find((v) => v && typeof v === "object" && v.id === 1); + expect(committed).toEqual({ id: 1, name: "Tipit" }); + }); + + 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("does not render the clear icon when the value is an empty-name object", () => { + queryRegistrationCompanies.mockImplementation(() => {}); + render( + {}} + /> + ); + // MUI's clear button uses aria-label="Clear". + expect(screen.queryByLabelText("Clear")).not.toBeInTheDocument(); + }); +});