[Dashboard] Fix setState in embeddables#260082
Conversation
| for (const uuid of Object.keys(currentChildren)) { | ||
| if (layoutToApply.panels[uuid] || layoutToApply.pinnedPanels[uuid]) { | ||
| const child = currentChildren[uuid]; | ||
| if (apiPublishesUnsavedChanges(child)) child.resetUnsavedChanges(); |
There was a problem hiding this comment.
this line is a bug - instead of child.resetUnsavedChanges() we should set the state to the new state passed from the top.
04ef3df to
ce7ca20
Compare
...kages/shared/presentation/presentation_publishing/interfaces/has_embeddable_state_manager.ts
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
d1ba831 to
3d95df5
Compare
3d95df5 to
210852b
Compare
ThomThomson
left a comment
There was a problem hiding this comment.
Posting partial review. A few changes are still needed. This is looking directionally correct, and I will finish my review before EOD today.
| @@ -181,8 +181,8 @@ export const ControlGroupRenderer = ({ | |||
| }; | |||
| }); | |||
| lastSavedState$Ref.current.next(newState); | |||
There was a problem hiding this comment.
nit: I don't think you need this anymore. updateInput used to be a hack that would set the last saved state to a value just in order to reset to it. Now you can applySerializedState to that new state directly.
There was a problem hiding this comment.
Ok, noted — I’ve added a TODO to revisit this later.
As discussed asynchronously, my goal with this PR was to fix an existing bug and improve the structure, while avoiding triggering extensive cross-team reviews, since we need to merge it by tomorrow at the latest. This hasn't happen 😅 But as I mentioned, I’m happy to revisit it after FF.
For now, I’d prefer not to take on additional risk with this PR. I’m not sufficiently familiar with the controls to confidently sign off on this change without thorough testing, and I don’t have the time to do that properly at the moment, despite already working at more than full capacity.
There was a problem hiding this comment.
To me, putting off changing the code here actually increases the risk of this PR rather than decreasing it, as we'd be merging partial code without working through the full change set.
Judging by the tags, I'd assume you're aiming this for 9.4.0 feature freeze? If so, you have until April 7th. And this behaviour is fixing a bug anyway, so the feature freeze cutoff leaves some leeway.
There was a problem hiding this comment.
I guess a 'nit' note has confused me. Thank you for explanation, tested and corrected.
...tform/plugins/shared/embeddable/public/react_embeddable_system/react_embeddable_renderer.tsx
Outdated
Show resolved
Hide resolved
src/platform/plugins/shared/embeddable/public/react_embeddable_system/types.ts
Outdated
Show resolved
Hide resolved
ad2d25a to
213b5fa
Compare
8cbd1f2 to
d85cc39
Compare
src/platform/packages/shared/controls/control-group-renderer/src/control_group_renderer.tsx
Show resolved
Hide resolved
rmyz
left a comment
There was a problem hiding this comment.
obs presentation changes LGTM
...rvability/plugins/slo/public/embeddable/slo/burn_rate/burn_rate_react_embeddable_factory.tsx
Show resolved
Hide resolved
stratoula
left a comment
There was a problem hiding this comment.
ES|QL team changes LGTM, I did a very brief testing in ES|QL controls and they work fine!
.../plugins/shared/embeddable_alerts_table/public/factories/alerts_table_embeddable_factory.tsx
Show resolved
Hide resolved
.../plugins/shared/embeddable_alerts_table/public/factories/alerts_table_embeddable_factory.tsx
Show resolved
Hide resolved
…e_for_children6 * commit '3402744f63ca1196e97b11ffac4e7f7efab240df': (80 commits) [PerUserAuth] Add EARS auth type for Connectors V2 (elastic#253695) Fix `@elastic/eui/require-aria-label-for-modals` lint violations across `@elastic/kibana-core` files (elastic#259757) [Entity Analytics][Leads generation][4] Add API routes, LeadDataClient, and async generation (elastic#257046) [Agent Builder] Agent-centric UX redesign (elastic#258005) fix query streams failing test (elastic#260277) [Lens as code] Add list layout to the new API (elastic#259967) [FTR] Add warning comments to deployment-agnostic FTR base configs (elastic#260018) [Discover][Logs profile] Fix missing search highlights (elastic#260056) Plugin system: safe deletion (elastic#259038) [Infra] Fix Hosts filter options to match selected schema (elastic#259825) Manual Entity Resolution and flyout representation (elastic#260162) [Cascade] Handle grouping on fields with unset values (elastic#260033) [Fleet] generate OTel config for integration packages with otelcol inputs (elastic#259968) [Search] Switch over to V2 index management details (elastic#259866) [inference] increase timeout for ES inference calls (elastic#260382) [ES|QL] Enable subqueries (elastic#257455) [ES|QL] Change Point order free options (elastic#260282) [Auth] Added authentication strategy for UIAM OAuth (elastic#256182) [Security Solution] Add "alerts_candidate_count" rule execution metric (elastic#259917) [api-docs] 2026-03-31 Daily api_docs build (elastic#260380) ...
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
|
ThomThomson
left a comment
There was a problem hiding this comment.
Changes LGTM! Thank you for applying the comments in the previous review, and everything is looking good now from a code standpoint.
I will write a followup PR to rename some of the arguments for more clarity, but we should get this one merged ASAP. Sorry for the review delay.
Also tested this locally and ensured resetting works as expected
…heck * commit '6f040b29a5220ce12886a9731f656613e50aff06': (34 commits) [Entity Analytics] Add entity resolution UI to service flyout (elastic#260504) [Dashboard] Fix setState in embeddables (elastic#260082) [EDR Workflows] Unskip FTR tests that failed due to transient Fleet service unavailability (elastic#260519) [Observability:Streams] Fix query streams error handling test (elastic#260777) [Alerting v2] Dispatcher grouping modes, throttle strategies, and matcher autosuggestion (elastic#260249) [Dashboard] State extraction as a consistent override (elastic#259839) [Alerting v2] [Rule authoring] Fix rule name validation and error visibility in create/edit flow (elastic#260337) [Fix] re-introduce sln breadcrumbs to unified rules (elastic#260289) [Security Solution][Endpoint] Updated kibana docs to include `xpack.securitySolution.maxEndpointScriptFileSize` as configurable in cloud (elastic#260568) [Alerting v2] updated the alerting-v2-constants package with artifacts constants, fix to the runbook max characters (elastic#260342) [Automatic Import V2] Provide user tooltips (elastic#260725) [One Workflow] Deduplicate step types by base type in workflow list (elastic#260763) [Security Solution] Execution results UI: Enable the feature flag (elastic#260711) [Metrics][Discover] internal/search/esql_async returns 200 but METRICS_INFO responds with error (elastic#260746) Collapse redundant anyOf/oneOf array unions in OAS query params (elastic#260585) [Unified rules] Hide stack rules from global search (elastic#260088) [Agent Builder] Sidebar navigation updates (elastic#260728) [* As Code] Use PUT for upserts (elastic#260318) Update EUI to v114.0.0 (elastic#259497) [Entity Resolution] Add contextual-security-apps as co-owner of resolution paths (elastic#260659) ... # Conflicts: # src/platform/plugins/shared/dashboard/public/index.ts
#### TL;DR: The architecture changed from “children know how to reset themselves” to “parents own the target serialized state and tell children exactly which state to apply.” ## To test Please test your embeddables by: 1. Making changes to the internals of your embeddable state (NOT grid changes) 2. Verify "Unsaved changes" badge appears 3. refresh the page - verify the state is as expected (with unsaved changes on) 4. "reset changes" on Dashboard 5. Verify the state has been properly reverted to the last saved state ## Summary I changed the embeddable state model so the framework no longer treats "reset to last saved" as the primitive operation. The primitive is now "apply this serialized state snapshot". This fixes the bug where dashboard-driven state updates were incorrectly using child reset behavior and therefore restoring the last saved child state instead of the state requested by the parent. ## What Changed The main architectural change is that child embeddables now expose an explicit serialized-state application capability through `applySerializedState(state)`. Before this change, the dashboard reset/apply flow relied on `resetUnsavedChanges()`, which only means "go back to your own last saved baseline". That is the wrong abstraction when the parent already has a target state and wants the child to adopt that exact state. Now the contract is: - Parent serializes and owns the desired dashboard state snapshot. - Layout manager deserializes child state from that snapshot. - Each child that supports serialized state application receives its exact next state through `applySerializedState(...)`. - Dirty-state tracking remains separate from the question of what state should be applied. ## Contract Changes `PublishesUnsavedChanges` was simplified and now represents only: - `serializeState()` - `applySerializedState(...)` - `hasUnsavedChanges$` `resetUnsavedChanges()` was removed from the shared embeddable contract, because it was just convenience sugar and not the real framework primitive - so it's better not to have it, because otherwise we'll have to keep both in sync. ## Dashboard / Container Flow The dashboard container and layout manager were updated to use the new capability directly. The flow is now: 1. `dashboardApi.setState(...)` receives a full dashboard state snapshot. 2. `layoutManager` deserializes layout and per-panel child state. 6. For each existing child, if it supports serialized-state application, the layout manager calls `child.applySerializedState(nextChildState)`. 7. Children rehydrate themselves from the state provided by the parent. This is the key architectural fix: the container now pushes the intended state down, instead of asking children to infer it by resetting themselves. ## Future improvements: 1. Make serialized-state support more explicit in the type system. Right now PublishesUnsavedChanges implies state management capability; we could split “dirty tracking” from “serialized state manager” even more clearly if we want the contracts to stay very precise - but that's a bigger effort (a lot of reviewers too!) and I am happy to work later down the road. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
TL;DR:
The architecture changed from “children know how to reset themselves” to “parents own the target serialized state and tell children exactly which state to apply.”
To test
Please test your embeddables by:
Summary
I changed the embeddable state model so the framework no longer treats "reset to last saved" as the primitive operation. The primitive is now "apply this serialized state snapshot". This fixes the bug where dashboard-driven state updates were incorrectly using child reset behavior and therefore restoring the last saved child state instead of the state requested by the parent.
What Changed
The main architectural change is that child embeddables now expose an explicit serialized-state application capability through
applySerializedState(state). Before this change, the dashboard reset/apply flow relied onresetUnsavedChanges(), which only means "go back to your own last saved baseline". That is the wrong abstraction when the parent already has a target state and wants the child to adopt that exact state.Now the contract is:
applySerializedState(...).Contract Changes
PublishesUnsavedChangeswas simplified and now represents only:serializeState()applySerializedState(...)hasUnsavedChanges$resetUnsavedChanges()was removed from the shared embeddable contract, because it was just convenience sugar and not the real framework primitive - so it's better not to have it, because otherwise we'll have to keep both in sync.Dashboard / Container Flow
The dashboard container and layout manager were updated to use the new capability directly.
The flow is now:
dashboardApi.setState(...)receives a full dashboard state snapshot.layoutManagerdeserializes layout and per-panel child state.child.applySerializedState(nextChildState).This is the key architectural fix: the container now pushes the intended state down, instead of asking children to infer it by resetting themselves.
Future improvements: