Skip to content

refactor(site): refactor workspace notifications #11520

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 27 commits into from
Jan 12, 2024
Merged

refactor(site): refactor workspace notifications #11520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0b28628
Refactor outdated warning
BrunoQuaresma Jan 9, 2024
ab96aab
Move unhealthy warning
BrunoQuaresma Jan 9, 2024
38f213e
Move dormant notification
BrunoQuaresma Jan 9, 2024
9ec5952
Remove mutation errors from alerts
BrunoQuaresma Jan 9, 2024
17ecf0e
Move pending in queue notification
BrunoQuaresma Jan 9, 2024
c969df5
Move deprecated notification
BrunoQuaresma Jan 9, 2024
8f6040d
Move outdated to notifications array
BrunoQuaresma Jan 9, 2024
691c028
Move unhealthy to notifications array
BrunoQuaresma Jan 9, 2024
706bbd5
Fix permissions for restart workspace
BrunoQuaresma Jan 9, 2024
d3e68fb
Move dormant notifications to notifications array
BrunoQuaresma Jan 9, 2024
2f1ac83
Structure notification pills
BrunoQuaresma Jan 9, 2024
c334031
Minor style adjustments
BrunoQuaresma Jan 9, 2024
a78d4cf
Remove bottom bar from workspace page
BrunoQuaresma Jan 9, 2024
24c7552
Display notifications under the pills
BrunoQuaresma Jan 9, 2024
b711018
Improve canAutoStart check
BrunoQuaresma Jan 11, 2024
121005c
Move css into a style
BrunoQuaresma Jan 11, 2024
3234d49
Move css into a style
BrunoQuaresma Jan 11, 2024
b30dc44
Remove unecessary boolean
BrunoQuaresma Jan 11, 2024
04994a4
Refactor props
BrunoQuaresma Jan 11, 2024
5ba0765
Improve semantics
BrunoQuaresma Jan 11, 2024
3d0d70b
Merge branch 'bq/move-alert' of https://github.com/coder/coder into b…
BrunoQuaresma Jan 11, 2024
9299c68
Merge branch 'main' of https://github.com/coder/coder into bq/move-alert
BrunoQuaresma Jan 11, 2024
6d3148b
Merge branch 'main' of https://github.com/coder/coder into bq/move-alert
BrunoQuaresma Jan 12, 2024
45be26c
Add tests
BrunoQuaresma Jan 12, 2024
d21be38
Merge branch 'main' of https://github.com/coder/coder into bq/move-alert
BrunoQuaresma Jan 12, 2024
478a333
Fix storybook
BrunoQuaresma Jan 12, 2024
d62833c
Merge branch 'main' of https://github.com/coder/coder into bq/move-alert
BrunoQuaresma Jan 12, 2024
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
Prev Previous commit
Next Next commit
Display notifications under the pills
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Jan 9, 2024
commit 24c7552e086977be5d5717c7356eb3438b193def
2 changes: 2 additions & 0 deletions site/src/pages/WorkspacePage/Workspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,8 @@ export const Workspace: FC<WorkspaceProps> = ({
latestVersion={latestVersion}
permissions={permissions}
onRestartWorkspace={handleRestart}
onUpdateWorkspace={handleUpdate}
onActivateWorkspace={handleDormantActivate}
/>

{workspace.latest_build.status === "deleted" && (
Expand Down
157 changes: 140 additions & 17 deletions site/src/pages/WorkspacePage/WorkspaceNotifications.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,30 @@ import { useIsWorkspaceActionsEnabled } from "components/Dashboard/DashboardProv
import formatDistanceToNow from "date-fns/formatDistanceToNow";
import { Pill } from "components/Pill/Pill";
import InfoOutlined from "@mui/icons-material/InfoOutlined";
import ErrorOutline from "@mui/icons-material/ErrorOutline";
import {
Popover,
PopoverContent,
PopoverTrigger,
} from "components/Popover/Popover";
import { Interpolation, Theme, useTheme } from "@emotion/react";
import Button, { ButtonProps } from "@mui/material/Button";
import { ThemeRole } from "theme/experimental";
import WarningRounded from "@mui/icons-material/WarningRounded";

type Notification = {
title: string;
severity: AlertProps["severity"];
detail?: ReactNode;
actions?: { label: string; onClick: () => void }[];
actions?: ReactNode;
};

type WorkspaceNotificationsProps = {
workspace: Workspace;
template: Template;
permissions: WorkspacePermissions;
onRestartWorkspace: () => void;
onUpdateWorkspace: () => void;
onActivateWorkspace: () => void;
latestVersion?: TemplateVersion;
};

Expand All @@ -35,6 +45,8 @@ export const WorkspaceNotifications: FC<WorkspaceNotificationsProps> = (
latestVersion,
permissions,
onRestartWorkspace,
onUpdateWorkspace,
onActivateWorkspace,
} = props;
const notifications: Notification[] = [];

Expand All @@ -51,18 +63,26 @@ export const WorkspaceNotifications: FC<WorkspaceNotificationsProps> = (
const requiresManualUpdate = updateRequired && autoStartFailing;

if (workspace.outdated && latestVersion) {
const actions = (
<NotificationActionButton onClick={onUpdateWorkspace}>
Update
</NotificationActionButton>
);
if (requiresManualUpdate) {
notifications.push({
title: "Autostart has been disabled for your workspace.",
severity: "warning",
detail:
"Autostart is unable to automatically update your workspace. Manually update your workspace to reenable Autostart.",

actions,
});
} else {
notifications.push({
title: "An update is available for your workspace",
severity: "info",
detail: latestVersion.message,
actions,
});
}
}
Expand All @@ -84,14 +104,11 @@ export const WorkspaceNotifications: FC<WorkspaceNotificationsProps> = (
.
</>
),
actions: permissions.updateWorkspace
? [
{
label: "Restart",
onClick: onRestartWorkspace,
},
]
: undefined,
actions: permissions.updateWorkspace ? (
<NotificationActionButton onClick={onRestartWorkspace}>
Restart
</NotificationActionButton>
) : undefined,
});
}

Expand All @@ -107,7 +124,13 @@ export const WorkspaceNotifications: FC<WorkspaceNotificationsProps> = (
...(timestamp ? { hour: "numeric", minute: "numeric" } : {}),
});
};
const actions = (
<NotificationActionButton onClick={onActivateWorkspace}>
Activate
</NotificationActionButton>
);
notifications.push({
actions,
title: "Workspace is dormant",
severity: "warning",
detail: workspace.deleting_at ? (
Expand Down Expand Up @@ -197,24 +220,124 @@ export const WorkspaceNotifications: FC<WorkspaceNotificationsProps> = (
});
}

const infoNotifications = notifications.filter((n) => n.severity === "info");
const warningNotifications = notifications.filter(
Copy link
Member

@Parkreiner Parkreiner Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering if it's practical/a good idea to try splitting up the notifications logic into a helper function. Something like this:

function getNotifications (...inputs) {

  /** Main logic goes here */

  return {
    info: [...],
    warnings: [...],
  };
}

My main worry is that a lot of the branches are kind of interspersed throughout the component, with a random useEffect call wedged between them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if I got it 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I know the code has changed a little bit since the comment, but I'm going to go with the code on this PR as an example. The way I see the WorkspaceNotifications component, it kind of has different sections:

export const WorkspaceNotifications = (props) => {
  <-- Props destructuring -->
  <-- Set up empty notifications array -->
  <-- Calculate values from workspace query -->
  <-- Mutate notifications array if outdated -->
  <-- Mutate notifications array if unhealthy -->
  <-- Set up state for checking whether actions are enabled -->
  <-- Mutate notifications array if actions are enabled -->
  <-- Set up state/effects for alerts -->
  <-- Mutate notifications array if alerts are pending -->
  <-- Mutate notifications array if template is deprecated -->
  <-- Mutate notifications array if template is deprecated -->
  <-- Split the notifications array by warning/info type -->
  <-- Return JSX output -->
};

Most of the sections are dealing with calculating the notifications array

export const WorkspaceNotifications = (props) => {
   <-- Props destructuring -->
+  <-- Set up empty notifications array -->
   <-- Calculate values from workspace query -->
+  <-- Mutate notifications array if outdated -->
+  <-- Mutate notifications array if unhealthy -->
   <-- Set up state for checking whether actions are enabled -->
+  <-- Mutate notifications array if actions are enabled -->
   <-- Set up state/effects for alerts -->
+  <-- Mutate notifications array if alerts are pending -->
+  <-- Mutate notifications array if template is deprecated -->
+  <-- Mutate notifications array if template is deprecated -->
+  <-- Split the notifications array by warning/info type -->
   <-- Return JSX output -->
};

But the notifications array should always be the same if we have the same inputs, so seemingly, there should be a way to extract it into a reusable function. If we had the function, we could reorganize things like this:

export const WorkspaceNotifications = (props) => {
   <-- Props destructuring -->
   <-- Calculate values from workspace query -->
   <-- Set up state for checking whether actions are enabled -->
   <-- Set up state/effects for alerts -->
+  <-- Call function to get notifications -->
   <-- Return JSX output -->
};

The function might still mutate an array under the hood to fill it with data, but as long as we don't have any side effects, it will be fine, and it might be easier to add more cases for adding extra notifications down the line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But for some of them, we need to fetch data... so how to deal with that without any effect?

Copy link
Member

@Parkreiner Parkreiner Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We would still have the effects and fetching – that's why I didn't remove the non-notification steps. It's just that their values would get passed into the function on each render

I did just realize I forgot the query section, though. It should be:

export const WorkspaceNotifications = (props) => {
   <-- Props destructuring -->
+  <-- Set up workspaces autostart query state -->
   <-- Calculate values from workspace query -->
   <-- Set up state for checking whether actions are enabled -->
   <-- Set up state/effects for alerts -->
+  <-- Call function to get notifications -->
   <-- Return JSX output -->
};

(n) => n.severity === "warning",
);

return (
<div
css={{
display: "flex",
alignItems: "center",
gap: 8,
gap: 12,
position: "fixed",
bottom: 48,
right: 48,
zIndex: 10,
}}
>
<Pill type="info" icon={<InfoOutlined />}>
2
</Pill>
<Pill type="warning" icon={<ErrorOutline />}>
4
</Pill>
{infoNotifications.length > 0 && (
<NotificationPill
notifications={infoNotifications}
type="info"
icon={<InfoOutlined />}
/>
)}

{warningNotifications.length > 0 && (
<NotificationPill
notifications={warningNotifications}
type="warning"
icon={<WarningRounded />}
/>
)}
</div>
);
};

