Skip to content

Nadro/adhoc/system io machine #6352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 76 commits into from
Apr 24, 2025
Merged

Nadro/adhoc/system io machine #6352

merged 76 commits into from
Apr 24, 2025

Conversation

nadr0
Copy link
Contributor

@nadr0 nadr0 commented Apr 16, 2025

Issue

  • proposing a new frontend architecture pattern

Implementation

Overall notes about the large PR, frontend architecture will be in the headers below

  • Port the projectsMachine to a new xstate machine + react components
  • Removed some know circular dependencies, new but similar ones were added for a net of -1.
  • Added a npm run circular-deps:diff:nodejs to see the add and remove of deps easier
  • Moved all of appMachine into singleton.ts because the AppActor needs the singletons and this will reduce the circular dependencies.
  • Improved the react router
    • Root.tsx component
    • Fixed the error page render so the root element stays for the entire runtime
    • Error pages are on the child elements now
  • I've updated the circular deps file. I cleaned up some old ones. The circular dependencies came from the singletons and the actors. We pass them from the rootContext now :)
  • commandBarActor now at the root level!!
  • Fixed some more circular deps for a second time since some projectCommand initialization caused some new circular deps.
  • initialized commands: it is a helper function with dependencies passed. Circular deps rn is going wild.

Frontend Architecture

When I say global I specifically mean the respective block of code/component/class will be initialized once for the entire runtime of the application (web browser or electron). In JS there should only be one initialization and reference. In React there should only ever be one mounted call and only one unmount technically when the application closes.

A lot of the documentation is related to what to do in the global case. Once we start using many parts of the system across each part of the system there needs to be more structure across the codebase for loading, accessing, and runtime lifecycles.

If you make a purely functional table react component, it can use xstate internally and whatever local patterns it wants to be a generic table render component. This PR isn't talking about this type of architecture.

  • We want a concept/implementation for global runtime xstate/react components. This documentation is specifically for the global runtime scenario.
  • global xstate machine: Only has javascript code, no react code!
    • web provided version
    • desktop provided version
  • components/Providers/: If the xstate machine needs to do react stuff, the react provider should listen to the machine and perform the react code. One for web and desktop
    • web react provider
    • desktop react provider
  • systemIO/utils.ts
    • Stores all the enum for the xstate runtime. This allows us to globally access the events in code.
    • easier reading of machine definition since we have enums with actor, events, etc.. in the name.
  • systemIO/hook.ts: A set of hooks that can be used throughout the entire codebase when needing to access the systemIOMachine in react
  • systemIO/snapshotContext: A set of sync functions that can be used throughout the entire codebase when needing to access the systemIOMachine state in javascript

FAQS

