-
Notifications
You must be signed in to change notification settings - Fork 84
Add page-level errors across admin UI #7188
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile SummaryThis PR adds comprehensive page-level error handling across the admin UI to improve user experience when queries fail. The implementation follows a consistent pattern: pages check for errors from their data-fetching hooks and display an Key changes:
Minor concern:
Confidence Score: 4/5
Important Files Changed
|
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.
51 files reviewed, 2 comments
| const [error, setError] = useState< | ||
| FetchBaseQueryError | SerializedError | null | ||
| >(null); | ||
|
|
||
| const onError = useCallback((e: FetchBaseQueryError | SerializedError) => { | ||
| setError(e); | ||
| }, []); |
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.
style: Inconsistent error handling pattern. Most other pages (systems, user-management, datamap, etc.) extract the error directly from their hooks and check it in the page component. This page uses a callback pattern with useState and onError.
Consider refactoring to match the standard pattern:
| const [error, setError] = useState< | |
| FetchBaseQueryError | SerializedError | null | |
| >(null); | |
| const onError = useCallback((e: FetchBaseQueryError | SerializedError) => { | |
| setError(e); | |
| }, []); | |
| const PropertiesPage: NextPage = () => { | |
| const { error } = usePropertiesTable(); | |
| const onError = useCallback((e: FetchBaseQueryError | SerializedError) => { | |
| setError(e); | |
| }, []); |
Or better yet, refactor PropertiesTable to return the error like other hooks do, eliminating the callback entirely.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| error, | ||
| data: properties, | ||
| } = useGetAllPropertiesQuery({ | ||
| page: pageIndex, | ||
| size: pageSize, |
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.
style: Consider returning the error from the hook instead of using a callback pattern. This would align with the standard pattern used in other hooks like useUserManagementTable, useDatamapTable, etc., making the codebase more consistent and easier to maintain.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Ticket ENG-2244
Description Of Changes
Adds error pages to many/most pages in the admin UI if the primary query fails.
Also some updates to error page component:
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works