Repository navigation
Validations for select component. - #2510
Conversation
🦋 Changeset detectedLatest commit: 0ae59fb The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces form validation logic for the ix-select component, including handling for the required attribute, form submission events, and visual error states. It also adds a suite of component tests covering various validation scenarios. The review identified several technical issues: potential memory leaks due to improper event listener cleanup and anonymous functions in connectedCallback, incorrect validity reporting to formInternals which could bypass browser validation, and the need to use standard HTMLFormElement.noValidate properties instead of manual attribute checks.
khathija-ahamadi
changed the title
|
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
🚥 Pre-merge checks | ✅ 5
Finishing Touches
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/select/select.tsx (1)
1-1: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
novalidateshould only suppress interactive validation, not clear validity.
packages/core/src/components/select/select.tsx#L576-L607:syncValidationClasses()should keepElementInternalsvalidity intact whenisFormNoValidate()is true; only the invalid-required class/visual feedback should be skipped.packages/core/src/components/select/test/select.ct.ts#L1692-L1719: update thenovalidatetest to keepform.checkValidity()false for an empty required select and assert only that the invalid UI is hidden.🤖 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/core/src/components/select/select.tsx` at line 1, Update syncValidationClasses() so isFormNoValidate() skips only invalid-required styling and visual feedback while preserving the ElementInternals validity state for an empty required select. Adjust the novalidate test to assert form.checkValidity() remains false and verify that invalid UI is hidden.
🤖 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 `@packages/core/src/components/select/select.tsx`:
- Around line 782-794: Remove the redundant syncValidationClasses() call from
the clear-value method containing emptyValue, since assigning this.value already
triggers the value watcher’s validation-class synchronization. Keep
emitValueChange, dropdown closing, and the onInputBlur synchronization
unchanged.
- Around line 402-407: Store the inline invalid-event callback used in
connectedCallback as a component-level handler reference, then remove that same
reference from hostElement in disconnectedCallback. Ensure reconnecting the
component does not accumulate invalid listeners while preserving the existing
formSubmissionAttempted, touched, and syncValidationClasses behavior.
- Around line 383-407: Update the submit callback assigned to formSubmitHandler
in connectedCallback to check isFormNoValidate() before preventing or stopping
submission for an empty required select. Preserve the existing validation state
updates, but allow submission when novalidate is present and only block required
empty submissions when interactive validation is enabled.
- Around line 576-607: Update syncValidationClasses so isFormNoValidate() only
removes the ix-invalid--required visual class and returns without calling
formInternals.setValidity({}); preserve the existing constraint validity for
required empty selects so checkValidity() and form.checkValidity() remain
invalid.
- Around line 314-321: Update the parentForm getter to return
this.formInternals.form instead of searching with hostElement.closest('form'),
so associated controls using form="..." resolve their owner correctly. Keep
isFormNoValidate() using parentForm and preserve its existing null/false
fallback behavior.
In `@packages/core/src/components/select/test/select.ct.ts`:
- Around line 1599-1779: Enhance the new required/invalid select tests around
the invalid state assertions to include axe-based accessibility coverage using
the established makeAxeBuilder() pattern. Run the accessibility check after
triggering invalid validation, including coverage for the invalid-text feedback
UI, while preserving the existing validation assertions.
- Around line 1753-1779: Remove the fixed page.waitForTimeout(100) call from the
“programmatic value setting updates validation state” test. Let the existing
expect(select).not.toHaveClass assertion poll until the validation class is
removed, preserving the test’s current behavior.
---
Outside diff comments:
In `@packages/core/src/components/select/select.tsx`:
- Line 1: Update syncValidationClasses() so isFormNoValidate() skips only
invalid-required styling and visual feedback while preserving the
ElementInternals validity state for an empty required select. Adjust the
novalidate test to assert form.checkValidity() remains false and verify that
invalid UI is hidden.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b4d155f-22af-4134-a7b2-86a39522c1c2
📒 Files selected for processing (2)
packages/core/src/components/select/select.tsxpackages/core/src/components/select/test/select.ct.ts
…handler blocks submission even when the form has novalidate.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/components/select/select.tsx (2)
389-408: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd a changeset or state that this change is internal-only.
This changes public
ix-selectvalidation and submission behavior. No changeset is included. Add a consumer-focused changeset, or explicitly justify why the change has no release impact.As per path instructions, “Changesets are required for public API updates, behavior changes, styling/theming changes, accessibility changes, and bug fixes with user impact.”
🤖 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/core/src/components/select/select.tsx` around lines 389 - 408, Add a consumer-focused changeset documenting the public ix-select validation and form-submission behavior change, including its user-facing impact and appropriate release bump. Do not treat this as internal-only, since the modified connectedCallback behavior affects consumers.Source: Path instructions
393-400: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
formnovalidateon the submitter.A submit button or input can disable constraint validation with
formnovalidate. This handler checks onlyform.noValidate, then prevents submission for an empty required select.Read
SubmitEvent.submitterand skip the custom prevention when itsformNoValidateproperty is true. Add a component test that submits an empty required select through aformnovalidatesubmitter.Proposed fix
this.formSubmitHandler = (event: Event) => { this.formSubmissionAttempted = true; this.touched = true; this.syncValidationClasses(); - if (!this.isFormNoValidate() && this.required && !this.hasValue()) { + const submitter = (event as SubmitEvent).submitter; + const submitterNoValidate = + (submitter instanceof HTMLButtonElement || + submitter instanceof HTMLInputElement) && + submitter.formNoValidate; + if ( + !this.isFormNoValidate() && + !submitterNoValidate && + this.required && + !this.hasValue() + ) { event.preventDefault(); event.stopPropagation(); return false;As per coding guidelines, update tests when user-facing behavior changes.
🤖 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/core/src/components/select/select.tsx` around lines 393 - 400, Update the formSubmitHandler in the select component to inspect SubmitEvent.submitter and bypass the required-value prevention when the submitter’s formNoValidate property is true, while preserving existing form-level noValidate behavior. Add a component test covering submission of an empty required select through a formnovalidate submit button or input.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@packages/core/src/components/select/select.tsx`:
- Around line 389-408: Add a consumer-focused changeset documenting the public
ix-select validation and form-submission behavior change, including its
user-facing impact and appropriate release bump. Do not treat this as
internal-only, since the modified connectedCallback behavior affects consumers.
- Around line 393-400: Update the formSubmitHandler in the select component to
inspect SubmitEvent.submitter and bypass the required-value prevention when the
submitter’s formNoValidate property is true, while preserving existing
form-level noValidate behavior. Add a component test covering submission of an
empty required select through a formnovalidate submit button or input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 158668fc-b435-4eb9-a216-9c612a46c669
📒 Files selected for processing (1)
packages/core/src/components/select/select.tsx
|
|
💡 What is the current behavior?
There are some errors which prevent validations from happening as expected.
GitHub Issue Number: #1799
JIRA: IX-2595
🆕 What is the new behavior?
The acceptance criteria for this ticket has been met for select component:
Required input:
Invalid input > Removing value with keyboard > Stays invalid
Invalid input > Remove touched state (e.g. clear button) > Valid again
Invalid input > Programmatically setting it to empty > Stays invalid
Valid input > Removing value with keyboard > It is invalid
Valid input > Remove touched state (e.g. autoclear button) > Valid
Valid input > Programmatically setting it to empty > It is invalid
Not required input:
Invalid input > Removing value with keyboard > Valid
Invalid input > Remove touched state (e.g. clear button) > Valid again
Invalid input > Programmatically setting it to empty > Valid
Valid input > Removing value with keyboard > Valid
Valid input > Remove touched state (e.g. clear button) > Valid
Valid input > Programmatically setting it to empty > Valid
Also form 'novalidate' cases is handled.
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
ix-selectin forms, preventing submission when required values are missing unless validation is disabled.invalid-text.