-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Embeddables Rebuild] Fix unsaved changes on new dashboards bug #184955
[Embeddables Rebuild] Fix unsaved changes on new dashboards bug #184955
Conversation
/ci |
return (async () => { | ||
const { | ||
explicitInput: currentInput, | ||
componentState: { lastSavedInput }, | ||
} = this.getState(); | ||
getDashboardUnsavedChanges | ||
.bind(this)(lastSavedInput, currentInput) | ||
.then((unsavedChanges) => { | ||
if (observer.closed) return; | ||
observer.next(unsavedChanges); | ||
}); | ||
}); | ||
const unsavedChanges = await getDashboardUnsavedChanges.bind(this)( | ||
lastSavedInput, | ||
currentInput | ||
); | ||
return unsavedChanges; | ||
})(); |
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.
Switched to return a promise rather than an observable - this is a stylistic change and has nothing to do with the actual bug.
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.
Thanks for making this change. IMO it's much more obvious what's actually happening here when we use a promise
💚 Build Succeeded
Metrics [docs]Async chunks
To update your PR or re-run it, just comment with: |
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
LGTM! Code only review, as the fix makes sense.
Closes #184174
Summary
This PR fixes the bug where, on a new and unsaved dashboard, adding both a legacy embeddable and an image embeddable would result in a dashboard getting stuck with unsaved changes on the first save. This was caused by a race condition - since the
debounce
on the React embeddable unsaved changes observable was before theswitchMap
, the React embeddable observable fired immediately on save which then triggered thediffingSubscription
to fire too early.Before:
Screen.Recording.2024-06-06.at.11.04.04.AM.mov
After:
Screen.Recording.2024-06-06.at.10.59.05.AM.mov