refactor: simplify actions menu (#1904)

Author: tyler-dane

packages/web/src/views/Forms/ActionsMenu/ActionsMenu.test.tsx

@@ -2,6 +2,7 @@ import { fireEvent, render, screen } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import { act, type ReactElement } from "react";
 import { ActionsMenu, useMenuContext } from "./ActionsMenu";
+import MenuItem from "./MenuItem";
 import { describe, expect, it } from "bun:test";
 
 const renderMenu = (ui: ReactElement) => render(ui);
@@ -81,4 +82,31 @@ describe("ActionsMenu", () => {
     expect(screen.queryByRole("menu")).toBeNull();
     expect(priorityButton).toHaveFocus();
   });
+
+  it("moves focus through every item as arrow keys are pressed", async () => {
+    const user = userEvent.setup();
+
+    renderMenu(
+      <ActionsMenu bgColor="#fff">
+        {() => (
+          <>
+            <MenuItem bgColor="#fff">First</MenuItem>
+            <MenuItem bgColor="#fff">Second</MenuItem>
+            <MenuItem bgColor="#fff">Third</MenuItem>
+          </>
+        )}
+      </ActionsMenu>,
+    );
+
+    await user.click(screen.getByLabelText("Open actions menu"));
+
+    await user.keyboard("{ArrowDown}");
+    expect(document.activeElement?.textContent).toBe("First");
+
+    await user.keyboard("{ArrowDown}");
+    expect(document.activeElement?.textContent).toBe("Second");
+
+    await user.keyboard("{ArrowDown}");
+    expect(document.activeElement?.textContent).toBe("Third");
+  });
 });

packages/web/src/views/Forms/ActionsMenu/ActionsMenu.tsx

@@ -18,7 +18,6 @@ import {
   createContext,
   type MouseEvent,
   useContext,
-  useEffect,
   useRef,
   useState,
 } from "react";
@@ -89,47 +88,11 @@ export const ActionsMenu: React.FC<ActionsMenuProps> = ({
   const click = useClick(context);
   const dismiss = useDismiss(context);
   const role = useRole(context, { role: "menu" });
-  // Create a dense array for FloatingUI and mapping back to sparse array
-  const denseListRef = useRef<Array<HTMLElement>>([]);
-  const sparseToCompactMap = useRef<Map<number, number>>(new Map());
-  const compactToSparseMap = useRef<Map<number, number>>(new Map());
-
-  // Update dense list and mappings when listRef changes
-  useEffect(() => {
-    const denseArray: HTMLElement[] = [];
-    sparseToCompactMap.current.clear();
-    compactToSparseMap.current.clear();
-
-    listRef.current.forEach((item, sparseIndex) => {
-      if (item !== null) {
-        const compactIndex = denseArray.length;
-        denseArray.push(item);
-        sparseToCompactMap.current.set(sparseIndex, compactIndex);
-        compactToSparseMap.current.set(compactIndex, sparseIndex);
-      }
-    });
-
-    denseListRef.current = denseArray;
-  });
-
-  // Convert sparse activeIndex to compact activeIndex for FloatingUI
-  const compactActiveIndex =
-    activeIndex !== null
-      ? (sparseToCompactMap.current.get(activeIndex) ?? null)
-      : null;
 
   const listNavigation = useListNavigation(context, {
-    listRef: denseListRef,
-    activeIndex: compactActiveIndex,
-    onNavigate: (compactIndex) => {
-      const sparseIndex =
-        compactIndex !== null && compactIndex !== undefined
-          ? (compactToSparseMap.current.get(compactIndex) ?? null)
-          : null;
-      if (sparseIndex !== null) {
-        setActiveIndex(sparseIndex);
-      }
-    },
+    listRef,
+    activeIndex,
+    onNavigate: setActiveIndex,
     focusItemOnHover: false,
     loop: true,
   });

packages/web/src/views/Forms/ActionsMenu/MenuItem.tsx

@@ -32,20 +32,24 @@ const MenuItem: React.FC<MenuItemProps> = ({
   const itemRef = useRef<HTMLButtonElement | null>(null);
   const indexRef = useRef<number | null>(null);
 
-  // Register with menu context
+  // Register with menu context. Depend on the stable `listRef` object
+  // (not `menuContext` itself, which is a new object every render because
+  // it carries `activeIndex`) so this doesn't tear down and re-register on
+  // every arrow-key press.
+  const listRef = menuContext?.listRef;
   useEffect(() => {
-    if (menuContext && itemRef.current) {
-      const index = menuContext.listRef.current.length;
+    if (listRef && itemRef.current) {
+      const index = listRef.current.length;
       indexRef.current = index;
-      menuContext.listRef.current[index] = itemRef.current;
+      listRef.current[index] = itemRef.current;
 
       return () => {
         if (indexRef.current !== null) {
-          menuContext.listRef.current[indexRef.current] = null;
+          listRef.current[indexRef.current] = null;
         }
       };
     }
-  }, [menuContext]);
+  }, [listRef]);
 
   const handleKeyDown = (e: React.KeyboardEvent<HTMLButtonElement>) => {
     // Handle Enter/Space for activation
@@ -83,7 +87,7 @@ const MenuItem: React.FC<MenuItemProps> = ({
       role="menuitem"
       tabIndex={tabIndex}
       type={type}
-      className="flex w-full cursor-pointer items-center gap-2 border-0 bg-[var(--actions-menu-item-bg)] px-2 py-1 text-left text-m text-text-dark outline-none hover:[text-shadow:0_0_0.5px_var(--compass-color-text-dark),0_0_0.5px_var(--compass-color-text-dark)] focus-visible:[text-shadow:0_0_0.5px_var(--compass-color-text-dark),0_0_0.5px_var(--compass-color-text-dark)]"
+      className="flex w-full cursor-pointer items-center gap-2 border-0 bg-(--actions-menu-item-bg) px-2 py-1 text-left text-m text-text-dark outline-none hover:[text-shadow:0_0_0.5px_var(--compass-color-text-dark),0_0_0.5px_var(--compass-color-text-dark)] focus-visible:[text-shadow:0_0_0.5px_var(--compass-color-text-dark),0_0_0.5px_var(--compass-color-text-dark)]"
       style={{ backgroundColor: bgColor }}
     >
       {children}