fix: adjust popup pattern, add isSaving on popups, return promises#994
Open
tomrndom wants to merge 3 commits into
Open
fix: adjust popup pattern, add isSaving on popups, return promises#994tomrndom wants to merge 3 commits into
tomrndom 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
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>
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
1 participant
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/9014802374/86bag2zk7
…emove open prop, adjust tests
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests