Display an error message when the service is unreachable #8490

khuddite · 2024-11-13T22:02:55Z

Summary
We block the rest of the UI if we fail to fetch client config from the back-end for whatever reason. Therefore, a white screen is displayed without any feedback whenever that happens.
ClientConfigProvider is the component that blocks UI when the client config has not been loaded.
Solution
It ended up being more changes than I anticipated but those changes were necessary. I removed ClientConfigProvider to unblock the rest of the UI even if the back-end is unreachable. Used enqueueSnackBar to display an error message when the client config failed to load. Used isClientConfigLoaded in a couple of places to ensure no further requests are made if client config fetch fails.
Recording

CleanShot.2024-11-13.at.17.01.03.mp4

Honestly, I am unsure if it's the best way to tackle the issue, but I'd like to receive feedback on this.

greptile-apps

PR Summary

This PR improves error handling when the backend service is unreachable by removing blocking UI behavior and adding clear error messaging.

Removed ClientConfigProvider component to prevent UI blocking when client config fails to load
Added error snackbar notification in ClientConfigProviderEffect when backend is unreachable
Added isClientConfigLoaded check in UserProviderEffect to prevent unnecessary network requests
Modified PageChangeEffect to check client config status before tracking analytics
Updated test decorators to reflect new non-blocking architecture

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

greptile-apps · 2024-11-13T22:03:24Z

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

      <ClientConfigProviderEffect />
-      <ClientConfigProvider>
-        <ChromeExtensionSidecarEffect />
-        <ChromeExtensionSidecarProvider>
-          <UserProviderEffect />
-          <UserProvider>
-            <AuthProvider>
-              <ApolloMetadataClientProvider>
-                <ObjectMetadataItemsProvider>
-                  <PrefetchDataProvider>
-                    <AppThemeProvider>
-                      <SnackBarProvider>
-                        <DialogManagerScope dialogManagerScopeId="dialog-manager">
-                          <DialogManager>
-                            <StrictMode>
-                              <PromiseRejectionEffect />
-                              <GotoHotkeysEffectsProvider />
-                              <PageTitle title={pageTitle} />
-                              <Outlet />
-                            </StrictMode>
-                          </DialogManager>
-                        </DialogManagerScope>
-                      </SnackBarProvider>
-                    </AppThemeProvider>
-                  </PrefetchDataProvider>
-                  <PageChangeEffect />
-                </ObjectMetadataItemsProvider>
-              </ApolloMetadataClientProvider>
-            </AuthProvider>
-          </UserProvider>
-        </ChromeExtensionSidecarProvider>
-      </ClientConfigProvider>
+      <ChromeExtensionSidecarEffect />


logic: ClientConfigProviderEffect and ChromeExtensionSidecarEffect may try to make API calls before SnackBarProvider is mounted, potentially losing error messages

greptile-apps · 2024-11-13T22:05:07Z

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

+      if (error !== undefined) {
+        enqueueSnackBar('Unable to reach backend', {
+          variant: SnackBarVariant.Error,
+        });
+        return;
+      }


logic: setIsClientConfigLoaded(false) should be called here to ensure the state accurately reflects the failed load

greptile-apps · 2024-11-13T22:05:08Z

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

@@ -90,5 +102,7 @@ export const ClientConfigProviderEffect = () => {
    setIsAnalyticsEnabled,
  ]);

+  console.log('error: ', error, loading);


style: remove console.log before merging to production

greptile-apps · 2024-11-13T22:05:54Z

packages/twenty-front/src/testing/decorators/PageDecorator.tsx

          <ClientConfigProviderEffect />
-          <ClientConfigProvider>
-            <UserProviderEffect />
-            <UserProvider>
-              <ApolloMetadataClientMockedProvider>
-                <ObjectMetadataItemsProvider>
-                  <FullHeightStorybookLayout>
-                    <HelmetProvider>
-                      <SnackBarProviderScope snackBarManagerScopeId="snack-bar-manager">
-                        <IconsProvider>
-                          <PrefetchDataProvider>
-                            <Outlet />
-                          </PrefetchDataProvider>
-                        </IconsProvider>
-                      </SnackBarProviderScope>
-                    </HelmetProvider>
-                  </FullHeightStorybookLayout>
-                </ObjectMetadataItemsProvider>
-              </ApolloMetadataClientMockedProvider>
-            </UserProvider>
-          </ClientConfigProvider>
+          <UserProviderEffect />
+          <UserProvider>


logic: UserProviderEffect depends on client config being loaded, but now runs regardless of client config status. This could cause cascading errors if client config fails

I don't think it's causing any issues here

greptile-apps

PR Summary

(updates since last review)

This PR continues the error handling improvements by implementing comprehensive error capture and display mechanisms across the application.

