[Modal] Dedup Ongoing Dialogs for Same Fragment #2594
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2594 +/- ##
=======================================
Coverage 95.86% 95.86%
=======================================
Files 176 176
Lines 46211 46215 +4
=======================================
+ Hits 44300 44304 +4
Misses 1911 1911 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Okan Sahin <39759830+mokimo@users.noreply.github.com>
Something else came up yesterday while I was reading the PR description 🥲 so I didn't get to finish it... I planned to record a video to reproduce the issue, but if @mokimo agrees that it's trivial enough, I am happy to not do that 🥺 I am going to open it up and ask for more approvals. Thanks, Okan! |
JingleH
requested review from
vhargrave,
chrischrischris,
mirafedas and
JasonHowellSlavin
|
Reminder to set the |
| @@ -246,6 +249,7 @@ export function delayedModal(el) { | |||
| export default function init(el) { | |||
| const { modalHash } = el.dataset; | |||
| if (delayedModal(el) || window.location.hash !== modalHash || document.querySelector(`div.dialog-modal${modalHash}`)) return null; | |||
| if (dialogLoadingSet.has(modalHash?.replace('#', ''))) return null; // prevent duplicate modal loading | |||
There was a problem hiding this comment.
nit: to save some bytes you could add this as an or || to the upper condition. That way we only have code to return null once if any of the conditions are met.
There exists a race condition that when the same modal hashes appear multiple times on the page, the same fragment could be loaded more than once. More specifically, after the hash change, there is a period when getModal() needs to fetch its content but the dialog-modal hasn't been appended to document yet. If another modal on the page gets its init() called (via section preloadLinks) during this period, all the early-exit safeguard in init() we currently have will fail. This PR aims to cover this condition by keeping track of what dialogs are being loaded but not yet appended.
How to reproduce:
Locally or using overrides, in modal.js, you can fake a longer getPathModal() call by making it into something like
await Promise.all([getPathModal(details.path, dialog), new Promise((r) => setTimeout(r, 4000))]);. Then to delay the second modal, either throw in more content, or just manually make it slower by making init() async and addingif (document.querySelector('a.modal') !== el) { await new Promise((r) => setTimeout(r, 2000))}to the beginning. Then clicking the primary CTA should make the FaaS fragment start overwriting itself and acting weird.Resolves: https://jira.corp.adobe.com/browse/MWPW-147304
Test URLs: