Skip to content

Commit 061e720

Browse files
setchysetchy
authored
feat(filters): split into two stages (base, detailed) to reduce api calls (#2156)
feat(filters): split into two stages (base, detailed) to reduce wasted api calls Signed-off-by: Adam Setch <[email protected]>
1 parent 4e8186c commit 061e720

File tree

3 files changed

+123
-86
lines changed

3 files changed

+123
-86
lines changed

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

Lines changed: 71 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@ import { partialMockNotification } from '../../../__mocks__/partial-mocks';
22
import { mockSettings } from '../../../__mocks__/state-mocks';
33
import { defaultSettings } from '../../../context/App';
44
import type { Link, SettingsState } from '../../../types';
5-
import { filterNotifications, hasAnyFiltersSet } from './filter';
5+
import {
6+
filterBaseNotifications,
7+
filterDetailedNotifications,
8+
hasAnyFiltersSet,
9+
} from './filter';
610

711
describe('renderer/utils/notifications/filters/filter.ts', () => {
812
afterEach(() => {
@@ -33,87 +37,91 @@ describe('renderer/utils/notifications/filters/filter.ts', () => {
3337
}),
3438
];
3539

36-
it('should ignore user type, handle filters and state filters if detailed notifications not enabled', async () => {
37-
const result = filterNotifications(mockNotifications, {
38-
...mockSettings,
39-
detailedNotifications: false,
40-
filterUserTypes: ['Bot'],
41-
filterIncludeHandles: ['github-user'],
42-
filterExcludeHandles: ['github-bot'],
43-
filterStates: ['merged'],
40+
describe('filterBaseNotifications', () => {
41+
it('should filter notifications by subject type when provided', async () => {
42+
mockNotifications[0].subject.type = 'Issue';
43+
mockNotifications[1].subject.type = 'PullRequest';
44+
const result = filterBaseNotifications(mockNotifications, {
45+
...mockSettings,
46+
filterSubjectTypes: ['Issue'],
47+
});
48+
49+
expect(result.length).toBe(1);
50+
expect(result).toEqual([mockNotifications[0]]);
4451
});
4552

46-
expect(result.length).toBe(2);
47-
expect(result).toEqual(mockNotifications);
48-
});
53+
it('should filter notifications by reasons when provided', async () => {
54+
mockNotifications[0].reason = 'subscribed';
55+
mockNotifications[1].reason = 'manual';
56+
const result = filterBaseNotifications(mockNotifications, {
57+
...mockSettings,
58+
filterReasons: ['manual'],
59+
});
4960

50-
it('should filter notifications by user type provided', async () => {
51-
const result = filterNotifications(mockNotifications, {
52-
...mockSettings,
53-
detailedNotifications: true,
54-
filterUserTypes: ['Bot'],
61+
expect(result.length).toBe(1);
62+
expect(result).toEqual([mockNotifications[1]]);
5563
});
56-
57-
expect(result.length).toBe(1);
58-
expect(result).toEqual([mockNotifications[1]]);
5964
});
6065

61-
it('should filter notifications that match include user handle', async () => {
62-
const result = filterNotifications(mockNotifications, {
63-
...mockSettings,
64-
detailedNotifications: true,
65-
filterIncludeHandles: ['github-user'],
66+
describe('filterDetailedNotifications', () => {
67+
it('should ignore user type, handle filters and state filters if detailed notifications not enabled', async () => {
68+
const result = filterDetailedNotifications(mockNotifications, {
69+
...mockSettings,
70+
detailedNotifications: false,
71+
filterUserTypes: ['Bot'],
72+
filterIncludeHandles: ['github-user'],
73+
filterExcludeHandles: ['github-bot'],
74+
filterStates: ['merged'],
75+
});
76+
77+
expect(result.length).toBe(2);
78+
expect(result).toEqual(mockNotifications);
6679
});
6780

68-
expect(result.length).toBe(1);
69-
expect(result).toEqual([mockNotifications[0]]);
70-
});
81+
it('should filter notifications by user type provided', async () => {
82+
const result = filterDetailedNotifications(mockNotifications, {
83+
...mockSettings,
84+
detailedNotifications: true,
85+
filterUserTypes: ['Bot'],
86+
});
7187

72-
it('should filter notifications that match exclude user handle', async () => {
73-
const result = filterNotifications(mockNotifications, {
74-
...mockSettings,
75-
detailedNotifications: true,
76-
filterExcludeHandles: ['github-bot'],
88+
expect(result.length).toBe(1);
89+
expect(result).toEqual([mockNotifications[1]]);
7790
});
7891

79-
expect(result.length).toBe(1);
80-
expect(result).toEqual([mockNotifications[0]]);
81-
});
92+
it('should filter notifications that match include user handle', async () => {
93+
const result = filterDetailedNotifications(mockNotifications, {
94+
...mockSettings,
95+
detailedNotifications: true,
96+
filterIncludeHandles: ['github-user'],
97+
});
8298

83-
it('should filter notifications by subject type when provided', async () => {
84-
mockNotifications[0].subject.type = 'Issue';
85-
mockNotifications[1].subject.type = 'PullRequest';
86-
const result = filterNotifications(mockNotifications, {
87-
...mockSettings,
88-
filterSubjectTypes: ['Issue'],
99+
expect(result.length).toBe(1);
100+
expect(result).toEqual([mockNotifications[0]]);
89101
});
90102

91-
expect(result.length).toBe(1);
92-
expect(result).toEqual([mockNotifications[0]]);
93-
});
103+
it('should filter notifications that match exclude user handle', async () => {
104+
const result = filterDetailedNotifications(mockNotifications, {
105+
...mockSettings,
106+
detailedNotifications: true,
107+
filterExcludeHandles: ['github-bot'],
108+
});
94109

95-
it('should filter notifications by state when provided', async () => {
96-
mockNotifications[0].subject.state = 'open';
97-
mockNotifications[1].subject.state = 'closed';
98-
const result = filterNotifications(mockNotifications, {
99-
...mockSettings,
100-
filterStates: ['closed'],
110+
expect(result.length).toBe(1);
111+
expect(result).toEqual([mockNotifications[0]]);
101112
});
102113

103-
expect(result.length).toBe(1);
104-
expect(result).toEqual([mockNotifications[1]]);
105-
});
114+
it('should filter notifications by state when provided', async () => {
115+
mockNotifications[0].subject.state = 'open';
116+
mockNotifications[1].subject.state = 'closed';
117+
const result = filterDetailedNotifications(mockNotifications, {
118+
...mockSettings,
119+
filterStates: ['closed'],
120+
});
106121

107-
it('should filter notifications by reasons when provided', async () => {
108-
mockNotifications[0].reason = 'subscribed';
109-
mockNotifications[1].reason = 'manual';
110-
const result = filterNotifications(mockNotifications, {
111-
...mockSettings,
112-
filterReasons: ['manual'],
122+
expect(result.length).toBe(1);
123+
expect(result).toEqual([mockNotifications[1]]);
113124
});
114-
115-
expect(result.length).toBe(1);
116-
expect(result).toEqual([mockNotifications[1]]);
117125
});
118126
});
119127

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

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,34 @@ import {
1010
userTypeFilter,
1111
} from '.';
1212

13-
export function filterNotifications(
13+
export function filterBaseNotifications(
14+
notifications: Notification[],
15+
settings: SettingsState,
16+
): Notification[] {
17+
return notifications.filter((notification) => {
18+
let passesFilters = true;
19+
20+
if (subjectTypeFilter.hasFilters(settings)) {
21+
passesFilters =
22+
passesFilters &&
23+
settings.filterSubjectTypes.some((subjectType) =>
24+
subjectTypeFilter.filterNotification(notification, subjectType),
25+
);
26+
}
27+
28+
if (reasonFilter.hasFilters(settings)) {
29+
passesFilters =
30+
passesFilters &&
31+
settings.filterReasons.some((reason) =>
32+
reasonFilter.filterNotification(notification, reason),
33+
);
34+
}
35+
36+
return passesFilters;
37+
});
38+
}
39+
40+
export function filterDetailedNotifications(
1441
notifications: Notification[],
1542
settings: SettingsState,
1643
): Notification[] {
@@ -51,22 +78,6 @@ export function filterNotifications(
5178
}
5279
}
5380

54-
if (subjectTypeFilter.hasFilters(settings)) {
55-
passesFilters =
56-
passesFilters &&
57-
settings.filterSubjectTypes.some((subjectType) =>
58-
subjectTypeFilter.filterNotification(notification, subjectType),
59-
);
60-
}
61-
62-
if (reasonFilter.hasFilters(settings)) {
63-
passesFilters =
64-
passesFilters &&
65-
settings.filterReasons.some((reason) =>
66-
reasonFilter.filterNotification(notification, reason),
67-
);
68-
}
69-
7081
return passesFilters;
7182
});
7283
}

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

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,18 @@
11
import { logError, logWarn } from '../../../shared/logger';
2-
import type { AccountNotifications, GitifyState } from '../../types';
2+
import type {
3+
AccountNotifications,
4+
GitifyState,
5+
SettingsState,
6+
} from '../../types';
37
import type { GitifySubject, Notification } from '../../typesGitHub';
48
import { listNotificationsForAuthenticatedUser } from '../api/client';
59
import { determineFailureType } from '../api/errors';
610
import { updateTrayIcon } from '../comms';
711
import { getGitifySubjectDetails } from '../subject';
8-
import { filterNotifications } from './filters/filter';
12+
import {
13+
filterBaseNotifications,
14+
filterDetailedNotifications,
15+
} from './filters/filter';
916

1017
export function setTrayIconColor(notifications: AccountNotifications[]) {
1118
const allNotificationsCount = getNotificationCount(notifications);
@@ -49,9 +56,20 @@ export async function getAllNotifications(
4956
account: accountNotifications.account,
5057
}));
5158

52-
notifications = await enrichNotifications(notifications, state);
59+
notifications = filterBaseNotifications(
60+
notifications,
61+
state.settings,
62+
);
5363

54-
notifications = filterNotifications(notifications, state.settings);
64+
notifications = await enrichNotifications(
65+
notifications,
66+
state.settings,
67+
);
68+
69+
notifications = filterDetailedNotifications(
70+
notifications,
71+
state.settings,
72+
);
5573

5674
return {
5775
account: accountNotifications.account,
@@ -79,9 +97,9 @@ export async function getAllNotifications(
7997

8098
export async function enrichNotifications(
8199
notifications: Notification[],
82-
state: GitifyState,
100+
settings: SettingsState,
83101
): Promise<Notification[]> {
84-
if (!state.settings.detailedNotifications) {
102+
if (!settings.detailedNotifications) {
85103
return notifications;
86104
}
87105

0 commit comments

Comments
 (0)