[Discover] Add unsaved changes modal (#218500) by AlexGPlay · Pull Request #225252 · elastic/kibana

AlexGPlay · 2025-06-25T10:34:12Z

Summary

When there are unsaved changes in Discover a modal will prompt the user to either risk losing them or not leaving the page.

unsaved.changes.modal.mov

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

kertal · 2025-06-26T10:30:07Z

src/platform/plugins/shared/discover/public/application/main/discover_main_route.tsx

+      const tabs = runtimeStateManager.tabs.byId;
+      const hasAnyUnsavedTab = Object.values(tabs).some((tab) => {
+        const stateContainer = tab.stateContainer$.getValue();
+        if (!stateContainer) {
+          return false;
+        }
+
+        return stateContainer.savedSearchState.getHasChanged$().getValue();
+      });
+
+      if (!hasAnyUnsavedTab) return actions.default();
+
+      return actions.confirm(
+        i18n.translate('discover.confirmModal.confirmTextDescription', {
+          defaultMessage: 'Leaving Discover without saving may result in lost changes.',
+        }),
+        i18n.translate('discover.confirmModal.title', {
+          defaultMessage: 'Unsaved changes',
+        })
+      );
+    });


Thx @AlexGPlay , so this already works for tabs, which is great, but I guess this could also be tied to

TABS_ENABLED?

kibana/src/platform/plugins/shared/discover/public/application/main/discover_main_route.tsx

Line 34 in c348586

import { TABS_ENABLED } from '../../constants';

or, it should just be tied getHasChanged$()., but then it would work out of the box for Discover, something we could consider

interested in hearing your opinion @elastic/kibana-data-discovery 🙏

It looks like the current logic would work both when tabs are and aren't enabled, which is great. But whether or not we want to enable it before tabs are released is a separate product question. If we don't want to enable it until then, we could definitely throw in a TABS_ENABLED condition.

FYI @ninoslavmiskovic this is a feature of tabs, we could already release, warning the users that they might loose there changes when leaving Discover. With our "Single" tab mode, there's less to loose, but still. wdyt

a) Yes, let's use it already
b) No, let's use it when tabs is enabled
c) Let's discuss it on Monday, after the beach party with the flaky ones

a) it seems risk free and good ux.

Just so I understand this is only for dirty state of a Discover session ?

Not general ad-hoc data exploration? If yes then a) .

Just want to make sure we do not annoy existing scratch pad / ad hoc explores.

Great, let's make the final call in our Monday sync then. I won't be there, but I'm happy with whatever the team decides.

++ to the "only for dirty state" comment. I haven't tested locally to confirm it works that way, but that should be a requirement.

Two questions to consider since I won't be in Monday:

Should we offer a third button to quickly open the save modal and reduce friction, e.g. "Cancel" / "Save my session" / "Continue without saving"? Again, probably need copy input for title/body/buttons.

Should we offer a "Don't ask me again" checkbox for users who find it annoying / don't care, similar to the ES|QL -> classic mode modal?

Might even be worth referring to the ES|QL modal for inspiration:

@florent-leborgne good hint, thx 👍

So I guess it should work like this

@kertal I agree with following the EUI guidelines. In this case, we'd need to update the ES|QL and Lens modals as well. We'll also need to align in the color of the button and the use (or not) of icons. Also, in the screenshot from EUI, I would make Leave anyway a danger action.

I've been taking a look at this and there are a couple of things here:

With the current tools that onAppLeave provides we can't do the "Don't ask me again" checkbox because it provides very limited functionality, pretty much a yes/no question and that's it. Hooking up our own component here isn't doable right now either.

This confirmation modal kind of works the opposite as we want, it only allows for actions when the user stays in the page but no when leaving it. What I mean is that we can't have a Save changes that saves the changes and leaves the page. It's pretty much the opposite, like "Stay and save" or "Leave the page"

thank you @AlexGPlay , So think we need to decide the following due to the limitations:

Should we move forward with the given approach as a first step?

I think what we are discussing here needs a separate issue, will involve more teams, and it's something we should do, but currently, as @AlexGPlay mentions we are limited in this area. We can't provide a custom onAppLeave experience, which would in our case similar like when users want to open new discover sessions, and they are warned they have unsaved changes.

interested in hearing the teams opinion 🙏

Hmm yeah that definitely has an impact on our plans, thanks for looking into it @AlexGPlay. I agree for the scope of this PR we don't have much choice but to work with what's available to us for now. We can definitely follow up after and see if we can enhance this since I think there's value in offering more than what's possible currently, but even the more limited experience is an improvement on what we have today.

