[Refactor] decouple settingsMachine from React #5142

franknoirot · 2025-01-23T17:11:21Z

Continuing the work from #5108 and #5110, next up is the settings. After #5110 gets in I'll rebase and then consolidate the commits into a readable story.

This pivots from #5110's approach of using invoke for the actor children of appMachine, opting instead to spawnChild them on machine initialization. Spawning is nice because they are not tied to the appMachine's current state (when we eventually add a state chart to it).

…eact

…rs/actions

…ingsAuthProvider

vercel · 2025-01-23T17:11:27Z

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

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Jan 23, 2025 8:20pm

qa-wolf · 2025-01-23T17:11:43Z

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!

…u-8-cores)

franknoirot · 2025-01-23T17:35:06Z

Oh hell yeah snapshots, great catch!

codecov · 2025-01-23T18:38:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.14%. Comparing base (20016b1) to head (a0cbc26).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5142   +/-   ##
=======================================
  Coverage   86.14%   86.14%           
=======================================
  Files          89       89           
  Lines       32489    32489           
=======================================
  Hits        27987    27987           
  Misses       4502     4502

Flag	Coverage Δ
wasm-lib	`86.14% <ø> (ø)`

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.

…missed

lf94

Really great job. This is a massive win and effort. I greatly appreciate it.

lf94 · 2025-01-27T13:55:31Z

src/components/AuthNavigationHandler.tsx

