Skip to content

Commit d6e493c

Browse files
committed
fix(notifications): addresses PR review findings
- wraps mutedSources in createSignal for proper SolidJS reactivity - swaps markAllAsRead/setDrawerOpen order so unread highlights render - removes dead errors prop from tab components and DashboardStore - adds type="button" to all notification drawer and toast buttons - adds createMemo for sorted notifications, classList consistency - adds 8 new tests: muting, pruning, aria-expanded, reset, cycling
1 parent 27445c2 commit d6e493c

File tree

16 files changed

+171
-41
lines changed

16 files changed

+171
-41
lines changed

src/app/components/dashboard/ActionsTab.tsx

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { createMemo, For, Show } from "solid-js";
22
import { createStore } from "solid-js/store";
3-
import type { WorkflowRun, ApiError } from "../../services/api";
3+
import type { WorkflowRun } from "../../services/api";
44
import { config } from "../../stores/config";
55
import { viewState, setViewState, setTabFilter, resetTabFilter, resetAllTabFilters, ignoreItem, unignoreItem, type ActionsFilterField } from "../../stores/view";
66
import WorkflowRunRow from "./WorkflowRunRow";
@@ -13,7 +13,6 @@ import ChevronIcon from "../shared/ChevronIcon";
1313
interface ActionsTabProps {
1414
workflowRuns: WorkflowRun[];
1515
loading?: boolean;
16-
errors?: ApiError[];
1716
}
1817

1918
interface WorkflowGroup {
@@ -201,7 +200,7 @@ export default function ActionsTab(props: ActionsTabProps) {
201200
{/* Empty */}
202201
<Show
203202
when={
204-
!props.loading && (!props.errors || props.errors.length === 0) && repoGroups().length === 0
203+
!props.loading && repoGroups().length === 0
205204
}
206205
>
207206
<div class="p-8 text-center text-gray-500 dark:text-gray-400">

src/app/components/dashboard/DashboardPage.tsx

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import IssuesTab from "./IssuesTab";
88
import PullRequestsTab from "./PullRequestsTab";
99
import { config } from "../../stores/config";
1010
import { viewState, updateViewState } from "../../stores/view";
11-
import type { Issue, PullRequest, WorkflowRun, ApiError } from "../../services/api";
11+
import type { Issue, PullRequest, WorkflowRun } from "../../services/api";
1212
import { createPollCoordinator, fetchAllData, type DashboardData } from "../../services/poll";
1313
import { clearAuth, user, onAuthCleared, DASHBOARD_STORAGE_KEY } from "../../stores/auth";
1414

@@ -20,7 +20,6 @@ interface DashboardStore {
2020
issues: Issue[];
2121
pullRequests: PullRequest[];
2222
workflowRuns: WorkflowRun[];
23-
errors: ApiError[];
2423
loading: boolean;
2524
lastRefreshedAt: Date | null;
2625
}
@@ -29,7 +28,6 @@ const initialDashboardState: DashboardStore = {
2928
issues: [],
3029
pullRequests: [],
3130
workflowRuns: [],
32-
errors: [],
3331
loading: true,
3432
lastRefreshedAt: null,
3533
};
@@ -49,7 +47,6 @@ function loadCachedDashboard(): DashboardStore {
4947
issues: parsed.issues as Issue[],
5048
pullRequests: parsed.pullRequests as PullRequest[],
5149
workflowRuns: parsed.workflowRuns as WorkflowRun[],
52-
errors: [],
5350
loading: false,
5451
lastRefreshedAt: typeof parsed.lastRefreshedAt === "string" ? new Date(parsed.lastRefreshedAt) : null,
5552
};
@@ -91,7 +88,6 @@ async function pollFetch(): Promise<DashboardData> {
9188
issues: data.issues,
9289
pullRequests: data.pullRequests,
9390
workflowRuns: data.workflowRuns,
94-
errors: data.errors,
9591
loading: false,
9692
lastRefreshedAt: now,
9793
});
@@ -195,23 +191,20 @@ export default function DashboardPage() {
195191
<IssuesTab
196192
issues={dashboardData.issues}
197193
loading={dashboardData.loading}
198-
errors={dashboardData.errors}
199194
userLogin={userLogin()}
200195
/>
201196
</Match>
202197
<Match when={activeTab() === "pullRequests"}>
203198
<PullRequestsTab
204199
pullRequests={dashboardData.pullRequests}
205200
loading={dashboardData.loading}
206-
errors={dashboardData.errors}
207201
userLogin={userLogin()}
208202
/>
209203
</Match>
210204
<Match when={activeTab() === "actions"}>
211205
<ActionsTab
212206
workflowRuns={dashboardData.workflowRuns}
213207
loading={dashboardData.loading}
214-
errors={dashboardData.errors}
215208
/>
216209
</Match>
217210
</Switch>

src/app/components/dashboard/IssuesTab.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { createEffect, createMemo, createSignal, For, Show } from "solid-js";
22
import { createStore } from "solid-js/store";
33
import { config } from "../../stores/config";
44
import { viewState, setSortPreference, setTabFilter, resetTabFilter, resetAllTabFilters, ignoreItem, unignoreItem, type IssueFilterField } from "../../stores/view";
5-
import type { Issue, ApiError } from "../../services/api";
5+
import type { Issue } from "../../services/api";
66
import ItemRow from "./ItemRow";
77
import IgnoreBadge from "./IgnoreBadge";
88
import SortIcon from "../shared/SortIcon";
@@ -18,7 +18,6 @@ import { groupByRepo, computePageLayout, slicePageGroups } from "../../lib/group
1818
export interface IssuesTabProps {
1919
issues: Issue[];
2020
loading?: boolean;
21-
errors?: ApiError[];
2221
userLogin: string;
2322
}
2423

src/app/components/dashboard/PullRequestsTab.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { createEffect, createMemo, createSignal, For, Show } from "solid-js";
22
import { createStore } from "solid-js/store";
33
import { config } from "../../stores/config";
44
import { viewState, setSortPreference, ignoreItem, unignoreItem, setTabFilter, resetTabFilter, resetAllTabFilters, type PullRequestFilterField } from "../../stores/view";
5-
import type { PullRequest, ApiError } from "../../services/api";
5+
import type { PullRequest } from "../../services/api";
66
import { deriveInvolvementRoles, prSizeCategory } from "../../lib/format";
77
import ItemRow from "./ItemRow";
88
import StatusDot from "../shared/StatusDot";
@@ -21,7 +21,6 @@ import { groupByRepo, computePageLayout, slicePageGroups } from "../../lib/group
2121
export interface PullRequestsTabProps {
2222
pullRequests: PullRequest[];
2323
loading?: boolean;
24-
errors?: ApiError[];
2524
userLogin: string;
2625
}
2726

src/app/components/layout/Header.tsx

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createMemo, createSignal, Show } from "solid-js";
1+
import { createSignal, Show } from "solid-js";
22
import { useNavigate } from "@solidjs/router";
33
import { user, clearAuth } from "../../stores/auth";
44
import { getCoreRateLimit, getSearchRateLimit } from "../../services/github";
@@ -15,16 +15,16 @@ export default function Header() {
1515
navigate("/login");
1616
}
1717

18-
function toggleDrawer() {
18+
function handleBellClick() {
1919
if (!drawerOpen()) {
20-
markAllAsRead();
2120
setDrawerOpen(true);
21+
markAllAsRead();
2222
} else {
2323
setDrawerOpen(false);
2424
}
2525
}
2626

27-
const unreadCount = createMemo(() => getUnreadCount());
27+
const unreadCount = () => getUnreadCount();
2828

2929
const coreRL = () => getCoreRateLimit();
3030
const searchRL = () => getSearchRateLimit();
@@ -109,7 +109,7 @@ export default function Header() {
109109
{/* Bell icon with unread badge */}
110110
<button
111111
type="button"
112-
onClick={toggleDrawer}
112+
onClick={handleBellClick}
113113
class="text-sm text-gray-500 dark:text-gray-400 hover:text-gray-700 dark:hover:text-gray-200 shrink-0 relative"
114114
aria-label={unreadCount() > 0 ? `Notifications, ${unreadCount()} unread` : "Notifications"}
115115
aria-expanded={drawerOpen()}

src/app/components/shared/NotificationDrawer.tsx

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
import { createEffect, createSignal, For, onCleanup, Show } from "solid-js";
1+
import { createEffect, createMemo, createSignal, For, onCleanup, Show } from "solid-js";
22
import {
33
getNotifications,
44
markAllAsRead,
55
clearNotifications,
66
dismissError,
7-
mutedSources,
7+
addMutedSource,
88
} from "../../lib/errors";
99
import { relativeTime } from "../../lib/format";
1010
import { severityConfig } from "./ToastContainer";
@@ -58,9 +58,11 @@ export default function NotificationDrawer(props: NotificationDrawerProps) {
5858
onCleanup(() => document.removeEventListener("keydown", handler));
5959
});
6060

61+
const sortedNotifications = createMemo(() => getNotifications().slice().reverse());
62+
6163
function handleDismissAll() {
6264
const current = getNotifications();
63-
for (const n of current) mutedSources.add(n.source);
65+
for (const n of current) addMutedSource(n.source);
6466
clearNotifications();
6567
}
6668

@@ -94,18 +96,21 @@ export default function NotificationDrawer(props: NotificationDrawerProps) {
9496
Notifications
9597
</h2>
9698
<button
99+
type="button"
97100
onClick={markAllAsRead}
98101
class="text-sm text-blue-500 hover:text-blue-700 dark:hover:text-blue-300"
99102
>
100103
Mark all as read
101104
</button>
102105
<button
106+
type="button"
103107
onClick={handleDismissAll}
104108
class="text-sm text-gray-400 hover:text-gray-600 dark:hover:text-gray-300"
105109
>
106110
Dismiss all
107111
</button>
108112
<button
113+
type="button"
109114
ref={closeButtonRef}
110115
onClick={props.onClose}
111116
aria-label="Close notifications"
@@ -124,15 +129,15 @@ export default function NotificationDrawer(props: NotificationDrawerProps) {
124129
{/* Notification list */}
125130
<div class="flex-1 overflow-y-auto">
126131
<Show
127-
when={getNotifications().length > 0}
132+
when={sortedNotifications().length > 0}
128133
fallback={
129134
<div class="flex items-center justify-center h-full text-gray-500 dark:text-gray-400 text-sm">
130135
No notifications
131136
</div>
132137
}
133138
>
134139
<ul>
135-
<For each={getNotifications().slice().reverse()}>
140+
<For each={sortedNotifications()}>
136141
{(notif) => {
137142
const cfg = severityConfig(notif.severity);
138143
return (
@@ -168,6 +173,7 @@ export default function NotificationDrawer(props: NotificationDrawerProps) {
168173
</p>
169174
</div>
170175
<button
176+
type="button"
171177
onClick={() => dismissError(notif.id)}
172178
aria-label="Dismiss notification"
173179
class="shrink-0 text-gray-400 hover:text-gray-600 dark:hover:text-gray-300 mt-0.5"

src/app/components/shared/ToastContainer.tsx

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
import { createEffect, createSignal, For, onCleanup } from "solid-js";
22
import {
33
getNotifications,
4-
mutedSources,
4+
isMuted,
55
type AppNotification,
66
type NotificationSeverity,
77
} from "../../lib/errors";
8-
9-
// Severity configuration — shared with NotificationDrawer (imported there after C4)
108
export interface SeverityConfig {
119
path: string;
1210
secondaryPath?: string;
@@ -134,7 +132,7 @@ export default function ToastContainer() {
134132
// Check suppression conditions
135133
const lastToasted = lastToastedAt.get(notif.source);
136134
const inCooldown = lastToasted !== undefined && Date.now() - lastToasted < COOLDOWN_MS;
137-
const muted = mutedSources.has(notif.source);
135+
const muted = isMuted(notif.source);
138136

139137
if (inCooldown || muted) continue;
140138

@@ -182,7 +180,11 @@ export default function ToastContainer() {
182180
return (
183181
<div
184182
role="alert"
185-
class={`flex items-start gap-3 rounded-lg border p-3 shadow-lg ${cfg.bgClass} ${cfg.textClass} ${cfg.borderColorClass} ${item.dismissing ? "animate-toast-out" : "animate-toast-in"}`}
183+
class={`flex items-start gap-3 rounded-lg border p-3 shadow-lg ${cfg.bgClass} ${cfg.textClass} ${cfg.borderColorClass}`}
184+
classList={{
185+
"animate-toast-in": !item.dismissing,
186+
"animate-toast-out": item.dismissing,
187+
}}
186188
>
187189
<svg
188190
class={`h-5 w-5 shrink-0 ${cfg.iconClass}`}
@@ -202,6 +204,7 @@ export default function ToastContainer() {
202204
)}
203205
</span>
204206
<button
207+
type="button"
205208
onClick={() => dismissToast(item.notification.id)}
206209
class="shrink-0 opacity-60 hover:opacity-100 transition-opacity"
207210
aria-label="Dismiss notification"

src/app/lib/errors.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,15 +97,27 @@ export function getErrors(): AppNotification[] {
9797

9898
// Muted sources — suppress toasts for these sources after "Dismiss all"
9999
// Session-only, reset on page reload and on logout (via resetNotificationState)
100-
export const mutedSources = new Set<string>();
100+
const [_mutedSources, _setMutedSources] = createSignal(new Set<string>());
101+
102+
export function addMutedSource(source: string): void {
103+
_setMutedSources((prev) => new Set([...prev, source]));
104+
}
105+
106+
export function isMuted(source: string): boolean {
107+
return _mutedSources().has(source);
108+
}
109+
110+
export function clearMutedSources(): void {
111+
_setMutedSources(new Set<string>());
112+
}
101113

102114
export function clearNotifications(): void {
103115
setNotifications([]);
104116
}
105117

106118
export function resetNotificationState(): void {
107119
setNotifications([]);
108-
mutedSources.clear();
120+
clearMutedSources();
109121
}
110122

111123
// Backward-compat alias

src/app/services/poll.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,6 @@ export function createPollCoordinator(
240240
let hiddenAt: number | null = null;
241241
let destroyed = false;
242242

243-
244243
async function doFetch(): Promise<void> {
245244
if (destroyed || isRefreshing()) return;
246245
setIsRefreshing(true);

tests/components/DashboardPage.test.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,9 @@ vi.mock("../../src/app/lib/errors", () => ({
5151
pushNotification: vi.fn(),
5252
clearErrors: vi.fn(),
5353
clearNotifications: vi.fn(),
54-
mutedSources: new Set(),
54+
addMutedSource: vi.fn(),
55+
isMuted: vi.fn(() => false),
56+
clearMutedSources: vi.fn(),
5557
}));
5658

5759
// capturedFetchAll is populated by the createPollCoordinator mock each time

0 commit comments

Comments
 (0)