diff --git a/ui/src/e2e/device-token-reconnect.e2e.test.ts b/ui/src/e2e/device-token-reconnect.e2e.test.ts index 35b3b66e6eda..81cc25f1a96d 100644 --- a/ui/src/e2e/device-token-reconnect.e2e.test.ts +++ b/ui/src/e2e/device-token-reconnect.e2e.test.ts @@ -247,15 +247,13 @@ describeControlUiE2e("Control UI device-token reconnect E2E", () => { await revokeButton.waitFor({ state: "visible" }); await revokeButton.scrollIntoViewIfNeeded(); await captureProof(wilfredDevices.page, "wilfred-before-revoke.png"); - const dialogPromise = wilfredDevices.page.waitForEvent("dialog"); - await Promise.all([ - dialogPromise.then(async (dialog) => { - expect(dialog.type()).toBe("confirm"); - expect(dialog.message()).toBe(`Revoke token for ${deviceId} (operator)?`); - await dialog.accept(); - }), - revokeButton.click(), - ]); + await revokeButton.click(); + // Revoke confirms in-page, not through window.confirm: webviews without a dialog + // bridge silently answer false and would drop the action with no visible outcome. + const revokeConfirm = wilfredDevices.page.locator("openclaw-modal-dialog"); + await revokeConfirm.getByText("Revoke the operator token?").waitFor(); + await revokeConfirm.getByText(`Device ID: ${deviceId}`).waitFor(); + await revokeConfirm.getByRole("button", { name: "Revoke", exact: true }).click(); const revoke = await wilfredDevices.gateway.waitForRequest("device.token.revoke"); expect(revoke.params).toEqual({ deviceId, role: "operator" }); const wilfredStoreKey = diff --git a/ui/src/i18n/locales/en.ts b/ui/src/i18n/locales/en.ts index fd3f064a0206..38a491dc7599 100644 --- a/ui/src/i18n/locales/en.ts +++ b/ui/src/i18n/locales/en.ts @@ -575,6 +575,11 @@ export const en: TranslationMap = { removeStalePromptTitle: "Remove {count} stale pairings?", removeStalePromptTitleOne: "Remove 1 stale pairing?", removeStalePromptBody: "Affected clients re-pair silently on their next connection.", + rejectDevicePromptTitle: "Reject this device pairing request?", + rejectNodePromptTitle: "Reject this node pairing request?", + rejectPromptBody: "The client must send a new pairing request before it can connect.", + revokePromptTitle: "Revoke the {role} token?", + revokePromptBody: "This token stops working immediately and cannot be restored.", gateway: "gateway", unpaired: "unpaired", unknownClient: "unknown client", diff --git a/ui/src/lib/nodes/index.ts b/ui/src/lib/nodes/index.ts index d781d41caeb5..718ffab79cea 100644 --- a/ui/src/lib/nodes/index.ts +++ b/ui/src/lib/nodes/index.ts @@ -1,4 +1,7 @@ // Shared Nodes operations used by the Control UI page and Gateway event hooks. +// Presentation-free by contract: destructive confirmations belong to the owning page, +// because native window.confirm silently returns false in webviews with no dialog bridge +// and would end the action with no outcome and no recorded reason. import { getPublicKeyAsync, signAsync, utils } from "@noble/ed25519"; import { gatewayCredentialScope } from "@openclaw/gateway-client/browser"; import { @@ -291,10 +294,6 @@ export async function rejectDevicePairing(state: DevicesState, requestId: string if (!client || !state.connected) { return; } - const confirmed = window.confirm("Reject this device pairing request?"); - if (!confirmed) { - return; - } const generation = state.requestGeneration; try { await client.request("device.pair.reject", { requestId }); @@ -343,8 +342,6 @@ async function reloadInventory(state: InventoryState, opts?: { error?: string }) } } -// Confirmation for these removals lives in the page (in-page dialog): native -// window.confirm silently returns false in webviews without a dialog bridge. export async function removeInventoryEntry(state: InventoryState, entry: InventoryRemovalRequest) { const client = state.client; if (!client || !state.connected) { @@ -400,10 +397,6 @@ export async function rejectNodePairingRequest(state: InventoryState, requestId: if (!state.client || !state.connected) { return; } - const confirmed = window.confirm("Reject this node pairing request?"); - if (!confirmed) { - return; - } try { await state.client.request("node.pair.reject", { requestId }); await reloadInventory(state); @@ -467,10 +460,6 @@ export async function revokeDeviceToken( if (!client || !state.connected) { return; } - const confirmed = window.confirm(`Revoke token for ${params.deviceId} (${params.role})?`); - if (!confirmed) { - return; - } const generation = state.requestGeneration; try { const { gatewayUrl, ...requestParams } = params; diff --git a/ui/src/pages/devices/devices-page.test.ts b/ui/src/pages/devices/devices-page.test.ts index 2d4e55a873e6..c20640aa2764 100644 --- a/ui/src/pages/devices/devices-page.test.ts +++ b/ui/src/pages/devices/devices-page.test.ts @@ -1,20 +1,22 @@ /* @vitest-environment jsdom */ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { GatewayBrowserClient } from "../../api/gateway.ts"; import type { PresenceEntry } from "../../api/types.ts"; import type { ApplicationContext, ApplicationGatewaySnapshot } from "../../app/context.ts"; -import { showConfirmDialog } from "../../components/confirm-dialog.ts"; +import { t } from "../../i18n/index.ts"; import { createInitialDevicesState, loadNodes, type InventoryRemovalRequest, } from "../../lib/nodes/index.ts"; +import { + installDialogPolyfill, + waitForRenderedModalDialog, +} from "../../test-helpers/modal-dialog.ts"; import type { DevicesRouteData } from "./devices-page.ts"; import "./devices-page.ts"; -vi.mock("../../components/confirm-dialog.ts", () => ({ showConfirmDialog: vi.fn() })); - type TestDevicesPage = HTMLElement & { context: ApplicationContext; pageState: ReturnType; @@ -39,8 +41,30 @@ type TestDevicesPage = HTMLElement & { kind: "entry"; entry: InventoryRemovalRequest; }) => Promise; + confirmPairingReject: (target: "device" | "node", requestId: string) => Promise; + confirmTokenRevoke: (deviceId: string, role: string) => Promise; }; +function clickDialogButton(label: string) { + const button = [...document.body.querySelectorAll("button")].find( + (candidate) => candidate.textContent?.trim() === label, + ); + if (!(button instanceof HTMLButtonElement)) { + throw new Error(`Expected ${label} button`); + } + button.click(); +} + +function createConnectedPage(client: GatewayBrowserClient) { + const page = document.createElement("openclaw-devices-page") as TestDevicesPage; + page.context = { + gateway: { connection: { gatewayUrl: "http://gateway.test" } }, + runtimeConfig: { state: { configSnapshot: null, configLoading: false } }, + } as unknown as ApplicationContext; + applyGatewaySnapshot(page, gatewaySnapshot(client, true)); + return page; +} + function applyGatewaySnapshot( page: TestDevicesPage, snapshot: ApplicationGatewaySnapshot, @@ -94,6 +118,17 @@ function gateway(client: GatewayBrowserClient | null): ApplicationContext["gatew } describe("DevicesPage gateway lifecycle", () => { + let restoreDialogPolyfill: () => void; + + beforeEach(() => { + restoreDialogPolyfill = installDialogPolyfill(); + }); + + afterEach(() => { + document.body.replaceChildren(); + restoreDialogPolyfill(); + }); + it("preserves matching initial route data, then resets it on provider replacement", () => { const client = null; const currentGateway = gateway(client); @@ -250,25 +285,70 @@ describe("DevicesPage gateway lifecycle", () => { it("cancels a pending removal confirmation when the connection resets", async () => { const request = vi.fn(); const client = { request } as unknown as GatewayBrowserClient; - const confirmation = deferred(); - vi.mocked(showConfirmDialog).mockReturnValueOnce(confirmation.promise); - const page = document.createElement("openclaw-devices-page") as TestDevicesPage; - page.pageState = createInitialDevicesState({ client, connected: true }); - page.context = { - runtimeConfig: { state: { configSnapshot: null, configLoading: false } }, - } as unknown as ApplicationContext; + const page = createConnectedPage(client); + const pending = page.confirmInventoryRemoval({ kind: "entry", entry: { id: "device-1", name: "Browser", removeNode: false, removeDevice: true }, }); - await Promise.resolve(); - const signal = vi.mocked(showConfirmDialog).mock.calls[0]?.[0].signal; + await waitForRenderedModalDialog(document.body); applyGatewaySnapshot(page, gatewaySnapshot(client, false)); - confirmation.resolve(true); await pending; - expect(signal?.aborted).toBe(true); expect(request).not.toHaveBeenCalled(); + expect(document.body.querySelector("openclaw-modal-dialog")).toBeNull(); + }); + + it("rejects a device pairing request after the in-app dialog is confirmed", async () => { + const request = vi.fn().mockResolvedValue({}); + const client = { request } as unknown as GatewayBrowserClient; + const page = createConnectedPage(client); + + const pending = page.confirmPairingReject("device", "request-1"); + const { dialog } = await waitForRenderedModalDialog(document.body); + expect(dialog.getAttribute("aria-label")).toBe(t("devices.inventory.rejectDevicePromptTitle")); + + clickDialogButton(t("devices.inventory.reject")); + await pending; + + expect(request).toHaveBeenCalledWith("device.pair.reject", { requestId: "request-1" }); + applyGatewaySnapshot(page, gatewaySnapshot(client, false)); + }); + + it("issues no node pairing request when the dialog is cancelled", async () => { + const request = vi.fn(); + const client = { request } as unknown as GatewayBrowserClient; + const page = createConnectedPage(client); + + const pending = page.confirmPairingReject("node", "request-2"); + await waitForRenderedModalDialog(document.body); + + clickDialogButton(t("common.cancel")); + await pending; + + expect(request).not.toHaveBeenCalled(); + applyGatewaySnapshot(page, gatewaySnapshot(client, false)); + }); + + it("drops a confirmed token revoke when the request generation moved on", async () => { + const request = vi.fn(); + const client = { request } as unknown as GatewayBrowserClient; + const page = createConnectedPage(client); + + const pending = page.confirmTokenRevoke("device-1", "operator"); + const { dialog } = await waitForRenderedModalDialog(document.body); + expect(dialog.getAttribute("aria-label")).toBe( + t("devices.inventory.revokePromptTitle", { role: "operator" }), + ); + // The awaited dialog is a real suspension point: a generation bump during it means the + // captured scope no longer owns the connection, so the revoke must not reach the server. + page.pageState.requestGeneration += 1; + + clickDialogButton(t("devices.inventory.revoke")); + await pending; + + expect(request).not.toHaveBeenCalled(); + applyGatewaySnapshot(page, gatewaySnapshot(client, false)); }); }); diff --git a/ui/src/pages/devices/devices-page.ts b/ui/src/pages/devices/devices-page.ts index a7b45731123a..3d1a831fcf6b 100644 --- a/ui/src/pages/devices/devices-page.ts +++ b/ui/src/pages/devices/devices-page.ts @@ -10,7 +10,7 @@ import { type ApplicationGatewaySnapshot, } from "../../app/context.ts"; import { hasOperatorAdminAccess } from "../../app/operator-access.ts"; -import { showConfirmDialog } from "../../components/confirm-dialog.ts"; +import { showConfirmDialog, type ConfirmDialogOptions } from "../../components/confirm-dialog.ts"; import { renderDocsLink } from "../../components/settings-ui.ts"; import { renderSettingsWorkspace } from "../../components/settings-workspace.ts"; import { t } from "../../i18n/index.ts"; @@ -89,7 +89,7 @@ class DevicesPage extends OpenClawLightDomElement { @state() private canPairDevice = false; @state() private execApprovalsTarget: "gateway" | "node" = "gateway"; @state() private execApprovalsTargetNodeId: string | null = null; - private inventoryRemovalConfirmation: AbortController | null = null; + private pendingConfirmation: AbortController | null = null; private routeDataInitialized = false; private readonly gateway = new GatewayPageController(this, { @@ -181,7 +181,7 @@ class DevicesPage extends OpenClawLightDomElement { } override disconnectedCallback() { - this.cancelInventoryRemovalConfirmation(); + this.cancelPendingConfirmation(); this.subscriptions.clear(); void this.presenceTask.run([null, null]); this.presence = []; @@ -245,7 +245,7 @@ class DevicesPage extends OpenClawLightDomElement { } private resetServerState(snapshot: ApplicationGatewaySnapshot) { - this.cancelInventoryRemovalConfirmation(); + this.cancelPendingConfirmation(); this.pageState.requestGeneration += 1; const next = createInitialDevicesState({ client: snapshot.client, @@ -313,45 +313,32 @@ class DevicesPage extends OpenClawLightDomElement { return this.presenceTask.run([gateway, client]); } - private cancelInventoryRemovalConfirmation() { - this.inventoryRemovalConfirmation?.abort(); - this.inventoryRemovalConfirmation = null; + private cancelPendingConfirmation() { + this.pendingConfirmation?.abort(); + this.pendingConfirmation = null; } - private async confirmInventoryRemoval(prompt: InventoryRemovalPrompt) { - if (this.inventoryRemovalConfirmation) { + // Every destructive Devices action confirms here, never through window.confirm: the + // awaited dialog lets the gateway reconnect or swap clients mid-prompt, so the captured + // scope is revalidated before the operation runs against a different server. + private async confirmDestructiveAction( + prompt: Omit, + run: (pageState: DevicesPageDataState) => unknown, + ) { + if (this.pendingConfirmation) { return; } const controller = new AbortController(); - this.inventoryRemovalConfirmation = controller; + this.pendingConfirmation = controller; const generation = this.requestGeneration; const client = this.gateway.client; - const title = - prompt.kind === "entry" - ? t("devices.inventory.removePromptTitle", { name: prompt.entry.name }) - : t( - prompt.entries.length === 1 - ? "devices.inventory.removeStalePromptTitleOne" - : "devices.inventory.removeStalePromptTitle", - { count: String(prompt.entries.length) }, - ); const confirmed = await showConfirmDialog({ - title, - message: t( - prompt.kind === "entry" - ? "devices.inventory.removePromptBody" - : "devices.inventory.removeStalePromptBody", - ), - details: - prompt.kind === "entry" - ? t("devices.inventory.deviceId", { id: prompt.entry.id }) - : undefined, - confirmLabel: t("devices.inventory.remove"), + ...prompt, danger: true, signal: controller.signal, }); - if (this.inventoryRemovalConfirmation === controller) { - this.inventoryRemovalConfirmation = null; + if (this.pendingConfirmation === controller) { + this.pendingConfirmation = null; } if ( !confirmed || @@ -362,11 +349,71 @@ class DevicesPage extends OpenClawLightDomElement { ) { return; } + await this.runPageTask(run); + } + + private confirmInventoryRemoval(prompt: InventoryRemovalPrompt) { if (prompt.kind === "entry") { - void this.runPageTask((pageState) => removeInventoryEntry(pageState, prompt.entry)); - return; + const entry = prompt.entry; + return this.confirmDestructiveAction( + { + title: t("devices.inventory.removePromptTitle", { name: entry.name }), + message: t("devices.inventory.removePromptBody"), + details: t("devices.inventory.deviceId", { id: entry.id }), + confirmLabel: t("devices.inventory.remove"), + }, + (pageState) => removeInventoryEntry(pageState, entry), + ); } - void this.runPageTask((pageState) => removeStaleInventoryEntries(pageState, prompt.entries)); + const entries = prompt.entries; + return this.confirmDestructiveAction( + { + title: t( + entries.length === 1 + ? "devices.inventory.removeStalePromptTitleOne" + : "devices.inventory.removeStalePromptTitle", + { count: String(entries.length) }, + ), + message: t("devices.inventory.removeStalePromptBody"), + confirmLabel: t("devices.inventory.remove"), + }, + (pageState) => removeStaleInventoryEntries(pageState, entries), + ); + } + + private confirmPairingReject(target: "device" | "node", requestId: string) { + return this.confirmDestructiveAction( + { + title: t( + target === "device" + ? "devices.inventory.rejectDevicePromptTitle" + : "devices.inventory.rejectNodePromptTitle", + ), + message: t("devices.inventory.rejectPromptBody"), + confirmLabel: t("devices.inventory.reject"), + }, + (pageState) => + target === "device" + ? rejectDevicePairing(pageState, requestId) + : rejectNodePairingRequest(pageState, requestId), + ); + } + + private confirmTokenRevoke(deviceId: string, role: string) { + return this.confirmDestructiveAction( + { + title: t("devices.inventory.revokePromptTitle", { role }), + message: t("devices.inventory.revokePromptBody"), + details: t("devices.inventory.deviceId", { id: deviceId }), + confirmLabel: t("devices.inventory.revoke"), + }, + (pageState) => + revokeDeviceToken(pageState, { + deviceId, + gatewayUrl: this.context.gateway.connection.gatewayUrl, + role, + }), + ); } private resolveExecApprovalsTarget(): ExecApprovalsTarget { @@ -420,12 +467,10 @@ class DevicesPage extends OpenClawLightDomElement { onDevicePairSetupOpen: () => void this.context.overlays.openDevicePairSetup(), onDeviceApprove: (requestId) => void this.runPageTask((pageState) => approveDevicePairing(pageState, requestId)), - onDeviceReject: (requestId) => - void this.runPageTask((pageState) => rejectDevicePairing(pageState, requestId)), + onDeviceReject: (requestId) => void this.confirmPairingReject("device", requestId), onNodeApprove: (requestId) => void this.runPageTask((pageState) => approveNodePairingRequest(pageState, requestId)), - onNodeReject: (requestId) => - void this.runPageTask((pageState) => rejectNodePairingRequest(pageState, requestId)), + onNodeReject: (requestId) => void this.confirmPairingReject("node", requestId), onInventoryRemove: (entry) => void this.confirmInventoryRemoval({ kind: "entry", entry }), onInventoryCleanup: (entries) => { if (entries.length > 0) { @@ -441,14 +486,7 @@ class DevicesPage extends OpenClawLightDomElement { scopes, }), ), - onDeviceRevoke: (deviceId, role) => - void this.runPageTask((pageState) => - revokeDeviceToken(pageState, { - deviceId, - gatewayUrl: this.context.gateway.connection.gatewayUrl, - role, - }), - ), + onDeviceRevoke: (deviceId, role) => void this.confirmTokenRevoke(deviceId, role), onLoadConfig: () => void this.context.runtimeConfig.refresh({ discardPendingChanges: true }), onLoadExecApprovals: () =>