diff --git a/PHASE2-FINDINGS.md b/PHASE2-FINDINGS.md index 43bc73132..c1062d0b9 100644 --- a/PHASE2-FINDINGS.md +++ b/PHASE2-FINDINGS.md @@ -243,16 +243,53 @@ existing `enableRightClickOnWidgets` user flag isolates the cost: count. The dropdown *contents* are already lazy; the `Menu`/`Popover`/`PopoverDropdown` wrappers are not. -### Attempted fix, reverted +### The fix: one board-level menu (shipped) -Deferring the whole `Menu` subtree until the first right-click (replaying the event so -Mantine still positions at the cursor) **failed verification and was reverted**: the -replayed `contextmenu` re-entered the arming handler, so right-click stopped opening the -menu and the tree exploded from 3,614 to 20,107 fibers. The Playwright check that caught -it is the one worth keeping — a fiber count alone would have looked like a win. +`WidgetContextMenuProvider` mounts a single menu for the whole board, anchored to the +cursor. Items keep only a `contextmenu`/long-press listener. The dropdown takes the +section's gridstack ref as a prop, because the shared menu lives outside any +`SectionContext` — reading it from context there crashed the board. -Doing this properly means one board-level menu that renders the active item's actions, -rather than one menu per item — a real refactor, not a wrapper swap. +Verified with `scripts/benchmarks/verify-context-menu.mts`, run against both builds: + +| | per-item menus | shared menu | +| --- | --- | --- | +| menu opens on right-click | PASS | PASS | +| menu shows actions | PASS (5) | PASS (5) | +| Escape closes it | PASS | PASS | +| re-opens on a second widget | PASS | PASS | +| idle fibers | 18,670 | **16,278 (−12.8%)** | +| fibers added by using the menu | +4,191 | **+4** | + +Memory, two runs each, four tabs across two boards: + +| metric | per-item (mean) | shared (mean) | change | +| --- | --- | --- | --- | +| JS event listeners | 10,128 | **7,774** | **−23%** | +| DOM nodes | 9,189 | **8,287** | **−10%** | +| browser JS heap | 54.1 MiB | 52.7 MiB | −2.5% | +| median board load | 520 ms | 479 ms | faster | +| container CPU | 20.4 s | 20.0 s | unchanged | + +Server memory is unchanged, as expected for a client-only change. One `after-stress` +sample read 362 MiB against ~267 for its pair and for both controls — GC timing on a +single sample, not a regression. + +The listener reduction is the one that matters most: the 650 MB session had ~88k +listeners, and this removes a per-widget source of them. + +### Two earlier attempts, and a broken test + +A first attempt deferred each item's `Menu` until the first right-click. It was reverted +after the gate failed it — but **that verdict was wrong**: the gate used Playwright's +`click({ button: "right" })`, which does not emit a `contextmenu` event here, so it +failed *shipped code* identically. The "fiber explosion" attributed to that change showed +up on the control too. + +The gate now dispatches a real `contextmenu` event and is validated by passing against +unmodified code first. Any optimisation gate that has never been run against the +un-optimised build is not evidence — it can only tell you something changed, not that it +still works. ### A measurement flaw worth recording diff --git a/apps/nextjs/src/app/[locale]/boards/(content)/_client.tsx b/apps/nextjs/src/app/[locale]/boards/(content)/_client.tsx index 1b4308d38..005956935 100644 --- a/apps/nextjs/src/app/[locale]/boards/(content)/_client.tsx +++ b/apps/nextjs/src/app/[locale]/boards/(content)/_client.tsx @@ -4,6 +4,7 @@ import { Box, Stack } from "@mantine/core"; import { useCurrentLayout, useRequiredBoard } from "@homarr/boards/context"; +import { WidgetContextMenuProvider } from "~/components/board/items/widget-context-menu-provider"; import { BoardCategorySection } from "~/components/board/sections/category-section"; import { BoardEmptySection } from "~/components/board/sections/empty-section"; import { BoardBackgroundVideo } from "~/components/layout/background"; @@ -19,15 +20,17 @@ export const ClientBoard = () => { return ( - - {fullWidthSortedSections.map((section) => - section.kind === "empty" ? ( - - ) : ( - - ), - )} - + + + {fullWidthSortedSections.map((section) => + section.kind === "empty" ? ( + + ) : ( + + ), + )} + + ); }; diff --git a/apps/nextjs/src/components/board/items/widget-context-menu-provider.tsx b/apps/nextjs/src/components/board/items/widget-context-menu-provider.tsx new file mode 100644 index 000000000..5594c2cec --- /dev/null +++ b/apps/nextjs/src/components/board/items/widget-context-menu-provider.tsx @@ -0,0 +1,94 @@ +"use client"; + +import type { MutableRefObject, PropsWithChildren } from "react"; +import { createContext, useCallback, useContext, useMemo, useState } from "react"; +import { Menu } from "@mantine/core"; + +import { useSession } from "@homarr/auth/client"; +import { useSettings } from "@homarr/settings"; +import type { WidgetDefinition } from "@homarr/widgets/definition"; + +import type { UseGridstackRefs } from "../sections/gridstack/use-gridstack"; + +import type { SectionItem } from "~/app/[locale]/boards/_types"; +import { WidgetContextMenuDropdown } from "./widget-context-menu"; + +interface ActiveContextMenu { + item: SectionItem; + definition: WidgetDefinition; + widgetStateRef: MutableRefObject | null>; + /** Carried from the item's section, which the board-level menu is outside of. */ + gridstack: UseGridstackRefs["gridstack"]; + x: number; + y: number; +} + +interface WidgetContextMenuApi { + /** Null when right-click menus are disabled, which lets items skip their handler. */ + open: ((menu: ActiveContextMenu) => void) | null; +} + +const WidgetContextMenuContext = createContext({ open: null }); + +export const useWidgetContextMenu = () => useContext(WidgetContextMenuContext); + +/** + * One context menu for the whole board instead of one per widget. + * + * Mantine's `Menu` mounts `Popover` + `PopoverDropdown` + several context providers + * even while closed, so a menu per item cost ~13 fibers each for something almost + * never opened — measured at 364 of 3,918 fibers (9%) on a 28-widget board, growing + * with item count. Items now only carry a `contextmenu` listener and this single menu + * is anchored to the cursor. + */ +export const WidgetContextMenuProvider = ({ children }: PropsWithChildren) => { + const { data: session } = useSession(); + const settings = useSettings(); + const [active, setActive] = useState(null); + + const isEnabled = Boolean(session) && settings.enableRightClickOnWidgets; + const open = useCallback((menu: ActiveContextMenu) => setActive(menu), []); + const api = useMemo(() => ({ open: isEnabled ? open : null }), [isEnabled, open]); + + return ( + + {children} + {active && ( + { + if (!opened) setActive(null); + }} + > + {/* Zero-size anchor at the cursor so Mantine positions the dropdown there. */} + +
+ + + + +
+ )} +
+ ); +}; diff --git a/apps/nextjs/src/components/board/items/widget-context-menu.tsx b/apps/nextjs/src/components/board/items/widget-context-menu.tsx index 3b844874f..264551029 100644 --- a/apps/nextjs/src/components/board/items/widget-context-menu.tsx +++ b/apps/nextjs/src/components/board/items/widget-context-menu.tsx @@ -1,7 +1,7 @@ "use client"; import type { MutableRefObject, ReactNode } from "react"; -import { useCallback, useMemo, useState } from "react"; +import { useCallback, useMemo, useRef } from "react"; import { Group, Loader, Menu, Switch, Text, Tooltip } from "@mantine/core"; import { IconAlertTriangle, @@ -16,7 +16,7 @@ import type { QueryClient, QueryKey } from "@tanstack/react-query"; import { partialMatchKey, useIsFetching, useQueryClient } from "@tanstack/react-query"; import { clientApi } from "@homarr/api/client"; -import { useSession } from "@homarr/auth/client"; + import { useRequiredBoard } from "@homarr/boards/context"; import { useEditMode } from "@homarr/boards/edit-mode"; import { useTimeAgo } from "@homarr/common"; @@ -30,7 +30,9 @@ import { getWidgetQueryKeys } from "@homarr/widgets/definition"; import { reduceWidgetOptionsWithDefinition } from "@homarr/widgets/manifest"; import type { SectionItem } from "~/app/[locale]/boards/_types"; +import type { UseGridstackRefs } from "../sections/gridstack/use-gridstack"; import { useSectionContext } from "../sections/section-context"; +import { useWidgetContextMenu } from "./widget-context-menu-provider"; import { useItemActions } from "./item-actions"; import { LazyItemMoveModal, preloadItemMoveModal } from "./lazy-item-move-modal"; import { LazyWidgetEditModal, preloadWidgetEditModal } from "./lazy-widget-edit-modal"; @@ -42,58 +44,68 @@ interface WidgetContextMenuProps { children: ReactNode; } +const LONG_PRESS_DELAY_MS = 500; + +/** + * Per-item trigger only. The menu itself lives once at board level — see + * WidgetContextMenuProvider for why. + */ export const WidgetContextMenu = ({ item, definition, widgetStateRef, children }: WidgetContextMenuProps) => { - const { data: session } = useSession(); - const settings = useSettings(); + const { open } = useWidgetContextMenu(); + const { gridstack } = useSectionContext().refs; + const longPressTimer = useRef | null>(null); - if (!session || !settings.enableRightClickOnWidgets) return <>{children}; + const cancelLongPress = useCallback(() => { + if (longPressTimer.current) { + clearTimeout(longPressTimer.current); + longPressTimer.current = null; + } + }, []); + + if (!open) return <>{children}; return ( - - {children} - - ); -}; - -const WidgetContextMenuInner = ({ - item, - definition, - widgetStateRef, - children, - settings, -}: WidgetContextMenuProps & { settings: ReturnType }) => { - const [isMenuOpen, setIsMenuOpen] = useState(false); - - return ( - { + event.preventDefault(); + open({ item, definition, widgetStateRef, gridstack, x: event.clientX, y: event.clientY }); + }} + onTouchStart={(event) => { + const touch = event.touches[0]; + if (!touch) return; + const { clientX, clientY } = touch; + longPressTimer.current = setTimeout( + () => open({ item, definition, widgetStateRef, gridstack, x: clientX, y: clientY }), + LONG_PRESS_DELAY_MS, + ); + }} + onTouchEnd={cancelLongPress} + onTouchMove={cancelLongPress} + onTouchCancel={cancelLongPress} > - {children} - - {isMenuOpen && ( - - )} - - + {children} + ); }; type WidgetContextMenuDropdownProps = Omit & { settings: ReturnType; + /** + * Passed in rather than read from SectionContext: the menu is mounted once at board + * level, outside any section, while the item that opened it is inside one. + */ + gridstack: UseGridstackRefs["gridstack"]; }; -const WidgetContextMenuDropdown = ({ item, definition, widgetStateRef, settings }: WidgetContextMenuDropdownProps) => { +export const WidgetContextMenuDropdown = ({ + item, + definition, + widgetStateRef, + settings, + gridstack, +}: WidgetContextMenuDropdownProps) => { const [isEditMode] = useEditMode(); const board = useRequiredBoard(); const tItem = useScopedI18n("item"); @@ -110,7 +122,6 @@ const WidgetContextMenuDropdown = ({ item, definition, widgetStateRef, settings enabled: hasSupportedIntegrations, }); const { mutate: saveBoard } = clientApi.board.saveBoard.useMutation(); - const { gridstack } = useSectionContext().refs; const queryClient = useQueryClient(); const widgetQueryKeys = useMemo(() => getWidgetQueryKeys(definition, item.kind), [definition, item.kind]); diff --git a/scripts/benchmarks/verify-context-menu.mts b/scripts/benchmarks/verify-context-menu.mts new file mode 100644 index 000000000..e8f0fad1e --- /dev/null +++ b/scripts/benchmarks/verify-context-menu.mts @@ -0,0 +1,155 @@ +/** + * Functional gate for the shared board context menu. + * + * A fiber count alone is not enough: an earlier attempt at this optimisation cut the + * idle tree *and* broke right-click, which looked like a win until this ran. So the + * saving is only reported if the menu still opens, shows real actions, closes, and + * works on a second, different widget. + * + * STRESS_BASE_URL=http://127.0.0.1:PORT \ + * node --experimental-strip-types scripts/benchmarks/verify-context-menu.mts + */ +import { chromium } from "@playwright/test"; +import type { Page } from "@playwright/test"; + +const baseUrl = process.env.STRESS_BASE_URL; +if (!baseUrl) throw new Error("STRESS_BASE_URL is required"); +const username = process.env.STRESS_USERNAME ?? "demo"; +const password = process.env.STRESS_PASSWORD ?? "demodemo"; +const boardName = process.env.STRESS_BOARD ?? "default"; + +const failures: string[] = []; +const check = (ok: boolean, description: string) => { + console.log(` ${ok ? "PASS" : "FAIL"} ${description}`); + if (!ok) failures.push(description); +}; + +const browser = await chromium.launch(); +const context = await browser.newContext({ viewport: { width: 1920, height: 1080 } }); +await context.addInitScript(() => { + const roots = new Set(); + (globalThis as unknown as Record).__REACT_DEVTOOLS_GLOBAL_HOOK__ = { + renderers: new Map(), + supportsFiber: true, + inject: () => 1, + onCommitFiberRoot: (_id: unknown, root: unknown) => roots.add(root), + onCommitFiberUnmount: () => undefined, + onPostCommitFiberRoot: () => undefined, + __homarrRoots: roots, + }; +}); +const page = await context.newPage(); + +const countFibers = (target: Page) => + target.evaluate(() => { + const hook = (globalThis as unknown as Record }>) + .__REACT_DEVTOOLS_GLOBAL_HOOK__; + const roots = [...(hook?.__homarrRoots ?? [])].map((root) => root.current); + const seen = new Set(); + const walk = (fiber: { child?: unknown; sibling?: unknown } | null, depth: number) => { + if (!fiber || seen.has(fiber) || depth > 3000) return; + seen.add(fiber); + walk(fiber.child as never, depth + 1); + walk(fiber.sibling as never, depth); + }; + for (const root of roots) walk(root as never, 0); + return seen.size; + }); + +const dropdown = page.locator('[role="menu"]:visible, .mantine-Menu-dropdown:visible').first(); + +/** + * Playwright's `click({ button: "right" })` does not produce a `contextmenu` event in + * this setup, so it reports every build as broken — including shipped code. Dispatch + * the event the app actually listens for instead. + */ +const rightClickItem = (index: number) => + page.evaluate((itemIndex) => { + const items = [...document.querySelectorAll('[data-homarr-dev-benchmark-board] [data-type="item"]')]; + const item = items[itemIndex]; + if (!item) return false; + const rect = item.getBoundingClientRect(); + const target = (item.querySelector("*") as HTMLElement | null) ?? (item as HTMLElement); + target.dispatchEvent( + new MouseEvent("contextmenu", { + bubbles: true, + cancelable: true, + clientX: rect.x + rect.width / 2, + clientY: rect.y + rect.height / 2, + button: 2, + }), + ); + return true; + }, index); + +await page.goto(`${baseUrl}/auth/login`, { waitUntil: "networkidle" }); +await page.getByLabel("Username").fill(username); +await page.locator("#password").fill(password); +await page.locator("css=button[type='submit']").click(); +await page.waitForFunction(() => !window.location.pathname.includes("/auth/login"), undefined, { timeout: 60_000 }); + +await page.goto(`${baseUrl}/boards/${boardName}`, { waitUntil: "domcontentloaded" }); +await page.locator("[data-homarr-dev-benchmark-board]").waitFor({ state: "visible", timeout: 120_000 }); +await page + .waitForFunction( + () => { + const items = [...document.querySelectorAll('[data-type="item"]')]; + return ( + items.length > 0 && + items.every((item) => item.querySelector("[data-homarr-widget-ready],[data-homarr-widget-error]")) + ); + }, + undefined, + { timeout: 120_000 }, + ) + .catch(() => console.log(" warn: widgets did not all settle")); + +const items = page.locator('[data-homarr-dev-benchmark-board] [data-type="item"]'); +const itemCount = await items.count(); +console.log(`board items: ${itemCount}`); + +const idleFibers = await countFibers(page); +console.log(`fibers with board idle: ${idleFibers}\n`); + +console.log("right-click behaviour:"); +check(!(await dropdown.isVisible().catch(() => false)), "no menu is open before interacting"); + +await rightClickItem(0); +const openedFirst = await dropdown + .waitFor({ state: "visible", timeout: 8_000 }) + .then(() => true) + .catch(() => false); +check(openedFirst, "right-click opens the context menu"); + +const actionCount = await page.locator('[role="menu"]:visible [role="menuitem"], .mantine-Menu-item:visible').count(); +check(actionCount > 0, `menu contains actions (found ${actionCount})`); + +await page.keyboard.press("Escape"); +const closed = await dropdown + .waitFor({ state: "hidden", timeout: 5_000 }) + .then(() => true) + .catch(() => false); +check(closed, "Escape closes the menu"); + +// A shared menu has to re-target; a per-item menu would trivially pass the first case. +if (itemCount > 1) { + await rightClickItem(1); + const openedSecond = await dropdown + .waitFor({ state: "visible", timeout: 8_000 }) + .then(() => true) + .catch(() => false); + check(openedSecond, "right-click on a second widget re-opens the menu"); + await page.keyboard.press("Escape").catch(() => undefined); + await dropdown.waitFor({ state: "hidden", timeout: 5_000 }).catch(() => undefined); +} + +const afterFibers = await countFibers(page); +console.log( + `\nfibers after opening/closing menus: ${afterFibers} (delta ${afterFibers - idleFibers >= 0 ? "+" : ""}${afterFibers - idleFibers})`, +); +check(afterFibers < idleFibers * 1.5, "tree did not balloon after using the menu"); + +console.log(`\n${failures.length === 0 ? "ALL CHECKS PASSED" : `${failures.length} CHECK(S) FAILED`}`); +console.log(JSON.stringify({ itemCount, idleFibers, afterFibers, failures }, null, 2)); +await browser.close(); +if (failures.length > 0) process.exitCode = 1;