Merge branch 'main' into matt/be-2793-cloud-secrets-e2e-structure

test: address review feedback on secrets + dialog tests
- Remove leaked focus-target button after focus-outside tests via a returned cleanup callback (test isolation) - Flush all pending microtasks instead of a hardcoded tick count so the focus-outside negative control is robust to handler await depth - Assert the secret is never echoed into a field with a single immediate check rather than poll-until-false, which could mask a transient leak Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-17 01:07:56 +00:00 · 2026-07-09 23:48:17 -04:00 · 2026-07-09 19:42:02 -07:00 · 2026-07-09 19:29:41 -07:00
3 changed files with 263 additions and 155 deletions
--- a/browser_tests/fixtures/utils/cloudSecretsMocks.ts
+++ b/browser_tests/fixtures/utils/cloudSecretsMocks.ts
@@ -0,0 +1,144 @@
+import type { Page, Route } from '@playwright/test'
+
+import type { RemoteConfig } from '@/platform/remoteConfig/types'
+
+import type { SettingDialog } from '@e2e/fixtures/components/SettingDialog'
+import { jsonRoute } from '@e2e/fixtures/utils/jsonRoute'
+
+/**
+ * Shared scaffolding for the cloud user-secrets (API keys) E2E, alongside the
+ * sibling cloud mocks (`cloudBillingMocks`, `workspaceMocks`): the stateful
+ * in-memory `/secrets` backend and the open-the-Secrets-panel helper, so the
+ * spec files hold only the behavioral flow.
+ */
+
+// `/api/features` is the remote-config source. Enabling user secrets is what
+// surfaces the Secrets settings panel for a signed-in user.
+export const SECRETS_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.
+export const SECRETS_BOOT_SETTINGS = { 'Comfy.TutorialCompleted': true }
+
+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).
+ */
+export 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. Returns the dialog root locator
+ * for scoping the caller's assertions.
+ */
+export async function openSecretsPanel(settingDialog: SettingDialog) {
+  const { page } = settingDialog
+  await settingDialog.open()
+
+  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 settingDialog.category('Secrets').click()
+
+  await Promise.all([providersResolved, listResolved])
+  return settingDialog.root
+}
--- a/browser_tests/tests/cloudSecrets.spec.ts
+++ b/browser_tests/tests/cloudSecrets.spec.ts
@@ -1,11 +1,13 @@
 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 { ComfyPage, comfyPageFixture as test } from '@e2e/fixtures/ComfyPage'
 import { bootCloud, mockCloudBoot } from '@e2e/fixtures/utils/cloudBootMocks'
-import { jsonRoute } from '@e2e/fixtures/utils/jsonRoute'
+import {
+  SECRETS_BOOT_FEATURES,
+  SECRETS_BOOT_SETTINGS,
+  mockSecretsBackend,
+  openSecretsPanel
+} from '@e2e/fixtures/utils/cloudSecretsMocks'

 /**
 * End-to-end coverage for the user-secrets (API keys) surface in the cloud app:
@@ -15,164 +17,28 @@ import { jsonRoute } from '@e2e/fixtures/utils/jsonRoute'
 *
 * 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.
+ * `bootCloud` boot the app signed-in, and `mockSecretsBackend` layers a
+ * stateful in-memory `/secrets` backend on top so the flow is deterministic
+ * and never touches a real server. A bare `ComfyPage` (constructed, never
+ * `setup()`) supplies the shared `SettingDialog` page object without the
+ * backend-touching fixture setup.
 */
 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
