Skip to content

Commit

Permalink
[Embeddable Rebuild] Fix stuck unsaved changes (#190165)
Browse files Browse the repository at this point in the history
Closes #190066

## Summary

In #189128 (comment),
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 🔥

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)
  • Loading branch information
Heenawter authored Aug 12, 2024
1 parent ce1bd65 commit 71b90a1
Showing 1 changed file with 0 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
combineLatestWith,
debounceTime,
map,
skip,
Subscription,
} from 'rxjs';
import {
Expand Down Expand Up @@ -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) => {
Expand Down

0 comments on commit 71b90a1

Please sign in to comment.