fix(web): an armed pull request confirm no longer outlives its button

apps/web/src/components/pullRequest/LinkPullRequestDialog.browser.test.tsx

@@ -0,0 +1,39 @@
+import { EnvironmentId, ThreadId } from "@t3tools/contracts";
+import { expect, it, vi } from "vite-plus/test";
+import { page } from "vite-plus/test/browser";
+
+vi.mock(import("~/hooks/usePullRequestLinking"), async (original) => ({
+  ...(await original()),
+  usePullRequestLinking: (() => ({
+    mode: "multiple",
+    canLink: () => true,
+    changeLink: vi.fn(),
+  })) as never,
+}));
+vi.mock(import("~/state/entities"), async (original) => ({
+  ...(await original()),
+  useProjects: () => [],
+  useThreadShell: () => null,
+}));
+
+import { RegistryContext } from "@effect/atom-react";
+
+import { appAtomRegistry } from "~/rpc/atomRegistry";
+import { act, render } from "../../../test/render";
+import { LinkPullRequestDialogHost, openLinkPullRequestDialog } from "./LinkPullRequestDialog";
+
+it("focuses the reference input when the dialog opens", async () => {
+  await using _screen = await render(
+    <RegistryContext.Provider value={appAtomRegistry}>
+      <LinkPullRequestDialogHost />
+    </RegistryContext.Provider>,
+  );
+  await act(() =>
+    openLinkPullRequestDialog({
+      environmentId: EnvironmentId.make("environment"),
+      threadId: ThreadId.make("thread"),
+    }),
+  );
+
+  await expect.element(page.getByPlaceholder("Pull request URL or #42")).toHaveFocus();
+});

apps/web/src/components/pullRequest/LinkPullRequestDialog.tsx

@@ -5,7 +5,7 @@ import {
 } from "@t3tools/client-runtime/pull-request-link-input";
 import { useAtomValue } from "@effect/atom-react";
 import { settlePromise, squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime";
-import { useEffect, useRef, useState } from "react";
+import { useRef, useState } from "react";
 
 import { useProjects, useThreadShell } from "~/state/entities";
 import { usePullRequestLinking } from "~/hooks/usePullRequestLinking";
@@ -85,11 +85,6 @@ function LinkPullRequestDialog({
       setSubmitError(null);
     }
   }
-  useEffect(() => {
-    if (!open) return;
-    const frame = window.requestAnimationFrame(() => inputRef.current?.focus());
-    return () => window.cancelAnimationFrame(frame);
-  }, [open]);
 
   const resolved = resolveLinkPullRequestInput({
     reference,
@@ -126,7 +121,7 @@ function LinkPullRequestDialog({
 
   return (
     <Dialog open={open} onOpenChange={(next) => (pending ? undefined : onOpenChange(next))}>
-      <DialogPopup className="max-w-xl">
+      <DialogPopup className="max-w-xl" initialFocus={inputRef}>
         <DialogHeader>
           <DialogTitle>Link pull request</DialogTitle>
           <DialogDescription>

apps/web/src/components/pullRequest/PullRequestCommentBody.tsx

@@ -1,4 +1,4 @@
-import { useEffect, useId, useRef, useState, type ComponentProps } from "react";
+import { useId, useRef, useState, type ComponentProps } from "react";
 
 import { cn } from "~/lib/utils";
 import { Button } from "../ui/button";
@@ -11,25 +11,25 @@ export function PullRequestCommentBody({
 }: ComponentProps<typeof PullRequestMarkdown>) {
   const [expanded, setExpanded] = useState(false);
   const [overflowing, setOverflowing] = useState(false);
-  const content = useRef<HTMLDivElement>(null);
+  const clip = useRef<HTMLDivElement>(null);
   const id = useId();
 
-  useEffect(() => {
-    const element = content.current;
-    if (!element) return;
-    return observeResize(element, ([entry]) => {
-      if (entry) setOverflowing(entry.contentRect.height > 240);
-    });
-  }, []);
-
   return (
     <div className={cn("min-w-0", className)}>
       <div
+        ref={clip}
         id={id}
         className={cn("relative", !expanded && "max-h-[240px] overflow-hidden")}
         onFocusCapture={() => setExpanded(true)}
       >
-        <div ref={content}>
+        <div
+          ref={(element) => {
+            if (element === null) return;
+            return observeResize(element, ([entry]) => {
+              if (entry) setOverflowing(entry.contentRect.height > 240);
+            });
+          }}
+        >
           <PullRequestMarkdown {...props} />
         </div>
         {overflowing && !expanded ? (
@@ -47,7 +47,7 @@ export function PullRequestCommentBody({
           aria-expanded={expanded}
           aria-controls={id}
           onClick={() => {
-            if (expanded) content.current?.parentElement?.scrollIntoView({ block: "nearest" });
+            if (expanded) clip.current?.scrollIntoView({ block: "nearest" });
             setExpanded(!expanded);
           }}
         >

apps/web/src/components/pullRequest/useInlineActionConfirm.browser.test.tsx

@@ -0,0 +1,78 @@
+import { render } from "../../../test/render";
+import { expect, it, vi } from "vite-plus/test";
+import { userEvent } from "vite-plus/test/browser";
+
+import { useInlineActionConfirm } from "./useInlineActionConfirm";
+
+function MergeButton({ scopeKey, onConfirm }: { scopeKey: string; onConfirm: () => void }) {
+  const confirm = useInlineActionConfirm(scopeKey);
+  return (
+    <button type="button" {...confirm.props("merge", onConfirm)}>
+      {confirm.isArmed("merge") ? "Confirm merge?" : "Merge"}
+    </button>
+  );
+}
+
+function pressEscape(): KeyboardEvent {
+  const event = new KeyboardEvent("keydown", { key: "Escape", bubbles: true, cancelable: true });
+  window.dispatchEvent(event);
+  return event;
+}
+
+it("confirms on the second click and disarms on Escape", async () => {
+  const onConfirm = vi.fn();
+  await using screen = await render(<MergeButton scopeKey="a" onConfirm={onConfirm} />);
+  const button = screen.getByRole("button");
+
+  await userEvent.click(button);
+  await expect.element(button).toHaveTextContent("Confirm merge?");
+  expect(pressEscape().defaultPrevented).toBe(true);
+  await expect.element(button).toHaveTextContent("Merge");
+
+  await userEvent.click(button);
+  await userEvent.click(button);
+  expect(onConfirm).toHaveBeenCalledTimes(1);
+  await expect.element(button).toHaveTextContent("Merge");
+});
+
+it("stops swallowing Escape once the scope changes", async () => {
+  const onConfirm = vi.fn();
+  await using screen = await render(<MergeButton scopeKey="a" onConfirm={onConfirm} />);
+  const button = screen.getByRole("button");
+
+  await userEvent.click(button);
+  await expect.element(button).toHaveTextContent("Confirm merge?");
+  await screen.rerender(<MergeButton scopeKey="b" onConfirm={onConfirm} />);
+  await expect.element(button).toHaveTextContent("Merge");
+  expect(pressEscape().defaultPrevented).toBe(false);
+
+  await screen.rerender(<MergeButton scopeKey="a" onConfirm={onConfirm} />);
+  await expect.element(button).toHaveTextContent("Merge");
+});
+
+// The hook's owner stays mounted while the armed button goes away, as when the primary
+// action changes under an armed merge button.
+function ToolbarWithMergeButton({ shown }: { shown: boolean }) {
+  const confirm = useInlineActionConfirm("a");
+  return shown ? (
+    <button type="button" {...confirm.props("merge", () => undefined)}>
+      {confirm.isArmed("merge") ? "Confirm merge?" : "Merge"}
+    </button>
+  ) : (
+    <span>gone</span>
+  );
+}
+
+it("stops swallowing Escape once the armed button unmounts", async () => {
+  await using screen = await render(<ToolbarWithMergeButton shown />);
+  const button = screen.getByRole("button");
+
+  await userEvent.click(button);
+  await expect.element(button).toHaveTextContent("Confirm merge?");
+  await screen.rerender(<ToolbarWithMergeButton shown={false} />);
+  await expect.element(screen.getByText("gone")).toBeVisible();
+  expect(pressEscape().defaultPrevented).toBe(false);
+
+  await screen.rerender(<ToolbarWithMergeButton shown />);
+  await expect.element(screen.getByRole("button")).toHaveTextContent("Merge");
+});

apps/web/src/components/pullRequest/useInlineActionConfirm.ts

@@ -1,51 +1,68 @@
-import { useEffect, useState, type MouseEvent } from "react";
+import { useState, type MouseEvent } from "react";
+
+interface ArmedAction {
+  readonly scopeKey: string;
+  readonly action: string;
+  readonly element: HTMLElement;
+  // Stable per arming, so React attaches it once. Detaching it (disarm, scope change, or the
+  // armed element unmounting) removes the listeners and disarms.
+  readonly ref: (node: HTMLElement | null) => (() => void) | undefined;
+}
 
 export function useInlineActionConfirm(scopeKey: string) {
-  const [armed, setArmed] = useState<{
-    scopeKey: string;
-    action: string;
-    element: HTMLElement;
-  } | null>(null);
+  const [armed, setArmed] = useState<ArmedAction | null>(null);
+  if (armed !== null && armed.scopeKey !== scopeKey) setArmed(null);
   const active = armed?.scopeKey === scopeKey ? armed : null;
 
-  useEffect(() => {
-    if (!armed) return;
-    const disarm = () => setArmed(null);
-    if (armed.scopeKey !== scopeKey) {
-      disarm();
-      return;
-    }
-    const onPointerDown = (event: PointerEvent) => {
-      if (event.target instanceof Node && !armed.element.contains(event.target)) disarm();
-    };
-    const onKeyDown = (event: KeyboardEvent) => {
-      if (event.key === "Escape") {
-        event.preventDefault();
-        event.stopPropagation();
-        disarm();
-      }
-    };
-    const timeout = window.setTimeout(disarm, 3000);
-    document.addEventListener("pointerdown", onPointerDown, true);
-    window.addEventListener("keydown", onKeyDown, true);
-    window.addEventListener("blur", disarm);
-    return () => {
-      window.clearTimeout(timeout);
-      document.removeEventListener("pointerdown", onPointerDown, true);
-      window.removeEventListener("keydown", onKeyDown, true);
-      window.removeEventListener("blur", disarm);
+  const arm = (action: string, element: HTMLElement) => {
+    const entry: ArmedAction = {
+      scopeKey,
+      action,
+      element,
+      ref: (node) => {
+        if (node === null) return undefined;
+        const controller = new AbortController();
+        const { signal } = controller;
+        const release = () => setArmed((current) => (current === entry ? null : current));
+        document.addEventListener(
+          "pointerdown",
+          (event) => {
+            if (event.target instanceof Node && !node.contains(event.target)) release();
+          },
+          { capture: true, signal },
+        );
+        window.addEventListener(
+          "keydown",
+          (event) => {
+            if (event.key !== "Escape") return;
+            event.preventDefault();
+            event.stopPropagation();
+            release();
+          },
+          { capture: true, signal },
+        );
+        window.addEventListener("blur", release, { signal });
+        const timeout = window.setTimeout(release, 3000);
+        return () => {
+          controller.abort();
+          window.clearTimeout(timeout);
+          release();
+        };
+      },
     };
-  }, [armed, scopeKey]);
+    setArmed(entry);
+  };
 
   return {
     isArmed: (action: string) => active?.action === action,
     props: (action: string, onConfirm: () => void) => ({
+      ref: active?.action === action ? active.ref : undefined,
       onClick: (event: MouseEvent<HTMLElement>) => {
         if (active?.action === action && active.element === event.currentTarget) {
           setArmed(null);
           onConfirm();
         } else {
-          setArmed({ scopeKey, action, element: event.currentTarget });
+          arm(action, event.currentTarget);
         }
       },
       onBlur: () => setArmed(null),