Make Modify with Text-to-CAD selection arg optional by honoring skip: false on non-required args (#6992)

* Make "skip = false" non-required args appear in header

* Make non-required, unskippable selection args work

* Make prompt-to-edit's selection arg optional but non-skippable

* Update src/components/CommandBar/CommandBarSelectionInput.tsx

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

* Fix dumb logic bug

Thanks for user testing @Irev-dev

* Update mixed input to show selection

Feel free to revert @Irev-Dev if this is the wrong move, but I found it
odd that this component doesn't show the current selection in the text
like the other selection input does, so I copied that over.

* Merge branch 'main' into franknoirot/adhoc/optional-selection-args

* Merge branch 'main' into franknoirot/adhoc/optional-selection-args

* Merge remote-tracking branch 'origin' into franknoirot/adhoc/optional-selection-args

* fix tests

* change copy again

* Update src/components/CommandBar/CommandBarSelectionMixedInput.tsx

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

e2e/playwright/prompt-to-edit-snapshot-tests.spec.ts

@@ -99,6 +99,8 @@ test.describe('edit with AI example snapshots', () => {
       await test.step('fire off edit prompt', async () => {
         await cmdBar.captureTextToCadRequestSnapshot(test.info())
         await cmdBar.openCmdBar('promptToEdit')
+        await page.waitForTimeout(100)
+        await cmdBar.progressCmdBar()
         // being specific about the color with a hex means asserting pixel color is more stable
         await page
           .getByTestId('cmd-bar-arg-value')

e2e/playwright/prompt-to-edit.spec.ts

@@ -88,6 +88,8 @@ test.describe('Prompt-to-edit tests', () => {
 
         await test.step('fire off edit prompt', async () => {
           await cmdBar.openCmdBar('promptToEdit')
+          await page.waitForTimeout(100)
+          await cmdBar.progressCmdBar()
           // being specific about the color with a hex means asserting pixel color is more stable
           await page
             .getByTestId('cmd-bar-arg-value')
@@ -165,6 +167,8 @@ test.describe('Prompt-to-edit tests', () => {
 
     await test.step('fire of bad prompt', async () => {
       await cmdBar.openCmdBar('promptToEdit')
+      await page.waitForTimeout(100)
+      await cmdBar.progressCmdBar()
       await page
         .getByTestId('cmd-bar-arg-value')
         .fill('ansheusha asnthuatshoeuhtaoetuhthaeu laughs in dvorak')

src/components/CommandBar/CommandBarHeader.tsx

@@ -102,10 +102,12 @@ function CommandBarHeader({ children }: React.PropsWithChildren<object>) {
               )}
             </p>
             {Object.entries(nonHiddenArgs || {})
-              .filter(([_, argConfig]) =>
+              .filter(
-                typeof argConfig.required === 'function'
+                ([_, argConfig]) =>
+                  argConfig.skip === false ||
+                  (typeof argConfig.required === 'function'
                     ? argConfig.required(commandBarState.context)
-                  : argConfig.required
+                    : argConfig.required)
               )
               .map(([argName, arg], i) => {
                 const argValue =

src/components/CommandBar/CommandBarSelectionInput.tsx

@@ -8,6 +8,7 @@ import {
   canSubmitSelectionArg,
   getSelectionCountByType,
   getSelectionTypeDisplayText,
+  type Selections,
 } from '@src/lib/selections'
 import { engineCommandManager, kclManager } from '@src/lib/singletons'
 import { reportRejection } from '@src/lib/trap'
@@ -56,9 +57,13 @@ function CommandBarSelectionInput({
   const selectionsByType = useMemo(() => {
     return getSelectionCountByType(selection)
   }, [selection])
+  const isArgRequired =
+    arg.required instanceof Function
+      ? arg.required(commandBarState.context)
+      : arg.required
   const canSubmitSelection = useMemo<boolean>(
-    () => canSubmitSelectionArg(selectionsByType, arg),
+    () => !isArgRequired || canSubmitSelectionArg(selectionsByType, arg),
-    [selectionsByType]
+    [selectionsByType, arg, isArgRequired]
   )
 
   useEffect(() => {
@@ -110,7 +115,18 @@ function CommandBarSelectionInput({
       return
     }
 
-    onSubmit(selection)
+    /**
+     * Now that arguments like this can be optional, we need to
+     * construct an empty selection if it's not required to get it past our validation.
+     */
+    const resolvedSelection: Selections | undefined = isArgRequired
+      ? selection
+      : selection || {
+          graphSelections: [],
+          otherSelections: [],
+        }
+
+    onSubmit(resolvedSelection)
   }
 
   // Clear selection if needed

src/components/CommandBar/CommandBarSelectionMixedInput.tsx

@@ -6,6 +6,7 @@ import type { Selections } from '@src/lib/selections'
 import {
   canSubmitSelectionArg,
   getSelectionCountByType,
+  getSelectionTypeDisplayText,
 } from '@src/lib/selections'
 import { kclManager } from '@src/lib/singletons'
 import { commandBarActor, useCommandBarState } from '@src/lib/singletons'
@@ -30,8 +31,14 @@ export default function CommandBarSelectionMixedInput({
   const selectionsByType = useMemo(() => {
     return getSelectionCountByType(selection)
   }, [selection])
+  const isArgRequired =
+    arg.required instanceof Function
+      ? arg.required(commandBarState.context)
+      : arg.required
 
   const canSubmitSelection = useMemo<boolean>(() => {
+    // Don't do additional checks if this argument is not required
+    if (!isArgRequired) return true
     if (!selection) return false
     const isNonZeroRange = selection.graphSelections.some((sel) => {
       const range = sel.codeRef.range
@@ -39,7 +46,7 @@ export default function CommandBarSelectionMixedInput({
     })
     if (isNonZeroRange) return true
     return canSubmitSelectionArg(selectionsByType, arg)
-  }, [selectionsByType, selection])
+  }, [selectionsByType, selection, arg, isArgRequired])
 
   useEffect(() => {
     inputRef.current?.focus()
@@ -76,7 +83,18 @@ export default function CommandBarSelectionMixedInput({
       return
     }
 
-    onSubmit(selection)
+    /**
+     * Now that arguments like this can be optional, we need to
+     * construct an empty selection if it's not required to get it past our validation.
+     */
+    const resolvedSelection: Selections | undefined = isArgRequired
+      ? selection
+      : selection || {
+          graphSelections: [],
+          otherSelections: [],
+        }
+
+    onSubmit(resolvedSelection)
   }
 
   const isMixedSelection = arg.inputType === 'selectionMixed'
@@ -92,9 +110,10 @@ export default function CommandBarSelectionMixedInput({
           (!hasSubmitted || canSubmitSelection || 'text-destroy-50')
         }
       >
-        {canSubmitSelection
+        {canSubmitSelection &&
-          ? 'Select objects in the scene'
+        (selection.graphSelections.length || selection.otherSelections.length)
-          : 'Select code or objects in the scene'}
+          ? getSelectionTypeDisplayText(selection) + ' selected'
+          : 'Select code/objects, or skip'}
 
         {showSceneSelection && (
           <div className="scene-selection mt-2">

src/lib/commandBarConfigs/modelingCommandConfig.ts

@@ -22,6 +22,7 @@ import {
   KCL_DEFAULT_DEGREE,
   KCL_DEFAULT_LENGTH,
   KCL_DEFAULT_TRANSFORM,
+  ML_EXPERIMENTAL_MESSAGE,
 } from '@src/lib/constants'
 import type { components } from '@src/lib/machine-api'
 import type { Selections } from '@src/lib/selections'
@@ -957,12 +958,13 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig<
           'edgeCutEdge',
         ],
         multiple: true,
-        required: true,
+        required: false,
         selectionSource: {
           allowSceneSelection: true,
           allowCodeSelection: true,
         },
-        skip: true,
+        skip: false,
+        warningMessage: ML_EXPERIMENTAL_MESSAGE,
       },
       prompt: {
         inputType: 'text',

src/machines/commandBarMachine.ts

@@ -146,8 +146,15 @@ export const commandBarMachine = setup({
             typeof argConfig.required === 'function'
               ? argConfig.required(context)
               : argConfig.required
+          /**
+           * TODO: we need to think harder about the relationship between
+           * `required`, `skip`, and `hidden`.
+           * This bit of logic essentially makes "skip false" arguments required.
+           * We may need a bit of state to mark an argument as "visited" for "skip false" args
+           * to truly not require any value to continue.
+           */
           const mustNotSkipArg =
-            argIsRequired &&
+            (argIsRequired || argConfig.skip === false) &&
             (!context.argumentsToSubmit.hasOwnProperty(argName) ||
               context.argumentsToSubmit[argName] === undefined ||
               (rejectedArg &&