From 584d6fe8e59d24895745d4188b521a749e0a3f8b Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 3 Aug 2026 20:28:07 -0400 Subject: [PATCH] feat(frontend): upload expenditure receipts for real FileUpload animated a progress bar over a setInterval that counted through the file's size in 15 ticks, with a standing TODO to use real progress. Nothing was transferred: the File went into component state and was dropped on submit, so every expenditure was recorded with receipt_url NULL despite the form refusing to submit without a receipt. Submitting the form now requests a presigned PUT from GET /expenditures/upload-url, sends the file to S3 with real byte progress, and passes the returned object URL to POST /expenditures as receiptUrl. If the upload fails, no expenditure is recorded and the error is shown. The upload runs on submit rather than on drop because the presigned URL is per project -- the project may not be chosen when the file lands -- and because a file dropped into an abandoned form would otherwise orphan an object in the bucket. FileUpload is now controlled: it reports the dropped file immediately and renders whatever progress its parent passes down. putWithProgress uses XHR because fetch exposes no upload progress, and sends the content type the URL was signed with rather than file.type, which S3 checks against the signature. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/app/components/AddExpenseModal.tsx | 32 +++- .../src/app/components/FileUpload.tsx | 63 +++----- apps/frontend/src/lib/upload.ts | 33 ++++ .../test/components/AddExpenseModal.test.tsx | 142 +++++++++++------- .../test/components/FileUpload.test.tsx | 48 ++++-- apps/frontend/test/lib/upload.test.ts | 121 +++++++++++++++ 6 files changed, 328 insertions(+), 111 deletions(-) create mode 100644 apps/frontend/src/lib/upload.ts create mode 100644 apps/frontend/test/lib/upload.test.ts diff --git a/apps/frontend/src/app/components/AddExpenseModal.tsx b/apps/frontend/src/app/components/AddExpenseModal.tsx index 15dc707b..4888b90d 100644 --- a/apps/frontend/src/app/components/AddExpenseModal.tsx +++ b/apps/frontend/src/app/components/AddExpenseModal.tsx @@ -4,9 +4,12 @@ import { useState } from 'react'; import { Button, Dialog, Portal, CloseButton, Stack } from '@chakra-ui/react'; import DropdownSelector from './DropdownSelector'; import { useApi } from '@/hooks/useApi'; -import FileUpload from './FileUpload'; +import FileUpload, { UploadProgress } from './FileUpload'; +import { putWithProgress } from '@/lib/upload'; import { FiDollarSign } from 'react-icons/fi'; import { Project } from '@/types'; + +const RECEIPT_CONTENT_TYPE = 'application/pdf'; interface AddExpenseModalProps { open: boolean; onClose: () => void; @@ -38,6 +41,8 @@ export default function AddExpenseModal({ const [projectError, setProjectError] = useState(false); const [submitError, setSubmitError] = useState(null); const [fileError, setFileError] = useState(null); + const [uploadProgress, setUploadProgress] = useState(null); + const [isSubmitting, setIsSubmitting] = useState(false); function resetForm() { setNewDate(''); @@ -45,6 +50,9 @@ export default function AddExpenseModal({ setNewDescription(''); setNewAmount(''); setNewProject(''); + setNewFile(null); + setUploadProgress(null); + setIsSubmitting(false); setDateError(false); setTypeError(false); setDescError(false); @@ -83,19 +91,38 @@ export default function AddExpenseModal({ return; } + setSubmitError(null); + setIsSubmitting(true); try { + // The receipt goes to S3 first: POST /expenditures stores the resulting + // object URL, so there is nothing to record until the file has landed. + const { uploadUrl, objectUrl } = await api.get<{ uploadUrl: string; objectUrl: string }>( + `/expenditures/upload-url?fileName=${encodeURIComponent(newFile.name)}` + + `&projectId=${selectedProject.project_id}`, + ); + + setUploadProgress({ transferredBytes: 0, totalBytes: newFile.size, fileName: newFile.name }); + await putWithProgress(uploadUrl, newFile, RECEIPT_CONTENT_TYPE, (transferredBytes) => + setUploadProgress({ transferredBytes, totalBytes: newFile.size, fileName: newFile.name }), + ); + setUploadProgress(null); + await api.post('/expenditures', { projectID: selectedProject.project_id, amount: Number(newAmount), category: newType, description: newDescription, spentOn: newDate, + receiptUrl: objectUrl, }); resetForm(); onSuccess(); } catch (err) { + setUploadProgress(null); setSubmitError(err instanceof Error ? err.message : 'Failed to create expense'); + } finally { + setIsSubmitting(false); } } @@ -304,6 +331,7 @@ export default function AddExpenseModal({ setFileError(null); }} onReject={() => setFileError('File type not supported')} + progress={uploadProgress} /> {fileError && ( @@ -332,6 +360,8 @@ export default function AddExpenseModal({ backgroundColor="var(--color-core-green)" color="var(--color-core-white)" onClick={handleSubmit} + loading={isSubmitting} + disabled={isSubmitting} > Submit For Review diff --git a/apps/frontend/src/app/components/FileUpload.tsx b/apps/frontend/src/app/components/FileUpload.tsx index d83390de..32719f39 100644 --- a/apps/frontend/src/app/components/FileUpload.tsx +++ b/apps/frontend/src/app/components/FileUpload.tsx @@ -1,51 +1,32 @@ 'use client'; -import { useCallback, useRef, useState } from 'react'; +import { useCallback } from 'react'; import { FileRejection, useDropzone } from 'react-dropzone'; import UploadProgressBar from './UploadProgressBar'; import FilePreview from './FilePreview'; const MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024; // 10MB +export interface UploadProgress { + transferredBytes: number; + totalBytes: number; + fileName: string; +} + interface FileUploadProps { value: File | null; onChange: (file: File | null) => void; onReject?: () => void; + /** + * Live progress of the real upload, driven by the parent. The file is sent + * when the form is submitted -- not on drop -- because the presigned URL is + * per project, and because a file dropped into an abandoned form would + * otherwise leave an orphaned object in the bucket. + */ + progress?: UploadProgress | null; } -export default function FileUpload({ value, onChange, onReject }: FileUploadProps) { - const [isUploading, setIsUploading] = useState(false); - const [transferredBytes, setTransferredBytes] = useState(0); - const [pendingFile, setPendingFile] = useState(null); - const intervalRef = useRef | null>(null); - - {/*Simulated progress for testing - TODO: update to use real progress of uploaded file */} - const simulateUpload = useCallback( - (file: File) => { - setPendingFile(file); - setIsUploading(true); - setTransferredBytes(0); - - const total = file.size; - const step = total / 15; // ~15 ticks to finish - let transferred = 0; - - intervalRef.current = setInterval(() => { - transferred += step; - if (transferred >= total) { - transferred = total; - if (intervalRef.current) clearInterval(intervalRef.current); - setTransferredBytes(total); - setIsUploading(false); - setPendingFile(null); - onChange(file); // flip to selected state - } else { - setTransferredBytes(transferred); - } - }, 100); - }, - [onChange], - ); +export default function FileUpload({ value, onChange, onReject, progress }: FileUploadProps) { + const isUploading = progress !== null && progress !== undefined; {/*When the file is dropped, check if it's accepted*/} const onDrop = useCallback( @@ -54,9 +35,9 @@ export default function FileUpload({ value, onChange, onReject }: FileUploadProp onReject?.(); return; } - if (accepted[0]) simulateUpload(accepted[0]); + if (accepted[0]) onChange(accepted[0]); }, - [simulateUpload, onReject], + [onChange, onReject], ); {/*Dropzone component to allow user to drop in files*/} @@ -73,12 +54,12 @@ export default function FileUpload({ value, onChange, onReject }: FileUploadProp ? 'var(--color-core-green)' : 'var(--color-black-200)'; - if (isUploading && pendingFile) { + if (isUploading) { return ( ); } diff --git a/apps/frontend/src/lib/upload.ts b/apps/frontend/src/lib/upload.ts new file mode 100644 index 00000000..34efa3f4 --- /dev/null +++ b/apps/frontend/src/lib/upload.ts @@ -0,0 +1,33 @@ +/** + * Uploads a file to a presigned S3 PUT, reporting real byte progress. + * + * XHR rather than `fetch`, which exposes no upload progress. The request is + * deliberately unauthenticated: the signature is in the URL, and an extra + * Authorization header would not be part of what was signed. + */ +export function putWithProgress( + uploadUrl: string, + file: File, + contentType: string, + onProgress: (transferredBytes: number) => void, +): Promise { + return new Promise((resolve, reject) => { + const xhr = new XMLHttpRequest(); + xhr.open('PUT', uploadUrl); + // Must match the ContentType the URL was signed with, or S3 rejects the + // signature -- so it comes from the caller, not from `file.type`. + xhr.setRequestHeader('Content-Type', contentType); + + xhr.upload.addEventListener('progress', (event) => { + if (event.lengthComputable) onProgress(event.loaded); + }); + xhr.addEventListener('load', () => { + if (xhr.status >= 200 && xhr.status < 300) resolve(); + else reject(new Error(`Upload failed (${xhr.status})`)); + }); + xhr.addEventListener('error', () => reject(new Error('Upload failed'))); + xhr.addEventListener('abort', () => reject(new Error('Upload cancelled'))); + + xhr.send(file); + }); +} diff --git a/apps/frontend/test/components/AddExpenseModal.test.tsx b/apps/frontend/test/components/AddExpenseModal.test.tsx index bfb55d8c..65104e19 100644 --- a/apps/frontend/test/components/AddExpenseModal.test.tsx +++ b/apps/frontend/test/components/AddExpenseModal.test.tsx @@ -1,12 +1,33 @@ import { render, screen, fireEvent, waitFor } from '../utils'; import AddExpenseModal from '@/app/components/AddExpenseModal'; import { authedFetch as apiFetch } from '@/lib/authClient'; +import { putWithProgress } from '@/lib/upload'; jest.mock('../../src/lib/authClient', () => ({ ...jest.requireActual('../../src/lib/authClient'), authedFetch: jest.fn(), })); +jest.mock('../../src/lib/upload', () => ({ + putWithProgress: jest.fn(), +})); + +const mockPut = putWithProgress as jest.Mock; + +const UPLOAD_URL = '/expenditures/upload-url?fileName=receipt.pdf&projectId=2'; +const OBJECT_URL = 'https://bucket.s3.us-east-2.amazonaws.com/receipts/2/1-receipt.pdf'; + +/** GET /expenditures/upload-url resolves, POST /expenditures resolves. */ +function mockHappyPath() { + (apiFetch as jest.Mock).mockImplementation((path: string) => { + if (path.startsWith('/expenditures/upload-url')) { + return Promise.resolve({ uploadUrl: 'https://signed.example/put', objectUrl: OBJECT_URL }); + } + return Promise.resolve({}); + }); + mockPut.mockResolvedValue(undefined); +} + // Mock DropdownSelector — render a simple native select so we can drive it jest.mock('../../src/app/components/DropdownSelector', () => { return function MockDropdownSelector({ @@ -72,6 +93,26 @@ beforeEach(() => { jest.clearAllMocks(); }); +/** Fills every required field with a valid value, including the receipt. */ +function fillValidForm() { + fireEvent.change(document.querySelector('input[type="date"]')!, { + target: { value: '2025-05-15' }, + }); + fireEvent.change(screen.getByPlaceholderText('Enter the Amount'), { + target: { value: '12000' }, + }); + fireEvent.change(screen.getByLabelText('Select type'), { + target: { value: 'Travel Foreign' }, + }); + fireEvent.change(screen.getByLabelText('Select a project'), { + target: { value: 'Project Name 2' }, + }); + fireEvent.change(screen.getByPlaceholderText('Placeholder'), { + target: { value: 'A test description' }, + }); + fireEvent.click(screen.getByText('mock-select-file')); +} + describe('AddExpenseModal Component', () => { it('renders the modal title when open', () => { render(); @@ -121,27 +162,10 @@ describe('AddExpenseModal Component', () => { expect(apiFetch).not.toHaveBeenCalled(); }); - it('calls apiFetch with the correct payload when the form is valid', async () => { - (apiFetch as jest.Mock).mockResolvedValueOnce({}); + it('uploads the receipt and posts its object URL as receiptUrl', async () => { + mockHappyPath(); render(); - - fireEvent.change(document.querySelector('input[type="date"]')!, { - target: { value: '2025-05-15' }, - }); - fireEvent.change(screen.getByPlaceholderText('Enter the Amount'), { - target: { value: '12000' }, - }); - fireEvent.change(screen.getByLabelText('Select type'), { - target: { value: 'Travel Foreign' }, - }); - fireEvent.change(screen.getByLabelText('Select a project'), { - target: { value: 'Project Name 2' }, - }); - fireEvent.change(screen.getByPlaceholderText('Placeholder'), { - target: { value: 'A test description' }, - }); - fireEvent.click(screen.getByText('mock-select-file')); - + fillValidForm(); fireEvent.click(screen.getByText('Submit For Review')); await waitFor(() => { @@ -155,32 +179,56 @@ describe('AddExpenseModal Component', () => { category: 'Travel Foreign', description: 'A test description', spentOn: '2025-05-15', + receiptUrl: OBJECT_URL, }), }), ); }); + + // The URL is requested for the chosen project and the dropped filename. + expect(apiFetch).toHaveBeenCalledWith(UPLOAD_URL, { method: 'GET' }); + + // The PUT must be signed for the same content type the backend signed. + expect(mockPut).toHaveBeenCalledWith( + 'https://signed.example/put', + expect.any(File), + 'application/pdf', + expect.any(Function), + ); }); - it('calls onSuccess after a successful submit', async () => { - (apiFetch as jest.Mock).mockResolvedValueOnce({}); + it('uploads the file before recording the expenditure', async () => { + mockHappyPath(); render(); + fillValidForm(); + fireEvent.click(screen.getByText('Submit For Review')); - fireEvent.change(document.querySelector('input[type="date"]')!, { - target: { value: '2025-05-15' }, - }); - fireEvent.change(screen.getByPlaceholderText('Enter the Amount'), { - target: { value: '12000' }, - }); - fireEvent.change(screen.getByLabelText('Select type'), { - target: { value: 'Travel Foreign' }, - }); - fireEvent.change(screen.getByLabelText('Select a project'), { - target: { value: 'Project Name 2' }, - }); - fireEvent.change(screen.getByPlaceholderText('Placeholder'), { - target: { value: 'A test description' }, + await waitFor(() => expect(baseProps.onSuccess).toHaveBeenCalled()); + + const postCall = (apiFetch as jest.Mock).mock.invocationCallOrder[ + (apiFetch as jest.Mock).mock.calls.findIndex(([path]) => path === '/expenditures') + ]; + expect(mockPut.mock.invocationCallOrder[0]).toBeLessThan(postCall); + }); + + it('does not record an expenditure when the receipt upload fails', async () => { + mockHappyPath(); + mockPut.mockRejectedValueOnce(new Error('Upload failed (403)')); + render(); + fillValidForm(); + fireEvent.click(screen.getByText('Submit For Review')); + + await waitFor(() => { + expect(screen.getByText('Upload failed (403)')).toBeInTheDocument(); }); - fireEvent.click(screen.getByText('mock-select-file')); + expect(apiFetch).not.toHaveBeenCalledWith('/expenditures', expect.anything()); + expect(baseProps.onSuccess).not.toHaveBeenCalled(); + }); + + it('calls onSuccess after a successful submit', async () => { + mockHappyPath(); + render(); + fillValidForm(); fireEvent.click(screen.getByText('Submit For Review')); await waitFor(() => { @@ -189,25 +237,9 @@ describe('AddExpenseModal Component', () => { }); it('shows a submit error if apiFetch rejects', async () => { - (apiFetch as jest.Mock).mockRejectedValueOnce(new Error('Server exploded')); + (apiFetch as jest.Mock).mockRejectedValue(new Error('Server exploded')); render(); - - fireEvent.change(document.querySelector('input[type="date"]')!, { - target: { value: '2025-05-15' }, - }); - fireEvent.change(screen.getByPlaceholderText('Enter the Amount'), { - target: { value: '12000' }, - }); - fireEvent.change(screen.getByLabelText('Select type'), { - target: { value: 'Travel Foreign' }, - }); - fireEvent.change(screen.getByLabelText('Select a project'), { - target: { value: 'Project Name 2' }, - }); - fireEvent.change(screen.getByPlaceholderText('Placeholder'), { - target: { value: 'A test description' }, - }); - fireEvent.click(screen.getByText('mock-select-file')); + fillValidForm(); fireEvent.click(screen.getByText('Submit For Review')); await waitFor(() => { diff --git a/apps/frontend/test/components/FileUpload.test.tsx b/apps/frontend/test/components/FileUpload.test.tsx index 4ddc803e..82e8b817 100644 --- a/apps/frontend/test/components/FileUpload.test.tsx +++ b/apps/frontend/test/components/FileUpload.test.tsx @@ -41,32 +41,52 @@ describe('FileUpload Component', () => { expect(screen.getByText('Upload Complete')).toBeInTheDocument(); }); - it('shows the upload progress bar after a valid file is dropped', () => { - jest.useFakeTimers(); - render( {}} />); + it('calls onChange as soon as a valid file is dropped', () => { + const onChange = jest.fn(); + render(); act(() => { capturedOnDrop!([makePdf()], []); }); - expect(screen.getByText(/uploading\.\.\./)).toBeInTheDocument(); - jest.useRealTimers(); + expect(onChange.mock.calls[0][0].name).toBe('receipt.pdf'); }); - it('calls onChange with the file once the simulated upload completes', () => { - jest.useFakeTimers(); - const onChange = jest.fn(); - render(); + it('shows no progress bar on drop — the upload happens on submit', () => { + render( {}} />); act(() => { capturedOnDrop!([makePdf()], []); }); - act(() => { - jest.advanceTimersByTime(2000); - }); - expect(onChange.mock.calls[0][0].name).toBe('receipt.pdf'); - jest.useRealTimers(); + expect(screen.queryByText(/uploading\.\.\./)).not.toBeInTheDocument(); + }); + + it('renders the real transferred bytes when the parent reports progress', () => { + render( + {}} + progress={{ transferredBytes: 512, totalBytes: 2048, fileName: 'receipt.pdf' }} + />, + ); + + expect(screen.getByText(/receipt\.pdf uploading\.\.\./)).toBeInTheDocument(); + expect(screen.getByText('25%')).toBeInTheDocument(); + }); + + it('returns to the preview once progress clears', () => { + const { rerender } = render( + {}} + progress={{ transferredBytes: 2048, totalBytes: 2048, fileName: 'receipt.pdf' }} + />, + ); + expect(screen.getByText('100%')).toBeInTheDocument(); + + rerender( {}} progress={null} />); + expect(screen.getByText('Upload Complete')).toBeInTheDocument(); }); it('calls onReject when a rejected file is dropped', () => { diff --git a/apps/frontend/test/lib/upload.test.ts b/apps/frontend/test/lib/upload.test.ts new file mode 100644 index 00000000..61d0427c --- /dev/null +++ b/apps/frontend/test/lib/upload.test.ts @@ -0,0 +1,121 @@ +import { putWithProgress } from '@/lib/upload'; + +/** Minimal XMLHttpRequest stand-in; jsdom's does not run in tests. */ +class MockXhr { + static last: MockXhr; + + status = 200; + method = ''; + url = ''; + headers: Record = {}; + body: unknown = null; + + private listeners: Record void>> = {}; + upload = { + listeners: [] as Array<(event: ProgressEvent) => void>, + addEventListener(_type: string, handler: (event: ProgressEvent) => void) { + this.listeners.push(handler); + }, + }; + + constructor() { + MockXhr.last = this; + } + + open(method: string, url: string) { + this.method = method; + this.url = url; + } + setRequestHeader(key: string, value: string) { + this.headers[key] = value; + } + addEventListener(type: string, handler: () => void) { + (this.listeners[type] ??= []).push(handler); + } + send(body: unknown) { + this.body = body; + } + + emit(type: string) { + this.listeners[type]?.forEach((handler) => handler()); + } + emitProgress(loaded: number, lengthComputable = true) { + this.upload.listeners.forEach((handler) => + handler({ loaded, lengthComputable } as ProgressEvent), + ); + } +} + +beforeEach(() => { + (global as unknown as { XMLHttpRequest: unknown }).XMLHttpRequest = MockXhr; +}); + +function pdf() { + return new File(['x'], 'receipt.pdf', { type: 'application/pdf' }); +} + +describe('putWithProgress', () => { + it('PUTs the file to the presigned URL with the signed content type', async () => { + const promise = putWithProgress('https://signed.example/put', pdf(), 'application/pdf', () => {}); + MockXhr.last.emit('load'); + await promise; + + expect(MockXhr.last.method).toBe('PUT'); + expect(MockXhr.last.url).toBe('https://signed.example/put'); + expect(MockXhr.last.headers['Content-Type']).toBe('application/pdf'); + expect(MockXhr.last.body).toBeInstanceOf(File); + }); + + it('does not send an Authorization header — the signature is in the URL', async () => { + const promise = putWithProgress('https://signed.example/put', pdf(), 'application/pdf', () => {}); + MockXhr.last.emit('load'); + await promise; + + expect(MockXhr.last.headers.Authorization).toBeUndefined(); + }); + + it('reports transferred bytes as they arrive', async () => { + const onProgress = jest.fn(); + const promise = putWithProgress('https://signed.example/put', pdf(), 'application/pdf', onProgress); + + MockXhr.last.emitProgress(256); + MockXhr.last.emitProgress(1024); + MockXhr.last.emit('load'); + await promise; + + expect(onProgress.mock.calls).toEqual([[256], [1024]]); + }); + + it('ignores progress events with no computable length', async () => { + const onProgress = jest.fn(); + const promise = putWithProgress('https://signed.example/put', pdf(), 'application/pdf', onProgress); + + MockXhr.last.emitProgress(256, false); + MockXhr.last.emit('load'); + await promise; + + expect(onProgress).not.toHaveBeenCalled(); + }); + + it('rejects on a non-2xx response, carrying the status', async () => { + const promise = putWithProgress('https://signed.example/put', pdf(), 'application/pdf', () => {}); + MockXhr.last.status = 403; + MockXhr.last.emit('load'); + + await expect(promise).rejects.toThrow('Upload failed (403)'); + }); + + it('rejects on a network error', async () => { + const promise = putWithProgress('https://signed.example/put', pdf(), 'application/pdf', () => {}); + MockXhr.last.emit('error'); + + await expect(promise).rejects.toThrow('Upload failed'); + }); + + it('rejects when the upload is aborted', async () => { + const promise = putWithProgress('https://signed.example/put', pdf(), 'application/pdf', () => {}); + MockXhr.last.emit('abort'); + + await expect(promise).rejects.toThrow('Upload cancelled'); + }); +});