feat: migrate track chair list and edit to mui components#918
Open
tomrndom wants to merge 4 commits into
Open
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 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, and tweaks an i18n delete warning string. ChangesTrack Chairs UI Modernization & Error Handling
Sequence Diagram(s)sequenceDiagram
participant UI as TrackChairListPage
participant Dialog as TrackChairDialog
participant Actions as track-chair-actions
participant API as BackendAPI
UI->>Actions: getTrackChairs(meta: trackId, term, order, perPage)
Actions->>API: GET /track-chairs?meta...
API-->>Actions: 200 + track chairs
Actions->>UI: dispatch RECEIVE_TRACK_CHAIRS
UI->>Dialog: open (new/edit)
Dialog->>Actions: addTrackChair / saveTrackChair (payload)
Actions->>API: POST/PUT /track-chairs
API-->>Actions: 200/201 or error
Actions->>UI: success -> refresh list
Actions->>UI: error -> snackbarErrorHandler shows message
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 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: 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
Comment on lines
+125
to
+131
| const handleSave = ({ id, member, trackIds }) => { | ||
| const newMember = dialogEntity?.originalMemberId !== member?.value; | ||
| const action = | ||
| !id || newMember | ||
| ? addTrackChair({ id: member.value }, trackIds) | ||
| : saveTrackChair(id, trackIds); | ||
| action.then(() => setDialogEntity(null)); |
There was a problem hiding this comment.
Editing the member currently creates a second assignment.
saveTrackChair only updates categories, so this branch to addTrackChair(...) leaves the original record in place when the member changes. In edit mode that turns a reassignment into a duplicate track-chair entry.
Suggested fix
const handleSave = ({ id, member, trackIds }) => {
- const newMember = dialogEntity?.originalMemberId !== member?.value;
- const action =
- !id || newMember
- ? addTrackChair({ id: member.value }, trackIds)
- : saveTrackChair(id, trackIds);
+ const action = !id
+ ? addTrackChair({ id: member.value }, trackIds)
+ : saveTrackChair(id, trackIds);
action.then(() => setDialogEntity(null));
};If member reassignment is supposed to be supported here, it needs an explicit replace flow; otherwise the member field should be read-only in edit mode.
🤖 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 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.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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
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/86b9pc89t
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation