fix: address the Copilot review of the Ace editor
- `isProgressToken` used `Number.isInteger`, but zod 4's `.int()` rejects anything past `MAX_SAFE_INTEGER` — verified against the pinned zod — so an unsafe integer passed the guard and was left for the server to reject. Now `isSafeInteger`, with the boundary covered in both directions. - `echoed` held only what this component last emitted, so it went stale the moment an external value was applied: A → B → A stranded the editor on B, because the second A matched the entry left from before B arrived and was dismissed as our own echo. It now advances to whatever is on screen. - `Input.Wrapper` associates a *Mantine* input through context, and Ace is not one — it renders its own DOM. The wrapper's label and error node therefore pointed at nothing: the editor was never marked `aria-invalid`, never described by the error text, and its visible label did not focus it. Wired explicitly through the wrapper's id in an effect. - Removed two leftover `[dbg]` logs an earlier regex cleanup missed. - Documented the editor integration and the shared `cssVariablesResolver` in the web README, per the rule about new dependencies and patterns. Both footguns are recorded: the `?url` worker import, and the gutter override keying off Ace's cssClass rather than the theme module name. 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:
parent
75f9c51146
commit
7f10fa971c
3 changed files with 178 additions and 51 deletions
|
|
@ -79,11 +79,6 @@ describe("JsonObjectInput", () => {
|
|||
const onChange = vi.fn();
|
||||
renderWithMantine(<Harness initial={{ keep: "me" }} onChange={onChange} />);
|
||||
await setAceText('{"a":');
|
||||
console.log("[dbg] emitted:", screen.getByTestId("emitted").textContent);
|
||||
console.log(
|
||||
"[dbg] body has error?",
|
||||
document.body.textContent?.includes("Not valid JSON"),
|
||||
);
|
||||
// Displayed verbatim — not re-escaped back through JSON.stringify.
|
||||
expect(getAceText()).toBe('{"a":');
|
||||
expect(
|
||||
|
|
@ -133,6 +128,76 @@ describe("JsonObjectInput", () => {
|
|||
expect(JSON.parse(getAceText())).toEqual({ b: 2 });
|
||||
});
|
||||
|
||||
it("applies an external value it showed before (A → B → A)", async () => {
|
||||
// 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 }) {
|
||||
return (
|
||||
<JsonObjectInput ariaLabel={LABEL} value={value} onChange={() => {}} />
|
||||
);
|
||||
}
|
||||
const { rerender } = renderWithMantine(<Externally value={{ a: 1 }} />);
|
||||
expect(JSON.parse(getAceText())).toEqual({ a: 1 });
|
||||
|
||||
rerender(<Externally value={{ b: 2 }} />);
|
||||
expect(JSON.parse(getAceText())).toEqual({ b: 2 });
|
||||
|
||||
// Back to the first value — a distinct object, same content.
|
||||
rerender(<Externally value={{ a: 1 }} />);
|
||||
expect(JSON.parse(getAceText())).toEqual({ a: 1 });
|
||||
});
|
||||
|
||||
describe("wiring to the Input.Wrapper label and error", () => {
|
||||
/** Ace's hidden textarea — the element the wrapper has to point at. */
|
||||
function textarea(): HTMLTextAreaElement {
|
||||
const el = document.querySelector<HTMLTextAreaElement>(
|
||||
"textarea.ace_text-input",
|
||||
);
|
||||
if (!el) throw new Error("Ace text input not mounted");
|
||||
return el;
|
||||
}
|
||||
|
||||
it("lets the visible label focus the editor", () => {
|
||||
renderWithMantine(
|
||||
<JsonObjectInput
|
||||
label="Request metadata"
|
||||
ariaLabel={LABEL}
|
||||
value={{}}
|
||||
onChange={() => {}}
|
||||
/>,
|
||||
);
|
||||
// `Input.Wrapper` renders `<label for={id}>`; the editor is only
|
||||
// reachable from it if Ace's textarea carries that same id.
|
||||
const label = screen.getByText("Request metadata");
|
||||
expect(label.getAttribute("for")).toBe(textarea().id);
|
||||
expect(textarea().id).not.toBe("");
|
||||
});
|
||||
|
||||
it("marks the editor invalid and points it at the error text", async () => {
|
||||
renderWithMantine(<Harness initial={{}} />);
|
||||
expect(textarea().getAttribute("aria-invalid")).toBeNull();
|
||||
|
||||
await setAceText("[1]");
|
||||
expect(textarea().getAttribute("aria-invalid")).toBe("true");
|
||||
const describedBy = textarea().getAttribute("aria-describedby");
|
||||
expect(describedBy).not.toBeNull();
|
||||
// The id must resolve to the message actually on screen.
|
||||
expect(document.getElementById(describedBy!)?.textContent).toMatch(
|
||||
/Must be a JSON object/,
|
||||
);
|
||||
});
|
||||
|
||||
it("clears the invalid marking once the text parses again", async () => {
|
||||
renderWithMantine(<Harness initial={{}} />);
|
||||
await setAceText("[1]");
|
||||
expect(textarea().getAttribute("aria-invalid")).toBe("true");
|
||||
await setAceText('{"a":1}');
|
||||
expect(textarea().getAttribute("aria-invalid")).toBeNull();
|
||||
expect(textarea().getAttribute("aria-describedby")).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
it("leaves an in-progress edit alone when the parent echoes it back", async () => {
|
||||
renderWithMantine(<Harness initial={{}} />);
|
||||
// The parent re-renders with the object this text parsed to; the draft must
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import { useRef, useState } from "react";
|
||||
import { useEffect, useId, useRef, useState } from "react";
|
||||
import { Input, useComputedColorScheme } from "@mantine/core";
|
||||
import type { Ace } from "ace-builds";
|
||||
import AceEditor from "react-ace";
|
||||
import ace from "ace-builds/src-noconflict/ace";
|
||||
import "ace-builds/src-noconflict/mode-json";
|
||||
|
|
@ -137,13 +138,47 @@ export function JsonObjectInput({
|
|||
});
|
||||
};
|
||||
|
||||
const wrapperId = useId();
|
||||
const editorRef = useRef<Ace.Editor | null>(null);
|
||||
|
||||
const parsed = parseJsonObjectDraft(draft);
|
||||
useValueChange(serialize(value), (next) => {
|
||||
if (next !== echoed) setDraft(next);
|
||||
if (next === echoed) return;
|
||||
setDraft(next);
|
||||
// `echoed` has to advance to the value now on screen, not just to what this
|
||||
// component last emitted. Leaving it behind makes an external A → B → A
|
||||
// sequence stick on B: the second A matches the stale `echoed` from before
|
||||
// B arrived and is dismissed as our own echo.
|
||||
setEchoed(next);
|
||||
});
|
||||
|
||||
// `Input.Wrapper` labels and describes a Mantine input through context. Ace
|
||||
// is not one — it renders its own DOM — so the wrapper's label and error node
|
||||
// exist but point at nothing, leaving the control unassociated with the error
|
||||
// a screen reader needs and its visible label inert on click.
|
||||
//
|
||||
// A genuine external-system sync, so an effect rather than `useValueChange`:
|
||||
// it writes to a DOM node React does not own. Giving the textarea the
|
||||
// wrapper's own id is what makes the `<label for>` focus the editor; the
|
||||
// error wiring follows Mantine's `${id}-error` naming.
|
||||
useEffect(() => {
|
||||
const element = editorRef.current?.textInput.getElement();
|
||||
/* v8 ignore next -- the ref is set in `onLoad`, which react-ace calls
|
||||
synchronously on mount, so this effect always sees an element. */
|
||||
if (!element) return;
|
||||
element.id = wrapperId;
|
||||
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]);
|
||||
|
||||
return (
|
||||
<Input.Wrapper
|
||||
id={wrapperId}
|
||||
label={label}
|
||||
description={description}
|
||||
error={parsed.ok ? undefined : parsed.error}
|
||||
|
|
@ -162,6 +197,7 @@ export function JsonObjectInput({
|
|||
showPrintMargin={false}
|
||||
editorProps={{ $blockScrolling: Infinity }}
|
||||
onLoad={(editor) => {
|
||||
editorRef.current = editor;
|
||||
// `textInputAriaLabel` alone is not enough: Ace composes the hidden
|
||||
// textarea's label as "<label>, Cursor at row N" inside
|
||||
// `TextInput.setAriaLabel`, and only recomputes it when the cursor
|
||||
|
|
|
|||
|
|
@ -1419,6 +1419,10 @@ describe("InspectorClient", () => {
|
|||
["an array", [1]],
|
||||
["null", null],
|
||||
["a boolean", true],
|
||||
// Past `Number.MAX_SAFE_INTEGER` the float64 value is no longer the
|
||||
// integer it was written as, and zod 4's `.int()` rejects it — so
|
||||
// `Number.isInteger` is too loose a check here.
|
||||
["an unsafe integer", Number.MAX_SAFE_INTEGER + 1],
|
||||
])(
|
||||
"drops a reserved progressToken that is %s rather than sending it",
|
||||
async (_label, badToken) => {
|
||||
|
|
@ -1511,6 +1515,8 @@ describe("InspectorClient", () => {
|
|||
it.each([
|
||||
["a string", "tok-1"],
|
||||
["an integer", 7],
|
||||
// The boundary itself is still valid.
|
||||
["the largest safe integer", Number.MAX_SAFE_INTEGER],
|
||||
])("keeps a valid progressToken that is %s", async (_label, token) => {
|
||||
client = new InspectorClient(
|
||||
{
|
||||
|
|
|
|||
Loading…
Reference in a new issue