Skip to content

Alternative way to make appMachine spawned children type safe#5890

Merged
franknoirot merged 4 commits intomainfrom
farskid/appMachine-type-safety
Apr 4, 2025
Merged

Alternative way to make appMachine spawned children type safe#5890
franknoirot merged 4 commits intomainfrom
farskid/appMachine-type-safety

Conversation

@farskid
Copy link
Contributor

@farskid farskid commented Mar 19, 2025

This is not perfect but because spawned children are direct children of the appMachine, we don't really use nor do we need any of the system properties here. They're direct children and can be typed strongly.

@farskid farskid requested a review from franknoirot March 19, 2025 20:53
@farskid farskid self-assigned this Mar 19, 2025
@vercel
Copy link

vercel bot commented Mar 19, 2025

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

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

@qa-wolf
Copy link

qa-wolf bot commented Mar 19, 2025

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!

export const authActor = appActor.system.get(AUTH) as ActorRefFrom<
typeof authMachine
>
export const authActor = appActor.getSnapshot().children.auth!
Copy link
Contributor Author

@farskid farskid Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting is safe here because, when the state snapshot is available, appMachine will have already initialized all spawned children.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. If these were invoked actors we'd have to be more cautious with the cast here, or if they were spawned in a more ephemeral way? I just want us to remember that if we make use of other patterns as we break apart the modeling machine, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if they were invoked, they'd legitimately be available partially depending on the state. Even spawned actors can be partially unavailable too. It's safe here because we have child actors that are spawned in the root state in the entry action meaning it's the first action that runs before the the first state transition happens in the machine so in other words, these spawns are always available.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I added a "gotcha" comment here

@codecov
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.39%. Comparing base (c7b3483) to head (a70b719).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5890      +/-   ##
==========================================
+ Coverage   85.37%   85.39%   +0.02%     
==========================================
  Files         110      110              
  Lines       44198    44198              
==========================================
+ Hits        37733    37743      +10     
+ Misses       6465     6455      -10     
Flag Coverage Δ
rust 85.39% <ø> (+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.

Copy link
Contributor

@franknoirot franknoirot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this @farskid

@franknoirot franknoirot merged commit fa612d5 into main Apr 4, 2025
38 checks passed
@franknoirot franknoirot deleted the farskid/appMachine-type-safety branch April 4, 2025 03:25
jessfraz added a commit that referenced this pull request Apr 5, 2025
* origin/main: (26 commits)
  attempt to import win-ca on windows (#6136)
  Upgrade e2e-tests windows runner from 4 cores to 8 (#6166)
  Follow-up fixes after bearing sample rename (#6164)
  Add test for #5799: "Only showing axis planes when there are no errors" (#6007)
  Wait for export button to make test more reliable (#6143)
  sketching on a mirror2d thats been extruded fixed! (#6149)
  Bump vite from 5.4.16 to 5.4.17 in /packages/codemirror-lang-kcl in the security group (#6150)
  Bump vite from 5.4.16 to 5.4.17 in the security group (#6151)
  Update all KCL-Samples to be more ME friendly (#6132)
  Shorten feedback cycle for legitimate failures (#6146)
  Remove the camera projection toggle from the UI (#6077)
  Use all available CPUs to run tests on CI (#6138)
  [fix] Get rid of risky useEffect in restart onboarding flow (#6133)
  Feature: Traditional menu actions in desktop application part II (#6030)
  [Bug] fix some UI friction from imports (#6139)
  Use scene fixture to make test more reliable on macOS (#6140)
  Fix: function composition during playwright setup created a massive page.reload loop (#6137)
  Alternative way to make appMachine spawned children type safe (#5890)
  [BUG] mutate ast to keep comments for pipe split ast-mod (#6128)
  Rename the app to Zoo Design Studio (#5974)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants