fix(core): memory leaks from connected callback states#129
Open
coryrylan wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughRefactors form and controller listener lifecycles to return cleanup handles, broadens control reset logic, adds DOM observer/property cleanup and EyeDropper logging guards, replaces several thrown errors with fallback behaviors, and adds tests for reset, reconnect, accessibility, SSR, and fallback paths. ChangesForm control state and reset handling
Controller event listener cleanup patterns
DOM utility cleanup support and environment safety
Graceful error handling and fallback behavior
PagePanel tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/core/src/internal/controllers/type-native-popover.controller.ts (1)
165-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard async legacy-trigger setup after RAF to prevent stale observers.
#setupLegacyTriggers()can create aMutationObserverafter disconnect (Line 166 await gap), which bypasseshostDisconnected()cleanup and can reintroduce duplicate observer behavior on reconnect.Suggested fix
async `#setupLegacyTriggers`() { await new Promise(r => requestAnimationFrame(r)); + if (!this.host.isConnected) { + return; + } // setup hidden updates for legacy triggers this.#observers.push( getAttributeListChanges(this.host, ['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 `@projects/core/src/internal/controllers/type-native-popover.controller.ts` around lines 165 - 180, The async gap in `#setupLegacyTriggers` can create and register a MutationObserver after the host has been disconnected, bypassing hostDisconnected cleanup and causing duplicate observers on reconnect; fix this by checking the host connectivity immediately after the await (e.g., if (!this.host.isConnected) return) before calling getAttributeListChanges and pushing into this.#observers, ensuring you only create/register the observer while the host is still connected; reference `#setupLegacyTriggers`, getAttributeListChanges, this.host.isConnected, and this.#observers to locate where to add the guard.
🤖 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 `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 13-35: The test currently calls removeFixture(fixture) only on the
success path, which can leak DOM if elementIsStable or runAxe throws; wrap the
async test body so that after creating fixture with createFixture(...) you
always call removeFixture(fixture) in a finally block (or ensure teardown via
afterEach) so the fixture is removed regardless of failures, adjusting
references to createFixture, elementIsStable, runAxe, and removeFixture in the
test named 'should pass axe check'.
---
Outside diff comments:
In `@projects/core/src/internal/controllers/type-native-popover.controller.ts`:
- Around line 165-180: The async gap in `#setupLegacyTriggers` can create and
register a MutationObserver after the host has been disconnected, bypassing
hostDisconnected cleanup and causing duplicate observers on reconnect; fix this
by checking the host connectivity immediately after the await (e.g., if
(!this.host.isConnected) return) before calling getAttributeListChanges and
pushing into this.#observers, ensuring you only create/register the observer
while the host is still connected; reference `#setupLegacyTriggers`,
getAttributeListChanges, this.host.isConnected, and this.#observers to locate
where to add the guard.
🪄 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: Enterprise
Run ID: 5192ca9b-8cbf-4a28-b047-c6a42d58991e
📒 Files selected for processing (26)
projects/core/src/checkbox/checkbox.test.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.ts
Comment on lines
+13
to
+35
| it('should pass axe check', async () => { | ||
| const fixture = await createFixture(html` | ||
| <nve-page> | ||
| <nve-page-panel id="panel"> | ||
| <nve-page-panel-header>panel heading</nve-page-panel-header> | ||
| <nve-icon-button | ||
| slot="actions" | ||
| commandfor="panel" | ||
| command="--close" | ||
| icon-name="cancel" | ||
| aria-label="close panel"> | ||
| </nve-icon-button> | ||
| <nve-page-panel-content>panel content</nve-page-panel-content> | ||
| <nve-page-panel-footer>panel footer</nve-page-panel-footer> | ||
| </nve-page-panel> | ||
| </nve-page> | ||
| `); | ||
|
|
||
| await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); | ||
| const results = await runAxe([PagePanel.metadata.tag]); | ||
| expect(results.violations.length).toBe(0); | ||
| removeFixture(fixture); | ||
| }); |
There was a problem hiding this comment.
Ensure fixture teardown always runs.
removeFixture(fixture) is only reached on the success path. If elementIsStable or runAxe throws, the fixture remains in document.body, which can leak DOM across tests and cause follow-on flakiness.
Suggested fix
describe(PagePanel.metadata.tag, () => {
it('should pass axe check', async () => {
const fixture = await createFixture(html`
...
`);
- await elementIsStable(fixture.querySelector(PagePanel.metadata.tag));
- const results = await runAxe([PagePanel.metadata.tag]);
- expect(results.violations.length).toBe(0);
- removeFixture(fixture);
+ try {
+ await elementIsStable(fixture.querySelector(PagePanel.metadata.tag));
+ const results = await runAxe([PagePanel.metadata.tag]);
+ expect(results.violations.length).toBe(0);
+ } finally {
+ removeFixture(fixture);
+ }
});
});📝 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.
Suggested change
| it('should pass axe check', async () => { | |
| const fixture = await createFixture(html` | |
| <nve-page> | |
| <nve-page-panel id="panel"> | |
| <nve-page-panel-header>panel heading</nve-page-panel-header> | |
| <nve-icon-button | |
| slot="actions" | |
| commandfor="panel" | |
| command="--close" | |
| icon-name="cancel" | |
| aria-label="close panel"> | |
| </nve-icon-button> | |
| <nve-page-panel-content>panel content</nve-page-panel-content> | |
| <nve-page-panel-footer>panel footer</nve-page-panel-footer> | |
| </nve-page-panel> | |
| </nve-page> | |
| `); | |
| await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); | |
| const results = await runAxe([PagePanel.metadata.tag]); | |
| expect(results.violations.length).toBe(0); | |
| removeFixture(fixture); | |
| }); | |
| it('should pass axe check', async () => { | |
| const fixture = await createFixture(html` | |
| <nve-page> | |
| <nve-page-panel id="panel"> | |
| <nve-page-panel-header>panel heading</nve-page-panel-header> | |
| <nve-icon-button | |
| slot="actions" | |
| commandfor="panel" | |
| command="--close" | |
| icon-name="cancel" | |
| aria-label="close panel"> | |
| </nve-icon-button> | |
| <nve-page-panel-content>panel content</nve-page-panel-content> | |
| <nve-page-panel-footer>panel footer</nve-page-panel-footer> | |
| </nve-page-panel> | |
| </nve-page> | |
| `); | |
| try { | |
| await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); | |
| const results = await runAxe([PagePanel.metadata.tag]); | |
| expect(results.violations.length).toBe(0); | |
| } finally { | |
| removeFixture(fixture); | |
| } | |
| }); |
🤖 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 `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 13 -
35, The test currently calls removeFixture(fixture) only on the success path,
which can leak DOM if elementIsStable or runAxe throws; wrap the async test body
so that after creating fixture with createFixture(...) you always call
removeFixture(fixture) in a finally block (or ensure teardown via afterEach) so
the fixture is removed regardless of failures, adjusting references to
createFixture, elementIsStable, runAxe, and removeFixture in the test named
'should pass axe check'.
coryrylan
force-pushed
the
topic-cleanup-core
branch
from
97a3f25 to
84daad7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
projects/core/src/page/page-panel/page-panel.test.axe.ts (1)
31-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuarantee fixture teardown on failure paths.
removeFixture(fixture)only runs on success. If Line 31 or Line 32 throws, the fixture leaks into later tests.Suggested fix
it('should pass axe check', async () => { const fixture = await createFixture(html` ... `); - await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); - const results = await runAxe([PagePanel.metadata.tag]); - expect(results.violations.length).toBe(0); - removeFixture(fixture); + try { + await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); + const results = await runAxe([PagePanel.metadata.tag]); + expect(results.violations.length).toBe(0); + } finally { + removeFixture(fixture); + } });🤖 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 `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 31 - 34, The test currently calls removeFixture(fixture) only on the success path which leaks DOM when elementIsStable(fixture.querySelector(PagePanel.metadata.tag)) or runAxe([PagePanel.metadata.tag]) throws; wrap the test actions in a try/finally (or equivalent test-framework teardown) so removeFixture(fixture) is executed in the finally block to guarantee cleanup; locate the test containing elementIsStable, runAxe, PagePanel.metadata.tag and fixture and move the removeFixture(fixture) call into the finally section (or add an afterEach that calls removeFixture using the same fixture variable) to ensure teardown on failures.
🤖 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 `@projects/core/src/format-relative-time/format-relative-time.test.ts`:
- Around line 412-424: The Date.now spy named now in the test 'should fall back
to raw string for relative values outside Intl range' can leak if an assertion
throws; wrap the setup and assertions so the spy is always restored (e.g., move
now.mockRestore() into a finally block or use a try/finally around the test body
or an afterEach that restores the spy), ensuring the spy created by
vi.spyOn(Date, 'now') is always cleaned up even if element.requestUpdate() or
the expect throws; reference the test name and the now spy to locate and update
the code.
In `@projects/core/src/forms/control/control.css`:
- Around line 189-190: The declaration-empty-line-before lint violation is
caused by missing a blank line before the `width` declaration; edit the block
containing `--control-height: auto;` and `width: fit-content;` (the
`--control-height` custom property and the `width` property) and insert a single
empty line before `width: fit-content;` so the `width` declaration is separated
from the preceding custom property, satisfying the stylelint
`declaration-empty-line-before` rule.
In `@projects/core/src/select/select.test.ts`:
- Around line 283-286: The test sets option.selected = true and immediately
asserts requestUpdate wasn't called, but MutationObserver callbacks are async;
update the test to wait for observer delivery before the negative assertion by
using the project's test helper pattern (e.g., call elementIsStable(element) or
await the fixture/frame microtask after setting option.selected) so any delayed
requestUpdate calls run first; locate the assertion around the spy on
element.requestUpdate in select.test.ts and insert the await/elementIsStable
wait before the expect(requestUpdate).not.toHaveBeenCalled() check.
---
Duplicate comments:
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 31-34: The test currently calls removeFixture(fixture) only on the
success path which leaks DOM when
elementIsStable(fixture.querySelector(PagePanel.metadata.tag)) or
runAxe([PagePanel.metadata.tag]) throws; wrap the test actions in a try/finally
(or equivalent test-framework teardown) so removeFixture(fixture) is executed in
the finally block to guarantee cleanup; locate the test containing
elementIsStable, runAxe, PagePanel.metadata.tag and fixture and move the
removeFixture(fixture) call into the finally section (or add an afterEach that
calls removeFixture using the same fixture variable) to ensure teardown on
failures.
🪄 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: Enterprise
Run ID: cb4672ee-95ad-43e9-821e-e07486e03d60
📒 Files selected for processing (26)
projects/core/src/checkbox/checkbox.test.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.ts
Comment on lines
+412
to
+424
| it('should fall back to raw string for relative values outside Intl range', async () => { | ||
| vi.useRealTimers(); | ||
| const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); | ||
|
|
||
| element.date = '2023-07-27T12:00:00.000Z'; | ||
| element.requestUpdate(); | ||
| await elementIsStable(element); | ||
|
|
||
| const time = element.shadowRoot!.querySelector('time'); | ||
| expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); | ||
|
|
||
| now.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
Ensure Date.now spy is always restored, even on assertion failure.
now.mockRestore() currently runs only on the happy path. If the test throws earlier, the global spy can leak into later tests.
Proposed fix
it('should fall back to raw string for relative values outside Intl range', async () => {
vi.useRealTimers();
const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY);
-
- element.date = '2023-07-27T12:00:00.000Z';
- element.requestUpdate();
- await elementIsStable(element);
-
- const time = element.shadowRoot!.querySelector('time');
- expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z');
-
- now.mockRestore();
+ try {
+ element.date = '2023-07-27T12:00:00.000Z';
+ element.requestUpdate();
+ await elementIsStable(element);
+
+ const time = element.shadowRoot!.querySelector('time');
+ expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z');
+ } finally {
+ now.mockRestore();
+ }
});📝 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.
Suggested change
| it('should fall back to raw string for relative values outside Intl range', async () => { | |
| vi.useRealTimers(); | |
| const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); | |
| element.date = '2023-07-27T12:00:00.000Z'; | |
| element.requestUpdate(); | |
| await elementIsStable(element); | |
| const time = element.shadowRoot!.querySelector('time'); | |
| expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); | |
| now.mockRestore(); | |
| }); | |
| it('should fall back to raw string for relative values outside Intl range', async () => { | |
| vi.useRealTimers(); | |
| const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); | |
| try { | |
| element.date = '2023-07-27T12:00:00.000Z'; | |
| element.requestUpdate(); | |
| await elementIsStable(element); | |
| const time = element.shadowRoot!.querySelector('time'); | |
| expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); | |
| } finally { | |
| now.mockRestore(); | |
| } | |
| }); |
🤖 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 `@projects/core/src/format-relative-time/format-relative-time.test.ts` around
lines 412 - 424, The Date.now spy named now in the test 'should fall back to raw
string for relative values outside Intl range' can leak if an assertion throws;
wrap the setup and assertions so the spy is always restored (e.g., move
now.mockRestore() into a finally block or use a try/finally around the test body
or an afterEach that restores the spy), ensuring the spy created by
vi.spyOn(Date, 'now') is always cleaned up even if element.requestUpdate() or
the expect throws; reference the test name and the now spy to locate and update
the code.
Comment on lines
+283
to
+286
| const requestUpdate = vi.spyOn(element, 'requestUpdate'); | ||
| option.selected = true; | ||
|
|
||
| expect(requestUpdate).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Wait for observer delivery before asserting no requestUpdate calls
MutationObserver notifications are async, so asserting immediately after option.selected = true can miss a late callback and produce a false pass. Add a microtask/frame wait before the negative assertion.
Proposed fix
const requestUpdate = vi.spyOn(element, 'requestUpdate');
option.selected = true;
+ await new Promise(resolve => setTimeout(resolve, 0));
expect(requestUpdate).not.toHaveBeenCalled();As per coding guidelines, "**/*.test.ts: Follow unit testing patterns from .../testing-unit.md including createFixture and elementIsStable patterns."
📝 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.
Suggested change
| const requestUpdate = vi.spyOn(element, 'requestUpdate'); | |
| option.selected = true; | |
| expect(requestUpdate).not.toHaveBeenCalled(); | |
| const requestUpdate = vi.spyOn(element, 'requestUpdate'); | |
| option.selected = true; | |
| await new Promise(resolve => setTimeout(resolve, 0)); | |
| expect(requestUpdate).not.toHaveBeenCalled(); |
🤖 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 `@projects/core/src/select/select.test.ts` around lines 283 - 286, The test
sets option.selected = true and immediately asserts requestUpdate wasn't called,
but MutationObserver callbacks are async; update the test to wait for observer
delivery before the negative assertion by using the project's test helper
pattern (e.g., call elementIsStable(element) or await the fixture/frame
microtask after setting option.selected) so any delayed requestUpdate calls run
first; locate the assertion around the spy on element.requestUpdate in
select.test.ts and insert the await/elementIsStable wait before the
expect(requestUpdate).not.toHaveBeenCalled() check.
Source: Coding guidelines
- Added tests to ensure that the checked state of checkboxes and radio buttons resets to their native defaults upon calling the reset method. - Improved the handling of observers in form controls to prevent duplication after reconnecting. - Enhanced the Select component to stop observing removed options and to reset selected options to their native defaults. - Added accessibility tests for the PagePanel component to ensure compliance with axe standards. Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cleanup-core
branch
from
84daad7 to
42af5d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
projects/core/src/format-relative-time/format-relative-time.test.ts (1)
412-424:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure
Date.nowspy is always restored, even on assertion failure.The spy cleanup on line 423 only runs if all assertions pass. If
expectthrows, the mock will leak into later tests.🛡️ Wrap cleanup in finally block
it('should fall back to raw string for relative values outside Intl range', async () => { vi.useRealTimers(); const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); - - element.date = '2023-07-27T12:00:00.000Z'; - element.requestUpdate(); - await elementIsStable(element); - - const time = element.shadowRoot!.querySelector('time'); - expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); - - now.mockRestore(); + try { + element.date = '2023-07-27T12:00:00.000Z'; + element.requestUpdate(); + await elementIsStable(element); + + const time = element.shadowRoot!.querySelector('time'); + expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); + } finally { + now.mockRestore(); + } });🤖 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 `@projects/core/src/format-relative-time/format-relative-time.test.ts` around lines 412 - 424, The Date.now spy (variable now) must be restored in a finally block to avoid leaking the mock if expect throws; update the test case (the it('should fall back to raw string for relative values outside Intl range', ...) block) to wrap the setup, DOM checks and assertions inside a try { ... } finally { now.mockRestore(); vi.useRealTimers(); } (or at minimum call now.mockRestore() in finally) so the spy is always restored even on assertion failure; refer to Date.now spy variable name now, and the calls element.requestUpdate(), await elementIsStable(element), and the time query selector to locate the code to wrap.projects/core/src/page/page-panel/page-panel.test.axe.ts (1)
13-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure fixture teardown always runs.
removeFixture(fixture)is only reached on the success path. IfelementIsStableorrunAxethrows, the fixture remains indocument.body, which can leak DOM across tests and cause follow-on flakiness.🛡️ Proposed fix to guarantee cleanup
describe(PagePanel.metadata.tag, () => { it('should pass axe check', async () => { const fixture = await createFixture(html` <nve-page> <nve-page-panel id="panel"> <nve-page-panel-header>panel heading</nve-page-panel-header> <nve-icon-button slot="actions" commandfor="panel" command="--close" icon-name="cancel" aria-label="close panel"> </nve-icon-button> <nve-page-panel-content>panel content</nve-page-panel-content> <nve-page-panel-footer>panel footer</nve-page-panel-footer> </nve-page-panel> </nve-page> `); - await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); - const results = await runAxe([PagePanel.metadata.tag]); - expect(results.violations.length).toBe(0); - removeFixture(fixture); + try { + await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); + const results = await runAxe([PagePanel.metadata.tag]); + expect(results.violations.length).toBe(0); + } finally { + removeFixture(fixture); + } }); });🤖 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 `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 13 - 35, The test currently calls removeFixture(fixture) only on the success path which can leak DOM if elementIsStable or runAxe throws; wrap the test body that uses createFixture, elementIsStable, runAxe and PagePanel.metadata.tag in a try/finally and call removeFixture(fixture) from the finally block (ensuring fixture is declared in the outer scope so finally can access it) so cleanup always runs regardless of failures.projects/core/src/forms/control/control.css (1)
188-191:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stylelint empty-line violation before
width.Line 190 violates
declaration-empty-line-before; an empty line is required before thewidthdeclaration.🛠️ Suggested fix
:host([nve-control='inline']) { --control-height: auto; + width: fit-content;🤖 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 `@projects/core/src/forms/control/control.css` around lines 188 - 191, The CSS block for :host([nve-control='inline']) is missing the required empty line before the width declaration; update the declaration block for the selector :host([nve-control='inline']) so there is a blank line immediately before the width property (i.e., ensure an empty line separates --control-height and width) to satisfy the declaration-empty-line-before rule.projects/core/src/select/select.test.ts (1)
270-287:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWait for observer delivery before asserting no
requestUpdatecalls.
MutationObservercallbacks are async, so asserting immediately afteroption.selected = truecan miss a late callback and produce a false pass. Add a microtask wait before the negative assertion to ensure any pending observer callbacks have fired.🐛 Proposed fix
const requestUpdate = vi.spyOn(element, 'requestUpdate'); option.selected = true; + await elementIsStable(element); expect(requestUpdate).not.toHaveBeenCalled();As per coding guidelines, "
**/*.test.ts: Follow unit testing patterns from .../testing-unit.md including createFixture and elementIsStable patterns."🤖 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 `@projects/core/src/select/select.test.ts` around lines 270 - 287, The negative assertion runs before the MutationObserver could fire; after setting option.selected = true in the 'should stop observing removed option selected state changes' test, wait for observer delivery (e.g. await a microtask or call elementIsStable(element) again) before asserting that element.requestUpdate was not called so any pending observer callbacks can run; update the test around the option.selected assignment and expect(requestUpdate).not.toHaveBeenCalled() to include that microtask wait using the existing elementIsStable helper.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 `@projects/core/src/internal/controllers/type-native-popover.controller.ts`:
- Around line 73-81: hostConnected() can schedule multiple async continuations
that each call `#setupLegacyTriggers`() and push duplicate observers into
`#observers`; fix by adding a per-instance cancellation token or incrementing
runId (e.g., this.#setupRunId) at the start of hostConnected() and capturing it
in the async continuation/await chain inside hostConnected() and inside
`#setupLegacyTriggers`(), then bail out if the captured id no longer matches the
current token before mutating `#observers` or installing listeners; apply the same
guard to the other async setup block referenced around lines 165-179 so stale
continuations cannot add observers after a disconnect/reconnect cycle.
- Around line 196-233: When switching an existing legacy trigger to 'hint' mode
the code never clears its prior native wiring because `#previousLegacyTrigger` ===
trigger prevents `#clearLegacyTrigger` from running; update `#updateLegacyTriggers`
to explicitly clear native wiring for the same trigger before installing hint
handlers: detect when host.popoverType === 'hint' and the current trigger
already has native attributes/fields (e.g., popovertarget attribute or
popoverTargetElement) or when `#previousLegacyTrigger` === trigger, call
this.#clearLegacyTrigger(trigger) (or otherwise remove
popovertarget/popoverTargetElement) and this.#removeHintTrigger() as needed,
then proceed to call this.#setupHintTrigger(trigger) and set
`#previousLegacyTrigger` = trigger so the button no longer retains native click
behavior.
- Around line 103-113: hostDisconnected() currently removes event listeners but
leaves a possible legacy trigger referencing the detached host; update
hostDisconnected to check this.#previousLegacyTrigger and, if present, clear its
popoverTargetElement property (set to null), remove the popovertarget attribute
(removeAttribute('popovertarget')), and then set this.#previousLegacyTrigger =
undefined (or null) so the trigger no longer holds a strong reference to the
host; also ensure this cleanup complements the existing
this.#removeHintTrigger() call.
In `@projects/core/src/internal/utils/dom.ts`:
- Around line 273-278: The catch assumes the thrown value is an Error and uses
(e as Error).message which can be undefined for non-Error throws; update the
EyeDropperConstructor try/catch so the LogService.warn logs a defensive error
message: detect if e is an Error and use e.message, else if it's a string use it
directly, otherwise attempt JSON.stringify(e) and fall back to String(e) on
failure, then include that derived message in the `EyeDropper selection failed:`
log; keep the existing return '' behavior.
- Around line 263-279: Add a JSDoc block above the exported function
openEyeDropper describing its purpose (open the browser EyeDropper and return
the selected color), its return type (Promise<string> — sRGB hex color or empty
string if unavailable/failed), behavior (logs a warning and returns '' when the
API is unavailable or selection fails), and any side effects (uses
LogService.warn). Follow the Google JSDoc style: include a one-line summary, a
longer description if needed, an `@returns` tag with the Promise<string>
explanation, and note that there are no parameters; place the comment
immediately above the openEyeDropper declaration.
In `@projects/core/src/select/select.ts`:
- Around line 184-202: The cleanup in `#syncOptionSelectedStates` currently
filters this.#observers inside the forEach for each removed option, causing
repeated O(n×m) passes; instead, collect removed observers in a temporary Set
(from this.#optionObservers entries where option is not in the new
this.#options) and perform a single this.#observers = this.#observers.filter(o
=> !removedSet.has(o)) pass, while still disconnecting and deleting each removed
observer from this.#optionObservers; update the logic inside
`#syncOptionSelectedStates` (references: this.#options, this.#optionObservers,
this.#observers, getElementUpdate, requestUpdate) to batch removals into one
filter operation.
---
Duplicate comments:
In `@projects/core/src/format-relative-time/format-relative-time.test.ts`:
- Around line 412-424: The Date.now spy (variable now) must be restored in a
finally block to avoid leaking the mock if expect throws; update the test case
(the it('should fall back to raw string for relative values outside Intl range',
...) block) to wrap the setup, DOM checks and assertions inside a try { ... }
finally { now.mockRestore(); vi.useRealTimers(); } (or at minimum call
now.mockRestore() in finally) so the spy is always restored even on assertion
failure; refer to Date.now spy variable name now, and the calls
element.requestUpdate(), await elementIsStable(element), and the time query
selector to locate the code to wrap.
In `@projects/core/src/forms/control/control.css`:
- Around line 188-191: The CSS block for :host([nve-control='inline']) is
missing the required empty line before the width declaration; update the
declaration block for the selector :host([nve-control='inline']) so there is a
blank line immediately before the width property (i.e., ensure an empty line
separates --control-height and width) to satisfy the
declaration-empty-line-before rule.
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 13-35: The test currently calls removeFixture(fixture) only on the
success path which can leak DOM if elementIsStable or runAxe throws; wrap the
test body that uses createFixture, elementIsStable, runAxe and
PagePanel.metadata.tag in a try/finally and call removeFixture(fixture) from the
finally block (ensuring fixture is declared in the outer scope so finally can
access it) so cleanup always runs regardless of failures.
In `@projects/core/src/select/select.test.ts`:
- Around line 270-287: The negative assertion runs before the MutationObserver
could fire; after setting option.selected = true in the 'should stop observing
removed option selected state changes' test, wait for observer delivery (e.g.
await a microtask or call elementIsStable(element) again) before asserting that
element.requestUpdate was not called so any pending observer callbacks can run;
update the test around the option.selected assignment and
expect(requestUpdate).not.toHaveBeenCalled() to include that microtask wait
using the existing elementIsStable helper.
🪄 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: Enterprise
Run ID: 278608d1-cca1-4b42-9a8b-2e34fa297ec4
📒 Files selected for processing (37)
projects/core/src/checkbox/checkbox.test.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/dropdown-group/dropdown-group.test.lighthouse.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/menu/menu.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/pagination/pagination.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/tree/tree.test.lighthouse.ts
Comment on lines
73
to
81
| async hostConnected() { | ||
| attachInternals(this.host); | ||
| this.host.popover = this.host.popoverType ?? null; | ||
| await this.host.updateComplete; | ||
| if (!this.host.isConnected) return; | ||
|
|
||
| this.host.setAttribute('nve-popover', ''); | ||
| this.#updateLegacyTriggers(); | ||
| this.#setupLegacyTriggers(); // eslint-disable-line @typescript-eslint/no-floating-promises |
There was a problem hiding this comment.
Cancel stale async setup across disconnect/reconnect cycles.
hostConnected() can still schedule two #setupLegacyTriggers() runs for the same instance: if the element disconnects and reconnects before the first updateComplete/requestAnimationFrame chain finishes, both continuations will survive and each one will push its own hidden observer into #observers. That recreates the leak/duplicate-callback problem on the legacy hidden-path even though the DOM listeners themselves are stable references.
Suggested fix
export class TypeNativePopoverController<T extends NativePopover> implements ReactiveController {
+ `#connectionEpoch` = 0;
+
async hostConnected() {
+ const connectionEpoch = ++this.#connectionEpoch;
attachInternals(this.host);
this.host.popover = this.host.popoverType ?? null;
await this.host.updateComplete;
- if (!this.host.isConnected) return;
+ if (!this.host.isConnected || connectionEpoch !== this.#connectionEpoch) return;
this.host.setAttribute('nve-popover', '');
this.#updateLegacyTriggers();
- this.#setupLegacyTriggers(); // eslint-disable-line `@typescript-eslint/no-floating-promises`
+ this.#setupLegacyTriggers(connectionEpoch); // eslint-disable-line `@typescript-eslint/no-floating-promises`
this.#setupModalLightDismiss();
this.host.inert = this.host.matches(':not(:popover-open)') && !!this.#nativeTriggers.length;
@@
hostDisconnected() {
+ this.#connectionEpoch++;
this.#observers.forEach(observer => observer.disconnect());
this.#observers.length = 0;
@@
- async `#setupLegacyTriggers`() {
+ async `#setupLegacyTriggers`(connectionEpoch: number) {
await new Promise(r => requestAnimationFrame(r));
+ if (!this.host.isConnected || connectionEpoch !== this.#connectionEpoch) return;
// setup hidden updates for legacy triggers
this.#observers.push(Also applies to: 165-179
🤖 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 `@projects/core/src/internal/controllers/type-native-popover.controller.ts`
around lines 73 - 81, hostConnected() can schedule multiple async continuations
that each call `#setupLegacyTriggers`() and push duplicate observers into
`#observers`; fix by adding a per-instance cancellation token or incrementing
runId (e.g., this.#setupRunId) at the start of hostConnected() and capturing it
in the async continuation/await chain inside hostConnected() and inside
`#setupLegacyTriggers`(), then bail out if the captured id no longer matches the
current token before mutating `#observers` or installing listeners; apply the same
guard to the other async setup block referenced around lines 165-179 so stale
continuations cannot add observers after a disconnect/reconnect cycle.
Comment on lines
103
to
+113
| hostDisconnected() { | ||
| this.#observers.forEach(observer => observer.disconnect()); | ||
| this.#observers.length = 0; | ||
| this.host.removeEventListener('beforetoggle', this.#onBeforeToggle); | ||
| this.host.removeEventListener('toggle', this.#onToggle as EventListener); | ||
| this.host.removeEventListener('command', this.#onCommand as EventListener); | ||
| this.host.removeEventListener('interest', this.#onInterest as EventListener); | ||
| this.host.removeEventListener('loseinterest', this.#onLoseInterest as EventListener); | ||
| this.host.removeEventListener('pointerdown', this.#onPointerDown); | ||
| this.host.removeEventListener('pointerup', this.#onPointerUp); | ||
| this.#removeHintTrigger(); |
There was a problem hiding this comment.
Disconnect cleanup still leaves the legacy trigger retaining the popover.
hostDisconnected() removes listeners, but it never clears #previousLegacyTrigger or the trigger’s popoverTargetElement/popovertarget. If the trigger stays in the DOM after the popover is removed, that button still holds a strong reference to the detached host and can keep routing native popover actions at a disconnected node.
Suggested fix
hostDisconnected() {
this.#observers.forEach(observer => observer.disconnect());
this.#observers.length = 0;
+ if (this.#previousLegacyTrigger) {
+ this.#clearLegacyTrigger(this.#previousLegacyTrigger);
+ this.#previousLegacyTrigger = null;
+ }
this.host.removeEventListener('beforetoggle', this.#onBeforeToggle);
this.host.removeEventListener('toggle', this.#onToggle as EventListener);
this.host.removeEventListener('command', this.#onCommand as EventListener);
this.host.removeEventListener('interest', this.#onInterest as EventListener);
this.host.removeEventListener('loseinterest', this.#onLoseInterest as EventListener);📝 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.
Suggested change
| hostDisconnected() { | |
| this.#observers.forEach(observer => observer.disconnect()); | |
| this.#observers.length = 0; | |
| this.host.removeEventListener('beforetoggle', this.#onBeforeToggle); | |
| this.host.removeEventListener('toggle', this.#onToggle as EventListener); | |
| this.host.removeEventListener('command', this.#onCommand as EventListener); | |
| this.host.removeEventListener('interest', this.#onInterest as EventListener); | |
| this.host.removeEventListener('loseinterest', this.#onLoseInterest as EventListener); | |
| this.host.removeEventListener('pointerdown', this.#onPointerDown); | |
| this.host.removeEventListener('pointerup', this.#onPointerUp); | |
| this.#removeHintTrigger(); | |
| hostDisconnected() { | |
| this.#observers.forEach(observer => observer.disconnect()); | |
| this.#observers.length = 0; | |
| if (this.#previousLegacyTrigger) { | |
| this.#clearLegacyTrigger(this.#previousLegacyTrigger); | |
| this.#previousLegacyTrigger = null; | |
| } | |
| this.host.removeEventListener('beforetoggle', this.#onBeforeToggle); | |
| this.host.removeEventListener('toggle', this.#onToggle as EventListener); | |
| this.host.removeEventListener('command', this.#onCommand as EventListener); | |
| this.host.removeEventListener('interest', this.#onInterest as EventListener); | |
| this.host.removeEventListener('loseinterest', this.#onLoseInterest as EventListener); | |
| this.host.removeEventListener('pointerdown', this.#onPointerDown); | |
| this.host.removeEventListener('pointerup', this.#onPointerUp); | |
| this.#removeHintTrigger(); |
🤖 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 `@projects/core/src/internal/controllers/type-native-popover.controller.ts`
around lines 103 - 113, hostDisconnected() currently removes event listeners but
leaves a possible legacy trigger referencing the detached host; update
hostDisconnected to check this.#previousLegacyTrigger and, if present, clear its
popoverTargetElement property (set to null), remove the popovertarget attribute
(removeAttribute('popovertarget')), and then set this.#previousLegacyTrigger =
undefined (or null) so the trigger no longer holds a strong reference to the
host; also ensure this cleanup complements the existing
this.#removeHintTrigger() call.
Comment on lines
184
to
202
| #syncOptionSelectedStates() { | ||
| const options = new Set(this.#options); | ||
|
|
||
| this.#optionObservers.forEach((observer, option) => { | ||
| if (!options.has(option)) { | ||
| observer.disconnect(); | ||
| this.#optionObservers.delete(option); | ||
| this.#observers = this.#observers.filter(o => o !== observer); | ||
| } | ||
| }); | ||
|
|
||
| this.#options.forEach(o => { | ||
| if (!this.#trackedOptions.has(o)) { | ||
| this.#trackedOptions.add(o); | ||
| this.#observers.push(getElementUpdate(o, 'selected', () => this.requestUpdate())); | ||
| if (!this.#optionObservers.has(o)) { | ||
| const observer = getElementUpdate(o, 'selected', () => this.requestUpdate()); | ||
| this.#optionObservers.set(o, observer); | ||
| this.#observers.push(observer); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Observer cleanup uses repeated array filter operations.
The implementation filters #observers once per removed option (line 191), which results in O(n×m) complexity when m options are removed (n = total observers, m = removed options). For typical select elements with a modest number of options this is acceptable, but if the component frequently reconciles large option sets, consider batching removals into a single filter pass.
♻️ Potential optimization
`#syncOptionSelectedStates`() {
const options = new Set(this.#options);
+ const observersToRemove = new Set<MutationObserver>();
this.#optionObservers.forEach((observer, option) => {
if (!options.has(option)) {
observer.disconnect();
this.#optionObservers.delete(option);
- this.#observers = this.#observers.filter(o => o !== observer);
+ observersToRemove.add(observer);
}
});
+ if (observersToRemove.size > 0) {
+ this.#observers = this.#observers.filter(o => !observersToRemove.has(o as MutationObserver));
+ }
this.#options.forEach(o => {
if (!this.#optionObservers.has(o)) {
const observer = getElementUpdate(o, 'selected', () => this.requestUpdate());
this.#optionObservers.set(o, observer);
this.#observers.push(observer);
}
});
}📝 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.
Suggested change
| #syncOptionSelectedStates() { | |
| const options = new Set(this.#options); | |
| this.#optionObservers.forEach((observer, option) => { | |
| if (!options.has(option)) { | |
| observer.disconnect(); | |
| this.#optionObservers.delete(option); | |
| this.#observers = this.#observers.filter(o => o !== observer); | |
| } | |
| }); | |
| this.#options.forEach(o => { | |
| if (!this.#trackedOptions.has(o)) { | |
| this.#trackedOptions.add(o); | |
| this.#observers.push(getElementUpdate(o, 'selected', () => this.requestUpdate())); | |
| if (!this.#optionObservers.has(o)) { | |
| const observer = getElementUpdate(o, 'selected', () => this.requestUpdate()); | |
| this.#optionObservers.set(o, observer); | |
| this.#observers.push(observer); | |
| } | |
| }); | |
| } | |
| `#syncOptionSelectedStates`() { | |
| const options = new Set(this.#options); | |
| const observersToRemove = new Set<MutationObserver>(); | |
| this.#optionObservers.forEach((observer, option) => { | |
| if (!options.has(option)) { | |
| observer.disconnect(); | |
| this.#optionObservers.delete(option); | |
| observersToRemove.add(observer); | |
| } | |
| }); | |
| if (observersToRemove.size > 0) { | |
| this.#observers = this.#observers.filter(o => !observersToRemove.has(o as MutationObserver)); | |
| } | |
| this.#options.forEach(o => { | |
| if (!this.#optionObservers.has(o)) { | |
| const observer = getElementUpdate(o, 'selected', () => this.requestUpdate()); | |
| this.#optionObservers.set(o, observer); | |
| this.#observers.push(observer); | |
| } | |
| }); | |
| } |
🤖 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 `@projects/core/src/select/select.ts` around lines 184 - 202, The cleanup in
`#syncOptionSelectedStates` currently filters this.#observers inside the forEach
for each removed option, causing repeated O(n×m) passes; instead, collect
removed observers in a temporary Set (from this.#optionObservers entries where
option is not in the new this.#options) and perform a single this.#observers =
this.#observers.filter(o => !removedSet.has(o)) pass, while still disconnecting
and deleting each removed observer from this.#optionObservers; update the logic
inside `#syncOptionSelectedStates` (references: this.#options,
this.#optionObservers, this.#observers, getElementUpdate, requestUpdate) to
batch removals into one filter operation.
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
1 participant
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.
Summary by CodeRabbit
Bug Fixes
Style
Tests