From 13a37b0cbfedda67591641869a8476def2ffcebb Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Mon, 26 Oct 2026 07:48:26 -0700 Subject: [PATCH] fix(web): an armed pull request confirm no longer outlives its button useInlineActionConfirm armed Merge/Close/Revert/Approve with document and window listeners owned by a mount-level effect in the hook's owner. When the armed button unmounted while its owner stayed (the primary action changed, a menu re-rendered without it), the listeners stayed attached for up to 3s and swallowed the next Escape (preventDefault + stopPropagation), so Escape did not close a dialog or menu. The listeners and the 3s timeout now live in a ref callback that is stable per arming and attaches to the armed element itself. Disarming, a scope change (reset during render), or the element unmounting detaches them and disarms. Also: - LinkPullRequestDialog uses the popup's initialFocus instead of a requestAnimationFrame focus effect. - PullRequestCommentBody observes its content's size from the content element's ref callback instead of a mount effect. Effects: useInlineActionConfirm 1 -> 0, LinkPullRequestDialog 1 -> 0, PullRequestCommentBody 1 -> 0. Co-Authored-By: Claude Opus 5.5 --- .../LinkPullRequestDialog.browser.test.tsx | 39 +++++++++ .../pullRequest/LinkPullRequestDialog.tsx | 9 +- .../pullRequest/PullRequestCommentBody.tsx | 24 +++--- .../useInlineActionConfirm.browser.test.tsx | 78 +++++++++++++++++ .../pullRequest/useInlineActionConfirm.ts | 85 +++++++++++-------- 5 files changed, 182 insertions(+), 53 deletions(-) create mode 100644 apps/web/src/components/pullRequest/LinkPullRequestDialog.browser.test.tsx create mode 100644 apps/web/src/components/pullRequest/useInlineActionConfirm.browser.test.tsx diff --git a/apps/web/src/components/pullRequest/LinkPullRequestDialog.browser.test.tsx b/apps/web/src/components/pullRequest/LinkPullRequestDialog.browser.test.tsx new file mode 100644 index 0000000000000..c1d7aa99fa903 --- /dev/null +++ b/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( + + + , + ); + await act(() => + openLinkPullRequestDialog({ + environmentId: EnvironmentId.make("environment"), + threadId: ThreadId.make("thread"), + }), + ); + + await expect.element(page.getByPlaceholder("Pull request URL or #42")).toHaveFocus(); +}); diff --git a/apps/web/src/components/pullRequest/LinkPullRequestDialog.tsx b/apps/web/src/components/pullRequest/LinkPullRequestDialog.tsx index 376da0f53b1ad..cecae8fbaf8ba 100644 --- a/apps/web/src/components/pullRequest/LinkPullRequestDialog.tsx +++ b/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 ( (pending ? undefined : onOpenChange(next))}> - + Link pull request diff --git a/apps/web/src/components/pullRequest/PullRequestCommentBody.tsx b/apps/web/src/components/pullRequest/PullRequestCommentBody.tsx index 73554b41ad91c..e598c80ce7963 100644 --- a/apps/web/src/components/pullRequest/PullRequestCommentBody.tsx +++ b/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) { const [expanded, setExpanded] = useState(false); const [overflowing, setOverflowing] = useState(false); - const content = useRef(null); + const clip = useRef(null); const id = useId(); - useEffect(() => { - const element = content.current; - if (!element) return; - return observeResize(element, ([entry]) => { - if (entry) setOverflowing(entry.contentRect.height > 240); - }); - }, []); - return (
setExpanded(true)} > -
+
{ + if (element === null) return; + return observeResize(element, ([entry]) => { + if (entry) setOverflowing(entry.contentRect.height > 240); + }); + }} + >
{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); }} > diff --git a/apps/web/src/components/pullRequest/useInlineActionConfirm.browser.test.tsx b/apps/web/src/components/pullRequest/useInlineActionConfirm.browser.test.tsx new file mode 100644 index 0000000000000..90a0cffd859b6 --- /dev/null +++ b/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 ( + + ); +} + +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(); + 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(); + const button = screen.getByRole("button"); + + await userEvent.click(button); + await expect.element(button).toHaveTextContent("Confirm merge?"); + await screen.rerender(); + await expect.element(button).toHaveTextContent("Merge"); + expect(pressEscape().defaultPrevented).toBe(false); + + await screen.rerender(); + 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 ? ( + + ) : ( + gone + ); +} + +it("stops swallowing Escape once the armed button unmounts", async () => { + await using screen = await render(); + const button = screen.getByRole("button"); + + await userEvent.click(button); + await expect.element(button).toHaveTextContent("Confirm merge?"); + await screen.rerender(); + await expect.element(screen.getByText("gone")).toBeVisible(); + expect(pressEscape().defaultPrevented).toBe(false); + + await screen.rerender(); + await expect.element(screen.getByRole("button")).toHaveTextContent("Merge"); +}); diff --git a/apps/web/src/components/pullRequest/useInlineActionConfirm.ts b/apps/web/src/components/pullRequest/useInlineActionConfirm.ts index 69224f7090110..3729e025544f0 100644 --- a/apps/web/src/components/pullRequest/useInlineActionConfirm.ts +++ b/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(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) => { 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),