Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 31, 2023
Merged

chore(UI): redirecting from workspace page if 404 #6880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 45 additions & 40 deletions site/src/pages/WorkspacePage/WorkspacePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { firstOrItem } from "util/array"
import { quotaMachine } from "xServices/quotas/quotasXService"
import { workspaceMachine } from "xServices/workspace/workspaceXService"
import { WorkspaceReadyPage } from "./WorkspaceReadyPage"
import { RequirePermission } from "components/RequirePermission/RequirePermission"

export const WorkspacePage: FC = () => {
const { username: usernameQueryParam, workspace: workspaceQueryParam } =
Expand Down Expand Up @@ -42,46 +43,50 @@ export const WorkspacePage: FC = () => {
}, [username, quotaSend])

return (
<ChooseOne>
<Cond condition={workspaceState.matches("error")}>
<div className={styles.error}>
{Boolean(getWorkspaceError) && (
<AlertBanner severity="error" error={getWorkspaceError} />
)}
{Boolean(getTemplateWarning) && (
<AlertBanner severity="error" error={getTemplateWarning} />
)}
{Boolean(getTemplateParametersWarning) && (
<AlertBanner
severity="error"
error={getTemplateParametersWarning}
/>
)}
{Boolean(checkPermissionsError) && (
<AlertBanner severity="error" error={checkPermissionsError} />
)}
{Boolean(getQuotaError) && (
<AlertBanner severity="error" error={getQuotaError} />
)}
</div>
</Cond>
<Cond
condition={
Boolean(workspace) &&
workspaceState.matches("ready") &&
quotaState.matches("success")
}
>
<WorkspaceReadyPage
workspaceState={workspaceState}
quotaState={quotaState}
workspaceSend={workspaceSend}
/>
</Cond>
<Cond>
<Loader />
</Cond>
</ChooseOne>
<RequirePermission
isFeatureVisible={getWorkspaceError?.response?.status !== 404}
Copy link
Member

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 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly!

>
Copy link
Collaborator

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.

Copy link
Member Author

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 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?

Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me!

<ChooseOne>
<Cond condition={workspaceState.matches("error")}>
<div className={styles.error}>
{Boolean(getWorkspaceError) && (
<AlertBanner severity="error" error={getWorkspaceError} />
)}
{Boolean(getTemplateWarning) && (
<AlertBanner severity="error" error={getTemplateWarning} />
)}
{Boolean(getTemplateParametersWarning) && (
<AlertBanner
severity="error"
error={getTemplateParametersWarning}
/>
)}
{Boolean(checkPermissionsError) && (
<AlertBanner severity="error" error={checkPermissionsError} />
)}
{Boolean(getQuotaError) && (
<AlertBanner severity="error" error={getQuotaError} />
)}
</div>
</Cond>
<Cond
condition={
Boolean(workspace) &&
workspaceState.matches("ready") &&
quotaState.matches("success")
}
>
<WorkspaceReadyPage
workspaceState={workspaceState}
quotaState={quotaState}
workspaceSend={workspaceSend}
/>
</Cond>
<Cond>
<Loader />
</Cond>
</ChooseOne>
</RequirePermission>
)
}

Expand Down
5 changes: 3 additions & 2 deletions site/src/xServices/workspace/workspaceXService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
displayError,
displaySuccess,
} from "../../components/GlobalSnackbar/utils"
import { AxiosError } from "axios"

const latestBuild = (builds: TypesGen.WorkspaceBuild[]) => {
// Cloning builds to not change the origin object with the sort()
Expand Down Expand Up @@ -56,7 +57,7 @@ export interface WorkspaceContext {
workspace?: TypesGen.Workspace
template?: TypesGen.Template
build?: TypesGen.WorkspaceBuild
getWorkspaceError?: Error | unknown
getWorkspaceError?: AxiosError
Copy link
Collaborator

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.

Copy link
Member Author

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.

getTemplateWarning: Error | unknown
getTemplateParametersWarning: Error | unknown
// Builds
Expand Down Expand Up @@ -491,7 +492,7 @@ export const workspaceMachine = createMachine(
workspace: (_, event) => event.data,
}),
assignGetWorkspaceError: assign({
getWorkspaceError: (_, event) => event.data,
getWorkspaceError: (_, event) => event.data as AxiosError,
}),
clearGetWorkspaceError: (context) =>
assign({ ...context, getWorkspaceError: undefined }),
Expand Down