Hotfix/file upload pooling - #221
Conversation
Signed-off-by: smarcet <smarcet@gmail.com>
📝 WalkthroughWalkthroughThe package version was bumped to Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Browser
participant Dropzone as Dropzone Component
participant UploadServer as Upload Server
participant StatusServer as Status Endpoint
Client->>Dropzone: User drops file
Dropzone->>UploadServer: POST file (multipart/form-data)
UploadServer-->>Dropzone: 202 Accepted (file_id, baseUrl)
Dropzone->>Dropzone: Extract file_id & baseUrl
Dropzone->>Dropzone: Start polling (every 2s, max 300 attempts)
loop Poll until completion
Dropzone->>Dropzone: Fetch auth token
Dropzone->>StatusServer: GET /status/{file_id}
StatusServer-->>Dropzone: {status: 'processing'}
end
Dropzone->>StatusServer: GET /status/{file_id}
StatusServer-->>Dropzone: {status: 'complete'} or {status: 'error'}
alt Completion
Dropzone->>Dropzone: onUploadComplete(data)
else Error
Dropzone->>Dropzone: onError(data)
end
Dropzone->>Dropzone: Clear polling interval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/inputs/dropzone/index.js`:
- Around line 321-327: Wrap the 202 handling in a try/catch when calling
JSON.parse(xhr.responseText) and call the component's onError callback on parse
failure; after parsing, validate that uploadResponse.file_id exists and call
onError if missing instead of starting the poll; avoid constructing the status
endpoint by appending to this.props.config.postUrl — either read a dedicated
statusUrl/statusPath config or sanitize postUrl (remove trailing slash and strip
query string) before calling _this.pollUploadStatus(fileId, baseUrl); update the
branch that currently references xhr, uploadResponse.file_id,
_this.pollUploadStatus and this.props.config.postUrl to perform these checks and
error reports before initiating polling.
- Around line 34-68: The current pollUploadStatus implementation overwrites a
single this._pollInterval, aborts polling on any transient error, fails to check
response.ok, and uses setInterval with async work causing overlapping calls; fix
by switching to a per-file map (e.g., this._pollIntervals[fileId]) and a
self-scheduling loop (use setTimeout recursion) inside pollUploadStatus that
checks response.ok before parsing, treats non‑ok or fetch/json errors as
retriable (only stop on maxAttempts or when data.status === 'error' or
'complete'), and clears and deletes the specific this._pollIntervals[fileId]
when finished; also update componentWillUnmount to iterate this._pollIntervals
and clear all timers, and keep using onUploadComplete/onError and getAccessToken
as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f97cad44-77e2-4a17-9295-3928bf82e2a7
📒 Files selected for processing (2)
package.jsonsrc/components/inputs/dropzone/index.js
| pollUploadStatus(fileId, baseUrl) { | ||
| const statusUrl = `${baseUrl}/status/${fileId}`; | ||
| const maxAttempts = 300; // 10 minutes at 2s intervals | ||
| let attempts = 0; | ||
|
|
||
| this._pollInterval = setInterval(async () => { | ||
| attempts++; | ||
| if (attempts > maxAttempts) { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = null; | ||
| this.onError({ message: 'Upload timed out' }); | ||
| return; | ||
| } | ||
| try { | ||
| const accessToken = await getAccessToken(); | ||
| const response = await fetch(statusUrl, { | ||
| headers: { 'Authorization': `Bearer ${accessToken}` } | ||
| }); | ||
| const data = await response.json(); | ||
| if (data.status === 'complete') { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = null; | ||
| this.onUploadComplete(data); | ||
| } else if (data.status === 'error') { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = null; | ||
| this.onError(data); | ||
| } | ||
| } catch (error) { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = null; | ||
| this.onError(error); | ||
| } | ||
| }, 2000); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for callers configuring parallelUploads / multi-file usage of DropzoneJS
rg -nP -C3 '\b(DropzoneJS|djsConfig|parallelUploads|maxFiles|uploadMultiple)\b'Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 23442
🏁 Script executed:
# Search for where pollUploadStatus is called
rg -nP 'pollUploadStatus' src/Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 271
🏁 Script executed:
# Search for HTTP 202 or async upload handling in the dropzone component
rg -nP '202|async.*upload' src/components/inputs/dropzone/Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 173
🏁 Script executed:
# Look for how the dropzone initializes defaults and what parallelUploads setting is used
rg -nP 'parallelUploads|Dropzone\(' src/components/inputs/dropzone/ -A 3 -B 3Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 1208
🏁 Script executed:
# Get context around line 326 where pollUploadStatus is called
sed -n '310,340p' src/components/inputs/dropzone/index.jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 1265
🏁 Script executed:
# Get the getDjsConfig method
sed -n '70,130p' src/components/inputs/dropzone/index.jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 2284
Polling mechanism has several robustness gaps that will bite in production.
-
Concurrent uploads clobber
this._pollInterval. Dropzone.js defaults toparallelUploads: 2, allowing multiple files to upload in parallel. When both files receive HTTP 202 responses, both callpollUploadStatus, overwritingthis._pollInterval. The first file's interval is orphaned and keeps running, callingonUploadComplete/onErrorspuriously. Only the most recent interval is cleared on unmount. Use a map (this._pollIntervals[fileId]) to track one interval per file and clean all on unmount. -
Any transient failure aborts polling. A single network blip, a 5xx error, or a non-JSON response body (causing
response.json()to throw) immediately clears the interval and firesonError. For a 10‑minute async upload, transient failures are almost guaranteed. Treat request failures as retriable; only give up aftermaxAttemptsor an explicitstatus === 'error'. -
No
response.okcheck. A401/403(e.g., token expired mid-poll) or500with a JSON body is currently parsed as a normal status payload. Checkresponse.okbefore parsing and treat non-2xx responses as transient failures. -
setInterval+asyncoverlaps. IfgetAccessToken()+fetchtakes >2s (plausible under load), the next tick fires before the previous completes, issuing overlapping token requests and status calls. Use self-schedulingsetTimeoutrecursion so the next poll is scheduled only after the previous one resolves.
♻️ Sketch of a safer polling loop
- pollUploadStatus(fileId, baseUrl) {
- const statusUrl = `${baseUrl}/status/${fileId}`;
- const maxAttempts = 300; // 10 minutes at 2s intervals
- let attempts = 0;
-
- this._pollInterval = setInterval(async () => {
- attempts++;
- if (attempts > maxAttempts) {
- clearInterval(this._pollInterval);
- this._pollInterval = null;
- this.onError({ message: 'Upload timed out' });
- return;
- }
- try {
- const accessToken = await getAccessToken();
- const response = await fetch(statusUrl, {
- headers: { 'Authorization': `Bearer ${accessToken}` }
- });
- const data = await response.json();
- if (data.status === 'complete') {
- clearInterval(this._pollInterval);
- this._pollInterval = null;
- this.onUploadComplete(data);
- } else if (data.status === 'error') {
- clearInterval(this._pollInterval);
- this._pollInterval = null;
- this.onError(data);
- }
- } catch (error) {
- clearInterval(this._pollInterval);
- this._pollInterval = null;
- this.onError(error);
- }
- }, 2000);
- }
+ pollUploadStatus(fileId, baseUrl) {
+ if (!fileId) {
+ this.onError({ message: 'Missing file_id for async upload' });
+ return;
+ }
+ const statusUrl = `${baseUrl}/status/${fileId}`;
+ const maxAttempts = 300; // 10 minutes at 2s intervals
+ let attempts = 0;
+ this._pollTimers = this._pollTimers || new Map();
+
+ const stop = () => {
+ const t = this._pollTimers.get(fileId);
+ if (t) { clearTimeout(t); this._pollTimers.delete(fileId); }
+ };
+
+ const tick = async () => {
+ if (++attempts > maxAttempts) {
+ stop();
+ this.onError({ message: 'Upload timed out' });
+ return;
+ }
+ try {
+ const accessToken = await getAccessToken();
+ const response = await fetch(statusUrl, {
+ headers: { 'Authorization': `Bearer ${accessToken}` }
+ });
+ if (response.ok) {
+ const data = await response.json();
+ if (data.status === 'complete') { stop(); this.onUploadComplete(data); return; }
+ if (data.status === 'error') { stop(); this.onError(data); return; }
+ }
+ // transient: fall through to reschedule
+ } catch (e) {
+ // transient: log and reschedule
+ console.warn('DropzoneJS::pollUploadStatus transient error', e);
+ }
+ this._pollTimers.set(fileId, setTimeout(tick, 2000));
+ };
+
+ this._pollTimers.set(fileId, setTimeout(tick, 2000));
+ }And update componentWillUnmount:
- if (this._pollInterval) {
- clearInterval(this._pollInterval);
- this._pollInterval = null;
- }
+ if (this._pollTimers) {
+ this._pollTimers.forEach((t) => clearTimeout(t));
+ this._pollTimers.clear();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pollUploadStatus(fileId, baseUrl) { | |
| const statusUrl = `${baseUrl}/status/${fileId}`; | |
| const maxAttempts = 300; // 10 minutes at 2s intervals | |
| let attempts = 0; | |
| this._pollInterval = setInterval(async () => { | |
| attempts++; | |
| if (attempts > maxAttempts) { | |
| clearInterval(this._pollInterval); | |
| this._pollInterval = null; | |
| this.onError({ message: 'Upload timed out' }); | |
| return; | |
| } | |
| try { | |
| const accessToken = await getAccessToken(); | |
| const response = await fetch(statusUrl, { | |
| headers: { 'Authorization': `Bearer ${accessToken}` } | |
| }); | |
| const data = await response.json(); | |
| if (data.status === 'complete') { | |
| clearInterval(this._pollInterval); | |
| this._pollInterval = null; | |
| this.onUploadComplete(data); | |
| } else if (data.status === 'error') { | |
| clearInterval(this._pollInterval); | |
| this._pollInterval = null; | |
| this.onError(data); | |
| } | |
| } catch (error) { | |
| clearInterval(this._pollInterval); | |
| this._pollInterval = null; | |
| this.onError(error); | |
| } | |
| }, 2000); | |
| } | |
| pollUploadStatus(fileId, baseUrl) { | |
| if (!fileId) { | |
| this.onError({ message: 'Missing file_id for async upload' }); | |
| return; | |
| } | |
| const statusUrl = `${baseUrl}/status/${fileId}`; | |
| const maxAttempts = 300; // 10 minutes at 2s intervals | |
| let attempts = 0; | |
| this._pollTimers = this._pollTimers || new Map(); | |
| const stop = () => { | |
| const t = this._pollTimers.get(fileId); | |
| if (t) { clearTimeout(t); this._pollTimers.delete(fileId); } | |
| }; | |
| const tick = async () => { | |
| if (++attempts > maxAttempts) { | |
| stop(); | |
| this.onError({ message: 'Upload timed out' }); | |
| return; | |
| } | |
| try { | |
| const accessToken = await getAccessToken(); | |
| const response = await fetch(statusUrl, { | |
| headers: { 'Authorization': `Bearer ${accessToken}` } | |
| }); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| if (data.status === 'complete') { stop(); this.onUploadComplete(data); return; } | |
| if (data.status === 'error') { stop(); this.onError(data); return; } | |
| } | |
| // transient: fall through to reschedule | |
| } catch (e) { | |
| // transient: log and reschedule | |
| console.warn('DropzoneJS::pollUploadStatus transient error', e); | |
| } | |
| this._pollTimers.set(fileId, setTimeout(tick, 2000)); | |
| }; | |
| this._pollTimers.set(fileId, setTimeout(tick, 2000)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/inputs/dropzone/index.js` around lines 34 - 68, The current
pollUploadStatus implementation overwrites a single this._pollInterval, aborts
polling on any transient error, fails to check response.ok, and uses setInterval
with async work causing overlapping calls; fix by switching to a per-file map
(e.g., this._pollIntervals[fileId]) and a self-scheduling loop (use setTimeout
recursion) inside pollUploadStatus that checks response.ok before parsing,
treats non‑ok or fetch/json errors as retriable (only stop on maxAttempts or
when data.status === 'error' or 'complete'), and clears and deletes the specific
this._pollIntervals[fileId] when finished; also update componentWillUnmount to
iterate this._pollIntervals and clear all timers, and keep using
onUploadComplete/onError and getAccessToken as before.
There was a problem hiding this comment.
@smarcet this seems a valid argument: Concurrent uploads clobber this._pollInterval. Dropzone.js defaults to parallelUploads: 2, allowing multiple files to upload in parallel. When both files receive HTTP 202 responses, both call pollUploadStatus, overwriting this._pollInterval. The first file's interval is orphaned and keeps running, calling onUploadComplete/onError spuriously. Only the most recent interval is cleared on unmount. Use a map (this._pollIntervals[fileId]) to track one interval per file and clean all on unmount.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| else if(xhr?.status == 202) { | ||
| // Async upload: server accepted the file, poll for completion | ||
| let uploadResponse = JSON.parse(xhr.responseText); | ||
| const fileId = uploadResponse.file_id; | ||
| const baseUrl = _this.props.config.postUrl; | ||
| _this.pollUploadStatus(fileId, baseUrl); | ||
| } |
There was a problem hiding this comment.
Validate the 202 payload before starting the poll.
JSON.parse(xhr.responseText)can throw on an empty/non-JSON body; the exception would propagate out ofxhr.onloadand noonErrorwould be reported to the consumer. Wrap in try/catch and callonErroron failure.- No guard for a missing
file_id. If the server returns 202 withoutfile_id, polling will hit${baseUrl}/status/undefinedfor 10 minutes before timing out. baseUrl = this.props.config.postUrlis concatenated as${baseUrl}/status/${fileId}. IfpostUrlever contains a query string or trailing slash, the resulting URL will be malformed. Consider using a dedicatedstatusUrl/statusPathconfig field rather than deriving it frompostUrl.
🛡️ Proposed guard
- else if(xhr?.status == 202) {
- // Async upload: server accepted the file, poll for completion
- let uploadResponse = JSON.parse(xhr.responseText);
- const fileId = uploadResponse.file_id;
- const baseUrl = _this.props.config.postUrl;
- _this.pollUploadStatus(fileId, baseUrl);
- }
+ else if(xhr?.status == 202) {
+ // Async upload: server accepted the file, poll for completion
+ let uploadResponse;
+ try {
+ uploadResponse = JSON.parse(xhr.responseText);
+ } catch (parseErr) {
+ _this.onError(parseErr, xhr?.status);
+ return;
+ }
+ const fileId = uploadResponse?.file_id;
+ if (!fileId) {
+ _this.onError({ message: 'Async upload accepted (202) but response is missing file_id' }, xhr?.status);
+ return;
+ }
+ _this.pollUploadStatus(fileId, _this.props.config.postUrl);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else if(xhr?.status == 202) { | |
| // Async upload: server accepted the file, poll for completion | |
| let uploadResponse = JSON.parse(xhr.responseText); | |
| const fileId = uploadResponse.file_id; | |
| const baseUrl = _this.props.config.postUrl; | |
| _this.pollUploadStatus(fileId, baseUrl); | |
| } | |
| else if(xhr?.status == 202) { | |
| // Async upload: server accepted the file, poll for completion | |
| let uploadResponse; | |
| try { | |
| uploadResponse = JSON.parse(xhr.responseText); | |
| } catch (parseErr) { | |
| _this.onError(parseErr, xhr?.status); | |
| return; | |
| } | |
| const fileId = uploadResponse?.file_id; | |
| if (!fileId) { | |
| _this.onError({ message: 'Async upload accepted (202) but response is missing file_id' }, xhr?.status); | |
| return; | |
| } | |
| _this.pollUploadStatus(fileId, _this.props.config.postUrl); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/inputs/dropzone/index.js` around lines 321 - 327, Wrap the 202
handling in a try/catch when calling JSON.parse(xhr.responseText) and call the
component's onError callback on parse failure; after parsing, validate that
uploadResponse.file_id exists and call onError if missing instead of starting
the poll; avoid constructing the status endpoint by appending to
this.props.config.postUrl — either read a dedicated statusUrl/statusPath config
or sanitize postUrl (remove trailing slash and strip query string) before
calling _this.pollUploadStatus(fileId, baseUrl); update the branch that
currently references xhr, uploadResponse.file_id, _this.pollUploadStatus and
this.props.config.postUrl to perform these checks and error reports before
initiating polling.
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
other than coderabbit comments, I'm good
| pollUploadStatus(fileId, baseUrl) { | ||
| const statusUrl = `${baseUrl}/status/${fileId}`; | ||
| const maxAttempts = 300; // 10 minutes at 2s intervals | ||
| let attempts = 0; | ||
|
|
||
| this._pollInterval = setInterval(async () => { | ||
| attempts++; | ||
| if (attempts > maxAttempts) { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = null; | ||
| this.onError({ message: 'Upload timed out' }); | ||
| return; | ||
| } | ||
| try { | ||
| const accessToken = await getAccessToken(); | ||
| const response = await fetch(statusUrl, { | ||
| headers: { 'Authorization': `Bearer ${accessToken}` } | ||
| }); | ||
| const data = await response.json(); | ||
| if (data.status === 'complete') { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = null; | ||
| this.onUploadComplete(data); | ||
| } else if (data.status === 'error') { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = null; | ||
| this.onError(data); | ||
| } | ||
| } catch (error) { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = null; | ||
| this.onError(error); | ||
| } | ||
| }, 2000); | ||
| } |
There was a problem hiding this comment.
@smarcet this seems a valid argument: Concurrent uploads clobber this._pollInterval. Dropzone.js defaults to parallelUploads: 2, allowing multiple files to upload in parallel. When both files receive HTTP 202 responses, both call pollUploadStatus, overwriting this._pollInterval. The first file's interval is orphaned and keeps running, calling onUploadComplete/onError spuriously. Only the most recent interval is cleared on unmount. Use a map (this._pollIntervals[fileId]) to track one interval per file and clean all on unmount.
2 participants
ref: https://app.clickup.com/t/86b9gt7w3
Summary by CodeRabbit
Bug Fixes
Chores