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 1 commit
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
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
11 changes: 10 additions & 1 deletion 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 @@ -115,7 +115,16 @@ const PageTemplateListPage = ({
};

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

const handleArchive = (item) =>
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
34 changes: 27 additions & 7 deletions src/pages/sponsors/popup/add-sponsor-popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useEffect } from "react";
import React, { useEffect, useState } from "react";
import T from "i18n-react/dist/i18n-react";
import { FormikProvider, useFormik } from "formik";
import * as yup from "yup";
Expand All @@ -23,7 +23,9 @@ import useScrollToError from "../../../hooks/useScrollToError";
import CompanyInputMUI from "../../../components/mui/formik-inputs/company-input-mui";
import SponsorshipsBySummitSelectMUI from "../../../components/mui/formik-inputs/sponsorship-summit-select-mui";

const AddSponsorDialog = ({ open, onClose, onSubmit, summitId }) => {
const AddSponsorDialog = ({ onClose, onSubmit, summitId }) => {
const [isSaving, setIsSaving] = useState(false);

const formik = useFormik({
initialValues: {
company: null,
Expand Down Expand Up @@ -51,14 +53,21 @@ const AddSponsorDialog = ({ open, onClose, onSubmit, summitId }) => {
)
.min(1, "At least one sponsorship is required")
}),
onSubmit,
onSubmit: (values) => {
if (isSaving) return;
setIsSaving(true);
onSubmit(values)
.then(() => onClose())
.finally(() => setIsSaving(false));
},
enableReinitialize: true
});

// SCROLL TO ERROR
useScrollToError(formik);

const handleClose = () => {
if (isSaving) return;
formik.resetForm();
onClose();
};
Expand All @@ -70,12 +79,23 @@ const AddSponsorDialog = ({ open, onClose, onSubmit, summitId }) => {
}, [formik.errors]);

return (
<Dialog open={open} onClose={handleClose} maxWidth="xs" fullWidth>
<Dialog
open
onClose={handleClose}
maxWidth="xs"
fullWidth
disableEscapeKeyDown={isSaving}
>
<DialogTitle sx={{ display: "flex", justifyContent: "space-between" }}>
<Typography fontSize="1.5rem">
{T.translate("sponsor_list.add_sponsor")}
</Typography>
<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 @@ -135,7 +155,8 @@ const AddSponsorDialog = ({ open, onClose, onSubmit, summitId }) => {
variant="contained"
disabled={
!formik.values.company ||
formik.values.sponsorships.length === 0
formik.values.sponsorships.length === 0 ||
isSaving
}
>
{T.translate("sponsor_list.add_sponsor")}
Expand All @@ -148,7 +169,6 @@ const AddSponsorDialog = ({ open, onClose, onSubmit, summitId }) => {
};

AddSponsorDialog.propTypes = {
open: PropTypes.bool.isRequired,
onClose: PropTypes.func.isRequired,
onSubmit: PropTypes.func.isRequired,
summitId: PropTypes.number.isRequired
Expand Down
Loading
Loading