Skip to content

Commit 46db36d

Browse files
setchysetchy
committed
Merge branch 'main' of https://github.com/gitify-app/gitify 
2 parents fc31f7d + 0863411 commit 46db36d

File tree

6 files changed

+270
-36
lines changed

6 files changed

+270
-36
lines changed

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import type { AccountNotifications } from '../types';
2-
import type { StateType, Subject, SubjectType } from '../typesGitHub';
2+
import type {
3+
Notification,
4+
StateType,
5+
Subject,
6+
SubjectType,
7+
} from '../typesGitHub';
38
import {
49
mockEnterpriseNotifications,
510
mockGitHubNotifications,
@@ -44,3 +49,14 @@ export function createSubjectMock(mocks: {
4449
latest_comment_url: null,
4550
};
4651
}
52+
53+
export function createMockNotificationForRepoName(
54+
id: string,
55+
repoFullName: string | null,
56+
): Notification {
57+
return {
58+
id,
59+
repository: repoFullName ? { full_name: repoFullName } : null,
60+
account: mockGitHubCloudAccount,
61+
} as Notification;
62+
}

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ interface IAccountNotifications {
3535
export const AccountNotifications: FC<IAccountNotifications> = (
3636
props: IAccountNotifications,
3737
) => {
38-
const { account, showAccountHeader, error, notifications } = props;
38+
const { account, showAccountHeader, notifications } = props;
3939

4040
const { settings } = useContext(AppContext);
4141

@@ -48,12 +48,10 @@ export const AccountNotifications: FC<IAccountNotifications> = (
4848
);
4949

5050
const groupedNotifications = useMemo(() => {
51-
const map = groupNotificationsByRepository([
52-
{ account, error, notifications: sortedNotifications },
53-
]);
51+
const map = groupNotificationsByRepository(sortedNotifications);
5452

5553
return Array.from(map.entries());
56-
}, [account, error, sortedNotifications]);
54+
}, [sortedNotifications]);
5755

5856
const hasNotifications = useMemo(
5957
() => notifications.length > 0,

‎src/renderer/utils/notifications/group.test.ts‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
import { createMockNotificationForRepoName } from '../../__mocks__/notifications-mocks';
2+
import { mockSettings } from '../../__mocks__/state-mocks';
3+
import { GroupBy } from '../../types';
4+
import type { Notification } from '../../typesGitHub';
5+
import {
6+
getFlattenedNotificationsByRepo,
7+
groupNotificationsByRepository,
8+
isGroupByRepository,
9+
} from './group';
10+
11+
describe('renderer/utils/notifications/group.ts', () => {
12+
describe('isGroupByRepository', () => {
13+
it('returns true when groupBy is REPOSITORY', () => {
14+
const settings = { ...mockSettings, groupBy: GroupBy.REPOSITORY };
15+
expect(isGroupByRepository(settings)).toBe(true);
16+
});
17+
18+
it('returns false when groupBy is DATE', () => {
19+
const settings = { ...mockSettings, groupBy: GroupBy.DATE };
20+
expect(isGroupByRepository(settings)).toBe(false);
21+
});
22+
});
23+
24+
describe('groupNotificationsByRepository', () => {
25+
it('groups notifications by repository full_name', () => {
26+
const notifications: Notification[] = [
27+
createMockNotificationForRepoName('1', 'owner/repo-a'),
28+
createMockNotificationForRepoName('2', 'owner/repo-b'),
29+
createMockNotificationForRepoName('3', 'owner/repo-a'),
30+
];
31+
32+
const result = groupNotificationsByRepository(notifications);
33+
34+
expect(result.size).toBe(2);
35+
expect(result.get('owner/repo-a')?.map((n) => n.id)).toEqual(['1', '3']);
36+
expect(result.get('owner/repo-b')?.map((n) => n.id)).toEqual(['2']);
37+
});
38+
39+
it('preserves first-seen repository order', () => {
40+
const notifications: Notification[] = [
41+
createMockNotificationForRepoName('1', 'owner/repo-c'),
42+
createMockNotificationForRepoName('2', 'owner/repo-a'),
43+
createMockNotificationForRepoName('3', 'owner/repo-b'),
44+
createMockNotificationForRepoName('4', 'owner/repo-a'),
45+
];
46+
47+
const result = groupNotificationsByRepository(notifications);
48+
const keys = Array.from(result.keys());
49+
50+
expect(keys).toEqual(['owner/repo-c', 'owner/repo-a', 'owner/repo-b']);
51+
});
52+
53+
it('skips notifications without repository data', () => {
54+
const notifications: Notification[] = [
55+
createMockNotificationForRepoName('1', 'owner/repo-a'),
56+
createMockNotificationForRepoName('2', null),
57+
createMockNotificationForRepoName('3', 'owner/repo-a'),
58+
];
59+
60+
const result = groupNotificationsByRepository(notifications);
61+
62+
expect(result.size).toBe(1);
63+
expect(result.get('owner/repo-a')?.map((n) => n.id)).toEqual(['1', '3']);
64+
});
65+
66+
it('returns empty map when no notifications', () => {
67+
const notifications: Notification[] = [];
68+
69+
const result = groupNotificationsByRepository(notifications);
70+
71+
expect(result.size).toBe(0);
72+
});
73+
74+
it('returns empty map when all notifications lack repository data', () => {
75+
const notifications: Notification[] = [
76+
createMockNotificationForRepoName('1', null),
77+
createMockNotificationForRepoName('2', null),
78+
];
79+
80+
const result = groupNotificationsByRepository(notifications);
81+
82+
expect(result.size).toBe(0);
83+
});
84+
});
85+
86+
describe('getFlattenedNotificationsByRepo', () => {
87+
it('returns repository-grouped order when groupBy is REPOSITORY', () => {
88+
const settings = { ...mockSettings, groupBy: GroupBy.REPOSITORY };
89+
const notifications: Notification[] = [
90+
createMockNotificationForRepoName('1', 'owner/repo-b'),
91+
createMockNotificationForRepoName('2', 'owner/repo-a'),
92+
createMockNotificationForRepoName('3', 'owner/repo-b'),
93+
createMockNotificationForRepoName('4', 'owner/repo-a'),
94+
];
95+
96+
const result = getFlattenedNotificationsByRepo(notifications, settings);
97+
98+
// First repo-b notifications, then repo-a notifications
99+
expect(result.map((n) => n.id)).toEqual(['1', '3', '2', '4']);
100+
});
101+
102+
it('returns natural account order when groupBy is DATE', () => {
103+
const settings = { ...mockSettings, groupBy: GroupBy.DATE };
104+
const notifications: Notification[] = [
105+
createMockNotificationForRepoName('1', 'owner/repo-b'),
106+
createMockNotificationForRepoName('2', 'owner/repo-a'),
107+
createMockNotificationForRepoName('3', 'owner/repo-b'),
108+
];
109+
110+
const result = getFlattenedNotificationsByRepo(notifications, settings);
111+
112+
// Natural order preserved
113+
expect(result.map((n) => n.id)).toEqual(['1', '2', '3']);
114+
});
115+
116+
it('returns empty array when no notifications', () => {
117+
const settings = { ...mockSettings, groupBy: GroupBy.REPOSITORY };
118+
const notifications: Notification[] = [];
119+
120+
const result = getFlattenedNotificationsByRepo(notifications, settings);
121+
122+
expect(result).toEqual([]);
123+
});
124+
125+
it('handles notifications without repository data when grouped', () => {
126+
const settings = { ...mockSettings, groupBy: GroupBy.REPOSITORY };
127+
const notifications: Notification[] = [
128+
createMockNotificationForRepoName('1', 'owner/repo-a'),
129+
createMockNotificationForRepoName('2', null),
130+
createMockNotificationForRepoName('3', 'owner/repo-a'),
131+
];
132+
133+
const result = getFlattenedNotificationsByRepo(notifications, settings);
134+
135+
// Only notifications with repository data are included when grouped
136+
expect(result.map((n) => n.id)).toEqual(['1', '3']);
137+
});
138+
139+
it('preserves notifications without repository data when not grouped', () => {
140+
const settings = { ...mockSettings, groupBy: GroupBy.DATE };
141+
const notifications: Notification[] = [
142+
createMockNotificationForRepoName('1', 'owner/repo-a'),
143+
createMockNotificationForRepoName('2', null),
144+
createMockNotificationForRepoName('3', 'owner/repo-a'),
145+
];
146+
147+
const result = getFlattenedNotificationsByRepo(notifications, settings);
148+
149+
// All notifications preserved in natural order
150+
expect(result.map((n) => n.id)).toEqual(['1', '2', '3']);
151+
});
152+
});
153+
});

‎src/renderer/utils/notifications/group.ts‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { AccountNotifications, SettingsState } from '../../types';
1+
import type { SettingsState } from '../../types';
22
import type { Notification } from '../../typesGitHub';
33

44
/**
@@ -14,26 +14,24 @@ export function isGroupByRepository(settings: SettingsState) {
1414
* Skips notifications without valid repository data.
1515
*/
1616
export function groupNotificationsByRepository(
17-
accounts: AccountNotifications[],
17+
notifications: Notification[],
1818
): Map<string, Notification[]> {
1919
const repoGroups = new Map<string, Notification[]>();
2020

21-
for (const account of accounts) {
22-
for (const notification of account.notifications) {
23-
const repo = notification.repository?.full_name;
21+
for (const notification of notifications) {
22+
const repo = notification.repository?.full_name;
2423

25-
// Skip notifications without valid repository data
26-
if (!repo) {
27-
continue;
28-
}
24+
// Skip notifications without valid repository data
25+
if (!repo) {
26+
continue;
27+
}
2928

30-
const group = repoGroups.get(repo);
29+
const group = repoGroups.get(repo);
3130

32-
if (group) {
33-
group.push(notification);
34-
} else {
35-
repoGroups.set(repo, [notification]);
36-
}
31+
if (group) {
32+
group.push(notification);
33+
} else {
34+
repoGroups.set(repo, [notification]);
3735
}
3836
}
3937

@@ -45,13 +43,14 @@ export function groupNotificationsByRepository(
4543
* (when grouped) or the natural account->notification order otherwise.
4644
*/
4745
export function getFlattenedNotificationsByRepo(
48-
accounts: AccountNotifications[],
46+
notifications: Notification[],
4947
settings: SettingsState,
5048
): Notification[] {
5149
if (isGroupByRepository(settings)) {
52-
const repoGroups = groupNotificationsByRepository(accounts);
53-
return Array.from(repoGroups.values()).flat();
50+
const groupedNotifications = groupNotificationsByRepository(notifications);
51+
52+
return Array.from(groupedNotifications.values()).flat();
5453
}
5554

56-
return accounts.flatMap((a) => a.notifications);
55+
return notifications;
5756
}

‎src/renderer/utils/notifications/notifications.test.ts‎

Lines changed: 70 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,29 @@
11
import axios from 'axios';
22
import nock from 'nock';
33

4-
import { mockSingleAccountNotifications } from '../../__mocks__/notifications-mocks';
4+
import {
5+
createMockNotificationForRepoName,
6+
mockSingleAccountNotifications,
7+
} from '../../__mocks__/notifications-mocks';
58
import { partialMockNotification } from '../../__mocks__/partial-mocks';
6-
import { mockSettings } from '../../__mocks__/state-mocks';
7-
import type { Link } from '../../types';
9+
import {
10+
mockGitHubCloudAccount,
11+
mockGitHubEnterpriseServerAccount,
12+
mockSettings,
13+
} from '../../__mocks__/state-mocks';
14+
import {
15+
type AccountNotifications,
16+
GroupBy,
17+
type Link,
18+
type SettingsState,
19+
} from '../../types';
820
import type { Repository } from '../../typesGitHub';
921
import * as logger from '../../utils/logger';
10-
import { enrichNotification, getNotificationCount } from './notifications';
22+
import {
23+
enrichNotification,
24+
getNotificationCount,
25+
stabilizeNotificationsOrder,
26+
} from './notifications';
1127

1228
describe('renderer/utils/notifications/notifications.ts', () => {
1329
beforeEach(() => {
@@ -55,4 +71,54 @@ describe('renderer/utils/notifications/notifications.ts', () => {
5571
mockNotification,
5672
);
5773
});
74+
75+
describe('stabilizeNotificationsOrder', () => {
76+
const acc1: AccountNotifications = {
77+
account: mockGitHubCloudAccount,
78+
notifications: [
79+
createMockNotificationForRepoName('a1', 'owner/repo-1'),
80+
createMockNotificationForRepoName('a2', 'owner/repo-2'),
81+
createMockNotificationForRepoName('a3', 'owner/repo-1'),
82+
],
83+
error: null,
84+
};
85+
86+
const acc2: AccountNotifications = {
87+
account: mockGitHubEnterpriseServerAccount,
88+
notifications: [
89+
createMockNotificationForRepoName('b1', 'owner/repo-3'),
90+
createMockNotificationForRepoName('b2', 'owner/repo-4'),
91+
createMockNotificationForRepoName('b3', 'owner/repo-3'),
92+
],
93+
error: null,
94+
};
95+
96+
const mockAccounts: AccountNotifications[] = [acc1, acc2];
97+
98+
it('assigns sequential order across all notifications when not grouped (DATE)', () => {
99+
const settings: SettingsState = {
100+
...mockSettings,
101+
groupBy: GroupBy.DATE,
102+
};
103+
104+
stabilizeNotificationsOrder(mockAccounts, settings);
105+
106+
expect(
107+
mockAccounts.flatMap((acc) => acc.notifications).map((n) => n.order),
108+
).toEqual([0, 1, 2, 3, 4, 5]);
109+
});
110+
111+
it('groups by repository when REPOSITORY and assigns order in first-seen repo groups', () => {
112+
const settings: SettingsState = {
113+
...mockSettings,
114+
groupBy: GroupBy.REPOSITORY,
115+
};
116+
117+
stabilizeNotificationsOrder(mockAccounts, settings);
118+
119+
expect(
120+
mockAccounts.flatMap((acc) => acc.notifications).map((n) => n.order),
121+
).toEqual([0, 2, 1, 3, 5, 4]);
122+
});
123+
});
58124
});

‎src/renderer/utils/notifications/notifications.ts‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -157,14 +157,16 @@ export function stabilizeNotificationsOrder(
157157
notifications: AccountNotifications[],
158158
settings: SettingsState,
159159
) {
160-
const flattenedNotifications = getFlattenedNotificationsByRepo(
161-
notifications,
162-
settings,
163-
);
164-
165160
let orderIndex = 0;
166161

167-
for (const n of flattenedNotifications) {
168-
n.order = orderIndex++;
162+
for (const account of notifications) {
163+
const flattenedNotifications = getFlattenedNotificationsByRepo(
164+
account.notifications,
165+
settings,
166+
);
167+
168+
for (const n of flattenedNotifications) {
169+
n.order = orderIndex++;
170+
}
169171
}
170172
}

0 commit comments

Comments
 (0)