From c096df563f5439f270bdf2954d805e3513f67fdb Mon Sep 17 00:00:00 2001 From: Sarah Milne <212941134+smilne3@users.noreply.github.com> Date: Thu, 5 Nov 2026 11:38:04 -0500 Subject: [PATCH 1/4] The category picker reloads itself after a stale refusal, instead of saying "Refresh and try again" (#139) The categorize route answered four refusals with "... Refresh and try again.": the transaction gone (404), removed by the bank (400), the category deleted (400), or the row changed under the write (409). The picker showed the words as they stood, and the installed app has no reload button, so the person was told to do something they couldn't, while the row kept its stale value. The picker now reloads itself after a 404 or 409, as CategoryRulesCard and the reimbursable controls already do, and the route says "Showing the latest." instead. The removed row and the deleted category answer 409 rather than 400, since they're stale rows too, so the reload removes the row or updates the list. The refusal's words stay through the reload that moves the select, so it doesn't change with nothing to say why. Any other refusal still neither reloads nor survives the next change from the server. Co-Authored-By: Claude Opus 5.5 --- app/api/transactions/categorize/route.ts | 18 +++--- components/CategoryPicker.tsx | 19 ++++++- tests/unit/categorize-route.test.ts | 30 ++++++---- tests/unit/category-picker.test.tsx | 71 +++++++++++++++++++++++- tests/unit/transaction-card.test.tsx | 4 +- tests/unit/transaction-row.test.tsx | 8 ++- 6 files changed, 118 insertions(+), 32 deletions(-) diff --git a/app/api/transactions/categorize/route.ts b/app/api/transactions/categorize/route.ts index 9d2b338..a0a8ba0 100644 --- a/app/api/transactions/categorize/route.ts +++ b/app/api/transactions/categorize/route.ts @@ -27,6 +27,10 @@ import { unknownIfLost } from '@/lib/lost-reply' // // It is not the only writer of user_category: app/api/categories/route.ts also writes it in bulk on // rename and delete (a delete clears picks), outside these guards (#100). +// +// 404 and 409 mean the picker's row is stale: the transaction is gone, the bank removed it, the +// category was deleted, or the row changed. CategoryPicker reloads on either, so these say the latest +// is showing. They used to say "Refresh and try again", which the installed app has no way to do (#139). const SAVE_FAILED = 'That could not be saved. Please try again.' export async function POST(req: Request) { @@ -68,13 +72,10 @@ export async function POST(req: Request) { return NextResponse.json({ error: SAVE_FAILED }, { status: 500 }) } if (!row) { - return NextResponse.json({ error: 'That transaction no longer exists. Refresh and try again.' }, { status: 404 }) + return NextResponse.json({ error: 'That transaction no longer exists. Showing the latest.' }, { status: 404 }) } if (row.removed) { - return NextResponse.json( - { error: 'Your bank removed that transaction. Refresh and try again.' }, - { status: 400 } - ) + return NextResponse.json({ error: 'Your bank removed that transaction. Showing the latest.' }, { status: 409 }) } // 4. Card payments, picked or not. @@ -98,10 +99,7 @@ export async function POST(req: Request) { } const picked = (cats ?? []).find((c) => c.name === category) if (!picked) { - return NextResponse.json( - { error: 'That category no longer exists. Refresh and try again.' }, - { status: 400 } - ) + return NextResponse.json({ error: 'That category no longer exists. Showing the latest.' }, { status: 409 }) } // 6. Write, with the guards INSIDE the update, so Plaid re-tagging or removing the row between the @@ -125,7 +123,7 @@ export async function POST(req: Request) { // Usually the row changed underneath us. A missing update policy would also land here, which is // why the log says both. console.error('[categorize] update matched no rows (row changed, or the update policy is missing)', transactionId) - return NextResponse.json({ error: 'This transaction just changed. Refresh and try again.' }, { status: 409 }) + return NextResponse.json({ error: 'This transaction just changed. Showing the latest.' }, { status: 409 }) } // 7. A record of every change, so a mistaken pick can be put back from the logs. diff --git a/components/CategoryPicker.tsx b/components/CategoryPicker.tsx index dbf7590..69d0cb9 100644 --- a/components/CategoryPicker.tsx +++ b/components/CategoryPicker.tsx @@ -34,6 +34,11 @@ export function CategoryPicker({ const val = pending ?? value const [saving, setSaving] = useState(false) const [error, setError] = useState(null) + // Set when a refusal that says this row is stale (404 or 409) triggers the reload, and consumed by + // the re-sync that reload brings. That reload is what moves the select to another device's pick, + // so the refusal's words, which say the latest is showing, stay with it rather than vanish as the + // select moves (#139). As CategoryRulesCard's refusedStale. + const [refusedStale, setRefusedStale] = useState(false) // Follow the server. When a refresh brings a new value, or the same name for a new reason (a rule // equal to the bank's category is removed: rule → bank), show it and drop any optimistic choice // or alert. React's documented way to adjust state when a prop changes; the same idiom as @@ -43,7 +48,8 @@ export function CategoryPicker({ if (seen !== serverKey) { setSeen(serverKey) setPending(null) - setError(null) + if (!refusedStale) setError(null) + setRefusedStale(false) } // The current value is always selectable, even when it is 'Uncategorized' or a stale name. @@ -59,8 +65,9 @@ export function CategoryPicker({ setPending(category) working(true) setError(null) - // What to do once the request has settled: reload after a save, or after one whose outcome is - // unknown. Never while it is in flight, and never after a refusal. + setRefusedStale(false) + // What to do once the request has settled: reload after a save, after one whose outcome is + // unknown, or after a refusal that says this row is stale. Never while it is in flight. let reload = false try { // The shared rule (lib/save-response.ts): a redirect to /login or an OK reply that is not JSON @@ -78,6 +85,12 @@ export function CategoryPicker({ setPending(null) if (result.outcome === 'refused') { setError(result.error) + // 404 and 409: the route says this row is stale. The transaction is gone, the bank removed + // it, the category was deleted, or it changed on another device. So show the latest under + // the message, which says so. The installed app has no other way to reload (#139). Any + // other refusal wrote nothing, and the server still holds what the picker shows. + reload = result.status === 404 || result.status === 409 + setRefusedStale(reload) } else { // Failed in flight, timed out, or a reply that could not be read. None of them means the // request never reached the server. diff --git a/tests/unit/categorize-route.test.ts b/tests/unit/categorize-route.test.ts index 03e32ef..f018f14 100644 --- a/tests/unit/categorize-route.test.ts +++ b/tests/unit/categorize-route.test.ts @@ -226,9 +226,13 @@ describe('POST /api/transactions/categorize', () => { expect(calls.updates).toHaveLength(0) }) - it('answers 404 when the transaction does not exist', async () => { + // A stale row answers 404 or 409, which CategoryPicker reloads on, so the words say the latest is + // showing. "Refresh and try again" asked for something the installed app has no way to do (#139). + it('answers 404 when the transaction does not exist, saying the latest is showing', async () => { makeSupabase({ row: null }) - expect((await post({ transactionId: 'txn-1', category: 'Grocery' })).status).toBe(404) + const res = await post({ transactionId: 'txn-1', category: 'Grocery' }) + expect(res.status).toBe(404) + expect((await res.json()).error).toBe('That transaction no longer exists. Showing the latest.') }) it('reads every column the later steps need', async () => { @@ -239,11 +243,12 @@ describe('POST /api/transactions/categorize', () => { } }) - it('answers 400 for a row the bank removed, and writes nothing', async () => { + // 409, not 400, so the picker reloads and the removed row goes from the list (#139). + it('answers 409 for a row the bank removed, and writes nothing', async () => { const calls = makeSupabase({ row: { ...ROW, removed: true } }) const res = await post({ transactionId: 'txn-1', category: 'Grocery' }) - expect(res.status).toBe(400) - expect((await res.json()).error).toBe('Your bank removed that transaction. Refresh and try again.') + expect(res.status).toBe(409) + expect((await res.json()).error).toBe('Your bank removed that transaction. Showing the latest.') expect(calls.updates).toHaveLength(0) }) @@ -282,12 +287,13 @@ describe('POST /api/transactions/categorize', () => { }) // A name that is not one of the household's categories, including the display fallback - // 'Uncategorized' (spec decision 7: clearing is not supported). - it.each(['Uncategorized', 'Not A Category'])('answers 400 for %s, and writes nothing', async (category) => { + // 'Uncategorized' (spec decision 7: clearing is not supported). From the picker, that's a category + // deleted since the list loaded, so 409, not 400: the picker reloads its options (#139). + it.each(['Uncategorized', 'Not A Category'])('answers 409 for %s, and writes nothing', async (category) => { const calls = makeSupabase({ row: ROW }) const res = await post({ transactionId: 'txn-1', category }) - expect(res.status).toBe(400) - expect((await res.json()).error).toBe('That category no longer exists. Refresh and try again.') + expect(res.status).toBe(409) + expect((await res.json()).error).toBe('That category no longer exists. Showing the latest.') expect(calls.updates).toHaveLength(0) }) @@ -336,7 +342,7 @@ describe('POST /api/transactions/categorize', () => { makeSupabase({ row: ROW, update: { error: null, count: 0 } }) const res = await post({ transactionId: 'txn-1', category: 'Grocery' }) expect(res.status).toBe(409) - expect((await res.json()).error).toBe('This transaction just changed. Refresh and try again.') + expect((await res.json()).error).toBe('This transaction just changed. Showing the latest.') }) // A count the server did not send is not a count of one. Treating it as a write would report a @@ -348,10 +354,10 @@ describe('POST /api/transactions/categorize', () => { }) // Category names match exactly, as stored. A near-miss is not that category. - it('answers 400 for a category in the wrong case, and writes nothing', async () => { + it('answers 409 for a category in the wrong case, and writes nothing', async () => { const calls = makeSupabase({ row: ROW }) const res = await post({ transactionId: 'txn-1', category: 'grocery' }) - expect(res.status).toBe(400) + expect(res.status).toBe(409) expect(calls.updates).toHaveLength(0) }) diff --git a/tests/unit/category-picker.test.tsx b/tests/unit/category-picker.test.tsx index 4d246c4..9c6e2c9 100644 --- a/tests/unit/category-picker.test.tsx +++ b/tests/unit/category-picker.test.tsx @@ -30,8 +30,9 @@ const select = () => screen.getByRole('combobox') as HTMLSelectElement // The route always answers in JSON, so every stand-in for its reply carries that content type. const JSON_TYPE = { 'content-type': 'application/json' } -const reply = (ok: boolean, body: unknown) => ({ +const reply = (ok: boolean, body: unknown, status = ok ? 200 : 400) => ({ ok, + status, redirected: false, headers: new Headers(JSON_TYPE), json: async () => body, @@ -41,7 +42,7 @@ describe('CategoryPicker', () => { // #28 spec §8.2: keyed on source and value, so a row whose REASON for a name changes (a rule equal // to the bank's category is removed: rule → bank, same name) still drops a stale alert. it('clears its alert when the source changes and the name does not', async () => { - vi.stubGlobal('fetch', vi.fn().mockResolvedValue(reply(false, { error: 'This transaction just changed. Refresh and try again.' }))) + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(reply(false, { error: 'A credit-card payment is already kept out of spending and income.' }))) const { rerender } = render() fireEvent.change(select(), { target: { value: 'Shopping' } }) expect(await screen.findByRole('alert')).toBeTruthy() @@ -95,7 +96,8 @@ describe('CategoryPicker', () => { }) // A refused save wrote nothing, so the server still holds the old value. The picker already shows - // it while the error explains why; a refresh would only re-render the same value. + // it while the error explains why; a refresh would only re-render the same value. A refusal that + // says the row is stale (404 or 409) is the exception, below. it('does not reload the page when the save is refused', async () => { vi.stubGlobal( 'fetch', @@ -429,3 +431,66 @@ describe('CategoryPicker', () => { expect(refresh).toHaveBeenCalledTimes(1) }) }) + +// #139: the route's 404 and 409 mean this row is stale: the transaction is gone, the bank removed it, +// the category was deleted, or it changed on another device. The route used to say "Refresh and try +// again", and in the installed app there's nothing to refresh with. So the picker reloads itself, and +// the words, which now say the latest is showing, stay as the reload moves the select. +describe('CategoryPicker: a refusal that says this row is stale (#139)', () => { + const STALE = [ + [404, 'That transaction no longer exists. Showing the latest.'], + [409, 'This transaction just changed. Showing the latest.'], + ] as const + + it.each(STALE)('reloads after a %s, and shows its words', async (status, words) => { + vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: words }, status))) + render() + fireEvent.change(select(), { target: { value: 'Shopping' } }) + await waitFor(() => expect(screen.getByRole('alert').textContent).toBe(words)) + expect(select().value).toBe('Food & Drink') + expect(refresh).toHaveBeenCalledTimes(1) + }) + + // The reload brings another device's pick. The words stay, so the select doesn't change with nothing + // to say why. The next change from the server clears them, as before. + it('keeps the words when the reload moves the select, and drops them at the next change', async () => { + vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: STALE[1][1] }, 409))) + const { rerender } = render() + fireEvent.change(select(), { target: { value: 'Shopping' } }) + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()) + rerender() + expect(select().value).toBe('Travel') + expect(screen.getByRole('alert').textContent).toBe(STALE[1][1]) + rerender() + expect(screen.queryByRole('alert')).toBeNull() + }) + + // Any other refusal wrote nothing and left the row as it was, so it neither reloads nor survives + // the next change from the server. + it.each([400, 500, 503])('does neither after a %s', async (status) => { + vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: 'nope' }, status))) + const { rerender } = render() + fireEvent.change(select(), { target: { value: 'Shopping' } }) + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()) + expect(refresh).not.toHaveBeenCalled() + rerender() + expect(screen.queryByRole('alert')).toBeNull() + }) + + // A pick after a stale refusal starts afresh: its own refusal of another kind doesn't outlast a + // change from the server. + it('forgets a stale refusal once the next pick is sent', async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(reply(false, { error: STALE[1][1] }, 409)) + .mockResolvedValueOnce(reply(false, { error: 'nope' }, 400)) + vi.stubGlobal('fetch', fetchMock) + const { rerender } = render() + fireEvent.change(select(), { target: { value: 'Shopping' } }) + await waitFor(() => expect(screen.getByRole('alert').textContent).toBe(STALE[1][1])) + fireEvent.change(select(), { target: { value: 'Travel' } }) + await waitFor(() => expect(screen.getByRole('alert').textContent).toBe('nope')) + rerender() + expect(screen.queryByRole('alert')).toBeNull() + }) +}) diff --git a/tests/unit/transaction-card.test.tsx b/tests/unit/transaction-card.test.tsx index ee41f8b..df87516 100644 --- a/tests/unit/transaction-card.test.tsx +++ b/tests/unit/transaction-card.test.tsx @@ -738,12 +738,14 @@ describe('TransactionCard learned marker (#28)', () => { // CategoryPicker keys its stale-alert reset on source as well as name, so the sheet must hand it // the source: a rule equal to the bank's category being removed changes the reason, not the name. + // A refusal that isn't about a stale row, since a stale one's words stay through the reload (#139). it("passes the category's source to the picker, so a change of reason clears its alert", async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, + status: 400, redirected: false, headers: new Headers({ 'content-type': 'application/json' }), - json: async () => ({ error: 'This transaction just changed. Refresh and try again.' }), + json: async () => ({ error: 'A credit-card payment is already kept out of spending and income.' }), })) const agreeing = { name: 'Food', source: 'rule' as const, ruleId: 'r1', bankName: 'Food' } const { rerender } = render() diff --git a/tests/unit/transaction-row.test.tsx b/tests/unit/transaction-row.test.tsx index c113309..c93a0ce 100644 --- a/tests/unit/transaction-row.test.tsx +++ b/tests/unit/transaction-row.test.tsx @@ -229,7 +229,7 @@ describe('TransactionRow learned marker (#28)', () => { // A save error renders inside the picker container, so it cannot push the marker off the line. it('keeps the marker beside the select while an error shows', async () => { - vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, redirected: false, headers: new Headers({ 'content-type': 'application/json' }), json: async () => ({ error: 'This transaction just changed. Refresh and try again.' }) })) + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, status: 400, redirected: false, headers: new Headers({ 'content-type': 'application/json' }), json: async () => ({ error: 'A credit-card payment is already kept out of spending and income.' }) })) const { container } = renderRow({}, LEARNED) fireEvent.change(screen.getByRole('combobox'), { target: { value: 'Food' } }) const alert = await screen.findByRole('alert') @@ -269,14 +269,16 @@ describe('TransactionRow suggested marker (#28 §14.6)', () => { }) // CategoryPicker keys its stale-alert reset on source as well as name, so the row must hand it the -// source: a rule equal to the bank's category being removed changes the reason, not the name. +// source: a rule equal to the bank's category being removed changes the reason, not the name. A +// refusal that isn't about a stale row, since a stale one's words stay through the reload (#139). describe('TransactionRow category source', () => { it("passes the category's source to the picker, so a change of reason clears its alert", async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, + status: 400, redirected: false, headers: new Headers({ 'content-type': 'application/json' }), - json: async () => ({ error: 'This transaction just changed. Refresh and try again.' }), + json: async () => ({ error: 'A credit-card payment is already kept out of spending and income.' }), })) const agreeing: ResolvedCategory = { name: 'Food', source: 'rule', ruleId: 'r1', bankName: 'Food' } const row = (category: ResolvedCategory) => ( From dd2792082c57e77958dc96f47e7dc7d6c3416675 Mon Sep 17 00:00:00 2001 From: Sarah Milne <212941134+smilne3@users.noreply.github.com> Date: Thu, 5 Nov 2026 11:39:50 -0500 Subject: [PATCH 2/4] A pick after a stale refusal with an unknown outcome loses its message at the next change (#139) The test's second pick was another refusal, which clears the stale mark itself, so it could not see a new pick that forgot to. A request lost in flight can. Co-Authored-By: Claude Opus 5.5 --- tests/unit/category-picker.test.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/unit/category-picker.test.tsx b/tests/unit/category-picker.test.tsx index 9c6e2c9..23df59d 100644 --- a/tests/unit/category-picker.test.tsx +++ b/tests/unit/category-picker.test.tsx @@ -477,19 +477,20 @@ describe('CategoryPicker: a refusal that says this row is stale (#139)', () => { expect(screen.queryByRole('alert')).toBeNull() }) - // A pick after a stale refusal starts afresh: its own refusal of another kind doesn't outlast a - // change from the server. + // A pick after a stale refusal starts afresh. If that one's outcome is unknown, its message goes + // when the server's value changes, as always: the stale refusal before it doesn't keep it. it('forgets a stale refusal once the next pick is sent', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}) const fetchMock = vi .fn() .mockResolvedValueOnce(reply(false, { error: STALE[1][1] }, 409)) - .mockResolvedValueOnce(reply(false, { error: 'nope' }, 400)) + .mockRejectedValueOnce(new TypeError('fetch failed')) vi.stubGlobal('fetch', fetchMock) const { rerender } = render() fireEvent.change(select(), { target: { value: 'Shopping' } }) await waitFor(() => expect(screen.getByRole('alert').textContent).toBe(STALE[1][1])) fireEvent.change(select(), { target: { value: 'Travel' } }) - await waitFor(() => expect(screen.getByRole('alert').textContent).toBe('nope')) + await waitFor(() => expect(screen.getByRole('alert').textContent).toBe('It may not have saved. Showing the latest.')) rerender() expect(screen.queryByRole('alert')).toBeNull() }) From cb0444d5020dd2306801e616766d29c9319f43b1 Mon Sep 17 00:00:00 2001 From: Sarah Milne <212941134+smilne3@users.noreply.github.com> Date: Thu, 5 Nov 2026 11:41:45 -0500 Subject: [PATCH 3/4] The picker's comments say what makes the route's 409, which isn't another device's pick (#139) The route has no check on a pick another device made: the later pick wins. Its 409 is the bank removing or re-tagging the row, or a deleted category. Co-Authored-By: Claude Opus 5.5 --- components/CategoryPicker.tsx | 15 ++++++++------- tests/unit/category-picker.test.tsx | 14 ++++++++------ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/components/CategoryPicker.tsx b/components/CategoryPicker.tsx index 69d0cb9..4569358 100644 --- a/components/CategoryPicker.tsx +++ b/components/CategoryPicker.tsx @@ -35,9 +35,9 @@ export function CategoryPicker({ const [saving, setSaving] = useState(false) const [error, setError] = useState(null) // Set when a refusal that says this row is stale (404 or 409) triggers the reload, and consumed by - // the re-sync that reload brings. That reload is what moves the select to another device's pick, - // so the refusal's words, which say the latest is showing, stay with it rather than vanish as the - // select moves (#139). As CategoryRulesCard's refusedStale. + // the re-sync below if that reload changes this row. The refusal's words, which say the latest is + // showing, stay through that change rather than vanish as the select moves (#139). As + // CategoryRulesCard's refusedStale. const [refusedStale, setRefusedStale] = useState(false) // Follow the server. When a refresh brings a new value, or the same name for a new reason (a rule // equal to the bank's category is removed: rule → bank), show it and drop any optimistic choice @@ -85,10 +85,11 @@ export function CategoryPicker({ setPending(null) if (result.outcome === 'refused') { setError(result.error) - // 404 and 409: the route says this row is stale. The transaction is gone, the bank removed - // it, the category was deleted, or it changed on another device. So show the latest under - // the message, which says so. The installed app has no other way to reload (#139). Any - // other refusal wrote nothing, and the server still holds what the picker shows. + // 404 and 409: the route says this row is stale. The transaction is gone or the bank + // removed it, the category was deleted, or the bank changed the row during the save. So + // show the latest under the message, which says so. The installed app has no other way to + // reload (#139). Any other refusal wrote nothing, and the server still holds what the + // picker shows. reload = result.status === 404 || result.status === 409 setRefusedStale(reload) } else { diff --git a/tests/unit/category-picker.test.tsx b/tests/unit/category-picker.test.tsx index 23df59d..359e36a 100644 --- a/tests/unit/category-picker.test.tsx +++ b/tests/unit/category-picker.test.tsx @@ -432,10 +432,11 @@ describe('CategoryPicker', () => { }) }) -// #139: the route's 404 and 409 mean this row is stale: the transaction is gone, the bank removed it, -// the category was deleted, or it changed on another device. The route used to say "Refresh and try -// again", and in the installed app there's nothing to refresh with. So the picker reloads itself, and -// the words, which now say the latest is showing, stay as the reload moves the select. +// #139: the route's 404 and 409 mean this row is stale: the transaction is gone or the bank removed +// it, the category was deleted, or the bank changed the row during the save. The route used to say +// "Refresh and try again", and in the installed app there's nothing to refresh with. So the picker +// reloads itself, and the words, which now say the latest is showing, stay if the reload moves the +// select. describe('CategoryPicker: a refusal that says this row is stale (#139)', () => { const STALE = [ [404, 'That transaction no longer exists. Showing the latest.'], @@ -451,8 +452,9 @@ describe('CategoryPicker: a refusal that says this row is stale (#139)', () => { expect(refresh).toHaveBeenCalledTimes(1) }) - // The reload brings another device's pick. The words stay, so the select doesn't change with nothing - // to say why. The next change from the server clears them, as before. + // The reload changes this row, say a category deleted elsewhere took the row's pick with it. The + // words stay, so the select doesn't change with nothing to say why. The next change from the server + // clears them, as before. it('keeps the words when the reload moves the select, and drops them at the next change', async () => { vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: STALE[1][1] }, 409))) const { rerender } = render() From 815b70fbd483674209ae657fa177aad7f9cde264 Mon Sep 17 00:00:00 2001 From: Sarah Milne <212941134+smilne3@users.noreply.github.com> Date: Thu, 5 Nov 2026 12:21:11 -0500 Subject: [PATCH 4/4] The picker drops its stale-refusal flag, and the reimbursable controls say "Showing the latest" too (#139) From the review of #251: - The flag that kept a stale refusal's words through the reload is gone. The route's 409s almost never leave the row on screen and change it, and when the reload left the row alone, the flag kept the words past the next unrelated change. The picker still reloads after a 404 or 409, and its words now go at the next change, as any refusal's do. - A row that's gone takes its message with it when the list reloads. That's also true of the reimbursable controls, so it's filed as #252 rather than half-fixed here. - The reimbursable route's stale-row refusals say "Showing the latest." too, since those controls already reload on a 404 or 409. Its removed row answers 409 rather than 400, so the controls reload for it as well, and one sheet no longer gives two answers for one removed row. Co-Authored-By: Claude Opus 5.5 --- app/api/reimbursable/route.ts | 10 +++-- components/CategoryPicker.tsx | 18 +++------ tests/unit/category-picker.test.tsx | 49 +++++++---------------- tests/unit/reimbursable-checkbox.test.tsx | 6 +-- tests/unit/reimbursable-editor.test.tsx | 10 ++--- tests/unit/reimbursable-route.test.ts | 14 +++---- 6 files changed, 41 insertions(+), 66 deletions(-) diff --git a/app/api/reimbursable/route.ts b/app/api/reimbursable/route.ts index 9ff3a6f..af0b1d6 100644 --- a/app/api/reimbursable/route.ts +++ b/app/api/reimbursable/route.ts @@ -50,12 +50,14 @@ export async function PATCH(req: Request) { console.error('[reimbursable] transaction read failed', { id: transactionId, code: readError.code, message: readError.message }) return fail(500, TRY_AGAIN) } - if (!txn) return fail(404, 'That transaction no longer exists. Refresh and try again.') + if (!txn) return fail(404, 'That transaction no longer exists. Showing the latest.') // `removed` is a soft flag (a Plaid repost), not a delete, so nothing stops a mark being written // to a row that no longer renders anywhere. That mark would then be unreachable while still - // counting toward what you are owed. - if (txn.removed) return fail(400, 'Your bank removed that transaction. Refresh and try again.') + // counting toward what you are owed. A 409, like the other stale-row refusals here, so the + // editors reload and the removed row goes. These used to say "Refresh and try again", which the + // installed app has no way to do (#139). + if (txn.removed) return fail(409, 'Your bank removed that transaction. Showing the latest.') // Guards #31. A credit-card payment is already excluded from both spending and income — the // purchases were counted when they happened — so marking it reduces nothing while still inflating @@ -94,7 +96,7 @@ export async function PATCH(req: Request) { if (!count) { // A stale-card refusal: the editors reload on a 409 to show the latest. console.error('[reimbursable] update matched no rows (row changed, or the update policy is missing)', { id: transactionId }) - return fail(409, 'This transaction just changed. Refresh and try again.') + return fail(409, 'This transaction just changed. Showing the latest.') } return NextResponse.json({ ok: true }) diff --git a/components/CategoryPicker.tsx b/components/CategoryPicker.tsx index 4569358..0654bb6 100644 --- a/components/CategoryPicker.tsx +++ b/components/CategoryPicker.tsx @@ -34,11 +34,6 @@ export function CategoryPicker({ const val = pending ?? value const [saving, setSaving] = useState(false) const [error, setError] = useState(null) - // Set when a refusal that says this row is stale (404 or 409) triggers the reload, and consumed by - // the re-sync below if that reload changes this row. The refusal's words, which say the latest is - // showing, stay through that change rather than vanish as the select moves (#139). As - // CategoryRulesCard's refusedStale. - const [refusedStale, setRefusedStale] = useState(false) // Follow the server. When a refresh brings a new value, or the same name for a new reason (a rule // equal to the bank's category is removed: rule → bank), show it and drop any optimistic choice // or alert. React's documented way to adjust state when a prop changes; the same idiom as @@ -48,8 +43,7 @@ export function CategoryPicker({ if (seen !== serverKey) { setSeen(serverKey) setPending(null) - if (!refusedStale) setError(null) - setRefusedStale(false) + setError(null) } // The current value is always selectable, even when it is 'Uncategorized' or a stale name. @@ -65,7 +59,6 @@ export function CategoryPicker({ setPending(category) working(true) setError(null) - setRefusedStale(false) // What to do once the request has settled: reload after a save, after one whose outcome is // unknown, or after a refusal that says this row is stale. Never while it is in flight. let reload = false @@ -87,11 +80,12 @@ export function CategoryPicker({ setError(result.error) // 404 and 409: the route says this row is stale. The transaction is gone or the bank // removed it, the category was deleted, or the bank changed the row during the save. So - // show the latest under the message, which says so. The installed app has no other way to - // reload (#139). Any other refusal wrote nothing, and the server still holds what the - // picker shows. + // show the latest, as the message says. The installed app has no other way to reload + // (#139). For a deleted category the row stays and so does the message, and the options + // lose the category. A row that's gone takes its message with it, and a pending + // transaction that posted shows as the posted one, to pick again (#252). Any other refusal + // wrote nothing, and the server still holds what the picker shows. reload = result.status === 404 || result.status === 409 - setRefusedStale(reload) } else { // Failed in flight, timed out, or a reply that could not be read. None of them means the // request never reached the server. diff --git a/tests/unit/category-picker.test.tsx b/tests/unit/category-picker.test.tsx index 359e36a..ab8156d 100644 --- a/tests/unit/category-picker.test.tsx +++ b/tests/unit/category-picker.test.tsx @@ -14,7 +14,8 @@ afterEach(() => { // A real spy, not a bare closure. With `refresh: () => {}` no test can tell whether the list is ever // told to reload, so deleting the call, or making it run after a refused save, leaves the suite // green. A refused save writes nothing: the server holds the old value, which the picker already -// shows while the error explains why, so a reload there is a wasted round trip. +// shows while the error explains why, so a reload there is a wasted round trip. The exception is a +// refusal that says the row is stale, where showing the latest is the point (#139). const refresh = vi.hoisted(() => vi.fn()) vi.mock('next/navigation', () => ({ useRouter: () => ({ refresh }) })) @@ -432,15 +433,15 @@ describe('CategoryPicker', () => { }) }) + // #139: the route's 404 and 409 mean this row is stale: the transaction is gone or the bank removed // it, the category was deleted, or the bank changed the row during the save. The route used to say // "Refresh and try again", and in the installed app there's nothing to refresh with. So the picker -// reloads itself, and the words, which now say the latest is showing, stay if the reload moves the -// select. +// reloads itself, and the words now say the latest is showing. describe('CategoryPicker: a refusal that says this row is stale (#139)', () => { const STALE = [ [404, 'That transaction no longer exists. Showing the latest.'], - [409, 'This transaction just changed. Showing the latest.'], + [409, 'That category no longer exists. Showing the latest.'], ] as const it.each(STALE)('reloads after a %s, and shows its words', async (status, words) => { @@ -452,48 +453,26 @@ describe('CategoryPicker: a refusal that says this row is stale (#139)', () => { expect(refresh).toHaveBeenCalledTimes(1) }) - // The reload changes this row, say a category deleted elsewhere took the row's pick with it. The - // words stay, so the select doesn't change with nothing to say why. The next change from the server - // clears them, as before. - it('keeps the words when the reload moves the select, and drops them at the next change', async () => { + // A deleted category: the reload leaves the row as it was, so the message stays while the options + // lose the category. Like any refusal's, it goes when the server's value next changes. + it('keeps its message while the reload leaves the row as it was', async () => { vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: STALE[1][1] }, 409))) const { rerender } = render() fireEvent.change(select(), { target: { value: 'Shopping' } }) await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()) - rerender() - expect(select().value).toBe('Travel') + rerender() expect(screen.getByRole('alert').textContent).toBe(STALE[1][1]) - rerender() + expect([...select().options].map((o) => o.value)).toEqual(['Food & Drink', 'Travel']) + rerender() expect(screen.queryByRole('alert')).toBeNull() }) - // Any other refusal wrote nothing and left the row as it was, so it neither reloads nor survives - // the next change from the server. - it.each([400, 500, 503])('does neither after a %s', async (status) => { + // Any other refusal wrote nothing and left the row as it was, so there's nothing to reload. + it.each([400, 500, 503])('does not reload after a %s', async (status) => { vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: 'nope' }, status))) - const { rerender } = render() + render() fireEvent.change(select(), { target: { value: 'Shopping' } }) await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()) expect(refresh).not.toHaveBeenCalled() - rerender() - expect(screen.queryByRole('alert')).toBeNull() - }) - - // A pick after a stale refusal starts afresh. If that one's outcome is unknown, its message goes - // when the server's value changes, as always: the stale refusal before it doesn't keep it. - it('forgets a stale refusal once the next pick is sent', async () => { - vi.spyOn(console, 'error').mockImplementation(() => {}) - const fetchMock = vi - .fn() - .mockResolvedValueOnce(reply(false, { error: STALE[1][1] }, 409)) - .mockRejectedValueOnce(new TypeError('fetch failed')) - vi.stubGlobal('fetch', fetchMock) - const { rerender } = render() - fireEvent.change(select(), { target: { value: 'Shopping' } }) - await waitFor(() => expect(screen.getByRole('alert').textContent).toBe(STALE[1][1])) - fireEvent.change(select(), { target: { value: 'Travel' } }) - await waitFor(() => expect(screen.getByRole('alert').textContent).toBe('It may not have saved. Showing the latest.')) - rerender() - expect(screen.queryByRole('alert')).toBeNull() }) }) diff --git a/tests/unit/reimbursable-checkbox.test.tsx b/tests/unit/reimbursable-checkbox.test.tsx index 8e5f29d..2267443 100644 --- a/tests/unit/reimbursable-checkbox.test.tsx +++ b/tests/unit/reimbursable-checkbox.test.tsx @@ -135,9 +135,9 @@ describe('ReimbursableCheckbox', () => { // nothing and needs no reload, unless it says this row is stale (404, 409). An unknown outcome may // have written, so it reloads to show what the server has. it.each([ - ['a refusal', () => refused(400, 'Your bank removed that transaction. Refresh and try again.'), 'Your bank removed that transaction. Refresh and try again.', 0], - ['a 404', () => refused(404, 'That transaction no longer exists. Refresh and try again.'), 'That transaction no longer exists. Refresh and try again.', 1], - ['a 409', () => refused(409, 'This transaction just changed. Refresh and try again.'), 'This transaction just changed. Refresh and try again.', 1], + ['a refusal', () => refused(400, 'A credit-card payment is already kept out of spending and income.'), 'A credit-card payment is already kept out of spending and income.', 0], + ['a 404', () => refused(404, 'That transaction no longer exists. Showing the latest.'), 'That transaction no longer exists. Showing the latest.', 1], + ['a 409', () => refused(409, 'This transaction just changed. Showing the latest.'), 'This transaction just changed. Showing the latest.', 1], ['a redirect to the login page', redirectedToLogin, 'Your session ended. Sign in again.', 0], // The #103 bug: any OK reply used to count as saved. ['a 200 without ok: true', () => new Response('{}', { headers: { 'content-type': 'application/json' } }), 'That could not be saved.', 0], diff --git a/tests/unit/reimbursable-editor.test.tsx b/tests/unit/reimbursable-editor.test.tsx index ade8132..278b460 100644 --- a/tests/unit/reimbursable-editor.test.tsx +++ b/tests/unit/reimbursable-editor.test.tsx @@ -208,9 +208,9 @@ describe('ReimbursableEditor', () => { // what was typed, so a retry is one tap, and says why. A refusal wrote nothing and needs no reload // unless it says this row is stale (404, 409); an unknown outcome may have written, so it reloads. it.each([ - ['a refusal', () => refused(400, 'Your bank removed that transaction. Refresh and try again.'), 'Your bank removed that transaction. Refresh and try again.', 0], - ['a 404', () => refused(404, 'That transaction no longer exists. Refresh and try again.'), 'That transaction no longer exists. Refresh and try again.', 1], - ['a 409', () => refused(409, 'This transaction just changed. Refresh and try again.'), 'This transaction just changed. Refresh and try again.', 1], + ['a refusal', () => refused(400, 'A credit-card payment is already kept out of spending and income.'), 'A credit-card payment is already kept out of spending and income.', 0], + ['a 404', () => refused(404, 'That transaction no longer exists. Showing the latest.'), 'That transaction no longer exists. Showing the latest.', 1], + ['a 409', () => refused(409, 'This transaction just changed. Showing the latest.'), 'This transaction just changed. Showing the latest.', 1], ['a redirect to the login page', redirectedToLogin, 'Your session ended. Sign in again.', 0], // The #103 bug: any OK reply used to close the dialog as if saved. ['a 200 without ok: true', () => new Response('{}', { headers: { 'content-type': 'application/json' } }), 'That could not be saved.', 0], @@ -276,8 +276,8 @@ describe('ReimbursableEditor', () => { const escape = () => fireEvent(screen.getByRole('dialog'), new Event('cancel', { bubbles: false, cancelable: true })) escape() expect(screen.getByRole('dialog')).toBeTruthy() - settle(refused(400, 'Your bank removed that transaction. Refresh and try again.')) - expect((await screen.findByRole('alert')).textContent).toBe('Your bank removed that transaction. Refresh and try again.') + settle(refused(400, 'A credit-card payment is already kept out of spending and income.')) + expect((await screen.findByRole('alert')).textContent).toBe('A credit-card payment is already kept out of spending and income.') expect(screen.getByRole('dialog')).toBeTruthy() // Settled, so Escape is the way out again. escape() diff --git a/tests/unit/reimbursable-route.test.ts b/tests/unit/reimbursable-route.test.ts index 39c7f19..6bc1a8d 100644 --- a/tests/unit/reimbursable-route.test.ts +++ b/tests/unit/reimbursable-route.test.ts @@ -179,17 +179,17 @@ describe('PATCH /api/reimbursable', () => { const calls = makeSupabase({ row: null }) const res = await mark() expect(res.status).toBe(404) - expect(await errorOf(res)).toBe('That transaction no longer exists. Refresh and try again.') + expect(await errorOf(res)).toBe('That transaction no longer exists. Showing the latest.') expect(calls.updates).toHaveLength(0) }) // `removed` is a soft flag (a Plaid repost), not a delete. A mark written to a removed row would be - // unreachable while still counting toward what you're owed. - it('answers 400 for a transaction the bank removed, and writes nothing', async () => { + // unreachable while still counting toward what you're owed. 409, so the editors reload (#139). + it('answers 409 for a transaction the bank removed, and writes nothing', async () => { const calls = makeSupabase({ row: { ...TXN, removed: true } }) const res = await mark() - expect(res.status).toBe(400) - expect(await errorOf(res)).toBe('Your bank removed that transaction. Refresh and try again.') + expect(res.status).toBe(409) + expect(await errorOf(res)).toBe('Your bank removed that transaction. Showing the latest.') expect(calls.updates).toHaveLength(0) }) @@ -262,7 +262,7 @@ describe('PATCH /api/reimbursable', () => { makeSupabase({ row, rowAtWrite }) const res = await mark() expect(res.status).toBe(409) - expect(await errorOf(res)).toBe('This transaction just changed. Refresh and try again.') + expect(await errorOf(res)).toBe('This transaction just changed. Showing the latest.') expect(logs).toContainEqual([ '[reimbursable] update matched no rows (row changed, or the update policy is missing)', { id: 'txn-1' }, @@ -309,7 +309,7 @@ describe('PATCH /api/reimbursable', () => { makeSupabase({ update: { error: null, count: 0 } }) const res = await mark() expect(res.status).toBe(409) - expect(await errorOf(res)).toBe('This transaction just changed. Refresh and try again.') + expect(await errorOf(res)).toBe('This transaction just changed. Showing the latest.') expect(logs).toContainEqual([ '[reimbursable] update matched no rows (row changed, or the update policy is missing)', { id: 'txn-1' },