Skip to content

Control group state diffing#189128

Merged
nreese merged 51 commits intoelastic:mainfrom
nreese:control_group_state_diffing
Aug 2, 2024
Merged

Control group state diffing#189128
nreese merged 51 commits intoelastic:mainfrom
nreese:control_group_state_diffing

Conversation

@nreese
Copy link
Copy Markdown
Contributor

@nreese nreese commented Jul 24, 2024

Screenshot 2024-07-29 at 3 48 24 PM

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Jul 25, 2024

/ci

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Jul 25, 2024

@elasticmachine merge upstream

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Jul 26, 2024

/ci

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Jul 26, 2024

/ci

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Jul 29, 2024

@elasticmachine merge upstream

@nreese nreese requested a review from a team as a code owner July 30, 2024 20:18
@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Aug 1, 2024

@elasticmachine merge upstream

@nreese nreese requested a review from Heenawter August 1, 2024 21:39
Copy link
Copy Markdown
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one nit about range slider, but nothing blocking - woohoo! Once we add #189580, I think controls will be in a really good state 🤞

@kibana-ci
Copy link
Copy Markdown

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 640 642 +2
controls 202 204 +2
dashboard 616 618 +2
dashboardEnhanced 92 94 +2
dataVisualizer 799 801 +2
embeddable 140 141 +1
imageEmbeddable 137 139 +2
infra 1577 1579 +2
lens 1491 1493 +2
links 127 129 +2
ml 2024 2026 +2
presentationPanel 101 103 +2
slo 867 869 +2
synthetics 976 978 +2
visualizations 447 449 +2
total +29

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/presentation-containers 71 76 +5
embeddable 460 461 +1
total +6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 560.5KB 563.9KB +3.4KB
controls 202.5KB 203.0KB +515.0B
dashboard 532.9KB 533.4KB +502.0B
dataVisualizer 797.8KB 799.7KB +1.9KB
imageEmbeddable 67.0KB 68.9KB +1.9KB
lens 1.5MB 1.5MB +1.0B
links 51.7KB 52.2KB +516.0B
ml 4.6MB 4.6MB +6.3KB
slo 883.0KB 883.5KB +517.0B
synthetics 853.1KB 853.6KB +518.0B
total +16.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dashboardEnhanced 17.8KB 17.8KB -6.0B
embeddable 71.3KB 71.2KB -49.0B
infra 102.0KB 102.5KB +516.0B
lens 50.4KB 50.9KB +514.0B
presentationPanel 43.5KB 44.0KB +516.0B
total +1.5KB
Unknown metric groups

API count

id before after diff
@kbn/presentation-containers 82 88 +6
embeddable 570 571 +1
total +7

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @nreese

@nreese nreese merged commit cf1222f into elastic:main Aug 2, 2024
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Aug 2, 2024
Heenawter added a commit that referenced this pull request Aug 12, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Embedding Embedding content via iFrame project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants