Repository: gitify-app/gitify
Pull request #1014 (2024-04-16 13:00:27)

feat(refresh): reset interval and disable manual refresh
This PR further helps reduce the chance of rate-limiting conditions by
- resetting the automated refresh timer upon a manual refresh (default: 1 minute) to prevent manual+auto fetching being close together
- disabling the manual refresh button, if clicked, until the next internal (default: 1 minute) to prevent repeated clicks


I have also taken the opportunity to refactor the hardcoded fetch interval into our Constants file

adufr: However, having a configurable interval could probably also help with the rate-limiting issue?
This way user would have more control over this issue, and if they report problems we could just instruct them to increase their refresh interval
setchy: > Just sharing my opinion about this: first point looks good to me but I don't agree with the second, IMO the user should be free to refresh whenever he wants, and if he gets rate-limited because he clicked the refresh button too many times, too bad for him but that's his problem I often use this button and this change would badly affect my experience 🙁

You have a need to refresh multiple times within the minute? 🤔  
setchy: > However, having a configurable interval could probably also help with the rate-limiting issue?

Raised this as a new FR #1027 1027
setchy: Kind of weird... `pnpm test -- --coverage --verbose` works locally, but `pnpm test -- --coverage --runInBand --verbose` isn't... 🤔 

Patch:
diff --git a/renovate.json b/renovate.json
index 7f210bd2..09fd7b45 100644
--- a/renovate.json
+++ b/renovate.json
@@ -1,12 +1,14 @@
 {
   "$schema": "https://docs.renovatebot.com/renovate-schema.json",
-  "extends": ["config:recommended"],
+  "extends": [
+    "config:recommended",
+    ":separateMultipleMajorReleases",
+    ":enableVulnerabilityAlerts",
+    "schedule:weekly"
+  ],
   "labels": ["dependency"],
   "prConcurrentLimit": 5,
   "rangeStrategy": "pin",
-  "schedule": ["monthly"],
-  "separateMultipleMajor": true,
-  "osvVulnerabilityAlerts": true,
   "packageRules": [
     {
       "matchDepTypes": ["engines"],
diff --git a/src/components/Sidebar.tsx b/src/components/Sidebar.tsx
index 216e5b2a..896c33a7 100644
--- a/src/components/Sidebar.tsx
+++ b/src/components/Sidebar.tsx
@@ -6,7 +6,14 @@ import {
 } from '@primer/octicons-react';
 import { ipcRenderer } from 'electron';
 
-import { type FC, useCallback, useContext, useMemo } from 'react';
+import {
+  type FC,
+  useCallback,
+  useContext,
+  useEffect,
+  useMemo,
+  useRef,
+} from 'react';
 import { useLocation, useNavigate } from 'react-router-dom';
 
 import { Logo } from '../components/Logo';
@@ -22,6 +29,38 @@ 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}`);
   }, []);
@@ -80,6 +119,7 @@ export const Sidebar: FC = () => {
               onClick={() => {
                 navigate('/', { replace: true });
                 fetchNotifications();
+                resetFetchInterval();
               }}
               disabled={isFetching}
             >
diff --git a/src/context/App.tsx b/src/context/App.tsx
index 013a586b..48d811a2 100644
--- a/src/context/App.tsx
+++ b/src/context/App.tsx
@@ -7,7 +7,6 @@ import {
   useState,
 } from 'react';
 
-import { useInterval } from '../hooks/useInterval';
 import { useNotifications } from '../hooks/useNotifications';
 import {
   type AccountNotifications,
@@ -111,10 +110,6 @@ export const AppProvider = ({ children }: { children: ReactNode }) => {
     accounts.enterpriseAccounts.length,
   ]);
 
-  useInterval(() => {
-    fetchNotifications(accounts, settings);
-  }, 60000);
-
   // biome-ignore lint/correctness/useExhaustiveDependencies: We need to update tray title when settings or notifications changes.
   useEffect(() => {
     const count = getNotificationCount(notifications);
diff --git a/src/typesGithub.ts b/src/typesGithub.ts
index fb7e8eb1..0a462d6a 100644
--- a/src/typesGithub.ts
+++ b/src/typesGithub.ts
@@ -272,6 +272,76 @@ export interface PullRequest {
   changed_files: number;
 }
 
+export interface Commit {
+  sha: string;
+  node_id: string;
+  commit: {
+    author: CommitUser;
+    committer: CommitUser;
+    message: string;
+    tree: {
+      sha: string;
+      url: string;
+    };
+    url: string;
+    comment_count: number;
+    verification: {
+      verified: boolean;
+      reason: string;
+      signature: string | null;
+      payload: string | null;
+    };
+  };
+  url: string;
+  html_url: string;
+  comments_url: string;
+  author: User;
+  committer: User;
+  parents: CommitParent[];
+  stats: {
+    total: number;
+    additions: number;
+    deletions: number;
+  };
+  files: CommitFiles[];
+}
+
+interface CommitUser {
+  name: string;
+  email: string;
+  date: string;
+}
+
+interface CommitParent {
+  sha: string;
+  url: string;
+  html_url: string;
+}
+
+interface CommitFiles {
+  sha: string;
+  filename: string;
+  status: string;
+  additions: number;
+  deletions: number;
+  changes: number;
+  blob_url: string;
+  raw_url: string;
+  contents_url: string;
+  patch: string;
+}
+export interface CommitComments {
+  url: string;
+  html_url: string;
+  issue_url: string;
+  id: number;
+  node_id: string;
+  user: User;
+  created_at: string;
+  updated_at: string;
+  body: string;
+}
+
 export interface Issue {
   url: string;
   repository_url: string;
diff --git a/src/utils/constants.ts b/src/utils/constants.ts
index f99d2025..ded5e63c 100644
--- a/src/utils/constants.ts
+++ b/src/utils/constants.ts
@@ -16,6 +16,8 @@ export const Constants = {
   STORAGE_KEY: 'gitify-storage',
 
   ALLREAD_EMOJIS: ['😉', '🎉', '🐯', '🙈', '🎈', '🎊', '👏', '🎪', '🍝'],
+
+  FETCH_INTERVAL: 60000,
 };
 
 export const Errors: Record<ErrorType, GitifyError> = {
diff --git a/src/utils/subject.ts b/src/utils/subject.ts
index dba28b6e..1ddba3d2 100644
--- a/src/utils/subject.ts
+++ b/src/utils/subject.ts
@@ -1,6 +1,7 @@
 import type {
   CheckSuiteAttributes,
   CheckSuiteStatus,
+  Commit,
   DiscussionStateType,
   GitifySubject,
   Issue,
@@ -23,6 +24,8 @@ export async function getGitifySubjectDetails(
   switch (notification.subject.type) {
     case 'CheckSuite':
       return getGitifySubjectForCheckSuite(notification);
+    case 'Commit':
+      return getGitifySubjectForCommit(notification, token);
     case 'Discussion':
       return await getGitifySubjectForDiscussion(notification, token);
     case 'Issue':
@@ -91,6 +94,31 @@ function getGitifySubjectForCheckSuite(
   };
 }
 
+async function getGitifySubjectForCommit(
+  notification: Notification,
+  token: string,
+): Promise<GitifySubject> {
+  try {
+    const commit: Commit = (
+      await apiRequestAuth(notification.subject.url, 'GET', token)
+    ).data;
+
+    const commitCommentUser = await getLatestCommentUser(notification, token);
+
+    return {
+      state: null,
+      user: {
+        login: commitCommentUser?.login ?? commit.author.login,
+        html_url: commitCommentUser?.html_url ?? commit.author.html_url,
+        avatar_url: commitCommentUser?.avatar_url ?? commit.author.avatar_url,
+        type: commitCommentUser?.type ?? commit.author.type,
+      },
+    };
+  } catch (err) {
+    console.error('Issue subject retrieval failed');
+  }
+}
+
 async function getGitifySubjectForDiscussion(
   notification: Notification,
   token: string,


Test patch:
diff --git a/src/components/Sidebar.test.tsx b/src/components/Sidebar.test.tsx
index 79ad0930..270cdbb3 100644
--- a/src/components/Sidebar.test.tsx
+++ b/src/components/Sidebar.test.tsx
@@ -1,10 +1,11 @@
-import { fireEvent, render, screen } from '@testing-library/react';
+import { act, 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();
@@ -19,12 +20,15 @@ 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();
   });
 
@@ -57,6 +61,37 @@ 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
@@ -68,8 +103,19 @@ describe('components/Sidebar.tsx', () => {
       </AppContext.Provider>,
     );
     fetchNotifications.mockReset();
-    fireEvent.click(screen.getByTitle('Refresh Notifications'));
+
+    const enabledRefreshButton = 'Refresh Notifications';
+
+    fireEvent.click(screen.getByTitle(enabledRefreshButton));
+
     expect(fetchNotifications).toHaveBeenCalledTimes(1);
+
+    act(() => {
+      jest.advanceTimersByTime(Constants.FETCH_INTERVAL);
+      return;
+    });
+
+    expect(fetchNotifications).toHaveBeenCalledTimes(2);
   });
 
   it('go to the settings route', () => {
diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx
index 0850df98..dde50505 100644
--- a/src/context/App.test.tsx
+++ b/src/context/App.test.tsx
@@ -58,36 +58,6 @@ 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(60000);
-        return;
-      });
-      expect(fetchNotificationsMock).toHaveBeenCalledTimes(1);
-
-      act(() => {
-        jest.advanceTimersByTime(60000);
-        return;
-      });
-      expect(fetchNotificationsMock).toHaveBeenCalledTimes(2);
-
-      act(() => {
-        jest.advanceTimersByTime(60000);
-        return;
-      });
-      expect(fetchNotificationsMock).toHaveBeenCalledTimes(3);
-    });
-
     it('should call fetchNotifications', async () => {
       const TestComponent = () => {
         const { fetchNotifications } = useContext(AppContext);
diff --git a/src/utils/subject.test.ts b/src/utils/subject.test.ts
index be887fb1..6ce48604 100644
--- a/src/utils/subject.test.ts
+++ b/src/utils/subject.test.ts
@@ -138,6 +138,65 @@ describe('utils/subject.ts', () => {
   });
 
   describe('getGitifySubjectDetails', () => {
+    describe('Commits', () => {
+      it('get commit commenter', async () => {
+        const mockNotification = {
+          ...mockedSingleNotification,
+          subject: {
+            title: 'This is a commit with comments',
+            url: 'https://api.github.com/repos/manosim/notifications-test/commits/d2a86d80e3d24ea9510d5de6c147e53c30f313a8',
+            latest_comment_url:
+              'https://api.github.com/repos/manosim/notifications-test/comments/141012658',
+            type: 'Commit' as SubjectType,
+          },
+        };
+
+        nock('https://api.github.com')
+          .get(
+            '/repos/manosim/notifications-test/commits/d2a86d80e3d24ea9510d5de6c147e53c30f313a8',
+          )
+          .reply(200, { author: { login: 'some-author' } });
+
+        nock('https://api.github.com')
+          .get('/repos/manosim/notifications-test/comments/141012658')
+          .reply(200, { user: { login: 'some-commenter' } });
+
+        const result = await getGitifySubjectDetails(
+          mockNotification,
+          mockAccounts.token,
+        );
+
+        expect(result.state).toBeNull();
+        expect(result.user).toEqual({ login: 'some-commenter' });
+      });
+
+      it('get commit without commenter', async () => {
+        const mockNotification = {
+          ...mockedSingleNotification,
+          subject: {
+            title: 'This is a commit with comments',
+            url: 'https://api.github.com/repos/manosim/notifications-test/commits/d2a86d80e3d24ea9510d5de6c147e53c30f313a8',
+            latest_comment_url: null,
+            type: 'Commit' as SubjectType,
+          },
+        };
+
+        nock('https://api.github.com')
+          .get(
+            '/repos/manosim/notifications-test/commits/d2a86d80e3d24ea9510d5de6c147e53c30f313a8',
+          )
+          .reply(200, { author: { login: 'some-author' } });
+
+        const result = await getGitifySubjectDetails(
+          mockNotification,
+          mockAccounts.token,
+        );
+
+        expect(result.state).toBeNull();
+        expect(result.user).toEqual({ login: 'some-author' });
+      });
+    });
+
     describe('Discussions', () => {
       it('answered discussion state', async () => {
         const mockNotification = {