fix: implement MUI table and components on speaker list page#898
Open
fix: implement MUI table and components on speaker list page#898
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughSummitSpeakerListPage converted from a class to a functional component using hooks; UI migrated to MUI components. getSpeakers now forwards perPage in its request context. Delete now calls deleteSpeaker directly then re-fetches. Reducer initial state and perPage persistence updated; tests and i18n updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Page as SummitSpeakerListPage
participant Actions as SpeakerActions
participant API as API Server
participant Store as Redux Store
User->>Page: open speakers page
Page->>Actions: getSpeakers(order, orderDir, page, term, perPage)
Actions->>API: GET /speakers?order...&perPage=...
API-->>Actions: speakers payload
Actions->>Store: dispatch RECEIVED_SPEAKERS
Store-->>Page: state update (speakers, total, perPage)
Page-->>User: render MuiTable or no_results
User->>Page: click delete on speaker
Page->>Actions: deleteSpeaker(id)
Actions->>API: DELETE /speakers/:id
API-->>Actions: 204 OK
Actions->>Actions: getSpeakers(DEFAULT_CURRENT_PAGE, perPage, ...)
Actions->>API: GET /speakers?...&perPage=...
API-->>Actions: updated speakers
Actions->>Store: dispatch RECEIVED_SPEAKERS
Store-->>Page: state update
Page-->>User: updated table / empty state
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)
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: 3
🧹 Nitpick comments (1)
src/pages/speakers/__tests__/summit-speakers-list-page.test.js (1)
191-205: Assert the post-delete refetch, not just the delete call.This test says “refreshes” but never checks the follow-up
getSpeakers(...)call, so the current pagination/filter refetch path can regress unnoticed.🧪 Tighten the delete test
- test("should call deleteSpeaker and refreshes on row delete", async () => { + test("should call deleteSpeaker and refresh current list params on row delete", async () => { const user = userEvent.setup(); renderWithRedux(<SummitSpeakerListPage history={mockHistory} />, { initialState: buildInitialState({ speakers: SAMPLE_SPEAKERS, - totalSpeakers: 2 + totalSpeakers: 2, + term: "alice", + currentPage: 2, + perPage: 25, + order: "email", + orderDir: -1 }) }); await user.click(screen.getAllByText("delete-row")[0]); await waitFor(() => { expect(speakerActions.deleteSpeaker).toHaveBeenCalledWith(1); + expect(speakerActions.getSpeakers).toHaveBeenLastCalledWith( + "alice", + 2, + 25, + "email", + -1 + ); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/speakers/__tests__/summit-speakers-list-page.test.js` around lines 191 - 205, The test only asserts speakerActions.deleteSpeaker was called but must also assert the refetch path: spy/mock speakerActions.getSpeakers and after triggering the delete in SummitSpeakerListPage, waitFor and expect speakerActions.getSpeakers to have been called with the current pagination/filter args (the same arguments the component uses to refresh, e.g., page, pageSize, filters); ensure the test sets up the getSpeakers mock before render and includes the waitFor assertion checking the call and its parameters so the post-delete refetch behavior is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/speakers/summit-speakers-list-page.js`:
- Around line 38-40: The useEffect currently calls getSpeakers() with defaults
and wipes persisted filters; instead read the persisted params from
currentSpeakerListState and call getSpeakers with those values on first mount
(e.g. compute params = currentSpeakerListState || existing defaults, then call
getSpeakers(params)). Update the useEffect block around useEffect(() => { ... },
[]) to fetch and pass the restored search, sort, page, and pageSize from
currentSpeakerListState (or guard that currentSpeakerListState is initialized)
so the persisted list params are preserved on remount.
- Around line 48-51: handleDelete currently calls getSpeakers with
DEFAULT_CURRENT_PAGE which forces the UI back to page 1 after any deletion;
change it to use the current page state/variable instead so pagination is
preserved. Update the handleDelete flow that calls
deleteSpeaker(speakerId).then(() => getSpeakers(...)) to pass the existing
current page (e.g., currentPage or page state) instead of DEFAULT_CURRENT_PAGE,
and ensure any surrounding state or props used by getSpeakers (term, perPage,
order, orderDir) remain unchanged; also propagate errors from deleteSpeaker to
avoid silent failures.
- Around line 92-98: The component currently gates actions via
table_options.actions but still passes onDelete/onEdit props unconditionally to
MuiTable; update the prop passing so onDelete and onEdit are null when the
member lacks permission (use memberObj.canDeleteSpeakers() and
memberObj.canEditSpeakers() to decide), i.e., pass
onDelete={memberObj.canDeleteSpeakers() ? handleDelete : null} and
onEdit={memberObj.canEditSpeakers() ? handleEdit : null} to ensure permissions
are enforced at the prop level (refer to handleDelete, handleEdit, memberObj,
MuiTable and table_options.actions).
---
Nitpick comments:
In `@src/pages/speakers/__tests__/summit-speakers-list-page.test.js`:
- Around line 191-205: The test only asserts speakerActions.deleteSpeaker was
called but must also assert the refetch path: spy/mock
speakerActions.getSpeakers and after triggering the delete in
SummitSpeakerListPage, waitFor and expect speakerActions.getSpeakers to have
been called with the current pagination/filter args (the same arguments the
component uses to refresh, e.g., page, pageSize, filters); ensure the test sets
up the getSpeakers mock before render and includes the waitFor assertion
checking the call and its parameters so the post-delete refetch behavior is
verified.
🪄 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: dc493bc5-9511-454a-a112-b462c01ce374
📒 Files selected for processing (5)
src/actions/speaker-actions.jssrc/i18n/en.jsonsrc/pages/speakers/__tests__/summit-speakers-list-page.test.jssrc/pages/speakers/summit-speakers-list-page.jssrc/reducers/speakers/speaker-list-reducer.js
Comment on lines
+38
to
+40
| useEffect(() => { | ||
| getSpeakers(); | ||
| }, []); |
There was a problem hiding this comment.
Restore the persisted list params on first load.
Calling getSpeakers() with defaults here drops the stored search term, sort, page, and page size every time the page remounts. Since those values are already persisted in currentSpeakerListState, this resets the list unexpectedly after navigation.
🔧 Suggested fix
useEffect(() => {
- getSpeakers();
+ getSpeakers(term, currentPage, perPage, order, orderDir);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
}, []);📝 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
| useEffect(() => { | |
| getSpeakers(); | |
| }, []); | |
| useEffect(() => { | |
| getSpeakers(term, currentPage, perPage, order, orderDir); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/speakers/summit-speakers-list-page.js` around lines 38 - 40, The
useEffect currently calls getSpeakers() with defaults and wipes persisted
filters; instead read the persisted params from currentSpeakerListState and call
getSpeakers with those values on first mount (e.g. compute params =
currentSpeakerListState || existing defaults, then call getSpeakers(params)).
Update the useEffect block around useEffect(() => { ... }, []) to fetch and pass
the restored search, sort, page, and pageSize from currentSpeakerListState (or
guard that currentSpeakerListState is initialized) so the persisted list params
are preserved on remount.
Comment on lines
+48
to
+51
| const handleDelete = (speakerId) => { | ||
| deleteSpeaker(speakerId).then(() => | ||
| getSpeakers(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | ||
| ); |
There was a problem hiding this comment.
Don’t reset pagination to page 1 after every delete.
This throws users back to the first page even when they deleted from a later page. On large speaker lists that’s a noticeable workflow regression.
🔧 Suggested fix
const handleDelete = (speakerId) => {
- deleteSpeaker(speakerId).then(() =>
- getSpeakers(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir)
- );
+ const nextPage =
+ currentPage > DEFAULT_CURRENT_PAGE && speakers.length === 1
+ ? currentPage - 1
+ : currentPage;
+
+ deleteSpeaker(speakerId).then(() =>
+ getSpeakers(term, nextPage, 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.
Suggested change
| const handleDelete = (speakerId) => { | |
| deleteSpeaker(speakerId).then(() => | |
| getSpeakers(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | |
| ); | |
| const handleDelete = (speakerId) => { | |
| const nextPage = | |
| currentPage > DEFAULT_CURRENT_PAGE && speakers.length === 1 | |
| ? currentPage - 1 | |
| : currentPage; | |
| deleteSpeaker(speakerId).then(() => | |
| getSpeakers(term, nextPage, perPage, order, orderDir) | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/speakers/summit-speakers-list-page.js` around lines 48 - 51,
handleDelete currently calls getSpeakers with DEFAULT_CURRENT_PAGE which forces
the UI back to page 1 after any deletion; change it to use the current page
state/variable instead so pagination is preserved. Update the handleDelete flow
that calls deleteSpeaker(speakerId).then(() => getSpeakers(...)) to pass the
existing current page (e.g., currentPage or page state) instead of
DEFAULT_CURRENT_PAGE, and ensure any surrounding state or props used by
getSpeakers (term, perPage, order, orderDir) remain unchanged; also propagate
errors from deleteSpeaker to avoid silent failures.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/pages/speakers/summit-speakers-list-page.js (2)
38-40:⚠️ Potential issue | 🟠 MajorRestore the persisted list params on first load.
This still calls
getSpeakers()with defaults, so remounting the page can wipe the storedterm,currentPage,perPage, and sort state fromcurrentSpeakerListState.Suggested fix
useEffect(() => { - getSpeakers(); + getSpeakers(term, currentPage, perPage, order, orderDir); + // eslint-disable-next-line react-hooks/exhaustive-deps }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/speakers/summit-speakers-list-page.js` around lines 38 - 40, The effect currently calls getSpeakers() immediately and overwrites persisted list params; update the mount logic so it first restores currentSpeakerListState (term, currentPage, perPage, sort) from storage/wherever you persist them and sets those values into the component/state before calling getSpeakers(); e.g. in the useEffect that currently calls getSpeakers(), add a restore/hydrate step that populates the state used by getSpeakers (referencing currentSpeakerListState and any setters) and only call getSpeakers() after restoration (or gate the call with an “initialized” flag) so remounting will use persisted params instead of defaults.
48-51:⚠️ Potential issue | 🟠 MajorDon’t reset pagination to page 1 after every delete.
The re-fetch still hardcodes
DEFAULT_CURRENT_PAGE, so deleting from page N throws the user back to the first page. PreservecurrentPage, and only step back when the last row on a non-first page was removed.Suggested fix
const handleDelete = (speakerId) => { + const nextPage = + currentPage > DEFAULT_CURRENT_PAGE && speakers.length === 1 + ? currentPage - 1 + : currentPage; + deleteSpeaker(speakerId).then(() => - getSpeakers(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) + getSpeakers(term, nextPage, perPage, order, orderDir) ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/speakers/summit-speakers-list-page.js` around lines 48 - 51, handleDelete currently calls getSpeakers with DEFAULT_CURRENT_PAGE which forces the list back to page 1 after any delete; change it to preserve the currentPage variable (use currentPage instead of DEFAULT_CURRENT_PAGE) and only decrement currentPage by 1 when the deleted item was the last item on a non-first page (i.e., when current speakers count % perPage === 1 and currentPage > 1). Update the handleDelete flow that calls deleteSpeaker(speakerId).then(...) to compute the newPage (either currentPage or currentPage - 1) and pass that to getSpeakers(term, newPage, perPage, order, orderDir).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pages/speakers/summit-speakers-list-page.js`:
- Around line 38-40: The effect currently calls getSpeakers() immediately and
overwrites persisted list params; update the mount logic so it first restores
currentSpeakerListState (term, currentPage, perPage, sort) from storage/wherever
you persist them and sets those values into the component/state before calling
getSpeakers(); e.g. in the useEffect that currently calls getSpeakers(), add a
restore/hydrate step that populates the state used by getSpeakers (referencing
currentSpeakerListState and any setters) and only call getSpeakers() after
restoration (or gate the call with an “initialized” flag) so remounting will use
persisted params instead of defaults.
- Around line 48-51: handleDelete currently calls getSpeakers with
DEFAULT_CURRENT_PAGE which forces the list back to page 1 after any delete;
change it to preserve the currentPage variable (use currentPage instead of
DEFAULT_CURRENT_PAGE) and only decrement currentPage by 1 when the deleted item
was the last item on a non-first page (i.e., when current speakers count %
perPage === 1 and currentPage > 1). Update the handleDelete flow that calls
deleteSpeaker(speakerId).then(...) to compute the newPage (either currentPage or
currentPage - 1) and pass that to getSpeakers(term, newPage, perPage, order,
orderDir).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95242b33-921d-45d9-9c8a-3400134c3b45
📒 Files selected for processing (1)
src/pages/speakers/summit-speakers-list-page.js
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
ref: https://app.clickup.com/t/86b9km191
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Tests