R8a: Escape scoping and focus-trap hardening (findings #16, #17)

web_ui/src/app/canvas/CodeExecutionApprovalPanel.test.tsx

@@ -234,4 +234,53 @@ describe("CodeExecutionApprovalPanel", () => {
     expect(screen.getByRole("button", { name: "Approve" })).toBeEnabled();
     expect(screen.getByRole("button", { name: "Deny" })).toBeEnabled();
   });
+
+  // -- R8a finding #17: focus trap must exclude disabled buttons ------------
+
+  it("R8a finding #17: while busy, Tab from the code display does not leak focus out of the panel", () => {
+    // Before this fix, FOCUSABLE included disabled buttons: with busy=true
+    // both Deny and Approve are disabled, so "last" was the disabled
+    // Approve button. Tab from the code display (first, not last) never
+    // matched the wrap condition, preventDefault() never fired, and the
+    // browser's own native Tab order - which correctly skips disabled
+    // buttons - walked straight out of the panel (its only other two
+    // stops both being disabled) onto whatever's next in the document.
+    // This is the one thing this panel exists to make impossible during a
+    // mandatory security approval.
+    renderPanel({ busy: true });
+    const codeDisplay = document.querySelector(".code-exec-approval-code") as HTMLElement;
+    const dialog = screen.getByRole("dialog");
+    codeDisplay.focus();
+    expect(document.activeElement).toBe(codeDisplay);
+
+    fireEvent.keyDown(dialog, { key: "Tab" });
+
+    expect(dialog.contains(document.activeElement)).toBe(true);
+  });
+
+  it("R8a finding #17: a focus escape past the trap is pulled back into the panel (backstop)", () => {
+    // Simulates a leak the primary trap missed by some cause other than the
+    // now-fixed disabled-button one. This panel shares overlays.tsx's own
+    // Tab-gated useFocusEscapeBackstop (see that hook's doc comment) rather
+    // than a hand-written unconditional listener - an earlier draft of this
+    // fix used an unconditional one and infinite-looped the instant two
+    // panels were open at once (see the "FIX B" test below), each treating
+    // the other's redirect as its own escape. The gating means the backstop
+    // only acts right after a real Tab keydown - fireEvent.keyDown(document,
+    // ...), not user.keyboard, for the same reason as overlays.test.tsx's
+    // twin of this test: dispatching directly at document arms the flag via
+    // this hook's capture-phase listener without also reaching the panel's
+    // own bubble-phase trap, isolating "Tab pressed but the primary trap
+    // missed it" from "Tab was handled correctly."
+    renderPanel({ busy: false });
+    const outside = document.createElement("button");
+    document.body.appendChild(outside);
+    try {
+      fireEvent.keyDown(document, { key: "Tab" });
+      outside.focus();
+      expect(screen.getByRole("dialog").contains(document.activeElement)).toBe(true);
+    } finally {
+      outside.remove();
+    }
+  });
 });

web_ui/src/app/canvas/CodeExecutionApprovalPanel.tsx

@@ -3,6 +3,7 @@ import { createPortal } from "react-dom";
 import ReactMarkdown from "react-markdown";
 import remarkGfm from "remark-gfm";
 import rehypeHighlight from "rehype-highlight";
