From 175c7f05258b08b125f5a8c124e2366171c2c9e6 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Sat, 26 Oct 2024 11:17:01 -0400 Subject: [PATCH] Fix mod component deletion behavior --- .../useCreateModFromModComponent.test.ts | 38 +++++-- .../hooks/useCreateModFromModComponent.ts | 14 ++- .../hooks/useDeleteDraftModComponent.test.ts | 98 ++++++++++++++----- .../hooks/useDeleteDraftModComponent.tsx | 21 +++- .../DraftModComponentListItem.test.tsx | 5 +- .../ModComponentActionMenu.tsx | 15 ++- .../modListingPanel/ModListingPanel.test.tsx | 21 +++- .../DraftModComponentListItem.test.tsx.snap | 2 +- .../store/editor/editorInvariantMiddleware.ts | 32 +++++- .../editorSelectors/editorModSelectors.ts | 18 ++++ .../store/editor/editorSliceHelpers.ts | 37 ++++--- 11 files changed, 233 insertions(+), 68 deletions(-) diff --git a/src/pageEditor/hooks/useCreateModFromModComponent.test.ts b/src/pageEditor/hooks/useCreateModFromModComponent.test.ts index dcd8a21d14..1e6a537f21 100644 --- a/src/pageEditor/hooks/useCreateModFromModComponent.test.ts +++ b/src/pageEditor/hooks/useCreateModFromModComponent.test.ts @@ -16,13 +16,14 @@ */ import useCreateModFromModComponent from "@/pageEditor/hooks/useCreateModFromModComponent"; -import { hookAct, renderHook, waitFor } from "@/pageEditor/testHelpers"; +import { renderHook, waitFor } from "@/pageEditor/testHelpers"; import { appApiMock } from "@/testUtils/appApiMock"; import { modMetadataFactory } from "@/testUtils/factories/modComponentFactories"; import { menuItemFormStateFactory } from "@/testUtils/factories/pageEditorFactories"; import reportEvent from "@/telemetry/reportEvent"; import { Events } from "@/telemetry/events"; import { API_PATHS } from "@/data/service/urlPaths"; +import { actions as editorActions } from "@/pageEditor/store/editor/editorSlice"; const reportEventMock = jest.mocked(reportEvent); @@ -42,7 +43,7 @@ describe("useCreateModFromModComponent", () => { appApiMock.reset(); }); - it("saves with no dirty changes", async () => { + it("creates mod with no dirty changes", async () => { const metadata = modMetadataFactory(); const menuItemFormState = menuItemFormStateFactory({ modMetadata: metadata, @@ -54,15 +55,15 @@ describe("useCreateModFromModComponent", () => { appApiMock.onGet(API_PATHS.BRICKS).reply(200, []); - const { result } = renderHook(() => useCreateModFromModComponent()); - - await hookAct(async () => { - // Wait for editable packages to be created + const { result, act } = renderHook(() => useCreateModFromModComponent(), { + setupRedux(dispatch) { + dispatch(editorActions.addModComponentFormState(menuItemFormState)); + }, }); - await hookAct(async () => { + await act(async () => { await result.current.createModFromComponent(menuItemFormState, metadata, { - keepLocalCopy: false, + keepLocalCopy: true, }); }); @@ -75,4 +76,25 @@ describe("useCreateModFromModComponent", () => { ); }); }); + + it("errors moving last mod component in a mod", async () => { + const metadata = modMetadataFactory(); + const menuItemFormState = menuItemFormStateFactory({ + modMetadata: metadata, + }); + + const { result } = renderHook(() => useCreateModFromModComponent(), { + setupRedux(dispatch) { + dispatch(editorActions.addModComponentFormState(menuItemFormState)); + }, + }); + + await expect( + result.current.createModFromComponent(menuItemFormState, metadata, { + keepLocalCopy: false, + }), + ).rejects.toThrow("Cannot remove the last starter brick in a mod"); + + expect(appApiMock.history.post).toHaveLength(0); + }); }); diff --git a/src/pageEditor/hooks/useCreateModFromModComponent.ts b/src/pageEditor/hooks/useCreateModFromModComponent.ts index 07e830b51e..49f1b32abc 100644 --- a/src/pageEditor/hooks/useCreateModFromModComponent.ts +++ b/src/pageEditor/hooks/useCreateModFromModComponent.ts @@ -22,7 +22,7 @@ import reportEvent from "@/telemetry/reportEvent"; import { useCallback } from "react"; import { Events } from "@/telemetry/events"; import { useCreateModDefinitionMutation } from "@/data/service/api"; -import { useDispatch } from "react-redux"; +import { useDispatch, useSelector } from "react-redux"; import { actions as editorActions } from "@/pageEditor/store/editor/editorSlice"; import { mapModDefinitionUpsertResponseToModDefinition } from "@/pageEditor/utils"; import useDeleteDraftModComponent from "@/pageEditor/hooks/useDeleteDraftModComponent"; @@ -30,6 +30,7 @@ import useBuildAndValidateMod from "@/pageEditor/hooks/useBuildAndValidateMod"; import { createPrivateSharing } from "@/utils/registryUtils"; import updateReduxForSavedModDefinition from "@/pageEditor/hooks/updateReduxForSavedModDefinition"; import { type AppDispatch } from "@/pageEditor/store/store"; +import { selectGetSiblingDraftModComponents } from "@/pageEditor/store/editor/editorSelectors"; type UseCreateModFromModReturn = { createModFromComponent: ( @@ -44,6 +45,9 @@ function useCreateModFromModComponent(): UseCreateModFromModReturn { const [createModDefinitionOnServer] = useCreateModDefinitionMutation(); const deleteDraftModComponent = useDeleteDraftModComponent(); const { buildAndValidateMod } = useBuildAndValidateMod(); + const getSiblingDraftModComponents = useSelector( + selectGetSiblingDraftModComponents, + ); const createModFromComponent = useCallback( ( @@ -60,6 +64,13 @@ function useCreateModFromModComponent(): UseCreateModFromModReturn { return; } + if ( + !keepLocalCopy && + getSiblingDraftModComponents(modComponentFormState.uuid).length === 1 + ) { + throw new Error("Cannot remove the last starter brick in a mod"); + } + const modId = newModMetadata.id; const draftModComponents = [modComponentFormState]; @@ -104,6 +115,7 @@ function useCreateModFromModComponent(): UseCreateModFromModReturn { createModDefinitionOnServer, dispatch, deleteDraftModComponent, + getSiblingDraftModComponents, ], ); diff --git a/src/pageEditor/hooks/useDeleteDraftModComponent.test.ts b/src/pageEditor/hooks/useDeleteDraftModComponent.test.ts index 6fc758cab5..c65670171c 100644 --- a/src/pageEditor/hooks/useDeleteDraftModComponent.test.ts +++ b/src/pageEditor/hooks/useDeleteDraftModComponent.test.ts @@ -17,39 +17,91 @@ import { renderHook } from "@/pageEditor/testHelpers"; import useDeleteDraftModComponent from "./useDeleteDraftModComponent"; -import { actions as editorActions } from "@/pageEditor/store/editor/editorSlice"; +import { + actions, + actions as editorActions, +} from "@/pageEditor/store/editor/editorSlice"; import { removeDraftModComponents } from "@/contentScript/messenger/api"; - -import { autoUUIDSequence } from "@/testUtils/factories/stringFactories"; +import { formStateFactory } from "@/testUtils/factories/pageEditorFactories"; +import { modMetadataFactory } from "@/testUtils/factories/modComponentFactories"; beforeEach(() => { jest.resetAllMocks(); }); -test("useDeleteModComponent", async () => { - const modComponentId = autoUUIDSequence(); +describe("useDeleteModComponent", () => { + it("deletes one component from mod", async () => { + const modMetadata = modMetadataFactory(); - const { - result: { current: deleteDraftModComponent }, - getReduxStore, - } = renderHook(() => useDeleteDraftModComponent(), { - setupRedux(_dispatch, { store }) { - jest.spyOn(store, "dispatch"); - }, - }); + const formState = formStateFactory({ + formStateConfig: { + modMetadata, + }, + }); + + const { uuid: modComponentId } = formState; + + const { + result: { current: deleteDraftModComponent }, + getReduxStore, + } = renderHook(() => useDeleteDraftModComponent(), { + setupRedux(dispatch, { store }) { + jest.spyOn(store, "dispatch"); + // Add another so they can be deleted + dispatch( + actions.addModComponentFormState( + formStateFactory({ + formStateConfig: { + modMetadata, + }, + }), + ), + ); + + dispatch(actions.addModComponentFormState(formState)); + }, + }); + + await deleteDraftModComponent({ + modComponentId, + }); - await deleteDraftModComponent({ - modComponentId, + const { dispatch } = getReduxStore(); + + expect(dispatch).toHaveBeenCalledWith( + editorActions.markModComponentFormStateAsDeleted(modComponentId), + ); + + expect(removeDraftModComponents).toHaveBeenCalledWith( + expect.any(Object), + modComponentId, + ); }); - const { dispatch } = getReduxStore(); + it("does not delete last mod component in mod", async () => { + const formState = formStateFactory(); + const { uuid: modComponentId } = formState; + + const { + result: { current: deleteDraftModComponent }, + getReduxStore, + } = renderHook(() => useDeleteDraftModComponent(), { + setupRedux(dispatch, { store }) { + jest.spyOn(store, "dispatch"); + dispatch(actions.addModComponentFormState(formState)); + }, + }); - expect(dispatch).toHaveBeenCalledWith( - editorActions.markModComponentFormStateAsDeleted(modComponentId), - ); + await deleteDraftModComponent({ + modComponentId, + }); - expect(removeDraftModComponents).toHaveBeenCalledWith( - expect.any(Object), - modComponentId, - ); + const { dispatch } = getReduxStore(); + + expect(dispatch).not.toHaveBeenCalledWith( + editorActions.markModComponentFormStateAsDeleted(modComponentId), + ); + + expect(removeDraftModComponents).not.toHaveBeenCalled(); + }); }); diff --git a/src/pageEditor/hooks/useDeleteDraftModComponent.tsx b/src/pageEditor/hooks/useDeleteDraftModComponent.tsx index 20f6ec3dbe..9a830680a6 100644 --- a/src/pageEditor/hooks/useDeleteDraftModComponent.tsx +++ b/src/pageEditor/hooks/useDeleteDraftModComponent.tsx @@ -30,6 +30,7 @@ import { actions as editorActions } from "@/pageEditor/store/editor/editorSlice" import { removeDraftModComponents } from "@/contentScript/messenger/api"; import { allFramesInInspectedTab } from "@/pageEditor/context/connection"; import { selectActivatedModComponentsMap } from "@/store/modComponents/modComponentSelectors"; +import { selectGetSiblingDraftModComponents } from "@/pageEditor/store/editor/editorSelectors"; type DeleteConfig = { modComponentId: UUID; @@ -63,6 +64,9 @@ function useDeleteDraftModComponent(): ( ) => Promise { const dispatch = useDispatch(); const sessionId = useSelector(selectSessionId); + const getSiblingDraftModComponents = useSelector( + selectGetSiblingDraftModComponents, + ); const activatedModComponentsMap = useSelector( selectActivatedModComponentsMap, ); @@ -70,6 +74,15 @@ function useDeleteDraftModComponent(): ( return useCallback( async ({ modComponentId, shouldShowConfirmation }) => { + // Prevent deletion of the last mod component in a mod. The editorSlice state currently stores some mod + // information on the mod components/form state. + if (getSiblingDraftModComponents(modComponentId).length === 1) { + notify.warning( + "You cannot delete/remove the last starter brick in a mod", + ); + return; + } + if (shouldShowConfirmation) { const confirm = await showConfirmation( activatedModComponentsMap.get(modComponentId) @@ -102,7 +115,13 @@ function useDeleteDraftModComponent(): ( }); } }, - [dispatch, sessionId, showConfirmation, activatedModComponentsMap], + [ + dispatch, + sessionId, + showConfirmation, + activatedModComponentsMap, + getSiblingDraftModComponents, + ], ); } diff --git a/src/pageEditor/modListingPanel/DraftModComponentListItem.test.tsx b/src/pageEditor/modListingPanel/DraftModComponentListItem.test.tsx index 1d3e28b1d8..92d737511c 100644 --- a/src/pageEditor/modListingPanel/DraftModComponentListItem.test.tsx +++ b/src/pageEditor/modListingPanel/DraftModComponentListItem.test.tsx @@ -19,9 +19,7 @@ import React from "react"; import { render } from "@/pageEditor/testHelpers"; import DraftModComponentListItem from "@/pageEditor/modListingPanel/DraftModComponentListItem"; import { actions as editorActions } from "@/pageEditor/store/editor/editorSlice"; -import { authActions } from "@/auth/authSlice"; import { formStateFactory } from "@/testUtils/factories/pageEditorFactories"; -import { authStateFactory } from "@/testUtils/factories/authFactories"; jest.mock("@/modDefinitions/modDefinitionHooks", () => ({ useAllModDefinitions: jest @@ -55,7 +53,6 @@ describe("DraftModComponentListItem", () => { { initialValues: formState, setupRedux(dispatch) { - dispatch(authActions.setAuth(authStateFactory())); // The addElement also sets the active element dispatch( editorActions.addModComponentFormState(formStateFactory()), @@ -63,6 +60,7 @@ describe("DraftModComponentListItem", () => { // Add new element to deactivate the previous one dispatch(editorActions.addModComponentFormState(formState)); + // Remove the active element and stay with one inactive item dispatch( editorActions.markModComponentFormStateAsDeleted(formState.uuid), @@ -84,7 +82,6 @@ describe("DraftModComponentListItem", () => { { initialValues: formState, setupRedux(dispatch) { - dispatch(authActions.setAuth(authStateFactory())); // The addElement also sets the active element dispatch(editorActions.addModComponentFormState(formState)); }, diff --git a/src/pageEditor/modListingPanel/ModComponentActionMenu.tsx b/src/pageEditor/modListingPanel/ModComponentActionMenu.tsx index cfa606ad7e..7d96c01f2f 100644 --- a/src/pageEditor/modListingPanel/ModComponentActionMenu.tsx +++ b/src/pageEditor/modListingPanel/ModComponentActionMenu.tsx @@ -30,7 +30,10 @@ import EllipsisMenu, { import useDeleteDraftModComponent from "@/pageEditor/hooks/useDeleteDraftModComponent"; import { type ModComponentFormState } from "@/pageEditor/starterBricks/formStateTypes"; import { useDispatch, useSelector } from "react-redux"; -import { selectModComponentIsDirty } from "@/pageEditor/store/editor/editorSelectors"; +import { + selectGetSiblingDraftModComponents, + selectModComponentIsDirty, +} from "@/pageEditor/store/editor/editorSelectors"; import useClearModComponentChanges from "@/pageEditor/hooks/useClearModComponentChanges"; import { actions } from "@/pageEditor/store/editor/editorSlice"; @@ -43,6 +46,14 @@ const ModComponentActionMenu: React.FC<{ const deleteDraftModComponent = useDeleteDraftModComponent(); const clearModComponentChanges = useClearModComponentChanges(); + // Prevent deletion of the last mod component in a mod. The editorSlice state currently stores some mod + // information on the mod components/form state. + const getSiblingDraftModComponents = useSelector( + selectGetSiblingDraftModComponents, + ); + const allowDelete = + getSiblingDraftModComponents(modComponentFormState.uuid).length > 1; + const isDirty = useSelector( selectModComponentIsDirty(modComponentFormState.uuid), ); @@ -79,6 +90,7 @@ const ModComponentActionMenu: React.FC<{ className={styles.moveIcon} /> ), + disabled: !allowDelete, async action() { dispatch( actions.showMoveCopyToModModal({ @@ -103,6 +115,7 @@ const ModComponentActionMenu: React.FC<{ { title: "Delete", icon: , + disabled: !allowDelete, async action() { await deleteDraftModComponent({ modComponentId: modComponentFormState.uuid, diff --git a/src/pageEditor/modListingPanel/ModListingPanel.test.tsx b/src/pageEditor/modListingPanel/ModListingPanel.test.tsx index d86f9a9ca9..332a1453bb 100644 --- a/src/pageEditor/modListingPanel/ModListingPanel.test.tsx +++ b/src/pageEditor/modListingPanel/ModListingPanel.test.tsx @@ -165,8 +165,7 @@ describe("ModListingPanel", () => { expect(screen.getByText(`${formState.label} (Copy)`)).toBeInTheDocument(); }); - // Regression test for the existing behavior. In the future, we might try to keep the empty mod item displayed - it("delete last activated mod component removes entire mod", async () => { + it("prevents deleting last mod component in a mod", async () => { const modDefinition = modDefinitionFactory(); const componentName = modDefinition.extensionPoints[0]!.label; @@ -196,11 +195,23 @@ describe("ModListingPanel", () => { await clickEllipsesMenu(componentName); - await clickMenuItemAndConfirm("Delete"); + expect( + screen.getByRole("menuitem", { + name: "Delete", + }), + ).toHaveAttribute("aria-disabled", "true"); + + expect( + screen.getByRole("menuitem", { + name: "Move to mod", + }), + ).toHaveAttribute("aria-disabled", "true"); expect( - screen.queryByText(modDefinition.metadata.name), - ).not.toBeInTheDocument(); + screen.getByRole("menuitem", { + name: "Copy to mod", + }), + ).not.toHaveAttribute("aria-disabled", "true"); }); it("deletes an activated mod component and undeletes on clear changes", async () => { diff --git a/src/pageEditor/modListingPanel/__snapshots__/DraftModComponentListItem.test.tsx.snap b/src/pageEditor/modListingPanel/__snapshots__/DraftModComponentListItem.test.tsx.snap index e4933e0f4f..3fce6b09bb 100644 --- a/src/pageEditor/modListingPanel/__snapshots__/DraftModComponentListItem.test.tsx.snap +++ b/src/pageEditor/modListingPanel/__snapshots__/DraftModComponentListItem.test.tsx.snap @@ -85,7 +85,7 @@ exports[`DraftModComponentListItem renders not active element 1`] = ` action="#" >
x.uuid).length !== + modComponentFormStates.length + ) { + throw new InvariantViolationError( + "modComponentFormStates contains duplicate mod component ids", + ); + } + + // Assert that mod component deletion flags are consistent with modComponentFormStates. In the future, this could + // be enforced by instead tracking an isDeleted flag on the form state. const deletedModComponentIds = selectAllDeletedModComponentIds(state); if ( - selectModComponentFormStates(state).some(({ uuid }) => - deletedModComponentIds.has(uuid), - ) + modComponentFormStates.some(({ uuid }) => deletedModComponentIds.has(uuid)) ) { throw new InvariantViolationError( "modComponentFormStates includes deleted mod component", @@ -81,7 +95,15 @@ export function assertEditorInvariants(state: EditorRootState): void { const editorInvariantMiddleware: Middleware = (storeAPI) => (next: Dispatch) => (action: AnyAction) => { const result = next(action); - assertEditorInvariants(storeAPI.getState()); + + try { + assertEditorInvariants(storeAPI.getState()); + } catch (error) { + throw new Error(`Action violated invariant: ${action.type}`, { + cause: error, + }); + } + return result; }; diff --git a/src/pageEditor/store/editor/editorSelectors/editorModSelectors.ts b/src/pageEditor/store/editor/editorSelectors/editorModSelectors.ts index 18de145884..0663527247 100644 --- a/src/pageEditor/store/editor/editorSelectors/editorModSelectors.ts +++ b/src/pageEditor/store/editor/editorSelectors/editorModSelectors.ts @@ -30,6 +30,8 @@ import { } from "@/pageEditor/store/editor/editorSelectors/editorModComponentSelectors"; import { selectActiveModId } from "@/pageEditor/store/editor/editorSelectors/editorNavigationSelectors"; import type { ModMetadata } from "@/types/modComponentTypes"; +import type { UUID } from "@/types/stringTypes"; +import { assertNotNullish } from "@/utils/nullishUtils"; /** * Select the mod id associated with the selected mod package or mod component. Should be used if the caller doesn't @@ -184,6 +186,22 @@ export const selectGetDraftModComponentsForMod = createSelector( }), ); +export const selectGetSiblingDraftModComponents = createSelector( + selectModComponentFormStates, + selectGetDraftModComponentsForMod, + (modComponentFormStates, getDraftModComponentsForMod) => + memoize((modComponentId: UUID) => { + const modComponentFormState = modComponentFormStates.find( + (x) => x.uuid === modComponentId, + ); + assertNotNullish( + modComponentFormState, + "Expected matching modComponentFormState", + ); + return getDraftModComponentsForMod(modComponentFormState.modMetadata.id); + }), +); + /// /// MOD OPTIONS ARGS /// diff --git a/src/pageEditor/store/editor/editorSliceHelpers.ts b/src/pageEditor/store/editor/editorSliceHelpers.ts index 0ff8e98f56..22ff298a18 100644 --- a/src/pageEditor/store/editor/editorSliceHelpers.ts +++ b/src/pageEditor/store/editor/editorSliceHelpers.ts @@ -126,21 +126,15 @@ export function markModComponentFormStateAsDeleted( state: Draft, formStateId: UUID, ) { - if (state.activeModComponentId === formStateId) { - state.activeModComponentId = null; - } - // Some mod components in a mod may not have a corresponding mod component form state due to having never been // selected by the user in the UI. In this case, the mod component form state will not be in Redux. - // In practice, in the Page Editor UI, the user must select a mod component to remove it, so there will be - // a mod component form state - const formStateIndex = state.modComponentFormStates.findIndex( + // However, in practice, in the Page Editor UI, the user must select a mod component to remove it, so there must + // be a corresponding form state in modComponentFormStates + const [removedFormState] = remove( + state.modComponentFormStates, (x) => x.uuid === formStateId, ); - const formState = state.modComponentFormStates[formStateIndex]; - if (formStateIndex > -1) { - state.modComponentFormStates.splice(formStateIndex, 1); - } + assertNotNullish(removedFormState, "modComponentFormState not found with id"); delete state.dirty[formStateId]; delete state.brickPipelineUIStateById[formStateId]; @@ -148,16 +142,21 @@ export function markModComponentFormStateAsDeleted( // Remove from list mod components available on the page, if available remove(state.availableDraftModComponentIds, formStateId); - if (formState) { - // XXX: ideally this would only mark if the form state corresponds to an activated mod component. However, - // there's currently no way to determine if there's an activated mod component solely from the form state. - // The effect of adding the draft to deletedModComponentFormStatesByModId is benign - the mod will show as dirty - // even if the only change is that you added/removed a draft mod component. - // See discussion at: https://github.com/pixiebrix/pixiebrix-extension/pull/9320 - (state.deletedModComponentFormStatesByModId[formState.modMetadata.id] ??= - []).push(formState); + // Change the selection from the mod component to the mod + if (state.activeModComponentId === formStateId) { + state.activeModComponentId = null; + state.activeModId = removedFormState.modMetadata.id; } + // XXX: ideally this would only mark if the form state corresponds to an activated mod component. However, + // there's currently no way to determine if there's an activated mod component solely from the form state. + // The effect of adding the draft to deletedModComponentFormStatesByModId is benign - the mod will show as dirty + // even if the only change is that you added/removed a draft mod component. + // See discussion at: https://github.com/pixiebrix/pixiebrix-extension/pull/9320 + (state.deletedModComponentFormStatesByModId[ + removedFormState.modMetadata.id + ] ??= []).push(removedFormState); + // Make sure we're not keeping any private data around from Page Editor sessions void clearModComponentTraces(formStateId); }