Make sure that there are only ever 12 slots in a hotbar (#3379)

* Make sure that there are only ever 12 slots in a hotbar

- Send notification if hotbar is full

- Filter out on every fromStore call

- Switch to broadcast on error

- Fix addToHotbar index check

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* change when to/fromStore are located

Signed-off-by: Sebastian Malton <sebastian@malton.name>

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

@@ -19,7 +19,9 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
 
+import { anyObject } from "jest-mock-extended";
 import mockFs from "mock-fs";
+import logger from "../../main/logger";
 import { ClusterStore } from "../cluster-store";
 import { HotbarStore } from "../hotbar-store";
 
@@ -187,7 +189,7 @@ describe("HotbarStore", () => {
       hotbarStore.removeFromHotbar("catalog-entity");
       const items = hotbarStore.getActive().items.filter(Boolean);
 
-      expect(items.length).toEqual(0);
+      expect(items).toStrictEqual([]);
     });
 
     it("does nothing if removing with invalid uid", () => {
@@ -245,6 +247,43 @@ describe("HotbarStore", () => {
       expect(items.slice(0, 4)).toEqual(["catalog-entity", "minikube", "aws", "test"]);
     });
 
+    it("logs an error if cellIndex is out of bounds", () => {
+      const hotbarStore = HotbarStore.getInstance();
+
+      hotbarStore.add({ name: "hottest", id: "hottest" });
+      hotbarStore.activeHotbarId = "hottest";
+
+      const { error } = logger;
+      const mocked = jest.fn();
+
+      logger.error = mocked;
+
+      hotbarStore.addToHotbar(testCluster, -1);
+      expect(mocked).toBeCalledWith("[HOTBAR-STORE]: cannot pin entity to hotbar outside of index range", anyObject());
+
+      hotbarStore.addToHotbar(testCluster, 12);
+      expect(mocked).toBeCalledWith("[HOTBAR-STORE]: cannot pin entity to hotbar outside of index range", anyObject());
+
+      hotbarStore.addToHotbar(testCluster, 13);
+      expect(mocked).toBeCalledWith("[HOTBAR-STORE]: cannot pin entity to hotbar outside of index range", anyObject());
+
+      logger.error = error;
+    });
+
+    it("throws an error if getId is invalid or returns not a string", () => {
+      const hotbarStore = HotbarStore.getInstance();
+
+      expect(() => hotbarStore.addToHotbar({} as any)).toThrowError(TypeError);
+      expect(() => hotbarStore.addToHotbar({ getId: () => true } as any)).toThrowError(TypeError);
+    });
+
+    it("throws an error if getName is invalid or returns not a string", () => {
+      const hotbarStore = HotbarStore.getInstance();
+
+      expect(() => hotbarStore.addToHotbar({ getId: () => "" } as any)).toThrowError(TypeError);
+      expect(() => hotbarStore.addToHotbar({ getId: () => "", getName: () => 4 } as any)).toThrowError(TypeError);
+    });
+
     it("does nothing when item moved to same cell", () => {
       const hotbarStore = HotbarStore.getInstance();
 

src/common/base-store.ts

@@ -28,6 +28,8 @@ import { getAppVersion, Singleton, toJS, Disposer } from "./utils";
 import logger from "../main/logger";
 import { broadcastMessage, ipcMainOn, ipcRendererOn } from "./ipc";
 import isEqual from "lodash/isEqual";
+import { isTestEnv } from "./vars";
+import { kebabCase } from "lodash";
 
 export interface BaseStoreParams<T> extends ConfOptions<T> {
   syncOptions?: IReactionOptions;
@@ -59,12 +61,14 @@ export abstract class BaseStore<T> extends Singleton {
     const res: any = this.fromStore(this.storeConfig.store);
 
     if (res instanceof Promise || (typeof res === "object" && res && typeof res.then === "function")) {
-      console.error(`${this.name} extends BaseStore<T>'s fromStore method returns a Promise or promise-like object. This is an error and must be fixed.`);
+      console.error(`${this.constructor.name} extends BaseStore<T>'s fromStore method returns a Promise or promise-like object. This is an error and must be fixed.`);
     }
 
     this.enableSync();
 
-    logger.info(`[STORE]: LOADED from ${this.path}`);
+    if (!isTestEnv) {
+      logger.info(`[${kebabCase(this.constructor.name).toUpperCase()}]: LOADED from ${this.path}`);
+    }
   }
 
   get name() {

src/common/hotbar-store.ts

@@ -22,11 +22,12 @@
 import { action, comparer, observable, makeObservable } from "mobx";
 import { BaseStore } from "./base-store";
 import migrations from "../migrations/hotbar-store";
-import isNull from "lodash/isNull";
 import { toJS } from "./utils";
 import { CatalogEntity } from "./catalog";
 import { catalogEntity } from "../main/catalog-sources/general";
-import { Hotbar, HotbarCreateOptions, HotbarItem, getEmptyHotbar } from "./hotbar-types";
+import logger from "../main/logger";
+import { broadcastMessage, HotbarTooManyItems } from "./ipc";
+import { defaultHotbarCells, getEmptyHotbar, Hotbar, HotbarCreateOptions } from "./hotbar-types";
 
 export interface HotbarStoreModel {
   hotbars: Hotbar[];
@@ -82,6 +83,8 @@ export class HotbarStore extends BaseStore<HotbarStoreModel> {
       this.hotbars = data.hotbars;
     }
 
+    this.hotbars.forEach(ensureExactHotbarItemLength);
+
     if (data.activeHotbarId) {
       if (this.getById(data.activeHotbarId)) {
         this.activeHotbarId = data.activeHotbarId;
@@ -93,6 +96,15 @@ export class HotbarStore extends BaseStore<HotbarStoreModel> {
     }
   }
 
+  toJSON(): HotbarStoreModel {
+    const model: HotbarStoreModel = {
+      hotbars: this.hotbars,
+      activeHotbarId: this.activeHotbarId
+    };
+
+    return toJS(model);
+  }
+
   getActive() {
     return this.getById(this.activeHotbarId);
   }
@@ -139,39 +151,52 @@ export class HotbarStore extends BaseStore<HotbarStoreModel> {
   }
 
   @action
-  addToHotbar(item: CatalogEntity, cellIndex = -1) {
+  addToHotbar(item: CatalogEntity, cellIndex?: number) {
     const hotbar = this.getActive();
+    const uid = item.metadata?.uid;
+    const name = item.metadata?.name;
+
+    if (typeof uid !== "string") {
+      throw new TypeError("CatalogEntity.metadata.uid must be a string");
+    }
+
+    if (typeof name !== "string") {
+      throw new TypeError("CatalogEntity.metadata.name must be a string");
+    }
+
     const newItem = { entity: {
-      uid: item.metadata.uid,
+      uid,
-      name: item.metadata.name,
+      name,
-      source: item.metadata.source
+      source: item.metadata.source,
     }};
 
-    if (hotbar.items.find(i => i?.entity.uid === item.metadata.uid)) {
+
+    if (hotbar.items.find(i => i?.entity.uid === uid)) {
       return;
     }
 
-    if (cellIndex == -1) {
+    if (cellIndex === undefined) {
       // Add item to empty cell
-      const emptyCellIndex = hotbar.items.findIndex(isNull);
+      const emptyCellIndex = hotbar.items.indexOf(null);
 
       if (emptyCellIndex != -1) {
         hotbar.items[emptyCellIndex] = newItem;
       } else {
-        // Add new item to the end of list
+        broadcastMessage(HotbarTooManyItems);
-        hotbar.items.push(newItem);
       }
-    } else {
+    } else if (0 <= cellIndex && cellIndex < hotbar.items.length) {
       hotbar.items[cellIndex] = newItem;
+    } else {
+      logger.error(`[HOTBAR-STORE]: cannot pin entity to hotbar outside of index range`, { entityId: uid, hotbarId: hotbar.id, cellIndex, });
     }
   }
 
   @action
   removeFromHotbar(uid: string): void {
     const hotbar = this.getActive();
-    const index = hotbar.items.findIndex((i) => i?.entity.uid === uid);
+    const index = hotbar.items.findIndex(item => item?.entity.uid === uid);
 
-    if (index == -1) {
+    if (index < 0) {
       return;
     }
 
@@ -185,13 +210,10 @@ export class HotbarStore extends BaseStore<HotbarStoreModel> {
    */
   @action
   removeAllHotbarItems(uid: string) {
-    const undoItems: [Hotbar, number, HotbarItem][] = [];
-
     for (const hotbar of this.hotbars) {
       const index = hotbar.items.findIndex((i) => i?.entity.uid === uid);
 
       if (index >= 0) {
-        undoItems.push([hotbar, index, hotbar.items[index]]);
         hotbar.items[index] = null;
       }
     }
@@ -252,13 +274,33 @@ export class HotbarStore extends BaseStore<HotbarStoreModel> {
 
     hotbarStore.activeHotbarId = hotbarStore.hotbars[index].id;
   }
+}
 
-  toJSON(): HotbarStoreModel {
+/**
-    const model: HotbarStoreModel = {
+ * This function ensures that there are always exactly `defaultHotbarCells`
-      hotbars: this.hotbars,
+ * worth of items in the hotbar.
-      activeHotbarId: this.activeHotbarId
+ * @param hotbar The hotbar to modify
-    };
+ */
+function ensureExactHotbarItemLength(hotbar: Hotbar) {
+  if (hotbar.items.length === defaultHotbarCells) {
+    // if we already have `defaultHotbarCells` then we are good to stop
+    return;
+  }
 
-    return toJS(model);
+  // otherwise, keep adding empty entries until full
+  while (hotbar.items.length < defaultHotbarCells) {
+    hotbar.items.push(null);
+  }
+
+  // if for some reason the hotbar was overfilled before, remove as many entries
+  // as needed, but prefer empty slots and items at the end first.
+  while (hotbar.items.length > defaultHotbarCells) {
+    const lastNull = hotbar.items.lastIndexOf(null);
+
+    if (lastNull >= 0) {
+      hotbar.items.splice(lastNull, 1);
+    } else {
+      hotbar.items.length = defaultHotbarCells;
+    }
   }
 }

src/common/ipc/hotbar.ts

@@ -0,0 +1,22 @@
+/**
+ * Copyright (c) 2021 OpenLens Authors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to
+ * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
+ * the Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
+ * IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+export const HotbarTooManyItems = "hotbar:too-many-items";

src/common/ipc/index.ts

@@ -24,3 +24,4 @@ export * from "./invalid-kubeconfig";
 export * from "./update-available.ipc";
 export * from "./cluster.ipc";
 export * from "./type-enforced-ipc";
+export * from "./hotbar";

src/renderer/ipc/index.tsx

@@ -21,13 +21,14 @@
 
 import React from "react";
 import { ipcRenderer, IpcRendererEvent } from "electron";
-import { areArgsUpdateAvailableFromMain, UpdateAvailableChannel, onCorrect, UpdateAvailableFromMain, BackchannelArg, ClusterListNamespaceForbiddenChannel, isListNamespaceForbiddenArgs, ListNamespaceForbiddenArgs } from "../../common/ipc";
+import { areArgsUpdateAvailableFromMain, UpdateAvailableChannel, onCorrect, UpdateAvailableFromMain, BackchannelArg, ClusterListNamespaceForbiddenChannel, isListNamespaceForbiddenArgs, ListNamespaceForbiddenArgs, HotbarTooManyItems } from "../../common/ipc";
 import { Notifications, notificationsStore } from "../components/notifications";
 import { Button } from "../components/button";
 import { isMac } from "../../common/vars";
 import { ClusterStore } from "../../common/cluster-store";
 import { navigate } from "../navigation";
 import { entitySettingsURL } from "../../common/routes";
+import { defaultHotbarCells } from "../../common/hotbar-types";
 
 function sendToBackchannel(backchannel: string, notificationId: string, data: BackchannelArg): void {
   notificationsStore.remove(notificationId);
@@ -112,6 +113,10 @@ function ListNamespacesForbiddenHandler(event: IpcRendererEvent, ...[clusterId]:
   );
 }
 
+function HotbarTooManyItemsHandler(): void {
+  Notifications.error(`Cannot have more than ${defaultHotbarCells} items pinned to a hotbar`);
+}
+
 export function registerIpcHandlers() {
   onCorrect({
     source: ipcRenderer,
@@ -125,4 +130,10 @@ export function registerIpcHandlers() {
     listener: ListNamespacesForbiddenHandler,
     verifier: isListNamespaceForbiddenArgs,
   });
+  onCorrect({
+    source: ipcRenderer,
+    channel: HotbarTooManyItems,
+    listener: HotbarTooManyItemsHandler,
+    verifier: (args: unknown[]): args is [] => args.length === 0,
+  });
 }