Added PromiseRejectionEffect to catch and display unhandled promise rejections via snackbar
Moved ClientConfigProviderEffect after SnackBarProvider in provider hierarchy to ensure error messages are displayed
Added error handling for ObjectMetadataItemNotFoundError with specific error message
Added Sentry integration placeholder in PromiseRejectionEffect for future error tracking

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

greptile-apps · 2024-11-13T22:39:07Z

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

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


style: consider adding an else clause to handle undefined clientConfig case explicitly

FelixMalfait · 2024-11-14T11:41:25Z

Thanks a lot!!!
I'm not sure this is the best solution too. @lucasbordeau what do you think? Should we keep this approach?
One thing that I like is that I feel it could improve performance positively by making this query less render-blocking and starting to display more components sooner.
But on the other hand having the ClientConfigProviderEffect so far down the component stack feels wrong as it's almost meant to be "environment variable for the frontend" so something quite generic that's accessible across the code.
Also the tests broke, not sure if it's easy to fix them @khuddite? Maybe let's wait for confirmation from @lucasbordeau to make sure you don't waste time

khuddite · 2024-11-18T03:01:21Z

Thanks a lot!!! I'm not sure this is the best solution too. @lucasbordeau what do you think? Should we keep this approach? One thing that I like is that I feel it could improve performance positively by making this query less render-blocking and starting to display more components sooner. But on the other hand having the ClientConfigProviderEffect so far down the component stack feels wrong as it's almost meant to be "environment variable for the frontend" so something quite generic that's accessible across the code. Also the tests broke, not sure if it's easy to fix them @khuddite? Maybe let's wait for confirmation from @lucasbordeau to make sure you don't waste time

Thanks for taking a look at this. I have had the same experience while working on this project. When this happened for the first time to me, I was a bit confused and couldn't quite figure out how to resolve it other than running the npx nx start command again, even when it's just a matter of order in which services run and hard refresh after few secs would resolve the issue. UI provided literally no clues.
I found the updated behavior a much better experience. I don't think these changes would have any other
negative implications, such as decreased performance, other than elevated user experience.

Bonapara · 2024-11-18T13:34:32Z

Design inspiration if needed

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=46186-139142&node-type=frame&t=ziP8MVSxvkKozDb5-11

FelixMalfait · 2024-11-18T13:40:50Z

@khuddite we discussed it internally and we think it's better not to change the structure like you did but simply reuse GenericErrorFallback and display this if we're not able to load the client config. Your change here can have unintended downstream consequence, clientConfig is something we want to be able to access through pretty much every component.

Do you want us to take care of it or do you want to give it another try?

Thanks!

khuddite · 2024-11-18T13:44:18Z

@khuddite we discussed it internally and we think it's better not to change the structure like you did but simply reuse GenericErrorFallback and display this if we're not able to load the client config. Your change here can have unintended downstream consequence, clientConfig is something we want to be able to access through pretty much every component.

Do you want us to take care of it or do you want to give it another try?

Thanks!

Thanks for providing the direction. Yeah, I've also noticed that something is not quite correct when I saw many storybook tests have failed for different reasons in consequence. And fixing them didn't seem straightforward.

I will give it another try.

FelixMalfait · 2024-11-18T13:47:01Z

Great thanks a lot!

- fix Icon variable Plus - add search input - fix dropdown height ## Before ![image](https://github.com/user-attachments/assets/49f73efd-21cc-4ecd-a494-f51edc34dc57) ## After ![image](https://github.com/user-attachments/assets/2af2c7ee-72fd-4dae-a1ef-19e75e1ac026)

## Before ![image](https://github.com/user-attachments/assets/b7384fb3-0f32-498b-99bb-27b8c15db00c) ## After ![image](https://github.com/user-attachments/assets/a8429849-559d-4494-8636-e5a6ddda7b47)

fixing twentyhq#7375 --------- Co-authored-by: guillim <guillaume@twenty.com>

@lucasbordeau

fix twentyhq#8204 I changed "API keys" to "API values". Stopped inputting special characters in Select field option keys. @lucasbordeau please check the changes and tell me if I need to do any other changes. :) --------- Co-authored-by: Félix Malfait <felix@twenty.com>

Fixes: twentyhq#8483 Co-authored-by: Félix Malfait <felix@twenty.com>

First step of twentyhq#6868 Adds min.., max.. queries for DATETIME fields adds min.., max.., avg.., sum.. queries for NUMBER fields (count distinct operation and composite fields such as CURRENCY handling will be dealt with in a future PR) <img width="1422" alt="Capture d’écran 2024-11-06 à 15 48 46" src="https://github.com/user-attachments/assets/4bcdece0-ad3e-4536-9720-fe4044a36719"> --------- Co-authored-by: Charles Bochet <charles@twenty.com> Co-authored-by: Weiko <corentin@twenty.com>

This reverts commit 5c7f48283529ef7644493f73e4967249c463a1a8.

