feat: move activity type list to mui#969
Conversation
📝 WalkthroughWalkthroughEvent type screens now use parameterized list fetching, richer list state, a dialog-based create/edit flow, updated delete copy, and a simplified layout that routes only to the list page. ChangesEvent Type Flow
Sequence Diagram(s)sequenceDiagram
participant EventTypeListPage
participant EventTypeDialog
participant EventTypeForm
participant saveEventType
EventTypeListPage->>EventTypeDialog: open for add/edit
EventTypeDialog->>EventTypeForm: render with isSaving and handlers
EventTypeForm->>saveEventType: submit entity data
saveEventType-->>EventTypeDialog: resolve or reject
EventTypeDialog-->>EventTypeListPage: close on success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
| }; | ||
| } 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
| 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.
* feat: move activity types new and edit to mui Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar> * fix: feedback from PR Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar> --------- Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 53-55: The `useEffect` in `event-type-list-page` currently calls
`getEventTypes()` on mount before `currentSummit.id` is available, which can
trigger a fetch with an undefined summit id and leave stale data. Update the
effect to wait until `currentSummit.id` is present, and include that id in the
dependency list so `getEventTypes` reruns when the summit becomes available; use
the existing `getEventTypes` and `currentSummit.id` references to place the fix.
- Around line 66-69: The success path in handleSave should not depend on the
follow-up refresh request, because EventTypeDialog only closes when onSave
resolves. Update handleSave in event-type-list-page.js so it awaits
saveEventType(eventTypeEntity) first and resolves immediately on success, then
trigger getEventTypes(...) separately with its own error handling or
fire-and-forget behavior. Keep the save completion independent from the list
refresh while preserving the existing refresh call after save.
🪄 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: 08db461c-3f17-4d10-b879-a849c7afd3ab
📒 Files selected for processing (6)
src/actions/event-type-actions.jssrc/components/forms/event-type-form.jssrc/layouts/event-type-layout.jssrc/pages/events/components/event-type-dialog.jssrc/pages/events/edit-event-type-page.jssrc/pages/events/event-type-list-page.js
💤 Files with no reviewable changes (1)
- src/pages/events/edit-event-type-page.js
| useEffect(() => { | ||
| getEventTypes(); | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wait for currentSummit.id before fetching.
The render path handles a missing summit id, but this effect still fires once and never reruns, so a direct load can request /summits/undefined/event-types and leave the list stale.
Proposed fix
useEffect(() => {
- getEventTypes();
- }, []);
+ if (!currentSummit.id) return;
+ getEventTypes();
+ }, [currentSummit.id, getEventTypes]);📝 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.
| useEffect(() => { | |
| getEventTypes(); | |
| }, []); | |
| useEffect(() => { | |
| if (!currentSummit.id) return; | |
| getEventTypes(); | |
| }, [currentSummit.id, getEventTypes]); |
🤖 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 53 - 55, The
`useEffect` in `event-type-list-page` currently calls `getEventTypes()` on mount
before `currentSummit.id` is available, which can trigger a fetch with an
undefined summit id and leave stale data. Update the effect to wait until
`currentSummit.id` is present, and include that id in the dependency list so
`getEventTypes` reruns when the summit becomes available; use the existing
`getEventTypes` and `currentSummit.id` references to place the fix.
| const handleSave = (eventTypeEntity) => | ||
| saveEventType(eventTypeEntity).then(() => | ||
| getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t make dialog success depend on the refresh request.
EventTypeDialog closes only when onSave resolves. If saveEventType succeeds but the follow-up list refresh fails, the dialog stays open as if the save failed, which can invite duplicate create retries.
Proposed fix
const handleSave = (eventTypeEntity) =>
- saveEventType(eventTypeEntity).then(() =>
- getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir)
- );
+ saveEventType(eventTypeEntity).then(() => {
+ getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir);
+ });📝 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 handleSave = (eventTypeEntity) => | |
| saveEventType(eventTypeEntity).then(() => | |
| getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | |
| ); | |
| const handleSave = (eventTypeEntity) => | |
| saveEventType(eventTypeEntity).then(() => { | |
| getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir); | |
| }); |
🤖 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 66 - 69, The success
path in handleSave should not depend on the follow-up refresh request, because
EventTypeDialog only closes when onSave resolves. Update handleSave in
event-type-list-page.js so it awaits saveEventType(eventTypeEntity) first and
resolves immediately on success, then trigger getEventTypes(...) separately with
its own error handling or fire-and-forget behavior. Keep the save completion
independent from the list refresh while preserving the existing refresh call
after save.
3 participants
ref: https://app.clickup.com/t/86bac70mf
Summary by CodeRabbit