From b96d8757764e335bb550eaddbe16bc1a4bb49c0e Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:51:42 +0100 Subject: [PATCH] fix: make the account picker and the admin OAuth and plugin code dialogs proper modals --- app/(main)/admin/_pages/auth.tsx | 163 +++++++++--------- .../admin/_pages/plugin-approvals-panel.tsx | 37 ++-- .../protocol-account-picker.test.tsx | 53 ++++++ .../protocol/protocol-account-picker.tsx | 25 ++- 4 files changed, 174 insertions(+), 104 deletions(-) create mode 100644 components/protocol/__tests__/protocol-account-picker.test.tsx diff --git a/app/(main)/admin/_pages/auth.tsx b/app/(main)/admin/_pages/auth.tsx index 04afb2e38..02538dcb6 100644 --- a/app/(main)/admin/_pages/auth.tsx +++ b/app/(main)/admin/_pages/auth.tsx @@ -8,6 +8,7 @@ import { useAdminUnsavedChanges } from '@/stores/admin-unsaved-store'; import { DEFAULT_OAUTH_SCOPES } from '@/lib/oauth/scopes'; import { SettingsSection, SettingItem, SectionNav } from '@/components/settings/settings-section'; import { Button } from '@/components/ui/button'; +import { Dialog } from '@/components/ui/dialog'; import { AdminPageHeader } from '@/components/admin/page-header'; import { AdminSaveBar } from '@/components/admin/save-bar'; import { withEdit } from '@/lib/admin/edits'; @@ -177,92 +178,86 @@ export function AuthPage() {
- - {setupOpen && ( -
{ if (e.target === e.currentTarget && !setupRunning) setSetupOpen(false); }} - > -
-
-

Auto-configure OAuth

-

- Verify the URLs below before continuing. The webmail and Stalwart can live on different domains. -

-
-
-
- - setSetupOrigin(e.target.value)} - disabled={setupRunning} - placeholder="https://webmail.example.com" - className={cn(inputClass, 'w-full')} - /> -

- Used to register redirect URIs (one per locale: {setupOrigin.trim().replace(/\/+$/, '') || 'https://…'}/<locale>/auth/callback) on Stalwart. -

- {!setupOriginValid && setupOrigin.length > 0 && ( -

Must be like https://host with no path.

- )} -
-
- - setSetupIssuer(e.target.value)} - disabled={setupRunning} - placeholder="https://mail.example.com" - className={cn(inputClass, 'w-full')} - /> -

- Where Stalwart serves /.well-known/oauth-authorization-server. Saved as OAUTH_ISSUER_URL. Pre-filled from your JMAP server URL. -

- {!setupIssuerValid && setupIssuer.length > 0 && ( -

Must be like https://host with no path.

- )} -
- -
-
- - -
+ {/* The shared dialog: focus moved in and given back, Tab kept inside, + Escape and an outside click close it unless setup is running. */} + { if (!setupRunning) setSetupOpen(false); }} + preventOutsideClose={setupRunning} + showClose={!setupRunning} + title="Auto-configure OAuth" + description="Verify the URLs below before continuing. The webmail and Stalwart can live on different domains." + footer={ + <> + + + + } + > +
+
+ + setSetupOrigin(e.target.value)} + disabled={setupRunning} + placeholder="https://webmail.example.com" + className={cn(inputClass, 'w-full')} + /> +

+ Used to register redirect URIs (one per locale: {setupOrigin.trim().replace(/\/+$/, '') || 'https://…'}/<locale>/auth/callback) on Stalwart. +

+ {!setupOriginValid && setupOrigin.length > 0 && ( +

Must be like https://host with no path.

+ )}
+
+ + setSetupIssuer(e.target.value)} + disabled={setupRunning} + placeholder="https://mail.example.com" + className={cn(inputClass, 'w-full')} + /> +

+ Where Stalwart serves /.well-known/oauth-authorization-server. Saved as OAUTH_ISSUER_URL. Pre-filled from your JMAP server URL. +