This reverts commit 92ea9a220cf2a9b35958b964700dbe5709fcd756.

…achable" This reverts commit abccf584d84b6ea44edc613fe04671ada8a3f0e2.

charlesBochet · 2024-11-18T15:11:10Z

@khuddite Thanks for the PR! We will review it shortly, in the meantime, could you fix the conflicts so we can test it functionally (sorry, the project is still evolving quite fast!)

khuddite · 2024-11-18T15:15:22Z

@charlesBochet I am so sorry, I should've converted it to a draft. This PR is not ready for review just yet. I am still in the middle of making changes and following the direction provided by @FelixMalfait

FelixMalfait · 2024-11-18T15:17:31Z

@khuddite might be easier if you start from scratch again instead of solving the many conflict? Don't mind if I close this one and you re-open it with the simplied approach we discussed, starting from a fresh main? Thanks a lot!

khuddite · 2024-11-18T15:17:58Z

@khuddite might be easier if you start from scratch again instead of solving the many conflict? Don't mind if I close this one and you re-open it with the simplied approach we discussed? Thanks a lot!

That makes sense, sure go ahead!

Unblock UI and display error message when the service is unreachable

2a91456

greptile-apps bot reviewed Nov 13, 2024

View reviewed changes

code enhancements

462a03f

khuddite marked this pull request as draft November 13, 2024 22:22

khuddite marked this pull request as ready for review November 13, 2024 22:37

greptile-apps bot reviewed Nov 13, 2024

View reviewed changes

NIT: we prefer early returns to nested inserts

09269ad

FelixMalfait self-assigned this Nov 15, 2024

martmull and others added 9 commits November 18, 2024 08:52

Fix open api specs for MULTI_SELECT (twentyhq#8494)

ed0a3a2

## Before ![image](https://github.com/user-attachments/assets/b7384fb3-0f32-498b-99bb-27b8c15db00c) ## After ![image](https://github.com/user-attachments/assets/a8429849-559d-4494-8636-e5a6ddda7b47)

Feature : Adding percentage option to Input Number (twentyhq#8481)

af99e24

fixing twentyhq#7375 --------- Co-authored-by: guillim <guillaume@twenty.com>

fix: search error in filter & sort dropdowns (twentyhq#8493)

5ba1155

Fixes: twentyhq#8483 Co-authored-by: Félix Malfait <felix@twenty.com>

Revert "NIT: we prefer early returns to nested inserts"

5d3c0fe

This reverts commit 5c7f48283529ef7644493f73e4967249c463a1a8.

Revert "code enhancements"

6fd4afd

This reverts commit 92ea9a220cf2a9b35958b964700dbe5709fcd756.

Revert "Unblock UI and display error message when the service is unre…

e0d0eb4

…achable" This reverts commit abccf584d84b6ea44edc613fe04671ada8a3f0e2.

charlesBochet added the -PR: awaiting author label Nov 18, 2024

khuddite marked this pull request as draft November 18, 2024 15:15

FelixMalfait closed this Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display an error message when the service is unreachable #8490

Display an error message when the service is unreachable #8490

khuddite commented Nov 13, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Nov 13, 2024

greptile-apps bot Nov 13, 2024

greptile-apps bot Nov 13, 2024

greptile-apps bot Nov 13, 2024

khuddite Nov 13, 2024

greptile-apps bot left a comment

greptile-apps bot Nov 13, 2024

FelixMalfait commented Nov 14, 2024

khuddite commented Nov 18, 2024 •

edited

Loading

Bonapara commented Nov 18, 2024

FelixMalfait commented Nov 18, 2024

khuddite commented Nov 18, 2024

FelixMalfait commented Nov 18, 2024

charlesBochet commented Nov 18, 2024

khuddite commented Nov 18, 2024

FelixMalfait commented Nov 18, 2024 •

edited

Loading

khuddite commented Nov 18, 2024

Display an error message when the service is unreachable #8490

Display an error message when the service is unreachable #8490

Conversation

khuddite commented Nov 13, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Nov 13, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 13, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 13, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 13, 2024

Choose a reason for hiding this comment

khuddite Nov 13, 2024

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Nov 13, 2024

Choose a reason for hiding this comment

FelixMalfait commented Nov 14, 2024

khuddite commented Nov 18, 2024 • edited Loading

Bonapara commented Nov 18, 2024

FelixMalfait commented Nov 18, 2024

khuddite commented Nov 18, 2024

FelixMalfait commented Nov 18, 2024

charlesBochet commented Nov 18, 2024

khuddite commented Nov 18, 2024

FelixMalfait commented Nov 18, 2024 • edited Loading

khuddite commented Nov 18, 2024

khuddite commented Nov 13, 2024 •

edited

Loading

khuddite commented Nov 18, 2024 •

edited

Loading

FelixMalfait commented Nov 18, 2024 •

edited

Loading