Q: Is this frontend architecture a hard requirement?
A: No. This is a suggested pattern to help structure our initialization of the Javascript code, the ReactDOM.createRoot(... and the data accessing patterns across all the react components within that single createRoot call. This should be a typical pattern that developers can use, extend, and feel confident using.

Always know why you are writing new state management, new loading patterns, new component patterns etc.. and pick the best tool or pattern for the job.

Q: Is this a magic bullet?
A: No. This will help reduce race conditions by having clear ownership of business logic. This will reduce multiple sources of truth. This will reduce circular dependencies. This will improve the initialization of the application (reduces circular dependencies).

Q: Does xstate and react always have to be separate?
A: No, an xstate machine can implement React code but please be sure you know why you are doing this. Does it actually need to be coupled or can you have a separate xstate machine and then a react provider component.

Q: Does every provider need to be in <Root/>
A: No. If it calls navigate() via the react router dom then yes. Otherwise you should be able to put it root.render(... in index.tsx

Q: What react components live the entire application lifecycle?
A: index.tsx the root.render(... then the <Root/> in the React router because I fixed the errorElement definition.

Q: How do we initialize the application without circular dependencies?
A: In the future all the code in singleton.ts should be mapped out to initialize in the order of dependencies then we call const root = ReactDOM.createRoot(... and pass whatever data we need if required during initialization.

Q: Can code live outside React? What about classes?
A: Yes, state management should live outside React (for application level state management). Local state management in components is fine. This is not a strict rule but please be aware why and where you are initializing state. Classes that do custom/dense javascript logic can live outside of React because sometimes the React lifecycle or API does not do what we need. Sometimes you need to just write raw JS code/ DOM code or import a library.

Q: Do I always need to implement an xstate machine for state or use the xstate library?
A: No, use your best judgement on what you are storing, the lifecycle of the data, how it is fetched, and what parts of the code base need to access it. Having calls to useState in a simple UI component are completely fine. If you start writing larges parts of business logic, fetching a ton of data within useState you should consider writing it as a global machine if other parts of the system need access to that data.

Diagram

modeling-api-deployment-2024-08-21-1710

Machine documentation

// A single react provider for desktop and web to do React code on behalf of the xstate machine!
├── Providers // Lifecycle will most likely be the entire runtime application in the Root.tsx, mounted only once!
│   ├── SystemIOProviderDesktop.tsx
│   └── SystemIOProviderWeb.tsx

// Global machine boilerplate
└── systemIO
    ├── hooks.ts <--- helper functions for developers to access the machine's context in React (useSelector)
    ├── snapshotContext.ts <--- helper functions for developers to access the machine's context in Javascrip (xstate API)
    ├── systemIOMachineDesktop.ts <--- Desktop provided implementation of the systemIOMachine
    ├── systemIOMachine.test.ts <-- unit testing the machines!
    ├── systemIOMachine.ts <--- Machine definition, could be unimplemented for web or desktop (prototype, templated etc...)
    ├── systemIOMachineWeb.ts <--- Web (browser) provided implementation of the systemIOMachine
    └── utils.ts <--- enums for the machines to use across the codebase and read the machines easier 

nadr0 added 30 commits April 4, 2025 09:59
@nadr0
Copy link
Contributor Author

nadr0 commented Apr 23, 2025

Irev-Dev Apr 23, 2025

Very cool.

I wouldn't mind hearing some of the in and outs of these tests are we try and expand it, i.e. when actions or actors need access to react/singleton/websocket stuff, how does that work, do we mock at that point?.

This is the same as running the machine in isolation aka the machine running in a javascript environment. The architecture I recommended would not be interacting with the react side because the providers are not mounted in a react root.

In Vue.js I have built out unit tests that mount or do partial mounts of vue components. We could find a react library that does react component test and we can have the machine run and the react components run. This would be outside of the playwright runtime.

One issue we ran into is the window.electron. I cannot fully test the current machine's actor logic because there are global dependencies that would need to be mocked. I talked to Jon that we would need to resolve some of these window level variables to make the testing easier.

In terms of testing the xstate machine itself, we can build out any number of tests to ensure the machine is working well, actions are being called properly, states are transitioning properly, and any business logic within the actors. Obviously the definition of the machine itself encodes a lot of safety in the code but we can actually test this stuff now. We can unit test the flows to make sure the correct actions are being called and the internal context is correct.

@franknoirot
Copy link
Contributor

We could find a react library that does react component test and we can have the machine run and the react components run

Ffr we have this and some tests that use it, see UpdaterRestartModal.test.tsx

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.

I'm really liking this. Thank you for all the thought going into how not just this code but future code should be written. I'm sure we'll have to iron some stuff out but this feels much more solid and in the immediate term the projects load fast again! Thanks for hooking in the command palette to the app actor too it's been floating

import { commandBarActor } from '@src/machines/commandBarMachine'
import { type IndexLoaderData } from '@src/lib/types'
import { engineStreamActor, useSettings, useToken } from '@src/lib/singletons'
import { commandBarActor } from '@src/lib/singletons'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these are from the same file, I'm surprised we don't have a lint rule about combining imports

onSubmit: () => {
refreshPage('Command palette').catch(reportRejection)
export function createAuthCommands({
authActor,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah smart yeah I see the circular deal with these imports

],
},
onError: {
target: SystemIOMachineStates.idle,
Copy link
Contributor

Choose a reason for hiding this comment

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

Alright just something we should keep an eye on. Maybe a toast or a error log could help diagnose if this error ever comes up for a user.

@nadr0 nadr0 merged commit 305d613 into main Apr 24, 2025
41 checks passed
@nadr0 nadr0 deleted the nadro/adhoc/system-io-machine branch April 24, 2025 18:32
modeling-app-github-app bot pushed a commit that referenced this pull request Apr 24, 2025
* chore: saving off skeleton

* fix: saving skeleton

* chore: skeleton for loading projects from project directory path

* chore: cleaning up useless state transition to be an on event direct to action state

* fix: new structure for web vs desktop vs react machine provider code

* chore: saving off skeleton

* fix: skeleton logic for react? going to move it from a string to obj.string

* fix: trying to prevent error element unmount on global react components. This is bricking JS state

* fix: we are so back

* chore: implemented navigating to specfic KCL file

* chore: implementing renaming project

* chore: deleting project

* fix: auto fixes

* fix: old debug/testing file oops

* chore: generic create new file

* chore: skeleton for web create file provide

* chore: basic machine vitest... need to figure out how to get window.electron implemented in vitest?

* chore: save off progress before deleting other project implementation, a few missing features still

* chore: trying a different init skeleton? most likely will migrate

* chore: first attempt of purging projects context provider

* chore: enabling toast for some machine state

* chore: enabling more toast success and error

* chore: writing read write state to the system io based on the project path

* fix: tsc fixes

* fix: use file system watcher, navigate to project after creation via the requestProjectName

* chore: open project command, hooks vs snapshot context helpers

* chore: implemented open and create project for e2e testing. They are hard coded in poor spot for now.

* fix: codespell fixes

* chore: implementing more project commands

* chore: PR improvements for root.tsx

* chore: leaving comment about new Router.tsx layout

* fix: removing debugging code

* fix: rewriting component for readability

* fix: improving web initialization

* chore: implementing import file from url which is not actually that?

* fix: clearing search params on import file from url

* fix: fixed two e2e tests, forgot needsReview when making new command

* fix: fixing some import from url business logic to pass e2e tests

* chore: script for diffing circular deps +/-

* fix: formatting

* fix: massive fix for circular depsga!

* fix: trying to fix some errors and auto fmt

* fix: updating deps

* fix: removing debugging code

* fix: big clean up

* fix: more deletion

* fix: tsc cleanup

* fix: TSC TSC TSC TSC!

* fix: typo fix

* fix: clear query params on web only, desktop not required

* fix: removing unused code

* fmt

* Bring back `trap` removed in merge

* Use explicit types instead of `any`s on arg configs

* Add project commands directly to command palette

* fix: deleting debugging code, from PR review

* fix: this got added back(?)

* fix: using referred type

* fix: more PR clean up

* fix: big block comment for xstate architecture decision

* fix: more pr comment fixes

* fix: merge conflict just added them back why dude

* fix: more PR comments

* fix: big ciruclar deps fix, commandBarActor in appActor

---------

Co-authored-by: Frank Noirot <[email protected]>
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.

3 participants