diff --git a/admin-ui/__specs__/bed-picker-help-flyover.spec.tsx b/admin-ui/__specs__/bed-picker-help-flyover.spec.tsx new file mode 100644 index 00000000..dbceaafa --- /dev/null +++ b/admin-ui/__specs__/bed-picker-help-flyover.spec.tsx @@ -0,0 +1,98 @@ +// @jest-environment jsdom +// gh-#431 — Station Imaging "Bed (optional)" gets a ? help flyover, matching the settings page's +// SettingHelpFlyover pattern: the shared `HelpFlyover` (gh-#209 extraction), the exact idiom the +// booth log's Mode column header already uses (LlmCallsFeed.tsx). +// +// Runner: Jest (jsdom) + @testing-library/react. BedPicker has no standalone page of its own — +// it renders inside SafeContentClient's Generate form (safe-content-redesign.spec.tsx's own +// coverage target), so this spec mounts that same client. VoiceControl fetches GET /api/voices +// on mount (STORY-098/F29.5); the fetch mock here is a generic ok/empty-array stub so that +// unrelated fetch never fails, mirroring safe-content-redesign.spec.tsx's own VOICES_MOUNT_SPEC +// precedent. + +import { describe, it, expect, beforeEach, afterEach, jest } from "@jest/globals"; +import { render, screen } from "@testing-library/react"; +import "@testing-library/jest-dom"; +import { Toaster } from "@/components/ui/toast"; +import { SafeContentClient } from "../app/(authed)/safe-content/SafeContentClient"; +import type { SafeContentClientProps } from "../app/(authed)/safe-content/SafeContentClient"; +import type { LibraryDto } from "../lib/library"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function makeLibraries(): LibraryDto[] { + return [{ id: 7, name: "safe", mediaCount: 2 }]; +} + +function renderClient(overrides: Partial = {}): ReturnType { + const props: SafeContentClientProps = { + libraries: makeLibraries(), + initialLibraryId: 7, + initialSegments: [], + initialOutOfScope: false, + defaultText: "You're listening to {StationName}. We'll be right back — stay tuned.", + defaultTitle: "Please Stand By", + ...overrides, + }; + return render( + <> + + + + ); +} + +// --------------------------------------------------------------------------- +// Feature: the Bed field explains itself (gh-#431) +// --------------------------------------------------------------------------- + +describe("Feature: the Bed field explains itself", () => { + let originalFetch: typeof fetch; + + beforeEach(() => { + originalFetch = global.fetch; + global.fetch = jest.fn().mockResolvedValue({ + ok: true, + status: 200, + json: jest.fn<() => Promise>().mockResolvedValue([]), + headers: new Headers({ "content-type": "application/json" }), + } as unknown as Response) as unknown as typeof fetch; + }); + + afterEach(() => { + global.fetch = originalFetch; + jest.clearAllMocks(); + }); + + describe("Scenario: a ? flyover sits next to the Bed label", () => { + it("renders the ducking/padding/bake-in/deployment-setting help copy, mounted but hidden until asked for", () => { + renderClient(); + + const trigger = screen.getByRole("button", { name: "Help: Bed" }); + expect(trigger).toHaveAttribute("aria-expanded", "false"); + + const panel = screen.getByTestId("bed-help"); + expect(panel).toBeInTheDocument(); + expect(panel).not.toBeVisible(); + + const copy = panel.textContent ?? ""; + expect(copy).toMatch(/mixed UNDER the generated voice/); + expect(copy).toMatch(/main-catalog jingle or instrumental/); + expect(copy).toMatch(/ducked \(−12 dB\)/); + expect(copy).toMatch(/padded 1\.5 s before and after the voice/); + expect(copy).toMatch(/loops if shorter/); + expect(copy).toMatch(/honours the track's cue points/); + expect(copy).toMatch(/baked into the audio file at generate time/); + expect(copy).toMatch(/regenerating the segment/); + expect(copy).toMatch(/Duck\/pad amounts are deployment settings \(env\), not live settings/); + }); + + it("does not change the Bed field's accessible name (still labelled 'Bed (optional)')", () => { + renderClient(); + + expect(screen.getByRole("combobox", { name: /bed \(optional\)/i })).toBeInTheDocument(); + }); + }); +}); diff --git a/admin-ui/__specs__/settings-area-tabs.spec.tsx b/admin-ui/__specs__/settings-area-tabs.spec.tsx index 040d6e5e..5f325a0d 100644 --- a/admin-ui/__specs__/settings-area-tabs.spec.tsx +++ b/admin-ui/__specs__/settings-area-tabs.spec.tsx @@ -281,7 +281,8 @@ describe("Feature: settings areas render as tabs", () => { describe("Scenario (sad path): a rejected key on a non-visible tab is surfaced", () => { it("a 400 auto-switches to the first offending tab and shows the inline error", async () => { const validationProblem = { - errors: { settings: ["Must be a non-empty absolute http/https URL"] }, + // gh-#425 real shape — keyed by the offending setting key, not a flat "settings" bucket. + errors: { "Tts:Endpoint": ["Must be a non-empty absolute http/https URL"] }, title: "One or more settings values are invalid.", status: 400, }; @@ -307,7 +308,8 @@ describe("Feature: settings areas render as tabs", () => { it("marks the offending tab with a validation-error flag", async () => { const validationProblem = { - errors: { settings: ["Must be a non-empty absolute http/https URL"] }, + // gh-#425 real shape — keyed by the offending setting key, not a flat "settings" bucket. + errors: { "Tts:Endpoint": ["Must be a non-empty absolute http/https URL"] }, status: 400, }; makeFetchMock(400, validationProblem); diff --git a/admin-ui/__specs__/settings-help-coverage.spec.tsx b/admin-ui/__specs__/settings-help-coverage.spec.tsx index 78293265..10959e83 100644 --- a/admin-ui/__specs__/settings-help-coverage.spec.tsx +++ b/admin-ui/__specs__/settings-help-coverage.spec.tsx @@ -158,6 +158,16 @@ describe("Feature: Every settings field explains itself", () => { expect( screen.getByTestId("setting-help-Library:CueDetection:MinSilenceDurationSec") ).toHaveTextContent(/greater than 0.*60/i); + // gh-#427 — pin the ±90/±180 lat/lon bounds stated in the help copy so they can't silently + // drift from the real coordinate ranges (SettingValidator itself is deliberately + // AlwaysValid for these two keys; WeatherContextProvider's own fail-closed check is what + // actually treats anything outside these ranges the same as blank — F108.1). + expect(screen.getByTestId("setting-help-Station:Location:Latitude")).toHaveTextContent( + /-90.*90/ + ); + expect(screen.getByTestId("setting-help-Station:Location:Longitude")).toHaveTextContent( + /-180.*180/ + ); }); }); diff --git a/admin-ui/__specs__/settings-page.spec.tsx b/admin-ui/__specs__/settings-page.spec.tsx index 01240935..f0e337f8 100644 --- a/admin-ui/__specs__/settings-page.spec.tsx +++ b/admin-ui/__specs__/settings-page.spec.tsx @@ -366,9 +366,10 @@ describe("Feature: Edit station settings", () => { describe("Scenario: a rejected setting is surfaced", () => { it("a 400 ValidationProblemDetails shows the real backend message and does not claim success", async () => { - // Real shape: ASP.NET Core ValidationProblemDetails with errors keyed under "settings" + // Real shape (gh-#425): ASP.NET Core ValidationProblemDetails, keyed by the actual + // offending setting key — not a flat "settings" bucket. const validationProblem = { - errors: { settings: ["Must be between -40 and 0"] }, + errors: { "Loudness:TargetLufs": ["Must be between -40 and 0"] }, title: "One or more settings values are invalid.", status: 400, }; @@ -394,5 +395,32 @@ describe("Feature: Edit station settings", () => { // "Settings saved." must NOT appear expect(screen.queryByRole("status")).toBeNull(); }); + + it("a per-key error paints only the offending field, not every changed field (gh-#425)", async () => { + // Two fields changed; the backend rejects only one of them, keyed by ITS key alone. + const validationProblem = { + errors: { "Loudness:TargetLufs": ["Must be between -40 and 0"] }, + title: "One or more settings values are invalid.", + status: 400, + }; + makeFetchMock(400, validationProblem); + const settings = makeSettings(); + renderWithProviders(); + + fireEvent.change(screen.getByLabelText(/Loudness:TargetLufs/), { target: { value: "50" } }); + fireEvent.change(screen.getByLabelText(/GW_XFADE_MAX/), { target: { value: "10" } }); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: /save settings/i })); + await Promise.resolve(); + }); + + await waitFor(() => { + // Exactly one alert region exists, and it names the offending key's own message — the + // valid GW_XFADE_MAX change gets no error at all. + expect(screen.getAllByRole("alert")).toHaveLength(1); + expect(screen.getByRole("alert")).toHaveTextContent("Must be between -40 and 0"); + }); + }); }); }); diff --git a/admin-ui/__specs__/settings-persona-control.spec.tsx b/admin-ui/__specs__/settings-persona-control.spec.tsx new file mode 100644 index 00000000..fa6f7e92 --- /dev/null +++ b/admin-ui/__specs__/settings-persona-control.spec.tsx @@ -0,0 +1,213 @@ +// @jest-environment jsdom +// gh-#426 — `Context:Weather:PersonaId`/`Context:History:PersonaId` get a persona dropdown +// instead of a bare number input. Both keys hold a persona ROW ID as a string on the wire; +// null/0 means "the on-air DJ (default)" (SPEC F107.7, SettingValidator's ContextPersonaIdMin +// remarks). +// +// Runner: Jest (jsdom) + @testing-library/react, mirroring settings-audience-control.spec.tsx's +// house pattern (renderWithProviders, makeSequencedFetchMock) — SettingsForm calls useConfirm() +// unconditionally, so every render needs a ConfirmDialogProvider ancestor. + +import { describe, it, expect, beforeEach, afterEach, jest } from "@jest/globals"; +import { render, screen, fireEvent, act, waitFor } from "@testing-library/react"; +import "@testing-library/jest-dom"; +import type { ReactElement } from "react"; +import { ConfirmDialogProvider } from "@/components/ui/confirm-dialog"; +import { Toaster } from "@/components/ui/toast"; +import { SettingsForm } from "../app/(authed)/settings/SettingsForm"; +import type { SettingDto } from "../app/(authed)/settings/SettingsForm"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +const WEATHER_PERSONA_KEY = "Context:Weather:PersonaId"; +const HISTORY_PERSONA_KEY = "Context:History:PersonaId"; + +/** Minimal roster fixture — `usePersonaList` only reads `id`/`name` off each row. */ +const PERSONAS = [ + { id: 3, name: "Flip" }, + { id: 7, name: "Mike Rophone" }, +]; + +function makePersonaIdSetting( + key: string, + overrides: Partial = {} +): SettingDto { + return { + key, + value: "0", + source: "default", + applyMode: "live", + kind: "number", + unit: "", + ...overrides, + }; +} + +interface MockResponseSpec { + status: number; + body?: unknown; +} + +/** A fetch mock that replays one response per call, in order (last spec repeats if exhausted). */ +function makeSequencedFetchMock(specs: MockResponseSpec[]): jest.MockedFunction { + let callIndex = 0; + const fn = jest.fn().mockImplementation(async () => { + const spec = specs[callIndex] ?? specs[specs.length - 1]!; + callIndex += 1; + return { + ok: spec.status >= 200 && spec.status < 300, + status: spec.status, + json: jest.fn<() => Promise>().mockResolvedValue(spec.body ?? {}), + headers: new Headers(), + } as unknown as Response; + }); + global.fetch = fn as unknown as typeof fetch; + return fn; +} + +function renderWithProviders(node: ReactElement): ReturnType { + return render( + + {node} + + + ); +} + +// --------------------------------------------------------------------------- +// Feature: Context:*:PersonaId's dedicated Settings control +// --------------------------------------------------------------------------- + +describe("Feature: Context:*:PersonaId's dedicated Settings control", () => { + let originalFetch: typeof fetch; + + beforeEach(() => { + originalFetch = global.fetch; + }); + + afterEach(() => { + global.fetch = originalFetch; + jest.clearAllMocks(); + }); + + describe("Scenario: the field renders as a persona dropdown", () => { + it("renders a select fed by GET /api/personas, default option first, current value preselected (gh-#426)", async () => { + makeSequencedFetchMock([{ status: 200, body: PERSONAS }]); + renderWithProviders( + + ); + + // "7" is already selectable before the roster loads (an unrecognized value always renders + // its own option — see the "unknown current persona id" scenario below), so wait for the + // roster itself to land rather than for `select.value`, which is stable across both states. + await waitFor(() => { + const select = screen.getByLabelText(new RegExp(WEATHER_PERSONA_KEY)) as HTMLSelectElement; + const optionLabels = Array.from(select.options).map((o) => o.textContent); + expect(optionLabels).toContain("Flip"); + }); + + const select = screen.getByLabelText(new RegExp(WEATHER_PERSONA_KEY)) as HTMLSelectElement; + expect(select.tagName).toBe("SELECT"); + expect(select.value).toBe("7"); + const optionLabels = Array.from(select.options).map((o) => o.textContent); + expect(optionLabels[0]).toBe("On-air DJ (default)"); + expect(optionLabels).toContain("Mike Rophone"); + }); + + it("preselects the default option for value '0' (unset means the on-air DJ, F107.7)", async () => { + makeSequencedFetchMock([{ status: 200, body: PERSONAS }]); + renderWithProviders( + + ); + + await waitFor(() => { + const select = screen.getByLabelText(new RegExp(WEATHER_PERSONA_KEY)) as HTMLSelectElement; + expect(select.value).toBe("0"); + }); + }); + + it("an unrecognized current persona id gets its own 'Unknown persona (#id)' option, not a silent drop", async () => { + makeSequencedFetchMock([{ status: 200, body: PERSONAS }]); + renderWithProviders( + + ); + + await waitFor(() => { + const select = screen.getByLabelText(new RegExp(WEATHER_PERSONA_KEY)) as HTMLSelectElement; + expect(select.value).toBe("42"); + }); + + expect(screen.getByText("Unknown persona (#42)")).toBeInTheDocument(); + }); + }); + + describe("Scenario: submission plumbing is untouched", () => { + it("picking a persona by name submits the id string on the shipped changed-keys PUT batch (F54.4)", async () => { + const mockFetch = makeSequencedFetchMock([ + { status: 200, body: PERSONAS }, + { status: 200 }, + ]); + renderWithProviders( + + ); + + const select = await waitFor( + () => screen.getByLabelText(new RegExp(WEATHER_PERSONA_KEY)) as HTMLSelectElement + ); + fireEvent.change(select, { target: { value: "3" } }); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: /save settings/i })); + await Promise.resolve(); + }); + + await waitFor(() => expect(mockFetch).toHaveBeenCalledTimes(2)); + const [url, init] = mockFetch.mock.calls[1] as [string, RequestInit]; + expect(url).toBe("/api/settings"); + expect(init.method).toBe("PUT"); + const body = JSON.parse(init.body as string) as Array<{ key: string; value: string }>; + expect(body).toEqual([{ key: WEATHER_PERSONA_KEY, value: "3" }]); + }); + + it("picking 'On-air DJ (default)' submits '0' explicitly", async () => { + const mockFetch = makeSequencedFetchMock([ + { status: 200, body: PERSONAS }, + { status: 200 }, + ]); + renderWithProviders( + + ); + + const select = await waitFor( + () => screen.getByLabelText(new RegExp(WEATHER_PERSONA_KEY)) as HTMLSelectElement + ); + fireEvent.change(select, { target: { value: "0" } }); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: /save settings/i })); + await Promise.resolve(); + }); + + await waitFor(() => expect(mockFetch).toHaveBeenCalledTimes(2)); + const [, init] = mockFetch.mock.calls[1] as [string, RequestInit]; + const body = JSON.parse(init.body as string) as Array<{ key: string; value: string }>; + expect(body).toEqual([{ key: WEATHER_PERSONA_KEY, value: "0" }]); + }); + }); + + describe("Scenario: both Context providers register the same control", () => { + it("Context:History:PersonaId also renders as a persona dropdown (gh-#426)", async () => { + makeSequencedFetchMock([{ status: 200, body: PERSONAS }]); + renderWithProviders( + + ); + + await waitFor(() => { + const select = screen.getByLabelText(new RegExp(HISTORY_PERSONA_KEY)) as HTMLSelectElement; + expect(select.tagName).toBe("SELECT"); + }); + }); + }); +}); diff --git a/admin-ui/app/(authed)/safe-content/BedPicker.tsx b/admin-ui/app/(authed)/safe-content/BedPicker.tsx index 3d121eeb..46ca1deb 100644 --- a/admin-ui/app/(authed)/safe-content/BedPicker.tsx +++ b/admin-ui/app/(authed)/safe-content/BedPicker.tsx @@ -2,8 +2,19 @@ import { useId, useState, type KeyboardEvent, type ReactNode } from "react"; import { Button } from "@/components/ui/button"; +import { HelpFlyover } from "@/components/ui/help-flyover"; import { cn } from "@/lib/utils"; +/** The bed field's `?` flyover copy (gh-#431), verbatim per the issue. */ +const BED_HELP_TEXT = + "An optional music bed mixed UNDER the generated voice when this segment is created — pick " + + "any main-catalog jingle or instrumental. The bed is ducked (−12 dB), padded 1.5 s " + + "before and after the voice, loops if shorter, and honours the track's cue points. It is " + + "baked into the audio file at generate time — changing it later means regenerating the " + + "segment. Duck/pad amounts are deployment settings (env), not live settings."; + +const BED_HELP_ID = "bed-search-help"; + /** A catalog row offered as a bed candidate — id is numeric (bedMediaId on the wire, F27.3). */ export interface BedCandidate { mediaId: number; @@ -129,9 +140,14 @@ export function BedPicker({ selected, onSelect, onClear, disabled }: BedPickerPr return (
- +
+ + + {BED_HELP_TEXT} + +
{selected !== null ? (
@@ -151,6 +167,7 @@ export function BedPicker({ selected, onSelect, onClear, disabled }: BedPickerPr aria-controls={listboxId} aria-autocomplete="list" aria-activedescendant={activeOptionId} + aria-describedby={BED_HELP_ID} value={query} onChange={(e) => setQuery(e.currentTarget.value)} onKeyDown={handleKeyDown} diff --git a/admin-ui/app/(authed)/settings/PersonaSettingControl.tsx b/admin-ui/app/(authed)/settings/PersonaSettingControl.tsx new file mode 100644 index 00000000..a1901b43 --- /dev/null +++ b/admin-ui/app/(authed)/settings/PersonaSettingControl.tsx @@ -0,0 +1,67 @@ +"use client"; + +import { type ChangeEvent, type ReactNode } from "react"; +import { usePersonaList } from "@/lib/use-persona-list"; +import type { SettingControlProps } from "./settings-types"; + +/** Matches SettingField's shipped single-line control styling — the `VoiceSettingControl` + * precedent. */ +const CONTROL_CLASSES = + "h-9 w-full max-w-md rounded-[6px] border border-line bg-surface px-2 text-[0.85rem] text-ink disabled:opacity-50"; + +/** The wire sentinel both `Context:{Weather,History}:PersonaId` share for "no explicit persona" — + * `SettingValidator`'s own `ContextPersonaIdMin` remarks: null/0 both mean the on-air DJ. GET + * never returns a literal `null`, only `""` (unset) or a numeric string, so both collapse to this + * same default option. */ +const DEFAULT_PERSONA_VALUE = "0"; + +/** + * `Context:Weather:PersonaId`/`Context:History:PersonaId`'s settings-page control (gh-#426; + * registered in `SettingsForm`'s per-key control-override registry, F54.1). Both keys hold a + * persona ROW ID as a string on the wire — 0 (or unset) means "the on-air DJ" (SPEC F107.7) — so + * the shipped plain number input made an operator go look up an id by hand before they could name + * a persona. This renders the roster by NAME instead; the submitted value is still the id string, + * the validator's own wire contract is unchanged. + * + * Sources the roster from `usePersonaList` (SPEC F79.5 — one `GET /api/personas` listing path for + * this control, not a second inline fetch; mirrors `VoiceSettingControl`'s own `useVoiceList` + * precedent after its gh-#426 refactor). A current value the fetched roster doesn't recognize (a + * deleted persona, or a value staged before the roster loaded) still gets its own option, marked + * "Unknown persona (#id)" — so simply reopening the page and saving never silently rewrites it to + * a different persona. + */ +export function PersonaSettingControl({ + controlId, + value, + onChange, + disabled, +}: SettingControlProps): ReactNode { + const status = usePersonaList(); + + const isLoading = status.kind === "loading"; + const personas = status.kind === "loaded" ? status.personas : []; + const selectedValue = value === "" ? DEFAULT_PERSONA_VALUE : value; + const currentIsUnknown = + selectedValue !== DEFAULT_PERSONA_VALUE && + !personas.some((persona) => String(persona.id) === selectedValue); + + return ( + + ); +} diff --git a/admin-ui/app/(authed)/settings/SettingsForm.tsx b/admin-ui/app/(authed)/settings/SettingsForm.tsx index fc42d0dd..aecf4bc9 100644 --- a/admin-ui/app/(authed)/settings/SettingsForm.tsx +++ b/admin-ui/app/(authed)/settings/SettingsForm.tsx @@ -19,6 +19,7 @@ import { AudienceSettingControl } from "./AudienceSettingControl"; import { ChoiceSettingControl } from "./ChoiceSettingControl"; import { CorrectionsSettingControl } from "./CorrectionsSettingControl"; import { EngineByKindSettingControl } from "./EngineByKindSettingControl"; +import { PersonaSettingControl } from "./PersonaSettingControl"; import { SafeScopeAvailabilityBadge } from "./SafeScopeAvailabilityBadge"; import { SettingHelpFlyover } from "./SettingHelpFlyover"; import type { SettingsHelpKey } from "./settings-help-keys"; @@ -345,13 +346,19 @@ const FIELD_HELP_TEXT: Record = { "Context:History:PersonaId": "Which persona voices history segments. 0 defers to the on-air DJ (the unset default does the same).", - // ── Station broadcast location (SPEC F108.1, F108.3, PLAN T226) ─────────────────────────── + // ── Station broadcast location (SPEC F108.1, F108.3, PLAN T226, gh-#427) ────────────────── "Station:Location:Latitude": - "The station's broadcast latitude, used only to fetch weather — never spoken or logged. " + - "Blank means no coordinate is configured; an invalid value behaves the same as blank.", + "Signed decimal degrees only (negative = south), with a period as the decimal separator — " + + "e.g. 51.0447 or -33.8688. Accepted range: -90 to 90; only the first 4 decimal places are " + + "used. Degrees-minutes-seconds and degrees-decimal-minutes formats are NOT accepted. Used " + + "only to fetch weather — never spoken or logged. Blank behaves exactly like an invalid " + + "value: weather stays silently off (F108.1).", "Station:Location:Longitude": - "The station's broadcast longitude, used only to fetch weather — never spoken or logged. " + - "Blank means no coordinate is configured; an invalid value behaves the same as blank.", + "Signed decimal degrees only (negative = west), with a period as the decimal separator — " + + "e.g. -114.0719 or 151.2093. Accepted range: -180 to 180; only the first 4 decimal places " + + "are used. Degrees-minutes-seconds and degrees-decimal-minutes formats are NOT accepted. " + + "Used only to fetch weather — never spoken or logged. Blank behaves exactly like an invalid " + + "value: weather stays silently off (F108.1).", "Station:Location:SpokenName": "The only location text ever spoken or logged, e.g. \"Calgary\" — coordinates themselves " + "never air. Blank means weather segments name no place at all.", @@ -393,6 +400,10 @@ const SETTING_CONTROL_REGISTRY: Record>(() => initialValuesFrom(settings)); const [status, setStatus] = useState({ kind: "idle" }); /** - * Per-field validation errors surfaced inline next to the relevant control. - * Populated when a 400 is returned from PUT — attributed to every key in the - * submitted batch, since the backend reports validation failures batch-wide - * under a single "settings" key (F28.9: field-level errors stay inline, never - * a page-wide banner). + * Per-field validation errors surfaced inline next to the relevant control. Populated when a + * 400 is returned from PUT, keyed exactly the way the backend's own `ValidationProblemDetails` + * keys them (gh-#425): a message under the offending setting's own key lands ONLY on that + * field; a message under "" — ASP.NET's conventional keyless bucket, used for both an + * empty-key entry and a cross-field `ValidateBatch` failure — has no single field to blame, so + * it paints every CHANGED field, exactly the old aggregate behavior, but scoped to just those + * messages (F28.9: field-level errors stay inline, never a page-wide banner). */ const [fieldErrors, setFieldErrors] = useState>({}); @@ -719,32 +732,52 @@ export function SettingsForm({ settings, libraries = [], timeZone }: SettingsFor } if (resp.status === 400) { - let messages: string[] = []; + // gh-#425 — the backend keys each message by the setting key it actually belongs to. A + // message under "" (or under a key that isn't part of THIS batch at all) has no single + // field to blame — it's batch-wide (an empty-key entry, or a cross-field ValidateBatch + // failure) and paints every changed field, exactly the old aggregate behavior; a message + // under a real submitted key stays scoped to that field alone, never leaking onto a + // valid sibling in the same batch. + const keyedErrors: Record = {}; + const batchWideMessages: string[] = []; try { const raw = (await resp.json()) as unknown; if (isValidationProblemDetails(raw)) { - const errors = raw.errors as Record; - const settingsErrors = errors["settings"]; - if (Array.isArray(settingsErrors) && settingsErrors.length > 0) { - messages = settingsErrors; - } else { - messages = Object.values(errors).flat(); + const changedKeys = new Set(changed.map((c) => c.key)); + for (const [key, messages] of Object.entries(raw.errors)) { + if (!Array.isArray(messages) || messages.length === 0) continue; + if (key !== "" && changedKeys.has(key)) { + keyedErrors[key] = messages; + } else { + batchWideMessages.push(...messages); + } } } } catch { - // malformed 400 body — fall through to empty messages + // malformed 400 body — fall through to no messages } - // AC5 — every key in the submitted batch gets the returned message(s) inline at its - // field (the backend reports validation failures batch-wide, not per key). Status - // resets to idle so isPending drops to false and the form re-enables, letting the + const nextFieldErrors: Record = { ...keyedErrors }; + if (batchWideMessages.length > 0) { + for (const { key } of changed) { + if (keyedErrors[key] === undefined) { + nextFieldErrors[key] = batchWideMessages; + } + } + } + + // Status resets to idle so isPending drops to false and the form re-enables, letting the // operator correct the value and retry (the K5 stuck-Saving regression class). setStatus({ kind: "idle" }); - setFieldErrors(Object.fromEntries(changed.map((c) => [c.key, messages]))); - // gh-#144 — the rejected batch may span tabs the operator isn't looking at. Auto-switch - // to the first offending tab (strip order) so the inline error is on screen; the other + setFieldErrors(nextFieldErrors); + // gh-#144/gh-#425 — the rejected batch may span tabs the operator isn't looking at. + // Auto-switch to the first tab (strip order) carrying an OFFENDING key: a per-key + // failure names its own tab precisely; a batch-wide-only failure (no per-key entries at + // all) falls back to any changed key, matching the pre-per-key behavior. The other // implicated tabs stay flagged by their danger dot in the strip. - const offendingTabId = firstTabWithAnyKey(changed.map((c) => c.key)); + const offendingKeys = + Object.keys(keyedErrors).length > 0 ? Object.keys(keyedErrors) : changed.map((c) => c.key); + const offendingTabId = firstTabWithAnyKey(offendingKeys); if (offendingTabId !== undefined) { setActiveTabId(offendingTabId); } diff --git a/admin-ui/app/(authed)/settings/VoiceSettingControl.tsx b/admin-ui/app/(authed)/settings/VoiceSettingControl.tsx index 1802b81e..76c691a3 100644 --- a/admin-ui/app/(authed)/settings/VoiceSettingControl.tsx +++ b/admin-ui/app/(authed)/settings/VoiceSettingControl.tsx @@ -1,17 +1,9 @@ "use client"; -import { useEffect, useState, type ChangeEvent, type ReactNode } from "react"; +import { type ChangeEvent, type ReactNode } from "react"; +import { useVoiceList } from "@/lib/use-voice-list"; import type { SettingControlProps } from "./settings-types"; -type VoiceListStatus = - | { kind: "loading" } - | { kind: "loaded"; voices: string[] } - | { kind: "error" }; - -function isVoiceIdList(raw: unknown): raw is string[] { - return Array.isArray(raw) && raw.every((entry) => typeof entry === "string"); -} - /** Matches SettingField's shipped single-line control styling (text/number inputs). */ const CONTROL_CLASSES = "h-9 w-full max-w-md rounded-[6px] border border-line bg-surface px-2 text-[0.85rem] text-ink disabled:opacity-50"; @@ -29,7 +21,9 @@ const CONTROL_CLASSES = * an external `Tts:Endpoint` may serve a voice set the shipped Kokoro list doesn't know * about (F36), so silently dropping it would strand the operator on an unrelated voice. * Fetch failure degrades to the same free-text-input-plus-notice fallback as the safe-content - * control. One fetch per mount, no polling/retry. + * control. Sources the roster from `useVoiceList` (SPEC F79.5 — the one `GET /api/voices` + * listing path; this control used to duplicate that fetch inline — fixed gh-#426, alongside + * `PersonaSettingControl` adopting the same one-hook idiom for `GET /api/personas`). */ export function VoiceSettingControl({ controlId, @@ -37,34 +31,7 @@ export function VoiceSettingControl({ onChange, disabled, }: SettingControlProps): ReactNode { - const [status, setStatus] = useState({ kind: "loading" }); - - useEffect(() => { - let cancelled = false; - - async function loadVoices(): Promise { - try { - const resp = await fetch("/api/voices"); - if (!resp.ok) { - if (!cancelled) setStatus({ kind: "error" }); - return; - } - const raw = (await resp.json()) as unknown; - if (!isVoiceIdList(raw)) { - if (!cancelled) setStatus({ kind: "error" }); - return; - } - if (!cancelled) setStatus({ kind: "loaded", voices: raw }); - } catch { - if (!cancelled) setStatus({ kind: "error" }); - } - } - - void loadVoices(); - return () => { - cancelled = true; - }; - }, []); + const status = useVoiceList(); if (status.kind === "error") { const noticeId = `${controlId}-notice`; diff --git a/admin-ui/lib/use-persona-list.ts b/admin-ui/lib/use-persona-list.ts new file mode 100644 index 00000000..f1d96204 --- /dev/null +++ b/admin-ui/lib/use-persona-list.ts @@ -0,0 +1,70 @@ +"use client"; + +import { useEffect, useState } from "react"; + +/** One row of the persona roster this hook feeds — just the two fields a picker control needs. */ +export interface PersonaListEntry { + id: number; + name: string; +} + +export type PersonaListState = + | { kind: "loading" } + | { kind: "loaded"; personas: PersonaListEntry[] } + | { kind: "error" }; + +function isPersonaListEntryList(raw: unknown): raw is PersonaListEntry[] { + return ( + Array.isArray(raw) && + raw.every((entry) => { + if (typeof entry !== "object" || entry === null) return false; + const obj = entry as Record; + return typeof obj["id"] === "number" && typeof obj["name"] === "string"; + }) + ); +} + +/** + * The one `GET /api/personas` fetch+parse implementation for a control that needs the full + * id/name roster to build a picker (SPEC F79.5's "never a second listing path" idiom, applied to + * personas the same way `useVoiceList` applies it to voices — `PersonaSettingControl` is this + * hook's only caller today, gh-#426). + * + * A sibling of `usePersonaDirectory` (`lib/use-persona-directory.ts`), not a reuse: that hook + * resolves a bare `personaId` to a display name for the booth log / now-playing surfaces and + * returns a `Map`; this one feeds a `