- Notifications
You must be signed in to change notification settings - Fork927
chore(UI): redirecting from workspace page if 404#6880
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -56,7 +57,7 @@ export interface WorkspaceContext { | |||
| workspace?: TypesGen.Workspace | |||
| template?: TypesGen.Template | |||
| build?: TypesGen.WorkspaceBuild | |||
| getWorkspaceError?:Error | unknown | |||
| getWorkspaceError?:AxiosError | |||
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.
Usually, errors are kinda tricky to identify because they can be anything. But I think it is ok for now we assume those errors from the API will be mostly AxiosError but just a heads up.
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 hear you! This error is used very narrowly so I think it's okay in this case.
| </ChooseOne> | ||
| <RequirePermission | ||
| isFeatureVisible={getWorkspaceError?.response?.status !== 404} | ||
| > |
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.
When I read this component, it is not clear to me that the user will be redirected and to what page the user will be redirected if the feature is not visible. What do you think? I'm also finding it difficult to understand when this data is "loading" because the getWorkspaceError will be false for a short period of time before getting true right? I see this condition is handled by the children but I would expect a component called "RequirePermission" only would display the children if the feature is visible.
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.
getWorkspaceError may be undefined for a short time; however, we will always be in the loader during that period because we will be waiting on the result ofgetWorkspace. So the user should never mistakenly see a flash of content before they are redirected.
I agree this page is a bit awkwardly structured logic-wise - I was just trying to avoid carving it up right now.
What do you think would make this component clearer? Would a simple rename work or would you rather we have auseEffect that depends upongetWorkspaceError and navigates to/workspaces conditionally? Or maybe there's something else you had in mind?
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 agree this page is a bit awkwardly structured logic-wise - I was just trying to avoid carving it up right now.
Totally understandable!
Let's just keep this in mind and wait until we need other components needing something similar to do a proper refactor.
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.
Sounds good to me!
About tests, sometimes they are just hard to get working properly 😔 but If I would have to test something, I would try to test the component or the logic that is handling the redirect flow. |
| </Cond> | ||
| </ChooseOne> | ||
| <RequirePermission | ||
| isFeatureVisible={getWorkspaceError?.response?.status !== 404} |
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.
If I'm reading this correctly, this essentially delegates the permission check to the result of fetching the workspace? This is exactly what the backend will return, so 👍
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.
Yes, exactly!
This is a new fix to take the place of#6786, which was reverted because it broke everything 😳
We now redirect if the
getWorkspacerequest returns a 404. We can't explicitly authcheck a users' permission to read a workspace because we won't have that workspace ID (bc of the 404).I have smoke-tested this a fair amount and would like to write some E2E tests, but I'm having trouble with Playwright as per usual (see#6879). Not sure how to move forward with those tests. We can either release this fix as smoke-tested only or hold off until we get E2E working, but I'm moving on for now.