Skip to content

Commit 514d005

Browse files
authored
feat(workspaces): change tab behavior to prevent modified tabs from being closed by accident COMPASS-5022 (#5952)
* feat(workspaces): add workspace helper methods to allow controlling tab closing flow; apply initial logic for preventing tabs from closing by accident * chore: refactor to canClose / canReplace sync handler response * fix(workspaces): always open new tab when explicitly required * chore: separate single handler to two distinct ones for close and replace * chore: lint fixes * chore(workspaces): clean-up store and add new unit tests * chore: add e2e tests for tab behavior * chore: don't click the query bar to not trigger AI feature by accident * chore: small text fixes
1 parent efedaa9 commit 514d005

File tree

13 files changed

+746
-223
lines changed

13 files changed

+746
-223
lines changed

packages/compass-aggregations/src/stores/store.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,14 @@ export function activateAggregationsPlugin(
262262
maxTimeMSChanged(preferences.getPreferences().maxTimeMS || null)
263263
);
264264

265+
const onCloseOrReplace = () => {
266+
return !store.getState().isModified;
267+
};
268+
269+
addCleanup(workspaces.onTabReplace?.(onCloseOrReplace));
270+
271+
addCleanup(workspaces.onTabClose?.(onCloseOrReplace));
272+
265273
return {
266274
store,
267275
deactivate: cleanup,

packages/compass-e2e-tests/helpers/commands/close-workspace-tabs.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import type { CompassBrowser } from '../compass-browser';
22
import * as Selectors from '../selectors';
33

44
export async function closeWorkspaceTabs(
5-
browser: CompassBrowser
5+
browser: CompassBrowser,
6+
autoConfirmTabClose = true
67
): Promise<void> {
78
const countTabs = async () => {
89
return (await browser.$$(Selectors.workspaceTab(null))).length;
@@ -15,6 +16,14 @@ export async function closeWorkspaceTabs(
1516
await currentActiveTab.click();
1617
await browser.waitUntil(async () => {
1718
await currentActiveTab.$(Selectors.CloseWorkspaceTab).click();
19+
if (autoConfirmTabClose) {
20+
// Tabs in "dirty" state can't be closed without confirmation
21+
if (await browser.$(Selectors.ConfirmTabCloseModal).isExisting()) {
22+
await browser.clickVisible(
23+
browser.$(Selectors.ConfirmTabCloseModal).$('button=Close tab')
24+
);
25+
}
26+
}
1827
return (await currentActiveTab.isExisting()) === false;
1928
});
2029
}

packages/compass-e2e-tests/helpers/commands/collection-workspaces.ts

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,21 @@ async function navigateToCollection(
55
browser: CompassBrowser,
66
// TODO(COMPASS-8002): take connectionName into account
77
dbName: string,
8-
collectionName: string
8+
collectionName: string,
9+
10+
// Close all the workspace tabs to get rid of all the state we
11+
// might have accumulated. This is the only way to get back to the zero
12+
// state of Schema, and Validation tabs without re-connecting.
13+
closeExistingTabs = true
914
): Promise<void> {
1015
const collectionSelector = Selectors.sidebarCollection(
1116
dbName,
1217
collectionName
1318
);
1419

15-
// Close all the workspace tabs to get rid of all the state we
16-
// might have accumulated. This is the only way to get back to the zero
17-
// state of Schema, and Validation tabs without re-connecting.
18-
await browser.closeWorkspaceTabs();
20+
if (closeExistingTabs) {
21+
await browser.closeWorkspaceTabs();
22+
}
1923

2024
// search for the collection and wait for the collection to be there and visible
2125
await browser.clickVisible(Selectors.SidebarFilterInput);
@@ -39,9 +43,15 @@ export async function navigateToCollectionTab(
3943
| 'Aggregations'
4044
| 'Schema'
4145
| 'Indexes'
42-
| 'Validation' = 'Documents'
46+
| 'Validation' = 'Documents',
47+
closeExistingTabs = true
4348
): Promise<void> {
44-
await navigateToCollection(browser, dbName, collectionName);
49+
await navigateToCollection(
50+
browser,
51+
dbName,
52+
collectionName,
53+
closeExistingTabs
54+
);
4555
await navigateWithinCurrentCollectionTabs(browser, tabName);
4656
}
4757

packages/compass-e2e-tests/helpers/selectors.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1160,6 +1160,8 @@ export const sidebarInstanceNavigationItem = (
11601160
return `${Sidebar} [aria-label="${tabName}"]`;
11611161
};
11621162
export const SidebarMyQueriesTab = `${Sidebar} [aria-label="My Queries"]`;
1163+
export const WorkspaceTab =
1164+
'[role="tablist"][aria-label="Workspace Tabs"] [role="tab"]';
11631165
export const workspaceTab = (
11641166
title: string | null,
11651167
active: boolean | null = null
@@ -1171,7 +1173,7 @@ export const workspaceTab = (
11711173
: ['My Queries', 'Performance', 'Databases'].includes(title)
11721174
? `[title="${title}"]`
11731175
: `[data-namespace="${title}"]`;
1174-
return `[role="tablist"][aria-label="Workspace Tabs"] [role="tab"]${_title}${_active}`;
1176+
return `${WorkspaceTab}${_title}${_active}`;
11751177
};
11761178
export const instanceWorkspaceTab = (
11771179
tabName: 'Performance' | 'Databases',
@@ -1303,3 +1305,6 @@ export const AgreeAndContinueButton = 'button=Agree and continue';
13031305

13041306
// Any toast
13051307
export const AnyToastDismissButton = '[data-testid="lg-toast-dismiss-button"]';
1308+
1309+
// Close tab confirmation
1310+
export const ConfirmTabCloseModal = '[data-testid="confirm-tab-close"]';
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
import type { CompassBrowser } from '../helpers/compass-browser';
2+
import { init, cleanup, screenshotIfFailed } from '../helpers/compass';
3+
import type { Compass } from '../helpers/compass';
4+
import * as Selectors from '../helpers/selectors';
5+
import { createNumbersCollection } from '../helpers/insert-data';
6+
import { expect } from 'chai';
7+
8+
describe('Global Tabs', function () {
9+
let compass: Compass;
10+
let browser: CompassBrowser;
11+
12+
const collections = ['a', 'b', 'c'];
13+
14+
before(async function () {
15+
compass = await init(this.test?.fullTitle());
16+
browser = compass.browser;
17+
});
18+
19+
beforeEach(async function () {
20+
for (const collName of collections) {
21+
await createNumbersCollection(collName, 1);
22+
await createNumbersCollection(collName, 1);
23+
await createNumbersCollection(collName, 1);
24+
}
25+
await browser.connectWithConnectionString();
26+
});
27+
28+
afterEach(async function () {
29+
await screenshotIfFailed(compass, this.currentTest);
30+
});
31+
32+
after(async function () {
33+
await cleanup(compass);
34+
});
35+
36+
it('should open tabs over each other when not modified', async function () {
37+
for (const collName of collections) {
38+
await browser.navigateToCollectionTab(
39+
'test',
40+
collName,
41+
'Documents',
42+
false
43+
);
44+
}
45+
expect(await browser.$$(Selectors.workspaceTab(null))).to.have.lengthOf(1);
46+
});
47+
48+
it('should open tabs over each other when not modified', async function () {
49+
for (const collName of collections) {
50+
await browser.navigateToCollectionTab(
51+
'test',
52+
collName,
53+
'Documents',
54+
false
55+
);
56+
// Click something to make sure we "modified" the tab
57+
await browser.clickVisible(
58+
Selectors.queryBarApplyFilterButton('Documents')
59+
);
60+
}
61+
expect(await browser.$$(Selectors.workspaceTab(null))).to.have.lengthOf(3);
62+
});
63+
64+
it('should close tabs without warning even when "modified" by interacting with the tab', async function () {
65+
for (const collName of collections) {
66+
await browser.navigateToCollectionTab(
67+
'test',
68+
collName,
69+
'Documents',
70+
false
71+
);
72+
// Click something to make sure we "modified" the tab
73+
await browser.clickVisible(
74+
Selectors.queryBarApplyFilterButton('Documents')
75+
);
76+
}
77+
await browser.closeWorkspaceTabs(false);
78+
expect(await browser.$$(Selectors.workspaceTab(null))).to.have.lengthOf(0);
79+
});
80+
81+
it('should ask for confirmation when closing modified Aggregations tab', async function () {
82+
await browser.navigateToCollectionTab('test', 'a', 'Aggregations');
83+
84+
await browser.clickVisible(
85+
Selectors.aggregationPipelineModeToggle('as-text')
86+
);
87+
88+
await browser.setCodemirrorEditorValue(
89+
Selectors.AggregationAsTextEditor,
90+
'[{$match: { i: 0 }}]'
91+
);
92+
93+
await browser.clickVisible(Selectors.CloseWorkspaceTab);
94+
await browser.$(Selectors.ConfirmTabCloseModal).waitForDisplayed();
95+
96+
await browser.clickVisible(
97+
browser.$(Selectors.ConfirmTabCloseModal).$('button=Cancel')
98+
);
99+
await browser
100+
.$(Selectors.ConfirmTabCloseModal)
101+
.waitForExist({ reverse: true });
102+
103+
// Checking first that cancel leaves the tab on the screen
104+
expect(await browser.$$(Selectors.workspaceTab(null))).to.have.lengthOf(1);
105+
106+
await browser.clickVisible(Selectors.CloseWorkspaceTab);
107+
await browser.$(Selectors.ConfirmTabCloseModal).waitForDisplayed();
108+
109+
await browser.clickVisible(
110+
browser.$(Selectors.ConfirmTabCloseModal).$('button=Close tab')
111+
);
112+
await browser
113+
.$(Selectors.ConfirmTabCloseModal)
114+
.waitForExist({ reverse: true });
115+
116+
// When confirmed, should remove the tab
117+
expect(await browser.$$(Selectors.workspaceTab(null))).to.have.lengthOf(0);
118+
});
119+
});
Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
import { useEffect, useRef } from 'react';
2+
import type { WorkspaceTab } from '../stores/workspaces';
3+
import { useWorkspaceTabId } from './workspace-tab-state-provider';
4+
5+
export type WorkspaceDestroyHandler = () => boolean;
6+
7+
const HANDLERS = {
8+
close: new Map<string, WorkspaceDestroyHandler[]>(),
9+
replace: new Map<string, WorkspaceDestroyHandler[]>(),
10+
} as const;
11+
12+
const resolveTabDestroyState = (
13+
type: 'close' | 'replace',
14+
tab: WorkspaceTab
15+
): boolean => {
16+
const handlers = HANDLERS[type].get(tab.id) ?? [];
17+
18+
for (const handler of handlers) {
19+
if (handler() === false) {
20+
return false;
21+
}
22+
}
23+
24+
return true;
25+
};
26+
27+
export const canCloseTab = (tab: WorkspaceTab) => {
28+
return resolveTabDestroyState('close', tab);
29+
};
30+
31+
export const canReplaceTab = (tab: WorkspaceTab) => {
32+
return resolveTabDestroyState('replace', tab);
33+
};
34+
35+
/**
36+
* Exported only for testing purposes
37+
* @internal
38+
*/
39+
export const setTabDestroyHandler = (
40+
type: 'close' | 'replace',
41+
tabId: string,
42+
handler: WorkspaceDestroyHandler
43+
) => {
44+
const handlerMap = HANDLERS[type];
45+
const handlers = handlerMap.get(tabId) ?? [];
46+
handlerMap.set(tabId, handlers.concat(handler));
47+
return () => {
48+
cleanupTabDestroyHandler(type, tabId, handler);
49+
};
50+
};
51+
52+
export const cleanupTabDestroyHandler = (
53+
type?: 'close' | 'replace',
54+
tabId?: string,
55+
handler?: WorkspaceDestroyHandler
56+
) => {
57+
if (handler && tabId && type) {
58+
HANDLERS[type].set(
59+
tabId,
60+
(HANDLERS[type].get(tabId) ?? []).filter((fn) => {
61+
return fn !== handler;
62+
})
63+
);
64+
} else if (tabId && type) {
65+
HANDLERS[type].delete(tabId);
66+
} else if (type) {
67+
HANDLERS[type].clear();
68+
} else {
69+
Object.values(HANDLERS).forEach((map) => {
70+
map.clear();
71+
});
72+
}
73+
};
74+
75+
function useOnTabDestroyHandler(
76+
type: 'close' | 'replace',
77+
handler: WorkspaceDestroyHandler
78+
) {
79+
const tabId = useWorkspaceTabId();
80+
const handlerRef = useRef(handler);
81+
handlerRef.current = handler;
82+
useEffect(() => {
83+
const onClose: WorkspaceDestroyHandler = () => {
84+
return handlerRef.current();
85+
};
86+
return setTabDestroyHandler(type, tabId, onClose);
87+
}, [type, tabId]);
88+
}
89+
90+
/**
91+
* A hook that registers a tab close handler. Before closing the tab, this
92+
* handler will be called and can return either `true` to allow the tab to be
93+
* closed, or `false`, preventing tab to be closed before user confirms the tab
94+
* closure.
95+
*
96+
* Multiple handlers can be registered for one tab, they will be called in order
97+
* of registration and any one of them returning `false` will prevent the tab
98+
* from closing.
99+
*
100+
* @example
101+
* function TabWithInput() {
102+
* const [value, setValue] = useState('');
103+
* // Will prevent tab from closing if text input is not empty. This will
104+
* // cause a confirmation modal to appear before closing the tab
105+
* useOnTabClose(() => {
106+
* return value !== '';
107+
* });
108+
* return <TextInput value={value} onChange={setValue} />
109+
* }
110+
*/
111+
export const useOnTabClose = useOnTabDestroyHandler.bind(null, 'close');
112+
113+
/**
114+
*
115+
* A hook that registers a tab replace handler. By default when opening a new
116+
* workspace, it will be opened in the same tab, destroying the content of the
117+
* current workspace. In that case, the registered handler can return either
118+
* `true` to allow the workspace to be destroyed, or `false` to prevent the tab
119+
* from being destroyed and forcing the new workspace to open in the new tab.
120+
*
121+
* Multiple handlers can be registered for one tab, they will be called in order
122+
* of registration and any one of them returning `false` will prevent the tab
123+
* from being replaced.
124+
*
125+
* @example
126+
* function TabWithInput() {
127+
* const [value, setValue] = useState('');
128+
* // Will prevent tab from being replaced if text input is not empty. This
129+
* // will cause new workspace to open in a new tab
130+
* useOnTabReplace(() => {
131+
* return value !== '';
132+
* });
133+
* return <TextInput value={value} onChange={setValue} />
134+
* }
135+
*/
136+
export const useOnTabReplace = useOnTabDestroyHandler.bind(null, 'replace');
137+
138+
export function useRegisterTabDestroyHandler() {
139+
let tabId: string | undefined;
140+
try {
141+
// We are not breaking rules of hooks here, this method will either always
142+
// fail or always return a value when rendered in a certain spot in the
143+
// application
144+
// eslint-disable-next-line react-hooks/rules-of-hooks
145+
tabId = useWorkspaceTabId();
146+
} catch {
147+
// do nothing, we are just outside of the workspace tab tree
148+
}
149+
150+
if (tabId) {
151+
return {
152+
onTabClose: setTabDestroyHandler.bind(null, 'close', tabId),
153+
onTabReplace: setTabDestroyHandler.bind(null, 'replace', tabId),
154+
};
155+
}
156+
}

packages/compass-workspaces/src/components/workspace-tab-state-provider.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ export const WorkspaceTabStateProvider = ({
9898
);
9999
};
100100

101-
function useWorkspaceTabId() {
101+
export function useWorkspaceTabId() {
102102
let tabId = useContext(WorkspaceTabIdContext);
103103
if (!tabId) {
104104
if (process.env.NODE_ENV !== 'test') {

0 commit comments

Comments
 (0)