From 4dc00d07dc61c9aef8b9fb1053f5310de10faff0 Mon Sep 17 00:00:00 2001 From: Daniel Frett Date: Wed, 7 Oct 2026 13:54:56 -0600 Subject: [PATCH 1/3] Stop the error link from hanging queries on non-array errors Apollo passes a failed response's `errors` to the error link unchecked. When it was an object or string, `graphQLErrors.forEach` threw inside Apollo's error callback, so the error never reached the query and pages like the Staff Expense Report stayed on their loading skeletons. The link now only walks `graphQLErrors` when it is an array; network errors are still toasted and reported. The Staff Expense Report also treats a failed Hcm query as a load error: it shows the existing alert instead of loading the report with the wrong fund types and no salary split, and Try Again refetches Hcm. Co-Authored-By: Claude Opus 5.5 --- .../StaffExpenseReport.test.tsx | 381 ++++++++++-------- .../StaffExpenseReport/StaffExpenseReport.tsx | 23 +- src/lib/apollo/client.test.ts | 64 +++ src/lib/apollo/client.ts | 5 +- 4 files changed, 297 insertions(+), 176 deletions(-) create mode 100644 src/lib/apollo/client.test.ts diff --git a/src/components/Reports/StaffExpenseReport/StaffExpenseReport.test.tsx b/src/components/Reports/StaffExpenseReport/StaffExpenseReport.test.tsx index 81805e72f..2fe9bb761 100644 --- a/src/components/Reports/StaffExpenseReport/StaffExpenseReport.test.tsx +++ b/src/components/Reports/StaffExpenseReport/StaffExpenseReport.test.tsx @@ -36,7 +36,8 @@ interface TestComponentProps { staffAccountId?: string; staffName?: string; personNumber?: string; - failReportCall?: FailReportCall; + failReportCall?: FailCall; + failHcmCall?: FailCall; } const salaryCategory = { @@ -87,32 +88,36 @@ const router = { push, }; -let reportCalls = 0; +const calls = { report: 0, hcm: 0 }; beforeEach(() => { - reportCalls = 0; + calls.report = 0; + calls.hcm = 0; }); type ReportMock = DeepPartialMock< ReportsStaffExpensesQuery['reportsStaffExpenses'] >; - -type FailReportCall = (call: number) => boolean; -const everyCall: FailReportCall = () => true; -const firstCall: FailReportCall = (call) => call === 0; -const secondCall: FailReportCall = (call) => call === 1; - -const failReportCallsWhen = ( - failReportCall: FailReportCall | undefined, - report: ReportMock, -): ReportMock => - failReportCall +type HcmMock = DeepPartialMock; + +type FailCall = (call: number) => boolean; +const everyCall: FailCall = () => true; +const firstCall: FailCall = (call) => call === 0; +const secondCall: FailCall = (call) => call === 1; + +const failCallsWhen = ( + failCall: FailCall | undefined, + query: keyof typeof calls, + message: string, + result: Result, +): Result => + failCall ? ((() => { - if (failReportCall(reportCalls++)) { - throw new Error('SAA is unavailable'); + if (failCall(calls[query]++)) { + throw new Error(message); } - return report; - }) as unknown as ReportMock) - : report; + return result; + }) as unknown as Result) + : result; const TestComponent: React.FC = ({ isEmpty, @@ -123,6 +128,7 @@ const TestComponent: React.FC = ({ staffName, personNumber, failReportCall, + failHcmCall, }) => ( = ({ }> mocks={{ ReportsStaffExpenses: { - reportsStaffExpenses: failReportCallsWhen(failReportCall, { - name: staffName ?? 'Test Account', - funds: isEmpty - ? [] - : [ - { - fundType: 'Primary', - total: -500, - startBalance: 1000, - endBalance: 2000, - categories: [ - ...(withSalary ? [salaryCategory] : []), - { - category: StaffExpenseCategoryEnum.Assessment, - total: -300, - averagePerMonth: -100, - subcategories: [ - { - subCategory: - StaffExpensesSubCategoryEnum.CreditCardFee, - total: -200, - averagePerMonth: -50, - breakdownByMonth: [ - { - month: '2020-01-01', - total: -200, - transactions: [ - { - id: 'transaction-1', - amount: -100, - transactedAt: '2020-01-15', - description: 'Star Wars Costume', - }, - { - id: 'transaction-2', - amount: -100, - transactedAt: '2020-01-24', - }, - - { - id: 'transaction-3', - amount: 50, - transactedAt: '2020-02-15', - description: 'Credit Card Fee', - }, - ], - }, - { - month: '2020-02-01', - total: -100, - }, - ], - }, - { - subCategory: - StaffExpensesSubCategoryEnum.CreditCardFee, - total: 150, - averagePerMonth: 75, - breakdownByMonth: [ - { - month: '2020-01-01', - total: 50, - }, - { - month: '2020-02-01', - total: 50, - transactions: [ - { - id: 'transaction-3', - amount: 50, - transactedAt: '2020-02-15', - description: 'Credit Card Fee', - }, - ], - }, - ], - }, - ], - breakdownByMonth: [ - { - month: '2020-01-01', - total: -150, - }, - { - month: '2020-02-01', - total: -150, - }, - ], - }, - ], - }, - { - fundType: 'Savings', - total: -100, - startBalance: 1000, - endBalance: 2000, - categories: [ - { - category: StaffExpenseCategoryEnum.Assessment, - total: -100, - averagePerMonth: -50, - subcategories: [ - { - subCategory: - StaffExpensesSubCategoryEnum.CreditCardFee, - total: -100, - averagePerMonth: -50, - breakdownByMonth: [ - { - month: '2020-01-01', - total: -100, - transactions: [ - { - id: 'transaction-savings-1', - amount: -100, - transactedAt: '2020-01-10', - description: 'Savings Expense', - }, - ], - }, - ], - }, - ], - breakdownByMonth: [ - { - month: '2020-01-01', - total: -100, - }, - ], - }, - ], - }, - ], - }), + reportsStaffExpenses: failCallsWhen( + failReportCall, + 'report', + 'SAA is unavailable', + { + name: staffName ?? 'Test Account', + funds: isEmpty + ? [] + : [ + { + fundType: 'Primary', + total: -500, + startBalance: 1000, + endBalance: 2000, + categories: [ + ...(withSalary ? [salaryCategory] : []), + { + category: StaffExpenseCategoryEnum.Assessment, + total: -300, + averagePerMonth: -100, + subcategories: [ + { + subCategory: + StaffExpensesSubCategoryEnum.CreditCardFee, + total: -200, + averagePerMonth: -50, + breakdownByMonth: [ + { + month: '2020-01-01', + total: -200, + transactions: [ + { + id: 'transaction-1', + amount: -100, + transactedAt: '2020-01-15', + description: 'Star Wars Costume', + }, + { + id: 'transaction-2', + amount: -100, + transactedAt: '2020-01-24', + }, + + { + id: 'transaction-3', + amount: 50, + transactedAt: '2020-02-15', + description: 'Credit Card Fee', + }, + ], + }, + { + month: '2020-02-01', + total: -100, + }, + ], + }, + { + subCategory: + StaffExpensesSubCategoryEnum.CreditCardFee, + total: 150, + averagePerMonth: 75, + breakdownByMonth: [ + { + month: '2020-01-01', + total: 50, + }, + { + month: '2020-02-01', + total: 50, + transactions: [ + { + id: 'transaction-3', + amount: 50, + transactedAt: '2020-02-15', + description: 'Credit Card Fee', + }, + ], + }, + ], + }, + ], + breakdownByMonth: [ + { + month: '2020-01-01', + total: -150, + }, + { + month: '2020-02-01', + total: -150, + }, + ], + }, + ], + }, + { + fundType: 'Savings', + total: -100, + startBalance: 1000, + endBalance: 2000, + categories: [ + { + category: StaffExpenseCategoryEnum.Assessment, + total: -100, + averagePerMonth: -50, + subcategories: [ + { + subCategory: + StaffExpensesSubCategoryEnum.CreditCardFee, + total: -100, + averagePerMonth: -50, + breakdownByMonth: [ + { + month: '2020-01-01', + total: -100, + transactions: [ + { + id: 'transaction-savings-1', + amount: -100, + transactedAt: '2020-01-10', + description: 'Savings Expense', + }, + ], + }, + ], + }, + ], + breakdownByMonth: [ + { + month: '2020-01-01', + total: -100, + }, + ], + }, + ], + }, + ], + }, + ), }, StaffAccount: { staffAccount: { @@ -288,22 +299,27 @@ const TestComponent: React.FC = ({ }, }, Hcm: { - hcm: [ - { - usStaffGroup, - staffInfo: { - personNumber: '000000111', - preferredName: 'Alex', + hcm: failCallsWhen( + failHcmCall, + 'hcm', + 'HCM is unavailable', + [ + { + usStaffGroup, + staffInfo: { + personNumber: '000000111', + preferredName: 'Alex', + }, }, - }, - { - usStaffGroup, - staffInfo: { - personNumber: '000000222', - preferredName: 'Jordan', + { + usStaffGroup, + staffInfo: { + personNumber: '000000222', + preferredName: 'Jordan', + }, }, - }, - ], + ], + ), }, }} onCall={mutationSpy} @@ -494,7 +510,7 @@ describe('StaffExpenseReport', () => { const alert = await findByRole('alert'); userEvent.click(within(alert).getByRole('button', { name: 'Try Again' })); - await waitFor(() => expect(reportCalls).toBe(2)); + await waitFor(() => expect(calls.report).toBe(2)); expect(getByRole('alert')).toBeInTheDocument(); }); @@ -540,6 +556,35 @@ describe('StaffExpenseReport', () => { }); }); + describe('when HCM fails to load', () => { + it('shows the error instead of loading forever', async () => { + const { findByRole, queryByTestId } = render( + , + ); + + expect(await findByRole('alert')).toHaveTextContent( + 'The Staff Expense report could not be loaded. Please try again later.', + ); + expect(queryByTestId('overall-balance')).not.toBeInTheDocument(); + expect(mutationSpy).not.toHaveGraphqlOperation('ReportsStaffExpenses'); + }); + + it('loads HCM and the report when Try Again is clicked', async () => { + const { findByRole, findByText, queryByRole } = render( + , + ); + + const alert = await findByRole('alert'); + userEvent.click(within(alert).getByRole('button', { name: 'Try Again' })); + + expect( + await findByText('Ending Balance (All Accounts): $4,000.00'), + ).toBeInTheDocument(); + expect(queryByRole('alert')).not.toBeInTheDocument(); + expect(calls.hcm).toBe(2); + }); + }); + it('shows a zero overall balance when the report has no funds', async () => { const { findByText } = render(); diff --git a/src/components/Reports/StaffExpenseReport/StaffExpenseReport.tsx b/src/components/Reports/StaffExpenseReport/StaffExpenseReport.tsx index 0cb4cf5ac..559d22bd1 100644 --- a/src/components/Reports/StaffExpenseReport/StaffExpenseReport.tsx +++ b/src/components/Reports/StaffExpenseReport/StaffExpenseReport.tsx @@ -104,7 +104,12 @@ export const StaffExpenseReport: React.FC = ({ // Person numbers tell the reader's payroll from their spouse's. HCM lists the reader first, then // their spouse. Held alongside the report data's own loading so salary is not rendered as one // household total and then split. - const { data: hcmData, loading: hcmLoading } = useHcmQuery({ + const { + data: hcmData, + loading: hcmLoading, + error: hcmError, + refetch: refetchHcm, + } = useHcmQuery({ variables: { personNumber }, skip: isSupervisorView && !personNumber, }); @@ -152,11 +157,15 @@ export const StaffExpenseReport: React.FC = ({ staffAccountId, ...getStaffExpenseMonthRange(filters, time), }, - skip: hcmLoading, + // Without HCM data the fund types and salary split would be wrong + skip: hcmLoading || !!hcmError, }); + const loadError = hcmError ?? reportError; + const refetchReport = () => { - refetch().catch(() => {}); + // The report reloads on its own once HCM succeeds + (hcmError ? refetchHcm() : refetch()).catch(() => {}); }; const { data: accountData } = useStaffAccountQuery({ @@ -410,7 +419,7 @@ export const StaffExpenseReport: React.FC = ({ ) : ( // A supervisor's staff name comes from the failed report, so there is no name to show - !(isSupervisorView && reportError) && ( + !(isSupervisorView && loadError) && ( ) )} @@ -448,7 +457,7 @@ export const StaffExpenseReport: React.FC = ({ - {!reportError && ( + {!loadError && ( = ({ )} - {reportError ? ( + {loadError ? ( // The global Apollo error link already shows the error details in a snackbar = ({ - {selectedFundType && !reportError && ( + {selectedFundType && !loadError && ( ({ + makeAuthLink: () => + new ApolloLink((operation, forward) => forward(operation)), + batchLink: new ApolloLink( + () => new Observable((observer) => observer.error(mockNetworkError)), + ), +})); + +const makeServerError = (result: unknown) => + Object.assign( + new Error('Response not successful: Received status code 500'), + { name: 'ServerError', statusCode: 500, result }, + ); + +describe('makeClient error link', () => { + it.each([ + ['an object', { errors: { title: 'Internal Server Error' } }], + ['a string', { errors: 'Internal Server Error' }], + ])( + 'passes the network error on when the response errors are %s', + async (_, result) => { + mockNetworkError = makeServerError(result); + + await expect( + makeClient('token').query({ + query: HcmDocument, + variables: { personNumber: null }, + }), + ).rejects.toThrow('Response not successful: Received status code 500'); + + expect(snackNotifications.error).toHaveBeenCalledWith( + 'Response not successful: Received status code 500', + ); + expect(reportNetworkError).toHaveBeenCalledWith( + mockNetworkError, + expect.objectContaining({ operationName: 'Hcm' }), + ); + }, + ); + + it('still shows each error when the response errors are an array', async () => { + mockNetworkError = makeServerError({ errors: [{ message: 'HCM failed' }] }); + + await expect( + makeClient('token').query({ + query: HcmDocument, + variables: { personNumber: null }, + }), + ).rejects.toThrow(); + + expect(snackNotifications.error).toHaveBeenCalledWith('HCM failed'); + }); +}); diff --git a/src/lib/apollo/client.ts b/src/lib/apollo/client.ts index 87003d3a8..e88157e90 100644 --- a/src/lib/apollo/client.ts +++ b/src/lib/apollo/client.ts @@ -40,7 +40,10 @@ const makeClient = (apiToken: string) => { onError(({ graphQLErrors, networkError, operation }) => { const suppressContext: SuppressErrorsContext = operation.getContext(); - graphQLErrors?.forEach((graphQLError) => { + // Apollo passes a failed response's `errors` through unchecked, so it + // may not be an array. Throwing here would leave the query loading forever. + const errors = Array.isArray(graphQLErrors) ? graphQLErrors : []; + errors.forEach((graphQLError) => { if (graphQLError?.extensions?.code === 'AUTHENTICATION_ERROR') { signOut({ redirect: true, callbackUrl: 'signOut' }).then(() => { clearDatadogUser(); From f172594f5882953e9bf32cbfc4ab98b8fc418c31 Mon Sep 17 00:00:00 2001 From: Daniel Frett Date: Wed, 7 Oct 2026 15:11:59 -0600 Subject: [PATCH 2/3] chore(review): record finding outcomes Co-Authored-By: Claude Opus 5.5 --- .claude/review/learnings/feedback.jsonl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.claude/review/learnings/feedback.jsonl b/.claude/review/learnings/feedback.jsonl index 0c55e2221..2dca3bfcc 100644 --- a/.claude/review/learnings/feedback.jsonl +++ b/.claude/review/learnings/feedback.jsonl @@ -45,3 +45,6 @@ {"ts":"2026-09-29T15:44:38.863Z","reviewId":"address-2092","id":"f3","signature":"2e44759de6ee","agent":"architecture","category":"pattern-consistency","severity":4,"file":"src/components/HrTools/PdsGoalCalculator/Setup/SetupStep.tsx","message":"The goal year is now passed by hand into three separate PDS calls to useGoalCalculatorConstants (SetupStep, MonthlyReimbursableSection, usePdsSummaryData). GoalCalculator does this differently: its context owns one year-scoped constants result and tells consumers not to call the hook themselves, so a future PDS consumer that calls the hook with no year will quietly get current-year rates.","outcome":"accepted"} {"ts":"2026-09-29T15:47:11.190Z","reviewId":"address-2092","id":"f7","signature":"9caaa653a14a","agent":"financial","category":"date-windows","severity":3,"file":"src/components/HrTools/PdsGoalCalculator/Shared/PdsGoalCalculatorLayout.tsx","message":"The past-year comparison reads DateTime.local().year fresh on every render instead of capturing it once, unlike the repos established now = useMemo(() => DateTime.now(), []) pattern for long-lived pages. On a session left open across a year boundary the past-year banner/chip can appear or disappear mid-session even though nothing about the goal changed.","outcome":"accepted"} {"ts":"2026-10-05T20:43:32.007Z","reviewId":"address-2123","id":"f1","signature":"53773db921b4","agent":"architecture","category":"state-consistency","severity":6,"file":"src/components/HrTools/AdditionalSalaryRequest/Shared/useAdditionalSalaryRequestForm.ts","message":"The new validator guard checks the sticky stableNonBackpayTotal (falls back to lastValidNonBackpayTotalRef), while useSalaryCalculations checks the live nonBackpayTotal, so the two exceedsCap answers split apart. An over-cap staffer who types a non-backpay amount and then clears it back to backpay-only sees \"Optional Comments\" and an enabled Submit, but Formik still fails additionalInfo as required.","outcome":"accepted"} +{"ts":"2026-10-07T21:11:54.597Z","reviewId":"address-2127","id":"f1","signature":"52aed5594f03","agent":"testing","category":"test-cannot-fail","severity":5,"file":"src/components/Reports/StaffExpenseReport/StaffExpenseReport.test.tsx","message":"`expect(calls.report).toBe(0)` can never fail: `calls.report` only increments inside `failCallsWhen`, and this test passes no `failReportCall`, so the counter is 0 even if the report query runs. The test does not prove the report query is skipped when HCM errors.","outcome":"accepted"} +{"ts":"2026-10-07T21:17:39.702Z","reviewId":"address-2127","id":"f2","signature":"5189466fa1ce","agent":"architecture","category":"pattern-consistency","severity":4,"file":"src/lib/apollo/client.ts","message":"The non-array `graphQLErrors` guard is fixed only in the browser error link; the sibling SSR error link in src/lib/apollo/ssrClient.ts still calls `graphQLErrors.map(...)` behind a truthiness check, so a string or object `errors` throws inside the link the same way.","outcome":"accepted"} +{"ts":"2026-10-07T21:42:15.282Z","reviewId":"address-2127","id":"f1","signature":"d423e73efc0c","agent":"architecture","category":"pattern-consistency","severity":4,"file":"src/components/Reports/StaffExpenseReport/StaffExpenseReport.tsx","message":"The new HCM failure path folds every Hcm error into the generic \"Staff Expense report could not be loaded. Please try again later.\" alert, while the repo already has a dedicated convention for the Hcm query failing with HCM_UNAVAILABLE (isHcmUnavailableError + HcmUnavailableAlert). When HCM is overloaded, this page tells users something different from every other HCM-backed page.","outcome":"dismissed","dismissalReason":"intentional","dismissalDetail":"it's no longer possible to get an HCM_UNAVAILABLE error on the Staff Expense Report except in an extreme edge case that isn't worth having special logic for"} From 9b409f11bbd0e6ea36f4f9f2bef1074bcc73f8f9 Mon Sep 17 00:00:00 2001 From: Daniel Frett Date: Wed, 7 Oct 2026 15:16:20 -0600 Subject: [PATCH 3/3] Guard the SSR error link against non-array errors The server-side error link had the same problem as the browser one: Apollo passes a failed response's `errors` through unchecked, and calling `.map` on an object or string threw inside the link, so the query never settled and getServerSideProps hung. Only walk the errors when they are an array; the network error is still sent to Rollbar. Co-Authored-By: Claude Opus 5.5 --- src/lib/apollo/ssrClient.test.ts | 61 ++++++++++++++++++++++++++++++++ src/lib/apollo/ssrClient.ts | 4 +-- 2 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 src/lib/apollo/ssrClient.test.ts diff --git a/src/lib/apollo/ssrClient.test.ts b/src/lib/apollo/ssrClient.test.ts new file mode 100644 index 000000000..1af9b3e64 --- /dev/null +++ b/src/lib/apollo/ssrClient.test.ts @@ -0,0 +1,61 @@ +import { ApolloLink, Observable } from '@apollo/client'; +import rollbar from 'pages/api/utils/rollBar'; +import { HcmDocument } from 'src/components/HrTools/Shared/HcmData/Hcm.generated'; +import makeSsrClient from './ssrClient'; + +jest.mock('pages/api/utils/rollBar', () => ({ + __esModule: true, + default: { error: jest.fn() }, + isRollBarEnabled: true, +})); + +let mockNetworkError: Error; +jest.mock('./link', () => ({ + makeAuthLink: () => + new ApolloLink((operation, forward) => forward(operation)), + batchLink: new ApolloLink( + () => new Observable((observer) => observer.error(mockNetworkError)), + ), +})); + +const makeServerError = (result: unknown) => + Object.assign( + new Error('Response not successful: Received status code 500'), + { name: 'ServerError', statusCode: 500, result }, + ); + +describe('makeSsrClient error link', () => { + it.each([ + ['an object', { errors: { title: 'Internal Server Error' } }], + ['a string', { errors: 'Internal Server Error' }], + ])( + 'passes the network error on when the response errors are %s', + async (_, result) => { + mockNetworkError = makeServerError(result); + + await expect( + makeSsrClient('token').query({ + query: HcmDocument, + variables: { personNumber: null }, + }), + ).rejects.toThrow('Response not successful: Received status code 500'); + + expect(rollbar.error).toHaveBeenCalledWith(mockNetworkError); + }, + ); + + it('still reports each error when the response errors are an array', async () => { + mockNetworkError = makeServerError({ + errors: [{ message: 'HCM failed', extensions: { code: 'X' } }], + }); + + await expect( + makeSsrClient('token').query({ + query: HcmDocument, + variables: { personNumber: null }, + }), + ).rejects.toThrow(); + + expect(rollbar.error).toHaveBeenCalledWith('HCM failed', { code: 'X' }); + }); +}); diff --git a/src/lib/apollo/ssrClient.ts b/src/lib/apollo/ssrClient.ts index 36dcd9599..d3bbac3a4 100644 --- a/src/lib/apollo/ssrClient.ts +++ b/src/lib/apollo/ssrClient.ts @@ -11,8 +11,8 @@ import generatedIntrospection from 'src/graphql/possibleTypes.generated'; import { batchLink, makeAuthLink } from './link'; const serverErrorLink = onError(({ graphQLErrors, networkError }) => { - if (graphQLErrors) { - graphQLErrors.map(({ message, extensions }) => { + if (Array.isArray(graphQLErrors)) { + graphQLErrors.forEach(({ message, extensions }) => { rollbar.error(message, extensions); }); }