+/**
+ * A simple HOC that listens to the auth state of the app and navigates
+ * accordingly.
+ */
+export function AuthNavigationHandler({


The lack of any wrapping or nested components makes me believe this should instead be a hook? Why shouldn't it be a hook? Wondering if I'm missing something here.

You're so right, great idea! For now it needs to live on in one of the components that is in the index route but it could absolutely be a hook there. In the future I think we should do something else to inject the ability to navigate into the appMachine but I need a team consultation on that. I'll switch it to a hook for now

lf94 · 2025-01-27T13:56:18Z

src/components/CameraProjectionToggle.tsx


  return (
    <Switch
      checked={checked}
      onChange={(newValue) => {
-        settings.send({
+        settingsActor.send({


So beautiful :')

lf94 · 2025-01-27T13:59:48Z

src/components/Settings/SettingsSearchBar.tsx

+          const s = setting as Setting
+          return (['project', 'user'] satisfies SettingsLevel[])


Ah, you didn't write this, all good

I did write this, just not in this PR. I think you and @jtran found a way for Object.entries to pass through its types, do you remember what that was? Was it simply that we upgraded TS to a version that supports it? I'll try removing this as and see if that works now.

lf94 · 2025-01-27T14:02:40Z

src/index.tsx

@@ -11,6 +11,9 @@ import { ToastUpdate } from 'components/ToastUpdate'
 import { markOnce } from 'lib/performance'
 import { AUTO_UPDATER_TOAST_ID } from 'lib/constants'
 import { initializeWindowExceptionHandler } from 'lib/exceptions'
+import { initPromise } from 'lang/wasm'
+import { appActor } from 'machines/appMachine'


So it begins! Nice :)

lf94 · 2025-01-27T14:03:51Z

src/index.tsx

+initPromise
+  .then(() => {
+    appActor.start()
+  })
+  .catch(reportRejection)


Is this a general pattern we'll use for other actors?

I believe this app actor should be the one responsible for starting the others.

Yes 100%, I want to have you all design the appActor too, but my inclination is that it should instantiate everything else. I have to go back and wire commandBarActor into it

lf94 · 2025-01-27T14:07:22Z

src/lib/commandBarConfigs/settingsCommandConfig.ts

+        'value' in data &&
+        'level' in data
+      ) {
+        const coercedData = data as unknown as SetEventTypes['data']


I'm guessing the type system was too much to deal with here

Yeah it was not happy with the type gymnastics I was attempting. I can add a comment

lf94 · 2025-01-27T14:08:01Z

src/lib/commandBarConfigs/settingsCommandConfig.ts

      } else {
-        send({ type })
+        console.error('Invalid data submitted to settings command')


Would be nice to have the data output? Makes debugging quick / could also show up in the coredump

lf94 · 2025-01-27T14:12:40Z

src/lib/settings/settingsUtils.ts

+        if (settingValue[level] !== undefined) {
+          settingValue[level] = undefined
+        }


Why not just settingValue[level] = undefined ? The code basically says "if it's set, unset it". Computationally you're spending more cycles on 1. doing equality computation and 2. doing the conditional check, vs just 1. assignment.

Simply setting to undefined will be faster and clearer I'd think.

Oh duh, thank you

lf94 · 2025-01-27T14:13:40Z

src/machines/appMachine.ts

+    spawnChild(AUTH, { id: AUTH, systemId: AUTH }),
+    spawnChild(SETTINGS, {
+      id: SETTINGS,
+      systemId: SETTINGS,
+      input: createSettings(),
+    }),


lf94 · 2025-01-27T14:18:05Z

src/machines/appMachine.ts

+export const authActor = appActor.system.get(AUTH) as ActorRefFrom<
+  typeof authMachine
+>


XState wasnt able to infer on its own? (just curious)

No! This honestly is so frustrating to me. I figure it's just this one POS line for now but I do not want to have to do this for every actor in the system. I want to find a better way but I was just getting any back from system.get(). Maybe there's a type parameter I should add?

Not a clue. We'll have to look together more closely but isn't a blocker for me.

franknoirot added 30 commits January 23, 2025 10:27

Remove unnecessary console.log

be63b6d

Create a global appMachine

cf2eb27

Strip authMachine of side-effects

ab833e1

Replace react-bound authMachine use with XState actor use

d013947

Fix import goof

ec9c260

Register auth commands directly!

a590841

Don't provide anything to settingsMachine from React

53b534c

Remove unecessary async

74d50ef

Make it possible to load project settings via a sent event, without R…

2a88d04

…eact

Make settingsMachine ready to be an actor

8efa41c

Remove settingsLoader use

04ecc21

Replace all useSettingsAuthContext use with direct actor use

66fb064

Add logic to clear project settings, fmt

b2109d5

fmt

67f547e

Clear and load project settings from routeLoaders, but using actor

cd9353c

Remove useRefreshSettings

0313b0f

Restore use of useToken() that wasn't working for some reason

97dd22e

Migrate useFileSystemWatcher use to RouteProvider

ff2ec6f

Surface wasm_bindgen unavailable error to console

e199845

Remove unnecessary use of Jest settings wrappers

3ad59f8

Replace dynamic import with actor.getSnapshot

ffdcf2f

Migrate system theme and theme color watching from useEffects to acto…

468b1e9

…rs/actions

Migrate cursor color effect

8c358a4

Remove unused code that is now in RouteProvider

12cae4c

Migrate route commands registration further down for now, out of Sett…

52e6fce

…ingsAuthProvider

Migrate settings command registration out of SettingsAuthProvider.tsx

4861d00

Delete SettingsAuthProvider.tsx!

0f0d961

Remove unused settingsLoader!

1f225c3

fmt and remove comments

4f53d09

Use actor for routeLoader

f45a9ec

franknoirot added 9 commits January 23, 2025 10:27

Fix project read error due to uninitialized WASM

57d0338

Fix user settings load error due to uninitialized WASM

6c1c453

Move settingsActor into appActor as a spawned child

8bd3971

Trying to fix unit tests

e89c57e

Remove unused imports and demo window attachments

cc8bdc3

fmt

d54a425

Fix testing issues caused by circular dependency

24ffa6d

Add setThemeColor to a few actions list it was missing from

3e2fe56

fmt

152997c

franknoirot added the refactor label Jan 23, 2025

franknoirot requested review from lf94, nadr0 and pierremtb January 23, 2025 17:12

A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubunt…

3166078

…u-8-cores)

vercel bot deployed to Preview January 23, 2025 17:30 View deployment

Fix "Execute AST" action in browser, where currentProject is undefined

3c56adb

vercel bot deployed to Preview January 23, 2025 18:23 View deployment

Update commands list when currentProject changes

a0cbc26

vercel bot deployed to Preview January 23, 2025 19:18 View deployment

franknoirot added 2 commits January 23, 2025 15:07

Fix clearProjectSettings, which was passing along non-settings context

fe39c98

Fix onboarding test that actually needed the onboarding initially dis…

ccc15e7

…missed

vercel bot deployed to Preview January 23, 2025 20:20 View deployment

lf94 reviewed Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] decouple settingsMachine from React #5142

[Refactor] decouple settingsMachine from React #5142

franknoirot commented Jan 23, 2025

vercel bot commented Jan 23, 2025 •

edited

Loading

qa-wolf bot commented Jan 23, 2025

franknoirot commented Jan 23, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

lf94 left a comment

lf94 Jan 27, 2025

franknoirot Jan 27, 2025

lf94 Jan 27, 2025

lf94 Jan 27, 2025

lf94 Jan 27, 2025

franknoirot Jan 27, 2025

lf94 Jan 27, 2025

lf94 Jan 27, 2025

franknoirot Jan 27, 2025

lf94 Jan 27, 2025

franknoirot Jan 27, 2025

lf94 Jan 27, 2025

franknoirot Jan 27, 2025

lf94 Jan 27, 2025

franknoirot Jan 27, 2025

lf94 Jan 27, 2025

lf94 Jan 27, 2025

franknoirot Jan 27, 2025

lf94 Jan 27, 2025

		const s = setting as Setting
		return (['project', 'user'] satisfies SettingsLevel[])

[Refactor] decouple settingsMachine from React #5142

Are you sure you want to change the base?

[Refactor] decouple settingsMachine from React #5142

Conversation

franknoirot commented Jan 23, 2025

vercel bot commented Jan 23, 2025 • edited Loading

qa-wolf bot commented Jan 23, 2025

franknoirot commented Jan 23, 2025

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

lf94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Jan 23, 2025 •

edited

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading