#9355: fix dirty flag on Formik reinitialization regression

end-to-end-tests/pageObjects/pageEditor/modListingPanel.ts

@@ -60,6 +60,12 @@ export class ModActionMenu extends BasePageObject {
   }
 }
 
+export class ModComponentActionMenu extends BasePageObject {
+  get clearChangesButton() {
+    return this.getByRole("menuitem", { name: "Clear changes" });
+  }
+}
+
 export class ModListItem extends BasePageObject {
   get saveButton() {
     return this.locator("[data-icon=save]");
@@ -69,6 +75,12 @@ export class ModListItem extends BasePageObject {
     return this.getByLabel(" - Ellipsis");
   }
 
+  get unsavedChangesIcon() {
+    return this.getByRole("img", {
+      name: "Unsaved changes",
+    });
+  }
+
   get unavailableIcon() {
     return this.getByRole("img", {
       name: "Not available on page",
@@ -83,6 +95,10 @@ export class ModListItem extends BasePageObject {
     return new ModActionMenu(this.page.getByLabel("Menu"));
   }
 
+  get modComponentActionMenu() {
+    return new ModComponentActionMenu(this.page.getByLabel("Menu"));
+  }
+
   async openModActionMenu(): Promise<ModActionMenu> {
     await this.select();
     await this.menuButton.click();

end-to-end-tests/tests/pageEditor/clearChanges.spec.ts

@@ -0,0 +1,89 @@
+/*
+ * Copyright (C) 2024 PixieBrix, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import { expect, test } from "../../fixtures/testBase";
+// @ts-expect-error -- https://youtrack.jetbrains.com/issue/AQUA-711/Provide-a-run-configuration-for-Playwright-tests-in-specs-with-fixture-imports-only
+import { test as base } from "@playwright/test";
+import { ActivateModPage } from "../../pageObjects/extensionConsole/modsPage";
+import { type PageEditorPage } from "end-to-end-tests/pageObjects/pageEditor/pageEditorPage";
+import { type ConfigurationForm } from "../../pageObjects/pageEditor/configurationForm";
+import { type ModListItem } from "../../pageObjects/pageEditor/modListingPanel";
+
+const testModDefinitionName = "brick-configuration";
+test.use({ modDefinitionNames: [testModDefinitionName] });
+test("clear mod component changes", async ({
+  page,
+  extensionId,
+  modDefinitionsMap,
+  newPageEditorPage,
+  verifyModDefinitionSnapshot,
+}) => {
+  const { id: modId } = modDefinitionsMap[testModDefinitionName]!;
+  let pageEditorPage: PageEditorPage;
+  let brickConfigurationPanel: ConfigurationForm;
+  let modListItem: ModListItem;
+
+  await test.step("Activate mods, and initialize page editor, and select the mod component", async () => {
+    const modActivationPage = new ActivateModPage(page, extensionId, modId);
+    await modActivationPage.goto();
+    await modActivationPage.clickActivateAndWaitForModsPageRedirect();
+
+    await page.goto("/");
+    pageEditorPage = await newPageEditorPage(page.url());
+
+    brickConfigurationPanel = pageEditorPage.brickConfigurationPanel;
+
+    // Expand the mod
+    await pageEditorPage.modListingPanel
+      .getModListItemByName("Test mod - Brick Configuration")
+      .select();
+
+    // Select the mod component
+    modListItem =
+      pageEditorPage.modListingPanel.getModListItemByName("Context menu item");
+    await modListItem.select();
+
+    // Change icon should not exist
+    await expect(modListItem.unsavedChangesIcon).toHaveCount(0);
+  });
+
+  await test.step("Modify the mod component name and expect change icon", async () => {
+    await brickConfigurationPanel.fillField("Name", "A cool menu action");
+
+    // Reselect the mod component
+    modListItem =
+      pageEditorPage.modListingPanel.getModListItemByName("A cool menu action");
+
+    await expect(modListItem.unsavedChangesIcon).toBeVisible();
+  });
+
+  await test.step("Clear changes and expect the icon to go away", async () => {
+    await modListItem.menuButton.click();
+
+    await modListItem.modComponentActionMenu.clearChangesButton.click();
+
+    const dialog = pageEditorPage.getByRole("dialog");
+    await dialog.getByRole("button", { name: "Clear Changes" }).click();
+
+    // Reselect the mod component
+    modListItem =
+      pageEditorPage.modListingPanel.getModListItemByName("Context menu item");
+
+    // Change icon should not exist
+    await expect(modListItem.unsavedChangesIcon).toHaveCount(0);
+  });
+});

src/icons/Icon.tsx

@@ -28,14 +28,20 @@ const Icon: React.FunctionComponent<{
   size?: number | string;
   className?: string;
   color?: string;
-}> = ({ icon, library, size = 16, className, color }) => {
+  title?: string;
+  role?: React.AriaRole;
+}> = ({ icon, library, size = 16, className, color, title, role }) => {
   const { data: svg = "" } = useAsyncState(
     async () => getSvgIcon({ id: icon, library, size, color }),
     [icon, library],
   );
 
   return (
     <span
+      // XXX: should be setting role attribute on svg and embedding <title> instead?
+      // See https://docs.fontawesome.com/web/dig-deeper/accessibility
+      role={role ?? "img"}
+      aria-label={title}
       className={cx(className, styles.root)}
       dangerouslySetInnerHTML={{
         __html: svg,

src/pageEditor/modListingPanel/ModComponentIcons.tsx

@@ -34,7 +34,7 @@ export const NotAvailableIcon: React.FunctionComponent = () => (
 );
 
 export const UnsavedChangesIcon: React.FunctionComponent = () => (
-  <Icon library="custom" icon="ic-unsaved" />
+  <Icon library="custom" icon="ic-unsaved" title="Unsaved changes" />
 );
 
 export const ModHasUpdateIcon: React.FunctionComponent<{

src/pageEditor/panes/ModComponentEditorPane.tsx

@@ -15,13 +15,12 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-import React, { useEffect, useMemo, useRef } from "react";
+import React, { useCallback, useEffect, useMemo, useRef } from "react";
 import {
   actions,
   actions as editorActions,
 } from "@/pageEditor/store/editor/editorSlice";
-import { useDispatch, useSelector } from "react-redux";
-import { useDebouncedCallback } from "use-debounce";
+import { useDispatch, useSelector, useStore } from "react-redux";
 import ErrorBoundary from "@/components/ErrorBoundary";
 // eslint-disable-next-line no-restricted-imports -- TODO: Fix over time
 import { Formik } from "formik";
@@ -37,39 +36,76 @@ import IntegrationsSliceModIntegrationsContextAdapter from "@/integrations/store
 import { assertNotNullish } from "@/utils/nullishUtils";
 import useRegisterDraftModInstanceOnAllFrames from "@/pageEditor/hooks/useRegisterDraftModInstanceOnAllFrames";
 import { usePreviousValue } from "@/hooks/usePreviousValue";
+import type { EditorRootState } from "@/pageEditor/store/editor/pageEditorTypes";
 
 // CHANGE_DETECT_DELAY_MILLIS should be low enough so that sidebar gets updated in a reasonable amount of time, but
 // high enough that there isn't an entry lag in the page editor
 const CHANGE_DETECT_DELAY_MILLIS = 100;
-const REDUX_SYNC_WAIT_MILLIS = 500;
+
+/**
+ * Returns callback to generate the current key to force reinitialization of Formik form.
+ */
+function useGetFormReinitializationKey(): () => string {
+  const store = useStore<EditorRootState>();
+
+  return useCallback(() => {
+    const state = store.getState();
+    const editorUpdateKey = selectEditorUpdateKey(state);
+    const activeModComponentFormState =
+      selectActiveModComponentFormState(state);
+
+    assertNotNullish(
+      activeModComponentFormState,
+      "ModComponentEditorPane requires activeModComponentFormState",
+    );
+
+    return `${activeModComponentFormState.uuid}-${activeModComponentFormState.installed}-${editorUpdateKey}`;
+  }, [store]);
+}
 
 const EditorPaneContent: React.VoidFunctionComponent<{
   modComponentFormState: ModComponentFormState;
 }> = ({ modComponentFormState }) => {
   const dispatch = useDispatch();
+  const getFormReinitializationKey = useGetFormReinitializationKey();
+  const previousKey = useRef<string | null>(getFormReinitializationKey());
 
-  // XXX: anti-pattern: callback to update the redux store based on the formik state
-  const syncReduxState = useDebouncedCallback(
+  // XXX: anti-pattern: callback to update the redux store based on the Formik state.
+  // Don't use useDebounceCallback because Effect component is already debounced
+  const syncReduxState = useCallback(
     (values: ModComponentFormState) => {
-      dispatch(
-        editorActions.setModComponentFormState({
-          modComponentFormState: values,
-          includesNonFormikChanges: false,
-          dirty: true,
-        }),
-      );
-      dispatch(actions.checkActiveModComponentAvailability());
+      const currentKey = getFormReinitializationKey();
+
+      // Don't sync on the first call after the reinitialization key changes because the call would cause
+      // the form to be marked dirty even if there are no changes.
+      //
+      // NOTE: there are some editor components that contain useEffect calls that perform normalization on mount.
+      // Therefore, in some cases, the first syncReduxState call will include changes, but we don't want to show
+      // the unsaved change icon as they aren't functional changes
+      //
+      // See: https://github.com/pixiebrix/pixiebrix-extension/issues/9355
+      if (previousKey.current === currentKey) {
+        dispatch(
+          editorActions.setModComponentFormState({
+            modComponentFormState: values,
+            includesNonFormikChanges: false,
+            dirty: true,
+          }),
+        );
+
+        dispatch(actions.checkActiveModComponentAvailability());
+      }
+
+      previousKey.current = currentKey;
     },
-    REDUX_SYNC_WAIT_MILLIS,
-    { trailing: true, leading: false },
+    [dispatch, getFormReinitializationKey, previousKey],
   );
 
+  // XXX: effect should be refactored to a middleware that listens for selected mod component
   useEffect(() => {
     const messageContext = {
       modComponentId: modComponentFormState.uuid,
-      modId: modComponentFormState.modMetadata
-        ? modComponentFormState.modMetadata.id
-        : undefined,
+      modId: modComponentFormState.modMetadata.id,
     };
     dispatch(logActions.setContext({ messageContext }));
   }, [modComponentFormState.uuid, modComponentFormState.modMetadata, dispatch]);
@@ -89,10 +125,12 @@ const EditorPaneContent: React.VoidFunctionComponent<{
 };
 
 /**
- * Returns the active mod component form state. Responds to updates in the editor state for use with triggering rerenders.
+ * Returns the initial mod component form state for the Formik form. Responds to updates in the editor state
+ * for use with Formik reinitialization.
  */
 function useInitialValues(): ModComponentFormState {
-  const editorUpdateKey = useSelector(selectEditorUpdateKey);
+  const getCurrentFormReinitializationKey = useGetFormReinitializationKey();
+
   const activeModComponentFormState = useSelector(
     selectActiveModComponentFormState,
   );
@@ -103,7 +141,7 @@ function useInitialValues(): ModComponentFormState {
   );
 
   // Key to force reinitialization of formik when user selects a different mod component from the sidebar
-  const key = `${activeModComponentFormState.uuid}-${activeModComponentFormState.installed}-${editorUpdateKey}`;
+  const key = getCurrentFormReinitializationKey();
   const prevKey = usePreviousValue(key);
   const activeModComponentFormStateRef = useRef(activeModComponentFormState);