From 74b066ac2c4a3123f9cf0784ed43d65108bf35cc Mon Sep 17 00:00:00 2001 From: "Joseph T. French" Date: Sat, 3 Oct 2026 19:07:13 -0500 Subject: [PATCH] fix(close): take the review follow-ups on changed transactions - Refusals go through friendlyError. The two the panel acts on (a change already settled, a change flagged again mid-review) are classes there, with a code the panel reads instead of matching text itself. - A change settled before it could be reviewed drops off the list. - When a full page hid further changes, the list is fetched again after each one is settled, and the note under it says so. - Opening a change clears the last outcome, and a graph switch clears the note, notice and errors too. - The reference links only to a web address. - The sidebar entry is added to the category that holds the reconciliations worklist, whatever it is labelled. - The close blocker leaves the count out when the calendar reports none. --- .../ChangedTransactionsPanel.test.tsx | 81 +++++++++- .../close/__tests__/PeriodClosePanel.test.tsx | 15 ++ .../ledger/close/__tests__/content.test.tsx | 7 +- .../components/ChangedTransactionsPanel.tsx | 150 ++++++++++-------- .../close/components/PeriodClosePanel.tsx | 4 +- src/app/(app)/ledger/close/content.tsx | 7 +- src/lib/ledger/__tests__/errors.test.ts | 26 +++ src/lib/ledger/errors.ts | 21 +++ 8 files changed, 236 insertions(+), 75 deletions(-) diff --git a/src/app/(app)/ledger/close/__tests__/ChangedTransactionsPanel.test.tsx b/src/app/(app)/ledger/close/__tests__/ChangedTransactionsPanel.test.tsx index 0ff64593..5dc504b2 100644 --- a/src/app/(app)/ledger/close/__tests__/ChangedTransactionsPanel.test.tsx +++ b/src/app/(app)/ledger/close/__tests__/ChangedTransactionsPanel.test.tsx @@ -392,8 +392,9 @@ describe('ChangedTransactionsPanel', () => { await openFirst() fireEvent.click(screen.getByRole('button', { name: 'Settle' })) + // The app's words for the refusal, not the API's. expect(await screen.findByRole('alert')).toHaveTextContent( - 'Event evt_invoice is being written by another process.' + 'Another process (usually a running sync) is writing this right now.' ) expect(screen.getByText('qb-inv-1042')).toBeInTheDocument() expect(screen.getByRole('button', { name: 'Settle' })).toBeEnabled() @@ -444,6 +445,84 @@ describe('ChangedTransactionsPanel', () => { expect(screen.getByText('qb-inv-1042')).toBeInTheDocument() }) + it('drops a change that was settled before it could be reviewed', async () => { + mockPreviewReconcilingItem.mockRejectedValue( + refusal( + 'Event evt_invoice is not a reconciling item — its payload matches the source system.' + ) + ) + render() + fireEvent.click(await screen.findByRole('button', { name: 'Review' })) + + expect( + await screen.findByText( + 'That change was already settled. Nothing was done.' + ) + ).toBeInTheDocument() + expect(screen.queryByText('qb-inv-1042')).not.toBeInTheDocument() + }) + + it('clears the last outcome when another change is opened', async () => { + mockListEventBlocks.mockResolvedValue([ + event(), + event({ id: 'evt_bill', externalId: 'qb-bill-7' }), + ]) + render() + fireEvent.click( + (await screen.findAllByRole('button', { name: 'Review' }))[0] + ) + await screen.findByText('Difference by account') + fireEvent.click(screen.getByRole('button', { name: 'Settle' })) + expect(await screen.findByText(/^Restated\./)).toBeInTheDocument() + + fireEvent.click(screen.getByRole('button', { name: 'Review' })) + await screen.findByText('Difference by account') + expect(screen.queryByText(/^Restated\./)).not.toBeInTheDocument() + }) + + it('brings in the changes a full page hid once one is settled', async () => { + const page = Array.from({ length: 200 }, (_, i) => + event({ id: `evt_${i}`, externalId: `qb-${i}` }) + ) + mockListEventBlocks.mockResolvedValueOnce(page) + mockListEventBlocks.mockResolvedValueOnce([ + ...page.slice(1), + event({ id: 'evt_next', externalId: 'qb-next' }), + ]) + render() + + expect( + await screen.findByText('Showing 200. More appear as these are settled.') + ).toBeInTheDocument() + fireEvent.click(screen.getAllByRole('button', { name: 'Review' })[0]) + await screen.findByText('Difference by account') + fireEvent.click(screen.getByRole('button', { name: 'Settle' })) + + expect(await screen.findByText('qb-next')).toBeInTheDocument() + expect(mockListEventBlocks).toHaveBeenCalledTimes(2) + expect(screen.queryByText('qb-0')).not.toBeInTheDocument() + }) + + it('does not refetch after a settle when the whole list was on screen', async () => { + await openFirst() + fireEvent.click(screen.getByRole('button', { name: 'Settle' })) + + await screen.findByText(/^Restated\./) + expect(mockListEventBlocks).toHaveBeenCalledTimes(1) + }) + + it('links a reference only to a web address', async () => { + mockListEventBlocks.mockResolvedValue([ + event({ externalUrl: 'javascript:alert(1)' }), + ]) + render() + + expect(await screen.findByText('qb-inv-1042')).toBeInTheDocument() + expect( + screen.queryByRole('link', { name: 'qb-inv-1042' }) + ).not.toBeInTheDocument() + }) + it('ignores a preview that lands after another change was opened', async () => { mockListEventBlocks.mockResolvedValue([ event(), diff --git a/src/app/(app)/ledger/close/__tests__/PeriodClosePanel.test.tsx b/src/app/(app)/ledger/close/__tests__/PeriodClosePanel.test.tsx index f32fb4a2..a4f807cd 100644 --- a/src/app/(app)/ledger/close/__tests__/PeriodClosePanel.test.tsx +++ b/src/app/(app)/ledger/close/__tests__/PeriodClosePanel.test.tsx @@ -330,6 +330,21 @@ describe('PeriodClosePanel — blockers name what is holding the close', () => { expect(onReviewChanges).toHaveBeenCalledTimes(1) }) + it('leaves the count out when the calendar reports none', async () => { + mockGetFiscalCalendar.mockResolvedValue({ + ...CALENDAR, + closeableNow: false, + blockers: ['reconciling_items'], + reconcilingItemCount: 0, + }) + render() + + const link = await screen.findByRole('button', { + name: 'Review changed transactions', + }) + expect(link.parentElement).not.toHaveTextContent('holding this close') + }) + it('offers no review link where nothing can open it', async () => { mockGetFiscalCalendar.mockResolvedValue({ ...CALENDAR, diff --git a/src/app/(app)/ledger/close/__tests__/content.test.tsx b/src/app/(app)/ledger/close/__tests__/content.test.tsx index 1609797f..b972012b 100644 --- a/src/app/(app)/ledger/close/__tests__/content.test.tsx +++ b/src/app/(app)/ledger/close/__tests__/content.test.tsx @@ -365,7 +365,8 @@ describe('CloseContent', () => { ], }, { - label: 'Reconciliations', + // Found by what it holds, so a renamed category keeps the entry. + label: 'Tie-outs', items: [ { id: 'reconciliations', @@ -381,9 +382,7 @@ describe('CloseContent', () => { render() - expect( - await screen.findByText('Reconciliations: 2 items') - ).toBeInTheDocument() + expect(await screen.findByText('Tie-outs: 2 items')).toBeInTheDocument() expect(screen.getByTestId('item-changed_transactions')).toBeInTheDocument() // Only that category gains the entry. expect(screen.getByText('Period Close: 1 items')).toBeInTheDocument() diff --git a/src/app/(app)/ledger/close/components/ChangedTransactionsPanel.tsx b/src/app/(app)/ledger/close/components/ChangedTransactionsPanel.tsx index 38d25f8d..429f8e76 100644 --- a/src/app/(app)/ledger/close/components/ChangedTransactionsPanel.tsx +++ b/src/app/(app)/ledger/close/components/ChangedTransactionsPanel.tsx @@ -1,7 +1,7 @@ 'use client' import DocsLink from '@/components/DocsLink' -import { extractDetail } from '@/lib/ledger/errors' +import { friendlyError, type FriendlyError } from '@/lib/ledger/errors' import { formatAmount, formatDate } from '@/lib/ledger/formatters' import type { ReconcilingItemPlan, @@ -42,9 +42,14 @@ function sourceName(source: string | null | undefined): string { // ── Helpers ──────────────────────────────────────────────────────────── -/** The API's own words for a refusal, or a fallback when there are none. */ -const describeError = (err: unknown, fallback: string): string => - err instanceof Error ? extractDetail(err.message) : fallback +/** A refusal in the app's words, or the fallback when nothing was said. */ +const describeError = (err: unknown, fallback: string): FriendlyError => + err instanceof Error ? friendlyError(err.message) : { message: fallback } + +/** A link only to a web address; anything else stays text. */ +function webHref(url: string | null | undefined): string | null { + return url && /^https?:\/\//i.test(url) ? url : null +} /** A signed amount in minor units, debit-positive, as the API reports it. */ function netAmount(cents: number, currency: string | null | undefined) { @@ -111,45 +116,74 @@ const ChangedTransactionsPanel: FC = ({ const [notice, setNotice] = useState(null) const [outcome, setOutcome] = useState(null) - // Sequence guards: a preview that lands after another row was opened, and - // anything that lands after the graph changed, is dropped. + // Sequence guards: a list or a preview that lands after a newer one was + // asked for, and anything that lands after the graph changed, is dropped. + const listSeq = useRef(0) const planSeq = useRef(0) const graphSeq = useRef(0) + const loadList = useCallback(async () => { + const seq = ++listSeq.current + try { + const list = await clients.ledger.listEventBlocks(graphId, { + isReconcilingItem: true, + limit: CHANGES_LIMIT, + }) + if (seq !== listSeq.current) return + list.sort( + (a, b) => + new Date(b.occurredAt).getTime() - new Date(a.occurredAt).getTime() + ) + setItems(list) + setTruncated(list.length >= CHANGES_LIMIT) + setError(null) + } catch (err) { + if (seq !== listSeq.current) return + console.error('Error loading changed transactions:', err) + setError('Failed to load changed transactions.') + // A failed refresh keeps the rows already on screen. + setItems((current) => current ?? []) + } + }, [graphId]) + useEffect(() => { - let cancelled = false planSeq.current += 1 graphSeq.current += 1 setItems(null) setTruncated(false) setSelectedId(null) setPlan(null) + setPlanError(null) + setSettleError(null) + setNotice(null) + setNote('') setOutcome(null) setError(null) - void (async () => { - try { - const list = await clients.ledger.listEventBlocks(graphId, { - isReconcilingItem: true, - limit: CHANGES_LIMIT, - }) - if (cancelled) return - list.sort( - (a, b) => - new Date(b.occurredAt).getTime() - new Date(a.occurredAt).getTime() - ) - setItems(list) - setTruncated(list.length >= CHANGES_LIMIT) - } catch (err) { - if (cancelled) return - console.error('Error loading changed transactions:', err) - setError('Failed to load changed transactions.') - setItems([]) - } - })() - return () => { - cancelled = true - } - }, [graphId]) + void loadList() + }, [loadList]) + + const closeItem = useCallback(() => { + planSeq.current += 1 + setSelectedId(null) + setPlan(null) + setPlanError(null) + setSettleError(null) + setNotice(null) + }, []) + + /** Takes a change that is no longer open off the list, and says why. */ + const dropRow = useCallback( + (eventId: string, message: string) => { + setItems((current) => + current ? current.filter((item) => item.id !== eventId) : current + ) + closeItem() + setOutcome(message) + // A full page hid the rest; bring the next ones in. + if (truncated) void loadList() + }, + [closeItem, loadList, truncated] + ) const openItem = useCallback( async (eventId: string) => { @@ -159,6 +193,7 @@ const ChangedTransactionsPanel: FC = ({ setPlanError(null) setSettleError(null) setNotice(null) + setOutcome(null) setNote('') try { const result = await clients.ledger.previewReconcilingItem( @@ -171,21 +206,17 @@ const ChangedTransactionsPanel: FC = ({ } catch (err) { if (seq !== planSeq.current) return console.error('Error previewing changed transaction:', err) - setPlanError(describeError(err, 'Failed to load this change.')) + const refusal = describeError(err, 'Failed to load this change.') + if (refusal.code === 'change_already_settled') { + dropRow(eventId, refusal.message) + } else { + setPlanError(refusal.message) + } } }, - [graphId] + [graphId, dropRow] ) - const closeItem = useCallback(() => { - planSeq.current += 1 - setSelectedId(null) - setPlan(null) - setPlanError(null) - setSettleError(null) - setNotice(null) - }, []) - const handleSettle = useCallback(async () => { if (!selectedId || !plan) return const trimmed = note.trim() @@ -199,35 +230,24 @@ const ChangedTransactionsPanel: FC = ({ ...(trimmed ? { note: trimmed } : {}), }) if (seq !== graphSeq.current) return - setOutcome(describeOutcome(result)) - setItems((current) => - current ? current.filter((item) => item.id !== selectedId) : current - ) - closeItem() + dropRow(selectedId, describeOutcome(result)) } catch (err) { console.error('Error settling changed transaction:', err) if (seq !== graphSeq.current) return - const detail = describeError(err, 'The change was not settled.') - if (detail.includes('not a reconciling item')) { - // Settled from somewhere else since the list loaded. - setItems((current) => - current ? current.filter((item) => item.id !== selectedId) : current - ) - closeItem() - setOutcome('That change was already settled. Nothing was done.') - } else if (detail.includes('re-flagged')) { - // The source changed again; what was on screen is no longer the plan. + const refusal = describeError(err, 'The change was not settled.') + if (refusal.code === 'change_already_settled') { + dropRow(selectedId, refusal.message) + } else if (refusal.code === 'change_flagged_again') { + // What was on screen is no longer the plan; show the newer one. await openItem(selectedId) - setNotice( - 'This transaction changed again at the source while it was open. These are the latest figures; nothing was settled.' - ) + setNotice(refusal.message) } else { - setSettleError(detail) + setSettleError(refusal.message) } } finally { setIsSettling(false) } - }, [graphId, selectedId, plan, treatment, note, closeItem, openItem]) + }, [graphId, selectedId, plan, treatment, note, dropRow, openItem]) const selected = items?.find((item) => item.id === selectedId) ?? null @@ -316,9 +336,9 @@ const ChangedTransactionsPanel: FC = ({ {sourceName(item.source)} - {item.externalUrl && item.externalId ? ( + {webHref(item.externalUrl) && item.externalId ? ( = ({ {truncated && (

- Showing the {CHANGES_LIMIT} most recent. Settle these to see the rest. + Showing {CHANGES_LIMIT}. More appear as these are settled.

)} diff --git a/src/app/(app)/ledger/close/components/PeriodClosePanel.tsx b/src/app/(app)/ledger/close/components/PeriodClosePanel.tsx index 3f17daab..9edbf958 100644 --- a/src/app/(app)/ledger/close/components/PeriodClosePanel.tsx +++ b/src/app/(app)/ledger/close/components/PeriodClosePanel.tsx @@ -893,7 +893,9 @@ const CalendarSummary: FC = ({ {obligationDetailFor(code, calendar)} {code === 'reconciling_items' && onReviewChanges && (
- {calendar.reconcilingItemCount} holding this close.{' '} + {calendar.reconcilingItemCount > 0 && ( + <>{calendar.reconcilingItemCount} holding this close. + )}