Skip to content

fix(core): memory leaks from connected callback states #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
coryrylan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(core): memory leaks from connected callback states #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions projects/core/src/checkbox/checkbox.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ describe(`${Checkbox.metadata.tag} - control base behavior`, () => {
expect((e as Event).composed).toBe(true);
});

it('should reset checked state to the native default', async () => {
const input = fixture.querySelector('input');
input.defaultChecked = true;
input.checked = false;

element.reset();
await elementIsStable(element);

expect(input.checked).toBe(true);
});

it('should disconnect observers when removed from DOM', async () => {
const input = fixture.querySelector('input');
expect(element.matches(':state(checked)')).toBe(false);
Expand Down
4 changes: 2 additions & 2 deletions projects/core/src/combobox/combobox.test.lighthouse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describe('combobox lighthouse report', () => {
expect(report.scores.performance).toBe(100);
expect(report.scores.accessibility).toBe(100);
expect(report.scores.bestPractices).toBe(100);
expect(report.payload.javascript.kb).toBeLessThan(36);
expect(report.payload.javascript.kb).toBeLessThan(36.1);
});

test('combobox multi select with large dataset should meet lighthouse benchmarks', async () => {
Expand All @@ -42,6 +42,6 @@ describe('combobox lighthouse report', () => {

expect(report.scores.performance).toBeGreaterThanOrEqual(96);
expect(report.scores.bestPractices).toBe(100);
expect(report.payload.javascript.kb).toBeLessThan(36);
expect(report.payload.javascript.kb).toBeLessThan(36.1);
});
});
2 changes: 1 addition & 1 deletion projects/core/src/dialog/dialog.test.lighthouse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ describe('dialog lighthouse report', () => {
expect(report.scores.performance).toBeGreaterThan(97); // bfcache
expect(report.scores.accessibility).toBe(100);
expect(report.scores.bestPractices).toBe(100);
expect(report.payload.javascript.kb).toBeLessThan(24.8);
expect(report.payload.javascript.kb).toBeLessThan(24.9);
});
});
2 changes: 1 addition & 1 deletion projects/core/src/drawer/drawer.test.lighthouse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ describe('drawer lighthouse report', () => {
expect(report.scores.performance).toBeGreaterThan(98); // bfcache
expect(report.scores.accessibility).toBe(100);
expect(report.scores.bestPractices).toBe(100);
expect(report.payload.javascript.kb).toBeLessThan(25.3);
expect(report.payload.javascript.kb).toBeLessThan(25.4);
});
});
2 changes: 1 addition & 1 deletion projects/core/src/dropdown-group/dropdown-group.test.lighthouse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,6 @@ describe('dropdown-group lighthouse report', () => {
expect(report.scores.performance).toBe(100);
expect(report.scores.accessibility).toBe(100);
expect(report.scores.bestPractices).toBe(100);
expect(report.payload.javascript.kb).toBeLessThan(24.7);
expect(report.payload.javascript.kb).toBeLessThan(24.8);
});
});
14 changes: 14 additions & 0 deletions projects/core/src/format-relative-time/format-relative-time.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,5 +408,19 @@ describe(FormatRelativeTime.metadata.tag, () => {
const time = element.shadowRoot!.querySelector('time');
expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z');
});

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();
});
Comment on lines +412 to +424

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

});
});
2 changes: 1 addition & 1 deletion projects/core/src/format-relative-time/format-relative-time.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export class FormatRelativeTime extends LitElement {
const value = Math.round(absDiff / UNIT_DIVISORS[unit]);
if (value < max) return { value: sign * value, unit };
}
throw new Error('format-relative-time: no relative time threshold matched');
return { value: sign * Math.round(absDiff / UNIT_DIVISORS.year), unit: 'year' };
}

#computeExplicitUnit(diffMs: number, unit: TimeUnitOption): number {
Expand Down
2 changes: 1 addition & 1 deletion projects/core/src/forms/control-group/control-group.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
--color: var(--nve-sys-interaction-field-color);
--opacity: 1;
--label-color: var(--nve-sys-interaction-field-color);
--label-text-transform: capitalize;
--label-text-transform: none;
--label-font-weight: var(--nve-ref-font-weight-medium);
--label-font-size: var(--nve-ref-font-size-100);
--_label-width: var(--label-width, 180px);
Expand Down
9 changes: 7 additions & 2 deletions projects/core/src/forms/control-group/control-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@ import {
associateControlGroup
} from '@nvidia-elements/core/internal';
import type { ControlMessage } from '../control-message/control-message.js';
import { setupControlStatusStates, setupControlGroupStates, inputQuery } from '../utils/states.js';
import {
setupControlStatusStates,
setupControlGroupStates,
inputQuery,
type ControlStateCleanup
} from '../utils/states.js';
import { setupControlLayoutStates } from '../utils/layout.js';
import styles from './control-group.css?inline';

Expand Down Expand Up @@ -53,7 +58,7 @@ export class ControlGroup extends LitElement {
return this.querySelectorAll ? Array.from(this.querySelectorAll<ControlMessage>('nve-control-message')) : [];
}

#observers: (MutationObserver | ResizeObserver)[] = [];
#observers: (MutationObserver | ResizeObserver | ControlStateCleanup)[] = [];

static styles = useStyles([styles]);

