fix(budget): stop receipts deleting files, and share the upload path

Attaching a receipt no longer lets the budget domain delete one. Trashing the
file was reachable from the REST route, the MCP tool and the plugin RPC, none
of which check file_delete, and a receipt id is any file on the trip, so anyone
who could edit an expense could trash a document they were not allowed to
touch. A concurrent edit could do it by accident: the client sends the full set
of receipt ids from a snapshot taken when the form opened, so saving an expense
someone else had just attached a file to moved that file to the trash. Removing
a receipt now removes the link and nothing else.

The link row is treated as shared, because it is. One row can tie a file to a
place and to an expense at once, so the sync clears only the receipt half and
leaves the rest, the foreign key is SET NULL rather than CASCADE, and a receipt
already in the trash keeps its link so restoring it comes back attached.

The upload and its rollback moved into one module both shells call. It was
copied into six places, and all six rolled back with the permanent-delete
route, which only accepts a file that is already in the trash and therefore
answered 404 every time: a failed save left every uploaded file on the trip,
attached to the expense, and the next attempt uploaded another copy. It now
trashes first, keeps the pending files for the retry, and says so when a file
could not be removed instead of swallowing it.

Escape in the receipt viewer no longer closes the expense form behind it, the
download fallback uses a real accent class rather than a utility that does not
exist, and the upload indicator is actually switched on.

Tests: the receipt link behaviour on both service paths, the four new branches
in the files service, the upload module, and the viewer.

client/src/components/Budget/CostsPanel.tsx

@@ -7,7 +7,8 @@ import { useSettingsStore } from '../../store/settingsStore'
 import { useCanDo } from '../../store/permissionsStore'
 import { useToast } from '../shared/Toast'
 import { useTranslation } from '../../i18n'
-import { budgetApi, filesApi } from '../../api/client'
+import { budgetApi } from '../../api/client'
+import { saveWithReceipts } from './receiptUploads'
 import { useExchangeRates } from '../../hooks/useExchangeRates'
 import { useIsMobile } from '../../hooks/useIsMobile'
 import { formatMoney, currencyDecimals, currencyLocale, localizeAmountInput, amountToInputString } from '../../utils/formatters'
@@ -1372,57 +1373,23 @@ export function ExpenseModal({ tripId, base, people, me, editing, prefill, onClo
       ...(!editing && prefill?.placeId ? { place_id: prefill.placeId } : {}),
     }
     try {
-      // Upload any pending receipt files
-      const uploadedFileIds: number[] = []
-      try {
-        for (const f of pendingReceiptFiles) {
-          const fd = new FormData()
-          fd.append('file', f)
-          if (editing) {
-            fd.append('budget_item_id', String(editing.id))
-          }
-          const res = await filesApi.upload(tripId, fd)
-          if (res?.file?.id) {
-            uploadedFileIds.push(res.file.id)
-          }
-        }
-      } catch (err) {
-        for (const fid of uploadedFileIds) {
-          try { await filesApi.permanentDelete(tripId, fid) } catch {}
-        }
-        throw err
-      }
-
-      let createdOrUpdated: BudgetItem
-      if (editing) {
-        try {
-          createdOrUpdated = await updateBudgetItem(tripId, editing.id, {
-            ...data,
-            receipt_file_ids: [...receipts.map(r => r.id), ...uploadedFileIds],
-          })
-        } catch (err) {
-          for (const fid of uploadedFileIds) {
-            try { await filesApi.permanentDelete(tripId, fid) } catch {}
-          }
-          throw err
-        }
-      } else {
-        try {
-          createdOrUpdated = await addBudgetItem(tripId, {
-            ...data,
-            receipt_file_ids: uploadedFileIds,
-          })
-        } catch (err) {
-          for (const fid of uploadedFileIds) {
-            try { await filesApi.permanentDelete(tripId, fid) } catch {}
-          }
-          throw err
-        }
-      }
+      setUploadingReceipt(pendingReceiptFiles.length > 0)
+      await saveWithReceipts(tripId, pendingReceiptFiles, editing ? editing.id : null, ids => (
+        editing
+          ? updateBudgetItem(tripId, editing.id, { ...data, receipt_file_ids: [...receipts.map(r => r.id), ...ids] })
+          : addBudgetItem(tripId, { ...data, receipt_file_ids: ids })
+      ))
+      // Only cleared once the save went through, so a retry after a failure
+      // does not upload a second copy of every file.
+      setPendingReceiptFiles([])
       onSaved()
-    } catch {
-      toast.error(t('common.unknownError'))
+    } catch (err) {
+      // A receipt the rollback could not remove is still on the trip, and the
+      // user is the only one who can clear it out of the Files tab.
+      const stuck = (err as { stuckReceiptIds?: number[] })?.stuckReceiptIds
+      toast.error(stuck?.length ? t('costs.receiptLeftBehind', { count: stuck.length }) : t('common.unknownError'))
     } finally {
+      setUploadingReceipt(false)
       setSaving(false)
     }
   }

