feat(core/date-input): dime input validation improvements#2543
feat(core/date-input): dime input validation improvements#2543RamVinayMandal wants to merge 47 commits into
Conversation
… state Co-authored-by: Copilot <copilot@github.com>
🦋 Changeset detectedLatest commit: 1c36671 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 a clear() method to the ix-date-input component and refines its validation logic to better handle novalidate forms and programmatic value resets. The changes include a new utility for clearing inputs and extensive regression tests. Review feedback suggests that date parsing should still be performed even when visual validation is suppressed to avoid invalid internal states. Furthermore, the reviewer recommends explicitly synchronizing validation classes during programmatic clears and ensuring that asynchronous validation calls are correctly awaited in property watchers.
… validation Co-authored-by: Copilot <copilot@github.com>
…e-is-empty-for-date
Co-authored-by: Copilot <copilot@github.com>
…x dates Co-authored-by: Copilot <copilot@github.com>
…e-is-empty-for-date
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a clear() method to the ix-date-input component and refactors its validation logic to correctly handle novalidate forms and programmatic value resets. The changes include adding a syncValidationClasses method, updating the value watcher, and providing comprehensive regression tests. Feedback focuses on ensuring that asynchronous validation methods are properly awaited, stale validation classes are removed when validation is suppressed (e.g., in novalidate forms), and that accessibility coverage is maintained with an axe-based component test as per the repository style guide.
…hen-value-is-empty-for-date
- Introduced a new `reportValidity` method to explicitly trigger validation and surface errors immediately, regardless of user interaction. - Updated validation logic to ensure required fields show appropriate error messages when empty. - Improved internal state management for invalid inputs, ensuring visual feedback aligns with user interactions. - Refactored validation classes and error messaging to provide clearer user feedback, including handling of required fields and parse errors. - Added tests to cover new validation scenarios, including behavior in `novalidate` forms and programmatic value changes.
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…sistency - Moved and restructured tests related to initial invalid values and user interactions. - Consolidated required and optional field behavior tests for better readability. - Enhanced test descriptions for improved understanding of scenarios. - Ensured consistent handling of visual validation states across tests.
…e-is-empty-for-date # Conflicts: # packages/core/src/components/date-input/date-input.tsx # packages/react/src/components.server.ts # packages/vue/src/components.ts
…add accessibility tests
…e-is-empty-for-date
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant validation improvements to the ix-date-input component, including new clear() and reportValidity() methods, a customizable i18nErrorRequired property, and proper validation suppression within novalidate forms. The feedback highlights a potential runtime crash in Luxon when minDate or maxDate is undefined, suggesting safe guards in getDateValidation. Additionally, the reviewer notes that when enableTopLayer is active, focus transitions to the dropdown are incorrectly treated as external, and recommends passing the dropdown element reference to handlePickerInputBlur and handlePickerHostFocusout to prevent premature validation and closure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ve validation handling
…d keyboard navigation tests
…e-is-empty-for-date
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/date-input/date-input.tsx (2)
307-308:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
validityTrackerwhenclear()returns the control to pristine.
emitPickerValidityStateChangeIfChanged()memoizes the last emittedpatternMismatch/valueMissingpair.clear()removes touched/visual state, but it leaves that cache intact, so re-entering the same invalid state after a clear is filtered as “unchanged” andvalidityStateChangeno longer fires.Suggested fix
async clear(): Promise<void> { this._hasInvalidInput = false; this._reportValidityCalled = false; + this.validityTracker = createPickerValidityStateTracker(); await clearInputValue(this, { defaultValue: '', additionalCleanup: () => { this.from = undefined; },Also applies to: 390-399
🤖 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/date-input/date-input.tsx` around lines 307 - 308, The cached validity state in validityTracker (created by createPickerValidityStateTracker()) must be reset when clear() returns the control to pristine so emitPickerValidityStateChangeIfChanged() won't suppress future identical invalid states; update the clear() implementation(s) to reinitialize the tracker (e.g., assign a new createPickerValidityStateTracker() or call a reset method on validityTracker) after you remove touched/visual state, and apply the same change to the other clear-like code path referenced in the file (the other clear/restore block that uses emitPickerValidityStateChangeIfChanged/validityTracker).
817-823:⚠️ Potential issue | 🔴 CriticalBind
handlePickerFocusoutCallbackbefore passing it intohandlePickerFocusoutWithValidation
handlePickerFocusoutWithValidation()calls the provided callback as a plain function (onValidateAndBlur(...)). Passingthis.handlePickerFocusoutCallbackdirectly drops the component instance, sothisinsidehandlePickerFocusoutCallbackcan be undefined while it usesthis.closeDropdown(),this.touched, etc.Suggested fix
onFocusout={(e: FocusEvent) => handlePickerFocusoutWithValidation( e, this.hostElement, - this.handlePickerFocusoutCallback, + (hasRelatedTarget) => + this.handlePickerFocusoutCallback(hasRelatedTarget), this.dropdownElementRef?.current ) }🤖 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/date-input/date-input.tsx` around lines 817 - 823, The passed callback this.handlePickerFocusoutCallback loses its instance context when invoked inside handlePickerFocusoutWithValidation; update the onFocusout call to pass a bound function so that inside handlePickerFocusoutCallback methods like this.closeDropdown() and properties like this.touched work correctly — e.g., bind the method to the component instance (this.handlePickerFocusoutCallback.bind(this)) or wrap it in an arrow that calls this.handlePickerFocusoutCallback(...); keep using handlePickerFocusoutWithValidation, hostElement and dropdownElementRef as the other arguments.
🤖 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/date-input/date-input.tsx`:
- Around line 307-308: The cached validity state in validityTracker (created by
createPickerValidityStateTracker()) must be reset when clear() returns the
control to pristine so emitPickerValidityStateChangeIfChanged() won't suppress
future identical invalid states; update the clear() implementation(s) to
reinitialize the tracker (e.g., assign a new createPickerValidityStateTracker()
or call a reset method on validityTracker) after you remove touched/visual
state, and apply the same change to the other clear-like code path referenced in
the file (the other clear/restore block that uses
emitPickerValidityStateChangeIfChanged/validityTracker).
- Around line 817-823: The passed callback this.handlePickerFocusoutCallback
loses its instance context when invoked inside
handlePickerFocusoutWithValidation; update the onFocusout call to pass a bound
function so that inside handlePickerFocusoutCallback methods like
this.closeDropdown() and properties like this.touched work correctly — e.g.,
bind the method to the component instance
(this.handlePickerFocusoutCallback.bind(this)) or wrap it in an arrow that calls
this.handlePickerFocusoutCallback(...); keep using
handlePickerFocusoutWithValidation, hostElement and dropdownElementRef as the
other arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: edfd2e0d-57b8-4ec1-ba39-2e5d47826a5c
📒 Files selected for processing (3)
packages/core/src/components/date-input/date-input.tsxpackages/core/src/components/input/input.util.tspackages/core/src/components/utils/input/picker-input.util.ts
…e-is-empty-for-date
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 (3)
packages/core/src/components/date-input/date-input.tsx (3)
142-146:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmit
validityStateChangewhenrequiredchanges.Line 145 resynchronizes CSS and
ElementInternals, butonRequiredChange()never emitsvalidityStateChange. Togglingrequiredon an empty field changes the component’s public validity state without notifying listeners of the change.Suggested fix
`@Watch`('required') async onRequiredChange() { await syncRequiredValidationClass(this.hostElement, this); this.syncFormInternalsValidity(); + emitPickerValidityState(this); }🤖 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/date-input/date-input.tsx` around lines 142 - 146, The onRequiredChange() watcher updates CSS and ElementInternals but doesn't notify listeners; after calling syncRequiredValidationClass(this.hostElement, this) and this.syncFormInternalsValidity() inside onRequiredChange(), dispatch or emit the component's validityStateChange event (or call the existing emitter/dispatcher for validityStateChange) so consumers are notified when the required attribute toggles; update the onRequiredChange() implementation in the DateInput component to emit validityStateChange after syncFormInternalsValidity() using the same event name/signature used elsewhere in the component.
727-733:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
reportValidity()still leaves pristine required-empty fields without public error state.These branches still derive required-empty state from
touchedonly. AfterreportValidity()on a pristine required field, the component can show the required CSS path via_reportValidityCalled, butgetValidityState()still reports valid andgetInvalidText()still returns nothing. That breaks the new contract described in the PR: required-missing feedback should surface whenreportValidity()is called.Suggested fix
+private shouldShowRequiredState(): boolean { + return !!this.required && (this.touched || this._reportValidityCalled); +} + /** `@internal` */ `@Method`() getValidityState(): Promise<ValidityState> { return Promise.resolve( createValidityState( this.isInputInvalid, - !!this.required && this.touched, + this.shouldShowRequiredState(), this.value ) ); } ... private getInvalidText(): string | undefined { - const isRequiredEmpty = !!this.required && !this.value && this.touched; + const isRequiredEmpty = this.shouldShowRequiredState() && !this.value;Also applies to: 788-802
🤖 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/date-input/date-input.tsx` around lines 727 - 733, getValidityState currently derives the required-empty condition from this.touched only, so calling reportValidity() doesn't flip public validity; update the required-missing logic in getValidityState (the Promise.resolve call around createValidityState) to treat required as missing when this.required && (this.touched || this._reportValidityCalled) rather than using this.touched alone, and make the same change in the corresponding getInvalidText branch (the block around lines 788-802) so both methods honor _reportValidityCalled when reportValidity() has been invoked.
344-355:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid firing
valueChangeduring initial state sync.Line 345 now routes
componentWillLoad()throughonInput(), but both downstream paths (handleEmptyInput()andhandleValidatedInput()) emitvalueChange. That publishes a change event during first render even when the consumer only supplied the initial prop value, which violates the event contract at Line 260 and can trigger phantom updates in two-way bindings.Suggested direction
override async componentWillLoad(): Promise<void> { - await this.onInput(this.value); + await this.onInput(this.value, { emitValueChange: false }); if (this._hasInvalidInput) { this.from = null; } else { this.watchValue(); }-async onInput(value: string | undefined) { +async onInput( + value: string | undefined, + options: { emitValueChange?: boolean } = {} +) { this.value = value; if (!value) { - await this.handleEmptyInput(value); + await this.handleEmptyInput(value, options); return; } ... - await this.handleValidatedInput(value); + await this.handleValidatedInput(value, options); }🤖 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/date-input/date-input.tsx` around lines 344 - 355, componentWillLoad currently calls onInput(this.value) which propagates through handleEmptyInput/handleValidatedInput and emits the valueChange event during initial render; change componentWillLoad to perform initial parsing/validation without emitting valueChange (e.g., call internal parsing helpers or add a flag to onInput like suppressEvent=true) so that handleEmptyInput and handleValidatedInput skip emitting valueChange during initial sync; update componentWillLoad to then call watchValue(), checkClassList(), updateFormInternalValue, and syncFormInternalsValidity as before, ensuring valueChange only fires on user-driven updates; reference componentWillLoad, onInput, handleEmptyInput, handleValidatedInput, and valueChange when making the change.
🤖 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/date-input/date-input.tsx`:
- Around line 142-146: The onRequiredChange() watcher updates CSS and
ElementInternals but doesn't notify listeners; after calling
syncRequiredValidationClass(this.hostElement, this) and
this.syncFormInternalsValidity() inside onRequiredChange(), dispatch or emit the
component's validityStateChange event (or call the existing emitter/dispatcher
for validityStateChange) so consumers are notified when the required attribute
toggles; update the onRequiredChange() implementation in the DateInput component
to emit validityStateChange after syncFormInternalsValidity() using the same
event name/signature used elsewhere in the component.
- Around line 727-733: getValidityState currently derives the required-empty
condition from this.touched only, so calling reportValidity() doesn't flip
public validity; update the required-missing logic in getValidityState (the
Promise.resolve call around createValidityState) to treat required as missing
when this.required && (this.touched || this._reportValidityCalled) rather than
using this.touched alone, and make the same change in the corresponding
getInvalidText branch (the block around lines 788-802) so both methods honor
_reportValidityCalled when reportValidity() has been invoked.
- Around line 344-355: componentWillLoad currently calls onInput(this.value)
which propagates through handleEmptyInput/handleValidatedInput and emits the
valueChange event during initial render; change componentWillLoad to perform
initial parsing/validation without emitting valueChange (e.g., call internal
parsing helpers or add a flag to onInput like suppressEvent=true) so that
handleEmptyInput and handleValidatedInput skip emitting valueChange during
initial sync; update componentWillLoad to then call watchValue(),
checkClassList(), updateFormInternalValue, and syncFormInternalsValidity as
before, ensuring valueChange only fires on user-driven updates; reference
componentWillLoad, onInput, handleEmptyInput, handleValidatedInput, and
valueChange when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b327045-511b-482a-a72a-09bd94137fec
📒 Files selected for processing (1)
packages/core/src/components/date-input/date-input.tsx
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 (1)
packages/core/src/components/date-input/date-input.tsx (1)
607-626: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract duplicated blur/focusout validation logic into a shared method.
The validation logic in
handlePickerFocusoutCallback(lines 619-625) and theonBlurcallback (lines 672-682) is nearly identical:this.touched = true; const suppress = await shouldSuppressInternalValidation(this); if (!suppress) { this.isInputInvalid = this._hasInvalidInput; } onInputBlurWithChange(this, this.inputElementRef.current, this.value); emitPickerValidityState(this);Consider extracting this to a private method to reduce duplication and simplify future maintenance.
♻️ Proposed refactor
+ private async runBlurValidation(): Promise<void> { + this.touched = true; + const suppress = await shouldSuppressInternalValidation(this); + if (!suppress) { + this.isInputInvalid = this._hasInvalidInput; + } + onInputBlurWithChange(this, this.inputElementRef.current, this.value); + emitPickerValidityState(this); + } + private readonly handlePickerFocusoutCallback = async ( hasRelatedTarget: boolean ) => { if (hasRelatedTarget) { this.closeDropdown(); } if (this._shouldSkipFocusoutValidation) { this._shouldSkipFocusoutValidation = false; return; } - this.touched = true; - const suppress = await shouldSuppressInternalValidation(this); - if (!suppress) { - this.isInputInvalid = this._hasInvalidInput; - } - onInputBlurWithChange(this, this.inputElementRef.current, this.value); - emitPickerValidityState(this); + await this.runBlurValidation(); };Then in the
onBlurcallback:onBlur={(e: FocusEvent) => suppressInputBlurWhenFocusMovedToPicker({ event: e, isDropdownOpen: this.show, hostElement: this.hostElement, onBlur: async () => { - this.touched = true; - const suppress = await shouldSuppressInternalValidation(this); - if (!suppress) { - this.isInputInvalid = this._hasInvalidInput; - } - onInputBlurWithChange( - this, - this.inputElementRef.current, - this.value - ); - emitPickerValidityState(this); + await this.runBlurValidation(); this._shouldSkipFocusoutValidation = true; }, pickerElement: this.dropdownElementRef?.current, }) }Also applies to: 666-687
🤖 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/date-input/date-input.tsx` around lines 607 - 626, The validation logic in the handlePickerFocusoutCallback method (lines 619-625) is duplicated in the onBlur callback (around lines 672-682). Extract this common validation sequence (which includes setting touched to true, calling shouldSuppressInternalValidation, conditionally setting isInputInvalid, and calling onInputBlurWithChange and emitPickerValidityState) into a new private async method. Then replace both instances of the duplicated code with calls to this new shared method to reduce duplication and improve maintainability.
🤖 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/date-input/date-input.tsx`:
- Around line 607-626: The validation logic in the handlePickerFocusoutCallback
method (lines 619-625) is duplicated in the onBlur callback (around lines
672-682). Extract this common validation sequence (which includes setting
touched to true, calling shouldSuppressInternalValidation, conditionally setting
isInputInvalid, and calling onInputBlurWithChange and emitPickerValidityState)
into a new private async method. Then replace both instances of the duplicated
code with calls to this new shared method to reduce duplication and improve
maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc1a05d0-3f83-463e-87c9-9e5b86380da2
📒 Files selected for processing (1)
packages/core/src/components/date-input/date-input.tsx
…ns and improve focusout handling
…on to prevent error flash on required empty fields
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/feat-date-input-validation.md (1)
1-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd issue linkage in the changeset summary (
Fixes #<issue-number>).The summary is missing the required issue-closing reference for release automation.
Suggested fix
Added `i18nErrorRequired` prop (`i18n-error-required`) to customize the required-field error message. + +Fixes #<issue-number>As per coding guidelines, "Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released."🤖 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 @.changeset/feat-date-input-validation.md around lines 1 - 20, The changeset file is missing the required issue-closing reference syntax. Add a "Fixes #<issue-number>" line to the changeset summary section (after the package and version type declaration and before or within the description) to enable automatic GitHub issue closure when the changeset is released. Replace <issue-number> with the actual GitHub issue number that this changeset addresses.Source: Coding guidelines
🤖 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/date-input/date-input.tsx`:
- Around line 795-804: The onFocusout event handler is closing the dropdown
whenever focus moves to any element outside the input field, including internal
component elements like the picker. Instead of only checking if relatedTarget is
not null, add an additional check to verify that the relatedTarget is outside
the component itself by using contains() to check if this.el (the component's
root element) contains the relatedTarget. Only call this.closeDropdown() when
focus actually moves outside the component boundaries, not just when moving
between internal child elements like the input and picker.
In `@packages/core/src/components/date-input/tests/date-input.ct.ts`:
- Around line 397-404: After the mount(...) call that creates the ix-date-input
component, add a hydration assertion before any interactions occur. Insert an
await expect statement that checks if the dateInput locator has the hydrated
class using the regex pattern /\bhydrated\b/. This assertion should be placed
immediately after the dateInput variable assignment and before the click() call
to open-calendar, ensuring the component is fully hydrated before attempting to
interact with it and avoiding potential timing flakes.
---
Outside diff comments:
In @.changeset/feat-date-input-validation.md:
- Around line 1-20: The changeset file is missing the required issue-closing
reference syntax. Add a "Fixes #<issue-number>" line to the changeset summary
section (after the package and version type declaration and before or within the
description) to enable automatic GitHub issue closure when the changeset is
released. Replace <issue-number> with the actual GitHub issue number that this
changeset addresses.
🪄 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: 6635ad04-c22a-4c03-bb89-472d07f83ed1
📒 Files selected for processing (4)
.changeset/feat-date-input-validation.mdpackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/utils/input/picker-input.util.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/input/picker-input.util.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/feat-date-input-validation.md (1)
1-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd issue linkage in the changeset summary (
Fixes #<issue-number>).The summary is missing the required issue-closing reference for release automation.
Suggested fix
Added `i18nErrorRequired` prop (`i18n-error-required`) to customize the required-field error message. + +Fixes #<issue-number>As per coding guidelines, "Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released."🤖 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 @.changeset/feat-date-input-validation.md around lines 1 - 20, The changeset file is missing the required issue-closing reference syntax. Add a "Fixes #<issue-number>" line to the changeset summary section (after the package and version type declaration and before or within the description) to enable automatic GitHub issue closure when the changeset is released. Replace <issue-number> with the actual GitHub issue number that this changeset addresses.Source: Coding guidelines
🤖 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/date-input/date-input.tsx`:
- Around line 795-804: The onFocusout event handler is closing the dropdown
whenever focus moves to any element outside the input field, including internal
component elements like the picker. Instead of only checking if relatedTarget is
not null, add an additional check to verify that the relatedTarget is outside
the component itself by using contains() to check if this.el (the component's
root element) contains the relatedTarget. Only call this.closeDropdown() when
focus actually moves outside the component boundaries, not just when moving
between internal child elements like the input and picker.
In `@packages/core/src/components/date-input/tests/date-input.ct.ts`:
- Around line 397-404: After the mount(...) call that creates the ix-date-input
component, add a hydration assertion before any interactions occur. Insert an
await expect statement that checks if the dateInput locator has the hydrated
class using the regex pattern /\bhydrated\b/. This assertion should be placed
immediately after the dateInput variable assignment and before the click() call
to open-calendar, ensuring the component is fully hydrated before attempting to
interact with it and avoiding potential timing flakes.
---
Outside diff comments:
In @.changeset/feat-date-input-validation.md:
- Around line 1-20: The changeset file is missing the required issue-closing
reference syntax. Add a "Fixes #<issue-number>" line to the changeset summary
section (after the package and version type declaration and before or within the
description) to enable automatic GitHub issue closure when the changeset is
released. Replace <issue-number> with the actual GitHub issue number that this
changeset addresses.
🪄 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: 6635ad04-c22a-4c03-bb89-472d07f83ed1
📒 Files selected for processing (4)
.changeset/feat-date-input-validation.mdpackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/utils/input/picker-input.util.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/input/picker-input.util.ts
🛑 Comments failed to post (2)
packages/core/src/components/date-input/date-input.tsx (1)
795-804:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDropdown is closed on internal focus moves, which can break picker interactions.
Line 803 closes the dropdown for any non-null
relatedTarget. Onfocusout, that includes focus transitions within the component (input → picker/dropdown), so the picker can close mid-interaction.Suggested fix
- onFocusout={(e: FocusEvent) => { - const relatedTarget = e.relatedTarget as Node; - - // Related target might be null during rerenders, which would cause the dropdown to close unexpectedly - if (!relatedTarget) { - return; - } - - this.closeDropdown(); - }} + onFocusout={(e: FocusEvent) => { + const relatedTarget = e.relatedTarget as Node | null; + + // Related target might be null during rerenders + if (!relatedTarget) { + return; + } + + // Keep dropdown open while focus stays within this component/picker + if (this.hostElement.contains(relatedTarget)) { + return; + } + + this.closeDropdown(); + }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.onFocusout={(e: FocusEvent) => { const relatedTarget = e.relatedTarget as Node | null; // Related target might be null during rerenders if (!relatedTarget) { return; } // Keep dropdown open while focus stays within this component/picker if (this.hostElement.contains(relatedTarget)) { return; } this.closeDropdown(); }}🤖 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/date-input/date-input.tsx` around lines 795 - 804, The onFocusout event handler is closing the dropdown whenever focus moves to any element outside the input field, including internal component elements like the picker. Instead of only checking if relatedTarget is not null, add an additional check to verify that the relatedTarget is outside the component itself by using contains() to check if this.el (the component's root element) contains the relatedTarget. Only call this.closeDropdown() when focus actually moves outside the component boundaries, not just when moving between internal child elements like the input and picker.packages/core/src/components/date-input/tests/date-input.ct.ts (1)
397-404:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd hydration assertion before interacting with the mounted component.
This test starts interactions immediately after
mount(...). Add the required hydration assertion first to avoid timing flakes.Suggested fix
await mount(`<ix-date-input required value=""></ix-date-input>`); const dateInput = page.locator('ix-date-input'); + await expect(dateInput).toHaveClass(/\bhydrated\b/); const input = dateInput.getByRole('textbox'); await dateInput.getByTestId('open-calendar').click();As per coding guidelines, "Always verify components are hydrated before making further assertions using
await expect(page.locator('ix-component')).toHaveClass(/\bhydrated\b/)."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await mount(`<ix-date-input required value=""></ix-date-input>`); const dateInput = page.locator('ix-date-input'); await expect(dateInput).toHaveClass(/\bhydrated\b/); const input = dateInput.getByRole('textbox'); await dateInput.getByTestId('open-calendar').click(); await expect(dateInput.getByTestId('date-dropdown')).toHaveClass(/show/);🤖 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/date-input/tests/date-input.ct.ts` around lines 397 - 404, After the mount(...) call that creates the ix-date-input component, add a hydration assertion before any interactions occur. Insert an await expect statement that checks if the dateInput locator has the hydrated class using the regex pattern /\bhydrated\b/. This assertion should be placed immediately after the dateInput variable assignment and before the click() call to open-calendar, ensuring the component is fully hydrated before attempting to interact with it and avoiding potential timing flakes.Source: Coding guidelines
… or invalid - test cases added
…d streamline form submission tests
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/feat-date-input-validation.md (1)
5-21: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
Fixes #<issue-number>to this changeset summary.The summary is missing the required issue-closing reference line. Please include the linked issue in
Fixes#1234`` format.As per coding guidelines, “Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released.”🤖 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 @.changeset/feat-date-input-validation.md around lines 5 - 21, The changeset summary in feat-date-input-validation.md is missing the required issue-closing reference line. Add a new line to the changeset summary that includes the `Fixes #<issue-number>` syntax (replacing issue-number with the actual GitHub issue number this changeset addresses) to ensure the linked issue is automatically closed when the changeset is released. This line should be included somewhere in the changeset content, typically at the beginning or end of the summary section.Source: Coding guidelines
🤖 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/date-input/date-input.tsx`:
- Around line 796-805: The onFocusout handler is closing the dropdown whenever
relatedTarget is non-null, but this also triggers when focus moves to internal
elements within the date-input/picker component itself. Modify the condition to
check whether the relatedTarget is actually outside the component before calling
closeDropdown(). You should verify that the relatedTarget is not a descendant of
the date-input component (or the component itself) before closing the dropdown,
allowing internal focus transitions to proceed without interrupting
keyboard/calendar interactions.
In `@packages/core/src/components/date-input/tests/date-input.ct.ts`:
- Around line 411-417: Add hydration verification before performing interactions
in the test blocks. After mounting the ix-date-input component, insert an
assertion to verify that the component has the hydrated class using the pattern
`await expect(page.locator('ix-date-input')).toHaveClass(/\bhydrated\b/)` before
calling any interaction methods like click() or type(). Apply this hydration
assertion to all the new test blocks mentioned in the additional ranges to
prevent racey test failures.
---
Outside diff comments:
In @.changeset/feat-date-input-validation.md:
- Around line 5-21: The changeset summary in feat-date-input-validation.md is
missing the required issue-closing reference line. Add a new line to the
changeset summary that includes the `Fixes #<issue-number>` syntax (replacing
issue-number with the actual GitHub issue number this changeset addresses) to
ensure the linked issue is automatically closed when the changeset is released.
This line should be included somewhere in the changeset content, typically at
the beginning or end of the summary section.
🪄 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: a9579f5a-614a-4f9c-8939-558f93e8a8bb
📒 Files selected for processing (5)
.changeset/feat-date-input-validation.mdpackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/input/tests/form-ready.ct.tspackages/core/src/components/utils/input/picker-input.util.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/input/picker-input.util.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/feat-date-input-validation.md (1)
5-21: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
Fixes #<issue-number>to this changeset summary.The summary is missing the required issue-closing reference line. Please include the linked issue in
Fixes#1234`` format.As per coding guidelines, “Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released.”🤖 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 @.changeset/feat-date-input-validation.md around lines 5 - 21, The changeset summary in feat-date-input-validation.md is missing the required issue-closing reference line. Add a new line to the changeset summary that includes the `Fixes #<issue-number>` syntax (replacing issue-number with the actual GitHub issue number this changeset addresses) to ensure the linked issue is automatically closed when the changeset is released. This line should be included somewhere in the changeset content, typically at the beginning or end of the summary section.Source: Coding guidelines
🤖 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/date-input/date-input.tsx`:
- Around line 796-805: The onFocusout handler is closing the dropdown whenever
relatedTarget is non-null, but this also triggers when focus moves to internal
elements within the date-input/picker component itself. Modify the condition to
check whether the relatedTarget is actually outside the component before calling
closeDropdown(). You should verify that the relatedTarget is not a descendant of
the date-input component (or the component itself) before closing the dropdown,
allowing internal focus transitions to proceed without interrupting
keyboard/calendar interactions.
In `@packages/core/src/components/date-input/tests/date-input.ct.ts`:
- Around line 411-417: Add hydration verification before performing interactions
in the test blocks. After mounting the ix-date-input component, insert an
assertion to verify that the component has the hydrated class using the pattern
`await expect(page.locator('ix-date-input')).toHaveClass(/\bhydrated\b/)` before
calling any interaction methods like click() or type(). Apply this hydration
assertion to all the new test blocks mentioned in the additional ranges to
prevent racey test failures.
---
Outside diff comments:
In @.changeset/feat-date-input-validation.md:
- Around line 5-21: The changeset summary in feat-date-input-validation.md is
missing the required issue-closing reference line. Add a new line to the
changeset summary that includes the `Fixes #<issue-number>` syntax (replacing
issue-number with the actual GitHub issue number this changeset addresses) to
ensure the linked issue is automatically closed when the changeset is released.
This line should be included somewhere in the changeset content, typically at
the beginning or end of the summary section.
🪄 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: a9579f5a-614a-4f9c-8939-558f93e8a8bb
📒 Files selected for processing (5)
.changeset/feat-date-input-validation.mdpackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/input/tests/form-ready.ct.tspackages/core/src/components/utils/input/picker-input.util.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/input/picker-input.util.ts
🛑 Comments failed to post (2)
packages/core/src/components/date-input/date-input.tsx (1)
796-805: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid closing the dropdown on internal focus moves.
Line 804 closes the dropdown for any non-null
relatedTarget. That also catches focus transitions inside the date-input/picker, which can interrupt keyboard/calendar interaction.Suggested fix
onFocusout={(e: FocusEvent) => { - const relatedTarget = e.relatedTarget as Node; + const relatedTarget = e.relatedTarget as Node | null; // Related target might be null during rerenders, which would cause the dropdown to close unexpectedly if (!relatedTarget) { return; } + const movedInsideHost = this.hostElement.contains(relatedTarget); + const movedInsidePicker = + this.dropdownElementRef.current?.contains(relatedTarget) ?? false; + + if (movedInsideHost || movedInsidePicker) { + return; + } + this.closeDropdown(); }}🤖 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/date-input/date-input.tsx` around lines 796 - 805, The onFocusout handler is closing the dropdown whenever relatedTarget is non-null, but this also triggers when focus moves to internal elements within the date-input/picker component itself. Modify the condition to check whether the relatedTarget is actually outside the component before calling closeDropdown(). You should verify that the relatedTarget is not a descendant of the date-input component (or the component itself) before closing the dropdown, allowing internal focus transitions to proceed without interrupting keyboard/calendar interactions.packages/core/src/components/date-input/tests/date-input.ct.ts (1)
411-417: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add hydration assertions before interactions in the new tests.
These new tests click/type immediately after mount. Please assert hydration first to avoid racey failures.
Suggested pattern (apply to each new test block)
const dateInput = page.locator('ix-date-input'); + await expect(dateInput).toHaveClass(/\bhydrated\b/); await page.locator('button[type="submit"]').click();As per coding guidelines, “Always verify components are hydrated before making further assertions using
await expect(page.locator('ix-component')).toHaveClass(/\bhydrated\b/)”.Also applies to: 1027-1038, 1051-1063, 1076-1088
🤖 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/date-input/tests/date-input.ct.ts` around lines 411 - 417, Add hydration verification before performing interactions in the test blocks. After mounting the ix-date-input component, insert an assertion to verify that the component has the hydrated class using the pattern `await expect(page.locator('ix-date-input')).toHaveClass(/\bhydrated\b/)` before calling any interaction methods like click() or type(). Apply this hydration assertion to all the new test blocks mentioned in the additional ranges to prevent racey test failures.Source: Coding guidelines
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 (1)
packages/core/src/components/date-input/date-input.tsx (1)
206-212: 📐 Maintainability & Code Quality | 🟠 MajorAdd Storybook stories for the new validation methods and error message customization.
The PR introduces user-visible APIs (
i18nErrorRequired,clear(),reportValidity()) with proper@since 5.1.0JSDoc tags and extensive component test coverage. However, the Storybook story atpackages/storybook-docs/src/stories/input-date.stories.tsonly demonstrates a basicrequiredcase and does not showcase these new validation features. Per path instructions, add story examples demonstrating the new methods and error message customization before merge.🤖 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/date-input/date-input.tsx` around lines 206 - 212, Add new Storybook story examples to the `input-date.stories.ts` file that demonstrate the newly introduced validation features: the `i18nErrorRequired` property for customizing the error message, the `clear()` method for clearing the field, and the `reportValidity()` method for triggering validation. These stories should showcase realistic usage patterns and clearly illustrate how users can customize error messages and programmatically interact with validation on the date input component.Source: 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/date-input/date-input.tsx`:
- Around line 206-212: Add new Storybook story examples to the
`input-date.stories.ts` file that demonstrate the newly introduced validation
features: the `i18nErrorRequired` property for customizing the error message,
the `clear()` method for clearing the field, and the `reportValidity()` method
for triggering validation. These stories should showcase realistic usage
patterns and clearly illustrate how users can customize error messages and
programmatically interact with validation on the date input component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 739906df-09c2-4cc8-b3b4-449e86110054
📒 Files selected for processing (1)
packages/core/src/components/date-input/date-input.tsx
RamVinayMandal
changed the title
… Updated since tag
…for-date' of github.com:RamVinayMandal/ix into fix-ix-3322-display-input-as-valid-when-value-is-empty-for-date
…e-is-empty-for-date
|
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 (1)
packages/core/src/components/date-input/date-input.tsx (1)
535-545: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the form-associated value in sync for invalid edits.
This branch updates
this.valueand validity state, but it skipsupdateFormInternalValue(...). After a touched field becomes invalid,ElementInternals.setFormValue()can still hold the previous valid date, soFormData(form)/manual form serialization can read stale data instead of the current control state.Suggested fix
if (this._hasInvalidInput) { this.invalidReason = validation.invalidReason; this.from = undefined; + this.updateFormInternalValue(value); } else { this.invalidReason = undefined; await syncRequiredValidationClass(this.hostElement, this); this.updateFormInternalValue(value);🤖 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/date-input/date-input.tsx` around lines 535 - 545, The invalid-edit branch in date-input is leaving the form-associated value stale because it updates validity state without refreshing the internal form value. Update the invalid path in date-input.tsx so the same form sync logic used in the valid path also runs after setting invalidReason and clearing from; specifically, make sure updateFormInternalValue(...) is called from the branch around this._hasInvalidInput so ElementInternals.setFormValue() stays aligned with the current control state.
🤖 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/date-input/date-input.tsx`:
- Around line 535-545: The invalid-edit branch in date-input is leaving the
form-associated value stale because it updates validity state without refreshing
the internal form value. Update the invalid path in date-input.tsx so the same
form sync logic used in the valid path also runs after setting invalidReason and
clearing from; specifically, make sure updateFormInternalValue(...) is called
from the branch around this._hasInvalidInput so ElementInternals.setFormValue()
stays aligned with the current control state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d4b72a6-3dba-42d1-8d67-ee9407cf4f76
⛔ Files ignored due to path filters (2)
packages/angular/standalone/src/components.tsis excluded by!packages/angular/standalone/src/components.tspackages/react/src/components/components.server.tsis excluded by!packages/react/src/components/**
📒 Files selected for processing (5)
packages/angular/src/components.tspackages/core/src/components.d.tspackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/input/input.util.ts
2 participants
💡 What is the current behavior?
ix-date-inputshows as invalid when cleared, even though empty should be validix-date-inputdoesn't consistently show as invalid when the value is removednovalidateforms still display visual validation feedback despite opting out of validationrequiredattributeJIRA: IX-2595 IX-3322
🆕 What is the new behavior?
Validation Fixes
reportValidity()novalidateforms: Suppress all visual validation feedback;reportValidity()overrides this suppressionrequiredattribute: Validation state updates immediately when togglingNew APIs
clear()method to clear value and validation state (removes touched flag and error classes)reportValidity()method to trigger validation and show errors immediatelyi18nErrorRequiredprop to customize the required-field error messageValidation Behavior
novalidatesuppression is overridden byreportValidity()to show errorsAdditional Improvements
Note: The
clear(),reportValidity()methods andi18nErrorRequiredprop on ix-date-input is tagged with@since 5.2.0. Please verify this matches the actual target release version and we need to update the JSDoc tag if needed before merging.🏁 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)Summary by CodeRabbit
Summary by CodeRabbit
Summary
New Features
clear()andreportValidity()toix-date-input, plusi18nErrorRequiredto customize required-field messaging.Bug Fixes
novalidateand form submission behavior to block/allow submit based on validation.Tests