+    page,
+    request
  }) => {
    test.slow()

    await mockCloudBoot(page, {
-      features: BOOT_FEATURES,
-      settings: BOOT_SETTINGS
+      features: SECRETS_BOOT_FEATURES,
+      settings: SECRETS_BOOT_SETTINGS
    })
    await bootCloud(page)
    const backend = await mockSecretsBackend(page, ['runway', 'gemini'])
@@ -182,7 +48,8 @@ test.describe('Cloud user secrets (API keys)', { tag: '@cloud' }, () => {
      timeout: 45_000
    })

-    const settingsDialog = await openSecretsPanel(page)
+    const comfyPage = new ComfyPage(page, request)
+    const settingsDialog = await openSecretsPanel(comfyPage.settingDialog)

    // Empty state before anything is added.
    await expect(settingsDialog.getByText(/No secrets stored/)).toBeVisible()
@@ -219,6 +86,22 @@ test.describe('Cloud user secrets (API keys)', { tag: '@cloud' }, () => {
    // ...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)
+    // `getByText` only sees text nodes; a value reflected into an `<input>` or
+    // masked field would slip past it, so assert no field carries it either.
+    // The list has already settled above, so this is a single immediate
+    // assertion — a poll-until-false could mask a value that briefly echoed
+    // into a field and then cleared within the polling window.
+    const secretEchoedInField = await page
+      .locator('input, textarea')
+      .evaluateAll(
+        (fields, value) =>
+          fields.some(
+            (field) =>
+              (field as HTMLInputElement | HTMLTextAreaElement).value === value
+          ),
+        RUNWAY_KEY_VALUE
+      )
+    expect(secretEchoedInField).toBe(false)

    // --- DELETE ----------------------------------------------------------
    await settingsDialog
@@ -238,13 +121,14 @@ test.describe('Cloud user secrets (API keys)', { tag: '@cloud' }, () => {
  })

  test('a non-entitled account never sees the gated providers', async ({
-    page
+    page,
+    request
  }) => {
    test.slow()

    await mockCloudBoot(page, {
-      features: BOOT_FEATURES,
-      settings: BOOT_SETTINGS
+      features: SECRETS_BOOT_FEATURES,
+      settings: SECRETS_BOOT_SETTINGS
    })
    await bootCloud(page)
    // Non-entitled: the server omits runway/gemini from the allowlist.
@@ -255,7 +139,8 @@ test.describe('Cloud user secrets (API keys)', { tag: '@cloud' }, () => {
      timeout: 45_000
    })

-    const settingsDialog = await openSecretsPanel(page)
+    const comfyPage = new ComfyPage(page, request)
+    const settingsDialog = await openSecretsPanel(comfyPage.settingDialog)
    await expect(settingsDialog.getByText(/No secrets stored/)).toBeVisible()

    // The add form opens, but its provider dropdown is empty — the gated
--- a/src/components/dialog/GlobalDialog.test.ts
+++ b/src/components/dialog/GlobalDialog.test.ts
@@ -32,6 +32,9 @@ const Body = defineComponent({
  setup: () => () => h('p', { 'data-testid': 'body' }, 'body content')
 })

+const flushPromises = () =>
+  new Promise<void>((resolve) => setTimeout(resolve, 0))
+
 const ClosedNonModalDialog = defineComponent({
  name: 'ClosedNonModalDialog',
  setup: () => () =>
@@ -361,6 +364,82 @@ describe('GlobalDialog Reka overlay scrim', () => {
  })
 })

+describe('GlobalDialog Reka focus-outside binding', () => {
+  beforeEach(() => {
+    setActivePinia(createTestingPinia({ stubActions: false }))
+  })
+
+  afterEach(() => {
+    cleanup()
+  })
+
+  // Reka's DismissableLayer fires focus-outside off a real focus transition
+  // (blur inside the layer, then focusin on the new target), so drive the
+  // mounted binding by moving focus to a fresh element outside the dialog
+  // rather than dispatching a synthetic event.
+  async function moveFocusToPlainElementOutside() {
+    const outside = document.createElement('button')
+    document.body.appendChild(outside)
+    outside.focus()
+    return () => outside.remove()
+  }
+
+  it('dismisses on focus-outside by default', async () => {
+    mountDialog()
+    const store = useDialogStore()
+
+    store.showDialog({
+      key: 'focus-default',
+      title: 'Focus dismisses',
+      component: Body,
+      dialogComponentProps: { renderer: 'reka', modal: false }
+    })
+
+    await screen.findByRole('dialog')
+    const removeOutside = await moveFocusToPlainElementOutside()
+    try {
+      await waitFor(() =>
+        expect(store.isDialogOpen('focus-default')).toBe(false)
+      )
+    } finally {
+      removeOutside()
+    }
+  })
+
+  it('does not dismiss on focus-outside when dismissOnFocusOutside is false', async () => {
+    // Exercises GlobalDialog's own template wiring
+    // `@focus-outside="(e) => onRekaFocusOutside(e, item.dialogComponentProps)"`
+    // through a mounted dialog — the direct `onRekaFocusOutside` unit test can't
+    // catch a regression that drops the props argument here. The positive
+    // control above proves the focus-outside path really fires, so this staying
+    // open isolates the opt-out flag rather than a dead event.
+    mountDialog()
+    const store = useDialogStore()
+
+    store.showDialog({
+      key: 'focus-opted-out',
+      title: 'Focus blocked',
+      component: Body,
+      dialogComponentProps: {
+        renderer: 'reka',
+        modal: false,
+        dismissOnFocusOutside: false
+      }
+    })
+
+    await screen.findByRole('dialog')
+    const removeOutside = await moveFocusToPlainElementOutside()
+    try {
+      // Drain every pending microtask so a wrongful dismiss lands before we
+      // assert, regardless of how many awaits deep the handler chain runs.
+      await flushPromises()
+      expect(store.isDialogOpen('focus-opted-out')).toBe(true)
+    } finally {
+      removeOutside()
+    }
+  })
+})
+
 describe('shouldPreventRekaDismiss', () => {
  function makeEvent(target: Element | null) {
    let prevented = false