test(widgets): pin the Tabs behaviour that keepMounted={false} depends on

health-monitoring and media-missing pass keepMounted={false} because Mantine
otherwise builds the inactive panel's whole subtree. That came from reading
Mantine's docs, 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 and nobody would notice.

Verified in the installed version instead. 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 changes, this fails and the prop should
come back out.

Also records two things checked while re-reading the audit:

_inputs/ is excluded from the widget sweep on purpose, not by oversight - its
2,748 lines are option editors reached only from the edit modal and the
custom-widget form, and the one widget importing from it uses `import type`,
which is erased at build.

The "statically analyzable dynamic imports" rule has exactly one exception,
translation/src/mapping.ts building import(`./lang/${language}.json`) over 36
languages. It stands: the rule targets accidental over-inclusion, and every
language is genuinely reachable since the user can switch at runtime. Server-side
message loading is already memoised per locale, so it is not per-request either.
This commit is contained in:
Thomas Camlong committed 2026-08-13 18:28:29 +02:00
1 parent 43cc40eb66
commit fd351e4adf
2 files changed
+121

No files matched your search

+19
View File
@@ -2,6 +2,12 @@
Every widget in `packages/widgets/src`, worked through one at a time. 50 widgets, 22,726 lines.
`_inputs/` is excluded on purpose, not overlooked. Its 2,748 lines are widget *option* editors, reached
only from the edit modal and the custom-widget form; the one widget that imports from it,
`timetable/component.tsx`, uses `import type`, which is erased at build. None of it is on the board
render path. The one real finding there — a drag handle rebuilt every frame of a drag — was found and
fixed by the anti-pattern audit, which does cover the directory.
The point of doing this exhaustively rather than opportunistically: the two largest client wins so far
(`OverflowBadge`, `content-visibility` on media-releases) were both found in widgets nobody suspected,
and both turned out to be *shared* problems that repeated across many widgets. A sweep finds the
@@ -41,6 +47,17 @@ their panels mounted and are left alone:
`health-monitoring` is the worse of the two: its panels are the system and cluster monitors, so a
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
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.
What remains unmeasured is the *magnitude* on a live board — `media-missing` and `health-monitoring`
need reachable integrations to render data, so neither could go through the per-widget component-count
harness. The mechanism is confirmed; the size of the saving is not.
## Changes made
| widget | change | why |
@@ -147,6 +164,8 @@ Sourced with the `skills` CLI — `npx skills find "react performance"`, then
| Ternary over `&&` for conditional render | rendering | **passes** — only 2 `&&` renders exist and both operands are already boolean or string, so nothing can render a literal `0` |
| Passive listeners for scroll | client fetching | **N/A** — no widget registers a scroll listener |
| `Set`/`Map` for membership over `Array.includes` | JS micro-perf | **passes** — the nested scans that exist are over configured integrations and column accessors, single digits either way. `use-persisted-table-layout` already uses a `Set` for the hot half |
| Hoist RegExp out of loops | JS micro-perf | **passes** — the only user-supplied pattern, the release version filter, is compiled once by `compileVersionRegex` before the `.filter()` that uses it |
| Statically analyzable dynamic imports | bundle | **one exception, justified** — `translation/src/mapping.ts` builds `import(`./lang/${language}.json`)` for 36 languages, 6.8 MiB of JSON. The rule targets accidental over-inclusion; here every language is genuinely reachable because the user can switch at runtime, so the context module includes exactly what is needed. Rewriting it as 36 explicit entries would emit the same 36 lazy chunks |
| `React.cache()` for per-request dedup | server | **applied** — 4 functions |
| TanStack Query for dedup, not hand-rolled `useEffect` + `fetch` | client fetching | **passes** — used throughout |
| Direct imports, not barrels | bundle | **partial** — see below |
@@ -0,0 +1,102 @@
import { MantineProvider, Tabs } from "@mantine/core";
import { act, createElement } from "react";
import { createRoot } from "react-dom/client";
import { afterEach, beforeAll, describe, expect, it } from "vitest";
import type { Root } from "react-dom/client";
/**
* `health-monitoring` and `media-missing` pass `keepMounted={false}` to `Tabs` because Mantine
* otherwise builds the inactive panel's whole subtree - two live monitoring views, or two full lists of
* media cards, when only one is ever visible.
*
* That reasoning came from Mantine's documentation, not from this codebase, and it is the entire
* justification for the prop. If a future Mantine version stops rendering inactive panels, the prop
* becomes dead weight and the remount cost it buys is being paid for nothing. This pins the behaviour
* in the version actually installed.
*/
describe("Mantine Tabs keepMounted", () => {
let container: HTMLDivElement;
let root: Root;
beforeAll(() => {
Object.defineProperty(globalThis, "IS_REACT_ACT_ENVIRONMENT", { value: true, writable: true });
Object.defineProperty(window, "matchMedia", {
writable: true,
value: (query: string) => ({
matches: false,
media: query,
onchange: null,
addListener: () => undefined,
removeListener: () => undefined,
addEventListener: () => undefined,
removeEventListener: () => undefined,
dispatchEvent: () => false,
}),
});
});
afterEach(() => {
root?.unmount();
container?.remove();
});
/** Two panels, each with a countable marker per "row", so hidden work is visible to the assertions. */
const renderTabs = async (keepMounted: boolean | undefined) => {
container = document.createElement("div");
document.body.appendChild(container);
root = createRoot(container);
// `children` goes in the props object rather than as variadic arguments, because Tabs.Panel
// declares it as a required prop.
const panel = (value: string, marker: string) =>
createElement(Tabs.Panel, {
value,
key: value,
children: Array.from({ length: 3 }, (_unused, index) =>
createElement("span", { className: marker, key: index }, `${marker}-${index}`),
),
});
await act(async () => {
root.render(
createElement(
MantineProvider,
null,
createElement(
Tabs,
{ defaultValue: "visible", ...(keepMounted === undefined ? {} : { keepMounted }) },
createElement(
Tabs.List,
null,
createElement(Tabs.Tab, { value: "visible" }, "Visible"),
createElement(Tabs.Tab, { value: "hidden" }, "Hidden"),
),
panel("visible", "shown"),
panel("hidden", "offscreen"),
),
),
);
});
return {
shown: container.querySelectorAll(".shown").length,
offscreen: container.querySelectorAll(".offscreen").length,
};
};
it("renders the inactive panel's children by default, which is the cost being avoided", async () => {
const counts = await renderTabs(undefined);
expect(counts.shown).toBe(3);
// The whole reason the widgets pass keepMounted={false}: this is not 0.
expect(counts.offscreen).toBe(3);
});
it("does not render the inactive panel's children when keepMounted is false", async () => {
const counts = await renderTabs(false);
expect(counts.shown).toBe(3);
expect(counts.offscreen).toBe(0);
});
});