+import { useFocusEscapeBackstop } from "../overlays/overlays";
 
 /**
  * The mandatory human-approval gate (Qt-removal plan R5.4, corrected in the
@@ -139,7 +140,16 @@ function toPythonFence(code: string): string {
 // Deny, Approve) - deliberately narrower than overlays.tsx's own shared
 // FOCUSABLE selector (which also matches inputs/selects/links/textareas),
 // since nothing else in this panel's markup is ever meant to receive focus.
-const FOCUSABLE = 'button, [tabindex]:not([tabindex="-1"])';
+//
+// R8a (UI/UX issue list finding #17): excludes [disabled], the same fix
+// overlays.tsx's own Dialog needed and for the identical reason - `busy`
+// (below) disables BOTH Deny and Approve simultaneously while a click is
+// in flight, and an undisabled trap boundary computed against a disabled
+// "last" button never actually receives Tab natively, so the wrap-around
+// branch below silently never fires and focus walks out of a panel whose
+// entire reason to exist is that focus must NEVER leave it during a
+// mandatory code-execution approval.
+const FOCUSABLE = 'button:not([disabled]), [tabindex]:not([tabindex="-1"])';
 
 export interface CodeExecutionApprovalPanelProps {
   /** No longer used to register with any shared overlay registry (there is
@@ -219,6 +229,18 @@ export function CodeExecutionApprovalPanel({
     return () => panel.removeEventListener("keydown", onKeyDown);
   }, [awaitingApproval]);
 
+  // R8a (finding #17): shares overlays.tsx's own backstop rather than a
+  // second hand-written copy - see that hook's own doc comment for why an
+  // unconditional version (which THIS file tried first) is actually wrong
+  // here specifically: two of these panels can be open at once for
+  // different nodes (FIX B, this file's own module doc), and an
+  // unconditional "redirect whenever focus lands outside, full stop"
+  // listener on each one infinite-loops the instant either redirects,
+  // since each treats the other's redirect as its own escape and redirects
+  // back. The shared hook's Tab-gating avoids this generally, not just for
+  // Dialog's own close()-restores-focus race.
+  useFocusEscapeBackstop(panelRef, awaitingApproval, FOCUSABLE);
+
   if (!awaitingApproval) return null;
 
   const showRequirements = kind === "code_sandbox" && !!requirements?.trim();

web_ui/src/app/canvas/GroupNodeView.test.tsx

@@ -82,7 +82,11 @@ describe("GroupNodeView (frame)", () => {
     fireEvent.doubleClick(container.querySelector(".group-node-header")!);
     const input = screen.getByRole("textbox") as HTMLInputElement;
     fireEvent.change(input, { target: { value: "Throwaway" } });
-    fireEvent.keyDown(input, { key: "Escape" });
+    // R8a finding #16: must claim the event (preventDefault -> dispatchEvent
+    // returns false) or overlays.tsx's document-level Escape handler would
+    // also close whatever dialog/popover is open elsewhere - see
+    // NoteNodeView's identical guard and overlays.tsx's own comment.
+    expect(fireEvent.keyDown(input, { key: "Escape" })).toBe(false);
 
     expect(onSetLabel).not.toHaveBeenCalled();
     expect(screen.queryByRole("textbox")).toBeNull();

web_ui/src/app/canvas/GroupNodeView.tsx

@@ -78,6 +78,11 @@ export function GroupNodeView({ id, data, selected }: NodeProps<GroupFlowNode>)
 
   function onKeyDown(event: React.KeyboardEvent<HTMLInputElement>) {
     if (event.key === "Escape") {
+      // R8a (UI/UX issue list finding #16): claims Escape so overlays.tsx's
+      // own document-level handler doesn't also close an unrelated open
+      // popover behind this frame/container - see NoteNodeView's identical
+      // guard for the full reasoning.
+      event.preventDefault();
       skipBlurRef.current = true;
       setDraft(data.label);
       setEditing(false);

web_ui/src/app/canvas/NoteNodeView.test.tsx

@@ -67,7 +67,13 @@ describe("NoteNodeView", () => {
 
     const textarea = screen.getByRole("textbox") as HTMLTextAreaElement;
     fireEvent.change(textarea, { target: { value: "throwaway edit" } });
-    fireEvent.keyDown(textarea, { key: "Escape" });
+    // fireEvent's own return value is the DOM dispatchEvent() result - false
+    // means something called preventDefault(). R8a finding #16: this MUST
+    // be false, or overlays.tsx's document-level Escape handler (which now
+    // checks event.defaultPrevented before closing anything) would also
+    // close whatever dialog/popover happens to be open elsewhere the
+    // instant a note revert fires - see overlays.tsx's own comment.
+    expect(fireEvent.keyDown(textarea, { key: "Escape" })).toBe(false);
 
     expect(onSetContent).not.toHaveBeenCalled();
     expect(screen.queryByRole("textbox")).toBeNull();

web_ui/src/app/canvas/NoteNodeView.tsx

@@ -68,6 +68,13 @@ export function NoteNodeView({ data, selected }: NodeProps<NoteFlowNode>) {
 
   function onKeyDown(event: React.KeyboardEvent<HTMLTextAreaElement>) {
     if (event.key === "Escape") {
+      // R8a (UI/UX issue list finding #16): claims Escape so the overlay
+      // system's own document-level handler (overlays.tsx) - which now
+      // checks event.defaultPrevented before closing anything - doesn't
+      // also close an unrelated open popover behind this note. Without
+      // this, Escape while editing a note with e.g. Pins open would revert
+      // the note AND close Pins in the same keystroke.
+      event.preventDefault();
       skipBlurRef.current = true;
       setDraft(data.content);
       setEditing(false);

web_ui/src/app/chrome/ChatLibraryDialog.test.tsx

@@ -214,15 +214,35 @@ describe("ChatLibraryDialog", () => {
     expect(screen.getByText("First Chat")).toBeInTheDocument();
   });
 
-  it("Escape while renaming closes the whole dialog (the app's global Escape-to-close policy) without committing a rename", async () => {
+  it("Escape while renaming cancels the rename WITHOUT closing the dialog (R8a finding #16)", async () => {
+    // This test used to assert the opposite - Escape closing the whole
+    // dialog mid-rename, mislabeled as "the app's global Escape-to-close
+    // policy" - which was exactly the bug the UI/UX issue list's finding
+    // #16 named this component as its own worked example of: the rename
+    // input's own onKeyDown already called event.preventDefault() on
+    // Escape (see onRenameKeyDown below), but overlays.tsx's document-level
+    // handler ran in capture phase with an unconditional stopPropagation(),
+    // so that preventDefault() could never matter - the global handler
+    // always won and closed the dialog out from under an in-progress
+    // rename. overlays.tsx now runs on bubble phase and checks
+    // event.defaultPrevented before closing anything, so this component's
+    // own already-correct handler finally has a chance to run.
     const { user, intents } = setup();
     await user.click(screen.getByText("open library"));
 
     await user.click(screen.getByLabelText('Rename "First Chat"'));
     await user.keyboard("{Escape}");
 
     expect(intents.filter((i) => i[1] === "renameChat")).toEqual([]);
-    expect(screen.queryByRole("dialog")).toBeNull();
+    // Reverts exactly like the Cancel (x) button does, not "the dialog is
+    // now gone" - the rename INPUT is gone (cancelled) and the original
+    // title is back, but Chat Library stays open underneath it. Checked by
+    // role, not just the accessible name: the pencil button that RESTARTS
+    // a rename carries the identical "Rename ..." label, so a label-only
+    // query would find that instead and pass even if cancelling had failed.
+    expect(screen.queryByRole("textbox", { name: 'Rename "First Chat"' })).toBeNull();
+    expect(screen.getByText("First Chat")).toBeInTheDocument();
+    expect(screen.getByRole("dialog")).toBeInTheDocument();
   });
 
   it("deleting a row: trash opens an inline scoped confirm, confirming dispatches deleteChat for THAT row only", async () => {

web_ui/src/app/chrome/PinOverlay.test.tsx

@@ -82,6 +82,45 @@ describe("PinOverlay", () => {
     expect(updatePin).toHaveBeenCalledWith("p1", "Renamed", "n");
   });
 
+  it("R8a finding #16: Escape while editing a pin cancels the edit WITHOUT closing the whole Pins popover", async () => {
+    // Neither the title input nor the note textarea had ANY onKeyDown
+    // before this fix - Escape fell through untouched to overlays.tsx's
+    // own document-level handler, which closed the entire Popover (the
+    // same bug class already fixed for Chat Library's rename input, just
+    // missed here since this file had no existing "Escape" handler to
+    // find via a grep for one - there was nothing to find, only something
+    // to add).
+    const user = userEvent.setup();
+    const { store, updatePin } = makeStore([{ id: "p1", title: "Origin", note: "n", x: 0, y: 0 }]);
+    renderOpen(store);
+    await user.click(screen.getByLabelText("Edit Origin"));
+    await user.clear(screen.getByLabelText("Pin title"));
+    await user.type(screen.getByLabelText("Pin title"), "Throwaway");
+
+    await user.keyboard("{Escape}");
+
+    expect(updatePin).not.toHaveBeenCalled();
+    // The edit reverted (back to the read-only row, original title intact)...
+    expect(screen.queryByLabelText("Pin title")).toBeNull();
+    expect(screen.getByText("Origin")).toBeInTheDocument();
+    // ...and the popover itself is still open, unlike before this fix.
+    expect(screen.getByText("PINS")).toBeInTheDocument();
+  });
+
+  it("R8a finding #16: Escape in the note textarea also cancels the edit without closing the popover", async () => {
+    const user = userEvent.setup();
+    const { store, updatePin } = makeStore([{ id: "p1", title: "Origin", note: "n", x: 0, y: 0 }]);
+    renderOpen(store);
+    await user.click(screen.getByLabelText("Edit Origin"));
+
+    await user.click(screen.getByLabelText("Pin note"));
+    await user.keyboard("{Escape}");
+
+    expect(updatePin).not.toHaveBeenCalled();
+    expect(screen.queryByLabelText("Pin note")).toBeNull();
+    expect(screen.getByText("PINS")).toBeInTheDocument();
+  });
+
   it("remove calls the intent", async () => {
     const user = userEvent.setup();
     const { store, removePin } = makeStore([{ id: "p1", title: "Origin", note: "", x: 0, y: 0 }]);

web_ui/src/app/chrome/PinOverlay.tsx

@@ -58,6 +58,22 @@ export function PinOverlay({ store }: { store: SceneStore }) {
     setDraftError(null);
   }
 
+  // R8a (UI/UX issue list finding #16): neither the title input nor the
+  // note textarea below had ANY onKeyDown, so Escape while editing a pin
+  // fell straight through to overlays.tsx's own document-level handler,
+  // which closed the whole Pins popover - discarding the entire panel,
+  // not just cancelling the one pin's edit. Same bug class already fixed
+  // for Chat Library's rename input and the Note/Frame inline label
+  // editors, missed here originally since this file has no `key ===
+  // "Escape"` literal to grep for at all - there was no handler to find,
+  // only one to add.
+  function onEditKeyDown(event: React.KeyboardEvent) {
+    if (event.key === "Escape") {
+      event.preventDefault();
+      cancelEditing();
+    }
+  }
+
   function saveEditing() {
     const trimmedTitle = draftTitle.trim();
     if (!trimmedTitle) {
@@ -111,13 +127,15 @@ export function PinOverlay({ store }: { store: SceneStore }) {
                   className="pins-edit-title"
                   value={draftTitle}
                   onChange={(e) => setDraftTitle(e.target.value)}
+                  onKeyDown={onEditKeyDown}
                   aria-label="Pin title"
                   autoFocus
                 />
                 <textarea
                   className="pins-edit-note"
                   value={draftNote}
                   onChange={(e) => setDraftNote(e.target.value)}
+                  onKeyDown={onEditKeyDown}
                   aria-label="Pin note"
                   rows={2}
                 />