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

gzdunek · 2024-12-11T16:01:18Z

Problem

Connect always tries to restore the previous workspace, even when it is run via a deep link. This is incorrect behavior- if the user wants to authorize a session for cluster A, we shouldn't force him to log in to a cluster B first.

Solution

Currently, restoring the workspaces from disk also actives the previously active workspace (this includes showing a login dialog if certs expired). Opening a deep link waits for that state to be restored, so it also waits for that dialog to be completed.
We don't want this, so we need to decouple workspace restoration from activating the previously active workspace. This will allow us to signal the frontend interface readiness before the login dialog finishes. When the frontend is ready, a deep link can be processed to activate its corresponding workspace.
Our modal service allows only one regular dialog is active at any time, so opening a new dialog will automatically cancel an existing one.

In other words:

The user has workspaces A and B, with workspace B previously active.
The user launches a deep link to cluster A.
The app restores the state (workspaces A and B) and starts an attempt to set workspace B as active. This attempt does not block the app's initialization.
Since the app is initialized, the deep link is processed, immediately replacing the dialog for activating workspace B with one for activating workspace A. This transition happens so quickly that the user doesn’t see the initial dialog.

While working on this fix, I cleaned up some code in workspacesService and documentsService:

setActiveWorkspace had a mix of async/await and .then, it has been rewritten to the newer syntax.
I moved some methods that do not need state outside the services.
I got rid of previous field in the workspace state. Instead, I store the entire restored state in a separate variable. I needed this, because if we don't restore the previous rootClusterUri immediately, it is overwritten with undefined in setState. I think this approach is actually simpler than what we had before.

…spaceDefaultState`

ravicious

So far I only looked through the code, I'll try to test this on Monday.

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsService.ts

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

ravicious · 2024-12-13T14:53:04Z

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

-    const persistedState = this.statePersistenceService.getWorkspacesState();
+  restorePersistedState(): void {
+    const restoredState = this.statePersistenceService.getWorkspacesState();
+    this.restoredState = restoredState;


Is there any way we can guarantee that this is not modified? Keep it as imerrized object instead of a standard JS one or something? Have a private setter and a public getter that always calls structuredClone before returning the value?

The way it is right now, it's way to easy to overwrite it by mistake or hand out references to objects inside this.restoredState. I tried using microsoft/TypeScript#13923 (comment) which would be pretty handy – the compiler would tell you to deep clone parts that you'd like to use for mutable purposes. Unfortunately, I couldn't come up with the right types for a function that would take a deep readonly value, call structuredClone on it and return a "deep mutable" version of it.

That's a good idea to make it immutable! I decided to use produce and Immutable from immer.js, I think it's the simplest option.

Yup, that seems much better than everything that I suggested.

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

avatus

I did a scan of the code but tested locally and it works. Leaving premp approval because I'm out next week

ravicious · 2024-12-16T13:39:09Z

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

@@ -119,6 +115,12 @@ export class WorkspacesService extends ImmutableStore<WorkspacesState> {
    workspaces: {},
    isInitialized: false,
  };
+  /**
+   * Keeps the state restored when the app was launched.


Suggested change

* Keeps the state restored when the app was launched.

* Keeps the state that was restored from disk when the app was launched.

ravicious · 2024-12-16T13:44:53Z

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

+              resourceKinds: d.queryParams?.resourceKinds
+                ? [...d.queryParams.resourceKinds] // makes the array mutable
+                : defaultParams.resourceKinds,


I wonder why commenting out sort above doesn't trigger a similar type error. I suppose types for readonly objects behave differently than types for readonly arrays?

ravicious · 2024-12-16T13:46:06Z

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

-    const persistedState = this.statePersistenceService.getWorkspacesState();
+  restorePersistedState(): void {
+    const restoredState = this.statePersistenceService.getWorkspacesState();
+    this.restoredState = restoredState;


Yup, that seems much better than everything that I suggested.

ravicious · 2024-12-16T14:00:26Z

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

+        });
+        break;
+      case 'discarded':
+        this.discardPreviousDocuments(clusterUri);


Could you add tests for this behavior? Specifically that closing the modal either through the X button or through the "Start new session" button discards previous documents, but opening a new regular modal does not discard them? This behavior requires a lot of things to be wired up correctly and I worry that it might be broken easily when refactoring either WorkspacesService, ModalsService, ModalsHost or how our modals work in general.

ravicious · 2024-12-16T14:06:51Z

web/packages/teleterm/src/ui/AppInitializer/AppInitializer.test.tsx

+  // The first one comes from restoring the previous session, but it is
+  // immediately canceled and replaced with a dialog to the cluster from
+  // the deep link.
+  await waitFor(


Couldn't we wait here for the connect-cluster dialog to be opened with the correct content rather than doing waitFor for a mocked function? As in expect(screen.findByText(<whatever is shown in the modal>)). If possible, we shouldn't use toHaveBeenCalled as something that moves tests forward, but rather the actual results that the action has on the interface that the user sees.

ravicious · 2024-12-16T14:32:44Z

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

-   * 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;


Let's add a comment above this line in vnetContext.tsx:

teleport/web/packages/teleterm/src/ui/Vnet/vnetContext.tsx

Line 126 in 8776788

isWorkspaceStateInitialized &&

which explains that accessing resources through VNet might trigger the MFA modal, so we have to wait for the tshd events service to be initialized.

ravicious · 2024-12-16T14:55:24Z

web/packages/teleterm/src/ui/appContext.ts

    await this.clustersService.syncRootClustersAndCatchErrors();
+    this.workspacesService.restorePersistedState();


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.

ravicious · 2024-12-16T15:14:24Z

web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts

-   * 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;


As discussed on the call, move setting this field out of restorePersistedState into a separate method that's called from pullInitialState.

gzdunek added 5 commits December 11, 2024 16:43

Refactor setActiveWorkspace to async/await

51c37e9

Keep all the logic that restores a single workspace state in `getWork…

