#5783 Only show axis planes when there are no errors

[andrewvarga]

As described in #5783 we were showing the axis planes when the artifactGraph was empty, which also happened when there were errors on the current kcl content. Added a guard to only show planes when there are no errors.

There was one weird thing I found that I'm commenting on inline.

I'm looking into how testing works, it's a small fix but maybe worth adding a test to cover it.

I wonder if the failed tests are unrelated? Seems other PRs are failing with similar cases.

[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	Mar 21, 2025 8:49am

[andrewvarga]

kclManager.hasErrors() seemed to return with false here even if kclManager.errors had the kcl errors in it..
If that's a bug we could refactor hasErrors to return errors.lenght > 0 or if these refer to different things it could be worth making their names clearer.

[Irev-Dev]

that's gotta be a bug in hasErrors so I'm happy to see a fix there too.

[Irev-Dev]

We could just make hasErrors be a getter that does return !!this.errors.length that way there can't be a bug?

[andrewvarga]

@Irev-Dev Yes I was thinking of doing exactly that, I looked at it again today and I want to make sure I'm not causing regressions by removing _hasErrors.

So in executeCode it seems safeParse() is invoked first, which sets _hasErrors to true if the AST parse failed. If the parse is successful then _hasErrors may still be set to true if the parse result has errors.

After this executeAst() is only called if the parsing is successful and has no errors.
If the parsing is successful and there are no errors then executeAst() is called which can have a new set of errors that the errors array is set to.
executeAst() may also be called individually without safeParse()

So there are multiple levels of errors:

• _hasErrors, which is set to true if parsing fails or there is a parsing result but it has errors
• _errors which is set from AST execution errors but is not updated when parsing has errors
• to add a bit of confusion I found that some execution errors can be considered parse errors as per this line:

const parseErrors = kclManager.errors.filter((e) => e.kind !== 'engine')

It's a bit unclear to me what is exactly the difference between the parse result errors in safeParse and the AST execution errors, I would love to learn more about that.

Regardless, it seems possible to me that the errors array is empty but _hasErrors is true.

With that in mind what I would do is:

• rename _hasErrors to something like _parseFailed
• rename errors to executionErrors (or keep it errors but at least the flag should be renamed to avoid confusion I think)
• hasErrors() would be implemented like: this.parseFailed || this._executionErrors.length > 0

@franknoirot @nrc I see you worked on these parts previously, just want to make sure I'm not going in a wrong direction.

[nrc]

So, for parsing and execution there are both fatal errors which immediately halt interpretation and are returned as the result of parsing or execution and there are non fatal errors (both errors and warnings) which do not cause an immediate halt (but except for warnings do halt proceeding to the next stage of interpretation). I think these are somewhat combined by the time the frontend sees them, but I don't recall exactly. There should really be no difference between a parsing and execution error.

[andrewvarga]

@nrc thanks, so it sounds like it's probably a good idea treat all of these similarly so hasErrors() returns true if there was either a parsing or an execution error, there shouldn't really be a difference in how they are handled in the frontend.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.77%. Comparing base (06b35b7) to head (72631a4).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5799   +/-   ##
=======================================
  Coverage   85.77%   85.77%           
=======================================
  Files         113      113           
  Lines       44025    44025           
=======================================
  Hits        37762    37762           
  Misses       6263     6263           

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

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]

Some snapshots have been modified by the github bot, @lee-at-zoo-corp @franknoirot is that something I should look at? They did seem to change visually.

[pierremtb]

Thank you @andrewvarga, that worked well here! Hitting merge.