type NotificationPillProps = {
notifications: Notification[];
type: ThemeRole;
icon: ReactNode;
};

const NotificationPill: FC<NotificationPillProps> = (props) => {
const { notifications, type, icon } = props;
const theme = useTheme();

return (
<Popover mode="hover">
<PopoverTrigger>
<div css={[styles.pillContainer]}>
<Pill type={type} icon={icon}>
{notifications.length}
</Pill>
</div>
</PopoverTrigger>
<PopoverContent
transformOrigin={{
horizontal: "right",
vertical: "bottom",
}}
anchorOrigin={{ horizontal: "right", vertical: "top" }}
css={{
"& .MuiPaper-root": {
borderColor: theme.experimental.roles[type].outline,
maxWidth: 400,
},
}}
>
{notifications.map((n) => (
<NotificationItem notification={n} key={n.title} />
))}
</PopoverContent>
</Popover>
);
};

const NotificationItem: FC<{ notification: Notification }> = (props) => {
const { notification } = props;
const theme = useTheme();

return (
<article css={{ padding: 16 }}>
<h4 css={{ margin: 0, fontWeight: 500 }}>{notification.title}</h4>
{notification.detail && (
<p
css={{
margin: 0,
color: theme.palette.text.secondary,
lineHeight: 1.6,
}}
>
{notification.detail}
</p>
)}
<div css={{ marginTop: 8 }}>{notification.actions}</div>
</article>
);
};

const NotificationActionButton: FC<ButtonProps> = (props) => {
return (
<Button
variant="text"
css={{
textDecoration: "underline",
padding: 0,
height: "auto",
minWidth: "auto",
"&:hover": { background: "none", textDecoration: "underline" },
}}
{...props}
/>
);
};

const styles = {
// Adds some spacing from the popover content
pillContainer: {
paddingTop: 8,
},
} satisfies Record<string, Interpolation<Theme>>;
2 changes: 1 addition & 1 deletion site/src/pages/WorkspacePage/WorkspacePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export const WorkspacePage: FC = () => {
const isLoading = !workspace || !template || !permissions;

return (
<div css={{ height: "100%" }}>
<div css={{ height: "100%", display: "flex", flexDirection: "column" }}>
<Navbar />
{pageError ? (
<Margins>
Expand Down