fix: adding forms does not close modal#824
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:
📝 WalkthroughWalkthroughIntroduces local isSaving state across sponsor form popups to prevent re-entrant submits, disables dialog dismissal during saves, keeps dialogs open on save failures, and removes no-op .catch handlers in sponsor-forms actions so stopLoading() runs in finally. Adds tests covering rejection, in-flight deduplication, and success flows. Changes
Sequence DiagramsequenceDiagram
participant User
participant Popup as "Popup Component"
participant Form as "Form Component"
participant Action as "Redux Action"
participant Async as "Async Operation"
User->>Popup: Click Submit
Popup->>Popup: if isSaving? (guard)
alt not saving
Popup->>Popup: set isSaving = true
Popup->>Form: pass isSaving (disables submit)
Popup->>Action: call save/clone action
Action->>Async: start async request
Note over Popup,Form: Dialog close/escape/icon disabled
Async-->>Action: resolve / reject
alt success
Action-->>Popup: resolve
Popup->>Popup: reset state, close dialog, call onClose/onSaved
else failure
Action-->>Popup: reject
Popup->>Popup: keep dialog open, preserve form state
end
Popup->>Popup: set isSaving = false
Popup->>Form: pass isSaving (re-enable submit)
else saving
Popup-->>User: ignore duplicate submit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🧹 Nitpick comments (3)
src/pages/sponsors/sponsor-forms-tab/index.js (1)
392-400: Minor redundancy in conditional rendering.The component is already conditionally rendered with
{openPopup === "template" && ...}, so theopen={openPopup === "template"}prop will always betruewhen the component mounts. This is functionally correct but slightly redundant.Consider either:
- Removing the conditional render and relying solely on the
openprop (preferred for consistent Dialog behavior)- Or using
open={true}since the condition is already checked♻️ Option 1: Remove conditional render
- {openPopup === "template" && ( - <AddSponsorFormTemplatePopup - open={openPopup === "template"} - onClose={() => setOpenPopup(null)} - onSubmit={handleSaveFormFromTemplate} - sponsor={sponsor} - summitId={summitId} - /> - )} + <AddSponsorFormTemplatePopup + open={openPopup === "template"} + onClose={() => setOpenPopup(null)} + onSubmit={handleSaveFormFromTemplate} + sponsor={sponsor} + summitId={summitId} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-tab/index.js` around lines 392 - 400, Remove the redundant conditional wrapper and render AddSponsorFormTemplatePopup unconditionally, relying on its open prop to control visibility; specifically, replace the `{openPopup === "template" && ( <AddSponsorFormTemplatePopup ... /> )}` pattern with a direct <AddSponsorFormTemplatePopup open={openPopup === "template"} onClose={() => setOpenPopup(null)} onSubmit={handleSaveFormFromTemplate} sponsor={sponsor} summitId={summitId} /> so the dialog component itself controls mount/unmount behavior via its open prop.src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js (2)
280-287: MissingisSavingguard for duplicate submission prevention.This popup lacks the
isSavingpattern that other popups in this PR implement (e.g.,CustomizedFormPopup). The submit button is only disabled when no forms are selected, but not during the save operation. This could allow duplicate submissions via double-click.Consider adding
isSavingstate similar to other popups:♻️ Suggested implementation
const AddSponsorFormTemplatePopup = ({ open, onClose, onSubmit, ... }) => { const [searchTerm, setSearchTerm] = useState(""); const [selectedForms, setSelectedForms] = useState([]); + const [isSaving, setIsSaving] = useState(false); // ... existing code ... const formik = useFormik({ // ... onSubmit: (values) => { + if (isSaving) return; const { add_ons } = values; const entity = { forms: selectedForms, add_ons }; - onSubmit(entity); + setIsSaving(true); + onSubmit(entity) + .finally(() => setIsSaving(false)); }, // ... });And update the button:
<Button type="submit" - disabled={selectedForms.length === 0} + disabled={selectedForms.length === 0 || isSaving} fullWidth variant="contained" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js` around lines 280 - 287, The submit button in AddSponsorFormTemplatePopup lacks an isSaving guard and can be double-submitted; add a boolean isSaving state in the component, set isSaving = true at the start of the submit handler (e.g., handleSubmit or onSubmit) and set it back to false on both success and error, and update the Button disabled prop to disabled={selectedForms.length === 0 || isSaving} (and optionally show a spinner when isSaving is true) to prevent duplicate submissions.
78-90: Consider resetting local state when popup opens.The
useEffectcorrectly fetches forms when the popup opens, but the local state (selectedForms,searchTerm) persists between open/close cycles. If a user closes and reopens the popup, their previous selections will still be checked.♻️ Suggested fix
useEffect(() => { if (open) { + setSelectedForms([]); + setSearchTerm(""); + formik.resetForm(); getSponsorForms( term, currentPage, FIVE_PER_PAGE, order, orderDir, false, sponsorshipTypeIds ); } }, [open]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js` around lines 78 - 90, The popup's local state (selectedForms and searchTerm) persists across open/close cycles causing previous selections to reappear; update the useEffect watching open (the block that calls getSponsorForms) to also reset selectedForms to an empty array and searchTerm to an empty string when open becomes true; ensure you update the same effect that calls getSponsorForms (or add a small helper) so selectedForms and searchTerm are cleared on open, leaving other params (term, currentPage, order, orderDir, sponsorshipTypeIds) unchanged.
🤖 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/sponsor-forms-list-page/components/form-template/form-template-popup.js`:
- Around line 34-35: The modal allows closing via backdrop/Escape and the close
icon even while isSaving is true, which resets the form before the in-flight
save settles; update the close logic so handleClose (and any close icon click
handler and the modal's onClose/backdrop/escape handlers) early-return when
isSaving is true, and disable the close UI while saving. Specifically, add a
guard like "if (isSaving) return" at the top of handleClose, ensure the modal's
onClose callback checks isSaving before calling handleClose, and disable/hide
the close icon (and prevent backdrop/Escape dismissal) while isSaving is true so
the form is only reset after the save promise resolves or rejects.
---
Nitpick comments:
In
`@src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js`:
- Around line 280-287: The submit button in AddSponsorFormTemplatePopup lacks an
isSaving guard and can be double-submitted; add a boolean isSaving state in the
component, set isSaving = true at the start of the submit handler (e.g.,
handleSubmit or onSubmit) and set it back to false on both success and error,
and update the Button disabled prop to disabled={selectedForms.length === 0 ||
isSaving} (and optionally show a spinner when isSaving is true) to prevent
duplicate submissions.
- Around line 78-90: The popup's local state (selectedForms and searchTerm)
persists across open/close cycles causing previous selections to reappear;
update the useEffect watching open (the block that calls getSponsorForms) to
also reset selectedForms to an empty array and searchTerm to an empty string
when open becomes true; ensure you update the same effect that calls
getSponsorForms (or add a small helper) so selectedForms and searchTerm are
cleared on open, leaving other params (term, currentPage, order, orderDir,
sponsorshipTypeIds) unchanged.
In `@src/pages/sponsors/sponsor-forms-tab/index.js`:
- Around line 392-400: Remove the redundant conditional wrapper and render
AddSponsorFormTemplatePopup unconditionally, relying on its open prop to control
visibility; specifically, replace the `{openPopup === "template" && (
<AddSponsorFormTemplatePopup ... /> )}` pattern with a direct
<AddSponsorFormTemplatePopup open={openPopup === "template"} onClose={() =>
setOpenPopup(null)} onSubmit={handleSaveFormFromTemplate} sponsor={sponsor}
summitId={summitId} /> so the dialog component itself controls mount/unmount
behavior via its open prop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abdffebb-1df3-4b81-963f-aae9f6fbf2be
📒 Files selected for processing (12)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/__tests__/form-template-popup.test.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/__tests__/global-template-popup.test.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/global-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/select-sponsorships-dialog.jssrc/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/__tests__/customized-form-popup.test.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form-popup.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.jssrc/pages/sponsors/sponsor-forms-tab/index.js
priscila-moneo
force-pushed
the
fix/adding-forms-does-not-close-modal
branch
from
fba522b to
798a7ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.js (1)
49-49: Good addition ofisSavingguard to prevent duplicate submissions.Disabling the button while saving is a solid UX improvement for preventing double-clicks.
One edge case to consider: pressing Enter in a text field can still trigger
formik.handleSubmiteven when the button is disabled. If the parent'sonSubmitcallback doesn't already guard against this, you could add form-level protection:💡 Optional enhancement
<Box component="form" - onSubmit={formik.handleSubmit} + onSubmit={(e) => { + if (isSaving) { + e.preventDefault(); + return; + } + formik.handleSubmit(e); + }} noValidate autoComplete="off" >Also applies to: 156-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.js` at line 49, Add a form-level guard so Enter-key submits can't run while isSaving is true: wrap or replace formik.handleSubmit with a small handler that returns immediately when isSaving is true before calling formik.handleSubmit or the parent onSubmit; update the form element's onSubmit to use this guardedSubmit and ensure any local submit handler (e.g., the component-level submit function or the prop onSubmit passed to the parent) also checks isSaving to avoid duplicate processing when Enter is pressed.
🤖 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/sponsor-forms-list-page/components/global-template/global-template-popup.js`:
- Around line 44-55: The dialog currently calls handleClose for all onClose
paths (Dialog and child components), which unconditionally resets
selectedTemplates and stage while an async clone/save (isSaving) may still be
pending, causing loss of state on failure; create a new handleDismiss that
checks isSaving and the close reason and only calls the reset when not saving,
pass handleDismiss to Dialog's onClose and to
SelectTemplatesDialog/SelectSponsorshipsDialog onClose props, add
disableEscapeKeyDown={isSaving} to Dialog, and move the unconditional state
reset into the success branch of the save handler (keep state intact on
failure).
In
`@src/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form-popup.js`:
- Around line 35-61: The current handleOnSave starts an async save but does not
guard against stale completions closing a newer dialog session; modify
handleOnSave/handleClose to track a session token (use a ref like sessionRef
incremented when the dialog opens or on handleClose) and capture the token
before calling save (saveSponsorCustomizedForm/updateSponsorCustomizedForm),
then in the .then()/.catch()/.finally() only call onSaved() and handleClose()
(and clear isSaving) if the captured token still matches sessionRef.current;
this prevents an earlier promise from resetting/closing a later session while
keeping existing logic and names (handleOnSave, handleClose, isSaving,
saveSponsorCustomizedForm, updateSponsorCustomizedForm).
In `@src/pages/sponsors/sponsor-forms-tab/index.js`:
- Around line 175-189: handleCustomizedFormSaved currently forces the list to
page 1 by passing DEFAULT_CURRENT_PAGE to getSponsorCustomizedForms; instead
destructure the current page from customizedForms (e.g. const { currentPage:
customizedCurrentPage, perPage: customizedPerPage, order: customizedOrder,
orderDir: customizedOrderDir } = customizedForms) and pass that
customizedCurrentPage to getSponsorCustomizedForms so the table stays on the
same page after create/edit; update handleCustomizedFormSaved to use that
variable in place of DEFAULT_CURRENT_PAGE.
---
Nitpick comments:
In
`@src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.js`:
- Line 49: Add a form-level guard so Enter-key submits can't run while isSaving
is true: wrap or replace formik.handleSubmit with a small handler that returns
immediately when isSaving is true before calling formik.handleSubmit or the
parent onSubmit; update the form element's onSubmit to use this guardedSubmit
and ensure any local submit handler (e.g., the component-level submit function
or the prop onSubmit passed to the parent) also checks isSaving to avoid
duplicate processing when Enter is pressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fb1011b-5c89-481d-ac2c-f2407a8436d7
📒 Files selected for processing (12)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/__tests__/form-template-popup.test.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/__tests__/global-template-popup.test.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/global-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/select-sponsorships-dialog.jssrc/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/__tests__/customized-form-popup.test.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form-popup.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.jssrc/pages/sponsors/sponsor-forms-tab/index.js
🚧 Files skipped from review as they are similar to previous changes (6)
- src/actions/sponsor-forms-actions.js
- src/pages/sponsors/sponsor-forms-list-page/components/global-template/select-sponsorships-dialog.js
- src/pages/sponsors/sponsor-forms-list-page/components/form-template/tests/form-template-popup.test.js
- src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.js
- src/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.js
- src/pages/sponsors/sponsor-forms-list-page/components/global-template/tests/global-template-popup.test.js
priscila-moneo
force-pushed
the
fix/adding-forms-does-not-close-modal
branch
2 times, most recently
from
8124e39 to
569a46f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/sponsors/sponsor-forms-list-page/components/global-template/global-template-popup.js (1)
22-25:⚠️ Potential issue | 🟡 MinorState not reset when dialog is dismissed via backdrop click.
handleDismissonly callsonClose()without resettingselectedTemplatesandstage. Since this component stays mounted and controls visibility via theopenprop, state persists across open/close cycles. If a user:
- Selects templates → advances to sponsorships stage
- Clicks backdrop to dismiss (calls
handleDismiss)- Reopens the dialog
The dialog will incorrectly show the sponsorships stage with stale
selectedTemplatesinstead of starting fresh at the templates stage.Suggested fix: Reset state in handleDismiss or add useEffect
Option 1 - Reset in handleDismiss:
const handleDismiss = () => { if (isSaving) return; + setSelectedTemplates([]); + setStage("templates"); onClose(); };Option 2 - Reset on open via useEffect:
+useEffect(() => { + if (open) { + setSelectedTemplates([]); + setStage("templates"); + } +}, [open]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-list-page/components/global-template/global-template-popup.js` around lines 22 - 25, handleDismiss currently only calls onClose(), so selectedTemplates and stage persist across opens; update handleDismiss in global-template-popup.js to reset selectedTemplates and stage to their initial values before calling onClose (or alternatively add a useEffect that watches the open prop and resets selectedTemplates and stage when open becomes false/true); reference the state variables selectedTemplates and stage and the handler handleDismiss (or implement the useEffect tied to the open prop) so the dialog always starts on the templates stage with an empty selection when dismissed and reopened.
🧹 Nitpick comments (3)
src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.js (1)
71-73: Minor: Inline arrow function is unnecessary.The wrapper
() => { handleClose(); }can be simplified to justhandleClosesince they're functionally equivalent here.Suggested simplification
<Dialog open={open} - onClose={() => { - handleClose(); - }} + onClose={handleClose} maxWidth="md"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.js` around lines 71 - 73, Replace the unnecessary inline arrow function passed to the onClose prop with the direct function reference to simplify the code: change the onClose handler currently written as () => { handleClose(); } to use handleClose directly (this affects the component where onClose is set and the handleClose function defined).src/pages/sponsors/sponsor-forms-tab/components/customized-form/__tests__/customized-form-popup.test.js (1)
86-120: Good test for save deduplication, but consider adding a success case test.The duplicate save prevention test is well-structured with the
pendingPromisepattern. However, unlike the parallelform-template-popup.test.js, this file is missing a test that verifiesonSavedandonCloseare called on successful save.Suggested test case to add
it("closes modal and calls onSaved after successful save", async () => { const onClose = jest.fn(); const onSaved = jest.fn(); saveSponsorCustomizedForm.mockReturnValue(() => Promise.resolve()); renderWithRedux( <CustomizedFormPopup formId={null} open onClose={onClose} onSaved={onSaved} sponsor={sponsor} summitId={69} />, { initialState } ); await act(async () => { await userEvent.click( screen.getByRole("button", { name: "submit-customized-form" }) ); await Promise.resolve(); }); expect(saveSponsorCustomizedForm).toHaveBeenCalledTimes(1); expect(onSaved).toHaveBeenCalledTimes(1); expect(onClose).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-tab/components/customized-form/__tests__/customized-form-popup.test.js` around lines 86 - 120, Add a new test to verify the success path: mock saveSponsorCustomizedForm to return a function that resolves (saveSponsorCustomizedForm.mockReturnValue(() => Promise.resolve())), render CustomizedFormPopup via renderWithRedux passing jest.fn() for onSaved and onClose, trigger the submit button via userEvent.click(screen.getByRole("button", { name: "submit-customized-form" })) inside act and await Promise.resolve() to flush promises, then assert saveSponsorCustomizedForm was called once and both onSaved and onClose were each called once.src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js (1)
296-303: Consider addingisSavingguard for consistency with other popups.Unlike other popups in this PR (
FormTemplatePopup,CustomizedFormPopup,GlobalTemplatePopup), this component doesn't have anisSavingguard to prevent duplicate submissions. While the parent handles the async save, adding a consistent pattern would prevent double-click issues if users rapidly click the submit button.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js` around lines 296 - 303, The submit flow lacks an isSaving guard which allows duplicate submissions; add a local isSaving state in the AddSponsorFormTemplatePopup component, update the Button to be disabled when either selectedForms.length === 0 or isSaving, and modify the submit handler (the function passed to the form's onSubmit or the existing handleSubmit) to early-return if isSaving, set isSaving = true at the start, await the parent async save call, and finally set isSaving = false (in a finally block) so rapid double-clicks are ignored and the UX matches FormTemplatePopup/CustomizedFormPopup/GlobalTemplatePopup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/pages/sponsors/sponsor-forms-list-page/components/global-template/global-template-popup.js`:
- Around line 22-25: handleDismiss currently only calls onClose(), so
selectedTemplates and stage persist across opens; update handleDismiss in
global-template-popup.js to reset selectedTemplates and stage to their initial
values before calling onClose (or alternatively add a useEffect that watches the
open prop and resets selectedTemplates and stage when open becomes false/true);
reference the state variables selectedTemplates and stage and the handler
handleDismiss (or implement the useEffect tied to the open prop) so the dialog
always starts on the templates stage with an empty selection when dismissed and
reopened.
---
Nitpick comments:
In
`@src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.js`:
- Around line 71-73: Replace the unnecessary inline arrow function passed to the
onClose prop with the direct function reference to simplify the code: change the
onClose handler currently written as () => { handleClose(); } to use handleClose
directly (this affects the component where onClose is set and the handleClose
function defined).
In
`@src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js`:
- Around line 296-303: The submit flow lacks an isSaving guard which allows
duplicate submissions; add a local isSaving state in the
AddSponsorFormTemplatePopup component, update the Button to be disabled when
either selectedForms.length === 0 or isSaving, and modify the submit handler
(the function passed to the form's onSubmit or the existing handleSubmit) to
early-return if isSaving, set isSaving = true at the start, await the parent
async save call, and finally set isSaving = false (in a finally block) so rapid
double-clicks are ignored and the UX matches
FormTemplatePopup/CustomizedFormPopup/GlobalTemplatePopup.
In
`@src/pages/sponsors/sponsor-forms-tab/components/customized-form/__tests__/customized-form-popup.test.js`:
- Around line 86-120: Add a new test to verify the success path: mock
saveSponsorCustomizedForm to return a function that resolves
(saveSponsorCustomizedForm.mockReturnValue(() => Promise.resolve())), render
CustomizedFormPopup via renderWithRedux passing jest.fn() for onSaved and
onClose, trigger the submit button via
userEvent.click(screen.getByRole("button", { name: "submit-customized-form" }))
inside act and await Promise.resolve() to flush promises, then assert
saveSponsorCustomizedForm was called once and both onSaved and onClose were each
called once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dee8308-7223-435f-8a48-8a0a9284bee9
📒 Files selected for processing (12)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/__tests__/form-template-popup.test.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/__tests__/global-template-popup.test.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/global-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/select-sponsorships-dialog.jssrc/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/__tests__/customized-form-popup.test.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form-popup.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.jssrc/pages/sponsors/sponsor-forms-tab/index.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.js
- src/pages/sponsors/sponsor-forms-tab/index.js
- src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.js
- src/pages/sponsors/sponsor-forms-list-page/components/global-template/tests/global-template-popup.test.js
martinquiroga-exo
requested review from
caseylocker,
martinquiroga-exo and
smarcet
| summitId={summitId} | ||
| /> | ||
| )} | ||
| <AddSponsorFormTemplatePopup |
There was a problem hiding this comment.
@priscila-moneo @martinquiroga-exo
this was a fix introduced at 49d33b8
The conditional rendering pattern ({openPopup === "template" && }) forces React to fully unmount the component when the condition becomes false. On next open, React mounts a brand new instance , every useState, useFormik, useRef, internal MUI state, etc. starts from scratch. There's zero chance of stale state leaking between opens.
The always-render pattern (<Component open={openPopup === "template"} />) keeps the component mounted in the tree at all times. The MUI Dialog just hides/shows itself. All internal state
survives between opens. This means you need to manually reset every piece of state in a useEffect([open]), and if you miss one (which this PR does with formik's add_ons), users see stale data
from their previous interaction
Bottom line
The reason conditional rendering was used for AddSponsorFormTemplatePopup is that it has complex internal state: formik with validation, an async multi-select group with its own cache, search
state, selection state. Manually tracking all of that for reset is error-prone. The unmount/remount approach makes the problem structurally impossible rather than relying on the developer
remembering to reset every piece of state.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review
@martinquiroga-exo for vis
| import MuiFormikSelectGroup from "../../../../../components/mui/formik-inputs/mui-formik-select-group"; | ||
|
|
||
| const AddSponsorFormTemplatePopup = ({ | ||
| open, |
There was a problem hiding this comment.
@priscila-moneo
this popup has no isSaving check
And its formik onSubmit calls the parent callback with no guard:
| disableEnforceFocus | ||
| disableAutoFocus | ||
| disableRestoreFocus | ||
| // ...existing code... |
priscila-moneo
force-pushed
the
fix/adding-forms-does-not-close-modal
branch
2 times, most recently
from
4d56f88 to
aa4612a
Compare
priscila-moneo
force-pushed
the
fix/adding-forms-does-not-close-modal
branch
from
aa4612a to
b072181
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/sponsors/sponsor-forms-tab/index.js (1)
175-189:⚠️ Potential issue | 🟡 MinorThis still resets the customized-forms table to page 1 after save.
Line 184 hardcodes
DEFAULT_CURRENT_PAGE, so saving an edit from page N still bounces the user back to the first page. If page 1 is only intentional for creates, this callback should distinguish create vs. update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-forms-tab/index.js` around lines 175 - 189, handleCustomizedFormSaved currently always passes DEFAULT_CURRENT_PAGE into getSponsorCustomizedForms, causing saves to reset the table to page 1; change the callback in index.js (handleCustomizedFormSaved) to detect whether the operation was a create vs update (e.g., by checking if the saved form has an id or by using a flag in customizedForms) and pass DEFAULT_CURRENT_PAGE only for creates, otherwise pass the current page (use customizedForms.currentPage or the component's current page state) when calling getSponsorCustomizedForms so edits preserve the user's page.
🤖 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/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js`:
- Around line 185-193: The Dialog component in add-sponsor-form-template-popup
is disabling built-in focus management via the props disableEnforceFocus,
disableAutoFocus, and disableRestoreFocus; remove these three props from the
Dialog JSX so the modal uses default MUI focus trapping and restores focus on
close (keep open, onClose={handleClose}, maxWidth="md", fullWidth as-is); verify
keyboard focus is trapped within the Dialog and focus returns to the trigger
after handleClose is called.
---
Duplicate comments:
In `@src/pages/sponsors/sponsor-forms-tab/index.js`:
- Around line 175-189: handleCustomizedFormSaved currently always passes
DEFAULT_CURRENT_PAGE into getSponsorCustomizedForms, causing saves to reset the
table to page 1; change the callback in index.js (handleCustomizedFormSaved) to
detect whether the operation was a create vs update (e.g., by checking if the
saved form has an id or by using a flag in customizedForms) and pass
DEFAULT_CURRENT_PAGE only for creates, otherwise pass the current page (use
customizedForms.currentPage or the component's current page state) when calling
getSponsorCustomizedForms so edits preserve the user's page.
🪄 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: efc9b977-789f-44b2-b59d-285b40b692e4
📒 Files selected for processing (12)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/__tests__/form-template-popup.test.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/__tests__/global-template-popup.test.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/global-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/select-sponsorships-dialog.jssrc/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/__tests__/customized-form-popup.test.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form-popup.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.jssrc/pages/sponsors/sponsor-forms-tab/index.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.js
- src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.js
- src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-popup.js
- src/pages/sponsors/sponsor-forms-list-page/components/global-template/tests/global-template-popup.test.js
- src/pages/sponsors/sponsor-forms-list-page/components/form-template/tests/form-template-popup.test.js
| <Dialog | ||
| open | ||
| onClose={handleClose} | ||
| maxWidth="md" | ||
| fullWidth | ||
| disableEnforceFocus | ||
| disableAutoFocus | ||
| disableRestoreFocus | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @mui/materialv6 Dialog, what dodisableEnforceFocus, disableAutoFocus, and disableRestoreFocus do, and what accessibility tradeoffs do they introduce for modal dialogs?
💡 Result:
In @mui/material v6 Dialog (which inherits Modal props), the focus-related props are: - disableAutoFocus: If true, the modal will not automatically shift focus to itself when it opens, and replace it to the last focused element when it closes. This also works correctly with any modal children that have the disableAutoFocus prop. Generally this should never be set to true as it makes the modal less accessible to assistive technologies, like screen readers. - disableEnforceFocus: If true, the modal will not prevent focus from leaving the modal while open. Generally this should never be set to true as it makes the modal less accessible to assistive technologies, like screen readers. - disableRestoreFocus: If true, the modal will not restore focus to previously focused element once modal is hidden or unmounted. Accessibility tradeoffs for modal dialogs: These props disable key WCAG/WAI-ARIA modal behaviors (focus trap, auto-focus on open/restore on close), which trap focus within the modal to prevent interaction with background content and ensure screen reader users stay oriented. Disabling them allows focus to escape (e.g., for non-modal or nested scenarios like chatbots), but violates modal dialog patterns, potentially confusing screen reader users, allowing accidental background interaction, and failing WCAG 2.4.3 Focus Order or 4.1.2 Name, Role, Value. Use only when necessary (e.g., non-modals), and manually manage focus/ARIA for compliance.
Citations:
- 1: https://v6.mui.com/material-ui/api/dialog
- 2: https://v6.mui.com/material-ui/api/modal
- 3: https://v6.mui.com/joy-ui/api/modal
- 4: https://mui.com/base-ui/react-modal/components-api/?srsltid=AfmBOorAoi9LhD-pl4XSXeuM3ycVjFTJDT2Zq08DL-mHh_cbF452Sn38
- 5: https://mui.com/material-ui/react-modal/
- 6: [Dialog] Focus lost on non-modal dialog after interacting with a full screen dialog mui/material-ui#44904
- 7: [Dialog] disableRestoreFocus only way to enable nested focus mui/material-ui#33330
🏁 Script executed:
# Check if the file exists and examine the code around the specified lines
if [ -f "src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js" ]; then
wc -l "src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js"
echo "---"
sed -n '170,210p' "src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js"
else
echo "File not found. Checking directory structure..."
fd -t f "add-sponsor-form-template-popup" --type f
fiRepository: fntechgit/summit-admin
Length of output: 1895
Remove focus management disabling props from this modal Dialog.
The three props—disableEnforceFocus, disableAutoFocus, and disableRestoreFocus—disable critical WCAG/WAI-ARIA focus management behaviors that prevent focus from escaping the modal and ensure focus is restored on close. Disabling these violates modal dialog patterns and causes accessibility regressions for keyboard and screen reader users, particularly when this modal is unrelated to the save-flow fix.
Suggested fix
<Dialog
open
onClose={handleClose}
maxWidth="md"
fullWidth
- disableEnforceFocus
- disableAutoFocus
- disableRestoreFocus
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Dialog | |
| open | |
| onClose={handleClose} | |
| maxWidth="md" | |
| fullWidth | |
| disableEnforceFocus | |
| disableAutoFocus | |
| disableRestoreFocus | |
| > | |
| <Dialog | |
| open | |
| onClose={handleClose} | |
| maxWidth="md" | |
| fullWidth | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js`
around lines 185 - 193, The Dialog component in add-sponsor-form-template-popup
is disabling built-in focus management via the props disableEnforceFocus,
disableAutoFocus, and disableRestoreFocus; remove these three props from the
Dialog JSX so the modal uses default MUI focus trapping and restores focus on
close (keep open, onClose={handleClose}, maxWidth="md", fullWidth as-is); verify
keyboard focus is trapped within the Dialog and focus returns to the trigger
after handleClose is called.
| add_ons: [] | ||
| }, | ||
| onSubmit: (values) => { | ||
| validationSchema: yup.object({ |
There was a problem hiding this comment.
@priscila-moneo @martinquiroga-exo this breaks functionally already merged
please review https://github.com/fntechgit/summit-admin/pull/823/changes
PR #823 explicitly removed the yup import and the validationSchema from AddSponsorFormTemplatePopup, with the stated goal: "Removed add-on validation when configuring sponsor form templates, allowing submission without a required add-on."
| <Button onClick={handleOnSave} fullWidth variant="contained"> | ||
| <Button | ||
| onClick={handleOnSave} | ||
| disabled={(selection.ids.length === 0 && !selection.all) || isSaving} |
There was a problem hiding this comment.
@priscila-moneo @martinquiroga-exo
Re-adds disabled condition on the Apply button that #823 removed
PR #823 intentionally removed the disabled condition from the Apply button in select-sponsorships-dialog.js, so users could proceed regardless of selection state:
PR #823:
- <Button onClick={handleOnSave} disabled={selection.ids.length === 0 && !selection.all} ...>
+ <Button onClick={handleOnSave} fullWidth variant="contained">
PR #824 puts back the original selection-based disabled condition plus isSaving:
disabled={(selection.ids.length === 0 && !selection.all) || isSaving}
The isSaving part is new and correct, but the selection-based part reverts #823's change.
| <Dialog open={open} onClose={handleClose} maxWidth={dialogSize} fullWidth> | ||
| <Dialog | ||
| open={open} | ||
| onClose={handleDismiss} |
There was a problem hiding this comment.
@priscila-moneo
puts back the original selection-based disabled condition plus isSaving:
disabled={(selection.ids.length === 0 && !selection.all) || isSaving}
The isSaving part is new and correct, but the selection-based part reverts #823's change.
- Reverts GlobalTemplatePopup close-state-reset fix
PR #823 fixed Dialog.onClose to use handleClose (which resets stage and selectedTemplates):
PR #823:
- <Dialog open={open} onClose={onClose} ...>
+ <Dialog open={open} onClose={handleClose} ...>
PR #824 changes it to a new handleDismiss that does NOT reset state:
const handleDismiss = () => {
if (isSaving) return;
onClose(); // no stage/selectedTemplates reset
};
Since this component is always-mounted (not conditionally rendered), this reverts #823's fix , backdrop close now leaks stale stage state to the next open.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comments
priscila-moneo
force-pushed
the
fix/adding-forms-does-not-close-modal
branch
from
835d1bb to
b072181
Compare
priscila-moneo
force-pushed
the
fix/adding-forms-does-not-close-modal
branch
from
cf05858 to
d314fdb
Compare
| }; | ||
| onSubmit(entity); | ||
|
|
||
| await Promise.resolve(onSubmit(entity)); |
There was a problem hiding this comment.
@priscila-moneo this is broken ( parent doesn't return promise )
onSubmit is the parent's handleSaveFormFromTemplate
sponsor-forms-tab/index.js:160
This function calls saveSponsorManagedForm(entity).then(...) but does not return the promise chain. The function returns undefined.
Back in the popup's onSubmit:
await Promise.resolve(onSubmit(entity));
// ^^^^^^^^^^^^^^^^
// returns undefined
//
// So this becomes:
// await Promise.resolve(undefined)
// which resolves immediately
Promise.resolve(undefined) resolves in the same microtask
The entire isSubmitting guard goes true → false in one render cycle, before the API call even starts executing its HTTP request. The user can double-click submit, close the dialog, or press escape during the actual save.
There was a problem hiding this comment.
That's correct, but onSubmit(entity); it's still broken it just does not wait for a microtask tick or, to be more technically accurate, one microtask queue flush. It returns immediately in the same call stack without any microtask delay.
In both versions, the parent still does:
saveSponsorManagedForm(entity).then(...)
So neither popup version knows about the actual HTTP request. That means both versions allow:
- double submit
- dialog close during save
- duplicate API calls
- race conditions
In essence both are wrong. Correct fix should be that the parent must return the async chain. @smarcet wouldn't you agree?
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comments
|
@smarcet Most of the regressions from PR#823 are due to the fact that both were open at the same time, hence using the same commit to branch off. Perhaps we want to look at some alternatives branching models when we have separate tasks that touch the same functionality. Just a suggestion, great catch btw. |
…to fix/adding-forms-does-not-close-modal
4 participants
ref: https://app.clickup.com/t/86b8tct5y
isSavingto prevent duplicate requests from double-clicks in form create/clone dialogs.Summary by CodeRabbit
Bug Fixes
Features
Tests