Validate the text of the CreatableSelect number input - #4073
Open
sholzer wants to merge 16 commits into
Open
Conversation
InputNumber renders a text input and only reports parseable numbers through onChange, so the number branch never saw what the user actually typed: "12aaa" left the last valid 12 in state while the field read "12aaa", deleting the leading digits re-validated nothing, and a below-min entry was silently rewritten to the boundary value on blur or Enter. Track the raw text through onInput alongside the committed value from onChange, and gate the Add button, the error message and handleAddOption on both. Passing the raw text as InputNumber's value is not an option -- feeding back a non-numeric string makes the controlled-value effect wipe the display -- so the committed value stays the value prop. changeOnBlur is disabled and the Enter key no longer propagates to InputNumber's own handler, because both flush the input and re-align it into [min, max]. min stays fully supported: it still bounds the steppers and sets aria-valuemin, but an out-of-range entry is now reported as invalid instead of being coerced. onChange also mirrors the committed value into the tracked text, since the steppers change the value without firing onInput. Fixes #1954 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Numeric bounds, custom parsers, and duplicate-entry resets are not handled correctly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves CreatableSelect number-input validation by tracking and validating raw user text.
Changes:
- Tracks raw numeric input independently from committed values.
- Prevents blur/Enter coercion and surfaces invalid-input errors.
- Adds locale-aware decimal validation.
File summaries
| File | Description |
|---|---|
assets/js/src/core/components/creatable-select/creatable-select.tsx |
Adds raw-text tracking and numeric validation. |
Review details
Suppressed comments (2)
assets/js/src/core/components/creatable-select/creatable-select.tsx:57
- This validity check ignores
numberInputProps.minandmax. Since blur coercion is now disabled and Enter flushing is suppressed, a consumer using onlynumberInputProps={{ min: 1 }}can type0and add it; the bounds affect the steppers but no longer constrain created options. Compare the parsed value against both bounds here rather than requiring callers to duplicate them invalidate.
if (inputType === 'number' && !Number.isFinite(Number(numberInputText.trim().replace(decimalSeparator, '.')))) {
assets/js/src/core/components/creatable-select/creatable-select.tsx:131
- The new raw-text state is only reset after a successful add. If an existing numeric option is submitted with Enter, the duplicate branch clears
newOptionTextand returns whilenumberInputTextremains stale; without a custom validator this leaves Add enabled for a blank input, and with one it can show “Invalid option” under the blank field. Reset both states in the duplicate branch.
setNumberInputText('')
- Files reviewed: 1/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+55
to
+62
| const isInputTextValid = useCallback((): boolean => { | ||
| // Grouping separators are rejected on purpose — "1,000" cannot be read unambiguously across locales. | ||
| if (inputType === 'number' && !Number.isFinite(Number(numberInputText.trim().replace(decimalSeparator, '.')))) { | ||
| return false | ||
| } | ||
|
|
||
| return validate === undefined || validate(newOptionText.trim()) | ||
| }, [inputType, numberInputText, decimalSeparator, newOptionText, validate]) |
Review follow-up to the raw-text validation, which had three gaps. min and max only constrained the steppers. InputNumber refuses to commit an out-of-range value, so requiring a committed value in the validity check enforces the bounds as well, rather than asking callers to repeat them in validate. Before this, a below-min entry left the Add button enabled while clicking it added nothing. Submitting an existing option with Enter cleared the committed value but left the tracked text behind, which showed "Invalid option" under a blank field. The duplicate branch now resets both. A parser supplied through numberInputProps was ignored, because the check applied Number to the displayed text. A consumer formatting the input would have had every entry reported invalid. Adds creatable-select.test.tsx covering the original issue and these three cases. Every one of the ten tests was confirmed to fail against the component before its respective fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Stale committed values can still be submitted, duplicate handling can discard invalid text, and consumer onInput callbacks are suppressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
assets/js/src/core/components/creatable-select/creatable-select.tsx:123
- Validate the raw numeric text before handling duplicates. If option
12already exists, typing12aaaleavesnewOptionTextas12; pressing Enter therefore enters this branch and clears the invalid text beforeisInputTextValid()runs, contrary to the new validation behavior. Preserve the existing string-input ordering while rejecting invalid numeric text first.
if (optionExists && !allowDuplicates) {
setNewOptionText('')
setNumberInputText('')
return
- Files reviewed: 2/3 changed files
- Comments generated: 2
- Review effort level: Balanced
The separator is read exclusively by the number branch of the validity check, but it was computed on every render of every instance. Resolving it builds an Intl.NumberFormat and calls formatToParts, so string-mode consumers -- the select document editable among them, which can appear many times in one editor -- paid for work they never use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up. Requiring a committed value was not enough, because that value can be left over from earlier keystrokes: entering 12 and then replacing it with a below-min 0 makes InputNumber withhold onChange, so the field read 0 while 12 was still committed -- and Add submitted 12. The parsed text must equal the committed number. handleAddOption checked for duplicates before validity, so submitting invalid text that shares a prefix with an existing option cleared the field instead of reporting the problem. Number inputs are now validated first; the string input keeps its previous ordering. An onInput handler passed through numberInputProps was dropped, since the explicit handler replaces it rather than sitting behind the spread. The component is exported from the SDK, so a consumer's callback simply stopped running. It is now invoked after the internal state update. The duplicate message was keyed to the committed value, so typing 12a with option 12 present showed both "already exists" -- describing a value no longer in the field -- and "invalid option". It is now gated on validity, which makes the two mutually exclusive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Locale ambiguity and invalid duplicate string submission remain incorrectly handled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
assets/js/src/core/components/creatable-select/creatable-select.tsx:125
- Restricting this early validity check to number inputs leaves string inputs inconsistent with the new message precedence. If an invalid string also matches an existing option, pressing Enter reaches the duplicate branch and clears the field even though the UI labels it “Invalid option.” Validate both input types before handling duplicates; valid duplicates will still be cleared as before.
if (inputType === 'number' && !isInputTextValid()) {
return
}
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Balanced
The validity check mirrored InputNumber's parsing, which reads "." as a decimal point in every locale. In de that made "1.000" commit 1, so the check agreed with it and Add created 1 from text denoting one thousand. It now rejects the separator the active locale does not use, so both "1.000" in de and "1,000" in en are reported instead of guessed. The separator itself still comes from the SDK's getDecimalSeparator. The two keydown handlers collapse into one. The number variant only existed to stop propagation before delegating, which meant testing for Enter twice across two callbacks. The test now renders the real InputNumber wrapper rather than raw antd. The wrapper supplies the locale decimalSeparator, without which the de cases pass or fail for reasons the component never sees. Comments trimmed to the decisions that are not evident from the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Number coercion breaks supported arbitrary-precision stringMode values and can accept stale committed values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Balanced
Reported by SonarQube on #4073. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Pressing Enter still clears invalid string input when it matches an existing option.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Balanced
The guard was scoped to number inputs, so a string that was both invalid and equal to an existing option reached the duplicate branch on Enter and had its text cleared before validation ran -- silently discarding input the UI had just marked invalid. The ordering predates this branch, but suppressing the duplicate message while the input is invalid made the inconsistency visible. Validity is now checked once, ahead of the duplicate branch, for both types. For string inputs the check reduces to the caller's validate, so callers that pass none see no change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Newly added nullish checks violate the project’s mandatory lodash type-check convention.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
assets/js/src/core/components/creatable-select/creatable-select.test.tsx:62
- These helpers use direct
nullcomparisons, contrary to the project’s required lodash type-check pattern. ImportisNulland use it for both query results (negating it for the positive checks).
assets/js/src/core/components/creatable-select/creatable-select.tsx:62 - The project’s type-safe-check guideline requires lodash nullish helpers instead of direct
undefinedcomparisons. Import and useisUndefined(numberInputProps.parser)here before invoking the optional parser.
assets/js/src/core/components/creatable-select/creatable-select.tsx:71 - This direct
undefinedcomparison also violates the project’s required lodash type-check pattern. ImportisUndefinedand useisUndefined(validate)for this optional callback check.
- Files reviewed: 2/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The contributor guidelines ask for isNull and isUndefined over direct comparisons. Applied to the four checks this branch introduced; the pre-existing ones in the same file are left alone to keep the diff on topic. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Numeric coercion can accept empty parser results and collapse distinct high-precision values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
assets/js/src/core/components/creatable-select/creatable-select.tsx:66
- Converting both sides to
NumberdefeatsInputNumber'sstringModeprecision guarantee: for example,9007199254740993and9007199254740992compare equal here. With a bound retaining the latter as the committed value, the former displayed text can therefore validate and submit the stale value. Compare canonical decimal representations with a precision-preserving mechanism instead.
if (committedText === '' || !Number.isFinite(parsedNumber) || parsedNumber !== Number(committedText)) {
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Balanced
|
xIrusux
approved these changes
Sep 3, 2026
xIrusux
left a comment
xIrusux
left a comment
Contributor
There was a problem hiding this comment.
LGTM :) just added a suggestion as we try to use - const { t } = useTranslation()
| ...selectProps | ||
| }: CreatableSelectProps): React.JSX.Element => { | ||
| const { t } = useTranslation() | ||
| const { t, i18n } = useTranslation() |
Contributor
There was a problem hiding this comment.
Suggested change
| const { t, i18n } = useTranslation() | |
| const { t } = useTranslation() |
| const [pendingSelection, setPendingSelection] = useState<SelectOptionType | null>(null) | ||
| const allOptions = [...options, ...customOptions] | ||
| const decimalSeparator = inputType === 'number' | ||
| ? numberInputProps.decimalSeparator ?? getDecimalSeparator(i18n?.language) |
Contributor
There was a problem hiding this comment.
Suggested change
| ? numberInputProps.decimalSeparator ?? getDecimalSeparator(i18n?.language) | |
| ? numberInputProps.decimalSeparator ?? getDecimalSeparator() |
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
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.
Changes in this pull request
Resolves #1954
InputNumberis a text input and only reports parseable numbers throughonChange, so the numberbranch never saw what was typed:
12aaakept the last valid12, deleting the digits re-validatednothing, and a below-
minentry was silently rewritten to the boundary value on blur or Enter.The raw text is now tracked via
onInput, and validity requires the parsed text to equal thecommitted value — so what the field shows is what gets validated and added.
Additional info
valueprop — feeding back a non-numeric string makesAntD's controlled-value effect wipe the display mid-typing.
onKeyDownflushes and re-aligns into[min, max], with noprop to disable it.
min/maxnow constrain created options and not just the steppers, without callers repeating thebounds in
validate. Aparserand anonInputpassed throughnumberInputPropsare bothhonoured — the latter had to be forwarded explicitly, since the component is SDK-exported.
Two other behaviour changes worth a look during review:
value, so typing
12awith option12present showed it alongside "Invalid option" whiledescribing a value no longer in the field. This also affects the
stringinput, where bothmessages could previously appear together.
No new translation keys, and no consumer changes — the fix is entirely in the shared component.
Verification
creatable-select.test.tsxadds 18 tests: the reported symptoms,minon the steppers and oncreated options, stale committed values, duplicate submission, message precedence, a custom
parser,onInputforwarding, both locale separator conventions, and the string input. Each wasconfirmed to fail against the component before its respective fix. Suite is 432/432;
eslintandtsc --noEmitclean. Also confirmed by hand in the Application Logger's refresh-interval selector.