Repository navigation
feat(uploads): select UploadInputV4 behind FILE_UPLOAD_USE_V4 - #1141
caseylocker wants to merge 7 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check |  |
This assessment has not completed. The check will be retried on the next pre-merge check run. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
cd8a61d to
c4f67aa
Compare
|
@smarcet ready for review. |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the feature-flagged upload migration.
No open issues 🎉
What changed in this PR
Adds a feature-flagged path to direct-to-storage uploads across six file-upload-api upload sites, retaining V3 chunked uploads by default. Enabling V4 remains gated on the backend deployment and a uicore removal-callback fix.
Changes:
- Introduces
FileUploadInput, selecting V4 only whenFILE_UPLOAD_USE_V4is"true". - Migrates company logos, event materials, upload dialogs, and badge feature images to the wrapper.
- Adds Uppy dependencies, build-time flag wiring, and tests for component selection and forwarded props.
| File | Description |
|---|---|
yarn.lock |
Locks Uppy dependencies and their transitive packages. |
src/components/upload-dialog/index.js |
Uses the shared upload wrapper. |
src/components/inputs/file-upload-input.js |
Selects V3 or V4 and configures upload endpoints. |
src/components/inputs/__tests__/file-upload-input.test.js |
Tests flag selection, endpoints, and forwarded props. |
src/components/forms/event-material-form.js |
Routes slide and media uploads through the wrapper. |
src/components/forms/company-form.js |
Routes both company logo uploads through the wrapper. |
src/components/forms/badge-feature-type-form.js |
Routes badge feature image uploads through the wrapper. |
src/app.js |
Exposes the build-time upload flag on window. |
package.json |
Adds Uppy peer dependencies required by V4. |
.env.example |
Adds the upload flag, unset by default. |
Review effort: Balanced
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
@caseylocker would we need this behavior also in sponsor-services or any other app ? If so we may want it in uicore
| const FileUploadInput = ({ parallelChunkUploads, ...rest }) => { | ||
| if (window.FILE_UPLOAD_USE_V4 === "true") { | ||
| return ( | ||
| <UploadInputV4 |
There was a problem hiding this comment.
Cancelling an in-flight upload behaves differently on the V4 path, and two call sites depend on the V3 behavior.
In V3, deleting a file that is still uploading goes through Dropzone's removedfile, which calls the caller's onRemove (upload-input-v3/index.js: eventHandlers = { removedfile: onRemove
}). In V4, handleDeleteUploading only calls uppy.removeFile: it never calls onRemove or onError.
That leaves the "uploading" flag stuck after onUploadStart has set it:
- badge-feature-type-form.js: handleRemoveImage is the only place that resets it on cancel (its comment says "also reached when cancelling an in-flight upload"). With the flag on,
isUploading stays true and Save stays disabled (edit-badge-feature-page.js:114) until a reload. - company-form.js: handleLogoRemove is what calls setIsSaving(false) for an existing company; with V4, isSaving stays true and handleSubmit returns early (line 148), so Save silently does
nothing.
Repro: set FILE_UPLOAD_USE_V4=true, edit a badge feature, start an image upload, delete the row while it's uploading, then try to save.
Suggested fix: restore V3 parity in uicore's V4 by notifying the caller when an in-flight file is cancelled. A dedicated onUploadCancel that FileUploadInput maps to the callers' reset is
safer than reusing onRemove, because on an existing company handleLogoRemove also fires the delete-logo API call. Then add a test that cancels with the flag on and checks that Save is
enabled again.
There was a problem hiding this comment.
Agreed on the behavior. This is gate 2 in the PR description: V4 never calls onRemove on a cancel, so the flag stays off everywhere until a uicore release with OpenStackweb/openstack-uicore-foundation#385 is pinned here.
On onUploadCancel vs onRemove: I kept onRemove in OpenStackweb/openstack-uicore-foundation#385 for three reasons.
- It is what V3 does today. Dropzone's
removedfilealready calls the caller'sonRemoveon a cancel, so this restores parity and keeps the "same props as V3" contract. - For an existing company the extra delete call is harmless.
canAdd={!formik.values.logo}means an upload can only be in flight when the form has no logo, and the endpoint just clears the logo field, so it is a no-op. - A new prop would need wiring at every call site, and each would map it to the same reset
onRemovealready does.
The form-level test you asked for is in 55c4e92 for the company form. The cancel behavior of the input itself is tested in OpenStackweb/openstack-uicore-foundation#385.
| onUploadComplete={handleLogoUploadComplete("logo")} | ||
| onUploadStart={() => setIsSaving(true)} | ||
| onError={() => setIsSaving(false)} | ||
| onRemove={handleLogoRemove("logo")} |
There was a problem hiding this comment.
@caseylocker Cancelling a logo upload on a new company (Add company page) leaves Save permanently disabled under V4, and uicore#385 does not fix this case.
onUploadStart sets isSaving to true (line 378). In V3, cancelling an in-flight upload resets it by accident:
removeFilecallscancelUpload, which emitscanceled.- Dropzone 5.7.2's default
canceledhandler re-emits it aserror(dropzone/dist/dropzone.js:937-938). - uicore's
DropzoneJSpasses everyerrortoprops.onError(dropzone/index.js:692-698), which here issetIsSaving(false). removedfilethen callsonRemove.
So V3 calls both onError and onRemove on a cancel.
V4 calls neither. OpenStackweb/openstack-uicore-foundation#385 adds only onRemove to handleDeleteUploading, not onError. That is the right call, since a cancel is not an error. But handleLogoRemove (line 94) resets isSaving only when initialEntity?.id is set:
const handleLogoRemove = (field) => () => {
formik.setFieldValue(field, "");
if (initialEntity?.id) {
setIsSaving(true);
...
.finally(() => setIsSaving(false));
}
};With id 0, isSaving stays true. Then Save is disabled (edit-company-page.js:169) and onSubmit returns early (if (isSaving) return), so the user has to reload. An existing company recovers through the removeLogo .finally, so the PR description's "Company form (existing company)" bullet doesn't cover this case. With the flag on and #385 pinned, the Add company form will still get stuck.
Repro (after #385 is pinned): set FILE_UPLOAD_USE_V4=true, open Companies → Add new, start a logo upload, delete the row while it is uploading, then try to save.
Suggested fix: reset isSaving on the no-id branch so the form doesn't depend on the upload input calling onError on a cancel. That matches the else branch of handleLogoUploadComplete above and handleRemoveImage in badge-feature-type-form.js:
const handleLogoRemove = (field) => () => {
formik.setFieldValue(field, "");
if (initialEntity?.id) {
setIsSaving(true);
const prevValue = formik.values[field];
onRemove(initialEntity, field)
.catch(() => formik.setFieldValue(field, prevValue))
.finally(() => setIsSaving(false));
} else {
setIsSaving(false);
}
};Please add a test for it too: with the flag set to "true", a new company cancels an in-flight logo upload and Save is enabled again.
There was a problem hiding this comment.
Confirmed, and fixed in 55c4e92. With id 0, handleLogoRemove never reset isSaving, and under V4 nothing else does, since OpenStackweb/openstack-uicore-foundation#385 adds onRemove on cancel but not onError.
I took your else branch as written. It matches the else in handleLogoUploadComplete, and the form no longer depends on the input reporting a cancel as an error.
Test added in company-form.test.js: with the flag set to "true", a new company starts a logo upload, cancels it, and the saving state clears without calling the delete API. The upload input is mocked in that file, so the test drives the V4 callback contract (onUploadStart, then onRemove, no onError) and not Uppy itself. It fails with the else branch removed.
I also added the new-company case to the PR description under gate 2.
|
@santipalenque on whether this belongs in uicore: sponsor-services has one upload site on file-upload-api today ( I would keep the selector out of uicore though. It is about 20 lines that read two app-level globals ( |
|
@smarcet changes are made and it's ready for another look. Many thanks. |
55c4e92 to
33a48a6
Compare
|
@smarcet bumped to uicore 5.0.75 and rebased onto master. Heads up that jest is red on this PR for a reason unrelated to the upload change: 5.0.74 and later replace the MuiTable Edit and Delete icon columns with the row actions menu (OpenStackweb/openstack-uicore-foundation#380), and four sponsor table suites still click the old buttons (show-pages-list-page, cart-view, sponsor-forms-tab, sponsor-pages-tab). #1147 updates exactly those, so this goes green once #1147 merges and this branch is rebased onto it. Those 12 tests pass on master's 5.0.73-beta.1 pin. |
… deleted responsive options
5.0.74 and later replace the MuiTable Edit and Delete icon columns with a row actions menu and pass the column count to a function child, which the cart total row and the sponsor table tests rely on.
|
@caseylocker #1153 bumps openstack-uicore-foundation to 5.0.75 and carries the row-actions test fixes from #1147, with CI green. It replaces #1147 as the dependency for this PR. Please rebase this branch onto #1153 (or onto master once it merges). Your |
| import React from "react"; | ||
| import PropTypes from "prop-types"; | ||
| import UploadInputV3 from "openstack-uicore-foundation/lib/components/inputs/upload-input-v3"; | ||
| import UploadInputV4 from "openstack-uicore-foundation/lib/components/inputs/upload-input-v4"; |
There was a problem hiding this comment.
@caseylocker UploadInputV4's stylesheet is never loaded, so with FILE_UPLOAD_USE_V4="true" every V4 site renders the legacy dropzone instead of V3's look.
summit-admin loads uicore styles only through openstack-uicore-foundation/lib/css/components/index.css (src/index.js:26). In 5.0.75 that file has the .upload-input-v3 ... rules but no .upload-input-v4 rules; V4 isn't barrel-exported, so its CSS ships separately in lib/css/components/inputs/upload-input-v4.css, and the V4 JS bundle doesn't inject it. What V4's .dropzone picks up instead is the legacy global .dropzone{border:2px dashed gray;height:150px} rule: no rounded 1px border, no min-height/flex layout, and no drag-hover highlight. A jsdom computed-style check gives 2px dashed gray with index.css alone and 1px dashed rgb(208, 213, 221), 8px radius, min-height: 160px once upload-input-v4.css is added.
Suggested fix: import it next to the component (the package has no exports map, so the deep import resolves):
import UploadInputV4 from "openstack-uicore-foundation/lib/components/inputs/upload-input-v4";
import "openstack-uicore-foundation/lib/css/components/inputs/upload-input-v4.css";It doesn't affect the flag-off path, so it can land any time before the flag is turned on.
There was a problem hiding this comment.
@smarcet fixed in f4749f1, and the branch is rebased onto #1153 (the duplicate bump commit dropped out as you said).
Checked with FILE_UPLOAD_USE_V4=true on Add Company: yarn build emits the .upload-input-v4 .dropzone rules, and both logo dropzones compute to 1px dashed rgb(208, 213, 221), 8px radius, min-height: 160px, flex. Full jest suite passes locally (265 suites), including the four sponsor table suites.
One more V4 styling defect showed up in the same browser check, and it needs your ruling on where to fix it:
The native file input is visible above each V4 dropzone ("Choose File / No file chosen").
- V4 renders
<input type="file" hidden>and relies on thehiddenattribute. - summit-admin loads Bootstrap 3.3.7, where
input[type=file]{display:block}has higher specificity than[hidden]{display:none}, so the input computes todisplay: block. - It only shows with the flag on, so it does not affect the flag-off path.
Options:
- Fix it here: a rule in summit-admin such as
.upload-input-v4 input[type="file"][hidden] { display: none; }. - Fix it in uicore: V4 hides the input with an inline
display: none(or its own stylesheet rule), so every host app with Bootstrap 3 is covered.
Not changed in this push. Which one do you want?
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@caseylocker please review #1141 (comment)
Route the five file-upload-api upload sites (company logo and big logo, event material slide and media upload, upload dialog) through a FileUploadInput wrapper. With FILE_UPLOAD_USE_V4="true" it renders uicore's UploadInputV4 against the direct-upload API base; anything else keeps UploadInputV3 on the chunked endpoint, so rollback is an env flip. Adds @uppy/core and @uppy/aws-s3, which UploadInputV4 takes as peers. Needs the uicore release that ships upload-input-v4. ClickUp: https://app.clickup.com/t/86bcgxnfp
The flag is inlined by Dotenv at build time, so rolling back means setting it to anything but "true" and rebuilding summit-admin, not a runtime flip. Also drops a redundant prop-forwarding assertion from the wrapper test. ClickUp: https://app.clickup.com/t/86bcgxnfp
…dInput The badge feature form (added on master after this branch was opened) posts its image to file-upload-api with UploadInputV3 directly. Route it through the wrapper like the other five sites so FILE_UPLOAD_USE_V4 covers it too. With the flag off it renders V3 with the same postUrl and djsConfig as before. ClickUp: https://app.clickup.com/t/86bcgxnfp
…o upload UploadInputV4 reports a cancelled upload through onRemove only. V3 also reached onError, because Dropzone re-emits canceled as error, and that was the only thing resetting isSaving for a company with no id. Reset it in handleLogoRemove so the form does not depend on the input calling onError.
uicore's components/index.css carries the V3 rules but not V4's, and the V4 bundle does not inject its own CSS, so with FILE_UPLOAD_USE_V4 on the dropzone fell back to the legacy global .dropzone rule.
33a48a6 to
f4749f1
Compare
ref: https://app.clickup.com/t/86bcgxnfp
Phase 4 of the direct-to-storage uploads SDS. Routes the six file-upload-api
upload sites (company logo and big logo, event material slide and media, upload
dialog, badge feature image) through a FileUploadInput wrapper that renders
uicore's UploadInputV4 when FILE_UPLOAD_USE_V4="true", and UploadInputV3 on the
chunked endpoint otherwise. event-category-form stays on V3 because it posts to
summit-api.
Adds @uppy/core and @uppy/aws-s3 (UploadInputV4 peers). Pins uicore 5.0.75,
which ships UploadInputV4 with the removal fix
(OpenStackweb/openstack-uicore-foundation#385).
The flag is inlined by Dotenv at build time, so rollback is changing the env and
rebuilding summit-admin.
Merging vs. turning the flag on
With the flag unset, every site renders V3 with the same postUrl and djsConfig as
master, so this can merge before the flag is turned on anywhere. Turning the flag on
in an environment needs both:
file-upload-api Phase 1 deployed there. Both Phase 1 PRs are merged
(file-upload-api FIX: ui tweaks #14, file-upload-api-docker Fixed access route #1). As of 2026-11-05 Phase 1 is
live on dev but not on prod (
POST /api/v1/files/uploads/singlereturns 404 onprod). The deploy script baked into the running dev image still runs FLUSHALL
until the image is rebuilt with Fixed access route #1, so nobody should run the dev deploy script
during dev testing with the flag on.
A uicore fix for UploadInputV4 removal. Done: 5.0.75 is pinned here. Fix:
fix(upload-input-v4): call onRemove when a row is deleted OpenStackweb/openstack-uicore-foundation#385
(ClickUp https://app.clickup.com/t/86bcz7mww). Before 5.0.75, V4 never called
onRemovewhen a row was deleted or an in-flight upload was cancelled:handleDeleteUploadingand itsfile-removedlistener only dropped the row. V3reaches
onRemovethrough dropzone'sremovedfileevent. With the flag on andan older uicore:
still submits it.
in its saving state.
V4 does not call
onErroron a cancel. Fixed here inhandleLogoRemove.The flag still stays off everywhere, staging included, until gate 1 is cleared
in that environment.
CI: jest is red until #1147 merges
uicore 5.0.74 and later replace the MuiTable Edit and Delete icon columns with a
row actions menu (OpenStackweb/openstack-uicore-foundation#380). Four sponsor
table suites still click the old buttons and fail on 5.0.75 (12 tests):
show-pages-list-page, cart-view, sponsor-forms-tab, sponsor-pages-tab. They pass
on master's 5.0.73-beta.1 pin and are unrelated to the upload change. #1147
updates exactly those suites, so this PR needs #1147 merged and a rebase onto it
to go green.