OpenConceptLab/ocl_issues#2454 | Expect 302/303 for export download#30
OpenConceptLab/ocl_issues#2454 | Expect 302/303 for export download#30snyaggarwal merged 1 commit intomasterfrom
Conversation
paynejd
left a comment
There was a problem hiding this comment.
Review
Overall this is a solid cleanup. The async/await migration, better error handling, and defensive filename parsing are all good improvements. A few things to flag:
Browser auto-follows the 302 — the code never actually sees it
Worth noting for future readers: in a browser environment, XHR/fetch always follows redirects transparently. So when the API returns 302→S3, the JavaScript code sees the final 200 from S3 with blob data — not a 302. That's why the status === 200 branch still handles the download. The PR title says "Expect 302/303" but really it's "expect the browser to follow a 302 and give us the blob from S3." This is correct behavior, just potentially confusing if someone reads the title and then looks for explicit 302 handling.
CORS on S3 is a prerequisite
The redirect sends the browser directly to S3. The S3 bucket must have a CORS policy allowing the oclweb2 origin, otherwise the redirected request will fail silently (caught by the catch block, but the user just sees a generic error). Worth validating this is configured before merging.
Additionally, if you want Content-Disposition from S3 to be readable by JavaScript (for the filename), the S3 CORS config needs Access-Control-Expose-Headers: Content-Disposition. Without it, the browser hides that header from JS and the fallback export.zip filename will always be used. That may be acceptable, but if you want accurate filenames, the CORS expose is needed.
Authorization header is safely stripped
On cross-origin redirect (API → S3), the browser strips custom headers like Authorization. This is fine because the S3 URL is pre-signed — auth is in the query params, not headers.
Small items
errorDetails: false→errorDetails: null— good, semantically correctlink.remove()— fixes a DOM leak the old code hadsetTimeout(() => revokeObjectURL, 1000)— good, prevents revoking before download starts- The RFC 5987
filename*=UTF-8''...parsing is a nice robustness improvement
paynejd
left a comment
There was a problem hiding this comment.
LGTM assuming CORS is taken care of
Linked Issue
Closes OpenConceptLab/ocl_issues#2454