Submit selection to command on unmount of selection arg input (#7047)

* Submit selection to command on unmount of selection arg input

This fixes #7024 by saving the user's selection to the command even if
they click another argument in the command palette's header. It does no
harm to save the selection to the argument, even if it's being torn down
because the user dismissed it has no negative effect.

* Refactor to not auto-submit before selection is cleared on mount

Thanks E2E test suite

* Update failing E2E tests with new behavior, which allows skip with preselection

---------

Co-authored-by: Pierre Jacquier <pierrejacquier39@gmail.com>

e2e/playwright/command-bar-tests.spec.ts

@@ -38,20 +38,22 @@ test.describe('Command bar tests', () => {
     // Click the line of code for xLine.
     await page.getByText(`startProfile(at = [-10, -10])`).click()
 
+    // Wait for the selection to register (TODO: we need a definitive way to wait for this)
+    await page.waitForTimeout(200)
+
     await toolbar.extrudeButton.click()
     await cmdBar.expectState({
       stage: 'arguments',
       commandName: 'Extrude',
-      currentArgKey: 'sketches',
+      currentArgKey: 'length',
-      currentArgValue: '',
+      currentArgValue: '5',
       headerArguments: {
-        Profiles: '',
+        Profiles: '1 profile',
         Length: '',
       },
-      highlightedHeaderArg: 'Profiles',
+      highlightedHeaderArg: 'length',
     })
     await cmdBar.progressCmdBar()
-    await cmdBar.progressCmdBar()
     await cmdBar.expectState({
       stage: 'review',
       commandName: 'Extrude',

e2e/playwright/editor-tests.spec.ts

@@ -1131,8 +1131,9 @@ sketch001 = startSketchOn(XZ)
     await page.waitForTimeout(100)
 
     await page.getByText('startProfile(at = [4.61, -14.01])').click()
+    // Wait for the selection to register (TODO: we need a definitive way to wait for this)
+    await page.waitForTimeout(200)
     await toolbar.extrudeButton.click()
-    await cmdBar.progressCmdBar()
     await cmdBar.expectState({
       stage: 'arguments',
       currentArgKey: 'length',
@@ -1354,7 +1355,9 @@ sketch001 = startSketchOn(XZ)
       const u = await getUtils(page)
       const projectLink = page.getByRole('link', { name: 'cube' })
       const gizmo = page.locator('[aria-label*=gizmo]')
-      const resetCameraButton = page.getByRole('button', { name: 'Reset view' })
+      const resetCameraButton = page.getByRole('button', {
+        name: 'Reset view',
+      })
       const locationToHaveColor = async (
         position: { x: number; y: number },
         color: [number, number, number]

e2e/playwright/point-click.spec.ts

@@ -74,15 +74,6 @@ test.describe('Point-and-click tests', () => {
 
     await test.step('do extrude flow and check extrude code is added to editor', async () => {
       await toolbar.extrudeButton.click()
-      await cmdBar.expectState({
-        stage: 'arguments',
-        currentArgKey: 'sketches',
-        currentArgValue: '',
-        headerArguments: { Profiles: '', Length: '' },
-        highlightedHeaderArg: 'Profiles',
-        commandName: 'Extrude',
-      })
-      await cmdBar.progressCmdBar()
       await cmdBar.expectState({
         stage: 'arguments',
         currentArgKey: 'length',
@@ -1654,15 +1645,6 @@ sketch002 = startSketchOn(plane001)
 
         await test.step(`Go through the command bar flow with preselected sketches`, async () => {
           await toolbar.loftButton.click()
-          await cmdBar.expectState({
-            stage: 'arguments',
-            currentArgKey: 'sketches',
-            currentArgValue: '',
-            headerArguments: { Profiles: '' },
-            highlightedHeaderArg: 'Profiles',
-            commandName: 'Loft',
-          })
-          await cmdBar.progressCmdBar()
           await cmdBar.expectState({
             stage: 'review',
             headerArguments: { Profiles: '2 profiles' },
@@ -2106,18 +2088,6 @@ extrude001 = extrude(sketch001, length = -12)
     await test.step(`Apply fillet to the preselected edge`, async () => {
       await page.waitForTimeout(100)
       await toolbar.filletButton.click()
-      await cmdBar.expectState({
-        commandName: 'Fillet',
-        highlightedHeaderArg: 'selection',
-        currentArgKey: 'selection',
-        currentArgValue: '',
-        headerArguments: {
-          Selection: '',
-          Radius: '',
-        },
-        stage: 'arguments',
-      })
-      await cmdBar.progressCmdBar()
       await cmdBar.expectState({
         commandName: 'Fillet',
         highlightedHeaderArg: 'radius',
@@ -2647,18 +2617,6 @@ extrude001 = extrude(profile001, length = 5)
       await test.step(`Apply fillet`, async () => {
         await page.waitForTimeout(100)
         await toolbar.filletButton.click()
-        await cmdBar.expectState({
-          commandName: 'Fillet',
-          highlightedHeaderArg: 'selection',
-          currentArgKey: 'selection',
-          currentArgValue: '',
-          headerArguments: {
-            Selection: '',
-            Radius: '',
-          },
-          stage: 'arguments',
-        })
-        await cmdBar.progressCmdBar()
         await cmdBar.expectState({
           commandName: 'Fillet',
           highlightedHeaderArg: 'radius',
@@ -2764,19 +2722,6 @@ extrude001 = extrude(sketch001, length = -12)
     await test.step(`Apply chamfer to the preselected edge`, async () => {
       await page.waitForTimeout(100)
       await toolbar.chamferButton.click()
-      await cmdBar.expectState({
-        commandName: 'Chamfer',
-        highlightedHeaderArg: 'selection',
-        currentArgKey: 'selection',
-        currentArgValue: '',
-        headerArguments: {
-          Selection: '',
-          Length: '',
-        },
-        stage: 'arguments',
-      })
-      await cmdBar.progressCmdBar()
-      await page.waitForTimeout(1000)
       await cmdBar.expectState({
         commandName: 'Chamfer',
         highlightedHeaderArg: 'length',
@@ -3260,8 +3205,6 @@ extrude001 = extrude(sketch001, length = 30)
         await test.step(`Go through the command bar flow with a preselected face (cap)`, async () => {
           await toolbar.shellButton.click()
           await cmdBar.progressCmdBar()
-          await page.waitForTimeout(500)
-          await cmdBar.progressCmdBar()
           await cmdBar.expectState({
             stage: 'review',
             headerArguments: {
@@ -3695,12 +3638,13 @@ tag=$rectangleSegmentC002,
       // revolve
       await editor.scrollToText(codeToSelection)
       await page.getByText(codeToSelection).click()
+      // Wait for the selection to register (TODO: we need a definitive way to wait for this)
+      await page.waitForTimeout(200)
       await toolbar.revolveButton.click()
       await cmdBar.progressCmdBar()
       await cmdBar.progressCmdBar()
       await cmdBar.progressCmdBar()
       await cmdBar.progressCmdBar()
-      await cmdBar.progressCmdBar()
 
       const newCodeToFind = `revolve001 = revolve(sketch002, angle = 360, axis = X)`
       expect(editor.expectEditor.toContain(newCodeToFind)).toBeTruthy()
@@ -4629,18 +4573,6 @@ path001 = startProfile(sketch001, at = [0, 0])
 
     await test.step('Go through command bar flow', async () => {
       await toolbar.extrudeButton.click()
-      await cmdBar.expectState({
-        stage: 'arguments',
-        currentArgKey: 'sketches',
-        currentArgValue: '',
-        headerArguments: {
-          Profiles: '',
-          Length: '',
-        },
-        highlightedHeaderArg: 'Profiles',
-        commandName: 'Extrude',
-      })
-      await cmdBar.progressCmdBar()
       await cmdBar.expectState({
         stage: 'arguments',
         currentArgKey: 'length',
@@ -4723,19 +4655,6 @@ path001 = startProfile(sketch001, at = [0, 0])
 
     await test.step('Go through command bar flow', async () => {
       await toolbar.sweepButton.click()
-      await cmdBar.expectState({
-        stage: 'arguments',
-        currentArgKey: 'sketches',
-        currentArgValue: '',
-        headerArguments: {
-          Profiles: '',
-          Path: '',
-          Sectional: '',
-        },
-        highlightedHeaderArg: 'Profiles',
-        commandName: 'Sweep',
-      })
-      await cmdBar.progressCmdBar()
       await cmdBar.expectState({
         stage: 'arguments',
         currentArgKey: 'path',
@@ -4820,19 +4739,6 @@ path001 = startProfile(sketch001, at = [0, 0])
     await test.step('Go through command bar flow', async () => {
       await toolbar.closePane('code')
       await toolbar.revolveButton.click()
-      await cmdBar.expectState({
-        stage: 'arguments',
-        currentArgKey: 'sketches',
-        currentArgValue: '',
-        headerArguments: {
-          Profiles: '',
-          AxisOrEdge: '',
-          Angle: '',
-        },
-        highlightedHeaderArg: 'Profiles',
-        commandName: 'Revolve',
-      })
-      await cmdBar.progressCmdBar()
       await cmdBar.expectState({
         stage: 'arguments',
         currentArgKey: 'axisOrEdge',

e2e/playwright/sketch-tests.spec.ts

@@ -1016,7 +1016,6 @@ profile001 = startProfile(sketch001, at = [${roundOff(scale * 69.6)}, ${roundOff
 
     // sketch selection should already have been made.
     // otherwise the cmdbar would be waiting for a selection.
-    await cmdBar.progressCmdBar()
     await cmdBar.expectState({
       stage: 'arguments',
       currentArgKey: 'length',

e2e/playwright/various.spec.ts

@@ -573,7 +573,6 @@ profile001 = startProfile(sketch002, at = [-12.34, 12.34])
   await expect(page.getByTestId('command-bar')).toBeVisible()
   await page.waitForTimeout(100)
 
-  await cmdBar.progressCmdBar()
   await cmdBar.progressCmdBar()
   await expect(page.getByText('Confirm Extrude')).toBeVisible()
   await cmdBar.progressCmdBar()

src/components/CommandBar/CommandBarSelectionInput.tsx

@@ -54,6 +54,7 @@ function CommandBarSelectionInput({
   const inputRef = useRef<HTMLInputElement>(null)
   const commandBarState = useCommandBarState()
   const [hasSubmitted, setHasSubmitted] = useState(false)
+  const [hasClearedSelection, setHasClearedSelection] = useState(false)
   const selection = useSelector(arg.machineActor, selectionSelector)
   const selectionsByType = useMemo(() => {
     return getSelectionCountByType(selection)
@@ -102,7 +103,7 @@ function CommandBarSelectionInput({
     if (canSubmitSelection && arg.skip && argValue === undefined) {
       handleSubmit()
     }
-  }, [canSubmitSelection])
+  }, [arg.name, canSubmitSelection])
 
   function handleChange() {
     inputRef.current?.focus()
@@ -139,7 +140,30 @@ function CommandBarSelectionInput({
           selectionType: 'singleCodeCursor',
         },
       })
-  }, [arg.clearSelectionFirst])
+    setHasClearedSelection(true)
+  }, [arg])
+
+  // Watch for outside teardowns of this component
+  // (such as clicking another argument in the command palette header)
+  // and quickly save the current selection if we can
+  useEffect(() => {
+    return () => {
+      const resolvedSelection: Selections | undefined = isArgRequired
+        ? selection
+        : selection || {
+            graphSelections: [],
+            otherSelections: [],
+          }
+
+      if (
+        !(arg.clearSelectionFirst && !hasClearedSelection) &&
+        canSubmitSelection &&
+        resolvedSelection
+      ) {
+        onSubmit(resolvedSelection)
+      }
+    }
+  }, [hasClearedSelection])
 
   // Set selection filter if needed, and reset it when the component unmounts
   useEffect(() => {

src/components/CommandBar/CommandBarSelectionMixedInput.tsx

@@ -63,7 +63,7 @@ export default function CommandBarSelectionMixedInput({
         setHasAutoSkipped(true)
       }
     }
-  }, [])
+  }, [arg.name])
 
   // Set selection filter if needed, and reset it when the component unmounts
   useEffect(() => {
@@ -71,6 +71,24 @@ export default function CommandBarSelectionMixedInput({
     return () => kclManager.defaultSelectionFilter(selection)
   }, [arg.selectionFilter])
 
+  // Watch for outside teardowns of this component
+  // (such as clicking another argument in the command palette header)
+  // and quickly save the current selection if we can
+  useEffect(() => {
+    return () => {
+      const resolvedSelection: Selections | undefined = isArgRequired
+        ? selection
+        : selection || {
+            graphSelections: [],
+            otherSelections: [],
+          }
+
+      if (canSubmitSelection && resolvedSelection) {
+        onSubmit(resolvedSelection)
+      }
+    }
+  }, [])
+
   function handleChange() {
     inputRef.current?.focus()
   }