-
Notifications
You must be signed in to change notification settings - Fork 19
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
[Issue #3628] allow error page to be interactive without refresh #3629
Conversation
0f1da69
to
57770f6
Compare
7e68146
to
e32fb7a
Compare
|
||
useEffect(() => { | ||
console.error(error); | ||
}, [error]); |
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.
Could we use or extend one of the errors from https://github.com/HHS/simpler-grants-gov/blob/main/frontend/src/errors.ts so we have more consistent error formatting and handling?
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.
We could punt to this #3651 if that is what yr thinking
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 errors caught here largely will be based on the definitions in our errors file, so not sure what to do here
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.
I would think we would wrap the output. I guess I don't see how this error is formed or called.
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.
👍🏿 with question about the error reporting.
Summary
Fixes #3628
Time to review: 10 mins
Changes proposed
Fixes bug where search error page had an unusable UI
Also introduces a usePrevious hook as pulled from https://www.developerway.com/posts/implementing-advanced-use-previous-hook (I think this hook implementation used to be in the React docs themselves, but I don't see it there anymore)
Context for reviewers
This was fixed by correctly using NextJs's
reset
method which is not well documented https://nextjs.org/docs/app/api-reference/file-conventions/error#resetBecause NextJS's error handling uses React error boundaries under the hood, which are basically designed to set an error state and take action based on that state, it makes sense that something explicit must be done to tell the error boundary that the app is no longer in an error state. That's what reset is for, so by calling reset whenever a user interaction occurs on the error page, the problem disappears.
Test steps
npm run dev
Additional information