clear modal stack synchronously to survive host unmount#202
Open
kashif-javaid-plt wants to merge 1 commit into
Open
clear modal stack synchronously to survive host unmount#202kashif-javaid-plt wants to merge 1 commit into
kashif-javaid-plt wants to merge 1 commit into
Conversation
fix: clear modal stack synchronously to survive host unmount
Body:
modalfy().closeAllModals() routed through queueClosingAction, which
only drains after the topmost <StackItem>'s exit animation finishes.
If the calling component unmounts the host right after (e.g. a
navigation transition), the queued action is dropped and openedItems
is never cleared — modals stay visible on the next screen.
Fix: call ModalState.closeAllModals() directly from modalfy() and
invoke the callback synchronously. The queued path still runs from
handleBackPress, which legitimately needs the animation.
Also fix a latent stale-closure bug in ModalProvider: the subscription
listener is defined outside useEffect but only subscribed once, so
setContextValue({ ...contextValue, ... }) could capture stale state
on back-to-back updates. Switch to a functional updater.
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
Fixes a race where
modalfy().closeAllModals()fails to clear the stack when thecalling component unmounts right after the call (e.g. navigating away on the same
tick). Modals stay visible on the next screen.
Root cause
modalfy().closeAllModalsrouted throughqueueClosingAction, which only drainsafter the topmost
<StackItem>'s exit animation completes. If theStackItemunmounts before the animation finishes, the queued action is dropped and
openedItemsis never cleared.Also fixes a latent stale-closure bug in
ModalProvider: the subscriptionlisteneris defined outside
useEffectbut only subscribed once on mount, sosetContextValue({ ...contextValue, ... })captured stalecontextValueandback-to-back state updates could silently revert to stale state.
Changes
src/lib/ModalState.ts—modalfy().closeAllModalsnow callsModalState.closeAllModals()directly and invokes the callback synchronously. Thequeued path still runs from
handleBackPress, which legitimately needs thesrc/lib/ModalProvider.tsx— switch the subscription listener to a functionalsetContextValue(prev => ...)updater so updates always compose against the lateststate.
Repro (before the fix)
modalfy().closeAllModals()and immediately navigate away (causing the hostcomponent to unmount synchronously).
After the fix, step 3 yields an empty stack as expected.
Notes for reviewers
src/was modified.lib/is regenerated byyarn prepare(bob build) atrelease time and is intentionally not in this diff.
yarn type,yarn lint, andyarn testall pass.modalfy().closeAllModals(callback)still accepts and callsthe callback — it just fires synchronously now instead of after an animation.