Display a generic fallback component when initial config load fails #8588
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,43 @@ | ||
| import { useRecoilValue } from 'recoil'; | ||
|
|
||
| import { clientConfigApiStatusState } from '@/client-config/states/clientConfigApiStatusState'; | ||
| import { GenericErrorFallback } from '@/error-handler/components/GenericErrorFallback'; | ||
| import styled from '@emotion/styled'; | ||
| import { MOBILE_VIEWPORT } from 'twenty-ui'; | ||
|
|
||
| const StyledContainer = styled.div` | ||
| background: ${({ theme }) => theme.background.noisy}; | ||
| box-sizing: border-box; | ||
| display: flex; | ||
| height: 100dvh; | ||
| width: 100%; | ||
| padding-top: ${({ theme }) => theme.spacing(3)}; | ||
| padding-left: ${({ theme }) => theme.spacing(3)}; | ||
| padding-bottom: 0; | ||
| @media (max-width: ${MOBILE_VIEWPORT}px) { | ||
| padding-left: 0; | ||
| padding-bottom: ${({ theme }) => theme.spacing(3)}; | ||
| } | ||
| `; | ||
|
|
||
| export const ClientConfigProvider: React.FC<React.PropsWithChildren> = ({ | ||
| children, | ||
| }) => { | ||
| const { isLoaded, isErrored } = useRecoilValue(clientConfigApiStatusState); | ||
|
|
||
| if (isLoaded && isErrored) { | ||
| return ( | ||
| <StyledContainer> | ||
|
There was a problem hiding this comment. Could you please extract this styled container into its own component next to GenericErrorFallback? Eg. There was a problem hiding this comment. Sure, that's a good idea! There was a problem hiding this comment. 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 |
||
| <GenericErrorFallback | ||
| error={new Error('Failed to fetch')} | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. @khuddite Agree with the AI comment, passing the error message from the API call would make sense (add errorMessage in your state?) |
||
| resetErrorBoundary={() => {}} | ||
|
There was a problem hiding this comment. logic: Empty resetErrorBoundary callback means users can't retry when the error occurs. Consider implementing a retry mechanism. There was a problem hiding this comment. 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 |
||
| title="Unable to Reach Back-end" | ||
| isInitialFetch | ||
| /> | ||
| </StyledContainer> | ||
| ); | ||
| } | ||
|
|
||
| return isLoaded ? <>{children}</> : null; | ||
|
There was a problem hiding this comment. style: Consider showing a loading state instead of null while isLoaded is false There was a problem hiding this comment. would |
||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { createState } from 'twenty-ui'; | ||
|
|
||
| type ClientConfigApiStatus = { | ||
| isLoaded: boolean; | ||
| isErrored: boolean; | ||
| }; | ||
|
|
||
| export const clientConfigApiStatusState = createState<ClientConfigApiStatus>({ | ||
| key: 'clientConfigApiStatus', | ||
| defaultValue: { isLoaded: false, isErrored: false }, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Moving ObjectMetadataItemsProvider before ClientConfigProvider could cause issues if metadata queries depend on client config values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
BaseThemeProviderat 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
AppThemeProvidertoUserThemeProviderand focus its role on updating the context only based on the WorkspaceMember.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a great solution to the problem. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to that, I will see if we can move
ErrorBoundaryup to the top level, so the initial config load error gets caught byErrorBoundary(not manually like we do now). If so, we won't even need to introduce any additional states likeisErroredorerrorMessage. The position ofErrorBoundarywrapper in the current setup doesn't quite make sense to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good callout, thanks! I could've wasted a lot of time just to figure that out 😁