fix: adjust popup pattern, add isSaving on popups, return promises#994
fix: adjust popup pattern, add isSaving on popups, return promises#994tomrndom wants to merge 3 commits into
Conversation
…emove open prop, adjust tests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAcross six popup/dialog components, ChangesDialog Save-State Internalization & Close-Responsibility Shift
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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/pages/sponsors-global/form-templates/sponsor-inventory-popup.js (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
useStateimport causes ReferenceError.Line 48 uses
useStatebut it is not imported from React on line 1. This will throw a runtime error when the component renders.🐛 Proposed fix to add the missing import
-import React from "react"; +import React, { useState } from "react";Also applies to: 48-49
🤖 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/form-templates/sponsor-inventory-popup.js` at line 1, The component uses the useState hook on lines 48-49 but it is not imported in the React import statement on line 1. Add useState to the destructured import from React by updating the import statement to include useState alongside React, so that the useState hook is available when the component renders.
🤖 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/pages/sponsors-global/inventory/inventory-list-page.js`:
- Around line 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.
In `@src/pages/sponsors-global/page-templates/page-template-list-page.js`:
- Around line 117-128: The handleSavePageTemplate function does not return a
promise, causing runtime failures when the popup contract attempts to chain
.then on the result. Add a return statement before the savePageTemplate promise
chain to ensure the entire promise returned from
savePageTemplate(entity).then(...) is returned from the handler, allowing
callers to properly chain .then(onClose) on the result.
- Around line 118-127: The savePageTemplate call in the
page-template-list-page.js file has a redundant .then() chain that invokes
getPageTemplates again, causing duplicate requests. Since savePageTemplate
already dispatches getPageTemplates internally, remove the entire .then() block
that calls getPageTemplates with the term, DEFAULT_CURRENT_PAGE, perPage, order,
orderDir, and showArchived parameters. Simply call savePageTemplate(entity)
without the chained getPageTemplates invocation to let the internal dispatch
handle the refresh with the appropriate state values.
In `@src/pages/sponsors/summit-sponsorship-list-page.js`:
- Around line 99-101: The saveSummitSponsorship thunk in
src/actions/sponsor-actions.js does not return the promise from the API request
(putRequest or postRequest), causing the .then() chain in
summit-sponsorship-list-page.js to execute prematurely. Modify the
saveSummitSponsorship function to return the actual promise from the
putRequest/postRequest call so that getSummitSponsorships is only invoked after
the API write completes, preventing the popup from closing before persistence
finishes.
---
Outside diff comments:
In `@src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js`:
- Line 1: The component uses the useState hook on lines 48-49 but it is not
imported in the React import statement on line 1. Add useState to the
destructured import from React by updating the import statement to include
useState alongside React, so that the useState hook is available when the
component renders.
🪄 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: 7a8515e5-6d59-4f49-a419-d75f0973d2c7
📒 Files selected for processing (21)
src/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/form-templates/form-template-list-page.jssrc/pages/sponsors-global/form-templates/form-template-popup.jssrc/pages/sponsors-global/form-templates/sponsor-inventory-popup.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/pages/sponsors-global/page-templates/page-template-list-page.jssrc/pages/sponsors-global/page-templates/page-template-popup/index.jssrc/pages/sponsors/popup/add-sponsor-popup.jssrc/pages/sponsors/popup/edit-tier-popup.jssrc/pages/sponsors/show-pages-list-page/__tests__/show-pages-list-page.test.jssrc/pages/sponsors/show-pages-list-page/index.jssrc/pages/sponsors/sponsor-list-page.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/__tests__/sponsor-pages-tab.test.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.jssrc/pages/sponsors/summit-sponsorship-list-page.jssrc/pages/sponsorship-types/__tests__/sponsorship-list-page.test.jssrc/pages/sponsorship-types/components/sponsorship-dialog.jssrc/pages/sponsorship-types/sponsorship-list-page.jssrc/pages/tags/tag-list-page.js
| const handleInventorySave = (item) => | ||
| saveInventoryItem(item).then(() => | ||
| getInventoryItems( | ||
| term, | ||
| currentPage, | ||
| perPage, | ||
| order, | ||
| orderDir, | ||
| showArchived | ||
| ) | ||
| .finally(() => setOpen(false)); | ||
| }; | ||
| ); |
There was a problem hiding this comment.
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
| saveSummitSponsorship(sponsorship).then(() => | ||
| getSummitSponsorships(DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | ||
| ); |
There was a problem hiding this comment.
Return the real save promise before chaining refresh.
Line 99 assumes saveSummitSponsorship resolves after the API write, but the current thunk (src/actions/sponsor-actions.js) does not return the inner putRequest/postRequest promise. This makes the refresh run early and can close the popup before persistence completes.
Suggested fix at the root cause (`src/actions/sponsor-actions.js`)
export const saveSummitSponsorship = (entity) => async (dispatch, getState) => {
// ...
dispatch(startLoading());
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}`,
normalizedEntity,
authErrorHandler,
entity
- )(params)(dispatch).then(() => {
- dispatch(
- showSuccessMessage(T.translate("edit_sponsorship.sponsorship_saved"))
- );
- });
+ )(params)(dispatch)
+ .then(() => {
+ dispatch(
+ showSuccessMessage(T.translate("edit_sponsorship.sponsorship_saved"))
+ );
+ })
+ .finally(() => {
+ dispatch(stopLoading());
+ });
} else {
const success_message = {
title: T.translate("general.done"),
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`,
normalizedEntity,
authErrorHandler,
entity
- )(params)(dispatch).then(() => {
- dispatch(showMessage(success_message));
- });
+ )(params)(dispatch)
+ .then(() => {
+ dispatch(showMessage(success_message));
+ })
+ .finally(() => {
+ dispatch(stopLoading());
+ });
}
};🤖 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/summit-sponsorship-list-page.js` around lines 99 - 101,
The saveSummitSponsorship thunk in src/actions/sponsor-actions.js does not
return the promise from the API request (putRequest or postRequest), causing the
.then() chain in summit-sponsorship-list-page.js to execute prematurely. Modify
the saveSummitSponsorship function to return the actual promise from the
putRequest/postRequest call so that getSummitSponsorships is only invoked after
the API write completes, preventing the popup from closing before persistence
finishes.
✅ Addressed in commit 19a9c72
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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/actions/inventory-item-actions.js`:
- Around line 236-250: The stopLoading() dispatch call inside the inner
.finally() block (after Promise.all) only executes when the putRequest succeeds.
If the putRequest fails, the outer catch block rethrows the error without ever
clearing the loading state. Move the stopLoading() call from the inner
.finally() block to an outer .finally() block at the end of the entire promise
chain, so it executes regardless of whether the putRequest or subsequent
Promise.all operations succeed or fail.
🪄 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: 506f0181-cdcb-423b-a046-3729b1f92efb
📒 Files selected for processing (4)
src/actions/inventory-item-actions.jssrc/actions/page-template-actions.jssrc/actions/sponsor-actions.jssrc/pages/sponsors-global/page-templates/page-template-list-page.js
💤 Files with no reviewable changes (1)
- src/actions/page-template-actions.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/sponsors-global/page-templates/page-template-list-page.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
|
||
| const handleSaveSummitSponsorship = (sponsorship) => { | ||
| saveSummitSponsorship(sponsorship).then(() => setShowAddTierModal(false)); | ||
| saveSummitSponsorship(sponsorship).then(() => |
There was a problem hiding this comment.
@tomrndom handleSaveSummitSponsorship doesn't return the promise chain (no return on the arrow function body), but EditTierDialog's new onSubmit (edit-tier-popup.js) does onSubmit(values).then(() => onClose()).finally(...). Since onSubmit(values) here evaluates to undefined, that .then throws a TypeError - the save itself still fires, but the dialog never closes and isSaving never resets (Save/Close/Escape stay disabled until the page is reloaded). Every other handler converted in this PR uses the implicit-return arrow form; this one was left as a block body - compare to handleNewSponsor in sponsor-list-page.js for the correct shape. Per the popup pattern doc (https://github.com/fntechgit/ftn-docsnsklz/blob/main/patterns/show-admin/summit-admin-popup-dialog-pattern.md), the parent's save handler must return the save promise.
| }) | ||
| .catch((err) => { | ||
| console.error(err); | ||
| throw err; |
There was a problem hiding this comment.
@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.
| orderDir, | ||
| showArchived | ||
| ) | ||
| const handleInventorySave = (item) => |
There was a problem hiding this comment.
@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.
| onSubmit: (values) => { | ||
| if (isSaving) return; | ||
| setIsSaving(true); | ||
| onSave(values) |
There was a problem hiding this comment.
@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.
|
|
||
| setIsSaving(true); | ||
| Promise.resolve(onSave(finalValues)) | ||
| onSave(finalValues) |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Pull request overview
This PR standardizes “popup/dialog save” behavior across the admin UI by having save handlers return promises, moving “isSaving” state into the popup components to prevent duplicate submissions/closure during saves, and refreshing lists after successful saves.
Changes:
- Refactored multiple page-level
handleSave*functions to return promise chains and refresh list data after saves. - Updated several dialogs/popups to manage
isSavinglocally and disable close/escape/save while saving. - Updated tests and selected Redux actions to better support async save flows (including improved error propagation for inventory saves).
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/tags/tag-list-page.js | Returns the refresh promise from tag save flow. |
| src/pages/sponsorship-types/sponsorship-list-page.js | Removes page-level isSaving; save handler now returns a promise for dialog-managed closing. |
| src/pages/sponsorship-types/components/sponsorship-dialog.js | Adds local isSaving gating and closes only after save promise resolves. |
| src/pages/sponsorship-types/tests/sponsorship-list-page.test.js | Updates mocked save to match new promise-based close flow. |
| src/pages/sponsors/summit-sponsorship-list-page.js | Refreshes list after tier save and renders popup conditionally (no open prop). |
| src/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.js | Refactors multiple save handlers to return promises and refresh lists. |
| src/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js | Adds isSaving and disables close interactions while saving; closes on fulfilled save. |
| src/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/tests/sponsor-pages-tab.test.js | Updates mocked save to match promise-based close flow. |
| src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js | Returns promise from item save so the inventory dialog can close itself on success. |
| src/pages/sponsors/sponsor-list-page.js | Removes open prop usage and returns promise from add-sponsor save flow. |
| src/pages/sponsors/show-pages-list-page/index.js | Resets pagination on per-page/archive changes; returns save promise and refreshes list with current filters. |
| src/pages/sponsors/show-pages-list-page/tests/show-pages-list-page.test.js | Updates mocked save to match promise-based close flow. |
| src/pages/sponsors/popup/edit-tier-popup.js | Moves isSaving into dialog; disables close interactions and closes on fulfilled save. |
| src/pages/sponsors/popup/add-sponsor-popup.js | Moves isSaving into dialog; disables close interactions and closes on fulfilled save. |
| src/pages/sponsors-global/page-templates/page-template-popup/index.js | Adds isSaving UX and closes on fulfilled save. |
| src/pages/sponsors-global/page-templates/page-template-list-page.js | Refreshes page templates list after save instead of closing immediately. |
| src/pages/sponsors-global/inventory/inventory-list-page.js | Returns promise chain from inventory save and refreshes list after save. |
| src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js | Adds isSaving UX and closes on fulfilled save. |
| src/pages/sponsors-global/form-templates/form-template-popup.js | Adjusts save flow to rely on promise-returning onSave. |
| src/pages/sponsors-global/form-templates/form-template-list-page.js | Refactors save handler to return promise and refresh list. |
| src/pages/sponsors-global/form-templates/form-template-item-list-page.js | Refactors item save handler to return promise and refresh list. |
| src/actions/sponsor-actions.js | Ensures saveSummitSponsorship returns request promises. |
| src/actions/page-template-actions.js | Removes auto-refresh side effect from action (callers now refresh). |
| src/actions/inventory-item-actions.js | Improves error propagation by rethrowing after logging and returning the inner promise chain. |
Comments suppressed due to low confidence (2)
src/actions/page-template-actions.js:180
- This
catchlogs the error but resolves the returned promise. Since popups now close only on fulfilled saves (via.then(() => onClose())), swallowing the rejection will cause the UI to close even when the API call fails. Re-throw after logging so callers can keep the dialog open on failure.
);
})
.catch((err) => {
console.error(err);
})
src/actions/page-template-actions.js:204
- Same as above: this
catchswallows errors, which will make callers treat a failed create as success and close the popup. Re-throw so the promise rejects on failure.
);
})
.catch((err) => {
console.error(err);
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onMetaFieldTypeValueDeleted, | ||
| entity: initialEntity | ||
| }) => { | ||
| const [isSaving, setIsSaving] = useState(false); |
| }), | ||
| enableReinitialize: true, | ||
| onSubmit: (values) => { | ||
| if (isSaving) return; |
| } | ||
| const success_message = { | ||
| title: T.translate("general.done"), |
ref: https://app.clickup.com/t/9014802374/86bag2zk7
…emove open prop, adjust tests
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests