Multi-profile sweeps and more robust edit flows in point-and-click by pierremtb · Pull Request #6437 · KittyCAD/modeling-app

pierremtb · 2025-04-22T19:00:38Z

This turned into a huge PR, sorry that wasn't the intent 😢

The goal of this refactor is to ensure a consistent order of operations across the different sweep calls and allow edit flows to be more robust to non-standard ways of creating these operations, namely:

support variable-assigned calls (already exisitng on main)
support standalone calls (not working on main, common in KCL Samples and AI-generated)
support in-pipe calls (not working on main, common in KCL Samples and AI-generated)

This includes the following codemods, and move their implementation in a modifyAst/addSweep.ts file along with utility functions:

Extrude
Revolve
Sweep
Loft (still no edit flow as it's selections only, but put into the mix for consistency)

This doesn't include Shell and others that have different argument flows and potentially need to be treated differently.

While I was building that, supporting multi-profile selections for this was straightforward to add so I took the chance to do it, since KCL supports it and it was an easy lift. This also ensures command bar argument to kcl argument naming consistency, biggest one being swapping selection for sketches.

Demo

Screenshare.-.2025-04-28.7_44_04.PM.mp4

…ions Fixes #6434

vercel · 2025-04-22T19:00:42Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 6, 2025 9:46pm

qa-wolf · 2025-04-22T19:00:43Z

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

codecov · 2025-04-22T19:22:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.35%. Comparing base (4d2bc18) to head (5c766a2).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6437      +/-   ##
==========================================
- Coverage   85.36%   85.35%   -0.02%     
==========================================
  Files         107      108       +1     
  Lines       47219    47355     +136     
==========================================
+ Hits        40309    40420     +111     
- Misses       6910     6935      +25

Flag	Coverage Δ
rust	`85.35% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…dit-flows-to-non-variable-sweep-declarations

pierremtb · 2025-05-05T14:04:29Z

src/lib/commandBarConfigs/validators.ts

+  if (!isSelections(context.argumentsToSubmit.sketches)) {
    return 'Unable to revolve, selections are missing'
  }
+  // Gotcha: this validation only works for the first sketch


Didn't want to rotate on this any more than what was necessary to not break the existing, since dry run validations aren't really something we're sure have a future atm

pierremtb · 2025-05-05T14:05:57Z

src/lib/operations.ts

-    ) {
-      return baseCommand
-    }
+const prepareToEditExtrude: PrepareToEditCallback = async ({ operation }) => {


Cleaned up all three below with an inverse util function to go from the sketches in the operation object to graph selections. Trying to make things as consistent as possible but still allowing for specific needs of each

pierremtb · 2025-05-05T14:07:43Z

src/machines/modelingMachine.ts

-      }
-
-      const pathToNode = getNodePathFromSourceRange(
+      if (!input) return Promise.reject(new Error('No input provided'))


And finally modelingMachine.ts, the code mods here are much shorter and follow the same pattern for all four. Including the use of Promise.reject to make sure the errors aren't silent, which is something we haven't been consistent about in the past

unpack args

call function from addSweep

if no error, call updateModelingState

franknoirot

Wow so much cleanup along with these UX improvements. Big PR forgiven 😄

franknoirot · 2025-05-06T17:58:36Z

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

+    await toolbar.extrudeButton.click()
+    await cmdBar.expectState({
+      stage: 'arguments',
+      commandName: 'Extrude',
+      currentArgKey: 'sketches',
+      currentArgValue: '',
+      headerArguments: {
+        Sketches: '',
+        Length: '',
+      },
+      highlightedHeaderArg: 'sketches',
+    })
+    await cmdBar.progressCmdBar()
+    await cmdBar.progressCmdBar()
+    await cmdBar.expectState({
+      stage: 'review',
+      commandName: 'Extrude',
+      headerArguments: {
+        Sketches: '1 segment',
+        Length: '5',
+      },
+    })
+    await cmdBar.progressCmdBar()


franknoirot · 2025-05-06T18:48:38Z

src/lang/modifyAst/addSweep.ts

+      KCL_DEFAULT_CONSTANT_PREFIXES.EXTRUDE
+    )
+    const declaration = createVariableDeclaration(name, call)
+    modifiedAst.body.push(declaration)


Nah do it haha I'm sick of dumb "smart" insertion

franknoirot · 2025-05-06T18:49:05Z

src/lang/modifyAst/addSweep.ts

+import type { Selections } from '@src/lib/selections'
+import { err } from '@src/lib/trap'
+
+export function addExtrude({


These seem like they might be good as comments around the code itself as well.

franknoirot · 2025-05-06T18:54:26Z

src/lang/modifyAst/addSweep.ts

+  return sketchesExpr
+}
+
+function insertVariable(


For future reference there is already a very similar function in modifyAst called insertVariableAndOffsetPathToNode. Might be good to flag a TODO to unify since they both look pretty generic?

Oh good call I remember that now let me look quick

This works perfectly!

franknoirot · 2025-05-06T18:55:53Z

src/lang/queryAst.ts

@Irev-Dev might want eyes on these AST helpers here and in addSweep. I'm trying to get in the right headspace for reviewing them 😅

franknoirot · 2025-05-06T19:03:51Z

src/lib/commandBarConfigs/modelingCommandConfig.ts

+    // KCL stdlib arguments
+    sketches: Selections
+    path: Selections
+    sectional?: boolean


Nice we should make it one of the first truly optional arguments like these ones with the +s here

franknoirot · 2025-05-06T19:04:01Z

src/lib/commandBarConfigs/modelingCommandConfig.ts

+    // KCL stdlib arguments
+    sketches: Selections
+    path: Selections
+    sectional?: boolean


franknoirot · 2025-05-06T19:05:49Z

src/lib/commandBarConfigs/modelingCommandConfig.ts

-      //   })),
-      // },
-      distance: {
+      length: {


I 100% agree this is good though. One of our goals for the UX is for the point-and-click experience to provide a smooth learning curve into KCL and more "power user" things. Making things match across that divide is lowkey important for supporting that

franknoirot · 2025-05-06T19:07:39Z

src/lib/operations.ts

I really like this cleanup here

…dit-flows-to-non-variable-sweep-declarations

pierremtb

Thanks so much for reviewing @franknoirot! Made two changes based on your suggestions in addSweep about comments and using an existing utility. Let's see if I didn't break anything!

pierremtb · 2025-05-06T19:58:05Z

src/lang/modifyAst/addSweep.ts

+import type { Selections } from '@src/lib/selections'
+import { err } from '@src/lib/trap'
+
+export function addExtrude({


You're totally right, added them!

pierremtb · 2025-05-06T19:58:18Z

src/lang/modifyAst/addSweep.ts

+      KCL_DEFAULT_CONSTANT_PREFIXES.EXTRUDE
+    )
+    const declaration = createVariableDeclaration(name, call)
+    modifiedAst.body.push(declaration)


Alright hahaha let's do this

pierremtb · 2025-05-06T19:58:57Z

src/lang/modifyAst/addSweep.ts

+  return sketchesExpr
+}
+
+function insertVariable(


This works perfectly!

pierremtb · 2025-05-06T19:59:10Z

src/lib/commandBarConfigs/modelingCommandConfig.ts

+    // KCL stdlib arguments
+    sketches: Selections
+    path: Selections
+    sectional?: boolean


pierremtb · 2025-05-06T19:59:22Z

src/lib/commandBarConfigs/modelingCommandConfig.ts

-      //   })),
-      // },
-      distance: {
+      length: {


Sounds great!

pierremtb · 2025-05-06T19:59:37Z

src/lib/operations.ts

pierremtb · 2025-05-06T21:24:54Z

src/lang/modifyAst.ts

  variable: KclCommandValue,
  modifiedAst: Node<Program>,
-  pathToNode: PathToNode
+  pathToNode?: PathToNode


@franknoirot @Irev-Dev this way I can use it in addSweep.ts. Doesn't seem like a big change since we had the typeof check already

Irev-Dev · 2025-05-07T09:57:59Z

src/lang/queryAst.ts

+  nodeToEdit?: PathToNode
+): Error | Expr[] {
+  const sketches: Expr[] = selection.graphSelections.flatMap((s) => {
+    const path = getNodePathFromSourceRange(ast, s?.codeRef.range)


I think this fine for this PR, but we should probably get in the habit of using codeRef.pathToNode directly. As since #6632 it should be more reliable than range (range does not update under circumstances like comments or white space changes which can really mess things up)

But there's an argument for me or Jon to go through and banish ranges from the app entirely one we're able to come up for air, so just socialising it at this point.

Oh nice yeah, thank you! Might do a follow up if that's quick and easy

WIP: Extend point-and-click edit flows to non-variable sweep declarat…

acc6d41

…ions Fixes #6434

vercel bot deployed to Preview April 22, 2025 19:02 View deployment

Merge branch 'main' into pierremtb/issue6434-Extend-point-and-click-e…

5119a90

…dit-flows-to-non-variable-sweep-declarations

vercel bot deployed to Preview April 23, 2025 19:43 View deployment

Merge branch 'main' into pierremtb/issue6434-Extend-point-and-click-e…

2c5c59c

…dit-flows-to-non-variable-sweep-declarations

vercel bot deployed to Preview April 25, 2025 13:46 View deployment

Merge branch 'main' into pierremtb/issue6434-Extend-point-and-click-e…

56dbaa1

…dit-flows-to-non-variable-sweep-declarations

vercel bot deployed to Preview April 28, 2025 13:22 View deployment

Clean up

8a8678e

vercel bot deployed to Preview April 28, 2025 19:03 View deployment

Clean up more and break out to external hopefully testable function

7c9387d

vercel bot deployed to Preview April 28, 2025 19:32 View deployment

Apply the same principles to Revolve

4450402

vercel bot deployed to Preview April 28, 2025 20:01 View deployment

Add variable insertion for revolve and extrude

c312420

vercel bot deployed to Preview April 28, 2025 20:34 View deployment

More clean up ahead of sweep migration

1f205bb

vercel bot deployed to Preview April 28, 2025 21:00 View deployment

pierremtb and others added 3 commits April 28, 2025 19:21

Merge branch 'main' into pierremtb/issue6434-Extend-point-and-click-e…

f37c3ac

…dit-flows-to-non-variable-sweep-declarations

Fix more tests

f517bd3

Lint

6b0cdb5

vercel bot deployed to Preview April 28, 2025 23:46 View deployment

Remove skips on extrude and revolve

7cdb181

vercel bot deployed to Preview April 29, 2025 10:06 View deployment

Migrate sweep

8c7c145

vercel bot deployed to Preview April 29, 2025 11:58 View deployment

Fix a few tests

d756fc6

pierremtb marked this pull request as draft May 5, 2025 13:42

Clean up operations.ts

00d2140

pierremtb commented May 5, 2025

View reviewed changes

vercel bot deployed to Preview May 5, 2025 14:15 View deployment

pierremtb marked this pull request as ready for review May 5, 2025 14:15

pierremtb enabled auto-merge (squash) May 5, 2025 14:27

maxammann disabled auto-merge May 6, 2025 09:48

maxammann enabled auto-merge (squash) May 6, 2025 09:49

maxammann disabled auto-merge May 6, 2025 09:49

pierremtb mentioned this pull request May 6, 2025

Fix sketchOnFace point&click for booleans #6713

Merged

2 tasks

franknoirot approved these changes May 6, 2025

View reviewed changes

Merge branch 'main' into pierremtb/issue6434-Extend-point-and-click-e…

c939a9b

…dit-flows-to-non-variable-sweep-declarations

vercel bot deployed to Preview May 6, 2025 19:24 View deployment

pierremtb commented May 6, 2025

View reviewed changes

Add Frank's suggestions

7f060cb

vercel bot deployed to Preview May 6, 2025 20:13 View deployment

Fix up some messssss

6bb8293

vercel bot deployed to Preview May 6, 2025 20:40 View deployment

pierremtb added 2 commits May 6, 2025 17:21

Last fix hopefully

2d1410e

Lint

a9cf5ec

pierremtb commented May 6, 2025

View reviewed changes

vercel bot deployed to Preview May 6, 2025 21:46 View deployment

franknoirot merged commit 996517f into main May 6, 2025
46 checks passed

franknoirot deleted the pierremtb/issue6434-Extend-point-and-click-edit-flows-to-non-variable-sweep-declarations branch May 6, 2025 21:57

Irev-Dev reviewed May 7, 2025

View reviewed changes

max-mrgrsk mentioned this pull request May 8, 2025

Extend point-and-click edit flow to non-pipe Chamfer and Fillet #6767

Merged

This was referenced May 12, 2025

cant "enter" thru extrusion, accessibility/ease of use problem #4851

Open

Update getSketchExprsFromSelection to use codeRef.pathToNode #6737

Merged

Comments

Conversation

pierremtb commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qa-wolf bot commented Apr 22, 2025

Uh oh!

codecov bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierremtb May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franknoirot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierremtb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pierremtb commented Apr 22, 2025 •

edited

Loading

vercel bot commented Apr 22, 2025 •

edited

Loading

codecov bot commented Apr 22, 2025 •

edited

Loading

pierremtb May 5, 2025 •

edited

Loading