From 17b14f9871531a35299839dc69824f9268399c6e Mon Sep 17 00:00:00 2001 From: Sebastian Malton Date: Wed, 4 Aug 2021 03:37:08 -0400 Subject: [PATCH] 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 * change when to/fromStore are located Signed-off-by: Sebastian Malton --- src/common/__tests__/hotbar-store.test.ts | 41 ++++++++++- src/common/base-store.ts | 8 ++- src/common/hotbar-store.ts | 88 +++++++++++++++++------ src/common/ipc/hotbar.ts | 22 ++++++ src/common/ipc/index.ts | 1 + src/renderer/ipc/index.tsx | 13 +++- 6 files changed, 146 insertions(+), 27 deletions(-) create mode 100644 src/common/ipc/hotbar.ts diff --git a/src/common/__tests__/hotbar-store.test.ts b/src/common/__tests__/hotbar-store.test.ts index 267fbe3e8e..8207aaafac 100644 --- a/src/common/__tests__/hotbar-store.test.ts +++ b/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(); diff --git a/src/common/base-store.ts b/src/common/base-store.ts index 095e0c1877..c311bd8808 100644 --- a/src/common/base-store.ts +++ b/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 extends ConfOptions { syncOptions?: IReactionOptions; @@ -59,12 +61,14 @@ export abstract class BaseStore 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'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'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() { diff --git a/src/common/hotbar-store.ts b/src/common/hotbar-store.ts index 6e771473fb..5bfb1ac73a 100644 --- a/src/common/hotbar-store.ts +++ b/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 { 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 { } } + 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 { } @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, - name: item.metadata.name, - source: item.metadata.source + uid, + name, + 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 - hotbar.items.push(newItem); + broadcastMessage(HotbarTooManyItems); } - } 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 { */ @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 { hotbarStore.activeHotbarId = hotbarStore.hotbars[index].id; } +} - toJSON(): HotbarStoreModel { - const model: HotbarStoreModel = { - hotbars: this.hotbars, - activeHotbarId: this.activeHotbarId - }; +/** + * This function ensures that there are always exactly `defaultHotbarCells` + * worth of items in the hotbar. + * @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; + } } } diff --git a/src/common/ipc/hotbar.ts b/src/common/ipc/hotbar.ts new file mode 100644 index 0000000000..01f91309ca --- /dev/null +++ b/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"; diff --git a/src/common/ipc/index.ts b/src/common/ipc/index.ts index f412e656e8..13c6af2fcb 100644 --- a/src/common/ipc/index.ts +++ b/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"; diff --git a/src/renderer/ipc/index.tsx b/src/renderer/ipc/index.tsx index 338f71c38d..139f4bd362 100644 --- a/src/renderer/ipc/index.tsx +++ b/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, + }); }