Fix image upload modal cancel loop (#1755)

* minimal repro. lmfao

* fix the most pressing part of the problem

* Revert "minimal repro. lmfao"

e9c51c2

* using e2e tests for evil again

* increase per-test timeout

* fix debug-ci-e2e-fail by filtering for CI workflow runs

* try giving the file upload slightly more time

* split click through networking test in two so safari might not be such a BABY about it

Author: david-crespo

app/test/e2e/image-upload.e2e.ts

@@ -10,9 +10,9 @@ import { expect, test } from '@playwright/test'
 
 import { MiB } from '@oxide/util'
 
-import { expectNotVisible, expectRowVisible, expectVisible } from './utils'
+import { expectNotVisible, expectRowVisible, expectVisible, sleep } from './utils'
 
-async function chooseFile(page: Page, size = 10 * MiB) {
+async function chooseFile(page: Page, size = 15 * MiB) {
   const fileChooserPromise = page.waitForEvent('filechooser')
   await page.getByText('Image file', { exact: true }).click()
   const fileChooser = await fileChooserPromise
@@ -49,6 +49,16 @@ async function expectUploadProcess(page: Page) {
   await done.click()
 }
 
+async function fillForm(page: Page, name: string) {
+  await page.goto('/projects/mock-project/images-new')
+
+  await page.fill('role=textbox[name="Name"]', name)
+  await page.fill('role=textbox[name="Description"]', 'image description')
+  await page.fill('role=textbox[name="OS"]', 'Ubuntu')
+  await page.fill('role=textbox[name="Version"]', 'Dapper Drake')
+  await chooseFile(page)
+}
+
 test.describe('Image upload', () => {
   test('happy path', async ({ page }) => {
     await page.goto('/projects/mock-project/images')
@@ -61,11 +71,7 @@ test.describe('Image upload', () => {
 
     await expectVisible(page, ['role=dialog[name="Upload image"]'])
 
-    await page.fill('role=textbox[name="Name"]', 'new-image')
-    await page.fill('role=textbox[name="Description"]', 'image description')
-    await page.fill('role=textbox[name="OS"]', 'Ubuntu')
-    await page.fill('role=textbox[name="Version"]', 'Dapper Drake')
-    await chooseFile(page)
+    await fillForm(page, 'new-image')
 
     await page.click('role=button[name="Upload image"]')
 
@@ -81,13 +87,7 @@ test.describe('Image upload', () => {
   })
 
   test('with name taken', async ({ page }) => {
-    await page.goto('/projects/mock-project/images-new')
-
-    await page.fill('role=textbox[name="Name"]', 'image-1')
-    await page.fill('role=textbox[name="Description"]', 'image description')
-    await page.fill('role=textbox[name="OS"]', 'Ubuntu')
-    await page.fill('role=textbox[name="Version"]', 'Dapper Drake')
-    await chooseFile(page)
+    await fillForm(page, 'image-1')
 
     await expectNotVisible(page, ['text="Image name already exists"'])
     await page.click('role=button[name="Upload image"]')
@@ -127,13 +127,7 @@ test.describe('Image upload', () => {
   })
 
   test('cancel', async ({ page }) => {
-    await page.goto('/projects/mock-project/images-new')
-
-    await page.fill('role=textbox[name="Name"]', 'new-image')
-    await page.fill('role=textbox[name="Description"]', 'image description')
-    await page.fill('role=textbox[name="OS"]', 'Ubuntu')
-    await page.fill('role=textbox[name="Version"]', 'Dapper Drake')
-    await chooseFile(page)
+    await fillForm(page, 'new-image')
 
     await page.click('role=button[name="Upload image"]')
 
@@ -147,14 +141,13 @@ test.describe('Image upload', () => {
     // form is disabled and semi-hidden
     // await expectNotVisible(page, ['role=textbox[name="Name"]'])
 
+    const progressModal = page.getByRole('dialog', { name: 'Image upload progress' })
+
     page.on('dialog', (dialog) => dialog.accept()) // click yes on the are you sure prompt
-    await page
-      .getByRole('dialog', { name: 'Image upload progress' })
-      .getByRole('button', { name: 'Cancel' })
-      .click()
+    await progressModal.getByRole('button', { name: 'Cancel' }).click()
 
     // modal has closed
-    await expectNotVisible(page, ['role=dialog[name="Image upload progress"]'])
+    await expect(progressModal).toBeHidden()
 
     // form's back
     await expectVisible(page, ['role=textbox[name="Name"]'])
@@ -168,14 +161,39 @@ test.describe('Image upload', () => {
     // await expectNotVisible(page, ['role=cell[name=tmp]'])
   })
 
-  test('Image upload cancel and retry', async ({ page }) => {
-    await page.goto('/projects/mock-project/images-new')
+  // testing the onFocusOutside fix
+  test('cancel canceling', async ({ page }) => {
+    await fillForm(page, 'new-image')
 
-    await page.fill('role=textbox[name="Name"]', 'new-image')
-    await page.fill('role=textbox[name="Description"]', 'image description')
-    await page.fill('role=textbox[name="OS"]', 'Ubuntu')
-    await page.fill('role=textbox[name="Version"]', 'Dapper Drake')
-    await chooseFile(page)
+    const progressModal = page.getByRole('dialog', { name: 'Image upload progress' })
+
+    await page.getByRole('button', { name: 'Upload image' }).click()
+    await expect(progressModal).toBeVisible()
+
+    let confirmCount = 0
+
+    page.on('dialog', (dialog) => {
+      confirmCount += 1
+      dialog.dismiss()
+    }) // click cancel on the are you sure prompt
+
+    await progressModal.getByRole('button', { name: 'Cancel' }).click()
+
+    // still visible because we canceled the cancel!
+    await expect(progressModal).toBeVisible()
+    expect(confirmCount).toEqual(1)
+
+    // now let's try canceling by clicking out on the background over the side modal
+    await page.getByLabel('4096').click()
+
+    await sleep(100)
+
+    // without the onFocusOutside fix this is a higher number
+    expect(confirmCount).toEqual(2)
+  })
+
+  test('Image upload cancel and retry', async ({ page }) => {
+    await fillForm(page, 'new-image')
 
     await page.click('role=button[name="Upload image"]')
 
@@ -197,12 +215,10 @@ test.describe('Image upload', () => {
     await expect(progressModal).toBeHidden()
 
     // form's back
-    await expectVisible(page, [
-      'role=textbox[name="Name"]',
-      // need to wait for submit button to come back because it's in a loading
-      // state while the cleanup runs
-      'role=button[name="Upload image"]',
-    ])
+    await expect(page.getByRole('textbox', { name: 'Name' })).toBeVisible()
+    // need to wait for submit button to come back because it's in a loading
+    // state while the cleanup runs
+    await expect(page.getByRole('button', { name: 'Upload image' })).toBeVisible()
 
     // resubmit and it should work fine
     await page.getByRole('button', { name: 'Upload image' }).click()
@@ -218,15 +234,7 @@ test.describe('Image upload', () => {
 
   for (const { imageName, stepText } of failureCases) {
     test(`failure ${imageName}`, async ({ page }) => {
-      await page.goto('/projects/mock-project/images-new')
-
-      // note special image name
-      await page.fill('role=textbox[name="Name"]', imageName)
-
-      await page.fill('role=textbox[name="Description"]', 'image description')
-      await page.fill('role=textbox[name="OS"]', 'Ubuntu')
-      await page.fill('role=textbox[name="Version"]', 'Dapper Drake')
-      await chooseFile(page)
+      await fillForm(page, imageName)
 
       await page.click('role=button[name="Upload image"]')
 

app/test/e2e/networking.e2e.ts

@@ -5,11 +5,11 @@
  *
  * Copyright Oxide Computer Company
  */
-import { test } from '@playwright/test'
+import { expect, test } from '@playwright/test'
 
 import { expectNotVisible, expectVisible } from './utils'
 
-test('Click through networking', async ({ page }) => {
+test('Create and edit VPC', async ({ page }) => {
   await page.goto('/projects/mock-project')
 
   await page.click('role=link[name*="Networking"]')
@@ -48,13 +48,15 @@ test('Click through networking', async ({ page }) => {
   // Close toast, it holds up the test for some reason
   await page.click('role=button[name="Dismiss notification"]')
 
-  await expectVisible(page, ['role=link[name="new-vpc"]'])
+  await expect(page.getByRole('link', { name: 'new-vpc' })).toBeVisible()
+})
 
-  await page.click('role=link[name="new-vpc"]')
+test('Create and edit subnet', async ({ page }) => {
+  await page.goto('/projects/mock-project/vpcs/mock-vpc')
 
   // VPC detail, subnets tab
   await expectVisible(page, [
-    'role=heading[name*="new-vpc"]',
+    'role=heading[name*="mock-vpc"]',
     'role=tab[name="Subnets"]',
     // 'role=tab[name="System Routes"]',
     // 'role=tab[name="Routers"]',
@@ -92,22 +94,6 @@ test('Click through networking', async ({ page }) => {
   await expectNotVisible(page, ['role=cell[name="new-subnet"]'])
   await expectVisible(page, ['role=cell[name="edited-subnet"]'])
 
-  // // System routes
-  // await page.click('role=tab[name="System Routes"]')
-  // await expectVisible(page, ['role=cell[name="system"]'])
-
-  // // Routers
-  // await page.click('role=tab[name="Routers"]')
-  // await expectVisible(page, ['role=cell[name="system"] >> nth=0'])
-  // await page.click('role=button[name="New router"]')
-  // await expectVisible(page, [
-  //   'role=heading[name="Create VPC Router"]',
-  //   'role=button[name="Create VPC Router"]',
-  // ])
-  // await page.fill('role=textbox[name="Name"]', 'new-router')
-  // await page.click('role=button[name="Create VPC Router"]')
-  // await expectVisible(page, ['role=cell[name="new-router"]', 'role=cell[name="custom"]'])
-
   // Firewall rules
   await page.click('role=tab[name="Firewall Rules"]')
   await expectVisible(page, [

app/test/e2e/utils.ts

@@ -140,3 +140,5 @@ export async function expectObscured(locator: Locator) {
     async () => await locator.click({ trial: true, timeout: 50 })
   ).rejects.toThrow(/locator.click: Timeout 50ms exceeded/)
 }
+
+export const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms))

libs/ui/lib/modal/Modal.tsx

@@ -66,6 +66,12 @@ export function Modal({ children, onDismiss, title, isOpen }: ModalProps) {
                   style={{
                     transform: y.to((value) => `translate3d(-50%, ${-50 + value}%, 0px)`),
                   }}
+                  // Prevents cancel loop on clicking on background over side
+                  // modal to get out of image upload modal. Canceling out of
+                  // confirm dialog returns focus to the dismissable layer,
+                  // which triggers onDismiss again. And again.
+                  // https://github.com/oxidecomputer/console/issues/1745
+                  onFocusOutside={(e) => e.preventDefault()}
                 >
                   {title && (
                     <Dialog.Title asChild>

playwright.config.ts

@@ -20,7 +20,7 @@ const config: PlaywrightTestConfig = {
   retries: process.env.CI ? 2 : 0,
   /* Opt out of parallel tests on CI. */
   workers: process.env.CI ? 1 : undefined,
-  timeout: 60000,
+  timeout: 2 * 60 * 1000, // 2 minutes, surely overkill
   fullyParallel: true,
   use: {
     trace: 'retain-on-failure',

tools/debug-ci-e2e-fail.sh

@@ -9,7 +9,8 @@ set -e
 set -o pipefail
 
 # Get the ID of the last github actions run if there was one
-RUN_ID=$(gh run list -b $(git rev-parse --abbrev-ref HEAD) -L 1 --json databaseId --jq '.[0].databaseId')
+BRANCH=$(git rev-parse --abbrev-ref HEAD)
+RUN_ID=$(gh run list -b "$BRANCH" -w CI -L 1 --json databaseId --jq '.[0].databaseId')
 
 if [ -z "$RUN_ID" ]; then
   echo "No action runs found for this branch"