test: address publish dialog review feedback

2026-04-19 22:09:37 +00:00 · 2026-04-08 14:14:22 +09:00
parent 1be1ac71f4
commit c92d0b8b14
3 changed files with 113 additions and 138 deletions
--- a/browser_tests/fixtures/components/PublishDialog.ts
+++ b/browser_tests/fixtures/components/PublishDialog.ts
@@ -22,10 +22,7 @@ export class PublishDialog extends BaseDialog {
   */
  async open(): Promise<void> {
    await this.page.evaluate(async () => {
-      const store = window.app!.extensionManager as {
-        dialog: { showPublishDialog: () => Promise<void> }
-      }
-      await store.dialog.showPublishDialog()
+      await window.app!.extensionManager.dialog.showPublishDialog()
    })
    await this.waitForVisible()
  }
--- a/browser_tests/fixtures/helpers/PublishApiHelper.ts
+++ b/browser_tests/fixtures/helpers/PublishApiHelper.ts
@@ -113,6 +113,7 @@ export class PublishApiHelper {
  async mockPublishWorkflow(
    response: WorkflowPublishInfo = DEFAULT_PUBLISH_RESPONSE
  ): Promise<void> {
+    await this.removeRoutes('**/hub/workflows')
    await this.addRoute('**/hub/workflows', async (route) => {
      if (route.request().method() !== 'POST') {
        await route.continue()
@@ -130,6 +131,7 @@ export class PublishApiHelper {
    statusCode = 500,
    message = 'Failed to publish workflow'
  ): Promise<void> {
+    await this.removeRoutes('**/hub/workflows')
    await this.addRoute('**/hub/workflows', async (route) => {
      if (route.request().method() !== 'POST') {
        await route.continue()
@@ -197,4 +199,16 @@ export class PublishApiHelper {
    this.routeHandlers.push({ pattern, handler })
    await this.page.route(pattern, handler)
  }
+
+  private async removeRoutes(pattern: string): Promise<void> {
+    const handlers = this.routeHandlers.filter(
+      (route) => route.pattern === pattern
+    )
+    for (const { handler } of handlers) {
+      await this.page.unroute(pattern, handler)
+    }
+    this.routeHandlers = this.routeHandlers.filter(
+      (route) => route.pattern !== pattern
+    )
+  }
 }
--- a/browser_tests/tests/dialogs/publishDialog.spec.ts
+++ b/browser_tests/tests/dialogs/publishDialog.spec.ts
@@ -1,34 +1,60 @@
 import { expect } from '@playwright/test'

+import type { ComfyPage } from '@e2e/fixtures/ComfyPage'
+
 import { comfyPageFixture as test } from '@e2e/fixtures/ComfyPage'
 import { PublishDialog } from '@e2e/fixtures/components/PublishDialog'
 import { PublishApiHelper } from '@e2e/fixtures/helpers/PublishApiHelper'

+const PUBLISH_FEATURE_FLAGS = {
+  comfyhub_upload_enabled: true,
+  comfyhub_profile_gate_enabled: true
+} as const
+
+async function setupPublishTest(
+  comfyPage: ComfyPage
+): Promise<{
+  dialog: PublishDialog
+  publishApi: PublishApiHelper
+}> {
+  const dialog = new PublishDialog(comfyPage.page)
+  const publishApi = new PublishApiHelper(comfyPage.page)
+
+  await comfyPage.featureFlags.setFlags(PUBLISH_FEATURE_FLAGS)
+
+  return { dialog, publishApi }
+}
+
+async function saveAndOpenPublishDialog(
+  comfyPage: ComfyPage,
+  dialog: PublishDialog,
+  workflowName: string
+): Promise<void> {
+  await comfyPage.menu.topbar.saveWorkflow(workflowName)
+  const overwriteDialog = comfyPage.page.locator(
+    '.p-dialog:has-text("Overwrite")'
+  )
+  if (await overwriteDialog.isVisible()) {
+    await comfyPage.confirmDialog.click('overwrite')
+  }
+
+  await dialog.open()
+}
+
 test.describe('Publish dialog - wizard navigation', () => {
  let dialog: PublishDialog
  let publishApi: PublishApiHelper

  test.beforeEach(async ({ comfyPage }) => {
-    dialog = new PublishDialog(comfyPage.page)
-    publishApi = new PublishApiHelper(comfyPage.page)
-
-    await comfyPage.featureFlags.setFlags({
-      comfyhub_upload_enabled: true,
-      comfyhub_profile_gate_enabled: true
-    })
-
+    const setup = await setupPublishTest(comfyPage)
+    dialog = setup.dialog
+    publishApi = setup.publishApi
    await publishApi.setupDefaultMocks()
+    await saveAndOpenPublishDialog(comfyPage, dialog, 'test-publish-wf')
+  })

-    await comfyPage.menu.topbar.saveWorkflow('test-publish-wf')
-    // Handle overwrite confirmation if the file already exists
-    const overwriteDialog = comfyPage.page.locator(
-      '.p-dialog:has-text("Overwrite")'
-    )
-    if (await overwriteDialog.isVisible()) {
-      await comfyPage.confirmDialog.click('overwrite')
-    }
-
-    await dialog.open()
+  test.afterEach(async () => {
+    await publishApi.cleanup()
  })

  test('opens on the Describe step by default', async () => {
@@ -82,24 +108,15 @@ test.describe('Publish dialog - Describe step', () => {
  let publishApi: PublishApiHelper

  test.beforeEach(async ({ comfyPage }) => {
-    dialog = new PublishDialog(comfyPage.page)
-    publishApi = new PublishApiHelper(comfyPage.page)
-
-    await comfyPage.featureFlags.setFlags({
-      comfyhub_upload_enabled: true,
-      comfyhub_profile_gate_enabled: true
-    })
-
+    const setup = await setupPublishTest(comfyPage)
+    dialog = setup.dialog
+    publishApi = setup.publishApi
    await publishApi.setupDefaultMocks()
-    await comfyPage.menu.topbar.saveWorkflow('test-describe-wf')
-    const overwriteDialog = comfyPage.page.locator(
-      '.p-dialog:has-text("Overwrite")'
-    )
-    if (await overwriteDialog.isVisible()) {
-      await comfyPage.confirmDialog.click('overwrite')
-    }
+    await saveAndOpenPublishDialog(comfyPage, dialog, 'test-describe-wf')
+  })

-    await dialog.open()
+  test.afterEach(async () => {
+    await publishApi.cleanup()
  })

  test('allows editing the workflow name', async () => {
@@ -135,27 +152,18 @@ test.describe('Publish dialog - Examples step', () => {
  let publishApi: PublishApiHelper

  test.beforeEach(async ({ comfyPage }) => {
-    dialog = new PublishDialog(comfyPage.page)
-    publishApi = new PublishApiHelper(comfyPage.page)
-
-    await comfyPage.featureFlags.setFlags({
-      comfyhub_upload_enabled: true,
-      comfyhub_profile_gate_enabled: true
-    })
-
+    const setup = await setupPublishTest(comfyPage)
+    dialog = setup.dialog
+    publishApi = setup.publishApi
    await publishApi.setupDefaultMocks()
-    await comfyPage.menu.topbar.saveWorkflow('test-examples-wf')
-    const overwriteDialog = comfyPage.page.locator(
-      '.p-dialog:has-text("Overwrite")'
-    )
-    if (await overwriteDialog.isVisible()) {
-      await comfyPage.confirmDialog.click('overwrite')
-    }
-
-    await dialog.open()
+    await saveAndOpenPublishDialog(comfyPage, dialog, 'test-examples-wf')
    await dialog.goNext() // Navigate to Examples step
  })

+  test.afterEach(async () => {
+    await publishApi.cleanup()
+  })
+
  test('shows thumbnail type toggle options', async () => {
    await expect(dialog.root.getByText('Image', { exact: true })).toBeVisible()
    await expect(dialog.root.getByText('Video', { exact: true })).toBeVisible()
@@ -176,27 +184,18 @@ test.describe('Publish dialog - Finish step with profile', () => {
  let publishApi: PublishApiHelper

  test.beforeEach(async ({ comfyPage }) => {
-    dialog = new PublishDialog(comfyPage.page)
-    publishApi = new PublishApiHelper(comfyPage.page)
-
-    await comfyPage.featureFlags.setFlags({
-      comfyhub_upload_enabled: true,
-      comfyhub_profile_gate_enabled: true
-    })
-
+    const setup = await setupPublishTest(comfyPage)
+    dialog = setup.dialog
+    publishApi = setup.publishApi
    await publishApi.setupDefaultMocks({ hasProfile: true })
-    await comfyPage.menu.topbar.saveWorkflow('test-finish-wf')
-    const overwriteDialog = comfyPage.page.locator(
-      '.p-dialog:has-text("Overwrite")'
-    )
-    if (await overwriteDialog.isVisible()) {
-      await comfyPage.confirmDialog.click('overwrite')
-    }
-
-    await dialog.open()
+    await saveAndOpenPublishDialog(comfyPage, dialog, 'test-finish-wf')
    await dialog.goToStep('Finish publishing')
  })

+  test.afterEach(async () => {
+    await publishApi.cleanup()
+  })
+
  test('shows profile card with username', async () => {
    await expect(dialog.finishStep).toBeVisible()
    await expect(dialog.root.getByText('@testuser')).toBeVisible()
@@ -213,30 +212,21 @@ test.describe('Publish dialog - Finish step with private assets', () => {
  let publishApi: PublishApiHelper

  test.beforeEach(async ({ comfyPage }) => {
-    dialog = new PublishDialog(comfyPage.page)
-    publishApi = new PublishApiHelper(comfyPage.page)
-
-    await comfyPage.featureFlags.setFlags({
-      comfyhub_upload_enabled: true,
-      comfyhub_profile_gate_enabled: true
-    })
-
+    const setup = await setupPublishTest(comfyPage)
+    dialog = setup.dialog
+    publishApi = setup.publishApi
    await publishApi.setupDefaultMocks({
      hasProfile: true,
      hasPrivateAssets: true
    })
-    await comfyPage.menu.topbar.saveWorkflow('test-assets-wf')
-    const overwriteDialog = comfyPage.page.locator(
-      '.p-dialog:has-text("Overwrite")'
-    )
-    if (await overwriteDialog.isVisible()) {
-      await comfyPage.confirmDialog.click('overwrite')
-    }
-
-    await dialog.open()
+    await saveAndOpenPublishDialog(comfyPage, dialog, 'test-assets-wf')
    await dialog.goToStep('Finish publishing')
  })

+  test.afterEach(async () => {
+    await publishApi.cleanup()
+  })
+
  test('publish button is disabled until assets acknowledged', async () => {
    await expect(dialog.finishStep).toBeVisible()
    await expect(dialog.publishButton).toBeDisabled()
@@ -254,27 +244,18 @@ test.describe('Publish dialog - no profile', () => {
  let publishApi: PublishApiHelper

  test.beforeEach(async ({ comfyPage }) => {
-    dialog = new PublishDialog(comfyPage.page)
-    publishApi = new PublishApiHelper(comfyPage.page)
-
-    await comfyPage.featureFlags.setFlags({
-      comfyhub_upload_enabled: true,
-      comfyhub_profile_gate_enabled: true
-    })
-
+    const setup = await setupPublishTest(comfyPage)
+    dialog = setup.dialog
+    publishApi = setup.publishApi
    await publishApi.setupDefaultMocks({ hasProfile: false })
-    await comfyPage.menu.topbar.saveWorkflow('test-noprofile-wf')
-    const overwriteDialog = comfyPage.page.locator(
-      '.p-dialog:has-text("Overwrite")'
-    )
-    if (await overwriteDialog.isVisible()) {
-      await comfyPage.confirmDialog.click('overwrite')
-    }
-
-    await dialog.open()
+    await saveAndOpenPublishDialog(comfyPage, dialog, 'test-noprofile-wf')
    await dialog.goToStep('Finish publishing')
  })

+  test.afterEach(async () => {
+    await publishApi.cleanup()
+  })
+
  test('shows profile creation prompt when user has no profile', async () => {
    await expect(dialog.profilePrompt).toBeVisible()
    await expect(
@@ -293,18 +274,17 @@ test.describe('Publish dialog - unsaved workflow', () => {
  let publishApi: PublishApiHelper

  test.beforeEach(async ({ comfyPage }) => {
-    dialog = new PublishDialog(comfyPage.page)
-    publishApi = new PublishApiHelper(comfyPage.page)
-
-    await comfyPage.featureFlags.setFlags({
-      comfyhub_upload_enabled: true,
-      comfyhub_profile_gate_enabled: true
-    })
-
+    const setup = await setupPublishTest(comfyPage)
+    dialog = setup.dialog
+    publishApi = setup.publishApi
    await publishApi.setupDefaultMocks()
    // Don't save workflow — open dialog on the default temporary workflow
  })

+  test.afterEach(async () => {
+    await publishApi.cleanup()
+  })
+
  test('shows save prompt for temporary workflow', async ({ comfyPage }) => {
    // Create a new workflow to ensure it's temporary
    await comfyPage.menu.topbar.triggerTopbarCommand(['New'])
@@ -324,26 +304,18 @@ test.describe('Publish dialog - submission', () => {
  let publishApi: PublishApiHelper

  test.beforeEach(async ({ comfyPage }) => {
-    dialog = new PublishDialog(comfyPage.page)
-    publishApi = new PublishApiHelper(comfyPage.page)
+    const setup = await setupPublishTest(comfyPage)
+    dialog = setup.dialog
+    publishApi = setup.publishApi
+  })

-    await comfyPage.featureFlags.setFlags({
-      comfyhub_upload_enabled: true,
-      comfyhub_profile_gate_enabled: true
-    })
+  test.afterEach(async () => {
+    await publishApi.cleanup()
  })

  test('successful publish closes dialog', async ({ comfyPage }) => {
    await publishApi.setupDefaultMocks({ hasProfile: true })
-    await comfyPage.menu.topbar.saveWorkflow('test-submit-wf')
-    const overwriteDialog = comfyPage.page.locator(
-      '.p-dialog:has-text("Overwrite")'
-    )
-    if (await overwriteDialog.isVisible()) {
-      await comfyPage.confirmDialog.click('overwrite')
-    }
-
-    await dialog.open()
+    await saveAndOpenPublishDialog(comfyPage, dialog, 'test-submit-wf')
    await dialog.goToStep('Finish publishing')
    await expect(dialog.finishStep).toBeVisible()

@@ -356,15 +328,7 @@ test.describe('Publish dialog - submission', () => {
    // Override publish mock with error response
    await publishApi.mockPublishWorkflowError(500, 'Internal error')

-    await comfyPage.menu.topbar.saveWorkflow('test-submit-fail-wf')
-    const overwriteDialog = comfyPage.page.locator(
-      '.p-dialog:has-text("Overwrite")'
-    )
-    if (await overwriteDialog.isVisible()) {
-      await comfyPage.confirmDialog.click('overwrite')
-    }
-
-    await dialog.open()
+    await saveAndOpenPublishDialog(comfyPage, dialog, 'test-submit-fail-wf')
    await dialog.goToStep('Finish publishing')
    await expect(dialog.finishStep).toBeVisible()