fix: implement MUI table and components on speaker list page#898
Open
tomrndom wants to merge 2 commits into
Open
fix: implement MUI table and components on speaker list page#898tomrndom wants to merge 2 commits into
tomrndom wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughSummitSpeakerListPage converted to a functional component with hooks and MUI; getSpeakers now forwards perPage in its request context; delete calls deleteSpeaker then re-fetches; handlers updated for search/pagination/per-page/sort; tests and i18n updated. ChangesSpeakers list refactor
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 Possibly related PRs
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.
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
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
fix/speaker-list-mui
branch
from
290acbb to
95ebada
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/speakers/__tests__/summit-speakers-list-page.test.js (1)
74-77: ⚡ Quick winUse the real speaker row shape in these fixtures.
These test rows include a synthetic
namefield, but the list fetch returnsfirst_name/last_name. That lets the suite pass even if the Name column renders empty in production. Build the fixtures with the actual fields and assert the composed display name instead.Fixture update
const SAMPLE_SPEAKERS = [ - { id: 1, name: "Alice Smith", email: "alice@example.com", member_id: 10 }, - { id: 2, name: "Bob Jones", email: "bob@example.com", member_id: 20 } + { + id: 1, + first_name: "Alice", + last_name: "Smith", + email: "alice@example.com", + member_id: 10 + }, + { + id: 2, + first_name: "Bob", + last_name: "Jones", + email: "bob@example.com", + member_id: 20 + } ];🤖 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/speakers/__tests__/summit-speakers-list-page.test.js` around lines 74 - 77, The SAMPLE_SPEAKERS test fixture uses a synthetic name field instead of the real API shape; update the SAMPLE_SPEAKERS constant to include first_name and last_name (replace name) and keep id, email, member_id, then update the test assertion to expect the composed display name (e.g., `${first_name} ${last_name}`) where the UI renders the Name column so the test verifies real rendering logic (refer to SAMPLE_SPEAKERS and the test that asserts the Name column).
🤖 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/speakers/summit-speakers-list-page.js`:
- Around line 75-84: The table columns expect a "name" field but getSpeakers
returns first_name and last_name, leaving the Name column blank and delete
dialogs without a display name; fix by populating a "name" property on each
speaker before passing speakers into MuiTable (e.g., map speakers => ({ ...s,
name: `${s.first_name} ${s.last_name}` })) and ensure MuiTable's getName prop
(used for the delete dialog) returns that combined name (or directly
`${first_name} ${last_name}`); update the code around the columns definition
(columnKey: "name") and the MuiTable usage (getName / onDelete wiring, also
check the similar block at 143-159) so the UI and delete dialog display the
speaker's full name.
---
Nitpick comments:
In `@src/pages/speakers/__tests__/summit-speakers-list-page.test.js`:
- Around line 74-77: The SAMPLE_SPEAKERS test fixture uses a synthetic name
field instead of the real API shape; update the SAMPLE_SPEAKERS constant to
include first_name and last_name (replace name) and keep id, email, member_id,
then update the test assertion to expect the composed display name (e.g.,
`${first_name} ${last_name}`) where the UI renders the Name column so the test
verifies real rendering logic (refer to SAMPLE_SPEAKERS and the test that
asserts the Name column).
🪄 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: 1f63cc6b-1a60-464e-8d9f-e61094614c62
📒 Files selected for processing (4)
src/actions/speaker-actions.jssrc/i18n/en.jsonsrc/pages/speakers/__tests__/summit-speakers-list-page.test.jssrc/pages/speakers/summit-speakers-list-page.js
✅ Files skipped from review due to trivial changes (1)
- src/i18n/en.json
Comment on lines
+75
to
+84
| const columns = [ | ||
| { columnKey: "id", header: T.translate("general.id"), sortable: true }, | ||
| { columnKey: "name", header: T.translate("general.name"), sortable: true }, | ||
| { | ||
| columnKey: "email", | ||
| header: T.translate("general.email"), | ||
| sortable: true | ||
| }, | ||
| { columnKey: "member_id", header: T.translate("speaker_list.member_id") } | ||
| ]; |
There was a problem hiding this comment.
The name column does not match the speaker rows returned by getSpeakers.
getSpeakers fetches first_name and last_name, but this table is keyed on name and passes speakers through unchanged. That leaves the Name column empty in production, and the delete dialog has no reliable display name either.
Proposed fix
+ const getSpeakerName = (speaker) =>
+ [speaker.first_name, speaker.last_name].filter(Boolean).join(" ");
+
+ const tableData = speakers.map((speaker) => ({
+ ...speaker,
+ name: getSpeakerName(speaker)
+ }));
+
const columns = [
{ columnKey: "id", header: T.translate("general.id"), sortable: true },
{ columnKey: "name", header: T.translate("general.name"), sortable: true },
{
columnKey: "email",
@@
<MuiTable
columns={columns}
- data={speakers}
+ data={tableData}
options={table_options}
perPage={perPage}
currentPage={currentPage}
totalRows={totalSpeakers}
onPageChange={handlePageChange}
onPerPageChange={handlePerPageChange}
onSort={handleSort}
onDelete={memberObj.canDeleteSpeakers() ? handleDelete : null}
onEdit={memberObj.canEditSpeakers() ? handleEdit : null}
+ getName={getSpeakerName}
deleteDialogBody={(name) =>
T.translate("speaker_list.delete_speaker_warning", { name })
}
/>Based on learnings: When using MuiTable from openstack-uicore-foundation/lib/components/mui/table in this codebase, the getName prop is used by MuiTable for displaying the delete dialog, not to transform the onDelete argument.
Also applies to: 143-159
🤖 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/speakers/summit-speakers-list-page.js` around lines 75 - 84, The
table columns expect a "name" field but getSpeakers returns first_name and
last_name, leaving the Name column blank and delete dialogs without a display
name; fix by populating a "name" property on each speaker before passing
speakers into MuiTable (e.g., map speakers => ({ ...s, name: `${s.first_name}
${s.last_name}` })) and ensure MuiTable's getName prop (used for the delete
dialog) returns that combined name (or directly `${first_name} ${last_name}`);
update the code around the columns definition (columnKey: "name") and the
MuiTable usage (getName / onDelete wiring, also check the similar block at
143-159) so the UI and delete dialog display the speaker's full name.
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
Bug Fixes / Behavior
Tests