From 4edc4bedc9ea3726d7693c22c9ca3396e3ad5165 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Mon, 12 Aug 2024 15:30:33 -0400 Subject: [PATCH] feat: improve error logging (#1455) Signed-off-by: Adam Setch --- src/hooks/useNotifications.test.ts | 10 ++++++ src/hooks/useNotifications.ts | 35 +++++++++++++++------ src/routes/Login.tsx | 2 +- src/routes/LoginWithOAuthApp.tsx | 2 ++ src/routes/LoginWithPersonalAccessToken.tsx | 2 ++ src/utils/api/client.test.ts | 2 +- src/utils/api/client.ts | 9 ++++-- src/utils/notifications.ts | 5 +++ 8 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index 30e6f327a..1b014a122 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -2,6 +2,7 @@ import { act, renderHook, waitFor } from '@testing-library/react'; import axios, { AxiosError } from 'axios'; import nock from 'nock'; +import log from 'electron-log'; import { mockAuth, mockGitHubCloudAccount, @@ -16,10 +17,13 @@ import { Errors } from '../utils/constants'; import { useNotifications } from './useNotifications'; describe('hooks/useNotifications.ts', () => { + const logErrorSpy = jest.spyOn(log, 'error').mockImplementation(); + beforeEach(() => { // axios will default to using the XHR adapter which can't be intercepted // by nock. So, configure axios to use the node adapter. axios.defaults.adapter = 'http'; + logErrorSpy.mockReset(); }); const id = mockSingleNotification.id; @@ -294,6 +298,7 @@ describe('hooks/useNotifications.ts', () => { }); expect(result.current.globalError).toBe(Errors.BAD_CREDENTIALS); + expect(logErrorSpy).toHaveBeenCalledTimes(2); }); it('should fetch notifications with different failures', async () => { @@ -336,6 +341,7 @@ describe('hooks/useNotifications.ts', () => { }); expect(result.current.globalError).toBeNull(); + expect(logErrorSpy).toHaveBeenCalledTimes(2); }); }); @@ -374,6 +380,7 @@ describe('hooks/useNotifications.ts', () => { }); expect(result.current.notifications.length).toBe(0); + expect(logErrorSpy).toHaveBeenCalledTimes(1); }); }); @@ -412,6 +419,7 @@ describe('hooks/useNotifications.ts', () => { }); expect(result.current.notifications.length).toBe(0); + expect(logErrorSpy).toHaveBeenCalledTimes(1); }); }); @@ -470,6 +478,7 @@ describe('hooks/useNotifications.ts', () => { }); expect(result.current.notifications.length).toBe(0); + expect(logErrorSpy).toHaveBeenCalledTimes(1); }); }); @@ -516,6 +525,7 @@ describe('hooks/useNotifications.ts', () => { }); expect(result.current.notifications.length).toBe(0); + expect(logErrorSpy).toHaveBeenCalledTimes(1); }); }); diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index a6d392938..31ff7d203 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -1,3 +1,4 @@ +import log from 'electron-log'; import { useCallback, useState } from 'react'; import type { AccountNotifications, @@ -109,10 +110,11 @@ export const useNotifications = (): NotificationsState => { setNotifications(updatedNotifications); setTrayIconColor(updatedNotifications); - setStatus('success'); } catch (err) { - setStatus('success'); + log.error('Error occurred while marking notification as read', err); } + + setStatus('success'); }, [notifications], ); @@ -138,10 +140,11 @@ export const useNotifications = (): NotificationsState => { setNotifications(updatedNotifications); setTrayIconColor(updatedNotifications); - setStatus('success'); } catch (err) { - setStatus('success'); + log.error('Error occurred while marking notification as done', err); } + + setStatus('success'); }, [notifications], ); @@ -157,10 +160,14 @@ export const useNotifications = (): NotificationsState => { notification.account.token, ); await markNotificationRead(state, notification); - setStatus('success'); } catch (err) { - setStatus('success'); + log.error( + 'Error occurred while unsubscribing from notification thread', + err, + ); } + + setStatus('success'); }, [markNotificationRead], ); @@ -187,10 +194,14 @@ export const useNotifications = (): NotificationsState => { setNotifications(updatedNotifications); setTrayIconColor(updatedNotifications); - setStatus('success'); } catch (err) { - setStatus('success'); + log.error( + 'Error occurred while marking repository notifications as read', + err, + ); } + + setStatus('success'); }, [notifications], ); @@ -230,10 +241,14 @@ export const useNotifications = (): NotificationsState => { setNotifications(updatedNotifications); setTrayIconColor(updatedNotifications); - setStatus('success'); } catch (err) { - setStatus('success'); + log.error( + 'Error occurred while marking repository notifications as done', + err, + ); } + + setStatus('success'); }, [notifications, markNotificationDone], ); diff --git a/src/routes/Login.tsx b/src/routes/Login.tsx index 6ac1f8793..325c019d9 100644 --- a/src/routes/Login.tsx +++ b/src/routes/Login.tsx @@ -23,7 +23,7 @@ export const LoginRoute: FC = () => { try { await login(); } catch (err) { - // Skip + log.error('Auth: failed to login with GitHub', err); } }, []); */ diff --git a/src/routes/LoginWithOAuthApp.tsx b/src/routes/LoginWithOAuthApp.tsx index 9d0e7df3e..f1dd0cd5f 100644 --- a/src/routes/LoginWithOAuthApp.tsx +++ b/src/routes/LoginWithOAuthApp.tsx @@ -1,4 +1,5 @@ import { BookIcon, PersonIcon, SignInIcon } from '@primer/octicons-react'; +import log from 'electron-log'; import { type FC, useCallback, useContext } from 'react'; import { Form, type FormRenderProps } from 'react-final-form'; import { Header } from '../components/Header'; @@ -120,6 +121,7 @@ export const LoginWithOAuthApp: FC = () => { try { await loginWithOAuthApp(data as LoginOAuthAppOptions); } catch (err) { + log.error('Auth: Failed to login with oauth app', err); // Skip } }, diff --git a/src/routes/LoginWithPersonalAccessToken.tsx b/src/routes/LoginWithPersonalAccessToken.tsx index e792ef12b..e52ac3f73 100644 --- a/src/routes/LoginWithPersonalAccessToken.tsx +++ b/src/routes/LoginWithPersonalAccessToken.tsx @@ -1,4 +1,5 @@ import { BookIcon, KeyIcon, SignInIcon } from '@primer/octicons-react'; +import log from 'electron-log'; import { type FC, useCallback, useContext, useState } from 'react'; import { Form, type FormRenderProps } from 'react-final-form'; import { useNavigate } from 'react-router-dom'; @@ -125,6 +126,7 @@ export const LoginWithPersonalAccessToken: FC = () => { ); navigate(-1); } catch (err) { + log.error('Auth: failed to login with personal access token', err); setIsValidToken(false); } }, diff --git a/src/utils/api/client.test.ts b/src/utils/api/client.test.ts index edf0b0325..7f6ac0c7a 100644 --- a/src/utils/api/client.test.ts +++ b/src/utils/api/client.test.ts @@ -292,7 +292,7 @@ describe('utils/api/client.ts', () => { '123' as Token, ); - expect(logErrorSpy).toHaveBeenCalledWith('Failed to get html url'); + expect(logErrorSpy).toHaveBeenCalledTimes(1); }); }); }); diff --git a/src/utils/api/client.ts b/src/utils/api/client.ts index d3b358376..a2f133e89 100644 --- a/src/utils/api/client.ts +++ b/src/utils/api/client.ts @@ -226,7 +226,7 @@ export async function getHtmlUrl(url: Link, token: Token): Promise { const response = (await apiRequestAuth(url, 'GET', token)).data; return response.html_url; } catch (err) { - log.error('Failed to get html url'); + log.error('Error occurred while fetching notification html url', err); } } @@ -272,5 +272,10 @@ export async function getLatestDiscussion( (discussion) => discussion.title === notification.subject.title, )[0] ?? null ); - } catch (err) {} + } catch (err) { + log.error( + 'Error occurred while fetching notification latest discussion', + err, + ); + } } diff --git a/src/utils/notifications.ts b/src/utils/notifications.ts index 35ad9d2a5..0a2dc618f 100644 --- a/src/utils/notifications.ts +++ b/src/utils/notifications.ts @@ -1,3 +1,4 @@ +import log from 'electron-log'; import type { AccountNotifications, GitifyState, @@ -148,6 +149,10 @@ export async function getAllNotifications( error: null, }; } catch (error) { + log.error( + 'Error occurred while fetching account notifications', + error, + ); return { account: accountNotifications.account, notifications: [],