Fix being able to view clusters outside the current workspace (#2355)

src/common/__tests__/cluster-store.test.ts

@@ -102,12 +102,6 @@ describe("empty config", () => {
       await clusterStore.removeById("foo");
       expect(clusterStore.getById("foo")).toBeNull();
     });
-
-    it("sets active cluster", () => {
-      clusterStore.setActive("foo");
-      expect(clusterStore.active.id).toBe("foo");
-      expect(workspaceStore.currentWorkspace.lastActiveClusterId).toBe("foo");
-    });
   });
 
   describe("with prod and dev clusters added", () => {

src/common/__tests__/workspace-store.test.ts

@@ -50,6 +50,12 @@ describe("workspace store tests", () => {
       expect(() => ws.removeWorkspaceById(WorkspaceStore.defaultId)).toThrowError("Cannot remove");
     });
 
+    it("has the default workspace as active", () => {
+      const ws = WorkspaceStore.getInstance<WorkspaceStore>();
+
+      expect(ws.isActive(WorkspaceStore.defaultId)).toBe(true);
+    });
+
     it("can update workspace description", () => {
       const ws = WorkspaceStore.getInstance<WorkspaceStore>();
       const workspace = ws.addWorkspace(new Workspace({

src/common/__tests__/workspace.test.ts

@@ -0,0 +1,182 @@
+import { Workspace } from "../workspace-store";
+import { clusterStore } from "../cluster-store";
+import { Cluster } from "../../main/cluster";
+
+jest.mock("../cluster-store");
+
+const mockedClusterStore = clusterStore as jest.Mocked<typeof clusterStore>;
+
+describe("Workspace tests", () => {
+  it("should be enabled if not managed", () => {
+    const w = new Workspace({
+      id: "f",
+      name: "f"
+    });
+
+    expect(w.enabled).toBe(true);
+    expect(w.isManaged).toBe(false);
+  });
+
+  it("should not be enabled initially if managed", () => {
+    const w = new Workspace({
+      id: "f",
+      name: "f",
+      ownerRef: "f"
+    });
+
+    expect(w.enabled).toBe(false);
+    expect(w.isManaged).toBe(true);
+  });
+
+  it("should be able to be enabled when managed", () => {
+    const w = new Workspace({
+      id: "f",
+      name: "f",
+      ownerRef: "f"
+    });
+
+    expect(w.enabled).toBe(false);
+    expect(w.isManaged).toBe(true);
+
+    w.enabled = true;
+    expect(w.enabled).toBe(true);
+  });
+
+  it("should allow valid clusterId to be set to activeClusterId", () => {
+    mockedClusterStore.getById.mockImplementationOnce(id => {
+      expect(id).toBe("foobar");
+
+      return {
+        workspace: "f",
+        id
+      } as Cluster;
+    });
+
+    const w = new Workspace({
+      id: "f",
+      name: "f"
+    });
+
+    w.setActiveCluster("foobar");
+    expect(w.activeClusterId).toBe("foobar");
+  });
+
+  it("should clear activeClusterId", () => {
+    mockedClusterStore.getById.mockImplementationOnce(id => {
+      expect(id).toBe("foobar");
+
+      return {
+        workspace: "f",
+        id
+      } as Cluster;
+    });
+
+    const w = new Workspace({
+      id: "f",
+      name: "f"
+    });
+
+    w.setActiveCluster("foobar");
+    expect(w.activeClusterId).toBe("foobar");
+
+    w.clearActiveCluster();
+    expect(w.activeClusterId).toBe(undefined);
+  });
+
+  it("should disallow valid clusterId to be set to activeClusterId", () => {
+    mockedClusterStore.getById.mockImplementationOnce(id => {
+      expect(id).toBe("foobar");
+
+      return undefined;
+    });
+
+    const w = new Workspace({
+      id: "f",
+      name: "f"
+    });
+
+    w.setActiveCluster("foobar");
+    expect(w.activeClusterId).toBe(undefined);
+  });
+
+  describe("Workspace.tryClearAsCurrentActiveCluster", () => {
+    it("should return false for non-matching ID", () => {
+      mockedClusterStore.getById.mockImplementationOnce(id => {
+        expect(id).toBe("foobar");
+
+        return {
+          workspace: "f",
+          id
+        } as Cluster;
+      });
+
+      const w = new Workspace({
+        id: "f",
+        name: "f",
+        activeClusterId: "foobar"
+      });
+
+      expect(w.tryClearAsActiveCluster("fa")).toBe(false);
+      expect(w.activeClusterId).toBe("foobar");
+    });
+    it("should return false for non-matching cluster", () => {
+      mockedClusterStore.getById.mockImplementationOnce(id => {
+        expect(id).toBe("foobar");
+
+        return {
+          workspace: "f",
+          id
+        } as Cluster;
+      });
+
+      const w = new Workspace({
+        id: "f",
+        name: "f",
+        activeClusterId: "foobar"
+      });
+
+      expect(w.tryClearAsActiveCluster({ id: "fa" } as Cluster)).toBe(false);
+      expect(w.activeClusterId).toBe("foobar");
+    });
+
+    it("should return true for matching ID", () => {
+      mockedClusterStore.getById.mockImplementationOnce(id => {
+        expect(id).toBe("foobar");
+
+        return {
+          workspace: "f",
+          id
+        } as Cluster;
+      });
+
+      const w = new Workspace({
+        id: "f",
+        name: "f",
+        activeClusterId: "foobar"
+      });
+
+      expect(w.tryClearAsActiveCluster("foobar")).toBe(true);
+      expect(w.activeClusterId).toBe(undefined);
+    });
+
+    it("should return true for matching cluster", () => {
+      mockedClusterStore.getById.mockImplementationOnce(id => {
+        expect(id).toBe("foobar");
+
+        return {
+          workspace: "f",
+          id
+        } as Cluster;
+      });
+
+      const w = new Workspace({
+        id: "f",
+        name: "f",
+        activeClusterId: "foobar"
+      });
+
+      expect(w.tryClearAsActiveCluster({ id: "foobar"} as Cluster)).toBe(true);
+      expect(w.activeClusterId).toBe(undefined);
+    });
+  });
+});

src/common/cluster-store.ts

@@ -15,7 +15,6 @@ import { handleRequest, requestMain, subscribeToBroadcast, unsubscribeAllFromBro
 import _ from "lodash";
 import move from "array-move";
 import type { WorkspaceId } from "./workspace-store";
-import { ResourceType } from "../renderer/components/+cluster-settings/components/cluster-metrics-setting";
 
 export interface ClusterIconUpload {
   clusterId: string;
@@ -34,7 +33,6 @@ export type ClusterPrometheusMetadata = {
 };
 
 export interface ClusterStoreModel {
-  activeCluster?: ClusterId; // last opened cluster
   clusters?: ClusterModel[];
 }
 
@@ -106,7 +104,6 @@ export class ClusterStore extends BaseStore<ClusterStoreModel> {
     return filePath;
   }
 
-  @observable activeCluster: ClusterId;
   @observable removedClusters = observable.map<ClusterId, Cluster>();
   @observable clusters = observable.map<ClusterId, Cluster>();
 
@@ -189,10 +186,6 @@ export class ClusterStore extends BaseStore<ClusterStoreModel> {
     });
   }
 
-  get activeClusterId() {
-    return this.activeCluster;
-  }
-
   @computed get clustersList(): Cluster[] {
     return Array.from(this.clusters.values());
   }
@@ -201,40 +194,10 @@ export class ClusterStore extends BaseStore<ClusterStoreModel> {
     return this.clustersList.filter((c) => c.enabled);
   }
 
-  @computed get active(): Cluster | null {
-    return this.getById(this.activeCluster);
-  }
-
   @computed get connectedClustersList(): Cluster[] {
     return this.clustersList.filter((c) => !c.disconnected);
   }
 
-  isActive(id: ClusterId) {
-    return this.activeCluster === id;
-  }
-
-  isMetricHidden(resource: ResourceType) {
-    return Boolean(this.active?.preferences.hiddenMetrics?.includes(resource));
-  }
-
-  @action
-  setActive(clusterId: ClusterId) {
-    const cluster = this.clusters.get(clusterId);
-
-    if (!cluster?.enabled) {
-      clusterId = null;
-    }
-
-    this.activeCluster = clusterId;
-    workspaceStore.setLastActiveClusterId(clusterId);
-  }
-
-  deactivate(id: ClusterId) {
-    if (this.isActive(id)) {
-      this.setActive(null);
-    }
-  }
-
   @action
   swapIconOrders(workspace: WorkspaceId, from: number, to: number) {
     const clusters = this.getByWorkspaceId(workspace);
@@ -268,28 +231,22 @@ export class ClusterStore extends BaseStore<ClusterStoreModel> {
 
   @action
   addClusters(...models: ClusterModel[]): Cluster[] {
-    const clusters: Cluster[] = [];
-
-    models.forEach(model => {
-      clusters.push(this.addCluster(model));
-    });
-
-    return clusters;
+    return models.map(model => this.addCluster(model));
   }
 
   @action
-  addCluster(model: ClusterModel | Cluster): Cluster {
+  addCluster(clusterOrModel: ClusterModel | Cluster): Cluster {
     appEventBus.emit({ name: "cluster", action: "add" });
-    let cluster = model as Cluster;
 
-    if (!(model instanceof Cluster)) {
-      cluster = new Cluster(model);
-    }
+    const cluster = clusterOrModel instanceof Cluster
+      ? clusterOrModel
+      : new Cluster(clusterOrModel);
 
     if (!cluster.isManaged) {
       cluster.enabled = true;
     }
-    this.clusters.set(model.id, cluster);
+
+    this.clusters.set(cluster.id, cluster);
 
     return cluster;
   }
@@ -304,12 +261,9 @@ export class ClusterStore extends BaseStore<ClusterStoreModel> {
     const cluster = this.getById(clusterId);
 
     if (cluster) {
+      workspaceStore.getById(cluster.workspace)?.tryClearAsActiveCluster(cluster);
       this.clusters.delete(clusterId);
 
-      if (this.activeCluster === clusterId) {
-        this.setActive(null);
-      }
-
       // remove only custom kubeconfigs (pasted as text)
       if (cluster.kubeConfigPath == ClusterStore.getCustomKubeConfigPath(clusterId)) {
         unlink(cluster.kubeConfigPath).catch(() => null);
@@ -325,7 +279,7 @@ export class ClusterStore extends BaseStore<ClusterStoreModel> {
   }
 
   @action
-  protected fromStore({ activeCluster, clusters = [] }: ClusterStoreModel = {}) {
+  protected fromStore({ clusters = [] }: ClusterStoreModel = {}) {
     const currentClusters = this.clusters.toJS();
     const newClusters = new Map<ClusterId, Cluster>();
     const removedClusters = new Map<ClusterId, Cluster>();
@@ -353,14 +307,12 @@ export class ClusterStore extends BaseStore<ClusterStoreModel> {
       }
     });
 
-    this.activeCluster = newClusters.get(activeCluster)?.enabled ? activeCluster : null;
     this.clusters.replace(newClusters);
     this.removedClusters.replace(removedClusters);
   }
 
   toJSON(): ClusterStoreModel {
     return toJS({
-      activeCluster: this.activeCluster,
       clusters: this.clustersList.map(cluster => cluster.toJSON()),
     }, {
       recurseEverything: true

src/common/workspace-store.ts

@@ -6,9 +6,14 @@ import { appEventBus } from "./event-bus";
 import { broadcastMessage, handleRequest, requestMain } from "../common/ipc";
 import logger from "../main/logger";
 import type { ClusterId } from "./cluster-store";
+import { Cluster } from "../main/cluster";
+import migrations from "../migrations/workspace-store";
+import { clusterViewURL } from "../renderer/components/cluster-manager/cluster-view.route";
 
 export type WorkspaceId = string;
 
+export class InvariantError extends Error {}
+
 export interface WorkspaceStoreModel {
   workspaces: WorkspaceModel[];
   currentWorkspace?: WorkspaceId;
@@ -19,7 +24,7 @@ export interface WorkspaceModel {
   name: string;
   description?: string;
   ownerRef?: string;
-  lastActiveClusterId?: ClusterId;
+  activeClusterId?: ClusterId;
 }
 
 export interface WorkspaceState {
@@ -61,18 +66,19 @@ export class Workspace implements WorkspaceModel, WorkspaceState {
    */
   @observable ownerRef?: string;
 
+  @observable private _enabled = false;
+
   /**
-   * Last active cluster id
-   *
-   * @observable
+   * The active cluster within this workspace
    */
-  @observable lastActiveClusterId?: ClusterId;
+  #activeClusterId = observable.box<ClusterId | undefined>();
 
+  get activeClusterId() {
+    return this.#activeClusterId.get();
+  }
 
-  @observable private _enabled: boolean;
-
-  constructor(data: WorkspaceModel) {
-    Object.assign(this, data);
+  constructor(model: WorkspaceModel) {
+    this[updateFromModel](model);
 
     if (!ipcRenderer) {
       reaction(() => this.getState(), () => {
@@ -86,9 +92,9 @@ export class Workspace implements WorkspaceModel, WorkspaceState {
    *
    * Workspaces that don't have ownerRef will be enabled by default. Workspaces with ownerRef need to explicitly enable a workspace.
    *
-   * @observable
+   * @computed
    */
-  get enabled(): boolean {
+  @computed get enabled(): boolean {
     return !this.isManaged || this._enabled;
   }
 
@@ -98,9 +104,82 @@ export class Workspace implements WorkspaceModel, WorkspaceState {
 
   /**
    * Is workspace managed by an extension
+   *
+   * @computed
    */
-  get isManaged(): boolean {
-    return !!this.ownerRef;
+  @computed get isManaged(): boolean {
+    return Boolean(this.ownerRef);
+  }
+
+  @computed get activeCluster(): Cluster | undefined {
+    return clusterStore.getById(this.activeClusterId);
+  }
+
+  /**
+   * Resolves the clusterId or cluster, checking some invariants
+   * @param clusterOrId The ID or cluster object to resolve
+   * @returns A Cluster instance of the specified cluster if it is in this workspace
+   * @throws if provided a falsey value or if it is an unknown ClusterId or if
+   * the cluster is not in this workspace.
+   */
+  private resolveClusterOrId(clusterOrId: ClusterId | Cluster): Cluster {
+    if (!clusterOrId) {
+      throw new InvariantError("Must provide a Cluster or a ClusterId");
+    }
+
+    const cluster = typeof clusterOrId === "string"
+      ? clusterStore.getById(clusterOrId)
+      : clusterOrId;
+
+    if (!cluster) {
+      throw new InvariantError(`ClusterId ${clusterOrId} is invalid`);
+    }
+
+    if (cluster.workspace !== this.id) {
+      throw new InvariantError(`Cluster ${cluster.name} is not in Workspace ${this.name}`);
+    }
+
+    return cluster;
+  }
+
+  /**
+   * Sets workspace's active cluster to resolved `clusterOrId`. As long as it
+   * is valid
+   * @param clusterOrId the cluster instance or its ID
+   */
+  @action setActiveCluster(clusterOrId?: ClusterId | Cluster) {
+    try {
+      if (clusterOrId === undefined) {
+        this.#activeClusterId.set(undefined);
+      } else {
+        this.#activeClusterId.set(this.resolveClusterOrId(clusterOrId).id);
+      }
+    } catch (error) {
+      logger.error("[WORKSPACE]: activeClusterId was attempted to be set to an invalid value", { error, workspaceName: this.name });
+    }
+  }
+
+  /**
+   * Tries to clear the cluster as this workspace's activeCluster.
+   * @param clusterOrId the cluster instance or its ID
+   * @returns true if it matches the `activeClusterId` (and is thus cleared) else false
+   */
+  @action tryClearAsActiveCluster(clusterOrId: ClusterId | Cluster): boolean {
+    const clusterId = typeof clusterOrId === "string"
+      ? clusterOrId
+      : clusterOrId.id;
+
+    const clearActive = this.activeClusterId === clusterId;
+
+    if (clearActive) {
+      this.clearActiveCluster();
+    }
+
+    return clearActive;
+  }
+
+  @action clearActiveCluster() {
+    this.#activeClusterId.set(undefined);
   }
 
   /**
@@ -129,11 +208,15 @@ export class Workspace implements WorkspaceModel, WorkspaceState {
    * @param state workspace state
    */
   @action setState(state: WorkspaceState) {
-    Object.assign(this, state);
+    this.enabled = state.enabled;
   }
 
   [updateFromModel] = action((model: WorkspaceModel) => {
-    Object.assign(this, model);
+    this.id = model.id;
+    this.name = model.name;
+    this.description = model.description;
+    this.ownerRef = model.ownerRef;
+    this.setActiveCluster(model.activeClusterId);
   });
 
   toJSON(): WorkspaceModel {
@@ -142,7 +225,7 @@ export class Workspace implements WorkspaceModel, WorkspaceState {
       name: this.name,
       description: this.description,
       ownerRef: this.ownerRef,
-      lastActiveClusterId: this.lastActiveClusterId
+      activeClusterId: this.activeClusterId,
     });
   }
 }
@@ -152,16 +235,18 @@ export class WorkspaceStore extends BaseStore<WorkspaceStoreModel> {
   private static stateRequestChannel = "workspace:states";
 
   @observable currentWorkspaceId = WorkspaceStore.defaultId;
+
   @observable workspaces = observable.map<WorkspaceId, Workspace>();
 
   private constructor() {
     super({
       configName: "lens-workspace-store",
+      migrations
     });
 
     this.workspaces.set(WorkspaceStore.defaultId, new Workspace({
       id: WorkspaceStore.defaultId,
-      name: "default"
+      name: "default",
     }));
   }
 
@@ -233,6 +318,19 @@ export class WorkspaceStore extends BaseStore<WorkspaceStoreModel> {
     return id === WorkspaceStore.defaultId;
   }
 
+  /**
+   * Checks if `workspaceOrId` represents `WorkspaceStore.currentWorkspaceId`
+   * @param workspaceOrId The workspace or its ID
+   * @returns true if the given workspace is the currently active on
+   */
+  isActive(workspaceOrId: Workspace | WorkspaceId): boolean {
+    const workspaceId = typeof workspaceOrId === "string"
+      ? workspaceOrId
+      : workspaceOrId.id;
+
+    return this.currentWorkspaceId === workspaceId;
+  }
+
   getById(id: WorkspaceId): Workspace {
     return this.workspaces.get(id);
   }
@@ -248,9 +346,34 @@ export class WorkspaceStore extends BaseStore<WorkspaceStoreModel> {
     if (!this.getById(id)) {
       throw new Error(`workspace ${id} doesn't exist`);
     }
+
     this.currentWorkspaceId = id;
   }
 
+  @action
+  async setActiveCluster(clusterOrId: ClusterId | Cluster): Promise<void> {
+    const cluster = typeof clusterOrId === "string"
+      ? clusterStore.getById(clusterOrId)
+      : clusterOrId;
+
+    if (!cluster?.enabled) {
+      throw new Error(`cluster ${(clusterOrId as Cluster)?.id ?? clusterOrId} doesn't exist`);
+    }
+
+    this.setActive(this.getById(cluster.workspace).id);
+
+    if (ipcRenderer) {
+      const { navigate } = await import("../renderer/navigation");
+
+      navigate(clusterViewURL({ params: { clusterId: cluster.id } }));
+    } else {
+      const { WindowManager } = await import("../main/window-manager");
+      const windowManager = WindowManager.getInstance() as any;
+
+      await windowManager.navigate(clusterViewURL({ params: { clusterId: cluster.id } }));
+    }
+  }
+
   @action
   addWorkspace(workspace: Workspace) {
     const { id, name } = workspace;
@@ -293,14 +416,20 @@ export class WorkspaceStore extends BaseStore<WorkspaceStoreModel> {
     if (this.currentWorkspaceId === id) {
       this.currentWorkspaceId = WorkspaceStore.defaultId; // reset to default
     }
+
     this.workspaces.delete(id);
+
     appEventBus.emit({name: "workspace", action: "remove"});
     clusterStore.removeByWorkspaceId(id);
   }
 
   @action
-  setLastActiveClusterId(clusterId?: ClusterId, workspaceId = this.currentWorkspaceId) {
-    this.getById(workspaceId).lastActiveClusterId = clusterId;
+  /**
+   * Attempts to clear `cluster` as the `activeCluster` from its own workspace
+   * @returns true if the cluster was previously the active one for its workspace
+   */
+  tryClearAsActiveCluster(cluster: Cluster): boolean {
+    return this.getById(cluster.workspace).tryClearAsActiveCluster(cluster);
   }
 
   @action