-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(editor): Prevent unloading when changes are pending in new canvas (no-changelog) #10474
fix(editor): Prevent unloading when changes are pending in new canvas (no-changelog) #10474
Conversation
*/ | ||
|
||
function onBeforeUnload(e: BeforeUnloadEvent) { | ||
if (isDemoRoute.value || window.preventNodeViewBeforeUnload) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this preventNodeViewBeforeUnload
flag? I don't see it being set anywhere in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's used in the e2e tests to skip this prompt
@@ -1410,6 +1427,10 @@ onBeforeMount(() => { | |||
} | |||
}); | |||
|
|||
onActivated(() => { | |||
window.addEventListener('beforeunload', onBeforeUnload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we remove this listener also when this component is unmounted?
* Unload | ||
*/ | ||
|
||
function onBeforeUnload(e: BeforeUnloadEvent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we extract this stuff into a useBeforeUnload
composable, instead of duplicating this code?
…the-warning-when-trying-to-close-the-tab-and-there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one comment about documentation and a question
onUnload() { | ||
// This will fire if users decides to leave the page after prompted | ||
// Clear the interval to prevent the notification from being sent | ||
clearTimeout(this.unloadTimeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This timeout is no longer cleared. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's cleared but not set anywhere in the code. I assume it's leftover code from another past change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the feedback 💟 LGTM now 🚀
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
✅ All Cypress E2E specs passed |
Got released with |
* master: 🚀 Release 1.56.0 (#10502) fix(editor): Sending 'Assistant session started event' to posthog (no-changelog) (#10500) fix(core): Use class-validator with XSS check for survey answers (#10490) fix(editor): Stop telemetry from triggering when initializing workflow in new canvas (no-changelog) (#10492) fix(AI Transform Node): Remove prompt as it's already set in ASK AI endpoint (no-changelog) (#10496) fix(editor): Prevent unloading when changes are pending in new canvas (no-changelog) (#10474) feat(core): Upgrade axios to address CVE-2024-39338 (no-changelog) (#10494)
Summary
Screen.Recording.2024-08-19.at.14.42.22.mov
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/N8N-7587/weve-lost-the-warning-when-trying-to-close-the-tab-and-there-are
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)