From beccef8abfdae55fde58a3b589cf801a77813e64 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 5 Apr 2024 23:19:10 -0400 Subject: [PATCH 01/14] feat: detailed error state handling --- src/components/error/BadCredentials.test.tsx | 12 +++++++ src/components/error/BadCredentials.tsx | 14 ++++++++ src/components/error/MissingScopes.test.tsx | 12 +++++++ src/components/error/MissingScopes.tsx | 16 +++++++++ src/components/{ => error}/Oops.test.tsx | 4 +-- src/components/{ => error}/Oops.tsx | 2 +- src/components/error/RateLimit.test.tsx | 12 +++++++ src/components/error/RateLimit.tsx | 14 ++++++++ src/context/App.tsx | 4 +++ src/hooks/useNotifications.ts | 36 +++++++++++++++++--- src/routes/Notifications.tsx | 18 ++++++++-- src/types.ts | 7 ++++ src/typesGithub.ts | 5 +++ 13 files changed, 145 insertions(+), 11 deletions(-) create mode 100644 src/components/error/BadCredentials.test.tsx create mode 100644 src/components/error/BadCredentials.tsx create mode 100644 src/components/error/MissingScopes.test.tsx create mode 100644 src/components/error/MissingScopes.tsx rename src/components/{ => error}/Oops.test.tsx (81%) rename src/components/{ => error}/Oops.tsx (92%) create mode 100644 src/components/error/RateLimit.test.tsx create mode 100644 src/components/error/RateLimit.tsx diff --git a/src/components/error/BadCredentials.test.tsx b/src/components/error/BadCredentials.test.tsx new file mode 100644 index 000000000..f0867890c --- /dev/null +++ b/src/components/error/BadCredentials.test.tsx @@ -0,0 +1,12 @@ +import * as React from 'react'; +import * as TestRenderer from 'react-test-renderer'; + +import { BadCredentials } from './BadCredentials'; + +describe('components/error/BadCredentials.tsx', function () { + it('should render itself & its children', function () { + const tree = TestRenderer.create(); + + expect(tree).toMatchSnapshot(); + }); +}); diff --git a/src/components/error/BadCredentials.tsx b/src/components/error/BadCredentials.tsx new file mode 100644 index 000000000..9cc3d313e --- /dev/null +++ b/src/components/error/BadCredentials.tsx @@ -0,0 +1,14 @@ +import * as React from 'react'; + +export const BadCredentials = () => { + return ( +
+

🔓

+ +

+ Bad Credentials. +

+
The credentials you are may be invalid or expired.
+
+ ); +}; diff --git a/src/components/error/MissingScopes.test.tsx b/src/components/error/MissingScopes.test.tsx new file mode 100644 index 000000000..6627a72d0 --- /dev/null +++ b/src/components/error/MissingScopes.test.tsx @@ -0,0 +1,12 @@ +import * as React from 'react'; +import * as TestRenderer from 'react-test-renderer'; + +import { MissingScopes } from './MissingScopes'; + +describe('components/error/MissingScopes.tsx', function () { + it('should render itself & its children', function () { + const tree = TestRenderer.create(); + + expect(tree).toMatchSnapshot(); + }); +}); diff --git a/src/components/error/MissingScopes.tsx b/src/components/error/MissingScopes.tsx new file mode 100644 index 000000000..91b32f62d --- /dev/null +++ b/src/components/error/MissingScopes.tsx @@ -0,0 +1,16 @@ +import * as React from 'react'; + +export const MissingScopes = () => { + return ( +
+

🙃

+ +

+ Missing Scopes. +

+
+ You are missing the notifications API scope. +
+
+ ); +}; diff --git a/src/components/Oops.test.tsx b/src/components/error/Oops.test.tsx similarity index 81% rename from src/components/Oops.test.tsx rename to src/components/error/Oops.test.tsx index 68aa3ee72..8f9aba1f7 100644 --- a/src/components/Oops.test.tsx +++ b/src/components/error/Oops.test.tsx @@ -1,11 +1,11 @@ import * as React from 'react'; import * as TestRenderer from 'react-test-renderer'; -import { mockMathRandom } from './test-utils'; +import { mockMathRandom } from '../test-utils'; import { Oops } from './Oops'; -describe('components/Oops.tsx', function () { +describe('components/error/Oops.tsx', function () { // The error emoji randomly rotates, but then the snapshots would never match // Have to make it consistent so the emojis are always the same mockMathRandom(0.1); diff --git a/src/components/Oops.tsx b/src/components/error/Oops.tsx similarity index 92% rename from src/components/Oops.tsx rename to src/components/error/Oops.tsx index f37e310e8..7e88a256d 100644 --- a/src/components/Oops.tsx +++ b/src/components/error/Oops.tsx @@ -1,6 +1,6 @@ import * as React from 'react'; -import { Constants } from '../utils/constants'; +import { Constants } from '../../utils/constants'; export const Oops = () => { const emoji = React.useMemo( diff --git a/src/components/error/RateLimit.test.tsx b/src/components/error/RateLimit.test.tsx new file mode 100644 index 000000000..98fbd1843 --- /dev/null +++ b/src/components/error/RateLimit.test.tsx @@ -0,0 +1,12 @@ +import * as React from 'react'; +import * as TestRenderer from 'react-test-renderer'; + +import { RateLimit } from './RateLimit'; + +describe('components/error/RateLimit.tsx', function () { + it('should render itself & its children', function () { + const tree = TestRenderer.create(); + + expect(tree).toMatchSnapshot(); + }); +}); diff --git a/src/components/error/RateLimit.tsx b/src/components/error/RateLimit.tsx new file mode 100644 index 000000000..6e611b279 --- /dev/null +++ b/src/components/error/RateLimit.tsx @@ -0,0 +1,14 @@ +import * as React from 'react'; + +export const RateLimit = () => { + return ( +
+

😮‍💨

+ +

+ Rate Limited. +

+
You have made too many requests.
+
+ ); +}; diff --git a/src/context/App.tsx b/src/context/App.tsx index b600889db..3b49e5d91 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -15,6 +15,7 @@ import { AuthState, AuthTokenOptions, SettingsState, + FailureType, } from '../types'; import { apiRequestAuth } from '../utils/api-requests'; import { setTheme } from '../utils/theme'; @@ -54,6 +55,7 @@ interface AppContextState { notifications: AccountNotifications[]; isFetching: boolean; requestFailed: boolean; + failureType: FailureType; removeNotificationFromState: (id: string, hostname: string) => void; fetchNotifications: () => Promise; markNotificationRead: (id: string, hostname: string) => Promise; @@ -75,6 +77,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { fetchNotifications, notifications, requestFailed, + failureType, isFetching, removeNotificationFromState, markNotificationRead, @@ -233,6 +236,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { notifications, isFetching, requestFailed, + failureType, removeNotificationFromState, fetchNotifications: fetchNotificationsWithAccounts, markNotificationRead: markNotificationReadWithAccounts, diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index d90bb7699..28cf29eda 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -1,8 +1,13 @@ -import axios from 'axios'; +import axios, { AxiosError } from 'axios'; import { useCallback, useState } from 'react'; -import { AccountNotifications, AuthState, SettingsState } from '../types'; -import { Notification } from '../typesGithub'; +import { + AccountNotifications, + AuthState, + SettingsState, + FailureType, +} from '../types'; +import { GithubRESTError, Notification } from '../typesGithub'; import { apiRequestAuth } from '../utils/api-requests'; import { getEnterpriseAccountToken, @@ -53,11 +58,14 @@ interface NotificationsState { ) => Promise; isFetching: boolean; requestFailed: boolean; + failureType: FailureType; } export const useNotifications = (colors: boolean): NotificationsState => { const [isFetching, setIsFetching] = useState(false); const [requestFailed, setRequestFailed] = useState(false); + const [failureType, setFailureType] = useState('NONE'); + const [notifications, setNotifications] = useState( [], ); @@ -65,7 +73,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { const fetchNotifications = useCallback( async (accounts: AuthState, settings: SettingsState) => { const isGitHubLoggedIn = accounts.token !== null; - const endpointSuffix = `notifications?participating=${settings.participating}`; + const endpointSuffix = `notifications?all=true&participating=${settings.participating}`; function getGitHubNotifications() { if (!isGitHubLoggedIn) { @@ -194,8 +202,25 @@ export const useNotifications = (colors: boolean): NotificationsState => { }); }), ) - .catch(() => { + .catch((err: AxiosError) => { + let failureType: FailureType = 'UNKNOWN'; + + const data = err.response.data as GithubRESTError; + + if (err.response.status === 401) { + failureType = 'BAD_CREDENTIALS'; + } else if (err.response.status === 403) { + if (data.message.includes('rate limit')) { + failureType = 'RATE_LIMIT'; + } else if ( + data.message.includes("Missing the 'notifications' scope") + ) { + failureType = 'MISSING_SCOPES'; + } + } + setIsFetching(false); + setFailureType(failureType); setRequestFailed(true); }); }, @@ -387,6 +412,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { return { isFetching, requestFailed, + failureType, notifications, removeNotificationFromState, diff --git a/src/routes/Notifications.tsx b/src/routes/Notifications.tsx index 478f549a4..3850d0987 100644 --- a/src/routes/Notifications.tsx +++ b/src/routes/Notifications.tsx @@ -3,11 +3,14 @@ import React, { useContext, useMemo } from 'react'; import { AccountNotifications } from '../components/AccountNotifications'; import { AllRead } from '../components/AllRead'; import { AppContext } from '../context/App'; -import { Oops } from '../components/Oops'; +import { Oops } from '../components/error/Oops'; import { getNotificationCount } from '../utils/notifications'; +import { RateLimit } from '../components/error/RateLimit'; +import { MissingScopes } from '../components/error/MissingScopes'; +import { BadCredentials } from '../components/error/BadCredentials'; export const NotificationsRoute: React.FC = (props) => { - const { notifications, requestFailed } = useContext(AppContext); + const { notifications, requestFailed, failureType } = useContext(AppContext); const hasMultipleAccounts = useMemo( () => notifications.length > 1, @@ -23,7 +26,16 @@ export const NotificationsRoute: React.FC = (props) => { ); if (requestFailed) { - return ; + switch (failureType) { + case 'BAD_CREDENTIALS': + return ; + case 'RATE_LIMIT': + return ; + case 'MISSING_SCOPES': + return ; + default: + return ; + } } if (!hasNotifications) { diff --git a/src/types.ts b/src/types.ts index f1a752847..0b1076532 100644 --- a/src/types.ts +++ b/src/types.ts @@ -74,3 +74,10 @@ export interface GitifyUser { name: string; id: number; } + +export type FailureType = + | 'BAD_CREDENTIALS' + | 'RATE_LIMIT' + | 'MISSING_SCOPES' + | 'UNKNOWN' + | 'NONE'; diff --git a/src/typesGithub.ts b/src/typesGithub.ts index cdef6ddf7..e49e651a4 100644 --- a/src/typesGithub.ts +++ b/src/typesGithub.ts @@ -366,3 +366,8 @@ export interface WorkflowRunAttributes { statusDisplayName: string; status: CheckSuiteStatus | null; } + +export interface GithubRESTError { + message: string; + documentation_url: string; +} From a9ba174b66253d630c8149cc960be0a52dec20d9 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 5 Apr 2024 23:33:36 -0400 Subject: [PATCH 02/14] feat: detailed error state handling --- src/components/error/BadCredentials.tsx | 4 +- src/components/error/MissingScopes.tsx | 4 +- src/components/error/Oops.tsx | 4 +- src/components/error/RateLimit.tsx | 6 +- .../BadCredentials.test.tsx.snap | 21 ++++ .../__snapshots__/MissingScopes.test.tsx.snap | 25 +++++ .../__snapshots__/Oops.test.tsx.snap | 6 +- .../__snapshots__/RateLimit.test.tsx.snap | 21 ++++ src/hooks/useNotifications.ts | 3 +- src/routes/Notifications.test.tsx | 2 +- src/utils/subject.ts | 101 +++++++++++------- 11 files changed, 144 insertions(+), 53 deletions(-) create mode 100644 src/components/error/__snapshots__/BadCredentials.test.tsx.snap create mode 100644 src/components/error/__snapshots__/MissingScopes.test.tsx.snap rename src/components/{ => error}/__snapshots__/Oops.test.tsx.snap (70%) create mode 100644 src/components/error/__snapshots__/RateLimit.test.tsx.snap diff --git a/src/components/error/BadCredentials.tsx b/src/components/error/BadCredentials.tsx index 9cc3d313e..ebcddbc64 100644 --- a/src/components/error/BadCredentials.tsx +++ b/src/components/error/BadCredentials.tsx @@ -6,9 +6,9 @@ export const BadCredentials = () => {

🔓

- Bad Credentials. + Bad Credentials

-
The credentials you are may be invalid or expired.
+
The credentials you are may be invalid or expired
); }; diff --git a/src/components/error/MissingScopes.tsx b/src/components/error/MissingScopes.tsx index 91b32f62d..49aba287d 100644 --- a/src/components/error/MissingScopes.tsx +++ b/src/components/error/MissingScopes.tsx @@ -6,10 +6,10 @@ export const MissingScopes = () => {

🙃

- Missing Scopes. + Missing Scopes

- You are missing the notifications API scope. + You are missing the notifications API scope
); diff --git a/src/components/error/Oops.tsx b/src/components/error/Oops.tsx index 7e88a256d..8cb39652d 100644 --- a/src/components/error/Oops.tsx +++ b/src/components/error/Oops.tsx @@ -16,9 +16,9 @@ export const Oops = () => {

{emoji}

- Something went wrong. + Something went wrong

-
Couldn't get your notifications.
+
Couldn't get your notifications
); }; diff --git a/src/components/error/RateLimit.tsx b/src/components/error/RateLimit.tsx index 6e611b279..3d6b4e520 100644 --- a/src/components/error/RateLimit.tsx +++ b/src/components/error/RateLimit.tsx @@ -5,10 +5,8 @@ export const RateLimit = () => {

😮‍💨

-

- Rate Limited. -

-
You have made too many requests.
+

Rate Limited

+
You have made too many requests
); }; diff --git a/src/components/error/__snapshots__/BadCredentials.test.tsx.snap b/src/components/error/__snapshots__/BadCredentials.test.tsx.snap new file mode 100644 index 000000000..292bc6f2c --- /dev/null +++ b/src/components/error/__snapshots__/BadCredentials.test.tsx.snap @@ -0,0 +1,21 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`components/error/BadCredentials.tsx should render itself & its children 1`] = ` +
+

+ 🔓 +

+

+ Bad Credentials +

+
+ The credentials you are may be invalid or expired +
+
+`; diff --git a/src/components/error/__snapshots__/MissingScopes.test.tsx.snap b/src/components/error/__snapshots__/MissingScopes.test.tsx.snap new file mode 100644 index 000000000..196f9ceee --- /dev/null +++ b/src/components/error/__snapshots__/MissingScopes.test.tsx.snap @@ -0,0 +1,25 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`components/error/MissingScopes.tsx should render itself & its children 1`] = ` +
+

+ 🙃 +

+

+ Missing Scopes +

+
+ You are missing the + + notifications + + API scope +
+
+`; diff --git a/src/components/__snapshots__/Oops.test.tsx.snap b/src/components/error/__snapshots__/Oops.test.tsx.snap similarity index 70% rename from src/components/__snapshots__/Oops.test.tsx.snap rename to src/components/error/__snapshots__/Oops.test.tsx.snap index 661f8d201..2b1fd3101 100644 --- a/src/components/__snapshots__/Oops.test.tsx.snap +++ b/src/components/error/__snapshots__/Oops.test.tsx.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`components/Oops.tsx should render itself & its children 1`] = ` +exports[`components/error/Oops.tsx should render itself & its children 1`] = `
@@ -12,10 +12,10 @@ exports[`components/Oops.tsx should render itself & its children 1`] = `

- Something went wrong. + Something went wrong

- Couldn't get your notifications. + Couldn't get your notifications
`; diff --git a/src/components/error/__snapshots__/RateLimit.test.tsx.snap b/src/components/error/__snapshots__/RateLimit.test.tsx.snap new file mode 100644 index 000000000..7b26db238 --- /dev/null +++ b/src/components/error/__snapshots__/RateLimit.test.tsx.snap @@ -0,0 +1,21 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`components/error/RateLimit.tsx should render itself & its children 1`] = ` +
+

+ 😮‍💨 +

+

+ Rate Limited +

+
+ You have made too many requests +
+
+`; diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 28cf29eda..c6d036d99 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -95,6 +95,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { } setIsFetching(true); + setFailureType('NONE'); setRequestFailed(false); return axios @@ -210,7 +211,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { if (err.response.status === 401) { failureType = 'BAD_CREDENTIALS'; } else if (err.response.status === 403) { - if (data.message.includes('rate limit')) { + if (data.message.includes('API rate limit exceeded')) { failureType = 'RATE_LIMIT'; } else if ( data.message.includes("Missing the 'notifications' scope") diff --git a/src/routes/Notifications.test.tsx b/src/routes/Notifications.test.tsx index 9f7f8ee65..f09f3f4b7 100644 --- a/src/routes/Notifications.test.tsx +++ b/src/routes/Notifications.test.tsx @@ -13,7 +13,7 @@ jest.mock('../components/AllRead', () => ({ AllRead: 'AllRead', })); -jest.mock('../components/Oops', () => ({ +jest.mock('../components/error/Oops', () => ({ Oops: 'Oops', })); diff --git a/src/utils/subject.ts b/src/utils/subject.ts index 35c70a66c..ee94d15f0 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -130,47 +130,61 @@ async function getGitifySubjectForIssue( notification: Notification, token: string, ): Promise { - const issue: Issue = ( - await apiRequestAuth(notification.subject.url, 'GET', token) - ).data; + try { + const issue: Issue = ( + await apiRequestAuth(notification.subject.url, 'GET', token) + ).data; - const issueCommentUser = await getLatestCommentUser(notification, token); + const issueCommentUser = await getLatestCommentUser(notification, token); - return { - state: issue.state_reason ?? issue.state, - user: { - login: issueCommentUser?.login ?? issue.user.login, - html_url: issueCommentUser?.html_url ?? issue.user.html_url, - type: issueCommentUser?.type ?? issue.user.type, - }, - }; + return { + state: issue.state_reason ?? issue.state, + user: { + login: issueCommentUser?.login ?? issue.user.login, + html_url: issueCommentUser?.html_url ?? issue.user.html_url, + type: issueCommentUser?.type ?? issue.user.type, + }, + }; + } catch (err) { + console.error( + 'Issue subject retrieval failed', + JSON.stringify(err.response.data), + ); + } } async function getGitifySubjectForPullRequest( notification: Notification, token: string, ): Promise { - const pr: PullRequest = ( - await apiRequestAuth(notification.subject.url, 'GET', token) - ).data; - - let prState: PullRequestStateType = pr.state; - if (pr.merged) { - prState = 'merged'; - } else if (pr.draft) { - prState = 'draft'; - } + try { + const pr: PullRequest = ( + await apiRequestAuth(notification.subject.url, 'GET', token) + ).data; + + let prState: PullRequestStateType = pr.state; + if (pr.merged) { + prState = 'merged'; + } else if (pr.draft) { + prState = 'draft'; + } - const prCommentUser = await getLatestCommentUser(notification, token); + const prCommentUser = await getLatestCommentUser(notification, token); - return { - state: prState, - user: { - login: prCommentUser?.login ?? pr.user.login, - html_url: prCommentUser?.html_url ?? pr.user.html_url, - type: prCommentUser?.type ?? pr.user.type, - }, - }; + return { + state: prState, + user: { + login: prCommentUser?.login ?? pr.user.login, + html_url: prCommentUser?.html_url ?? pr.user.html_url, + type: prCommentUser?.type ?? pr.user.type, + }, + }; + } catch (err) { + console.error( + 'Pull Request subject retrieval failed', + JSON.stringify(err.response.data), + ); + } } async function getGitifySubjectForRelease( @@ -240,11 +254,22 @@ async function getLatestCommentUser( return null; } - const response: IssueComments | ReleaseComments = ( - await apiRequestAuth(notification.subject.latest_comment_url, 'GET', token) - )?.data; - - return ( - (response as IssueComments)?.user ?? (response as ReleaseComments).author - ); + try { + const response: IssueComments | ReleaseComments = ( + await apiRequestAuth( + notification.subject.latest_comment_url, + 'GET', + token, + ) + )?.data; + + return ( + (response as IssueComments)?.user ?? (response as ReleaseComments).author + ); + } catch (err) { + console.error( + 'Discussion latest comment retrieval failed', + JSON.stringify(err.response.data), + ); + } } From 088fb0be05fc38738332c6c5a78961597a12d1ba Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 5 Apr 2024 23:36:25 -0400 Subject: [PATCH 03/14] feat: detailed error state handling --- src/hooks/useNotifications.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index c6d036d99..76397dd9d 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -211,7 +211,10 @@ export const useNotifications = (colors: boolean): NotificationsState => { if (err.response.status === 401) { failureType = 'BAD_CREDENTIALS'; } else if (err.response.status === 403) { - if (data.message.includes('API rate limit exceeded')) { + if ( + data.message.includes('API rate limit exceeded') || + data.message.includes('You have exceeded a secondary rate limit') + ) { failureType = 'RATE_LIMIT'; } else if ( data.message.includes("Missing the 'notifications' scope") From 25329549ff1e38848b6bf1088549eac87a51ff46 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 6 Apr 2024 06:52:23 -0400 Subject: [PATCH 04/14] feat: detailed error state handling --- src/hooks/useNotifications.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 76397dd9d..916a030a3 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -73,7 +73,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { const fetchNotifications = useCallback( async (accounts: AuthState, settings: SettingsState) => { const isGitHubLoggedIn = accounts.token !== null; - const endpointSuffix = `notifications?all=true&participating=${settings.participating}`; + const endpointSuffix = `notifications?participating=${settings.participating}`; function getGitHubNotifications() { if (!isGitHubLoggedIn) { @@ -204,6 +204,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { }), ) .catch((err: AxiosError) => { + console.log(JSON.stringify(err)); let failureType: FailureType = 'UNKNOWN'; const data = err.response.data as GithubRESTError; From 645b1e79203cffc2dd03542dbf62f68362730834 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 6 Apr 2024 07:11:04 -0400 Subject: [PATCH 05/14] feat: detailed error state handling --- src/hooks/useNotifications.test.ts | 256 +++++++++++++++++++++++++---- src/hooks/useNotifications.ts | 15 +- src/types.ts | 5 +- 3 files changed, 236 insertions(+), 40 deletions(-) diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index b5ca27b19..fb0736a12 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -48,30 +48,129 @@ describe('hooks/useNotifications.ts', () => { expect(result.current.notifications[1].hostname).toBe('github.com'); }); - it('should fetch notifications with failure - github.com & enterprise', async () => { - const message = 'Oops! Something went wrong.'; + describe('should fetch notifications with failures - github.com & enterprise', () => { + it('bad credentials', async () => { + const status = 401; + const message = 'Bad credentials'; - nock('https://api.github.com/') - .get('/notifications?participating=false') - .reply(400, { message }); + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); - nock('https://github.gitify.io/api/v3/') - .get('/notifications?participating=false') - .reply(400, { message }); + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications(false)); - act(() => { - result.current.fetchNotifications(mockAccounts, mockSettings); + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('BAD_CREDENTIALS'); + }); }); - expect(result.current.isFetching).toBe(true); + it('missing scopes', async () => { + const status = 403; + const message = "Missing the 'notifications' scope"; - await waitFor(() => { - expect(result.current.isFetching).toBe(false); + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('MISSING_SCOPES'); + }); }); - expect(result.current.requestFailed).toBe(true); + it('rate limited - primary', async () => { + const status = 403; + const message = 'API rate limit exceeded'; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('RATE_LIMITED'); + }); + }); + + it('rate limited - secondary', async () => { + const status = 403; + const message = 'You have exceeded a secondary rate limit'; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('RATE_LIMITED'); + }); + }); + + it('default error', async () => { + const status = 400; + const message = 'Oops! Something went wrong.'; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + expect(result.current.isFetching).toBe(true); + + await waitFor(() => { + expect(result.current.isFetching).toBe(false); + }); + + expect(result.current.requestFailed).toBe(true); + }); }); }); @@ -158,24 +257,125 @@ describe('hooks/useNotifications.ts', () => { expect(result.current.notifications[0].notifications.length).toBe(2); }); - it('should fetch notifications with failure - github.com only', async () => { - const accounts: AuthState = { - ...mockAccounts, - enterpriseAccounts: [], - }; + describe('should fetch notifications with failures - github.com only', () => { + it('bad credentials', async () => { + const status = 401; + const message = 'Bad credentials'; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; - nock('https://api.github.com/') - .get('/notifications?participating=false') - .reply(400, { message: 'Oops! Something went wrong.' }); + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications(false)); - act(() => { - result.current.fetchNotifications(accounts, mockSettings); + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('BAD_CREDENTIALS'); + }); }); - await waitFor(() => { - expect(result.current.requestFailed).toBe(true); + it('missing scopes', async () => { + const status = 403; + const message = "Missing the 'notifications' scope"; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('MISSING_SCOPES'); + }); + }); + + it('rate limited - primary', async () => { + const status = 403; + const message = 'API rate limit exceeded'; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('RATE_LIMITED'); + }); + }); + + it('rate limited - secondary', async () => { + const status = 403; + const message = 'You have exceeded a secondary rate limit'; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('RATE_LIMITED'); + }); + }); + + it('default error', async () => { + const status = 400; + const message = 'Oops! Something went wrong.'; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('UNKNOWN'); + }); }); }); }); diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 916a030a3..776880375 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -64,7 +64,7 @@ interface NotificationsState { export const useNotifications = (colors: boolean): NotificationsState => { const [isFetching, setIsFetching] = useState(false); const [requestFailed, setRequestFailed] = useState(false); - const [failureType, setFailureType] = useState('NONE'); + const [failureType, setFailureType] = useState(); const [notifications, setNotifications] = useState( [], @@ -95,7 +95,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { } setIsFetching(true); - setFailureType('NONE'); + setFailureType(null); setRequestFailed(false); return axios @@ -204,7 +204,6 @@ export const useNotifications = (colors: boolean): NotificationsState => { }), ) .catch((err: AxiosError) => { - console.log(JSON.stringify(err)); let failureType: FailureType = 'UNKNOWN'; const data = err.response.data as GithubRESTError; @@ -212,15 +211,13 @@ export const useNotifications = (colors: boolean): NotificationsState => { if (err.response.status === 401) { failureType = 'BAD_CREDENTIALS'; } else if (err.response.status === 403) { - if ( + if (data.message.includes("Missing the 'notifications' scope")) { + failureType = 'MISSING_SCOPES'; + } else if ( data.message.includes('API rate limit exceeded') || data.message.includes('You have exceeded a secondary rate limit') ) { - failureType = 'RATE_LIMIT'; - } else if ( - data.message.includes("Missing the 'notifications' scope") - ) { - failureType = 'MISSING_SCOPES'; + failureType = 'RATE_LIMITED'; } } diff --git a/src/types.ts b/src/types.ts index 0b1076532..5168700c1 100644 --- a/src/types.ts +++ b/src/types.ts @@ -77,7 +77,6 @@ export interface GitifyUser { export type FailureType = | 'BAD_CREDENTIALS' - | 'RATE_LIMIT' | 'MISSING_SCOPES' - | 'UNKNOWN' - | 'NONE'; + | 'RATE_LIMITED' + | 'UNKNOWN'; From d3e5655637ba933a08ec472e858770baf2037c5e Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 6 Apr 2024 07:11:45 -0400 Subject: [PATCH 06/14] feat: detailed error state handling --- src/routes/Notifications.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/routes/Notifications.tsx b/src/routes/Notifications.tsx index 3850d0987..c0710f693 100644 --- a/src/routes/Notifications.tsx +++ b/src/routes/Notifications.tsx @@ -29,10 +29,10 @@ export const NotificationsRoute: React.FC = (props) => { switch (failureType) { case 'BAD_CREDENTIALS': return ; - case 'RATE_LIMIT': - return ; case 'MISSING_SCOPES': return ; + case 'RATE_LIMITED': + return ; default: return ; } From 79a1737398db65057e6a31480f34a2c3685a0c16 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 6 Apr 2024 07:37:55 -0400 Subject: [PATCH 07/14] feat: detailed error state handling --- src/utils/subject.ts | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/utils/subject.ts b/src/utils/subject.ts index ee94d15f0..9df974f21 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -146,10 +146,7 @@ async function getGitifySubjectForIssue( }, }; } catch (err) { - console.error( - 'Issue subject retrieval failed', - JSON.stringify(err.response.data), - ); + console.error('Issue subject retrieval failed'); } } @@ -180,10 +177,7 @@ async function getGitifySubjectForPullRequest( }, }; } catch (err) { - console.error( - 'Pull Request subject retrieval failed', - JSON.stringify(err.response.data), - ); + console.error('Pull Request subject retrieval failed'); } } @@ -267,9 +261,6 @@ async function getLatestCommentUser( (response as IssueComments)?.user ?? (response as ReleaseComments).author ); } catch (err) { - console.error( - 'Discussion latest comment retrieval failed', - JSON.stringify(err.response.data), - ); + console.error('Discussion latest comment retrieval failed'); } } From 595e074475912e2b27b6e64cfbc96a58a5176ebc Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 6 Apr 2024 07:43:19 -0400 Subject: [PATCH 08/14] feat: detailed error state handling --- src/utils/helpers.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index 4a503dca8..4fa4a2064 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -78,11 +78,14 @@ export function formatSearchQueryString( } export async function getHtmlUrl(url: string, token: string): Promise { - const response: Issue | IssueComments | PullRequest = ( - await apiRequestAuth(url, 'GET', token) - ).data; - - return response.html_url; + try { + const response: Issue | IssueComments | PullRequest = ( + await apiRequestAuth(url, 'GET', token) + ).data; + return response.html_url; + } catch (err) { + console.error('Failed to get html url'); + } } export function getCheckSuiteUrl(notification: Notification) { @@ -280,6 +283,10 @@ export async function generateGitHubWebUrl( } } + if (!url) { + url; + } + url = addNotificationReferrerIdToUrl(url, notification.id, accounts.user?.id); return url; From b28a95530a6ae8c4c752e75f7f58a524f16ca1ba Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 6 Apr 2024 07:50:51 -0400 Subject: [PATCH 09/14] feat: detailed error state handling --- ...ateLimit.test.tsx => RateLimited.test.tsx} | 6 +- .../error/{RateLimit.tsx => RateLimited.tsx} | 2 +- ...est.tsx.snap => RateLimited.test.tsx.snap} | 2 +- src/routes/Notifications.test.tsx | 82 +++++++++++++++++-- src/routes/Notifications.tsx | 4 +- .../__snapshots__/Notifications.test.tsx.snap | 8 +- 6 files changed, 89 insertions(+), 15 deletions(-) rename src/components/error/{RateLimit.test.tsx => RateLimited.test.tsx} (54%) rename src/components/error/{RateLimit.tsx => RateLimited.tsx} (91%) rename src/components/error/__snapshots__/{RateLimit.test.tsx.snap => RateLimited.test.tsx.snap} (81%) diff --git a/src/components/error/RateLimit.test.tsx b/src/components/error/RateLimited.test.tsx similarity index 54% rename from src/components/error/RateLimit.test.tsx rename to src/components/error/RateLimited.test.tsx index 98fbd1843..5f1732a0f 100644 --- a/src/components/error/RateLimit.test.tsx +++ b/src/components/error/RateLimited.test.tsx @@ -1,11 +1,11 @@ import * as React from 'react'; import * as TestRenderer from 'react-test-renderer'; -import { RateLimit } from './RateLimit'; +import { RateLimited } from './RateLimited'; -describe('components/error/RateLimit.tsx', function () { +describe('components/error/RateLimited.tsx', function () { it('should render itself & its children', function () { - const tree = TestRenderer.create(); + const tree = TestRenderer.create(); expect(tree).toMatchSnapshot(); }); diff --git a/src/components/error/RateLimit.tsx b/src/components/error/RateLimited.tsx similarity index 91% rename from src/components/error/RateLimit.tsx rename to src/components/error/RateLimited.tsx index 3d6b4e520..707bffd12 100644 --- a/src/components/error/RateLimit.tsx +++ b/src/components/error/RateLimited.tsx @@ -1,6 +1,6 @@ import * as React from 'react'; -export const RateLimit = () => { +export const RateLimited = () => { return (

😮‍💨

diff --git a/src/components/error/__snapshots__/RateLimit.test.tsx.snap b/src/components/error/__snapshots__/RateLimited.test.tsx.snap similarity index 81% rename from src/components/error/__snapshots__/RateLimit.test.tsx.snap rename to src/components/error/__snapshots__/RateLimited.test.tsx.snap index 7b26db238..9aa55258d 100644 --- a/src/components/error/__snapshots__/RateLimit.test.tsx.snap +++ b/src/components/error/__snapshots__/RateLimited.test.tsx.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`components/error/RateLimit.tsx should render itself & its children 1`] = ` +exports[`components/error/RateLimited.tsx should render itself & its children 1`] = `
diff --git a/src/routes/Notifications.test.tsx b/src/routes/Notifications.test.tsx index f09f3f4b7..d6731f901 100644 --- a/src/routes/Notifications.test.tsx +++ b/src/routes/Notifications.test.tsx @@ -13,10 +13,22 @@ jest.mock('../components/AllRead', () => ({ AllRead: 'AllRead', })); +jest.mock('../components/error/BadCredentials', () => ({ + BadCredentials: 'BadC redentials', +})); + +jest.mock('../components/error/MissingScopes', () => ({ + MissingScopes: 'Missing Scopes', +})); + jest.mock('../components/error/Oops', () => ({ Oops: 'Oops', })); +jest.mock('../components/error/RateLimited', () => ({ + RateLimited: 'Rate Limited', +})); + describe('routes/Notifications.tsx', () => { it('should render itself & its children (with notifications)', () => { const tree = TestRenderer.create( @@ -39,13 +51,69 @@ describe('routes/Notifications.tsx', () => { expect(tree).toMatchSnapshot(); }); - it('should render itself & its children (error page - oops)', () => { - const tree = TestRenderer.create( - - - , - ); + describe('should render itself & its children (error conditions - oops)', () => { + it('bad credentials', () => { + const tree = TestRenderer.create( + + + , + ); - expect(tree).toMatchSnapshot(); + expect(tree).toMatchSnapshot(); + }); + + it('missing scopes', () => { + const tree = TestRenderer.create( + + + , + ); + + expect(tree).toMatchSnapshot(); + }); + + it('rate limited', () => { + const tree = TestRenderer.create( + + + , + ); + + expect(tree).toMatchSnapshot(); + }); + + it('default error', () => { + const tree = TestRenderer.create( + + + , + ); + + expect(tree).toMatchSnapshot(); + }); }); }); diff --git a/src/routes/Notifications.tsx b/src/routes/Notifications.tsx index c0710f693..efd96dacc 100644 --- a/src/routes/Notifications.tsx +++ b/src/routes/Notifications.tsx @@ -5,7 +5,7 @@ import { AllRead } from '../components/AllRead'; import { AppContext } from '../context/App'; import { Oops } from '../components/error/Oops'; import { getNotificationCount } from '../utils/notifications'; -import { RateLimit } from '../components/error/RateLimit'; +import { RateLimited } from '../components/error/RateLimited'; import { MissingScopes } from '../components/error/MissingScopes'; import { BadCredentials } from '../components/error/BadCredentials'; @@ -32,7 +32,7 @@ export const NotificationsRoute: React.FC = (props) => { case 'MISSING_SCOPES': return ; case 'RATE_LIMITED': - return ; + return ; default: return ; } diff --git a/src/routes/__snapshots__/Notifications.test.tsx.snap b/src/routes/__snapshots__/Notifications.test.tsx.snap index 7ca8cdc41..70d5f53dd 100644 --- a/src/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/routes/__snapshots__/Notifications.test.tsx.snap @@ -2,7 +2,13 @@ exports[`routes/Notifications.tsx should render itself & its children (all read notifications) 1`] = ``; -exports[`routes/Notifications.tsx should render itself & its children (error page - oops) 1`] = ``; +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) bad credentials 1`] = ``; + +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) default error 1`] = ``; + +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) missing scopes 1`] = ``; + +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) rate limited 1`] = ``; exports[`routes/Notifications.tsx should render itself & its children (with notifications) 1`] = `
Date: Sat, 6 Apr 2024 10:45:47 -0400 Subject: [PATCH 10/14] feat: detailed error state handling --- src/hooks/useNotifications.ts | 42 +++++++++++++++++++---------------- src/utils/constants.ts | 7 ++++++ 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 776880375..435213a19 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -203,27 +203,11 @@ export const useNotifications = (colors: boolean): NotificationsState => { }); }), ) - .catch((err: AxiosError) => { - let failureType: FailureType = 'UNKNOWN'; - - const data = err.response.data as GithubRESTError; - - if (err.response.status === 401) { - failureType = 'BAD_CREDENTIALS'; - } else if (err.response.status === 403) { - if (data.message.includes("Missing the 'notifications' scope")) { - failureType = 'MISSING_SCOPES'; - } else if ( - data.message.includes('API rate limit exceeded') || - data.message.includes('You have exceeded a secondary rate limit') - ) { - failureType = 'RATE_LIMITED'; - } - } - + .catch((err: AxiosError) => { setIsFetching(false); - setFailureType(failureType); setRequestFailed(true); + const failureType: FailureType = determineFailureType(err); + setFailureType(failureType); }); }, [notifications], @@ -426,3 +410,23 @@ export const useNotifications = (colors: boolean): NotificationsState => { markRepoNotificationsDone, }; }; + +function determineFailureType(err: AxiosError) { + let failureType: FailureType = 'UNKNOWN'; + const status = err.response.status; + const message = err.response.data.message; + + if (status === 401) { + failureType = 'BAD_CREDENTIALS'; + } else if (status === 403) { + if (message.includes(Constants.ERROR_MESSAGES.MISSING_SCOPES)) { + failureType = 'MISSING_SCOPES'; + } else if ( + message.includes(Constants.ERROR_MESSAGES.RATE_LIMITED_PRIMARY) || + message.includes(Constants.ERROR_MESSAGES.RATE_LIMITED_SECONDARY) + ) { + failureType = 'RATE_LIMITED'; + } + } + return failureType; +} diff --git a/src/utils/constants.ts b/src/utils/constants.ts index e83de689f..336cb8a78 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -16,6 +16,13 @@ export const Constants = { ALLREAD_EMOJIS: ['😉', '🎉', '🐯', '🙈', '🎈', '🎊', '👏', '🎪', '🍝'], ERROR_EMOJIS: ['🤔', '😞', '😤', '😱', '😭'], + + ERROR_MESSAGES: { + BAD_CREDENTIALS: 'Bad credentials', + MISSING_SCOPES: "Missing the 'notifications' scope", + RATE_LIMITED_PRIMARY: 'API rate limit exceeded', + RATE_LIMITED_SECONDARY: 'You have exceeded a secondary rate limit', + }, }; export default Constants; From 4bca336c41cce8858409a9f156481ebbbea0dcbf Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 6 Apr 2024 18:37:03 -0400 Subject: [PATCH 11/14] feat: detailed error state handling --- src/routes/__snapshots__/Notifications.test.tsx.snap | 2 +- src/utils/constants.ts | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/routes/__snapshots__/Notifications.test.tsx.snap b/src/routes/__snapshots__/Notifications.test.tsx.snap index b47143823..dd8b1b9a7 100644 --- a/src/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/routes/__snapshots__/Notifications.test.tsx.snap @@ -52,7 +52,7 @@ exports[`routes/Notifications.tsx should render itself & its children (error con Date: Sat, 6 Apr 2024 18:53:25 -0400 Subject: [PATCH 12/14] remove this --- src/utils/helpers.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index bcc2832ee..79a5d56de 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -286,10 +286,6 @@ export async function generateGitHubWebUrl( } } - if (!url) { - url; - } - url = addNotificationReferrerIdToUrl(url, notification.id, accounts.user?.id); return url; From 17c10a809c75ff685c295808614d81f4588cc1ce Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 7 Apr 2024 07:38:22 -0400 Subject: [PATCH 13/14] feat: detailed error state handling --- .husky/pre-commit | 2 +- README.md | 2 +- src/components/Error.test.tsx | 17 ++ src/components/{Oops.tsx => Error.tsx} | 17 +- src/components/Oops.test.tsx | 18 -- ...Oops.test.tsx.snap => Error.test.tsx.snap} | 8 +- src/context/App.tsx | 4 + src/hooks/useNotifications.test.ts | 256 ++++++++++++++++-- src/hooks/useNotifications.ts | 40 ++- src/routes/Notifications.test.tsx | 74 ++++- src/routes/Notifications.tsx | 17 +- .../__snapshots__/Notifications.test.tsx.snap | 60 +++- src/types.ts | 12 + src/typesGithub.ts | 5 + src/utils/constants.ts | 23 +- src/utils/helpers.ts | 13 +- src/utils/subject.ts | 96 ++++--- 17 files changed, 541 insertions(+), 123 deletions(-) mode change 100644 => 100755 .husky/pre-commit create mode 100644 src/components/Error.test.tsx rename src/components/{Oops.tsx => Error.tsx} (57%) delete mode 100644 src/components/Oops.test.tsx rename src/components/__snapshots__/{Oops.test.tsx.snap => Error.test.tsx.snap} (68%) diff --git a/.husky/pre-commit b/.husky/pre-commit old mode 100644 new mode 100755 index 88c91d674..c6254913e --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -1,2 +1,2 @@ pnpx lint-staged -pnpm test -- --onlyChanged \ No newline at end of file +pnpm test -- --onlyChanged diff --git a/README.md b/README.md index f740f0ba3..e0b38aa8c 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ Gitify is licensed under the MIT Open Source license. For more information, see [brew]: http://brew.sh/ [homebrew-cask]: https://formulae.brew.sh/cask/gitify [coveralls]: https://coveralls.io/github/gitify-app/gitify -[coveralls-badge]: https://img.shields.io/coverallsCoverage/github/gitify-app/gitify +[coveralls-badge]: https://coveralls.io/repos/github/gitify-app/gitify/badge.svg [build-workflow-badge]: https://github.com/gitify-app/gitify/actions/workflows/build-app.yml/badge.svg [release-workflow-badge]: https://github.com/gitify-app/gitify/actions/workflows/release.yml/badge.svg [downloads-total-badge]: https://img.shields.io/github/downloads/gitify-app/gitify/total?label=downloads@all diff --git a/src/components/Error.test.tsx b/src/components/Error.test.tsx new file mode 100644 index 000000000..90b7b25d7 --- /dev/null +++ b/src/components/Error.test.tsx @@ -0,0 +1,17 @@ +import * as React from 'react'; +import * as TestRenderer from 'react-test-renderer'; + +import { Error } from './Error'; + +describe('components/Error.tsx', function () { + it('should render itself & its children', function () { + const mockError = { + title: 'Error title', + description: 'Error description', + emojis: ['🔥'], + }; + const tree = TestRenderer.create(); + + expect(tree).toMatchSnapshot(); + }); +}); diff --git a/src/components/Oops.tsx b/src/components/Error.tsx similarity index 57% rename from src/components/Oops.tsx rename to src/components/Error.tsx index f37e310e8..a5ecaffe2 100644 --- a/src/components/Oops.tsx +++ b/src/components/Error.tsx @@ -1,13 +1,14 @@ import * as React from 'react'; -import { Constants } from '../utils/constants'; +import { GitifyError } from '../types'; -export const Oops = () => { +interface IProps { + error: GitifyError; +} + +export const Error: React.FC = ({ error }) => { const emoji = React.useMemo( - () => - Constants.ERROR_EMOJIS[ - Math.floor(Math.random() * Constants.ERROR_EMOJIS.length) - ], + () => error.emojis[Math.floor(Math.random() * error.emojis.length)], [], ); @@ -16,9 +17,9 @@ export const Oops = () => {

{emoji}

- Something went wrong. + {error.title}

-
Couldn't get your notifications.
+
{error.description}
); }; diff --git a/src/components/Oops.test.tsx b/src/components/Oops.test.tsx deleted file mode 100644 index 68aa3ee72..000000000 --- a/src/components/Oops.test.tsx +++ /dev/null @@ -1,18 +0,0 @@ -import * as React from 'react'; -import * as TestRenderer from 'react-test-renderer'; - -import { mockMathRandom } from './test-utils'; - -import { Oops } from './Oops'; - -describe('components/Oops.tsx', function () { - // The error emoji randomly rotates, but then the snapshots would never match - // Have to make it consistent so the emojis are always the same - mockMathRandom(0.1); - - it('should render itself & its children', function () { - const tree = TestRenderer.create(); - - expect(tree).toMatchSnapshot(); - }); -}); diff --git a/src/components/__snapshots__/Oops.test.tsx.snap b/src/components/__snapshots__/Error.test.tsx.snap similarity index 68% rename from src/components/__snapshots__/Oops.test.tsx.snap rename to src/components/__snapshots__/Error.test.tsx.snap index 661f8d201..dbf0ee056 100644 --- a/src/components/__snapshots__/Oops.test.tsx.snap +++ b/src/components/__snapshots__/Error.test.tsx.snap @@ -1,21 +1,21 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`components/Oops.tsx should render itself & its children 1`] = ` +exports[`components/Error.tsx should render itself & its children 1`] = `

- 🤔 + 🔥

- Something went wrong. + Error title

- Couldn't get your notifications. + Error description
`; diff --git a/src/context/App.tsx b/src/context/App.tsx index 9b785fb08..6f94a5a5c 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -15,6 +15,7 @@ import { AuthState, AuthTokenOptions, SettingsState, + FailureType, } from '../types'; import { apiRequestAuth } from '../utils/api-requests'; import { setTheme } from '../utils/theme'; @@ -55,6 +56,7 @@ interface AppContextState { notifications: AccountNotifications[]; isFetching: boolean; requestFailed: boolean; + failureType: FailureType; removeNotificationFromState: (id: string, hostname: string) => void; fetchNotifications: () => Promise; markNotificationRead: (id: string, hostname: string) => Promise; @@ -76,6 +78,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { fetchNotifications, notifications, requestFailed, + failureType, isFetching, removeNotificationFromState, markNotificationRead, @@ -234,6 +237,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { notifications, isFetching, requestFailed, + failureType, removeNotificationFromState, fetchNotifications: fetchNotificationsWithAccounts, markNotificationRead: markNotificationReadWithAccounts, diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index 5fe63c040..13d94e9a8 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -48,30 +48,129 @@ describe('hooks/useNotifications.ts', () => { expect(result.current.notifications[1].hostname).toBe('github.com'); }); - it('should fetch notifications with failure - github.com & enterprise', async () => { - const message = 'Oops! Something went wrong.'; + describe('should fetch notifications with failures - github.com & enterprise', () => { + it('bad credentials', async () => { + const status = 401; + const message = 'Bad credentials'; - nock('https://api.github.com/') - .get('/notifications?participating=false') - .reply(400, { message }); + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); - nock('https://github.gitify.io/api/v3/') - .get('/notifications?participating=false') - .reply(400, { message }); + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications(false)); - act(() => { - result.current.fetchNotifications(mockAccounts, mockSettings); + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('BAD_CREDENTIALS'); + }); }); - expect(result.current.isFetching).toBe(true); + it('missing scopes', async () => { + const status = 403; + const message = "Missing the 'notifications' scope"; - await waitFor(() => { - expect(result.current.isFetching).toBe(false); + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('MISSING_SCOPES'); + }); }); - expect(result.current.requestFailed).toBe(true); + it('rate limited - primary', async () => { + const status = 403; + const message = 'API rate limit exceeded'; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('RATE_LIMITED'); + }); + }); + + it('rate limited - secondary', async () => { + const status = 403; + const message = 'You have exceeded a secondary rate limit'; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('RATE_LIMITED'); + }); + }); + + it('default error', async () => { + const status = 400; + const message = 'Oops! Something went wrong.'; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + nock('https://github.gitify.io/api/v3/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(mockAccounts, mockSettings); + }); + + expect(result.current.isFetching).toBe(true); + + await waitFor(() => { + expect(result.current.isFetching).toBe(false); + }); + + expect(result.current.requestFailed).toBe(true); + }); }); }); @@ -158,24 +257,125 @@ describe('hooks/useNotifications.ts', () => { expect(result.current.notifications[0].notifications.length).toBe(2); }); - it('should fetch notifications with failure - github.com only', async () => { - const accounts: AuthState = { - ...mockAccounts, - enterpriseAccounts: [], - }; + describe('should fetch notifications with failures - github.com only', () => { + it('bad credentials', async () => { + const status = 401; + const message = 'Bad credentials'; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; - nock('https://api.github.com/') - .get('/notifications?participating=false') - .reply(400, { message: 'Oops! Something went wrong.' }); + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications(false)); - act(() => { - result.current.fetchNotifications(accounts, mockSettings); + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('BAD_CREDENTIALS'); + }); }); - await waitFor(() => { - expect(result.current.requestFailed).toBe(true); + it('missing scopes', async () => { + const status = 403; + const message = "Missing the 'notifications' scope"; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('MISSING_SCOPES'); + }); + }); + + it('rate limited - primary', async () => { + const status = 403; + const message = 'API rate limit exceeded'; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('RATE_LIMITED'); + }); + }); + + it('rate limited - secondary', async () => { + const status = 403; + const message = 'You have exceeded a secondary rate limit'; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('RATE_LIMITED'); + }); + }); + + it('default error', async () => { + const status = 400; + const message = 'Oops! Something went wrong.'; + const accounts: AuthState = { + ...mockAccounts, + enterpriseAccounts: [], + }; + + nock('https://api.github.com/') + .get('/notifications?participating=false') + .reply(status, { message }); + + const { result } = renderHook(() => useNotifications(false)); + + act(() => { + result.current.fetchNotifications(accounts, mockSettings); + }); + + await waitFor(() => { + expect(result.current.requestFailed).toBe(true); + expect(result.current.failureType).toBe('UNKNOWN'); + }); }); }); }); diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index d90bb7699..0f633ac36 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -1,8 +1,13 @@ -import axios from 'axios'; +import axios, { AxiosError } from 'axios'; import { useCallback, useState } from 'react'; -import { AccountNotifications, AuthState, SettingsState } from '../types'; -import { Notification } from '../typesGithub'; +import { + AccountNotifications, + AuthState, + SettingsState, + FailureType, +} from '../types'; +import { GithubRESTError, Notification } from '../typesGithub'; import { apiRequestAuth } from '../utils/api-requests'; import { getEnterpriseAccountToken, @@ -53,11 +58,14 @@ interface NotificationsState { ) => Promise; isFetching: boolean; requestFailed: boolean; + failureType: FailureType; } export const useNotifications = (colors: boolean): NotificationsState => { const [isFetching, setIsFetching] = useState(false); const [requestFailed, setRequestFailed] = useState(false); + const [failureType, setFailureType] = useState(); + const [notifications, setNotifications] = useState( [], ); @@ -87,6 +95,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { } setIsFetching(true); + setFailureType(null); setRequestFailed(false); return axios @@ -194,9 +203,11 @@ export const useNotifications = (colors: boolean): NotificationsState => { }); }), ) - .catch(() => { + .catch((err: AxiosError) => { setIsFetching(false); setRequestFailed(true); + const failureType: FailureType = determineFailureType(err); + setFailureType(failureType); }); }, [notifications], @@ -387,6 +398,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { return { isFetching, requestFailed, + failureType, notifications, removeNotificationFromState, @@ -398,3 +410,23 @@ export const useNotifications = (colors: boolean): NotificationsState => { markRepoNotificationsDone, }; }; + +function determineFailureType(err: AxiosError) { + let failureType: FailureType = 'UNKNOWN'; + const status = err.response.status; + const message = err.response.data.message; + + if (status === 401) { + failureType = 'BAD_CREDENTIALS'; + } else if (status === 403) { + if (message.includes("Missing the 'notifications' scope")) { + failureType = 'MISSING_SCOPES'; + } else if ( + message.includes('API rate limit exceeded') || + message.includes('You have exceeded a secondary rate limit') + ) { + failureType = 'RATE_LIMITED'; + } + } + return failureType; +} diff --git a/src/routes/Notifications.test.tsx b/src/routes/Notifications.test.tsx index 2ad5db7e1..7d79dea1d 100644 --- a/src/routes/Notifications.test.tsx +++ b/src/routes/Notifications.test.tsx @@ -14,8 +14,8 @@ jest.mock('../components/AllRead', () => ({ AllRead: 'AllRead', })); -jest.mock('../components/Oops', () => ({ - Oops: 'Oops', +jest.mock('../components/Error', () => ({ + Error: 'Error', })); describe('routes/Notifications.tsx', () => { @@ -54,13 +54,69 @@ describe('routes/Notifications.tsx', () => { expect(tree).toMatchSnapshot(); }); - it('should render itself & its children (error page - oops)', () => { - const tree = TestRenderer.create( - - - , - ); + describe('should render itself & its children (error conditions - oops)', () => { + it('bad credentials', () => { + const tree = TestRenderer.create( + + + , + ); - expect(tree).toMatchSnapshot(); + expect(tree).toMatchSnapshot(); + }); + + it('missing scopes', () => { + const tree = TestRenderer.create( + + + , + ); + + expect(tree).toMatchSnapshot(); + }); + + it('rate limited', () => { + const tree = TestRenderer.create( + + + , + ); + + expect(tree).toMatchSnapshot(); + }); + + it('default error', () => { + const tree = TestRenderer.create( + + + , + ); + + expect(tree).toMatchSnapshot(); + }); }); }); diff --git a/src/routes/Notifications.tsx b/src/routes/Notifications.tsx index 75b794ca4..fbcb21324 100644 --- a/src/routes/Notifications.tsx +++ b/src/routes/Notifications.tsx @@ -3,11 +3,13 @@ import React, { useContext, useMemo } from 'react'; import { AccountNotifications } from '../components/AccountNotifications'; import { AllRead } from '../components/AllRead'; import { AppContext } from '../context/App'; -import { Oops } from '../components/Oops'; +import { Error } from '../components/Error'; import { getNotificationCount } from '../utils/notifications'; +import Constants from '../utils/constants'; export const NotificationsRoute: React.FC = (props) => { - const { notifications, requestFailed, settings } = useContext(AppContext); + const { notifications, requestFailed, failureType, settings } = + useContext(AppContext); const hasMultipleAccounts = useMemo( () => notifications.length > 1, @@ -23,7 +25,16 @@ export const NotificationsRoute: React.FC = (props) => { ); if (requestFailed) { - return ; + switch (failureType) { + case 'BAD_CREDENTIALS': + return ; + case 'MISSING_SCOPES': + return ; + case 'RATE_LIMITED': + return ; + default: + return ; + } } if (!hasNotifications) { diff --git a/src/routes/__snapshots__/Notifications.test.tsx.snap b/src/routes/__snapshots__/Notifications.test.tsx.snap index 4c8936550..dd8b1b9a7 100644 --- a/src/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/routes/__snapshots__/Notifications.test.tsx.snap @@ -2,7 +2,65 @@ exports[`routes/Notifications.tsx should render itself & its children (all read notifications) 1`] = ``; -exports[`routes/Notifications.tsx should render itself & its children (error page - oops) 1`] = ``; +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) bad credentials 1`] = ` + +`; + +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) default error 1`] = ` + +`; + +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) missing scopes 1`] = ` + +`; + +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) rate limited 1`] = ` + +`; exports[`routes/Notifications.tsx should render itself & its children (show account hostname) 1`] = `
{ - const response: Issue | IssueComments | PullRequest = ( - await apiRequestAuth(url, 'GET', token) - ).data; - - return response.html_url; + try { + const response: Issue | IssueComments | PullRequest = ( + await apiRequestAuth(url, 'GET', token) + ).data; + return response.html_url; + } catch (err) { + console.error('Failed to get html url'); + } } export function getCheckSuiteUrl(notification: Notification) { diff --git a/src/utils/subject.ts b/src/utils/subject.ts index d28dd1c61..7a585699d 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -137,49 +137,57 @@ async function getGitifySubjectForIssue( notification: Notification, token: string, ): Promise { - const issue: Issue = ( - await apiRequestAuth(notification.subject.url, 'GET', token) - ).data; + try { + const issue: Issue = ( + await apiRequestAuth(notification.subject.url, 'GET', token) + ).data; - const issueCommentUser = await getLatestCommentUser(notification, token); + const issueCommentUser = await getLatestCommentUser(notification, token); - return { - state: issue.state_reason ?? issue.state, - user: { - login: issueCommentUser?.login ?? issue.user.login, - html_url: issueCommentUser?.html_url ?? issue.user.html_url, - avatar_url: issueCommentUser?.avatar_url ?? issue.user.avatar_url, - type: issueCommentUser?.type ?? issue.user.type, - }, - }; + return { + state: issue.state_reason ?? issue.state, + user: { + login: issueCommentUser?.login ?? issue.user.login, + html_url: issueCommentUser?.html_url ?? issue.user.html_url, + avatar_url: issueCommentUser?.avatar_url ?? issue.user.avatar_url, + type: issueCommentUser?.type ?? issue.user.type, + }, + }; + } catch (err) { + console.error('Issue subject retrieval failed'); + } } async function getGitifySubjectForPullRequest( notification: Notification, token: string, ): Promise { - const pr: PullRequest = ( - await apiRequestAuth(notification.subject.url, 'GET', token) - ).data; - - let prState: PullRequestStateType = pr.state; - if (pr.merged) { - prState = 'merged'; - } else if (pr.draft) { - prState = 'draft'; - } + try { + const pr: PullRequest = ( + await apiRequestAuth(notification.subject.url, 'GET', token) + ).data; + + let prState: PullRequestStateType = pr.state; + if (pr.merged) { + prState = 'merged'; + } else if (pr.draft) { + prState = 'draft'; + } - const prCommentUser = await getLatestCommentUser(notification, token); + const prCommentUser = await getLatestCommentUser(notification, token); - return { - state: prState, - user: { - login: prCommentUser?.login ?? pr.user.login, - html_url: prCommentUser?.html_url ?? pr.user.html_url, - avatar_url: prCommentUser?.avatar_url ?? pr.user.avatar_url, - type: prCommentUser?.type ?? pr.user.type, - }, - }; + return { + state: prState, + user: { + login: prCommentUser?.login ?? pr.user.login, + html_url: prCommentUser?.html_url ?? pr.user.html_url, + avatar_url: prCommentUser?.avatar_url ?? pr.user.avatar_url, + type: prCommentUser?.type ?? pr.user.type, + }, + }; + } catch (err) { + console.error('Pull Request subject retrieval failed'); + } } async function getGitifySubjectForRelease( @@ -250,11 +258,19 @@ async function getLatestCommentUser( return null; } - const response: IssueComments | ReleaseComments = ( - await apiRequestAuth(notification.subject.latest_comment_url, 'GET', token) - )?.data; - - return ( - (response as IssueComments)?.user ?? (response as ReleaseComments).author - ); + try { + const response: IssueComments | ReleaseComments = ( + await apiRequestAuth( + notification.subject.latest_comment_url, + 'GET', + token, + ) + )?.data; + + return ( + (response as IssueComments)?.user ?? (response as ReleaseComments).author + ); + } catch (err) { + console.error('Discussion latest comment retrieval failed'); + } } From 09c2510061044464b6442a22432496a84350f034 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 7 Apr 2024 11:32:49 -0400 Subject: [PATCH 14/14] feat: detailed error state handling --- src/context/App.tsx | 8 ++-- src/hooks/useNotifications.test.ts | 19 ++++---- src/hooks/useNotifications.ts | 30 ++++++------- src/routes/Notifications.test.tsx | 25 +++++++++-- src/routes/Notifications.tsx | 15 ++----- .../__snapshots__/Notifications.test.tsx.snap | 18 ++++++++ src/types.ts | 4 +- src/utils/constants.ts | 44 ++++++++++--------- 8 files changed, 96 insertions(+), 67 deletions(-) diff --git a/src/context/App.tsx b/src/context/App.tsx index 6f94a5a5c..acc5e6ddf 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -15,7 +15,7 @@ import { AuthState, AuthTokenOptions, SettingsState, - FailureType, + GitifyError, } from '../types'; import { apiRequestAuth } from '../utils/api-requests'; import { setTheme } from '../utils/theme'; @@ -56,7 +56,7 @@ interface AppContextState { notifications: AccountNotifications[]; isFetching: boolean; requestFailed: boolean; - failureType: FailureType; + errorDetails: GitifyError; removeNotificationFromState: (id: string, hostname: string) => void; fetchNotifications: () => Promise; markNotificationRead: (id: string, hostname: string) => Promise; @@ -78,7 +78,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { fetchNotifications, notifications, requestFailed, - failureType, + errorDetails, isFetching, removeNotificationFromState, markNotificationRead, @@ -237,7 +237,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { notifications, isFetching, requestFailed, - failureType, + errorDetails, removeNotificationFromState, fetchNotifications: fetchNotificationsWithAccounts, markNotificationRead: markNotificationReadWithAccounts, diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index 13d94e9a8..5ba55f6a1 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -6,6 +6,7 @@ import { mockAccounts, mockSettings } from '../__mocks__/mock-state'; import { mockedNotificationUser, mockedUser } from '../__mocks__/mockedData'; import { AuthState } from '../types'; import { useNotifications } from './useNotifications'; +import { Errors } from '../utils/constants'; describe('hooks/useNotifications.ts', () => { beforeEach(() => { @@ -69,7 +70,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('BAD_CREDENTIALS'); + expect(result.current.errorDetails).toBe(Errors.BAD_CREDENTIALS); }); }); @@ -93,7 +94,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('MISSING_SCOPES'); + expect(result.current.errorDetails).toBe(Errors.MISSING_SCOPES); }); }); @@ -117,7 +118,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('RATE_LIMITED'); + expect(result.current.errorDetails).toBe(Errors.RATE_LIMITED); }); }); @@ -141,7 +142,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('RATE_LIMITED'); + expect(result.current.errorDetails).toBe(Errors.RATE_LIMITED); }); }); @@ -278,7 +279,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('BAD_CREDENTIALS'); + expect(result.current.errorDetails).toBe(Errors.BAD_CREDENTIALS); }); }); @@ -302,7 +303,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('MISSING_SCOPES'); + expect(result.current.errorDetails).toBe(Errors.MISSING_SCOPES); }); }); @@ -326,7 +327,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('RATE_LIMITED'); + expect(result.current.errorDetails).toBe(Errors.RATE_LIMITED); }); }); @@ -350,7 +351,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('RATE_LIMITED'); + expect(result.current.errorDetails).toBe(Errors.RATE_LIMITED); }); }); @@ -374,7 +375,7 @@ describe('hooks/useNotifications.ts', () => { await waitFor(() => { expect(result.current.requestFailed).toBe(true); - expect(result.current.failureType).toBe('UNKNOWN'); + expect(result.current.errorDetails).toBe(Errors.UNKNOWN); }); }); }); diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 0f633ac36..f8c7e552b 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -5,7 +5,7 @@ import { AccountNotifications, AuthState, SettingsState, - FailureType, + GitifyError, } from '../types'; import { GithubRESTError, Notification } from '../typesGithub'; import { apiRequestAuth } from '../utils/api-requests'; @@ -20,7 +20,7 @@ import { triggerNativeNotifications, setTrayIconColor, } from '../utils/notifications'; -import Constants from '../utils/constants'; +import Constants, { Errors } from '../utils/constants'; import { removeNotifications } from '../utils/remove-notifications'; import { getGitifySubjectDetails } from '../utils/subject'; @@ -58,13 +58,13 @@ interface NotificationsState { ) => Promise; isFetching: boolean; requestFailed: boolean; - failureType: FailureType; + errorDetails: GitifyError; } export const useNotifications = (colors: boolean): NotificationsState => { const [isFetching, setIsFetching] = useState(false); const [requestFailed, setRequestFailed] = useState(false); - const [failureType, setFailureType] = useState(); + const [errorDetails, setErrorDetails] = useState(); const [notifications, setNotifications] = useState( [], @@ -95,7 +95,6 @@ export const useNotifications = (colors: boolean): NotificationsState => { } setIsFetching(true); - setFailureType(null); setRequestFailed(false); return axios @@ -206,8 +205,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { .catch((err: AxiosError) => { setIsFetching(false); setRequestFailed(true); - const failureType: FailureType = determineFailureType(err); - setFailureType(failureType); + setErrorDetails(determineFailureType(err)); }); }, [notifications], @@ -398,7 +396,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { return { isFetching, requestFailed, - failureType, + errorDetails, notifications, removeNotificationFromState, @@ -411,22 +409,24 @@ export const useNotifications = (colors: boolean): NotificationsState => { }; }; -function determineFailureType(err: AxiosError) { - let failureType: FailureType = 'UNKNOWN'; +function determineFailureType(err: AxiosError): GitifyError { const status = err.response.status; const message = err.response.data.message; if (status === 401) { - failureType = 'BAD_CREDENTIALS'; - } else if (status === 403) { + return Errors.BAD_CREDENTIALS; + } + + if (status === 403) { if (message.includes("Missing the 'notifications' scope")) { - failureType = 'MISSING_SCOPES'; + return Errors.MISSING_SCOPES; } else if ( message.includes('API rate limit exceeded') || message.includes('You have exceeded a secondary rate limit') ) { - failureType = 'RATE_LIMITED'; + return Errors.RATE_LIMITED; } } - return failureType; + + return Errors.UNKNOWN; } diff --git a/src/routes/Notifications.test.tsx b/src/routes/Notifications.test.tsx index 7d79dea1d..a26d3a91a 100644 --- a/src/routes/Notifications.test.tsx +++ b/src/routes/Notifications.test.tsx @@ -5,6 +5,7 @@ import { AppContext } from '../context/App'; import { mockedAccountNotifications } from '../__mocks__/mockedData'; import { NotificationsRoute } from './Notifications'; import { mockSettings } from '../__mocks__/mock-state'; +import { Errors } from '../utils/constants'; jest.mock('../components/AccountNotifications', () => ({ AccountNotifications: 'AccountNotifications', @@ -61,7 +62,7 @@ describe('routes/Notifications.tsx', () => { value={{ notifications: [], requestFailed: true, - failureType: 'BAD_CREDENTIALS', + errorDetails: Errors.BAD_CREDENTIALS, }} > @@ -77,7 +78,7 @@ describe('routes/Notifications.tsx', () => { value={{ notifications: [], requestFailed: true, - failureType: 'MISSING_SCOPES', + errorDetails: Errors.MISSING_SCOPES, }} > @@ -93,7 +94,23 @@ describe('routes/Notifications.tsx', () => { value={{ notifications: [], requestFailed: true, - failureType: 'RATE_LIMITED', + errorDetails: Errors.RATE_LIMITED, + }} + > + + , + ); + + expect(tree).toMatchSnapshot(); + }); + + it('unknown error', () => { + const tree = TestRenderer.create( + @@ -109,7 +126,7 @@ describe('routes/Notifications.tsx', () => { value={{ notifications: [], requestFailed: true, - failureType: 'UNKNOWN', + errorDetails: null, }} > diff --git a/src/routes/Notifications.tsx b/src/routes/Notifications.tsx index fbcb21324..831a4a6ab 100644 --- a/src/routes/Notifications.tsx +++ b/src/routes/Notifications.tsx @@ -5,10 +5,10 @@ import { AllRead } from '../components/AllRead'; import { AppContext } from '../context/App'; import { Error } from '../components/Error'; import { getNotificationCount } from '../utils/notifications'; -import Constants from '../utils/constants'; +import { Errors } from '../utils/constants'; export const NotificationsRoute: React.FC = (props) => { - const { notifications, requestFailed, failureType, settings } = + const { notifications, requestFailed, errorDetails, settings } = useContext(AppContext); const hasMultipleAccounts = useMemo( @@ -25,16 +25,7 @@ export const NotificationsRoute: React.FC = (props) => { ); if (requestFailed) { - switch (failureType) { - case 'BAD_CREDENTIALS': - return ; - case 'MISSING_SCOPES': - return ; - case 'RATE_LIMITED': - return ; - default: - return ; - } + return ; } if (!hasNotifications) { diff --git a/src/routes/__snapshots__/Notifications.test.tsx.snap b/src/routes/__snapshots__/Notifications.test.tsx.snap index dd8b1b9a7..ea9564f19 100644 --- a/src/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/routes/__snapshots__/Notifications.test.tsx.snap @@ -62,6 +62,24 @@ exports[`routes/Notifications.tsx should render itself & its children (error con /> `; +exports[`routes/Notifications.tsx should render itself & its children (error conditions - oops) unknown error 1`] = ` + +`; + exports[`routes/Notifications.tsx should render itself & its children (show account hostname) 1`] = `
= { + BAD_CREDENTIALS: { + title: 'Bad Credentials', + description: 'Your credentials are either invalid or expired.', + emojis: ['🔓'], + }, + MISSING_SCOPES: { + title: 'Missing Scopes', + description: 'Your credentials are missing a required API scope.', + emojis: ['🙃'], + }, + RATE_LIMITED: { + title: 'Rate Limited', + description: 'Please wait a while before trying again.', + emojis: ['😮‍💨'], + }, + UNKNOWN: { + title: 'Oops! Something went wrong', + description: 'Please try again later.', + emojis: ['🤔', '😞', '😤', '😱', '😭'], }, };