Add point-and-click Insert from local project files by pierremtb · Pull Request #6129 · KittyCAD/modeling-app

pierremtb · 2025-04-03T11:17:58Z

Ok here we go this is the first step towards #4735, and is about importing existing parts in project as whole modules and executing as such. This is a first pass and does not include:

inserting the same part twice, waiting on Add clone #5462 for that;
loading a file from a different location on disk, I'd like this to be a separate flow that we don't confuse, potentially once we have both changing to 'Load & Insert' depending on the type. TBD
deleting the operation in the feature tree;
setting translate and rotate on the object;
all of which are layed out as separate subissues in Support basic assemblies in point-and-click UI #4735

Note that the lingo here is Insert, not Import. As the codemod suggests, inserting is about importing and the instantiating (in our case here executing the whole module), and is akin to what regular CAD software looks like for assemblies.

But it does include:

a sidebar button
a project menu and native file menu button
a command bar flow prefilled with importable files from the project with a field for the local alias
tests for all of that

Recording.2025-04-04.133516.mp4

Will eventually fix #6120 Right now the whole loop is there but the codemod doesn't work yet

qa-wolf · 2025-04-03T11:18:01Z

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!

vercel · 2025-04-03T11:18:03Z

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

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Apr 7, 2025 3:42pm

src/components/CustomIcon.tsx

src/machines/modelingMachine.ts

codecov · 2025-04-03T11:48:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.09%. Comparing base (df6f571) to head (a699e94).
Report is 11 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6129   +/-   ##
=======================================
  Coverage   85.09%   85.09%           
=======================================
  Files         110      110           
  Lines       44684    44684           
=======================================
  Hits        38024    38024           
  Misses       6660     6660

Flag	Coverage Δ
rust	`85.09% <ø> (ø)`

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.

…rt-for-geometry

pierremtb · 2025-04-04T17:39:09Z

src/components/FileMachineProvider.tsx

        (command) => kclSamples.length || command.name !== 'open-kcl-example'
      ),
-    [codeManager, kclManager, send, kclSamples]
+    [codeManager, kclManager, send, kclSamples, project, file]


This right here is the reason why this command was added in kclCommands and not in modelingMachine as it was at first, since I wanted the path input to be prefilled with project files which is a lot easier and faster to navigate than a full blown filesystem dialog. But I didn't really know how to nicely inject thse dependencies into the modelingMachine, so I went this way instead. Feedback on this super welcome!

@nadr0 is in the process of excavating the projectMachine out of React. When that is done, then the inputs will be available anywhere the appMachine is, and it should be easier to get into a modeling command definition. But I think this is understandable as an approach!

Ok phew that's awesome!

pierremtb · 2025-04-04T17:40:45Z

src/lang/modelingWorkflows.ts

+  // TODO: understand why this skip flag is needed for insertAstMod.
+  // It's unclear why we double casts the AST


This took a lot of time from a few of us to debug. Thanks a lot to @franknoirot @nadr0 @lee-at-zoo-corp

The solution below is not perfect but helps unblocking us without changing the code path too much.

pierremtb · 2025-04-04T17:41:38Z

src/lib/kclCommands.ts

+      onSubmit: (data) => {
+        if (!data) {
+          return new Error('No input provided')
+        }
+
+        const ast = kclManager.ast
+        const { path, localName } = data
+        const { modifiedAst, pathToImportNode, pathToInsertNode } =
+          addImportAndInsert({
+            node: ast,
+            path,
+            localName,
+          })
+        updateModelingState(
+          modifiedAst,
+          EXECUTION_TYPE_REAL,
+          { kclManager, editorManager, codeManager },
+          {
+            skipUpdateAst: true,
+            focusPath: [pathToImportNode, pathToInsertNode],
+          }
+        ).catch(reportRejection)
+      },
+    },


This started as insertAstMod in the modelingMachine file, moved it here after the transition to prefilled paths. Not entirely sure this is the correct way forward.

my only problem with is the name, I know you didn't name it but updateModelingState does not strike me as something that's about executing, updating the kcl-singleton and editor, sounds like it should be part of the modeling machine.

Irev-Dev

Quick thought from having just watched the vid, surely we should pre-fill the localName with something derived from the fileNam?

Irev-Dev · 2025-04-07T06:41:23Z

e2e/playwright/native-file-menu.spec.ts

+        if (!tronApp) {
+          throwTronAppMissing()
+          return
+        }


Shouldn't this be abstracted into one of the fixtures, and just be a oneLiner, or maybe part of "goToModelingScene"?

I might leave this with @nadr0 as a separate issue if that's cool. This pattern only exists in e2e/playwright/native-file-menu.spec.ts, not sure how much it should be generalized.

But I agree with the sentiment yes!

Irev-Dev

Nice work, played around with it a little.

btw
I think we're going to get a lot of questions about this error

There's no way to fix this without opening the editor, and editing the imported file, I think maybe we need a one click fix of like "return the last variable" kind of thing.

src/lang/modelingWorkflows.ts

Irev-Dev · 2025-04-07T08:36:42Z

