fix: resolve swal promise when its dismissed by another swal #2689
Conversation
WalkthroughThe recent updates to the codebase focus on ensuring that promises from SweetAlert2 dialogs are properly resolved or rejected when a dialog is interrupted by another, such as a toast notification. This change addresses a specific issue where promises were left hanging in such scenarios, leading to potential bugs in applications using SweetAlert2. Changes
Assessment against linked issues
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .gitignore (1 hunks)
- cypress/e2e/tests.cy.js (1 hunks)
- src/SweetAlert.js (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments: 2
cypress/e2e/tests.cy.js (2)
489-497: This test case checks if a SweetAlert modal is dismissed by another SweetAlert modal, the promise should resolve with
{ isDismissed: true }. The test case is correctly structured to simulate the scenario and validate the expected outcome. However, it's important to ensure that the dismissal of the first modal by the second modal is intentional and not due to a race condition or a bug in the implementation.499-511: This test case is designed to ensure that if a SweetAlert modal is dismissed by another modal after
clickConfirm()is called, the promise resolves with the expected values indicating that the modal was confirmed. It's crucial to verify that the behavior ofclickConfirm()followed by another modal invocation is consistent with the intended logic and that there are no side effects or race conditions.
| showWarningsForParams(Object.assign({}, mixinParams, userParams)) | ||
|
|
||
| if (globalState.currentInstance) { | ||
| const swalPromiseResolve = privateMethods.swalPromiseResolve.get(globalState.currentInstance) | ||
| const { isAwaitingPromise } = globalState.currentInstance | ||
| globalState.currentInstance._destroy() | ||
| if (!isAwaitingPromise) { | ||
| swalPromiseResolve({ isDismissed: true }) | ||
| } | ||
| if (dom.isModal()) { | ||
| unsetAriaHidden() | ||
| } |
There was a problem hiding this comment.
The changes here address the issue described in the pull request summary. The logic checks if there is an existing currentInstance and if it is not awaiting a promise, it resolves the promise with { isDismissed: true }. This ensures that the promise is resolved when a modal is dismissed by another modal. Additionally, the check for dom.isModal() and the call to unsetAriaHidden() are important for accessibility reasons, ensuring that screen readers can correctly interpret the state of the application when modals are dismissed.
However, there is a potential issue with the current implementation. The unsetAriaHidden() function is called without checking if the current modal is the one being dismissed. If there are multiple modals, this could lead to incorrect behavior where aria-hidden attributes are removed from elements that are not currently visible. This should be verified and tested to ensure that it behaves as expected in scenarios with multiple modals.
- if (dom.isModal()) {
+ if (dom.isModal() && !isAwaitingPromise) {
unsetAriaHidden()
}By adding the !isAwaitingPromise condition, we ensure that unsetAriaHidden() is only called when the current modal is actually being dismissed and not awaiting another promise.
Commitable 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.
| showWarningsForParams(Object.assign({}, mixinParams, userParams)) | |
| if (globalState.currentInstance) { | |
| const swalPromiseResolve = privateMethods.swalPromiseResolve.get(globalState.currentInstance) | |
| const { isAwaitingPromise } = globalState.currentInstance | |
| globalState.currentInstance._destroy() | |
| if (!isAwaitingPromise) { | |
| swalPromiseResolve({ isDismissed: true }) | |
| } | |
| if (dom.isModal()) { | |
| unsetAriaHidden() | |
| } | |
| showWarningsForParams(Object.assign({}, mixinParams, userParams)) | |
| if (globalState.currentInstance) { | |
| const swalPromiseResolve = privateMethods.swalPromiseResolve.get(globalState.currentInstance) | |
| const { isAwaitingPromise } = globalState.currentInstance | |
| globalState.currentInstance._destroy() | |
| if (!isAwaitingPromise) { | |
| swalPromiseResolve({ isDismissed: true }) | |
| } | |
| if (dom.isModal() && !isAwaitingPromise) { | |
| unsetAriaHidden() | |
| } |
limonte
deleted the
fix/resolve-swal-promise-when-its-dismissed-by-other-swal
branch
## [11.10.1](v11.10.0...v11.10.1) (2023-11-19) ### Bug Fixes * resolve swal promise when its dismissed by another swal ([#2689](#2689)) ([0b4329b](0b4329b))
|
🎉 This PR is included in version 11.10.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
For those looking to detect the dismissal of a Swal modal by another modal within the promise, I've found a solution that works well. I use the following check: |
closes #2681
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
.gitignoreto exclude thecypress/downloadsdirectory, keeping the repository clean from test artifacts.