fix: exclude undefined from _meta values, and cover the JSON worker

- `JsonValue` admits `undefined`, so `RequestMetadata` accepted values that do
  not survive serialization: `JSON.stringify` drops an `undefined` object
  member and rewrites an `undefined` array element to `null`. The type
  contradicted the one promise this payload makes. Added `StrictJsonValue` /
  `StrictJsonObject` and used them for the metadata path.

  Deliberately not a change to the repo-wide `JsonValue`: for tool *arguments*
  `undefined` legitimately means "not supplied". The narrowing is sound
  wherever it lands here, since every one of these values comes from
  `JSON.parse`, which cannot produce `undefined`. The CLI's `parseKeyValuePair`
  needed the same tightening — caught by `clients/cli`'s typecheck, which the
  web `tsc -b` does not cover.

- Nothing asserted the JSON worker's annotations, and the wrapper's own "Not
  valid JSON" message comes from our parser rather than from Ace — so a broken
  worker URL would have left every test passing while the gutter diagnostics
  silently disappeared. Added a Storybook play function that waits for a real
  `session.getAnnotations()` error entry, and verified it fails when the worker
  module URL is pointed at a bad path.

- `aria-describedby` referenced only the error, so a supplied `description` was
  never announced and would have been dropped entirely once the draft went
  invalid. Both ids are referenced now, with the description surviving the
  error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-23 18:27:28 -04:00
commit 616dd9c196
4 changed files with 151 additions and 30 deletions

View file

@ -1,7 +1,7 @@
import { useState } from "react";
import type { Meta, StoryObj } from "@storybook/react-vite";
import { expect, userEvent, within } from "storybook/test";
import type { JsonObject } from "@inspector/core/json/jsonUtils.js";
import { expect, userEvent, waitFor, within } from "storybook/test";
import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js";
import { JsonObjectInput } from "./JsonObjectInput";
const LABEL = "Request metadata JSON";
@ -15,8 +15,8 @@ const LABEL = "Request metadata JSON";
* place Ace's keyboard behavior can be tested: the unit suite runs in
* happy-dom, where Ace's hidden-textarea input path does not work at all.
*/
function ControlledJsonObjectInput({ initial }: { initial: JsonObject }) {
const [value, setValue] = useState<JsonObject>(initial);
function ControlledJsonObjectInput({ initial }: { initial: StrictJsonObject }) {
const [value, setValue] = useState<StrictJsonObject>(initial);
return (
<JsonObjectInput
label="Request metadata"
@ -40,6 +40,7 @@ function aceEditor(canvasElement: HTMLElement): {
focus(): void;
selectAll(): void;
getValue(): string;
session: { getAnnotations(): { row: number; type: string }[] };
} {
const container = canvasElement.querySelector(".ace_editor");
const editor = (
@ -50,6 +51,7 @@ function aceEditor(canvasElement: HTMLElement): {
focus(): void;
selectAll(): void;
getValue(): string;
session: { getAnnotations(): { row: number; type: string }[] };
};
};
})
@ -182,3 +184,34 @@ export const NotAnObject: Story = {
).toBeVisible();
},
};
/**
* The JSON worker marks the offending line in the gutter — the reason this
* component registers `worker-json` through `ace.config.setModuleUrl` with a
* Vite `?url` asset.
*
* Asserted because nothing else covers it: the worker runs out-of-process, so
* if its URL registration breaks, Ace fails to start it and silently falls back
* to no annotations. Every other test here — including the wrapper's own
* "Not valid JSON" message, which comes from our parser, not from Ace — would
* still pass while the gutter diagnostics quietly disappeared.
*/
export const AnnotatesTheOffendingLine: Story = {
args: { initial: {} },
play: async ({ canvasElement }) => {
await focusAndClear(canvasElement);
// A trailing comma on line 2: valid-looking until the closing brace, so the
// worker has to locate it rather than just reject the document.
await userEvent.keyboard('{{"a": 1,{Enter}}');
await waitFor(
async () => {
const annotations = aceEditor(canvasElement).session.getAnnotations();
await expect(annotations.length).toBeGreaterThan(0);
await expect(annotations.some((a) => a.type === "error")).toBe(true);
},
// The worker is asynchronous and debounced, so this needs a real wait
// rather than a tick.
{ timeout: 5000 },
);
},
};

View file

