Skip to content

Clamp NumberInput numeric values#1978

Open
Tomeshwari-02 wants to merge 1 commit into
Karanjot786:mainfrom
Tomeshwari-02:fix-numberinput-clamp
Open

Clamp NumberInput numeric values#1978
Tomeshwari-02 wants to merge 1 commit into
Karanjot786:mainfrom
Tomeshwari-02:fix-numberinput-clamp

Conversation

@Tomeshwari-02

@Tomeshwari-02 Tomeshwari-02 commented Jul 3, 2026

Copy link
Copy Markdown

Summary

  • Clamp NumberInput numericValue through the configured min/max range
  • Cover typed input so callbacks and submit receive bounded values

Fixes #1968

Testing

  • Not run: Bun is not installed in this environment

Summary by CodeRabbit

  • Bug Fixes
    • Number inputs now consistently clamp typed values to the configured minimum and maximum.
    • Change and submit actions now return the corrected in-range value instead of an out-of-range number.
    • Improved keyboard interaction handling so input behavior remains stable during key presses.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Fixes NumberInput so typed values are clamped to the configured min/max range via the numericValue getter, matching existing arrow-key clamping behavior. Test file updated with explicit event callback stubs and a new test validating clamped numericValue, onChange, and onSubmit behavior.

Changes

Typed Value Clamping Fix

Layer / File(s) Summary
Clamp numericValue getter
packages/ui/src/NumberInput.ts
numericValue now returns this._clamp(n) instead of the raw parsed number, ensuring typed values respect _min/_max bounds.
Tests for clamped typed input
packages/ui/src/NumberInput.test.ts
Imports vi, adds explicit stopPropagation/preventDefault callbacks to existing keypress tests, and adds a new test asserting typed 99 with max: 10 clamps numericValue, submit, onChange, and onSubmit to 10.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested labels: type:bug

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is missing most required template sections, including Description, Which package(s)?, Type of Change, and Checklist. Fill in the required template sections: Description, Related Issue/Closes #, Which package(s)?, Type of Change, Checklist, and reviewer notes if needed.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main change: clamping NumberInput numeric values.
Linked Issues check ✅ Passed The code clamps parsed numericValue and the tests cover typed input submitting the bounded value, matching issue #1968.
Out of Scope Changes check ✅ Passed The changes are limited to NumberInput logic and its tests, with no unrelated refactors or extra scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the needs-star PR author has not starred the repo. label Jul 3, 2026
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Hi @Tomeshwari-02 👋

Star this repo before your PR merges.

Why? GSSoC 2026 contributors who star get priority review and points credit. After you star, push any commit (or re-run this check). The needs-star label lifts automatically.

Thanks for your contribution to TermUI.

@github-actions github-actions Bot added area:ui @termuijs/ui type:testing +10 pts. Tests. and removed needs-star PR author has not starred the repo. labels Jul 3, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Thanks for your first PR to TermUI, @Tomeshwari-02.

Before your PR merges:

  1. Star the repo. Required. The star-check job blocks your merge otherwise.
  2. ✅ All checks green: build, test, typecheck.
  3. 🏷 PR title follows type: short description. Example: fix: handle empty list.
  4. 🔗 Link your closing issue in the description.

GSSoC 2026 points come from labels after merge:

  • gssoc:approved. +50 base points.
  • level:beginner / intermediate / advanced / critical. +20 / +35 / +55 / +80.
  • quality:clean / exceptional. x 1.2 / x 1.5.
  • type:*. Stackable bonus.

Your reviewer responds within 48 hours. Ping @Karanjot786 on Discord for urgent help.

🚀 Welcome to the cohort.

@Tomeshwari-02 Tomeshwari-02 marked this pull request as ready for review July 3, 2026 14:40

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/ui/src/NumberInput.test.ts (1)

70-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing test for typed value clamped below min.

Only the above-max typed-clamp case is covered. The PR objectives explicitly call for coverage of values typed below min clamping to min, in addition to above-max. Consider adding a symmetric test.

✅ Suggested additional test
     it('clamps typed values exposed through numericValue and submit', () => {
         const onChange = vi.fn();
         const onSubmit = vi.fn();
         const input = new NumberInput({}, { min: 0, max: 10, onChange, onSubmit });
 
         input.insertChar('9');
         input.insertChar('9');
         input.submit();
 
         expect(input.rawValue).toBe('99');
         expect(input.numericValue).toBe(10);
         expect(onChange).toHaveBeenLastCalledWith(10);
         expect(onSubmit).toHaveBeenCalledWith(10);
     });
+
+    it('clamps typed values below min exposed through numericValue and submit', () => {
+        const onChange = vi.fn();
+        const onSubmit = vi.fn();
+        const input = new NumberInput({}, { min: 5, max: 10, onChange, onSubmit });
+
+        input.insertChar('2');
+        input.submit();
+
+        expect(input.rawValue).toBe('2');
+        expect(input.numericValue).toBe(5);
+        expect(onChange).toHaveBeenLastCalledWith(5);
+        expect(onSubmit).toHaveBeenCalledWith(5);
+    });
🤖 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 `@packages/ui/src/NumberInput.test.ts` around lines 70 - 84, Add a symmetric
NumberInput test covering typed values clamped below min: in
NumberInput.test.ts, create a case using NumberInput with min/max plus
onChange/onSubmit, type a value below min via insertChar, call submit, and
assert rawValue stays typed while numericValue, onChange, and onSubmit all
reflect the clamped min. Reuse the existing clamping pattern around
NumberInput.numericValue and NumberInput.submit so the suite covers both
below-min and above-max typed input.
🤖 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.

Nitpick comments:
In `@packages/ui/src/NumberInput.test.ts`:
- Around line 70-84: Add a symmetric NumberInput test covering typed values
clamped below min: in NumberInput.test.ts, create a case using NumberInput with
min/max plus onChange/onSubmit, type a value below min via insertChar, call
submit, and assert rawValue stays typed while numericValue, onChange, and
onSubmit all reflect the clamped min. Reuse the existing clamping pattern around
NumberInput.numericValue and NumberInput.submit so the suite covers both
below-min and above-max typed input.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 70faff52-59d3-4c4d-8a66-cde7ec69871b

📥 Commits

Reviewing files that changed from the base of the PR and between 5f479e6 and 7ef1b45.

📒 Files selected for processing (2)
  • packages/ui/src/NumberInput.test.ts
  • packages/ui/src/NumberInput.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ui @termuijs/ui type:testing +10 pts. Tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] NumberInput typed values bypass min/max clamping

1 participant