fix: move media file type list page and edit into MUI components#897
Open
tomrndom wants to merge 3 commits into
Open
fix: move media file type list page and edit into MUI components#897tomrndom wants to merge 3 commits into
tomrndom wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughRefactors media file type management: class -> hooks-based list page with in-page dialog, unified snackbar-based error/success handlers in actions, loader cleanup moved to .finally, reducer/pagination state tweaks, new unit tests, layout route cleanup, and updated delete i18n. ChangesMedia File Types
Sequence Diagram(s)sequenceDiagram
actor User
participant List as MediaFileTypeListPage
participant Dialog as MediaFileTypeDialog
participant Actions as MediaFileTypeActions
participant Reducer as MediaFileTypeReducer
participant Snackbar as SnackbarHandler
User->>List: Click "Edit" / "Add"
List->>Actions: dispatch(getMediaFileType(id)) / resetMediaFileTypeForm
Actions->>Reducer: REQUEST_MEDIA_FILE_TYPE
Reducer-->>List: loading state
Actions->>Reducer: RECEIVE_MEDIA_FILE_TYPE
Reducer-->>List: entity loaded
List->>Dialog: open with entity
User->>Dialog: Submit form
Dialog->>List: onSave(formData)
List->>Actions: dispatch(saveMediaFileType(entity))
Actions->>Reducer: REQUEST_SAVE_MEDIA_FILE_TYPE
Reducer-->>List: loading
Actions->>Actions: perform API request (snackbarErrorHandler used)
Actions->>Snackbar: snackbarSuccessHandler / snackbarErrorHandler
Snackbar-->>User: show notification
Actions->>Reducer: RECEIVE_SAVE_MEDIA_FILE_TYPE
Reducer-->>List: updated state
List->>Actions: dispatch(getMediaFileTypes())
List-->>User: refreshed list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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-file-type-actions.js (1)
211-212:⚠️ Potential issue | 🟠 MajorNormalize and filter
allowed_extensionsbefore sending.An empty field currently becomes
[""], and inputs likePDF, DOCkeep the leading space onDOC. That can persist invalid extension values or trip backend validation.Suggested fix
- normalizedEntity.allowed_extensions = entity.allowed_extensions.split(","); + normalizedEntity.allowed_extensions = (entity.allowed_extensions ?? "") + .split(",") + .map((ext) => ext.trim()) + .filter(Boolean);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/media-file-type-actions.js` around lines 211 - 212, Update the assignment that builds normalizedEntity.allowed_extensions so it trims whitespace and removes empty entries: when entity.allowed_extensions exists, split on "," then map each part with trim() and filter out empty strings (e.g., filter(Boolean)); if entity.allowed_extensions is falsy, set normalizedEntity.allowed_extensions to an empty array. Locate the normalizedEntity.allowed_extensions = entity.allowed_extensions.split(","); line and replace it with this normalized-and-filtered logic to avoid values like "" or " DOC".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/media_file_types/media-file-type-list-page.js`:
- Around line 74-99: The async UI handlers (handleRowEdit, handleSave,
handleDelete and similar) currently start promise chains without returning them
or adding terminal catches; update each to return the promise chain and attach a
.catch that routes errors to the existing snackbarErrorHandler (or otherwise
handles the error) so rejections are not unhandled; specifically: return
getMediaFileType(...) from handleRowEdit, return the reset/setOpen flows where
applicable, make handleSave return the
saveMediaFileType(...).then(getMediaFileTypes...).then(...).catch(snackbarErrorHandler)
so Formik can await it, and return
deleteMediaFileType(...).then(getMediaFileTypes...).catch(snackbarErrorHandler)
from handleDelete; keep using resetMediaFileTypeForm and setOpen as before but
ensure all chains are returned and terminally caught.
- Around line 186-187: The delete dialog is using the old i18n key
"media_file_type.remove_warning" so the translation is missing; update the
T.translate call inside the deleteDialogBody prop (currently
T.translate("media_file_type.remove_warning", { name })) to use the new key
"media_file_type.delete_warning" while keeping the { name } interpolation intact
so the localized message displays correctly.
---
Outside diff comments:
In `@src/actions/media-file-type-actions.js`:
- Around line 211-212: Update the assignment that builds
normalizedEntity.allowed_extensions so it trims whitespace and removes empty
entries: when entity.allowed_extensions exists, split on "," then map each part
with trim() and filter out empty strings (e.g., filter(Boolean)); if
entity.allowed_extensions is falsy, set normalizedEntity.allowed_extensions to
an empty array. Locate the normalizedEntity.allowed_extensions =
entity.allowed_extensions.split(","); line and replace it with this
normalized-and-filtered logic to avoid values like "" or " DOC".
🪄 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: 0974360a-d264-4b65-b15d-cf377a433a8d
📒 Files selected for processing (7)
src/actions/media-file-type-actions.jssrc/i18n/en.jsonsrc/pages/media_file_types/__tests__/media-file-type-list-page.test.jssrc/pages/media_file_types/components/__tests__/media-file-type-dialog.test.jssrc/pages/media_file_types/components/media-file-type-dialog.jssrc/pages/media_file_types/media-file-type-list-page.jssrc/reducers/media_file_types/media-file-type-list-reducer.js
Comment on lines
+74
to
+99
| const handleRowEdit = (row) => { | ||
| getMediaFileType(row.id).then(() => setOpen(true)); | ||
| }; | ||
|
|
||
| const handleNew = () => { | ||
| resetMediaFileTypeForm(); | ||
| setOpen(true); | ||
| }; | ||
|
|
||
| const handleClose = () => { | ||
| resetMediaFileTypeForm(); | ||
| setOpen(false); | ||
| }; | ||
|
|
||
| const handleSave = (entity) => { | ||
| saveMediaFileType(entity) | ||
| .then(() => | ||
| getMediaFileTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | ||
| ) | ||
| .then(() => setOpen(false)); | ||
| }; | ||
|
|
||
| const handleDelete = (mediaFileTypeId) => { | ||
| deleteMediaFileType(mediaFileTypeId).then(() => | ||
| getMediaFileTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | ||
| ); |
There was a problem hiding this comment.
Return/catch these async UI flows.
These handlers start request chains without a terminal catch, so failed fetch/save/delete calls can surface as unhandled promise rejections. handleSave also needs to return the promise so Formik can await the submit lifecycle.
Suggested fix
- const handleRowEdit = (row) => {
- getMediaFileType(row.id).then(() => setOpen(true));
- };
+ const handleRowEdit = async (row) => {
+ try {
+ await getMediaFileType(row.id);
+ setOpen(true);
+ } catch {
+ // snackbarErrorHandler already surfaced the error
+ }
+ };
@@
- const handleSave = (entity) => {
- saveMediaFileType(entity)
- .then(() =>
- getMediaFileTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir)
- )
- .then(() => setOpen(false));
- };
+ const handleSave = async (entity) => {
+ try {
+ await saveMediaFileType(entity);
+ await getMediaFileTypes(
+ term,
+ DEFAULT_CURRENT_PAGE,
+ perPage,
+ order,
+ orderDir
+ );
+ setOpen(false);
+ } catch {
+ // snackbarErrorHandler already surfaced the error
+ }
+ };
@@
- const handleDelete = (mediaFileTypeId) => {
- deleteMediaFileType(mediaFileTypeId).then(() =>
- getMediaFileTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir)
- );
- };
+ const handleDelete = async (mediaFileTypeId) => {
+ try {
+ await deleteMediaFileType(mediaFileTypeId);
+ await getMediaFileTypes(
+ term,
+ DEFAULT_CURRENT_PAGE,
+ perPage,
+ order,
+ orderDir
+ );
+ } catch {
+ // snackbarErrorHandler already surfaced the error
+ }
+ };Based on learnings, snackbarErrorHandler passed to request helpers already handles errors internally, and adding a catch after such requests is intentional to prevent unhandled promise rejections.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/media_file_types/media-file-type-list-page.js` around lines 74 -
99, The async UI handlers (handleRowEdit, handleSave, handleDelete and similar)
currently start promise chains without returning them or adding terminal
catches; update each to return the promise chain and attach a .catch that routes
errors to the existing snackbarErrorHandler (or otherwise handles the error) so
rejections are not unhandled; specifically: return getMediaFileType(...) from
handleRowEdit, return the reset/setOpen flows where applicable, make handleSave
return the
saveMediaFileType(...).then(getMediaFileTypes...).then(...).catch(snackbarErrorHandler)
so Formik can await it, and return
deleteMediaFileType(...).then(getMediaFileTypes...).catch(snackbarErrorHandler)
from handleDelete; keep using resetMediaFileTypeForm and setOpen as before but
ensure all chains are returned and terminally caught.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/media_file_types/media-file-type-list-page.js (1)
116-120: Consider defensive check forallowed_extensions.The render assumes
allowed_extensionsis always an array. If the API ever returnsnullorundefinedfor this field, calling.join()will throw.🛡️ Optional defensive fix
{ columnKey: "allowed_extensions", header: T.translate("media_file_type.allowed_extensions"), - render: (row) => row.allowed_extensions.join(", ") + render: (row) => (row.allowed_extensions || []).join(", ") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/media_file_types/media-file-type-list-page.js` around lines 116 - 120, The render function for the column with columnKey "allowed_extensions" assumes row.allowed_extensions is always an array, which will throw if it's null/undefined; update the render in media-file-type-list-page (the render callback) to defensively handle non-array values by checking Array.isArray(row.allowed_extensions) (or using a fallback like an empty array or a placeholder string) before calling join, so it safely returns a string when allowed_extensions is missing or not an array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pages/media_file_types/media-file-type-list-page.js`:
- Around line 116-120: The render function for the column with columnKey
"allowed_extensions" assumes row.allowed_extensions is always an array, which
will throw if it's null/undefined; update the render in
media-file-type-list-page (the render callback) to defensively handle non-array
values by checking Array.isArray(row.allowed_extensions) (or using a fallback
like an empty array or a placeholder string) before calling join, so it safely
returns a string when allowed_extensions is missing or not an array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ab7c805-66c9-4bf9-a359-67053eecc659
📒 Files selected for processing (2)
src/pages/media_file_types/components/media-file-type-dialog.jssrc/pages/media_file_types/media-file-type-list-page.js
smarcet
reviewed
May 19, 2026
smarcet
reviewed
May 19, 2026
smarcet
requested changes
May 19, 2026
smarcet
requested changes
May 19, 2026
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…fix typo Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
fix/migrate-media-file-type-list
branch
from
2d3dbd9 to
57b1683
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/layouts/media-file-type-layout.js (1)
30-32: ⚡ Quick winDerive the fallback redirect from
match.url.Line 32 hardcodes the same base path that Line 31 already derives from the router. Reusing
match.urlkeeps the fallback aligned if this layout is ever remounted under a different route prefix.♻️ Suggested change
- <Redirect to="/app/media-file-types" /> + <Redirect to={match.url} />🤖 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-file-type-layout.js` around lines 30 - 32, The Redirect uses a hardcoded "/app/media-file-types" fallback which can get out of sync; change the Redirect to use the derived route prefix from match.url instead (i.e. replace the hardcoded path in the <Redirect> with match.url or a join of match.url + desired suffix) so the fallback stays aligned with the <Route exact strict path={match.url} component={MediaFileTypeListPage}> and the surrounding <Switch>/<Route>/<Redirect> logic.src/pages/media_file_types/components/media-file-type-dialog.js (2)
88-88: 💤 Low valueRemove unnecessary
size={12}prop on Grid2 container.The Grid2 container at line 88 is a direct child of
DialogContent, not nested within another grid. Thesize={12}prop has no effect in this context and can be removed for clarity.🧹 Cleanup suggestion
- <Grid2 container spacing={2} size={12} sx={{ p: 3 }}> + <Grid2 container spacing={2} sx={{ p: 3 }}>🤖 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/media_file_types/components/media-file-type-dialog.js` at line 88, In the MediaFileTypeDialog component remove the unused size={12} prop from the Grid2 container (the element created with <Grid2 container ...>) inside DialogContent; simply delete the size={12} attribute so the container uses its container behavior only and no redundant column sizing remains.
34-36: ⚡ Quick winConsider adding validation for
allowed_extensions.The
allowed_extensionsfield is converted from an array to a CSV string but has no validation. Users can submit an empty string or malformed input. Consider adding a Yup validator to ensure at least one extension is provided and that entries are non-empty after trimming.✅ Example validation
validationSchema: yup.object().shape({ - name: requiredStringValidation() + name: requiredStringValidation(), + allowed_extensions: yup + .string() + .required('At least one extension is required') + .test('has-valid-extensions', 'Extensions must be comma-separated non-empty values', (value) => { + if (!value) return false; + const extensions = value.split(',').map(e => e.trim()).filter(e => e.length > 0); + return extensions.length > 0; + }) }),Also applies to: 121-130
🤖 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/media_file_types/components/media-file-type-dialog.js` around lines 34 - 36, The allowed_extensions value is converted to CSV but lacks validation; update the form validation schema (the Yup schema used in media-file-type-dialog.js) to add a rule for allowed_extensions that splits the CSV by commas, trims each entry, filters out empty strings, and requires the resulting array to have length > 0 and all entries match a simple extension pattern (e.g., non-empty alphanumeric/underscore/dot characters); ensure the same validation is applied where initialEntity?.allowed_extensions is handled (the CSV conversion at allowed_extensions: Array.isArray(initialEntity?.allowed_extensions) ... and the corresponding form submit/parse logic around lines ~121-130) so malformed or empty entries are rejected with a clear error message.
🤖 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/media_file_types/components/media-file-type-dialog.js`:
- Line 27: The component is using the isSaving prop but it isn't declared in
PropTypes; update the MediaFileTypeDialog component (media-file-type-dialog.js)
to add isSaving: PropTypes.bool (or PropTypes.oneOfType([PropTypes.bool,
PropTypes.any]) if other types used) to the component's PropTypes declaration
and add a sensible default in defaultProps (e.g., isSaving: false) so prop
validation covers the prop referenced throughout the component (also update any
exported or wrapped component PropTypes if there are multiple declarations
around the component).
- Line 62: The dialog's onClose (onClose={handleClose}) allows backdrop clicks
to close/reset while isSaving is true; update the Dialog's onClose prop to
ignore backdrop clicks during saves by using the onClose signature (e, reason)
and returning early when reason === 'backdropClick' && isSaving, otherwise call
handleClose; reference the existing symbols handleClose and isSaving in
media-file-type-dialog.js.
---
Nitpick comments:
In `@src/layouts/media-file-type-layout.js`:
- Around line 30-32: The Redirect uses a hardcoded "/app/media-file-types"
fallback which can get out of sync; change the Redirect to use the derived route
prefix from match.url instead (i.e. replace the hardcoded path in the <Redirect>
with match.url or a join of match.url + desired suffix) so the fallback stays
aligned with the <Route exact strict path={match.url}
component={MediaFileTypeListPage}> and the surrounding
<Switch>/<Route>/<Redirect> logic.
In `@src/pages/media_file_types/components/media-file-type-dialog.js`:
- Line 88: In the MediaFileTypeDialog component remove the unused size={12} prop
from the Grid2 container (the element created with <Grid2 container ...>) inside
DialogContent; simply delete the size={12} attribute so the container uses its
container behavior only and no redundant column sizing remains.
- Around line 34-36: The allowed_extensions value is converted to CSV but lacks
validation; update the form validation schema (the Yup schema used in
media-file-type-dialog.js) to add a rule for allowed_extensions that splits the
CSV by commas, trims each entry, filters out empty strings, and requires the
resulting array to have length > 0 and all entries match a simple extension
pattern (e.g., non-empty alphanumeric/underscore/dot characters); ensure the
same validation is applied where initialEntity?.allowed_extensions is handled
(the CSV conversion at allowed_extensions:
Array.isArray(initialEntity?.allowed_extensions) ... and the corresponding form
submit/parse logic around lines ~121-130) so malformed or empty entries are
rejected with a clear error message.
🪄 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: 7c35f84e-a9fa-4190-a5b8-ea8785b8d1ef
📒 Files selected for processing (8)
src/actions/media-file-type-actions.jssrc/i18n/en.jsonsrc/layouts/media-file-type-layout.jssrc/pages/media_file_types/__tests__/media-file-type-list-page.test.jssrc/pages/media_file_types/components/__tests__/media-file-type-dialog.test.jssrc/pages/media_file_types/components/media-file-type-dialog.jssrc/pages/media_file_types/edit-media-file-type-page.jssrc/pages/media_file_types/media-file-type-list-page.js
💤 Files with no reviewable changes (1)
- src/pages/media_file_types/edit-media-file-type-page.js
✅ Files skipped from review due to trivial changes (1)
- src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/media_file_types/components/tests/media-file-type-dialog.test.js
- src/pages/media_file_types/media-file-type-list-page.js
- src/pages/media_file_types/tests/media-file-type-list-page.test.js
- src/actions/media-file-type-actions.js
| entity: initialEntity, | ||
| onClose, | ||
| onSave, | ||
| isSaving = false |
There was a problem hiding this comment.
Add isSaving to PropTypes.
The isSaving prop is used throughout the component but is not declared in PropTypes. This creates a maintenance risk and bypasses prop validation.
📝 Proposed fix
MediaFileTypeDialog.propTypes = {
entity: PropTypes.object,
onClose: PropTypes.func.isRequired,
- onSave: PropTypes.func.isRequired
+ onSave: PropTypes.func.isRequired,
+ isSaving: PropTypes.bool
};Also applies to: 151-155
🤖 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/media_file_types/components/media-file-type-dialog.js` at line 27,
The component is using the isSaving prop but it isn't declared in PropTypes;
update the MediaFileTypeDialog component (media-file-type-dialog.js) to add
isSaving: PropTypes.bool (or PropTypes.oneOfType([PropTypes.bool,
PropTypes.any]) if other types used) to the component's PropTypes declaration
and add a sensible default in defaultProps (e.g., isSaving: false) so prop
validation covers the prop referenced throughout the component (also update any
exported or wrapped component PropTypes if there are multiple declarations
around the component).
| return ( | ||
| <Dialog | ||
| open | ||
| onClose={handleClose} |
There was a problem hiding this comment.
Disable backdrop click during save to prevent race conditions.
The dialog's onClose handler is called on backdrop click even when isSaving is true. A user clicking outside the dialog mid-save will reset the form and close it, potentially causing concurrent saves or data loss if they reopen and save again.
🔒 Proposed fix
<Dialog
open
- onClose={handleClose}
+ onClose={(event, reason) => {
+ if (reason === 'backdropClick' && isSaving) return;
+ handleClose();
+ }}
maxWidth="sm"
fullWidth
disableEscapeKeyDown={isSaving}
>📝 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
| onClose={handleClose} | |
| <Dialog | |
| open | |
| onClose={(event, reason) => { | |
| if (reason === 'backdropClick' && isSaving) return; | |
| handleClose(); | |
| }} | |
| maxWidth="sm" | |
| fullWidth | |
| disableEscapeKeyDown={isSaving} | |
| > |
🤖 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/media_file_types/components/media-file-type-dialog.js` at line 62,
The dialog's onClose (onClose={handleClose}) allows backdrop clicks to
close/reset while isSaving is true; update the Dialog's onClose prop to ignore
backdrop clicks during saves by using the onClose signature (e, reason) and
returning early when reason === 'backdropClick' && isSaving, otherwise call
handleClose; reference the existing symbols handleClose and isSaving in
media-file-type-dialog.js.
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
2 participants
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/86b9jqczj
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Refactor
Tests