Expand Down
13 changes: 4 additions & 9 deletions projects/core/src/forms/control/control.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
--accent-color: var(--nve-sys-accent-secondary-background);
--color: var(--nve-sys-interaction-field-color);
--label-color: var(--nve-sys-interaction-field-color);
--label-text-transform: capitalize;
--label-text-transform: none;
--label-font-weight: var(--nve-ref-font-weight-medium);
--label-font-size: var(--nve-ref-font-size-100);
--control-width: 100%;
Expand All @@ -18,10 +18,6 @@
contain: content;
}

:host([nve-control='inline']) {
--control-height: auto;
}

::slotted(*) {
color-scheme: var(--nve-sys-color-scheme);
font-family: inherit;
Expand All @@ -38,10 +34,6 @@
align-self: center !important;
}

::slotted(label::first-letter) {
text-transform: capitalize;
}

:host(:state(disabled)),
:host(:state(disabled)) ::slotted(nve-control-message:not([status])) {
--cursor: not-allowed;
Expand Down Expand Up @@ -194,6 +186,9 @@ slot[name='messages'] {

/** inline controls */
:host([nve-control='inline']) {
--control-height: auto;
width: fit-content;
Comment thread
coderabbitai[bot] marked this conversation as resolved.

[internal-host] {
grid-template-areas: 'input input input';

Expand Down
29 changes: 28 additions & 1 deletion projects/core/src/forms/control/control.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

import { html } from 'lit';
import { describe, expect, it, beforeEach, afterEach } from 'vitest';
import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest';
import { createFixture, removeFixture, elementIsStable, untilEvent } from '@internals/testing';
import { Control, ControlMessage } from '@nvidia-elements/core/forms';
import '@nvidia-elements/core/forms/define.js';
Expand Down Expand Up @@ -167,6 +167,33 @@ describe(Control.metadata.tag, () => {
expect(getComputedStyle(message).display).toBe('block');
expect(validationMessage.hidden).toBe(true);
});

it('should not duplicate form reset listeners after reconnect', async () => {
removeFixture(fixture);
fixture = await createFixture(html`
<form>
<nve-control>
<label>label</label>
<input type="text" required />
<nve-control-message>message</nve-control-message>
</nve-control>
</form>
`);
const form = fixture.querySelector('form');
element = fixture.querySelector(Control.metadata.tag);
await elementIsStable(element);

element.remove();
form.appendChild(element);
await elementIsStable(element);
element.shadowRoot!.dispatchEvent(new Event('slotchange'));
await elementIsStable(element);

const requestUpdate = vi.spyOn(element, 'requestUpdate');
form.dispatchEvent(new Event('reset'));

expect(requestUpdate).toHaveBeenCalledTimes(2);
});
});

describe(`${Control.metadata.tag}: custom`, () => {
Expand Down
45 changes: 40 additions & 5 deletions projects/core/src/forms/control/control.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,20 @@ import {
setupControlValidationStates,
setupControlStates,
setupControlStatusStates,
inputQuery
inputQuery,
type ControlStateCleanup
} from '../utils/states.js';
import { setupControlLayoutStates } from '../utils/layout.js';
import globalStyles from './control.global.css?inline';
import styles from './control.css?inline';

interface ResettableControl {
getAttribute: Element['getAttribute'];
value: string;
}

type ControlInput = HTMLInputElement | HTMLSelectElement | HTMLTextAreaElement | ResettableControl;

/**
* @element nve-control
* @description Wraps a form input with its associated label and validation messages, managing layout and accessibility associations.
Expand Down Expand Up @@ -118,7 +126,7 @@ export class Control extends LitElement {
/** @private */
declare _internals: ElementInternals;

#observers: (MutationObserver | ResizeObserver)[] = [];
#observers: (MutationObserver | ResizeObserver | ControlStateCleanup)[] = [];

protected _associateDatalist = true;

Expand Down Expand Up @@ -181,15 +189,42 @@ export class Control extends LitElement {

/** Resets control value to initial attribute value and clears any active validation rules. */
reset() {
this.input.value = this.input.getAttribute('value') ?? '';
this.#resetInputValue(this.input as ControlInput);
this.requestUpdate();
this.dispatchEvent(new CustomEvent('reset', { bubbles: true, composed: true }));
}

#setupInput() {
setupControlValidationStates(this, this.#messages);
#resetInputValue(input: ControlInput) {
if (input instanceof HTMLSelectElement) {
this.#resetSelectValue(input);
return;
}

if (this.#isCheckedInput(input)) {
input.checked = input.defaultChecked;
input.indeterminate = false;
return;
}

input.value =
input instanceof HTMLInputElement || input instanceof HTMLTextAreaElement
? input.defaultValue
: (input.getAttribute('value') ?? '');
}

#resetSelectValue(input: HTMLSelectElement) {
const hasDefaultSelection = Array.from(input.options).some(option => option.defaultSelected);
Array.from(input.options).forEach(option => (option.selected = option.defaultSelected));
if (!input.multiple && !hasDefaultSelection) input.selectedIndex = 0;
}

#isCheckedInput(input: ControlInput): input is HTMLInputElement {
return input instanceof HTMLInputElement && (input.type === 'checkbox' || input.type === 'radio');
}

#setupInput() {
this.#observers.push(
...setupControlValidationStates(this, this.#messages),
...setupControlStates(this),
...setupControlStatusStates(this, this.#messages),
setupControlLayoutStates(this),
Expand Down
Loading