feature: migrate media upload types grid to MUI#1005
Open
santipalenque 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:
📝 WalkthroughWalkthroughMedia upload management moves from separate route-based forms to an in-page Formik dialog. List actions, routing, Redux pagination state, snackbar handling, translations, and dialog/list tests are updated. ChangesMedia uploads dialog rewrite
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MediaUploadListPage
participant MediaUploadDialog
participant saveMediaUpload
participant getMediaUploads
User->>MediaUploadListPage: open add or edit dialog
MediaUploadListPage->>MediaUploadDialog: provide entity and errors
User->>MediaUploadDialog: submit form
MediaUploadDialog->>saveMediaUpload: submit normalized entity
saveMediaUpload-->>MediaUploadDialog: resolve or reject
MediaUploadDialog-->>MediaUploadListPage: close after success
MediaUploadListPage->>getMediaUploads: refresh list
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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/media-upload-actions.js (1)
238-255: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap
deleteMediaUploadin loading state
deleteMediaUploadis the only action here that skipsstartLoading()/stopLoading(). The list page also doesn’t disable delete elsewhere, so a second delete can be fired while the first request is still in flight.🤖 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/media-upload-actions.js` around lines 238 - 255, Wrap deleteMediaUpload in the same loading lifecycle used by the other media-upload actions: start loading before the delete request begins and stop loading in a finally path after it completes or fails. Update the deleteMediaUpload thunk in media-upload-actions.js to dispatch startLoading()/stopLoading() around deleteRequest so the UI is blocked while the request is in flight and duplicate deletes can’t be triggered.
🤖 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/actions/media-upload-actions.js`:
- Around line 147-194: The saveMediaUpload thunk in media-upload-actions.js can
reject after snackbarErrorHandler, so callers using
saveMediaUpload(...).then(...) need to handle failures explicitly. Update the
caller(s) of saveMediaUpload to add a .catch() (or equivalent rejection
handling) alongside the existing .then() success flow, so rejected
putRequest/postRequest promises do not become unhandled rejections. Use the
saveMediaUpload symbol to find the async action and preserve the current success
snackbar behavior.
In `@src/pages/media_uploads/components/__tests__/media-upload-dialog.test.js`:
- Around line 208-223: The media upload dialog validation is still rejecting a
cleared max_size field because positiveNumberValidation() only allows integers
and min(0), so onSubmit never reaches onSave when the input is empty. Update the
validation/schema used by the media-upload-dialog flow so the max_size field can
accept an empty string and normalize it to 0 (or the intended empty-state value)
before submission, and keep the test in media-upload-dialog.test.js aligned with
the behavior by asserting the submitted value from the dialog’s save path.
---
Outside diff comments:
In `@src/actions/media-upload-actions.js`:
- Around line 238-255: Wrap deleteMediaUpload in the same loading lifecycle used
by the other media-upload actions: start loading before the delete request
begins and stop loading in a finally path after it completes or fails. Update
the deleteMediaUpload thunk in media-upload-actions.js to dispatch
startLoading()/stopLoading() around deleteRequest so the UI is blocked while the
request is in flight and duplicate deletes can’t be triggered.
🪄 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: 66af4e14-fb5e-4488-a5b7-cd7d79d61583
📒 Files selected for processing (11)
src/actions/media-upload-actions.jssrc/components/forms/media-upload-form.jssrc/i18n/en.jsonsrc/layouts/media-upload-layout.jssrc/pages/media_uploads/__tests__/media-upload-list-page.test.jssrc/pages/media_uploads/components/__tests__/media-upload-dialog.test.jssrc/pages/media_uploads/components/media-upload-dialog.jssrc/pages/media_uploads/edit-media-upload-page.jssrc/pages/media_uploads/media-upload-list-page.jssrc/reducers/media_uploads/media-upload-reducer.jssrc/reducers/media_uploads/media-uploads-list-reducer.js
💤 Files with no reviewable changes (2)
- src/pages/media_uploads/edit-media-upload-page.js
- src/components/forms/media-upload-form.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 `@package.json`:
- Line 96: The package.json dependency for openstack-uicore-foundation is pinned
to a beta version instead of a stable release. Update the version in
package.json to the stable release currently in use unless this PR explicitly
requires the beta, and keep the dependency aligned with the stable tag for this
package.
🪄 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: 074439ba-9f84-476f-881b-dc0ce401054b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.jsonsrc/pages/media_uploads/components/__tests__/media-upload-dialog.test.jssrc/pages/media_uploads/components/media-upload-dialog.jssrc/pages/media_uploads/media-upload-list-page.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/media_uploads/components/media-upload-dialog.js
- src/pages/media_uploads/components/tests/media-upload-dialog.test.js
- src/pages/media_uploads/media-upload-list-page.js
There was a problem hiding this comment.
Pull request overview
This PR migrates the “Media Upload Types” list + edit/create flow to the newer MUI-based UI Core components, replacing the dedicated edit page with an in-page dialog while also improving list paging/total tracking and snackbar-based feedback.
Changes:
- Replaced the legacy list/table/search/pagination UI with MUI equivalents and introduced an in-page
MediaUploadDialogfor create/edit. - Updated media upload actions/reducers to support totals/paging metadata and snackbar success/error handling.
- Added Jest/RTL coverage for the new dialog and the updated list page behavior.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates lockfile for the newer openstack-uicore-foundation dependency. |
| package.json | Bumps openstack-uicore-foundation to 5.0.41-beta.0. |
| src/actions/media-upload-actions.js | Migrates error/success handling to snackbar handlers; adjusts list request payload; updates save/copy flows. |
| src/reducers/media_uploads/media-uploads-list-reducer.js | Tracks totalMediaUploads and threads perPage through request state. |
| src/reducers/media_uploads/media-upload-reducer.js | Clears errors on receive and reset paths for cleaner dialog behavior. |
| src/pages/media_uploads/media-upload-list-page.js | Migrates the list page to MUI table/search; adds dialog-based create/edit/delete behavior. |
| src/pages/media_uploads/edit-media-upload-page.js | Removes the dedicated edit page (replaced by dialog flow). |
| src/pages/media_uploads/components/media-upload-dialog.js | Adds the new MUI/Formik/Yup dialog for creating/editing media upload types. |
| src/pages/media_uploads/components/tests/media-upload-dialog.test.js | Adds unit tests for dialog behaviors (validation, saving, TTL toggle, etc.). |
| src/pages/media_uploads/tests/media-upload-list-page.test.js | Adds unit tests for list page wiring (fetch on mount, paging/sort/search, dialog open/save/delete/copy). |
| src/layouts/media-upload-layout.js | Removes now-obsolete routes and simplifies the layout to render only the list page. |
| src/i18n/en.json | Adds new strings used by snackbar and dialog UI (e.g., copied success, “minutes”). |
| src/components/forms/media-upload-form.js | Removes the legacy bootstrap-based form (replaced by dialog). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
santipalenque
force-pushed
the
feature/migrate-media-upload-types
branch
from
July 9, 2026 15:52
2aa7e2f to
d25c54d
Compare
smarcet
self-requested a review
July 13, 2026 14:42
smarcet
reviewed
Jul 13, 2026
smarcet
reviewed
Jul 13, 2026
smarcet
reviewed
Jul 13, 2026
santipalenque
force-pushed
the
feature/migrate-media-upload-types
branch
from
July 13, 2026 20:02
d25c54d to
2ee5084
Compare
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/layouts/media-upload-layout.js`:
- Line 34: Remove the strict prop from the Route rendering MediaUploadListPage,
while retaining exact, match.url, and the existing component. This allows the
trailing-slash form to resolve without changing the route’s exact path matching.
🪄 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: c16df829-b8e6-430b-ba6b-adeaf215b72a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
src/actions/media-upload-actions.jssrc/components/forms/media-upload-form.jssrc/i18n/en.jsonsrc/layouts/media-upload-layout.jssrc/pages/media_uploads/__tests__/media-upload-list-page.test.jssrc/pages/media_uploads/components/__tests__/media-upload-dialog.test.jssrc/pages/media_uploads/components/media-upload-dialog.jssrc/pages/media_uploads/edit-media-upload-page.jssrc/pages/media_uploads/media-upload-list-page.jssrc/reducers/media_uploads/media-upload-reducer.jssrc/reducers/media_uploads/media-uploads-list-reducer.js
💤 Files with no reviewable changes (2)
- src/components/forms/media-upload-form.js
- src/pages/media_uploads/edit-media-upload-page.js
🚧 Files skipped from review as they are similar to previous changes (8)
- src/reducers/media_uploads/media-upload-reducer.js
- src/i18n/en.json
- src/reducers/media_uploads/media-uploads-list-reducer.js
- src/pages/media_uploads/components/tests/media-upload-dialog.test.js
- src/pages/media_uploads/tests/media-upload-list-page.test.js
- src/pages/media_uploads/components/media-upload-dialog.js
- src/pages/media_uploads/media-upload-list-page.js
- src/actions/media-upload-actions.js
| }} | ||
| /> | ||
| <Switch> | ||
| <Route exact strict path={match.url} component={MediaUploadListPage} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'FILES\n'
git ls-files 'src/layouts/media-upload-layout.js' 'src/**/*media*upload*' 'src/**/*route*' 'src/**/*link*' | sed 's#^`#-` #'
printf '\nMEDIA UPLOAD LAYOUT\n'
sed -n '1,140p' src/layouts/media-upload-layout.js
printf '\nREFERENCES\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'media-uploads|MediaUploadLayout|/new|media_upload_id|match\.url|strict exact|strict' srcRepository: fntechgit/summit-admin
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
# placeholder to keep shell capability active if needed later
PYRepository: fntechgit/summit-admin
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'MEDIA-RELATED ROUTE FILES\n'
fd -a 'media.*upload|upload.*media|media-upload' src
printf '\nPOTENTIAL ROUTE DEFINITIONS\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'path=.*media|Route .*media|Redirect .*media|history\.push\(.*media|to=.*media' srcRepository: fntechgit/summit-admin
Length of output: 1280
Remove strict from this route. exact already blocks the removed child paths; strict also makes the trailing-slash form (/app/.../media-uploads/) fall through to NoMatchPage.
Proposed fix
- <Route exact strict path={match.url} component={MediaUploadListPage} />
+ <Route exact path={match.url} component={MediaUploadListPage} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Route exact strict path={match.url} component={MediaUploadListPage} /> | |
| <Route exact path={match.url} component={MediaUploadListPage} /> |
🤖 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/media-upload-layout.js` at line 34, Remove the strict prop from
the Route rendering MediaUploadListPage, while retaining exact, match.url, and
the existing component. This allows the trailing-slash form to resolve without
changing the route’s exact path matching.
smarcet
reviewed
Jul 17, 2026
|
@santipalenque LGTM please fix conflicts many thanks |
santipalenque
force-pushed
the
feature/migrate-media-upload-types
branch
from
July 23, 2026 14:38
8d07b14 to
c1a551b
Compare
Author
|
done @smarcet |
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.
https://app.clickup.com/t/9014802374/86bapy7m1
Summary by CodeRabbit