fix: adjust popup conditions to open, fetch data before open popup#802
Open
fix: adjust popup conditions to open, fetch data before open popup#802
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughRemoved external Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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
🤖 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-global/form-templates/form-template-item-list-page.js`:
- Around line 116-124: The handlers handleRowEdit and handleNewInventoryItem
currently open modals only on successful promises and swallow rejections; update
their promise chains for getFormTemplateItem(formTemplateId, row.id) and
getInventoryItems() to add .catch handlers that log the error (using
console.error or the app logger) and surface feedback to the user (e.g., set an
error toast/state) before returning, while keeping the existing .then calls that
call setShowInventoryItemModal(true) and setShowAddInventoryItemsModal(true) on
success.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/sponsors-global/form-templates/add-form-template-item-popup.jssrc/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/form-templates/sponsor-inventory-popup.js
Comment on lines
116
to
124
| const handleRowEdit = (row) => { | ||
| if (row) getFormTemplateItem(formTemplateId, row.id); | ||
| setShowInventoryItemModal(true); | ||
| getFormTemplateItem(formTemplateId, row.id).then(() => | ||
| setShowInventoryItemModal(true) | ||
| ); | ||
| }; | ||
|
|
||
| const handleNewInventoryItem = () => { | ||
| getInventoryItems(); | ||
| setShowAddInventoryItemsModal(true); | ||
| getInventoryItems().then(() => setShowAddInventoryItemsModal(true)); | ||
| }; |
There was a problem hiding this comment.
Handle prefetch failures before opening modals.
If either fetch rejects, the UI silently does nothing. Add a rejection path to surface the failure (or at least log it) so users aren’t left without feedback.
🔧 Suggested minimal error handling
const handleRowEdit = (row) => {
- getFormTemplateItem(formTemplateId, row.id).then(() =>
- setShowInventoryItemModal(true)
- );
+ getFormTemplateItem(formTemplateId, row.id)
+ .then(() => setShowInventoryItemModal(true))
+ .catch((error) => {
+ console.error(error);
+ });
};
const handleNewInventoryItem = () => {
- getInventoryItems().then(() => setShowAddInventoryItemsModal(true));
+ getInventoryItems()
+ .then(() => setShowAddInventoryItemsModal(true))
+ .catch((error) => {
+ console.error(error);
+ });
};📝 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.
Suggested change
| const handleRowEdit = (row) => { | |
| if (row) getFormTemplateItem(formTemplateId, row.id); | |
| setShowInventoryItemModal(true); | |
| getFormTemplateItem(formTemplateId, row.id).then(() => | |
| setShowInventoryItemModal(true) | |
| ); | |
| }; | |
| const handleNewInventoryItem = () => { | |
| getInventoryItems(); | |
| setShowAddInventoryItemsModal(true); | |
| getInventoryItems().then(() => setShowAddInventoryItemsModal(true)); | |
| }; | |
| const handleRowEdit = (row) => { | |
| getFormTemplateItem(formTemplateId, row.id) | |
| .then(() => setShowInventoryItemModal(true)) | |
| .catch((error) => { | |
| console.error(error); | |
| }); | |
| }; | |
| const handleNewInventoryItem = () => { | |
| getInventoryItems() | |
| .then(() => setShowAddInventoryItemsModal(true)) | |
| .catch((error) => { | |
| console.error(error); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors-global/form-templates/form-template-item-list-page.js`
around lines 116 - 124, The handlers handleRowEdit and handleNewInventoryItem
currently open modals only on successful promises and swallow rejections; update
their promise chains for getFormTemplateItem(formTemplateId, row.id) and
getInventoryItems() to add .catch handlers that log the error (using
console.error or the app logger) and surface feedback to the user (e.g., set an
error toast/state) before returning, while keeping the existing .then calls that
call setShowInventoryItemModal(true) and setShowAddInventoryItemsModal(true) on
success.
smarcet
reviewed
Feb 24, 2026
src/pages/sponsors-global/form-templates/form-template-item-list-page.js
Show resolved
Hide resolved
smarcet
reviewed
Feb 24, 2026
src/pages/sponsors-global/form-templates/form-template-item-list-page.js
Show resolved
Hide resolved
smarcet
requested changes
Feb 24, 2026
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/form-template-item-actions.js (1)
203-237:⚠️ Potential issue | 🟡 MinorRemove double-dispatch around snackbarSuccessHandler.
Line 230 wraps
snackbarSuccessHandlerindispatch(dispatch(...)), which will dispatch the same action twice and can show duplicate snackbars.🐛 Proposed fix
- dispatch( - dispatch( - snackbarSuccessHandler({ - title: T.translate("general.success"), - html: T.translate( - "edit_form_template_item.form_template_item_saved" - ) - }) - ) - ); + dispatch( + snackbarSuccessHandler({ + title: T.translate("general.success"), + html: T.translate( + "edit_form_template_item.form_template_item_saved" + ) + }) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/form-template-item-actions.js` around lines 203 - 237, The success snackbar is being dispatched twice because the code calls dispatch(dispatch(snackbarSuccessHandler(...))) inside the Promise.all . Replace the double-dispatch with a single dispatch call: invoke snackbarSuccessHandler(...) and pass its result once to dispatch (i.e., dispatch(snackbarSuccessHandler({...}))) so the action is only dispatched a single time; update the code path that currently references snackbarSuccessHandler within the .then of Promise.all in form-template-item-actions.js accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/actions/form-template-item-actions.js`:
- Around line 203-237: The success snackbar is being dispatched twice because
the code calls dispatch(dispatch(snackbarSuccessHandler(...))) inside the
Promise.all . Replace the double-dispatch with a single dispatch call: invoke
snackbarSuccessHandler(...) and pass its result once to dispatch (i.e.,
dispatch(snackbarSuccessHandler({...}))) so the action is only dispatched a
single time; update the code path that currently references
snackbarSuccessHandler within the .then of Promise.all in
form-template-item-actions.js accordingly.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/form-template-item-actions.js (1)
113-119:⚠️ Potential issue | 🔴 CriticalMove
stopLoading()into.finally()blocks to ensure loading state clears on request failures.
startLoading()is dispatched, butstopLoading()only runs in.then(). SincesnackbarErrorHandlerdoesn't clear loading state (it only displays an error message), failed requests leave the UI stuck with loading active. MovestopLoading()to.finally()to guarantee cleanup in both success and error cases.🛠️ Suggested pattern (apply similarly to other requests)
- )(params)(dispatch).then(() => { - dispatch(stopLoading()); - }); + )(params)(dispatch) + .finally(() => { + dispatch(stopLoading()); + });Applies to lines: 113–119, 135–140, 155–164, 203–210, 250–256, 311–317. For lines 203–210 and 250–256, also add
.finally()to the outer promise chain, not just the nested Promise.all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/form-template-item-actions.js` around lines 113 - 119, The action dispatches call startLoading() but only invoke stopLoading() inside .then() handlers (e.g., the getRequest call that creates REQUEST_FORM_TEMPLATE_ITEMS/RECEIVE_FORM_TEMPLATE_ITEMS using getRequest, createAction and snackbarErrorHandler), so failed requests leave loading stuck; move stopLoading() into a .finally() on the promise returned by getRequest (and similarly for other occurrences listed) so stopLoading() always runs, and for cases with nested Promise.all ensure you attach .finally() to the outer promise chain (not only the inner Promise.all) to guarantee cleanup even on failures.
♻️ Duplicate comments (1)
src/pages/sponsors-global/form-templates/form-template-item-list-page.js (1)
116-125:⚠️ Potential issue | 🟡 MinorAdd rejection handling before opening modals.
Guarding a missing row is good, but fetch failures still leave the UI silent. Add a
.catchto surface errors before returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors-global/form-templates/form-template-item-list-page.js` around lines 116 - 125, Both handleRowEdit and handleNewInventoryItem currently open modals after async fetches but lack rejection handling; update the chains that call getFormTemplateItem(formTemplateId, row.id) in handleRowEdit and getInventoryItems() in handleNewInventoryItem to add .catch(...) so failures surface before opening the modal (log or surface the error via process/UI error handler, then do not call setShowInventoryItemModal or setShowAddInventoryItemsModal). Ensure the .catch references the same promise chain that currently calls setShowInventoryItemModal and setShowAddInventoryItemsModal so errors block modal opening and are reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/actions/form-template-item-actions.js`:
- Around line 113-119: The action dispatches call startLoading() but only invoke
stopLoading() inside .then() handlers (e.g., the getRequest call that creates
REQUEST_FORM_TEMPLATE_ITEMS/RECEIVE_FORM_TEMPLATE_ITEMS using getRequest,
createAction and snackbarErrorHandler), so failed requests leave loading stuck;
move stopLoading() into a .finally() on the promise returned by getRequest (and
similarly for other occurrences listed) so stopLoading() always runs, and for
cases with nested Promise.all ensure you attach .finally() to the outer promise
chain (not only the inner Promise.all) to guarantee cleanup even on failures.
---
Duplicate comments:
In `@src/pages/sponsors-global/form-templates/form-template-item-list-page.js`:
- Around line 116-125: Both handleRowEdit and handleNewInventoryItem currently
open modals after async fetches but lack rejection handling; update the chains
that call getFormTemplateItem(formTemplateId, row.id) in handleRowEdit and
getInventoryItems() in handleNewInventoryItem to add .catch(...) so failures
surface before opening the modal (log or surface the error via process/UI error
handler, then do not call setShowInventoryItemModal or
setShowAddInventoryItemsModal). Ensure the .catch references the same promise
chain that currently calls setShowInventoryItemModal and
setShowAddInventoryItemsModal so errors block modal opening and are reported.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/actions/form-template-item-actions.jssrc/pages/sponsors-global/form-templates/form-template-item-list-page.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b8m74u3
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit