feat: migrate track chair list and edit to mui components#918
Open
tomrndom wants to merge 8 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Track Chairs to a hooks-based MUI page, adds a Formik-based TrackChairDialog, updates track-chair action requests/error handling (adds member.id, passes meta, uses snackbarErrorHandler and .finally()), adjusts reducer reset/perPage behavior, enhances async select prop forwarding, and tweaks an i18n delete warning string. ChangesTrack Chairs UI Modernization & Error Handling
Sequence Diagram(s)sequenceDiagram
participant User
participant ListPage as TrackChairListPage
participant Dialog as TrackChairDialog
participant Actions as Redux Actions
participant API as Backend API
rect rgba(100, 150, 200, 0.5)
Note over User,API: Initial page load
ListPage->>Actions: getTrackChairs(meta: trackId, term, order, orderDir, perPage)
Actions->>API: GET /track-chairs?meta...
API-->>Actions: 200 + trackChairs (with member.id)
Actions->>ListPage: dispatch RECEIVE_TRACK_CHAIRS
end
rect rgba(150, 200, 150, 0.5)
Note over User,Dialog: Add new track chair
User->>ListPage: click Add button
ListPage->>Dialog: render with empty entity
User->>Dialog: select member, track(s), Save
Dialog->>Actions: addTrackChair(memberId, trackIds)
Actions->>API: POST /track-chairs
API-->>Actions: 201 + new chair
Actions->>ListPage: dispatch success
ListPage->>Actions: getTrackChairs (refresh)
ListPage->>Dialog: close (dialogEntity=null)
end
rect rgba(200, 150, 100, 0.5)
Note over User,Dialog: Edit existing track chair
User->>ListPage: click edit icon
ListPage->>Dialog: render with populated entity
User->>Dialog: modify tracks, Save
Dialog->>Actions: saveTrackChair(chairId, trackIds)
Actions->>API: PUT /track-chairs/{id}
API-->>Actions: 200 + updated chair
ListPage->>Actions: getTrackChairs (refresh)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/track-chair-actions.js (1)
149-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways clear loading state on add failures.
This path still dispatches
stopLoading()only on success. IfpostRequestrejects, the global loading state stays active and the new dialog flow gets stuck after a failed create.Suggested fix
return postRequest( null, createAction(TRACK_CHAIR_ADDED), `${window.API_BASE_URL}/api/v1/summits/${currentSummit.id}/track-chairs`, { member_id: member.id, categories: trackIds }, snackbarErrorHandler - )(params)(dispatch).then(() => { + )(params)(dispatch).finally(() => { dispatch(stopLoading()); });🤖 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/track-chair-actions.js` around lines 149 - 157, The add-track-chair flow only calls dispatch(stopLoading()) in the success .then, leaving the global loading state active on postRequest rejection; update the returned promise chain that wraps postRequest(...)(params)(dispatch) so that dispatch(stopLoading()) runs regardless of outcome (use .finally or a .catch that rethrows after dispatching stopLoading()). Reference the postRequest call that uses createAction(TRACK_CHAIR_ADDED), snackbarErrorHandler, and ensure stopLoading is dispatched in the failure path as well.
🤖 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/track_chairs/track-chair-list-page.js`:
- Around line 125-131: The current handleSave treats a changed member in edit
mode as an add, causing duplicate assignments; update handleSave to perform an
explicit replace instead of add when dialogEntity?.originalMemberId !==
member?.value and id exists: either call a replaceTrackChair(member.value, id,
trackIds) helper (preferred) or sequentially call removeTrackChair(id) then
addTrackChair({ id: member.value }, trackIds), and finally clear the dialog;
alternatively, if reassignment is not allowed make the member field read-only in
edit mode (in the dialog render) so the branch to addTrackChair never runs for
edits. Ensure you reference dialogEntity.originalMemberId, handleSave,
addTrackChair, saveTrackChair and remove/replace helper names when implementing
the change.
- Around line 57-63: The early return based on currentSummit.id happens before
hooks run and can cause hook-order errors; keep the declarations (e.g., const
[dialogEntity, setDialogEntity] = useState(null)) and the useEffect hook in
place, move the render guard that checks currentSummit.id below the hooks (so
hooks always register), and change the effect to depend on currentSummit?.id
(useEffect(() => { if (currentSummit?.id) getTrackChairs(); },
[currentSummit?.id])) so getTrackChairs runs when the summit arrives.
---
Outside diff comments:
In `@src/actions/track-chair-actions.js`:
- Around line 149-157: The add-track-chair flow only calls
dispatch(stopLoading()) in the success .then, leaving the global loading state
active on postRequest rejection; update the returned promise chain that wraps
postRequest(...)(params)(dispatch) so that dispatch(stopLoading()) runs
regardless of outcome (use .finally or a .catch that rethrows after dispatching
stopLoading()). Reference the postRequest call that uses
createAction(TRACK_CHAIR_ADDED), snackbarErrorHandler, and ensure stopLoading is
dispatched in the failure path as well.
🪄 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: f4ec317b-b43b-40a7-8d38-7a99aa796a4b
📒 Files selected for processing (5)
src/actions/track-chair-actions.jssrc/i18n/en.jsonsrc/pages/track_chairs/components/track-chair-dialog.jssrc/pages/track_chairs/track-chair-list-page.jssrc/reducers/track_chairs/track-chair-list-reducer.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/track_chairs/track-chair-list-page.js (1)
57-63:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRuntime crash:
currentSummit.tracksaccessed before guard.Line 63 accesses
currentSummit.tracksbefore the guard at line 157. IfcurrentSummitis undefined, this crashes. Additionally, theuseEffecthas an empty dependency array, sogetTrackChairswon't be called when the summit loads after mount.Move
chairTracksderivation and the early return guard together, and add proper dependencies to the effect:Proposed fix
const [dialogEntity, setDialogEntity] = useState(null); + const chairTracks = currentSummit?.tracks?.filter((t) => t.chair_visible) ?? []; + useEffect(() => { - if (currentSummit) getTrackChairs(); - }, []); + if (currentSummit?.id) getTrackChairs(); + }, [currentSummit?.id]); - const chairTracks = currentSummit.tracks.filter((t) => t.chair_visible); + if (!currentSummit?.id) return <div />;Then remove the duplicate guard at line 157.
🤖 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/track_chairs/track-chair-list-page.js` around lines 57 - 63, Move the derivation of chairTracks so it runs only after the early-return guard that checks currentSummit (i.e., return/loading when !currentSummit) to avoid accessing currentSummit.tracks when currentSummit is undefined; update the useEffect that calls getTrackChairs to include currentSummit and getTrackChairs in its dependency array (useEffect(() => { if (currentSummit) getTrackChairs(); }, [currentSummit, getTrackChairs])) so the fetch runs when the summit loads, and remove the now-duplicate early-return guard later in the component; references: chairTracks, currentSummit, getTrackChairs, useEffect.
🤖 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.
Duplicate comments:
In `@src/pages/track_chairs/track-chair-list-page.js`:
- Around line 57-63: Move the derivation of chairTracks so it runs only after
the early-return guard that checks currentSummit (i.e., return/loading when
!currentSummit) to avoid accessing currentSummit.tracks when currentSummit is
undefined; update the useEffect that calls getTrackChairs to include
currentSummit and getTrackChairs in its dependency array (useEffect(() => { if
(currentSummit) getTrackChairs(); }, [currentSummit, getTrackChairs])) so the
fetch runs when the summit loads, and remove the now-duplicate early-return
guard later in the component; references: chairTracks, currentSummit,
getTrackChairs, useEffect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b6b9930-5b7b-4fbd-8a77-4a9210e5efbe
📒 Files selected for processing (1)
src/pages/track_chairs/track-chair-list-page.js
smarcet
reviewed
Jun 18, 2026
smarcet
reviewed
Jun 18, 2026
smarcet
reviewed
Jun 18, 2026
smarcet
reviewed
Jun 18, 2026
smarcet
requested changes
Jun 18, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/mui/formik-inputs/mui-formik-async-select.js`:
- Line 124: The spread operator `{...rest}` is positioned after critical
internal props in the component's prop list, allowing consumer-provided props to
override essential handlers like `onChange`, `value`, `loading`, and `multiple`,
which breaks Formik form synchronization. Move the `{...rest}` spread to appear
before all critical internal props (such as `onChange`, `value`, `loading`,
`multiple`, `onBlur`, etc.) so that the internal props always take precedence
and cannot be overridden by consumer input.
🪄 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: 156c349d-b0e9-4cba-b712-1f5bd86f29fe
📒 Files selected for processing (5)
src/components/mui/formik-inputs/mui-formik-async-select.jssrc/pages/track_chairs/__tests__/track-chair-list-page.test.jssrc/pages/track_chairs/components/__tests__/track-chair-dialog.test.jssrc/pages/track_chairs/components/track-chair-dialog.jssrc/pages/track_chairs/track-chair-list-page.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/track_chairs/components/track-chair-dialog.js
- src/pages/track_chairs/track-chair-list-page.js
smarcet
reviewed
Jun 23, 2026
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Code review findings from deep review + ClickUp pattern audit (86bag2zk7).
smarcet
reviewed
Jun 23, 2026
smarcet
reviewed
Jun 23, 2026
smarcet
reviewed
Jun 23, 2026
tomrndom
force-pushed
the
fix/track-chair-list-mui
branch
from
f3caafd to
94153b1
Compare
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…se function Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
fix/track-chair-list-mui
branch
from
8ee9b3d to
780d45f
Compare
smarcet
reviewed
Jun 30, 2026
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@tomrndom adversarial review pass — 9 findings below (finding on snackbarErrorHandler/401 is intentionally omitted — accepted behaviour).
src/reducers/track_chairs/track-chair-list-reducer.js — TRACK_CHAIR_ADDED name format (line 71)
TRACK_CHAIR_ADDED builds name as "${first_name} ${last_name}" (no email), while RECEIVE_TRACK_CHAIRS (line 54) produces "${first_name} ${last_name} (${email})". The two cases are inconsistent.
In practice the mismatch is masked by the immediate getTrackChairs reload, but if the reload is ever removed or delayed the Name column will briefly show the wrong format for newly added chairs.
Fix:
newTrackChair.name = `${newTrackChair.member.first_name} ${newTrackChair.member.last_name} (${newTrackChair.member.email})`;src/reducers/track_chairs/track-chair-list-reducer.js — totalTrackChairs not decremented on delete (line 93)
TRACK_CHAIR_DELETED filters the item out of trackChairs but does not decrement totalTrackChairs. The count shown in the header and the value passed to MuiTable's totalRows prop are both stale after every deletion.
On the last full page this causes MuiTable to believe there are still perPage items when there are only perPage - 1, which can render an empty but paginated last page.
Fix:
return {
...state,
trackChairs: state.trackChairs.filter((tc) => tc.id !== trackChairId),
totalTrackChairs: state.totalTrackChairs - 1
};There was a problem hiding this comment.
Pull request overview
This PR migrates the Track Chair list and edit/create flows to Material UI components, introducing a dialog-driven UX and updating supporting Redux/action behavior for pagination and error handling.
Changes:
- Refactors the Track Chair list page to a hooks-based MUI layout using the MUI table component and dialog-driven add/edit/delete.
- Adds a new
TrackChairDialogcomponent using Formik + Yup for validation and async member lookup. - Updates track-chair actions/reducer to track
perPage, includemember.idin fetched fields, and use snackbar-based error handling; adds Jest/RTL tests for the new UI.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/reducers/track_chairs/track-chair-list-reducer.js | Stores perPage in state when requesting track chairs and minor reducer cleanups. |
| src/pages/track_chairs/track-chair-list-page.js | Rebuilds the list page UI using MUI components, adds dialog-based add/edit flow, and wires paging/sorting/filtering to Redux actions. |
| src/pages/track_chairs/components/track-chair-dialog.js | Introduces a Formik/Yup-powered MUI dialog for creating/editing track chairs with validation and async member selection. |
| src/pages/track_chairs/components/tests/track-chair-dialog.test.js | Adds tests for dialog validation, submit behavior, and save/close disabling while saving. |
| src/pages/track_chairs/tests/track-chair-list-page.test.js | Adds tests for dialog visibility, edit/delete wiring, and save-path selection (add vs update). |
| src/i18n/en.json | Tweaks deletion confirmation text for the new delete dialog. |
| src/actions/track-chair-actions.js | Updates fetching fields, request payload (incl. perPage), error handling via snackbar, and loading-state handling for most actions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
smarcet
reviewed
Jun 30, 2026
smarcet
reviewed
Jun 30, 2026
smarcet
reviewed
Jun 30, 2026
smarcet
reviewed
Jun 30, 2026
smarcet
reviewed
Jun 30, 2026
smarcet
reviewed
Jun 30, 2026
smarcet
reviewed
Jun 30, 2026
smarcet
requested changes
Jun 30, 2026
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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
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/86b9pc89t
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit