Open
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…oad tables after save Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds single-managed-page fetch and update thunks, splits create vs update save flows with a new normalize-to-customize payload, surfaces a “Customize” UI flow and popup title prop, updates i18n keys, and extends tests to cover the customize flow. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as "Sponsor Pages UI"
participant Redux as "Actions / Thunks"
participant API as "API Server"
participant Store as "Redux Store"
User->>UI: Click "Customize" on managed page
UI->>Redux: dispatch getSponsorManagedPage(pageId)
Redux->>API: GET /show-pages/{pageId}?expand=modules
API-->>Redux: managed page + modules
Redux->>Store: dispatch RECEIVE_SPONSOR_MANAGED_PAGE
Store-->>UI: currentEditPage (deadlines converted)
UI->>User: open managedPagePopup with data
User->>UI: Edit and Save
UI->>Redux: dispatch saveSponsorManagedPage(entity)
Redux->>Redux: normalizeSponsorManagedPageToCustomize(entity)
Redux->>API: PUT /show-pages/{pageId}
API-->>Redux: success
Redux->>Store: dispatch SPONSOR_CUSTOMIZED_PAGE_UPDATED
Redux->>Redux: dispatch getSponsorManagedPages & getSponsorCustomizedPages (refresh)
UI->>UI: close popup and refresh lists
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/sponsor-pages-actions.js (1)
282-293:⚠️ Potential issue | 🔴 Critical
normalizeSponsorManagedPagenow throws before the create request.Line 287 calls
includesonnormalizedEntity.allowed_add_ons, but that field is never initialized here. The “add managed page from template” path will fail with aTypeErrorbefore the POST is sent.Proposed fix
const normalizeSponsorManagedPage = (entity) => { const normalizedEntity = { show_page_ids: entity.pages }; + const selectedAddOns = entity.add_ons ?? []; - if (normalizedEntity.allowed_add_ons.includes("all")) { + if (selectedAddOns.includes("all")) { normalizedEntity.apply_to_all_add_ons = true; normalizedEntity.allowed_add_ons = []; } else { - normalizedEntity.allowed_add_ons = entity.add_ons.map((a) => a.id); + normalizedEntity.allowed_add_ons = selectedAddOns.map((a) => a.id ?? a); normalizedEntity.apply_to_all_add_ons = false; } return normalizedEntity; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-pages-actions.js` around lines 282 - 293, normalizeSponsorManagedPage currently calls normalizedEntity.allowed_add_ons.includes(...) before allowed_add_ons is initialized; fix by initializing allowed_add_ons from entity.add_ons (e.g., normalizedEntity.allowed_add_ons = (entity.add_ons || []).map(a => a.id)) before the if-check, then perform the includes("all") test and set apply_to_all_add_ons and allowed_add_ons (to [] when "all" is present) accordingly; ensure you handle a missing/null entity.add_ons by defaulting to an empty array so the add-managed-page flow doesn’t throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/actions/sponsor-pages-actions.js`:
- Around line 298-305: The call to normalizeSelectAllField inside
normalizeSponsorManagedPageToCustomize is passing the wrong flag name
("apply_to_all_types"), causing the companion flag for allowed_add_ons to be
emitted incorrectly; change the third argument to "apply_to_all_add_ons" so
normalizeSelectAllField(serialization of allowed_add_ons) writes the expected
apply_to_all_add_ons flag (update the call in
normalizeSponsorManagedPageToCustomize that currently references
allowed_add_ons).
In `@src/pages/sponsors/sponsor-pages-tab/index.js`:
- Around line 274-292: The popup is being closed in the .finally block causing
edits to be lost on save failure and skipping cleanup; change the flow in
handleSaveManagedPage so setOpenPopup(null) is called only on successful save
(inside the .then chain after refresh calls) and also invoke
handleClosePagePopup() there to clear currentEditPage, while leaving the popup
open on .catch so the user can correct errors; reference saveSponsorManagedPage,
getSponsorManagedPages, getSponsorCustomizedPages, setOpenPopup, and
handleClosePagePopup when making this change.
---
Outside diff comments:
In `@src/actions/sponsor-pages-actions.js`:
- Around line 282-293: normalizeSponsorManagedPage currently calls
normalizedEntity.allowed_add_ons.includes(...) before allowed_add_ons is
initialized; fix by initializing allowed_add_ons from entity.add_ons (e.g.,
normalizedEntity.allowed_add_ons = (entity.add_ons || []).map(a => a.id)) before
the if-check, then perform the includes("all") test and set apply_to_all_add_ons
and allowed_add_ons (to [] when "all" is present) accordingly; ensure you handle
a missing/null entity.add_ons by defaulting to an empty array so the
add-managed-page flow doesn’t throw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76ed2285-9141-418e-bda7-94bd694fbb28
📒 Files selected for processing (4)
src/actions/sponsor-pages-actions.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-pages-tab/index.jssrc/reducers/sponsors/sponsor-page-pages-list-reducer.js
Comment on lines
+274
to
+292
| const handleSaveManagedPage = (entity) => { | ||
| saveSponsorManagedPage(entity) | ||
| .then(() => { | ||
| getSponsorManagedPages( | ||
| term, | ||
| DEFAULT_CURRENT_PAGE, | ||
| managedPages.perPage, | ||
| managedPages.order, | ||
| managedPages.orderDir | ||
| ); | ||
| getSponsorCustomizedPages( | ||
| term, | ||
| DEFAULT_CURRENT_PAGE, | ||
| customizedPages.perPage, | ||
| customizedPages.order, | ||
| customizedPages.orderDir | ||
| ); | ||
| }) | ||
| .finally(() => setOpenPopup(null)); |
There was a problem hiding this comment.
Don't close the managed-page editor in finally.
Line 292 closes the dialog even when saveSponsorManagedPage fails, which drops the user's edits. It also skips handleClosePagePopup, so currentEditPage stays populated and can leak into the next “New Page” open.
Proposed fix
const handleSaveManagedPage = (entity) => {
saveSponsorManagedPage(entity)
.then(() => {
getSponsorManagedPages(
term,
DEFAULT_CURRENT_PAGE,
managedPages.perPage,
managedPages.order,
managedPages.orderDir
);
getSponsorCustomizedPages(
term,
DEFAULT_CURRENT_PAGE,
customizedPages.perPage,
customizedPages.order,
customizedPages.orderDir
);
- })
- .finally(() => setOpenPopup(null));
+ handleClosePagePopup();
+ });
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors/sponsor-pages-tab/index.js` around lines 274 - 292, The
popup is being closed in the .finally block causing edits to be lost on save
failure and skipping cleanup; change the flow in handleSaveManagedPage so
setOpenPopup(null) is called only on successful save (inside the .then chain
after refresh calls) and also invoke handleClosePagePopup() there to clear
currentEditPage, while leaving the popup open on .catch so the user can correct
errors; reference saveSponsorManagedPage, getSponsorManagedPages,
getSponsorCustomizedPages, setOpenPopup, and handleClosePagePopup when making
this change.
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/sponsors/sponsor-pages-tab/index.js (1)
199-200: Remove the temporary console log.
console.log("CHECK!", item)will run on every Customize click and dump the row payload into the browser console.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-pages-tab/index.js` around lines 199 - 200, Remove the temporary console.log("CHECK!", item) that prints the row payload on every Customize click; inside the click handler where getSponsorManagedPage(item.id).then(() => setOpenPopup("managedPagePopup")) is invoked, delete the console.log statement (or replace it with a controlled debug/logging call if persistent logging is required) so only getSponsorManagedPage and setOpenPopup remain.src/pages/sponsors/sponsor-pages-tab/__tests__/sponsor-pages-tab.test.js (1)
305-307: Assert the refresh arguments, not only the call counts.These expectations still pass if the save handler rebuilds either grid with the wrong filters or pagination. Verifying the last
getSponsorManagedPages/getSponsorCustomizedPagesarguments here would catch the missinghideArchivedregression inhandleSaveManagedPage.Example assertion upgrade
await waitFor(() => { - expect(getSponsorManagedPages).toHaveBeenCalledTimes(2); - expect(getSponsorCustomizedPages).toHaveBeenCalledTimes(2); + expect(getSponsorManagedPages).toHaveBeenLastCalledWith( + "", + 1, + 10, + "id", + 1, + false + ); + expect(getSponsorCustomizedPages).toHaveBeenLastCalledWith( + "", + 1, + 10, + "id", + 1, + false + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-pages-tab/__tests__/sponsor-pages-tab.test.js` around lines 305 - 307, The test currently only checks call counts for getSponsorManagedPages and getSponsorCustomizedPages; change it to assert the last call arguments to ensure the correct filters/pagination are passed (e.g., verify that the last call to getSponsorManagedPages and getSponsorCustomizedPages includes the expected hideArchived flag and other filter/pagination params), and update the assertions around the save flow (handleSaveManagedPage) to use toHaveBeenLastCalledWith-style checks or expect.objectContaining on the last call args rather than relying solely on toHaveBeenCalledTimes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/sponsors/sponsor-pages-tab/index.js`:
- Around line 279-292: The post-save refresh calls to getSponsorManagedPages and
getSponsorCustomizedPages are missing the current hideArchived flag, causing
grids to repopulate incorrectly when "Hide archived" is enabled; update the two
calls in the post-save refresh to pass the appropriate hideArchived value (e.g.,
managedPages.hideArchived and customizedPages.hideArchived or the shared
hideArchived state) as an additional parameter to getSponsorManagedPages(...)
and getSponsorCustomizedPages(...) so the refresh respects the archived filter.
---
Nitpick comments:
In `@src/pages/sponsors/sponsor-pages-tab/__tests__/sponsor-pages-tab.test.js`:
- Around line 305-307: The test currently only checks call counts for
getSponsorManagedPages and getSponsorCustomizedPages; change it to assert the
last call arguments to ensure the correct filters/pagination are passed (e.g.,
verify that the last call to getSponsorManagedPages and
getSponsorCustomizedPages includes the expected hideArchived flag and other
filter/pagination params), and update the assertions around the save flow
(handleSaveManagedPage) to use toHaveBeenLastCalledWith-style checks or
expect.objectContaining on the last call args rather than relying solely on
toHaveBeenCalledTimes.
In `@src/pages/sponsors/sponsor-pages-tab/index.js`:
- Around line 199-200: Remove the temporary console.log("CHECK!", item) that
prints the row payload on every Customize click; inside the click handler where
getSponsorManagedPage(item.id).then(() => setOpenPopup("managedPagePopup")) is
invoked, delete the console.log statement (or replace it with a controlled
debug/logging call if persistent logging is required) so only
getSponsorManagedPage and setOpenPopup remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35d93b55-7a8e-486d-b8ac-1a682644832e
📒 Files selected for processing (4)
src/i18n/en.jsonsrc/pages/sponsors-global/page-templates/page-template-popup/index.jssrc/pages/sponsors/sponsor-pages-tab/__tests__/sponsor-pages-tab.test.jssrc/pages/sponsors/sponsor-pages-tab/index.js
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/actions/sponsor-pages-actions.js (1)
282-293:⚠️ Potential issue | 🔴 CriticalRestore the
add_onssource for the “all add-ons” check.
normalizedEntityonly containsshow_page_idshere, so Line 287 dereferencesundefined.includes("all"). Every create save will throw before the POST. Read fromentity.add_ons(or initializenormalizedEntity.allowed_add_onsfirst) before mapping IDs.Suggested fix
- if (normalizedEntity.allowed_add_ons.includes("all")) { + if (entity.add_ons.includes("all")) { normalizedEntity.apply_to_all_add_ons = true; normalizedEntity.allowed_add_ons = []; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-pages-actions.js` around lines 282 - 293, In normalizeSponsorManagedPage, the code checks normalizedEntity.allowed_add_ons.includes("all") before allowed_add_ons is set, causing a crash; change the logic to read from entity.add_ons (or initialize normalizedEntity.allowed_add_ons first) — e.g., build allowed_add_ons from entity.add_ons (mapping to ids) or set normalizedEntity.allowed_add_ons = entity.add_ons.map(a => a.id) before the includes("all") check, then set apply_to_all_add_ons and clear allowed_add_ons when "all" is present; update the branches inside normalizeSponsorManagedPage to reference entity.add_ons or the initialized normalizedEntity.allowed_add_ons consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/actions/sponsor-pages-actions.js`:
- Around line 282-293: In normalizeSponsorManagedPage, the code checks
normalizedEntity.allowed_add_ons.includes("all") before allowed_add_ons is set,
causing a crash; change the logic to read from entity.add_ons (or initialize
normalizedEntity.allowed_add_ons first) — e.g., build allowed_add_ons from
entity.add_ons (mapping to ids) or set normalizedEntity.allowed_add_ons =
entity.add_ons.map(a => a.id) before the includes("all") check, then set
apply_to_all_add_ons and clear allowed_add_ons when "all" is present; update the
branches inside normalizeSponsorManagedPage to reference entity.add_ons or the
initialized normalizedEntity.allowed_add_ons consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1092a026-c347-4b5d-8b5c-4f658b2517ed
📒 Files selected for processing (1)
src/actions/sponsor-pages-actions.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b7991c7
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests