Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reload button on JS error does not do anything #5027

Closed
FelixMalfait opened this issue Apr 18, 2024 · 10 comments
Closed

Reload button on JS error does not do anything #5027

FelixMalfait opened this issue Apr 18, 2024 · 10 comments
Assignees
@ehconitin
Labels
scope: front Issues that are affecting the frontend side only

Comments

@FelixMalfait
Copy link
Member

Screenshot 2024-04-18 at 10 15 02

Once it's that state it's very annoying because it's impossible to access any page (reload button doesn't solve anything).
We should either trigger a full reload or find a way to let the user go back to the previous page / click on menu, etc.
@FelixMalfait FelixMalfait added type: bug scope: front Issues that are affecting the frontend side only labels Apr 18, 2024
@yurimutti
Copy link

Hi, can you assign to me please ?

@FelixMalfait
Copy link
Member Author

@yurimutti sure thanks! To get into that state you'll need to create a fake frontend error

@yurimutti yurimutti mentioned this issue Apr 22, 2024
Closed
@yurimutti yurimutti removed their assignment Apr 29, 2024
@yurimutti
Copy link

@FelixMalfait I unassigned myself to an experienced contributor can work more fast to solve this.

@FelixMalfait
Copy link
Member Author

Let's change this button behavior to entirely reload the page (browser level), this will be more efficient. Right now I think we're attempting something else which is more local but then in many case it doesn't work.

Also the title "Server's on a coffee break is misleading" since this is actually for frontend error. Let's change it to "Sorry, something went wrong."

@ehconitin

@FelixMalfait FelixMalfait mentioned this issue Nov 21, 2024
Merged
@khuddite
Copy link
Contributor

khuddite commented Nov 21, 2024 •
edited
Loading

@FelixMalfait I am just curious, but if we want to hard refresh the current page at the browser level, does it even need to be a prop?

I think If we just execute window.location.reload(); for any errors, it doesn't need to be a prop

@khuddite
Copy link
Contributor

khuddite commented Nov 21, 2024 •
edited
Loading

Oh ignore my comment, it seems I didn't fully understand how ErrorBoundary component works.

Actually, it will make more sense to address this issue as part of my PR #8588

@khuddite
Copy link
Contributor

@ehconitin Don't worry about this, I am addressing this in #8588

FelixMalfait added a commit that referenced this issue Nov 22, 2024
@khuddite @FelixMalfait
Display a generic fallback component when initial config load fails (#…
62df0f0 
…8588)

Fixes: #8487 #5027 

1. 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.

2. 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`](https://github.com/khuddite/twenty/blob/ae2f193d68c6168e4c8323297d58f6dbc1de9fdf/packages/twenty-front/src/modules/object-metadata/hooks/useObjectMetadataItem.ts#L22)
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
    
3. Recording



https://github.com/user-attachments/assets/c2f43573-8006-4118-8e18-8576099d78fd



https://github.com/user-attachments/assets/9c5853d3-539b-4880-aa38-c416c3e13594

---------

Co-authored-by: Félix Malfait <[email protected]>
@ehconitin
Copy link
Contributor

Thanks @khuddite
@FelixMalfait the PR seems to be merged, was this issue fixed? Could we close this issue?

@khuddite
Copy link
Contributor

I believe so, we can close out this issue.

@FelixMalfait
Copy link
Member Author

Thanks!

@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Product development ✅ Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ehconitin ehconitin

Labels
scope: front Issues that are affecting the frontend side only
Projects
Product development ✅
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: added key on error boundary
7 participants
@FelixMalfait @Bonapara @khuddite @yurimutti @ehconitin @CoreTeamTwenty and others