[TSVB] Fix missing unsaved changes on linked visualizations by markov00 · Pull Request #228685 · elastic/kibana

markov00 · 2025-07-18T23:57:53Z

Summary

This PR tries to apply a patch to a logic that is not very clear about what a serialized state is and how this is used to evaluate if a dashboard has unsaved changes or not.

The current implementation leverage the type SerializedPanelState that has a rawState property and a references array.

kibana/src/platform/packages/shared/presentation/presentation_publishing/interfaces/has_serializable_state.ts

Lines 12 to 19 in 2eb9972

    
           /** 
        
            * A package containing the serialized Embeddable state, with references extracted. When saving Embeddables using any 
        
            * strategy, this is the format that should be used. 
        
            */ 
        
           export interface SerializedPanelState<RawStateType extends object = object> { 
        
             references?: Reference[]; 
        
             rawState: RawStateType; 
        
           }

This type is the result of the call to the passed method called serializeState, an as described in the type, the references are extracted. To my understanding, extracted refers to the fact that the rawState should only have a link/name to an external reference, and should not contain any trace of the original referenced object id.

If this is the right way to understand that type definition, the associated method and the operation of reference extraction then I'm inclined to say that the current implementation of initializeUnsavedChanges doesn't take in consideration the references array in the equality check, because the only property passed and used with the comparators is the rawState as seen here:

kibana/src/platform/packages/shared/presentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts

Lines 48 to 59 in c9b441e

    
           const hasUnsavedChanges$ = anyStateChange$.pipe( 
        
             combineLatestWith( 
        
               parentApi.lastSavedStateForChild$(uuid).pipe(map((panelState) => panelState?.rawState)) 
        
             ), 
        
             debounceTime(UNSAVED_CHANGES_DEBOUNCE), 
        
             map(([, lastSavedState]) => { 
        
               const currentState = serializeState().rawState; 
        
               return !areComparatorsEqual( 
        
                 getComparators(), 
        
                 lastSavedState, 
        
                 currentState, 
        
                 defaultState,

This can create unwanted situation where a change to the used references can be undetected by the "unsaved changes" method. One example is coming from the linked issue where TSVB was extracting the references as requested, replacing, on the serializedState, the index_pattern: {id: "123abc"} with a index_pattern_ref_name: "metric_0_index_pattern" and moving the actual index pattern references in the array outside the rawState. This generates the issue where switching an index pattern doesn't result in an unsaved change detected at the dashboard level because both the current rawState and the latest saved rawState are in fact the same (but not he references array).

An AggBased visualization, that uses the same embeddable as TSVB, suffers from the same problem. If you the switch the data view the change is not picked up.

TSVB and AggBased issues are both regression coming from #215947 (I tested before and after that PR)

Lens probably has the same issue, but due to a wrong implementation, the rawState also has it's own copy of the references array that is replaced at the time of the references injection, but that is not removed at the time of the reference extractions (yes it is saved in the SO as part of the attributes and could differ from the dashboard reference array if a reference id has been changed in the dashboard). (I had also a discussion with @nickpeihl about this wrong behaviour).
So basically Lens, for a wrong implementation, doesn't suffer of this problem, but if we are going to implement the reference extraction correctly will suffer of this issue.

The implementation is simple, but can be definitely improved.

One thing that is not clear now is the following: The dashboard is saved after the first click to the Save button, but it will keep showing the usaved changes until:

you click save again
or you refresh the page
So basically the changes are correctly saved to ES but the latestSavedState is not updated immediately. I believe this is an issue with the current Spaghetti Observable used here.

fix #228203

Release Note

Fixes a bug that prevented saving linked TSVB visualizations when change data view.

nickofthyme · 2025-07-24T14:34:00Z

...resentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts

  getComparators: () => StateComparators<StateType>;
  defaultState?: Partial<StateType>;
  onReset: (lastSavedPanelState?: SerializedPanelState<StateType>) => MaybePromise<void>;
+  checkRefEquality?: boolean;


For some reason checking ref equality on links causes an erroneous diff where the lastSavedState.references is initially empty.

Not sure why this is but also to err on the side of caution, we made this ref equality check opt-in.

...resentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts

ThomThomson

The changes in this PR make sense, and solve the problem shown above, so approving to unblock. That said, in the near term we will need to re-think this as our intention is to remove references entirely from clientside code as part of the Dashboards as code project.

Because references are a saved object concern primarily, we will be injecting and extracting references as part of the out and in transforms respectively on the server.

I think the reason why Vis / TSVB needs to diff references specifically is because when it extracts the reference, it deletes the link to the ID. Theoretically, when this happens serverside instead, the lastSavedState should have the old dataViewId and the currentState should have the new dataViewId which should allow us to remove this logic.

...resentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts

- sort refs - add missing db index pattern ref

nickofthyme · 2025-07-30T17:42:51Z

Hey @ThomThomson Could you please review this again with the latest changes from #228685 (comment). Thanks!

ThomThomson

Approving as a bugfix. Reference comparison, equality and passing has historically been extremely ad-hoc and confusing so thank you for digging into this and finding a fix. I would prefer it if after extraction the visualize embeddable left some key in its state that showed the active data view Id - which would have prevented this problem - but I'm happy that the comparison logic added here is opt-in.

I very much hope that these problems are resolved once-and-for-all with our work to move them all serverside. And we should remember that this should be revisited, and the reference comparison logic added here should be removed once it is no longer needed.

FYI, work on serverside references has already started CC @nreese @nickpeihl.

ThomThomson · 2025-07-30T20:08:59Z

src/platform/test/functional/fixtures/kbn_archiver/dashboard/current/kibana.json

      "id": "50643b60-3dd3-11e8-b2b9-5d5dc1715159"
    },
+    {
+      "name":"ffc13252-56b4-4e3f-847e-61373fa0be86:kibanaSavedObjectMeta.searchSourceJSON.index",


Fixture update LGTM.

walterra · 2025-07-31T08:54:36Z

Tested this and was able to reproduce the original issue on main and can verify the fix makes "unsaved changes" show up on the dashboard and the "Save" button is enabled again. Things I noticed that are puzzling though:

If the dashboard is in the state described ("unsaved changes" + "Save"-button enabled), if I then do not hit "Save" but instead do a full page refresh, the dashboard still was saved and the cloned tsvb vis keeps the updated data view.
If I update the cloned chart a second time and change it back to the original same data view, if I then do "Save and return" on the vis, then there is no "unsaved changes" badge and the "Save" button is disabled. The dashboard got saved though and if I do a full page refresh the cloned charts keeps the original data view.

ThomThomson · 2025-07-31T18:27:19Z

@walterra I wonder if this behaviour is simply because the Dashboard backs up its unsaved state to the browser's session storage. Because of this when you refresh the page, you should see the exact same state as before the refresh. This does not mean that the Dashboard SO has been updated.

walterra · 2025-08-01T07:05:22Z

@ThomThomson you're right, I shared this with @markov00 on zoom yesterday and we were able to confirm that it ends up in session storage and gets restored, which is good! The problem is that the dashboard misses to restore the info that this state is unsaved and the badge stating this doesn't show up and the save button is disabled. We found out if you then navigate away from the dashboard and then come back the dashboards correctly will show the badge and enabled the save button. Here's a screencast that documents the issue:

dashboard-unsaves-changes-0001.mp4

kibanamachine · 2025-11-12T14:49:14Z