Skip to content

fix: adjust popup pattern, add isSaving on popups, return promises #994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomrndom wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: adjust popup pattern, add isSaving on popups, return promises #994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9de307d
fix: adjust popup pattern, add isSaving on popups, return promises, r…
tomrndom Jun 19, 2026
19a9c72
fix: add return for actions
tomrndom Jun 19, 2026
1344f0c
fix: move stopLoading outisde Promise array
tomrndom Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions src/actions/inventory-item-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,23 +233,20 @@ export const saveInventoryItem = (entity) => async (dispatch) => {
promises.push(saveItemMetaFieldTypes(normalizedEntity)(dispatch));
}

Promise.all(promises)
.then(() => {
dispatch(
showSuccessMessage(
T.translate("edit_inventory_item.inventory_item_saved")
)
);
})
.catch((err) => {
console.error(err);
})
.finally(() => {
dispatch(stopLoading());
});
return Promise.all(promises).then(() => {
dispatch(
showSuccessMessage(
T.translate("edit_inventory_item.inventory_item_saved")
)
);
});
})
.catch((err) => {
console.error(err);
throw err;

@smarcet smarcet Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom This PR fixes the update branch here to return/rethrow on failure, but the sibling create branch below (postRequest(...), ~line 259) still has the same bug this fix addresses: the inner Promise.all(promises)...catch(err => console.error(err)) is never returned, and the outer .catch((err) => { console.error(err); }) doesn't rethrow - so saveInventoryItem for a new item always resolves, even when the POST fails. Combined with the new self-close contract in sponsor-inventory-popup.js (onSave(values).then(() => onClose())), a failed "Add Item" now silently closes the dialog as if it succeeded. Worth applying the same fix to the create branch.

})
.finally(() => {
dispatch(stopLoading());
});
}

