feat: move event category groups to mui - #978
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughEvent category group listing now supports search, pagination, and sorting through parameterized fetching, reducer-backed query state, and a hooks-based page using MUI table controls. List translations were also updated. ChangesEvent Category Groups List Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant EventCategoryGroupListPage
participant getEventCategoryGroups
participant event_category_group_list_reducer as event-category-group-list-reducer
participant MuiTable
EventCategoryGroupListPage->>getEventCategoryGroups: request term, page, perPage, order, orderDir
getEventCategoryGroups->>event_category_group_list_reducer: dispatch request and receive actions
event_category_group_list_reducer->>EventCategoryGroupListPage: provide rows and pagination state
EventCategoryGroupListPage->>MuiTable: render rows and controls
MuiTable->>EventCategoryGroupListPage: invoke search, sort, page, and delete callbacks
EventCategoryGroupListPage->>getEventCategoryGroups: refresh with updated query
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/pages/events/event-category-group-list-page.js (2)
42-46: ⚡ Quick winMissing dependencies in
useEffecthook.The
useEffecthook has an empty dependency array but usescurrentSummitandgetEventCategoryGroupsinside. According to React hooks rules, all values from the component scope used inside the effect should be included in the dependency array.While the current code may work (since Redux
connectstabilizes action creators and route changes likely remount the component), this violates React hooks best practices and may cause issues if the component behavior changes.♻️ Add missing dependencies
useEffect(() => { if (currentSummit) { getEventCategoryGroups(); } - }, []); + }, [currentSummit, getEventCategoryGroups]);Note: If
currentSummit.idis the actual dependency (not the entire object), you could use:- }, []); + }, [currentSummit?.id, getEventCategoryGroups]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/events/event-category-group-list-page.js` around lines 42 - 46, The useEffect currently reads currentSummit and calls getEventCategoryGroups but has an empty dependency array; update the effect to include the real dependencies (e.g., include getEventCategoryGroups and the summit identifier such as currentSummit or currentSummit.id) so the effect re-runs correctly when the summit changes, and keep the existing null-check (if (currentSummit) ...) to avoid calling the action with no summit; reference the useEffect, currentSummit (or currentSummit.id) and getEventCategoryGroups symbols when making the change.
58-68: ⚡ Quick winConsider adding error handling to the delete operation.
The
handleDeletefunction has no error handling. IfdeleteEventCategoryGroupfails (network error, server error, etc.), the user receives no feedback, and the list won't refresh. This could leave the UI in an inconsistent state.🛡️ Add error handling
const handleDelete = (groupId) => { - deleteEventCategoryGroup(groupId).then(() => + deleteEventCategoryGroup(groupId) + .then(() => getEventCategoryGroups( term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir ) - ); + ) + .catch((err) => { + console.error("Failed to delete group:", err); + // Optionally show a user-facing error message + }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/events/event-category-group-list-page.js` around lines 58 - 68, handleDelete currently calls deleteEventCategoryGroup without handling failures; update handleDelete to handle errors (e.g., make it async and use try/catch or attach .catch) so that on success you call getEventCategoryGroups(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) to refresh the list, and on failure you surface an error to the user (toast/alert) and/or log the error (include the caught error from deleteEventCategoryGroup). Ensure getEventCategoryGroups is only invoked after a successful delete and include references to handleDelete, deleteEventCategoryGroup, getEventCategoryGroups, DEFAULT_CURRENT_PAGE, term, perPage, order, and orderDir.src/actions/event-category-actions.js (1)
421-424: 💤 Low valueMinor inconsistency with codebase pattern for order direction.
This code uses an empty string
""for ascending sort (whenorderDir === 1), whereassrc/actions/attendee-actions.jsuses"+"for the same case. Both are typically valid API syntax, but maintaining consistency across the codebase improves readability.♻️ Align with existing pattern
- const orderDirSign = orderDir === 1 ? "" : "-"; + const orderDirSign = orderDir === 1 ? "+" : "-";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/actions/event-category-actions.js` around lines 421 - 424, The sort direction assignment in the block that sets params.order uses an empty string for ascending (orderDir === 1); change it to use "+" to match the codebase pattern used in attendee-actions.js—update the orderDirSign logic (the variable orderDirSign and the params.order assignment) so ascending maps to "+" and descending to "-" before setting params.order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/actions/event-category-actions.js`:
- Around line 421-424: The sort direction assignment in the block that sets
params.order uses an empty string for ascending (orderDir === 1); change it to
use "+" to match the codebase pattern used in attendee-actions.js—update the
orderDirSign logic (the variable orderDirSign and the params.order assignment)
so ascending maps to "+" and descending to "-" before setting params.order.
In `@src/pages/events/event-category-group-list-page.js`:
- Around line 42-46: The useEffect currently reads currentSummit and calls
getEventCategoryGroups but has an empty dependency array; update the effect to
include the real dependencies (e.g., include getEventCategoryGroups and the
summit identifier such as currentSummit or currentSummit.id) so the effect
re-runs correctly when the summit changes, and keep the existing null-check (if
(currentSummit) ...) to avoid calling the action with no summit; reference the
useEffect, currentSummit (or currentSummit.id) and getEventCategoryGroups
symbols when making the change.
- Around line 58-68: handleDelete currently calls deleteEventCategoryGroup
without handling failures; update handleDelete to handle errors (e.g., make it
async and use try/catch or attach .catch) so that on success you call
getEventCategoryGroups(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) to
refresh the list, and on failure you surface an error to the user (toast/alert)
and/or log the error (include the caught error from deleteEventCategoryGroup).
Ensure getEventCategoryGroups is only invoked after a successful delete and
include references to handleDelete, deleteEventCategoryGroup,
getEventCategoryGroups, DEFAULT_CURRENT_PAGE, term, perPage, order, and
orderDir.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e540225a-5efe-492f-9179-3b3ea7dda4c5
📒 Files selected for processing (4)
src/actions/event-category-actions.jssrc/i18n/en.jsonsrc/pages/events/event-category-group-list-page.jssrc/reducers/events/event-category-group-list-reducer.js
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Event Category Groups list view by migrating it to the MUI table components and adds server-driven list controls (search/sort/pagination) backed by updated Redux state and API parameters.
Changes:
- Updated
getEventCategoryGroupsto support term filtering, sorting, and pagination, including escaping filter values. - Expanded the event category group list reducer to track term/order/paging metadata from requests and responses.
- Replaced the legacy table UI with
MuiTable+SearchInput, including a color swatch renderer and updated copy.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/reducers/events/event-category-group-list-reducer.js |
Tracks term/sort/paging state and parses paginated responses for the groups list. |
src/pages/events/event-category-group-list-page.js |
Migrates the list UI to MUI components and wires up search/sort/pagination handlers. |
src/i18n/en.json |
Updates list title/labels and delete warning copy. |
src/actions/event-category-actions.js |
Adds query params for filtering/sorting/paging when fetching category groups. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| createAction(RECEIVE_EVENT_CATEGORY_GROUPS), | ||
| `${window.API_BASE_URL}/api/v1/summits/${currentSummit.id}/track-groups`, | ||
| authErrorHandler, | ||
| { order, orderDir, term, perPage } |
There was a problem hiding this comment.
@priscila-moneo page is missing from this REQUEST payload — only order, orderDir, term, and perPage are included. The reducer's REQUEST_EVENT_CATEGORY_GROUPS case can't update currentPage optimistically, so the pagination component keeps the old page highlighted until the API response arrives. Consider adding currentPage: page here and destructuring it in the reducer's REQUEST case.
| } | ||
| case EVENT_CATEGORY_GROUP_DELETED: { | ||
| let { groupId } = payload; | ||
| const { groupId } = payload; |
There was a problem hiding this comment.
@priscila-moneo When an item is deleted, totalEventCategoryGroups is not decremented — only the array is filtered. The count shown above the table ("N Groups") stays stale until the full server reload in handleDelete completes. Adding totalEventCategoryGroups: state.totalEventCategoryGroups - 1 to this return would keep it in sync immediately after deletion.
2d543dc to
f9057ba
Compare
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
f9057ba to
94c083b
Compare
ref: https://app.clickup.com/t/9014802374/86badg7ka
Summary by CodeRabbit