Display a generic fallback component when initial config load fails #8588

khuddite · 2024-11-19T15:27:21Z

Summary
The purpose of these changes is to elevate the dev/user experience when the initial config load call fails for whatever reason by displaying a fallback component.
Solution
I ended up making more changes than I initially planned. I had to update the order of the contexts a bit because GenericErrorFallback is dependent on AppThemeProvider for styling and AppThemeProvider is dependent on ObjectMetadataItemsProvider for useObjectMetadataItem hook (AppThemeProvider -> useColorScheme -> useUpdateOneRecord -> useObjectMetadataItem). I had to create a wrapper component for AppThemeProvider and stylize it in a way that it looks responsive on both mobile and desktop devices. Finally, I had to introduce the isErrored flag to differentiate the loading and error states.

There are some improvements we can make later -
- Display a loading state for the initial config load
- Implement a refetch logic for the initial config loading failure
Recording

CleanShot.2024-11-19.at.10.24.53.mp4

CleanShot.2024-11-19.at.10.25.51.mp4

…sInitialFetch flag

greptile-apps

PR Summary

This PR enhances error handling during initial configuration load by implementing a user-friendly error display when the backend server is unreachable.

Introduced new clientConfigApiStatusState in /packages/twenty-front/src/modules/client-config/states/clientConfigApiStatusState.ts to track both loading and error states
Reordered providers in /packages/twenty-front/src/modules/app/components/AppRouterProviders.tsx to ensure proper styling of error states
Modified /packages/twenty-front/src/modules/client-config/components/ClientConfigProvider.tsx to display responsive error UI with mobile viewport support
Enhanced /packages/twenty-front/src/modules/error-handler/components/GenericErrorFallback.tsx with initial fetch handling and location-based error boundary reset
Replaced deprecated isClientConfigLoadedState with more comprehensive status tracking across auth and config components

