feat: move selection plans grid to mui#917
Open
priscila-moneo wants to merge 5 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:
📝 WalkthroughWalkthroughModal-driven selection-plans list/editor using hooks; add per-page pagination through actions/reducer/components, update routing to redirect to the list, adjust imports, and add tests for save/delete flows. ChangesSelection Plans Modal-Driven UI Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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 unit tests (beta)
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
santipalenque
approved these changes
May 8, 2026
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
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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/forms/selection-plan-form.js (1)
145-151: 💤 Low valueConsider guarding against undefined
marketing_settings.The code assumes
marketing_settingsexists when callinghasOwnProperty, but the render code uses optional chaining (?.) when reading these values. IfpropsEntity.marketing_settingsis ever undefined, this will throw a TypeError. The same pattern appears inhandleOnSwitchChange.♻️ Suggested defensive guard
if (id.startsWith("cfp_")) { + if (!newEntity.marketing_settings) { + newEntity.marketing_settings = {}; + } if ( !Object.prototype.hasOwnProperty.call(newEntity.marketing_settings, id) ) {Apply similar guard in
handleOnSwitchChangearound line 247.🤖 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/components/forms/selection-plan-form.js` around lines 145 - 151, The code assumes newEntity.marketing_settings exists before calling Object.prototype.hasOwnProperty.call and before assigning keys; add a defensive guard to initialize marketing_settings when missing (e.g., if (!newEntity.marketing_settings) newEntity.marketing_settings = {}) before the block that checks id.startsWith("cfp_") and likewise in handleOnSwitchChange so both the hasOwnProperty call and subsequent assignment to newEntity.marketing_settings[id].value are safe when propsEntity.marketing_settings is undefined; update references to newEntity.marketing_settings in those functions (the id.startsWith("cfp_") branch and the handleOnSwitchChange handler) to rely on the initialized object.src/components/forms/__tests__/selection-plan-form.test.js (1)
150-172: ⚡ Quick winConsider asserting cleanup after successful save.
Test 3 verifies that
onSavingChange(false)is called after rejection, but this test does not verify the same cleanup happens after success. According to the implementation in context snippet 1,.finally()always callsonSavingChange(false), so asserting that behavior here would make the test more complete.✨ Suggested enhancement
await act(async () => { resolveSave({ id: 1 }); await Promise.resolve(); }); + + await waitFor(() => { + expect(onSavingChange).toHaveBeenCalledWith(false); + });🤖 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/components/forms/__tests__/selection-plan-form.test.js` around lines 150 - 172, Add an assertion that the component runs its cleanup after a successful save: after resolving the mocked onSubmit promise (resolveSave) in the test "disables submit and calls onSavingChange(true) while save is pending", assert that onSavingChange was later called with false and that the submit button is enabled again (verifying the .finally() cleanup path implemented around onSubmit/onSavingChange). Reference the mocked onSubmit, the onSavingChange spy, and resolveSave when adding these assertions.
🤖 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.
Nitpick comments:
In `@src/components/forms/__tests__/selection-plan-form.test.js`:
- Around line 150-172: Add an assertion that the component runs its cleanup
after a successful save: after resolving the mocked onSubmit promise
(resolveSave) in the test "disables submit and calls onSavingChange(true) while
save is pending", assert that onSavingChange was later called with false and
that the submit button is enabled again (verifying the .finally() cleanup path
implemented around onSubmit/onSavingChange). Reference the mocked onSubmit, the
onSavingChange spy, and resolveSave when adding these assertions.
In `@src/components/forms/selection-plan-form.js`:
- Around line 145-151: The code assumes newEntity.marketing_settings exists
before calling Object.prototype.hasOwnProperty.call and before assigning keys;
add a defensive guard to initialize marketing_settings when missing (e.g., if
(!newEntity.marketing_settings) newEntity.marketing_settings = {}) before the
block that checks id.startsWith("cfp_") and likewise in handleOnSwitchChange so
both the hasOwnProperty call and subsequent assignment to
newEntity.marketing_settings[id].value are safe when
propsEntity.marketing_settings is undefined; update references to
newEntity.marketing_settings in those functions (the id.startsWith("cfp_")
branch and the handleOnSwitchChange handler) to rely on the initialized object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5256af02-8c39-4b53-b37b-4d45ea80ad1e
📒 Files selected for processing (7)
src/actions/__tests__/selection-plan-actions.test.jssrc/actions/selection-plan-actions.jssrc/components/forms/__tests__/selection-plan-form.test.jssrc/components/forms/selection-plan-form.jssrc/pages/selection-plans/__tests__/selection-plan-list-page.test.jssrc/pages/selection-plans/edit-selection-plan-page.jssrc/pages/selection-plans/selection-plan-list-page.js
✅ Files skipped from review due to trivial changes (1)
- src/actions/tests/selection-plan-actions.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/selection-plans/edit-selection-plan-page.js
- src/pages/selection-plans/selection-plan-list-page.js
smarcet
reviewed
Jun 12, 2026
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
13befb9 to
14973bc
Compare
smarcet
reviewed
Jun 17, 2026
| const handleSort = (index, key, dir) => { | ||
| getSelectionPlans(term, currentPage, key, dir); | ||
| const handleSavingChange = (saving) => { | ||
| isSavingRef.current = saving; |
There was a problem hiding this comment.
Popup pattern deviation: duplicated isSaving + onSavingChange callback
The dominant pattern across this codebase (media-file-types, form-templates, tags, etc.) is:
isSavinglives exclusively in the popup/form component- the parent passes a single
onSaveprop that returns a Promise - the popup drives its own close via
.then(() => onClose())
Here the form already has its own isSaving (correct), but it also calls onSavingChange(true/false) to bubble the state up to the list page, which then maintains a parallel isSaving + isSavingRef to control the Dialog chrome (escape key, close icon). The isSavingRef workaround exists precisely because of this cross-layer sync.
The cleaner approach: extract the Dialog + DialogTitle into a dedicated SelectionPlanPopup component that receives isSaving as a prop from the form directly, or passes onSavingChange only down to that wrapper — eliminating the ref and the duplicated state in the list page.
Note: the complexity of selection plan (embedded full-page form, multiple sub-resources) makes this a harder refactor than the simpler popups. The deviation is understandable given the constraints, but worth tracking.
There was a problem hiding this comment.
https://app.clickup.com/t/9014802374/86bag2zk7
Partially addressed, but I would not consider this fully resolved yet.
The PR moved isSaving out of the list page and into SelectionPlanPopup, and the dialog now disables close/escape/save while saving. However, it still does not follow the established popup/save pattern from the task.
The expected pattern is: the parent owns the Redux save action and list reload, the popup owns isSaving and closes itself only after the onSave promise resolves, and the popup/form boundary uses a single onSave prop.
Currently the save flow is still split across the form, popup, and parent through onSubmit, saveSelectionPlanSettings, onSaved, and onSavingChange, and the popup still uses an isSavingRef. This is different from the reference media-file-types pattern.
Please refactor this to the established single-onSave popup pattern before resolving the thread.
Also, the current implementation still uses isSavingRef to guard close operations. That should not be necessary if the popup owns the save flow: handleClose can rely directly on local isSaving, as in the reference popup implementations.
smarcet
reviewed
Jun 17, 2026
smarcet
reviewed
Jun 17, 2026
smarcet
requested changes
Jun 17, 2026
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comments
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
14973bc to
b93c014
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/forms/__tests__/selection-plan-form.test.js (2)
94-98: 💤 Low valueAdd
time_zone_idtocurrentSummitfor test robustness.The form uses
currentSummit.time_zone_idfor date normalization inbuildInitialValuesandhandleFormikSubmit. Tests pass because all date fields arenull, but if any test later sets date values,moment.tz(value, undefined)will behave unexpectedly.const defaultProps = { entity: defaultEntity, errors: {}, - currentSummit: { id: 1 }, + currentSummit: { id: 1, time_zone_id: "UTC" },🤖 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/components/forms/__tests__/selection-plan-form.test.js` around lines 94 - 98, In the defaultProps object, add the missing time_zone_id property to the currentSummit object. Set it to a valid timezone string value (such as "UTC" or another appropriate timezone identifier) so that when the form's buildInitialValues and handleFormikSubmit methods use currentSummit.time_zone_id for date normalization with moment.tz(), the timezone will be properly defined rather than undefined.
18-68: 💤 Low valueIncomplete mocks for form's direct imports.
The form imports components from specific subpaths (e.g.,
openstack-uicore-foundation/lib/components/inputs/text-input,openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker) but the mock on line 18 targets the parent/lib/componentspath. Jest doesn't automatically mock subpaths.Missing mocks for:
MuiFormikDatepicker,Input(text-input),Panel,Dropdown,TextEditorV3,SortableTable(mui),SimpleLinkList.Tests may pass now because these components render without throwing, but this creates fragility—if any component adds required context or validation, tests will fail unexpectedly.
🧪 Add targeted mocks for imported components
+jest.mock( + "openstack-uicore-foundation/lib/components/inputs/text-input", + () => ({ + __esModule: true, + default: ({ id, value, onChange }) => ( + <input id={id} value={value ?? ""} onChange={onChange} /> + ) + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker", + () => ({ + __esModule: true, + default: () => null + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/sections/panel", + () => ({ + __esModule: true, + default: ({ children }) => <div>{children}</div> + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/inputs/dropdown", + () => ({ + __esModule: true, + default: () => null + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/inputs/editor-input-v3", + () => ({ + __esModule: true, + default: () => null + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/mui/sortable-table", + () => ({ + __esModule: true, + default: () => null + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/simple-link-list", + () => ({ + __esModule: true, + default: () => null + }) +);🤖 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/components/forms/__tests__/selection-plan-form.test.js` around lines 18 - 68, The current mocks only target the parent path `openstack-uicore-foundation/lib/components` but the form imports components from specific subpaths like `openstack-uicore-foundation/lib/components/inputs/text-input` and `openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker`. Add individual jest.mock() calls for each specific subpath that the selection-plan-form component imports (including MuiFormikDatepicker, Input from text-input, Panel, Dropdown, TextEditorV3, SortableTable from mui, and SimpleLinkList), ensuring each mock properly exports the component as either a default export or named export matching what the component expects.
🤖 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 83-89: The handleDelete function currently expects selectionPlan
to be an object and tries to access selectionPlan?.id, but MuiTable's onDelete
callback passes only the primitive ID value directly (not the full row object).
Change the parameter name from selectionPlan to id, update the null check from
if (!selectionPlan?.id) to if (!id), and pass id directly to deleteSelectionPlan
instead of selectionPlan.id.
---
Nitpick comments:
In `@src/components/forms/__tests__/selection-plan-form.test.js`:
- Around line 94-98: In the defaultProps object, add the missing time_zone_id
property to the currentSummit object. Set it to a valid timezone string value
(such as "UTC" or another appropriate timezone identifier) so that when the
form's buildInitialValues and handleFormikSubmit methods use
currentSummit.time_zone_id for date normalization with moment.tz(), the timezone
will be properly defined rather than undefined.
- Around line 18-68: The current mocks only target the parent path
`openstack-uicore-foundation/lib/components` but the form imports components
from specific subpaths like
`openstack-uicore-foundation/lib/components/inputs/text-input` and
`openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker`. Add
individual jest.mock() calls for each specific subpath that the
selection-plan-form component imports (including MuiFormikDatepicker, Input from
text-input, Panel, Dropdown, TextEditorV3, SortableTable from mui, and
SimpleLinkList), ensuring each mock properly exports the component as either a
default export or named export matching what the component expects.
🪄 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: 10a5bc10-1a24-4623-b74c-1416870b6991
📒 Files selected for processing (13)
src/actions/__tests__/selection-plan-actions.test.jssrc/actions/selection-plan-actions.jssrc/components/forms/__tests__/selection-plan-form.test.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/__tests__/selection-plan-list-page.test.jssrc/pages/selection-plans/edit-selection-plan-page.jssrc/pages/selection-plans/selection-plan-list-page.jssrc/pages/selection-plans/selection-plan-popup.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 (7)
- src/layouts/selection-plan-id-layout.js
- src/pages/selection-plans/tests/selection-plan-list-page.test.js
- src/pages/sponsors/tests/edit-sponsor-page.test.js
- src/actions/tests/selection-plan-actions.test.js
- src/reducers/selection_plans/selection-plan-list-reducer.js
- src/actions/selection-plan-actions.js
- src/pages/selection-plans/edit-selection-plan-page.js
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/i18n/en.json`:
- Around line 571-572: The placeholder text for the link_question and
link_presentation_action_type keys in the English localization file contains the
misspelled word "Existent" which should be "Existing" for correct grammar and
user-facing copy. Update both occurrences by replacing "Existent" with
"Existing" in the respective message strings.
🪄 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: 063b12fd-914d-43d5-b0bb-eadbec7d7a1b
📒 Files selected for processing (5)
src/actions/selection-plan-actions.jssrc/components/forms/__tests__/selection-plan-form.test.jssrc/components/forms/selection-plan-form.jssrc/i18n/en.jsonsrc/pages/selection-plans/selection-plan-popup.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/selection-plans/selection-plan-popup.js
smarcet
reviewed
Jun 23, 2026
| <label> | ||
| {T.translate("edit_selection_plan.submission_end_date")} | ||
| </label> | ||
| <MuiFormikDatepicker name="submission_end_date" /> |
There was a problem hiding this comment.
@priscila-moneo this is a regresion
original code send to api the time too
DatePicker only allow to select date portion u should be using https://mui.com/x/react-date-pickers/date-time-picker/
smarcet
requested changes
Jun 23, 2026
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review this still has major deviations
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
8fa5e75 to
aea5f94
Compare
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Selection Plans admin UX by migrating the list/grid and editor flow to MUI components, introducing a modal-based editor, and wiring the list to support paging/sorting/search with consistent refresh behavior after mutations.
Changes:
- Reworked Selection Plans list page to use
MuiTable, MUI layout controls, and a modal-based editor (SelectionPlanPopup). - Refactored
SelectionPlanFormfrom a class component to a hook-based Formik + MUI implementation (including a new Formik DateTimePicker wrapper). - Updated selection plan list/action plumbing to carry paging/sort/search state through requests and added/expanded Jest coverage.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/reducers/selection_plans/selection-plan-list-reducer.js | Tracks list query parameters in reducer state and updates list payload handling. |
| src/pages/sponsors/tests/edit-sponsor-page.test.js | Adds an additional mock to stabilize sponsor page tests. |
| src/pages/selection-plans/selection-plan-popup.js | New MUI Dialog-based popup wrapper around the selection plan editor. |
| src/pages/selection-plans/selection-plan-list-page.js | Migrates list UI to MUI, adds paging/sorting/search integration and modal edit/create flow. |
| src/pages/selection-plans/edit-selection-plan-page.js | Adjusts editor page to act as a form container used by the popup (passes callbacks). |
| src/pages/selection-plans/tests/selection-plan-list-page.test.js | Adds coverage for list refresh behavior after save/delete. |
| src/layouts/selection-plan-layout.js | Redirects /new route back to list (supports modal-driven creation). |
| src/layouts/selection-plan-id-layout.js | Removes direct edit route under /:selection_plan_id and redirects back to list. |
| src/i18n/en.json | Adds new delete-confirm copy and fixes “Existent” → “Existing”; adds new placeholders for searches. |
| src/components/mui/formik-inputs/mui-formik-datetimepicker.js | New reusable Formik + MUI X DateTimePicker wrapper. |
| src/components/inputs/import-modal/index.jsx | Switches to the dedicated UploadInput import path used elsewhere in the codebase. |
| src/components/forms/selection-plan-form.js | Major refactor to hooks/Formik/MUI, tabbed UI, and improved save guarding. |
| src/components/forms/tests/selection-plan-form.test.js | Adds tests for save guarding and post-save callback behavior. |
| src/actions/selection-plan-actions.js | Extends list action to support per-page and request payload state; switches success messaging to snackbar handler. |
| src/actions/tests/selection-plan-actions.test.js | Adds tests asserting saveSelectionPlan returns a resolving Promise and dispatch ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+163
to
+168
| useEffect(() => { | ||
| scrollToError(propsErrors); | ||
| if (propsErrors && Object.keys(propsErrors).length > 0) { | ||
| formik.setErrors(propsErrors); | ||
| } | ||
| }, [propsErrors]); |
Comment on lines
63
to
67
| ...state, | ||
| selectionPlans, | ||
| totalSelectionPlans: total, | ||
| currentPage: current_page, | ||
| lastPage: last_page | ||
| }; |
Comment on lines
+34
to
+37
| <button | ||
| type="button" | ||
| onClick={() => onDelete({ id: 1, name: "CFP 2026" })} | ||
| > |
| <DialogTitle sx={{ display: "flex", justifyContent: "space-between" }}> | ||
| {isEditing ? T.translate("general.edit") : T.translate("general.add")}{" "} | ||
| {T.translate("edit_selection_plan.selection_plan")} | ||
| <IconButton size="small" onClick={handleClose} disabled={isSaving}> |
smarcet
reviewed
Jun 25, 2026
| const formik = useFormik({ | ||
| initialValues: buildInitialValues(propsEntity, currentSummit.time_zone_id), | ||
| onSubmit: handleFormikSubmit, | ||
| enableReinitialize: true, |
There was a problem hiding this comment.
@priscila-moneo this is breaking the form changes
when i add an event type the values changes at main tab got reset
smarcet
reviewed
Jun 25, 2026
| Table, | ||
| Dropdown | ||
| } from "openstack-uicore-foundation/lib/components"; | ||
| import Input from "openstack-uicore-foundation/lib/components/inputs/text-input"; |
There was a problem hiding this comment.
this is not a mui control
the reason of this migration is using MUI control
u should be using
the MUI counter part
smarcet
reviewed
Jun 25, 2026
| import Input from "openstack-uicore-foundation/lib/components/inputs/text-input"; | ||
| import SortableTable from "openstack-uicore-foundation/lib/components/mui/sortable-table"; | ||
| import Table from "openstack-uicore-foundation/lib/components/mui/table"; | ||
| import Dropdown from "openstack-uicore-foundation/lib/components/inputs/dropdown"; |
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
aea5f94 to
fcfe8e4
Compare
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
fcfe8e4 to
f432f25
Compare
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/86b9pbavj
Summary by CodeRabbit
onSaved).