Skip to content

Commit

Permalink
Allow only one setActiveWorkspace execution at a time
Browse files Browse the repository at this point in the history
  • Loading branch information
gzdunek committed Dec 18, 2024
1 parent a9421c6 commit fd749e4
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 30 deletions.
18 changes: 15 additions & 3 deletions web/packages/teleterm/src/ui/services/clusters/clustersService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,22 @@ import {
PasswordlessPrompt,
CreateGatewayRequest,
} from 'gen-proto-ts/teleport/lib/teleterm/v1/service_pb';
import { isAbortError } from 'shared/utils/abortError';

import * as uri from 'teleterm/ui/uri';
import { NotificationsService } from 'teleterm/ui/services/notifications';
import { MainProcessClient } from 'teleterm/mainProcess/types';
import { UsageService } from 'teleterm/ui/services/usage';
import { AssumedRequest } from 'teleterm/services/tshd/types';
import {
TshdClient,
CloneableAbortSignal,
cloneAbortSignal,
} from 'teleterm/services/tshd';

import { ImmutableStore } from '../immutableStore';

import type * as types from './types';
import type { TshdClient, CloneableAbortSignal } from 'teleterm/services/tshd';

const { routing } = uri;

Expand Down Expand Up @@ -338,13 +343,20 @@ export class ClustersService extends ImmutableStore<types.ClustersServiceState>
]);
}

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;
}
const notificationId = this.notificationsService.notifyError({
title: 'Could not fetch root clusters',
description: error.message,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,19 +283,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(
Expand Down Expand Up @@ -370,10 +372,46 @@ describe('setActiveWorkspace', () => {
})
);
});

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<string, Workspace>;
}) {
const { cluster } = options;
Expand All @@ -397,9 +435,14 @@ function getTestSetup(options: {
saveWorkspacesState: jest.fn(),
};

const normalizedClusters = (
Array.isArray(cluster) ? cluster : [cluster]
).filter(Boolean);
const clustersService: Partial<ClustersService> = {
findCluster: jest.fn(() => cluster),
getRootClusters: () => [cluster].filter(Boolean),
findCluster: jest.fn(clusterUri =>
normalizedClusters.find(c => c.uri === clusterUri)
),
getRootClusters: () => normalizedClusters,
syncRootClustersAndCatchErrors: async () => {},
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ export class WorkspacesService extends ImmutableStore<WorkspacesState> {
* When a workspace is removed, it's removed from the restored state too.
*/
private restoredState?: Immutable<WorkspacesPersistedState>;
/**
* 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,
Expand Down Expand Up @@ -277,6 +282,7 @@ export class WorkspacesService extends ImmutableStore<WorkspacesState> {
* 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.
*/
Expand All @@ -303,6 +309,10 @@ export class WorkspacesService extends ImmutableStore<WorkspacesState> {
*/
isAtDesiredWorkspace: boolean;
}> {
this.setActiveWorkspaceAbortController.abort();
this.setActiveWorkspaceAbortController = new AbortController();
const abortSignal = this.setActiveWorkspaceAbortController.signal;

if (!clusterUri) {
this.setState(draftState => {
draftState.rootClusterUri = undefined;
Expand All @@ -323,7 +333,7 @@ export class WorkspacesService extends ImmutableStore<WorkspacesState> {
}

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
Expand All @@ -346,14 +356,17 @@ export class WorkspacesService extends ImmutableStore<WorkspacesState> {

if (!cluster.connected) {
const connected = await new Promise<boolean>(resolve =>
this.modalsService.openRegularDialog({
kind: 'cluster-connect',
clusterUri,
reason: undefined,
prefill,
onCancel: () => resolve(false),
onSuccess: () => resolve(true),
})
this.modalsService.openRegularDialog(
{
kind: 'cluster-connect',
clusterUri,
reason: undefined,
prefill,
onCancel: () => resolve(false),
onSuccess: () => resolve(true),
},
abortSignal
)
);
if (!connected) {
return { isAtDesiredWorkspace: false };
Expand Down Expand Up @@ -385,14 +398,17 @@ export class WorkspacesService extends ImmutableStore<WorkspacesState> {
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'),
})
this.modalsService.openRegularDialog(
{
kind: 'documents-reopen',
rootClusterUri: clusterUri,
numberOfDocuments: restoredWorkspace.documents.length,
onConfirm: () => resolve('confirmed'),
onDiscard: () => resolve('discarded'),
onCancel: () => resolve('canceled'),
},
abortSignal
)
);
switch (documentsReopen) {
case 'confirmed':
Expand Down

0 comments on commit fd749e4

Please sign in to comment.