fix(core): avoid unhandled validation rejection when native input is missing EIX-154 - #2690
fix(core): avoid unhandled validation rejection when native input is missing EIX-154#2690alexkaduk wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 8ce699a 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 |
📝 WalkthroughWalkthroughThe validation lifecycle now identifies missing native inputs, cancels deferred checks, checks host connectivity during asynchronous operations, and prevents unhandled rejections during select unmount. ChangesValidation lifecycle handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 `@packages/core/src/components/select/test/select.ct.ts`:
- Around line 1632-1657: Update the teardown validation test around the mocked
getNativeInputElement and valueChange dispatch so it triggers the actual
validator event/path used by select.tsx. Replace fixed 50 ms waits with a
deferred lookup promise and explicit synchronization for lookup completion and
teardown; if a valueChange handler exists, assert the mock lookup was invoked
before validating inputNotFoundErrors remains empty.
In `@packages/core/src/components/utils/input/validation.ts`:
- Around line 47-49: Update isValidationHostActive and the active-host guards at
the indicated validation-check points to accept the captured state and require
validationLifecycleState.get(host) === state, in addition to host.isConnected.
Pass each check’s own state object through every guard so resumed checks cannot
mutate the host after a reconnect creates a new lifecycle entry.
- Around line 281-286: The observer setup around componentWillLoad must also run
after reconnection, since disconnectedCallback clears
state.classMutationObserver. Move or reuse the existing class-observer creation
logic in the connectedCallback path of HookValidationLifecycle, guard it so
setup is idempotent, and add a regression test covering disconnect, reattach,
and subsequent class mutations.
🪄 Autofix
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 Plus
Run ID: 20ea8449-2d48-4d9e-92f5-c92a05a8657c
📒 Files selected for processing (3)
.changeset/select-validation-unmount-rejection.mdpackages/core/src/components/select/test/select.ct.tspackages/core/src/components/utils/input/validation.ts
50b0791 to
81e7e24
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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)
packages/core/src/components/utils/input/validation.ts (1)
116-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStore the initial-validation timer per host.
HookValidationLifecyclecreatescheckTimeoutIdonce for the decorated prototype. It is shared by all component instances. When a second instance connects, Lines 125-128 cancel the first instance's initial validation. When either instance disconnects, Lines 264-267 can cancel the other instance's validation.This can leave required-state classes and ARIA attributes uninitialized. Store timeout state in a
WeakMapkeyed byhost, and clear only that host's timer.Also applies to: 235-240, 264-267
🤖 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/utils/input/validation.ts` around lines 116 - 128, Update HookValidationLifecycle so the initial-validation timeout is stored per host in a WeakMap keyed by the component element, rather than in the shared checkTimeoutId variable. In connectedCallback, the initial-validation scheduling logic, and disconnectedCallback, read, clear, reset, and assign only the timer associated with that host, preserving independent validation state across component instances.
🤖 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/test/select.ct.ts`:
- Around line 1600-1603: Replace the plain Playwright test declaration for “does
not emit unhandled rejection when select is unmounted during validation” with
regressionTest, ensuring regressionTest is imported from `@utils/test` and the
existing test body remains unchanged.
In `@packages/core/src/components/utils/input/validation.ts`:
- Around line 144-149: Update the validation flow around getNativeInputElement
and the complete validation catch so only the known unavailable-input condition
is treated as validationElement undefined. Preserve that expected
connect/teardown behavior, but route all other failures—including errors from
shouldSuppressInternalValidation, hasValidValue, isTouched, validity retrieval,
and ARIA helpers—through the project’s established error-reporting path instead
of silently discarding them.
---
Outside diff comments:
In `@packages/core/src/components/utils/input/validation.ts`:
- Around line 116-128: Update HookValidationLifecycle so the initial-validation
timeout is stored per host in a WeakMap keyed by the component element, rather
than in the shared checkTimeoutId variable. In connectedCallback, the
initial-validation scheduling logic, and disconnectedCallback, read, clear,
reset, and assign only the timer associated with that host, preserving
independent validation state across component instances.
🪄 Autofix
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 Plus
Run ID: 33a603c2-094c-4147-965e-20d1b9ff10aa
📒 Files selected for processing (3)
.changeset/select-validation-unmount-rejection.mdpackages/core/src/components/select/test/select.ct.tspackages/core/src/components/utils/input/validation.ts
|
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 (1)
1303-1307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public failure behavior.
getNativeInputElement()is exposed with@Method(). It now rejects when the native input is unavailable, but its JSDoc describes only successful resolution. Document this lifecycle failure behavior without makingNativeInputNotFoundErroran unintended public dependency.As per coding guidelines, “Update component JSDoc, metadata inputs, Storybook stories, or test-app examples when public usage 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 1303 - 1307, Update the JSDoc for the public getNativeInputElement() method to document that its Promise rejects when the native input is unavailable during the component lifecycle, describing the failure behavior without exposing NativeInputNotFoundError as a required public dependency.Source: Coding guidelines
packages/core/src/components/utils/input/validation.ts (1)
143-146: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftScope lifecycle state per host instance.
checkIfRequiredFunction,checkTimeoutId, andclassMutationObserverare captured by the decorator closure. All instances of the decorated component share these variables. A secondix-selectcan clear the first timer or replace its callback. Disconnecting one instance can remove listeners or destroy the observer for another instance.Store lifecycle state in a
WeakMapkeyed by the host, or on the instance. Add a two-instance connect/disconnect regression test.Also applies to: 290-298
🤖 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/utils/input/validation.ts` around lines 143 - 146, Move checkIfRequiredFunction, checkTimeoutId, and classMutationObserver out of the shared decorator closure and scope them per host instance using a WeakMap keyed by the host (or equivalent instance state). Update the lifecycle logic around the timeout cleanup and the related lines near 290-298 so each instance only manages its own callback, timer, observer, and listeners. Add a regression test that connects and disconnects two ix-select instances and verifies their lifecycle state remains independent.
🤖 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 1303-1307: Update the JSDoc for the public getNativeInputElement()
method to document that its Promise rejects when the native input is unavailable
during the component lifecycle, describing the failure behavior without exposing
NativeInputNotFoundError as a required public dependency.
In `@packages/core/src/components/utils/input/validation.ts`:
- Around line 143-146: Move checkIfRequiredFunction, checkTimeoutId, and
classMutationObserver out of the shared decorator closure and scope them per
host instance using a WeakMap keyed by the host (or equivalent instance state).
Update the lifecycle logic around the timeout cleanup and the related lines near
290-298 so each instance only manages its own callback, timer, observer, and
listeners. Add a regression test that connects and disconnects two ix-select
instances and verifies their lifecycle state remains independent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7a0aea8-6052-4e7d-b0a4-ed89de9b060c
📒 Files selected for processing (2)
packages/core/src/components/select/select.tsxpackages/core/src/components/utils/input/validation.ts
2 participants
💡 What is the current behavior?
HookValidationLifecycleschedules a fire-and-forget validation check withsetTimeoutinconnectedCallback. That timer is not cleared on disconnect, and rejections fromgetNativeInputElement()are not caught.For
ix-select,getNativeInputElement()rejects withError: Input element not foundwhen the internal<input>is missing (early connect or teardown). That becomes an unhandled promise rejection — console noise in production and failed runs in environments that treat unhandled rejections as test failures (e.g. Vitest browser / Storybook interaction tests).GitHub Issue Number: #2669
Jira: EIX-154
🆕 What is the new behavior?
Changes in
packages/core/src/components/utils/input/validation.ts:try/catcharoundgetNativeInputElement()— missing native input skips aria wiring instead of rejectingsetTimeoutid andclearTimeouton disconnect (and before scheduling again on reconnect).catch()on the scheduled validation promise!host.isConnectedafter awaits so teardown does not write classes/ARIA on a detached hostAlso:
Input element not found)🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test) Select CT added; core build + select unmount CT run locallypnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit