From ab24b4c1626a9221075a4eda438a58c141215bee Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 7 May 2024 22:15:15 -0400 Subject: [PATCH 1/5] feat: delay notification removal --- src/__mocks__/mock-state.ts | 1 + src/components/NotificationRow.tsx | 7 +- .../NotificationRow.test.tsx.snap | 2 + src/context/App.test.tsx | 12 +- src/context/App.tsx | 17 ++- src/hooks/useNotifications.test.ts | 117 +++++++++++++++--- src/hooks/useNotifications.ts | 67 ++++++++-- src/routes/Settings.tsx | 17 +++ .../__snapshots__/Settings.test.tsx.snap | 47 +++++++ src/types.ts | 1 + src/utils/constants.ts | 2 + src/utils/remove-notification.test.ts | 2 + src/utils/remove-notification.ts | 10 +- 13 files changed, 263 insertions(+), 39 deletions(-) diff --git a/src/__mocks__/mock-state.ts b/src/__mocks__/mock-state.ts index 01cb113a3..365e41391 100644 --- a/src/__mocks__/mock-state.ts +++ b/src/__mocks__/mock-state.ts @@ -18,4 +18,5 @@ export const mockSettings: SettingsState = { detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, + delayRemoval: false, }; diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index 75eaa3994..bae5af4bc 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -47,7 +47,7 @@ export const NotificationRow: FC = ({ notification, hostname }) => { markNotificationDone(notification.id, hostname); } else { // no need to mark as read, github does it by default when opening it - removeNotificationFromState(notification.id, hostname); + removeNotificationFromState(settings, notification.id, hostname); } }, [notifications, notification, accounts, settings]); // notifications required here to prevent weird state issues @@ -84,7 +84,10 @@ export const NotificationRow: FC = ({ notification, hostname }) => { ]); return ( -
+
{ expect(markNotificationReadMock).toHaveBeenCalledTimes(1); expect(markNotificationReadMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null, user: null }, + { + enterpriseAccounts: [], + token: null, + user: null, + }, + mockSettings, '123-456', 'github.com', ); @@ -160,6 +165,7 @@ describe('context/App.tsx', () => { expect(markNotificationDoneMock).toHaveBeenCalledTimes(1); expect(markNotificationDoneMock).toHaveBeenCalledWith( { enterpriseAccounts: [], token: null, user: null }, + mockSettings, '123-456', 'github.com', ); @@ -188,6 +194,7 @@ describe('context/App.tsx', () => { expect(unsubscribeNotificationMock).toHaveBeenCalledTimes(1); expect(unsubscribeNotificationMock).toHaveBeenCalledWith( { enterpriseAccounts: [], token: null, user: null }, + mockSettings, '123-456', 'github.com', ); @@ -221,6 +228,7 @@ describe('context/App.tsx', () => { expect(markRepoNotificationsMock).toHaveBeenCalledTimes(1); expect(markRepoNotificationsMock).toHaveBeenCalledWith( { enterpriseAccounts: [], token: null, user: null }, + mockSettings, 'gitify-app/notifications-test', 'github.com', ); @@ -325,6 +333,7 @@ describe('context/App.tsx', () => { detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, + delayRemoval: false, }, ); }); @@ -369,6 +378,7 @@ describe('context/App.tsx', () => { detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, + delayRemoval: false, }, ); }); diff --git a/src/context/App.tsx b/src/context/App.tsx index c240cc85f..571688e54 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -44,6 +44,7 @@ export const defaultSettings: SettingsState = { detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, + delayRemoval: false, }; interface AppContextState { @@ -57,7 +58,11 @@ interface AppContextState { notifications: AccountNotifications[]; status: Status; errorDetails: GitifyError; - removeNotificationFromState: (id: string, hostname: string) => void; + removeNotificationFromState: ( + settings: SettingsState, + id: string, + hostname: string, + ) => void; fetchNotifications: () => Promise; markNotificationRead: (id: string, hostname: string) => Promise; markNotificationDone: (id: string, hostname: string) => Promise; @@ -199,31 +204,31 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { const markNotificationReadWithAccounts = useCallback( async (id: string, hostname: string) => - await markNotificationRead(accounts, id, hostname), + await markNotificationRead(accounts, settings, id, hostname), [accounts, notifications], ); const markNotificationDoneWithAccounts = useCallback( async (id: string, hostname: string) => - await markNotificationDone(accounts, id, hostname), + await markNotificationDone(accounts, settings, id, hostname), [accounts, notifications], ); const unsubscribeNotificationWithAccounts = useCallback( async (id: string, hostname: string) => - await unsubscribeNotification(accounts, id, hostname), + await unsubscribeNotification(accounts, settings, id, hostname), [accounts, notifications], ); const markRepoNotificationsWithAccounts = useCallback( async (repoSlug: string, hostname: string) => - await markRepoNotifications(accounts, repoSlug, hostname), + await markRepoNotifications(accounts, settings, repoSlug, hostname), [accounts, notifications], ); const markRepoNotificationsDoneWithAccounts = useCallback( async (repoSlug: string, hostname: string) => - await markRepoNotificationsDone(accounts, repoSlug, hostname), + await markRepoNotificationsDone(accounts, settings, repoSlug, hostname), [accounts, notifications], ); diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index 146fa85de..053cf0cdf 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -427,6 +427,7 @@ describe('hooks/useNotifications.ts', () => { act(() => { result.current.removeNotificationFromState( + mockSettings, result.current.notifications[0].notifications[0].id, result.current.notifications[0].hostname, ); @@ -451,7 +452,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markNotificationRead(accounts, id, hostname); + result.current.markNotificationRead( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -469,7 +475,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markNotificationRead(accounts, id, hostname); + result.current.markNotificationRead( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -492,7 +503,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markNotificationRead(accounts, id, hostname); + result.current.markNotificationRead( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -510,7 +526,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markNotificationRead(accounts, id, hostname); + result.current.markNotificationRead( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -537,7 +558,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markNotificationDone(accounts, id, hostname); + result.current.markNotificationDone( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -555,7 +581,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markNotificationDone(accounts, id, hostname); + result.current.markNotificationDone( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -578,7 +609,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markNotificationDone(accounts, id, hostname); + result.current.markNotificationDone( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -596,7 +632,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markNotificationDone(accounts, id, hostname); + result.current.markNotificationDone( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -629,7 +670,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.unsubscribeNotification(accounts, id, hostname); + result.current.unsubscribeNotification( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -653,7 +699,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.unsubscribeNotification(accounts, id, hostname); + result.current.unsubscribeNotification( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -682,7 +733,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.unsubscribeNotification(accounts, id, hostname); + result.current.unsubscribeNotification( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -706,7 +762,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.unsubscribeNotification(accounts, id, hostname); + result.current.unsubscribeNotification( + accounts, + mockSettings, + id, + hostname, + ); }); await waitFor(() => { @@ -733,7 +794,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markRepoNotifications(accounts, repoSlug, hostname); + result.current.markRepoNotifications( + accounts, + mockSettings, + repoSlug, + hostname, + ); }); await waitFor(() => { @@ -751,7 +817,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markRepoNotifications(accounts, repoSlug, hostname); + result.current.markRepoNotifications( + accounts, + mockSettings, + repoSlug, + hostname, + ); }); await waitFor(() => { @@ -774,7 +845,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markRepoNotifications(accounts, repoSlug, hostname); + result.current.markRepoNotifications( + accounts, + mockSettings, + repoSlug, + hostname, + ); }); await waitFor(() => { @@ -792,7 +868,12 @@ describe('hooks/useNotifications.ts', () => { const { result } = renderHook(() => useNotifications()); act(() => { - result.current.markRepoNotifications(accounts, repoSlug, hostname); + result.current.markRepoNotifications( + accounts, + mockSettings, + repoSlug, + hostname, + ); }); await waitFor(() => { @@ -822,6 +903,7 @@ describe('hooks/useNotifications.ts', () => { act(() => { result.current.markRepoNotificationsDone( accounts, + mockSettings, repoSlug, hostname, ); @@ -844,6 +926,7 @@ describe('hooks/useNotifications.ts', () => { act(() => { result.current.markRepoNotificationsDone( accounts, + mockSettings, repoSlug, hostname, ); @@ -871,6 +954,7 @@ describe('hooks/useNotifications.ts', () => { act(() => { result.current.markRepoNotificationsDone( accounts, + mockSettings, repoSlug, hostname, ); @@ -893,6 +977,7 @@ describe('hooks/useNotifications.ts', () => { act(() => { result.current.markRepoNotificationsDone( accounts, + mockSettings, repoSlug, hostname, ); diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 9558e444c..84b0b9823 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -25,33 +25,42 @@ import { removeNotifications } from '../utils/remove-notifications'; interface NotificationsState { notifications: AccountNotifications[]; - removeNotificationFromState: (id: string, hostname: string) => void; + removeNotificationFromState: ( + settings: SettingsState, + id: string, + hostname: string, + ) => void; fetchNotifications: ( accounts: AuthState, settings: SettingsState, ) => Promise; markNotificationRead: ( accounts: AuthState, + settings: SettingsState, id: string, hostname: string, ) => Promise; markNotificationDone: ( accounts: AuthState, + settings: SettingsState, id: string, hostname: string, ) => Promise; unsubscribeNotification: ( accounts: AuthState, + settings: SettingsState, id: string, hostname: string, ) => Promise; markRepoNotifications: ( accounts: AuthState, + settings: SettingsState, repoSlug: string, hostname: string, ) => Promise; markRepoNotificationsDone: ( accounts: AuthState, + settings: SettingsState, repoSlug: string, hostname: string, ) => Promise; @@ -94,7 +103,12 @@ export const useNotifications = (): NotificationsState => { ); const markNotificationRead = useCallback( - async (accounts: AuthState, id: string, hostname: string) => { + async ( + accounts: AuthState, + settings: SettingsState, + id: string, + hostname: string, + ) => { setStatus('loading'); const token = getTokenForHost(hostname, accounts); @@ -103,6 +117,7 @@ export const useNotifications = (): NotificationsState => { await markNotificationThreadAsRead(id, hostname, token); const updatedNotifications = removeNotification( + settings, id, notifications, hostname, @@ -119,7 +134,12 @@ export const useNotifications = (): NotificationsState => { ); const markNotificationDone = useCallback( - async (accounts: AuthState, id: string, hostname: string) => { + async ( + accounts: AuthState, + settings: SettingsState, + id: string, + hostname: string, + ) => { setStatus('loading'); const token = getTokenForHost(hostname, accounts); @@ -128,6 +148,7 @@ export const useNotifications = (): NotificationsState => { await markNotificationThreadAsDone(id, hostname, token); const updatedNotifications = removeNotification( + settings, id, notifications, hostname, @@ -144,14 +165,19 @@ export const useNotifications = (): NotificationsState => { ); const unsubscribeNotification = useCallback( - async (accounts: AuthState, id: string, hostname: string) => { + async ( + accounts: AuthState, + settings: SettingsState, + id: string, + hostname: string, + ) => { setStatus('loading'); const token = getTokenForHost(hostname, accounts); try { await ignoreNotificationThreadSubscription(id, hostname, token); - await markNotificationRead(accounts, id, hostname); + await markNotificationRead(accounts, settings, id, hostname); setStatus('success'); } catch (err) { setStatus('success'); @@ -161,18 +187,26 @@ export const useNotifications = (): NotificationsState => { ); const markRepoNotifications = useCallback( - async (accounts: AuthState, repoSlug: string, hostname: string) => { + async ( + accounts: AuthState, + settings: SettingsState, + repoSlug: string, + hostname: string, + ) => { setStatus('loading'); const token = getTokenForHost(hostname, accounts); try { await markRepositoryNotificationsAsRead(repoSlug, hostname, token); - const updatedNotifications = removeNotifications( - repoSlug, - notifications, - hostname, - ); + let updatedNotifications = notifications; + if (settings.delayRemoval) { + updatedNotifications = removeNotifications( + repoSlug, + notifications, + hostname, + ); + } setNotifications(updatedNotifications); setTrayIconColor(updatedNotifications); @@ -185,7 +219,12 @@ export const useNotifications = (): NotificationsState => { ); const markRepoNotificationsDone = useCallback( - async (accounts: AuthState, repoSlug: string, hostname: string) => { + async ( + accounts: AuthState, + settings: SettingsState, + repoSlug: string, + hostname: string, + ) => { setStatus('loading'); try { @@ -204,6 +243,7 @@ export const useNotifications = (): NotificationsState => { notificationsToRemove.map((notification) => markNotificationDone( accounts, + settings, notification.id, notifications[accountIndex].hostname, ), @@ -228,8 +268,9 @@ export const useNotifications = (): NotificationsState => { ); const removeNotificationFromState = useCallback( - (id: string, hostname: string) => { + (settings: SettingsState, id: string, hostname: string) => { const updatedNotifications = removeNotification( + settings, id, notifications, hostname, diff --git a/src/routes/Settings.tsx b/src/routes/Settings.tsx index 1145bc9a0..b5b245f76 100644 --- a/src/routes/Settings.tsx +++ b/src/routes/Settings.tsx @@ -236,6 +236,23 @@ export const SettingsRoute: FC = () => { updateSetting('markAsDoneOnOpen', evt.target.checked) } /> + + updateSetting('delayRemoval', evt.target.checked) + } + tooltip={ +
+
+ Keep the notification within Gitify window upon interaction + (click, mark as read, mark as done, etc) until the next + refresh window (scheduled or user initiated) +
+
+ } + />
diff --git a/src/routes/__snapshots__/Settings.test.tsx.snap b/src/routes/__snapshots__/Settings.test.tsx.snap index fd658a36a..69aaddb4e 100644 --- a/src/routes/__snapshots__/Settings.test.tsx.snap +++ b/src/routes/__snapshots__/Settings.test.tsx.snap @@ -380,6 +380,53 @@ exports[`routes/Settings.tsx General should render itself & its children 1`] = `
+
+
+
+ +
+
+ +
+
+
= { diff --git a/src/utils/remove-notification.test.ts b/src/utils/remove-notification.test.ts index dfbf8a8a9..998440af4 100644 --- a/src/utils/remove-notification.test.ts +++ b/src/utils/remove-notification.test.ts @@ -1,3 +1,4 @@ +import { mockSettings } from '../__mocks__/mock-state'; import { mockedSingleAccountNotifications, mockedSingleNotification, @@ -12,6 +13,7 @@ describe('utils/remove-notification.ts', () => { expect(mockedSingleAccountNotifications[0].notifications.length).toBe(1); const result = removeNotification( + mockSettings, notificationId, mockedSingleAccountNotifications, hostname, diff --git a/src/utils/remove-notification.ts b/src/utils/remove-notification.ts index c7cd69899..dd149d317 100644 --- a/src/utils/remove-notification.ts +++ b/src/utils/remove-notification.ts @@ -1,10 +1,18 @@ -import type { AccountNotifications } from '../types'; +import type { AccountNotifications, SettingsState } from '../types'; +import Constants from './constants'; export const removeNotification = ( + settings: SettingsState, id: string, notifications: AccountNotifications[], hostname: string, ): AccountNotifications[] => { + if (settings.delayRemoval) { + const notificationRow = document.getElementById(id); + notificationRow.className += ` ${Constants.READ_CLASS_NAME}`; + return notifications; + } + const accountIndex = notifications.findIndex( (accountNotifications) => accountNotifications.hostname === hostname, ); From 216b62e6a93ae0afeb3dae14e6166c3abc62c0cc Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 7 May 2024 22:21:24 -0400 Subject: [PATCH 2/5] feat: delay notification removal --- src/__mocks__/mock-state.ts | 2 +- src/context/App.test.tsx | 4 +-- src/context/App.tsx | 2 +- src/hooks/useNotifications.ts | 2 +- src/routes/Settings.test.tsx | 28 +++++++++++++++++++ src/routes/Settings.tsx | 8 +++--- .../__snapshots__/Settings.test.tsx.snap | 10 +++---- src/types.ts | 2 +- src/utils/remove-notification.ts | 2 +- 9 files changed, 44 insertions(+), 16 deletions(-) diff --git a/src/__mocks__/mock-state.ts b/src/__mocks__/mock-state.ts index 365e41391..6594be1d0 100644 --- a/src/__mocks__/mock-state.ts +++ b/src/__mocks__/mock-state.ts @@ -18,5 +18,5 @@ export const mockSettings: SettingsState = { detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, - delayRemoval: false, + delayNotificationState: false, }; diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx index 66f47bc7f..f687aeed8 100644 --- a/src/context/App.test.tsx +++ b/src/context/App.test.tsx @@ -333,7 +333,7 @@ describe('context/App.tsx', () => { detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, - delayRemoval: false, + delayNotificationState: false, }, ); }); @@ -378,7 +378,7 @@ describe('context/App.tsx', () => { detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, - delayRemoval: false, + delayNotificationState: false, }, ); }); diff --git a/src/context/App.tsx b/src/context/App.tsx index 571688e54..45eacda12 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -44,7 +44,7 @@ export const defaultSettings: SettingsState = { detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, - delayRemoval: false, + delayNotificationState: false, }; interface AppContextState { diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 84b0b9823..321a48047 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -200,7 +200,7 @@ export const useNotifications = (): NotificationsState => { try { await markRepositoryNotificationsAsRead(repoSlug, hostname, token); let updatedNotifications = notifications; - if (settings.delayRemoval) { + if (settings.delayNotificationState) { updatedNotifications = removeNotifications( repoSlug, notifications, diff --git a/src/routes/Settings.test.tsx b/src/routes/Settings.test.tsx index 94b8e5e2f..22d88f833 100644 --- a/src/routes/Settings.test.tsx +++ b/src/routes/Settings.test.tsx @@ -336,6 +336,34 @@ describe('routes/Settings.tsx', () => { expect(updateSetting).toHaveBeenCalledTimes(1); expect(updateSetting).toHaveBeenCalledWith('markAsDoneOnOpen', false); }); + + it('should toggle the delayNotificationState checkbox', async () => { + await act(async () => { + render( + + + + + , + ); + }); + + fireEvent.click(screen.getByLabelText('Delay notification state'), { + target: { checked: true }, + }); + + expect(updateSetting).toHaveBeenCalledTimes(1); + expect(updateSetting).toHaveBeenCalledWith( + 'delayNotificationState', + false, + ); + }); }); describe('System section', () => { diff --git a/src/routes/Settings.tsx b/src/routes/Settings.tsx index b5b245f76..9709b136a 100644 --- a/src/routes/Settings.tsx +++ b/src/routes/Settings.tsx @@ -237,11 +237,11 @@ export const SettingsRoute: FC = () => { } /> - updateSetting('delayRemoval', evt.target.checked) + updateSetting('delayNotificationState', evt.target.checked) } tooltip={
diff --git a/src/routes/__snapshots__/Settings.test.tsx.snap b/src/routes/__snapshots__/Settings.test.tsx.snap index 69aaddb4e..f8a185eb6 100644 --- a/src/routes/__snapshots__/Settings.test.tsx.snap +++ b/src/routes/__snapshots__/Settings.test.tsx.snap @@ -391,7 +391,7 @@ exports[`routes/Settings.tsx General should render itself & its children 1`] = ` >
@@ -400,13 +400,13 @@ exports[`routes/Settings.tsx General should render itself & its children 1`] = ` >