Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
… templates Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughThe changes introduce a template-based workflow for adding sponsor-managed pages. This includes a new Redux action (saveSponsorManagedPage) for persisting pages, a new modal popup component for selecting pages and add-ons with sorting and search capabilities, integration into the sponsor pages tab, a form component enhancement for handling empty states, and supporting translation keys. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as React Component<br/>(SponsorPagesTab)
participant Popup as AddSponsorPageTemplate<br/>Popup
participant Form as Formik Form<br/>(Validation)
participant Redux as Redux Store<br/>(Actions)
participant API as Backend API
User->>UI: Click "Using template"
UI->>UI: Set openPopup = "template"
UI->>Popup: Render popup with onSubmit<br/>callback
Popup->>Popup: Fetch pages on mount
Popup->>Redux: Dispatch getShowPages
Redux->>API: GET pages
API-->>Redux: Pages data
Redux-->>Popup: Pages loaded
Popup->>Popup: Render pages table
User->>Popup: Select pages, sort, search
User->>Popup: Select add-ons
User->>Popup: Click "Add"
Popup->>Form: Validate selections
alt Validation passes
Form-->>Popup: Valid
Popup->>Popup: Package entity<br/>(pages, add-ons)
Popup->>UI: Call onSubmit(entity)
UI->>Redux: Dispatch saveSponsorManagedPage
Redux->>API: POST sponsor-managed-page
API-->>Redux: Created
Redux->>Redux: Dispatch SPONSOR_MANAGED_PAGE_ADDED
UI->>Redux: Refresh managed pages list
UI->>Popup: Close popup
else Validation fails
Form-->>Popup: Show error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/actions/sponsor-pages-actions.js (1)
172-185:normalizeSponsorManagedPagehandles "all" correctly, but the initial.map()on line 175 is wasteful when "all" is present.When
add_onsis["all"], line 175 calls"all".id→undefined, then lines 179-182 immediately overwriteallowed_add_onswith[]. Consider checking the "all" case first to avoid the unnecessary mapping and the transient[undefined]value.♻️ Suggested refactor
const normalizeSponsorManagedPage = (entity) => { + const isAll = entity.add_ons.includes("all"); const normalizedEntity = { show_page_ids: entity.pages, - allowed_add_ons: entity.add_ons.map((a) => a.id), - apply_to_all_add_ons: false + allowed_add_ons: isAll ? [] : entity.add_ons.map((a) => a.id), + apply_to_all_add_ons: isAll }; - if (entity.add_ons.includes("all")) { - normalizedEntity.apply_to_all_add_ons = true; - normalizedEntity.allowed_add_ons = []; - } - return normalizedEntity; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-pages-actions.js` around lines 172 - 185, normalizeSponsorManagedPage currently maps entity.add_ons to ids before checking for the "all" sentinel, producing a transient [undefined]; change the logic so you first check if entity.add_ons.includes("all") and if so set apply_to_all_add_ons = true and allowed_add_ons = [] immediately, otherwise compute allowed_add_ons = entity.add_ons.map(a => a.id); preserve show_page_ids assignment and default apply_to_all_add_ons = false, and update only within the normalizeSponsorManagedPage function.src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js (1)
301-303: Incomplete PropTypes declaration.Only
onCloseis declared.onSubmit,sponsor, andsummitIdare required for correct operation but are missing from propTypes.♻️ Suggested improvement
AddSponsorPageTemplatePopup.propTypes = { - onClose: PropTypes.func.isRequired + onClose: PropTypes.func.isRequired, + onSubmit: PropTypes.func.isRequired, + sponsor: PropTypes.object.isRequired, + summitId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js` around lines 301 - 303, Add missing PropTypes for AddSponsorPageTemplatePopup: extend AddSponsorPageTemplatePopup.propTypes to include onSubmit (PropTypes.func.isRequired), sponsor (PropTypes.object.isRequired or a more specific shape if available), and summitId (PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired) alongside the existing onClose declaration so the component's required props are explicitly validated.
🤖 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/actions/sponsor-pages-actions.js`:
- Around line 161-170: The loading state isn't cleared on failures because
stopLoading is only dispatched in the .then() of the postRequest call; update
the postRequest invocation (the call that returns
postRequest(...)(params)(dispatch) in this file) to call .finally(() => {
dispatch(stopLoading()); }) instead of .then(...), so stopLoading runs on both
success and failure (follow the same pattern used in cloneGlobalPage).
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 229-242: Replace the two hardcoded English strings with i18n
calls: change the selected count label currently rendered in the Grid2 that
shows "{selectedPages.length} items selected" to use T.translate with a key like
"forms_tab.items_selected" (or reuse an existing key) and interpolate
selectedPages.length, and change the MenuButton children text "sort by" to
T.translate("forms_tab.sort_by") (or the appropriate locale key); keep the
SwapVertIcon and the MenuButton menuItems using
T.translate("forms_tab.sort_asc_label") and
T.translate("forms_tab.sort_desc_label") for "A-Z"/"Z-A" if not already, and
update code around MenuButton, handleSort, selectedPages to pass the translated
strings.
- Around line 46-51: sponsor.sponsorships can be null/undefined so mapping
directly causes runtime errors; fix by using a safe fallback when creating
sponsorshipIds and sponsorshipTypeIds (e.g. map over sponsor?.sponsorships ?? []
or (sponsor.sponsorships || [])) so the declarations for sponsorshipIds and
sponsorshipTypeIds use that fallback and avoid throwing when sponsorships is
missing.
- Around line 89-124: handlePageChange and handleSort call getShowPages with the
Redux prop term while handleOnSearch uses local searchTerm, causing the search
to be lost on pagination/sort; fix by using a single source of truth—either
(preferred) pass the local searchTerm from handlePageChange and handleSort into
getShowPages (replace term with searchTerm) or update the Redux term when
handleOnSearch runs so all handlers use the Redux term. Update the function
calls in handlePageChange and handleSort (and any other pagination/sort
handlers) to use the chosen search value and keep other parameters (currentPage,
perPage/FIVE_PER_PAGE, order, orderDir, sponsorshipTypeIds) unchanged so
searches persist across pages and sorts.
In `@src/pages/sponsors/sponsor-pages-tab/index.js`:
- Around line 195-200: The handler handleSaveManagedPageFromTemplate currently
calls saveSponsorManagedPage(entity).then(...) but has no .catch(), allowing
snackbarErrorHandler to swallow errors and still run setOpenPopup(null) and
getSponsorManagedPages(); change it to only close the popup and refresh on true
success by attaching a .catch() (or using async/await with try/catch) around
saveSponsorManagedPage: on success call setOpenPopup(null) and
getSponsorManagedPages(), on failure do not close the popup and let
snackbarErrorHandler display the error (or rethrow/return a rejected promise) so
failure paths prevent closing and refreshing.
---
Nitpick comments:
In `@src/actions/sponsor-pages-actions.js`:
- Around line 172-185: normalizeSponsorManagedPage currently maps entity.add_ons
to ids before checking for the "all" sentinel, producing a transient
[undefined]; change the logic so you first check if
entity.add_ons.includes("all") and if so set apply_to_all_add_ons = true and
allowed_add_ons = [] immediately, otherwise compute allowed_add_ons =
entity.add_ons.map(a => a.id); preserve show_page_ids assignment and default
apply_to_all_add_ons = false, and update only within the
normalizeSponsorManagedPage function.
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 301-303: Add missing PropTypes for AddSponsorPageTemplatePopup:
extend AddSponsorPageTemplatePopup.propTypes to include onSubmit
(PropTypes.func.isRequired), sponsor (PropTypes.object.isRequired or a more
specific shape if available), and summitId
(PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired) alongside
the existing onClose declaration so the component's required props are
explicitly validated.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/actions/sponsor-pages-actions.jssrc/components/mui/formik-inputs/mui-formik-select-group.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.jssrc/pages/sponsors/sponsor-pages-tab/index.js
| return postRequest( | ||
| null, | ||
| createAction(SPONSOR_MANAGED_PAGE_ADDED), | ||
| `${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/sponsors/${sponsorId}/managed-pages`, | ||
| normalizedEntity, | ||
| snackbarErrorHandler | ||
| )(params)(dispatch).then(() => { | ||
| dispatch(stopLoading()); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Loading state never cleared on request failure.
stopLoading is dispatched inside .then(), so if the POST request fails, the loading indicator stays indefinitely. Compare with cloneGlobalPage (line 80) which uses .finally().
🐛 Proposed fix
return postRequest(
null,
createAction(SPONSOR_MANAGED_PAGE_ADDED),
`${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/sponsors/${sponsorId}/managed-pages`,
normalizedEntity,
snackbarErrorHandler
- )(params)(dispatch).then(() => {
- dispatch(stopLoading());
- });
+ )(params)(dispatch).finally(() => {
+ dispatch(stopLoading());
+ });📝 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.
| return postRequest( | |
| null, | |
| createAction(SPONSOR_MANAGED_PAGE_ADDED), | |
| `${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/sponsors/${sponsorId}/managed-pages`, | |
| normalizedEntity, | |
| snackbarErrorHandler | |
| )(params)(dispatch).then(() => { | |
| dispatch(stopLoading()); | |
| }); | |
| }; | |
| return postRequest( | |
| null, | |
| createAction(SPONSOR_MANAGED_PAGE_ADDED), | |
| `${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/sponsors/${sponsorId}/managed-pages`, | |
| normalizedEntity, | |
| snackbarErrorHandler | |
| )(params)(dispatch).finally(() => { | |
| dispatch(stopLoading()); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/sponsor-pages-actions.js` around lines 161 - 170, The loading
state isn't cleared on failures because stopLoading is only dispatched in the
.then() of the postRequest call; update the postRequest invocation (the call
that returns postRequest(...)(params)(dispatch) in this file) to call
.finally(() => { dispatch(stopLoading()); }) instead of .then(...), so
stopLoading runs on both success and failure (follow the same pattern used in
cloneGlobalPage).
| const [searchTerm, setSearchTerm] = useState(""); | ||
| const [selectedPages, setSelectedPages] = useState([]); | ||
|
|
||
| const sponsorshipIds = sponsor.sponsorships.map((e) => e.id); | ||
|
|
||
| const sponsorshipTypeIds = sponsor.sponsorships.map((e) => e.type.id); |
There was a problem hiding this comment.
Missing null-safety on sponsor.sponsorships.
If sponsor.sponsorships is undefined or null, lines 49 and 51 will throw. Consider adding a fallback.
🛡️ Proposed fix
- const sponsorshipIds = sponsor.sponsorships.map((e) => e.id);
- const sponsorshipTypeIds = sponsor.sponsorships.map((e) => e.type.id);
+ const sponsorships = sponsor?.sponsorships || [];
+ const sponsorshipIds = sponsorships.map((e) => e.id);
+ const sponsorshipTypeIds = sponsorships.map((e) => e.type.id);📝 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.
| const [searchTerm, setSearchTerm] = useState(""); | |
| const [selectedPages, setSelectedPages] = useState([]); | |
| const sponsorshipIds = sponsor.sponsorships.map((e) => e.id); | |
| const sponsorshipTypeIds = sponsor.sponsorships.map((e) => e.type.id); | |
| const [searchTerm, setSearchTerm] = useState(""); | |
| const [selectedPages, setSelectedPages] = useState([]); | |
| const sponsorships = sponsor?.sponsorships || []; | |
| const sponsorshipIds = sponsorships.map((e) => e.id); | |
| const sponsorshipTypeIds = sponsorships.map((e) => e.type.id); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`
around lines 46 - 51, sponsor.sponsorships can be null/undefined so mapping
directly causes runtime errors; fix by using a safe fallback when creating
sponsorshipIds and sponsorshipTypeIds (e.g. map over sponsor?.sponsorships ?? []
or (sponsor.sponsorships || [])) so the declarations for sponsorshipIds and
sponsorshipTypeIds use that fallback and avoid throwing when sponsorships is
missing.
| const handlePageChange = (page) => { | ||
| getShowPages( | ||
| term, | ||
| page, | ||
| FIVE_PER_PAGE, | ||
| order, | ||
| orderDir, | ||
| false, | ||
| sponsorshipTypeIds | ||
| ); | ||
| }; | ||
|
|
||
| const handleSort = (key, dir) => { | ||
| getShowPages( | ||
| term, | ||
| currentPage, | ||
| FIVE_PER_PAGE, | ||
| key, | ||
| dir, | ||
| false, | ||
| sponsorshipTypeIds | ||
| ); | ||
| }; | ||
|
|
||
| const handleOnSearch = (ev) => { | ||
| if (ev.key === "Enter") | ||
| getShowPages( | ||
| searchTerm, | ||
| currentPage, | ||
| perPage, | ||
| order, | ||
| orderDir, | ||
| false, | ||
| sponsorshipTypeIds | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Search term inconsistency between local state and Redux prop causes pagination/sort to lose search query.
handlePageChange (line 90-92) and handleSort (line 101-104) use term from Redux props, but handleOnSearch (line 115-116) uses local searchTerm state. After searching, paginating or sorting would revert to the stale Redux term, losing the user's search.
Use a single source of truth — either the local searchTerm for all handlers, or update the Redux term when searching.
🐛 Proposed fix (use local searchTerm consistently)
const handlePageChange = (page) => {
getShowPages(
- term,
+ searchTerm,
page,
FIVE_PER_PAGE,
order,
orderDir,
false,
sponsorshipTypeIds
);
};
const handleSort = (key, dir) => {
getShowPages(
- term,
+ searchTerm,
currentPage,
FIVE_PER_PAGE,
key,
dir,
false,
sponsorshipTypeIds
);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`
around lines 89 - 124, handlePageChange and handleSort call getShowPages with
the Redux prop term while handleOnSearch uses local searchTerm, causing the
search to be lost on pagination/sort; fix by using a single source of
truth—either (preferred) pass the local searchTerm from handlePageChange and
handleSort into getShowPages (replace term with searchTerm) or update the Redux
term when handleOnSearch runs so all handlers use the Redux term. Update the
function calls in handlePageChange and handleSort (and any other pagination/sort
handlers) to use the chosen search value and keep other parameters (currentPage,
perPage/FIVE_PER_PAGE, order, orderDir, sponsorshipTypeIds) unchanged so
searches persist across pages and sorts.
| <Grid2 size={4}>{selectedPages.length} items selected</Grid2> | ||
| </Grid2> | ||
| <Grid2 container spacing={2} size={6}> | ||
| <Grid2 size={4}> | ||
| <MenuButton | ||
| buttonId="sort-button" | ||
| menuId="sort-menu" | ||
| menuItems={[ | ||
| { label: "A-Z", onClick: () => handleSort("name", 1) }, | ||
| { label: "Z-A", onClick: () => handleSort("name", 0) } | ||
| ]} | ||
| > | ||
| <SwapVertIcon fontSize="large" sx={{ mr: 1 }} /> sort by | ||
| </MenuButton> |
There was a problem hiding this comment.
Hardcoded English strings — use i18n translations.
"items selected" (line 229) and "sort by" (line 241) are hardcoded in English. The rest of the UI properly uses T.translate(). The forms_tab section already has sort_asc_label / sort_desc_label translations for "A-Z" / "Z-A" that could be reused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`
around lines 229 - 242, Replace the two hardcoded English strings with i18n
calls: change the selected count label currently rendered in the Grid2 that
shows "{selectedPages.length} items selected" to use T.translate with a key like
"forms_tab.items_selected" (or reuse an existing key) and interpolate
selectedPages.length, and change the MenuButton children text "sort by" to
T.translate("forms_tab.sort_by") (or the appropriate locale key); keep the
SwapVertIcon and the MenuButton menuItems using
T.translate("forms_tab.sort_asc_label") and
T.translate("forms_tab.sort_desc_label") for "A-Z"/"Z-A" if not already, and
update code around MenuButton, handleSort, selectedPages to pass the translated
strings.
| const handleSaveManagedPageFromTemplate = (entity) => { | ||
| saveSponsorManagedPage(entity).then(() => { | ||
| setOpenPopup(null); | ||
| getSponsorManagedPages(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "snackbarErrorHandler" --type=js -A 10 | head -80Repository: fntechgit/summit-admin
Length of output: 5635
🏁 Script executed:
rg -n "export.*snackbarErrorHandler\|const snackbarErrorHandler\|function snackbarErrorHandler" --type=js -B 2 -A 15Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
fd -type f -name "*.js" | xargs rg -l "saveSponsorManagedPage" | head -10Repository: fntechgit/summit-admin
Length of output: 336
🏁 Script executed:
cat src/actions/base-actions.js | head -100Repository: fntechgit/summit-admin
Length of output: 1946
🏁 Script executed:
rg -n "export.*saveSponsorManagedPage\|const saveSponsorManagedPage" src/actions/sponsor-pages-actions.js -A 20Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
rg -n "saveSponsorManagedPage" src/actions/sponsor-pages-actions.js -A 25Repository: fntechgit/summit-admin
Length of output: 1038
🏁 Script executed:
grep -n "saveSponsorManagedPage" src/actions/sponsor-pages-actions.jsRepository: fntechgit/summit-admin
Length of output: 107
🏁 Script executed:
rg -n "postRequest" openstack-uicore-foundation 2>/dev/null || echo "Library not in repo, checking usage patterns"Repository: fntechgit/summit-admin
Length of output: 110
🏁 Script executed:
# Check how other components handle saveSponsorManagedPage or similar patterns
rg -B 2 -A 8 "\.then\(\(\) => {.*setOpen" src/pages --type=js | head -40Repository: fntechgit/summit-admin
Length of output: 191
🏁 Script executed:
# Search for similar then/catch patterns after saveSponsorManagedPage or similar save actions
rg -n "saveSponsorManagedPage.*then|getSponsorManagedPages" src/pages/sponsors/sponsor-pages-tab/index.js -A 5 -B 2Repository: fntechgit/summit-admin
Length of output: 2263
🏁 Script executed:
# Look for .catch() patterns in the same file
rg -n "\.catch\(" src/pages/sponsors/sponsor-pages-tab/index.jsRepository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check getSponsorManagedPages implementation
rg -n "export const getSponsorManagedPages" src/actions/sponsor-pages-actions.js -A 15Repository: fntechgit/summit-admin
Length of output: 601
Add error handling to prevent closing the popup and refreshing the list on failed save.
handleSaveManagedPageFromTemplate chains .then() on saveSponsorManagedPage(entity) without a .catch(). Since snackbarErrorHandler (used in the action) doesn't re-throw the error, the promise resolves normally even on failure. This causes setOpenPopup(null) and getSponsorManagedPages() to execute regardless of whether the save succeeded, closing the popup and refreshing the list even when the request fails.
Add a .catch() handler or restructure to ensure the popup only closes and the list only refreshes on confirmed success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors/sponsor-pages-tab/index.js` around lines 195 - 200, The
handler handleSaveManagedPageFromTemplate currently calls
saveSponsorManagedPage(entity).then(...) but has no .catch(), allowing
snackbarErrorHandler to swallow errors and still run setOpenPopup(null) and
getSponsorManagedPages(); change it to only close the popup and refresh on true
success by attaching a .catch() (or using async/await with try/catch) around
saveSponsorManagedPage: on success call setOpenPopup(null) and
getSponsorManagedPages(), on failure do not close the popup and let
snackbarErrorHandler display the error (or rethrow/return a rejected promise) so
failure paths prevent closing and refreshing.
ref: https://app.clickup.com/t/86b7991c2
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
UI/UX Improvements