Repository navigation
chore(core): allow non-null assertion for specific scenarios only - #2612
Merged
Merged
Conversation
✅ 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ESLint restrictions for non-null and definite-assignment assertions. Updates core components, utilities, input handling, and tests with optional types, explicit null checks, throwing guards, and safer lookup paths. ChangesNon-null Assertion Elimination
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces new ESLint rules to restrict non-null (!) and definite assignment assertions, and updates numerous components and test files to use safer optional chaining, explicit null/undefined checks, or proper error handling instead. Feedback on these changes includes addressing a potential inconsistent state in dropdown.tsx by moving an active class assignment after a dropdown existence check, and removing a redundant duplicate call to updateOptionalFormInternalValue in number-input.tsx.
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.
![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
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/event-list/event-list.tsx (1)
119-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn after resolving when animations are disabled.
At Line 120, the promise resolves but execution continues, so fade-out logic can still run when
animatedisfalse.Suggested fix
if (!this.animated) { resolve(); + return; }🤖 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/event-list/event-list.tsx` around lines 119 - 121, In the conditional block that checks `!this.animated` and calls `resolve()`, add a return statement immediately after the `resolve()` call to prevent execution from continuing to the fade-out logic. This ensures that when animations are disabled, the promise resolves and the function exits without executing any subsequent animation code.packages/core/src/components/card-accordion/card-accordion.tsx (1)
83-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid re-querying with a non-null assertion after the guard.
Line 83 guards existence, but Line 89 performs a fresh
querySelector(... )!that can still throw if the node is removed beforescrollIntoView. Reuse one local reference for both calls.Suggested fix
private scrollExpandedContentIntoView() { setTimeout(() => { - if (!this.expandedContent) { + const expandedContent = this.expandedContent; + if (!expandedContent) { return; } - const rect = this.expandedContent.getBoundingClientRect(); + const rect = expandedContent.getBoundingClientRect(); if (rect.bottom > window.innerHeight) { - this.hostElement - .shadowRoot!.querySelector('.expand-content')! - .scrollIntoView(false); + expandedContent.scrollIntoView(false); } }, 150); }🤖 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/card-accordion/card-accordion.tsx` around lines 83 - 90, The code guards against this.expandedContent being null on line 83 but then performs a fresh DOM query with querySelector on line 89 to select the expand-content element, which can fail if the node is removed between the guard check and the scrollIntoView call. Instead of re-querying the DOM, directly reuse the this.expandedContent reference that was already validated in the guard condition by calling scrollIntoView directly on that reference, eliminating both the redundant query and the potential race condition.
🤖 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/dropdown/dropdown.tsx`:
- Around line 922-929: The classList.add operation that adds the
`ix-dropdown-submenu-trigger-active` class to the event.detail.activeElement is
being executed before validating that the submenu exists via the
dropdownController.getDropdownById call. Move the classList.add call to after
the null/undefined check for the dropdown variable, so the active class is only
applied to the trigger element when the submenu lookup succeeds and the dropdown
is confirmed to exist.
In `@packages/core/src/components/input/input.util.ts`:
- Around line 243-246: The `ixChange` event has a type mismatch between its
declaration and actual emission behavior. The `ixChange` EventEmitter is
declared as `EventEmitter<number>` in number-input.tsx, but the
`onInputBlurWithChange` method in input.util.ts emits `undefined` when
currentValue is undefined. Update the type declaration of the `ixChange` event
in number-input.tsx from `EventEmitter<number>` to `EventEmitter<number |
undefined>` to match the actual emission behavior where the event can emit
either a number value or undefined.
In `@packages/core/src/components/input/number-input.tsx`:
- Around line 307-313: Remove the duplicate call to
updateOptionalFormInternalValue(parsedValue) in the blur event handler. The
method is being called twice consecutively on lines 307 and 312, with the second
invocation being redundant. Delete the second occurrence of
updateOptionalFormInternalValue(parsedValue) that appears before the
onInputBlurWithChange call to eliminate unnecessary form value updates and
validity checks.
---
Outside diff comments:
In `@packages/core/src/components/card-accordion/card-accordion.tsx`:
- Around line 83-90: The code guards against this.expandedContent being null on
line 83 but then performs a fresh DOM query with querySelector on line 89 to
select the expand-content element, which can fail if the node is removed between
the guard check and the scrollIntoView call. Instead of re-querying the DOM,
directly reuse the this.expandedContent reference that was already validated in
the guard condition by calling scrollIntoView directly on that reference,
eliminating both the redundant query and the potential race condition.
In `@packages/core/src/components/event-list/event-list.tsx`:
- Around line 119-121: In the conditional block that checks `!this.animated` and
calls `resolve()`, add a return statement immediately after the `resolve()` call
to prevent execution from continuing to the fade-out logic. This ensures that
when animations are disabled, the promise resolves and the function exits
without executing any subsequent animation code.
🪄 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: 4a593e55-3898-45c0-afc2-55fef4f683f3
📒 Files selected for processing (38)
packages/core/eslint.config.cjspackages/core/src/components/application-header/test/application-header.ct.tspackages/core/src/components/blind/blind.tsxpackages/core/src/components/card-accordion/card-accordion.tsxpackages/core/src/components/date-picker/date-picker.tsxpackages/core/src/components/dropdown/dropdown-focus.tspackages/core/src/components/dropdown/dropdown.tsxpackages/core/src/components/dropdown/test/dropdown-top-layer.ct.tspackages/core/src/components/dropdown/test/dropdown.ct.tspackages/core/src/components/event-list/event-list.tsxpackages/core/src/components/field-wrapper/tests/field-wrapper.ct.tspackages/core/src/components/group/group.tsxpackages/core/src/components/icon-button/icon-button.tsxpackages/core/src/components/input/input.util.tspackages/core/src/components/input/number-input.tsxpackages/core/src/components/menu-category/menu-category.tsxpackages/core/src/components/menu/menu.tsxpackages/core/src/components/modal-header/modal-header.tsxpackages/core/src/components/modal/modal.tsxpackages/core/src/components/select/select.tsxpackages/core/src/components/select/test/select-keyboard.ct.tspackages/core/src/components/split-button/test/split-button.ct.tspackages/core/src/components/tab-set/test/tab-set.ct.tspackages/core/src/components/tabs/test/tabs.ct.tspackages/core/src/components/time-picker/test/time-picker-compute-time.spec.tspackages/core/src/components/time-picker/test/time-picker-constraints.spec.tspackages/core/src/components/time-picker/test/time-picker-range.spec.tspackages/core/src/components/time-picker/test/time-picker.ct.tspackages/core/src/components/time-picker/time-picker.tsxpackages/core/src/components/tooltip/test/tooltip.ct.tspackages/core/src/components/tooltip/tooltip.tsxpackages/core/src/components/tree/test/tree.ct.tspackages/core/src/components/tree/tree.tsxpackages/core/src/components/utils/animation.tspackages/core/src/components/utils/internal/mixins/accessibility/inherit-aria-attributes.mixin.tspackages/core/src/components/utils/lazy-list/lazy.tspackages/core/src/components/utils/shadow-dom.tspackages/core/src/tests/utils/test/page.ts
nuke-ellington
requested changes
Jun 24, 2026
|
|
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: 4
🤖 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/application-header/test/application-header.ct.ts`:
- Around line 55-57: Update both `#mount` lookup sites in the application-header
test to validate that document.querySelector('`#mount`') returns an element before
appending header. Remove optional chaining and throw an explicit error when the
required mount element is missing, preserving the append behavior when it
exists.
In `@packages/core/src/components/dropdown/test/dropdown-top-layer.ct.ts`:
- Around line 195-199: Add a makeAxeBuilder() accessibility scan and a
hydration/render assertion to the dropdown component tests in the existing test
suite. Reuse the established component setup and selectors, ensuring the
accessibility check runs against the rendered dropdown and the render test
verifies successful hydration without changing the current positioning
assertions.
In `@packages/core/src/components/utils/lazy-list/lazy.ts`:
- Line 1: Remove the file-wide no-restricted-syntax disable in lazy.ts and limit
the ESLint exception to the specific assertion that requires it by adding an
eslint-disable-next-line comment immediately before that assertion; otherwise
narrow or remove the assertion so no exception is needed.
- Line 1: Add the repository-standard SPDX license header at the very beginning
of lazy.ts, before the existing no-restricted-syntax ESLint directive, without
changing the directive or other file content.
🪄 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: 8c57380c-700c-4799-be4d-94500dcc0404
📒 Files selected for processing (42)
packages/core/eslint.config.cjspackages/core/src/components/application-header/test/application-header.ct.tspackages/core/src/components/badge/test/badge.page.tspackages/core/src/components/blind/blind.tsxpackages/core/src/components/card-accordion/card-accordion.tsxpackages/core/src/components/chat-input/chat-input.tsxpackages/core/src/components/chip/test/chip.ct.tspackages/core/src/components/date-picker/date-picker.tsxpackages/core/src/components/dropdown/dropdown-focus.tspackages/core/src/components/dropdown/dropdown.tsxpackages/core/src/components/dropdown/test/dropdown-top-layer.ct.tspackages/core/src/components/dropdown/test/dropdown.ct.tspackages/core/src/components/event-list/event-list.tsxpackages/core/src/components/field-wrapper/tests/field-wrapper.ct.tspackages/core/src/components/group/group.tsxpackages/core/src/components/icon-button/icon-button.tsxpackages/core/src/components/input/input.util.tspackages/core/src/components/input/number-input.tsxpackages/core/src/components/menu-category/menu-category.tsxpackages/core/src/components/menu/menu.tsxpackages/core/src/components/modal-header/modal-header.tsxpackages/core/src/components/modal/modal.tsxpackages/core/src/components/popover/test/popover.page.tspackages/core/src/components/select/select.tsxpackages/core/src/components/select/test/select-keyboard.ct.tspackages/core/src/components/split-button/test/split-button.ct.tspackages/core/src/components/tab-set/test/tab-set.ct.tspackages/core/src/components/tabs/test/tabs.ct.tspackages/core/src/components/time-picker/test/time-picker-compute-time.spec.tspackages/core/src/components/time-picker/test/time-picker-constraints.spec.tspackages/core/src/components/time-picker/test/time-picker-range.spec.tspackages/core/src/components/time-picker/test/time-picker.ct.tspackages/core/src/components/time-picker/time-picker.tsxpackages/core/src/components/tooltip/test/tooltip.ct.tspackages/core/src/components/tooltip/tooltip.tsxpackages/core/src/components/tree/test/tree.ct.tspackages/core/src/components/tree/tree.tsxpackages/core/src/components/utils/animation.tspackages/core/src/components/utils/internal/mixins/accessibility/inherit-aria-attributes.mixin.tspackages/core/src/components/utils/lazy-list/lazy.tspackages/core/src/components/utils/shadow-dom.tspackages/core/src/tests/utils/test/page.ts
lzeiml
requested changes
Aug 11, 2026
9 tasks
9 tasks
nuke-ellington
previously approved these changes
Aug 17, 2026
|
nuke-ellington
enabled auto-merge (squash)
August 17, 2026 13:26
lzeiml
approved these changes
Aug 17, 2026
This was referenced Aug 19, 2026
2 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🆕 What is the new behavior?
Enhance eslint config to
🏁 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
Release Notes
Bug Fixes
Tests
Chores