#4469 Improve toolbar ux

[andrewvarga]

Issue: #4469

Improvements:

• Not displaying Esc hotkey anymore: I think it's intuitive enough that users know about escape in general, so no need to display it. If need, I can change that to only display the Escape hotkey when the tool is active
• Fixed the issue of selecting center rectangle not showing the selection in the toolbar
• Added Esc hotkey to abort 3 point circle creation - this causes an error though, as well as for aborting other tools in progress:

createIntersectionPlane called when it already exists
createIntersectionPlane @ sceneEntities.ts:271
2trap.ts:36 Error: Expected a KCL value to be a sketch, but it wasn't.
    at Reason.toError (trap.ts:18:12)
    at sketchFromKclValue (wasm.ts:402:19)
    at prepareTruncatedAst (sceneEntities.ts:2491:14)
    at SceneEntities.prepareTruncatedAst (sceneEntities.ts:1935:5)
    at SceneEntities.setupSketch (sceneEntities.ts:584:27)
    at modelingMachine.ts:768:36
    at setup client side sketch segments [as exec] (modelingMachine.ts:787:7)
    at exec (chunk-OHGAGAVB.js?v=b2aeabb4:535:20)
    at Object.actionExecutor (chunk-OHGAGAVB.js?v=b2aeabb4:541:11)
    at resolveAndExecuteActionsWithContext (chunk-OHGAGAVB.js?v=b2aeabb4:1999:18)
err @ trap.ts:36

After this the toolbar is stuck, you need to exit the sketch or rather restart the app.

Next, if we agree on it:

• remove the focus stage / tooltip

[qa-wolf]

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]

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

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Apr 9, 2025 7:08am

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.03%. Comparing base (ae9d8be) to head (a6581c1).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5841   +/-   ##
=======================================
  Coverage   85.03%   85.03%           
=======================================
  Files         109      109           
  Lines       44758    44758           
=======================================
  Hits        38062    38062           
  Misses       6696     6696           

Flag	Coverage Δ
rust	85.03% <ø> (ø)

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.

[andrewvarga]

I think I found the issue. Could be reproduced by:

• Start a new sketch
• select plane to draw on
• select the rectangle tool
• place the first point
• before placing the second point, press ESC
  -> various errors on console, can't change the tool anymore without exiting the sketch

The error:
This line sets sketchEntryNodePath to undefined:

sketchEntryNodePath: event.output.updatedEntryNodePath

This is called by:

id: 'corner-rectangle',
          onClick: ({ modelingState, modelingSend }) =>
            modelingSend({
              type: 'change tool',
              data: {
                tool: !modelingState.matches({ Sketch: 'Rectangle tool' })
                  ? 'rectangle'
                  : 'none',
              },
            }),

I still haven't found exactly where the undefined is coming from, but a possible fix is to ensure that never happens in 'update sketchDetails':

 sketchEntryNodePath: event.output.updatedEntryNodePath || []

There is already a similar line above this one that does this, so I assume that can be a good fix but it would be still useful to know where it comes from.

Is there a way to enable xstate logging? These lines don't seem to work for me.

I found this btw after @franknoirot pointed out to me that you can log errors by turning this:

'setup sketch for tool': {
   ...
   onError: '#Modeling.Sketch.SketchIdle',

into this:

'setup sketch for tool': {
   ...
   onError: {
      target: '#Modeling.Sketch.SketchIdle',
      actions: (context, event) => {
         console.warn('ERROR occurred:', context)
      },
   },

This gave me this error: "Cannot read properties of undefined (reading 'length')" Which pointed me to this line in ModelingMachineProvider.tsx:

if (!sketchDetails.sketchEntryNodePath.length) return

My immediate question is, could we / should we enable error logging, at least during development for everything? There are possible errors like this which are silently being ignored causing similar problems potentially.

[andrewvarga]

Merging from main I see this error is fixed already, probably by @Irev-Dev (?)
I think it would be still useful to log all errors but in that case what's left is to remove the focus stage - shall I just go ahead and do that?

[pierremtb]

Diff looks good and it feels great testing locally. Thank you! I know this has been going on for a while.