@ -1,7 +1,7 @@
import { useState } from "react";
import { describe, it, expect, vi } from "vitest";
import { act, screen } from "@testing-library/react";
import type { JsonObject } from "@inspector/core/json/jsonUtils.js";
import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js";
import { renderWithMantine } from "../../../test/renderWithMantine";
import { aceLabel, getAceText, setAceText } from "../../../test/aceEditor";
import { JsonObjectInput } from "./JsonObjectInput";
@ -13,10 +13,10 @@ function Harness({
initial,
onChange,
}: {
initial: JsonObject;
onChange?: (v: JsonObject) => void;
initial: StrictJsonObject;
onChange?: (v: StrictJsonObject) => void;
}) {
const [value, setValue] = useState<JsonObject>(initial);
const [value, setValue] = useState<StrictJsonObject>(initial);
return (
<>
<JsonObjectInput
@ -109,7 +109,7 @@ describe("JsonObjectInput", () => {
// An external reset (switching servers, a reloaded catalog) must reach the
// editor; an in-progress edit must not be overwritten by what it emitted.
function Externally() {
const [value, setValue] = useState<JsonObject>({ a: 1 });
const [value, setValue] = useState<StrictJsonObject>({ a: 1 });
return (
<>
<JsonObjectInput
@ -132,7 +132,7 @@ describe("JsonObjectInput", () => {
// Regression: `echoed` used to hold only what this component last emitted,
// so returning to a previously-shown value matched the stale entry and was
// dismissed as our own echo, stranding the editor on B.
function Externally({ value }: { value: JsonObject }) {
function Externally({ value }: { value: StrictJsonObject }) {
return (
<JsonObjectInput ariaLabel={LABEL} value={value} onChange={() => {}} />
);
@ -208,6 +208,38 @@ describe("JsonObjectInput", () => {
);
});
it("announces a supplied description, and keeps it when the draft breaks", async () => {
function WithDescription() {
const [value, setValue] = useState<StrictJsonObject>({});
return (
<JsonObjectInput
ariaLabel={LABEL}
description="Sent with every request"
value={value}
onChange={setValue}
/>
);
}
renderWithMantine(<WithDescription />);
const described = () =>
(textarea().getAttribute("aria-describedby") ?? "")
.split(" ")
.filter(Boolean)
.map((id) => document.getElementById(id)?.textContent);
// Valid draft: described by the description alone.
expect(described()).toEqual(["Sent with every request"]);
// Invalid draft: the error joins it rather than replacing it — the
// description is still true, and dropping it would silently un-describe
// the control at the moment it needs explaining most.
await setAceText("[1]");
expect(described()).toHaveLength(2);
expect(described()[0]).toBe("Sent with every request");
expect(described()[1]).toMatch(/Must be a JSON object/);
});
it("clears the invalid marking once the text parses again", async () => {
renderWithMantine(<Harness initial={{}} />);
await setAceText("[1]");

View file

@ -10,7 +10,7 @@ import "ace-builds/src-noconflict/theme-github_dark";
// Without it Ace fetches `worker-json.js` from a path that does not exist in a
// bundled app, silently loses its annotations, and logs a fetch error.
import jsonWorkerUrl from "ace-builds/src-noconflict/worker-json.js?url";
import type { JsonObject } from "@inspector/core/json/jsonUtils.js";
import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js";
import { useValueChange } from "../../../hooks/useValueChange";
import { parseJsonObjectDraft } from "../../../utils/jsonObjectDraft";
@ -19,13 +19,13 @@ import { parseJsonObjectDraft } from "../../../utils/jsonObjectDraft";
ace.config.setModuleUrl("ace/mode/json_worker", jsonWorkerUrl);
/** Two-space canonical form — the same shape the catalog file is written in. */
function serialize(value: JsonObject): string {
function serialize(value: StrictJsonObject): string {
return JSON.stringify(value, null, 2);
}
export interface JsonObjectInputProps {
value: JsonObject;
onChange: (value: JsonObject) => void;
value: StrictJsonObject;
onChange: (value: StrictJsonObject) => void;
label?: string;
description?: string;
/**
@ -167,14 +167,29 @@ export function JsonObjectInput({
synchronously on mount, so this effect always sees an element. */
if (!element) return;
element.id = wrapperId;
// Both the description and the error are rendered by the wrapper and both
// describe this control, so both ids belong here — a supplied description
// is otherwise never announced, and it must not be dropped just because the
// draft went invalid. Mantine renders each only when its prop is set, so
// referencing an absent one would point at nothing.
const describedBy = [
description === undefined ? null : `${wrapperId}-description`,
parsed.ok ? null : `${wrapperId}-error`,
].filter((id) => id !== null);
if (describedBy.length > 0) {
element.setAttribute("aria-describedby", describedBy.join(" "));
} else {
element.removeAttribute("aria-describedby");
}
if (parsed.ok) {
element.removeAttribute("aria-invalid");
element.removeAttribute("aria-describedby");
} else {
element.setAttribute("aria-invalid", "true");
element.setAttribute("aria-describedby", `${wrapperId}-error`);
}
}, [wrapperId, parsed.ok]);
}, [wrapperId, parsed.ok, description]);
return (
<Input.Wrapper

View file

@ -1,15 +1,21 @@
import type { JsonObject } from "@inspector/core/json/jsonUtils.js";
import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js";
/**
* What a JSON-object editor's draft text currently means: the object to emit,
* or why it cannot be emitted.
*/
export type JsonObjectDraft =
| { ok: true; value: JsonObject }
| { ok: true; value: StrictJsonObject }
| { ok: false; error: string };
/** A plain JSON object, i.e. not an array and not a scalar. */
export function isJsonObject(value: unknown): value is JsonObject {
/**
* A plain JSON object, i.e. not an array and not a scalar.
*
* The `StrictJsonObject` narrowing is sound because the only caller parses with
* `JSON.parse`, whose output cannot contain `undefined` — the value that
* separates the strict type from `JsonValue`.
*/
export function isJsonObject(value: unknown): value is StrictJsonObject {
return value !== null && typeof value === "object" && !Array.isArray(value);
}