feat: move selection plans grid to mui#917
Conversation
📝 WalkthroughWalkthroughModal-driven selection-plans list/editor using hooks; add per-page pagination through actions/reducer/components, update routing to redirect to the list, and adjust imports and table ordering APIs. ChangesSelection Plans Modal-Driven UI Refactor
Sequence DiagramsequenceDiagram
participant User as User
participant List as SelectionPlanListPage
participant API as API
participant Dialog as Dialog
participant EditPage as EditSelectionPlanPage
participant Form as SelectionPlanForm
User->>List: View list / Click edit or route contains id
List->>API: getSelectionPlan / getMarketingSettingsBySelectionPlan
API-->>List: Return entity & marketing settings
List->>Dialog: openEditModal(entity)
Dialog->>EditPage: render with entity & onSaved
EditPage->>Form: render form with onSaved callback
User->>Form: Submit changes
Form->>API: POST/PUT selection plan & save marketing settings
API-->>Form: Save response
Form->>EditPage: invoke onSaved(response)
EditPage->>List: handleSelectionPlanSaved(response)
List->>API: refresh selection plans (getSelectionPlans with page/perPage/order)
API-->>List: updated list
List->>User: render refreshed list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/pages/selection-plans/selection-plan-list-page.js (2)
88-95: 💤 Low valueEdit flow goes through URL push — works but worth a brief inline comment.
Clicking edit pushes
/selection-plans/:idand relies on therouteSelectionPlanIdeffect to open the dialog. This is a nice property for deep-linking, but it's non-obvious and easy to break in future refactors. A one-line comment explaining the URL ↔ modal relationship would help.🤖 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/selection-plans/selection-plan-list-page.js` around lines 88 - 95, Add a one-line inline comment in the handleEdit function explaining that calling history.push(`/app/summits/${currentSummit.id}/selection-plans/${selectionPlanId}`) intentionally updates the URL to trigger the routeSelectionPlanId effect which opens the edit modal/dialog (enables deep-linking), so future refactors know the URL change is relied upon to show the modal rather than directly invoking a modal open method.
60-73: ⚡ Quick winAdd a rejection handler to
openEditModal.If
getSelectionPlanorgetMarketingSettingsBySelectionPlanrejects (e.g., 404 on a deep-linked stale ID, network failure), the popup never opens and the user is left on the list with no signal. A.catchensures the route-driven open flow degrades gracefully.🛡️ Proposed fix
const openEditModal = (selectionPlanId) => { if (!selectionPlanId) return; getSelectionPlan(selectionPlanId) .then(() => getMarketingSettingsBySelectionPlan( selectionPlanId, null, DEFAULT_CURRENT_PAGE, MAX_PER_PAGE ) ) - .then(() => setOpenSelectionPlanPopup(true)); + .then(() => setOpenSelectionPlanPopup(true)) + .catch(() => { + if (routeSelectionPlanId) { + history.replace(`/app/summits/${currentSummit.id}/selection-plans`); + } + }); };🤖 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/selection-plans/selection-plan-list-page.js` around lines 60 - 73, openEditModal's promise chain currently has no rejection handler so failures from getSelectionPlan or getMarketingSettingsBySelectionPlan leave the UI unchanged; add a .catch to the chain that handles errors (log/show a notification and/or update error state) and still calls setOpenSelectionPlanPopup(true) so the modal opens in a degraded state; keep references to openEditModal, getSelectionPlan, getMarketingSettingsBySelectionPlan and setOpenSelectionPlanPopup when implementing the catch behavior.src/layouts/selection-plan-layout.js (1)
41-52: 💤 Low valueLGTM, with a small readability note.
Routing is correct: the
exact strictroute on:selection_plan_id(\\d+)renders the list page (which auto-opens the edit dialog from the URL), while the non-exact one continues to delegate sub-paths (/extra-questions,/rating-types) toSelectionPlanIdLayout. Two<Route>entries with the samepathis unusual at first glance — a brief inline comment ("exact → list with auto-open dialog; non-exact → nested sub-pages") would save future readers a double-take.🤖 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/layouts/selection-plan-layout.js` around lines 41 - 52, Add a brief inline comment above the two Route entries explaining their intent: the exact/strict Route with path `${match.url}/:selection_plan_id(\\d+)` renders SelectionPlanListPage (which auto-opens the edit dialog from the URL), while the non-exact Route with the same path delegates nested sub-pages to SelectionPlanIdLayout (e.g., `/extra-questions`, `/rating-types`); place the comment near the two Route elements so future readers immediately understand why both routes share the same path.
🤖 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/forms/selection-plan-form.js`:
- Around line 191-205: handleSubmit currently chains onSubmit →
saveSelectionPlanSettings → onSaved with no error handling; update handleSubmit
to return the promise chain, add a .catch at the end, and guard the intermediate
result before using e.id. Concretely: in handleSubmit (method name) make the
call return this.props.onSubmit(this.state.entity).then((e) => { if (!e ||
!e.id) throw new Error("Missing id from onSubmit result"); return
this.props.saveSelectionPlanSettings(this.state.entity.marketing_settings,
e.id).then(() => { if (onSaved) onSaved(e); }); }).catch(err => { /* handle
error: set local state (e.g. this.setState({saveError: err, saving:false}))
and/or call this.props.onError(err) if provided, and rethrow or swallow as
appropriate */ }); ensure you reference onSubmit, saveSelectionPlanSettings,
onSaved and this.state.entity.marketing_settings in your changes.
In `@src/pages/selection-plans/selection-plan-list-page.js`:
- Around line 174-242: Remove the redundant actions.edit entry from
tableOptions: locate the tableOptions constant and delete the actions: { edit: {
onClick: handleEdit } } block so that edit/delete behavior is provided only via
the top-level onEdit={handleEdit} and onDelete={handleDelete} props passed to
MuiTable; keep tableOptions containing sortCol and sortDir unchanged and ensure
no other references to actions.edit remain.
- Around line 246-254: Add an inline comment above the Dialog usage explaining
that focus management props (disableEnforceFocus, disableAutoFocus,
disableRestoreFocus) are intentionally disabled because EditSelectionPlanPage
triggers nested SweetAlert2 modals via Swal.fire() which conflict with MUI
Dialog's focus trap; mention that this is a deliberate tradeoff to allow those
modals to function and warn future maintainers not to re-enable those props
without addressing the SweetAlert2/MUI focus conflict.
- Around line 97-102: handleDelete currently performs a silent delete via
deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()) without
user confirmation or error handling; update this flow so the table shows a
confirmation dialog (add the deleteDialogBody prop to the MuiTable instance for
this page, matching the pattern used in sponsor users table and keep
confirmButtonColor="error"), and modify handleDelete to attach a .catch(...) to
deleteSelectionPlan to surface server errors to the user (e.g., via the existing
notification/snackbar mechanism) and only call refreshSelectionPlans() on
success; ensure you reference the handleDelete function and
deleteSelectionPlan/refreshSelectionPlans identifiers when making these changes.
- Around line 75-83: The first useEffect calling getSelectionPlans should
include all referenced variables (term, perPage, order, orderDir, and
getSelectionPlans) in its dependency array (or, if it truly must only run on
mount, replace the array with [] and add a comment explaining why it must not
re-run); the second effect that checks routeSelectionPlanId must include a
stable openEditModal in its deps—wrap openEditModal in useCallback to stabilize
its identity and then add openEditModal (and routeSelectionPlanId) to that
effect's dependency array so react-hooks/exhaustive-deps is satisfied.
---
Nitpick comments:
In `@src/layouts/selection-plan-layout.js`:
- Around line 41-52: Add a brief inline comment above the two Route entries
explaining their intent: the exact/strict Route with path
`${match.url}/:selection_plan_id(\\d+)` renders SelectionPlanListPage (which
auto-opens the edit dialog from the URL), while the non-exact Route with the
same path delegates nested sub-pages to SelectionPlanIdLayout (e.g.,
`/extra-questions`, `/rating-types`); place the comment near the two Route
elements so future readers immediately understand why both routes share the same
path.
In `@src/pages/selection-plans/selection-plan-list-page.js`:
- Around line 88-95: Add a one-line inline comment in the handleEdit function
explaining that calling
history.push(`/app/summits/${currentSummit.id}/selection-plans/${selectionPlanId}`)
intentionally updates the URL to trigger the routeSelectionPlanId effect which
opens the edit modal/dialog (enables deep-linking), so future refactors know the
URL change is relied upon to show the modal rather than directly invoking a
modal open method.
- Around line 60-73: openEditModal's promise chain currently has no rejection
handler so failures from getSelectionPlan or getMarketingSettingsBySelectionPlan
leave the UI unchanged; add a .catch to the chain that handles errors (log/show
a notification and/or update error state) and still calls
setOpenSelectionPlanPopup(true) so the modal opens in a degraded state; keep
references to openEditModal, getSelectionPlan,
getMarketingSettingsBySelectionPlan and setOpenSelectionPlanPopup when
implementing the catch behavior.
🪄 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: e66a2e0b-b7c8-462d-8a25-e51770a30ccf
📒 Files selected for processing (8)
src/actions/selection-plan-actions.jssrc/components/forms/selection-plan-form.jssrc/components/inputs/import-modal/index.jsxsrc/layouts/selection-plan-id-layout.jssrc/layouts/selection-plan-layout.jssrc/pages/selection-plans/edit-selection-plan-page.jssrc/pages/selection-plans/selection-plan-list-page.jssrc/reducers/selection_plans/selection-plan-list-reducer.js
| const handleDelete = (row) => { | ||
| const selectionPlanId = row?.id || row; | ||
| if (!selectionPlanId) return; | ||
|
|
||
| deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether MuiTable wraps onDelete in a confirmation dialog and
# what `confirmButtonColor` actually controls.
fd -i 'mui' -t d
fd -i 'table' -t f --extension js --extension jsx --extension ts --extension tsx | xargs rg -nP -C3 'onDelete|confirmButtonColor' 2>/dev/null | head -200
rg -nP -C3 '"openstack-uicore-foundation/lib/components/mui/table"' --type=js --type=jsxRepository: fntechgit/summit-admin
Length of output: 10887
🏁 Script executed:
cat -n src/pages/selection-plans/selection-plan-list-page.js | sed -n '170,250p'Repository: fntechgit/summit-admin
Length of output: 2799
🏁 Script executed:
# Look for MuiTable component definition locally
find src/components -name "*table*" -o -name "*Table*" | head -20Repository: fntechgit/summit-admin
Length of output: 1142
🏁 Script executed:
# Search for MuiTable imports and their usage pattern
rg "from.*openstack-uicore-foundation.*table" -A 5 -B 2Repository: fntechgit/summit-admin
Length of output: 50378
🏁 Script executed:
sed -n '100,200p' src/pages/sponsors/sponsor-users-list-page/components/users-table.jsRepository: fntechgit/summit-admin
Length of output: 1332
🏁 Script executed:
sed -n '97,102p' src/pages/selection-plans/selection-plan-list-page.jsRepository: fntechgit/summit-admin
Length of output: 262
Add delete confirmation dialog and error handling to handleDelete.
handleDelete silently deletes selection plans without user confirmation. Unlike other MuiTable instances in the codebase (e.g., sponsor users table), this one lacks the deleteDialogBody prop required to display a confirmation dialog. confirmButtonColor="error" alone does not provide confirmation UX. Additionally, no .catch() is attached, so server-side failures leave the list unrefreshed and unsignaled to the user.
Suggested fix
const handleDelete = (row) => {
const selectionPlanId = row?.id || row;
if (!selectionPlanId) return;
- deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans());
+ deleteSelectionPlan(selectionPlanId)
+ .then(() => refreshSelectionPlans())
+ .catch(() => {
+ // surfaced via authErrorHandler; no-op
+ });
};Also add deleteDialogBody prop to MuiTable to match the pattern used elsewhere:
<MuiTable
// ...
onDelete={handleDelete}
+ deleteDialogBody={(plan) =>
+ T.translate("selection_plan_list.delete_confirmation_body", { plan })
+ }
confirmButtonColor="error"
/>📝 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 handleDelete = (row) => { | |
| const selectionPlanId = row?.id || row; | |
| if (!selectionPlanId) return; | |
| deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()); | |
| }; | |
| const handleDelete = (row) => { | |
| const selectionPlanId = row?.id || row; | |
| if (!selectionPlanId) return; | |
| deleteSelectionPlan(selectionPlanId) | |
| .then(() => refreshSelectionPlans()) | |
| .catch(() => { | |
| // surfaced via authErrorHandler; no-op | |
| }); | |
| }; |
🤖 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/selection-plans/selection-plan-list-page.js` around lines 97 - 102,
handleDelete currently performs a silent delete via
deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()) without
user confirmation or error handling; update this flow so the table shows a
confirmation dialog (add the deleteDialogBody prop to the MuiTable instance for
this page, matching the pattern used in sponsor users table and keep
confirmButtonColor="error"), and modify handleDelete to attach a .catch(...) to
deleteSelectionPlan to surface server errors to the user (e.g., via the existing
notification/snackbar mechanism) and only call refreshSelectionPlans() on
success; ensure you reference the handleDelete function and
deleteSelectionPlan/refreshSelectionPlans identifiers when making these changes.
| <Dialog | ||
| open={openSelectionPlanPopup} | ||
| onClose={handleClosePopup} | ||
| maxWidth="xl" | ||
| fullWidth | ||
| disableEnforceFocus | ||
| disableAutoFocus | ||
| disableRestoreFocus | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MUI Dialog disableEnforceFocus disableAutoFocus disableRestoreFocus accessibility implications
💡 Result:
In MUI, Dialog is built on Modal/FocusTrap focus management. The props disableEnforceFocus, disableAutoFocus, and disableRestoreFocus (and disableRestoreFocus) all change focus handling and therefore have accessibility implications. 1) disableEnforceFocus (focus trap disabled) - What it does: “If true, the modal will not prevent focus from leaving the modal while open.” [1] - Accessibility implication: This makes the modal less accessible to assistive technologies because focus may move outside the dialog while it’s open. MUI’s Modal docs explicitly say this should generally never be set to true for accessibility reasons (screen readers). [1] - Practical effect: With enforce-focus off, keyboard users/screen-reader focus can escape the dialog content, which can break the expected modal “user must interact with the dialog” pattern. (MUI documentation emphasizes the focus trap behavior for modals.) [2] 2) disableAutoFocus (initial focus shift disabled) - What it does: “If true, the modal will not automatically shift focus to itself when it opens, and replace it to the last focused element when it closes.” [1] - Accessibility implication: MUI says this should generally never be set to true because it makes the modal less accessible to assistive technologies like screen readers. [1] - Practical effect: When the dialog opens, focus may not land on the dialog content (or the intended first control). This can cause screen readers to announce the wrong element/order, or delay users reaching dialog content. - Related community symptoms: For example, autofocus behavior in dialogs can be affected by focus restoration/management; one workaround reported was using disableRestoreFocus. [3] 3) disableRestoreFocus (restoring previous focus disabled) - What it does: “If true, the modal will not restore focus to previously focused element once modal is hidden.” [1] - Accessibility implication: Because focus won’t be returned to the element that opened the dialog, dismissing the dialog can leave keyboard/screen-reader users disoriented about where they are in the app. - Practical effect: In at least some nested-dialog scenarios, disabling focus restoration is used as a workaround to make nested focus behavior work. [4][3] 4) What to watch for when you combine these props - Disabling enforce focus + disabling autofocus + disabling restore focus removes (a) trapping focus inside the dialog, (b) initial focus placement into the dialog, and (c) restoring focus on close. Together, that can significantly degrade keyboard navigation and screen-reader experience, which is why MUI’s docs caution against setting these to true. [1] 5) Recommended approach (accessibility-safe alternatives) - Prefer leaving these defaults (all false) so MUI can manage modal focus appropriately. MUI’s modal docs describe moving focus to the modal content and keeping it there until closed, plus appropriate ARIA roles. [2] - If your goal is to allow interaction in the background (e.g., a chatbot), consider whether you truly need a modal dialog. If you must, disableEnforceFocus is the knob that allows focus to leave, but you should test keyboard and screen-reader behavior carefully because MUI warns it reduces accessibility. [1] Grounding: the accessibility caveats and exact prop behaviors come from MUI’s Modal API docs (the underlying implementation used by Dialog). [1][2]
Citations:
- 1: https://mui.com/material-ui/api/modal/
- 2: https://mui.com/components/modal/
- 3: https://stackoverflow.com/questions/75644447/autofocus-not-working-on-open-form-dialog-with-button-component-in-material-ui-v
- 4: [Dialog] disableRestoreFocus only way to enable nested focus mui/material-ui#33330
🏁 Script executed:
cat -n src/pages/selection-plans/selection-plan-list-page.js | sed -n '240,260p'Repository: fntechgit/summit-admin
Length of output: 872
🏁 Script executed:
# Search for child components of this Dialog that might require focus props to be disabled
rg -A 30 'disableEnforceFocus' src/pages/selection-plans/selection-plan-list-page.jsRepository: fntechgit/summit-admin
Length of output: 1007
🏁 Script executed:
# Check for any comments explaining these focus props
rg -B 5 -A 5 'disableRestoreFocus' src/pages/selection-plans/selection-plan-list-page.jsRepository: fntechgit/summit-admin
Length of output: 435
🏁 Script executed:
# Look for TextEditorV3 or SweetAlert mentions in the file
rg -n 'TextEditorV3|SweetAlert' src/pages/selection-plans/selection-plan-list-page.jsRepository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
fd -t f 'EditSelectionPlanPage' --or -path '*selection*plan*' -type f | head -20Repository: fntechgit/summit-admin
Length of output: 311
🏁 Script executed:
# Search for EditSelectionPlanPage import
rg -n 'EditSelectionPlanPage' src/pages/selection-plans/selection-plan-list-page.jsRepository: fntechgit/summit-admin
Length of output: 170
🏁 Script executed:
# Look for TextEditorV3 or SweetAlert in the entire selection-plans directory
rg -r 'TextEditorV3|SweetAlert|swal|sweetalert' src/pages/selection-plans/Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
fd 'edit-selection-plan' src/pages/selection-plans/Repository: fntechgit/summit-admin
Length of output: 189
🏁 Script executed:
# Also search for any comments in the entire list-page file about focus management
rg -B 2 -A 2 'handleClosePopup|openSelectionPlanPopup' src/pages/selection-plans/selection-plan-list-page.js | head -40Repository: fntechgit/summit-admin
Length of output: 726
🏁 Script executed:
# Read the edit-selection-plan-page.js file to see what components it uses
head -100 src/pages/selection-plans/edit-selection-plan-page.jsRepository: fntechgit/summit-admin
Length of output: 3168
🏁 Script executed:
# Search specifically for TextEditorV3, SweetAlert, or other nested modals in edit-selection-plan-page
rg -n 'TextEditorV3|SweetAlert|swal|Dialog|Modal' src/pages/selection-plans/edit-selection-plan-page.js | head -30Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Look for any comments about focus management in the entire codebase related to dialogs
rg -n 'disable.*Focus|focus.*trap|accessibility' src/pages/selection-plans/Repository: fntechgit/summit-admin
Length of output: 321
🏁 Script executed:
rg -n 'Swal\.' src/pages/selection-plans/edit-selection-plan-page.jsRepository: fntechgit/summit-admin
Length of output: 125
🏁 Script executed:
# Also check SelectionPlanForm to see if it uses any modals
rg -n 'Swal\.|Dialog|Modal' src/components/forms/selection-plan-form.js | head -20Repository: fntechgit/summit-admin
Length of output: 550
Document why focus management is disabled — this Dialog contains nested SweetAlert2 dialogs.
The EditSelectionPlanPage component uses Swal.fire() for three different confirmation modals, which conflict with MUI Dialog's focus trap. Disabling disableEnforceFocus, disableAutoFocus, and disableRestoreFocus is a necessary workaround for this interaction, but add an inline comment explaining this to prevent accidental re-enablement and to clarify the tradeoff for future maintainers.
🤖 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/selection-plans/selection-plan-list-page.js` around lines 246 -
254, Add an inline comment above the Dialog usage explaining that focus
management props (disableEnforceFocus, disableAutoFocus, disableRestoreFocus)
are intentionally disabled because EditSelectionPlanPage triggers nested
SweetAlert2 modals via Swal.fire() which conflict with MUI Dialog's focus trap;
mention that this is a deliberate tradeoff to allow those modals to function and
warn future maintainers not to re-enable those props without addressing the
SweetAlert2/MUI focus conflict.
| } from "../../reducers/selection_plans/selection-plan-reducer"; | ||
| import history from "../../history"; | ||
|
|
||
| class SelectionPlanForm extends React.Component { |
There was a problem hiding this comment.
please refactor to functional component
There was a problem hiding this comment.
I thought this would be out of scope so I didn't do It initially but I'm adding this now.
| entity, | ||
| allowedMembers, | ||
| errors, | ||
| hideHeader = false, |
There was a problem hiding this comment.
do we really need a prop ? I don't see where this is used WITH the header
There was a problem hiding this comment.
Just did a refactor so this is not needed anymore! Thanks
| totalSelectionPlans: total, | ||
| currentPage: current_page, | ||
| lastPage: last_page | ||
| lastPage: last_page, |
There was a problem hiding this comment.
no need to do this if you are already doing it in the request action
| getSelectionPlans(term, currentPage, perPage, order, orderDir); | ||
|
|
||
| const handleEdit = (selectionPlan) => { | ||
| const selectionPlanId = selectionPlan?.id || selectionPlan; |
There was a problem hiding this comment.
why selectionPlan can be both an object or an id ? we should consolidate this on either one or the other
| if (!selectionPlanId) return; | ||
|
|
||
| const handleEdit = (selectionPlanId) => { | ||
| history.push( |
There was a problem hiding this comment.
@smarcet linking the user to /id kind of defeats the purpose of having the edit screen in a modal, shouldn't we just open the modal in this case ? or do we want the url to reflect the edit action ?
There was a problem hiding this comment.
The edit page for a selection plan uses sub-routes (e.g. /extra-questions, /rating-types) that depend on the selection_plan_id URL parameter being present. Removing the ID from the URL on edit would break navigation to those nested routes inside SelectionPlanIdLayout. Keeping the ID ensures the layout can resolve the correct selection plan context for all child routes.
There was a problem hiding this comment.
Not really, you can just open the popup on edit but keep the /id a valid route for the rest of the routes
| const handleNew = () => { | ||
| history.push(`/app/summits/${currentSummit.id}/selection-plans/new`); | ||
| resetSelectionPlanForm(); | ||
| setOpenSelectionPlanPopup(true); |
There was a problem hiding this comment.
on edit you do change the url, but on new not .. this is not consistent
There was a problem hiding this comment.
The /new path existed in the layout but was never functional — it just redirected back to the list. Creation is handled directly from the list page by opening the form with id=0. Once the plan is saved, the API returns the new ID and the user is redirected to the proper edit URL. There was no reason to keep a dead redirect route.
There was a problem hiding this comment.
Also, in task description it was requested to delete this route legacy route https://showadmin.dev.fnopen.com/app/summits/73/selection-plans/new should be removed
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
Assuming the form is out of scope. Good job @priscila-moneo
| }; | ||
|
|
||
| return ( | ||
| <form className="selection-plan-form"> |
There was a problem hiding this comment.
shouldn't we use MUI components ? or this file is out of scope ? cc @smarcet
There was a problem hiding this comment.
good observation, I had some time so I updated this file with MUI components
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
c8488e1 to
e66daff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/selection-plans/selection-plan-list-page.js (1)
96-100:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
handleDeleteto accept the row id, not a row object.Line 97 returns early for the value
MuiTableactually sends toonDelete, so delete clicks become no-ops here.Suggested fix
- const handleDelete = (selectionPlan) => { - if (!selectionPlan?.id) return; - - deleteSelectionPlan(selectionPlan.id).then(() => refreshSelectionPlans()); + const handleDelete = (selectionPlanId) => { + if (!selectionPlanId) return; + + deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()); };Based on learnings: When using
MuiTablefromopenstack-uicore-foundation/lib/components/mui/tablein this codebase,onDeleteis called with the primitive row identifier, not the full row object.🤖 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/selection-plans/selection-plan-list-page.js` around lines 96 - 100, The handleDelete function currently expects a selectionPlan object and guards on selectionPlan?.id, but MuiTable's onDelete passes the primitive row id instead of the row object; update handleDelete to accept an id parameter (e.g., id) and return early if id is falsy, then call deleteSelectionPlan(id).then(() => refreshSelectionPlans()); adjust any references that call handleDelete to ensure they pass the id directly; keep function name handleDelete and preserve use of deleteSelectionPlan and refreshSelectionPlans for locating the code.
🤖 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/selection-plans/selection-plan-list-page.js`:
- Around line 255-256: The IconButton rendering the CloseIcon (the element using
IconButton with onClick={handleClosePopup} and CloseIcon) lacks an accessible
name; add an aria-label (for example aria-label="Close" or a localized string)
to the IconButton so screen readers announce its purpose and optionally add a
tooltip/title if desired for sighted users.
---
Duplicate comments:
In `@src/pages/selection-plans/selection-plan-list-page.js`:
- Around line 96-100: The handleDelete function currently expects a
selectionPlan object and guards on selectionPlan?.id, but MuiTable's onDelete
passes the primitive row id instead of the row object; update handleDelete to
accept an id parameter (e.g., id) and return early if id is falsy, then call
deleteSelectionPlan(id).then(() => refreshSelectionPlans()); adjust any
references that call handleDelete to ensure they pass the id directly; keep
function name handleDelete and preserve use of deleteSelectionPlan and
refreshSelectionPlans for locating the code.
🪄 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: 8e9228ae-e203-4da7-9b8d-a3a6c7c0c3f3
📒 Files selected for processing (9)
src/actions/selection-plan-actions.jssrc/components/forms/selection-plan-form.jssrc/components/inputs/import-modal/index.jsxsrc/layouts/selection-plan-id-layout.jssrc/layouts/selection-plan-layout.jssrc/pages/selection-plans/edit-selection-plan-page.jssrc/pages/selection-plans/selection-plan-list-page.jssrc/pages/sponsors/__tests__/edit-sponsor-page.test.jssrc/reducers/selection_plans/selection-plan-list-reducer.js
✅ Files skipped from review due to trivial changes (1)
- src/components/inputs/import-modal/index.jsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pages/selection-plans/edit-selection-plan-page.js
- src/reducers/selection_plans/selection-plan-list-reducer.js
- src/layouts/selection-plan-layout.js
- src/actions/selection-plan-actions.js
- src/components/forms/selection-plan-form.js
| <IconButton size="small" onClick={handleClosePopup}> | ||
| <CloseIcon fontSize="small" /> |
There was a problem hiding this comment.
Add an accessible name to the close button.
The icon-only IconButton on Line 255 has no label, so assistive tech will announce a generic button. Add an aria-label.
Suggested fix
- <IconButton size="small" onClick={handleClosePopup}>
+ <IconButton
+ size="small"
+ onClick={handleClosePopup}
+ aria-label={T.translate("general.close")}
+ >
<CloseIcon fontSize="small" />
</IconButton>🤖 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/selection-plans/selection-plan-list-page.js` around lines 255 -
256, The IconButton rendering the CloseIcon (the element using IconButton with
onClick={handleClosePopup} and CloseIcon) lacks an accessible name; add an
aria-label (for example aria-label="Close" or a localized string) to the
IconButton so screen readers announce its purpose and optionally add a
tooltip/title if desired for sighted users.
2 participants
ref: https://app.clickup.com/t/86b9pbavj
Summary by CodeRabbit
New Features
Improvements
Bug Fixes