From b9c548c7e2f069199f6b165e6d0b386edcc68125 Mon Sep 17 00:00:00 2001
From: David Crespo <david.crespo@oxidecomputer.com>
Date: Wed, 11 Dec 2024 23:02:14 -0600
Subject: [PATCH] fix instance disk menus closing on poll

---
 .../instances/instance/tabs/StorageTab.tsx    | 36 ++++++++++-----
 test/e2e/instance-disks.e2e.ts                | 44 +++++++++++++++++++
 test/e2e/instance.e2e.ts                      |  4 ++
 3 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx
index 8008d5aedf..f99c85bfd9 100644
--- a/app/pages/project/instances/instance/tabs/StorageTab.tsx
+++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx
@@ -146,10 +146,6 @@ export function Component() {
     [disks.items, instance.bootDiskId]
   )
 
-  // Needed to keep them the same while setting boot disk.
-  // Extracted to keep dep array appropriately zealous.
-  const { ncpus, memory } = instance
-
   const makeBootDiskActions = useCallback(
     (disk: InstanceDisk): MenuAction[] => [
       getSnapshotAction(disk),
@@ -168,8 +164,8 @@ export function Component() {
                 path: { instance: instance.id },
                 body: {
                   bootDisk: undefined,
-                  ncpus,
-                  memory,
+                  ncpus: instance.ncpus,
+                  memory: instance.memory,
                   // this would get unset if we left it out
                   autoRestartPolicy: instance.autoRestartPolicy,
                 },
@@ -200,7 +196,16 @@ export function Component() {
         onActivate() {}, // it's always disabled, so noop is ok
       },
     ],
-    [instanceUpdate, instance, getSnapshotAction, ncpus, memory]
+    [
+      instanceUpdate,
+      // don't put the entire instance in here. it is not referentially
+      // stable across polls, so the menus will close during polling
+      instance.id,
+      instance.autoRestartPolicy,
+      instance.ncpus,
+      instance.memory,
+      getSnapshotAction,
+    ]
   )
 
   const makeOtherDiskActions = useCallback(
@@ -223,8 +228,8 @@ export function Component() {
                 path: { instance: instance.id },
                 body: {
                   bootDisk: disk.id,
-                  ncpus,
-                  memory,
+                  ncpus: instance.ncpus,
+                  memory: instance.memory,
                   // this would get unset if we left it out
                   autoRestartPolicy: instance.autoRestartPolicy,
                 },
@@ -262,7 +267,18 @@ export function Component() {
         },
       },
     ],
-    [detachDisk, instanceUpdate, instance, getSnapshotAction, bootDisks, ncpus, memory]
+    [
+      detachDisk,
+      instanceUpdate,
+      // don't put the entire instance in here. it is not referentially
+      // stable across polls, so the menus will close during polling
+      instance.id,
+      instance.autoRestartPolicy,
+      instance.ncpus,
+      instance.memory,
+      getSnapshotAction,
+      bootDisks,
+    ]
   )
 
   const attachDisk = useApiMutation('instanceDiskAttach', {
diff --git a/test/e2e/instance-disks.e2e.ts b/test/e2e/instance-disks.e2e.ts
index b53e872e4d..f12c24ea4d 100644
--- a/test/e2e/instance-disks.e2e.ts
+++ b/test/e2e/instance-disks.e2e.ts
@@ -7,12 +7,14 @@
  */
 import {
   clickRowAction,
+  closeToast,
   expect,
   expectNoToast,
   expectNotVisible,
   expectRowVisible,
   expectToast,
   expectVisible,
+  openRowActions,
   stopInstance,
   test,
 } from './utils'
@@ -224,3 +226,45 @@ test('Change boot disk', async ({ page }) => {
 
   await expect(page.getByText('Attach a disk to be able to set a boot disk')).toBeVisible()
 })
+
+// silly test but we've reintroduced this bug like 3 times
+test("polling doesn't close row actions menu", async ({ page }) => {
+  await page.goto('/projects/mock-project/instances/db1')
+
+  // stop, but don't wait until the state has changed
+  await page.getByRole('button', { name: 'Stop' }).click()
+  await page.getByRole('button', { name: 'Confirm' }).click()
+  await closeToast(page)
+
+  const menu = page.getByRole('menu')
+  const stopped = page.getByText('statestopped')
+
+  await expect(menu).toBeHidden()
+  await expect(stopped).toBeHidden()
+
+  await openRowActions(page, 'disk-1')
+  await expect(stopped).toBeHidden() // still not stopped yet
+  await expect(menu).toBeVisible()
+
+  // now we're stopped, which means polling has happened, but the
+  // menu remains visible
+  await expect(stopped).toBeVisible()
+  await expect(menu).toBeVisible()
+
+  // now start it so we can check the non-boot disks table
+  await page.getByRole('button', { name: 'Start' }).click()
+  await page.getByRole('button', { name: 'Confirm' }).click()
+  await closeToast(page)
+
+  const running = page.getByText('staterunning') // not running yet
+  await expect(running).toBeHidden()
+  await expect(menu).toBeHidden()
+
+  await openRowActions(page, 'disk-2')
+  await expect(running).toBeHidden() // still not running yet
+  await expect(menu).toBeVisible()
+
+  // state change means polling has happened. menu is still visible
+  await expect(running).toBeVisible()
+  await expect(menu).toBeVisible()
+})
diff --git a/test/e2e/instance.e2e.ts b/test/e2e/instance.e2e.ts
index 338e2392f2..25e9f1bed2 100644
--- a/test/e2e/instance.e2e.ts
+++ b/test/e2e/instance.e2e.ts
@@ -240,3 +240,7 @@ test('instance table', async ({ page }) => {
     state: expect.stringMatching(/^starting\d+s$/),
   })
 })
+
+test("polling doesn't close row actions menu", async ({ page }) => {
+  await page.goto('/projects/mock-project/instances/db1')
+})