Apply Kayla suggestions

coder · BrunoQuaresma · Mar 12, 2025 · Mar 5, 2025 · Mar 5, 2025 · Mar 11, 2025
commit 0473e7b53891b507ff3a9108e2f279c20b43ca1f
diff --git a/site/src/components/Button/Button.tsx b/site/src/components/Button/Button.tsx
@@ -20,7 +20,7 @@ export const buttonVariants = cva(
 				default:
 					"bg-surface-invert-primary text-content-invert hover:bg-surface-invert-secondary border-none disabled:bg-surface-secondary font-semibold",
 				outline:
-					"border border-border-default text-content-primary bg-transparent bg-surface-primary hover:bg-surface-secondary",
+					"border border-border-default text-content-primary bg-transparent hover:bg-surface-secondary",
 				subtle:
 					"border-none bg-transparent text-content-secondary hover:text-content-primary",
 				destructive:

diff --git a/site/src/components/ScrollArea/ScrollArea.tsx b/site/src/components/ScrollArea/ScrollArea.tsx
@@ -1,8 +1,8 @@
-import * as ScrollAreaPrimitive from "@radix-ui/react-scroll-area";
 /**
  * Copied from shadc/ui on 03/05/2025
  * @see {@link https://ui.shadcn.com/docs/components/scroll-area}
  */
+import * as ScrollAreaPrimitive from "@radix-ui/react-scroll-area";
 import * as React from "react";
 import { cn } from "utils/cn";
 

diff --git a/site/src/modules/notifications/NotificationsInbox/InboxItem.tsx b/site/src/modules/notifications/NotificationsInbox/InboxItem.tsx
@@ -49,7 +49,7 @@ export const InboxItem: FC<InboxItemProps> = ({
 
 						<Button
 							onClick={() => onMarkNotificationAsRead(notification.id)}
-							className="hidden group-focus:flex group-hover:flex"
+							className="hidden group-focus:flex group-hover:flex bg-surface-primary"
 							variant="outline"
 							size="sm"
 						>

diff --git a/site/src/modules/notifications/NotificationsInbox/InboxPopover.stories.tsx b/site/src/modules/notifications/NotificationsInbox/InboxPopover.stories.tsx
@@ -6,6 +6,9 @@ import { InboxPopover } from "./InboxPopover";
 const meta: Meta<typeof InboxPopover> = {
 	title: "modules/notifications/NotificationsInbox/InboxPopover",
 	component: InboxPopover,
+	args: {
+		defaultOpen: true,
+	},
 	render: (args) => {
 		return (
 			<div className="w-full max-w-screen-xl p-6">
@@ -22,31 +25,28 @@ type Story = StoryObj<typeof InboxPopover>;
 
 export const Default: Story = {
 	args: {
-		defaultOpen: true,
+		defaultOpen: false,
 		unreadCount: 2,
 		notifications: MockNotifications.slice(0, 3),
 	},
 };
 
 export const Scrollable: Story = {
 	args: {
-		defaultOpen: true,
 		unreadCount: 2,
 		notifications: MockNotifications,
 	},
 };
 
 export const Loading: Story = {
 	args: {
-		defaultOpen: true,
 		unreadCount: 0,
 		notifications: undefined,
 	},
 };
 
 export const LoadingFailure: Story = {
 	args: {
-		defaultOpen: true,
 		unreadCount: 0,
 		notifications: undefined,
 		error: new Error("Failed to load notifications"),
@@ -55,15 +55,13 @@ export const LoadingFailure: Story = {
 
 export const Empty: Story = {
 	args: {
-		defaultOpen: true,
 		unreadCount: 0,
 		notifications: [],
 	},
 };
 
 export const OnRetry: Story = {
 	args: {
-		defaultOpen: true,
 		unreadCount: 0,
 		notifications: undefined,
 		error: new Error("Failed to load notifications"),
@@ -104,7 +102,6 @@ export const OnMarkAllAsRead: Story = {
 
 export const OnMarkNotificationAsRead: Story = {
 	args: {
-		defaultOpen: true,
 		unreadCount: 2,
 		notifications: MockNotifications.slice(0, 3),
 		onMarkNotificationAsRead: fn(),

diff --git a/site/src/modules/notifications/NotificationsInbox/InboxPopover.tsx b/site/src/modules/notifications/NotificationsInbox/InboxPopover.tsx
@@ -77,15 +77,13 @@ export const InboxPopover: FC<InboxPopoverProps> = ({
 									"[&>[role=menuitem]]:border-solid [&>[role=menuitem]]:border-border",
 								])}
 							>
-								{notifications.map((notification) => {
-									return (
-										<InboxItem
-											key={notification.id}
-											notification={notification}
-											onMarkNotificationAsRead={onMarkNotificationAsRead}
-										/>
-									);
-								})}
+								{notifications.map((notification) => (
+									<InboxItem
+										key={notification.id}
+										notification={notification}
+										onMarkNotificationAsRead={onMarkNotificationAsRead}
+									/>
+								))}
 							</div>
 						) : (
 							<div className="p-6 flex items-center justify-center min-h-48">

diff --git a/site/src/modules/notifications/NotificationsInbox/NotificationsInbox.stories.tsx b/site/src/modules/notifications/NotificationsInbox/NotificationsInbox.stories.tsx
@@ -24,22 +24,21 @@ type Story = StoryObj<typeof NotificationsInbox>;
 export const Default: Story = {
 	args: {
 		defaultOpen: true,
-		fetchNotifications: fn(() =>
-			Promise.resolve({ notifications: MockNotifications, unread_count: 2 }),
-		),
+		fetchNotifications: fn(async () => ({
+			notifications: MockNotifications,
+			unread_count: 2,
+		})),
 	},
 };
 
 export const Failure: Story = {
 	args: {
 		defaultOpen: true,
-		fetchNotifications: fn(() =>
-			Promise.reject(
-				mockApiError({
-					message: "Failed to load notifications",
-				}),
-			),
-		),
+		fetchNotifications: fn(() => {
+			throw mockApiError({
+				message: "Failed to load notifications",
+			});
+		}),
 	},
 };
 
@@ -49,23 +48,21 @@ export const FailAndRetry: Story = {
 		fetchNotifications: (() => {
 			let count = 0;
 
-			return fn(() => {
+			return fn(async () => {
 				count += 1;
 
 				// Fail on the first 3 attempts
 				// 3 is the maximum number of retries from react-query
 				if (count < 3) {
-					return Promise.reject(
-						mockApiError({
-							message: "Failed to load notifications",
-						}),
-					);
+					throw mockApiError({
+						message: "Failed to load notifications",
+					});
 				}
 
-				return Promise.resolve({
+				return {
 					notifications: MockNotifications,
 					unread_count: 2,
-				});
+				};
 			});
 		})(),
 	},
@@ -86,10 +83,11 @@ export const FailAndRetry: Story = {
 export const MarkAllAsRead: Story = {
 	args: {
 		defaultOpen: true,
-		fetchNotifications: fn(() =>
-			Promise.resolve({ notifications: MockNotifications, unread_count: 2 }),
-		),
-		markAllAsRead: fn(() => Promise.resolve()),
+		fetchNotifications: fn(async () => ({
+			notifications: MockNotifications,
+			unread_count: 2,
+		})),
+		markAllAsRead: fn(),
 	},
 	play: async ({ canvasElement }) => {
 		const body = within(canvasElement.ownerDocument.body);
@@ -109,14 +107,15 @@ export const MarkAllAsReadFailure: Story = {
 	decorators: [withGlobalSnackbar],
 	args: {
 		defaultOpen: true,
-		fetchNotifications: fn(() =>
-			Promise.resolve({ notifications: MockNotifications, unread_count: 2 }),
-		),
-		markAllAsRead: fn(() =>
-			Promise.reject(
-				mockApiError({ message: "Failed to mark all notifications as read" }),
-			),
-		),
+		fetchNotifications: fn(async () => ({
+			notifications: MockNotifications,
+			unread_count: 2,
+		})),
+		markAllAsRead: fn(async () => {
+			throw mockApiError({
+				message: "Failed to mark all notifications as read",
+			});
+		}),
 	},
 	play: async ({ canvasElement }) => {
 		const body = within(canvasElement.ownerDocument.body);
@@ -131,14 +130,11 @@ export const MarkAllAsReadFailure: Story = {
 export const MarkNotificationAsRead: Story = {
 	args: {
 		defaultOpen: true,
-		fetchNotifications: fn(() =>
-			Promise.resolve({ notifications: MockNotifications, unread_count: 2 }),
-		),
-		markNotificationAsRead: fn(() =>
-			// true as true is necessary to solve a really strange TypeScript error
-			// https://stackoverflow.com/questions/75864591/type-boolean-is-not-assignable-to-type-true
-			Promise.resolve({ is_read: true as true }),
-		),
+		fetchNotifications: fn(async () => ({
+			notifications: MockNotifications,
+			unread_count: 2,
+		})),
+		markNotificationAsRead: fn(),
 	},
 	play: async ({ canvasElement }) => {
 		const body = within(canvasElement.ownerDocument.body);
@@ -157,14 +153,13 @@ export const MarkNotificationAsReadFailure: Story = {
 	decorators: [withGlobalSnackbar],
 	args: {
 		defaultOpen: true,
-		fetchNotifications: fn(() =>
-			Promise.resolve({ notifications: MockNotifications, unread_count: 2 }),
-		),
-		markNotificationAsRead: fn(() =>
-			Promise.reject(
-				mockApiError({ message: "Failed to mark notification as read" }),
-			),
-		),
+		fetchNotifications: fn(async () => ({
+			notifications: MockNotifications,
+			unread_count: 2,
+		})),
+		markNotificationAsRead: fn(() => {
+			throw mockApiError({ message: "Failed to mark notification as read" });
+		}),
 	},
 	play: async ({ canvasElement }) => {
 		const body = within(canvasElement.ownerDocument.body);

diff --git a/site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx b/site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx
@@ -16,9 +16,7 @@ type NotificationsInboxProps = {
 	defaultOpen?: boolean;
 	fetchNotifications: () => Promise<NotificationsResponse>;
 	markAllAsRead: () => Promise<void>;
-	markNotificationAsRead: (
-		notificationId: string,
-	) => Promise<{ is_read: true }>;
+	markNotificationAsRead: (notificationId: string) => Promise<void>;
 };
 
 export const NotificationsInbox: FC<NotificationsInboxProps> = ({