fix(plans): live range/digit validation on plan-creation number fields - #714
Conversation
Add wireRangeInput helper to plans.ts and call it for all five numeric inputs on the plan create/edit modal: plan-coverage (0-100), ramp-step-percent (1-100), ramp-interval-days (1-365), plan-notify-days (1-30). Validation fires on `input` and `blur`, shows a sibling .field-error span, sets aria-invalid, and rejects scientific notation via regex ^\d+$. Also adds max="365" to ramp-interval-days in HTML, which was previously unbounded. Closes #702
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements live validation for five plan-creation numeric inputs to reject out-of-range, non-integer, and scientific-notation values inline. The max attribute constraint is added to the ramp-interval-days HTML input, and JavaScript event handlers are wired on modal open to validate against bounds and display accessible error messages. ChangesPlan numeric field range validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@frontend/src/__tests__/plans-range-validation.test.ts`:
- Line 62: The mock's showToast callback currently has an untyped parameter
(opts) causing TS7006; annotate opts to a concrete type to satisfy
noImplicitAny—either import/use the proper toast option type from your toast
utilities or derive it from the existing mock using a helper like
Parameters<typeof mockShowToast>[0]; update the showToast entry so it reads
showToast: (opts: <derived-or-imported-type>) => mockShowToast(opts) referencing
mockShowToast to locate the mock.
In `@frontend/src/plans.ts`:
- Around line 947-955: The validation marks out-of-range or non-integer values
using integerPattern, raw, min, max, input and error but doesn't clamp them;
update the logic in the same block so that when parseInt(raw, 10) < min or > max
you clamp the numeric value to the nearest bound and write the clamped string
back into input.value (use a single parsed value variable to avoid
double-parsing), and still set aria-invalid and error text when the original raw
was invalid; ensure integerPattern continues to guard non-integer input before
clamping.
- Around line 918-920: The idempotency guard using input.dataset['rangeWired']
causes early return and leaves any existing .field-error UI unchanged; change
the logic so that if input.dataset['rangeWired'] is set you do not re-bind event
listeners but you DO run the validation/cleanup path (clear or re-run the
validation that toggles .field-error for this input) before returning, or
alternatively remove the early return and only skip addEventListener calls while
still invoking the function that syncs error state; reference
input.dataset['rangeWired'] and ensure the code that toggles .field-error for
the input is executed on each modal open/rebind.
🪄 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: 1904262b-aebb-445a-8d8f-eb36612e3ad6
📒 Files selected for processing (4)
frontend/src/__tests__/html.test.tsfrontend/src/__tests__/plans-range-validation.test.tsfrontend/src/index.htmlfrontend/src/plans.ts
Finding 1 (test line 62, TS7006): annotate the showToast mock callback parameter as ToastOptions (imported from '../toast') instead of implicit any. Both the jest.fn implementation and the factory callback are now explicitly typed, satisfying noImplicitAny without using a widening unknown that would lose the shape. Finding 2 (plans.ts ~918): the idempotency guard exited early without re-validating the current field value, so stale .field-error / aria-invalid UI could persist across modal reopens. Fix: dispatch a synthetic 'input' event before returning so the existing check listener reconciles the error state on every reopen. Finding 3 (plans.ts ~955): out-of-range integers were flagged but never clamped, leaving invalid raw values in the field. Fix: add a clampOnBlur listener that silently clamps integer values to [min, max] on blur and clears the error; non-integer values (scientific notation, fractions) are skipped by the guard so the error stays visible. The inline check function now separates the integer-pattern branch from the range branch to avoid double-parsing and allow a clean early return. New tests: blur-clamps-to-max, blur-clamps-to-min, non-integer not clamped on blur, stale error reconciled on modal reopen (19 tests total). Refs #702
|
Resolved all 3 actionable CR findings in commit 22a4502. Finding 1 - TS7006 implicit any in showToast mock ( Finding 2 - Stale error UI on modal reopen ( Finding 3 - Out-of-range values flagged but never clamped ( Total tests: 19 (up from 15). TypeScript clean ( @coderabbitai review |
|
Triggering a review of the updated commit. ✅ Actions performedReview triggered.
|
…776) Add wireRangeInput('add-purchases-count', 1, 52) in openAddPurchasesModal so the Number of Purchases field shows a "Must be a whole number between 1 and 52" error inline as the user types, instead of only at save time. The submit button is also disabled while the field is aria-invalid. Harden the save-time guard in handleAddPurchases to use Number() + Number.isInteger() instead of parseInt, so a fractional value like 2.5 is rejected before reaching the API (feedback_strict_int_parse). Export openAddPurchasesModal for test access and add 13 new cases in plans-range-validation.test.ts covering below-range (0), above-range (53), boundary values (1/26/52), fractions (2.5), button-disabled toggling, a11y attributes, and the save-time guard. Mirrors the wireRangeInput pattern from #702/#714 across the codebase.
1 participant
Closes #702. Five QA rows (3.11/3.16/3.17/3.19/3.24) where plan-creation number inputs accepted out-of-range or scientific-notation values until Save, then surfaced a backend error.
Changes
frontend/src/index.html:830— addedmax="365"to#ramp-interval-days(previously hadmin="1"with no upper bound, so the up-arrow ran unbounded — covers row 3.17).frontend/src/plans.ts— added two helpers:wireRangeInput(inputId, min, max):input+blurlisteners; rejects non-integer via/^\d+$/(blocks1e+30, decimals, negatives — covers row 3.19); toggles a sibling.field-errorspan; sets/clearsaria-invalid. Uses adata-range-wiredattribute for idempotency so re-opening the modal does not stack duplicate listeners.wirePlanRangeInputs(): wires all four inputs —plan-coverage(0-100),ramp-step-percent(1-100),ramp-interval-days(1-365),plan-notify-days(1-30).wirePlanRangeInputs()fromopenCreatePlanModal,openNewPlanModal, andeditPlan.Tests
plans-range-validation.test.ts: out-of-range for each input, scientific-notation rejection, valid values clearing errors, blur triggers validation, idempotency, aria attributes.html.test.tsto assertramp-interval-dayshasmax="365".Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests