Repository: gitify-app/gitify (ts)

Issue:
On first load, notification icons are missing colors
### 🔍 Is there already an issue for your problem?

- [X] I have checked older issues, open and closed

### 📝 Description

When opening the app (or reloading it by pressing Cmd/Control+R), notifications are loaded without their color.

The issue is caused by the following block:
https://github.com/gitify-app/gitify/blob/1c3c248fd89299d519457ed1d7c3444ab6bf92c2/src/context/App.tsx#L69-L78

When first running the app, it looks like `useNotifications` is called with `colors: false` because the settings might not have already be initialized, thus the following code block is not being called, resulting in the missing colors:
https://github.com/gitify-app/gitify/blob/1c3c248fd89299d519457ed1d7c3444ab6bf92c2/src/hooks/useNotifications.ts#L142-L163

### 🪜 Steps To Reproduce

1. Open Gitify -> notification icons are missing their color
2. Click the reload notifications button -> notifications do now have their color

### ℹ System Info

```Markdown
`main` branch
```


### 📸 Screenshots

On first load:
<img width="499" alt="image" src="https://github.com/gitify-app/gitify/assets/26418696/edc52cc5-3962-4072-b396-75b8e731ac71">

After clicking reload:
<img width="500" alt="image" src="https://github.com/gitify-app/gitify/assets/26418696/11c2eb83-c57c-470b-8b34-7a3443d29574">



Pull request:
fix: missing icon colors on first load
## Related issues

- Fixes #786 

## Context

Show notification icons colors on app first load / app refresh.

## Discussion

I've identified 2 possible fixes:

- First one would be to simply drop this code so that we fetch the `state` of each notifications, even though `settings.colors` is falsy.
https://github.com/gitify-app/gitify/blob/1c3c248fd89299d519457ed1d7c3444ab6bf92c2/src/hooks/useNotifications.ts#L107-L117
Since `notification.state` is, for now, only required to load the icons colors, this would result on making a lot of unnecessary api calls if the setting is disabled.

- Second one is the one I've implemented: init `settings.colors` to null and explicitely check for `settings.colors === false`, thus fixing the bug AND avoiding making unnecessary api calls

Patch:
diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx
index 7f1dbc4a..360df284 100644
--- a/src/components/NotificationRow.tsx
+++ b/src/components/NotificationRow.tsx
@@ -23,6 +23,7 @@ export const NotificationRow: React.FC<IProps> = ({
   const {
     settings,
     accounts,
+    removeNotificationFromState,
     markNotification,
     markNotificationDone,
     unsubscribeNotification,
@@ -33,6 +34,9 @@ export const NotificationRow: React.FC<IProps> = ({
 
     if (settings.markAsDoneOnOpen) {
       markNotificationDone(notification.id, hostname);
+    } else {
+      // no need to mark as read, github does it by default when opening it
+      removeNotificationFromState(notification.id, hostname);
     }
   }, [settings]);
 
diff --git a/src/context/App.tsx b/src/context/App.tsx
index d009a160..72978d93 100644
--- a/src/context/App.tsx
+++ b/src/context/App.tsx
@@ -36,7 +36,7 @@ export const defaultSettings: SettingsState = {
   showNotifications: true,
   openAtStartup: false,
   appearance: Appearance.SYSTEM,
-  colors: false,
+  colors: null,
   markAsDoneOnOpen: false,
 };
 
@@ -51,6 +51,7 @@ interface AppContextState {
   notifications: AccountNotifications[];
   isFetching: boolean;
   requestFailed: boolean;
+  removeNotificationFromState: (id: string, hostname: string) => void;
   fetchNotifications: () => Promise<void>;
   markNotification: (id: string, hostname: string) => Promise<void>;
   markNotificationDone: (id: string, hostname: string) => Promise<void>;
@@ -71,6 +72,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => {
     notifications,
     requestFailed,
     isFetching,
+    removeNotificationFromState,
     markNotification,
     markNotificationDone,
     unsubscribeNotification,
@@ -210,6 +212,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => {
         notifications,
         isFetching,
         requestFailed,
+        removeNotificationFromState,
         fetchNotifications: fetchNotificationsWithAccounts,
         markNotification: markNotificationWithAccounts,
         markNotificationDone: markNotificationDoneWithAccounts,
diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts
index 41875fb8..5634d259 100644
--- a/src/hooks/useNotifications.ts
+++ b/src/hooks/useNotifications.ts
@@ -19,6 +19,7 @@ import { removeNotifications } from '../utils/remove-notifications';
 
 interface NotificationsState {
   notifications: AccountNotifications[];
+  removeNotificationFromState: (id: string, hostname: string) => void;
   fetchNotifications: (
     accounts: AuthState,
     settings: SettingsState,
@@ -104,7 +105,7 @@ export const useNotifications = (colors: boolean): NotificationsState => {
                 ]
               : [...enterpriseNotifications];
 
-            if (!colors) {
+            if (colors === false) {
               setNotifications(data);
               triggerNativeNotifications(
                 notifications,
@@ -312,11 +313,26 @@ export const useNotifications = (colors: boolean): NotificationsState => {
     [notifications],
   );
 
+  const removeNotificationFromState = useCallback(
+    (id, hostname) => {
+      const updatedNotifications = removeNotification(
+        id,
+        notifications,
+        hostname,
+      );
+
+      setNotifications(updatedNotifications);
+      setTrayIconColor(updatedNotifications);
+    },
+    [notifications],
+  );
+
   return {
     isFetching,
     requestFailed,
     notifications,
 
+    removeNotificationFromState,
     fetchNotifications,
     markNotification,
     markNotificationDone,
diff --git a/src/types.ts b/src/types.ts
index 5925a24f..dc28c33a 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -12,7 +12,7 @@ export interface SettingsState {
   showNotifications: boolean;
   openAtStartup: boolean;
   appearance: Appearance;
-  colors: boolean;
+  colors: boolean | null;
   markAsDoneOnOpen: boolean;
 }
 


Test patch:
diff --git a/src/components/NotificationRow.test.tsx b/src/components/NotificationRow.test.tsx
index dcd601b1..06426188 100644
--- a/src/components/NotificationRow.test.tsx
+++ b/src/components/NotificationRow.test.tsx
@@ -31,7 +31,7 @@ describe('components/Notification.js', () => {
   });
 
   it('should open a notification in the browser', () => {
-    const markNotification = jest.fn();
+    const removeNotificationFromState = jest.fn();
 
     const props = {
       notification: mockedSingleNotification,
@@ -42,7 +42,7 @@ describe('components/Notification.js', () => {
       <AppContext.Provider
         value={{
           settings: { ...mockSettings, markAsDoneOnOpen: false },
-          markNotification,
+          removeNotificationFromState,
           accounts: mockAccounts,
         }}
       >
@@ -52,6 +52,7 @@ describe('components/Notification.js', () => {
 
     fireEvent.click(getByRole('main'));
     expect(shell.openExternal).toHaveBeenCalledTimes(1);
+    expect(removeNotificationFromState).toHaveBeenCalledTimes(1);
   });
 
   it('should open a notification in browser & mark it as done', () => {
diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx
index 1c788a9e..6f704f9f 100644
--- a/src/context/App.test.tsx
+++ b/src/context/App.test.tsx
@@ -290,7 +290,7 @@ describe('context/App.tsx', () => {
         participating: true,
         playSound: true,
         showNotifications: true,
-        colors: false,
+        colors: null,
         markAsDoneOnOpen: false,
       },
     );
@@ -328,7 +328,7 @@ describe('context/App.tsx', () => {
         participating: false,
         playSound: true,
         showNotifications: true,
-        colors: false,
+        colors: null,
         markAsDoneOnOpen: false,
       },
     );
diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts
index c6f2b753..ca789184 100644
--- a/src/hooks/useNotifications.test.ts
+++ b/src/hooks/useNotifications.test.ts
@@ -269,6 +269,42 @@ describe('hooks/useNotifications.ts', () => {
     });
   });
 
+  describe('removeNotificationFromState', () => {
+    it('should remove a notification from state', async () => {
+      const notifications = [
+        { id: 1, title: 'This is a notification.' },
+        { id: 2, title: 'This is another one.' },
+      ];
+
+      nock('https://api.github.com')
+        .get('/notifications?participating=false')
+        .reply(200, notifications);
+
+      nock('https://github.gitify.io/api/v3')
+        .get('/notifications?participating=false')
+        .reply(200, notifications);
+
+      const { result } = renderHook(() => useNotifications(false));
+
+      act(() => {
+        result.current.fetchNotifications(mockAccounts, mockSettings);
+      });
+
+      await waitFor(() => {
+        expect(result.current.isFetching).toBe(false);
+      });
+
+      act(() => {
+        result.current.removeNotificationFromState(
+          result.current.notifications[0].notifications[0].id,
+          result.current.notifications[0].hostname,
+        );
+      });
+
+      expect(result.current.notifications[0].notifications.length).toBe(1);
+    });
+  });
+
   describe('markNotification', () => {
     const id = 'notification-123';