From a28193a00ce9cb11f599a420ec431313ce17f865 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 17 Sep 2026 23:45:41 +0200 Subject: [PATCH] test: pin the focus restore, the choice labels and the phone layout to their fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three round-1 fixes whose tests passed against the broken version too. The focus restore keeps watching until the element that closed the box is GONE, rather than spending itself on the first render after the change — the distinction Chrome exposed and jsdom did not, because jsdom commits the save in one render where Chrome takes two. Driven now through the general case instead: the user moved the focus elsewhere while the request was out, renders keep coming while that element is still there, and the restore happens only once it disappears. The choice test's i18n mock returned an option's bare value, which is exactly what dropping the `config.choices.*` lookup renders — a Hungarian admin would have seen `on`/`off` instead of `GPU`/`Szoftver` with the gate green. Marked labels now, still short enough not to move the segmented-bar budget the same file measures. The admin config row's stacking had no test at all, only its sibling fix did — and stacking is the only fix for a text or number key, whose fixed-width input is the wide control. Class contract, as SearchBar.dom.test.tsx already does for the narrow-header rules, since jsdom does no layout. Also: the time-zone cleanup deleted TZ, and Node re-reads it on assignment only, so the process kept the last zone the file set. Harmless until someone appends a test below it. --- .../ConfigPanel.choice.dom.test.tsx | 39 +++++++--- .../components/ConfigPanel.field.dom.test.tsx | 76 +++++++++++++++++++ .../DiagnosticsSettings.dom.test.tsx | 33 ++++++++ frontend/src/lib/format.test.ts | 8 +- 4 files changed, 142 insertions(+), 14 deletions(-) create mode 100644 frontend/src/components/ConfigPanel.field.dom.test.tsx diff --git a/frontend/src/components/ConfigPanel.choice.dom.test.tsx b/frontend/src/components/ConfigPanel.choice.dom.test.tsx index 24a81332..d22dab5f 100644 --- a/frontend/src/components/ConfigPanel.choice.dom.test.tsx +++ b/frontend/src/components/ConfigPanel.choice.dom.test.tsx @@ -4,8 +4,10 @@ // a dropdown, and the pick is what Save sends. A cleared draft (reset to the default) shows the default // as the active option rather than no option at all. // -// i18n is a key-passthrough, except that an option's label is its value (the bar-or-dropdown choice -// weighs the real label length, which a full key would distort); the admin config API is stubbed. +// i18n is a key-passthrough, except for an option's label: a full key would distort the +// bar-or-dropdown choice, which weighs the real label length, so a choice id resolves to its value +// under a one-character marker. The marker is what makes a lost lookup visible — rendering the bare +// value instead of the id would be indistinguishable from `label: c`. The admin config API is stubbed. import { afterEach, describe, expect, it, vi } from "vitest"; import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; @@ -16,7 +18,10 @@ vi.mock("react-i18next", async (importOriginal) => { return { ...actual, useTranslation: () => ({ - t: (key: string) => (key.startsWith("config.choices.") ? key.split(".").pop() : key), + // Only an id the fixture really defines resolves: a dropped lookup renders the bare value and + // a malformed id renders the key (long enough to turn every bar into a dropdown), so the + // assertions below see both. + t: (key: string) => CHOICE_LABELS[key] ?? key, i18n: { language: "en" }, }), }; @@ -37,6 +42,18 @@ const choice = (key: string, choices: string[], value: string, is_set = false): }); const PRESETS = ["ultrafast", "superfast", "veryfast", "faster", "fast", "medium", "slow"]; +const LEVELS = ["TRACE", "DEBUG", "INFO", "WARNING", "ERROR"]; +const CHOICE_LABELS: Record = Object.fromEntries( + ( + [ + ["plex_transcode_hw", ["auto", "on", "off"]], + ["plex_transcode_preset", PRESETS], + ["log_level", LEVELS], + ] as const + ).flatMap(([key, choices]) => + choices.map((c) => [`config.choices.${key}.${c}`, `~${c}`] as const), + ), +); const setConfig = vi.fn((_key: string, _value: unknown) => Promise.resolve({})); const resetConfig = vi.fn((_key: string) => Promise.resolve({})); @@ -52,7 +69,7 @@ vi.mock("../lib/api", async (importOriginal) => { g: [ choice("plex_transcode_hw", ["auto", "on", "off"], "on", true), choice("plex_transcode_preset", PRESETS, "veryfast"), - choice("log_level", ["TRACE", "DEBUG", "INFO", "WARNING", "ERROR"], "INFO"), + choice("log_level", LEVELS, "INFO"), ], }, secrets_manageable: true, @@ -84,10 +101,10 @@ describe("ConfigPanel choice keys", () => { const bar = await screen.findByRole("group", { name: "config.fields.plex_transcode_hw.label" }); const pressed = () => [...bar.querySelectorAll('[aria-pressed="true"]')].map((b) => b.textContent); - expect(pressed()).toEqual(["on"]); + expect(pressed()).toEqual(["~on"]); expect(screen.queryByRole("textbox")).toBeNull(); // no free-text field for a choice key - fireEvent.click(screen.getByRole("button", { name: "off" })); + fireEvent.click(screen.getByRole("button", { name: "~off" })); fireEvent.click(await screen.findByRole("button", { name: "config.save" })); await waitFor(() => expect(setConfig).toHaveBeenCalledWith("plex_transcode_hw", "off")); }); @@ -96,7 +113,7 @@ describe("ConfigPanel choice keys", () => { mount(); await screen.findByRole("group", { name: "config.fields.plex_transcode_hw.label" }); fireEvent.click(screen.getByRole("button", { name: /config\.reset$/ })); - expect(screen.getByRole("button", { name: "auto" }).getAttribute("aria-pressed")).toBe("true"); + expect(screen.getByRole("button", { name: "~auto" }).getAttribute("aria-pressed")).toBe("true"); fireEvent.click(await screen.findByRole("button", { name: "config.save" })); await waitFor(() => expect(resetConfig).toHaveBeenCalledWith("plex_transcode_hw")); }); @@ -104,20 +121,20 @@ describe("ConfigPanel choice keys", () => { it("renders a long list as a dropdown showing only the value", async () => { mount(); const trigger = await screen.findByRole("button", { - name: "config.fields.plex_transcode_preset.label: veryfast", + name: "config.fields.plex_transcode_preset.label: ~veryfast", }); - expect(trigger.textContent).toBe("veryfast"); + expect(trigger.textContent).toBe("~veryfast"); fireEvent.click(trigger); // The popover rows are queried as FilterSelect.dom.test does (`hidden: true`, picked by text). const rows = screen.getAllByRole("menuitemradio", { hidden: true }); - fireEvent.click(rows.find((r) => r.textContent === "slow")!); + fireEvent.click(rows.find((r) => r.textContent === "~slow")!); fireEvent.click(await screen.findByRole("button", { name: "config.save" })); await waitFor(() => expect(setConfig).toHaveBeenCalledWith("plex_transcode_preset", "slow")); }); it("a few options with long labels are a dropdown too: a bar that wide does not fit a phone", async () => { mount(); - await screen.findByRole("button", { name: "config.fields.log_level.label: INFO" }); + await screen.findByRole("button", { name: "config.fields.log_level.label: ~INFO" }); expect(screen.queryByRole("group", { name: "config.fields.log_level.label" })).toBeNull(); }); }); diff --git a/frontend/src/components/ConfigPanel.field.dom.test.tsx b/frontend/src/components/ConfigPanel.field.dom.test.tsx new file mode 100644 index 00000000..38d4ab24 --- /dev/null +++ b/frontend/src/components/ConfigPanel.field.dom.test.tsx @@ -0,0 +1,76 @@ +// @vitest-environment jsdom +// +// E22 round-2 review: locks the stacking that makes an admin config row usable on a phone. The +// control column cannot shrink, so beside a wide control it left the label and its hint a sliver of +// the ~250 px a phone leaves next to the navigation rail. The sibling fix — a long-labelled choice +// becoming a dropdown — is covered in ConfigPanel.choice.dom.test.tsx, but that one does nothing for +// a text, number or secret key, whose `w-52` input is the wide control: this is their only fix. +// +// jsdom does no layout, so what is asserted is the class contract those rules rest on, the same +// trade SearchBar.dom.test.tsx already makes for the narrow-header rules. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, render, screen } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ConfigItem } from "../lib/api"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), + }; +}); + +const text = (key: string): ConfigItem => ({ + key, + type: "str", + group: "g", + secret: false, + min: null, + max: null, + is_set: false, + allow_empty: true, + choices: [], + value: "", + default: "", +}); + +vi.mock("../lib/api", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + api: { + ...actual.api, + adminConfig: () => + Promise.resolve({ groups: { g: [text("site_name")] }, secrets_manageable: true }), + }, + }; +}); +vi.mock("../lib/useMe", () => ({ useMe: () => ({ email_configured: true }) })); + +import ConfigPanel from "./ConfigPanel"; + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +describe("ConfigPanel field row (E22 round 2)", () => { + it("stacks the label above the control below `sm`, and lets the control fit the screen", async () => { + render( + + + , + ); + const label = await screen.findByText("config.fields.site_name.label"); + const row = label.parentElement!.parentElement!; // label → the label/hint column → the row + // One column on a phone, the two-column row only from `sm` up: + expect(row.className).toContain("flex-col"); + expect(row.className).toContain("sm:flex-row"); + // The control column may not out-width the screen, and only stops shrinking from `sm` up: + const control = row.lastElementChild as HTMLElement; + expect(control.className).toContain("max-w-full"); + expect(control.className).toContain("sm:shrink-0"); + expect(control.className).not.toMatch(/(^|\s)shrink-0(\s|$)/); + }); +}); diff --git a/frontend/src/components/DiagnosticsSettings.dom.test.tsx b/frontend/src/components/DiagnosticsSettings.dom.test.tsx index f4531b10..f4fb8c7b 100644 --- a/frontend/src/components/DiagnosticsSettings.dom.test.tsx +++ b/frontend/src/components/DiagnosticsSettings.dom.test.tsx @@ -136,6 +136,39 @@ describe("DiagnosticsSettings", () => { expect(document.activeElement).toBe(levelButton("settings.diagnostics.levels.INFO")); }); + it("the watch outlives a render in which the element it is watching is still there", async () => { + // The distinguishing property of the restore, and the one the browser found: it keeps looking + // until the element is GONE, instead of spending itself on the first render after the change. + // Chrome commits a saved level as two renders and jsdom as one, so this drives the general + // case directly — the user moved the focus somewhere else while the request was out, and that + // somewhere else disappears later, several renders on. + vi.useFakeTimers(); + try { + // Raised, so the status line's countdown is mounted and its 30 s tick keeps re-rendering. + const until = new Date(Date.now() + 3_600_000).toISOString(); + mount({ diagnostics: { level: "DEBUG", until, retention_days: 7 } }); + fireEvent.click(levelButton("settings.diagnostics.levels.TRACE")); + const elsewhere = document.createElement("button"); + document.body.append(elsewhere); + elsewhere.focus(); + + // Cancel closes the box; the watch is armed on `elsewhere`, which is still in the page. + fireEvent.click(screen.getByRole("button", { name: "settings.diagnostics.consent.cancel" })); + expect(document.activeElement).toBe(elsewhere); // their place is kept, nothing is stolen + + // Renders keep coming (the countdown ticks) while it is still there, and must not spend it. + await vi.advanceTimersByTimeAsync(60_000); + expect(document.activeElement).toBe(elsewhere); + + elsewhere.remove(); // now the focus really has fallen to + expect(document.activeElement).toBe(document.body); + await vi.advanceTimersByTimeAsync(30_000); + expect(document.activeElement).toBe(levelButton("settings.diagnostics.levels.DEBUG")); + } finally { + vi.useRealTimers(); + } + }); + it("the ticking status line is not a live region", async () => { const until = new Date(Date.now() + 3_600_000).toISOString(); mount({ diagnostics: { level: "DEBUG", until, retention_days: 7 } }); diff --git a/frontend/src/lib/format.test.ts b/frontend/src/lib/format.test.ts index cb723338..e5193c59 100644 --- a/frontend/src/lib/format.test.ts +++ b/frontend/src/lib/format.test.ts @@ -198,11 +198,13 @@ describe("formatRuntime", () => { // runtime's own locale and time zone are the dev box's, so both languages — and a zone west of UTC — // are pinned explicitly. describe("formatDateTime / formatDate", () => { - const savedTz = process.env.TZ; + // Node re-reads TZ when it is ASSIGNED and not when it is deleted, so the zone has to be put back + // by name — and `process.env.TZ` is unset here, so the name has to come from the resolved zone. + // Vitest's per-file process would contain a leak today, but only until someone adds a test below. + const savedTz = process.env.TZ ?? Intl.DateTimeFormat().resolvedOptions().timeZone; afterEach(() => { i18nStub.language = "en"; - if (savedTz === undefined) delete process.env.TZ; - else process.env.TZ = savedTz; + process.env.TZ = savedTz; }); it("formats a timestamp in the UI language, not in the runtime's", () => {