Bugfix: wait for settings loading before onboarding redirect check (#5470)

* Bugfix: wait for settings loading before onboarding redirect check

If you refresh the app while viewing a file, the settingsActor could not
have loaded the user settings before checking the onboardingStatus
setting. This uses a subscription on the settingsActor to await the
"init" state, after the user settings have loaded.

* Adjust approach to not use routeLoaders

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

src/Router.tsx

@@ -24,12 +24,7 @@ import ModelingMachineProvider from 'components/ModelingMachineProvider'
 import FileMachineProvider from 'components/FileMachineProvider'
 import { MachineManagerProvider } from 'components/MachineManagerProvider'
 import { PATHS } from 'lib/paths'
-import {
+import { fileLoader, homeLoader, telemetryLoader } from 'lib/routeLoaders'
-  fileLoader,
-  homeLoader,
-  onboardingRedirectLoader,
-  telemetryLoader,
-} from 'lib/routeLoaders'
 import LspProvider from 'components/LspProvider'
 import { KclContextProvider } from 'lang/KclProvider'
 import { ASK_TO_OPEN_QUERY_PARAM, BROWSER_PROJECT_NAME } from 'lib/constants'
@@ -113,11 +108,6 @@ const router = createRouter([
           {
             id: PATHS.FILE + 'SETTINGS',
             children: [
-              {
-                loader: onboardingRedirectLoader,
-                index: true,
-                element: <></>,
-              },
               {
                 path: makeUrlPathRelative(PATHS.SETTINGS),
                 element: <Settings />,

src/components/RouteProvider.tsx

@@ -4,11 +4,12 @@ import {
   useLocation,
   useNavigate,
   useRouteLoaderData,
+  redirect,
 } from 'react-router-dom'
 import { PATHS } from 'lib/paths'
 import { markOnce } from 'lib/performance'
 import { useAuthNavigation } from 'hooks/useAuthNavigation'
-import { useAuthState } from 'machines/appMachine'
+import { useAuthState, useSettings } from 'machines/appMachine'
 import { IndexLoaderData } from 'lib/types'
 import { getAppSettingsFilePath } from 'lib/desktop'
 import { isDesktop } from 'lib/isDesktop'
@@ -16,6 +17,9 @@ import { trap } from 'lib/trap'
 import { useFileSystemWatcher } from 'hooks/useFileSystemWatcher'
 import { loadAndValidateSettings } from 'lib/settings/settingsUtils'
 import { settingsActor } from 'machines/appMachine'
+import makeUrlPathRelative from 'lib/makeUrlPathRelative'
+import { OnboardingStatus } from 'wasm-lib/kcl/bindings/OnboardingStatus'
+import { SnapshotFrom } from 'xstate'
 
 export const RouteProviderContext = createContext({})
 
@@ -29,6 +33,7 @@ export function RouteProvider({ children }: { children: ReactNode }) {
   const navigation = useNavigation()
   const navigate = useNavigate()
   const location = useLocation()
+  const settings = useSettings()
 
   const authState = useAuthState()
   useEffect(() => {
@@ -43,6 +48,32 @@ export function RouteProvider({ children }: { children: ReactNode }) {
       markOnce('code/willLoadHome')
     } else if (isFile) {
       markOnce('code/willLoadFile')
+
+      /**
+       * TODO: Move to XState. This block has been moved from routerLoaders
+       * and is borrowing the `isFile` logic from the rest of this
+       * telemetry-focused `useEffect`. Once `appMachine` knows about
+       * the current route and navigation, this can be moved into settingsMachine
+       * to fire as soon as the user settings have been read.
+       */
+      const onboardingStatus: OnboardingStatus =
+        settings.app.onboardingStatus.current || ''
+      // '' is the initial state, 'completed' and 'dismissed' are the final states
+      const needsToOnboard =
+        onboardingStatus.length === 0 ||
+        !(onboardingStatus === 'completed' || onboardingStatus === 'dismissed')
+      const shouldRedirectToOnboarding = isFile && needsToOnboard
+
+      if (
+        shouldRedirectToOnboarding &&
+        settingsActor.getSnapshot().matches('idle')
+      ) {
+        navigate(
+          (first ? location.pathname : navigation.location?.pathname) +
+            PATHS.ONBOARDING.INDEX +
+            onboardingStatus.slice(1)
+        )
+      }
     }
     setFirstState(false)
   }, [navigation])

src/lib/routeLoaders.ts

@@ -23,30 +23,6 @@ export const telemetryLoader: LoaderFunction = async ({
   return null
 }
 
-// Redirect users to the appropriate onboarding page if they haven't completed it
-export const onboardingRedirectLoader: ActionFunction = async (args) => {
-  const settings = getSettings()
-  const onboardingStatus: OnboardingStatus =
-    settings.app.onboardingStatus.current || ''
-  const notEnRouteToOnboarding = !args.request.url.includes(
-    PATHS.ONBOARDING.INDEX
-  )
-  // '' is the initial state, 'completed' and 'dismissed' are the final states
-  const hasValidOnboardingStatus =
-    onboardingStatus.length === 0 ||
-    !(onboardingStatus === 'completed' || onboardingStatus === 'dismissed')
-  const shouldRedirectToOnboarding =
-    notEnRouteToOnboarding && hasValidOnboardingStatus
-
-  if (shouldRedirectToOnboarding) {
-    return redirect(
-      makeUrlPathRelative(PATHS.ONBOARDING.INDEX) + onboardingStatus.slice(1)
-    )
-  }
-
-  return null
-}
-
 export const fileLoader: LoaderFunction = async (
   routerData
 ): Promise<FileLoaderData | Response> => {