From 71b90a1af4df7ea2b3fb51244b4cf3ffec70d7a9 Mon Sep 17 00:00:00 2001
From: Hannah Mudge <Heenawter@users.noreply.github.com>
Date: Mon, 12 Aug 2024 10:31:34 -0600
Subject: [PATCH] [Embeddable Rebuild] Fix stuck unsaved changes (#190165)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes https://github.com/elastic/kibana/issues/190066

## Summary

In https://github.com/elastic/kibana/pull/189128#discussion_r1700724700,
I suggested adding a `skip` to the comparators subscription in order to
prevent `unsavedChanges` from emitting twice - however, while I was on
the right track with this suggestion, I didn't look closely enough at
how this worked. Essentially, this is how it worked without the skip
when creating a brand new React embeddable:

1. We initialize `unsavedChanges` by calling `runComparators` to get its
initial value - since it is a new embeddable and therefore **does not
have** unsaved state, it returns the entire state per the early return
in `runComparators`
2. The comparator subjects all emit and, after the debounce,
`unsavedChanges` gets set **again** by calling `runComparators` -
similarly, it returns the entire state due to the early return.
3. Unsaved changes emits twice unnecessarily - once at initialization,
once after a debounce :fire:

By adding the `skip` where I suggested, this is what happened:
1. We initialize `unsavedChanges` by calling `runComparators` to get its
initial value - it returns the entire state due to the early return.
2. We **ignore** the first emit of every comparator due to the `skip` -
this is good, because unsaved changes emits only once!
3. But, when we go to save the dashboard, the placement of the `skip`
made it so that the subscription **did not fire** when `lastSavedState`
emits - it is waiting for the comparators to emit again before firing!
So, the unsaved changes remains unnecessarily 🔥

Essentially, we want to `skip` the first emit of the **whole**
`comparator` + `lastSavedState` observable that results from the `pipe`
- **not** just the comparators. Otherwise, we don't get to the
`combineLatestWith` until **after** the comparators emit a second time,
which means we don't respond when`lastSavedState` emits after a React
embeddable panel is freshly added. By rearranging the order of
operations in the `pipe`, we achieve the desired behaviour.

To test the above, move the `skip` to **before** the `lastSavedState`
emit. Add a React embeddable panel, and try to save - unsaved changes
will be stuck. On a fresh dashboard, add a React embeddable panel again,
**edit** that React embeddable to trigger a comparator emit, **then**
save - unsaved changes is cleared!

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
---
 .../interfaces/unsaved_changes/initialize_unsaved_changes.ts    | 2 --
 1 file changed, 2 deletions(-)

diff --git a/packages/presentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts b/packages/presentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts
index 7f4770d39cd2d..aac397e85ca95 100644
--- a/packages/presentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts
+++ b/packages/presentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts
@@ -12,7 +12,6 @@ import {
   combineLatestWith,
   debounceTime,
   map,
-  skip,
   Subscription,
 } from 'rxjs';
 import {
@@ -74,7 +73,6 @@ export const initializeUnsavedChanges = <RuntimeState extends {} = {}>(
   subscriptions.push(
     combineLatest(comparatorSubjects)
       .pipe(
-        skip(1),
         debounceTime(COMPARATOR_SUBJECTS_DEBOUNCE),
         map((latestStates) =>
           comparatorKeys.reduce((acc, key, index) => {