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

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"