src/lib/kclCommands.ts

+      onSubmit: (data) => {
+        if (!data) {
+          return new Error('No input provided')
+        }
+
+        const ast = kclManager.ast
+        const { path, localName } = data
+        const { modifiedAst, pathToImportNode, pathToInsertNode } =
+          addImportAndInsert({
+            node: ast,
+            path,
+            localName,
+          })
+        updateModelingState(
+          modifiedAst,
+          EXECUTION_TYPE_REAL,
+          { kclManager, editorManager, codeManager },
+          {
+            skipUpdateAst: true,
+            focusPath: [pathToImportNode, pathToInsertNode],
+          }
+        ).catch(reportRejection)
+      },
+    },


my only problem with is the name, I know you didn't name it but updateModelingState does not strike me as something that's about executing, updating the kcl-singleton and editor, sounds like it should be part of the modeling machine.

pierremtb · 2025-04-07T12:36:33Z

@Irev-Dev Re:

my only problem with is the name, I know you didn't name it but updateModelingState does not strike me as something that's about executing, updating the kcl-singleton and editor, sounds like it should be part of the modeling machine.

Totally agree, would probably make sense to be something clearer like applyCodeMod or something like that which we would all understand I think?

Co-authored-by: Kurt Hutten <k.hutten@protonmail.ch>

pierremtb · 2025-04-07T12:39:05Z

Quick thought from having just watched the vid, surely we should pre-fill the localName with something derived from the fileNam?

@Irev-Dev Yes I actually wanted to do this and I think I just forgot. I have two follow up PRs to this one and I can for sure create a dedicated one for a nice little generateModuleAliasFromFilename or something like that. Can turn this into a new subissue in #4735

pierremtb · 2025-04-07T12:42:13Z

@Irev-Dev re:

I think we're going to get a lot of questions about this error
There's no way to fix this without opening the editor, and editing the imported file, I think maybe we need a one click fix of like "return the last variable" kind of thing.

You're super right and it actualy gets worse as we start talking about #6020, since in the case of module with no return value (say any part ending with a variable-assigned extrude for instance), you can use aliasName as a statement to get your solid, but you wan't be able to translate it or rotate it. We had a rather long thread about this thing as well as clone() in this rather long Slack thread

…rt-for-geometry

Irev-Dev · 2025-04-07T19:12:33Z

You're super right and it actualy gets worse as we start talking about #6020

Ahh kk so long as your on top of it.

Oh btw, we should allow inserting static assets with the point and click as well right? (maybe a follow up PR thing)

Screenshare.-.2025-04-08.5_11_28.AM.mp4

pierremtb · 2025-04-07T19:49:35Z

@Irev-Dev oh yea yea yea I have #6152 coming for that!!

pierremtb · 2025-04-07T19:50:00Z

Wrong link, the linked PR is #6159

* WIP: Add point-and-click Import for geometry Will eventually fix #6120 Right now the whole loop is there but the codemod doesn't work yet * Better pathToNOde, log on non-working cm dispatch call * Add workaround to updateModelingState not working * Back to updateModelingState with a skip flag * Better todo * Change working from Import to Insert, cleanups * Sister command in kclCommands to populate file options * Improve path selector * Unsure: move importAstMod to kclCommands onSubmit 😶 * Add e2e test * Clean up for review * Add native file menu entry and test * No await yo lint said so * @lrev-Dev's suggestion to remove a comment Co-authored-by: Kurt Hutten <k.hutten@protonmail.ch> * Update to scene.settled(cmdBar) * Lint --------- Co-authored-by: Kurt Hutten <k.hutten@protonmail.ch>

Irev-Dev · 2025-04-07T21:11:14Z

Sorry post merge.

But one other thing I thought of is your e2e test, is the happy path, should there be a sad path test?
I'm not actually sure what that would look like, maybe importing the same thing twice?