Expand Down
2 changes: 0 additions & 2 deletions src/actions/page-template-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,6 @@ export const savePageTemplate = (entity) => async (dispatch) => {
html: T.translate("page_template_list.page_crud.page_saved")
})
);
getPageTemplates()(dispatch);
})
.catch((err) => {
console.error(err);
Expand All @@ -199,7 +198,6 @@ export const savePageTemplate = (entity) => async (dispatch) => {
html: T.translate("page_template_list.page_crud.page_created")
})
);
getPageTemplates()(dispatch);
})
.catch((err) => {
console.error(err);
Expand Down
8 changes: 4 additions & 4 deletions src/actions/sponsor-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,7 +1126,7 @@ export const saveSummitSponsorship = (entity) => async (dispatch, getState) => {
const normalizedEntity = normalizeSponsorship(entity);

if (entity.id) {
putRequest(
return putRequest(
createAction(UPDATE_SUMMIT_SPONSORSHIP),
createAction(SUMMIT_SPONSORSHIP_UPDATED),
`${window.API_BASE_URL}/api/v1/summits/${currentSummit.id}/sponsorships-types/${entity.id}`,
Expand All @@ -1138,14 +1138,14 @@ export const saveSummitSponsorship = (entity) => async (dispatch, getState) => {
showSuccessMessage(T.translate("edit_sponsorship.sponsorship_saved"))
);
});
} else {
}
const success_message = {
title: T.translate("general.done"),
Comment on lines +1141 to 1143
html: T.translate("edit_sponsorship.sponsorship_created"),
type: "success"
};

postRequest(
return postRequest(
createAction(UPDATE_SUMMIT_SPONSORSHIP),
createAction(SUMMIT_SPONSORSHIP_ADDED),
`${window.API_BASE_URL}/api/v1/summits/${currentSummit.id}/sponsorships-types`,
Expand All @@ -1155,7 +1155,7 @@ export const saveSummitSponsorship = (entity) => async (dispatch, getState) => {
)(params)(dispatch).then(() => {
dispatch(showMessage(success_message));
});
}

};

export const uploadSponsorshipBadgeImage =
Expand Down
4 changes: 1 addition & 3 deletions src/pages/sponsors-global/form-templates/form-template-item-list-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ const FormTemplateItemListPage = ({
);
};

const handleFormTemplateSave = (item) => {
const handleFormTemplateSave = (item) =>
saveFormTemplateItem(formTemplateId, item).then(() =>
getFormTemplateItems(
formTemplateId,
Expand All @@ -189,8 +189,6 @@ const FormTemplateItemListPage = ({
showArchived
)
);
setShowInventoryItemModal(false);
};

const columns = [
{
Expand Down
20 changes: 10 additions & 10 deletions src/pages/sponsors-global/form-templates/form-template-list-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,17 +212,17 @@ const FormTemplateListPage = ({
sortDir: orderDir
};

const handleOnSave = async (values) => {
await saveFormTemplate(values);
getFormTemplates(
"",
DEFAULT_CURRENT_PAGE,
perPage,
order,
orderDir,
showArchived
const handleOnSave = (values) =>
saveFormTemplate(values).then(() =>
getFormTemplates(
"",
DEFAULT_CURRENT_PAGE,
perPage,
order,
orderDir,
showArchived
)
);
};

return (
<div className="container">
Expand Down
4 changes: 2 additions & 2 deletions src/pages/sponsors-global/form-templates/form-template-popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ const FormTemplateDialog = ({
if (isSaving) return;

setIsSaving(true);
Promise.resolve(onSave(finalValues))
onSave(finalValues)

@smarcet smarcet Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom This dialog still takes an open prop (line 30 / <Dialog open={open}>) and its parent (form-template-list-page.js) still renders it unconditionally with open={formTemplatePopupOpen}, instead of the conditional-mount pattern ({formTemplatePopupOpen && <FormTemplateDialog .../>}, no open prop) that every other dialog in this PR was migrated to. Per the doc (https://github.com/fntechgit/ftn-docsnsklz/blob/main/patterns/show-admin/summit-admin-popup-dialog-pattern.md), always-render is explicitly called out as the anti-pattern to avoid, since it requires manually resetting internal state instead of getting it for free from unmount/remount. Not an active bug today (the catch/finally here is complete and enableReinitialize covers reopening for a different entity), but it's now the one outlier relative to the rest of the PR's own convention.

.then(() => {
closePopup();
onClose();
})
.catch(() => {
// keep dialog open on save error to preserve user input
Expand Down
26 changes: 23 additions & 3 deletions src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ const SponsorItemDialog = ({
onMetaFieldTypeValueDeleted,
entity: initialEntity
}) => {
const [isSaving, setIsSaving] = useState(false);

const formik = useFormik({
initialValues: {
...initialEntity,
Expand All @@ -65,7 +67,13 @@ const SponsorItemDialog = ({
quantity_limit_per_show: positiveNumberValidation(),
meta_fields: formMetafieldsValidation()
}),
onSubmit: (values) => onSave(values)
onSubmit: (values) => {
if (isSaving) return;
setIsSaving(true);
onSave(values)

@smarcet smarcet Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom This dialog (and page-template-popup/index.js, add-sponsor-popup.js, edit-tier-popup.js, add-sponsor-page-template-popup/index.js, sponsorship-dialog.js) is missing the .catch(() => {}) between .then(() => onClose()) and .finally(...). form-template-popup.js has it and is the only one of the seven dialogs that matches the doc's Save Guard Pattern checklist item "Dialog stays open on save error (.catch)" (https://github.com/fntechgit/ftn-docsnsklz/blob/main/patterns/show-admin/summit-admin-popup-dialog-pattern.md). Functionally the dialog still stays open on a rejected save (since .then just doesn't fire), but the rejection goes unhandled - console noise and potential false positives in error monitoring on every failed save across 6 of 7 dialogs.

.then(() => onClose())
.finally(() => setIsSaving(false));
}
});

const mediaType = {
Expand All @@ -79,6 +87,7 @@ const SponsorItemDialog = ({
useScrollToError(formik);

const handleClose = () => {
if (isSaving) return;
formik.resetForm();
onClose();
};
Expand All @@ -92,12 +101,18 @@ const SponsorItemDialog = ({
disableEnforceFocus
disableAutoFocus
disableRestoreFocus
disableEscapeKeyDown={isSaving}
>
<DialogTitle sx={{ display: "flex", justifyContent: "space-between" }}>
{initialEntity.id
? T.translate("edit_inventory_item.edit_item")
: T.translate("edit_inventory_item.new_item")}
<IconButton size="small" onClick={handleClose} sx={{ mr: 1 }}>
<IconButton
size="small"
onClick={handleClose}
sx={{ mr: 1 }}
disabled={isSaving}
>
<CloseIcon fontSize="small" />
</IconButton>
</DialogTitle>
Expand Down Expand Up @@ -227,7 +242,12 @@ const SponsorItemDialog = ({
</DialogContent>
<Divider />
<DialogActions>
<Button type="submit" fullWidth variant="contained">
<Button
type="submit"
fullWidth
variant="contained"
disabled={isSaving}
>
{T.translate("edit_inventory_item.save_changes")}
</Button>
</DialogActions>
Expand Down
23 changes: 10 additions & 13 deletions src/pages/sponsors-global/inventory/inventory-list-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,20 +246,17 @@ const InventoryListPage = ({
setOpen(true);
};

const handleInventorySave = (item) => {
saveInventoryItem(item)
.then(() =>
getInventoryItems(
term,
currentPage,
perPage,
order,
orderDir,
showArchived
)
const handleInventorySave = (item) =>

@smarcet smarcet Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom Per the popup pattern doc's Responsibility Split section (https://github.com/fntechgit/ftn-docsnsklz/blob/main/patterns/show-admin/summit-admin-popup-dialog-pattern.md), the list refresh inside the parent's save handler should be fire-and-forget (getMyEntities(...).catch(() => {})) so a refresh failure can't reject the promise the popup is waiting on to close. Here - and in every other handler converted by this PR (handleFormTemplateSave, handleOnSave in form-template-list-page.js, handleSavePageTemplate, handleSaveShowPage, handleNewSponsor, handleItemSave, the three handlers in sponsor-pages-tab/index.js, handleSave in sponsorship-list-page.js, handleSaveTag) - the refresh call isn't wrapped in its own .catch(). None of the underlying list actions (getInventoryItems, getFormTemplates, getSummitSponsorships, etc.) swallow their own errors either, so a transient refresh failure right after a successful save currently rejects the whole chain and leaves the dialog open - exactly the scenario the doc calls out.

saveInventoryItem(item).then(() =>
getInventoryItems(
term,
currentPage,
perPage,
order,
orderDir,
showArchived
)
.finally(() => setOpen(false));
};
);
Comment on lines +249 to +259

@coderabbitai coderabbitai Bot Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Ensure the returned save promise reflects real completion/failure.

handleInventorySave now drives post-save flow from the returned promise, but saveInventoryItem (in src/actions/inventory-item-actions.js:205-300) currently swallows errors and resolves before nested image/meta saves finish. This can refresh stale data and let dialog-close logic treat failed saves as success.

Suggested fix in src/actions/inventory-item-actions.js 
-      .then(() => {
+      .then(() => {
         const promises = [];
@@
-        Promise.all(promises)
+        return Promise.all(promises)
           .then(() => {
             dispatch(
               showSuccessMessage(
                 T.translate("edit_inventory_item.inventory_item_saved")
               )
             );
           })
-          .catch((err) => {
-            console.error(err);
-          })
           .finally(() => {
             dispatch(stopLoading());
           });
       })
       .catch((err) => {
         console.error(err);
+        throw err;
       });
@@
-    .then(({ response }) => {
+    .then(({ response }) => {
@@
-      Promise.all(promises)
+      return Promise.all(promises)
         .then(() => {
           dispatch(
             showMessage(success_message, () => {
               history.push("/app/inventory");
             })
           );
         })
-        .catch((err) => {
-          console.error(err);
-        })
         .finally(() => {
           dispatch(stopLoading());
         });
     })
     .catch((err) => {
       console.error(err);
+      throw err;
     });
🤖 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-global/inventory/inventory-list-page.js` around lines 249
- 259, The `handleInventorySave` function relies on the promise returned by
`saveInventoryItem` to determine when the save operation is truly complete and
whether it succeeded or failed, but the `saveInventoryItem` function in
src/actions/inventory-item-actions.js (around lines 205-300) is currently
swallowing errors and resolving prematurely before nested image and metadata
saves finish. Fix this by ensuring the `saveInventoryItem` function waits for
all nested save operations (image saves and metadata saves) to complete before
resolving, and properly rejects the returned promise if any part of the save
process fails, so that the promise chain accurately reflects the true completion
status and allows error handling in downstream code like dialog-close logic to
respond correctly.

✅ Addressed in commit 19a9c72


const handleArchiveItem = (item) =>
item.is_archived
Expand Down
14 changes: 11 additions & 3 deletions src/pages/sponsors-global/page-templates/page-template-list-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,17 @@ const PageTemplateListPage = ({
setOpenCloneDialog(true);
};

const handleSavePageTemplate = (entity) => {
savePageTemplate(entity).then(() => setOpenPageDialog(false));
};
const handleSavePageTemplate = (entity) =>
savePageTemplate(entity).then(() =>
getPageTemplates(
term,
DEFAULT_CURRENT_PAGE,
perPage,
order,
orderDir,
showArchived
)
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const handleArchive = (item) =>
item.is_archived
Expand Down
37 changes: 31 additions & 6 deletions src/pages/sponsors-global/page-templates/page-template-popup/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from "react";
import React, { useState } from "react";
import T from "i18n-react/dist/i18n-react";
import PropTypes from "prop-types";
import {
Expand Down Expand Up @@ -43,6 +43,7 @@ const PageTemplatePopup = ({
sponsorshipIds,
title
}) => {
const [isSaving, setIsSaving] = useState(false);
const popupTitle =
title ??
(pageTemplate?.id
Expand Down Expand Up @@ -129,14 +130,17 @@ const PageTemplatePopup = ({
}),
modules: yup.array().of(moduleSchema)
}),
enableReinitialize: true,
onSubmit: (values) => {
if (isSaving) return;
Comment on lines 132 to +134
setIsSaving(true);
const modulesWithOrder = values.modules.map((m, idx) => ({
...m,
custom_order: idx
}));

onSave({ ...values, modules: modulesWithOrder });
onSave({ ...values, modules: modulesWithOrder })
.then(() => onClose())
.finally(() => setIsSaving(false));
}
});

Expand Down Expand Up @@ -179,11 +183,27 @@ const PageTemplatePopup = ({
});
};

const handleClose = () => {
if (isSaving) return;
onClose();
};

return (
<Dialog open onClose={onClose} maxWidth="md" fullWidth>
<Dialog
open
onClose={handleClose}
maxWidth="md"
fullWidth
disableEscapeKeyDown={isSaving}
>
<DialogTitle sx={{ display: "flex", justifyContent: "space-between" }}>
<Typography fontSize="1.5rem">{popupTitle}</Typography>
<IconButton size="small" onClick={onClose} sx={{ mr: 1 }}>
<IconButton
size="small"
onClick={handleClose}
sx={{ mr: 1 }}
disabled={isSaving}
>
<CloseIcon fontSize="small" />
</IconButton>
</DialogTitle>
Expand Down Expand Up @@ -306,7 +326,12 @@ const PageTemplatePopup = ({
</DialogContent>
<Divider />
<DialogActions>
<Button type="submit" fullWidth variant="contained">
<Button
type="submit"
fullWidth
variant="contained"
disabled={isSaving}
>
{T.translate("page_template_list.page_crud.save")}
</Button>
</DialogActions>
Expand Down
Loading
Loading