fix: flatpickr popup closes immediately when options are passed inline - #262
Open
iahmedgamal wants to merge 1 commit into
Open
fix: flatpickr popup closes immediately when options are passed inline#262iahmedgamal wants to merge 1 commit into
iahmedgamal wants to merge 1 commit into
Conversation
- add regression test for passing options call the component once
There was a problem hiding this comment.
Pull request overview
Fixes a regression where re-rendering DateTimePicker with inline (non-memoized) options/callbacks could cause the flatpickr instance to be destroyed/recreated mid-interaction, closing the popup.
Changes:
- Make the flatpickr instance lifecycle mount-only by moving
mergedOptions,onCreate, andonDestroyinto refs and switching the create/destroy effect dependencies to[]. - Keep options/value synchronization via the existing update effect.
- Add a regression test that re-renders with inline
optionsand asserts the instance isn’t recreated (viaonCreatecall count).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/DateTimePicker.tsx | Keeps a single flatpickr instance for the component lifetime by using refs and a mount-only lifecycle effect. |
| test/index.spec.tsx | Adds a regression test ensuring inline options re-renders don’t recreate the flatpickr instance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
99
to
+102
| return () => { | ||
| destroyFlatpickrInstance(); | ||
| }; | ||
| }, [mergedOptions, onCreate, onDestroy]); | ||
| }, []); |
Collaborator
There was a problem hiding this comment.
@iahmedgamal i am concerned about this, there was a reason mergedOptions was a dependency
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.
Description
The lifecycle effect depended on
mergedOptions,onCreate, andonDestroy. When an inline prop changed, it created a new reference on every render, causing the effect to re-run and destroy/recreate the flatpickr instance mid-interaction.To fix this, the values were moved into refs that update on every render. The effect dependencies were changed to
[], so the instance now lives for the full component lifetime, regardless of prop memoization.Issue
closes #261
This fixes the problem where the flatpickr popup would close immediately after selecting a time or changing AM/PM when options were passed inline without memoization. The same issue occurred with inline
onCreateandonDestroycallbacks.Change Type
How Has This Been Tested?
A regression test was added to render the component with inline options, trigger a re-render, and check that
onCreatewas called exactly once, confirming the flatpickr instance was not destroyed and recreated. The fix was also tested locally.