feat: sponsor pages global cont #764
Open
Conversation
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
📝 WalkthroughWalkthroughAdds a page-template cloning flow (action, UI popup, list integration), renames/refactors the select-dialog to support multi/single selection, updates thunk signatures and reducer payload shape, and adjusts numerous import paths and i18n entries. Changes
Sequence DiagramsequenceDiagram
actor User as User
participant List as PageTemplateList
participant Popup as PageTemplateClonePopup
participant Dialog as SelectPageTemplateDialog
participant Redux as Redux Action
participant API as API
participant Store as Store
User->>List: Click "Clone"
List->>Popup: open dialog
Popup->>Dialog: render template selector
User->>Dialog: select template(s) and confirm
Dialog->>Redux: dispatch clonePageTemplate(templateId)
Redux->>API: POST /page-templates/:id/clone
API-->>Redux: success
Redux->>Store: dispatch GLOBAL_PAGE_CLONED
Redux->>Store: dispatch getPageTemplates() (refresh)
Store-->>Popup: update / success
Popup->>User: show success snackbar and close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 1
🤖 Fix all issues with AI agents
In `@src/pages/sponsors-global/page-templates/page-template-clone-popup/index.js`:
- Around line 8-13: handleOnSave is passing the whole selection array to
clonePageTemplate but clonePageTemplate expects a single templateId; update
handleOnSave to extract a single id (e.g. first selected ID or destructured id)
and validate it before calling clonePageTemplate (for example: if
Array.isArray(template) use template[0] and ensure it's defined), then call
clonePageTemplate(singleId).finally(() => onClose()); this ensures the URL built
by clonePageTemplate receives a single ID.
🧹 Nitpick comments (2)
src/components/select-page-template-dialog/index.js (2)
197-200: AddisMultito propTypes for the new API.🧾 Suggested propTypes update
SelectPageTemplateDialog.propTypes = { onClose: PropTypes.func.isRequired, - onSave: PropTypes.func.isRequired + onSave: PropTypes.func.isRequired, + isMulti: PropTypes.bool };
66-75: Use functional state updaters for safer state management.While the current implementation works correctly in React 16.13 with single checkbox clicks, using functional updaters like
setSelectedRows((prev) => ...)is a best practice that ensures correct behavior regardless of batching scenarios or future React upgrades.♻️ Suggested refactor
if (isMulti) { if (checked) { - setSelectedRows([...selectedRows, rowId]); + setSelectedRows((prev) => (prev.includes(rowId) ? prev : [...prev, rowId])); } else { - setSelectedRows(selectedRows.filter((r) => r !== rowId)); + setSelectedRows((prev) => prev.filter((r) => r !== rowId)); } } else { // For single selection (radio), always set to the selected row setSelectedRows(checked ? [rowId] : []); }
Comment on lines
+8
to
+13
| const PageTemplateClonePopup = ({ open, onClose, clonePageTemplate }) => { | ||
| const handleOnSave = (template) => { | ||
| clonePageTemplate(template).finally(() => { | ||
| onClose(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Fix cloning payload: clonePageTemplate expects a single ID.
SelectPageTemplateDialog returns an array of selected IDs, but clonePageTemplate builds a URL using a single templateId. Passing an array will yield an invalid path.
🔧 Suggested fix
-const PageTemplateClonePopup = ({ open, onClose, clonePageTemplate }) => {
- const handleOnSave = (template) => {
- clonePageTemplate(template).finally(() => {
+const PageTemplateClonePopup = ({ open, onClose, clonePageTemplate }) => {
+ const handleOnSave = (selected) => {
+ const templateId = Array.isArray(selected) ? selected[0] : selected;
+ if (!templateId) return;
+ clonePageTemplate(templateId).finally(() => {
onClose();
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const PageTemplateClonePopup = ({ open, onClose, clonePageTemplate }) => { | |
| const handleOnSave = (template) => { | |
| clonePageTemplate(template).finally(() => { | |
| onClose(); | |
| }); | |
| }; | |
| const PageTemplateClonePopup = ({ open, onClose, clonePageTemplate }) => { | |
| const handleOnSave = (selected) => { | |
| const templateId = Array.isArray(selected) ? selected[0] : selected; | |
| if (!templateId) return; | |
| clonePageTemplate(templateId).finally(() => { | |
| onClose(); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In `@src/pages/sponsors-global/page-templates/page-template-clone-popup/index.js`
around lines 8 - 13, handleOnSave is passing the whole selection array to
clonePageTemplate but clonePageTemplate expects a single templateId; update
handleOnSave to extract a single id (e.g. first selected ID or destructured id)
and validate it before calling clonePageTemplate (for example: if
Array.isArray(template) use template[0] and ensure it's defined), then call
clonePageTemplate(singleId).finally(() => onClose()); this ensures the URL built
by clonePageTemplate receives a single ID.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@src/components/select-page-template-dialog/__tests__/select-page-template-dialog.test.js`:
- Around line 344-355: The test "Load more functionality" only asserts the
initial call to getPageTemplates and doesn't trigger loadMoreData on the
MuiInfiniteTable; either update the test to mock the MuiInfiniteTable component
so it calls the provided loadMoreData prop (causing a second call to
getPageTemplates) and then assert getPageTemplates was called twice, or change
the test name to reflect it only verifies the initial fetch; locate the test in
select-page-template-dialog.test.js and adjust the mock for MuiInfiniteTable (or
the assertion) accordingly, referencing MuiInfiniteTable, loadMoreData,
getPageTemplates and renderWithStore.
- Around line 358-370: The "Sorting functionality" test currently only asserts
the initial mount call and doesn't simulate user sorting, so update the test in
select-page-template-dialog.test.js: either simulate a sort interaction on the
MuiInfiniteTable (e.g., trigger a sort change event or call the table's
onSort/onChange handler after renderWithStore) and then assert that
getPageTemplates was called again with the new sort field and direction, or
remove/rename the test to reflect that it only checks initial sort params;
target symbols to change: the test block named "Sorting functionality", the test
"calls getPageTemplates with sort parameters", the renderWithStore helper, and
the mocked getPageTemplates from page-template-actions.
- Around line 326-341: The test "resets selected rows when closing" currently
clicks a checkbox and closes the dialog but never asserts that selection was
cleared; update the test (the one using renderWithStore, checkboxes and
closeButton) to assert the selection count after closing—for example verify that
the "1 items selected" message is gone or that the UI shows "0 items selected"
(use screen.queryByText or getByText as appropriate) immediately after calling
the closeButton click and after expecting onClose toHaveBeenCalled.
🧹 Nitpick comments (2)
src/components/select-page-template-dialog/__tests__/select-page-template-dialog.test.js (2)
112-120: Avoid hard-coding the page size in expectations.This couples the test to
10even if the component’s per-page constant changes. Consider importing the shared constant (if available) or loosening the assertion to avoid brittle failures.
128-133: Prefer querying the close button by an accessible name.Using
name: ""masks missing aria-labels and can break if the label is added. Consider asserting a label (e.g., “Close”) and updating the component if needed.
Comment on lines
+326
to
+341
| test("resets selected rows when closing", async () => { | ||
| const onClose = jest.fn(); | ||
| renderWithStore({ isMulti: true, onClose }); | ||
|
|
||
| const checkboxes = screen.getAllByRole("checkbox"); | ||
|
|
||
| // Select an item | ||
| await userEvent.click(checkboxes[0]); | ||
| expect(screen.getByText("1 items selected")).toBeInTheDocument(); | ||
|
|
||
| // Close dialog | ||
| const closeButton = screen.getByRole("button", { name: "" }); | ||
| await userEvent.click(closeButton); | ||
|
|
||
| expect(onClose).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The “resets selected rows” test doesn’t assert the reset.
Add an assertion for the selection count after closing to verify the behavior.
Suggested fix
const closeButton = screen.getByRole("button", { name: "" });
await userEvent.click(closeButton);
expect(onClose).toHaveBeenCalled();
+ expect(screen.getByText("0 items selected")).toBeInTheDocument();🤖 Prompt for AI Agents
In
`@src/components/select-page-template-dialog/__tests__/select-page-template-dialog.test.js`
around lines 326 - 341, The test "resets selected rows when closing" currently
clicks a checkbox and closes the dialog but never asserts that selection was
cleared; update the test (the one using renderWithStore, checkboxes and
closeButton) to assert the selection count after closing—for example verify that
the "1 items selected" message is gone or that the UI shows "0 items selected"
(use screen.queryByText or getByText as appropriate) immediately after calling
the closeButton click and after expecting onClose toHaveBeenCalled.
Comment on lines
+344
to
+355
| describe("Load more functionality", () => { | ||
| test("calls getPageTemplates to load more when scrolling", () => { | ||
| const { | ||
| getPageTemplates | ||
| } = require("../../../actions/page-template-actions"); | ||
| renderWithStore({}, { total: 20 }); // More items available than displayed | ||
|
|
||
| // The component should be rendered with loadMoreData available | ||
| // This would typically be triggered by scrolling in the MuiInfiniteTable | ||
| // The test verifies the component is set up with the correct total | ||
| expect(getPageTemplates).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
Load‑more test doesn’t exercise load‑more.
Right now it only checks the initial fetch. Consider mocking MuiInfiniteTable to invoke loadMoreData and assert an additional getPageTemplates call, or rename the test to match what it actually verifies.
🤖 Prompt for AI Agents
In
`@src/components/select-page-template-dialog/__tests__/select-page-template-dialog.test.js`
around lines 344 - 355, The test "Load more functionality" only asserts the
initial call to getPageTemplates and doesn't trigger loadMoreData on the
MuiInfiniteTable; either update the test to mock the MuiInfiniteTable component
so it calls the provided loadMoreData prop (causing a second call to
getPageTemplates) and then assert getPageTemplates was called twice, or change
the test name to reflect it only verifies the initial fetch; locate the test in
select-page-template-dialog.test.js and adjust the mock for MuiInfiniteTable (or
the assertion) accordingly, referencing MuiInfiniteTable, loadMoreData,
getPageTemplates and renderWithStore.
Comment on lines
+358
to
+370
| describe("Sorting functionality", () => { | ||
| test("calls getPageTemplates with sort parameters", () => { | ||
| const { | ||
| getPageTemplates | ||
| } = require("../../../actions/page-template-actions"); | ||
| renderWithStore(); | ||
|
|
||
| // Initial call on mount | ||
| expect(getPageTemplates).toHaveBeenCalledWith("", 1, 10, "id", 1, true); | ||
|
|
||
| // The actual sort interaction would be handled by MuiInfiniteTable | ||
| // This test verifies the component is initialized with correct sort params | ||
| }); |
There was a problem hiding this comment.
Sorting test doesn’t trigger sorting.
Either simulate a sort via the table (and assert a new getPageTemplates call) or rename/remove the test to avoid a false sense of coverage.
🤖 Prompt for AI Agents
In
`@src/components/select-page-template-dialog/__tests__/select-page-template-dialog.test.js`
around lines 358 - 370, The "Sorting functionality" test currently only asserts
the initial mount call and doesn't simulate user sorting, so update the test in
select-page-template-dialog.test.js: either simulate a sort interaction on the
MuiInfiniteTable (e.g., trigger a sort change event or call the table's
onSort/onChange handler after renderWithStore) and then assert that
getPageTemplates was called again with the new sort field and direction, or
remove/rename the test to reflect that it only checks initial sort params;
target symbols to change: the test block named "Sorting functionality", the test
"calls getPageTemplates with sort parameters", the renderWithStore helper, and
the mocked getPageTemplates from page-template-actions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://app.clickup.com/t/86b7u3q3c
https://app.clickup.com/t/86b79917u
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.