Display an error message when the service is unreachable #8490
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR improves error handling when the backend service is unreachable by removing blocking UI behavior and adding clear error messaging.
- Removed
ClientConfigProvidercomponent to prevent UI blocking when client config fails to load - Added error snackbar notification in
ClientConfigProviderEffectwhen backend is unreachable - Added
isClientConfigLoadedcheck inUserProviderEffectto prevent unnecessary network requests - Modified
PageChangeEffectto 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
| <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 /> |
There was a problem hiding this comment.
logic: ClientConfigProviderEffect and ChromeExtensionSidecarEffect may try to make API calls before SnackBarProvider is mounted, potentially losing error messages
| if (error !== undefined) { | ||
| enqueueSnackBar('Unable to reach backend', { | ||
| variant: SnackBarVariant.Error, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
logic: setIsClientConfigLoaded(false) should be called here to ensure the state accurately reflects the failed load
| @@ -90,5 +102,7 @@ export const ClientConfigProviderEffect = () => { | |||
| setIsAnalyticsEnabled, | |||
| ]); | |||
|
|
|||
| console.log('error: ', error, loading); | |||
There was a problem hiding this comment.
style: remove console.log before merging to production
| <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> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I don't think it's causing any issues here
There was a problem hiding this comment.
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
PromiseRejectionEffectto catch and display unhandled promise rejections via snackbar - Moved
ClientConfigProviderEffectafterSnackBarProviderin provider hierarchy to ensure error messages are displayed - Added error handling for
ObjectMetadataItemNotFoundErrorwith specific error message - Added Sentry integration placeholder in
PromiseRejectionEffectfor future error tracking
3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| }); | ||
| return; | ||
| } | ||
| if (isDefined(data?.clientConfig)) { |
There was a problem hiding this comment.
style: consider adding an else clause to handle undefined clientConfig case explicitly
|
Thanks a lot!!! |
Fixes #8487
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.
ClientConfigProvideris 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
ClientConfigProviderto unblock the rest of the UI even if the back-end is unreachable. UsedenqueueSnackBarto display an error message when the client config failed to load. UsedisClientConfigLoadedin 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.