elasticmachine · 2025-07-24T06:09:44Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

davismcphee

Code changes look good overall and it worked well when testing! Just left a bit of feedback.

davismcphee · 2025-07-30T01:03:10Z

src/platform/plugins/shared/discover/public/application/main/discover_main_route.test.tsx

Thanks for updating these tests to RTL!

src/platform/plugins/shared/discover/public/application/main/discover_main_route.test.tsx

src/platform/plugins/shared/discover/public/application/main/discover_main_route.tsx

davismcphee · 2025-07-30T01:03:25Z

src/platform/plugins/shared/discover/public/application/main/discover_main_route.tsx

+      return actions.confirm(
+        i18n.translate('discover.confirmModal.confirmTextDescription', {
+          defaultMessage: 'Leaving Discover without saving may result in lost changes.',
+        }),
+        i18n.translate('discover.confirmModal.title', {
+          defaultMessage: 'Unsaved changes',
+        })
+      );
+    });


@florent-leborgne Could we get a quick copy check on this? We're a bit limited in what we can do with the modal technically since it's managed by code outside of Discover, basically a "yes, leave" or "no, stay" choice for the user. We can only customize the title, body, and confirm button text + colour.

The caveat is that they're not guaranteed to lose their changes (depends on if they navigate to another Discover link before returning), but there's a chance they will if they don't save. This is what it currently looks like:

And the Lens one for comparison:

I know there was some discussion about making broader changes to the various confirmation modals, but we're aiming to keep this PR scoped to just this Discover modal for now.

Could something like this work? We can tweak the body if what I wrote isn't exactly it, but I feel like saying just "may" is too vague

Thanks @florent-leborgne, I think that works for now and is an improvement over what we had before. There might be a bit more nuance to consider here, but not worth me holding up the PR over. @AlexGPlay Could you please implement these changes in the PR?

CC @ninoslavmiskovic for awareness. We may want to discuss more and clarify the messaging here, but we can merge this for now and do any necessary changes in a small followup PR if needed.

Updated - it looks like this

.../shared/discover/public/application/main/state_management/redux/mocks/runtime_state.mocks.ts

davismcphee

Apologies for the delay, and thanks for making the requested changes! Just some minor changes to the confirmation modal left, but otherwise LGTM so approving now 👍

davismcphee · 2025-08-05T01:52:25Z

src/platform/plugins/shared/discover/public/application/main/discover_main_route.tsx

+      return actions.confirm(
+        i18n.translate('discover.confirmModal.confirmTextDescription', {
+          defaultMessage: 'Leaving Discover without saving may result in lost changes.',
+        }),
+        i18n.translate('discover.confirmModal.title', {
+          defaultMessage: 'Unsaved changes',
+        })
+      );
+    });


Thanks @florent-leborgne, I think that works for now and is an improvement over what we had before. There might be a bit more nuance to consider here, but not worth me holding up the PR over. @AlexGPlay Could you please implement these changes in the PR?

CC @ninoslavmiskovic for awareness. We may want to discuss more and clarify the messaging here, but we can merge this for now and do any necessary changes in a small followup PR if needed.

davismcphee · 2025-08-05T02:02:25Z

src/platform/test/functional/apps/discover/group6/_unsaved_changes_modal.ts

Thanks for adding functional tests 👍

davismcphee · 2025-08-05T02:17:43Z

...red/discover/public/application/main/state_management/redux/__mocks__/runtime_state.mocks.ts

I won't hold up merging the PR over it, but I still don't like the idea of maintaining dedicated mocks for things when the real implementations are readily available, and in general would encourage us to use the real implementations whenever possible. It can help isolate the test logic, but pulls it further away from the actual runtime behaviour which is more important IMO.

…no-cache --fix'

elasticmachine · 2025-08-11T10:13:10Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a23c649

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #60 / Dataset Quality Dataset quality handles user privileges User can read logs-* User can monitor some data streams "before all" hook for "shows underprivileged warning when size cannot be accessed for some data streams"
[job] [logs] FTR Configs #38 / Search solution tests Search Home page Solution Nav - Search search home page with existing indices "before each" hook for "displays the feedbacks link"

Metrics [docs]

Async chunks

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

id	before	after	diff
`discover`	1.1MB	1.1MB	+693.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`discover`	28	29	+1

Page load bundle

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

id	before	after	diff
`discover`	22.8KB	22.8KB	+24.0B

History

💛 Build #324612 was flaky 17dccfb
💛 Build #321252 was flaky f2a3e69
💚 Build #316400 succeeded e3c24ad
💔 Build #316150 failed 3562306
💔 Build #315831 failed a32106f
💔 Build #313494 failed 43cad8b

cc @AlexGPlay

## Summary Closes elastic#218500 When there are unsaved changes in Discover a modal will prompt the user to either risk losing them or not leaving the page. https://github.com/user-attachments/assets/c516b3a7-0954-496b-8494-574ba8547d27 ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [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 - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

AlexGPlay self-assigned this Jun 25, 2025

kertal reviewed Jun 26, 2025

View reviewed changes

AlexGPlay force-pushed the 218500-unsaved-changes-modal branch from a32106f to 3562306 Compare July 7, 2025 06:57

AlexGPlay force-pushed the 218500-unsaved-changes-modal branch 2 times, most recently from 7cbe949 to f2a3e69 Compare July 21, 2025 06:38

AlexGPlay marked this pull request as ready for review July 21, 2025 13:21

AlexGPlay requested a review from a team as a code owner July 21, 2025 13:21

AlexGPlay added release_note:enhancement Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// labels Jul 24, 2025

davismcphee reviewed Jul 30, 2025

View reviewed changes

AlexGPlay force-pushed the 218500-unsaved-changes-modal branch from aaa0dab to 535000c Compare July 30, 2025 08:07

AlexGPlay requested a review from a team as a code owner July 30, 2025 08:07

AlexGPlay removed the request for review from a team July 30, 2025 08:44

kertal added the Feature:Discover Discover Application label Jul 31, 2025

davismcphee approved these changes Aug 5, 2025

View reviewed changes

davismcphee added the backport:skip This PR does not require backporting label Aug 5, 2025

AlexGPlay and others added 7 commits August 11, 2025 09:54

Add onAppLeave handler

a8f9624

Add mocks and tests

061be85

[CI] Auto-commit changed files from 'node scripts/yarn_deduplicate'

90690ee

Update typing

89a04fd

Fix some functional tests

726e96b

Fix more tests

8447811

Show modal only for saved sessions

1301547

AlexGPlay and others added 7 commits August 11, 2025 09:54

Revert functional tests

cde8cd8

Revert pending functional test

9970b70

Update tests

06f57a1

Fix mock type

eeafbc5

Update unit test

c45c95e

Add functional tests

336ee7a

[CI] Auto-commit changed files from 'node scripts/eslint_all_files --…

0fe671d

…no-cache --fix'

AlexGPlay force-pushed the 218500-unsaved-changes-modal branch from 17dccfb to 0fe671d Compare August 11, 2025 07:55

Update leave modal texts

a23c649

AlexGPlay merged commit 14828f6 into elastic:main Aug 11, 2025
12 checks passed

kibanamachine added the v9.2.0 label Aug 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Add unsaved changes modal (#218500)#225252

[Discover] Add unsaved changes modal (#218500)#225252
AlexGPlay merged 15 commits intoelastic:mainfrom
AlexGPlay:218500-unsaved-changes-modal

AlexGPlay commented Jun 25, 2025 •

edited

Loading

kertal Jun 26, 2025

davismcphee Jun 27, 2025

kertal Jun 27, 2025 •

edited

Loading

ninoslavmiskovic Jun 27, 2025

davismcphee Jun 27, 2025

kertal Jul 1, 2025

andreadelrio Jul 2, 2025 •

edited

Loading

AlexGPlay Jul 8, 2025

kertal Jul 9, 2025

davismcphee Jul 10, 2025

elasticmachine commented Jul 24, 2025

davismcphee left a comment

davismcphee Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

davismcphee Jul 30, 2025

florent-leborgne Jul 30, 2025

davismcphee Aug 5, 2025

AlexGPlay Aug 11, 2025

Uh oh!

davismcphee left a comment •

edited

Loading

davismcphee Aug 5, 2025

davismcphee Aug 5, 2025

davismcphee Aug 5, 2025

elasticmachine commented Aug 11, 2025

Uh oh!

Labels

8 participants

Conversation

AlexGPlay commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreadelrio Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 24, 2025

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

davismcphee left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 11, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

Public APIs missing exports

Page load bundle

History

Uh oh!

Labels

8 participants

AlexGPlay commented Jun 25, 2025 •

edited

Loading

kertal Jun 27, 2025 •

edited

Loading

andreadelrio Jul 2, 2025 •

edited

Loading

davismcphee left a comment •

edited

Loading