client/src/components/Budget/ReceiptPreviewModal.test.tsx

@@ -0,0 +1,99 @@
+// FE-BUDGET-PREVIEW-001 to FE-BUDGET-PREVIEW-008: the receipt viewer that opens
+// on top of the expense form.
+import { vi } from 'vitest'
+import { render, screen, waitFor, fireEvent } from '../../../tests/helpers/render'
+import { ReceiptPreviewModal } from './ReceiptPreviewModal'
+import type { BudgetItemReceipt } from '../../types'
+
+const authUrl = vi.hoisted(() => ({ get: vi.fn() }))
+vi.mock('../../api/authUrl', () => ({ getAuthUrl: (...a: unknown[]) => authUrl.get(...a) }))
+
+function receipt(over: Partial<BudgetItemReceipt> = {}): BudgetItemReceipt {
+  return {
+    id: 1,
+    filename: 'stored.jpg',
+    original_name: 'lunch.jpg',
+    mime_type: 'image/jpeg',
+    file_size: 100,
+    url: '/uploads/files/stored.jpg',
+    ...over,
+  } as BudgetItemReceipt
+}
+
+beforeEach(() => {
+  authUrl.get.mockReset().mockResolvedValue('/signed/stored.jpg')
+})
+
+describe('ReceiptPreviewModal', () => {
+  it('FE-BUDGET-PREVIEW-001: renders an image receipt once the signed url is in', async () => {
+    render(<ReceiptPreviewModal receipts={[receipt()]} onClose={vi.fn()} />)
+    await waitFor(() => expect(screen.getByRole('img')).toHaveAttribute('src', '/signed/stored.jpg'))
+  })
+
+  it('FE-BUDGET-PREVIEW-002: renders a PDF in a frame rather than an image', async () => {
+    render(<ReceiptPreviewModal receipts={[receipt({ mime_type: 'application/pdf', original_name: 'bill.pdf' })]} onClose={vi.fn()} />)
+    // The viewer renders through a portal, so it lives on document.body.
+    await waitFor(() => expect(document.body.querySelector('object[type="application/pdf"]')).not.toBeNull())
+    expect(screen.queryByRole('img')).toBeNull()
+  })
+
+  it('FE-BUDGET-PREVIEW-003: a type it cannot show falls back instead of rendering an empty frame', async () => {
+    render(<ReceiptPreviewModal receipts={[receipt({ mime_type: 'application/zip', original_name: 'receipts.zip' })]} onClose={vi.fn()} />)
+    await waitFor(() => expect(authUrl.get).toHaveBeenCalled())
+    expect(document.body.querySelector('object')).toBeNull()
+    expect(screen.queryByRole('img')).toBeNull()
+    expect(screen.getAllByText('receipts.zip').length).toBeGreaterThan(0)
+  })
+
+  it('FE-BUDGET-PREVIEW-004: a rejected signed url leaves the viewer up rather than blank-screening', async () => {
+    authUrl.get.mockRejectedValue(new Error('403'))
+    render(<ReceiptPreviewModal receipts={[receipt()]} onClose={vi.fn()} />)
+    await waitFor(() => expect(authUrl.get).toHaveBeenCalled())
+    expect(screen.getAllByText('lunch.jpg').length).toBeGreaterThan(0)
+  })
+
+  it('FE-BUDGET-PREVIEW-005: arrows walk the list and stop at both ends', async () => {
+    const two = [receipt({ id: 1, original_name: 'one.jpg' }), receipt({ id: 2, original_name: 'two.jpg' })]
+    render(<ReceiptPreviewModal receipts={two} onClose={vi.fn()} />)
+    await waitFor(() => expect(screen.getAllByText('one.jpg').length).toBeGreaterThan(0))
+
+    fireEvent.keyDown(window, { key: 'ArrowLeft' })
+    expect(screen.getAllByText('one.jpg').length).toBeGreaterThan(0)
+
+    fireEvent.keyDown(window, { key: 'ArrowRight' })
+    await waitFor(() => expect(screen.getAllByText('two.jpg').length).toBeGreaterThan(0))
+
+    fireEvent.keyDown(window, { key: 'ArrowRight' })
+    expect(screen.getAllByText('two.jpg').length).toBeGreaterThan(0)
+  })
+
+  it('FE-BUDGET-PREVIEW-006: an out-of-range initialIndex is clamped instead of rendering nothing', async () => {
+    const two = [receipt({ id: 1, original_name: 'one.jpg' }), receipt({ id: 2, original_name: 'two.jpg' })]
+    render(<ReceiptPreviewModal receipts={two} initialIndex={99} onClose={vi.fn()} />)
+    await waitFor(() => expect(screen.getAllByText('two.jpg').length).toBeGreaterThan(0))
+  })
+
+  it('FE-BUDGET-PREVIEW-007: Escape closes the viewer and is stopped before the form behind it sees it', async () => {
+    const onClose = vi.fn()
+    const parent = vi.fn()
+    document.addEventListener('keydown', parent)
+    try {
+      render(<ReceiptPreviewModal receipts={[receipt()]} onClose={onClose} />)
+      await waitFor(() => expect(authUrl.get).toHaveBeenCalled())
+
+      fireEvent.keyDown(window, { key: 'Escape' })
+      expect(onClose).toHaveBeenCalledTimes(1)
+      // The expense form registers its own Escape handler on `document`; one
+      // keypress must not close both and throw away the user's edits.
+      expect(parent).not.toHaveBeenCalled()
+    } finally {
+      document.removeEventListener('keydown', parent)
+    }
+  })
+
+  it('FE-BUDGET-PREVIEW-008: an empty list renders nothing at all', () => {
+    render(<ReceiptPreviewModal receipts={[]} onClose={vi.fn()} />)
+    expect(document.body.querySelector('object')).toBeNull()
+    expect(screen.queryByRole('img')).toBeNull()
+  })
+})

