diff --git a/src/layouts/shared/NetworkErrorBoundary/NetworkErrorBoundary.jsx b/src/layouts/shared/NetworkErrorBoundary/NetworkErrorBoundary.jsx index f4ab7a4eaf..dfc4c609e7 100644 --- a/src/layouts/shared/NetworkErrorBoundary/NetworkErrorBoundary.jsx +++ b/src/layouts/shared/NetworkErrorBoundary/NetworkErrorBoundary.jsx @@ -1,4 +1,3 @@ -import * as Sentry from '@sentry/react' import { useQueryClient } from '@tanstack/react-query' import cs from 'classnames' import PropTypes from 'prop-types' @@ -171,21 +170,6 @@ class NetworkErrorBoundary extends Component { // if the error is not a network error, we don't do anything and // another error boundary will take it from there if (Object.keys(errorToUI).includes(String(error.status))) { - // only capture network errors if they are not a rate limit error - // this will typically only be schema parsing errors - if (error.status !== 429 && error.dev && error.error) { - Sentry.captureMessage('Network Error', { - // group all network errors together based off of their dev message - fingerprint: error.dev, - // add a breadcrumb for with the message and error - addBreadcrumb: { - category: 'network-error', - message: error.dev, - level: 'error', - data: error.error, - }, - }) - } return { hasNetworkError: true, error } } diff --git a/src/layouts/shared/NetworkErrorBoundary/NetworkErrorBoundary.test.jsx b/src/layouts/shared/NetworkErrorBoundary/NetworkErrorBoundary.test.jsx index e2943bfa48..5bb2d0b16a 100644 --- a/src/layouts/shared/NetworkErrorBoundary/NetworkErrorBoundary.test.jsx +++ b/src/layouts/shared/NetworkErrorBoundary/NetworkErrorBoundary.test.jsx @@ -13,18 +13,6 @@ import NetworkErrorBoundary from './NetworkErrorBoundary' vi.spyOn(console, 'error').mockImplementation(() => undefined) vi.mock('config') -const mocks = vi.hoisted(() => ({ - captureMessage: vi.fn(), -})) - -vi.mock('@sentry/react', async () => { - const actual = await vi.importActual('@sentry/react') - return { - ...actual, - captureMessage: mocks.captureMessage, - } -}) - const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } }, }) @@ -194,34 +182,6 @@ describe('NetworkErrorBoundary', () => { const returnButton = await screen.findByText('Return to previous page') expect(returnButton).toBeInTheDocument() }) - - it('calls captureMessage with the error when dev and error are provided', async () => { - const { user } = setup() - render( - , - { wrapper: wrapper() } - ) - - const textBox = await screen.findByRole('textbox') - await user.type(textBox, 'fail') - - await waitFor(() => - expect(mocks.captureMessage).toHaveBeenCalledWith('Network Error', { - fingerprint: '401 - cool error', - addBreadcrumb: { - category: 'network-error', - data: 'cool error', - level: 'error', - message: '401 - cool error', - }, - }) - ) - }) }) describe('when the children component has a 403 error', () => { @@ -263,34 +223,6 @@ describe('NetworkErrorBoundary', () => { const button = await screen.findByText('Return to previous page') expect(button).toBeInTheDocument() }) - - it('calls captureMessage with the error when dev and error are provided', async () => { - const { user } = setup() - render( - , - { wrapper: wrapper() } - ) - - const textBox = await screen.findByRole('textbox') - await user.type(textBox, 'fail') - - await waitFor(() => - expect(mocks.captureMessage).toHaveBeenCalledWith('Network Error', { - fingerprint: '403 - cool error', - addBreadcrumb: { - category: 'network-error', - data: 'cool error', - level: 'error', - message: '403 - cool error', - }, - }) - ) - }) }) describe('when the children component has a 404 error', () => { @@ -320,34 +252,6 @@ describe('NetworkErrorBoundary', () => { const button = await screen.findByText('Return to previous page') expect(button).toBeInTheDocument() }) - - it('calls captureMessage with the error when dev and error are provided', async () => { - const { user } = setup() - render( - , - { wrapper: wrapper() } - ) - - const textBox = await screen.findByRole('textbox') - await user.type(textBox, 'fail') - - await waitFor(() => - expect(mocks.captureMessage).toHaveBeenCalledWith('Network Error', { - fingerprint: '404 - cool error', - addBreadcrumb: { - category: 'network-error', - data: 'cool error', - level: 'error', - message: '404 - cool error', - }, - }) - ) - }) }) describe('when running in self hosted mode', () => { @@ -376,34 +280,6 @@ describe('NetworkErrorBoundary', () => { const button = await screen.findByText('Return to previous page') expect(button).toBeInTheDocument() }) - - it('calls captureMessage with the error when dev and error are provided', async () => { - const { user } = setup({ isSelfHosted: true }) - render( - , - { wrapper: wrapper() } - ) - - const textBox = await screen.findByRole('textbox') - await user.type(textBox, 'fail') - - await waitFor(() => - expect(mocks.captureMessage).toHaveBeenCalledWith('Network Error', { - fingerprint: '404 - cool error', - addBreadcrumb: { - category: 'network-error', - data: 'cool error', - level: 'error', - message: '404 - cool error', - }, - }) - ) - }) }) }) @@ -454,24 +330,6 @@ describe('NetworkErrorBoundary', () => { // Clean up the mock global.fetch.mockRestore() }) - - it('does not call captureMessage ', async () => { - const { user } = setup() - render( - , - { wrapper: wrapper() } - ) - - const textBox = await screen.findByRole('textbox') - await user.type(textBox, 'fail') - - await waitFor(() => expect(mocks.captureMessage).not.toHaveBeenCalled()) - }) }) describe('when the children component has a 500 error', () => { @@ -501,34 +359,6 @@ describe('NetworkErrorBoundary', () => { const button = await screen.findByText('Return to previous page') expect(button).toBeInTheDocument() }) - - it('calls captureMessage with the error when dev and error are provided', async () => { - const { user } = setup() - render( - , - { wrapper: wrapper() } - ) - - const textBox = await screen.findByRole('textbox') - await user.type(textBox, 'fail') - - await waitFor(() => - expect(mocks.captureMessage).toHaveBeenCalledWith('Network Error', { - fingerprint: '500 - cool error', - addBreadcrumb: { - category: 'network-error', - data: 'cool error', - level: 'error', - message: '500 - cool error', - }, - }) - ) - }) }) describe('when running in self-hosted mode', () => { @@ -557,34 +387,6 @@ describe('NetworkErrorBoundary', () => { const button = await screen.findByText('Return to previous page') expect(button).toBeInTheDocument() }) - - it('calls captureMessage with the error when dev and error are provided', async () => { - const { user } = setup({ isSelfHosted: true }) - render( - , - { wrapper: wrapper() } - ) - - const textBox = await screen.findByRole('textbox') - await user.type(textBox, 'fail') - - await waitFor(() => - expect(mocks.captureMessage).toHaveBeenCalledWith('Network Error', { - fingerprint: '500 - cool error', - addBreadcrumb: { - category: 'network-error', - data: 'cool error', - level: 'error', - message: '500 - cool error', - }, - }) - ) - }) }) }) diff --git a/src/services/bundleAnalysis/useBundleSummary.tsx b/src/services/bundleAnalysis/useBundleSummary.tsx index b73d8a0063..f630e013a1 100644 --- a/src/services/bundleAnalysis/useBundleSummary.tsx +++ b/src/services/bundleAnalysis/useBundleSummary.tsx @@ -8,7 +8,7 @@ import { useRepoOverview, } from 'services/repo' import Api from 'shared/api/api' -import type { NetworkErrorObject } from 'shared/api/helpers' +import { NetworkErrorObject, rejectNetworkError } from 'shared/api/helpers' import A from 'ui/A' const BundleDataSchema = z.object({ @@ -167,12 +167,12 @@ export const useBundleSummary = ({ const parsedData = RequestSchema.safeParse(res?.data) if (!parsedData.success) { - return Promise.reject({ + return rejectNetworkError({ status: 404, data: {}, dev: 'useBundleSummary - 404 Failed to parse data', error: parsedData.error, - } satisfies NetworkErrorObject) + }) } const data = parsedData.data diff --git a/src/services/commit/useCommit.test.tsx b/src/services/commit/useCommit.test.tsx index c5f5ebca01..59683d6b7d 100644 --- a/src/services/commit/useCommit.test.tsx +++ b/src/services/commit/useCommit.test.tsx @@ -306,7 +306,7 @@ describe('useCommit', () => { }), graphql.query(`CompareTotals`, (info) => { if (skipPolling) { - return HttpResponse.json({ data: {} }) + return HttpResponse.json({ data: { owner: null } }) } return HttpResponse.json({ data: compareDoneData }) }) diff --git a/src/services/commit/useCommit.tsx b/src/services/commit/useCommit.tsx index 51dfc77d57..51ebbd3c4f 100644 --- a/src/services/commit/useCommit.tsx +++ b/src/services/commit/useCommit.tsx @@ -18,7 +18,7 @@ import { RepoOwnerNotActivatedErrorSchema, } from 'services/repo/schemas' import Api from 'shared/api' -import { NetworkErrorObject } from 'shared/api/helpers' +import { rejectNetworkError } from 'shared/api/helpers' import { ErrorCodeEnum, UploadStateEnum, @@ -356,25 +356,26 @@ export function useCommit({ const parsedRes = RequestSchema.safeParse(res?.data) if (!parsedRes.success) { - return Promise.reject({ + return rejectNetworkError({ status: 404, data: {}, dev: 'useCommit - 404 failed to parse', - } satisfies NetworkErrorObject) + error: parsedRes.error, + }) } const data = parsedRes.data if (data?.owner?.repository?.__typename === 'NotFoundError') { - return Promise.reject({ + return rejectNetworkError({ status: 404, data: {}, dev: 'useCommit - 404 not found', - } satisfies NetworkErrorObject) + }) } if (data?.owner?.repository?.__typename === 'OwnerNotActivatedError') { - return Promise.reject({ + return rejectNetworkError({ status: 403, data: { detail: ( @@ -387,7 +388,7 @@ export function useCommit({ ), }, dev: 'useCommit - 403 owner not activated', - } satisfies NetworkErrorObject) + }) } const commit = data?.owner?.repository?.commit diff --git a/src/shared/api/helpers.test.ts b/src/shared/api/helpers.test.ts index 35d8c0f638..bf8097cf3c 100644 --- a/src/shared/api/helpers.test.ts +++ b/src/shared/api/helpers.test.ts @@ -2,7 +2,32 @@ import Cookie from 'js-cookie' import config from 'config' -import { generatePath, getHeaders } from './helpers' +import { generatePath, getHeaders, rejectNetworkError } from './helpers' + +const mocks = vi.hoisted(() => ({ + withScope: vi.fn(), + addBreadcrumb: vi.fn(), + captureMessage: vi.fn(), + setFingerprint: vi.fn(), +})) + +vi.mock('@sentry/react', async () => { + const actual = await vi.importActual('@sentry/react') + return { + ...actual, + withScope: mocks.withScope.mockImplementation((fn) => + fn({ + addBreadcrumb: mocks.addBreadcrumb, + setFingerprint: mocks.setFingerprint, + captureMessage: mocks.captureMessage, + }) + ), + } +}) + +afterEach(() => { + vi.clearAllMocks() +}) describe('generatePath', () => { it('generates a path without a query', () => { @@ -77,3 +102,61 @@ describe('getHeaders', () => { }) }) }) + +describe('rejectNetworkError', () => { + describe('when the error has a dev message and error', () => { + it('adds a breadcrumb', () => { + rejectNetworkError({ + status: 404, + data: {}, + dev: 'useCoolHook - 404 not found', + error: Error('not found'), + }).catch((e) => {}) + + expect(mocks.addBreadcrumb).toHaveBeenCalledWith({ + category: 'network.error', + level: 'error', + message: 'useCoolHook - 404 not found', + data: Error('not found'), + }) + }) + + it('sets the fingerprint', () => { + rejectNetworkError({ + status: 404, + data: {}, + dev: 'useCoolHook - 404 not found', + error: Error('not found'), + }).catch((e) => {}) + + expect(mocks.setFingerprint).toHaveBeenCalledWith([ + 'useCoolHook - 404 not found', + ]) + }) + + it('captures the error with Sentry', () => { + rejectNetworkError({ + status: 404, + data: {}, + dev: 'useCoolHook - 404 not found', + error: Error('not found'), + }).catch((e) => {}) + + expect(mocks.captureMessage).toHaveBeenCalledWith('Network Error') + }) + }) + + describe('when the error does not have an error', () => { + it('does not call any Sentry methods', () => { + rejectNetworkError({ + status: 404, + data: {}, + dev: 'useCoolHook - 404 not found', + }).catch((e) => {}) + + expect(mocks.addBreadcrumb).not.toHaveBeenCalled() + expect(mocks.setFingerprint).not.toHaveBeenCalled() + expect(mocks.captureMessage).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/shared/api/helpers.ts b/src/shared/api/helpers.ts index 76d2177c4a..692e810c22 100644 --- a/src/shared/api/helpers.ts +++ b/src/shared/api/helpers.ts @@ -1,4 +1,5 @@ /* eslint-disable camelcase */ +import * as Sentry from '@sentry/react' import qs from 'qs' import config from 'config' @@ -14,6 +15,29 @@ export interface NetworkErrorObject { error?: Error } +export function rejectNetworkError(error: NetworkErrorObject) { + // only capture network errors if they are not a rate limit error + // this will typically only be schema parsing errors + if (error.status !== 429 && error.dev && error.error) { + Sentry.withScope((scope) => { + scope.addBreadcrumb({ + category: 'network.error', + level: 'error', + message: error.dev, + data: error.error, + }) + scope.setFingerprint([error.dev]) + scope.captureMessage('Network Error') + }) + } + + return Promise.reject({ + status: error.status, + dev: error.dev, + data: error.data, + }) +} + export const AllProvidersArray = [ 'gh', 'gl', diff --git a/src/vitest.setup.ts b/src/vitest.setup.ts index 196161d0a2..e20d3ff5b7 100644 --- a/src/vitest.setup.ts +++ b/src/vitest.setup.ts @@ -17,6 +17,7 @@ vi.mock('@sentry/react', async () => { return { ...originalModule, setUser: vi.fn(), + withScope: vi.fn(), metrics: { ...originalModule.metrics!, distribution: vi.fn(),