Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent activation of previous workspace when launching Connect via deep link to a different cluster #50063

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,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,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion web/packages/teleterm/src/ui/appContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,9 @@ export default class AppContext implements IAppContext {

this.subscribeToDeepLinkLaunch();
this.notifyMainProcessAboutClusterListChanges();
this.clustersService.syncGatewaysAndCatchErrors();
void this.clustersService.syncGatewaysAndCatchErrors();
await this.clustersService.syncRootClustersAndCatchErrors();
this.workspacesService.restorePersistedState();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I close the app while I'm in workspace A with a valid cert and open it again through a deep link to workspace B, I can close the login modal and continue to use workspace A without making a decision wrt restored state. The user should be forced to make an action before interacting with a workspace.

valid-cert-on-launch.mov

How do we fix this? I almost feel like DeepLinksService should always bail out to the "Connect" screen. As in, when loginAndSetActiveWorkspace is called, we check if the current workspace is equal to the workspace from the URL. If not, then we always zero rootClusterUri in WorkspacesService to force the user back to cluster choice. Otherwise, I feel it kind of makes little sense to launch the app through a link to workspace B, interact with a modal about workspace B (the login modal), then close it and be back at workspace A, just because it was the last used workspace before the app was closed.

I'm just not sure at this point how this is going to interact with the rest of the app.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ export type WorkspacesPersistedState = Omit<
WorkspacesState,
'workspaces' | 'isInitialized'
> & {
workspaces: Record<string, Omit<Workspace, 'accessRequests'>>;
workspaces: Record<
string,
Omit<Workspace, 'accessRequests' | 'documentsRestoredOrDiscarded'>
>;
};

export interface StatePersistenceState {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,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: {
Expand All @@ -69,14 +69,16 @@ describe('restoring workspace', () => {
location: '/docs/some_uri',
};

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({
Expand All @@ -91,10 +93,7 @@ describe('restoring workspace', () => {
localClusterUri: testWorkspace.localClusterUri,
documents: [expect.objectContaining({ kind: 'doc.cluster' })],
location: expect.any(String),
previous: {
documents: testWorkspace.documents,
location: testWorkspace.location,
},
documentsRestoredOrDiscarded: false,
connectMyComputer: undefined,
unifiedResourcePreferences: {
defaultTab: DefaultTab.ALL,
Expand All @@ -104,9 +103,12 @@ 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 } = getTestSetup({
cluster,
Expand All @@ -115,7 +117,7 @@ describe('restoring workspace', () => {

expect(workspacesService.state.isInitialized).toEqual(false);

await workspacesService.restorePersistedState();
workspacesService.restorePersistedState();

expect(workspacesService.state.isInitialized).toEqual(true);
expect(workspacesService.getWorkspaces()).toStrictEqual({
Expand All @@ -130,7 +132,7 @@ describe('restoring workspace', () => {
localClusterUri: cluster.uri,
documents: [expect.objectContaining({ kind: 'doc.cluster' })],
location: expect.any(String),
previous: undefined,
documentsRestoredOrDiscarded: false,
connectMyComputer: undefined,
unifiedResourcePreferences: {
defaultTab: DefaultTab.ALL,
Expand All @@ -140,53 +142,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({});
});
});

Expand Down Expand Up @@ -356,6 +312,70 @@ describe('setActiveWorkspace', () => {
);
expect(workspacesService.getRootClusterUri()).toBeUndefined();
});

it('location is set to first document if it points to non-existing document when reopening documents', async () => {
gzdunek marked this conversation as resolved.
Show resolved Hide resolved
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',
})
);
});
});

function getTestSetup(options: {
Expand Down
Loading