perf(date-picker): defer flatpickr destroy to survive portal-driven DOM moves#4000
Merged
perf(date-picker): defer flatpickr destroy to survive portal-driven DOM moves#4000
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new DeferredDestroy utility and tests; integrates it into DatePickerCalendar to defer picker teardown on disconnect and cancel on reconnect. Also adds defensive guards and nulling in Picker methods and reorders destroy/unbind steps in Month/Quarter/Year picker subclasses. Changes
Sequence DiagramsequenceDiagram
participant DOM as DOM Lifecycle
participant Calendar as DatePickerCalendar
participant Deferred as DeferredDestroy
participant Picker as Picker
DOM->>Calendar: disconnectedCallback()
Calendar->>Deferred: schedule(destroyCallback)
Deferred->>Deferred: setTimeout(callback, 0)
Note over Deferred: Destruction pending...
alt Element reconnected before timeout
DOM->>Calendar: connectedCallback()
Calendar->>Deferred: cancel()
Deferred->>Deferred: clear pending timeout
Note over Calendar: Destruction prevented
else Timeout fires
Deferred->>Picker: invoke destroyCallback()
Picker->>Picker: remove listeners / clean up
Picker->>Picker: flatpickr = null
Note over Calendar: Destruction completed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4000/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
src/components/date-picker/pickers/picker.ts (1)
77-84:⚠️ Potential issue | 🟠 Major
destroy()now breaks subclass cleanup paths.Line 83 nulls
this.flatpickrimmediately, butsrc/components/date-picker/pickers/year-picker.tsx(Line 41-53) performs additional listener cleanup aftersuper.destroy(). That cleanup now becomes a no-op and can leave custom handlers unremoved.Suggested fix (preserve subclass teardown hook before nulling)
export abstract class Picker { + protected beforeDestroy(_instance: flatpickr.Instance): void { + // Subclasses can override for custom cleanup. + } + public destroy() { if (!this.flatpickr) { return; } - this.flatpickr.destroy(); - this.flatpickr = null; + const instance = this.flatpickr; + this.beforeDestroy(instance); + instance.destroy(); + this.flatpickr = null; } }Then move YearPicker-specific listener removal into
beforeDestroy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/date-picker/pickers/picker.ts` around lines 77 - 84, The current destroy() implementation nulls this.flatpickr immediately which prevents subclass cleanup (e.g., YearPicker in year-picker.tsx that calls super.destroy() and then removes listeners); modify the base class (picker.ts) so destroy() calls a protected hook method (e.g., beforeDestroy()) while this.flatpickr is still intact, then perform this.flatpickr.destroy() and only null this.flatpickr after calling beforeDestroy(); update YearPicker to move its listener removal logic from after super.destroy() into an override of beforeDestroy() so its cleanup runs while flatpickr is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/date-picker/pickers/picker.ts`:
- Around line 77-84: The current destroy() implementation nulls this.flatpickr
immediately which prevents subclass cleanup (e.g., YearPicker in year-picker.tsx
that calls super.destroy() and then removes listeners); modify the base class
(picker.ts) so destroy() calls a protected hook method (e.g., beforeDestroy())
while this.flatpickr is still intact, then perform this.flatpickr.destroy() and
only null this.flatpickr after calling beforeDestroy(); update YearPicker to
move its listener removal logic from after super.destroy() into an override of
beforeDestroy() so its cleanup runs while flatpickr is available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67ec2bd7-6889-4f90-87aa-3d89da7ef28a
📒 Files selected for processing (4)
src/components/date-picker/flatpickr-adapter/deferred-destroy.spec.tssrc/components/date-picker/flatpickr-adapter/deferred-destroy.tssrc/components/date-picker/flatpickr-adapter/flatpickr-adapter.tsxsrc/components/date-picker/pickers/picker.ts
jgroth
force-pushed
the
854-fix-flatpickr-memory-leak
branch
from
37895a5 to
a8dd181
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/date-picker/flatpickr-adapter/flatpickr-adapter.tsx`:
- Line 75: The field deferredDestroy is initialized once and never reassigned;
mark it readonly to reflect immutability. Update the class field declaration for
deferredDestroy (the instance of DeferredDestroy) to be readonly so that the
compiler enforces it cannot be reassigned while preserving current
initialization and usage in the class.
🪄 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: 820c9b3d-e381-4b3e-b82d-b1c49207a171
📒 Files selected for processing (7)
src/components/date-picker/flatpickr-adapter/deferred-destroy.spec.tssrc/components/date-picker/flatpickr-adapter/deferred-destroy.tssrc/components/date-picker/flatpickr-adapter/flatpickr-adapter.tsxsrc/components/date-picker/pickers/month-picker.tsxsrc/components/date-picker/pickers/picker.tssrc/components/date-picker/pickers/quarter-picker.tsxsrc/components/date-picker/pickers/year-picker.tsx
…OM 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>
Kiarokh
force-pushed
the
854-fix-flatpickr-memory-leak
branch
from
a8dd181 to
01a0d84
Compare
Contributor
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)
src/components/date-picker/pickers/picker.ts (1)
23-23:⚠️ Potential issue | 🟠 MajorChange field type to
flatpickr.Instance | nullto match runtime behavior.Line 23 declares
flatpickras non-nullable, but Line 83 assignsnullin thedestroy()method. The type declaration should reflect this:protected flatpickr: flatpickr.Instance | null;This aligns with the code's actual usage patterns, where optional chaining (
?.) is employed in methods likesetValue()andredraw(), and subclasses conditionally access the field after potential destruction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/date-picker/pickers/picker.ts` at line 23, The flatpickr field is declared as non-nullable but assigned null in destroy(), so change the declaration of the protected field flatpickr on the Picker class from flatpickr.Instance to flatpickr.Instance | null to match runtime behavior; update the type for the field declaration (referencing the protected flatpickr property) so usages in destroy(), setValue(), redraw(), and any subclass access are type-correct without needing unsafe non-null assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/date-picker/flatpickr-adapter/deferred-destroy.spec.ts`:
- Line 22: Rename the test title to use "timer tick" or "macrotask" instead of
"microtask": update the test whose description currently reads "schedule calls
the callback after the microtask completes" to something like "schedule calls
the callback after the timer tick completes" (or "after the macrotask
completes") so it accurately reflects that setTimeout(..., 0) runs on the
timer/macrotask queue; keep the rest of the test (the schedule invocation and
assertions in deferred-destroy.spec.ts) unchanged and only change the string
passed to test(...).
---
Outside diff comments:
In `@src/components/date-picker/pickers/picker.ts`:
- Line 23: The flatpickr field is declared as non-nullable but assigned null in
destroy(), so change the declaration of the protected field flatpickr on the
Picker class from flatpickr.Instance to flatpickr.Instance | null to match
runtime behavior; update the type for the field declaration (referencing the
protected flatpickr property) so usages in destroy(), setValue(), redraw(), and
any subclass access are type-correct without needing unsafe non-null assertions.
🪄 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: 0579ce97-8d59-45f1-b010-8f4a1b63395c
📒 Files selected for processing (7)
src/components/date-picker/flatpickr-adapter/deferred-destroy.spec.tssrc/components/date-picker/flatpickr-adapter/deferred-destroy.tssrc/components/date-picker/flatpickr-adapter/flatpickr-adapter.tsxsrc/components/date-picker/pickers/month-picker.tsxsrc/components/date-picker/pickers/picker.tssrc/components/date-picker/pickers/quarter-picker.tsxsrc/components/date-picker/pickers/year-picker.tsx
| expect(callback).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('schedule calls the callback after the microtask completes', () => { |
Contributor
There was a problem hiding this comment.
Use “timer tick/macrotask” wording instead of “microtask” in the test name.
Line 22 currently describes microtask timing, but setTimeout(..., 0) executes on the timer queue.
Suggested wording update
-test('schedule calls the callback after the microtask completes', () => {
+test('schedule calls the callback on the next timer tick', () => {📝 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
| test('schedule calls the callback after the microtask completes', () => { | |
| test('schedule calls the callback on the next timer tick', () => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/date-picker/flatpickr-adapter/deferred-destroy.spec.ts` at
line 22, Rename the test title to use "timer tick" or "macrotask" instead of
"microtask": update the test whose description currently reads "schedule calls
the callback after the microtask completes" to something like "schedule calls
the callback after the timer tick completes" (or "after the macrotask
completes") so it accurately reflects that setTimeout(..., 0) runs on the
timer/macrotask queue; keep the rest of the test (the schedule invocation and
assertions in deferred-destroy.spec.ts) unchanged and only change the string
passed to test(...).
Kiarokh
approved these changes
Apr 8, 2026
Contributor
Kiarokh
left a comment
Kiarokh
left a comment
There was a problem hiding this comment.
testing and things are working fine.
AI reviews just pointed out ignorable nipicks in the tests
Collaborator
|
🎉 This PR is included in version 39.11.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
3 participants
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
New Features
Bug Fixes
Tests
Review:
Browsers tested:
Windows:
Linux:
macOS:
Mobile:
Closes Lundalogik/crm-client#854