f43b0ae

…spaceDefaultState`

Separate restoring state from setting active workspace

dc70bdf

Allow the dialog to reopen documents to be closed without any decision

a6bb953

Add a test that verifies if the correct workspace is activated

e941385

gzdunek added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 backport/branch/v17 labels Dec 11, 2024

gzdunek requested review from ravicious and avatus December 11, 2024 16:01

github-actions bot requested a review from rudream December 11, 2024 16:01

github-actions bot added size/md ui labels Dec 11, 2024

gzdunek removed the request for review from rudream December 11, 2024 16:02

ravicious reviewed Dec 13, 2024

View reviewed changes

avatus approved these changes Dec 13, 2024

View reviewed changes

ravicious self-requested a review December 13, 2024 15:36

gzdunek added 5 commits December 13, 2024 17:26

Docs improvements

9fc2003

Return early if there's no restoredWorkspace

cc2644a

Fix logger name

3dde323

Improve test name

8c4dbec

Make restored state immutable

8776788

ravicious reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gzdunek commented Dec 11, 2024

ravicious left a comment

ravicious Dec 13, 2024

gzdunek Dec 16, 2024

ravicious Dec 16, 2024

avatus left a comment

ravicious Dec 16, 2024

ravicious Dec 16, 2024

ravicious Dec 16, 2024

ravicious Dec 16, 2024

ravicious Dec 16, 2024

ravicious Dec 16, 2024 •

edited

Loading

ravicious Dec 16, 2024

ravicious Dec 16, 2024 •

edited

Loading

	* Keeps the state restored when the app was launched.
	* Keeps the state that was restored from disk when the app was launched.

		await this.clustersService.syncRootClustersAndCatchErrors();
		this.workspacesService.restorePersistedState();

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

Are you sure you want to change the base?

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

Conversation

gzdunek commented Dec 11, 2024

Problem

Solution

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

ravicious Dec 16, 2024 •

edited

Loading

ravicious Dec 16, 2024 •

edited

Loading