Controls: Add range validation in Number Control #32539
Conversation
WalkthroughThe Number control’s change handler now parses typed input, clamps numeric values to provided min/max before calling onChange, clears parse errors on valid inputs, and updates the displayed input if clamping occurred. The change handler’s dependency array was updated to include min and max. No public API signatures were changed. Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant NC as NumberControl
participant V as Parser/Validator
participant C as Clamper(min,max)
participant P as onChange Prop
participant UI as Input Display
U->>NC: type value
NC->>V: parse input
alt invalid number (NaN)
V-->>NC: NaN
NC->>UI: show parse error
else valid number
V-->>NC: number
NC->>C: apply bounds (if min/max provided)
C-->>NC: finalValue (original or clamped)
NC->>P: onChange(finalValue)
NC->>UI: update input display (reflect clamped value if different)
end
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (4)
code/addons/docs/src/blocks/controls/Number.tsx (4)
45-76: Deduplicate parsing and avoid double setState in handleChange.Reuse the existing parse() helper and set inputValue once per event to reduce re-renders and keep parsing semantics consistent.
Apply this diff:
- (event: ChangeEvent<HTMLInputElement>) => { - setInputValue(event.target.value); - - const result = parseFloat(event.target.value); - if (Number.isNaN(result)) { - setParseError(new Error(`'${event.target.value}' is not a number`)); - } else { - // Initialize the final value as the user's input - let finalValue = result; - - // Clamp to minimum: if finalValue is less than min, use min - if (typeof min === 'number' && finalValue < min) { - finalValue = min; - } - - // Clamp to maximum: if finalValue is greater than max, use max - if (typeof max === 'number' && finalValue > max) { - finalValue = max; - } - - // Pass the clamped final value to the onChange callback - onChange(finalValue); - // Clear any previous parse errors - setParseError(null); - - // If the value was clamped, update the input display to the final value - if (finalValue !== result) { - setInputValue(String(finalValue)); - } - } - }, - [onChange, setParseError, min, max] + (event: ChangeEvent<HTMLInputElement>) => { + const raw = event.target.value; + const parsed = parse(raw); + if (parsed === undefined) { + setInputValue(raw); + setParseError(new Error(`'${raw}' is not a number`)); + return; + } + let finalValue = parsed; + if (typeof min === 'number' && finalValue < min) finalValue = min; + if (typeof max === 'number' && finalValue > max) finalValue = max; + onChange(finalValue); + setParseError(null); + setInputValue(finalValue !== parsed ? String(finalValue) : raw); + }, + [onChange, setParseError, min, max]
56-63: Defensive: min > max edge case.If misconfigured (min > max), the sequential clamps will end up forcing to max. Consider normalizing bounds or swapping to avoid surprising results.
Example:
const lo = typeof min === 'number' ? min : -Infinity; const hi = typeof max === 'number' ? max : Infinity; const lower = Math.min(lo, hi); const upper = Math.max(lo, hi); let finalValue = Math.min(upper, Math.max(lower, parsed));
45-76: Optional: step alignment (follow‑up).Native spinners respect step, but typed values still accept off‑step numbers. If desired, snap to step after clamping.
Example:
if (typeof step === 'number' && step > 0 && Number.isFinite(step)) { const base = typeof min === 'number' ? min : 0; finalValue = base + Math.round((finalValue - base) / step) * step; }
45-76: Clamp the "Set number" default too (outside this hunk).Clicking “Set number” always sets 0, which can violate min/max. Mirror the same clamping there.
Suggested change outside the selected lines:
const onForceVisible = useCallback(() => { const initial = 0; let final = initial; if (typeof min === 'number' && final < min) final = min; if (typeof max === 'number' && final > max) final = max; setInputValue(String(final)); onChange(final); setForceVisible(true); }, [setForceVisible, onChange, min, max]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/addons/docs/src/blocks/controls/Number.tsx(1 hunks)
🔇 Additional comments (1)
code/addons/docs/src/blocks/controls/Number.tsx (1)
45-76: Clamping on change matches the issue’s ask; good fix. Please confirm "eager clamping" UX.This clamps immediately while typing and also updates the input to the clamped bound. That solves the bug. Confirm that this eager clamping (vs clamping on blur) is the intended UX for NumberControl.
ia319
changed the title
ia319
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/addons/docs/src/blocks/controls/Number.tsx (2)
48-51: Unify parsing and guard non‑finite; avoid double setState.
- Use the existing parse() helper for consistency and to centralize number parsing.
- Guard against non‑finite values (Infinity/-Infinity) to avoid passing them through.
- Compute final value first, then set inputValue once (original code sets it twice on clamp).
Apply this diff:
- setInputValue(event.target.value); - - const result = parseFloat(event.target.value); - if (Number.isNaN(result)) { - setParseError(new Error(`'${event.target.value}' is not a number`)); - } else { - // Initialize the final value as the user's input - let finalValue = result; + const raw = event.target.value; + const parsed = parse(raw); + if (parsed === undefined || !Number.isFinite(parsed)) { + setInputValue(raw); + setParseError(new Error(`'${raw}' is not a number`)); + return; + } + // Initialize the final value as the user's input + let finalValue = parsed; - // Clamp to minimum: if finalValue is less than min, use min + // Clamp to minimum: if finalValue is less than min, use min if (typeof min === 'number' && finalValue < min) { finalValue = min; } - // Clamp to maximum: if finalValue is greater than max, use max + // Clamp to maximum: if finalValue is greater than max, use max if (typeof max === 'number' && finalValue > max) { finalValue = max; } - // Pass the clamped final value to the onChange callback - onChange(finalValue); - // Clear any previous parse errors - setParseError(null); - - // If the value was clamped, update the input display to the final value - if (finalValue !== result) { - setInputValue(String(finalValue)); - } - } + // Pass the clamped final value to the onChange callback + onChange(finalValue); + // Clear any previous parse errors + setParseError(null); + // Update the input once: clamped value if changed, otherwise raw + setInputValue(finalValue !== parsed ? String(finalValue) : raw);Also applies to: 52-73
79-84: Clamp the default “Set number” value too.onForceVisible emits 0 even when min > 0, momentarily violating the range guarantee.
Apply this diff:
- const onForceVisible = useCallback(() => { - setInputValue('0'); - onChange(0); - setForceVisible(true); - }, [setForceVisible]); + const onForceVisible = useCallback(() => { + const initial = typeof min === 'number' ? Math.max(min, 0) : 0; + setInputValue(String(initial)); + onChange(initial); + setForceVisible(true); + }, [setForceVisible, min]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/addons/docs/src/blocks/controls/Number.tsx(1 hunks)code/frameworks/sveltekit/tsconfig.json(1 hunks)code/lib/eslint-plugin/docs/rules/no-stories-of.md(0 hunks)
💤 Files with no reviewable changes (1)
- code/lib/eslint-plugin/docs/rules/no-stories-of.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/addons/docs/src/blocks/controls/Number.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/addons/docs/src/blocks/controls/Number.tsx
code/**/tsconfig*.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep TypeScript strict mode enabled; do not relax strict compiler options in tsconfig files
Files:
code/frameworks/sveltekit/tsconfig.json
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to test-storybooks/** : Maintain test configurations and assets under test-storybooks/ for Storybook testing
Applied to files:
code/frameworks/sveltekit/tsconfig.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/frameworks/sveltekit/tsconfig.json (1)
6-6: LGTM — JSON punctuation fix only.No compiler options changed; strictness inherits from the extended config. As per coding guidelines.
code/addons/docs/src/blocks/controls/Number.tsx (1)
52-67: Good fix — clamping typed values addresses #32454.Clamping before invoking onChange prevents out‑of‑range values from propagating; adding min/max to deps is correct.
Please add a few tests to lock behavior:
- below min, above max → onChange receives clamped values
- exact boundaries (min, max) → unchanged
- empty/invalid input (''/'-'/'.') → no onChange, shows error
- exponent forms ('1e3', '1e-3')
- regression: arrow keys still bounded
Would you like me to draft these?Also applies to: 76-76
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 73 KB | 73 KB | 0 B |
| Dependency size | 31.91 MB | 31.88 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 126 KB | 126 KB | 0 B |
| Dependency size | 30.04 MB | 30.02 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 191 | 191 | 0 |
| Self size | 17 KB | 17 KB | 0 B |
| Dependency size | 28.62 MB | 28.60 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 533 | 533 | 0 |
| Self size | 749 KB | 749 KB | 0 B |
| Dependency size | 58.95 MB | 58.93 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 3.83 MB | 3.83 MB | 0 B |
| Dependency size | 21.89 MB | 21.87 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 157 | 157 | 0 |
| Self size | 31 KB | 31 KB | 0 B |
| Dependency size | 23.14 MB | 23.11 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 114 | 114 | 0 |
| Self size | 37 KB | 37 KB | 0 B |
| Dependency size | 19.69 MB | 19.67 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 273 | 273 | 0 |
| Self size | 25 KB | 25 KB | 0 B |
| Dependency size | 43.91 MB | 43.89 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 199 | 199 | 0 |
| Self size | 17 KB | 17 KB | 0 B |
| Dependency size | 33.16 MB | 33.14 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 928 KB | 928 KB | 0 B |
| Dependency size | 72.98 MB | 72.96 MB | 🎉 -18 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 69.41 MB | 69.39 MB | 🎉 -18 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 170 | 170 | 0 |
| Self size | 21 KB | 21 KB | 0 B |
| Dependency size | 31.22 MB | 31.20 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 57 | 57 | 0 |
| Self size | 833 KB | 833 KB | 0 B |
| Dependency size | 12.94 MB | 12.92 MB | 🎉 -23 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
Sidnioulz
changed the title
|
Hi,is there any problem with this PR? |
|
View your CI Pipeline Execution ↗ for commit 0676eef
☁️ Nx Cloud last updated this comment at |
yannbf
left a comment
yannbf
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution! Sorry it take time, there's a lot going on in Storybook so it's hard to keep track :)
yannbf
changed the title
yannbf
added
the
patch:yes
Thanks! Happy to help. Totally understand about the busy Storybook pace. |
Controls: Add range validation in Number Control (cherry picked from commit 0484989)
github-actions
bot
added
the
patch:done
3 participants
Closes #32454
What I did
NumberControl uses the native to render the number field, and the native element does not enforce min and max for keyboard input, so I added validation logic in the code to check the input value
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Keyboard input beyond the min or max value
Arrow key control
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Fixes #32454
Summary by CodeRabbit
Bug Fixes
New Features
Chores / Documentation