-
Notifications
You must be signed in to change notification settings - Fork 4
Feature/summit sponsor pages #755
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds sponsor-pages feature (actions, reducer, store slice, list page, clone dialogs), PageModules Formik UI with Info/Document/Media subcomponents and tests, multiple Formik/MUI input tweaks, menu/route additions, new constants/translations, refactors media-file-type actions, and updates page-template normalization to transform module fields (MEDIA.upload_deadline → UTC Unix via moment-timezone, MEDIA.file_type_id normalization, DOCUMENT.file → first item/null, remove _tempId). Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Page as SponsorPagesListPage
participant Redux as Redux
participant API as API
participant Store as Store
User->>Page: open sponsor pages list
Page->>Redux: dispatch getSponsorPages(filters)
Redux->>Redux: dispatch startLoading
Redux->>API: GET /sponsor_pages?params
API-->>Redux: return pages response
Redux->>Store: dispatch RECEIVE_SPONSOR_PAGES (normalize)
Store-->>Page: updated props/state
Redux->>Redux: dispatch stopLoading
Page-->>User: render table
sequenceDiagram
participant User as User
participant Popup as GlobalPagePopup
participant PagesDlg as SelectPagesDialog
participant TiersDlg as SelectSponsorshipsDialog
participant Redux as Redux
participant API as API
User->>Popup: open popup
Popup->>PagesDlg: show templates list
User->>PagesDlg: select templates and click Add
PagesDlg-->>Popup: onSave(selectedTemplateIds)
Popup->>TiersDlg: open sponsorships stage
User->>TiersDlg: choose sponsorships and click Save
TiersDlg-->>Popup: onSave(selectedTiers/allSponsors)
Popup->>Redux: dispatch cloneGlobalPage(templateIds, sponsorIds, allSponsors)
Redux->>API: POST /sponsor_pages/clone {payload}
API-->>Redux: success
Redux->>Redux: dispatch GLOBAL_PAGE_CLONED and getSponsorForms
Popup-->>User: close popup and show snackbar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/mui/formik-inputs/mui-formik-datepicker.js (1)
8-8:propsis undefined — this will throw a ReferenceError at runtime.The component destructures only
{ name, label, required }but line 40 spreads{...props}which doesn't exist in scope.Additionally,
marginis not a valid prop on MUI'sDatePickercomponent — it should remain inslotProps.textField.🐛 Proposed fix
-const MuiFormikDatepicker = ({ name, label, required }) => { +const MuiFormikDatepicker = ({ name, label, required, ...props }) => {And move
marginback intoslotProps.textField:textField: { name, label: required ? requiredLabel : label, error: meta.touched && Boolean(meta.error), helperText: meta.touched && meta.error, - fullWidth: true + fullWidth: true, + margin: "normal" },}} - margin="normal" // eslint-disable-next-line react/jsx-props-no-spreading {...props}Also applies to: 38-40
src/components/mui/formik-inputs/mui-formik-select.js (1)
74-79: Missing PropTypes definition forlabelprop.The component accepts a
labelprop (line 16) but it's not declared in PropTypes.Suggested fix
MuiFormikSelect.propTypes = { name: PropTypes.string.isRequired, children: PropTypes.node.isRequired, placeholder: PropTypes.string, - isClearable: PropTypes.bool + isClearable: PropTypes.bool, + label: PropTypes.string };
🤖 Fix all issues with AI agents
In `@src/actions/sponsor-pages-actions.js`:
- Around line 95-133: The cloneGlobalPage action dispatches startLoading() but
never dispatches stopLoading(), leaving the loading state active; update
cloneGlobalPage to dispatch stopLoading() in both the success path (before/after
dispatching getSponsorForms() and snackbarSuccessHandler) and in the
catch/failure path (before handling errors via snackbarErrorHandler) so that
stopLoading() is always called regardless of postRequest resolving or rejecting;
locate the postRequest(...) promise chain in cloneGlobalPage and add
dispatch(stopLoading()) in both the .then and .catch handlers (or use finally)
to guarantee loading is cleared.
- Around line 124-125: The call to getSponsorForms() inside the cloneGlobalPage
promise chain will throw because getSponsorForms is not imported; fix this by
adding an import for getSponsorForms at the top of the file from the module that
exports it (the same module where sponsor form actions live) and keep the
dispatch(getSponsorForms()) call, or if the intent was to refresh sponsor pages
instead, replace that call with the correct exported action (e.g.,
dispatch(getSponsorPages()) )—ensure the chosen function is imported and
referenced (symbols: getSponsorForms, cloneGlobalPage, dispatch).
In `@src/components/mui/formik-inputs/mui-formik-select.js`:
- Around line 34-45: The label can overlap the placeholder because hasValue only
reflects a selected value; update the hasValue logic in the component that
renders InputLabel and Select so it treats a provided placeholder (and
displayEmpty=true) as a value — e.g., compute hasValue = Boolean(field.value) ||
(displayEmpty && Boolean(placeholder)) — then continue using hasValue for
InputLabel's shrink and Select's notched props (symbols: InputLabel, Select,
hasValue, placeholder, displayEmpty, field).
In `@src/components/mui/formik-inputs/mui-formik-upload.js`:
- Line 65: Remove the debug console.log in the UploadFormik component (the line
logging "CHJECK RESPONSE" in mui-formik-upload.js); delete that console.log
statement (or replace it with a proper logger call without the typo if
intentional) so no stray debug output remains in the upload handling code.
- Line 16: Remove the debug console.log statement currently logging "images: "
and field.value inside the MuiFormikUpload component; delete the line
console.log("images: ", field.value) (or replace it with a proper debug-level
logger if persistent logging is required) so no console output remains in
production.
- Around line 93-101: The canAddMore function references mediaType.is_editable
but mediaType lacks that property; update the mediaType object used by this
component (the mediaType declaration near the top of mui-formik-upload.js) to
include is_editable: true or derive it from props, so canAddMore correctly
evaluates editable media types; ensure changes touch the mediaType definition
(not canAddMore) and keep the singleFile and field.value logic intact.
In `@src/layouts/sponsor-layout.js`:
- Line 28: The import for the React component is using camelCase; change the
import identifier from sponsorPagesListPage to PascalCase SponsorPagesListPage
and update all places where sponsorPagesListPage is referenced (e.g., the
component usage currently referencing it) to use SponsorPagesListPage so the
import and usage match the React component naming convention.
In `@src/pages/sponsors-global/page-templates/page-template-modules-form.js`:
- Around line 182-193: InputLabel's htmlFor is incorrectly targeting
buildFieldName("max_file_size") while the actual form control is
MuiFormikTextField with name buildFieldName("description"); update the
InputLabel so htmlFor uses buildFieldName("description") to match the
MuiFormikTextField name and restore proper accessibility (locate the InputLabel
and adjust the buildFieldName argument from "max_file_size" to "description").
- Around line 92-98: The MuiFormikUpload instance inside the
DocumentDownloadModule uses a hardcoded id ("document-module-upload") which will
duplicate across multiple modules; change the id to be unique per module by
deriving it from the module instance (e.g., include the module index, module id
prop, or reuse buildFieldName("file") as part of the id) so each MuiFormikUpload
has a distinct id attribute; update the id prop on the MuiFormikUpload in
page-template-modules-form.js accordingly and ensure any label/for references
use the same generated id.
- Around line 167-178: The InputLabel uses the wrong htmlFor value; change
htmlFor in the InputLabel surrounding the MuiFormikSelect to use
buildFieldName("file_type_id") instead of buildFieldName("max_file_size") so the
label correctly targets the select; update the InputLabel that wraps the
MuiFormikSelect (referencing buildFieldName and the file_type_id field name) to
restore proper label-input association for accessibility.
In
`@src/pages/sponsors/sponsor-pages-list-page/components/global-page/global-page-popup.js`:
- Around line 31-43: The Dialog currently passes the external onClose prop
directly which prevents resetting internal state; change the Dialog's onClose to
call the local handleClose function (the handler that resets stage and
selectedTemplates) instead of the external onClose, so backdrop clicks/Escape
trigger resetting internal state; keep using the external onClose (prop named
onClose) inside handleClose if it needs to notify parents after internal reset.
In `@src/pages/sponsors/sponsor-pages-list-page/index.js`:
- Around line 143-154: The Checkbox is uncontrolled because it lacks a checked
prop tied to the Redux state; update the Checkbox in the FormControlLabel to
pass checked={hideArchived} (ensure hideArchived is derived from Redux state in
this component) so the visual state matches the filter on initial render, and
keep using handleHideArchivedForms to dispatch/toggle the hideArchived value; if
hideArchived can be undefined ensure it is coerced to a boolean (e.g.,
!!hideArchived) before passing to checked.
- Around line 31-42: The component SponsorPagesListPage destructures
getSponsorForm but it isn’t wired to Redux; either add getSponsorForm to the
connect(...) mapDispatchToProps alongside getSponsorPages so the prop is
provided, or remove getSponsorForm from SponsorPagesListPage’s parameter list
and any usages (e.g., handleRowEdit) if the action isn’t implemented; ensure any
call sites like handleRowEdit reference the correctly connected action name.
In `@src/reducers/sponsors/sponsor-pages-list-reducer.js`:
- Around line 61-70: The reducer currently builds sponsorPages with module
counts using hardcoded kinds ("Upload"/"Download") causing
upload_mod/download_mod to be zero; update the modules.filter calls in the
sponsorPages mapping to use the PAGES_MODULE_KINDS constants (e.g.,
PAGES_MODULE_KINDS.DOCUMENT and PAGES_MODULE_KINDS.MEDIA) when computing those
counts, rename the fields from upload_mod/download_mod to document_mod/media_mod
to match the API constants, and propagate those new field names to the
corresponding table column and UI references so they consume document_mod and
media_mod instead of the old names.
🧹 Nitpick comments (15)
src/components/mui/formik-inputs/mui-formik-radio-group.js (1)
13-19: LGTM! Clean addition of configurable margin prop.The implementation correctly adds an optional
marginprop with a sensible default, preserving backward compatibility while enabling layout customization. This aligns with the PR-wide pattern of exposing more customization options on Formik inputs.Optional: Consider using
PropTypes.oneOfto restrict values to MUI's valid options:- margin: PropTypes.string, + margin: PropTypes.oneOf(["none", "dense", "normal"]),Also applies to: 25-25, 65-65
src/actions/page-template-actions.js (1)
145-160: Consider adding null-safety for the modules array.The
normalizeEntityfunction assumesentity.modulesis always a valid array. Ifentity.modulesisnullorundefined, the.map()call will throw a runtime error.Suggested defensive check
const normalizeEntity = (entity) => { const normalizedEntity = { ...entity }; - normalizedEntity.modules = entity.modules.map((module) => { + normalizedEntity.modules = (entity.modules || []).map((module) => { const normalizedModule = { ...module }; if (module.kind === PAGES_MODULE_KINDS.MEDIA && module.upload_deadline) { normalizedModule.upload_deadline = moment(module.upload_deadline).unix(); } delete normalizedModule._tempId; return normalizedModule; }); return normalizedEntity; };src/pages/sponsors/sponsor-pages-list-page/components/global-page/select-pages-dialog.js (3)
140-143: Hardcoded text should use i18n translation.The text "items selected" is hardcoded in English. For consistency with the rest of the component, this should use
T.translate().Suggested fix
<Grid2 size={4} sx={{ mt: 1 }}> - {selectedRows.length} items selected + {T.translate("sponsor_pages.global_page_popup.items_selected", { count: selectedRows.length })} </Grid2>
180-183: Consider adding PropTypes for Redux-injected props.The component receives several props from Redux (
pageTemplates,items,currentPage,term,order,orderDir,total,getPageTemplates) but onlyonCloseandonSaveare declared in PropTypes. Adding PropTypes for all props improves documentation and development-time validation.
38-40: AddgetPageTemplatesto useEffect dependency array.The effect references
getPageTemplatesbut the dependency array is empty. While this works because the function reference is stable (from Redux), adding it satisfies the exhaustive-deps rule and makes the intent clearer.Suggested fix
useEffect(() => { getPageTemplates("", 1, DEFAULT_PER_PAGE, "id", 1, true); - }, []); + }, [getPageTemplates]);src/components/mui/formik-inputs/mui-formik-upload.js (1)
125-128: Missingidin PropTypes.The
idprop is used (line 109) but not declared in PropTypes.Suggested fix
MuiFormikUpload.propTypes = { + id: PropTypes.string, name: PropTypes.string.isRequired, singleFile: PropTypes.bool };src/actions/sponsor-pages-actions.js (1)
133-133: Silent catch block may hide errors.The empty catch block swallows all errors silently. While the comment explains the intent to catch promise rejections, consider at minimum logging the error for debugging purposes.
Suggested fix
- .catch(() => {}); // need to catch promise reject + .catch((err) => { + console.error("cloneGlobalPage failed:", err); + });src/pages/sponsors/sponsor-pages-list-page/components/global-page/global-page-popup.js (1)
46-49: Consider addingcloneGlobalPageto PropTypes.The
cloneGlobalPageprop is injected byconnectbut not declared in PropTypes. While optional, documenting injected props improves maintainability.Proposed fix
GlobalPagePopup.propTypes = { open: PropTypes.bool.isRequired, - onClose: PropTypes.func.isRequired + onClose: PropTypes.func.isRequired, + cloneGlobalPage: PropTypes.func.isRequired };src/pages/sponsors-global/page-templates/page-template-popup.js (1)
160-162: Remove unnecessaryformikprop from PageModules.The
PageModulescomponent usesuseFormikContext()internally to access formik state (as seen in the relevant code snippet). Theformikprop passed here is unused.Proposed fix
<Box sx={{ py: 2 }}> - <PageModules formik={formik} /> + <PageModules /> </Box>src/pages/sponsors-global/page-templates/page-template-module-form.test.js (2)
64-64: Minor: Comment in Spanish should be in English for consistency.The comment "Mock DragAndDropList que captura onReorder" should be in English to maintain consistency with the rest of the codebase.
Suggested fix
-// Mock DragAndDropList que captura onReorder +// Mock DragAndDropList that captures onReorder
303-316: Consider: Tests rely on MUI implementation details.Tests use
.closest(".MuiAccordionSummary-root")which couples them to MUI's class naming. This could break if MUI changes its class names in a future version. Consider addingdata-testidattributes to theAccordionSummarycomponents in the actual implementation for more resilient testing.src/pages/sponsors/sponsor-pages-list-page/index.js (3)
45-47: Missing dependency inuseEffect.The
useEffecthook usesgetSponsorPagesbut doesn't include it in the dependency array. While this works becausegetSponsorPagesis a stable reference from Redux, ESLint'sreact-hooks/exhaustive-depsrule would flag this.Suggested fix
useEffect(() => { getSponsorPages(); - }, []); + }, [getSponsorPages]);
63-71: Stubbed handlers with console.log statements.
handleRowDeleteandhandleArchiveItemcontainconsole.logstatements and commented-out code. These appear to be work-in-progress placeholders. Consider adding TODO comments or removing the console.log before merging.Suggested change
const handleRowDelete = (itemId) => { - console.log("DELETE ITEM ID...", itemId); - // deleteSponsorForm(itemId); + // TODO: Implement delete functionality + // deleteSponsorPage(itemId); }; - const handleArchiveItem = (item) => console.log("archive ITEM...", item); - // item.is_archived - // ? unarchiveSponsorForm(item.id) - // : archiveSponsorForm(item.id); + const handleArchiveItem = (item) => { + // TODO: Implement archive functionality + // item.is_archived ? unarchiveSponsorPage(item.id) : archiveSponsorPage(item.id); + };
205-212: Commented code for FormPagePopup.The
FormPagePopupcomponent is commented out, indicating incomplete functionality. Consider adding a TODO comment to track this work item.src/i18n/en.json (1)
2644-2658: Minor: Inconsistent casing in translation value.Line 2648 has
"name": "name"(lowercase) while the corresponding column header on line 2632 has"name_column_label": "Name"(capitalized). The popup'snamefield label should likely be capitalized for consistency.Suggested fix
"global_page_popup": { "title": "Add Page Template", "items_selected": "items selected", "code": "Code", - "name": "name", + "name": "Name", "info_mod": "Info Mod",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/actions/page-template-actions.jssrc/actions/sponsor-pages-actions.jssrc/components/menu/index.jssrc/components/mui/formik-inputs/mui-formik-datepicker.jssrc/components/mui/formik-inputs/mui-formik-radio-group.jssrc/components/mui/formik-inputs/mui-formik-select.jssrc/components/mui/formik-inputs/mui-formik-upload.jssrc/i18n/en.jsonsrc/layouts/sponsor-layout.jssrc/pages/sponsors-global/page-templates/page-template-module-form.test.jssrc/pages/sponsors-global/page-templates/page-template-modules-form.jssrc/pages/sponsors-global/page-templates/page-template-popup.jssrc/pages/sponsors/sponsor-pages-list-page/components/global-page/global-page-popup.jssrc/pages/sponsors/sponsor-pages-list-page/components/global-page/select-pages-dialog.jssrc/pages/sponsors/sponsor-pages-list-page/index.jssrc/reducers/sponsors/sponsor-pages-list-reducer.jssrc/store.jssrc/utils/constants.js
🧰 Additional context used
🧬 Code graph analysis (8)
src/store.js (1)
src/reducers/sponsors/sponsor-pages-list-reducer.js (1)
sponsorPagesListReducer(33-83)
src/components/menu/index.js (1)
src/reducers/notes/notes-reducer.js (1)
summitId(48-48)
src/pages/sponsors-global/page-templates/page-template-popup.js (2)
src/pages/sponsors-global/page-templates/page-template-modules-form.js (2)
modules(200-200)PageModules(198-343)src/utils/constants.js (4)
PAGES_MODULE_KINDS(245-249)PAGES_MODULE_KINDS(245-249)PAGE_MODULES_MEDIA_TYPES(251-254)PAGE_MODULES_MEDIA_TYPES(251-254)
src/components/mui/formik-inputs/mui-formik-select.js (6)
src/components/mui/formik-inputs/mui-formik-datepicker.js (1)
field(9-9)src/components/mui/formik-inputs/mui-formik-upload.js (1)
field(14-14)src/components/mui/formik-inputs/mui-formik-textfield.js (1)
field(7-7)src/components/mui/formik-inputs/sponsorship-input-mui.js (2)
field(31-31)field(37-37)src/components/mui/formik-inputs/mui-formik-checkbox.js (1)
field(12-12)src/components/mui/formik-inputs/company-input-mui.js (2)
field(32-32)field(38-38)
src/actions/page-template-actions.js (1)
src/utils/constants.js (2)
PAGES_MODULE_KINDS(245-249)PAGES_MODULE_KINDS(245-249)
src/components/mui/formik-inputs/mui-formik-upload.js (1)
src/utils/constants.js (6)
MAX_INVENTORY_IMAGE_UPLOAD_SIZE(190-190)MAX_INVENTORY_IMAGE_UPLOAD_SIZE(190-190)MAX_INVENTORY_IMAGES_UPLOAD_QTY(191-191)MAX_INVENTORY_IMAGES_UPLOAD_QTY(191-191)ALLOWED_INVENTORY_IMAGE_FORMATS(192-192)ALLOWED_INVENTORY_IMAGE_FORMATS(192-192)
src/pages/sponsors-global/page-templates/page-template-modules-form.js (3)
src/utils/constants.js (4)
PAGE_MODULES_MEDIA_TYPES(251-254)PAGE_MODULES_MEDIA_TYPES(251-254)PAGES_MODULE_KINDS(245-249)PAGES_MODULE_KINDS(245-249)src/components/mui/showConfirmDialog.js (1)
showConfirmDialog(5-42)src/components/mui/dnd-list.js (1)
DragAndDropList(16-72)
src/reducers/sponsors/sponsor-pages-list-reducer.js (1)
src/actions/sponsor-pages-actions.js (4)
REQUEST_SPONSOR_PAGES(31-31)REQUEST_SPONSOR_PAGES(31-31)RECEIVE_SPONSOR_PAGES(32-32)RECEIVE_SPONSOR_PAGES(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (22)
src/components/mui/formik-inputs/mui-formik-select.js (2)
1-12: LGTM!The new imports for
IconButtonandInputLabelare appropriate for the added label and clearable functionality.
29-30: LGTM!The
hasValueguard correctly checks for non-empty values, handling empty string,undefined, andnullcases appropriately.src/utils/constants.js (1)
244-254: LGTM!The new constants are well-organized and follow the existing patterns in this file.
PAGES_MODULE_KINDSandPAGE_MODULES_MEDIA_TYPESprovide clear, centralized definitions for module kinds and media types used throughout the sponsor pages feature.src/actions/page-template-actions.js (1)
14-33: LGTM!The new imports for
moment-timezoneandPAGES_MODULE_KINDSare correctly placed and support the enhanced module normalization logic.src/actions/sponsor-pages-actions.js (1)
68-72: Verify the filter logic for sponsorship types.The filter uses
sponsorship_type_id_not_inwhich appears to exclude rather than include the specified sponsorship types. Confirm this is the intended behavior for the use case.src/components/mui/formik-inputs/mui-formik-upload.js (1)
76-91: Verify the property name of the imageFile parameter passed by UploadInputV2's onRemove callback.The filter on line 84 compares
i.filename !== imageFile.name, but without access to the external UploadInputV2 library (fromopenstack-uicore-foundation), it cannot be definitively confirmed whetherimageFile.nameis correct. Cross-checking with similar code in inventory-item-form.js and form-template-form.js shows identical patterns, but this could indicate either consistent behavior or a systemic issue. Confirm the property name that UploadInputV2 passes to the onRemove callback—if it's notname, the filter will fail silently and files won't be removed from the array.src/reducers/sponsors/sponsor-pages-list-reducer.js (2)
21-31: LGTM!The DEFAULT_STATE structure is well-defined with appropriate defaults for pagination, sorting, and filtering.
36-52: LGTM!Proper handling of state reset on summit change/logout and request state management with cleared data during loading.
src/store.js (2)
170-170: LGTM!Import correctly added for the new sponsor pages list reducer.
254-254: LGTM!New state slice properly registered following the existing naming conventions and placed logically with other sponsor-related state.
src/pages/sponsors/sponsor-pages-list-page/components/global-page/global-page-popup.js (1)
25-29: LGTM!Good use of
.finally()to ensure cleanup regardless of success or failure.src/layouts/sponsor-layout.js (1)
67-87: LGTM!The route structure correctly follows the existing pattern with proper breadcrumb integration and nested Switch/Route setup.
src/pages/sponsors-global/page-templates/page-template-popup.js (3)
33-41: LGTM!Good extraction of common module creation logic into
addModulehelper. The temporary ID and order assignment pattern is clean.
43-70: LGTM!Module handlers properly initialize all required fields for each module type using the extracted
addModulehelper and imported constants.
72-89: LGTM!Good approach to normalize module ordering on submit by mapping indices to
custom_order, ensuring consistent ordering regardless of user reordering during editing.src/components/menu/index.js (1)
247-251: LGTM!The new
sponsor_pagesmenu item follows the established pattern for other sponsor-related menu items, with appropriate routing and access control viaadmin-sponsors.src/pages/sponsors-global/page-templates/page-template-module-form.test.js (1)
83-116: LGTM!The test helpers
renderWithFormikandcreateModuleare well-designed and provide good isolation for testing the PageModules component. ThebeforeEachblock properly resets state between tests.src/pages/sponsors-global/page-templates/page-template-modules-form.js (3)
240-243: Consider: Module ordering may have gaps after deletion.After deleting a module, the
custom_ordervalues of remaining modules are not recalculated. For example, if modules have orders [0, 1, 2] and module at index 1 is deleted, the remaining modules will have orders [0, 2]. This may or may not be intentional depending on how the backend handles ordering.Verify if the backend expects contiguous
custom_ordervalues or if gaps are acceptable. If contiguous values are required, consider recalculating orders after deletion:if (isConfirmed) { const updated = modules .filter((_, i) => i !== index) .map((m, i) => ({ ...m, custom_order: i })); setFieldValue(name, updated); }
198-211: LGTM!The auto-scroll functionality using refs and
useEffectis well-implemented. It correctly detects when a new module is added and scrolls to make it visible.
319-342: LGTM!The main render logic correctly handles the empty state, renders the DragAndDropList with appropriate configuration, and includes the scroll target element.
src/i18n/en.json (2)
2626-2643: LGTM!The new
sponsor_pagestranslation object is well-structured with appropriate keys for the list page, column labels, filters, and empty state message.
3950-3964: LGTM!The new module-related translation keys under
page_template_list.page_crudare comprehensive and cover all the module types (Info, Document, Media) and their fields.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <Grid2 size={12}> | ||
| <InputLabel htmlFor={buildFieldName("max_file_size")}> | ||
| {T.translate("page_template_list.page_crud.description")} | ||
| </InputLabel> | ||
| <MuiFormikTextField | ||
| name={buildFieldName("description")} | ||
| fullWidth | ||
| multiline | ||
| margin="none" | ||
| rows={2} | ||
| /> | ||
| </Grid2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
htmlFor attribute points to wrong field name.
The InputLabel on line 183 has htmlFor={buildFieldName("max_file_size")} but the associated field is description. This is another copy-paste error affecting accessibility.
Suggested fix
<Grid2 size={12}>
- <InputLabel htmlFor={buildFieldName("max_file_size")}>
+ <InputLabel htmlFor={buildFieldName("description")}>
{T.translate("page_template_list.page_crud.description")}
</InputLabel>
<MuiFormikTextField
name={buildFieldName("description")}
fullWidth
multiline
margin="none"
rows={2}
/>
</Grid2>📝 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.
| <Grid2 size={12}> | |
| <InputLabel htmlFor={buildFieldName("max_file_size")}> | |
| {T.translate("page_template_list.page_crud.description")} | |
| </InputLabel> | |
| <MuiFormikTextField | |
| name={buildFieldName("description")} | |
| fullWidth | |
| multiline | |
| margin="none" | |
| rows={2} | |
| /> | |
| </Grid2> | |
| <Grid2 size={12}> | |
| <InputLabel htmlFor={buildFieldName("description")}> | |
| {T.translate("page_template_list.page_crud.description")} | |
| </InputLabel> | |
| <MuiFormikTextField | |
| name={buildFieldName("description")} | |
| fullWidth | |
| multiline | |
| margin="none" | |
| rows={2} | |
| /> | |
| </Grid2> |
🤖 Prompt for AI Agents
In `@src/pages/sponsors-global/page-templates/page-template-modules-form.js`
around lines 182 - 193, InputLabel's htmlFor is incorrectly targeting
buildFieldName("max_file_size") while the actual form control is
MuiFormikTextField with name buildFieldName("description"); update the
InputLabel so htmlFor uses buildFieldName("description") to match the
MuiFormikTextField name and restore proper accessibility (locate the InputLabel
and adjust the buildFieldName argument from "max_file_size" to "description").
src/pages/sponsors/sponsor-pages-list-page/components/global-page/global-page-popup.js
Show resolved
Hide resolved
| const sponsorPages = payload.response.data.map((a) => ({ | ||
| id: a.id, | ||
| code: a.code, | ||
| name: a.name, | ||
| tier: a.sponsorship_types.map((s) => s.name).join(", "), | ||
| info_mod: a.modules.filter((m) => m.kind === "Info").length, | ||
| upload_mod: a.modules.filter((m) => m.kind === "Upload").length, | ||
| download_mod: a.modules.filter((m) => m.kind === "Download").length, | ||
| is_archived: a.is_archived | ||
| })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the PAGES_MODULE_KINDS constant definition
rg -n "PAGES_MODULE_KINDS" --type=js -A 5 -B 2Repository: fntechgit/summit-admin
Length of output: 26166
🏁 Script executed:
#!/bin/bash
# Search for module kind string values used in filtering/comparisons
rg -n "kind\s*[=!:]\s*['\"]" --type=js | head -50Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look for module kind references in API responses or types
rg -n "\"kind\":|'kind':" --type=js -A 2 | head -100Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check the sponsor pages file structure for context
fd "sponsor" --type=f -e js | head -20Repository: fntechgit/summit-admin
Length of output: 1011
🏁 Script executed:
#!/bin/bash
# Check the actual sponsor pages list reducer file
cat -n src/reducers/sponsors/sponsor-pages-list-reducer.js | sed -n '55,75p'Repository: fntechgit/summit-admin
Length of output: 865
🏁 Script executed:
#!/bin/bash
# Also check what the property names should be and any usages of these properties
rg -n "upload_mod|download_mod|document_mod|media_mod" --type=jsRepository: fntechgit/summit-admin
Length of output: 2388
🏁 Script executed:
#!/bin/bash
# Look for API response examples or test data
rg -n "kind.*:.*['\"]" src/pages/sponsors-global/page-templates/page-template-module-form.test.js -A 2 | head -30Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Search for sponsor pages API examples in actions or tests
fd "sponsor-pages" --type=f -e js | xargs rg -l "modules"Repository: fntechgit/summit-admin
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Check page-template-list-reducer for comparison
cat -n src/reducers/sponsors_inventory/page-template-list-reducer.js | sed -n '70,85p'Repository: fntechgit/summit-admin
Length of output: 728
🏁 Script executed:
#!/bin/bash
# Check if there are any test fixtures or mock data for sponsor pages
fd "sponsor-pages" --type=f -e test.js | xargs cat | head -200Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look for API response mock data in sponsor pages actions
rg -n "modules.*\[" src/actions/sponsor-pages-actions.js -A 5 | head -50Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if "Upload" and "Download" strings appear anywhere else as module kinds
rg -n '"Upload"|"Download"' --type=js | grep -i moduleRepository: fntechgit/summit-admin
Length of output: 605
🏁 Script executed:
#!/bin/bash
# Check the UI components that use these properties to understand intent
rg -n "upload_mod|download_mod" src/pages/sponsors/sponsor-pages-list-page/index.js -B 2 -A 2Repository: fntechgit/summit-admin
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Check what the actual API might return - look at sponsor-pages-actions
cat src/actions/sponsor-pages-actions.js | head -100Repository: fntechgit/summit-admin
Length of output: 3182
🏁 Script executed:
#!/bin/bash
# Look for API response examples, mocks, or fixtures
fd . --type=f -e json | xargs rg -l "modules" | head -10Repository: fntechgit/summit-admin
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Search for test data or fixtures related to sponsor pages
find . -path ./node_modules -prune -o -type f \( -name "*.test.js" -o -name "*.spec.js" -o -name "*fixture*" -o -name "*mock*" \) -print | xargs rg -l "sponsor.*page" 2>/dev/null | head -10Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about module kinds
rg -n "Info|Upload|Download" src/actions/sponsor-pages-actions.js -B 2 -A 2Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if the API endpoint documentation or swagger is available
fd . --type=f \( -name "swagger*" -o -name "openapi*" -o -name "*.yaml" -o -name "*.yml" \) 2>/dev/null | head -10Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check i18n for module kind labels
rg -n "upload_mod|download_mod|document_module|media_module|Info|Upload|Download" src/i18n/en.json | head -30Repository: fntechgit/summit-admin
Length of output: 1625
🏁 Script executed:
#!/bin/bash
# Check if there's backend API documentation or type definitions
fd . --type=f -e ts | xargs rg -l "modules|kind" 2>/dev/null | head -10Repository: fntechgit/summit-admin
Length of output: 505
🏁 Script executed:
#!/bin/bash
# Look at the complete page-template-list-reducer file structure for comparison
cat -n src/reducers/sponsors_inventory/page-template-list-reducer.js | head -100Repository: fntechgit/summit-admin
Length of output: 3581
🏁 Script executed:
#!/bin/bash
# Verify what properties are accessed from sponsorPages in the UI
rg -n "sponsorPages" src/pages/sponsors/sponsor-pages-list-page/index.js -A 3 -B 3 | head -50Repository: fntechgit/summit-admin
Length of output: 863
🏁 Script executed:
#!/bin/bash
# Check if there are any actual API examples with modules data structure
rg -n "modules.*\[" --type=js -A 10 | grep -A 10 "kind"Repository: fntechgit/summit-admin
Length of output: 4340
Fix module kind filtering to use constants instead of hardcoded strings.
The reducer filters modules using hardcoded strings "Upload" and "Download", but the PAGES_MODULE_KINDS constant defines the actual API values as "Document" and "Media". This mismatch causes upload_mod and download_mod to always be 0.
Update to use the constants for consistency and correctness:
Suggested fix
+import { PAGES_MODULE_KINDS } from "../../utils/constants";
...
- info_mod: a.modules.filter((m) => m.kind === "Info").length,
- upload_mod: a.modules.filter((m) => m.kind === "Upload").length,
- download_mod: a.modules.filter((m) => m.kind === "Download").length,
+ info_mod: a.modules.filter((m) => m.kind === PAGES_MODULE_KINDS.INFO).length,
+ document_mod: a.modules.filter((m) => m.kind === PAGES_MODULE_KINDS.DOCUMENT).length,
+ media_mod: a.modules.filter((m) => m.kind === PAGES_MODULE_KINDS.MEDIA).length,Note: Also update the corresponding table columns and UI references to use document_mod and media_mod instead of upload_mod and download_mod.
📝 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 sponsorPages = payload.response.data.map((a) => ({ | |
| id: a.id, | |
| code: a.code, | |
| name: a.name, | |
| tier: a.sponsorship_types.map((s) => s.name).join(", "), | |
| info_mod: a.modules.filter((m) => m.kind === "Info").length, | |
| upload_mod: a.modules.filter((m) => m.kind === "Upload").length, | |
| download_mod: a.modules.filter((m) => m.kind === "Download").length, | |
| is_archived: a.is_archived | |
| })); | |
| const sponsorPages = payload.response.data.map((a) => ({ | |
| id: a.id, | |
| code: a.code, | |
| name: a.name, | |
| tier: a.sponsorship_types.map((s) => s.name).join(", "), | |
| info_mod: a.modules.filter((m) => m.kind === PAGES_MODULE_KINDS.INFO).length, | |
| document_mod: a.modules.filter((m) => m.kind === PAGES_MODULE_KINDS.DOCUMENT).length, | |
| media_mod: a.modules.filter((m) => m.kind === PAGES_MODULE_KINDS.MEDIA).length, | |
| is_archived: a.is_archived | |
| })); |
🤖 Prompt for AI Agents
In `@src/reducers/sponsors/sponsor-pages-list-reducer.js` around lines 61 - 70,
The reducer currently builds sponsorPages with module counts using hardcoded
kinds ("Upload"/"Download") causing upload_mod/download_mod to be zero; update
the modules.filter calls in the sponsorPages mapping to use the
PAGES_MODULE_KINDS constants (e.g., PAGES_MODULE_KINDS.DOCUMENT and
PAGES_MODULE_KINDS.MEDIA) when computing those counts, rename the fields from
upload_mod/download_mod to document_mod/media_mod to match the API constants,
and propagate those new field names to the corresponding table column and UI
references so they consume document_mod and media_mod instead of the old names.
| ); | ||
| }) | ||
| .catch(() => {}); // need to catch promise reject | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing finally stopLoading
src/actions/page-template-actions.js
Outdated
| const normalizedModule = { ...module }; | ||
|
|
||
| if (module.kind === PAGES_MODULE_KINDS.MEDIA && module.upload_deadline) { | ||
| normalizedModule.upload_deadline = moment(module.upload_deadline).unix(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems wrong, moment will parse the date in client's timezone
| const MuiFormikRadioGroup = ({ | ||
| name, | ||
| label, | ||
| margin = "normal", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to add margin if we are spreading props
| }; | ||
|
|
||
| const hasValue = | ||
| field.value !== "" && field.value !== undefined && field.value !== null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
field?.value && field.value !== ""
| const MuiFormikUpload = ({ id, name, onImageDeleted, singleFile = false }) => { | ||
| const [field, meta, helpers] = useField(name); | ||
|
|
||
| console.log("images: ", field.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove console log
| filename: response.name | ||
| }; | ||
| helpers.setValue([...field.value, image]); | ||
| console.log("CHJECK RESPONSE", response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove log
| console.log("CHJECK RESPONSE", response); | ||
| const image = buildFileObject(response); | ||
| if (singleFile) { | ||
| helpers.setValue(image); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't we always return an array ? is more consistent
src/layouts/sponsor-layout.js
Outdated
| pathname: props.match.url | ||
| }} | ||
| /> | ||
| <Switch> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a switch ? are we expecting more pages under /pages ?
| import FormikTextEditor from "../../../components/inputs/formik-text-editor"; | ||
| import MuiFormikUpload from "../../../components/mui/formik-inputs/mui-formik-upload"; | ||
|
|
||
| const InfoModule = ({ baseName, index }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split modules into files please, lets try to keep files not too long
| code: a.code, | ||
| name: a.name, | ||
| tier: a.sponsorship_types.map((s) => s.name).join(", "), | ||
| info_mod: a.modules.filter((m) => m.kind === "Info").length, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use KINDS constant
| </Grid2> | ||
| <Grid2 size={12}> | ||
| <MuiFormikUpload | ||
| id="document-module-upload" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this id should be unique dupe id will lead to issues on dom render
id={document-module-upload-${index}}
|
@tomrndom please review comments and do a general second round testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/formik-inputs/mui-formik-select.js (1)
74-79: Missinglabelprop in PropTypes.The
labelprop is destructured and used in the component (line 16) but is not declared in PropTypes. This will cause PropTypes validation warnings in development and reduces component self-documentation.Proposed fix
MuiFormikSelect.propTypes = { name: PropTypes.string.isRequired, children: PropTypes.node.isRequired, + label: PropTypes.string, placeholder: PropTypes.string, isClearable: PropTypes.bool };
🤖 Fix all issues with AI agents
In `@src/actions/sponsor-pages-actions.js`:
- Around line 85-93: The current getRequest(...) call in
sponsor-pages-actions.js (using createAction(REQUEST_SPONSOR_PAGES),
createAction(RECEIVE_SPONSOR_PAGES), authErrorHandler, etc.) only dispatches
stopLoading() in the .then path so a rejected request leaves the spinner
running; update the promise handling on the getRequest(...) invocation to catch
rejections and ensure stopLoading() is always dispatched (use .catch(...) to
handle errors and/or .finally(...) to call dispatch(stopLoading())), preserving
existing error handling via authErrorHandler and the original action creators.
In `@src/components/mui/formik-inputs/mui-formik-datepicker.js`:
- Line 22: The DatePicker's margin prop is currently set at the component level
and is ignored; update the mui DatePicker usage in mui-formik-datepicker (the
DatePicker component instance) to move margin="normal" into slotProps.textField
(e.g., slotProps={{ textField: { margin: "normal", ... } }}) so the TextField
receives the margin; ensure any existing slotProps.textField merging preserves
other props.
In `@src/components/mui/formik-inputs/mui-formik-upload.js`:
- Around line 57-65: The removal logic in handleRemove is comparing i.filename
to imageFile.name but uploaded file objects use file_name (created by
buildFileObject), so newly uploaded files never match; update handleRemove to
compare using the correct properties (e.g., compare (i.file_name || i.filename)
to (imageFile.name || imageFile.file_name)) and/or also fall back to matching on
id when available (i.id === imageFile.id) before calling helpers.setValue and
onImageDeleted to ensure both existing and newly uploaded files can be removed.
- Around line 37-52: The buildFileObject function currently maps response.name
to file.file_name but doesn't set file.filename, causing newly uploaded files to
miss the key expected by getInputValue and removal/display logic; update
buildFileObject to also set file.filename (e.g., from response.name ||
response.file_name) so handleUploadComplete will push objects compatible with
getInputValue and existing removal logic.
In
`@src/pages/sponsors-global/page-templates/modules/page-template-document-download-module.js`:
- Around line 48-52: The MuiFormikUpload usage is passing an invalid prop
singleFile; replace it with maxFiles={1} to restrict uploads to a single file.
Locate the MuiFormikUpload component instance
(id={`document-module-upload-${index}`} name={buildFieldName("file")}) and
remove the singleFile prop, adding maxFiles={1} instead so it matches other
usages of MuiFormikUpload across the codebase.
In
`@src/pages/sponsors-global/page-templates/modules/page-template-info-module.js`:
- Around line 18-22: FormikTextEditor currently reads form state with direct
property access (value={values[name]}, touched?.[name], errors?.[name]) which
breaks for nested field paths like "modules[0].content"; update FormikTextEditor
usage to import and use Formik's getIn utility and replace reads with
getIn(values, name) for value and use getIn(touched, name) and getIn(errors,
name) when computing the error string (e.g., error = getIn(touched, name) &&
getIn(errors, name) ? getIn(errors, name) : ""); keep existing setFieldValue and
setFieldTouched calls as they already support nested paths.
In
`@src/pages/sponsors-global/page-templates/modules/page-template-media-request-module.js`:
- Around line 76-87: The InputLabel's htmlFor is incorrectly set to
buildFieldName("max_file_size") rather than the associated select's name; update
the htmlFor to buildFieldName("file_type_id") so the InputLabel correctly
targets the MuiFormikSelect with name buildFieldName("file_type_id") (look for
the InputLabel and the MuiFormikSelect components in
page-template-media-request-module.js).
In `@src/pages/sponsors-global/page-templates/page-template-modules-form.js`:
- Around line 129-134: Add an accessible label to the icon-only delete button:
update the IconButton element that calls handleDeleteModule (the one wrapping
<DeleteIcon />) to include an aria-label using the existing translation key
'general.delete' (e.g., via t('general.delete')) so screen readers can announce
the button purpose while keeping the onClick behavior intact.
♻️ Duplicate comments (1)
src/reducers/sponsors/sponsor-pages-list-reducer.js (1)
67-73: Variable names still don't align with constant names.The past review suggested renaming
upload_modtomedia_modanddownload_modtodocument_modto match thePAGES_MODULE_KINDSconstants being used. While the hardcoded strings have been replaced with constants (good!), the variable naming inconsistency remains.
🧹 Nitpick comments (5)
src/layouts/sponsor-layout.js (1)
67-85: Consider wrapping inner Route in Switch for consistency.The route structure is functional, but unlike the
/formsroute (lines 50-63) which wraps nested routes in a<Switch>, this uses a bare<Route>. If nested routes are planned (e.g.,/:page_idfor editing individual pages), adding a<Switch>now would maintain consistency and simplify future additions.Optional: Add Switch wrapper
<Breadcrumb data={{ title: T.translate("sponsor_pages.pages"), pathname: props.match.url }} /> + <Switch> <Route strict exact path={props.match.url} component={SponsorPagesListPage} /> + </Switch>src/reducers/sponsors_inventory/page-template-list-reducer.js (1)
77-83: Variable names don't align with constant names, which may cause confusion.The variable names
upload_modanddownload_moddon't semantically match the constants they're filtering on (PAGES_MODULE_KINDS.MEDIAandPAGES_MODULE_KINDS.DOCUMENTrespectively). This creates a disconnect that could confuse maintainers.Consider renaming for clarity:
upload_mod→media_mod(since it filtersMEDIAmodules)download_mod→document_mod(since it filtersDOCUMENTmodules)Alternatively, if these names are intentional for UI purposes, add a brief comment explaining the mapping.
src/pages/sponsors-global/page-templates/modules/page-template-media-request-module.js (1)
84-86: Hardcoded file type options may need expansion.The select currently only offers "PDF" as an option. If more file types should be supported, consider making this dynamic via props or constants.
src/pages/sponsors-global/page-templates/page-template-modules-form.js (1)
155-162: Consider makingdroppableIdunique per instance.If multiple PageModules are ever rendered on the same page, a static ID can collide in the DnD layer.
♻️ Proposed refactor
- droppableId="modules-list" + droppableId={`modules-list-${name}`}src/pages/sponsors/sponsor-pages-list-page/components/global-page/global-page-popup.js (1)
25-28: Consider closing the dialog only on success.Closing in
finallyhides errors and forces users to re-open and re-select after a failed clone.♻️ Proposed refactor
- cloneGlobalPage(selectedTemplates, selectedTiers, allTiers).finally(() => { - handleClose(); - }); + cloneGlobalPage(selectedTemplates, selectedTiers, allTiers) + .then(() => { + handleClose(); + }) + .catch(() => {});
| return getRequest( | ||
| createAction(REQUEST_SPONSOR_PAGES), | ||
| createAction(RECEIVE_SPONSOR_PAGES), | ||
| `${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/show-pages`, | ||
| authErrorHandler, | ||
| { order, orderDir, page, term, hideArchived } | ||
| )(params)(dispatch).then(() => { | ||
| dispatch(stopLoading()); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file and lines mentioned
cat -n src/actions/sponsor-pages-actions.js | sed -n '80,100p'Repository: fntechgit/summit-admin
Length of output: 975
🏁 Script executed:
# Find where getRequest is defined
rg "const getRequest|function getRequest|export.*getRequest" --type js -B 2 -A 10Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check imports in the file to see where getRequest comes from
head -30 src/actions/sponsor-pages-actions.jsRepository: fntechgit/summit-admin
Length of output: 1180
🏁 Script executed:
# Find cloneGlobalPage implementation
sed -n '96,150p' src/actions/sponsor-pages-actions.jsRepository: fntechgit/summit-admin
Length of output: 1204
🏁 Script executed:
# Search for other uses of getRequest to see error handling patterns
rg "getRequest\(" src/actions/ -A 6 -B 1 | head -100Repository: fntechgit/summit-admin
Length of output: 6135
🏁 Script executed:
# Search for authErrorHandler patterns with catch or finally
rg "authErrorHandler.*\)" -A 8 | grep -E "(catch|finally)" -B 5Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Search for postRequest patterns to see if they use different error handling
rg "postRequest\(" src/actions/ -A 6 | grep -E "(catch|finally|\.then)" -B 2 -A 2 | head -60Repository: fntechgit/summit-admin
Length of output: 2526
🏁 Script executed:
# Search for .catch() handlers in actions to understand error handling patterns
rg "\.catch\(" src/actions/ -B 3 -A 3 | head -80Repository: fntechgit/summit-admin
Length of output: 4194
🏁 Script executed:
# Check sponsor-forms-actions.js to see those catch patterns in context
rg "need to catch promise reject" src/actions/sponsor-forms-actions.js -B 10 -A 3 | head -80Repository: fntechgit/summit-admin
Length of output: 2389
🏁 Script executed:
# Let's verify the exact getSponsorPages implementation one more time to be certain
sed -n '60,100p' src/actions/sponsor-pages-actions.jsRepository: fntechgit/summit-admin
Length of output: 1336
Ensure loading state clears on request failure.
stopLoading() runs only on success; if getRequest rejects, the spinner can remain stuck. Similar functions in the codebase explicitly handle rejection with .catch() and .finally() patterns (see sponsor-forms-actions.js with comments "need to catch promise reject").
🐛 Proposed fix
return getRequest(
createAction(REQUEST_SPONSOR_PAGES),
createAction(RECEIVE_SPONSOR_PAGES),
`${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/show-pages`,
authErrorHandler,
{ order, orderDir, page, term, hideArchived }
- )(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 getRequest( | |
| createAction(REQUEST_SPONSOR_PAGES), | |
| createAction(RECEIVE_SPONSOR_PAGES), | |
| `${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/show-pages`, | |
| authErrorHandler, | |
| { order, orderDir, page, term, hideArchived } | |
| )(params)(dispatch).then(() => { | |
| dispatch(stopLoading()); | |
| }); | |
| return getRequest( | |
| createAction(REQUEST_SPONSOR_PAGES), | |
| createAction(RECEIVE_SPONSOR_PAGES), | |
| `${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/show-pages`, | |
| authErrorHandler, | |
| { order, orderDir, page, term, hideArchived } | |
| )(params)(dispatch).finally(() => { | |
| dispatch(stopLoading()); | |
| }); |
🤖 Prompt for AI Agents
In `@src/actions/sponsor-pages-actions.js` around lines 85 - 93, The current
getRequest(...) call in sponsor-pages-actions.js (using
createAction(REQUEST_SPONSOR_PAGES), createAction(RECEIVE_SPONSOR_PAGES),
authErrorHandler, etc.) only dispatches stopLoading() in the .then path so a
rejected request leaves the spinner running; update the promise handling on the
getRequest(...) invocation to catch rejections and ensure stopLoading() is
always dispatched (use .catch(...) to handle errors and/or .finally(...) to call
dispatch(stopLoading())), preserving existing error handling via
authErrorHandler and the original action creators.
| helperText: meta.touched && meta.error, | ||
| fullWidth: true, | ||
| margin: "normal" | ||
| fullWidth: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
find . -name "mui-formik-datepicker.js" -type fRepository: fntechgit/summit-admin
Length of output: 125
🏁 Script executed:
head -50 src/components/mui/formik-inputs/mui-formik-datepicker.jsRepository: fntechgit/summit-admin
Length of output: 1536
🌐 Web query:
MUI X DatePicker margin prop API documentation
💡 Result:
In MUI X DatePicker, the margin prop is not a direct DatePicker prop; it belongs to the underlying field component (a pickers TextField).
-
API docs for
margin: it’s documented onPickersTextField(and also onDateField) asmargin: 'none' | 'dense' | 'normal'with default'none'. [1], [2] -
How to set it on
DatePicker(current MUI X): pass it to the field viaslotProps.textField:<DatePicker slotProps={{ textField: { margin: 'dense' } }} />
This “Customize the TextField” approach is shown in the Custom field docs. [3]
Sources:
[1] PickersTextField API (margin) (mui.com)
[2] DateField API (margin) (mui.com)
[3] Custom field docs (use slotProps.textField to pass TextField props) (mui.com)
Citations:
- 1: https://mui.com/x/api/date-pickers/pickers-text-field/?utm_source=openai
- 2: https://mui.com/x/api/date-pickers/date-field/?utm_source=openai
- 3: https://mui.com/x/react-date-pickers/custom-field/?utm_source=openai
🌐 Web query:
"DatePicker" "slotProps" "textField" margin MUI X
💡 Result:
To set the margin on the input used by MUI X DatePicker, pass it through the textField slotProps:
<DatePicker
label="Start date"
slotProps={{
textField: { margin: 'dense' }, // or 'normal' | 'none'
}}
/>slotProps.textField forwards props to the underlying picker field TextField, and margin is a supported prop ('dense' | 'normal' | 'none', default 'none'). [1], [2]
margin="normal" won't be applied on DatePicker.
MUI X DatePicker does not accept a margin prop directly—it must be passed through slotProps.textField. Placing it on the component level will be silently ignored. Move margin into slotProps.textField for it to take effect.
🐛 Proposed fix
slotProps={{
textField: {
name,
label: required ? requiredLabel : label,
error: meta.touched && Boolean(meta.error),
helperText: meta.touched && meta.error,
- fullWidth: true
+ fullWidth: true,
+ margin: "normal"
},
day: {
sx: {
fontSize: "1.2rem",
fontWeight: 600
}
},
layout: {
sx: {
"& .MuiDayCalendar-weekDayLabel": {
fontSize: "1rem"
}
}
}
}}
- margin="normal"
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}🤖 Prompt for AI Agents
In `@src/components/mui/formik-inputs/mui-formik-datepicker.js` at line 22, The
DatePicker's margin prop is currently set at the component level and is ignored;
update the mui DatePicker usage in mui-formik-datepicker (the DatePicker
component instance) to move margin="normal" into slotProps.textField (e.g.,
slotProps={{ textField: { margin: "normal", ... } }}) so the TextField receives
the margin; ensure any existing slotProps.textField merging preserves other
props.
| const buildFileObject = (response) => { | ||
| const file = {}; | ||
| if (response.id !== undefined) file.id = response.id; | ||
| if (response.name) file.file_name = response.name; | ||
| if (response.md5) file.md5 = response.md5; | ||
| if (response.mime_type) file.mime_type = response.mime_type; | ||
| if (response.source_bucket) file.bucket = response.source_bucket; | ||
| if (response.path && response.name) | ||
| file.file_path = `${response.path}${response.name}`; | ||
| return file; | ||
| }; | ||
|
|
||
| const handleUploadComplete = (response) => { | ||
| if (response) { | ||
| const image = { | ||
| file_path: `${response.path}${response.name}`, | ||
| filename: response.name | ||
| }; | ||
| helpers.setValue([...field.value, image]); | ||
| const image = buildFileObject(response); | ||
| helpers.setValue([...(field.value || []), image]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Consider adding filename property in buildFileObject for consistency.
The getInputValue function expects a filename property to exist for display purposes (lines 32-33 show fallback chain). Adding it in buildFileObject would ensure newly uploaded files are immediately compatible with the removal logic and display.
Suggested fix
const buildFileObject = (response) => {
const file = {};
if (response.id !== undefined) file.id = response.id;
if (response.name) file.file_name = response.name;
+ if (response.name) file.filename = response.name;
if (response.md5) file.md5 = response.md5;
if (response.mime_type) file.mime_type = response.mime_type;
if (response.source_bucket) file.bucket = response.source_bucket;
if (response.path && response.name)
file.file_path = `${response.path}${response.name}`;
return file;
};📝 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 buildFileObject = (response) => { | |
| const file = {}; | |
| if (response.id !== undefined) file.id = response.id; | |
| if (response.name) file.file_name = response.name; | |
| if (response.md5) file.md5 = response.md5; | |
| if (response.mime_type) file.mime_type = response.mime_type; | |
| if (response.source_bucket) file.bucket = response.source_bucket; | |
| if (response.path && response.name) | |
| file.file_path = `${response.path}${response.name}`; | |
| return file; | |
| }; | |
| const handleUploadComplete = (response) => { | |
| if (response) { | |
| const image = { | |
| file_path: `${response.path}${response.name}`, | |
| filename: response.name | |
| }; | |
| helpers.setValue([...field.value, image]); | |
| const image = buildFileObject(response); | |
| helpers.setValue([...(field.value || []), image]); | |
| const buildFileObject = (response) => { | |
| const file = {}; | |
| if (response.id !== undefined) file.id = response.id; | |
| if (response.name) file.file_name = response.name; | |
| if (response.name) file.filename = response.name; | |
| if (response.md5) file.md5 = response.md5; | |
| if (response.mime_type) file.mime_type = response.mime_type; | |
| if (response.source_bucket) file.bucket = response.source_bucket; | |
| if (response.path && response.name) | |
| file.file_path = `${response.path}${response.name}`; | |
| return file; | |
| }; | |
| const handleUploadComplete = (response) => { | |
| if (response) { | |
| const image = buildFileObject(response); | |
| helpers.setValue([...(field.value || []), image]); |
🤖 Prompt for AI Agents
In `@src/components/mui/formik-inputs/mui-formik-upload.js` around lines 37 - 52,
The buildFileObject function currently maps response.name to file.file_name but
doesn't set file.filename, causing newly uploaded files to miss the key expected
by getInputValue and removal/display logic; update buildFileObject to also set
file.filename (e.g., from response.name || response.file_name) so
handleUploadComplete will push objects compatible with getInputValue and
existing removal logic.
| const handleRemove = (imageFile) => { | ||
| const updated = field.value.filter((i) => i.filename !== imageFile.name); | ||
| const updated = (field.value || []).filter( | ||
| (i) => i.filename !== imageFile.name | ||
| ); | ||
| helpers.setValue(updated); | ||
|
|
||
| if (onImageDeleted) { | ||
| onImageDeleted(imageFile.id); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: handleRemove won't work for newly uploaded files due to property name mismatch.
The handleRemove function filters by i.filename !== imageFile.name, but buildFileObject creates objects with file_name (not filename). Newly uploaded files won't have a filename property, so they can never be removed.
Additionally, imageFile.name comes from the UploadInputV2 callback, but the file object stored has file_name. The comparison logic needs alignment.
Suggested fix
const handleRemove = (imageFile) => {
const updated = (field.value || []).filter(
- (i) => i.filename !== imageFile.name
+ (i) => (i.filename || i.file_name) !== imageFile.name
);
helpers.setValue(updated);
if (onImageDeleted) {
onImageDeleted(imageFile.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 handleRemove = (imageFile) => { | |
| const updated = field.value.filter((i) => i.filename !== imageFile.name); | |
| const updated = (field.value || []).filter( | |
| (i) => i.filename !== imageFile.name | |
| ); | |
| helpers.setValue(updated); | |
| if (onImageDeleted) { | |
| onImageDeleted(imageFile.id); | |
| } | |
| }; | |
| const handleRemove = (imageFile) => { | |
| const updated = (field.value || []).filter( | |
| (i) => (i.filename || i.file_name) !== imageFile.name | |
| ); | |
| helpers.setValue(updated); | |
| if (onImageDeleted) { | |
| onImageDeleted(imageFile.id); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@src/components/mui/formik-inputs/mui-formik-upload.js` around lines 57 - 65,
The removal logic in handleRemove is comparing i.filename to imageFile.name but
uploaded file objects use file_name (created by buildFileObject), so newly
uploaded files never match; update handleRemove to compare using the correct
properties (e.g., compare (i.file_name || i.filename) to (imageFile.name ||
imageFile.file_name)) and/or also fall back to matching on id when available
(i.id === imageFile.id) before calling helpers.setValue and onImageDeleted to
ensure both existing and newly uploaded files can be removed.
src/pages/sponsors-global/page-templates/modules/page-template-document-download-module.js
Show resolved
Hide resolved
src/pages/sponsors-global/page-templates/modules/page-template-info-module.js
Show resolved
Hide resolved
src/pages/sponsors-global/page-templates/modules/page-template-media-request-module.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/pages/sponsors-global/page-templates/page-template-list-page.js`:
- Around line 122-133: handleDelete currently passes the full row object to
deletePageTemplate but that action expects a template ID (formTemplateId);
update handleDelete to extract and pass row.formTemplateId to deletePageTemplate
(and optionally guard against missing formTemplateId by returning early or
throwing), e.g. call deletePageTemplate(row.formTemplateId).then(() =>
getPageTemplates(...)) so the API receives the correct ID.
In `@src/pages/sponsors-global/page-templates/page-template-modules-form.js`:
- Around line 88-143: The Accordion returned by renderModule lacks a React key;
update the renderModule function to add a stable key prop to the top-level
Accordion (for example key={module.id || `module-${index}`}) so React warnings
are avoided and list reconciliation is stable—modify the Accordion element in
renderModule to include this key.
In `@src/pages/sponsors-global/page-templates/page-template-popup.js`:
- Around line 160-162: The PageModules component is being passed an unused
formik prop even though it uses useFormikContext() internally and accepts an
optional name prop (default "modules"); remove the formik={formik} prop from the
PageModules invocation in page-template-popup.js (the JSX inside the Box) so it
is called simply as <PageModules /> or with an explicit name if needed, and
ensure there are no other references expecting a formik prop in the PageModules
usage.
🧹 Nitpick comments (3)
src/pages/sponsors-global/page-templates/modules/page-template-media-request-module.js (1)
80-86: Consider making file type options configurable or fetching from API.The file type select currently only offers "PDF" as a hardcoded option. If more file types are expected in the future, consider fetching available file types from an API or defining them as constants.
src/pages/sponsors-global/page-templates/page-template-modules-form.js (1)
38-49: Consider translating the default "Module" string.The default case returns a hardcoded "Module" string while other cases use
T.translate(). For consistency and i18n support, consider adding a translation key for the default case.Suggested fix
case PAGES_MODULE_KINDS.MEDIA: return T.translate("page_template_list.page_crud.media_module"); default: - return "Module"; + return T.translate("page_template_list.page_crud.module"); }src/pages/sponsors-global/page-templates/page-template-list-page.js (1)
105-107: Stub implementation for clone functionality.
handleClonePageTemplateonly logs to console. Ensure this is intentional placeholder code and will be implemented before release.Would you like me to help implement the clone functionality or create an issue to track this task?
| const handleDelete = (row) => { | ||
| console.log("DELETE", row); | ||
| deletePageTemplate(row).then(() => | ||
| getPageTemplates( | ||
| term, | ||
| currentPage, | ||
| perPage, | ||
| order, | ||
| orderDir, | ||
| hideArchived | ||
| ) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: deletePageTemplate receives wrong argument type.
The deletePageTemplate action expects a template ID (formTemplateId), but it's being called with the entire row object. This will cause the API call to fail or behave unexpectedly.
Suggested fix
const handleDelete = (row) => {
- deletePageTemplate(row).then(() =>
+ deletePageTemplate(row.id).then(() =>
getPageTemplates(
term,
currentPage,
perPage,
order,
orderDir,
hideArchived
)
);
};📝 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 handleDelete = (row) => { | |
| console.log("DELETE", row); | |
| deletePageTemplate(row).then(() => | |
| getPageTemplates( | |
| term, | |
| currentPage, | |
| perPage, | |
| order, | |
| orderDir, | |
| hideArchived | |
| ) | |
| ); | |
| }; | |
| const handleDelete = (row) => { | |
| deletePageTemplate(row.id).then(() => | |
| getPageTemplates( | |
| term, | |
| currentPage, | |
| perPage, | |
| order, | |
| orderDir, | |
| hideArchived | |
| ) | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In `@src/pages/sponsors-global/page-templates/page-template-list-page.js` around
lines 122 - 133, handleDelete currently passes the full row object to
deletePageTemplate but that action expects a template ID (formTemplateId);
update handleDelete to extract and pass row.formTemplateId to deletePageTemplate
(and optionally guard against missing formTemplateId by returning early or
throwing), e.g. call deletePageTemplate(row.formTemplateId).then(() =>
getPageTemplates(...)) so the API receives the correct ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
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/page-templates/page-template-popup.js (1)
176-180: Add missingpageTemplateprop type.The
pageTemplateprop is received from Redux state viamapStateToPropsbut is not declared in PropTypes.🔧 Suggested fix
PageTemplatePopup.propTypes = { + pageTemplate: PropTypes.object, open: PropTypes.bool.isRequired, onClose: PropTypes.func.isRequired, onSave: PropTypes.func.isRequired };
🧹 Nitpick comments (1)
src/pages/sponsors-global/page-templates/page-template-popup.js (1)
97-99: Consider simplifying onClick handlers.The arrow function wrappers are unnecessary since the handlers don't require arguments or event binding.
♻️ Suggested simplification
- <IconButton size="small" onClick={() => handleClose()} sx={{ mr: 1 }}> + <IconButton size="small" onClick={handleClose} sx={{ mr: 1 }}>- onClick={() => handleAddInfo()} + onClick={handleAddInfo}- onClick={() => handleAddDocument()} + onClick={handleAddDocument}- onClick={() => handleAddMedia()} + onClick={handleAddMedia}Also applies to: 132-132, 142-142, 152-152
src/pages/sponsors-global/page-templates/page-template-list-page.js
Outdated
Show resolved
Hide resolved
src/pages/sponsors-global/page-templates/page-template-popup.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/pages/sponsors-global/page-templates/modules/page-template-media-request-module.js`:
- Around line 30-105: The MuiFormikSelect component currently sets
labelId={`${name}-label`} while the InputLabel uses htmlFor={name}, so the
underlying Select is missing id={name} and the label-input association breaks
for screen readers; update the MuiFormikSelect implementation (the component
named MuiFormikSelect) to pass id={name} to the Select element (alongside
labelId) so htmlFor and id match, ensuring proper accessibility.
🧹 Nitpick comments (2)
src/pages/sponsors-global/page-templates/modules/page-template-media-request-module.js (1)
80-87: WIP placeholder:file_type_iddropdown has only a hardcoded PDF option.The
MuiFormikSelectcontains a single hardcodedMenuItemwith string value"PDF". This appears incomplete based on the{/* WIP DDL Options */}comment. Consider:
- Fetching allowed file types from an API or config
- Using numeric IDs if
file_type_idexpects an ID (the field name suggests it should be an ID, not a string label)Would you like me to open an issue to track completing the file type options?
src/pages/sponsors-global/page-templates/page-template-modules-form.js (1)
38-49: Default fallback string should be internationalized.Line 47 returns a hardcoded
"Module"string for unknown kinds. Consider using a translation key for consistency with the rest of the UI.Suggested fix
default: - return "Module"; + return T.translate("page_template_list.page_crud.module");
src/pages/sponsors-global/page-templates/modules/page-template-media-request-module.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/formik-inputs/mui-formik-select.js (1)
75-80: Missing PropTypes definition forlabelprop.The
labelprop is destructured and used in the component (lines 16, 35, 45) but is not declared in PropTypes.Proposed fix
MuiFormikSelect.propTypes = { name: PropTypes.string.isRequired, children: PropTypes.node.isRequired, + label: PropTypes.string, placeholder: PropTypes.string, isClearable: PropTypes.bool };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/actions/media-file-type-actions.js (1)
141-186: Missing return statement in save action.The
putRequestbranch (lines 152-163) does not return its promise, unlike thepostRequestbranch. This inconsistency means callers cannot chain.then()on the returned promise when updating an existing entity.🐛 Proposed fix
if (entity.id) { - putRequest( + return putRequest( createAction(UPDATE_MEDIA_FILE_TYPE), createAction(MEDIA_FILE_TYPE_UPDATED), `${window.API_BASE_URL}/api/v1/summit-media-file-types/${entity.id}`, normalizedEntity, authErrorHandler, entity )(params)(dispatch).then(() => {
🤖 Fix all issues with AI agents
In `@src/actions/media-file-type-actions.js`:
- Around line 218-238: The file uses _.debounce in queryMediaFileTypes but
lodash isn't imported and fetch errors never call the autocomplete callback; add
an import for debounce (or lodash) at the top (e.g., import { debounce } from
'lodash' or import _ from 'lodash') and change queryMediaFileTypes to use the
imported debounce (or _) instead of the undefined _; also update the fetch
.catch handler (associated with fetchErrorHandler) to ensure callback is invoked
on error (e.g., call callback([]) or callback(null) after calling
fetchErrorHandler) so the async autocomplete is not left loading.
In `@src/actions/page-template-actions.js`:
- Around line 145-169: normalizeEntity currently assumes entity.modules is
always an array and calls .map, which throws if modules is null/undefined;
modify normalizeEntity to defensively handle missing modules by using a fallback
array (e.g., const modules = entity.modules || [] or entity.modules ?? []) or
optional chaining before mapping so normalization proceeds without error, then
assign normalizedEntity.modules = modules.map(...) as before (keeping the same
per-module handling like upload_deadline, file_type_id, file and deleting
_tempId).
In `@src/components/inputs/formik-text-editor.js`:
- Line 12: The Formik field value can be undefined causing the editor to switch
between uncontrolled/controlled; in the FormikTextEditor component replace
direct use of field.value with a safe fallback (e.g., when setting the editor's
value prop in the component use a null/undefined-safe default like an empty
string) so the editor remains controlled; update the value reference
(field.value) used in the component's render/props to use the fallback and
ensure any related onChange still writes back into Formik via the same field
helpers (e.g., setFieldValue or field.onChange).
In `@src/i18n/en.json`:
- Around line 2647-2656: Update the localization keys used in the sponsor page
popup to use page-appropriate wording: change the "name" value from lowercase to
"Name" (capitalize for field label consistency) and replace the "error" value
that mentions “forms” with a page-centric message such as referencing "page" or
"page template" (e.g., "There was a problem creating the page, please try
again." or "There was a problem creating the page template, please try again.")
so the copy in the Pages flow is consistent (edit the JSON values for "name" and
"error").
🧹 Nitpick comments (1)
src/actions/media-file-type-actions.js (1)
226-229: Avoid reassigning function parameter.Reassigning the
inputparameter on line 227 is a minor code smell that can cause confusion. Consider using a separate variable.♻️ Suggested improvement
if (input) { - input = escapeFilterValue(input); - apiUrl.addQuery("filter[]", `name=@${input}`); + const escapedInput = escapeFilterValue(input); + apiUrl.addQuery("filter[]", `name=@${escapedInput}`); }
| export const queryMediaFileTypes = _.debounce(async (input, callback) => { | ||
| const accessToken = await getAccessTokenSafely(); | ||
| const apiUrl = URI(`${window.API_BASE_URL}/api/v1/summit-media-file-types`); | ||
|
|
||
| apiUrl.addQuery("access_token", accessToken); | ||
| apiUrl.addQuery("order", "name"); | ||
| apiUrl.addQuery("per_page", FIVE_PER_PAGE); | ||
|
|
||
| if (input) { | ||
| input = escapeFilterValue(input); | ||
| apiUrl.addQuery("filter[]", `name=@${input}`); | ||
| } | ||
|
|
||
| fetch(apiUrl.toString()) | ||
| .then(fetchResponseHandler) | ||
| .then((json) => { | ||
| const options = [...json.data]; | ||
| callback(options); | ||
| }) | ||
| .catch(fetchErrorHandler); | ||
| }, DEBOUNCE_WAIT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing lodash import causes runtime error.
_.debounce is used on line 218, but lodash is not imported in this file. This will throw a ReferenceError: _ is not defined at runtime.
Additionally, when fetch fails, fetchErrorHandler is called but the callback is never invoked, which will leave the async autocomplete component in a loading state indefinitely.
🐛 Proposed fix
Add the lodash import at the top of the file with other imports:
import T from "i18n-react/dist/i18n-react";
+import debounce from "lodash/debounce";
import {
getRequest,Then update the function to use the named import and handle errors properly:
-export const queryMediaFileTypes = _.debounce(async (input, callback) => {
+export const queryMediaFileTypes = debounce(async (input, callback) => {
const accessToken = await getAccessTokenSafely();
const apiUrl = URI(`${window.API_BASE_URL}/api/v1/summit-media-file-types`);
apiUrl.addQuery("access_token", accessToken);
apiUrl.addQuery("order", "name");
apiUrl.addQuery("per_page", FIVE_PER_PAGE);
if (input) {
input = escapeFilterValue(input);
apiUrl.addQuery("filter[]", `name=@${input}`);
}
fetch(apiUrl.toString())
.then(fetchResponseHandler)
.then((json) => {
const options = [...json.data];
callback(options);
})
- .catch(fetchErrorHandler);
+ .catch((err) => {
+ fetchErrorHandler(err);
+ callback([]);
+ });
}, DEBOUNCE_WAIT);🤖 Prompt for AI Agents
In `@src/actions/media-file-type-actions.js` around lines 218 - 238, The file uses
_.debounce in queryMediaFileTypes but lodash isn't imported and fetch errors
never call the autocomplete callback; add an import for debounce (or lodash) at
the top (e.g., import { debounce } from 'lodash' or import _ from 'lodash') and
change queryMediaFileTypes to use the imported debounce (or _) instead of the
undefined _; also update the fetch .catch handler (associated with
fetchErrorHandler) to ensure callback is invoked on error (e.g., call
callback([]) or callback(null) after calling fetchErrorHandler) so the async
autocomplete is not left loading.
| const normalizeEntity = (entity) => { | ||
| const normalizedEntity = { ...entity }; | ||
|
|
||
| normalizedEntity.modules = []; | ||
| normalizedEntity.modules = entity.modules.map((module) => { | ||
| const normalizedModule = { ...module }; | ||
|
|
||
| if (module.kind === PAGES_MODULE_KINDS.MEDIA && module.upload_deadline) { | ||
| normalizedModule.upload_deadline = moment | ||
| .utc(module.upload_deadline) | ||
| .unix(); | ||
| } | ||
|
|
||
| if (module.kind === PAGES_MODULE_KINDS.MEDIA && module.file_type_id) { | ||
| normalizedModule.file_type_id = | ||
| module.file_type_id?.value || module.file_type_id; | ||
| } | ||
|
|
||
| if (module.kind === PAGES_MODULE_KINDS.DOCUMENT && module.file) { | ||
| normalizedModule.file = module.file[0] || null; | ||
| } | ||
|
|
||
| delete normalizedModule._tempId; | ||
|
|
||
| return normalizedModule; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against missing modules during normalization.
If entity.modules is null/undefined (e.g., legacy templates or partial payloads), .map will throw and block save. Add a defensive fallback to keep saves resilient.
Proposed fix
- normalizedEntity.modules = entity.modules.map((module) => {
+ const modules = Array.isArray(entity.modules) ? entity.modules : [];
+ normalizedEntity.modules = modules.map((module) => {📝 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 normalizeEntity = (entity) => { | |
| const normalizedEntity = { ...entity }; | |
| normalizedEntity.modules = []; | |
| normalizedEntity.modules = entity.modules.map((module) => { | |
| const normalizedModule = { ...module }; | |
| if (module.kind === PAGES_MODULE_KINDS.MEDIA && module.upload_deadline) { | |
| normalizedModule.upload_deadline = moment | |
| .utc(module.upload_deadline) | |
| .unix(); | |
| } | |
| if (module.kind === PAGES_MODULE_KINDS.MEDIA && module.file_type_id) { | |
| normalizedModule.file_type_id = | |
| module.file_type_id?.value || module.file_type_id; | |
| } | |
| if (module.kind === PAGES_MODULE_KINDS.DOCUMENT && module.file) { | |
| normalizedModule.file = module.file[0] || null; | |
| } | |
| delete normalizedModule._tempId; | |
| return normalizedModule; | |
| }); | |
| const normalizeEntity = (entity) => { | |
| const normalizedEntity = { ...entity }; | |
| const modules = Array.isArray(entity.modules) ? entity.modules : []; | |
| normalizedEntity.modules = modules.map((module) => { | |
| const normalizedModule = { ...module }; | |
| if (module.kind === PAGES_MODULE_KINDS.MEDIA && module.upload_deadline) { | |
| normalizedModule.upload_deadline = moment | |
| .utc(module.upload_deadline) | |
| .unix(); | |
| } | |
| if (module.kind === PAGES_MODULE_KINDS.MEDIA && module.file_type_id) { | |
| normalizedModule.file_type_id = | |
| module.file_type_id?.value || module.file_type_id; | |
| } | |
| if (module.kind === PAGES_MODULE_KINDS.DOCUMENT && module.file) { | |
| normalizedModule.file = module.file[0] || null; | |
| } | |
| delete normalizedModule._tempId; | |
| return normalizedModule; | |
| }); |
🤖 Prompt for AI Agents
In `@src/actions/page-template-actions.js` around lines 145 - 169, normalizeEntity
currently assumes entity.modules is always an array and calls .map, which throws
if modules is null/undefined; modify normalizeEntity to defensively handle
missing modules by using a fallback array (e.g., const modules = entity.modules
|| [] or entity.modules ?? []) or optional chaining before mapping so
normalization proceeds without error, then assign normalizedEntity.modules =
modules.map(...) as before (keeping the same per-module handling like
upload_deadline, file_type_id, file and deleting _tempId).
| onChange={(e) => setFieldValue(name, e.target.value)} | ||
| onBlur={() => setFieldTouched(name, true)} | ||
| error={touched?.[name] && errors?.[name] ? errors?.[name] : ""} | ||
| value={field.value} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against undefined to keep the editor controlled.
Formik can leave field.value undefined if initial values don’t include this field, which can trigger controlled/uncontrolled warnings or editor quirks. Consider a safe fallback.
♻️ Proposed fix
- value={field.value}
+ value={field.value ?? ""}📝 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.
| value={field.value} | |
| value={field.value ?? ""} |
🤖 Prompt for AI Agents
In `@src/components/inputs/formik-text-editor.js` at line 12, The Formik field
value can be undefined causing the editor to switch between
uncontrolled/controlled; in the FormikTextEditor component replace direct use of
field.value with a safe fallback (e.g., when setting the editor's value prop in
the component use a null/undefined-safe default like an empty string) so the
editor remains controlled; update the value reference (field.value) used in the
component's render/props to use the fallback and ensure any related onChange
still writes back into Formik via the same field helpers (e.g., setFieldValue or
field.onChange).
| "title": "Add Page Template", | ||
| "items_selected": "items selected", | ||
| "code": "Code", | ||
| "name": "name", | ||
| "info_mod": "Info Mod", | ||
| "download_mod": "Download Mod", | ||
| "upload_mod": "Upload Mod", | ||
| "add_selected": "Add Selected Page Template", | ||
| "success": "Page created successfully.", | ||
| "error": "There was a problem creating the forms, please try again.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align sponsor page popup copy with page terminology.
name is lowercase and the error message references “forms”, which is confusing in the Pages flow.
Proposed fix
- "name": "name",
+ "name": "Name",
...
- "error": "There was a problem creating the forms, please try again.",
+ "error": "There was a problem creating the pages, please try again.",📝 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.
| "title": "Add Page Template", | |
| "items_selected": "items selected", | |
| "code": "Code", | |
| "name": "name", | |
| "info_mod": "Info Mod", | |
| "download_mod": "Download Mod", | |
| "upload_mod": "Upload Mod", | |
| "add_selected": "Add Selected Page Template", | |
| "success": "Page created successfully.", | |
| "error": "There was a problem creating the forms, please try again.", | |
| "title": "Add Page Template", | |
| "items_selected": "items selected", | |
| "code": "Code", | |
| "name": "Name", | |
| "info_mod": "Info Mod", | |
| "download_mod": "Download Mod", | |
| "upload_mod": "Upload Mod", | |
| "add_selected": "Add Selected Page Template", | |
| "success": "Page created successfully.", | |
| "error": "There was a problem creating the pages, please try again.", |
🤖 Prompt for AI Agents
In `@src/i18n/en.json` around lines 2647 - 2656, Update the localization keys used
in the sponsor page popup to use page-appropriate wording: change the "name"
value from lowercase to "Name" (capitalize for field label consistency) and
replace the "error" value that mentions “forms” with a page-centric message such
as referencing "page" or "page template" (e.g., "There was a problem creating
the page, please try again." or "There was a problem creating the page template,
please try again.") so the copy in the Pages flow is consistent (edit the JSON
values for "name" and "error").
…opup Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…750) * feat: page modules generic component, test, adjusting formik inputs Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: add implementation for pages modules on sponsor global Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> --------- Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…ments on formik components Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
594e50c to
dbab089
Compare
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/formik-inputs/mui-formik-select.js (1)
14-20: Addlabelto PropTypes to match the public API.
labelis accepted as a prop (line 16) and used throughout the component (lines 34, 45), but it's missing fromMuiFormikSelect.propTypes, preventing prop validation and documentation.🛠️ Suggested fix
MuiFormikSelect.propTypes = { name: PropTypes.string.isRequired, + label: PropTypes.string, children: PropTypes.node.isRequired, placeholder: PropTypes.string, isClearable: PropTypes.bool };
🤖 Fix all issues with AI agents
In `@src/components/mui/formik-inputs/mui-formik-select.js`:
- Around line 29-31: The hasValue/shouldShrink logic and the renderValue code
treat falsy-but-valid values like 0 and false as empty; update checks to
explicitly compare against "", undefined, and null instead of using truthiness.
In the Select component replace the current hasValue and shouldShrink
(referenced as hasValue and shouldShrink) with explicit checks: consider a value
present when field.value !== "" && field.value !== undefined && field.value !==
null; likewise update renderValue (the renderValue function/prop) to detect
empty values by testing value === "" || value === undefined || value === null
and only return the placeholder in those cases; apply the same explicit-empty
checks to the code referenced around lines 48-51 (the other render/placeholder
branch).
In `@src/pages/sponsors-global/page-templates/page-template-module-form.test.js`:
- Around line 381-386: Update the test assertion for showConfirmDialog to match
the component's API change: replace the object key "type" with "iconType" while
keeping the same value ("warning") and other properties (title and
showCancelButton). Locate the expectation that calls
expect(showConfirmDialog).toHaveBeenCalledWith(expect.objectContaining(...)) in
the page-template-module-form.test and change the inner object to include
iconType: "warning" instead of type: "warning".
In `@src/pages/sponsors-global/page-templates/page-template-modules-form.js`:
- Around line 54-63: The confirm dialog call assigned to isConfirmed is passing
properties that are ignored; replace the type prop with iconType: "warning"
(since showConfirmDialog destructures iconType) and remove the showCancelButton
prop (it's ignored); update the showConfirmDialog invocation (the call where
isConfirmed is set) to use iconType instead of type so the warning icon displays
correctly.
In `@src/pages/sponsors/sponsor-pages-list-page/index.js`:
- Around line 69-73: The "new" popup is not rendered because the popup component
was commented out, so handleRowEdit/getSponsorForm calling setOpenPopup("new")
does nothing; either restore the popup import and its JSX so it renders when
openPopup === "new" (and wire it to the sponsor form state fetched by
getSponsorForm), or disable/remove the toolbar button and row-edit handler that
call setOpenPopup("new") (update handleRowEdit and any toolbar onClick) so they
no longer trigger the no-op; apply the same fix to the other occurrences that
toggle openPopup to "new".
- Around line 75-84: handleRowDelete and handleArchiveItem are stubs that only
log, leaving non-functional action buttons; either wire them to the real API
helpers or remove/hide the buttons. Implement handleRowDelete to call
deleteSponsorForm(itemId) and handle success/error flows; implement
handleArchiveItem to call archiveSponsorForm(item.id) or
unarchiveSponsorForm(item.id) depending on item.is_archived and update UI state
accordingly; alternatively remove the action column or conditionally hide action
buttons until those functions (deleteSponsorForm, archiveSponsorForm,
unarchiveSponsorForm) are implemented to avoid dead controls.
🧹 Nitpick comments (3)
.env.example (1)
20-20: Consider reordering to satisfy dotenv-linter key ordering.If you want to keep the linter clean, move
SPONSOR_PAGES_SCOPESaboveSPONSOR_USERS_API_SCOPESas suggested by the UnorderedKey warning.src/components/mui/formik-inputs/mui-formik-radio-group.js (1)
13-19: LGTM! Clean addition of configurable margin prop.The
marginWrapperprop with a sensible default maintains backward compatibility while allowing flexibility. The implementation follows the prop-forwarding patterns used in other formik input components.Optional: Consider using stricter PropTypes validation for margin values.
🔧 Stricter PropTypes for margin values
MuiFormikRadioGroup.propTypes = { name: PropTypes.string.isRequired, label: PropTypes.string, - marginWrapper: PropTypes.string, + marginWrapper: PropTypes.oneOf(["none", "dense", "normal"]), options: PropTypes.array.isRequired };Also applies to: 25-25, 65-65
src/pages/sponsors-global/page-templates/page-template-popup.js (1)
33-41: Strip client-only_tempIdbefore submit to avoid leaking local fields.
_tempIdis only for UI keys, but it’s currently sent in the payload. Consider removing it when buildingmodulesWithOrder.♻️ Suggested tweak
- const modulesWithOrder = values.modules.map((m, idx) => ({ - ...m, - custom_order: idx - })); + const modulesWithOrder = values.modules.map(({ _tempId, ...m }, idx) => ({ + ...m, + custom_order: idx + }));Also applies to: 122-128
| const hasValue = field?.value && field.value !== ""; | ||
| const shouldShrink = hasValue || Boolean(placeholder); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/mui/formik-inputs/mui-formik-select.jsRepository: fntechgit/summit-admin
Length of output: 2726
🏁 Script executed:
rg "MuiFormikSelect" --type jsx --type js -B 2 -A 5 | head -100Repository: fntechgit/summit-admin
Length of output: 93
🏁 Script executed:
rg "MuiFormikSelect" --type js -B 2 -A 5 | head -150Repository: fntechgit/summit-admin
Length of output: 13434
🏁 Script executed:
cat -n src/pages/sponsors/popup/edit-tier-popup.js | sed -n '1,200p'Repository: fntechgit/summit-admin
Length of output: 8124
🏁 Script executed:
cat -n src/pages/sponsors/popup/edit-tier-popup.js | sed -n '200,350p'Repository: fntechgit/summit-admin
Length of output: 7242
Handle falsy-but-valid values (0, false) correctly in the Select component.
The hasValue check and renderValue function use truthiness tests (&& and !), which incorrectly treat 0 and false as empty values. This causes the label to fail to shrink and the placeholder to render even when a valid falsy value is selected. Replace these with explicit checks for "", undefined, and null.
Suggested fix
- const hasValue = field?.value && field.value !== "";
+ const hasValue =
+ field.value !== "" && field.value !== undefined && field.value !== null;
...
- renderValue={(selected) => {
- if (!selected || selected === "") {
+ renderValue={(selected) => {
+ if (selected === "" || selected === undefined || selected === null) {
return <span style={{ color: "#aaa" }}>{placeholder}</span>;
}
return selected;
}}Also applies to: lines 48-51
🤖 Prompt for AI Agents
In `@src/components/mui/formik-inputs/mui-formik-select.js` around lines 29 - 31,
The hasValue/shouldShrink logic and the renderValue code treat falsy-but-valid
values like 0 and false as empty; update checks to explicitly compare against
"", undefined, and null instead of using truthiness. In the Select component
replace the current hasValue and shouldShrink (referenced as hasValue and
shouldShrink) with explicit checks: consider a value present when field.value
!== "" && field.value !== undefined && field.value !== null; likewise update
renderValue (the renderValue function/prop) to detect empty values by testing
value === "" || value === undefined || value === null and only return the
placeholder in those cases; apply the same explicit-empty checks to the code
referenced around lines 48-51 (the other render/placeholder branch).
| expect(showConfirmDialog).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| title: "general.are_you_sure", | ||
| type: "warning", | ||
| showCancelButton: true | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align confirm dialog expectation with the actual API (iconType).
Once the component uses iconType, this assertion should match.
🔧 Proposed update
expect.objectContaining({
title: "general.are_you_sure",
- type: "warning",
- showCancelButton: true
+ iconType: "warning"
})
);🤖 Prompt for AI Agents
In `@src/pages/sponsors-global/page-templates/page-template-module-form.test.js`
around lines 381 - 386, Update the test assertion for showConfirmDialog to match
the component's API change: replace the object key "type" with "iconType" while
keeping the same value ("warning") and other properties (title and
showCancelButton). Locate the expectation that calls
expect(showConfirmDialog).toHaveBeenCalledWith(expect.objectContaining(...)) in
the page-template-module-form.test and change the inner object to include
iconType: "warning" instead of type: "warning".
| const isConfirmed = await showConfirmDialog({ | ||
| title: T.translate("general.are_you_sure"), | ||
| text: T.translate("page_template_list.page_crud.module_remove_warning", { | ||
| name: moduleName | ||
| }), | ||
| type: "warning", | ||
| showCancelButton: true, | ||
| confirmButtonColor: "#DD6B55", | ||
| confirmButtonText: T.translate("general.yes_delete") | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use iconType—type/showCancelButton are ignored by showConfirmDialog.
showConfirmDialog destructures iconType, so the warning icon won’t show with type.
🐛 Proposed fix
const isConfirmed = await showConfirmDialog({
title: T.translate("general.are_you_sure"),
text: T.translate("page_template_list.page_crud.module_remove_warning", {
name: moduleName
}),
- type: "warning",
- showCancelButton: true,
+ iconType: "warning",
confirmButtonColor: "#DD6B55",
confirmButtonText: T.translate("general.yes_delete")
});🤖 Prompt for AI Agents
In `@src/pages/sponsors-global/page-templates/page-template-modules-form.js`
around lines 54 - 63, The confirm dialog call assigned to isConfirmed is passing
properties that are ignored; replace the type prop with iconType: "warning"
(since showConfirmDialog destructures iconType) and remove the showCancelButton
prop (it's ignored); update the showConfirmDialog invocation (the call where
isConfirmed is set) to use iconType instead of type so the warning icon displays
correctly.
| const handleRowEdit = (row) => { | ||
| getSponsorForm(row.id).then(() => { | ||
| setOpenPopup("new"); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“New page” flow is a no‑op right now.
Both the toolbar button and row edit set openPopup to "new", but the corresponding popup is commented out, so users won’t see anything. Either re-enable the popup (and import it) or disable/remove the action until it’s implemented.
Also applies to: 170-193, 223-226
🤖 Prompt for AI Agents
In `@src/pages/sponsors/sponsor-pages-list-page/index.js` around lines 69 - 73,
The "new" popup is not rendered because the popup component was commented out,
so handleRowEdit/getSponsorForm calling setOpenPopup("new") does nothing; either
restore the popup import and its JSX so it renders when openPopup === "new" (and
wire it to the sponsor form state fetched by getSponsorForm), or disable/remove
the toolbar button and row-edit handler that call setOpenPopup("new") (update
handleRowEdit and any toolbar onClick) so they no longer trigger the no-op;
apply the same fix to the other occurrences that toggle openPopup to "new".
| const handleRowDelete = (itemId) => { | ||
| console.log("DELETE ITEM ID...", itemId); | ||
| // deleteSponsorForm(itemId); | ||
| }; | ||
|
|
||
| const handleArchiveItem = (item) => console.log("archive ITEM...", item); | ||
| // item.is_archived | ||
| // ? unarchiveSponsorForm(item.id) | ||
| // : archiveSponsorForm(item.id); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete/archive actions are stubbed, leaving dead UI controls.
handleRowDelete and handleArchiveItem only log to console, but the table will still render action buttons. Wire the real actions or remove the handlers to avoid non-functional UI.
💡 Suggested stopgap (hide actions until implemented)
- const handleRowDelete = (itemId) => {
- console.log("DELETE ITEM ID...", itemId);
- // deleteSponsorForm(itemId);
- };
-
- const handleArchiveItem = (item) => console.log("archive ITEM...", item);
+ // TODO: wire delete/archive actions before enabling table actions- onDelete={handleRowDelete}
...
- onArchive={handleArchiveItem}Also applies to: 209-214
🤖 Prompt for AI Agents
In `@src/pages/sponsors/sponsor-pages-list-page/index.js` around lines 75 - 84,
handleRowDelete and handleArchiveItem are stubs that only log, leaving
non-functional action buttons; either wire them to the real API helpers or
remove/hide the buttons. Implement handleRowDelete to call
deleteSponsorForm(itemId) and handle success/error flows; implement
handleArchiveItem to call archiveSponsorForm(item.id) or
unarchiveSponsorForm(item.id) depending on item.is_archived and update UI state
accordingly; alternatively remove the action column or conditionally hide action
buttons until those functions (deleteSponsorForm, archiveSponsorForm,
unarchiveSponsorForm) are implemented to avoid dead controls.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
96f1b58 to
8739b2e
Compare
ref:
https://app.clickup.com/t/86b7ubdfg
https://app.clickup.com/t/86b79918r
https://app.clickup.com/t/86b79918z
https://app.clickup.com/t/86b7wedvv
https://app.clickup.com/t/86b7wee1f
https://app.clickup.com/t/86b7wee6u
https://app.clickup.com/t/86b7wee9d
Summary by CodeRabbit
New Features
Improvements
State & API
Translations & Config
Tests
✏️ Tip: You can customize this high-level summary in your review settings.