feat: replace UploadInputV2 with UploadInputV3#891
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR migrates upload consumers from UploadInputV2 to UploadInputV3, deletes the local MuiFormikUpload adapter, updates imports and sponsor upload wiring to use the library-provided MuiFormikUpload, simplifies an event-comment component's lifecycle/state handling, and updates a test to match the new upload integration. ChangesUpload/UploadAdapter migration
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
priscila-moneo
requested review from
martinquiroga-exo,
santipalenque and
smarcet
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/forms/event-comment-form.js (1)
136-146:⚠️ Potential issue | 🟡 MinorMinor: body field no longer surfaces validation errors.
The
errorprop was removed from thebodyfield per the summary, but the currenttextareais a plain HTML element that never rendered an error message anyway — so removal is a no-op here. If body validation errors are expected server-side (e.g., length limits), consider surfacing them viahasErrors("body", errors)to keep UX consistent withowner_full_nameabove.💡 Optional: surface body validation errors
<div className="col-md-12"> <label> {T.translate("edit_event_comment.body")}</label> <textarea id="body" value={entity.body} onChange={this.handleChange} className="form-control" /> + {hasErrors("body", errors) && ( + <p className="error-label">{hasErrors("body", errors)}</p> + )} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forms/event-comment-form.js` around lines 136 - 146, The body textarea currently never shows validation errors because it is a plain HTML element; update the JSX in the EventCommentForm to render validation feedback the same way owner_full_name does: use hasErrors("body", errors) to set an error/invalid state and render the corresponding error message from errors for entity.body, keeping the id="body", value={entity.body} and onChange={this.handleChange} intact so server-side length/validation messages surface to the user.src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js (2)
41-47:⚠️ Potential issue | 🟠 MajorDropping
onImageDeletedcauses server-side image files to leak.Both parent pages pass
onImageDeleted(inventory-list-page.js line 421, form-template-item-list-page.js line 318), but the dialog no longer destructures it or invokes it. When users delete images in the UI,MuiFormikUploadremoves them from the Formik array, which are then filtered out of thePUTrequest bynormalizeEntity. However, this only removes the image reference from the inventory item—the actual image file on the server is never deleted, leaving orphaned files on disk.Restore
onImageDeletedto the component signature and wire it into the image deletion handler (likely withinMuiFormikUpload'sonRemovecallback if supported, or via custom state tracking), ensuringdeleteInventoryItemImage/deleteItemImageare explicitly called when users remove images.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js` around lines 41 - 47, The SponsorItemDialog component signature dropped onImageDeleted which causes server-side image files to leak; restore onImageDeleted in the SponsorItemDialog props and wire it into the image-removal flow used by MuiFormikUpload so the parent-provided handler is invoked when an image is removed. Specifically, re-add onImageDeleted to the destructured props in SponsorItemDialog, locate the image remove callback used by MuiFormikUpload (or the component/handler that calls onRemove), and call onImageDeleted (or call deleteInventoryItemImage/deleteItemImage via that prop) with the relevant image id/path when an image is removed so that normalizeEntity still filters references but the server-side delete is invoked. Ensure the call uses the same identifiers expected by the parent pages that pass onImageDeleted so the existing deleteInventoryItemImage/deleteItemImage logic runs.
72-78:⚠️ Potential issue | 🟠 MajorMost of
mediaTypeis unused; file size and format validation are not enforced.The
mediaTypeobject (lines 72–78) constructs constraints for file size and allowed formats, but onlymax_uploads_qtyis passed toMuiFormikUploadvia themaxFilesprop. TheMAX_INVENTORY_IMAGE_UPLOAD_SIZEandALLOWED_INVENTORY_IMAGE_FORMATSvalues are not used anywhere—MuiFormikUploaddoes not accept amediaTypeprop or equivalent configuration for these constraints.The Formik validation schema (line 60) defines
imagesas a basicyup.array()with no file size or extension validation. Either wire the constraints through props ifMuiFormikUploadsupports them (review its prop interface), or remove the unused configuration and implement server-side validation.Also applies to: 220-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js` around lines 72 - 78, mediaType is building file-size and format constraints (using MAX_INVENTORY_IMAGE_UPLOAD_SIZE and ALLOWED_INVENTORY_IMAGE_FORMATS) but those values are never applied: only max_uploads_qty is passed to MuiFormikUpload and the Formik/Yup schema for images is just yup.array(), so file size/extension checks aren’t enforced. Fix by either: (A) if MuiFormikUpload supports props for max file size and allowed extensions, pass MAX_INVENTORY_IMAGE_UPLOAD_SIZE and ALLOWED_INVENTORY_IMAGE_FORMATS into the component alongside maxFiles (update the MuiFormikUpload usage), or (B) remove the unused mediaType and add client-side validation to the Formik schema (the images field currently defined as yup.array()) to validate each file’s size and extension against MAX_INVENTORY_IMAGE_UPLOAD_SIZE and ALLOWED_INVENTORY_IMAGE_FORMATS, and ensure server-side validation is implemented as a fallback; update or remove mediaType accordingly and keep only the used constraints.
🤖 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/pages/sponsors-global/form-templates/sponsor-inventory-popup.js`:
- Line 21: The import in
src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js currently
brings in MuiFormikUpload but the PR intends to standardize on UploadInputV3;
update the import to use UploadInputV3 (or explicitly document why
Formik-specific MuiFormikUpload is required in the PR description), and then
update any usage of MuiFormikUpload in this file (e.g., where the component is
rendered) to the UploadInputV3 component name so the file aligns with the other
migrated files (or add a clear comment in the file/PR explaining the deliberate
Formik choice if you must keep MuiFormikUpload).
---
Outside diff comments:
In `@src/components/forms/event-comment-form.js`:
- Around line 136-146: The body textarea currently never shows validation errors
because it is a plain HTML element; update the JSX in the EventCommentForm to
render validation feedback the same way owner_full_name does: use
hasErrors("body", errors) to set an error/invalid state and render the
corresponding error message from errors for entity.body, keeping the id="body",
value={entity.body} and onChange={this.handleChange} intact so server-side
length/validation messages surface to the user.
In `@src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js`:
- Around line 41-47: The SponsorItemDialog component signature dropped
onImageDeleted which causes server-side image files to leak; restore
onImageDeleted in the SponsorItemDialog props and wire it into the image-removal
flow used by MuiFormikUpload so the parent-provided handler is invoked when an
image is removed. Specifically, re-add onImageDeleted to the destructured props
in SponsorItemDialog, locate the image remove callback used by MuiFormikUpload
(or the component/handler that calls onRemove), and call onImageDeleted (or call
deleteInventoryItemImage/deleteItemImage via that prop) with the relevant image
id/path when an image is removed so that normalizeEntity still filters
references but the server-side delete is invoked. Ensure the call uses the same
identifiers expected by the parent pages that pass onImageDeleted so the
existing deleteInventoryItemImage/deleteItemImage logic runs.
- Around line 72-78: mediaType is building file-size and format constraints
(using MAX_INVENTORY_IMAGE_UPLOAD_SIZE and ALLOWED_INVENTORY_IMAGE_FORMATS) but
those values are never applied: only max_uploads_qty is passed to
MuiFormikUpload and the Formik/Yup schema for images is just yup.array(), so
file size/extension checks aren’t enforced. Fix by either: (A) if
MuiFormikUpload supports props for max file size and allowed extensions, pass
MAX_INVENTORY_IMAGE_UPLOAD_SIZE and ALLOWED_INVENTORY_IMAGE_FORMATS into the
component alongside maxFiles (update the MuiFormikUpload usage), or (B) remove
the unused mediaType and add client-side validation to the Formik schema (the
images field currently defined as yup.array()) to validate each file’s size and
extension against MAX_INVENTORY_IMAGE_UPLOAD_SIZE and
ALLOWED_INVENTORY_IMAGE_FORMATS, and ensure server-side validation is
implemented as a fallback; update or remove mediaType accordingly and keep only
the used constraints.
🪄 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: 9254e0ee-ed4f-49cc-a14c-eda33e9d7051
📒 Files selected for processing (7)
src/components/forms/event-comment-form.jssrc/components/forms/event-material-form.jssrc/components/mui/formik-inputs/mui-formik-upload.jssrc/components/upload-dialog/index.jssrc/pages/sponsors-global/form-templates/sponsor-inventory-popup.jssrc/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-document-download-module.jssrc/pages/sponsors-global/page-templates/page-template-popup/page-template-module-form.test.js
💤 Files with no reviewable changes (2)
- src/pages/sponsors-global/page-templates/page-template-popup/page-template-module-form.test.js
- src/components/mui/formik-inputs/mui-formik-upload.js
| } from "@mui/material"; | ||
| import CloseIcon from "@mui/icons-material/Close"; | ||
| import { UploadInputV2 } from "openstack-uicore-foundation/lib/components"; | ||
| import { MuiFormikUpload } from "openstack-uicore-foundation/lib/components"; |
There was a problem hiding this comment.
Inconsistent with PR intent — this file uses MuiFormikUpload, not UploadInputV3.
The PR title and sibling files migrate UploadInputV2 → UploadInputV3, but here the replacement is MuiFormikUpload. If intentional (Formik-integrated variant), consider calling this out in the PR description; otherwise align on UploadInputV3 for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js` at line
21, The import in
src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js currently
brings in MuiFormikUpload but the PR intends to standardize on UploadInputV3;
update the import to use UploadInputV3 (or explicitly document why
Formik-specific MuiFormikUpload is required in the PR description), and then
update any usage of MuiFormikUpload in this file (e.g., where the component is
rendered) to the UploadInputV3 component name so the file aligns with the other
migrated files (or add a clear comment in the file/PR explaining the deliberate
Formik choice if you must keep MuiFormikUpload).
There was a problem hiding this comment.
@priscila-moneo this point the same question that i did please change it to v3
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!
priscila-moneo
force-pushed
the
feature/replace-v2-upload-for-v3-upload
branch
from
7c52cd0 to
244bdbf
Compare
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comments and please fix the merge conflicts
priscila-moneo
force-pushed
the
feature/replace-v2-upload-for-v3-upload
branch
from
244bdbf to
a820f85
Compare
3 participants
ref: https://app.clickup.com/t/86b9hy7n5
Summary by CodeRabbit
Refactor
Tests