Skip to content

Commit 4b5ed0f

Browse files
setchysetchy
authored
refactor: simplify tray color/count updates (#2339)
* fix: correctly set count when using delayNotificationState Signed-off-by: Adam Setch <adam.setch@outlook.com> * fix: correctly set count when using delayNotificationState Signed-off-by: Adam Setch <adam.setch@outlook.com> * refactor simplify Signed-off-by: Adam Setch <adam.setch@outlook.com> * refactor simplify Signed-off-by: Adam Setch <adam.setch@outlook.com> * Merge branch 'main' into feat/count-unread-only Signed-off-by: Adam Setch <adam.setch@outlook.com> * refactor simplify Signed-off-by: Adam Setch <adam.setch@outlook.com> * refactor simplify Signed-off-by: Adam Setch <adam.setch@outlook.com> * refactor simplify Signed-off-by: Adam Setch <adam.setch@outlook.com> * refactor simplify Signed-off-by: Adam Setch <adam.setch@outlook.com> * refactor simplify Signed-off-by: Adam Setch <adam.setch@outlook.com> * refactor Signed-off-by: Adam Setch <adam.setch@outlook.com> * test coverage Signed-off-by: Adam Setch <adam.setch@outlook.com> * test coverage Signed-off-by: Adam Setch <adam.setch@outlook.com> --------- Signed-off-by: Adam Setch <adam.setch@outlook.com>
1 parent 46db36d commit 4b5ed0f

28 files changed

+755
-389
lines changed

‎src/renderer/__mocks__/notifications-mocks.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { AccountNotifications } from '../types';
1+
import type { AccountNotifications, GitifyError } from '../types';
22
import type {
33
Notification,
44
StateType,
@@ -50,6 +50,14 @@ export function createSubjectMock(mocks: {
5050
};
5151
}
5252

53+
export function mockAccountWithError(error: GitifyError): AccountNotifications {
54+
return {
55+
account: mockGitHubCloudAccount,
56+
notifications: [],
57+
error,
58+
};
59+
}
60+
5361
export function createMockNotificationForRepoName(
5462
id: string,
5563
repoFullName: string | null,

‎src/renderer/components/Sidebar.tsx‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { type FC, useContext, useMemo } from 'react';
1+
import { type FC, useContext } from 'react';
22
import { useLocation, useNavigate } from 'react-router-dom';
33

44
import {
@@ -23,20 +23,20 @@ import {
2323
openGitHubPulls,
2424
} from '../utils/links';
2525
import { hasActiveFilters } from '../utils/notifications/filters/filter';
26-
import { getNotificationCount } from '../utils/notifications/notifications';
2726
import { LogoIcon } from './icons/LogoIcon';
2827

2928
export const Sidebar: FC = () => {
3029
const navigate = useNavigate();
3130
const location = useLocation();
3231

3332
const {
34-
notifications,
3533
fetchNotifications,
3634
isLoggedIn,
3735
status,
3836
settings,
3937
auth,
38+
unreadCount,
39+
hasNotifications,
4040
} = useContext(AppContext);
4141

4242
// We naively assume that the first account is the primary account for the purposes of our sidebar quick links
@@ -65,10 +65,6 @@ export const Sidebar: FC = () => {
6565
fetchNotifications();
6666
};
6767

68-
const notificationsCount = useMemo(() => {
69-
return getNotificationCount(notifications);
70-
}, [notifications]);
71-
7268
const sidebarButtonStyle = { color: 'white' };
7369

7470
return (
@@ -98,14 +94,14 @@ export const Sidebar: FC = () => {
9894
<IconButton
9995
aria-label="Notifications"
10096
data-testid="sidebar-notifications"
101-
description={`${notificationsCount} unread notifications ↗`}
97+
description={`${unreadCount} unread notifications ↗`}
10298
icon={BellIcon}
10399
onClick={() => openGitHubNotifications(primaryAccountHostname)}
104100
size="small"
105101
sx={sidebarButtonStyle}
106102
tooltipDirection="e"
107103
unsafeDisableTooltip={false}
108-
variant={notificationsCount > 0 ? 'primary' : 'invisible'}
104+
variant={hasNotifications ? 'primary' : 'invisible'}
109105
/>
110106

111107
{isLoggedIn && (

‎src/renderer/components/__snapshots__/Sidebar.test.tsx.snap‎

Lines changed: 8 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/renderer/components/notifications/AccountNotifications.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,9 @@ export const AccountNotifications: FC<IAccountNotifications> = (
127127
{showAccountNotifications && (
128128
<>
129129
{props.error && <Oops error={props.error} fullHeight={false} />}
130+
130131
{!hasNotifications && !props.error && <AllRead fullHeight={false} />}
132+
131133
{isGroupByRepository(settings)
132134
? groupedNotifications.map(([repoSlug, repoNotifications]) => (
133135
<RepositoryNotifications

‎src/renderer/context/App.test.tsx‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import * as apiRequests from '../utils/api/request';
1010
import * as comms from '../utils/comms';
1111
import * as notifications from '../utils/notifications/notifications';
1212
import * as storage from '../utils/storage';
13+
import * as tray from '../utils/tray';
1314
import { AppContext, AppProvider } from './App';
1415
import { defaultSettings } from './defaults';
1516

@@ -38,11 +39,13 @@ describe('renderer/context/App.tsx', () => {
3839
});
3940

4041
describe('notification methods', () => {
41-
const getNotificationCountMock = jest.spyOn(
42-
notifications,
43-
'getNotificationCount',
44-
);
45-
getNotificationCountMock.mockReturnValue(1);
42+
const setTrayIconColorAndTitleMock = jest
43+
.spyOn(tray, 'setTrayIconColorAndTitle')
44+
.mockImplementation(jest.fn());
45+
46+
jest
47+
.spyOn(notifications, 'getUnreadNotificationCount')
48+
.mockImplementation(jest.fn());
4649

4750
const fetchNotificationsMock = jest.fn();
4851
const markNotificationsAsReadMock = jest.fn();
@@ -70,8 +73,6 @@ describe('renderer/context/App.tsx', () => {
7073
it('fetch notifications every minute', async () => {
7174
customRender(null);
7275

73-
// Wait for the useEffects, for settings.participating and accounts, to run.
74-
// Those aren't what we're testing
7576
await waitFor(() =>
7677
expect(fetchNotificationsMock).toHaveBeenCalledTimes(1),
7778
);
@@ -80,23 +81,20 @@ describe('renderer/context/App.tsx', () => {
8081
jest.advanceTimersByTime(
8182
Constants.DEFAULT_FETCH_NOTIFICATIONS_INTERVAL_MS,
8283
);
83-
return;
8484
});
8585
expect(fetchNotificationsMock).toHaveBeenCalledTimes(2);
8686

8787
act(() => {
8888
jest.advanceTimersByTime(
8989
Constants.DEFAULT_FETCH_NOTIFICATIONS_INTERVAL_MS,
9090
);
91-
return;
9291
});
9392
expect(fetchNotificationsMock).toHaveBeenCalledTimes(3);
9493

9594
act(() => {
9695
jest.advanceTimersByTime(
9796
Constants.DEFAULT_FETCH_NOTIFICATIONS_INTERVAL_MS,
9897
);
99-
return;
10098
});
10199
expect(fetchNotificationsMock).toHaveBeenCalledTimes(4);
102100
});
@@ -144,6 +142,7 @@ describe('renderer/context/App.tsx', () => {
144142
mockDefaultState,
145143
[mockSingleNotification],
146144
);
145+
expect(setTrayIconColorAndTitleMock).toHaveBeenCalledTimes(1);
147146
});
148147

149148
it('should call markNotificationsAsDone', async () => {
@@ -169,6 +168,7 @@ describe('renderer/context/App.tsx', () => {
169168
mockDefaultState,
170169
[mockSingleNotification],
171170
);
171+
expect(setTrayIconColorAndTitleMock).toHaveBeenCalledTimes(1);
172172
});
173173

174174
it('should call unsubscribeNotification', async () => {
@@ -194,6 +194,7 @@ describe('renderer/context/App.tsx', () => {
194194
mockDefaultState,
195195
mockSingleNotification,
196196
);
197+
expect(setTrayIconColorAndTitleMock).toHaveBeenCalledTimes(1);
197198
});
198199
});
199200

‎src/renderer/context/App.tsx‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -47,17 +47,16 @@ import {
4747
setKeyboardShortcut,
4848
setUseAlternateIdleIcon,
4949
setUseUnreadActiveIcon,
50-
updateTrayColor,
51-
updateTrayTitle,
5250
} from '../utils/comms';
53-
import { getNotificationCount } from '../utils/notifications/notifications';
51+
import { getUnreadNotificationCount } from '../utils/notifications/notifications';
5452
import { clearState, loadState, saveState } from '../utils/storage';
5553
import {
5654
DEFAULT_DAY_COLOR_SCHEME,
5755
DEFAULT_NIGHT_COLOR_SCHEME,
5856
mapThemeModeToColorMode,
5957
mapThemeModeToColorScheme,
6058
} from '../utils/theme';
59+
import { setTrayIconColorAndTitle } from '../utils/tray';
6160
import { zoomPercentageToLevel } from '../utils/zoom';
6261
import { defaultAuth, defaultFilters, defaultSettings } from './defaults';
6362

@@ -75,6 +74,8 @@ interface AppContextState {
7574
globalError: GitifyError;
7675

7776
notifications: AccountNotifications[];
77+
unreadCount: number;
78+
hasNotifications: boolean;
7879
fetchNotifications: () => Promise<void>;
7980
removeAccountNotifications: (account: Account) => Promise<void>;
8081

@@ -110,6 +111,10 @@ export const AppProvider = ({ children }: { children: ReactNode }) => {
110111
unsubscribeNotification,
111112
} = useNotifications();
112113

114+
const unreadCount = getUnreadNotificationCount(notifications);
115+
116+
const hasNotifications = useMemo(() => unreadCount > 0, [unreadCount]);
117+
113118
// biome-ignore lint/correctness/useExhaustiveDependencies: restoreSettings is stable and should run only once
114119
useEffect(() => {
115120
restoreSettings();
@@ -166,24 +171,16 @@ export const AppProvider = ({ children }: { children: ReactNode }) => {
166171
}
167172
}, Constants.REFRESH_ACCOUNTS_INTERVAL_MS);
168173

174+
// biome-ignore lint/correctness/useExhaustiveDependencies: We also want to update the tray on setting changes
169175
useEffect(() => {
170-
const count = getNotificationCount(notifications);
171-
172-
let title = '';
173-
if (settings.showNotificationsCountInTray && count > 0) {
174-
title = count.toString();
175-
}
176-
177176
setUseUnreadActiveIcon(settings.useUnreadActiveIcon);
178177
setUseAlternateIdleIcon(settings.useAlternateIdleIcon);
179-
180-
updateTrayColor(count);
181-
updateTrayTitle(title);
178+
setTrayIconColorAndTitle(unreadCount, settings);
182179
}, [
183180
settings.showNotificationsCountInTray,
184181
settings.useUnreadActiveIcon,
185182
settings.useAlternateIdleIcon,
186-
notifications,
183+
unreadCount,
187184
]);
188185

189186
useEffect(() => {
@@ -362,6 +359,8 @@ export const AppProvider = ({ children }: { children: ReactNode }) => {
362359
globalError,
363360

364361
notifications,
362+
unreadCount,
363+
hasNotifications,
365364
fetchNotifications: fetchNotificationsWithAccounts,
366365

367366
markNotificationsAsRead: markNotificationsAsReadWithAccounts,
@@ -386,6 +385,8 @@ export const AppProvider = ({ children }: { children: ReactNode }) => {
386385
globalError,
387386

388387
notifications,
388+
unreadCount,
389+
hasNotifications,
389390
fetchNotificationsWithAccounts,
390391
markNotificationsAsReadWithAccounts,
391392
markNotificationsAsDoneWithAccounts,

0 commit comments

Comments
 (0)