Skip to content

Commit 37895a5

Browse files
jgrothclaude
andcommitted
perf(date-picker): defer flatpickr destroy to survive portal-driven DOM moves
When a portal moves a DOM element, the browser fires `disconnectedCallback` followed by `connectedCallback` in the same microtask. The immediate `destroy()` in `disconnectedCallback` killed the flatpickr instance permanently, even though the element was about to be reattached. `DeferredDestroy` pushes the teardown into a `setTimeout(0)` so that `connectedCallback` can cancel it if the element is reconnected. `Picker` methods now guard against a nulled-out flatpickr instance. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 603788b commit 37895a5

4 files changed

Lines changed: 121 additions & 5 deletions

File tree

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { vi } from 'vitest';
2+
import { DeferredDestroy } from './deferred-destroy';
3+
4+
let deferredDestroy: DeferredDestroy;
5+
6+
beforeEach(() => {
7+
vi.useFakeTimers();
8+
deferredDestroy = new DeferredDestroy();
9+
});
10+
11+
afterEach(() => {
12+
vi.useRealTimers();
13+
});
14+
15+
test('schedule does not call the callback synchronously', () => {
16+
const callback = vi.fn();
17+
deferredDestroy.schedule(callback);
18+
19+
expect(callback).not.toHaveBeenCalled();
20+
});
21+
22+
test('schedule calls the callback after the microtask completes', () => {
23+
const callback = vi.fn();
24+
deferredDestroy.schedule(callback);
25+
vi.runAllTimers();
26+
27+
expect(callback).toHaveBeenCalledTimes(1);
28+
});
29+
30+
test('cancel prevents the callback from firing', () => {
31+
const callback = vi.fn();
32+
deferredDestroy.schedule(callback);
33+
deferredDestroy.cancel();
34+
vi.runAllTimers();
35+
36+
expect(callback).not.toHaveBeenCalled();
37+
});
38+
39+
test('cancel is safe to call when nothing is scheduled', () => {
40+
expect(() => deferredDestroy.cancel()).not.toThrow();
41+
});
42+
43+
test('scheduling twice only fires the second callback', () => {
44+
const first = vi.fn();
45+
const second = vi.fn();
46+
deferredDestroy.schedule(first);
47+
deferredDestroy.schedule(second);
48+
vi.runAllTimers();
49+
50+
expect(first).not.toHaveBeenCalled();
51+
expect(second).toHaveBeenCalledTimes(1);
52+
});
53+
54+
test('schedule works after a previous cancel', () => {
55+
const first = vi.fn();
56+
const second = vi.fn();
57+
deferredDestroy.schedule(first);
58+
deferredDestroy.cancel();
59+
deferredDestroy.schedule(second);
60+
vi.runAllTimers();
61+
62+
expect(first).not.toHaveBeenCalled();
63+
expect(second).toHaveBeenCalledTimes(1);
64+
});
65+
66+
test('schedule works after a previous callback has fired', () => {
67+
const first = vi.fn();
68+
const second = vi.fn();
69+
deferredDestroy.schedule(first);
70+
vi.runAllTimers();
71+
deferredDestroy.schedule(second);
72+
vi.runAllTimers();
73+
74+
expect(first).toHaveBeenCalledTimes(1);
75+
expect(second).toHaveBeenCalledTimes(1);
76+
});
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/**
2+
* Defers a destroy callback so that portal-driven DOM moves
3+
* (disconnect + reconnect in the same microtask) can cancel the
4+
* destroy before it fires.
5+
*/
6+
export class DeferredDestroy {
7+
private pendingDestroyTimer: ReturnType<typeof setTimeout> | null = null;
8+
9+
/**
10+
* Schedule a deferred destroy callback. Any previously pending
11+
* callback is cancelled first.
12+
* @param callback
13+
*/
14+
public schedule(callback: () => void): void {
15+
this.cancel();
16+
this.pendingDestroyTimer = setTimeout(() => {
17+
this.pendingDestroyTimer = null;
18+
callback();
19+
}, 0);
20+
}
21+
22+
/**
23+
* Cancel the pending destroy callback, if any.
24+
*/
25+
public cancel(): void {
26+
if (this.pendingDestroyTimer !== null) {
27+
clearTimeout(this.pendingDestroyTimer);
28+
this.pendingDestroyTimer = null;
29+
}
30+
}
31+
}

src/components/date-picker/flatpickr-adapter/flatpickr-adapter.tsx

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { QuarterPicker } from '../pickers/quarter-picker';
99
import { TimePicker } from '../pickers/time-picker';
1010
import { WeekPicker } from '../pickers/week-picker';
1111
import { YearPicker } from '../pickers/year-picker';
12+
import { DeferredDestroy } from './deferred-destroy';
1213

1314
/**
1415
* This component is internal and only supposed to be used by
@@ -71,6 +72,7 @@ export class DatePickerCalendar {
7172

7273
private picker: Picker;
7374
private flatPickrCreated: boolean = false;
75+
private deferredDestroy = new DeferredDestroy();
7476

7577
private container: HTMLElement;
7678

@@ -173,9 +175,15 @@ export class DatePickerCalendar {
173175
this.flatPickrCreated = true;
174176
}
175177

178+
public connectedCallback() {
179+
this.deferredDestroy.cancel();
180+
}
181+
176182
public disconnectedCallback() {
177-
this.picker.destroy();
178-
this.flatPickrCreated = false;
183+
this.deferredDestroy.schedule(() => {
184+
this.picker.destroy();
185+
this.flatPickrCreated = false;
186+
});
179187
}
180188

181189
public render() {

src/components/date-picker/pickers/picker.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,11 @@ export abstract class Picker {
6767
}
6868

6969
public setValue(value: Date) {
70-
this.flatpickr.setDate(value, false);
70+
this.flatpickr?.setDate(value, false);
7171
}
7272

7373
public redraw() {
74-
this.flatpickr.redraw();
74+
this.flatpickr?.redraw();
7575
}
7676

7777
public destroy() {
@@ -80,6 +80,7 @@ export abstract class Picker {
8080
}
8181

8282
this.flatpickr.destroy();
83+
this.flatpickr = null;
8384
}
8485

8586
public abstract getConfig(
@@ -139,6 +140,6 @@ export abstract class Picker {
139140
}
140141

141142
private handleOnClose() {
142-
this.flatpickr.element.focus();
143+
this.flatpickr?.element.focus();
143144
}
144145
}

0 commit comments

Comments
 (0)