Add edit flow for Parameters to Feature Tree (#7536)

* Add edit flow to named parameters, show their value in feature tree * Amend a feature tree test to include editing a parameter * Enforce disallowing "create new variable" in edit parameter flow * Add wrapping behavior! Sorry forgot to commit this * Update src/machines/commandBarMachine.ts Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> --------- Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
2025-06-19 11:14:37 -04:00
parent 9dd6e3e852
commit 23a01e86e6
8 changed files with 273 additions and 118 deletions
--- a/e2e/playwright/feature-tree-pane.spec.ts
+++ b/e2e/playwright/feature-tree-pane.spec.ts
@ -229,11 +229,12 @@ test.describe('Feature Tree pane', () => {
    const initialCode = `sketch001 = startSketchOn(XZ)
      |> circle(center = [0, 0], radius = 5)
      renamedExtrude = extrude(sketch001, length = ${initialInput})`
-    const newConstantName = 'length001'
-    const expectedCode = `${newConstantName} = 23
+    const newParameterName = 'length001'
+    const expectedCode = `${newParameterName} = 23
    sketch001 = startSketchOn(XZ)
      |> circle(center = [0, 0], radius = 5)
-            renamedExtrude = extrude(sketch001, length = ${newConstantName})`
+            renamedExtrude = extrude(sketch001, length = ${newParameterName})`
+    const editedParameterValue = '23 * 2'

    await context.folderSetupFn(async (dir) => {
      const testDir = join(dir, 'test-sample')
@ -279,7 +280,7 @@ test.describe('Feature Tree pane', () => {
      })
    })

-    await test.step('Add a named constant for distance argument and submit', async () => {
+    await test.step('Add a parameter for distance argument and submit', async () => {
      await expect(cmdBar.currentArgumentInput).toBeVisible()
      await cmdBar.variableCheckbox.click()
      await cmdBar.progressCmdBar()
@ -296,13 +297,43 @@ test.describe('Feature Tree pane', () => {
        highlightedCode: '',
        diagnostics: [],
        activeLines: [
-          `renamedExtrude = extrude(sketch001, length = ${newConstantName})`,
+          `renamedExtrude = extrude(sketch001, length = ${newParameterName})`,
        ],
      })
      await editor.expectEditor.toContain(expectedCode, {
        shouldNormalise: true,
      })
    })
+
+    await test.step('Edit the parameter via the feature tree', async () => {
+      const parameter = await toolbar.getFeatureTreeOperation('Parameter', 0)
+      await parameter.dblclick()
+      await cmdBar.expectState({
+        commandName: 'Edit parameter',
+        currentArgKey: 'value',
+        currentArgValue: '23',
+        headerArguments: {
+          Name: newParameterName,
+          Value: '23',
+        },
+        stage: 'arguments',
+        highlightedHeaderArg: 'value',
+      })
+      await cmdBar.argumentInput
+        .locator('[contenteditable]')
+        .fill(editedParameterValue)
+      await cmdBar.progressCmdBar()
+      await cmdBar.expectState({
+        stage: 'review',
+        commandName: 'Edit parameter',
+        headerArguments: {
+          Name: newParameterName,
+          Value: '46', // Shows calculated result
+        },
+      })
+      await cmdBar.progressCmdBar()
+      await editor.expectEditor.toContain(editedParameterValue)
+    })
  })
  test(`User can edit an offset plane operation from the feature tree`, async ({
    context,
--- a/src/components/CommandBar/CommandBarKclInput.tsx
+++ b/src/components/CommandBar/CommandBarKclInput.tsx
@ -286,65 +286,67 @@ function CommandBarKclInput({
          )}
        </span>
      </label>
-      <div className="flex items-baseline gap-4 mx-4">
-        <input
-          type="checkbox"
-          id="variable-checkbox"
-          data-testid="cmd-bar-variable-checkbox"
-          checked={createNewVariable}
-          onChange={(e) => {
-            setCreateNewVariable(e.target.checked)
-          }}
-          className="bg-chalkboard-10 dark:bg-chalkboard-80"
-        />
-        <label
-          htmlFor="variable-checkbox"
-          className="text-blue border-none bg-transparent font-sm flex gap-1 items-center pl-0 pr-1"
-        >
-          Create new variable
-        </label>
-        {createNewVariable && (
-          <>
-            <input
-              type="text"
-              id="variable-name"
-              name="variable-name"
-              className="flex-1  border-solid border-0 border-b border-chalkboard-50 bg-transparent focus:outline-none"
-              placeholder="Variable name"
-              value={newVariableName}
-              autoCapitalize="off"
-              autoCorrect="off"
-              autoComplete="off"
-              spellCheck="false"
-              autoFocus
-              onChange={(e) => setNewVariableName(e.target.value)}
-              onKeyDown={(e) => {
-                if (
-                  e.currentTarget.value === '' &&
-                  e.key === 'Backspace' &&
-                  arg.createVariable !== 'force'
-                ) {
-                  setCreateNewVariable(false)
+      {arg.createVariable !== 'disallow' && (
+        <div className="flex items-baseline gap-4 mx-4">
+          <input
+            type="checkbox"
+            id="variable-checkbox"
+            data-testid="cmd-bar-variable-checkbox"
+            checked={createNewVariable}
+            onChange={(e) => {
+              setCreateNewVariable(e.target.checked)
+            }}
+            className="bg-chalkboard-10 dark:bg-chalkboard-80"
+          />
+          <label
+            htmlFor="variable-checkbox"
+            className="text-blue border-none bg-transparent font-sm flex gap-1 items-center pl-0 pr-1"
+          >
+            Create new variable
+          </label>
+          {createNewVariable && (
+            <>
+              <input
+                type="text"
+                id="variable-name"
+                name="variable-name"
+                className="flex-1  border-solid border-0 border-b border-chalkboard-50 bg-transparent focus:outline-none"
+                placeholder="Variable name"
+                value={newVariableName}
+                autoCapitalize="off"
+                autoCorrect="off"
+                autoComplete="off"
+                spellCheck="false"
+                autoFocus
+                onChange={(e) => setNewVariableName(e.target.value)}
+                onKeyDown={(e) => {
+                  if (
+                    e.currentTarget.value === '' &&
+                    e.key === 'Backspace' &&
+                    arg.createVariable !== 'force'
+                  ) {
+                    setCreateNewVariable(false)
+                  }
+                }}
+                onKeyUp={(e) => {
+                  if (e.key === 'Enter' && canSubmit) {
+                    handleSubmit()
+                  }
+                }}
+              />
+              <span
+                className={
+                  isNewVariableNameUnique
+                    ? 'text-succeed-60 dark:text-succeed-40'
+                    : 'text-destroy-60 dark:text-destroy-40'
                }
-              }}
-              onKeyUp={(e) => {
-                if (e.key === 'Enter' && canSubmit) {
-                  handleSubmit()
-                }
-              }}
-            />
-            <span
-              className={
-                isNewVariableNameUnique
-                  ? 'text-succeed-60 dark:text-succeed-40'
-                  : 'text-destroy-60 dark:text-destroy-40'
-              }
-            >
-              {isNewVariableNameUnique ? 'Available' : 'Unavailable'}
-            </span>
-          </>
-        )}
-      </div>
+              >
+                {isNewVariableNameUnique ? 'Available' : 'Unavailable'}
+              </span>
+            </>
+          )}
+        </div>
+      )}
    </form>
  )
 }
--- a/src/components/ModelingSidebar/ModelingPanes/FeatureTreePane.tsx
+++ b/src/components/ModelingSidebar/ModelingPanes/FeatureTreePane.tsx
@ -4,7 +4,7 @@ import type { ComponentProps } from 'react'
 import { useCallback, useEffect, useMemo, useRef } from 'react'
 import type { Actor, Prop } from 'xstate'

-import type { Operation } from '@rust/kcl-lib/bindings/Operation'
+import type { Operation, OpKclValue } from '@rust/kcl-lib/bindings/Operation'

 import { ContextMenu, ContextMenuItem } from '@src/components/ContextMenu'
 import type { CustomIconName } from '@src/components/CustomIcon'
@ -241,6 +241,7 @@ const OperationItemWrapper = ({
  name,
  variableName,
  visibilityToggle,
+  valueDetail,
  menuItems,
  errors,
  customSuffix,
@ -252,6 +253,7 @@ const OperationItemWrapper = ({
  name: string
  variableName?: string
  visibilityToggle?: VisibilityToggleProps
+  valueDetail?: { calculated: OpKclValue; display: string }
  customSuffix?: JSX.Element
  menuItems?: ComponentProps<typeof ContextMenu>['items']
  errors?: Diagnostic[]
@ -266,19 +268,24 @@ const OperationItemWrapper = ({
    >
      <button
        {...props}
-        className={`reset flex-1 flex items-center gap-2 text-left text-base ${selectable ? 'border-transparent dark:border-transparent' : 'border-none cursor-default'} ${className}`}
+        className={`reset !py-0.5 !px-1 flex-1 flex items-center gap-2 text-left text-base ${selectable ? 'border-transparent dark:border-transparent' : 'border-none cursor-default'} ${className}`}
      >
        <CustomIcon name={icon} className="w-5 h-5 block" />
-        <div className="flex items-baseline align-baseline">
-          <div className="mr-2">
+        <div className="flex flex-1 items-baseline align-baseline">
+          <div className="flex-1 inline-flex items-baseline flex-wrap gap-x-2">
            {name}
            {variableName && (
-              <span className="ml-2 opacity-50 text-[11px] font-semibold">
+              <span className="text-chalkboard-70 dark:text-chalkboard-40 text-xs">
                {variableName}
              </span>
            )}
+            {customSuffix && customSuffix}
          </div>
-          {customSuffix && customSuffix}
+          {valueDetail && (
+            <code className="px-1 text-right text-chalkboard-70 dark:text-chalkboard-40 text-xs">
+              {valueDetail.display}
+            </code>
+          )}
        </div>
      </button>
      {errors && errors.length > 0 && (
@ -302,6 +309,19 @@ const OperationItem = (props: {
 }) => {
  const kclContext = useKclContext()
  const name = getOperationLabel(props.item)
+  const valueDetail = useMemo(
+    () =>
+      props.item.type === 'VariableDeclaration'
+        ? {
+            display: kclContext.code.slice(
+              props.item.sourceRange[0],
+              props.item.sourceRange[1]
+            ),
+            calculated: props.item.value,
+          }
+        : undefined,
+    [props.item, kclContext.code]
+  )

  const variableName = useMemo(() => {
    return getOperationVariableName(props.item, kclContext.ast)
@ -334,7 +354,10 @@ const OperationItem = (props: {
   * TODO: https://github.com/KittyCAD/modeling-app/issues/4442
   */
  function enterEditFlow() {
-    if (props.item.type === 'StdLibCall') {
+    if (
+      props.item.type === 'StdLibCall' ||
+      props.item.type === 'VariableDeclaration'
+    ) {
      props.send({
        type: 'enterEditFlow',
        data: {
@ -449,15 +472,25 @@ const OperationItem = (props: {
            </ContextMenuItem>,
          ]
        : []),
-      ...(props.item.type === 'StdLibCall'
+      ...(props.item.type === 'StdLibCall' ||
+      props.item.type === 'VariableDeclaration'
        ? [
            <ContextMenuItem
-              disabled={!stdLibMap[props.item.name]?.prepareToEdit}
+              disabled={
+                !(
+                  stdLibMap[props.item.name]?.prepareToEdit ||
+                  props.item.type === 'VariableDeclaration'
+                )
+              }
              onClick={enterEditFlow}
              hotkey="Double click"
            >
              Edit
            </ContextMenuItem>,
+          ]
+        : []),
+      ...(props.item.type === 'StdLibCall'
+        ? [
            <ContextMenuItem
              disabled={!stdLibMap[props.item.name]?.supportsAppearance}
              onClick={enterAppearanceFlow}
@ -517,6 +550,7 @@ const OperationItem = (props: {
      icon={getOperationIcon(props.item)}
      name={name}
      variableName={variableName}
+      valueDetail={valueDetail}
      menuItems={menuItems}
      onClick={selectOperation}
      onDoubleClick={enterEditFlow}
--- a/src/lang/queryAstNodePathUtils.ts
+++ b/src/lang/queryAstNodePathUtils.ts
@ -149,6 +149,7 @@ function moreNodePathFromSourceRange(
        return moreNodePathFromSourceRange(init, sourceRange, path)
      }
    }
+    return path
  }
  if (_node.type === 'UnaryExpression' && isInRange) {
    const { argument } = _node
--- a/src/lib/commandBarConfigs/modelingCommandConfig.ts
+++ b/src/lib/commandBarConfigs/modelingCommandConfig.ts
@ -803,7 +803,9 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig<
        valueSummary: (nodeToEdit: PathToNode) => {
          const node = getNodeFromPath<VariableDeclarator>(
            kclManager.ast,
-            nodeToEdit
+            nodeToEdit,
+            'VariableDeclarator',
+            true
          )
          if (err(node) || node.node.type !== 'VariableDeclarator')
            return 'Error'
@ -993,16 +995,46 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig<
        inputType: 'options',
        required: true,
        options: [
-          { name: 'Red', value: '#FF0000' },
-          { name: 'Green', value: '#00FF00' },
-          { name: 'Blue', value: '#0000FF' },
-          { name: 'Turquoise', value: '#00FFFF' },
-          { name: 'Purple', value: '#FF00FF' },
-          { name: 'Yellow', value: '#FFFF00' },
-          { name: 'Black', value: '#000000' },
-          { name: 'Dark Grey', value: '#080808' },
-          { name: 'Light Grey', value: '#D3D3D3' },
-          { name: 'White', value: '#FFFFFF' },
+          {
+            name: 'Red',
+            value: '#FF0000',
+          },
+          {
+            name: 'Green',
+            value: '#00FF00',
+          },
+          {
+            name: 'Blue',
+            value: '#0000FF',
+          },
+          {
+            name: 'Turquoise',
+            value: '#00FFFF',
+          },
+          {
+            name: 'Purple',
+            value: '#FF00FF',
+          },
+          {
+            name: 'Yellow',
+            value: '#FFFF00',
+          },
+          {
+            name: 'Black',
+            value: '#000000',
+          },
+          {
+            name: 'Dark Grey',
+            value: '#080808',
+          },
+          {
+            name: 'Light Grey',
+            value: '#D3D3D3',
+          },
+          {
+            name: 'White',
+            value: '#FFFFFF',
+          },
          {
            name: 'Default (clear appearance)',
            value: COMMAND_APPEARANCE_COLOR_DEFAULT,
@ -1124,7 +1156,11 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig<
            KCL_DEFAULT_CONSTANT_PREFIXES.CLONE
          )
        },
-        validation: async ({ data }: { data: string }) => {
+        validation: async ({
+          data,
+        }: {
+          data: string
+        }) => {
          // Be conservative and error out if there is an item or module with the same name.
          const variableExists =
            kclManager.variables[data] || kclManager.variables['__mod_' + data]
--- a/src/lib/operations.ts
+++ b/src/lib/operations.ts
@ -62,6 +62,45 @@ interface StdLibCallInfo {
  supportsTransform?: boolean
 }

+/**
+ * Gather up the a Parameter operation's data
+ * to be used in the command bar edit flow.
+ */
+const prepareToEditParameter: PrepareToEditCallback = async ({ operation }) => {
+  if (operation.type !== 'VariableDeclaration') {
+    return { reason: 'Called on something not a variable declaration' }
+  }
+
+  const baseCommand = {
+    name: 'event.parameter.edit',
+    groupId: 'modeling',
+  }
+
+  // 1. Convert from the parameter's Operation to a KCL-type arg value
+  const value = await stringToKclExpression(
+    codeManager.code.slice(operation.sourceRange[0], operation.sourceRange[1])
+  )
+  if (err(value) || 'errors' in value) {
+    return { reason: "Couldn't retrieve length argument" }
+  }
+
+  // 2. The nodeToEdit is much simpler to transform.
+  // We need the VariableDeclarator PathToNode though, so we slice it
+  const nodeToEdit = pathToNodeFromRustNodePath(operation.nodePath).slice(0, -1)
+
+  // 3. Assemble the default argument values for the command,
+  // with `nodeToEdit` set, which will let the actor know
+  // to edit the node that corresponds to the StdLibCall.
+  const argDefaultValues: ModelingCommandSchema['event.parameter.edit'] = {
+    value,
+    nodeToEdit,
+  }
+  return {
+    ...baseCommand,
+    argDefaultValues,
+  }
+}
+
 /**
 * Gather up the argument values for the Extrude command
 * to be used in the command bar edit flow.
@ -1240,6 +1279,23 @@ export async function enterEditFlow({
  operation,
  artifact,
 }: EnterEditFlowProps): Promise<Error | CommandBarMachineEvent> {
+  // Operate on VariableDeclarations differently from StdLibCall's
+  if (operation.type === 'VariableDeclaration') {
+    const eventPayload = await prepareToEditParameter({
+      operation,
+    })
+
+    if ('reason' in eventPayload) {
+      return new Error(eventPayload.reason)
+    }
+
+    return {
+      type: 'Find and select command',
+      data: eventPayload,
+    }
+  }
+
+  // Begin StdLibCall processing
  if (operation.type !== 'StdLibCall') {
    return new Error(
      'Feature tree editing not yet supported for user-defined functions or modules. Please edit in the code editor.'
--- a/src/machines/commandBarMachine.ts
+++ b/src/machines/commandBarMachine.ts
@ -397,6 +397,7 @@ export const commandBarMachine = setup({
                    typeof o.value === 'object' &&
                    typeof argValue === 'object'
                  ) {
+                    
                    return JSON.stringify(o.value) === JSON.stringify(argValue)
                  } else {
                    return o.value === argValue
--- a/src/machines/modelingMachine.ts
+++ b/src/machines/modelingMachine.ts
@ -3794,12 +3794,12 @@ export const modelingMachine = setup({
        },

        'event.parameter.create': {
-          target: '#Modeling.parameter.creating',
+          target: '#Modeling.state:parameter:creating',
          guard: 'no kcl errors',
        },

        'event.parameter.edit': {
-          target: '#Modeling.parameter.editing',
+          target: '#Modeling.state:parameter:editing',
          guard: 'no kcl errors',
        },

@ -5206,39 +5206,33 @@ export const modelingMachine = setup({
      },
    },

-    parameter: {
-      type: 'parallel',
-
-      states: {
-        creating: {
-          invoke: {
-            src: 'actor.parameter.create',
-            id: 'actor.parameter.create',
-            input: ({ event }) => {
-              if (event.type !== 'event.parameter.create') return undefined
-              return event.data
-            },
-            onDone: ['#Modeling.idle'],
-            onError: {
-              target: '#Modeling.idle',
-              actions: 'toastError',
-            },
-          },
+    'state:parameter:creating': {
+      invoke: {
+        src: 'actor.parameter.create',
+        id: 'actor.parameter.create',
+        input: ({ event }) => {
+          if (event.type !== 'event.parameter.create') return undefined
+          return event.data
        },
-        editing: {
-          invoke: {
-            src: 'actor.parameter.edit',
-            id: 'actor.parameter.edit',
-            input: ({ event }) => {
-              if (event.type !== 'event.parameter.edit') return undefined
-              return event.data
-            },
-            onDone: ['#Modeling.idle'],
-            onError: {
-              target: '#Modeling.idle',
-              actions: 'toastError',
-            },
-          },
+        onDone: ['#Modeling.idle'],
+        onError: {
+          target: '#Modeling.idle',
+          actions: 'toastError',
+        },
+      },
+    },
+    'state:parameter:editing': {
+      invoke: {
+        src: 'actor.parameter.edit',
+        id: 'actor.parameter.edit',
+        input: ({ event }) => {
+          if (event.type !== 'event.parameter.edit') return undefined
+          return event.data
+        },
+        onDone: ['#Modeling.idle'],
+        onError: {
+          target: '#Modeling.idle',
+          actions: 'toastError',
        },
      },
    },