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 Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughEvent category groups listing now supports search, pagination, and sorting through parameterized fetching, reducer-backed query state, and a hooks-based page using MUI table controls. The list strings were also updated. ChangesEvent Category Groups List Refactor
Sequence Diagram(s)sequenceDiagram
participant EventCategoryGroupListPage
participant getEventCategoryGroups
participant event_category_group_list_reducer as event-category-group-list-reducer
participant MuiTable
EventCategoryGroupListPage->>getEventCategoryGroups: getEventCategoryGroups(term, page, perPage, order, orderDir)
getEventCategoryGroupListPage->>event_category_group_list_reducer: REQUEST_EVENT_CATEGORY_GROUPS
getEventCategoryGroupListPage->>event_category_group_list_reducer: RECEIVE_EVENT_CATEGORY_GROUPS
event_category_group_list_reducer->>EventCategoryGroupListPage: updated rows and pagination state
EventCategoryGroupListPage->>MuiTable: render rows, sorting, paging, delete actions
MuiTable->>EventCategoryGroupListPage: sort/page/delete callbacks
EventCategoryGroupListPage->>getEventCategoryGroups: refresh with current query state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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.
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-event-category-groups-mui
branch
from
2d543dc to
f9057ba
Compare
ref: https://app.clickup.com/t/9014802374/86badg7ka
Summary by CodeRabbit
New Features
Improvements
UI/Style Updates
Bug Fixes