From 0152d9dc4206f2fb7e74aeef9c30de77ad047b17 Mon Sep 17 00:00:00 2001 From: Nik <5053591+treiber@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:07:55 +0100 Subject: [PATCH] fix(web): Escape while recording a shortcut no longer closes Settings --- .../KeybindingsSettings.browser.test.tsx | 94 +++++++++++++++++++ .../settings/KeybindingsSettings.tsx | 27 +++++- 2 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 apps/web/src/components/settings/KeybindingsSettings.browser.test.tsx diff --git a/apps/web/src/components/settings/KeybindingsSettings.browser.test.tsx b/apps/web/src/components/settings/KeybindingsSettings.browser.test.tsx new file mode 100644 index 0000000000000..e73e36631e61a --- /dev/null +++ b/apps/web/src/components/settings/KeybindingsSettings.browser.test.tsx @@ -0,0 +1,94 @@ +import { describe, expect, it, vi } from "vite-plus/test"; +import { page, userEvent, type Locator } from "vite-plus/test/browser"; + +import { render } from "../../../test/render"; + +vi.mock(import("../../state/session"), async (original) => ({ + ...(await original()), + useEnvironmentScope: () => true, + useEnvironmentsWithScope: ((environments: ReadonlyArray<{ environmentId: string }>) => + new Set(environments.map(({ environmentId }) => environmentId))) as never, + readEnvironmentScope: () => true, +})); +vi.mock(import("../../state/use-atom-command"), async (original) => ({ + ...(await original()), + useAtomCommand: (() => vi.fn()) as never, +})); +vi.mock(import("./SettingsScopeContext"), async (original) => ({ + ...(await original()), + useSettingsScope: (() => { + const environment = { environmentId: "env", serverConfig: { keybindings: [] } }; + return { environment, connectedEnvironments: [environment] }; + }) as never, +})); + +import { Dialog, DialogPopup, DialogTitle } from "../ui/dialog"; +import { KeybindingsSettingsPanel } from "./KeybindingsSettings"; + +// The modal dialog's inert layer swallows pointer events in this harness, so drive the DOM. +async function activate(locator: Locator, action: "click" | "focus") { + await expect.element(locator).toBeVisible(); + (locator.element() as HTMLElement)[action](); +} + +const existingField = page.getByRole("textbox", { name: "Keybinding for Sidebar: Toggle" }); +const existingPill = page.getByRole("button", { name: /^Edit shortcut for Sidebar: Toggle:/ }); +const newField = page.getByRole("textbox", { name: "Keybinding for new keybinding" }); + +describe("Esc in a shortcut recorder", () => { + it.each<{ + name: string; + field: Locator; + open: () => Promise; + expectReset: () => Promise; + }>([ + { + name: "existing binding", + field: existingField, + open: () => activate(existingPill, "click"), + expectReset: async () => { + await expect.element(existingField).not.toBeInTheDocument(); + await expect.element(existingPill).toHaveFocus(); + await expect.element(page.getByRole("button", { name: "Save" })).not.toBeInTheDocument(); + }, + }, + { + name: "new binding", + field: newField, + open: async () => { + await activate(page.getByRole("button", { name: "Add keybinding" }), "click"); + await activate(newField, "focus"); + }, + expectReset: async () => { + await expect.element(newField).toHaveValue(""); + await expect.element(newField).toHaveAttribute("placeholder", "Unassigned"); + }, + }, + ])("leaves the $name field without closing Settings", async ({ field, open, expectReset }) => { + const onOpenChange = vi.fn(); + await using _screen = await render( + + + Settings + + + , + ); + + await open(); + await userEvent.keyboard("{Control>}k{/Control}"); + await expect.element(field).not.toHaveValue(""); + await userEvent.tab(); + await activate(field, "focus"); + await expect.element(field).toHaveAttribute("placeholder", "Press shortcut"); + + await userEvent.keyboard("{Escape}"); + + expect(onOpenChange).not.toHaveBeenCalled(); + expect(document.activeElement?.matches("[data-keybinding-capture]")).toBe(false); + await expectReset(); + + await userEvent.keyboard("{Escape}"); + expect(onOpenChange).toHaveBeenCalledWith(false, expect.anything()); + }); +}); diff --git a/apps/web/src/components/settings/KeybindingsSettings.tsx b/apps/web/src/components/settings/KeybindingsSettings.tsx index b7b333900fd9d..70a2207ead819 100644 --- a/apps/web/src/components/settings/KeybindingsSettings.tsx +++ b/apps/web/src/components/settings/KeybindingsSettings.tsx @@ -20,6 +20,7 @@ import { useRef, useState, } from "react"; +import { flushSync } from "react-dom"; import { AuthOrchestrationOperateScope, AuthSettingsWriteScope, @@ -717,6 +718,7 @@ function useKeybindingRowEditor({ onSave: (input: ServerUpsertKeybindingInput) => void; }) { const [draft, setDraft] = useReducer(keybindingRowDraftReducer, row, createKeybindingRowDraft); + const pillRef = useRef(null); const { keyDraft, whenDraft, isRecording, isWhenDraftValid } = draft; const whenDraftExpression = whenAstToExpression(whenDraft); const isDirty = keyDraft !== row.key || whenDraftExpression !== row.when; @@ -739,7 +741,13 @@ function useKeybindingRowEditor({ if (event.key === "Tab" && !isRecording) return; event.preventDefault(); if (event.key === "Escape") { - setDraft({ keyDraft: row.key, isRecording: false }); + // An unhandled Escape closes the settings page. + event.stopPropagation(); + const field = event.currentTarget; + flushSync(() => setDraft({ keyDraft: row.key, isRecording: false })); + // The field may unmount for the pill; focus would otherwise drop to the page. + if (pillRef.current) pillRef.current.focus(); + else field.blur(); return; } const next = keybindingFromKeyboardEvent(event.nativeEvent, navigator.platform, { @@ -760,6 +768,7 @@ function useKeybindingRowEditor({ setDraft, save, captureKeybinding, + pillRef, }; } @@ -790,8 +799,16 @@ function KeybindingKeyControl({ isSaving: boolean; pillClassName?: string | undefined; }) { - const { keyDraft, isRecording, isDirty, isWhenDraftValid, setDraft, save, captureKeybinding } = - editor; + const { + keyDraft, + isRecording, + isDirty, + isWhenDraftValid, + setDraft, + save, + captureKeybinding, + pillRef, + } = editor; const showPill = !isRecording && keyDraft === row.key && row.key.length > 0 && !isDirty; return ( @@ -807,6 +824,7 @@ function KeybindingKeyControl({ ) : null} {showPill ? (