Skip to content
Open
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
20 changes: 8 additions & 12 deletions src/renderer/components/notifications/AccountNotifications.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export const AccountNotifications: FC<IAccountNotifications> = (
{ account, error, notifications: sortedNotifications },
]);

return Array.from(map.values());
return Array.from(map.entries());
}, [account, error, sortedNotifications]);

const hasNotifications = useMemo(
Expand Down Expand Up @@ -131,17 +131,13 @@ export const AccountNotifications: FC<IAccountNotifications> = (
{props.error && <Oops error={props.error} fullHeight={false} />}
{!hasNotifications && !props.error && <AllRead fullHeight={false} />}
{isGroupByRepository(settings)
? groupedNotifications.map((repoNotifications) => {
const repoSlug = repoNotifications[0].repository.full_name;

return (
<RepositoryNotifications
key={repoSlug}
repoName={repoSlug}
repoNotifications={repoNotifications}
/>
);
})
? groupedNotifications.map(([repoSlug, repoNotifications]) => (
<RepositoryNotifications
key={repoSlug}
repoName={repoSlug}
repoNotifications={repoNotifications}
/>
))
: sortedNotifications.map((notification) => (
<NotificationRow
key={notification.id}
Expand Down
2 changes: 0 additions & 2 deletions src/renderer/hooks/useNotifications.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,6 @@ describe('renderer/hooks/useNotifications.ts', () => {
});

expect(result.current.notifications.length).toBe(0);
expect(mockSingleNotification.unread).toBeFalsy();
});

it('should mark notifications as read with failure', async () => {
Expand Down Expand Up @@ -414,7 +413,6 @@ describe('renderer/hooks/useNotifications.ts', () => {
});

expect(result.current.notifications.length).toBe(0);
expect(mockSingleNotification.unread).toBeFalsy();
});

it('should mark notifications as done with failure', async () => {
Expand Down
38 changes: 28 additions & 10 deletions src/renderer/hooks/useNotifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,25 @@ import {
import { removeNotifications } from '../utils/notifications/remove';

/**
* Apply optimistic local updates for read state. This helps with some
* rendering edge cases between fetch notification intervals.
* Apply read state updates to all notifications, replacing the target notifications
* with new objects that have unread: false. This is used for optimistic UI updates.
*/
function markNotificationsAsReadLocally(targetNotifications: Notification[]) {
for (const n of targetNotifications) {
n.unread = false;
}
function applyReadStateToNotifications(
Copy link
Member Author

Choose a reason for hiding this comment

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

prevents any direct state mutation

allNotifications: AccountNotifications[],
readNotifications: Notification[],
): AccountNotifications[] {
const readNotificationIDs = new Set<string>(
readNotifications.map((notification) => notification.id),
);

return allNotifications.map((accountNotifications) => ({
...accountNotifications,
notifications: accountNotifications.notifications.map((notification) =>
readNotificationIDs.has(notification.id)
? { ...notification, unread: false }
: notification,
),
}));
}

interface NotificationsState {
Expand Down Expand Up @@ -129,13 +141,16 @@ export const useNotifications = (): NotificationsState => {
),
);

const updatedNotifications = removeNotifications(
let updatedNotifications = removeNotifications(
state.settings,
readNotifications,
notifications,
);

markNotificationsAsReadLocally(readNotifications);
updatedNotifications = applyReadStateToNotifications(
updatedNotifications,
readNotifications,
);

setNotifications(updatedNotifications);
setTrayIconColor(updatedNotifications);
Expand Down Expand Up @@ -171,13 +186,16 @@ export const useNotifications = (): NotificationsState => {
),
);

const updatedNotifications = removeNotifications(
let updatedNotifications = removeNotifications(
state.settings,
doneNotifications,
notifications,
);

markNotificationsAsReadLocally(doneNotifications);
updatedNotifications = applyReadStateToNotifications(
updatedNotifications,
doneNotifications,
);

setNotifications(updatedNotifications);
setTrayIconColor(updatedNotifications);
Expand Down
9 changes: 8 additions & 1 deletion src/renderer/utils/notifications/group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export function isGroupByRepository(settings: SettingsState) {
/**
* Group notifications by repository.full_name preserving first-seen repo order.
* Returns a Map where keys are repo full_names and values are arrays of notifications.
* Skips notifications without valid repository data.
*/
export function groupNotificationsByRepository(
accounts: AccountNotifications[],
Expand All @@ -19,7 +20,13 @@ export function groupNotificationsByRepository(

for (const account of accounts) {
for (const notification of account.notifications) {
const repo = notification.repository?.full_name ?? '';
const repo = notification.repository?.full_name;

// Skip notifications without valid repository data
if (!repo) {
Copy link
Member Author

Choose a reason for hiding this comment

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

more defensive handling

continue;
}

const group = repoGroups.get(repo);

if (group) {
Expand Down