client/src/components/Budget/ReceiptPreviewModal.tsx

@@ -45,12 +45,23 @@ export function ReceiptPreviewModal({ receipts, initialIndex = 0, onClose }: Rec
 
   useEffect(() => {
     const handleKey = (e: KeyboardEvent) => {
-      if (e.key === 'Escape') onClose()
+      if (e.key === 'Escape') {
+        // The preview opens on top of the expense form, and both the desktop
+        // Modal and the phone MSheet close themselves on Escape from a listener
+        // on `document`. Without this the one keypress closed the form too and
+        // threw away whatever the user had typed into it.
+        e.preventDefault()
+        e.stopPropagation()
+        onClose()
+        return
+      }
       if (e.key === 'ArrowLeft' && index > 0) setIndex(i => i - 1)
       if (e.key === 'ArrowRight' && index < receipts.length - 1) setIndex(i => i + 1)
     }
-    window.addEventListener('keydown', handleKey)
-    return () => window.removeEventListener('keydown', handleKey)
+    // Capture phase, so this runs before the listeners the parents put on
+    // `document`; stopPropagation on the bubble phase would be too late.
+    window.addEventListener('keydown', handleKey, true)
+    return () => window.removeEventListener('keydown', handleKey, true)
   }, [index, receipts.length, onClose])
 
   if (!current) return null
@@ -371,7 +382,7 @@ export function ReceiptPreviewModal({ receipts, initialIndex = 0, onClose }: Rec
               <button
                 type="button"
                 onClick={handleDownload}
-                className="bg-primary text-white"
+                className="bg-accent text-accent-text"
                 style={{
                   padding: '9px 18px',
                   borderRadius: 10,

client/src/components/Budget/receiptUploads.test.ts

@@ -0,0 +1,91 @@
+// FE-BUDGET-RECEIPT-001 to FE-BUDGET-RECEIPT-008: uploading receipts with an
+// expense, and taking the uploads back out when the expense fails to save.
+import { vi } from 'vitest'
+import { uploadReceipts, discardReceipts, saveWithReceipts } from './receiptUploads'
+
+const files = vi.hoisted(() => ({
+  upload: vi.fn(),
+  delete: vi.fn(),
+  permanentDelete: vi.fn(),
+}))
+
+vi.mock('../../api/client', () => ({ filesApi: files }))
+
+function file(name: string) {
+  return new File(['x'], name, { type: 'image/jpeg' })
+}
+
+beforeEach(() => {
+  files.upload.mockReset().mockImplementation(() => Promise.resolve({ file: { id: 7 } }))
+  files.delete.mockReset().mockResolvedValue({})
+  files.permanentDelete.mockReset().mockResolvedValue({})
+})
+
+describe('uploadReceipts', () => {
+  it('FE-BUDGET-RECEIPT-001: names the expense on the form data when there is one', async () => {
+    await uploadReceipts(3, [file('a.jpg')], 42)
+    const fd = files.upload.mock.calls[0][1] as FormData
+    expect(fd.get('budget_item_id')).toBe('42')
+  })
+
+  it('FE-BUDGET-RECEIPT-002: leaves it out for an expense that does not exist yet', async () => {
+    await uploadReceipts(3, [file('a.jpg')], null)
+    const fd = files.upload.mock.calls[0][1] as FormData
+    expect(fd.get('budget_item_id')).toBeNull()
+  })
+
+  it('FE-BUDGET-RECEIPT-003: returns the ids in order and skips a response without one', async () => {
+    files.upload
+      .mockResolvedValueOnce({ file: { id: 11 } })
+      .mockResolvedValueOnce({})
+      .mockResolvedValueOnce({ file: { id: 13 } })
+    expect(await uploadReceipts(3, [file('a'), file('b'), file('c')], null)).toEqual([11, 13])
+  })
+})
+
+describe('discardReceipts', () => {
+  it('FE-BUDGET-RECEIPT-004: trashes before it deletes, because the permanent route only accepts a trashed file', async () => {
+    const order: string[] = []
+    files.delete.mockImplementation(() => { order.push('delete'); return Promise.resolve({}) })
+    files.permanentDelete.mockImplementation(() => { order.push('permanent'); return Promise.resolve({}) })
+
+    expect(await discardReceipts(3, [9])).toEqual([])
+    expect(order).toEqual(['delete', 'permanent'])
+  })
+
+  it('FE-BUDGET-RECEIPT-005: reports the ids it could not remove instead of swallowing them', async () => {
+    files.delete.mockRejectedValueOnce(new Error('403'))
+    expect(await discardReceipts(3, [9, 10])).toEqual([9])
+  })
+})
+
+describe('saveWithReceipts', () => {
+  it('FE-BUDGET-RECEIPT-006: hands the uploaded ids to the save and returns its result', async () => {
+    files.upload.mockResolvedValueOnce({ file: { id: 5 } }).mockResolvedValueOnce({ file: { id: 6 } })
+    const save = vi.fn().mockResolvedValue('saved')
+    const out = await saveWithReceipts(3, [file('a'), file('b')], null, save)
+    expect(save).toHaveBeenCalledWith([5, 6])
+    expect(out.result).toBe('saved')
+    expect(files.delete).not.toHaveBeenCalled()
+  })
+
+  it('FE-BUDGET-RECEIPT-007: a failing save takes every upload back out again', async () => {
+    files.upload.mockResolvedValueOnce({ file: { id: 5 } }).mockResolvedValueOnce({ file: { id: 6 } })
+    const save = vi.fn().mockRejectedValue(new Error('nope'))
+
+    await expect(saveWithReceipts(3, [file('a'), file('b')], null, save)).rejects.toThrow('nope')
+    expect(files.delete.mock.calls.map(c => c[1])).toEqual([5, 6])
+    expect(files.permanentDelete.mock.calls.map(c => c[1])).toEqual([5, 6])
+  })
+
+  it('FE-BUDGET-RECEIPT-008: a failure partway through the upload rolls back what already went up, and says what stuck', async () => {
+    files.upload.mockResolvedValueOnce({ file: { id: 5 } }).mockRejectedValueOnce(new Error('offline'))
+    files.delete.mockRejectedValueOnce(new Error('403'))
+    const save = vi.fn()
+
+    const err = await saveWithReceipts(3, [file('a'), file('b')], null, save).catch(e => e)
+    expect(save).not.toHaveBeenCalled()
+    expect(files.delete.mock.calls.map(c => c[1])).toEqual([5])
+    expect(err.stuckReceiptIds).toEqual([5])
+  })
+})

client/src/components/Budget/receiptUploads.ts

@@ -0,0 +1,87 @@
+import { filesApi } from '../../api/client'
+
+/**
+ * Uploading receipts alongside an expense, and undoing it when the expense
+ * itself fails to save.
+ *
+ * One module for both shells. The desktop form and the phone sheet each had
+ * their own copy of this, three near-identical try/catch blocks apiece, which is
+ * six places for the same bug to sit and six copies against the duplication
+ * budget the PR gate measures.
+ */
+
+/** Upload each file and return the ids the server handed back, in order. */
+export async function uploadReceipts(
+  tripId: number | string,
+  files: readonly File[],
+  budgetItemId: number | null,
+): Promise<number[]> {
+  const ids: number[] = []
+  for (const file of files) {
+    const fd = new FormData()
+    fd.append('file', file)
+    // Only an existing expense can be named here. A new one has no id yet, so
+    // its receipts are linked by the create call instead.
+    if (budgetItemId != null) fd.append('budget_item_id', String(budgetItemId))
+    const res = await filesApi.upload(tripId, fd)
+    if (res?.file?.id) ids.push(res.file.id)
+  }
+  return ids
+}
+
+/**
+ * Undo an upload after the save it belonged to failed.
+ *
+ * Two calls per file, and the order matters: the permanent route only accepts a
+ * file that is already in the trash (`deleted_at IS NOT NULL`), so calling it on
+ * a freshly uploaded file answers 404 and the file stays on the trip. The soft
+ * delete puts it there first. It also drops the receipt link the upload created,
+ * so a failed edit does not leave the file attached to the expense.
+ *
+ * Returns the ids it could not remove. The caller is expected to say so rather
+ * than swallow it: a file left behind is a file the user has to find in the
+ * Files tab and delete by hand.
+ */
+export async function discardReceipts(
+  tripId: number | string,
+  ids: readonly number[],
+): Promise<number[]> {
+  const stuck: number[] = []
+  for (const id of ids) {
+    try {
+      await filesApi.delete(tripId, id)
+      await filesApi.permanentDelete(tripId, id)
+    } catch {
+      stuck.push(id)
+    }
+  }
+  return stuck
+}
+
+/**
+ * Upload the pending receipts, run the save, and take the uploads back out
+ * again if it throws.
+ *
+ * The rollback runs for a failure in the save AND for one partway through the
+ * upload loop, which is why the ids are collected outside the try.
+ */
+export async function saveWithReceipts<T>(
+  tripId: number | string,
+  files: readonly File[],
+  budgetItemId: number | null,
+  save: (uploadedIds: number[]) => Promise<T>,
+): Promise<{ result: T; stuckIds: number[] }> {
+  const uploaded: number[] = []
+  try {
+    for (const file of files) {
+      const [id] = await uploadReceipts(tripId, [file], budgetItemId)
+      if (id != null) uploaded.push(id)
+    }
+    const result = await save(uploaded)
+    return { result, stuckIds: [] }
+  } catch (err) {
+    const stuck = await discardReceipts(tripId, uploaded)
+    ;(err as { stuckReceiptIds?: number[] }).stuckReceiptIds = stuck
+    throw err
+  }
+}