Repository: gitify-app/gitify
Pull request #1305 (2024-06-21 15:03:13)

feat: fetch notifications on navigation
Currently `fetchNotifcations` is called each time when a subset of settings are changed.  

This can cause rate limiting if enough settings are changed in quick succession.

Instead, this PR moves the retrieve new notifications to occur when moving back to the Notifications screen, either by;

- clicking the `Go Back` button in the header
- clicking on the Sidebar `Settings` icon


Patch:
diff --git a/src/components/Header.tsx b/src/components/Header.tsx
index 23f7e365..98698d86 100644
--- a/src/components/Header.tsx
+++ b/src/components/Header.tsx
@@ -1,21 +1,34 @@
 import { ArrowLeftIcon } from '@primer/octicons-react';
-import type { FC, ReactNode } from 'react';
+import { type FC, type ReactNode, useContext } from 'react';
 import { useNavigate } from 'react-router-dom';
+import { AppContext } from '../context/App';
 import { Size } from '../types';
 
 interface IHeader {
   children: ReactNode;
+  fetchOnBack?: boolean;
 }
 
-export const Header: FC<IHeader> = ({ children }: IHeader) => {
+export const Header: FC<IHeader> = ({
+  children,
+  fetchOnBack = false,
+}: IHeader) => {
   const navigate = useNavigate();
+
+  const { fetchNotifications } = useContext(AppContext);
+
   return (
     <div className="mx-8 mt-2 flex items-center justify-between py-2">
       <button
         type="button"
         className="focus:outline-none"
         title="Go Back"
-        onClick={() => navigate(-1)}
+        onClick={() => {
+          navigate(-1);
+          if (fetchOnBack) {
+            fetchNotifications();
+          }
+        }}
       >
         <ArrowLeftIcon
           size={Size.XLARGE}
diff --git a/src/components/Sidebar.tsx b/src/components/Sidebar.tsx
index b85c3507..355c3711 100644
--- a/src/components/Sidebar.tsx
+++ b/src/components/Sidebar.tsx
@@ -31,6 +31,7 @@ export const Sidebar: FC = () => {
   const toggleSettings = () => {
     if (location.pathname.startsWith('/settings')) {
       navigate('/', { replace: true });
+      fetchNotifications();
     } else {
       navigate('/settings');
     }
diff --git a/src/context/App.tsx b/src/context/App.tsx
index 9ed3d246..16904b58 100644
--- a/src/context/App.tsx
+++ b/src/context/App.tsx
@@ -122,16 +122,10 @@ export const AppProvider = ({ children }: { children: ReactNode }) => {
     setTheme(settings.theme);
   }, [settings.theme]);
 
-  // biome-ignore lint/correctness/useExhaustiveDependencies: We only want fetchNotifications to be called for certain account or setting changes.
+  // biome-ignore lint/correctness/useExhaustiveDependencies: We only want fetchNotifications to be called for account changes
   useEffect(() => {
     fetchNotifications({ auth, settings });
-  }, [
-    settings.participating,
-    settings.showBots,
-    settings.detailedNotifications,
-    settings.delayNotificationState,
-    auth.accounts.length,
-  ]);
+  }, [auth.accounts]);
 
   useInterval(() => {
     fetchNotifications({ auth, settings });
diff --git a/src/routes/Settings.tsx b/src/routes/Settings.tsx
index 5e607760..f1635937 100644
--- a/src/routes/Settings.tsx
+++ b/src/routes/Settings.tsx
@@ -56,7 +56,7 @@ export const SettingsRoute: FC = () => {
 
   return (
     <div className="flex h-screen flex-col" data-testid="settings">
-      <Header>Settings</Header>
+      <Header fetchOnBack={true}>Settings</Header>
       <div className="flex-grow overflow-x-auto px-8">
         <fieldset className="mb-3">
           <legend id="appearance" className="mb-1 mt-2 font-semibold">


Test patch:
diff --git a/src/components/Header.test.tsx b/src/components/Header.test.tsx
index aec69a95..81ad82db 100644
--- a/src/components/Header.test.tsx
+++ b/src/components/Header.test.tsx
@@ -1,4 +1,5 @@
 import { fireEvent, render, screen } from '@testing-library/react';
+import { AppContext } from '../context/App';
 import { Header } from './Header';
 
 const mockNavigate = jest.fn();
@@ -8,6 +9,12 @@ jest.mock('react-router-dom', () => ({
 }));
 
 describe('components/Header.tsx', () => {
+  const fetchNotifications = jest.fn();
+
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
+
   it('should render itself & its children', () => {
     const tree = render(<Header>Test Header</Header>);
 
@@ -18,9 +25,26 @@ describe('components/Header.tsx', () => {
     render(<Header>Test Header</Header>);
 
     fireEvent.click(screen.getByLabelText('Go Back'));
-    expect(mockNavigate).toHaveBeenNthCalledWith(1, -1);
 
     expect(mockNavigate).toHaveBeenCalledTimes(1);
     expect(mockNavigate).toHaveBeenCalledWith(-1);
   });
+
+  it('should navigate back and fetch notifications', () => {
+    render(
+      <AppContext.Provider
+        value={{
+          fetchNotifications,
+        }}
+      >
+        <Header fetchOnBack={true}>Test Header</Header>
+      </AppContext.Provider>,
+    );
+
+    fireEvent.click(screen.getByLabelText('Go Back'));
+
+    expect(mockNavigate).toHaveBeenCalledTimes(1);
+    expect(mockNavigate).toHaveBeenCalledWith(-1);
+    expect(fetchNotifications).toHaveBeenCalledTimes(1);
+  });
 });
diff --git a/src/components/Sidebar.test.tsx b/src/components/Sidebar.test.tsx
index 0d2588e7..15e011ce 100644
--- a/src/components/Sidebar.test.tsx
+++ b/src/components/Sidebar.test.tsx
@@ -15,6 +15,7 @@ jest.mock('react-router-dom', () => ({
 
 describe('components/Sidebar.tsx', () => {
   const fetchNotifications = jest.fn();
+
   const openExternalLinkMock = jest.spyOn(comms, 'openExternalLink');
 
   afterEach(() => {
@@ -230,7 +231,9 @@ describe('components/Sidebar.tsx', () => {
 
     it('go to the home if settings path already shown', () => {
       render(
-        <AppContext.Provider value={{ isLoggedIn: true, notifications: [] }}>
+        <AppContext.Provider
+          value={{ isLoggedIn: true, notifications: [], fetchNotifications }}
+        >
           <MemoryRouter initialEntries={['/settings']}>
             <Sidebar />
           </MemoryRouter>
@@ -239,6 +242,7 @@ describe('components/Sidebar.tsx', () => {
 
       fireEvent.click(screen.getByTitle('Settings'));
 
+      expect(fetchNotifications).toHaveBeenCalledTimes(1);
       expect(mockNavigate).toHaveBeenCalledWith('/', { replace: true });
     });
   });
diff --git a/src/routes/Settings.test.tsx b/src/routes/Settings.test.tsx
index 67b876a8..333cc184 100644
--- a/src/routes/Settings.test.tsx
+++ b/src/routes/Settings.test.tsx
@@ -15,6 +15,7 @@ jest.mock('react-router-dom', () => ({
 describe('routes/Settings.tsx', () => {
   let originalPlatform: NodeJS.Platform;
   const updateSetting = jest.fn();
+  const fetchNotifications = jest.fn();
 
   beforeAll(() => {
     // Save the original platform value
@@ -62,6 +63,7 @@ describe('routes/Settings.tsx', () => {
             value={{
               auth: mockAuth,
               settings: mockSettings,
+              fetchNotifications,
             }}
           >
             <MemoryRouter>
@@ -72,6 +74,7 @@ describe('routes/Settings.tsx', () => {
       });
 
       fireEvent.click(screen.getByLabelText('Go Back'));
+      expect(fetchNotifications).toHaveBeenCalledTimes(1);
       expect(mockNavigate).toHaveBeenNthCalledWith(1, -1);
     });
   });