Open
Conversation
Signed-off-by: Tomás Castillo <[email protected]>
Signed-off-by: Tomás Castillo <[email protected]>
… templates Signed-off-by: Tomás Castillo <[email protected]>
|
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:
📝 WalkthroughWalkthroughAdds a template-based workflow for creating sponsor-managed pages: new saveSponsorManagedPage action and normalize helper, a modal AddSponsorPageTemplatePopup component integrated into SponsorPagesTab, a Formik select-group empty-state prop, several i18n keys, and localized sort labels across form template UIs. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as SponsorPagesTab
participant Popup as AddSponsorPageTemplatePopup
participant Form as FormikForm
participant Redux as ReduxStore
participant API as BackendAPI
User->>UI: Click "Using template"
UI->>UI: set openPopup = "template"
UI->>Popup: render with onSubmit/onClose
Popup->>Redux: dispatch getShowPages (on mount / search / sort / page)
Redux->>API: GET /show-pages
API-->>Redux: pages list
Redux-->>Popup: pages loaded
Popup->>User: render table (select, search, sort, paginate)
User->>Popup: select pages & add-ons, click "Add"
Popup->>Form: validate selections
alt valid
Form-->>Popup: valid
Popup->>UI: call onSubmit(entity)
UI->>Redux: dispatch saveSponsorManagedPage(entity)
Redux->>API: POST /sponsor-managed-pages
API-->>Redux: created resource
Redux->>Redux: dispatch SPONSOR_MANAGED_PAGE_ADDED
UI->>Redux: refresh managed pages list
UI->>Popup: close popup
else invalid
Form-->>Popup: show errors
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
tomrndom
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Signed-off-by: Tomás Castillo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js (2)
52-54:⚠️ Potential issue | 🟠 MajorGuard sponsor access before mapping and query param usage.
Line 52 and Line 54 can throw if
sponsor/sponsorshipsis missing, and Line 213 also assumessponsor.idexists.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) + .filter((id) => id != null); ... - queryParams={[summitId, sponsor.id, sponsorshipIds]} + queryParams={[summitId, sponsor?.id, sponsorshipIds]}Also applies to: 213-213
🤖 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 52 - 54, Guard against missing sponsor or sponsor.sponsorships before mapping and before using sponsor.id: ensure sponsor is defined and sponsor.sponsorships is an array (e.g., default to [] or use optional chaining) before computing sponsorshipIds and sponsorshipTypeIds in the code that defines those constants, and validate sponsor.id exists before using it at the location referencing sponsor.id (line ~213) to avoid runtime errors in functions/components that reference sponsorshipIds, sponsorshipTypeIds, and sponsor.id.
232-232:⚠️ Potential issue | 🟡 MinorReplace remaining hardcoded labels with i18n keys.
Line 232 (
items selected) and Line 250 (sort by) are still hardcoded English.Proposed fix
- <Grid2 size={4}>{selectedPages.length} items selected</Grid2> + <Grid2 size={4}> + {selectedPages.length}{" "} + {T.translate("show_pages.global_page_popup.items_selected")} + </Grid2> ... - <SwapVertIcon fontSize="large" sx={{ mr: 1 }} /> sort by + <SwapVertIcon fontSize="large" sx={{ mr: 1 }} />{" "} + {T.translate("show_pages.sort_by")}Also applies to: 250-250
🤖 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` at line 232, Replace the hardcoded English labels with i18n keys: change the Grid2 text that renders "{selectedPages.length} items selected" to use the translation helper (e.g. t or useTranslation) and pass the count via interpolation (for example t('sponsors.selectedItems', { count: selectedPages.length }) or a pluralized key), and likewise replace the "sort by" label with a translation key (e.g. t('sponsors.sortBy')). Ensure you import/use the i18n hook/function in the component and add appropriate keys (sponsors.selectedItems, sponsors.sortBy) to the locale files.
🤖 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-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 116-127: handleOnSearch currently triggers getShowPages on Enter
but because the input is inside a form pressing Enter also causes a form submit;
update the Enter branch in handleOnSearch to call ev.preventDefault() and
ev.stopPropagation() before invoking getShowPages to avoid the unintended
onSubmit, and apply the same fix to the other search key handler used at the
block referenced around lines 201-203 so both handlers prevent default
submission and propagation when ev.key === "Enter".
- Around line 278-291: The code hides the MuiTable when showPages is empty which
suppresses the table’s built-in empty state; remove the conditional around the
MuiTable so it always renders (pass showPages as the data prop even when length
is 0) and keep existing props (columns, tableOptions, currentPage, perPage,
totalCount, onSort: handleSort, onPageChange: handlePageChange) so the table can
display its empty/no-results UI.
---
Duplicate comments:
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 52-54: Guard against missing sponsor or sponsor.sponsorships
before mapping and before using sponsor.id: ensure sponsor is defined and
sponsor.sponsorships is an array (e.g., default to [] or use optional chaining)
before computing sponsorshipIds and sponsorshipTypeIds in the code that defines
those constants, and validate sponsor.id exists before using it at the location
referencing sponsor.id (line ~213) to avoid runtime errors in
functions/components that reference sponsorshipIds, sponsorshipTypeIds, and
sponsor.id.
- Line 232: Replace the hardcoded English labels with i18n keys: change the
Grid2 text that renders "{selectedPages.length} items selected" to use the
translation helper (e.g. t or useTranslation) and pass the count via
interpolation (for example t('sponsors.selectedItems', { count:
selectedPages.length }) or a pluralized key), and likewise replace the "sort by"
label with a translation key (e.g. t('sponsors.sortBy')). Ensure you import/use
the i18n hook/function in the component and add appropriate keys
(sponsors.selectedItems, sponsors.sortBy) to the locale files.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/i18n/en.jsonsrc/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js
Comment on lines
116
to
127
| const handleOnSearch = (ev) => { | ||
| if (ev.key === "Enter") | ||
| getShowPages( | ||
| searchTerm, | ||
| DEFAULT_CURRENT_PAGE, | ||
| perPage, | ||
| order, | ||
| orderDir, | ||
| false, | ||
| sponsorshipTypeIds | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Pressing Enter in search can trigger an unintended submit.
Because the search field is inside a <form>, Enter can submit and call onSubmit while trying to search.
Proposed fix
const handleOnSearch = (ev) => {
- if (ev.key === "Enter")
+ if (ev.key === "Enter") {
+ ev.preventDefault();
+ ev.stopPropagation();
getShowPages(
searchTerm,
DEFAULT_CURRENT_PAGE,
perPage,
order,
orderDir,
false,
sponsorshipTypeIds
);
+ }
};Also applies to: 201-203
🤖 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 116 - 127, handleOnSearch currently triggers getShowPages on Enter
but because the input is inside a form pressing Enter also causes a form submit;
update the Enter branch in handleOnSearch to call ev.preventDefault() and
ev.stopPropagation() before invoking getShowPages to avoid the unintended
onSubmit, and apply the same fix to the other search key handler used at the
block referenced around lines 201-203 so both handlers prevent default
submission and propagation when ev.key === "Enter".
Signed-off-by: Tomás Castillo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js (3)
52-54:⚠️ Potential issue | 🔴 Critical
sponsor.sponsorshipsstill lacks null-safety.
sponsor.sponsorships.map(...)throws ifsponsorshipsisnullorundefined.🛡️ 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);🤖 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 52 - 54, sponsor.sponsorships can be null/undefined causing sponsorshipIds and sponsorshipTypeIds to throw; update the code that builds sponsorshipIds and sponsorshipTypeIds to guard against a missing array (e.g., use optional chaining or default to an empty array when mapping) so both const sponsorshipIds and const sponsorshipTypeIds operate on a safe array derived from sponsor.sponsorships.
92-114:⚠️ Potential issue | 🔴 Critical
handlePageChangeandhandleSortstill use stale Reduxterm, breaking search-then-paginate/sort.Both handlers pass the Redux
termprop instead of the localsearchTerm, so any active search is silently discarded when the user paginates or sorts.🐛 Proposed fix
const handlePageChange = (page) => { getShowPages( - term, + searchTerm, page, FIVE_PER_PAGE, order, orderDir, false, sponsorshipTypeIds ); }; const handleSort = (key, dir) => { getShowPages( - term, + searchTerm, DEFAULT_CURRENT_PAGE, 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 92 - 114, The handlers handlePageChange and handleSort are passing the stale Redux prop term instead of the local controlled searchTerm, causing active searches to be lost when paginating/sorting; update both functions so they call getShowPages with searchTerm (not term) as the first argument—keep all other args (page/current, FIVE_PER_PAGE, order/orderDir or key/dir, false, sponsorshipTypeIds, and DEFAULT_CURRENT_PAGE for handleSort) unchanged so pagination and sorting respect the current local search state.
235-235:⚠️ Potential issue | 🟡 MinorHardcoded English strings —
"items selected"and"sort by"are not translated.Line 235 (
"{n} items selected") and line 253 ("sort by") bypassT.translate()while every other UI label in the component uses it.🛡️ Proposed fix
- <Grid2 size={4}>{selectedPages.length} items selected</Grid2> + <Grid2 size={4}> + {T.translate("edit_sponsor.pages_tab.items_selected", { count: selectedPages.length })} + </Grid2>- <SwapVertIcon fontSize="large" sx={{ mr: 1 }} /> sort by + <SwapVertIcon fontSize="large" sx={{ mr: 1 }} /> + {T.translate("edit_sponsor.pages_tab.sort_by")}🤖 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` at line 235, The strings "items selected" and "sort by" are hardcoded and must use the project's translation helper; replace the literal text in the JSX where Grid2 renders "{selectedPages.length} items selected" and where the sort label is rendered with calls to T.translate (e.g., T.translate('items_selected', { count: selectedPages.length }) or the project's pluralization pattern) and T.translate('sort_by') respectively, ensuring you pass the dynamic count to the translation function for correct pluralization and match the existing T.translate usage elsewhere in this component.
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js (1)
318-320:PropTypesis incomplete —onSubmit,sponsor,summitIdare required but undeclared.Only
onCloseis declared. The component will silently receive wrong types or miss required props without any dev-time warning.♻️ Proposed additions
AddSponsorPageTemplatePopup.propTypes = { - onClose: PropTypes.func.isRequired + onClose: PropTypes.func.isRequired, + onSubmit: PropTypes.func.isRequired, + summitId: PropTypes.number.isRequired, + sponsor: PropTypes.shape({ + id: PropTypes.number.isRequired, + sponsorships: PropTypes.arrayOf( + PropTypes.shape({ + id: PropTypes.number.isRequired, + type: PropTypes.shape({ + id: PropTypes.number.isRequired + }).isRequired + }) + ) + }).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 318 - 320, Add missing PropTypes for AddSponsorPageTemplatePopup: declare onSubmit as PropTypes.func.isRequired, sponsor as PropTypes.object.isRequired (or a PropTypes.shape(...) if specific fields are known), and summitId as PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired alongside the existing onClose, so the AddSponsorPageTemplatePopup component validates all required props at dev time.
🤖 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-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Line 65: Replace the hardcoded validation message "Select at least one add-on"
in the Yup schema inside the AddSponsorPageTemplatePopup component with a call
to the i18n helper (T.translate) and use the translation key (e.g.
T.translate('sponsor_pages.select_at_least_one_addon')); locate the validation
rule in
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js
(the Yup validationSchema / the min(1, ... ) message) and swap the literal
string for T.translate(...), then add the corresponding key to the translation
files.
---
Duplicate comments:
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 52-54: sponsor.sponsorships can be null/undefined causing
sponsorshipIds and sponsorshipTypeIds to throw; update the code that builds
sponsorshipIds and sponsorshipTypeIds to guard against a missing array (e.g.,
use optional chaining or default to an empty array when mapping) so both const
sponsorshipIds and const sponsorshipTypeIds operate on a safe array derived from
sponsor.sponsorships.
- Around line 92-114: The handlers handlePageChange and handleSort are passing
the stale Redux prop term instead of the local controlled searchTerm, causing
active searches to be lost when paginating/sorting; update both functions so
they call getShowPages with searchTerm (not term) as the first argument—keep all
other args (page/current, FIVE_PER_PAGE, order/orderDir or key/dir, false,
sponsorshipTypeIds, and DEFAULT_CURRENT_PAGE for handleSort) unchanged so
pagination and sorting respect the current local search state.
- Line 235: The strings "items selected" and "sort by" are hardcoded and must
use the project's translation helper; replace the literal text in the JSX where
Grid2 renders "{selectedPages.length} items selected" and where the sort label
is rendered with calls to T.translate (e.g., T.translate('items_selected', {
count: selectedPages.length }) or the project's pluralization pattern) and
T.translate('sort_by') respectively, ensuring you pass the dynamic count to the
translation function for correct pluralization and match the existing
T.translate usage elsewhere in this component.
---
Nitpick comments:
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 318-320: Add missing PropTypes for AddSponsorPageTemplatePopup:
declare onSubmit as PropTypes.func.isRequired, sponsor as
PropTypes.object.isRequired (or a PropTypes.shape(...) if specific fields are
known), and summitId as PropTypes.oneOfType([PropTypes.string,
PropTypes.number]).isRequired alongside the existing onClose, so the
AddSponsorPageTemplatePopup component validates all required props at dev time.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/i18n/en.jsonsrc/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/i18n/en.json
smarcet
reviewed
Feb 26, 2026
smarcet
reviewed
Feb 26, 2026
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js
Outdated
Show resolved
Hide resolved
Signed-off-by: Tomás Castillo <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js (2)
49-113:⚠️ Potential issue | 🟠 MajorPagination/sort still drops the searched query.
Line 94 and Line 106 use
term, while Line 121 searches with localsearchTerm. After searching, sorting/paging can revert results to a stale filter.🐛 Proposed fix
- const [searchTerm, setSearchTerm] = useState(""); + const [searchTerm, setSearchTerm] = useState(term); const handlePageChange = (page) => { getShowPages( - term, + searchTerm, page, FIVE_PER_PAGE, order, orderDir, false, sponsorshipTypeIds ); }; const handleSort = (key, dir) => { getShowPages( - term, + searchTerm, DEFAULT_CURRENT_PAGE, FIVE_PER_PAGE, key, dir, false, sponsorshipTypeIds ); };Also applies to: 116-128
🤖 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 49 - 113, The pagination/sort calls use the wrong query variable (term) causing searches to be lost; change all getShowPages invocations in useEffect, handlePageChange, handleSort (and any other paging/sorting helpers around those lines) to use the component state searchTerm instead of term, ensure searchTerm is updated via setSearchTerm when the user searches, and add searchTerm to the useEffect dependency array so fetching reacts to search changes; references: searchTerm, setSearchTerm, getShowPages, handlePageChange, handleSort, and the existing useEffect.
235-235:⚠️ Potential issue | 🟡 MinorTwo UI labels are still hardcoded in English.
Line 235 (
items selected) and Line 253 (sort by) should be translated for i18n consistency.🌐 Proposed fix
- <Grid2 size={4}>{selectedPages.length} items selected</Grid2> + <Grid2 size={4}> + {T.translate("edit_sponsor.pages_tab.items_selected", { + count: selectedPages.length + })} + </Grid2> - <SwapVertIcon fontSize="large" sx={{ mr: 1 }} /> sort by + <SwapVertIcon fontSize="large" sx={{ mr: 1 }} /> + {T.translate("general.sort_by")}Also applies to: 253-253
🤖 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` at line 235, Replace the two hardcoded English labels in the AddSponsorPageTemplatePopup component with i18n translation keys: wrap the "items selected" string rendered in the Grid2 that shows {selectedPages.length} and the "sort by" label (the control near line 253) with the project's translation function (e.g., useTranslation().t or t) and use descriptive keys like "sponsorPages.itemsSelected" and "sponsorPages.sortBy" so the UI reads t('sponsorPages.itemsSelected', { count: selectedPages.length }) and t('sponsorPages.sortBy'); update the translation resource files with these keys accordingly.
🤖 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-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 49-113: The pagination/sort calls use the wrong query variable
(term) causing searches to be lost; change all getShowPages invocations in
useEffect, handlePageChange, handleSort (and any other paging/sorting helpers
around those lines) to use the component state searchTerm instead of term,
ensure searchTerm is updated via setSearchTerm when the user searches, and add
searchTerm to the useEffect dependency array so fetching reacts to search
changes; references: searchTerm, setSearchTerm, getShowPages, handlePageChange,
handleSort, and the existing useEffect.
- Line 235: Replace the two hardcoded English labels in the
AddSponsorPageTemplatePopup component with i18n translation keys: wrap the
"items selected" string rendered in the Grid2 that shows {selectedPages.length}
and the "sort by" label (the control near line 253) with the project's
translation function (e.g., useTranslation().t or t) and use descriptive keys
like "sponsorPages.itemsSelected" and "sponsorPages.sortBy" so the UI reads
t('sponsorPages.itemsSelected', { count: selectedPages.length }) and
t('sponsorPages.sortBy'); update the translation resource files with these keys
accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/i18n/en.jsonsrc/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.jssrc/pages/sponsors/sponsor-pages-tab/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/i18n/en.json
smarcet
reviewed
Feb 26, 2026
smarcet
reviewed
Feb 26, 2026
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js
Outdated
Show resolved
Hide resolved
smarcet
reviewed
Feb 26, 2026
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js
Outdated
Show resolved
Hide resolved
smarcet
reviewed
Feb 26, 2026
src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js
Outdated
Show resolved
Hide resolved
smarcet
requested changes
Feb 26, 2026
Signed-off-by: Tomás Castillo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/i18n/en.json`:
- Line 2486: The i18n string "no_pages" currently reads "There is no pages that
match the criteria" and uses incorrect singular verb; update the value for the
"no_pages" key to use plural agreement—e.g., change it to "There are no pages
that match the criteria" (or "There are no pages matching the criteria") so the
copy is grammatically correct.
- Line 6: Update the errors.empty_list message string in en.json to correct the
grammar; replace the current text ("there not are any {item} that match your
criteria.") with a natural sentence such as "There are no {item} that match your
criteria." keeping the {item} placeholder intact and preserving JSON string
formatting and punctuation/capitalization.
In
`@src/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 116-124: The search handler handleOnSearch calls getShowPages with
perPage which is inconsistent with the rest of the popup that uses
FIVE_PER_PAGE; update the getShowPages invocation inside handleOnSearch to pass
FIVE_PER_PAGE (and ensure any other pagination-related calls in this component
consistently use FIVE_PER_PAGE) so search requests and pagination share the same
page size.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/actions/sponsor-pages-actions.jssrc/i18n/en.jsonsrc/pages/sponsors-global/form-templates/add-form-template-item-popup.jssrc/pages/sponsors-global/form-templates/form-template-from-duplicate-popup.jssrc/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.jssrc/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-form-item-from-inventory.jssrc/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js
| "user_not_set": "Hold on. Can not get any valid user.", | ||
| "session_expired": "Hold on. Your session expired!.", | ||
| "empty_list": "there not are any item that match your criteria.", | ||
| "empty_list": "there not are any {item} that match your criteria.", |
There was a problem hiding this comment.
Fix grammar in the errors.empty_list message.
Line [6] currently reads unnaturally: "there not are any {item} that match your criteria.".
✍️ Proposed fix
- "empty_list": "there not are any {item} that match your criteria.",
+ "empty_list": "There are no {item} that match your criteria.",📝 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
| "empty_list": "there not are any {item} that match your criteria.", | |
| "empty_list": "There are no {item} that match your criteria.", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/i18n/en.json` at line 6, Update the errors.empty_list message string in
en.json to correct the grammar; replace the current text ("there not are any
{item} that match your criteria.") with a natural sentence such as "There are no
{item} that match your criteria." keeping the {item} placeholder intact and
preserving JSON string formatting and punctuation/capitalization.
| "unarchive": "Unarchive", | ||
| "items_selected": "items selected", | ||
| "no_add_ons": "No Add-ons Available", | ||
| "no_pages": "There is no pages that match the criteria", |
There was a problem hiding this comment.
Fix grammar in sponsor pages empty-state copy.
Line [2486] should use plural agreement (are + pages) for correct UX copy.
✍️ Proposed fix
- "no_pages": "There is no pages that match the criteria",
+ "no_pages": "There are no pages that match the criteria",📝 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
| "no_pages": "There is no pages that match the criteria", | |
| "no_pages": "There are no pages that match the criteria", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/i18n/en.json` at line 2486, The i18n string "no_pages" currently reads
"There is no pages that match the criteria" and uses incorrect singular verb;
update the value for the "no_pages" key to use plural agreement—e.g., change it
to "There are no pages that match the criteria" (or "There are no pages matching
the criteria") so the copy is grammatically correct.
Comment on lines
+116
to
+124
| const handleOnSearch = (ev) => { | ||
| if (ev.key === "Enter") { | ||
| ev.preventDefault(); | ||
| ev.stopPropagation(); | ||
| getShowPages( | ||
| searchTerm, | ||
| DEFAULT_CURRENT_PAGE, | ||
| perPage, | ||
| order, |
There was a problem hiding this comment.
Use a consistent page size for search requests.
Line [123] uses perPage, while the rest of this popup uses FIVE_PER_PAGE (Lines [84], [96], [108]). That can cause inconsistent result sizing and pagination state.
🔧 Proposed fix
getShowPages(
searchTerm,
DEFAULT_CURRENT_PAGE,
- perPage,
+ FIVE_PER_PAGE,
order,
orderDir,
false,
sponsorshipTypeIds
);📝 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 handleOnSearch = (ev) => { | |
| if (ev.key === "Enter") { | |
| ev.preventDefault(); | |
| ev.stopPropagation(); | |
| getShowPages( | |
| searchTerm, | |
| DEFAULT_CURRENT_PAGE, | |
| perPage, | |
| order, | |
| const handleOnSearch = (ev) => { | |
| if (ev.key === "Enter") { | |
| ev.preventDefault(); | |
| ev.stopPropagation(); | |
| getShowPages( | |
| searchTerm, | |
| DEFAULT_CURRENT_PAGE, | |
| FIVE_PER_PAGE, | |
| order, | |
| orderDir, | |
| 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 116 - 124, The search handler handleOnSearch calls getShowPages
with perPage which is inconsistent with the rest of the popup that uses
FIVE_PER_PAGE; update the getShowPages invocation inside handleOnSearch to pass
FIVE_PER_PAGE (and ensure any other pagination-related calls in this component
consistently use FIVE_PER_PAGE) so search requests and pagination share the same
page size.
Added licensing information and copyright notice to the file. Signed-off-by: Santi <[email protected]>
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
3 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/86b7991c2
Signed-off-by: Tomás Castillo [email protected]
Summary by CodeRabbit
New Features
UI/UX Improvements
Localization