-
Notifications
You must be signed in to change notification settings - Fork 193
[full-ci] feat: [OCISDEV-541] add crash page #13426
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
Conversation
6327c7a to
6f0c6da
Compare
252134a to
8df0b22
Compare
|
@nirajacharya2 can you please review the new tests? THX |
This comment was marked as resolved.
This comment was marked as resolved.
This error is strange as it should use the base URL and not only path... |
f33a0bb to
562ff6e
Compare
| errorCode: string | ||
| }) { | ||
| const { page } = actorsEnvironment.getActor({ key: 'Alice' }) | ||
| return page.goto(`${config.baseUrl}/crash?errorCode=${errorCode}`) |
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.
it will be better to pass user to function. if we logged in with brian this test would have failed
| errorCode: string | |
| }) { | |
| const { page } = actorsEnvironment.getActor({ key: 'Alice' }) | |
| return page.goto(`${config.baseUrl}/crash?errorCode=${errorCode}`) | |
| errorCode: string | |
| user: string | |
| }) { | |
| const { page } = actorsEnvironment.getActor({ key: user }) | |
| return page.goto(`${config.baseUrl}/crash?errorCode=${errorCode}`) |
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 did slightly different behaviour where I am passing the page directly. Saves one extra param and when doing multiple steps, we get the page from the actor only once instead of inside every step. Let me please know whether this change is fine.
nirajacharya2
left a comment
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.
test code looks good to me
| ) | ||
| }} | ||
| </h1> | ||
| <p v-if="errorCode !== ''" class="error-code"> |
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.
do we need to improve accessibility for this one? I guess some users won't be able to understand the connection between the error and the code. What do you 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.
For now, the error is there only for us. I.e. if we get a bug report, we know the code and know right away into which part of the code we should look into. In the future, we might consider adding some documentation where the codes could be listed and users/admins could orient themselves but as since we haven't got any error codes correlating with errors that the users could "fix" themselves I don't see too much value in doing that now.
We've added a crash page to the application. This page is displayed when the application encounters an error that it cannot recover from. The first such error we are catching is when spaces loading fails.
d9de271 to
53e7481
Compare
|




Description
We've added a crash page to the application. This page is displayed when the application encounters an error that it cannot recover from. The first such error we are catching is when spaces loading fails.
Motivation and Context
Handle errors that the app cannot recover from instead of showing the UI in a "stuck" state.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes