[backport core/1.47] fix(dialog): keep Settings open when nested dialogs shift focus + BYOK secrets E2E (#13621)

Backport of #13510 to `core/1.47`

Automatically created by backport workflow.

Co-authored-by: Matt Miller <matt@miller-media.com>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: 4 people

browser_tests/tests/cloudSecrets.spec.ts

@@ -0,0 +1,279 @@
+import { expect } from '@playwright/test'
+import type { Page, Route } from '@playwright/test'
+
+import type { RemoteConfig } from '@/platform/remoteConfig/types'
+
+import { comfyPageFixture as test } from '@e2e/fixtures/ComfyPage'
+import { bootCloud, mockCloudBoot } from '@e2e/fixtures/utils/cloudBootMocks'
+import { jsonRoute } from '@e2e/fixtures/utils/jsonRoute'
+
+/**
+ * End-to-end coverage for the user-secrets (API keys) surface in the cloud app:
+ * add a provider key, see it listed, delete it — the full CRUD round-trip —
+ * plus the entitlement contract that a non-entitled account never sees the
+ * gated providers.
+ *
+ * Drives a raw `page` against fully-mocked endpoints (the `comfyPage` fixture
+ * would reach the OSS devtools backend during setup); `mockCloudBoot` +
+ * `bootCloud` boot the app signed-in, and this spec layers a stateful in-memory
+ * `/secrets` backend on top so the flow is deterministic and never touches a
+ * real server.
+ */
+const APP_URL = process.env.PLAYWRIGHT_TEST_URL || 'http://localhost:8188'
+
+// `/api/features` is the remote-config source. Enabling user secrets is what
+// surfaces the Secrets settings panel for a signed-in user.
+const BOOT_FEATURES = {
+  user_secrets_enabled: true
+} satisfies RemoteConfig
+
+// TutorialCompleted suppresses the new-user template browser, whose modal
+// overlay (z-1700) would otherwise intercept clicks on the settings dialog.
+const BOOT_SETTINGS = { 'Comfy.TutorialCompleted': true }
+
+// The plaintext key a user types in. It must be sent on create but NEVER echoed
+// back by the API or rendered anywhere in the UI.
+const RUNWAY_KEY_VALUE = 'sk-runway-do-not-echo-0xDEADBEEF'
+
+interface SecretRecord {
+  id: string
+  name: string
+  provider?: string
+  created_at: string
+  updated_at: string
+  last_used_at?: string
+}
+
+interface CreateCapture {
+  name?: string
+  provider?: string
+  secret_value?: string
+}
+
+interface SecretsBackend {
+  /** Bodies received by POST /secrets, in order — for asserting what was sent. */
+  createRequests: CreateCapture[]
+  /** Current server-side store — for asserting delete actually removed a row. */
+  store: SecretRecord[]
+}
+
+/**
+ * Stateful mock of the ingest `/secrets` surface. A single route handler
+ * branches on path + method so registration order can never make a specific
+ * path (`/secrets/providers`, `/secrets/:id`) lose to the collection glob.
+ *
+ * `providerIds` models entitlement: an entitled account sees runway/gemini,
+ * a non-entitled account gets an empty list (the server omits them).
+ */
+async function mockSecretsBackend(
+  page: Page,
+  providerIds: string[]
+): Promise<SecretsBackend> {
+  const backend: SecretsBackend = { createRequests: [], store: [] }
+  let idSeq = 0
+
+  const respondList = (route: Route) =>
+    route.fulfill(jsonRoute({ data: backend.store }))
+
+  await page.route('**/api/secrets**', async (route) => {
+    const request = route.request()
+    const { pathname } = new URL(request.url())
+    const method = request.method()
+
+    // The glob `**/api/secrets**` also matches the panel's own lazy-loaded
+    // source module (`/src/platform/secrets/api/secretsApi.ts`), whose path
+    // contains the `/api/secrets` substring. Fulfilling that dev-server module
+    // request with JSON breaks the dynamic import and the panel never mounts.
+    // Anchor to the start of the pathname so only genuine `/api/secrets…` API
+    // routes are handled; everything else falls through to the real Vite server.
+    if (!/^\/api\/secrets(\/|$)/.test(pathname)) {
+      return route.continue()
+    }
+
+    // GET /secrets/providers — the entitlement-gated provider allowlist.
+    if (pathname.endsWith('/secrets/providers')) {
+      return route.fulfill(
+        jsonRoute({ data: providerIds.map((id) => ({ id })) })
+      )
+    }
+
+    // /secrets/:id — item routes (only DELETE is exercised by this flow).
+    const itemMatch = pathname.match(/\/secrets\/([^/]+)$/)
+    if (itemMatch) {
+      const id = itemMatch[1]
+      if (method === 'DELETE') {
+        backend.store = backend.store.filter((s) => s.id !== id)
+        return route.fulfill({ status: 204, body: '' })
+      }
+      return respondList(route)
+    }
+
+    // /secrets — collection routes.
+    if (method === 'POST') {
+      const body = (request.postDataJSON() ?? {}) as CreateCapture
+      backend.createRequests.push(body)
+      idSeq += 1
+      const created: SecretRecord = {
+        id: `00000000-0000-4000-8000-${String(idSeq).padStart(12, '0')}`,
+        name: body.name ?? '',
+        provider: body.provider,
+        created_at: '2026-07-08T00:00:00Z',
+        updated_at: '2026-07-08T00:00:00Z'
+      }
+      backend.store.push(created)
+      // Response echoes metadata ONLY — the schema has no secret_value field.
+      return route.fulfill(jsonRoute(created))
+    }
+
+    // GET /secrets (list).
+    return respondList(route)
+  })
+
+  return backend
+}
+
+/**
+ * Open the settings dialog and land on the Secrets panel, waiting for both the
+ * provider allowlist and the secret list to resolve so subsequent assertions
+ * are not racing the panel's on-mount fetches.
+ */
+async function openSecretsPanel(page: Page) {
+  const settingsDialog = page.getByTestId('settings-dialog')
+
+  await page.evaluate(() => {
+    const app = window.app
+    if (!app) throw new Error('window.app is not available')
+    return app.extensionManager.command.execute('Comfy.ShowSettingsDialog')
+  })
+  await settingsDialog.waitFor({ state: 'visible' })
+
+  const providersResolved = page.waitForResponse((r) =>
+    r.url().includes('/api/secrets/providers')
+  )
+  const listResolved = page.waitForResponse(
+    (r) =>
+      /\/api\/secrets(\?|$)/.test(r.url()) && r.request().method() === 'GET'
+  )
+
+  await settingsDialog
+    .locator('nav')
+    .getByRole('button', { name: 'Secrets' })
+    .click()
+
+  await Promise.all([providersResolved, listResolved])
+  return settingsDialog
+}
+
+test.describe('Cloud user secrets (API keys)', { tag: '@cloud' }, () => {
+  test('an entitled account can add, list, and delete a provider key', async ({
+    page
+  }) => {
+    test.slow()
+
+    await mockCloudBoot(page, {
+      features: BOOT_FEATURES,
+      settings: BOOT_SETTINGS
+    })
+    await bootCloud(page)
+    const backend = await mockSecretsBackend(page, ['runway', 'gemini'])
+
+    await page.goto(APP_URL)
+    await page.waitForFunction(() => !!window.app?.extensionManager, null, {
+      timeout: 45_000
+    })
+
+    const settingsDialog = await openSecretsPanel(page)
+
+    // Empty state before anything is added.
+    await expect(settingsDialog.getByText(/No secrets stored/)).toBeVisible()
+
+    // --- ADD -------------------------------------------------------------
+    await settingsDialog.getByRole('button', { name: 'Add Secret' }).click()
+
+    const formDialog = page
+      .getByRole('dialog')
+      .filter({ hasText: 'Secret Value' })
+    await expect(formDialog).toBeVisible()
+
+    // Pick the entitled Runway provider from the server-driven dropdown.
+    await formDialog.locator('#secret-provider').click()
+    await page.getByRole('option', { name: 'Runway' }).click()
+
+    await formDialog.locator('#secret-name').fill('My Runway Key')
+    await formDialog.locator('input[type="password"]').fill(RUNWAY_KEY_VALUE)
+
+    await formDialog.getByRole('button', { name: 'Save', exact: true }).click()
+    await expect(formDialog).toBeHidden()
+
+    // --- LIST ------------------------------------------------------------
+    await expect(settingsDialog.getByText('My Runway Key')).toBeVisible()
+    await expect(settingsDialog.getByText(/No secrets stored/)).toBeHidden()
+
+    // The create request carried the plaintext value + provider...
+    expect(backend.createRequests).toHaveLength(1)
+    expect(backend.createRequests[0]).toMatchObject({
+      name: 'My Runway Key',
+      provider: 'runway',
+      secret_value: RUNWAY_KEY_VALUE
+    })
+    // ...but the value must never be echoed back into the list — the API
+    // response carries metadata only, so nothing should render it as text.
+    await expect(page.getByText(RUNWAY_KEY_VALUE)).toHaveCount(0)
+
+    // --- DELETE ----------------------------------------------------------
+    await settingsDialog
+      .getByRole('button', { name: 'Delete', exact: true })
+      .click()
+
+    const confirmDialog = page
+      .getByRole('dialog')
+      .filter({ hasText: 'Delete Secret' })
+    await confirmDialog
+      .getByRole('button', { name: 'Delete', exact: true })
+      .click()
+
+    await expect(settingsDialog.getByText('My Runway Key')).toBeHidden()
+    await expect(settingsDialog.getByText(/No secrets stored/)).toBeVisible()
+    expect(backend.store).toHaveLength(0)
+  })
+
+  test('a non-entitled account never sees the gated providers', async ({
+    page
+  }) => {
+    test.slow()
+
+    await mockCloudBoot(page, {
+      features: BOOT_FEATURES,
+      settings: BOOT_SETTINGS
+    })
+    await bootCloud(page)
+    // Non-entitled: the server omits runway/gemini from the allowlist.
+    await mockSecretsBackend(page, [])
+
+    await page.goto(APP_URL)
+    await page.waitForFunction(() => !!window.app?.extensionManager, null, {
+      timeout: 45_000
+    })
+
+    const settingsDialog = await openSecretsPanel(page)
+    await expect(settingsDialog.getByText(/No secrets stored/)).toBeVisible()
+
+    // The add form opens, but its provider dropdown is empty — the gated
+    // providers must not appear anywhere.
+    await settingsDialog.getByRole('button', { name: 'Add Secret' }).click()
+    const formDialog = page
+      .getByRole('dialog')
+      .filter({ hasText: 'Secret Value' })
+    await expect(formDialog).toBeVisible()
+
+    await formDialog.locator('#secret-provider').click()
+    // Anchor on the opened listbox so the absence assertions below can't pass
+    // vacuously against a dropdown that never opened.
+    const providerListbox = page.getByRole('listbox')
+    await expect(providerListbox).toBeVisible()
+    // An empty allowlist must yield an empty dropdown. Asserting zero options
+    // (not just runway/gemini absent) also rejects the fetch-failure fallback,
+    // where `availableProviders` is null and the default providers would show.
+    await expect(providerListbox.getByRole('option')).toHaveCount(0)
+  })
+})

src/components/dialog/GlobalDialog.test.ts

@@ -449,6 +449,12 @@ describe('shouldPreventRekaDismiss', () => {
     expect(event.defaultPrevented).toBe(false)
   })
 
+  it('focus-outside never dismisses when dismissOnFocusOutside is false', () => {
+    const event = makeEvent(document.body)
+    onRekaFocusOutside(event, { dismissOnFocusOutside: false })
+    expect(event.defaultPrevented).toBe(true)
+  })
+
   it('focus-outside on a sibling Reka portal does not dismiss the parent', () => {
     const portal = document.createElement('div')
     portal.setAttribute('role', 'dialog')

src/components/dialog/GlobalDialog.vue

@@ -32,7 +32,9 @@
                 dialogStore.activeKey === item.key
               )
           "
-          @focus-outside="onRekaFocusOutside"
+          @focus-outside="
+            (e) => onRekaFocusOutside(e, item.dialogComponentProps)
+          "
           @mousedown="() => dialogStore.riseDialog({ key: item.key })"
         >
           <template v-if="item.dialogComponentProps.headless">

src/components/dialog/rekaPrimeVueBridge.ts

@@ -53,7 +53,22 @@ export function onRekaPointerDownOutside(
 // nested Reka or PrimeVue dialog teleported to body). Without this guard a
 // non-modal Reka dialog would dismiss itself the moment a nested dialog
 // receives focus.
-export function onRekaFocusOutside(event: OutsideEvent) {
+//
+// A container dialog (e.g. Settings) that hosts nested confirm/edit dialogs can
+// also lose focus to an ordinary app element — not just a portal — when a
+// nested dialog closes and the element it focused was removed (deleting the
+// selected row). That programmatic focus shift is not a dismiss intent, so such
+// a dialog opts out of focus-outside dismissal entirely via
+// `dismissOnFocusOutside: false`; it still dismisses on escape or an outside
+// pointer.
+export function onRekaFocusOutside(
+  event: OutsideEvent,
+  options: { dismissOnFocusOutside?: boolean } = {}
+) {
+  if (options.dismissOnFocusOutside === false) {
+    event.preventDefault()
+    return
+  }
   if (isInsideOverlay(event.detail.originalEvent.target)) {
     event.preventDefault()
   }

src/platform/secrets/components/SecretFormDialog.vue

@@ -41,7 +41,7 @@
                     <img
                       v-if="option.logo"
                       :src="option.logo"
-                      :alt="option.label"
+                      alt=""
                       class="size-4"
                     />
                     {{ option.label }}

src/platform/secrets/composables/useSecretForm.ts

@@ -101,8 +101,11 @@ export function useSecretForm(options: UseSecretFormOptions) {
 
   // Once the server allowlist resolves, drop a selection the resolved list no
   // longer offers so the user cannot submit an unlisted provider.
-  watch(providerOptions, (options) => {
-    if (form.provider && !options.some((o) => o.value === form.provider)) {
+  watch(providerOptions, (resolvedOptions) => {
+    if (
+      form.provider &&
+      !resolvedOptions.some((o) => o.value === form.provider)
+    ) {
       form.provider = null
     }
   })

src/platform/settings/composables/useSettingsDialog.ts

@@ -39,6 +39,11 @@ export function useSettingsDialog() {
         // breaks those nested dialogs' autofocus and click handling. Non-modal
         // keeps the visual overlay without those traps.
         modal: false,
+        // A nested dialog closing (e.g. confirming a Secrets delete) can move
+        // focus onto an app element once the row it focused is removed. As a
+        // non-modal dialog Settings would treat that as an outside focus and
+        // dismiss itself, so opt out — escape and outside clicks still close it.
+        dismissOnFocusOutside: false,
         size: 'full',
         contentClass: SETTINGS_CONTENT_CLASS,
         overlayClass: isWorkspaceMode ? 'p-8' : undefined

src/stores/dialogStore.ts

@@ -38,6 +38,14 @@ interface CustomDialogComponentProps {
   pt?: DialogPassThroughOptions
   closeOnEscape?: boolean
   dismissableMask?: boolean
+  /**
+   * When `false`, the Reka dialog does not dismiss when focus leaves its
+   * content. Set on container dialogs (e.g. Settings) that host nested dialogs,
+   * where a nested dialog closing can move focus onto an ordinary app element
+   * — a programmatic shift that must not be read as a dismiss. Escape and
+   * outside-pointer dismissal are unaffected. Defaults to `true`.
+   */
+  dismissOnFocusOutside?: boolean
   unstyled?: boolean
   headless?: boolean
   renderer?: DialogRenderer