fix: add missing file remove calls from inventory popup - #1002
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds authenticated sponsor form image deletion thunks, expands customized form item image retrieval, routes image deletion callbacks through upload components and dialogs, and updates Redux state after deletion. ChangesImage deletion feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MuiFormikUpload
participant SponsorInventoryDialog
participant Page
participant SponsorFormsActions
participant API
participant ReduxStore
MuiFormikUpload->>SponsorInventoryDialog: delete image id
SponsorInventoryDialog->>Page: invoke onImageDeleted with image id
Page->>SponsorFormsActions: invoke deletion thunk with item context
SponsorFormsActions->>API: send authenticated DELETE request
API-->>SponsorFormsActions: return deletion response
SponsorFormsActions->>ReduxStore: dispatch image-deleted action
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/sponsor-forms-actions.js (1)
1448-1465: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCall
saveItemImageshere —saveImagesis undefined in this module, so this branch throws aReferenceErrorafter the POST succeeds and skips the success snackbar/return flow. PassformId,response.id, and the images array tosaveItemImagesinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/actions/sponsor-forms-actions.js` around lines 1448 - 1465, The success branch in the sponsor form item save flow is calling an undefined saveImages helper, which causes a ReferenceError after the POST succeeds. Update the promise chain in the action that handles the POST response to use saveItemImages instead, and pass the formId, response.id, and the images array so the success snackbar and return flow complete normally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actions/sponsor-forms-actions.js`:
- Around line 1627-1649: The removeSponsorCustomizedFormItemImages action is
using the whole-item deletion action for an image-only endpoint, which can cause
the item row to be removed instead of just the image. Update the deleteRequest
callback in removeSponsorCustomizedFormItemImages to dispatch a dedicated
image-delete action (or add one if missing) and ensure
sponsor-customized-form-items-list-reducer.js handles that image-specific action
separately from SPONSOR_CUSTOMIZED_FORM_ITEM_DELETED.
In `@src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js`:
- Around line 87-90: The optional onImageDeleted callback is being invoked
unconditionally in handleIDeleteImage, which can throw when the prop is omitted.
Update handleIDeleteImage in sponsor-inventory-popup.js to guard the callback
before calling it, keeping the image id check but only invoking onImageDeleted
when it is defined.
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js`:
- Around line 60-63: `handleDeleteImage` in `SponsorFormItemForm` should
defensively guard the `onImageDeleted` callback before calling it, since it may
be undefined like in the sibling `SponsorItemDialog`. Update the delete handler
to check that `onImageDeleted` exists alongside the `id` check, keeping the same
behavior when the callback is provided.
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js`:
- Around line 55-60: The dialog title in SponsorFormItemPopup includes leftover
debug text after the translated label, so remove the literal string from the
Typography block and keep only the T.translate result in the popup title. Use
the existing SponsorFormItemPopup component and its title rendering logic to
locate and clean up this stray text.
---
Outside diff comments:
In `@src/actions/sponsor-forms-actions.js`:
- Around line 1448-1465: The success branch in the sponsor form item save flow
is calling an undefined saveImages helper, which causes a ReferenceError after
the POST succeeds. Update the promise chain in the action that handles the POST
response to use saveItemImages instead, and pass the formId, response.id, and
the images array so the success snackbar and return flow complete normally.
🪄 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: 57ad779e-180d-41ad-9758-c88bd93dd6d4
📒 Files selected for processing (7)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/form-templates/sponsor-inventory-popup.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js (1)
74-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
removeItemFilemissing from propTypes.The prop is destructured and used at Line 29/46 but not declared in
propTypes, unlike the other injected action props.🐛 Proposed fix
SponsorFormItemPopup.propTypes = { open: PropTypes.bool.isRequired, onClose: PropTypes.func.isRequired, formId: PropTypes.string.isRequired, resetSponsorFormItem: PropTypes.func.isRequired, saveSponsorFormItem: PropTypes.func.isRequired, - updateSponsorFormItem: PropTypes.func.isRequired + updateSponsorFormItem: PropTypes.func.isRequired, + removeItemFile: PropTypes.func.isRequired };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js` around lines 74 - 81, The SponsorFormItemPopup component is using the removeItemFile prop but it is missing from its propTypes declaration. Update SponsorFormItemPopup.propTypes to include removeItemFile alongside the other injected action props so the component’s expected props match the destructuring and usage in SponsorFormItemPopup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js`:
- Around line 74-81: The SponsorFormItemPopup component is using the
removeItemFile prop but it is missing from its propTypes declaration. Update
SponsorFormItemPopup.propTypes to include removeItemFile alongside the other
injected action props so the component’s expected props match the destructuring
and usage in SponsorFormItemPopup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9041e12b-b634-4ccf-855c-df050b4cb506
📒 Files selected for processing (1)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js
d27136e to
564071b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actions/sponsor-forms-actions.js`:
- Around line 1277-1285: Update the image-delete flow using
SPONSOR_FORM_ITEM_FILE_DELETED so deleteRequest receives an action payload
containing fileId rather than an unbound action creator. Add handling for this
action in both relevant reducers, removing the deleted image by fileId from
currentItem.images while preserving the remaining images.
In `@src/pages/sponsors/sponsor-form-item-list-page/index.js`:
- Around line 134-136: Update handleRemoveItemImage to pass the current form
item’s ID, rather than the currentItem object, as removeItemFile’s second
argument so the generated item URL contains the actual identifier.
🪄 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: b9f71fad-e979-4706-be14-026d85894e15
📒 Files selected for processing (8)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/form-templates/sponsor-inventory-popup.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pages/sponsors-global/form-templates/form-template-item-list-page.js
- src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js
- src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js
- src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js
- src/pages/sponsors-global/inventory/inventory-list-page.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/reducers/sponsors/sponsor-form-items-list-reducer.js`:
- Around line 120-134: Update the SPONSOR_FORM_ITEM_FILE_DELETED flow and its
removeItemFile dispatch to include itemId: formItemId in the payload. In the
reducer, match state.items by itemId and only remove the image from currentItem
when currentItem.id matches itemId; otherwise preserve currentItem unchanged.
🪄 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: 361dafbf-da5f-4a22-b851-b8ca9427a8d1
📒 Files selected for processing (3)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/reducers/sponsors/sponsor-form-items-list-reducer.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/sponsors/sponsor-form-item-list-page/index.js
- src/actions/sponsor-forms-actions.js
| }; | ||
|
|
||
| const handleRemoveItemImage = (imageId) => | ||
| removeItemFile(formId, currentItem?.id, imageId); |
There was a problem hiding this comment.
@tomrndom This needs a guard for unsaved items. The new-item flows reset to a non-persisted entity but still pass onImageDeleted, so removing an uploaded image before saving calls the backend with an invalid item id.
Examples:
src/pages/sponsors/sponsor-form-item-list-page/index.js:134can callremoveItemFile(formId, undefined, imageId).src/pages/sponsors-global/inventory/inventory-list-page.js:152can calldeleteInventoryItemImage(0, imageId)from the default entity.
Repro: Add item -> upload image -> click the image trash button before saving. The request becomes /items/undefined/images/{id} or /inventory-items/0/images/{id}.
Please only call the backend delete for persisted item images. For unsaved uploads, the upload component/Formik state should remove the image locally without issuing an item-image DELETE.
| }) | ||
| ); | ||
| }); | ||
| dispatch( |
There was a problem hiding this comment.
@tomrndom This simplification now relies entirely on the update body to persist images, but RECEIVE_SPONSOR_FORM_ITEM (src/reducers/sponsors/sponsor-form-items-list-reducer.js) stores fetched images without mapping file_url to file_path — so saving any existing item wipes all of its images server-side.
ShowFormItemImageSerializer in purchases-api marks file_path write_only=True — a GET response only ever contains file_url. normalizeItem keeps only images with a truthy file_path before sending them here. Since currentItem.images never has file_path, editing any existing item (even just the name) sends images: [] in this PUT, and ShowFormItemService.update() treats 'images' in payload as True — deleting every attached image before repopulating from the (empty) list. No error is shown; the save reports success.
This is the same class of bug the sibling reducer already fixed — see sponsor-customized-form-items-list-reducer.js's RECEIVE_SPONSOR_CUSTOMIZED_FORM_ITEM case, which maps file_path: img.file_url on each image specifically for this reason (its own test is titled "the edit-form image fix"). This reducer never got the same fix.
Suggested fix — mirror it in sponsor-form-items-list-reducer.js's RECEIVE_SPONSOR_FORM_ITEM case:
case RECEIVE_SPONSOR_FORM_ITEM: {
const item = payload.response;
const currentItem = {
...item,
images: (item.images || []).map((img) => ({
...img,
file_path: img.file_url
})),
meta_fields: item.meta_fields.length > 0 ? item.meta_fields : []
};
return { ...state, currentItem };
}
| ); | ||
|
|
||
| const handleRemoveImage = (imageId) => { | ||
| deleteItemImage(formTemplateId, currentFormTemplateItem.id, imageId); |
There was a problem hiding this comment.
@tomrndom handleRemoveImage calls deleteItemImage(formTemplateId, currentFormTemplateItem.id, imageId) without checking that currentFormTemplateItem.id is set first.
This PR added the exact same guard — if (!current*Item?.id) return; — to the three sibling handlers in index.js, inventory-list-page.js, and sponsor-forms-manage-items.js, specifically to stop calling the delete endpoint with an invalid/undefined item id for an unsaved entity. This file is the one place that pattern was skipped, so it's inconsistent with the rest of the PR and would resurrect the same bug the moment this dialog gets a "new blank item" entry point (today it only opens via handleRowEdit on an already-persisted row, so it's not yet reachable, but nothing prevents that from changing).
Suggested fix:
const handleRemoveImage = (imageId) => {
if (!currentFormTemplateItem?.id) return;
deleteItemImage(formTemplateId, currentFormTemplateItem.id, imageId);
};
| ); | ||
| }; | ||
|
|
||
| const handleRemoveItemImage = (imageId) => { |
There was a problem hiding this comment.
@tomrndom This guard has no test covering it. It's the direct fix for the bug flagged in review on 2026-08-06: removing an uploaded image on a not-yet-saved item was calling the backend with an invalid item id. sponsor-form-item-list-page.test.js already exists for this file, so adding coverage here is a few lines in an established suite, the same ask already applied earlier in this PR to the reducer tests.
Suggested fix: add a test asserting removeItemFile is not called when currentItem.id is unset/0, and is called with (formId, currentItem.id, imageId) when it is.
| }); | ||
| settings.errorHandler ?? authErrorHandler | ||
| )(params)(dispatch) | ||
| .catch(() => {}) |
There was a problem hiding this comment.
@tomrndom The .catch(() => {}).finally(() => dispatch(stopLoading())) added here fixes a real bug — previously (.then(() => dispatch(stopLoading())) only) a failed delete request left stopLoading() never dispatched, so the loading spinner stayed on indefinitely on error. That fix has no test, and deleteFile has no test file at all in src/actions/__tests__/.
Suggested fix: add a test that mocks deleteRequest to reject and asserts stopLoading is still dispatched.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
… save Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…e_path on images Mirrors the customized-item reducer's existing file_url -> file_path mapping. Currently red: RECEIVE_SPONSOR_FORM_ITEM stores fetched images as-is, so normalizeItem strips them from the update body and ShowFormItemService.update() deletes all images on save.
… from item without id Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
… scope Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
b5d4031 to
c72289f
Compare
ref: https://app.clickup.com/t/9014802374/86bakf3mp
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit