feat: add edit popup on sponsor pages list #770
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.githubusercontent.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.
| async (dispatch, getState) => { | ||
| const { currentSummitState } = getState(); | ||
| const { currentSummit } = currentSummitState; | ||
| const summitTZ = currentSummit.time_zone.name; |
There was a problem hiding this comment.
Guard against missing summit time zone before dereferencing.
currentSummit or time_zone can be null during initial load; this would throw and break the list fetch. Add a safe fallback.
🛠️ Suggested fix
- const { currentSummit } = currentSummitState;
- const summitTZ = currentSummit.time_zone.name;
+ const { currentSummit } = currentSummitState || {};
+ const summitTZ = currentSummit?.time_zone?.name ?? "UTC";Also applies to: 98-98
🤖 Prompt for AI Agents
In `@src/actions/page-template-actions.js` around lines 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.
src/actions/sponsor-pages-actions.js
Outdated
| export const getSponsorPage = (pageId) => async (dispatch, getState) => { | ||
| const { currentSummitState } = getState(); | ||
| const { currentSummit } = currentSummitState; | ||
| const accessToken = await getAccessTokenSafely(); | ||
|
|
||
| dispatch(startLoading()); | ||
|
|
||
| const params = { | ||
| access_token: accessToken, | ||
| expand: "modules.file_type" | ||
| }; | ||
|
|
||
| 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()); | ||
| }); | ||
| }; |
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
| 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; |
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.
| const normalizeModules = (modules = [], summitTZ = "UTC") => | ||
| modules.map((m) => { | ||
| if (m.kind === PAGES_MODULE_KINDS.MEDIA) { | ||
| const normalizeModule = { ...m }; | ||
| if (m.upload_deadline) { | ||
| normalizeModule.upload_deadline = epochToMomentTimeZone( | ||
| m.upload_deadline, | ||
| summitTZ | ||
| ); | ||
| } | ||
| if (m.file_type) { | ||
| normalizeModule.file_type_id = { | ||
| value: m.file_type.id, | ||
| label: `${m.file_type.name} (${m.file_type.allowed_extensions})` | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check the actual code at the specified lines
cat -n "src/pages/sponsors-global/page-templates/page-template-popup.js" | sed -n '110,140p'Repository: fntechgit/summit-admin
Length of output: 1121
🏁 Script executed:
# Search for how normalizeModules is called throughout the codebase
rg "normalizeModules\(" --max-count=20Repository: fntechgit/summit-admin
Length of output: 200
🏁 Script executed:
# Find the epochToMomentTimeZone function definition
rg "epochToMomentTimeZone" -A 5 --max-count=10Repository: fntechgit/summit-admin
Length of output: 50378
🏁 Script executed:
# Find where summitTZ is defined/used in the page-template-popup.js file
grep -n "summitTZ" "src/pages/sponsors-global/page-templates/page-template-popup.js" | head -20Repository: fntechgit/summit-admin
Length of output: 246
🏁 Script executed:
# Look at the component definition and props to understand summitTZ source
sed -n '1,150p' "src/pages/sponsors-global/page-templates/page-template-popup.js" | grep -A 5 -B 5 "summitTZ\|export\|const\|function"Repository: fntechgit/summit-admin
Length of output: 3546
🏁 Script executed:
# Find parent components that use PageTemplatePopup to see how summitTZ is passed
rg "PageTemplatePopup" --max-count=10Repository: fntechgit/summit-admin
Length of output: 817
🏁 Script executed:
# Check if there are any null checks or guards for summitTZ elsewhere in similar components
rg "summitTZ\s*\|\|" --max-count=10Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check how summitTZ is passed in page-template-list-page.js
grep -A 10 "PageTemplatePopup" "src/pages/sponsors-global/page-templates/page-template-list-page.js" | head -20Repository: fntechgit/summit-admin
Length of output: 464
🏁 Script executed:
# Check how summitTZ is passed in sponsor-pages-list-page
grep -A 10 "PageTemplatePopup" "src/pages/sponsors/sponsor-pages-list-page/index.js" | head -20Repository: fntechgit/summit-admin
Length of output: 501
🏁 Script executed:
# Check where summitTZ comes from in page-template-list-page.js
grep -B 20 "summitTZ={summitTZ}" "src/pages/sponsors-global/page-templates/page-template-list-page.js" | grep -E "summitTZ|const|useState|mapStateToProps"Repository: fntechgit/summit-admin
Length of output: 93
🏁 Script executed:
# Check the component definition and props for sponsor-pages-list-page to see why summitTZ is missing
head -100 "src/pages/sponsors/sponsor-pages-list-page/index.js" | tail -80Repository: fntechgit/summit-admin
Length of output: 1915
🏁 Script executed:
# Check if summitTZ is ever set to null anywhere in the component files
rg "summitTZ\s*=\s*null" --max-count=5Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check the mapStateToProps for page-template-list-page to see how summitTZ is sourced
grep -A 30 "mapStateToProps\|const.*summitTZ" "src/pages/sponsors-global/page-templates/page-template-list-page.js" | head -40Repository: fntechgit/summit-admin
Length of output: 332
🏁 Script executed:
# Find the page template list reducer to see what's in the state
fd "page-template-list" -t f | grep -i reducerRepository: fntechgit/summit-admin
Length of output: 127
🏁 Script executed:
# Search for where pageTemplateListState is defined/reduced
rg "pageTemplateListState|PAGE_TEMPLATE" --type js -l | head -5Repository: fntechgit/summit-admin
Length of output: 334
🏁 Script executed:
# Check the page-template-list-reducer to see the state structure and how summitTZ is set
cat -n "src/reducers/sponsors_inventory/page-template-list-reducer.js" | head -60Repository: fntechgit/summit-admin
Length of output: 2231
🏁 Script executed:
# Continue viewing the reducer to find where summitTZ comes from
cat -n "src/reducers/sponsors_inventory/page-template-list-reducer.js" | tail -60Repository: fntechgit/summit-admin
Length of output: 2221
🏁 Script executed:
# Also search for where summitTZ is imported or defined in store or root reducer
grep -r "summitTZ" "src/store.js" "src/reducers/index.js" 2>/dev/null | head -10Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check the full page-template-list-page.js to see where summitTZ comes from
cat -n "src/pages/sponsors-global/page-templates/page-template-list-page.js"Repository: fntechgit/summit-admin
Length of output: 9102
🏁 Script executed:
# Check the root reducer to see what state is available
cat -n "src/reducers/index.js" | head -80Repository: fntechgit/summit-admin
Length of output: 119
🏁 Script executed:
# Also check store.js to see the full state structure
cat -n "src/store.js"Repository: fntechgit/summit-admin
Length of output: 24864
🏁 Script executed:
# Check the currentSummitReducer to see if it has summitTZ
grep -n "summitTZ\|time_zone" "src/reducers/summits/current-summit-reducer.js" | head -20Repository: fntechgit/summit-admin
Length of output: 137
🏁 Script executed:
# Look at other pages that use summitTZ to see how they source it
grep -B 5 "summitTZ" "src/pages/sponsors/sponsor-pages-list-page/index.js" | head -20Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check if pageTemplateListReducer should have summitTZ by checking other similar list reducers
grep -l "summitTZ" "src/reducers/sponsors"/*.js | head -3Repository: fntechgit/summit-admin
Length of output: 228
🏁 Script executed:
# Get the actual state props passed to PageTemplateListPage by checking the full component
grep -A 50 "const PageTemplateListPage" "src/pages/sponsors-global/page-templates/page-template-list-page.js" | grep -E "summitTZ|currentSummit"Repository: fntechgit/summit-admin
Length of output: 76
🏁 Script executed:
# Check if summitTZ might come from currentSummitState instead
grep "currentSummit.*time_zone\|summitTZ" "src/reducers/summits/current-summit-reducer.js" | head -10Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check what summitTZ should actually be in sponsor-page-forms-list-reducer
grep -A 30 "DEFAULT_STATE\|summitTZ" "src/reducers/sponsors/sponsor-page-forms-list-reducer.js" | head -50Repository: fntechgit/summit-admin
Length of output: 1075
🏁 Script executed:
# Verify if PageTemplateListPage is actually receiving summitTZ correctly from Redux
grep "mapStateToProps\|summitTZ" "src/pages/sponsors-global/page-templates/page-template-list-page.js"Repository: fntechgit/summit-admin
Length of output: 204
🏁 Script executed:
# Check how summitTZ gets into sponsor-page-forms-list-reducer's REQUEST action
grep -B 10 "REQUEST_SPONSOR_MANAGED_FORMS\|getSponsorPages" "src/pages/sponsors-global/page-templates/page-template-list-page.js" | head -20Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check the actions to see if summitTZ is being passed to getPageTemplates
grep -A 20 "export.*getPageTemplates" "src/actions/page-template-actions.js" | head -30Repository: fntechgit/summit-admin
Length of output: 689
🏁 Script executed:
# Check if summitTZ is dispatched to pageTemplateListReducer
grep -n "pageTemplateListState\|REQUEST_PAGE_TEMPLATES\|RECEIVE_PAGE_TEMPLATES" "src/actions/page-template-actions.js" | head -20Repository: fntechgit/summit-admin
Length of output: 293
🏁 Script executed:
# Check the full action to see what's dispatched
cat -n "src/actions/page-template-actions.js" | head -80Repository: fntechgit/summit-admin
Length of output: 3355
Pass the timezone from state to avoid relying on default parameters.
The summitTZ prop passed to PageTemplatePopup is currently undefined because it's not included in pageTemplateListState. While the default parameter "UTC" catches undefined, this is fragile—it relies on accident rather than intent. For consistency with similar components (e.g., sponsor-page-forms-list-reducer), add summitTZ to pageTemplateListReducer's state so it's properly tracked and passed down, or explicitly coalesce it at the call site to make the behavior intentional.
🤖 Prompt for AI Agents
In `@src/pages/sponsors-global/page-templates/page-template-popup.js` around lines
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.
| const handleRowEdit = (row) => { | ||
| getSponsorForm(row.id).then(() => { | ||
| getSponsorPage(row.id).then(() => { | ||
| setOpenPopup("new"); | ||
| }); |
There was a problem hiding this comment.
Edit vs. new mode can overwrite existing pages.
Both “edit” and “new” open the same popup state and reuse currentSponsorPage, so clicking “new” after an edit can retain id and update instead of creating. Separate modes or clear the template for new.
🛠️ Suggested fix
- const [openPopup, setOpenPopup] = useState(null);
+ const [popupMode, setPopupMode] = useState(null);
const handleRowEdit = (row) => {
getSponsorPage(row.id).then(() => {
- setOpenPopup("new");
+ setPopupMode("edit");
});
};
...
- onClick={() => setOpenPopup("new")}
+ onClick={() => setPopupMode("new")}
...
- <GlobalPagePopup
- open={openPopup === "clone"}
- onClose={() => setOpenPopup(null)}
- />
+ <GlobalPagePopup
+ open={popupMode === "clone"}
+ onClose={() => setPopupMode(null)}
+ />
<PageTemplatePopup
- open={openPopup === "new"}
- pageTemplate={currentSponsorPage}
- onClose={() => setOpenPopup(null)}
+ open={popupMode === "new" || popupMode === "edit"}
+ pageTemplate={popupMode === "edit" ? currentSponsorPage : null}
+ onClose={() => setPopupMode(null)}
onSave={handleSaveSponsorPage}
/>Also applies to: 232-237
🤖 Prompt for AI Agents
In `@src/pages/sponsors/sponsor-pages-list-page/index.js` around lines 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.
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 };
| <PageTemplatePopup | ||
| open={openPopup === "new"} | ||
| onClose={() => setOpenPopup(null)} | ||
| /> */} | ||
| pageTemplate={currentSponsorPage} | ||
| onClose={handleTemplatePopupClose} | ||
| onSave={handleSaveSponsorPage} | ||
| /> |
There was a problem hiding this comment.
Pass summitTZ into PageTemplatePopup for correct timezone conversion.
The popup now supports timezone-aware normalization but isn’t receiving a TZ here. Since summitTZ is in state, pass it through (with a safe fallback).
🛠️ Suggested fix
-const SponsorPagesListPage = ({
+const SponsorPagesListPage = ({
sponsorPages,
@@
- currentSponsorPage,
+ currentSponsorPage,
+ summitTZ,
@@
- <PageTemplatePopup
+ <PageTemplatePopup
open={openPopup === "new"}
pageTemplate={currentSponsorPage}
+ summitTZ={summitTZ ?? "UTC"}
onClose={handleTemplatePopupClose}
onSave={handleSaveSponsorPage}
/>🤖 Prompt for AI Agents
In `@src/pages/sponsors/sponsor-pages-list-page/index.js` around lines 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.
src/actions/sponsor-pages-actions.js
Outdated
| }); | ||
| }; | ||
|
|
||
| export const getSponsorPage = (pageId) => async (dispatch, getState) => { |
There was a problem hiding this comment.
@tomrndom this is not a sponsor page its a show page please rename the action to
getShowPage and move to a new action file
actions/show-pages-actions.js
| import GlobalPagePopup from "./components/global-page/global-page-popup"; | ||
| import PageTemplatePopup from "../../sponsors-global/page-templates/page-template-popup"; | ||
|
|
||
| const SponsorPagesListPage = ({ |
There was a problem hiding this comment.
this should be renamed to ShowPagesListPage
refactor the file structure accordingly
| summitTZ: null | ||
| }; | ||
|
|
||
| const sponsorPagesListReducer = (state = DEFAULT_STATE, action) => { |
There was a problem hiding this comment.
this should be renamed to
showPagesListReducer
refactor file structure accordingly
src/actions/sponsor-pages-actions.js
Outdated
| }); | ||
| }; | ||
|
|
||
| const normalizeSponsorPage = (entity) => { |
There was a problem hiding this comment.
this should be renamed to normalizeShowPage
and move to a new action file
actions/show-pages-actions.js
src/actions/sponsor-pages-actions.js
Outdated
| return normalizedEntity; | ||
| }; | ||
|
|
||
| export const saveSponsorPage = (entity) => async (dispatch, getState) => { |
There was a problem hiding this comment.
this should be renamed to saveShowPage and move to a new action file
actions/show-pages-actions.js
src/actions/sponsor-pages-actions.js
Outdated
| ); | ||
| }) | ||
| .catch((err) => { | ||
| console.error(err); |
There was a problem hiding this comment.
dont do this errors should be properly managed
src/actions/sponsor-pages-actions.js
Outdated
| ); | ||
| }) | ||
| .catch((err) => { | ||
| console.error(err); |
src/actions/sponsor-pages-actions.js
Outdated
| }); | ||
| }; | ||
|
|
||
| export const resetSponsorPageForm = () => (dispatch) => { |
There was a problem hiding this comment.
this should be renamed to
resetShowPageForm and move to a new action file
actions/show-pages-actions.js
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.
2 participants
ref: https://app.clickup.com/t/86b79919e
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Updates