From 9533f43d3a0787c34e93c2a80be061436b58f0a8 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Tue, 31 Dec 2024 15:28:28 +0100 Subject: [PATCH] Prevent activation of previous workspace when launching Connect via deep link to a different cluster (#50063) * Refactor `setActiveWorkspace` to async/await * Keep all the logic that restores a single workspace state in `getWorkspaceDefaultState` * Separate restoring state from setting active workspace * Allow the dialog to reopen documents to be closed without any decision * Add a test that verifies if the correct workspace is activated * Docs improvements * Return early if there's no restoredWorkspace * Fix logger name * Improve test name * Make restored state immutable * Fix comment * Do not wait for functions to be called in tests * Add tests for discarding documents reopen dialog * Move setting `isInitialized` to a separate method * Remove restored workspace when logging out so that we won't try to restore it when logging in again * Do not try to restore previous documents if the user already opened new ones * Do not close dialog in test * Improve `isInitialized` comment * Call `setActiveWorkspace` again when we fail to change the workspace * Fix the logic around restoring previous documents * Try to reopen documents also after `cluster-connect` is canceled * canRestoreDocuments -> hasDocumentsToReopen * Disallow parallel `setActiveWorkspace` calls (#50384) * Allow passing abortSignal to `open*Modal` methods, allow closing everything at once * Close all dialogs when activating a deep link * Allow only one `setActiveWorkspace` execution at a time * Review comments * Always unregister event listeners when a dialog is closed * Lint * Add a diagram for launching a deep link * Remove checking if the signal is aborted in `openImportantDialog`, rename it (cherry picked from commit 8e9b004fc59f11906d96e24e28792d3414a55ad3) --- .../jest/jest-environment-patched-jsdom.js | 8 + web/packages/teleterm/README.md | 45 +++ .../ui/AppInitializer/AppInitializer.test.tsx | 248 ++++++++++++ .../src/ui/AppInitializer/AppInitializer.tsx | 7 + .../showStartupModalsAndNotifications.ts | 5 - .../DocumentsReopen/DocumentsReopen.story.tsx | 8 +- .../ui/DocumentsReopen/DocumentsReopen.tsx | 9 +- .../src/ui/ModalsHost/ModalsHost.story.tsx | 1 + .../teleterm/src/ui/ModalsHost/ModalsHost.tsx | 21 +- .../teleterm/src/ui/StatusBar/StatusBar.tsx | 2 +- .../teleterm/src/ui/Vnet/vnetContext.tsx | 2 + web/packages/teleterm/src/ui/appContext.ts | 6 +- .../ui/services/clusters/clustersService.ts | 18 +- .../ui/services/deepLinks/deepLinksService.ts | 26 +- .../ui/services/modals/modalsService.test.ts | 135 +++++++ .../src/ui/services/modals/modalsService.ts | 94 ++++- .../statePersistenceService.ts | 5 +- .../documentsService/documentsService.ts | 26 +- .../workspacesService.test.ts | 216 ++++++---- .../workspacesService/workspacesService.ts | 380 ++++++++++-------- 20 files changed, 959 insertions(+), 303 deletions(-) create mode 100644 web/packages/teleterm/src/ui/AppInitializer/AppInitializer.test.tsx create mode 100644 web/packages/teleterm/src/ui/services/modals/modalsService.test.ts diff --git a/web/packages/build/jest/jest-environment-patched-jsdom.js b/web/packages/build/jest/jest-environment-patched-jsdom.js index cbc704768bfb8..d80f8460502dd 100644 --- a/web/packages/build/jest/jest-environment-patched-jsdom.js +++ b/web/packages/build/jest/jest-environment-patched-jsdom.js @@ -48,6 +48,14 @@ export default class PatchedJSDOMEnvironment extends JSDOMEnvironment { dispatchEvent: () => {}, }); } + + // TODO(gzdunek): JSDOM doesn't support AbortSignal.any(). + // Overwriting only this function doesn't help much, something between + // AbortSignal and AbortController is missing. + if (!global.AbortSignal.any) { + global.AbortSignal = AbortSignal; + global.AbortController = AbortController; + } } } export const TestEnvironment = PatchedJSDOMEnvironment; diff --git a/web/packages/teleterm/README.md b/web/packages/teleterm/README.md index ea1629fa4fae7..b6e0be4b6bbb2 100644 --- a/web/packages/teleterm/README.md +++ b/web/packages/teleterm/README.md @@ -242,3 +242,48 @@ resource availability as possible. ### PTY communication overview (Renderer Process <=> Shared Process) ![PTY communication](docs/ptyCommunication.png) + +### Overview of a deep link launch process + +The diagram below illustrates the process of launching a deep link, +depending on the state of the workspaces. +It assumes that the app is not running and that the deep link targets a workspace +different from the persisted one. + +
+Diagram + +```mermaid +flowchart TD +Start([Start]) --> IsPreviousWorkspaceConnected{Is the previously active workspace connected?} +IsPreviousWorkspaceConnected --> |Valid certificate| PreviousWorkspaceReopenDocuments{Has documents to reopen from the previous workspace?} +IsPreviousWorkspaceConnected --> |Expired certificate| CancelPreviousWorkspaceLogin[Cancel the login dialog] +IsPreviousWorkspaceConnected --> |No persisted workspace| SwitchWorkspace + + PreviousWorkspaceReopenDocuments --> |Yes| CancelPreviousWorkspaceDocumentsReopen[Cancel the reopen dialog without discarding documents] + PreviousWorkspaceReopenDocuments --> |No| SwitchWorkspace[Switch to a deep link workspace] + + CancelPreviousWorkspaceDocumentsReopen --> SwitchWorkspace + CancelPreviousWorkspaceLogin --> SwitchWorkspace + + SwitchWorkspace --> IsDeepLinkWorkspaceConnected{Is the deep link workspace connected?} + IsDeepLinkWorkspaceConnected --> |Valid certificate| DeepLinkWorkspaceReopenDocuments{Has documents to reopen from the deep link workspace?} + IsDeepLinkWorkspaceConnected --> |Not added| AddDeepLinkCluster[Add new cluster] + IsDeepLinkWorkspaceConnected --> |Expired certificate| LogInToDeepLinkWorkspace[Log in to workspace] + + AddDeepLinkCluster --> AddDeepLinkClusterSuccess{Was the cluster added successfully?} + AddDeepLinkClusterSuccess --> |Yes| LogInToDeepLinkWorkspace + AddDeepLinkClusterSuccess --> |No| ReturnToPreviousWorkspace[Return to the previously active workspace and try to reopen its documents again] + + LogInToDeepLinkWorkspace --> IsLoginToDeepLinkWorkspaceSuccess{Was login successful?} + IsLoginToDeepLinkWorkspaceSuccess --> |Yes| DeepLinkWorkspaceReopenDocuments + IsLoginToDeepLinkWorkspaceSuccess --> |No| ReturnToPreviousWorkspace + + DeepLinkWorkspaceReopenDocuments --> |Yes| ReopenDeepLinkWorkspaceDocuments[Reopen documents] + DeepLinkWorkspaceReopenDocuments --> |No| End + + ReopenDeepLinkWorkspaceDocuments --> End + ReturnToPreviousWorkspace --> End +``` + +
diff --git a/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.test.tsx b/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.test.tsx new file mode 100644 index 0000000000000..102a9d4d23e32 --- /dev/null +++ b/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.test.tsx @@ -0,0 +1,248 @@ +/** + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import 'jest-canvas-mock'; + +import { act, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { mockIntersectionObserver } from 'jsdom-testing-mocks'; + +import { render } from 'design/utils/testing'; + +import Logger, { NullService } from 'teleterm/logger'; +import { MockedUnaryCall } from 'teleterm/services/tshd/cloneableClient'; +import { makeRootCluster } from 'teleterm/services/tshd/testHelpers'; +import { ResourcesContextProvider } from 'teleterm/ui/DocumentCluster/resourcesContext'; +import { MockAppContextProvider } from 'teleterm/ui/fixtures/MockAppContextProvider'; +import { MockAppContext } from 'teleterm/ui/fixtures/mocks'; +import { ConnectionsContextProvider } from 'teleterm/ui/TopBar/Connections/connectionsContext'; +import { IAppContext } from 'teleterm/ui/types'; +import { VnetContextProvider } from 'teleterm/ui/Vnet'; + +import { AppInitializer } from './AppInitializer'; + +mockIntersectionObserver(); +beforeAll(() => { + Logger.init(new NullService()); +}); + +jest.mock('teleterm/ui/ClusterConnect', () => ({ + ClusterConnect: props => ( +
+ Connect to {props.dialog.clusterUri} + +
+ ), +})); + +test('activating a workspace via deep link overrides the previously active workspace', async () => { + // Before closing the app, both clusters were present in the state, with previouslyActiveCluster being active. + // However, the user clicked a deep link pointing to deepLinkCluster. + // The app should prioritize the user's intent by activating the workspace for the deep link, + // rather than reactivating the previously active cluster. + const previouslyActiveCluster = makeRootCluster({ + uri: '/clusters/teleport-previously-active', + proxyHost: 'teleport-previously-active:3080', + name: 'teleport-previously-active', + connected: false, + }); + const deepLinkCluster = makeRootCluster({ + uri: '/clusters/teleport-deep-link', + proxyHost: 'teleport-deep-link:3080', + name: 'teleport-deep-link', + connected: false, + }); + const appContext = new MockAppContext(); + jest + .spyOn(appContext.statePersistenceService, 'getWorkspacesState') + .mockReturnValue({ + rootClusterUri: previouslyActiveCluster.uri, + workspaces: { + [previouslyActiveCluster.uri]: { + localClusterUri: previouslyActiveCluster.uri, + documents: [], + location: undefined, + }, + [deepLinkCluster.uri]: { + localClusterUri: deepLinkCluster.uri, + documents: [], + location: undefined, + }, + }, + }); + appContext.mainProcessClient.configService.set( + 'usageReporting.enabled', + false + ); + jest.spyOn(appContext.tshd, 'listRootClusters').mockReturnValue( + new MockedUnaryCall({ + clusters: [deepLinkCluster, previouslyActiveCluster], + }) + ); + + render( + + + + + + + + + + ); + + expect( + await screen.findByText(`Connect to ${previouslyActiveCluster.uri}`) + ).toBeInTheDocument(); + + // Launch a deep link and do not wait for the result. + act(() => { + void appContext.deepLinksService.launchDeepLink({ + status: 'success', + url: { + host: deepLinkCluster.proxyHost, + hostname: deepLinkCluster.name, + port: '1234', + pathname: '/authenticate_web_device', + username: deepLinkCluster.loggedInUser.name, + searchParams: { + id: '123', + redirect_uri: '', + token: 'abc', + }, + }, + }); + }); + + // The previous dialog has been replaced without a user interaction. + // In the real app, this happens fast enough that the user doesn't see the previous dialog. + expect( + await screen.findByText(`Connect to ${deepLinkCluster.uri}`) + ).toBeInTheDocument(); + + // We confirm the current cluster-connect dialog. + const dialogSuccessButton = await screen.findByRole('button', { + name: 'Connect to cluster', + }); + await userEvent.click(dialogSuccessButton); + + // Check if the first activated workspace is the one from the deep link. + expect(await screen.findByTitle(/Current cluster:/)).toBeVisible(); + expect( + screen.queryByTitle(`Current cluster: ${deepLinkCluster.name}`) + ).toBeVisible(); +}); + +test.each<{ + name: string; + action(appContext: IAppContext): Promise; + expectHasDocumentsToReopen: boolean; +}>([ + { + name: 'closing documents reopen dialog via close button discards previous documents', + action: async () => { + await userEvent.click(await screen.findByTitle('Close')); + }, + expectHasDocumentsToReopen: false, + }, + { + name: 'starting new session in document reopen dialog discards previous documents', + action: async () => { + await userEvent.click( + await screen.findByRole('button', { name: 'Start New Session' }) + ); + }, + expectHasDocumentsToReopen: false, + }, + { + name: 'overwriting document reopen dialog with another regular dialog does not discard documents', + action: async appContext => { + act(() => { + appContext.modalsService.openRegularDialog({ + kind: 'change-access-request-kind', + onConfirm() {}, + onCancel() {}, + }); + }); + }, + expectHasDocumentsToReopen: true, + }, +])('$name', async testCase => { + const rootCluster = makeRootCluster(); + const appContext = new MockAppContext(); + jest + .spyOn(appContext.statePersistenceService, 'getWorkspacesState') + .mockReturnValue({ + rootClusterUri: rootCluster.uri, + workspaces: { + [rootCluster.uri]: { + localClusterUri: rootCluster.uri, + documents: [ + { + kind: 'doc.access_requests', + uri: '/docs/123', + state: 'browsing', + clusterUri: rootCluster.uri, + requestId: '', + title: 'Access Requests', + }, + ], + location: undefined, + }, + }, + }); + appContext.mainProcessClient.configService.set( + 'usageReporting.enabled', + false + ); + jest.spyOn(appContext.tshd, 'listRootClusters').mockReturnValue( + new MockedUnaryCall({ + clusters: [rootCluster], + }) + ); + + render( + + + + + + + + + + ); + + expect( + await screen.findByText( + 'Do you want to reopen tabs from the previous session?' + ) + ).toBeInTheDocument(); + + await testCase.action(appContext); + + expect( + appContext.workspacesService.getWorkspace(rootCluster.uri) + .hasDocumentsToReopen + ).toBe(testCase.expectHasDocumentsToReopen); +}); diff --git a/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.tsx b/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.tsx index dc25aca95867d..c858678900730 100644 --- a/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.tsx +++ b/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.tsx @@ -39,6 +39,13 @@ export const AppInitializer = () => { await appContext.pullInitialState(); setShouldShowUi(true); await showStartupModalsAndNotifications(appContext); + // If there's a workspace that was active before closing the app, + // activate it. + const rootClusterUri = + appContext.workspacesService.getRestoredState()?.rootClusterUri; + if (rootClusterUri) { + void appContext.workspacesService.setActiveWorkspace(rootClusterUri); + } appContext.mainProcessClient.signalUserInterfaceReadiness({ success: true, }); diff --git a/web/packages/teleterm/src/ui/AppInitializer/showStartupModalsAndNotifications.ts b/web/packages/teleterm/src/ui/AppInitializer/showStartupModalsAndNotifications.ts index 6f5e93a5cfd4c..39af7b9472387 100644 --- a/web/packages/teleterm/src/ui/AppInitializer/showStartupModalsAndNotifications.ts +++ b/web/packages/teleterm/src/ui/AppInitializer/showStartupModalsAndNotifications.ts @@ -45,11 +45,6 @@ export async function showStartupModalsAndNotifications( // "User job role" dialog is shown on the second launch (only if user agreed to reporting earlier). await setUpUsageReporting(configService, ctx.modalsService); - // If there's a workspace that was active before the user closed the app, restorePersistedState - // will block until the user interacts with the login modal (if the cert is not valid anymore) and - // the modal for restoring documents. - await ctx.workspacesService.restorePersistedState(); - notifyAboutConfigErrors(configService, ctx.notificationsService); notifyAboutDuplicatedShortcutsCombinations( ctx.keyboardShortcutsService, diff --git a/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.story.tsx b/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.story.tsx index bf2c2e0c63015..7405229fbb01d 100644 --- a/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.story.tsx +++ b/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.story.tsx @@ -33,7 +33,7 @@ export const Story = () => { rootClusterUri="/clusters/foo.cloud.gravitational.io" numberOfDocuments={8} onConfirm={() => {}} - onCancel={() => {}} + onDiscard={() => {}} /> ); @@ -46,7 +46,7 @@ export const OneTab = () => { rootClusterUri="/clusters/foo.cloud.gravitational.io" numberOfDocuments={1} onConfirm={() => {}} - onCancel={() => {}} + onDiscard={() => {}} /> ); @@ -59,7 +59,7 @@ export const LongClusterName = () => { rootClusterUri="/clusters/foo.bar.baz.quux.cloud.gravitational.io" numberOfDocuments={42} onConfirm={() => {}} - onCancel={() => {}} + onDiscard={() => {}} /> ); @@ -75,7 +75,7 @@ export const LongContinuousClusterName = () => { .join('')}`} numberOfDocuments={680} onConfirm={() => {}} - onCancel={() => {}} + onDiscard={() => {}} /> ); diff --git a/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.tsx b/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.tsx index c1dfce958c849..0f2f02d9f65c0 100644 --- a/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.tsx +++ b/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.tsx @@ -33,7 +33,7 @@ import { RootClusterUri, routing } from 'teleterm/ui/uri'; export function DocumentsReopen(props: { rootClusterUri: RootClusterUri; numberOfDocuments: number; - onCancel(): void; + onDiscard(): void; onConfirm(): void; hidden?: boolean; }) { @@ -49,7 +49,7 @@ export function DocumentsReopen(props: { ({ maxWidth: '400px', width: '100%', @@ -71,7 +71,8 @@ export function DocumentsReopen(props: { @@ -107,7 +108,7 @@ export function DocumentsReopen(props: { Reopen - + Start new session diff --git a/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.story.tsx b/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.story.tsx index 29cdebecec897..2245ab90b5d02 100644 --- a/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.story.tsx +++ b/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.story.tsx @@ -56,6 +56,7 @@ const documentsReopenDialog: DialogDocumentsReopen = { rootClusterUri: '/clusters/foo', numberOfDocuments: 1, onConfirm: () => {}, + onDiscard: () => {}, onCancel: () => {}, }; diff --git a/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx b/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx index 8023f500ef219..b53a685f6d02c 100644 --- a/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx +++ b/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx @@ -38,22 +38,21 @@ export default function ModalsHost() { const { regular: regularDialog, important: importantDialogs } = modalsService.useState(); - const closeRegularDialog = () => modalsService.closeRegularDialog(); - return ( <> - {renderDialog({ - dialog: regularDialog, - handleClose: closeRegularDialog, - hidden: !!importantDialogs.length, - })} - {importantDialogs.map(({ dialog, id }, index) => { + {regularDialog && + renderDialog({ + dialog: regularDialog.dialog, + handleClose: regularDialog.close, + hidden: !!importantDialogs.length, + })} + {importantDialogs.map(({ dialog, id, close }, index) => { const isLast = index === importantDialogs.length - 1; return ( {renderDialog({ dialog: dialog, - handleClose: () => modalsService.closeImportantDialog(id), + handleClose: close, hidden: !isLast, })} @@ -118,9 +117,9 @@ function renderDialog({ hidden={hidden} rootClusterUri={dialog.rootClusterUri} numberOfDocuments={dialog.numberOfDocuments} - onCancel={() => { + onDiscard={() => { handleClose(); - dialog.onCancel(); + dialog.onDiscard(); }} onConfirm={() => { handleClose(); diff --git a/web/packages/teleterm/src/ui/StatusBar/StatusBar.tsx b/web/packages/teleterm/src/ui/StatusBar/StatusBar.tsx index 6d2748412df80..ab8b0add0d223 100644 --- a/web/packages/teleterm/src/ui/StatusBar/StatusBar.tsx +++ b/web/packages/teleterm/src/ui/StatusBar/StatusBar.tsx @@ -45,7 +45,7 @@ export function StatusBar() { css={` white-space: nowrap; `} - title={clusterBreadcrumbs} + title={clusterBreadcrumbs && `Current cluster: ${clusterBreadcrumbs}`} > {clusterBreadcrumbs} diff --git a/web/packages/teleterm/src/ui/Vnet/vnetContext.tsx b/web/packages/teleterm/src/ui/Vnet/vnetContext.tsx index 887f4a86b82ea..47cccc34f45bc 100644 --- a/web/packages/teleterm/src/ui/Vnet/vnetContext.tsx +++ b/web/packages/teleterm/src/ui/Vnet/vnetContext.tsx @@ -124,6 +124,8 @@ export const VnetContextProvider: FC = props => { if ( isSupported && autoStart && + // Accessing resources through VNet might trigger the MFA modal, + // so we have to wait for the tshd events service to be initialized. isWorkspaceStateInitialized && startAttempt.status === '' ) { diff --git a/web/packages/teleterm/src/ui/appContext.ts b/web/packages/teleterm/src/ui/appContext.ts index c58911076eace..9c63a90f3f313 100644 --- a/web/packages/teleterm/src/ui/appContext.ts +++ b/web/packages/teleterm/src/ui/appContext.ts @@ -176,8 +176,12 @@ export default class AppContext implements IAppContext { this.subscribeToDeepLinkLaunch(); this.notifyMainProcessAboutClusterListChanges(); - this.clustersService.syncGatewaysAndCatchErrors(); + void this.clustersService.syncGatewaysAndCatchErrors(); await this.clustersService.syncRootClustersAndCatchErrors(); + this.workspacesService.restorePersistedState(); + // The app has been initialized (callbacks are set up, state is restored). + // The UI is visible. + this.workspacesService.markAsInitialized(); } /** diff --git a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts index 26f6771066a1d..7260cafafa8f8 100644 --- a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts +++ b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts @@ -38,10 +38,15 @@ import { DbType, formatDatabaseInfo, } from 'shared/services/databases'; +import { isAbortError } from 'shared/utils/abortError'; import { pipe } from 'shared/utils/pipe'; import { MainProcessClient } from 'teleterm/mainProcess/types'; -import type { CloneableAbortSignal, TshdClient } from 'teleterm/services/tshd'; +import { + cloneAbortSignal, + type CloneableAbortSignal, + type TshdClient, +} from 'teleterm/services/tshd'; import { NotificationsService } from 'teleterm/ui/services/notifications'; import { UsageService } from 'teleterm/ui/services/usage'; import * as uri from 'teleterm/ui/uri'; @@ -318,13 +323,20 @@ export class ClustersService extends ImmutableStore ]); } - async syncRootClustersAndCatchErrors() { + async syncRootClustersAndCatchErrors(abortSignal?: AbortSignal) { let clusters: Cluster[]; try { - const { response } = await this.client.listRootClusters({}); + const { response } = await this.client.listRootClusters( + {}, + { abortSignal: abortSignal && cloneAbortSignal(abortSignal) } + ); clusters = response.clusters; } catch (error) { + if (isAbortError(error)) { + this.logger.info('Listing root clusters aborted'); + return; + } this.notificationsService.notifyError({ title: 'Could not fetch root clusters', description: error.message, diff --git a/web/packages/teleterm/src/ui/services/deepLinks/deepLinksService.ts b/web/packages/teleterm/src/ui/services/deepLinks/deepLinksService.ts index fa412f280fde5..fe179950501c0 100644 --- a/web/packages/teleterm/src/ui/services/deepLinks/deepLinksService.ts +++ b/web/packages/teleterm/src/ui/services/deepLinks/deepLinksService.ts @@ -74,6 +74,14 @@ export class DeepLinksService { return; } + // Before we start, let's close any open dialogs, for a few reasons: + // 1. Activating a deep link may require changing the workspace, and we don't + // want to see dialogs from the previous one. + // 2. A login dialog could be covered by an important dialog. + // 3. The user could be confused, since Connect My Computer or Authorize Web + // Session documents would be displayed below a dialog. + this.modalsService.cancelAndCloseAll(); + // launchDeepLink cannot throw if it receives a pathname that doesn't match any supported // pathnames. The user might simply be using a version of Connect that doesn't support the given // pathname yet. Generally, such cases should be caught outside of DeepLinksService by @@ -163,6 +171,16 @@ export class DeepLinksService { rootClusterUri: RootClusterUri; } > { + const currentlyActiveWorkspace = this.workspacesService.getRootClusterUri(); + // If we closed the dialog to reopen documents when launching a deep link, + // setting the active workspace again will reopen it. + const reopenCurrentlyActiveWorkspace = async () => { + if (currentlyActiveWorkspace) { + await this.workspacesService.setActiveWorkspace( + currentlyActiveWorkspace + ); + } + }; const rootClusterId = url.hostname; const clusterAddress = url.host; const prefill = { @@ -186,6 +204,7 @@ export class DeepLinksService { }); if (canceled) { + await reopenCurrentlyActiveWorkspace(); return { isAtDesiredWorkspace: false, }; @@ -201,6 +220,11 @@ export class DeepLinksService { prefill ); - return { isAtDesiredWorkspace, rootClusterUri }; + if (isAtDesiredWorkspace) { + return { isAtDesiredWorkspace: true, rootClusterUri }; + } + + await reopenCurrentlyActiveWorkspace(); + return { isAtDesiredWorkspace: false }; } } diff --git a/web/packages/teleterm/src/ui/services/modals/modalsService.test.ts b/web/packages/teleterm/src/ui/services/modals/modalsService.test.ts new file mode 100644 index 0000000000000..dc32892f1c249 --- /dev/null +++ b/web/packages/teleterm/src/ui/services/modals/modalsService.test.ts @@ -0,0 +1,135 @@ +/** + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { makeRootCluster } from 'teleterm/services/tshd/testHelpers'; + +import { DialogClusterConnect, ModalsService } from './modalsService'; + +const rootCluster = makeRootCluster(); + +function makeDialogClusterConnect(): DialogClusterConnect { + return { + kind: 'cluster-connect', + clusterUri: rootCluster.uri, + reason: undefined, + prefill: undefined, + onSuccess: jest.fn(), + onCancel: jest.fn(), + }; +} + +test('closing all dialogs', () => { + const dialogClusterConnect1 = makeDialogClusterConnect(); + const dialogClusterConnect2 = makeDialogClusterConnect(); + const modalsService = new ModalsService(); + + modalsService.openRegularDialog(dialogClusterConnect1); + modalsService.openImportantDialog(dialogClusterConnect2); + expect(modalsService.state.regular.dialog).toStrictEqual( + dialogClusterConnect1 + ); + expect(modalsService.state.important).toHaveLength(1); + expect(modalsService.state.important[0].dialog).toStrictEqual( + dialogClusterConnect2 + ); + + modalsService.cancelAndCloseAll(); + expect(modalsService.state.regular).toStrictEqual(undefined); + expect(modalsService.state.important).toHaveLength(0); + expect(dialogClusterConnect1.onCancel).toHaveBeenCalledTimes(1); + expect(dialogClusterConnect2.onCancel).toHaveBeenCalledTimes(1); +}); + +test('closing regular dialog with abort signal', () => { + const dialogClusterConnect = makeDialogClusterConnect(); + const modalsService = new ModalsService(); + const controller = new AbortController(); + + modalsService.openRegularDialog(dialogClusterConnect, controller.signal); + expect(modalsService.state.regular.dialog).toStrictEqual( + dialogClusterConnect + ); + controller.abort(); + expect(modalsService.state.regular).toStrictEqual(undefined); + expect(dialogClusterConnect.onCancel).toHaveBeenCalledTimes(1); +}); + +test('aborting dialog is ignored after it has been closed', () => { + const dialogClusterConnect1 = makeDialogClusterConnect(); + const modalsService = new ModalsService(); + const controller = new AbortController(); + + modalsService.openRegularDialog(dialogClusterConnect1, controller.signal); + expect(modalsService.state.regular.dialog).toStrictEqual( + dialogClusterConnect1 + ); + dialogClusterConnect1.onSuccess(''); + + const dialogClusterConnect2 = makeDialogClusterConnect(); + modalsService.openRegularDialog(dialogClusterConnect2); + expect(modalsService.state.regular.dialog).toStrictEqual( + dialogClusterConnect2 + ); + + controller.abort(); + // The currently open dialog is not closed. + expect(modalsService.state.regular.dialog).toStrictEqual( + dialogClusterConnect2 + ); +}); + +test('opening a new regular dialog while another is active invokes onCancel callback of the previous one', () => { + const dialogClusterConnect1 = makeDialogClusterConnect(); + const dialogClusterConnect2 = makeDialogClusterConnect(); + const modalsService = new ModalsService(); + + modalsService.openRegularDialog(dialogClusterConnect1); + expect(modalsService.state.regular.dialog).toStrictEqual( + dialogClusterConnect1 + ); + modalsService.openRegularDialog(dialogClusterConnect2); + expect(dialogClusterConnect1.onCancel).toHaveBeenCalledTimes(1); +}); + +test('when dialog is canceled in multiple ways, onCancel callback is invoked once', () => { + const dialogClusterConnect = makeDialogClusterConnect(); + const modalsService = new ModalsService(); + const controller = new AbortController(); + + const { closeDialog } = modalsService.openRegularDialog( + dialogClusterConnect, + controller.signal + ); + expect(modalsService.state.regular.dialog).toStrictEqual( + dialogClusterConnect + ); + closeDialog(); + controller.abort(); + expect(dialogClusterConnect.onCancel).toHaveBeenCalledTimes(1); +}); + +test('dialog opened with aborted signal returns immediately', () => { + const dialogClusterConnect = makeDialogClusterConnect(); + const modalsService = new ModalsService(); + const controller = new AbortController(); + controller.abort(); + + modalsService.openRegularDialog(dialogClusterConnect, controller.signal); + expect(modalsService.state.regular).toStrictEqual(undefined); + expect(dialogClusterConnect.onCancel).toHaveBeenCalledTimes(1); +}); diff --git a/web/packages/teleterm/src/ui/services/modals/modalsService.ts b/web/packages/teleterm/src/ui/services/modals/modalsService.ts index bc69205ea4269..638594071c3d6 100644 --- a/web/packages/teleterm/src/ui/services/modals/modalsService.ts +++ b/web/packages/teleterm/src/ui/services/modals/modalsService.ts @@ -38,8 +38,9 @@ type State = { // The important dialogs are displayed above the regular one. This is to avoid losing the state of // the regular modal if we happen to need to interrupt whatever the user is doing and display an // important modal. - important: { dialog: Dialog; id: string }[]; - regular: Dialog | undefined; + // `close` function closes the dialog and unregisters event listeners. + important: { dialog: Dialog; id: string; close(): void }[]; + regular: { dialog: Dialog; close(): void } | undefined; }; export class ModalsService extends ImmutableStore { @@ -48,6 +49,15 @@ export class ModalsService extends ImmutableStore { regular: undefined, }; + private allDialogsController = new AbortController(); + + private getSharedAbortSignal(dialogSignal?: AbortSignal): AbortSignal { + if (!dialogSignal) { + return this.allDialogsController.signal; + } + return AbortSignal.any([dialogSignal, this.allDialogsController.signal]); + } + /** * openRegularDialog opens the given dialog as a regular dialog. A regular dialog can get covered * by an important dialog. The regular dialog won't get unmounted if an important dialog is shown @@ -57,20 +67,52 @@ export class ModalsService extends ImmutableStore { * old dialog with the new one. * The old dialog is canceled, if possible. * - * The returned closeDialog function can be used to close the dialog and automatically call the - * dialog's onCancel callback (if present). + * The passed `abortSignal` or the returned `closeDialog` function can be used to close the dialog + * and automatically call the dialog's onCancel callback (if present). + * The onCancel function can be called without a user interaction. As of now, + * this happens when the user launches a deep link while a regular dialog is open. + * As such, if the dialog presents the user with a decision, onCancel should be + * treated as no decision being made. + * If possible, the user should be prompted again to make the decision later on. */ - openRegularDialog(dialog: Dialog): { closeDialog: () => void } { - this.state.regular?.['onCancel']?.(); + openRegularDialog( + dialog: Dialog, + abortSignal?: AbortSignal + ): { + closeDialog: () => void; + } { + const onCancelDialog = () => dialog['onCancel']?.(); + const sharedSignal = this.getSharedAbortSignal(abortSignal); + if (sharedSignal.aborted) { + onCancelDialog(); + return { + closeDialog: () => {}, + }; + } + + // If there's a previous dialog, cancel and close it. + const previousDialog = this.state.regular; + if (previousDialog) { + previousDialog.dialog['onCancel']?.(); + previousDialog.close(); + } + + const close = () => { + sharedSignal.removeEventListener('abort', cancelAndClose); + this.closeRegularDialog(); + }; + const cancelAndClose = () => { + close(); + onCancelDialog(); + }; + sharedSignal.addEventListener('abort', cancelAndClose); + this.setState(draftState => { - draftState.regular = dialog; + draftState.regular = { dialog, close }; }); return { - closeDialog: () => { - this.closeRegularDialog(); - dialog['onCancel']?.(); - }, + closeDialog: cancelAndClose, }; } @@ -92,27 +134,36 @@ export class ModalsService extends ImmutableStore { * dialog's onCancel callback (if present). */ openImportantDialog(dialog: Dialog): { closeDialog: () => void; id: string } { + const onCancelDialog = () => dialog['onCancel']?.(); + const allDialogsSignal = this.allDialogsController.signal; const id = crypto.randomUUID(); + + const close = () => { + allDialogsSignal.removeEventListener('abort', cancelAndClose); + this.closeImportantDialog(id); + }; + const cancelAndClose = () => { + close(); + onCancelDialog(); + }; + allDialogsSignal.addEventListener('abort', cancelAndClose); this.setState(draftState => { - draftState.important.push({ dialog, id }); + draftState.important.push({ dialog, id, close }); }); return { id, - closeDialog: () => { - this.closeImportantDialog(id); - dialog['onCancel']?.(); - }, + closeDialog: cancelAndClose, }; } - closeRegularDialog() { + private closeRegularDialog() { this.setState(draftState => { draftState.regular = undefined; }); } - closeImportantDialog(id: string) { + private closeImportantDialog(id: string) { this.setState(draftState => { const index = draftState.important.findIndex(d => d.id === id); if (index >= 0) { @@ -121,6 +172,11 @@ export class ModalsService extends ImmutableStore { }); } + cancelAndCloseAll(): void { + this.allDialogsController.abort(); + this.allDialogsController = new AbortController(); + } + useState() { return useStore(this).state; } @@ -166,6 +222,8 @@ export interface DialogDocumentsReopen { rootClusterUri: RootClusterUri; numberOfDocuments: number; onConfirm?(): void; + onDiscard?(): void; + /** Cancels the dialog, without discarding documents. */ onCancel?(): void; } diff --git a/web/packages/teleterm/src/ui/services/statePersistence/statePersistenceService.ts b/web/packages/teleterm/src/ui/services/statePersistence/statePersistenceService.ts index ba37afef48ecf..75caa7cbc5cf5 100644 --- a/web/packages/teleterm/src/ui/services/statePersistence/statePersistenceService.ts +++ b/web/packages/teleterm/src/ui/services/statePersistence/statePersistenceService.ts @@ -35,7 +35,10 @@ export type WorkspacesPersistedState = Omit< WorkspacesState, 'workspaces' | 'isInitialized' > & { - workspaces: Record>; + workspaces: Record< + string, + Omit + >; }; export interface StatePersistenceState { diff --git a/web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsService.ts b/web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsService.ts index 83b3d89ff05d2..79420a6cab0a1 100644 --- a/web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsService.ts +++ b/web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsService.ts @@ -84,19 +84,12 @@ export class DocumentsService { }; } + /** @deprecated Use createClusterDocument function instead of the method on DocumentsService. */ createClusterDocument(opts: { clusterUri: uri.ClusterUri; queryParams?: DocumentClusterQueryParams; }): DocumentCluster { - const uri = routing.getDocUri({ docId: unique() }); - const clusterName = routing.parseClusterName(opts.clusterUri); - return { - uri, - clusterUri: opts.clusterUri, - title: clusterName, - kind: 'doc.cluster', - queryParams: opts.queryParams || getDefaultDocumentClusterQueryParams(), - }; + return createClusterDocument(opts); } /** @@ -508,6 +501,21 @@ export class DocumentsService { } } +export function createClusterDocument(opts: { + clusterUri: uri.ClusterUri; + queryParams?: DocumentClusterQueryParams; +}): DocumentCluster { + const uri = routing.getDocUri({ docId: unique() }); + const clusterName = routing.parseClusterName(opts.clusterUri); + return { + uri, + clusterUri: opts.clusterUri, + title: clusterName, + kind: 'doc.cluster', + queryParams: opts.queryParams || getDefaultDocumentClusterQueryParams(), + }; +} + export function getDefaultDocumentClusterQueryParams(): DocumentClusterQueryParams { return { resourceKinds: [], diff --git a/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts b/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts index 628bbedd62190..8fd108779e9c7 100644 --- a/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts +++ b/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts @@ -49,7 +49,7 @@ beforeEach(() => { }); describe('restoring workspace', () => { - it('restores the workspace if there is a persisted state for given clusterUri', async () => { + it('restores the workspace if there is a persisted state for given clusterUri', () => { const cluster = makeRootCluster(); const testWorkspace: Workspace = { accessRequests: { @@ -67,16 +67,15 @@ describe('restoring workspace', () => { location: '/docs/some_uri', }; - const { workspacesService, clusterDocument } = getTestSetup({ + const persistedWorkspace = { [cluster.uri]: testWorkspace }; + + const { workspacesService } = getTestSetup({ cluster, - persistedWorkspaces: { [cluster.uri]: testWorkspace }, + persistedWorkspaces: persistedWorkspace, }); - expect(workspacesService.state.isInitialized).toEqual(false); - - await workspacesService.restorePersistedState(); + workspacesService.restorePersistedState(); - expect(workspacesService.state.isInitialized).toEqual(true); expect(workspacesService.getWorkspaces()).toStrictEqual({ [cluster.uri]: { accessRequests: { @@ -87,12 +86,9 @@ describe('restoring workspace', () => { isBarCollapsed: false, }, localClusterUri: testWorkspace.localClusterUri, - documents: [clusterDocument], - location: clusterDocument.uri, - previous: { - documents: testWorkspace.documents, - location: testWorkspace.location, - }, + documents: [expect.objectContaining({ kind: 'doc.cluster' })], + location: expect.any(String), + hasDocumentsToReopen: true, connectMyComputer: undefined, unifiedResourcePreferences: { defaultTab: DefaultTab.ALL, @@ -102,20 +98,20 @@ describe('restoring workspace', () => { }, }, }); + expect(workspacesService.getRestoredState().workspaces).toStrictEqual( + persistedWorkspace + ); }); - it('creates empty workspace if there is no persisted state for given clusterUri', async () => { + it('creates empty workspace if there is no persisted state for given clusterUri', () => { const cluster = makeRootCluster(); - const { workspacesService, clusterDocument } = getTestSetup({ + const { workspacesService } = getTestSetup({ cluster, persistedWorkspaces: {}, }); - expect(workspacesService.state.isInitialized).toEqual(false); - - await workspacesService.restorePersistedState(); + workspacesService.restorePersistedState(); - expect(workspacesService.state.isInitialized).toEqual(true); expect(workspacesService.getWorkspaces()).toStrictEqual({ [cluster.uri]: { accessRequests: { @@ -126,9 +122,9 @@ describe('restoring workspace', () => { }, }, localClusterUri: cluster.uri, - documents: [clusterDocument], - location: clusterDocument.uri, - previous: undefined, + documents: [expect.objectContaining({ kind: 'doc.cluster' })], + location: expect.any(String), + hasDocumentsToReopen: false, connectMyComputer: undefined, unifiedResourcePreferences: { defaultTab: DefaultTab.ALL, @@ -138,53 +134,7 @@ describe('restoring workspace', () => { }, }, }); - }); - - it('location is set to first document if it points to non-existing document', async () => { - const cluster = makeRootCluster(); - const testWorkspace: Workspace = { - accessRequests: { - isBarCollapsed: true, - pending: getEmptyPendingAccessRequest(), - }, - localClusterUri: cluster.uri, - documents: [ - { - kind: 'doc.terminal_shell', - uri: '/docs/terminal_shell_uri_1', - title: '/Users/alice/Documents', - }, - { - kind: 'doc.terminal_shell', - uri: '/docs/terminal_shell_uri_2', - title: '/Users/alice/Documents', - }, - ], - location: '/docs/non-existing-doc', - }; - - const { workspacesService } = getTestSetup({ - cluster, - persistedWorkspaces: { [cluster.uri]: testWorkspace }, - }); - - await workspacesService.restorePersistedState(); - - expect(workspacesService.getWorkspace(cluster.uri).previous).toStrictEqual({ - documents: [ - { - kind: 'doc.terminal_shell', - uri: '/docs/terminal_shell_uri_1', - title: '/Users/alice/Documents', - }, - { - kind: 'doc.terminal_shell', - uri: '/docs/terminal_shell_uri_2', - title: '/Users/alice/Documents', - }, - ], - location: '/docs/terminal_shell_uri_1', - }); + expect(workspacesService.getRestoredState().workspaces).toStrictEqual({}); }); }); @@ -331,19 +281,21 @@ describe('setActiveWorkspace', () => { }); it('does not switch the workspace if the cluster has a profile status error', async () => { + const rootCluster = makeRootCluster({ + connected: false, + loggedInUser: undefined, + profileStatusError: 'no YubiKey device connected', + }); const { workspacesService, notificationsService } = getTestSetup({ - cluster: makeRootCluster({ - connected: false, - loggedInUser: undefined, - profileStatusError: 'no YubiKey device connected', - }), + cluster: rootCluster, persistedWorkspaces: {}, }); jest.spyOn(notificationsService, 'notifyError'); - const { isAtDesiredWorkspace } = - await workspacesService.setActiveWorkspace('/clusters/foo'); + const { isAtDesiredWorkspace } = await workspacesService.setActiveWorkspace( + rootCluster.uri + ); expect(isAtDesiredWorkspace).toBe(false); expect(notificationsService.notifyError).toHaveBeenCalledWith( @@ -354,10 +306,110 @@ describe('setActiveWorkspace', () => { ); expect(workspacesService.getRootClusterUri()).toBeUndefined(); }); + + it('sets location to first document if location points to non-existing document when reopening documents', async () => { + const cluster = makeRootCluster(); + const testWorkspace: Workspace = { + accessRequests: { + isBarCollapsed: true, + pending: getEmptyPendingAccessRequest(), + }, + localClusterUri: cluster.uri, + documents: [ + { + kind: 'doc.terminal_shell', + uri: '/docs/terminal_shell_uri_1', + title: '/Users/alice/Documents', + }, + { + kind: 'doc.terminal_shell', + uri: '/docs/terminal_shell_uri_2', + title: '/Users/alice/Documents', + }, + ], + location: '/docs/non-existing-doc', + }; + + const { workspacesService, modalsService } = getTestSetup({ + cluster, + persistedWorkspaces: { [cluster.uri]: testWorkspace }, + }); + + jest + .spyOn(modalsService, 'openRegularDialog') + .mockImplementation(dialog => { + if (dialog.kind === 'documents-reopen') { + dialog.onConfirm(); + } else { + throw new Error(`Got unexpected dialog ${dialog.kind}`); + } + + return { + closeDialog: () => {}, + }; + }); + + workspacesService.restorePersistedState(); + await workspacesService.setActiveWorkspace(cluster.uri); + + expect(workspacesService.getWorkspace(cluster.uri)).toStrictEqual( + expect.objectContaining({ + documents: [ + { + kind: 'doc.terminal_shell', + uri: '/docs/terminal_shell_uri_1', + title: '/Users/alice/Documents', + }, + { + kind: 'doc.terminal_shell', + uri: '/docs/terminal_shell_uri_2', + title: '/Users/alice/Documents', + }, + ], + location: '/docs/terminal_shell_uri_1', + }) + ); + }); + + it('ongoing setActive call is canceled when the method is called again', async () => { + const clusterFoo = makeRootCluster({ uri: '/clusters/foo' }); + const clusterBar = makeRootCluster({ uri: '/clusters/bar' }); + const workspace1: Workspace = { + accessRequests: { + isBarCollapsed: true, + pending: getEmptyPendingAccessRequest(), + }, + localClusterUri: clusterFoo.uri, + documents: [ + { + kind: 'doc.terminal_shell', + uri: '/docs/terminal_shell_uri_1', + title: '/Users/alice/Documents', + }, + ], + location: '/docs/non-existing-doc', + }; + + const { workspacesService } = getTestSetup({ + cluster: [clusterFoo, clusterBar], + persistedWorkspaces: { [clusterFoo.uri]: workspace1 }, + }); + + workspacesService.restorePersistedState(); + await Promise.all([ + // Activating the workspace foo will be stuck on restoring previous documents, + // since we don't have any handler. This dialog will be canceled be a request + // to set workspace bar (which doesn't have any documents). + workspacesService.setActiveWorkspace(clusterFoo.uri), + workspacesService.setActiveWorkspace(clusterBar.uri), + ]); + + expect(workspacesService.getRootClusterUri()).toStrictEqual(clusterBar.uri); + }); }); function getTestSetup(options: { - cluster: tshd.Cluster | undefined; // assumes that only one cluster can be added + cluster: tshd.Cluster | tshd.Cluster[] | undefined; persistedWorkspaces: Record; }) { const { cluster } = options; @@ -381,9 +433,14 @@ function getTestSetup(options: { saveWorkspacesState: jest.fn(), }; + const normalizedClusters = ( + Array.isArray(cluster) ? cluster : [cluster] + ).filter(Boolean); const clustersService: Partial = { - findCluster: jest.fn(() => cluster), - getRootClusters: () => [cluster].filter(Boolean), + findCluster: jest.fn(clusterUri => + normalizedClusters.find(c => c.uri === clusterUri) + ), + getRootClusters: () => normalizedClusters, syncRootClustersAndCatchErrors: async () => {}, }; @@ -411,7 +468,6 @@ function getTestSetup(options: { return { workspacesService, - clusterDocument, modalsService, notificationsService, statePersistenceService, diff --git a/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts b/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts index 129b961759e2e..b21678d226a7f 100644 --- a/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts +++ b/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts @@ -16,6 +16,7 @@ * along with this program. If not, see . */ +import { Immutable, produce } from 'immer'; import { z } from 'zod'; import { @@ -28,6 +29,7 @@ import { import { useStore } from 'shared/libs/stores'; import { arrayObjectIsEqual } from 'shared/utils/highbar'; +import Logger from 'teleterm/logger'; import { ClustersService } from 'teleterm/ui/services/clusters'; import { ImmutableStore } from 'teleterm/ui/services/immutableStore'; import { ModalsService } from 'teleterm/ui/services/modals'; @@ -50,6 +52,7 @@ import { PendingAccessRequest, } from './accessRequestsService'; import { + createClusterDocument, Document, DocumentCluster, DocumentGateway, @@ -63,21 +66,13 @@ export interface WorkspacesState { rootClusterUri?: RootClusterUri; workspaces: Record; /** - * isInitialized signifies whether WorkspacesState has finished state restoration during the start - * of the app. It is useful in places that want to wait for the state to be restored before - * proceeding. - * - * If during the previous start of the app the user was logged into a workspace which cert has - * since expired, isInitialized will be set to true only _after_ the user logs in to that - * workspace (or closes the login modal). + * isInitialized signifies whether the app has finished setting up + * callbacks and restoring state during the start of the app. + * This also means that the UI can be considered visible, because soon after + * isInitialized is flipped to true, AppInitializer removes the loading indicator + * and shows the usual app UI. * * This field is not persisted to disk. - * - * Side note: Arguably, depending on the use case, the moment isInitialized is set to true could - * be changed to happen right before the modal is shown. Ultimately, the thing that interests us - * the most is whether the state from disk was loaded into memory. Maybe in the future we will - * need to separate values or an enum. - * */ isInitialized: boolean; } @@ -85,7 +80,7 @@ export interface WorkspacesState { export interface Workspace { localClusterUri: ClusterUri; documents: Document[]; - location: DocumentUri; + location: DocumentUri | undefined; accessRequests: { isBarCollapsed: boolean; pending: PendingAccessRequest; @@ -97,10 +92,13 @@ export interface Workspace { // This requires updating many of tests // where we construct the workspace manually. unifiedResourcePreferences?: UnifiedResourcePreferences; - previous?: { - documents: Document[]; - location: DocumentUri; - }; + /** + * Tracks whether the user has documents to reopen from a previous session. + * This is used to ensure that the prompt to restore a previous session is shown only once. + * + * This field is not persisted to disk. + */ + hasDocumentsToReopen?: boolean; } export class WorkspacesService extends ImmutableStore { @@ -114,6 +112,18 @@ export class WorkspacesService extends ImmutableStore { workspaces: {}, isInitialized: false, }; + /** + * Keeps the state that was restored from the disk when the app was launched. + * This state is not processed in any way, so it may, for example, + * contain clusters that are no longer available. + * When a workspace is removed, it's removed from the restored state too. + */ + private restoredState?: Immutable; + /** + * Ensures `setActiveWorkspace` calls are not executed in parallel. + * An ongoing call is canceled when a new one is initiated. + */ + private setActiveWorkspaceAbortController = new AbortController(); constructor( private modalsService: ModalsService, @@ -270,6 +280,7 @@ export class WorkspacesService extends ImmutableStore { * If the root cluster doesn't have a workspace yet, setActiveWorkspace creates a default * workspace state for the cluster and then asks the user about restoring documents from the * previous session if there are any. + * Only one call can be executed at a time. Any ongoing call is canceled when a new one is initiated. * * setActiveWorkspace never returns a rejected promise on its own. */ @@ -296,23 +307,15 @@ export class WorkspacesService extends ImmutableStore { */ isAtDesiredWorkspace: boolean; }> { - const setWorkspace = () => { - this.setState(draftState => { - // adding a new workspace - if (!draftState.workspaces[clusterUri]) { - draftState.workspaces[clusterUri] = - this.getWorkspaceDefaultState(clusterUri); - } - draftState.rootClusterUri = clusterUri; - }); - }; + this.setActiveWorkspaceAbortController.abort(); + this.setActiveWorkspaceAbortController = new AbortController(); + const abortSignal = this.setActiveWorkspaceAbortController.signal; - // empty cluster URI - no cluster selected if (!clusterUri) { this.setState(draftState => { draftState.rootClusterUri = undefined; }); - return Promise.resolve({ isAtDesiredWorkspace: true }); + return { isAtDesiredWorkspace: true }; } let cluster = this.clustersService.findCluster(clusterUri); @@ -324,11 +327,11 @@ export class WorkspacesService extends ImmutableStore { this.logger.warn( `Could not find cluster with uri ${clusterUri} when changing active cluster` ); - return Promise.resolve({ isAtDesiredWorkspace: false }); + return { isAtDesiredWorkspace: false }; } if (cluster.profileStatusError) { - await this.clustersService.syncRootClustersAndCatchErrors(); + await this.clustersService.syncRootClustersAndCatchErrors(abortSignal); // Update the cluster. cluster = this.clustersService.findCluster(clusterUri); // If the problem persists (because, for example, the user still hasn't @@ -342,57 +345,80 @@ export class WorkspacesService extends ImmutableStore { } } - return new Promise((resolve, reject) => { - if (cluster.connected) { - setWorkspace(); - return resolve(); + if (!cluster.connected) { + const connected = await new Promise(resolve => + this.modalsService.openRegularDialog( + { + kind: 'cluster-connect', + clusterUri, + reason: undefined, + prefill, + onCancel: () => resolve(false), + onSuccess: () => resolve(true), + }, + abortSignal + ) + ); + if (!connected) { + return { isAtDesiredWorkspace: false }; } - this.modalsService.openRegularDialog({ - kind: 'cluster-connect', - clusterUri, - reason: undefined, - prefill, - onCancel: () => { - reject(); - }, - onSuccess: () => { - setWorkspace(); - resolve(); + } + // If we don't have a workspace for this cluster, add it. + this.setState(draftState => { + if (!draftState.workspaces[clusterUri]) { + draftState.workspaces[clusterUri] = + getWorkspaceDefaultState(clusterUri); + } + draftState.rootClusterUri = clusterUri; + }); + + const { hasDocumentsToReopen } = this.getWorkspace(clusterUri); + if (!hasDocumentsToReopen) { + return { isAtDesiredWorkspace: true }; + } + + const restoredWorkspace = this.restoredState?.workspaces?.[clusterUri]; + const documentsReopen = await new Promise< + 'confirmed' | 'discarded' | 'canceled' + >(resolve => + this.modalsService.openRegularDialog( + { + kind: 'documents-reopen', + rootClusterUri: clusterUri, + numberOfDocuments: restoredWorkspace.documents.length, + onConfirm: () => resolve('confirmed'), + onDiscard: () => resolve('discarded'), + onCancel: () => resolve('canceled'), }, - }); - }).then( - () => { - return new Promise<{ isAtDesiredWorkspace: boolean }>(resolve => { - const previousWorkspaceState = - this.getWorkspace(clusterUri)?.previous; - if (!previousWorkspaceState) { - return resolve({ isAtDesiredWorkspace: true }); - } - const numberOfDocuments = previousWorkspaceState.documents.length; - - this.modalsService.openRegularDialog({ - kind: 'documents-reopen', - rootClusterUri: clusterUri, - numberOfDocuments, - onConfirm: () => { - this.reopenPreviousDocuments(clusterUri); - resolve({ isAtDesiredWorkspace: true }); - }, - onCancel: () => { - this.discardPreviousDocuments(clusterUri); - resolve({ isAtDesiredWorkspace: true }); - }, - }); - }); - }, - () => ({ isAtDesiredWorkspace: false }) // catch ClusterConnectDialog cancellation + abortSignal + ) ); + switch (documentsReopen) { + case 'confirmed': + this.reopenPreviousDocuments(clusterUri, { + documents: restoredWorkspace.documents, + location: restoredWorkspace.location, + }); + break; + case 'discarded': + this.discardPreviousDocuments(clusterUri); + break; + case 'canceled': + break; + default: + documentsReopen satisfies never; + } + + return { isAtDesiredWorkspace: true }; } removeWorkspace(clusterUri: RootClusterUri): void { this.setState(draftState => { delete draftState.workspaces[clusterUri]; }); + this.restoredState = produce(this.restoredState, draftState => { + delete draftState.workspaces[clusterUri]; + }); } getConnectedWorkspacesClustersUri() { @@ -401,69 +427,55 @@ export class WorkspacesService extends ImmutableStore { ); } - async restorePersistedState(): Promise { - const persistedState = this.statePersistenceService.getWorkspacesState(); + /** + * Returns the state that was restored when the app was launched. + * This state is not processed in any way, so it may, for example, + * contain clusters that are no longer available. + * When a workspace is removed, it's removed from the restored state too. + */ + getRestoredState(): Immutable | undefined { + return this.restoredState; + } + + /** + * Loads the state from disk into the app. + */ + restorePersistedState(): void { + const restoredState = this.statePersistenceService.getWorkspacesState(); + // Make the restored state immutable. + this.restoredState = produce(restoredState, () => {}); const restoredWorkspaces = this.clustersService .getRootClusters() .reduce((workspaces, cluster) => { - const persistedWorkspace = persistedState.workspaces[cluster.uri]; - const workspaceDefaultState = this.getWorkspaceDefaultState( - persistedWorkspace?.localClusterUri || cluster.uri + const restoredWorkspace = this.restoredState.workspaces[cluster.uri]; + workspaces[cluster.uri] = getWorkspaceDefaultState( + cluster.uri, + restoredWorkspace ); - const persistedWorkspaceDocuments = persistedWorkspace?.documents; - - workspaces[cluster.uri] = { - ...workspaceDefaultState, - previous: this.canReopenPreviousDocuments({ - previousDocuments: persistedWorkspaceDocuments, - currentDocuments: workspaceDefaultState.documents, - }) - ? { - location: getLocationToRestore( - persistedWorkspaceDocuments, - persistedWorkspace.location - ), - documents: persistedWorkspaceDocuments, - } - : undefined, - connectMyComputer: persistedWorkspace?.connectMyComputer, - unifiedResourcePreferences: this.parseUnifiedResourcePreferences( - persistedWorkspace?.unifiedResourcePreferences - ), - }; return workspaces; }, {}); this.setState(draftState => { draftState.workspaces = restoredWorkspaces; }); + } - if (persistedState.rootClusterUri) { - await this.setActiveWorkspace(persistedState.rootClusterUri); - } - - this.setState(draft => { - draft.isInitialized = true; + markAsInitialized(): void { + this.setState(draftState => { + draftState.isInitialized = true; }); } - // TODO(gzdunek): Parse the entire workspace state read from disk like below. - private parseUnifiedResourcePreferences( - unifiedResourcePreferences: unknown - ): UnifiedResourcePreferences | undefined { - try { - return unifiedResourcePreferencesSchema.parse( - unifiedResourcePreferences - ) as UnifiedResourcePreferencesSchemaAsRequired; - } catch (e) { - this.logger.error('Failed to parse unified resource preferences', e); + private reopenPreviousDocuments( + rootClusterUri: RootClusterUri, + reopen: { + documents: Immutable; + location: DocumentUri; } - } - - private reopenPreviousDocuments(clusterUri: RootClusterUri): void { + ): void { this.setState(draftState => { - const workspace = draftState.workspaces[clusterUri]; - workspace.documents = workspace.previous.documents.map(d => { + const workspace = draftState.workspaces[rootClusterUri]; + workspace.documents = reopen.documents.map(d => { //TODO: create a function that will prepare a new document, it will be used in: // DocumentsService // TrackedConnectionOperationsFactory @@ -500,6 +512,9 @@ export class WorkspacesService extends ImmutableStore { ...defaultParams.sort, ...d.queryParams?.sort, }, + resourceKinds: d.queryParams?.resourceKinds + ? [...d.queryParams.resourceKinds] // makes the array mutable + : defaultParams.resourceKinds, }, }; return documentCluster; @@ -507,54 +522,21 @@ export class WorkspacesService extends ImmutableStore { return d; }); - workspace.location = workspace.previous.location; - workspace.previous = undefined; + workspace.location = getLocationToRestore( + reopen.documents, + reopen.location + ); + workspace.hasDocumentsToReopen = false; }); } private discardPreviousDocuments(clusterUri: RootClusterUri): void { this.setState(draftState => { const workspace = draftState.workspaces[clusterUri]; - workspace.previous = undefined; + workspace.hasDocumentsToReopen = false; }); } - private canReopenPreviousDocuments({ - previousDocuments, - currentDocuments, - }: { - previousDocuments?: Document[]; - currentDocuments: Document[]; - }): boolean { - const omitUriAndTitle = (documents: Document[]) => - documents.map(d => ({ ...d, uri: undefined, title: undefined })); - - return ( - previousDocuments?.length && - !arrayObjectIsEqual( - omitUriAndTitle(previousDocuments), - omitUriAndTitle(currentDocuments) - ) - ); - } - - private getWorkspaceDefaultState(localClusterUri: ClusterUri): Workspace { - const rootClusterUri = routing.ensureRootClusterUri(localClusterUri); - const defaultDocument = this.getWorkspaceDocumentService( - rootClusterUri - ).createClusterDocument({ clusterUri: localClusterUri }); - return { - accessRequests: { - pending: getEmptyPendingAccessRequest(), - isBarCollapsed: false, - }, - localClusterUri, - location: defaultDocument.uri, - documents: [defaultDocument], - unifiedResourcePreferences: getDefaultUnifiedResourcePreferences(), - }; - } - private persistState(): void { const stateToSave: WorkspacesPersistedState = { rootClusterUri: this.state.rootClusterUri, @@ -562,13 +544,11 @@ export class WorkspacesService extends ImmutableStore { }; for (let w in this.state.workspaces) { const workspace = this.state.workspaces[w]; - const documentsToPersist = getDocumentsToPersist( - workspace.previous?.documents || workspace.documents - ); + const documentsToPersist = getDocumentsToPersist(workspace.documents); stateToSave.workspaces[w] = { localClusterUri: workspace.localClusterUri, - location: workspace.previous?.location || workspace.location, + location: workspace.location, documents: documentsToPersist, connectMyComputer: workspace.connectMyComputer, unifiedResourcePreferences: workspace.unifiedResourcePreferences, @@ -624,8 +604,78 @@ function getDocumentsToPersist(documents: Document[]): Document[] { } function getLocationToRestore( - documents: Document[], + documents: Immutable, location: DocumentUri ): DocumentUri | undefined { return documents.find(d => d.uri === location) ? location : documents[0]?.uri; } + +function getWorkspaceDefaultState( + rootClusterUri: RootClusterUri, + restoredWorkspace?: Immutable> +): Workspace { + const defaultDocument = createClusterDocument({ clusterUri: rootClusterUri }); + const defaultWorkspace: Workspace = { + accessRequests: { + pending: getEmptyPendingAccessRequest(), + isBarCollapsed: false, + }, + location: defaultDocument.uri, + documents: [defaultDocument], + connectMyComputer: undefined, + hasDocumentsToReopen: false, + localClusterUri: rootClusterUri, + unifiedResourcePreferences: parseUnifiedResourcePreferences(undefined), + }; + if (!restoredWorkspace) { + return defaultWorkspace; + } + + defaultWorkspace.localClusterUri = restoredWorkspace.localClusterUri; + defaultWorkspace.unifiedResourcePreferences = parseUnifiedResourcePreferences( + restoredWorkspace.unifiedResourcePreferences + ); + defaultWorkspace.connectMyComputer = restoredWorkspace.connectMyComputer; + defaultWorkspace.hasDocumentsToReopen = hasDocumentsToReopen({ + previousDocuments: restoredWorkspace.documents, + currentDocuments: defaultWorkspace.documents, + }); + + return defaultWorkspace; +} + +// TODO(gzdunek): Parse the entire workspace state read from disk like below. +function parseUnifiedResourcePreferences( + unifiedResourcePreferences: unknown +): UnifiedResourcePreferences | undefined { + try { + return unifiedResourcePreferencesSchema.parse( + unifiedResourcePreferences + ) as UnifiedResourcePreferencesSchemaAsRequired; + } catch (e) { + new Logger('WorkspacesService').error( + 'Failed to parse unified resource preferences', + e + ); + } +} + +function hasDocumentsToReopen({ + previousDocuments, + currentDocuments, +}: { + previousDocuments?: Immutable; + currentDocuments: Document[]; +}): boolean { + const omitUriAndTitle = (documents: Immutable) => + documents.map(d => ({ ...d, uri: undefined, title: undefined })); + + if (!previousDocuments?.length) { + return false; + } + + return !arrayObjectIsEqual( + omitUriAndTitle(previousDocuments), + omitUriAndTitle(currentDocuments) + ); +}