* origin/main: Quick app rename typo fix in settings.md (#6198) Add point-and-click Insert from local project files (#6129) Install and start Vector on macOS CI runners (#6147) Implement polar std function in KCL (#6180) Bump typescript from 5.8.2 to 5.8.3 in /packages/codemirror-lsp-client in the patch group (#6188) Bump @types/node from 22.13.13 to 22.14.0 in /packages/codemirror-lsp-client in the minor group (#6189) Bump the major group in /packages/codemirror-lang-kcl with 2 updates (#6194) Bump taiki-e/install-action from 2.49.30 to 2.49.45 in the patch group (#6185) Bump the patch group with 6 updates (#6186) Bump the patch group in /rust/kcl-language-server with 3 updates (#6183) Bump the patch group in /packages/codemirror-lang-kcl with 2 updates (#6193) Remove unnecessary timeouts waiting for command bar (#6199) Stream handling / Stream idle mode v2; a ton of network related changes (ping; scene indicator -> stream indicator, stream resizing (even on pause)) (#5312) More propagation of numeric types (#6177) Apply type-directed coercions to arguments in calls of user functions (#6179) Erase comment start positions from snapshot tests (#6178) Implement coercion of numeric types for ascription and arithmetic (off by default) (#6175) Reduce the number of reps in the add_lots test (#6174) take things off the batch in a more safe way (#6171)

pierremtb · 2025-04-07T21:43:13Z

@Irev-Dev yeah you're totally right. Once clone is available inserting the same part work will work through a clone() codemod, but now, importing twice will fail to execute. Which I thought wouldn't be horrible from a user standpoint but I guess we should check for the execution errors to occur. I'll make it part of #6152

WIP: Add point-and-click Import for geometry

9001006

Will eventually fix #6120 Right now the whole loop is there but the codemod doesn't work yet

pierremtb commented Apr 3, 2025

View reviewed changes

src/components/CustomIcon.tsx Outdated Show resolved Hide resolved

pierremtb commented Apr 3, 2025

View reviewed changes

src/machines/modelingMachine.ts Outdated Show resolved Hide resolved

Better pathToNOde, log on non-working cm dispatch call

3afcb33

vercel bot deployed to Preview April 3, 2025 11:42 View deployment

Add workaround to updateModelingState not working

e00f6c2

vercel bot deployed to Preview April 3, 2025 16:13 View deployment

pierremtb added 2 commits April 3, 2025 12:42

Back to updateModelingState with a skip flag

2bae5ff

Better todo

79fd435

vercel bot deployed to Preview April 3, 2025 17:06 View deployment

pierremtb changed the title ~~WIP: Add point-and-click Import for geometry~~ WIP: Add point-and-click Insert for local kcl files Apr 3, 2025

Update from main

2d08d47

vercel bot deployed to Preview April 3, 2025 17:59 View deployment

Change working from Import to Insert, cleanups

1bb37fc

vercel bot deployed to Preview April 3, 2025 19:16 View deployment

Sister command in kclCommands to populate file options

9371860

vercel bot deployed to Preview April 3, 2025 21:11 View deployment

Merge branch 'main' into pierremtb/issue6120-Add-point-and-click-Impo…

69a0198

…rt-for-geometry

vercel bot deployed to Preview April 3, 2025 23:54 View deployment

Merge branch 'main' into pierremtb/issue6120-Add-point-and-click-Impo…

58c87ac

…rt-for-geometry

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

Improve path selector

9e32ce3

vercel bot deployed to Preview April 4, 2025 04:21 View deployment

Merge branch 'main' into pierremtb/issue6120-Add-point-and-click-Impo…

5d8f7a5

…rt-for-geometry

vercel bot deployed to Preview April 4, 2025 13:16 View deployment

Unsure: move importAstMod to kclCommands onSubmit 😶

7b7d8b4

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

pierremtb commented Apr 4, 2025

View reviewed changes

pierremtb requested a review from a team April 4, 2025 17:41

pierremtb marked this pull request as ready for review April 4, 2025 17:41

pierremtb mentioned this pull request Apr 4, 2025

UX improvements around foreign file imports #6152

Closed

Irev-Dev reviewed Apr 7, 2025

View reviewed changes

Irev-Dev approved these changes Apr 7, 2025

View reviewed changes

@lrev-Dev's suggestion to remove a comment

4698339

Co-authored-by: Kurt Hutten <k.hutten@protonmail.ch>

Merge branch 'main' into pierremtb/issue6120-Add-point-and-click-Impo…

a932339

…rt-for-geometry

vercel bot deployed to Preview April 7, 2025 13:01 View deployment

Update to scene.settled(cmdBar)

c5e18ba

vercel bot deployed to Preview April 7, 2025 13:36 View deployment

Lint

a699e94

vercel bot deployed to Preview April 7, 2025 15:42 View deployment

pierremtb enabled auto-merge (squash) April 7, 2025 19:57

pierremtb merged commit bc0f5b5 into main Apr 7, 2025
38 checks passed

pierremtb deleted the pierremtb/issue6120-Add-point-and-click-Import-for-geometry branch April 7, 2025 20:28

pierremtb mentioned this pull request Apr 17, 2025

Check for local name clash with existing variables in Insert flow #6375

Merged

		// TODO: understand why this skip flag is needed for insertAstMod.
		// It's unclear why we double casts the AST

Conversation

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

Uh oh!

Uh oh!

qa-wolf bot commented Apr 3, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 3, 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!

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!

Irev-Dev 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!

Irev-Dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierremtb commented Apr 7, 2025

Uh oh!

pierremtb commented Apr 7, 2025

Uh oh!

pierremtb commented Apr 7, 2025

Uh oh!

Irev-Dev commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierremtb commented Apr 7, 2025

Uh oh!

pierremtb commented Apr 7, 2025

Uh oh!

Uh oh!

Irev-Dev commented Apr 7, 2025

Uh oh!

pierremtb commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pierremtb commented Apr 3, 2025 •

edited

Loading

vercel bot commented Apr 3, 2025 •

edited

Loading

codecov bot commented Apr 3, 2025 •

edited

Loading

Irev-Dev commented Apr 7, 2025 •

edited

Loading