From 4d9c31f8e889792d8badb6b794a1158be7744c27 Mon Sep 17 00:00:00 2001 From: smarcet Date: Sat, 22 Aug 2026 08:29:19 -0300 Subject: [PATCH 1/3] fix: cancelling an upload must not commit its result Removing a file while the server was still processing it (HTTP 202) left the status poll running, so when processing finished the result was still pushed to the parent. The cancelled file reappeared as "Complete" next to the "Upload canceled." row and was persisted on the next save. - keep one status-poll interval per file instead of a single component-level slot, so a second file starting to poll cannot orphan the first one's interval (which also outlived the component on unmount) - mark a removed file as cancelled and stop its polling in the removedfile handler - guard both commit points against a cancelled file: the status poll, including the tick already awaiting its response, and xhr.onload, where abort() on an already-DONE request is a no-op - treat a user-initiated cancel in UploadInputV3 as a cancel rather than an error row the user has to dismiss --- .../dropzone/__tests__/dropzone.test.js | 230 ++++++++++++++++++ src/components/inputs/dropzone/index.js | 74 ++++-- .../__tests__/upload-input-v3.test.js | 28 +++ .../inputs/upload-input-v3/index.js | 10 +- 4 files changed, 324 insertions(+), 18 deletions(-) diff --git a/src/components/inputs/dropzone/__tests__/dropzone.test.js b/src/components/inputs/dropzone/__tests__/dropzone.test.js index 39ece6d3..9a1c2497 100644 --- a/src/components/inputs/dropzone/__tests__/dropzone.test.js +++ b/src/components/inputs/dropzone/__tests__/dropzone.test.js @@ -285,6 +285,236 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { }, 2500); }, 10); }, 10000); + /** + * Resolves a handler DropzoneJS registered on the underlying Dropzone instance, + * so a test can drive the real wiring instead of poking at internals. + */ + const getEventHandler = (instance, eventName) => { + const call = instance.dropzone.on.mock.calls + .slice() + .reverse() + .find(([evt]) => evt === eventName); + return call ? call[1] : null; + }; + + /** + * Test Case 6: cancelling a file stops ITS status polling - and only its own + * + * While the server processes an upload asynchronously (HTTP 202) the row sits in + * "Loading" with a delete button. Removing the file there used to leave the status + * interval running, so when processing finished the result was still pushed to the + * parent and the "cancelled" file ended up committed to the form. + */ + 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' }) }) + ); + + const ref = React.createRef(); + + render( + + ); + + setTimeout(() => { + const instance = ref.current; + const fileA = { name: 'a.png', size: 1024 }; + const fileB = { name: 'b.png', size: 2048 }; + + instance.pollUploadStatus('file-a', 'https://example.com/upload', fileA); + instance.pollUploadStatus('file-b', 'https://example.com/upload', fileB); + + setTimeout(() => { + expect(global.fetch).toHaveBeenCalled(); + + const onRemovedFile = getEventHandler(instance, 'removedfile'); + expect(typeof onRemovedFile).toBe('function'); + onRemovedFile(fileA); + + const callsAtCancel = global.fetch.mock.calls.length; + + setTimeout(() => { + const urlsAfterCancel = global.fetch.mock.calls + .slice(callsAtCancel) + .map(([url]) => url); + + // fileB is untouched and keeps polling on its own interval... + expect(urlsAfterCancel.length).toBeGreaterThan(0); + // ...while the cancelled file never asks for its status again. + expect(urlsAfterCancel.some((url) => url.endsWith('/status/file-a'))).toBe(false); + + done(); + }, 2500); + }, 2500); + }, 10); + }, 15000); + + /** + * Test Case 7: a status response that lands AFTER the cancel must not be committed + * + * Clearing the interval is not enough on its own: the tick that was already awaiting + * its status request resolves after the user cancelled, and used to fire the deferred + * success callback plus onUploadComplete. + */ + test('test_dropzone_cancel_while_status_request_in_flight_does_not_commit_result', (done) => { + let respondComplete = null; + global.fetch = jest.fn( + () => + new Promise((resolve) => { + respondComplete = () => + resolve({ + json: () => + Promise.resolve({ status: 'complete', name: 'test.pdf', size: 1024000 }) + }); + }) + ); + + const ref = React.createRef(); + + render( + + ); + + setTimeout(() => { + const instance = ref.current; + const chunksUploadedDone = jest.fn(); + const mockFile = { + name: 'test.pdf', + size: 1024000, + _asyncProcessing: true, + _chunksUploadedDone: chunksUploadedDone + }; + + instance.pollUploadStatus('file-123', 'https://example.com/upload', mockFile); + + setTimeout(() => { + // The first tick fired and is parked on the status request. + expect(typeof respondComplete).toBe('function'); + + // The user cancels while that request is still in flight... + getEventHandler(instance, 'removedfile')(mockFile); + // ...and only then does the server report the upload as processed. + respondComplete(); + + setTimeout(() => { + expect(chunksUploadedDone).not.toHaveBeenCalled(); + expect(onUploadCompleteMock).not.toHaveBeenCalled(); + done(); + }, 100); + }, 2500); + }, 10); + }, 15000); + + /** + * Test Case 8: unmounting stops polling for EVERY file in flight + * + * The interval id used to live in a single component-level slot, so a second file + * starting to poll orphaned the first one's interval and it outlived the component. + */ + test('test_dropzone_unmount_stops_polling_for_every_file', (done) => { + global.fetch = jest.fn(() => + Promise.resolve({ json: () => Promise.resolve({ status: 'uploading' }) }) + ); + + const ref = React.createRef(); + + const { unmount } = render( + + ); + + setTimeout(() => { + const instance = ref.current; + + instance.pollUploadStatus('file-a', 'https://example.com/upload', { + name: 'a.png', + size: 1024 + }); + instance.pollUploadStatus('file-b', 'https://example.com/upload', { + name: 'b.png', + size: 2048 + }); + + setTimeout(() => { + expect(global.fetch).toHaveBeenCalled(); + + unmount(); + const callsAtUnmount = global.fetch.mock.calls.length; + + setTimeout(() => { + expect(global.fetch.mock.calls.length).toBe(callsAtUnmount); + done(); + }, 2500); + }, 2500); + }, 10); + }, 15000); + + /** + * Test Case 9: an upload response that arrives after the cancel must not be committed + * + * xhr.abort() on a request already in DONE state is a no-op, so the synchronous (200) + * path could still reach onUploadComplete for a file the user had just removed. + */ + test('test_dropzone_upload_response_after_cancel_does_not_commit_result', (done) => { + const ref = React.createRef(); + + render( + + ); + + setTimeout(() => { + const instance = ref.current; + const mockFile = { + name: 'test.pdf', + size: 1024000, + accessToken: 'mock-token', + md5: 'mock-md5' + }; + const mockXhr = { + readyState: XMLHttpRequest.DONE, + status: 200, + responseText: JSON.stringify({ + name: 'test.pdf', + path: 'uploads/', + size: 1024000 + }), + setRequestHeader: jest.fn(), + onload: jest.fn(), + onerror: jest.fn(), + abort: jest.fn() + }; + + // 'sending' is what installs the wrapper deciding what to do with the response. + getEventHandler(instance, 'sending')(mockFile, mockXhr, { append: jest.fn() }); + + // The user cancels; the response for the last chunk is already on its way back. + getEventHandler(instance, 'removedfile')(mockFile); + mockXhr.onload({}); + + expect(onUploadCompleteMock).not.toHaveBeenCalled(); + done(); + }, 10); + }); }); describe('DropzoneJS - Progress Bar Monotonicity', () => { diff --git a/src/components/inputs/dropzone/index.js b/src/components/inputs/dropzone/index.js index f41f9990..d619fc1e 100644 --- a/src/components/inputs/dropzone/index.js +++ b/src/components/inputs/dropzone/index.js @@ -22,6 +22,26 @@ 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(); + } + + /** + * 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 + * 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; } onError(e, status){ @@ -77,12 +97,15 @@ export class DropzoneJS extends React.Component { const maxAttempts = 300; // 10 minutes at 2s intervals let attempts = 0; - this._pollInterval = setInterval(async () => { + const intervalId = setInterval(async () => { + // The file may have been removed since the last tick. + if (file._canceled) { + this.stopPolling(file); + return; + } attempts++; if (attempts > maxAttempts) { - clearInterval(this._pollInterval); - this._pollInterval = null; - file._pollingActive = false; + this.stopPolling(file); this.onError({ message: 'Upload timed out' }); return; } @@ -92,28 +115,32 @@ export class DropzoneJS extends React.Component { headers: { 'Authorization': `Bearer ${accessToken}` } }); 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.stopPolling(file); + return; + } if (data.status === 'complete') { - clearInterval(this._pollInterval); - this._pollInterval = null; - file._pollingActive = false; + this.stopPolling(file); // Call the stored done callback to trigger Dropzone's success event if (file?._chunksUploadedDone) { file._chunksUploadedDone(); } this.onUploadComplete(data); } else if (data.status === 'error') { - clearInterval(this._pollInterval); - this._pollInterval = null; - file._pollingActive = false; + this.stopPolling(file); this.onError(data); } } catch (error) { - clearInterval(this._pollInterval); - this._pollInterval = null; - file._pollingActive = false; + this.stopPolling(file); this.onError(error); } }, 2000); + + file._pollIntervalId = intervalId; + this._pollIntervals.add(intervalId); } /** @@ -204,10 +231,8 @@ export class DropzoneJS extends React.Component { * Removes dropzone.js (and all its globals) if the component is being unmounted */ componentWillUnmount () { - if (this._pollInterval) { - clearInterval(this._pollInterval); - this._pollInterval = null; - } + this._pollIntervals.forEach(intervalId => clearInterval(intervalId)); + this._pollIntervals.clear(); // Clear chunk queue and cancel all pending XHR requests this.chunkQueue = []; @@ -343,6 +368,14 @@ export class DropzoneJS extends React.Component { this.dropzone.on('removedfile', (file) => { if (!file) return; + // Mark the file dead FIRST: both commit points (xhr.onload below and the status + // poll) check this flag, so a result that lands after the user cancelled is + // dropped instead of being pushed to the parent. + file._canceled = true; + this.stopPolling(file); + // A removed file must not fire Dropzone's deferred success event either. + file._chunksUploadedDone = null; + // Cancel all active XHR requests for this file const xhrs = this.activeXHRs.get(file); if (xhrs) { @@ -434,6 +467,13 @@ export class DropzoneJS extends React.Component { dropzoneOnLoad(e); + // The user may have cancelled while this response was in flight: abort() on an + // already-DONE xhr is a no-op, so without this check the result would still be + // committed for a file that is no longer in the list. 'canceled' is the value of + // Dropzone.CANCELED, compared as a literal so the guard does not depend on the + // Dropzone module being loaded. + if (file._canceled || file.status === 'canceled') return; + if(xhr?.status == 200) { if (typeof uploadResponse.name === 'string') { _this.onUploadComplete(uploadResponse); 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 a855c3d6..600bad3f 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 @@ -265,6 +265,34 @@ describe('UploadInputV3', () => { expect(container.querySelector('.dropzone-mock')).toBeVisible(); }); + test('does not show an error row when the user cancels the upload themselves', () => { + const removeFile = jest.fn(); + const dzFile = { name: 'sample.png', size: 11264 }; + + render(); + act(() => { + dropzoneCallbacks.onDropzoneReady({ files: [dzFile], removeFile }); + }); + act(() => { + dropzoneCallbacks.onAddedFile({ name: 'sample.png', size: 11264 }); + }); + expect(screen.getByText(/Loading/)).toBeInTheDocument(); + + // The delete button on the uploading row is the only button on screen. + act(() => { + fireEvent.click(screen.getByRole('button')); + }); + expect(removeFile).toHaveBeenCalledWith(dzFile); + + // Dropzone reports a cancelled upload as an error carrying dictUploadCanceled. + act(() => { + dropzoneCallbacks.onFileError(dzFile, 'Upload canceled.'); + }); + + expect(screen.queryByText('Upload canceled.')).not.toBeInTheDocument(); + expect(screen.queryByText('sample.png')).not.toBeInTheDocument(); + }); + test('hides dropzone when an error is present', () => { 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 46a7766c..4b42b114 100644 --- a/src/components/inputs/upload-input-v3/index.js +++ b/src/components/inputs/upload-input-v3/index.js @@ -211,6 +211,9 @@ const UploadInputV3 = ({ 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 + // the user asked for is not a failure to report back to them - the row just goes away. + if (file._userCanceled) return; setErrorFiles(prev => [...prev, { name: file.name, size: file.size, message }]); }, []); @@ -232,7 +235,12 @@ const UploadInputV3 = ({ const dzFile = dropzoneInstanceRef.current.files?.find( f => f.name === file.name && f.size === file.size ); - if (dzFile) dropzoneInstanceRef.current.removeFile(dzFile); + if (dzFile) { + // Flag it before removing so handleFileError can tell this deliberate cancel + // apart from a real upload failure. + dzFile._userCanceled = true; + dropzoneInstanceRef.current.removeFile(dzFile); + } } }, []); From 3fb802044021d95ec9e1f16cac0de08c9a56dd96 Mon Sep 17 00:00:00 2001 From: smarcet Date: Sat, 22 Aug 2026 08:36:49 -0300 Subject: [PATCH 2/3] v5.0.52-beta.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index bab3b790..99c92dfa 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "openstack-uicore-foundation", - "version": "5.0.50", + "version": "5.0.52-beta.0", "description": "ui reactjs components for openstack marketing site", "main": "lib/openstack-uicore-foundation.js", "scripts": { From 4765ef111037460cffe2171a9a39a9858039bd48 Mon Sep 17 00:00:00 2001 From: smarcet Date: Sat, 22 Aug 2026 08:38:10 -0300 Subject: [PATCH 3/3] fix: suppress the error row for any Dropzone cancellation state Addresses review feedback: handleFileError keyed only on our own _userCanceled flag, which is set by the uploading row's delete button. Any cancel is a cancel, so match Dropzone's own CANCELED status too - it covers cancels this component did not initiate. --- .../__tests__/upload-input-v3.test.js | 19 +++++++++++++++++++ .../inputs/upload-input-v3/index.js | 7 +++++-- 2 files changed, 24 insertions(+), 2 deletions(-) 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 600bad3f..59f3b058 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 @@ -293,6 +293,25 @@ describe('UploadInputV3', () => { expect(screen.queryByText('sample.png')).not.toBeInTheDocument(); }); + test('does not show an error row for a file Dropzone reports as canceled', () => { + render(); + act(() => { + dropzoneCallbacks.onAddedFile({ name: 'sample.png', size: 11264 }); + }); + + // Cancelled by something other than the row's delete button (unmount, a consumer + // calling cancelUpload): Dropzone marks the file, our own flag is absent. + act(() => { + dropzoneCallbacks.onFileError( + { name: 'sample.png', size: 11264, status: 'canceled' }, + 'Upload canceled.' + ); + }); + + expect(screen.queryByText('Upload canceled.')).not.toBeInTheDocument(); + expect(screen.queryByText('sample.png')).not.toBeInTheDocument(); + }); + test('hides dropzone when an error is present', () => { 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 4b42b114..8e2d474c 100644 --- a/src/components/inputs/upload-input-v3/index.js +++ b/src/components/inputs/upload-input-v3/index.js @@ -212,8 +212,11 @@ const UploadInputV3 = ({ return prev.filter(f => !(f.name === file.name && f.size === file.size)); }); // Dropzone turns a cancelled upload into an error carrying dictUploadCanceled. A cancel - // the user asked for is not a failure to report back to them - the row just goes away. - if (file._userCanceled) return; + // 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. + if (file._userCanceled || file.status === 'canceled') return; setErrorFiles(prev => [...prev, { name: file.name, size: file.size, message }]); }, []);