chore(UI): redirecting from workspace page if 404 #6880
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.
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.
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.
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.
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 of getWorkspace. 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 a useEffect that depends upon getWorkspaceError and navigates to /workspaces conditionally? Or maybe there's something else you had in mind?
There was a problem hiding this comment.
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.
|
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.
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 👍
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.