Skip to content

Commit bb320db

Browse files
fix(app): confirm before discarding unsaved changes when closing filter editor (#3005)
## Summary In the Edit Dashboard Filters modal, clicking outside the modal or pressing ESC while editing a filter closed the editor immediately, silently discarding any pending form changes. This mirrors the fix already in place for the tile editor. The filter edit form (`DashboardFilterEditForm`) now prompts for confirmation before closing when the form has pending edits: - Uses `react-hook-form`'s `formState.isDirty` to detect pending changes — no confirm is shown for a clean form. - Reuses the shared `useConfirm` dialog ("You have unsaved changes. Discard them and close the editor?" / **Discard**), same pattern and copy as the tile editor. - Guards against re-entrancy (`isConfirmingRef`) so Mantine's focus management can't stack a second confirm dialog. - Sets the edit-form modal's `zIndex` via `useZIndex()` so the root confirm dialog reliably layers on top — the same z-index handling the tile editor uses. Without this the two default-z-index modals stacked unpredictably. The confirm applies to ESC, click-outside, and the modal's X. The in-form **Cancel** button (which returns to the filters list) is unchanged, since that is an explicit user action. ### Screenshots or video | After | | :---- | | [Confirmation dialog shown when closing the filter editor with unsaved changes](https://cursor.com/agents/bc-c7e04f97-bd8f-49d4-b41f-55bf763048a5/artifacts?path=%2Fopt%2Fcursor%2Fartifacts%2Ffilter_editor_discard_confirmation.png) | ### How to test on Vercel preview **Preview routes:** /dashboards **Steps:** 1. Open /dashboards and create a new dashboard. 2. Click the filter icon in the dashboard header (data-testid `edit-filters-button`) to open the filters editor. 3. Click "Add new filter" (data-testid `add-filter-button`). 4. Type any text into the "Display name" field (data-testid `filter-name-input`). 5. Press ESC. Verify a confirmation dialog appears reading "You have unsaved changes. Discard them and close the editor?" with Cancel and Discard buttons. 6. Click Cancel. Verify the filter form is still open with the typed name intact. 7. Press ESC again, then click Discard. Verify the editor closes and no filter was saved. ### References - Linear Issue: HDX-5167 - Related PRs: #2963 ### Testing - Added Playwright E2E cases in `dashboard.spec.ts`: one asserting the confirm-on-close (Cancel keeps edits, Discard closes) and one asserting a clean form closes without a prompt. Both pass in full-stack mode. - `tsc --noEmit` and `eslint` pass (no new warnings). <sub>To show artifacts inline, <a href="https://cursor.com/dashboard/cloud-agents#team-pull-requests">enable</a> in settings.</sub> Linear Issue: [HDX-5167](https://linear.app/clickhouse/issue/HDX-5167/add-confirmation-dialog-when-closing-filter-editing-modal) <div><a href="https://cursor.com/agents/bc-c7e04f97-bd8f-49d4-b41f-55bf763048a5?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a href="https://cursor.com/background-agent?bcId=bc-c7e04f97-bd8f-49d4-b41f-55bf763048a5&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div> Co-authored-by: Cursor Agent <199161495+cursoragent@users.noreply.github.com>
1 parent 8f3126f commit bb320db

4 files changed

Lines changed: 99 additions & 2 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@hyperdx/app': patch
3+
---
4+
5+
fix: Confirm before discarding unsaved changes when closing the dashboard filter editor

packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useEffect, useMemo, useState } from 'react';
1+
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
22
import { Controller, useForm, useWatch } from 'react-hook-form';
33
import { TableConnection } from '@hyperdx/common-utils/dist/core/metadata';
44
import {
@@ -38,7 +38,9 @@ import SourceSchemaPreview, {
3838
import { SourceSelectControlled } from '@/components/SourceSelect';
3939
import { SQLInlineEditorControlled } from '@/components/SQLEditor/SQLInlineEditor';
4040
import { useSource } from '@/source';
41+
import { useConfirm } from '@/useConfirm';
4142
import { getMetricTableName } from '@/utils';
43+
import { useZIndex } from '@/zIndex';
4244

4345
import { MODAL_SIZE } from './constants';
4446
import { CustomInputWrapper } from './CustomInputWrapper';
@@ -96,6 +98,38 @@ export const DashboardFilterEditForm = ({
9698
defaultValues: toFormValues(filter),
9799
});
98100

101+
const confirm = useConfirm();
102+
// Read during render so react-hook-form subscribes this component to it.
103+
const isDirty = formState.isDirty;
104+
105+
// Keep this modal below the root confirm dialog (Mantine's default 200) so
106+
// the "discard unsaved changes?" prompt stacks on top of it — mirrors the
107+
// tile editor's z-index handling.
108+
const modalZIndex = useZIndex() + 10;
109+
110+
// Guards against re-entrancy while the confirm dialog is open: Mantine's
111+
// focus management can re-fire the modal's onClose after the confirm modal
112+
// closes, which would otherwise stack a second dialog.
113+
const isConfirmingRef = useRef(false);
114+
115+
const handleClose = useCallback(() => {
116+
if (isConfirmingRef.current) return;
117+
if (!isDirty) {
118+
onClose();
119+
return;
120+
}
121+
isConfirmingRef.current = true;
122+
confirm(
123+
'You have unsaved changes. Discard them and close the editor?',
124+
'Discard',
125+
).then(ok => {
126+
isConfirmingRef.current = false;
127+
if (ok) {
128+
onClose();
129+
}
130+
});
131+
}, [confirm, isDirty, onClose]);
132+
99133
// Gates the auto-fill of Variable Name from Name below. Seeded true for a filter
100134
// that already has a stored name, because renaming an existing filter must not
101135
// silently break the tiles referencing its old token.
@@ -213,8 +247,9 @@ export const DashboardFilterEditForm = ({
213247
<Modal
214248
title={isNew ? 'Add filter' : 'Edit filter'}
215249
opened
216-
onClose={onClose}
250+
onClose={handleClose}
217251
size={MODAL_SIZE}
252+
zIndex={modalZIndex}
218253
>
219254
<form
220255
onSubmit={handleSubmit(values => {

packages/app/tests/e2e/features/dashboard.spec.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,45 @@ test.describe('Dashboard', { tag: ['@dashboard'] }, () => {
446446
await expect(dashboardPage.unsavedChangesConfirmModal).toBeHidden();
447447
});
448448

449+
test('should warn when closing filter editor with unsaved changes', async () => {
450+
await dashboardPage.createNewDashboard();
451+
await dashboardPage.openEditFiltersModal();
452+
await dashboardPage.openAddFilterForm();
453+
await expect(dashboardPage.getFilterForm()).toBeVisible();
454+
455+
await dashboardPage.fillFilterName('Unsaved filter');
456+
457+
await dashboardPage.page.keyboard.press('Escape');
458+
await expect(dashboardPage.unsavedChangesConfirmModal).toBeAttached({
459+
timeout: 5000,
460+
});
461+
462+
// Cancelling keeps the editor open with the pending edit intact.
463+
await dashboardPage.unsavedChangesConfirmCancelButton.click();
464+
await expect(dashboardPage.unsavedChangesConfirmModal).toBeHidden();
465+
await expect(dashboardPage.getFilterNameInput()).toHaveValue(
466+
'Unsaved filter',
467+
);
468+
469+
await dashboardPage.page.keyboard.press('Escape');
470+
await expect(dashboardPage.unsavedChangesConfirmModal).toBeAttached({
471+
timeout: 5000,
472+
});
473+
await dashboardPage.unsavedChangesConfirmDiscardButton.click();
474+
await expect(dashboardPage.getFilterForm()).toBeHidden({ timeout: 5000 });
475+
});
476+
477+
test('should close filter editor without confirm when there are no unsaved changes', async () => {
478+
await dashboardPage.createNewDashboard();
479+
await dashboardPage.openEditFiltersModal();
480+
await dashboardPage.openAddFilterForm();
481+
await expect(dashboardPage.getFilterForm()).toBeVisible();
482+
483+
await dashboardPage.page.keyboard.press('Escape');
484+
await expect(dashboardPage.getFilterForm()).toBeHidden({ timeout: 5000 });
485+
await expect(dashboardPage.unsavedChangesConfirmModal).toBeHidden();
486+
});
487+
449488
test('should create and populate filters', {}, async () => {
450489
test.setTimeout(30000);
451490

packages/app/tests/e2e/page-objects/DashboardPage.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -844,6 +844,24 @@ export class DashboardPage {
844844
await this.addFiltersButton.click();
845845
}
846846

847+
/** The filter edit form's display-name input. */
848+
getFilterNameInput(): Locator {
849+
return this.page.getByTestId('filter-name-input');
850+
}
851+
852+
/**
853+
* Fill only the filter's display name in the open edit form. Enough to mark
854+
* the react-hook-form dirty without completing the whole form.
855+
*/
856+
async fillFilterName(name: string) {
857+
await this.getFilterNameInput().fill(name);
858+
}
859+
860+
/** Open the edit form for an already-saved filter, without saving. */
861+
async openEditFilterForm(filterName: string) {
862+
await this.page.getByTestId(`edit-filter-button-${filterName}`).click();
863+
}
864+
847865
/** Pick the data source in the filter edit form. */
848866
async selectFilterSource(sourceName: string) {
849867
await this.filtersSourceSelector.click();

0 commit comments

Comments
 (0)