+ {!setupIssuerValid && setupIssuer.length > 0 && ( +

Must be like https://host with no path.

+ )} +
+
- )} +
diff --git a/app/(main)/admin/_pages/plugin-approvals-panel.tsx b/app/(main)/admin/_pages/plugin-approvals-panel.tsx index 0baedaf67..ea923ed46 100644 --- a/app/(main)/admin/_pages/plugin-approvals-panel.tsx +++ b/app/(main)/admin/_pages/plugin-approvals-panel.tsx @@ -6,6 +6,7 @@ import { apiFetch } from '@/lib/browser-navigation'; import { HIGH_RISK_PERMISSIONS } from '@/lib/plugin-sandbox/permission-text'; import { SettingsSection } from '@/components/settings/settings-section'; import { Button } from '@/components/ui/button'; +import { Dialog } from '@/components/ui/dialog'; import { useAdminShell } from '@/components/admin/shell-context'; import { Badge, type BadgeTone } from '@/components/admin/badge'; @@ -186,24 +187,26 @@ export function PluginApprovalsPanel() {
)} - {source && ( -
{ if (e.target === e.currentTarget) setSource(null); }}> -
-
- {source.entry.manifest.name ?? source.entry.pluginId} v{source.entry.manifest.version} - -
- {source.warnings && source.warnings.length > 0 && ( -
- {source.warnings.join('; ')} -
- )} - 
-              {source.error ?? source.code ?? ( Loading)}
-
+ {/* The shared dialog: focus moved in and given back, Tab kept inside, + Escape closes, and the title names the plugin. */} + setSource(null)} + title={source ? `Code of ${source.entry.manifest.name ?? source.entry.pluginId} v${source.entry.manifest.version}` : ''} + size="lg" + className="max-w-4xl" + bodyClassName="flex flex-col" + > + {source?.warnings && source.warnings.length > 0 && ( +
+
-
- )} + )} + {/* Focusable, so the keyboard can scroll the code. */} + 
+          {source?.error ?? source?.code ?? ()}
+
+ ); } diff --git a/components/protocol/__tests__/protocol-account-picker.test.tsx b/components/protocol/__tests__/protocol-account-picker.test.tsx new file mode 100644 index 000000000..81cfe8610 --- /dev/null +++ b/components/protocol/__tests__/protocol-account-picker.test.tsx @@ -0,0 +1,53 @@ +import { fireEvent, render, screen } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { ProtocolAccountPicker } from '../protocol-account-picker'; +import type { AccountEntry } from '@/stores/account-store'; + +// The "which account?" question for a mailto: or webcal: link is a modal: +// the focus starts on the account in use, Escape cancels, and the focus +// comes back to where it was. + +const account = (id: string, email: string) => + ({ id, email, label: email, username: email, serverUrl: 'https://mail.example.org', displayName: '' }) as unknown as AccountEntry; + +let opener: HTMLButtonElement; +beforeEach(() => { + opener = document.createElement('button'); + document.body.appendChild(opener); + opener.focus(); +}); +afterEach(() => opener.remove()); + +describe('ProtocolAccountPicker', () => { + it('focuses the active account, cancels on Escape and gives the focus back', () => { + const onCancel = vi.fn(); + const { unmount } = render( + {}} + onCancel={onCancel} + />, + ); + const dialog = screen.getByRole('dialog', { name: 'select_account_title' }); + const active = screen.getByText('bob@example.org', { selector: 'span' }).closest('button')!; + expect(active).toHaveFocus(); + expect(active).toHaveAttribute('aria-current', 'true'); + + fireEvent.keyDown(active, { key: 'Escape' }); + expect(onCancel).toHaveBeenCalledTimes(1); + expect(dialog).toBeInTheDocument(); + unmount(); + expect(opener).toHaveFocus(); + }); + + it('ignores Escape while it switches accounts', () => { + const onCancel = vi.fn(); + render( + {}} onCancel={onCancel} />, + ); + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' }); + expect(onCancel).not.toHaveBeenCalled(); + }); +}); diff --git a/components/protocol/protocol-account-picker.tsx b/components/protocol/protocol-account-picker.tsx index b30159652..891e092bf 100644 --- a/components/protocol/protocol-account-picker.tsx +++ b/components/protocol/protocol-account-picker.tsx @@ -7,6 +7,8 @@ import type { ParsedWebcal } from "@/lib/protocol-handlers/webcal"; import type { AccountEntry } from "@/stores/account-store"; import { cn } from "@/lib/utils"; import { Avatar } from "@/components/ui/avatar"; +import { useId } from "react"; +import { useFocusTrap } from "@/hooks/use-focus-trap"; type ProtocolAccountPickerProps = { accounts: AccountEntry[]; @@ -50,18 +52,32 @@ export function ProtocolAccountPicker({ ] : []; + // A modal: the focus starts on the account in use (else the first one), + // Tab stays inside, Escape cancels unless a switch is under way, and the + // focus goes back to where it was. + const titleId = useId(); + const panelRef = useFocusTrap({ + isActive: true, + onEscape: () => { if (!isSwitching) onCancel(); }, + restoreFocus: true, + initialFocus: () => + panelRef.current?.querySelector('[data-active-account="true"]') + ?? panelRef.current?.querySelector('[data-account-option]'), + }); + return (