fix: move media file type list page and edit into MUI components#897
Open
fix: move media file type list page and edit into MUI components#897
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 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/total state extended, and new unit tests and i18n delete message updated. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant List as MediaFileTypeListPage
participant Dialog as MediaFileTypeDialog
participant Actions as ActionCreators
participant Reducer as Reducer
participant Snackbar as SnackbarHandler
User->>List: Click "Edit" / "Add"
List->>Actions: dispatch(getMediaFileType(id)) / open dialog
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
Actions->>Snackbar: dispatch(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 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)
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.
…fix typo Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b9jqczj
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Refactor
Tests