Fix argument persistence and accidental submission command bar bugs (#…

…3021)
KittyCAD · Jul 14, 2024 · 1852e61 · 1852e61
1 parent 29bf77b
commit 1852e61
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 36 deletions.
diff --git a/e2e/playwright/flow-tests.spec.ts b/e2e/playwright/flow-tests.spec.ts
@@ -3500,11 +3500,24 @@ test.describe('Command bar tests', () => {
       `const extrude001 = extrude(${KCL_DEFAULT_LENGTH}, sketch001)`
     )
   })
-  test('Command bar works and can change a setting', async ({ page }) => {
+  test('Command bar can change a setting, and switch back and forth between arguments', async ({
+    page,
+  }) => {
     const u = await getUtils(page)
     await page.setViewportSize({ width: 1200, height: 500 })
     await u.waitForAuthSkipAppStart()
 
+    const commandBarButton = page.getByRole('button', { name: 'Commands' })
+    const cmdSearchBar = page.getByPlaceholder('Search commands')
+    const themeOption = page.getByRole('option', {
+      name: 'theme',
+      exact: false,
+    })
+    const commandLevelArgButton = page.getByRole('button', { name: 'level' })
+    const commandThemeArgButton = page.getByRole('button', { name: 'value' })
+    // This selector changes after we set the setting
+    let commandOptionInput = page.getByPlaceholder('Select an option')
+
     await expect(
       page.getByRole('button', { name: 'Start Sketch' })
     ).not.toBeDisabled()
@@ -3515,23 +3528,17 @@ test.describe('Command bar tests', () => {
       .or(page.getByRole('button', { name: '⌘K' }))
       .click()
 
-    let cmdSearchBar = page.getByPlaceholder('Search commands')
     await expect(cmdSearchBar).toBeVisible()
     await page.keyboard.press('Escape')
-    cmdSearchBar = page.getByPlaceholder('Search commands')
     await expect(cmdSearchBar).not.toBeVisible()
 
     // Now try the same, but with the keyboard shortcut, check focus
     await page.keyboard.press('Meta+K')
-    cmdSearchBar = page.getByPlaceholder('Search commands')
     await expect(cmdSearchBar).toBeVisible()
     await expect(cmdSearchBar).toBeFocused()
 
     // Try typing in the command bar
-    await page.keyboard.type('theme')
-    const themeOption = page.getByRole('option', {
-      name: 'Settings · app · theme',
-    })
+    await cmdSearchBar.fill('theme')
     await expect(themeOption).toBeVisible()
     await themeOption.click()
     const themeInput = page.getByPlaceholder('Select an option')
@@ -3553,6 +3560,24 @@ test.describe('Command bar tests', () => {
     ).toBeVisible()
     // Check that the theme changed
     await expect(page.locator('body')).not.toHaveClass(`body-bg dark`)
+
+    commandOptionInput = page.getByPlaceholder('system')
+
+    // Test case for https://github.com/KittyCAD/modeling-app/issues/2882
+    await commandBarButton.click()
+    await cmdSearchBar.focus()
+    await cmdSearchBar.fill('theme')
+    await themeOption.click()
+    await expect(commandThemeArgButton).toBeDisabled()
+    await commandOptionInput.focus()
+    await commandOptionInput.fill('lig')
+    await commandLevelArgButton.click()
+    await expect(commandLevelArgButton).toBeDisabled()
+
+    // Test case for https://github.com/KittyCAD/modeling-app/issues/2881
+    await commandThemeArgButton.click()
+    await expect(commandThemeArgButton).toBeDisabled()
+    await expect(commandLevelArgButton).toHaveText('level: project')
   })
 
   test('Command bar keybinding works from code editor and can change a setting', async ({
@@ -3577,7 +3602,7 @@ test.describe('Command bar tests', () => {
     await expect(cmdSearchBar).toBeFocused()
 
     // Try typing in the command bar
-    await page.keyboard.type('theme')
+    await cmdSearchBar.fill('theme')
     const themeOption = page.getByRole('option', {
       name: 'Settings · app · theme',
     })
@@ -3648,7 +3673,9 @@ test.describe('Command bar tests', () => {
     await page.mouse.click(700, 200)
 
     // Assert that we're on the distance step
-    await expect(page.getByRole('button', { name: 'distance' })).toBeDisabled()
+    await expect(
+      page.getByRole('button', { name: 'distance', exact: false })
+    ).toBeDisabled()
 
     // Assert that the an alternative variable name is chosen,
     // since the default variable name is already in use (distance)
@@ -3663,11 +3690,12 @@ test.describe('Command bar tests', () => {
 
     // Review step and argument hotkeys
     await expect(submitButton).toBeEnabled()
-    await page.keyboard.press('Backspace')
+    await expect(submitButton).toBeFocused()
+    await submitButton.press('Backspace')
 
     // Assert we're back on the distance step
     await expect(
-      page.getByRole('button', { name: 'Distance 5', exact: false })
+      page.getByRole('button', { name: 'distance', exact: false })
     ).toBeDisabled()
 
     await continueButton.click()
@@ -3724,6 +3752,7 @@ const extrude001 = extrude(distance001, sketch001)`.replace(
     // Click in the scene a couple times to draw a line
     // so tangential arc is valid
     await page.mouse.click(700, 200)
+    await page.mouse.move(700, 300, { steps: 5 })
     await page.mouse.click(700, 300)
 
     // switch to tangential arc via command bar
@@ -4682,10 +4711,10 @@ test.describe('Sketch tests', () => {
     // click extrude
     await page.getByRole('button', { name: 'Extrude' }).click()
 
-    // sketch selection should already have been made. "Selection 1 face" only show up when the selection has been made already
+    // sketch selection should already have been made. "Selection: 1 face" only show up when the selection has been made already
     // otherwise the cmdbar would be waiting for a selection.
     await expect(
-      page.getByRole('button', { name: 'Selection 1 face' })
+      page.getByRole('button', { name: 'selection : 1 face', exact: false })
     ).toBeVisible()
   })
   test("Existing sketch with bad code delete user's code", async ({ page }) => {

diff --git a/...ests.spec.ts-snapshots/Zoom-to-fit-on-load---solid-2d-1-Google-Chrome-linux.png b/...ests.spec.ts-snapshots/Zoom-to-fit-on-load---solid-2d-1-Google-Chrome-linux.png
diff --git a/src/components/CommandBar/CommandArgOptionInput.tsx b/src/components/CommandBar/CommandArgOptionInput.tsx
@@ -41,6 +41,7 @@ function CommandArgOptionInput({
   )
   const inputRef = useRef<HTMLInputElement>(null)
   const formRef = useRef<HTMLFormElement>(null)
+  const [shouldSubmitOnChange, setShouldSubmitOnChange] = useState(false)
   const [selectedOption, setSelectedOption] = useState<
     CommandArgumentOption<unknown>
   >(currentOption || resolvedOptions[0])
@@ -82,8 +83,10 @@ function CommandArgOptionInput({
     // We deal with the whole option object internally
     setSelectedOption(option)
 
-    // But we only submit the value
-    onSubmit(option.value)
+    // But we only submit the value itself
+    if (shouldSubmitOnChange) {
+      onSubmit(option.value)
+    }
   }
 
   function handleSubmit(e: React.FormEvent<HTMLFormElement>) {
@@ -94,7 +97,18 @@ function CommandArgOptionInput({
   }
 
   return (
-    <form id="arg-form" onSubmit={handleSubmit} ref={formRef}>
+    <form
+      id="arg-form"
+      onSubmit={handleSubmit}
+      ref={formRef}
+      onKeyDownCapture={(e) => {
+        if (e.key === 'Enter') {
+          setShouldSubmitOnChange(true)
+        } else {
+          setShouldSubmitOnChange(false)
+        }
+      }}
+    >
       <Combobox
         value={selectedOption}
         onChange={handleSelectOption}
@@ -118,6 +132,12 @@ function CommandArgOptionInput({
               if (event.key === 'Backspace' && !event.currentTarget.value) {
                 stepBack()
               }
+
+              if (event.key === 'Enter') {
+                setShouldSubmitOnChange(true)
+              } else {
+                setShouldSubmitOnChange(false)
+              }
             }}
             value={query}
             placeholder={
@@ -136,6 +156,9 @@ function CommandArgOptionInput({
         <Combobox.Options
           static
           className="overflow-y-auto max-h-96 cursor-pointer"
+          onMouseDown={() => {
+            setShouldSubmitOnChange(true)
+          }}
         >
           {filteredOptions?.map((option) => (
             <Combobox.Option

diff --git a/src/components/CommandBar/CommandBarHeader.tsx b/src/components/CommandBar/CommandBarHeader.tsx
@@ -114,6 +114,7 @@ function CommandBarHeader({ children }: React.PropsWithChildren<{}>) {
                     >
                       {argName}
                     </span>
+                    <span className="sr-only">:&nbsp;</span>
                     {argValue ? (
                       arg.inputType === 'selection' ? (
                         getSelectionTypeDisplayText(argValue as Selections)

diff --git a/src/machines/commandBarMachine.ts b/src/machines/commandBarMachine.ts
@@ -203,7 +203,7 @@ export const commandBarMachine = createMachine(
           'Change current argument': {
             target: 'Gathering arguments',
             internal: true,
-            actions: ['Remove current argument and set a new one'],
+            actions: ['Set current argument'],
           },
 
           'Deselect command': {
@@ -359,29 +359,13 @@ export const commandBarMachine = createMachine(
           switch (event.type) {
             case 'Edit argument':
               return event.data.arg
+            case 'Change current argument':
+              return Object.values(event.data)[0]
             default:
               return context.currentArgument
           }
         },
       }),
-      'Remove current argument and set a new one': assign({
-        argumentsToSubmit: (context, event) => {
-          if (
-            event.type !== 'Change current argument' ||
-            !context.currentArgument
-          )
-            return context.argumentsToSubmit
-          const { name } = context.currentArgument
-
-          const { [name]: _, ...rest } = context.argumentsToSubmit
-          return rest
-        },
-        currentArgument: (context, event) => {
-          if (event.type !== 'Change current argument')
-            return context.currentArgument
-          return Object.values(event.data)[0]
-        },
-      }),
       'Clear argument data': assign({
         selectedCommand: undefined,
         currentArgument: undefined,