fix: settings error layout #7674
Conversation
|
@charlesBochet I need your review on this. |
![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 pull request introduces a new error handling approach for settings pages, improving the user experience and error reporting.
- Added
SettingsErrorFallbackcomponent with dynamic breadcrumb generation for settings-specific error pages - Created
ErrorBoundaryWrapperto conditionally render appropriate error fallbacks based on route - Modified
AppErrorBoundaryto use Sentry for exception capturing and support different fallback components - Updated
DefaultLayoutto use the newErrorBoundaryWrapperfor consistent error handling across the application - Implemented error reset functionality when navigating between different settings pages
4 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| <AnimatedPlaceholder type="errorIndex" /> | ||
| <AnimatedPlaceholderEmptyTextContainer> | ||
| <AnimatedPlaceholderEmptyTitle> | ||
| Server’s on a coffee break |
There was a problem hiding this comment.
syntax: Server's on a coffee break
| } | ||
| }, [previousLocation, location, resetErrorBoundary]); | ||
|
|
||
| const generateBreadcrumbLinks = () => { |
There was a problem hiding this comment.
style: Consider moving generateBreadcrumbLinks to a separate utility file for better organization and reusability
| </AnimatedPlaceholderEmptySubTitle> | ||
| </AnimatedPlaceholderEmptyTextContainer> | ||
| <Button | ||
| Icon={IconRefresh} |
There was a problem hiding this comment.
style: title prop is not typically used with Button component, consider using children prop instead
|
@Bonapara Should we implement this not only in settings but everywhere in the app ? |
CleanShot.2024-10-18.at.19.02.25.mp4Don't we already do this? |
Thanks for contributing @harshrajeevsingh! Can you remove the |
Yeah! I will remove it. |
|
Oh you're right @harshrajeevsingh, thanks.
All the designs are here: |
|
@harshrajeevsingh modules/error-handlers seems like the right place ? |
|
@Bonapara Do we need that view bar, too? I think it's better to throw errors for that whole container. If any error occurs in viewBar, too, it will show the default GenericErrorFallback. @lucasbordeau wdyt?
|
|
Hi @harshrajeevsingh, no we do not need the view bar, especially if it's a page error and not a table error |
harshrajeevsingh
force-pushed
the
fix/settings-error-layout
branch
from
48fc0a5 to
b773444
Compare
|
@lucasbordeau what should be passed in header of both RecordIndexError & RecordShowError fallback and how?
|
|
/award 150 |
|
Awarding harshrajeevsingh: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/harshrajeevsingh |
|
@harshrajeevsingh Let's try to keep the breadcrumb as it was before the error occurs, and then make sure the new white background is on every error page. We don't need to spend too much time on this. |
|
@lucasbordeau Sorry, I didn’t quite understand. Are you asking me to remove the whole breadcrumbs part and just use a white container for every page with no headings or titles? Like this?
|
|
Yes please, also remove the breadcrumb computing part in your code, we'll see that later, thank you. |
harshrajeevsingh
force-pushed
the
fix/settings-error-layout
branch
from
b773444 to
4b53746
Compare
Fixes: #7460
Changes & Why
Since all the settings pages lie in the Outlet of DefaultLayout, there was no way to handle it apart from creating a separate errorFallback for the settings route.
So, I created a settingsErrorFallback component that uses the same styling of settings pages.
Created ErrorBoundaryWrapper that checks if its settings route then show SettingsErrorFallback else show GenericErrorFallback.
Now, for the breadcrumb part. I found that all the settings pages use hardcoded title. So, I created generateBreadcrumbLinks function that will provide different title and links based on how it's respective settings page has them.
If this approach looks fine, I will add the other remaining titles and links to the generateBreadcrumbLinks function and move that whole function to its separate file. And will fix linting errors.
If there is any different approach to handle it, lemme know. I'm happy to implement it.