Skip to content

chore: simplify workspace routing #17981

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 5 commits into from
May 27, 2025
Merged

chore: simplify workspace routing #17981

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0adfb04
chore: simplify workspace routing
BrunoQuaresma May 21, 2025
e00d0e5
Fix absolute route
BrunoQuaresma May 21, 2025
16e79b1
Skip flake test
BrunoQuaresma May 22, 2025
f09266a
Don't use state or effect
BrunoQuaresma May 22, 2025
5eb2302
Improve Regex
BrunoQuaresma May 24, 2025
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
Next Next commit
chore: simplify workspace routing
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed May 21, 2025
commit 0adfb0437c4af650b21e817e0dd4f1a1eb6123fa
19 changes: 2 additions & 17 deletions site/src/modules/dashboard/DashboardLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { AnnouncementBanners } from "modules/dashboard/AnnouncementBanners/Annou
import { LicenseBanner } from "modules/dashboard/LicenseBanner/LicenseBanner";
import { type FC, type HTMLAttributes, Suspense } from "react";
import { Outlet } from "react-router-dom";
import { dashboardContentBottomPadding } from "theme/constants";
import { docs } from "utils/docs";
import { DeploymentBanner } from "./DeploymentBanner/DeploymentBanner";
import { Navbar } from "./Navbar/Navbar";
Expand All @@ -24,23 +23,10 @@ export const DashboardLayout: FC = () => {
{canViewDeployment && <LicenseBanner />}
<AnnouncementBanners />

<div
css={{
display: "flex",
minHeight: "100%",
flexDirection: "column",
}}
>
<div className="flex flex-col min-h-full">
<Navbar />

<div
css={{
flex: 1,
paddingBottom: dashboardContentBottomPadding, // Add bottom space since we don't use a footer
display: "flex",
flexDirection: "column",
}}
>
<div className="flex flex-col flex-1">
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried that we missed something in the conversion here:

  • The old version didn't treat this div as a flex item itself. It just defined how the item grows in the parent flex container
  • We didn't add any padding, when the old version of the element had it

Was that intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeap. I did some storybook reviews and QA around and I didn't find anything broken. Maybe it was a leftover? I would appreciate you helping me with some QA 🙏

<Suspense fallback={<Loader />}>
<Outlet />
</Suspense>
Expand Down Expand Up @@ -111,7 +97,6 @@ export const DashboardFullPage: FC<HTMLAttributes<HTMLDivElement>> = ({
<div
{...attrs}
css={{
marginBottom: `-${dashboardContentBottomPadding}px`,
flex: 1,
display: "flex",
flexDirection: "column",
Expand Down
24 changes: 22 additions & 2 deletions site/src/modules/dashboard/DeploymentBanner/DeploymentBanner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,39 @@
import { health } from "api/queries/debug";
import { deploymentStats } from "api/queries/deployment";
import { useAuthenticated } from "hooks";
import type { FC } from "react";
import { type FC, useEffect, useState } from "react";
import { useQuery } from "react-query";
import { useLocation } from "react-router-dom";
import { DeploymentBannerView } from "./DeploymentBannerView";

const HIDE_DEPLOYMENT_BANNER_PATHS = [
// Workspace page.
// Hide the banner on workspace page because it already has a lot of information.
/^\/@[^\/]+\/[^\/]+$/,
Copy link
Member

Choose a reason for hiding this comment

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

What types of paths are we trying to catch with this? My gut feeling is that the current regex might be a little too loose in some ways, and a little too strict in others

Like, right now, we're exclusively matching patterns that go like /@something/something-else, but we'll never match anything like /@something/something-else/a-third-segment, because the regex only supports two segments. On the other hand, we're letting the regex accept characters that aren't valid in a URL

Copy link
Member

@Parkreiner Parkreiner May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If this wasn't what you wanted, you can let me know what you were going for, and I can try to whip up a regex for it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is exactly what we want. It is to catch the workspace page route, but I'm open in case you have a better regex idea or a way to match the path. Since the route path def is /:username/:workspace it was not working well with the react-router-dom path matching utility.

Copy link
Member

@Parkreiner Parkreiner May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In that case, try ^\/@(?<username>[a-zA-Z0-9-]+)\/(?<workspace_name>[a-zA-Z0-9-]+)$

  • It adds names to the main groups that we're checking for, so it'll be a little more self-documenting
  • It redefines each group to only allow the characters A-Z (lowercase or uppercase), numbers, and hyphens

I just checked what characters we allow for usernames and workspace names, and those seemed to be the only characters we support right now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good one 👍

];

export const DeploymentBanner: FC = () => {
const { permissions } = useAuthenticated();
const deploymentStatsQuery = useQuery(deploymentStats());
const healthQuery = useQuery({
...health(),
enabled: permissions.viewDeploymentConfig,
});
const location = useLocation();
const [visible, setVisible] = useState(true);

useEffect(() => {
const isHidden = HIDE_DEPLOYMENT_BANNER_PATHS.some((regex) =>
regex.test(location.pathname),
);
setVisible(!isHidden);
}, [location.pathname]);
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't need to be an effect, you're pulling state from a hook. just use a const.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

100%!!! Idk why I did that 🤦


if (!permissions.viewDeploymentConfig || !deploymentStatsQuery.data) {
if (
!visible ||
!permissions.viewDeploymentConfig ||
!deploymentStatsQuery.data
) {
return null;
}

Expand Down
37 changes: 12 additions & 25 deletions site/src/pages/WorkspacePage/WorkspacePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import { displayError } from "components/GlobalSnackbar/utils";
import { Loader } from "components/Loader/Loader";
import { Margins } from "components/Margins/Margins";
import { useEffectEvent } from "hooks/hookPolyfills";
import { AnnouncementBanners } from "modules/dashboard/AnnouncementBanners/AnnouncementBanners";
import { Navbar } from "modules/dashboard/Navbar/Navbar";
import { type FC, useEffect } from "react";
import { useQuery, useQueryClient } from "react-query";
import { useParams } from "react-router-dom";
Expand Down Expand Up @@ -106,29 +104,18 @@ const WorkspacePage: FC = () => {
workspaceQuery.error ?? templateQuery.error ?? permissionsQuery.error;
const isLoading = !workspace || !template || !permissions;

return (
<>
<AnnouncementBanners />
<div css={{ height: "100%", display: "flex", flexDirection: "column" }}>
<Navbar />
{pageError ? (
<Margins>
<ErrorAlert
error={pageError}
css={{ marginTop: 16, marginBottom: 16 }}
/>
</Margins>
) : isLoading ? (
<Loader />
) : (
<WorkspaceReadyPage
workspace={workspace}
template={template}
permissions={permissions}
/>
)}
</div>
</>
return pageError ? (
<Margins>
<ErrorAlert error={pageError} css={{ marginTop: 16, marginBottom: 16 }} />
</Margins>
) : isLoading ? (
<Loader />
) : (
<WorkspaceReadyPage
workspace={workspace}
template={template}
permissions={permissions}
/>
);
};

Expand Down
25 changes: 12 additions & 13 deletions site/src/router.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,20 +532,20 @@ export const router = createBrowserRouter(

{/* In order for the 404 page to work properly the routes that start with
top level parameter must be fully qualified. */}
<Route
path="/:username/:workspace/builds/:buildNumber"
element={<WorkspaceBuildPage />}
/>
<Route
path="/:username/:workspace/settings"
element={<WorkspaceSettingsLayout />}
>
<Route index element={<WorkspaceSettingsPage />} />
<Route path="/:username/:workspace">
<Route index element={<WorkspacePage />} />
<Route
path="parameters"
element={<WorkspaceParametersExperimentRouter />}
path="/builds/:buildNumber"
element={<WorkspaceBuildPage />}
/>
<Route path="schedule" element={<WorkspaceSchedulePage />} />
<Route path="/settings" element={<WorkspaceSettingsLayout />}>
<Route index element={<WorkspaceSettingsPage />} />
<Route
path="parameters"
element={<WorkspaceParametersExperimentRouter />}
/>
<Route path="schedule" element={<WorkspaceSchedulePage />} />
</Route>
</Route>

<Route path="/health" element={<HealthLayout />}>
Expand Down Expand Up @@ -574,7 +574,6 @@ export const router = createBrowserRouter(
</Route>

{/* Pages that don't have the dashboard layout */}
<Route path="/:username/:workspace" element={<WorkspacePage />} />
<Route
path="/templates/:template/versions/:version/edit"
element={<TemplateVersionEditorPage />}
Expand Down
1 change: 0 additions & 1 deletion site/src/theme/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export const navHeight = 62;
export const containerWidth = 1380;
export const containerWidthMedium = 1080;
export const sidePadding = 24;
export const dashboardContentBottomPadding = 8 * 6;

// MUI does not have aligned heights for buttons and inputs so we have to "hack" it a little bit
export const BUTTON_XL_HEIGHT = 44;
Expand Down
Loading