From 97850ddfdcdde933794179eb8bdb9a2e04c5b57c Mon Sep 17 00:00:00 2001 From: Santiago Palenque Date: Thu, 27 Aug 2026 16:58:57 -0300 Subject: [PATCH 1/7] chore: Fix chunk upload error handling - release timeout slot, dedupe rows, surface status, route polling failures to error UI --- .../dropzone/__tests__/dropzone.test.js | 147 +++++++++++++++++- src/components/inputs/dropzone/index.js | 18 ++- .../__tests__/upload-input-v3.test.js | 15 ++ .../inputs/upload-input-v3/index.js | 7 +- 4 files changed, 180 insertions(+), 7 deletions(-) diff --git a/src/components/inputs/dropzone/__tests__/dropzone.test.js b/src/components/inputs/dropzone/__tests__/dropzone.test.js index 9a1c2497..58630e70 100644 --- a/src/components/inputs/dropzone/__tests__/dropzone.test.js +++ b/src/components/inputs/dropzone/__tests__/dropzone.test.js @@ -31,13 +31,22 @@ let mockCapturedOptions = {}; jest.mock('dropzone', () => { return jest.fn().mockImplementation((element, options) => { mockCapturedOptions = options; - return { + const dz = { options, on: jest.fn(), off: jest.fn(), destroy: jest.fn(() => null), getActiveFiles: jest.fn(() => []) }; + // Mimics Dropzone's real Emitter: replays every handler registered via `on` + // for that event, in registration order - the same "multiple listeners on + // one event" behavior the ontimeout/pollUploadStatus fixes rely on. + dz.emit = jest.fn((event, ...args) => { + dz.on.mock.calls + .filter(([evt]) => evt === event) + .forEach(([, handler]) => handler(...args)); + }); + return dz; }); }); @@ -515,6 +524,142 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { done(); }, 10); }); + + /** + * Test Case 10: a chunk that times out releases its concurrency slot + * + * Dropzone's own default xhr.ontimeout (set before the 'sending' handler runs) + * still fires 'error', but without wrapping it here onChunkComplete() never + * runs, so chunksInFlight never decrements and later chunks stay queued forever. + */ + test('test_dropzone_ontimeout_releases_chunk_slot', () => { + const ref = React.createRef(); + + render( + + ); + + const instance = ref.current; + const mockFile = { name: 'test.pdf', size: 1024000 }; + const dropzoneOnTimeout = jest.fn(); + const mockXhr = { + readyState: XMLHttpRequest.DONE, + setRequestHeader: jest.fn(), + onload: jest.fn(), + onerror: jest.fn(), + ontimeout: dropzoneOnTimeout, + abort: jest.fn() + }; + + instance.chunksInFlight = 1; + getEventHandler(instance, 'sending')(mockFile, mockXhr, { append: jest.fn() }); + + mockXhr.ontimeout({}); + + expect(instance.chunksInFlight).toBe(0); + // Dropzone's own timeout handling (which still reports the error) must still run. + expect(dropzoneOnTimeout).toHaveBeenCalledTimes(1); + }); + + /** + * Test Cases 11-13: pollUploadStatus's three failure branches route through the + * file-level error channel (dropzone.emit('error', file, message)) instead of + * calling onError directly, so the row clears and the consumer is told exactly once. + */ + test('test_dropzone_poll_timeout_emits_a_string_message_and_calls_onError_once', async () => { + jest.useFakeTimers({ doNotFake: ['queueMicrotask'] }); + global.fetch = jest.fn(() => + Promise.resolve({ json: () => Promise.resolve({ status: 'uploading' }) }) + ); + + const ref = React.createRef(); + render( + + ); + + const instance = ref.current; + const mockFile = { name: 'big.pdf', size: 1024000 }; + instance.pollUploadStatus('file-timeout', 'https://example.com/upload', mockFile); + + // maxAttempts is 300 at 2s/tick - advance one tick past the ceiling. + await jest.advanceTimersByTimeAsync(2000 * 301); + + expect(onErrorMock).toHaveBeenCalledTimes(1); + const [message] = onErrorMock.mock.calls[0]; + expect(typeof message).toBe('string'); + expect(message).toBe('Upload timed out'); + + jest.useRealTimers(); + }, 20000); + + test('test_dropzone_poll_server_error_status_emits_readable_message_not_object', (done) => { + global.fetch = jest.fn(() => + Promise.resolve({ + json: () => Promise.resolve({ status: 'error', message: 'processing failed' }) + }) + ); + + const ref = React.createRef(); + render( + + ); + + setTimeout(() => { + const instance = ref.current; + const mockFile = { name: 'test.pdf', size: 1024000 }; + instance.pollUploadStatus('file-server-error', 'https://example.com/upload', mockFile); + + setTimeout(() => { + expect(onErrorMock).toHaveBeenCalledTimes(1); + const [message] = onErrorMock.mock.calls[0]; + expect(message).toBe('processing failed'); + expect(message).not.toBe('[object Object]'); + done(); + }, 2500); + }, 10); + }, 10000); + + test('test_dropzone_poll_fetch_rejection_emits_readable_message_and_calls_onError_once', (done) => { + global.fetch = jest.fn(() => Promise.reject(new Error('network down'))); + + const ref = React.createRef(); + render( + + ); + + setTimeout(() => { + const instance = ref.current; + const mockFile = { name: 'test.pdf', size: 1024000 }; + instance.pollUploadStatus('file-network-error', 'https://example.com/upload', mockFile); + + setTimeout(() => { + expect(onErrorMock).toHaveBeenCalledTimes(1); + const [message] = onErrorMock.mock.calls[0]; + expect(message).toBe('network down'); + done(); + }, 2500); + }, 10); + }, 10000); }); describe('DropzoneJS - Progress Bar Monotonicity', () => { diff --git a/src/components/inputs/dropzone/index.js b/src/components/inputs/dropzone/index.js index d619fc1e..c1bfb85e 100644 --- a/src/components/inputs/dropzone/index.js +++ b/src/components/inputs/dropzone/index.js @@ -106,7 +106,7 @@ export class DropzoneJS extends React.Component { attempts++; if (attempts > maxAttempts) { this.stopPolling(file); - this.onError({ message: 'Upload timed out' }); + this.dropzone.emit('error', file, 'Upload timed out'); return; } try { @@ -131,11 +131,11 @@ export class DropzoneJS extends React.Component { this.onUploadComplete(data); } else if (data.status === 'error') { this.stopPolling(file); - this.onError(data); + this.dropzone.emit('error', file, data.message || data.error || 'Upload failed'); } } catch (error) { this.stopPolling(file); - this.onError(error); + this.dropzone.emit('error', file, error.message || 'Network error'); } }, 2000); @@ -494,11 +494,19 @@ export class DropzoneJS extends React.Component { _this.onChunkComplete(); if (dropzoneOnError) dropzoneOnError(e); } + + // Without this wrapper a timed-out chunk never releases its concurrency slot. + let dropzoneOnTimeout = xhr.ontimeout; + xhr.ontimeout = function(e) { + _this.onChunkComplete(); + if (dropzoneOnTimeout) dropzoneOnTimeout(e); + } }) - this.dropzone.on('error', (file, message) => { + // xhr.status is 0 for a transport failure, vs a real non-2xx server response. + this.dropzone.on('error', (file, message, xhr) => { console.log(`DropzoneJS::error`, message); - this.onError(message); + this.onError(message, xhr?.status); }); } diff --git a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js index 59f3b058..a2f2bc3a 100644 --- a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js +++ b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js @@ -246,6 +246,21 @@ describe('UploadInputV3', () => { expect(screen.getByText(/File is too big/)).toBeInTheDocument(); }); + test('two failed chunks for the same file collapse into a single error row', () => { + render(); + act(() => { + dropzoneCallbacks.onAddedFile({ name: 'big-file.png', size: 9999999 }); + }); + act(() => { + dropzoneCallbacks.onFileError({ name: 'big-file.png', size: 9999999 }, 'Server responded with 0 code.'); + }); + act(() => { + dropzoneCallbacks.onFileError({ name: 'big-file.png', size: 9999999 }, 'Server responded with 0 code.'); + }); + expect(screen.getAllByText('big-file.png')).toHaveLength(1); + expect(screen.getAllByText('Server responded with 0 code.')).toHaveLength(1); + }); + test('dismissing an error removes it from the view and restores the dropzone', () => { const { container } = render(); act(() => { diff --git a/src/components/inputs/upload-input-v3/index.js b/src/components/inputs/upload-input-v3/index.js index 8e2d474c..eecbf12e 100644 --- a/src/components/inputs/upload-input-v3/index.js +++ b/src/components/inputs/upload-input-v3/index.js @@ -217,7 +217,12 @@ const UploadInputV3 = ({ // Dropzone module being loaded here; _userCanceled also covers files Dropzone never got // to mark, such as one removed before its upload reached the UPLOADING state. if (file._userCanceled || file.status === 'canceled') return; - setErrorFiles(prev => [...prev, { name: file.name, size: file.size, message }]); + setErrorFiles(prev => { + const existingIndex = prev.findIndex(f => f.name === file.name && f.size === file.size); + const entry = { name: file.name, size: file.size, message }; + if (existingIndex === -1) return [...prev, entry]; + return prev.map((f, i) => (i === existingIndex ? entry : f)); + }); }, []); const handleDismissError = useCallback((file) => { From 1aa13e91aee1b62354510964e6be33149fe97d4e Mon Sep 17 00:00:00 2001 From: Santiago Palenque Date: Thu, 27 Aug 2026 17:37:08 -0300 Subject: [PATCH 2/7] chore: pass xdr status to onError --- .../__tests__/dropzone-v3.test.js | 40 +++++++++++++++++++ .../__tests__/upload-input-v3.test.js | 24 +++++++++-- .../inputs/upload-input-v3/dropzone-v3.js | 6 +-- .../inputs/upload-input-v3/index.js | 19 +++++---- src/i18n/en.json | 3 +- 5 files changed, 78 insertions(+), 14 deletions(-) diff --git a/src/components/inputs/upload-input-v3/__tests__/dropzone-v3.test.js b/src/components/inputs/upload-input-v3/__tests__/dropzone-v3.test.js index 3bdae599..a9042f8b 100644 --- a/src/components/inputs/upload-input-v3/__tests__/dropzone-v3.test.js +++ b/src/components/inputs/upload-input-v3/__tests__/dropzone-v3.test.js @@ -90,3 +90,43 @@ describe('DropzoneV3 - uploadprogress to React bridging', () => { expect(onUploadProgress).toHaveBeenCalledWith(file, 40); }); }); + +describe('DropzoneV3 - error status forwarding', () => { + beforeEach(() => { + capturedEventHandlers = null; + }); + + test('forwards the xhr status from the error event to onFileError', () => { + const onFileError = jest.fn(); + render( + + ); + + const file = { name: 'video.mp4' }; + capturedEventHandlers.error(file, 'Server responded with 0 code.', { status: 0 }); + + expect(onFileError).toHaveBeenCalledWith(file, 'Server responded with 0 code.', 0); + }); + + test('passes undefined status when Dropzone emits error without an xhr (e.g. client-side validation)', () => { + const onFileError = jest.fn(); + render( + + ); + + const file = { name: 'huge.mp4' }; + capturedEventHandlers.error(file, 'File is too big.'); + + expect(onFileError).toHaveBeenCalledWith(file, 'File is too big.', undefined); + }); +}); diff --git a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js index a2f2bc3a..b8c84691 100644 --- a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js +++ b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js @@ -252,13 +252,31 @@ describe('UploadInputV3', () => { dropzoneCallbacks.onAddedFile({ name: 'big-file.png', size: 9999999 }); }); act(() => { - dropzoneCallbacks.onFileError({ name: 'big-file.png', size: 9999999 }, 'Server responded with 0 code.'); + dropzoneCallbacks.onFileError({ name: 'big-file.png', size: 9999999 }, 'Server responded with 0 code.', 0); }); act(() => { - dropzoneCallbacks.onFileError({ name: 'big-file.png', size: 9999999 }, 'Server responded with 0 code.'); + dropzoneCallbacks.onFileError({ name: 'big-file.png', size: 9999999 }, 'Server responded with 0 code.', 0); }); expect(screen.getAllByText('big-file.png')).toHaveLength(1); - expect(screen.getAllByText('Server responded with 0 code.')).toHaveLength(1); + expect(screen.queryByText('Server responded with 0 code.')).not.toBeInTheDocument(); + expect(screen.getAllByText('Upload interrupted by a connection error. Please retry.')).toHaveLength(1); + }); + + test('a status-0 connection failure shows a readable message instead of the raw "Server responded with 0 code."', () => { + render(); + act(() => { + dropzoneCallbacks.onFileError({ name: 'video.mp4', size: 9999999 }, 'Server responded with 0 code.', 0); + }); + expect(screen.getByText('Upload interrupted by a connection error. Please retry.')).toBeInTheDocument(); + expect(screen.queryByText('Server responded with 0 code.')).not.toBeInTheDocument(); + }); + + test('a real non-zero-status server error still shows the server-provided message', () => { + render(); + act(() => { + dropzoneCallbacks.onFileError({ name: 'video.mp4', size: 9999999 }, 'File type not allowed', 415); + }); + expect(screen.getByText('File type not allowed')).toBeInTheDocument(); }); test('dismissing an error removes it from the view and restores the dropzone', () => { diff --git a/src/components/inputs/upload-input-v3/dropzone-v3.js b/src/components/inputs/upload-input-v3/dropzone-v3.js index c38495b7..f351d770 100644 --- a/src/components/inputs/upload-input-v3/dropzone-v3.js +++ b/src/components/inputs/upload-input-v3/dropzone-v3.js @@ -63,9 +63,9 @@ export const DropzoneV3 = ({ if (onFileCompleted) onFileCompleted(file); if (eventHandlers.success) eventHandlers.success(file); }, - error: (file, message) => { - if (onFileError) onFileError(file, message); - if (eventHandlers.error) eventHandlers.error(file, message); + error: (file, message, xhr) => { + if (onFileError) onFileError(file, message, xhr?.status); + if (eventHandlers.error) eventHandlers.error(file, message, xhr?.status); }, }; diff --git a/src/components/inputs/upload-input-v3/index.js b/src/components/inputs/upload-input-v3/index.js index eecbf12e..24cf04cd 100644 --- a/src/components/inputs/upload-input-v3/index.js +++ b/src/components/inputs/upload-input-v3/index.js @@ -205,21 +205,26 @@ const UploadInputV3 = ({ })); }, [value]); - const handleFileError = useCallback((file, message) => { + const handleFileError = useCallback((file, message, status) => { setUploadingFiles(prev => { const entry = prev.find(f => f.name === file.name && f.size === file.size); if (entry?.previewUrl) URL.revokeObjectURL(entry.previewUrl); return prev.filter(f => !(f.name === file.name && f.size === file.size)); }); - // Dropzone turns a cancelled upload into an error carrying dictUploadCanceled. A cancel - // is not a failure to report back to the user - the row just goes away. 'canceled' is the - // value of Dropzone.CANCELED, matched as a literal so this does not depend on the - // Dropzone module being loaded here; _userCanceled also covers files Dropzone never got - // to mark, such as one removed before its upload reached the UPLOADING state. + // 'canceled' is the value of Dropzone.CANCELED, matched as a literal so this does not depend on the + // Dropzone module being loaded here; _userCanceled is when removed before its upload reached the UPLOADING state. if (file._userCanceled || file.status === 'canceled') return; + + // status 0 means the request never got a real server response (connection dropped/timed + // out) - Dropzone's own message for that case is "Server responded with 0 code.", which + // is not something a user can act on. + const displayMessage = status === 0 + ? T.translate('upload_input_v3.network_error') + : message; + setErrorFiles(prev => { const existingIndex = prev.findIndex(f => f.name === file.name && f.size === file.size); - const entry = { name: file.name, size: file.size, message }; + const entry = { name: file.name, size: file.size, message: displayMessage }; if (existingIndex === -1) return [...prev, entry]; return prev.map((f, i) => (i === existingIndex ? entry : f)); }); diff --git a/src/i18n/en.json b/src/i18n/en.json index 8086c50b..00ded059 100644 --- a/src/i18n/en.json +++ b/src/i18n/en.json @@ -187,7 +187,8 @@ "drag_and_drop": "or drag and drop", "see_preview": "See Preview", "preview_file": "Preview file", - "complete": "Complete" + "complete": "Complete", + "network_error": "Upload interrupted by a connection error. Please retry." }, "grid_filter": { "filter_by": "Filter by ", From 94751b26e62dbaea406194ca95a2381bea2e630b Mon Sep 17 00:00:00 2001 From: Santiago Palenque Date: Thu, 27 Aug 2026 18:26:01 -0300 Subject: [PATCH 3/7] chore: improve error messages --- src/components/inputs/dropzone/__tests__/dropzone.test.js | 2 +- src/components/inputs/dropzone/index.js | 3 ++- .../upload-input-v3/__tests__/upload-input-v3.test.js | 8 ++++++++ src/components/inputs/upload-input-v3/index.js | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/components/inputs/dropzone/__tests__/dropzone.test.js b/src/components/inputs/dropzone/__tests__/dropzone.test.js index 58630e70..36d18c6b 100644 --- a/src/components/inputs/dropzone/__tests__/dropzone.test.js +++ b/src/components/inputs/dropzone/__tests__/dropzone.test.js @@ -655,7 +655,7 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { setTimeout(() => { expect(onErrorMock).toHaveBeenCalledTimes(1); const [message] = onErrorMock.mock.calls[0]; - expect(message).toBe('network down'); + expect(message).toBe('Network error'); done(); }, 2500); }, 10); diff --git a/src/components/inputs/dropzone/index.js b/src/components/inputs/dropzone/index.js index c1bfb85e..846c1ab3 100644 --- a/src/components/inputs/dropzone/index.js +++ b/src/components/inputs/dropzone/index.js @@ -135,7 +135,8 @@ export class DropzoneJS extends React.Component { } } catch (error) { this.stopPolling(file); - this.dropzone.emit('error', file, error.message || 'Network error'); + // fetch fail is always connection error + this.dropzone.emit('error', file, 'Network error'); } }, 2000); diff --git a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js index b8c84691..3a81bf68 100644 --- a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js +++ b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js @@ -271,6 +271,14 @@ describe('UploadInputV3', () => { expect(screen.queryByText('Server responded with 0 code.')).not.toBeInTheDocument(); }); + test('a fetch-rejection during status polling ("Network error") shows the same readable connection message', () => { + render(); + act(() => { + dropzoneCallbacks.onFileError({ name: 'video.mp4', size: 9999999 }, 'Network error'); + }); + expect(screen.getByText('Upload interrupted by a connection error. Please retry.')).toBeInTheDocument(); + }); + test('a real non-zero-status server error still shows the server-provided message', () => { render(); act(() => { diff --git a/src/components/inputs/upload-input-v3/index.js b/src/components/inputs/upload-input-v3/index.js index 24cf04cd..bb9a97d6 100644 --- a/src/components/inputs/upload-input-v3/index.js +++ b/src/components/inputs/upload-input-v3/index.js @@ -218,7 +218,7 @@ const UploadInputV3 = ({ // status 0 means the request never got a real server response (connection dropped/timed // out) - Dropzone's own message for that case is "Server responded with 0 code.", which // is not something a user can act on. - const displayMessage = status === 0 + const displayMessage = status === 0 || message === 'Network error' ? T.translate('upload_input_v3.network_error') : message; From da3e86cde2833db13a6775847b04595abd3935a4 Mon Sep 17 00:00:00 2001 From: Santiago Palenque Date: Thu, 27 Aug 2026 18:30:01 -0300 Subject: [PATCH 4/7] v5.0.58-beta.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 565cb8d7..1e862b46 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "openstack-uicore-foundation", - "version": "5.0.56", + "version": "5.0.58-beta.0", "description": "ui reactjs components for openstack marketing site", "main": "lib/openstack-uicore-foundation.js", "scripts": { From aa7f80a1b47cf1337377b52721fe03f6d773d5e3 Mon Sep 17 00:00:00 2001 From: Santiago Palenque Date: Thu, 3 Sep 2026 14:45:05 -0300 Subject: [PATCH 5/7] chore: pr review --- src/components/inputs/dropzone/index.js | 21 ++++++++++++++++--- .../inputs/upload-input-v3/index.js | 17 +++++++++++---- src/i18n/en.json | 7 ++++++- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/components/inputs/dropzone/index.js b/src/components/inputs/dropzone/index.js index 846c1ab3..b6cb2f19 100644 --- a/src/components/inputs/dropzone/index.js +++ b/src/components/inputs/dropzone/index.js @@ -49,6 +49,16 @@ export class DropzoneJS extends React.Component { this.props.onError(e, status, this.props.id); } + // this.dropzone may already be dropzone.destroy()'s return value (an Array, + // not the Dropzone instance) if a poll tick resolves after unmount. + reportPollingError(file, message) { + if (typeof this.dropzone?.emit === 'function') { + this.dropzone.emit('error', file, message); + } else { + this.onError(message); + } + } + onUploadComplete(response){ if(this.props.onUploadComplete) this.props.onUploadComplete(response, this.props.id, this.props.data); @@ -106,7 +116,7 @@ export class DropzoneJS extends React.Component { attempts++; if (attempts > maxAttempts) { this.stopPolling(file); - this.dropzone.emit('error', file, 'Upload timed out'); + this.reportPollingError(file, 'Upload timed out'); return; } try { @@ -114,6 +124,11 @@ export class DropzoneJS extends React.Component { const response = await fetch(statusUrl, { headers: { 'Authorization': `Bearer ${accessToken}` } }); + if (!response.ok) { + this.stopPolling(file); + this.reportPollingError(file, 'Network error'); + return; + } const data = await response.json(); // Clearing the interval is not enough on its own: this tick was already // awaiting its response when the user cancelled, and committing it now @@ -131,12 +146,12 @@ export class DropzoneJS extends React.Component { this.onUploadComplete(data); } else if (data.status === 'error') { this.stopPolling(file); - this.dropzone.emit('error', file, data.message || data.error || 'Upload failed'); + this.reportPollingError(file, data.message || 'Upload failed'); } } catch (error) { this.stopPolling(file); // fetch fail is always connection error - this.dropzone.emit('error', file, 'Network error'); + this.reportPollingError(file, 'Network error'); } }, 2000); diff --git a/src/components/inputs/upload-input-v3/index.js b/src/components/inputs/upload-input-v3/index.js index bb9a97d6..e0d36ef2 100644 --- a/src/components/inputs/upload-input-v3/index.js +++ b/src/components/inputs/upload-input-v3/index.js @@ -218,9 +218,18 @@ const UploadInputV3 = ({ // status 0 means the request never got a real server response (connection dropped/timed // out) - Dropzone's own message for that case is "Server responded with 0 code.", which // is not something a user can act on. - const displayMessage = status === 0 || message === 'Network error' - ? T.translate('upload_input_v3.network_error') - : message; + const displayMessage = + status === 0 || message === 'Network error' + ? T.translate('upload_input_v3.network_error') + : message === 'Upload timed out' + ? T.translate('upload_input_v3.upload_timed_out') + : message === 'Upload failed' + ? T.translate('upload_input_v3.upload_failed') + : message === 'Auth error' + ? T.translate('upload_input_v3.auth_error') + : message === 'Max files reached.' + ? T.translate('upload_input_v3.max_files_reached') + : message; setErrorFiles(prev => { const existingIndex = prev.findIndex(f => f.name === file.name && f.size === file.size); @@ -391,7 +400,7 @@ const UploadInputV3 = ({ {file.name} - {formatFileSize(file.size)} · {file.complete ? 'Complete' : 'Loading'} + {formatFileSize(file.size)} · {file.complete ? T.translate("upload_input_v3.complete") : T.translate("upload_input_v3.loading")} {!file.complete && ( Date: Thu, 3 Sep 2026 15:51:13 -0300 Subject: [PATCH 6/7] chore: fix tests --- .../dropzone/__tests__/dropzone.test.js | 6 +++-- .../__tests__/upload-input-v3.test.js | 26 +++++++++---------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/components/inputs/dropzone/__tests__/dropzone.test.js b/src/components/inputs/dropzone/__tests__/dropzone.test.js index 36d18c6b..8d860dd8 100644 --- a/src/components/inputs/dropzone/__tests__/dropzone.test.js +++ b/src/components/inputs/dropzone/__tests__/dropzone.test.js @@ -192,6 +192,7 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { // Mock fetch to return complete status global.fetch = jest.fn(() => Promise.resolve({ + ok: true, json: () => Promise.resolve({ status: 'complete', name: 'test.pdf', @@ -317,7 +318,7 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { test('test_dropzone_cancel_stops_polling_for_that_file_only', (done) => { // Server is still processing: every poll answers 'uploading', so polling keeps going. global.fetch = jest.fn(() => - Promise.resolve({ json: () => Promise.resolve({ status: 'uploading' }) }) + Promise.resolve({ ok: true, json: () => Promise.resolve({ status: 'uploading' }) }) ); const ref = React.createRef(); @@ -574,7 +575,7 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { test('test_dropzone_poll_timeout_emits_a_string_message_and_calls_onError_once', async () => { jest.useFakeTimers({ doNotFake: ['queueMicrotask'] }); global.fetch = jest.fn(() => - Promise.resolve({ json: () => Promise.resolve({ status: 'uploading' }) }) + Promise.resolve({ ok: true, json: () => Promise.resolve({ status: 'uploading' }) }) ); const ref = React.createRef(); @@ -605,6 +606,7 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { test('test_dropzone_poll_server_error_status_emits_readable_message_not_object', (done) => { global.fetch = jest.fn(() => Promise.resolve({ + ok: true, json: () => Promise.resolve({ status: 'error', message: 'processing failed' }) }) ); diff --git a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js index 3a81bf68..05973137 100644 --- a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js +++ b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js @@ -185,7 +185,7 @@ describe('UploadInputV3', () => { dropzoneCallbacks.onAddedFile({ name: 'sample.png', size: 11264 }); }); expect(screen.getByText('sample.png')).toBeInTheDocument(); - expect(screen.getByText(/Loading/)).toBeInTheDocument(); + expect(screen.getByText(/Uploading/)).toBeInTheDocument(); }); test('shows Complete status and hides progress bar after upload finishes', () => { @@ -197,7 +197,7 @@ describe('UploadInputV3', () => { dropzoneCallbacks.onFileCompleted({ name: 'sample.png', size: 11264 }); }); expect(screen.getByText(/Complete/)).toBeInTheDocument(); - expect(screen.queryByText(/Loading/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Uploading/)).not.toBeInTheDocument(); }); test('hides dropzone when max files reached', () => { @@ -234,7 +234,7 @@ describe('UploadInputV3', () => { act(() => { dropzoneCallbacks.onAddedFile({ name: 'big-file.png', size: 9999999 }); }); - expect(screen.getByText(/Loading/)).toBeInTheDocument(); + expect(screen.getByText(/Uploading/)).toBeInTheDocument(); act(() => { dropzoneCallbacks.onFileError( @@ -242,7 +242,7 @@ describe('UploadInputV3', () => { 'File is too big (9.54MiB). Max filesize: 5MiB.' ); }); - expect(screen.queryByText(/Loading/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Uploading/)).not.toBeInTheDocument(); expect(screen.getByText(/File is too big/)).toBeInTheDocument(); }); @@ -317,7 +317,7 @@ describe('UploadInputV3', () => { act(() => { dropzoneCallbacks.onAddedFile({ name: 'sample.png', size: 11264 }); }); - expect(screen.getByText(/Loading/)).toBeInTheDocument(); + expect(screen.getByText(/Uploading/)).toBeInTheDocument(); // The delete button on the uploading row is the only button on screen. act(() => { @@ -662,7 +662,7 @@ describe('UploadInputV3', () => { // Assert: file should still show "Loading" (not "Complete") because async processing is in progress expect(screen.getByText('video.mp4')).toBeInTheDocument(); - expect(screen.getByText(/Loading/)).toBeInTheDocument(); + expect(screen.getByText(/Uploading/)).toBeInTheDocument(); expect(screen.queryByText(/Complete/)).not.toBeInTheDocument(); }); @@ -685,7 +685,7 @@ describe('UploadInputV3', () => { }); // File should still be Loading despite progress being 100 - expect(screen.getByText(/Loading/)).toBeInTheDocument(); + expect(screen.getByText(/Uploading/)).toBeInTheDocument(); expect(screen.queryByText(/Complete/)).not.toBeInTheDocument(); // Polling completes - onUploadComplete fires after async processing finishes @@ -696,7 +696,7 @@ describe('UploadInputV3', () => { // Assert: file should now show "Complete" expect(screen.getByText('video.mp4')).toBeInTheDocument(); expect(screen.getByText(/Complete/)).toBeInTheDocument(); - expect(screen.queryByText(/Loading/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Uploading/)).not.toBeInTheDocument(); }); test('completing one file does not mark other in-flight files as complete when maxFiles > 1', () => { @@ -713,7 +713,7 @@ describe('UploadInputV3', () => { // Both should be Loading expect(screen.getByText('file-a.png')).toBeInTheDocument(); expect(screen.getByText('file-b.png')).toBeInTheDocument(); - expect(screen.getAllByText(/Loading/)).toHaveLength(2); + expect(screen.getAllByText(/Uploading/)).toHaveLength(2); // File A finishes uploading all chunks - progress reaches 100 act(() => { @@ -737,7 +737,7 @@ describe('UploadInputV3', () => { // Assert: file B should still show "Loading" - it has not finished uploading expect(screen.getByText('file-b.png')).toBeInTheDocument(); - expect(screen.getByText(/Loading/)).toBeInTheDocument(); + expect(screen.getByText(/Uploading/)).toBeInTheDocument(); }); test('HTTP 202 parallel: onUploadComplete for one file does not prematurely mark sibling file as complete', () => { @@ -758,7 +758,7 @@ describe('UploadInputV3', () => { dropzoneCallbacks.onFileCompleted({ name: 'clip-b.mp4', size: 8000000, _asyncProcessing: true }); }); - expect(screen.getAllByText(/Loading/)).toHaveLength(2); + expect(screen.getAllByText(/Uploading/)).toHaveLength(2); // Only clip-a.mp4 polling finishes act(() => { @@ -767,9 +767,9 @@ describe('UploadInputV3', () => { // clip-b must still be Loading - it has not finished async processing expect(screen.getByText('clip-b.mp4')).toBeInTheDocument(); - expect(screen.getByText(/Loading/)).toBeInTheDocument(); + expect(screen.getByText(/Uploading/)).toBeInTheDocument(); // Only one Loading entry should remain (clip-b) - expect(screen.getAllByText(/Loading/)).toHaveLength(1); + expect(screen.getAllByText(/Uploading/)).toHaveLength(1); }); }); }); From d016c24227666c0b6a220cdb777e42df5aaf1ff0 Mon Sep 17 00:00:00 2001 From: Santiago Palenque Date: Fri, 4 Sep 2026 18:59:29 -0300 Subject: [PATCH 7/7] chore: pr review --- .../dropzone/__tests__/dropzone.test.js | 112 +++++++++++++++++- src/components/inputs/dropzone/index.js | 77 ++++++------ .../inputs/upload-input-v3/index.js | 19 ++- 3 files changed, 167 insertions(+), 41 deletions(-) diff --git a/src/components/inputs/dropzone/__tests__/dropzone.test.js b/src/components/inputs/dropzone/__tests__/dropzone.test.js index 8d860dd8..53c1f1a4 100644 --- a/src/components/inputs/dropzone/__tests__/dropzone.test.js +++ b/src/components/inputs/dropzone/__tests__/dropzone.test.js @@ -254,6 +254,7 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { test('test_dropzone_poll_upload_status_encodes_file_id_in_url', (done) => { global.fetch = jest.fn(() => Promise.resolve({ + ok: true, json: () => Promise.resolve({ status: 'complete', name: 'HPE_OCPSanJose_Backdrop_20x10ft50_Ver1.4_PRINT.pdf', @@ -379,6 +380,7 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { new Promise((resolve) => { respondComplete = () => resolve({ + ok: true, json: () => Promise.resolve({ status: 'complete', name: 'test.pdf', size: 1024000 }) }); @@ -434,7 +436,7 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { */ test('test_dropzone_unmount_stops_polling_for_every_file', (done) => { global.fetch = jest.fn(() => - Promise.resolve({ json: () => Promise.resolve({ status: 'uploading' }) }) + Promise.resolve({ ok: true, json: () => Promise.resolve({ status: 'uploading' }) }) ); const ref = React.createRef(); @@ -546,7 +548,8 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { ); const instance = ref.current; - const mockFile = { name: 'test.pdf', size: 1024000 }; + // Only requests that went through the chunk-throttle queue occupy a concurrency slot. + const mockFile = { name: 'test.pdf', size: 1024000, _isThrottledChunk: true }; const dropzoneOnTimeout = jest.fn(); const mockXhr = { readyState: XMLHttpRequest.DONE, @@ -567,6 +570,50 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { expect(dropzoneOnTimeout).toHaveBeenCalledTimes(1); }); + /** + * Test Case 10b: a non-chunked upload's completion must not free a concurrency slot + * it never occupied. + * + * chunksInFlight is only ever incremented for requests routed through the chunk-throttle + * queue (setupChunkThrottle). A file that bypasses the queue (no _isThrottledChunk tag) + * finishing its single request used to still call onChunkComplete() unconditionally, + * decrementing the counter for chunked uploads that were still genuinely in flight and + * letting more than maxConcurrentChunks run at once. + */ + test('test_dropzone_non_chunked_upload_does_not_release_a_chunk_slot', () => { + const ref = React.createRef(); + + render( + + ); + + const instance = ref.current; + // No _isThrottledChunk tag - this file bypassed the throttle queue. + const mockFile = { name: 'small.pdf', size: 1024 }; + const mockXhr = { + readyState: XMLHttpRequest.DONE, + status: 200, + responseText: JSON.stringify({ name: 'small.pdf', path: 'uploads/', size: 1024 }), + setRequestHeader: jest.fn(), + onload: jest.fn(), + onerror: jest.fn(), + abort: jest.fn() + }; + + // Two real chunked uploads for a different file are genuinely in flight. + instance.chunksInFlight = 2; + getEventHandler(instance, 'sending')(mockFile, mockXhr, { append: jest.fn() }); + + mockXhr.onload({}); + + expect(instance.chunksInFlight).toBe(2); + }); + /** * Test Cases 11-13: pollUploadStatus's three failure branches route through the * file-level error channel (dropzone.emit('error', file, message)) instead of @@ -636,6 +683,67 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { }, 10); }, 10000); + /** + * Test Case 14: a slow status request must not let a second one start alongside it. + * + * The old setInterval-based poll fired every 2s regardless of whether the previous + * tick's fetch had resolved yet, so a slow response could still be in flight when a + * later tick's request resolved first - letting two terminal results race (e.g. a + * stray error landing after an already-committed completion). The self-scheduling loop + * only calls sleep(2000) again after its current request fully resolves, so no second + * request can ever start while one is outstanding - this asserts that directly, then + * confirms the single in-flight request still completes normally once it resolves. + */ + test('test_dropzone_poll_does_not_start_a_new_request_while_one_is_still_in_flight', async () => { + jest.useFakeTimers({ doNotFake: ['queueMicrotask'] }); + let resolveFetch; + global.fetch = jest.fn( + () => new Promise((resolve) => { resolveFetch = resolve; }) + ); + + const ref = React.createRef(); + render( + + ); + + const instance = ref.current; + const mockFile = { + name: 'test.pdf', + size: 1024000, + _asyncProcessing: true, + _chunksUploadedDone: jest.fn() + }; + + instance.pollUploadStatus('file-123', 'https://example.com/upload', mockFile); + + // First tick fires at 2000ms and parks on the still-unresolved request. + await jest.advanceTimersByTimeAsync(2000); + expect(global.fetch).toHaveBeenCalledTimes(1); + + // Several more tick-intervals' worth of time pass while that request is still + // outstanding - a second request must not start. + await jest.advanceTimersByTimeAsync(2000 * 5); + expect(global.fetch).toHaveBeenCalledTimes(1); + + // Now the one in-flight request finally resolves with a terminal result. + resolveFetch({ + ok: true, + json: () => Promise.resolve({ status: 'complete', name: 'test.pdf', size: 1024000 }) + }); + await jest.advanceTimersByTimeAsync(0); + + expect(mockFile._chunksUploadedDone).toHaveBeenCalledTimes(1); + expect(onUploadCompleteMock).toHaveBeenCalledTimes(1); + expect(onErrorMock).not.toHaveBeenCalled(); + + jest.useRealTimers(); + }, 10000); + test('test_dropzone_poll_fetch_rejection_emits_readable_message_and_calls_onError_once', (done) => { global.fetch = jest.fn(() => Promise.reject(new Error('network down'))); diff --git a/src/components/inputs/dropzone/index.js b/src/components/inputs/dropzone/index.js index b6cb2f19..81d3f130 100644 --- a/src/components/inputs/dropzone/index.js +++ b/src/components/inputs/dropzone/index.js @@ -22,25 +22,22 @@ export class DropzoneJS extends React.Component { this.activeXHRs = new Map(); // Track active XHR requests per file this.chunkQueue = []; this.chunksInFlight = 0; - // Status-poll interval ids, one per file in flight. Kept as a set (and mirrored on - // the file itself) rather than a single slot so a second file starting to poll - // cannot orphan the first one's interval. - this._pollIntervals = new Set(); + // Checked by every in-flight pollUploadStatus loop so unmount stops all of them. + this._unmounted = false; + } + + sleep(ms) { + return new Promise(resolve => setTimeout(resolve, ms)); } /** - * Stops the status polling started by pollUploadStatus for this file, if any. - * Cancelling an upload has to reach the interval too: a file the user removed while the + * Stops the status polling loop started by pollUploadStatus for this file, if any. + * Cancelling an upload has to reach the loop too: a file the user removed while the * server was still processing it must stop asking for its status, otherwise the result * lands later and gets committed as if the upload had been kept. */ stopPolling(file) { if (!file) return; - if (file._pollIntervalId) { - clearInterval(file._pollIntervalId); - this._pollIntervals.delete(file._pollIntervalId); - file._pollIntervalId = null; - } file._pollingActive = false; } @@ -78,8 +75,10 @@ export class DropzoneJS extends React.Component { // Wrap _uploadData to queue chunked uploads with concurrency limit this._originalUploadData = this.dropzone._uploadData.bind(this.dropzone); this.dropzone._uploadData = (files, dataBlocks) => { - // Only throttle chunked uploads (single dataBlock with chunkIndex) - if (dataBlocks.length === 1 && dataBlocks[0].chunkIndex !== undefined) { + // Tag the file so 'sending' below only releases a slot for requests that took one. + const isThrottledChunk = dataBlocks.length === 1 && dataBlocks[0].chunkIndex !== undefined; + files.forEach(file => { file._isThrottledChunk = isThrottledChunk; }); + if (isThrottledChunk) { this.chunkQueue.push({ files, dataBlocks }); this.processChunkQueue(); } else { @@ -94,8 +93,8 @@ export class DropzoneJS extends React.Component { this.processChunkQueue(); } - pollUploadStatus(fileId, baseUrl, file) { - // Guard against multiple polling intervals for the same file + async pollUploadStatus(fileId, baseUrl, file) { + // Guard against multiple polling loops for the same file if (file._pollingActive) { return; } @@ -107,9 +106,12 @@ export class DropzoneJS extends React.Component { const maxAttempts = 300; // 10 minutes at 2s intervals let attempts = 0; - const intervalId = setInterval(async () => { - // The file may have been removed since the last tick. - if (file._canceled) { + // A self-scheduling loop (not setInterval) keeps only one request in flight at a time. + while (true) { + await this.sleep(2000); + + // The file may have been removed, or the component unmounted, since the last check. + if (file._canceled || this._unmounted) { this.stopPolling(file); return; } @@ -126,14 +128,17 @@ export class DropzoneJS extends React.Component { }); if (!response.ok) { this.stopPolling(file); - this.reportPollingError(file, 'Network error'); + // Don't report an error for a file the user already removed, or after unmount. + if (!file._canceled && !this._unmounted) { + this.reportPollingError(file, response.status === 403 ? 'Auth error' : 'Network error'); + } return; } const data = await response.json(); - // Clearing the interval is not enough on its own: this tick was already - // awaiting its response when the user cancelled, and committing it now - // would restore a file they removed. - if (file._canceled) { + // This request was already awaiting its response when the user cancelled + // (or the component unmounted), and committing it now would restore a file + // they removed. + if (file._canceled || this._unmounted) { this.stopPolling(file); return; } @@ -144,19 +149,20 @@ export class DropzoneJS extends React.Component { file._chunksUploadedDone(); } this.onUploadComplete(data); + return; } else if (data.status === 'error') { this.stopPolling(file); this.reportPollingError(file, data.message || 'Upload failed'); + return; } + // any other status (e.g. 'uploading') means keep polling } catch (error) { this.stopPolling(file); // fetch fail is always connection error this.reportPollingError(file, 'Network error'); + return; } - }, 2000); - - file._pollIntervalId = intervalId; - this._pollIntervals.add(intervalId); + } } /** @@ -247,8 +253,7 @@ export class DropzoneJS extends React.Component { * Removes dropzone.js (and all its globals) if the component is being unmounted */ componentWillUnmount () { - this._pollIntervals.forEach(intervalId => clearInterval(intervalId)); - this._pollIntervals.clear(); + this._unmounted = true; // Clear chunk queue and cancel all pending XHR requests this.chunkQueue = []; @@ -463,8 +468,10 @@ export class DropzoneJS extends React.Component { if (index > -1) xhrs.splice(index, 1); } - // Release a slot in the chunk queue for the next chunk - _this.onChunkComplete(); + // Release a slot only if this request actually took one from the queue. + if (file._isThrottledChunk) { + _this.onChunkComplete(); + } // Track completed bytes for accurate progress (prevents oscillation) const chunkSize = _this.dropzone?.options?.chunkSize || 2000000; @@ -507,14 +514,18 @@ export class DropzoneJS extends React.Component { let dropzoneOnError = xhr.onerror; xhr.onerror = function(e) { - _this.onChunkComplete(); + if (file._isThrottledChunk) { + _this.onChunkComplete(); + } if (dropzoneOnError) dropzoneOnError(e); } // Without this wrapper a timed-out chunk never releases its concurrency slot. let dropzoneOnTimeout = xhr.ontimeout; xhr.ontimeout = function(e) { - _this.onChunkComplete(); + if (file._isThrottledChunk) { + _this.onChunkComplete(); + } if (dropzoneOnTimeout) dropzoneOnTimeout(e); } }) diff --git a/src/components/inputs/upload-input-v3/index.js b/src/components/inputs/upload-input-v3/index.js index e0d36ef2..7192ab40 100644 --- a/src/components/inputs/upload-input-v3/index.js +++ b/src/components/inputs/upload-input-v3/index.js @@ -215,21 +215,28 @@ const UploadInputV3 = ({ // Dropzone module being loaded here; _userCanceled is when removed before its upload reached the UPLOADING state. if (file._userCanceled || file.status === 'canceled') return; + // Dropzone parses any application/json error body before checking status, so a real + // server validation error (e.g. file-upload-api's {"message": "..."}) arrives as an + // object here, not a string - render it as one or React crashes on the JSX below. + const text = typeof message === 'string' + ? message + : (message?.message ?? T.translate('upload_input_v3.upload_failed')); + // status 0 means the request never got a real server response (connection dropped/timed // out) - Dropzone's own message for that case is "Server responded with 0 code.", which // is not something a user can act on. const displayMessage = - status === 0 || message === 'Network error' + status === 0 || text === 'Network error' ? T.translate('upload_input_v3.network_error') - : message === 'Upload timed out' + : text === 'Upload timed out' ? T.translate('upload_input_v3.upload_timed_out') - : message === 'Upload failed' + : text === 'Upload failed' ? T.translate('upload_input_v3.upload_failed') - : message === 'Auth error' + : text === 'Auth error' ? T.translate('upload_input_v3.auth_error') - : message === 'Max files reached.' + : text === 'Max files reached.' ? T.translate('upload_input_v3.max_files_reached') - : message; + : text; setErrorFiles(prev => { const existingIndex = prev.findIndex(f => f.name === file.name && f.size === file.size);