fix: duplicate system notifications (#1038)

Author: setchy

src/components/Sidebar.test.tsx

@@ -1,11 +1,10 @@
-import { act, fireEvent, render, screen } from '@testing-library/react';
+import { fireEvent, render, screen } from '@testing-library/react';
 import { MemoryRouter } from 'react-router-dom';
 import * as TestRenderer from 'react-test-renderer';
 const { shell, ipcRenderer } = require('electron');
 import { mockSettings } from '../__mocks__/mock-state';
 import { mockedAccountNotifications } from '../__mocks__/mockedData';
 import { AppContext } from '../context/App';
-import Constants from '../utils/constants';
 import { Sidebar } from './Sidebar';
 
 const mockNavigate = jest.fn();
@@ -20,15 +19,12 @@ describe('components/Sidebar.tsx', () => {
   beforeEach(() => {
     fetchNotifications.mockReset();
 
-    jest.useFakeTimers();
-
     jest.spyOn(ipcRenderer, 'send');
     jest.spyOn(shell, 'openExternal');
     jest.spyOn(window, 'clearInterval');
   });
 
   afterEach(() => {
-    jest.clearAllTimers();
     jest.clearAllMocks();
   });
 
@@ -61,37 +57,6 @@ describe('components/Sidebar.tsx', () => {
     expect(tree).toMatchSnapshot();
   });
 
-  it('should fetch notifications every minute', async () => {
-    render(
-      <AppContext.Provider
-        value={{ isLoggedIn: true, notifications: [], fetchNotifications }}
-      >
-        <MemoryRouter>
-          <Sidebar />
-        </MemoryRouter>
-      </AppContext.Provider>,
-    );
-    fetchNotifications.mockReset();
-
-    act(() => {
-      jest.advanceTimersByTime(Constants.FETCH_INTERVAL);
-      return;
-    });
-    expect(fetchNotifications).toHaveBeenCalledTimes(1);
-
-    act(() => {
-      jest.advanceTimersByTime(Constants.FETCH_INTERVAL);
-      return;
-    });
-    expect(fetchNotifications).toHaveBeenCalledTimes(2);
-
-    act(() => {
-      jest.advanceTimersByTime(Constants.FETCH_INTERVAL);
-      return;
-    });
-    expect(fetchNotifications).toHaveBeenCalledTimes(3);
-  });
-
   it('should refresh the notifications', () => {
     render(
       <AppContext.Provider
@@ -103,19 +68,9 @@ describe('components/Sidebar.tsx', () => {
       </AppContext.Provider>,
     );
     fetchNotifications.mockReset();
-
-    const enabledRefreshButton = 'Refresh Notifications';
-
-    fireEvent.click(screen.getByTitle(enabledRefreshButton));
+    fireEvent.click(screen.getByTitle('Refresh Notifications'));
 
     expect(fetchNotifications).toHaveBeenCalledTimes(1);
-
-    act(() => {
-      jest.advanceTimersByTime(Constants.FETCH_INTERVAL);
-      return;
-    });
-
-    expect(fetchNotifications).toHaveBeenCalledTimes(2);
   });
 
   it('go to the settings route', () => {

src/components/Sidebar.tsx

@@ -6,14 +6,7 @@ import {
 } from '@primer/octicons-react';
 import { ipcRenderer } from 'electron';
 
-import {
-  type FC,
-  useCallback,
-  useContext,
-  useEffect,
-  useMemo,
-  useRef,
-} from 'react';
+import { type FC, useCallback, useContext, useMemo } from 'react';
 import { useLocation, useNavigate } from 'react-router-dom';
 
 import { Logo } from '../components/Logo';
@@ -29,38 +22,6 @@ export const Sidebar: FC = () => {
   const { notifications, fetchNotifications, isLoggedIn, isFetching } =
     useContext(AppContext);
 
-  const useFetchInterval = (callback, delay: number) => {
-    const savedCallback = useRef(callback);
-    const intervalRef = useRef(null);
-
-    useEffect(() => {
-      savedCallback.current = callback;
-    }, [callback]);
-
-    useEffect(() => {
-      if (delay !== null) {
-        const id = setInterval(savedCallback.current, delay);
-        intervalRef.current = id;
-        return () => clearInterval(id);
-      }
-    }, [delay]);
-
-    const resetFetchInterval = useCallback(() => {
-      if (intervalRef.current !== null) {
-        clearInterval(intervalRef.current);
-        intervalRef.current = setInterval(savedCallback.current, delay);
-      }
-    }, [delay]);
-
-    return { resetFetchInterval };
-  };
-
-  const { resetFetchInterval } = useFetchInterval(() => {
-    if (isLoggedIn) {
-      fetchNotifications();
-    }
-  }, Constants.FETCH_INTERVAL);
-
   const onOpenBrowser = useCallback(() => {
     openExternalLink(`https://github.com/${Constants.REPO_SLUG}`);
   }, []);
@@ -119,7 +80,6 @@ export const Sidebar: FC = () => {
               onClick={() => {
                 navigate('/', { replace: true });
                 fetchNotifications();
-                resetFetchInterval();
               }}
               disabled={isFetching}
             >

src/context/App.test.tsx

@@ -6,6 +6,7 @@ import { useNotifications } from '../hooks/useNotifications';
 import type { AuthState, SettingsState } from '../types';
 import * as apiRequests from '../utils/api-requests';
 import * as comms from '../utils/comms';
+import Constants from '../utils/constants';
 import * as notifications from '../utils/notifications';
 import * as storage from '../utils/storage';
 import { AppContext, AppProvider } from './App';
@@ -58,6 +59,36 @@ describe('context/App.tsx', () => {
       });
     });
 
+    it('fetch notifications every minute', async () => {
+      customRender(null);
+
+      // Wait for the useEffects, for settings.participating and accounts, to run.
+      // Those aren't what we're testing
+      await waitFor(() =>
+        expect(fetchNotificationsMock).toHaveBeenCalledTimes(1),
+      );
+
+      fetchNotificationsMock.mockReset();
+
+      act(() => {
+        jest.advanceTimersByTime(Constants.FETCH_INTERVAL);
+        return;
+      });
+      expect(fetchNotificationsMock).toHaveBeenCalledTimes(1);
+
+      act(() => {
+        jest.advanceTimersByTime(Constants.FETCH_INTERVAL);
+        return;
+      });
+      expect(fetchNotificationsMock).toHaveBeenCalledTimes(2);
+
+      act(() => {
+        jest.advanceTimersByTime(Constants.FETCH_INTERVAL);
+        return;
+      });
+      expect(fetchNotificationsMock).toHaveBeenCalledTimes(3);
+    });
+
     it('should call fetchNotifications', async () => {
       const TestComponent = () => {
         const { fetchNotifications } = useContext(AppContext);

src/context/App.tsx

@@ -7,6 +7,7 @@ import {
   useState,
 } from 'react';
 
+import { useInterval } from '../hooks/useInterval';
 import { useNotifications } from '../hooks/useNotifications';
 import {
   type AccountNotifications,
@@ -110,6 +111,10 @@ export const AppProvider = ({ children }: { children: ReactNode }) => {
     accounts.enterpriseAccounts.length,
   ]);
 
+  useInterval(() => {
+    fetchNotifications(accounts, settings);
+  }, Constants.FETCH_INTERVAL);
+
   // biome-ignore lint/correctness/useExhaustiveDependencies: We need to update tray title when settings or notifications changes.
   useEffect(() => {
     const count = getNotificationCount(notifications);