perf(widgets): stop building collapsed Accordion panels
From reading PR #6582, which adds keepMounted={false} to the two coolify accordions. The widget sweep had checked Tabs and never checked Accordion, so it missed this on all five sites - an honest gap that PR closed for two of them. Accordion behaves exactly like Tabs, verified in the installed version rather than assumed: with three marker children per panel, the default renders 3 of 3 children of the *collapsed* panel and keepMounted={false} renders 0. It matters more here than in Tabs, because collapsed is an accordion section's normal state rather than the exception: coolify instance-card three remote-data lists: servers, applications, services - all built while collapsed coolify single-instance-layout same three lists health-monitoring cluster per-node resource tables (beyond #6582) firewall interface rows (beyond #6582) Left mounted: the multiReleasesRepositories input, which holds selection state and is edit-modal only rather than on the board render path. In every fixed case the open/closed state lives in the parent, so unmounting a collapsed panel discards nothing. The spec is renamed tabs-keep-mounted -> keep-mounted since it now pins both components, and three verdicts in the audit table are corrected: coolify was recorded "clean (bounded)" and firewall triaged on other grounds, while both were carrying this cost.
This commit is contained in:
6 files changed
+122
-10
No files matched your search
@@ -28,6 +28,33 @@ Two companion scripts: `audit-widget-render.mjs` finds specific anti-patterns an
|
||||
whether they sit inside a `.map()`, and `measure-widget-cost.mts` counts components and fibers per
|
||||
widget on a real board.
|
||||
|
||||
## The same cost in `Accordion`, which this sweep missed
|
||||
|
||||
Credit where it is due: this came from reading [PR #6582](https://github.com/homarr-labs/homarr/pull/6582),
|
||||
which adds `keepMounted={false}` to the two coolify accordions. The sweep below checked `Tabs` and never
|
||||
checked `Accordion`, so it missed the problem entirely on five sites.
|
||||
|
||||
**`Accordion` behaves exactly like `Tabs`: collapsed panels render their children.** Verified in the
|
||||
installed version — with three marker children per panel, the default renders **3 of 3** children of the
|
||||
*collapsed* panel, and `keepMounted={false}` renders **0**.
|
||||
|
||||
It matters more here than in `Tabs`, because collapsed is the *normal* state of an accordion section
|
||||
rather than the exception:
|
||||
|
||||
| widget | collapsed panels were building | verdict |
|
||||
| --- | --- | --- |
|
||||
| `coolify` instance-card | three remote-data lists — servers, applications, services | **fixed** |
|
||||
| `coolify` single-instance-layout | same three lists | **fixed** |
|
||||
| `health-monitoring` cluster-health | per-node resource tables | **fixed** — beyond what #6582 did |
|
||||
| `firewall` | interface rows | **fixed** — beyond what #6582 did |
|
||||
| `_inputs/widget-multiReleasesRepositories-input` | repository picker | left mounted — it holds selection state, and it is edit-modal only, not on the board render path |
|
||||
|
||||
In every fixed case the open/closed state lives in the parent component, so unmounting a collapsed
|
||||
panel discards nothing.
|
||||
|
||||
This also corrects three verdicts in the table below: `coolify` was recorded as "clean (bounded)" and
|
||||
`firewall` and `health-monitoring` as triaged/fixed on other grounds. All three were carrying this cost.
|
||||
|
||||
## The systemic finding this pass: inactive tab panels render in full
|
||||
|
||||
**Mantine's `Tabs` keeps inactive panels mounted by default.** In Mantine 9 on React 19 that is
|
||||
@@ -49,7 +76,7 @@ board showing one was building the other's whole tree as well.
|
||||
|
||||
**The premise is verified, not inferred.** This started as a reading of Mantine's documentation, which
|
||||
is a weak basis for a prop that trades a remount cost for a mount saving — if Mantine ever stopped
|
||||
rendering inactive panels, the prop would be pure loss. `common/tabs-keep-mounted.spec.ts` pins the
|
||||
rendering inactive panels, the prop would be pure loss. `common/keep-mounted.spec.ts` pins the
|
||||
behaviour in the installed version: with three marker children per panel, the default renders **3 of 3**
|
||||
children of the *inactive* panel into the DOM, and `keepMounted={false}` renders **0**. If that ever
|
||||
changes, the test fails and the prop should come back out.
|
||||
@@ -62,8 +89,9 @@ harness. The mechanism is confirmed; the size of the saving is not.
|
||||
|
||||
| widget | change | why |
|
||||
| --- | --- | --- |
|
||||
| `health-monitoring` | `keepMounted={false}` | the hidden monitoring view was fully built |
|
||||
| `media-missing` | `keepMounted={false}` | both tabs built every card |
|
||||
| `health-monitoring` | `keepMounted={false}` on `Tabs` | the hidden monitoring view was fully built |
|
||||
| `media-missing` | `keepMounted={false}` on `Tabs` | both tabs built every card |
|
||||
| `coolify` ×2, `health-monitoring` cluster, `firewall` | `keepMounted={false}` on `Accordion` | collapsed panels built their lists; collapsed is an accordion's normal state |
|
||||
| `media-missing` | `offscreenRowStyle(CARD_HEIGHT[density])` | renders every card into a scrolling tile; the card height is fixed per density, so the intrinsic size is exact rather than a guess |
|
||||
| `media-missing` | `loading="lazy"` on the poster | a poster per card, all fetched regardless of scroll position |
|
||||
| `calendar` | inline SVG instead of `placehold.co` | a failed poster made a **third-party network request** from a self-hosted dashboard, once per failing event, to draw a grey square |
|
||||
@@ -95,9 +123,9 @@ cannot get long" are different strengths of claim:
|
||||
| [x] | 7 | `media-requests` | 542 | fixed | `content-visibility` + `loading="lazy"` on both images |
|
||||
| [x] | 8 | `tracearr` | 445 | clean (read) | poster is a CSS background per stream card, and the list is active streams — a handful |
|
||||
| [x] | 9 | `custom-api` | 1014 | triaged | 6 index keys, all safe: stateless read-only rows from user JSONPath data with no stable id |
|
||||
| [x] | 10 | `firewall` | 334 | triaged | 4 inline-style-per-row, children not memoised so nothing is defeated |
|
||||
| [x] | 10 | `firewall` | 334 | **fixed** | its `Accordion` built the interface rows while collapsed. The 4 inline-style-per-row findings stay declined: children are not memoised, so nothing is defeated |
|
||||
| [x] | 11 | `releases` | 817 | fixed | `OverflowBadge` mounted a `Popover` per row even with nothing to overflow |
|
||||
| [x] | 12 | `coolify` | 752 | clean (bounded) | one map over configured instances; the icon is a fixed constant URL, not per-row data |
|
||||
| [x] | 12 | `coolify` | 752 | **fixed** | its `Accordion` built all three resource lists while collapsed; the icon is a fixed constant URL, not per-row data |
|
||||
| [x] | 13 | `notebook` | 1189 | clean (read) | rich-text editor. Deliberately **not** given `content-visibility`: it measures itself, and skipping its layout would be wrong |
|
||||
| [x] | 14 | `umami` | 595 | clean (read) | the top-list is bounded by the widget's own `limit` option and each row is three `<Text>` — containment would cost more than it saves |
|
||||
| [x] | 15 | `traefik` | 289 | clean (read) | already bounded: `.slice(0, getEntryPointLimit(width))` |
|
||||
|
||||
+69
-1
@@ -1,4 +1,4 @@
|
||||
import { MantineProvider, Tabs } from "@mantine/core";
|
||||
import { Accordion, MantineProvider, Tabs } from "@mantine/core";
|
||||
import { act, createElement } from "react";
|
||||
import { createRoot } from "react-dom/client";
|
||||
import { afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
@@ -100,3 +100,71 @@ describe("Mantine Tabs keepMounted", () => {
|
||||
expect(counts.offscreen).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* `Accordion` has the same behaviour and the same prop, which the widget sweep originally missed - it
|
||||
* checked `Tabs` and not `Accordion`. Collapsed panels matter more here, because a collapsed accordion
|
||||
* section is the *normal* state rather than the exception: coolify renders three remote-data resource
|
||||
* lists this way, cluster-health its node tables, firewall its interface rows.
|
||||
*/
|
||||
describe("Mantine Accordion keepMounted", () => {
|
||||
let container: HTMLDivElement;
|
||||
let root: Root;
|
||||
|
||||
afterEach(() => {
|
||||
root?.unmount();
|
||||
container?.remove();
|
||||
});
|
||||
|
||||
const renderAccordion = async (keepMounted: boolean | undefined) => {
|
||||
container = document.createElement("div");
|
||||
document.body.appendChild(container);
|
||||
root = createRoot(container);
|
||||
|
||||
const item = (value: string, marker: string) =>
|
||||
createElement(Accordion.Item, { value, key: value }, [
|
||||
createElement(Accordion.Control, { key: "control" }, value),
|
||||
createElement(
|
||||
Accordion.Panel,
|
||||
{ key: "panel" },
|
||||
Array.from({ length: 3 }, (_unused, index) =>
|
||||
createElement("span", { className: marker, key: index }, `${marker}-${index}`),
|
||||
),
|
||||
),
|
||||
]);
|
||||
|
||||
await act(async () => {
|
||||
root.render(
|
||||
createElement(
|
||||
MantineProvider,
|
||||
null,
|
||||
createElement(
|
||||
Accordion,
|
||||
{ defaultValue: "open", ...(keepMounted === undefined ? {} : { keepMounted }) },
|
||||
item("open", "shown"),
|
||||
item("collapsed", "offscreen"),
|
||||
),
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
return {
|
||||
shown: container.querySelectorAll(".shown").length,
|
||||
offscreen: container.querySelectorAll(".offscreen").length,
|
||||
};
|
||||
};
|
||||
|
||||
it("renders collapsed panel children by default, which is the cost being avoided", async () => {
|
||||
const counts = await renderAccordion(undefined);
|
||||
|
||||
expect(counts.shown).toBe(3);
|
||||
expect(counts.offscreen).toBe(3);
|
||||
});
|
||||
|
||||
it("does not render collapsed panel children when keepMounted is false", async () => {
|
||||
const counts = await renderAccordion(false);
|
||||
|
||||
expect(counts.shown).toBe(3);
|
||||
expect(counts.offscreen).toBe(0);
|
||||
});
|
||||
});
|
||||
@@ -90,7 +90,16 @@ export function InstanceCard({ instance, options, isTiny, widgetKey }: InstanceC
|
||||
</Group>
|
||||
</Group>
|
||||
|
||||
<Accordion variant="filled" chevronPosition="right" multiple value={openSections} onChange={setOpenSections}>
|
||||
{/* Collapsed panels are rendered by default, so all three resource lists were built even when
|
||||
none is open. The open sections are parent state, so nothing is lost by unmounting. */}
|
||||
<Accordion
|
||||
variant="filled"
|
||||
chevronPosition="right"
|
||||
multiple
|
||||
keepMounted={false}
|
||||
value={openSections}
|
||||
onChange={setOpenSections}
|
||||
>
|
||||
{options.showServers && (
|
||||
<ServersSection
|
||||
servers={instance.instanceInfo.servers}
|
||||
|
||||
@@ -56,7 +56,14 @@ export function SingleInstanceLayout({ instance, options, isTiny, widgetKey }: S
|
||||
</Anchor>
|
||||
</Group>
|
||||
|
||||
<Accordion variant="contained" chevronPosition="right" multiple value={openSections} onChange={setOpenSections}>
|
||||
<Accordion
|
||||
variant="contained"
|
||||
chevronPosition="right"
|
||||
multiple
|
||||
keepMounted={false}
|
||||
value={openSections}
|
||||
onChange={setOpenSections}
|
||||
>
|
||||
{options.showServers && (
|
||||
<ServersSection
|
||||
servers={instance.instanceInfo.servers}
|
||||
|
||||
@@ -120,7 +120,7 @@ export default function FirewallWidget({ integrationIds, width, itemId }: Widget
|
||||
{firewallsInterfacesData
|
||||
.filter(({ integration }) => integration.id === (selectedFirewall || initialSelectedFirewall))
|
||||
.map(({ summary }) => (
|
||||
<Accordion key="interfaces" value={accordionValue} onChange={setAccordionValue}>
|
||||
<Accordion key="interfaces" keepMounted={false} value={accordionValue} onChange={setAccordionValue}>
|
||||
<Accordion.Item value="interfaces">
|
||||
<Accordion.Control icon={isTiny ? null : <IconTopologyBus size={16} />}>
|
||||
<Text size={isTiny ? "8px" : "xs"}> {t("widget.firewall.widget.interfaces.title")} </Text>
|
||||
|
||||
@@ -81,7 +81,7 @@ export const ClusterHealthMonitoring = ({
|
||||
isTiny={isTiny}
|
||||
/>
|
||||
{options.visibleClusterSections.length >= 1 && (
|
||||
<Accordion variant="contained" chevronPosition="right" multiple defaultValue={defaultValue}>
|
||||
<Accordion variant="contained" chevronPosition="right" multiple keepMounted={false} defaultValue={defaultValue}>
|
||||
{options.visibleClusterSections.includes("node") && (
|
||||
<ResourceAccordionItem
|
||||
value="node"
|
||||
|
||||
Reference in new issue
Block a user