_{8 file(s) reviewed, 9 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-11-19T15:27:55Z

packages/twenty-front/src/modules/app/components/AppRouterProviders.tsx

+      <ApolloMetadataClientProvider>
+        <ObjectMetadataItemsProvider>
+          <AppThemeProvider>


logic: Moving ObjectMetadataItemsProvider before ClientConfigProvider could cause issues if metadata queries depend on client config values

@khuddite I think the AI is right here, one example of a bug that was introduced is the always-loading state here:

From a system architecture perspective I think it makes more sense to have the clientConfig stay at the higher-level. It's definitely a tougher problem than it looked initially!

I think what would make the most sense is to introduce a BaseThemeProvider at a higher level that would be similar to AppThemeProvider but with a simplified logic just based on system preference. This might be helpful if we start having more logged-out pages...

We can then rename AppThemeProvider to UserThemeProvider and focus its role on updating the context only based on the WorkspaceMember.

That would be a great solution to the problem. Thanks!

In addition to that, I will see if we can move ErrorBoundary up to the top level, so the initial config load error gets caught by ErrorBoundary(not manually like we do now). If so, we won't even need to introduce any additional states like isErrored or errorMessage. The position of ErrorBoundary wrapper in the current setup doesn't quite make sense to me.

The issue to move ErrorBoundary is that the Sentry config comes from ClientConfig... Maybe one day we should push it during the build process in env-config.js instead but today it won't work I think 🤔

greptile-apps · 2024-11-19T15:28:51Z

packages/twenty-front/src/modules/client-config/components/ClientConfigProvider.tsx

+    return (
+      <StyledContainer>
+        <GenericErrorFallback
+          error={new Error('Failed to fetch')}


logic: Generic 'Failed to fetch' error may not be descriptive enough of the actual error that occurred. Consider passing the actual error from the API call.

@khuddite Agree with the AI comment, passing the error message from the API call would make sense (add errorMessage in your state?)

greptile-apps · 2024-11-19T15:28:52Z

packages/twenty-front/src/modules/client-config/components/ClientConfigProvider.tsx

+      <StyledContainer>
+        <GenericErrorFallback
+          error={new Error('Failed to fetch')}
+          resetErrorBoundary={() => {}}


logic: Empty resetErrorBoundary callback means users can't retry when the error occurs. Consider implementing a retry mechanism.

Didn't test that but yes we should be able to reload from an error page

See also #5027 @ehconitin will work on this now so it might be fixed as part of his PR

greptile-apps · 2024-11-19T15:28:52Z

packages/twenty-front/src/modules/client-config/components/ClientConfigProvider.tsx


-  return isClientConfigLoaded ? <>{children}</> : <></>;
+  return isLoaded ? <>{children}</> : null;


style: Consider showing a loading state instead of null while isLoaded is false

would return isLoaded ? children : null; work then? If you decide not to wrap in component makes sense to do it on both side

greptile-apps · 2024-11-19T15:29:26Z

packages/twenty-front/src/modules/client-config/components/ClientConfigProviderEffect.tsx

+    setClientConfigApiStatus((currentStatus) => ({
+      ...currentStatus,
+      isLoaded: true,
+    }));


logic: Setting isLoaded:true before checking for errors could cause race conditions. Move this after error checks.

greptile-apps · 2024-11-19T15:29:27Z

packages/twenty-front/src/modules/client-config/components/ClientConfigProviderEffect.tsx

+    if (!isDefined(data?.clientConfig)) {
+      return;
    }


logic: Should set isErrored:true here since missing clientConfig is an error state

greptile-apps · 2024-11-19T15:29:27Z

packages/twenty-front/src/modules/client-config/components/ClientConfigProviderEffect.tsx

+    setAuthProviders({
+      google: data?.clientConfig.authProviders.google,
+      microsoft: data?.clientConfig.authProviders.microsoft,
+      password: data?.clientConfig.authProviders.password,
+      magicLink: false,
+      sso: data?.clientConfig.authProviders.sso,
+    });


style: Optional chaining is inconsistent - using ?. for clientConfig but not for nested authProviders properties

greptile-apps · 2024-11-19T15:30:34Z

packages/twenty-front/src/modules/error-handler/components/GenericErrorFallback.tsx

      <PageBody>
        <AnimatedPlaceholderEmptyContainer>
          <AnimatedPlaceholder type="errorIndex" />
          <AnimatedPlaceholderEmptyTextContainer>
            <AnimatedPlaceholderEmptyTitle>
-              Server’s on a coffee break
+              {title}
            </AnimatedPlaceholderEmptyTitle>
            <AnimatedPlaceholderEmptySubTitle>
              {error.message}


logic: error.message may be undefined - consider adding a fallback message

FelixMalfait

Almost there!! Thanks a lot!

FelixMalfait · 2024-11-21T13:40:06Z

packages/twenty-front/src/modules/client-config/components/ClientConfigProvider.tsx

+
+  if (isLoaded && isErrored) {
+    return (
+      <StyledContainer>


Could you please extract this styled container into its own component next to GenericErrorFallback? Eg. ClientConfigError?

Sure, that's a good idea!

Thanks! Just to be clear I didn't mean just the styled container but do a big wraper component. I think my comment wasn't clear

FelixMalfait · 2024-11-21T13:44:22Z

packages/twenty-front/src/modules/error-handler/components/GenericErrorFallback.tsx


 export const GenericErrorFallback = ({
  error,
  resetErrorBoundary,
+  title = 'Something went wrong',
+  isInitialFetch = false,


I think it would be a cleaner component API (and more reusable) to just name this as showPageHeader

This flag is also used to determine the visibility of refetch button. I am not sure if showPageHeader is still a good name in that case.

Let's assume that it will be hard refresh in the future! Even if it's broken for a few days it should come soon

FelixMalfait · 2024-11-21T15:31:19Z

packages/twenty-front/src/modules/app/components/AppRouterProviders.tsx

+      <ApolloMetadataClientProvider>
+        <ObjectMetadataItemsProvider>
+          <AppThemeProvider>


@khuddite I think the AI is right here, one example of a bug that was introduced is the always-loading state here:

From a system architecture perspective I think it makes more sense to have the clientConfig stay at the higher-level. It's definitely a tougher problem than it looked initially!

I think what would make the most sense is to introduce a BaseThemeProvider at a higher level that would be similar to AppThemeProvider but with a simplified logic just based on system preference. This might be helpful if we start having more logged-out pages...

We can then rename AppThemeProvider to UserThemeProvider and focus its role on updating the context only based on the WorkspaceMember.

khuddite added 2 commits November 19, 2024 09:57

Introduce isErrored status for handling initial page loading failure

552d6d1

Style updates for fallback container for responsiveness & introduce i…

ae2f193

…sInitialFetch flag

greptile-apps bot reviewed Nov 19, 2024

View reviewed changes

FelixMalfait self-assigned this Nov 21, 2024

FelixMalfait reviewed Nov 21, 2024

View reviewed changes

twentyhq deleted a comment from greptile-apps bot Nov 21, 2024

khuddite mentioned this pull request Nov 21, 2024

Reload button on JS error does not do anything #5027

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display a generic fallback component when initial config load fails #8588

Display a generic fallback component when initial config load fails #8588

khuddite commented Nov 19, 2024

greptile-apps bot left a comment

greptile-apps bot Nov 19, 2024

FelixMalfait Nov 21, 2024

khuddite Nov 21, 2024

khuddite Nov 21, 2024

FelixMalfait Nov 21, 2024

greptile-apps bot Nov 19, 2024

FelixMalfait Nov 21, 2024

greptile-apps bot Nov 19, 2024

FelixMalfait Nov 21, 2024

greptile-apps bot Nov 19, 2024

FelixMalfait Nov 21, 2024

greptile-apps bot Nov 19, 2024

greptile-apps bot Nov 19, 2024

greptile-apps bot Nov 19, 2024

greptile-apps bot Nov 19, 2024

FelixMalfait left a comment

FelixMalfait Nov 21, 2024

khuddite Nov 21, 2024

FelixMalfait Nov 21, 2024

FelixMalfait Nov 21, 2024

khuddite Nov 21, 2024

FelixMalfait Nov 21, 2024

FelixMalfait Nov 21, 2024


		return isClientConfigLoaded ? <>{children}</> : <></>;
		return isLoaded ? <>{children}</> : null;

Display a generic fallback component when initial config load fails #8588

Are you sure you want to change the base?

Display a generic fallback component when initial config load fails #8588

Conversation

khuddite commented Nov 19, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Nov 19, 2024

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

greptile-apps bot Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot Nov 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 19, 2024

Choose a reason for hiding this comment

FelixMalfait 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