feat: move activity type list to mui#969
Conversation
📝 WalkthroughWalkthroughEvent type list fetching now accepts search, paging, and sorting inputs, stores list controls and totals in reducer state, and renders the page with updated hook-based table wiring and delete text. ChangesEvent Type List Flow
Sequence Diagram(s)sequenceDiagram
participant EventTypeListPage
participant getEventTypes
participant eventTypeAPI
participant eventTypeListReducer
EventTypeListPage->>getEventTypes: term, page, perPage, order, orderDir
getEventTypes->>eventTypeListReducer: REQUEST_EVENT_TYPES
getEventTypes->>eventTypeAPI: request filter[] and order
eventTypeAPI->>getEventTypes: response.items, currentPage, lastPage, totalEventTypes
getEventTypes->>eventTypeListReducer: RECEIVE_EVENT_TYPES
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| }; | ||
| } else { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
totalEventTypes is not updated here. The new page renders a count badge ({totalEventTypes} Activity Types) that will show the pre-seed number after seeding completes.
Suggested fix:
case EVENT_TYPES_SEEDED: {
const eventTypesAdded = payload.response.data;
if (eventTypesAdded.length > 0) {
return {
...state,
eventTypes: [...state.eventTypes, ...eventTypesAdded],
totalEventTypes: state.totalEventTypes + eventTypesAdded.length
};
}
return state;
}
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-activity-types-grid-mui
branch
from
77cea30 to
6872754
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/pages/events/event-type-list-page.js`:
- Around line 45-47: The initial mount fetch in the event type list page is
passing a null search term into state, which then propagates to SearchInput and
paging/sorting handlers. Update the getEventTypes flow in
event-type-list-page.js so the first load uses the current term value or an
empty string instead of null, and ensure the reducer state.term stays normalized
to a string. Use the getEventTypes call inside useEffect and the
SearchInput/handlePageChange/handleSort path as the key places to verify the
fix.
- Around line 75-77: Sorting in handleSort keeps the existing currentPage, which
can leave the list on a later page after a new sort. Update handleSort in
event-type-list-page.js to call getEventTypes with DEFAULT_CURRENT_PAGE instead
of currentPage so sorting always restarts from the first page.
- Around line 124-136: The seed action in event-type-list-page is updating the
store without refreshing the server-paginated table, so the current page can
show extra rows and become out of sync. Update the seed button flow in
event-type-list-page to use a handler similar to handleDelete: await dispatching
seedEventTypes, then trigger a fresh page-1 fetch so the list resets to the
server state. Keep the fix centered around the seedEventTypes click handler and
the existing pagination fetch logic used by handleDelete/currentPage/perPage.
🪄 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: 94243037-e829-4dfe-b4ec-f4f93d3a1e3a
📒 Files selected for processing (4)
src/actions/event-type-actions.jssrc/i18n/en.jsonsrc/pages/events/event-type-list-page.jssrc/reducers/events/event-type-list-reducer.js
| useEffect(() => { | ||
| getEventTypes(); | ||
| }, []); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Mount fetch stores term: null in state.
getEventTypes() runs with term = null, which the reducer writes into state.term. defaultProps.term = "" only applies when the prop is undefined, so SearchInput term={term} receives null, risking a controlled→uncontrolled value warning and null being threaded through handlePageChange/handleSort. Consider defaulting the fetch term to term (or "").
🤖 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-type-list-page.js` around lines 45 - 47, The initial
mount fetch in the event type list page is passing a null search term into
state, which then propagates to SearchInput and paging/sorting handlers. Update
the getEventTypes flow in event-type-list-page.js so the first load uses the
current term value or an empty string instead of null, and ensure the reducer
state.term stays normalized to a string. Use the getEventTypes call inside
useEffect and the SearchInput/handlePageChange/handleSort path as the key places
to verify the fix.
| const handleSort = (key, dir) => { | ||
| getEventTypes(term, currentPage, perPage, key, dir); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sorting retains the current page.
handleSort passes currentPage instead of resetting to DEFAULT_CURRENT_PAGE. Re-sorting while on a later page can leave the user on a page that no longer reflects the new ordering's first results. Resetting to page 1 on sort is the typical expectation.
♻️ Proposed fix
const handleSort = (key, dir) => {
- getEventTypes(term, currentPage, perPage, key, dir);
+ getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, key, dir);
};📝 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 handleSort = (key, dir) => { | |
| getEventTypes(term, currentPage, perPage, key, dir); | |
| }; | |
| const handleSort = (key, dir) => { | |
| getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, key, dir); | |
| }; |
🤖 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-type-list-page.js` around lines 75 - 77, Sorting in
handleSort keeps the existing currentPage, which can leave the list on a later
page after a new sort. Update handleSort in event-type-list-page.js to call
getEventTypes with DEFAULT_CURRENT_PAGE instead of currentPage so sorting always
restarts from the first page.
| <Button | ||
| variant="outlined" | ||
| onClick={seedEventTypes} | ||
| sx={{ | ||
| height: "36px", | ||
| padding: "6px 16px", | ||
| fontSize: "1.4rem", | ||
| lineHeight: "2.4rem", | ||
| letterSpacing: "0.4px" | ||
| }} | ||
| > | ||
| {T.translate("event_type_list.seed_event_types")} | ||
| </Button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Seeding does not refresh the paginated list.
onClick={seedEventTypes} dispatches the seed thunk directly. The reducer appends the seeded items to eventTypes and bumps totalEventTypes, but since the table is server-paginated (totalRows/onPageChange), the appended rows extend the currently displayed page instead of re-fetching page 1. This can render more than perPage rows and desync currentPage/lastPage. Refetch after seeding, mirroring handleDelete.
♻️ Proposed fix
+ const handleSeed = () => {
+ seedEventTypes().then(() =>
+ getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir)
+ );
+ };- onClick={seedEventTypes}
+ onClick={handleSeed}📝 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.
| <Button | |
| variant="outlined" | |
| onClick={seedEventTypes} | |
| sx={{ | |
| height: "36px", | |
| padding: "6px 16px", | |
| fontSize: "1.4rem", | |
| lineHeight: "2.4rem", | |
| letterSpacing: "0.4px" | |
| }} | |
| > | |
| {T.translate("event_type_list.seed_event_types")} | |
| </Button> | |
| const handleSeed = () => { | |
| seedEventTypes().then(() => | |
| getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | |
| ); | |
| }; | |
| <Button | |
| variant="outlined" | |
| onClick={handleSeed} | |
| sx={{ | |
| height: "36px", | |
| padding: "6px 16px", | |
| fontSize: "1.4rem", | |
| lineHeight: "2.4rem", | |
| letterSpacing: "0.4px" | |
| }} | |
| > | |
| {T.translate("event_type_list.seed_event_types")} | |
| </Button> |
🤖 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-type-list-page.js` around lines 124 - 136, The seed
action in event-type-list-page is updating the store without refreshing the
server-paginated table, so the current page can show extra rows and become out
of sync. Update the seed button flow in event-type-list-page to use a handler
similar to handleDelete: await dispatching seedEventTypes, then trigger a fresh
page-1 fetch so the list resets to the server state. Keep the fix centered
around the seedEventTypes click handler and the existing pagination fetch logic
used by handleDelete/currentPage/perPage.
3 participants
ref: https://app.clickup.com/t/86bac70mf
Summary by CodeRabbit
New Features
Bug Fixes