Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
"webpack-cli": "6.0.1",
"webpack-merge": "6.0.1"
},
"packageManager": "pnpm@10.18.0",
"packageManager": "pnpm@10.17.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was the downgrade intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, timing issue

"pnpm": {
"onlyBuiltDependencies": [
"@biomejs/biome",
Expand Down
8 changes: 6 additions & 2 deletions src/main/events.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,12 @@ describe('main/events', () => {
it('sendRendererEvent forwards event to webContents with data', () => {
const send = jest.fn();
const mb: MockMenubar = { window: { webContents: { send } } };
sendRendererEvent(mb as unknown as Menubar, EVENTS.UPDATE_TITLE, 'title');
expect(send).toHaveBeenCalledWith(EVENTS.UPDATE_TITLE, 'title');
sendRendererEvent(
mb as unknown as Menubar,
EVENTS.UPDATE_ICON_TITLE,
'title',
);
expect(send).toHaveBeenCalledWith(EVENTS.UPDATE_ICON_TITLE, 'title');
});

it('sendRendererEvent forwards event without data', () => {
Expand Down
51 changes: 26 additions & 25 deletions src/main/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,33 +165,26 @@ app.whenReady().then(async () => {
EVENTS.USE_UNREAD_ACTIVE_ICON,
(_, useUnreadActiveIcon: boolean) => {
shouldUseUnreadActiveIcon = useUnreadActiveIcon;

if (shouldUseUnreadActiveIcon) {
setActiveIcon();
} else {
setIdleIcon();
}
},
);

onMainEvent(EVENTS.ICON_ERROR, () => {
onMainEvent(EVENTS.UPDATE_ICON_COLOR, (_, notificationsCount: number) => {
if (!mb.tray.isDestroyed()) {
mb.tray.setImage(TrayIcons.error);
}
});
if (notificationsCount < 0) {
setErrorIcon();
return;
}

onMainEvent(EVENTS.ICON_ACTIVE, () => {
if (!mb.tray.isDestroyed() && shouldUseUnreadActiveIcon) {
}
});
if (notificationsCount > 0) {
setActiveIcon();
return;
}

onMainEvent(EVENTS.ICON_IDLE, () => {
if (!mb.tray.isDestroyed()) {
setIdleIcon();
}
});
Comment on lines -177 to 185

Choose a reason for hiding this comment

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

This is a naive quick skim of this code; but the logic around setErrorIcon looks weird. Is that correct?

Might just be the diff making things look more confusing; but currently it sort of looks like the error icon will be used if notificationsCount < 0?

Choose a reason for hiding this comment

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

Actually, nevermind. Reading through the rest of it I see that -1 is used for an error state, so this makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for taking a look.

That's correct, at the moment a negative number is used to flag that the app is in error state.

Some further room for improvement


onMainEvent(EVENTS.UPDATE_TITLE, (_, title: string) => {
onMainEvent(EVENTS.UPDATE_ICON_TITLE, (_, title: string) => {
if (!mb.tray.isDestroyed()) {
mb.tray.setTitle(title);
}
Expand Down Expand Up @@ -256,14 +249,6 @@ const handleURL = (url: string) => {
}
};

function setActiveIcon() {
mb.tray.setImage(
menuBuilder.isUpdateAvailable()
? TrayIcons.activeWithUpdate
: TrayIcons.active,
);
}

function setIdleIcon() {
if (shouldUseAlternateIdleIcon) {
mb.tray.setImage(
Expand All @@ -279,3 +264,19 @@ function setIdleIcon() {
);
}
}

function setActiveIcon() {
if (shouldUseUnreadActiveIcon) {
mb.tray.setImage(
menuBuilder.isUpdateAvailable()
? TrayIcons.activeWithUpdate
: TrayIcons.active,
);
} else {
setIdleIcon();
}
}

function setErrorIcon() {
mb.tray.setImage(TrayIcons.error);
}
16 changes: 8 additions & 8 deletions src/preload/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class MockNotification {
MockNotification;

interface TestApi {
tray: { updateIcon: (n?: number) => void };
tray: { updateColor: (n?: number) => void };
openExternalLink: (u: string, f: boolean) => void;
app: { version: () => Promise<string>; show?: () => void; hide?: () => void };
onSystemThemeUpdate: (cb: (t: string) => void) => void;
Expand Down Expand Up @@ -90,15 +90,15 @@ describe('preload/index', () => {
expect(api).toHaveProperty('openExternalLink');
});

it('tray.updateIcon sends correct events', async () => {
it('tray.updateColor sends correct events', async () => {
await importPreload();
const api = (window as unknown as { gitify: TestApi }).gitify; // casting only in test boundary
api.tray.updateIcon(-1);
api.tray.updateIcon(5);
api.tray.updateIcon(0);
expect(sendMainEvent).toHaveBeenNthCalledWith(1, EVENTS.ICON_ERROR);
expect(sendMainEvent).toHaveBeenNthCalledWith(2, EVENTS.ICON_ACTIVE);
expect(sendMainEvent).toHaveBeenNthCalledWith(3, EVENTS.ICON_IDLE);
api.tray.updateColor(-1);
expect(sendMainEvent).toHaveBeenNthCalledWith(
1,
EVENTS.UPDATE_ICON_COLOR,
-1,
);
});

it('openExternalLink sends event with payload', async () => {
Expand Down
16 changes: 3 additions & 13 deletions src/preload/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,11 @@ export const api = {
},

tray: {
updateIcon: (notificationsLength = 0) => {
if (notificationsLength < 0) {
sendMainEvent(EVENTS.ICON_ERROR);
return;
}

if (notificationsLength > 0) {
sendMainEvent(EVENTS.ICON_ACTIVE);
return;
}

sendMainEvent(EVENTS.ICON_IDLE);
updateColor: (notificationsCount = 0) => {
sendMainEvent(EVENTS.UPDATE_ICON_COLOR, notificationsCount);
},

updateTitle: (title = '') => sendMainEvent(EVENTS.UPDATE_TITLE, title),
updateTitle: (title = '') => sendMainEvent(EVENTS.UPDATE_ICON_TITLE, title),

useAlternateIdleIcon: (value: boolean) =>
sendMainEvent(EVENTS.USE_ALTERNATE_IDLE_ICON, value),
Expand Down
9 changes: 6 additions & 3 deletions src/preload/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe('preload/utils', () => {
it('onRendererEvent registers listener and receives emitted data', () => {
const handler = jest.fn();
onRendererEvent(
EVENTS.UPDATE_TITLE,
EVENTS.UPDATE_ICON_TITLE,
handler as unknown as (
e: Electron.IpcRendererEvent,
args: string,
Expand All @@ -62,8 +62,11 @@ describe('preload/utils', () => {
ipcRenderer as unknown as {
__emit: (channel: string, ...a: unknown[]) => void;
}
).__emit(EVENTS.UPDATE_TITLE, 'payload');
expect(ipcRenderer.on).toHaveBeenCalledWith(EVENTS.UPDATE_TITLE, handler);
).__emit(EVENTS.UPDATE_ICON_TITLE, 'payload');
expect(ipcRenderer.on).toHaveBeenCalledWith(
EVENTS.UPDATE_ICON_TITLE,
handler,
);
expect(handler).toHaveBeenCalledWith({}, 'payload');
});
});
2 changes: 1 addition & 1 deletion src/renderer/__helpers__/jest.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ window.gitify = {
setLevel: jest.fn(),
},
tray: {
updateIcon: jest.fn(),
updateColor: jest.fn(),
updateTitle: jest.fn(),
useAlternateIdleIcon: jest.fn(),
useUnreadActiveIcon: jest.fn(),
Expand Down
25 changes: 12 additions & 13 deletions src/renderer/context/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
setKeyboardShortcut,
setUseAlternateIdleIcon,
setUseUnreadActiveIcon,
updateTrayColor,
updateTrayTitle,
} from '../utils/comms';
import { getNotificationCount } from '../utils/notifications/notifications';
Expand Down Expand Up @@ -154,24 +155,22 @@ export const AppProvider = ({ children }: { children: ReactNode }) => {
useEffect(() => {
const count = getNotificationCount(notifications);

let title = '';
if (settings.showNotificationsCountInTray && count > 0) {
updateTrayTitle(count.toString());
} else {
updateTrayTitle();
title = count.toString();
}
}, [settings.showNotificationsCountInTray, notifications]);

useEffect(() => {
const count = getNotificationCount(notifications);

setUseUnreadActiveIcon(settings.useUnreadActiveIcon);

updateTrayTitle(count.toString());
}, [settings.useUnreadActiveIcon, notifications]);

useEffect(() => {
setUseAlternateIdleIcon(settings.useAlternateIdleIcon);
}, [settings.useAlternateIdleIcon]);

updateTrayColor(count);
updateTrayTitle(title);
}, [
settings.showNotificationsCountInTray,
settings.useUnreadActiveIcon,
settings.useAlternateIdleIcon,
notifications,
]);

useEffect(() => {
setKeyboardShortcut(settings.keyboardShortcut);
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/hooks/useNotifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
markNotificationThreadAsDone,
markNotificationThreadAsRead,
} from '../utils/api/client';
import { updateTrayIcon } from '../utils/comms';
import { updateTrayColor } from '../utils/comms';
import { isMarkAsDoneFeatureSupported } from '../utils/features';
import { rendererLogError } from '../utils/logger';
import { triggerNativeNotifications } from '../utils/notifications/native';
Expand Down Expand Up @@ -93,7 +93,7 @@ export const useNotifications = (): NotificationsState => {
if (allAccountsHaveErrors) {
setStatus('error');
setGlobalError(accountErrorsAreAllSame ? accountError : null);
updateTrayIcon(-1);
updateTrayColor(-1);
return;
}

Expand Down
7 changes: 4 additions & 3 deletions src/renderer/routes/Accounts.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ describe('renderer/routes/Accounts.tsx', () => {

it('should logout', async () => {
const logoutFromAccountMock = jest.fn();
const updateTrayIconMock = jest.spyOn(comms, 'updateTrayIcon');
const updateTrayColorMock = jest.spyOn(comms, 'updateTrayColor');
const updateTrayTitleMock = jest.spyOn(comms, 'updateTrayTitle');

await act(async () => {
Expand All @@ -280,10 +280,11 @@ describe('renderer/routes/Accounts.tsx', () => {
await userEvent.click(screen.getByTestId('account-logout'));

expect(logoutFromAccountMock).toHaveBeenCalledTimes(1);
expect(updateTrayIconMock).toHaveBeenCalledTimes(1);
expect(updateTrayIconMock).toHaveBeenCalledWith();
expect(updateTrayColorMock).toHaveBeenCalledTimes(1);
expect(updateTrayColorMock).toHaveBeenCalledWith();
expect(updateTrayTitleMock).toHaveBeenCalledTimes(1);
expect(updateTrayTitleMock).toHaveBeenCalledWith();

expect(mockNavigate).toHaveBeenNthCalledWith(1, -1);
});
});
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/routes/Accounts.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
getAccountUUID,
refreshAccount,
} from '../utils/auth/utils';
import { updateTrayIcon, updateTrayTitle } from '../utils/comms';
import { updateTrayColor, updateTrayTitle } from '../utils/comms';
import { getAuthMethodIcon, getPlatformIcon } from '../utils/icons';
import {
openAccountProfile,
Expand All @@ -57,7 +57,7 @@ export const AccountsRoute: FC = () => {
(account: Account) => {
logoutFromAccount(account);
navigate(-1);
updateTrayIcon();
updateTrayColor();
updateTrayTitle();
},
[logoutFromAccount],
Expand Down
18 changes: 10 additions & 8 deletions src/renderer/utils/comms.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
setKeyboardShortcut,
setUseAlternateIdleIcon,
showWindow,
updateTrayIcon,
updateTrayColor,
updateTrayTitle,
} from './comms';
import * as storage from './storage';
Expand Down Expand Up @@ -140,16 +140,17 @@ describe('renderer/utils/comms.ts', () => {
});

describe('tray helpers', () => {
it('updates tray icon with count', () => {
updateTrayIcon(5);
it('updates tray icon color with count', () => {
updateTrayColor(5);

expect(window.gitify.tray.updateIcon).toHaveBeenCalledTimes(1);
expect(window.gitify.tray.updateIcon).toHaveBeenCalledWith(5);
expect(window.gitify.tray.updateColor).toHaveBeenCalledTimes(1);
expect(window.gitify.tray.updateColor).toHaveBeenCalledWith(5);
});

it('updates tray icon with default count', () => {
updateTrayIcon();
expect(window.gitify.tray.updateIcon).toHaveBeenCalledTimes(1);
it('updates tray icon color with default count', () => {
updateTrayColor();

expect(window.gitify.tray.updateColor).toHaveBeenCalledTimes(1);
});

it('updates tray title with provided value', () => {
Expand All @@ -161,6 +162,7 @@ describe('renderer/utils/comms.ts', () => {

it('updates tray title with default value', () => {
updateTrayTitle();

expect(window.gitify.tray.updateTitle).toHaveBeenCalledTimes(1);
});
});
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/utils/comms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ export function setKeyboardShortcut(keyboardShortcut: boolean): void {
window.gitify.setKeyboardShortcut(keyboardShortcut);
}

export function updateTrayIcon(notificationsLength = 0): void {
window.gitify.tray.updateIcon(notificationsLength);
export function updateTrayColor(notificationsLength = 0): void {
window.gitify.tray.updateColor(notificationsLength);
}

export function updateTrayTitle(title = ''): void {
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/utils/notifications/notifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
import type { GitifySubject, Notification } from '../../typesGitHub';
import { listNotificationsForAuthenticatedUser } from '../api/client';
import { determineFailureType } from '../api/errors';
import { updateTrayIcon } from '../comms';
import { updateTrayColor } from '../comms';
import { rendererLogError, rendererLogWarn } from '../logger';
import {
filterBaseNotifications,
Expand All @@ -17,7 +17,7 @@ import { createNotificationHandler } from './handlers';
export function setTrayIconColor(notifications: AccountNotifications[]) {
const allNotificationsCount = getNotificationCount(notifications);

updateTrayIcon(allNotificationsCount);
updateTrayColor(allNotificationsCount);
}

export function getNotificationCount(notifications: AccountNotifications[]) {
Expand Down
7 changes: 3 additions & 4 deletions src/shared/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@ const P = APPLICATION.EVENT_PREFIX;

export const EVENTS = {
AUTH_CALLBACK: `${P}auth-callback`,
ICON_IDLE: `${P}icon-idle`,
ICON_ACTIVE: `${P}icon-active`,
ICON_ERROR: `${P}icon-error`,
QUIT: `${P}quit`,
WINDOW_SHOW: `${P}window-show`,
WINDOW_HIDE: `${P}window-hide`,
VERSION: `${P}version`,
UPDATE_TITLE: `${P}update-title`,
UPDATE_ICON_COLOR: `${P}update-icon-color`,
UPDATE_ICON_TITLE: `${P}update-icon-title`,
USE_ALTERNATE_IDLE_ICON: `${P}use-alternate-idle-icon`,
USE_UNREAD_ACTIVE_ICON: `${P}use-unread-active-icon`,
UPDATE_KEYBOARD_SHORTCUT: `${P}update-keyboard-shortcut`,
Expand Down Expand Up @@ -44,6 +42,7 @@ export interface IOpenExternal {

export type EventData =
| string
| number
| boolean
| IKeyboardShortcut
| IAutoLaunch
Expand Down