feat: use formik on additional input list, unify use #744
Conversation
… adjust actions urls Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughConsolidates page-specific metafield editors into new Formik-backed components under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AdditionalInputList as Component
participant FormikState as Formik
participant API as Server
Note over User,Component: Remove meta-field value flow
User->>Component: clicks delete on value
Component->>User: show confirmation dialog
alt user confirms
Component->>API: DELETE /.../values/ID
API-->>Component: 200 OK
Component->>FormikState: remove value from meta_fields[...] values
FormikState-->>Component: updated form state
Component->>User: UI updates (value removed)
else user cancels
Component->>User: no-op (value retained)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 6
Fix all issues with AI Agents 🤖
In @src/actions/form-template-actions.js:
- Line 318: The URL in the form-template delete call currently includes a
trailing slash (`.../values/`) while the inventory-shared-actions change removed
the trailing slash; update the URL template string that builds
`${window.INVENTORY_API_BASE_URL}/api/v1/form-templates/${templateId}/meta-field-types/${metaFieldId}/values/`
to remove the trailing slash (`.../values`) so it matches the format used
elsewhere (ensure the code that constructs/uses templateId and metaFieldId is
unchanged), and run or adjust any related callers/tests to confirm all delete
endpoints use the same pattern.
In @src/actions/form-template-item-actions.js:
- Line 378: The URL for deleting meta field values in
form-template-item-actions.js currently includes a trailing slash (`url:
`${window.INVENTORY_API_BASE_URL}/api/v1/form-templates/${formTemplateId}/items/${formTemplateItemId}/meta-field-types/${metaFieldId}/values/``)
which is inconsistent with the style used in inventory-shared-actions.js where
the trailing slash was removed; update this URL to remove the trailing slash
(ending with `/values`) so it matches the convention, and scan other meta field
value delete endpoints (references to metaFieldId, formTemplateId,
formTemplateItemId and the `url` construction) to ensure they all use the same
no-trailing-slash format.
In @src/components/mui/formik-inputs/additional-input/meta-field-values.js:
- Around line 165-172: The DragAndDropList prop name is incorrect: replace the
prop updateOrder with updateOrderKey on the DragAndDropList invocation so the
component receives the correct prop name; keep the same value ("order") and
leave other props (items={sortedValues}, onReorder={onReorder},
renderItem={renderMetaFieldValue}, idKey="id",
droppableId={`droppable-values-${fieldIndex}`}) unchanged so the reordering
logic in DragAndDropList uses the expected updateOrderKey prop.
- Around line 88-90: The deletion call to onMetaFieldTypeValueDeleted(...)
currently only handles success and will swallow rejections; add error handling
by attaching a .catch(...) to that promise to handle failures (log the error
and/or surface a user-facing message) and consider a .finally(...) if you need
to update UI state regardless; ensure you reference onMetaFieldTypeValueDeleted
and the success callback removeValueFromFields so the catch shows the error
(e.g., process/log or trigger an error toast) and does not leave the UI
inconsistent.
- Around line 83-87: Remove the debug console.log in meta-field-values.js that
prints "Delete params:" with entityId, field.id, and metaFieldValue.id before
merging; locate the logging statement (console.log("Delete params:", { entityId,
fieldId: field.id, valueId: metaFieldValue.id })) and delete it or replace it
with a proper logger call if structured logging is required (e.g., use the app's
logger at debug level), ensuring no sensitive IDs are emitted to stdout in
production.
In @src/components/mui/formik-inputs/mui-formik-select.js:
- Around line 13-19: The placeholder prop is destructured in the MuiFormikSelect
component but not declared in its PropTypes; update the component's PropTypes
for MuiFormikSelect to include placeholder (e.g., PropTypes.string or
PropTypes.node as appropriate) alongside the existing name, children, and
isClearable entries and mark it required only if needed.
🧹 Nitpick comments (3)
src/components/mui/formik-inputs/additional-input/additional-input-list.js (1)
3-3: Inconsistent i18n-react import path.This file imports from
"i18n-react"while other files in the codebase (e.g.,meta-field-values.js,additional-input.js) import from"i18n-react/dist/i18n-react". Consider aligning import paths for consistency.🔎 Proposed fix
-import T from "i18n-react"; +import T from "i18n-react/dist/i18n-react";src/components/mui/__tests__/meta-field-values.test.js (1)
96-96: Translate Spanish comments to English for consistency.Several comments are in Spanish. For codebase consistency and maintainability, translate them to English:
- Line 96:
// Componente wrapper que sincroniza field con Formik→// Wrapper component that syncs field with Formik- Line 119:
// Verificar cantidad inicial→// Verify initial count- Line 125:
// Click en agregar→// Click add button- Line 129:
// Esperar actualización→// Wait for updateAlso applies to: 119-119, 125-125, 129-129
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
54-54: Use existing constant instead of duplicating the definition.
fieldTypesWithOptionsduplicatesMETAFIELD_TYPES_WITH_OPTIONSfromutils/constants. This creates a maintenance risk if the types change.🔎 Proposed fix
Update the import on line 26 to include
METAFIELD_TYPES_WITH_OPTIONS:import { ALLOWED_INVENTORY_IMAGE_FORMATS, MAX_INVENTORY_IMAGE_UPLOAD_SIZE, MAX_INVENTORY_IMAGES_UPLOAD_QTY, - METAFIELD_TYPES + METAFIELD_TYPES, + METAFIELD_TYPES_WITH_OPTIONS } from "../../../utils/constants";Then remove the local definition and use the constant in the validation schema:
- const fieldTypesWithOptions = ["CheckBoxList", "ComboBox", "RadioButtonList"]; ... values: yup.array().when("type", { - is: (type) => fieldTypesWithOptions.includes(type), + is: (type) => METAFIELD_TYPES_WITH_OPTIONS.includes(type),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/actions/form-template-actions.jssrc/actions/form-template-item-actions.jssrc/actions/inventory-item-actions.jssrc/actions/inventory-shared-actions.jssrc/components/mui/__tests__/additional-input-list.test.jssrc/components/mui/__tests__/additional-input.test.jssrc/components/mui/__tests__/meta-field-values.test.jssrc/components/mui/formik-inputs/additional-input/additional-input-list.jssrc/components/mui/formik-inputs/additional-input/additional-input.jssrc/components/mui/formik-inputs/additional-input/meta-field-values.jssrc/components/mui/formik-inputs/mui-formik-select.jssrc/i18n/en.jsonsrc/pages/sponsors/components/additional-input-list.jssrc/pages/sponsors/components/additional-input.jssrc/pages/sponsors/sponsor-form-item-list-page/components/item-form.jssrc/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.jssrc/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.jssrc/pages/sponsors_inventory/inventory-list-page.jssrc/pages/sponsors_inventory/popup/form-template-popup.jssrc/pages/sponsors_inventory/popup/meta-field-values.jssrc/pages/sponsors_inventory/popup/sponsor-inventory-popup.js
💤 Files with no reviewable changes (3)
- src/pages/sponsors/components/additional-input.js
- src/pages/sponsors_inventory/popup/meta-field-values.js
- src/pages/sponsors/components/additional-input-list.js
🧰 Additional context used
🧬 Code graph analysis (10)
src/actions/inventory-item-actions.js (1)
src/pages/sponsors_inventory/edit-inventory-item-page.js (1)
inventoryItemId(40-40)
src/components/mui/formik-inputs/additional-input/meta-field-values.js (6)
src/components/mui/formik-inputs/additional-input/additional-input-list.js (2)
useFormikContext(18-18)metaFields(20-20)src/components/mui/formik-inputs/additional-input/additional-input.js (1)
useFormikContext(34-34)src/components/mui/showConfirmDialog.js (1)
showConfirmDialog(5-42)src/components/mui/formik-inputs/mui-formik-textfield.js (1)
MuiFormikTextField(6-35)src/components/mui/formik-inputs/mui-formik-checkbox.js (1)
MuiFormikCheckbox(11-37)src/components/mui/dnd-list.js (1)
DragAndDropList(16-72)
src/components/mui/formik-inputs/additional-input/additional-input.js (3)
src/components/mui/formik-inputs/additional-input/additional-input-list.js (1)
useFormikContext(18-18)src/components/mui/formik-inputs/additional-input/meta-field-values.js (2)
useFormikContext(19-19)MetaFieldValues(12-185)src/components/menu/menu-item.js (1)
MenuItem(19-37)
src/components/mui/__tests__/additional-input.test.js (1)
src/components/mui/formik-inputs/additional-input/additional-input.js (1)
AdditionalInput(24-183)
src/components/mui/__tests__/meta-field-values.test.js (1)
src/components/mui/formik-inputs/additional-input/meta-field-values.js (2)
MetaFieldValues(12-185)useFormikContext(19-19)
src/components/mui/__tests__/additional-input-list.test.js (2)
src/components/mui/formik-inputs/additional-input/additional-input-list.js (2)
AdditionalInputList(17-96)useFormikContext(18-18)src/components/mui/showConfirmDialog.js (1)
showConfirmDialog(5-42)
src/actions/form-template-item-actions.js (2)
src/pages/sponsors_inventory/edit-form-template-page.js (1)
formTemplateId(40-40)src/pages/sponsors_inventory/edit-form-template-item-page.js (1)
formTemplateItemId(41-41)
src/pages/sponsors_inventory/inventory-list-page.js (1)
src/actions/inventory-item-actions.js (4)
saveInventoryItem(214-309)saveInventoryItem(214-309)getInventoryItems(98-148)getInventoryItems(98-148)
src/pages/sponsors_inventory/popup/form-template-popup.js (1)
src/components/mui/formik-inputs/additional-input/additional-input-list.js (1)
AdditionalInputList(17-96)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
src/components/mui/formik-inputs/additional-input/additional-input-list.js (1)
AdditionalInputList(17-96)
🔇 Additional comments (21)
src/components/mui/formik-inputs/mui-formik-select.js (1)
34-39: Verify that returning the raw value in renderValue works correctly.When a value is selected, line 38 returns the raw
selectedvalue directly. MUI's default behavior (without a customrenderValue) displays the children of the matching MenuItem. By returning the raw value, if a MenuItem'svalueprop differs from its display text, the Select will show the value instead of the label.For example:
<MuiFormikSelect name="status" placeholder="Select..."> <MenuItem value="1">Active</MenuItem> <MenuItem value="2">Inactive</MenuItem> </MuiFormikSelect>With this implementation, selecting "Active" would display "1" instead of "Active" in the select box.
Please verify that all usages of MuiFormikSelect use MenuItem values that match their display labels, or update the renderValue to look up and return the matching MenuItem's children.
#!/bin/bash # Description: Find all usages of MuiFormikSelect to verify MenuItem value/label patterns # Search for MuiFormikSelect usage patterns rg -A 10 -B 2 'MuiFormikSelect' --type tsx --type jsx --type js | head -100src/pages/sponsors/sponsor-form-item-list-page/components/item-form.js (1)
18-18: LGTM!The import path update correctly reflects the component reorganization, moving AdditionalInputList to a centralized location under
src/components/mui/formik-inputs/additional-input/. This improves code organization and reusability.src/pages/sponsors_inventory/inventory-list-page.js (1)
134-145: Improved dialog close timing.The refactor to use
.finally()for closing the dialog is an improvement over the previous implementation. The dialog now remains open during the save operation, providing better user feedback, and closes reliably after the operation completes regardless of success or failure.src/actions/inventory-shared-actions.js (1)
210-210: The identified inconsistency does not exist. All files (inventory-item-actions.js,form-template-actions.js, andform-template-item-actions.js) pass the same URL pattern todeleteMetaFieldTypeValue(): the base URL ending with/values/(with trailing slash). The shared function then appends thevalueIdwithout adding an additional trailing slash, resulting in consistent endpoint URLs (/api/v1/.../values/{valueId}) across all implementations.Likely an incorrect or invalid review comment.
src/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.js (1)
19-19: LGTM! Import path updated to centralized component location.The import path correctly points to the new shared location of
AdditionalInputListundersrc/components/mui/formik-inputs/additional-input/. This consolidation aligns with the PR's objective to unify the usage of Formik-based additional input components across the application.src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.js (1)
20-20: LGTM! Import path updated to centralized component location.Consistent with the broader refactoring, the import path now correctly references the unified
AdditionalInputListcomponent from its new shared location.src/i18n/en.json (1)
2471-2476: LGTM! Well-structured i18n placeholders for additional input fields.The new placeholder keys properly support localization of the AdditionalInput component's form fields. The strings are clear and consistent with the existing i18n structure.
src/components/mui/__tests__/additional-input.test.js (1)
1-253: Excellent test coverage for the AdditionalInput component!The test suite is comprehensive and well-structured:
- ✅ Proper mocking of dependent components (MetaFieldValues)
- ✅ Formik-aware render helper for realistic testing
- ✅ Organized test structure with clear describe blocks
- ✅ Covers all conditional rendering paths (field types)
- ✅ Tests both enabled and disabled states
- ✅ Validates error display with Formik's touched/error states
- ✅ Uses
userEventfor realistic user interactionsThis provides strong confidence in the component's behavior across different scenarios.
src/actions/inventory-item-actions.js (1)
355-355: The trailing slash is used consistently across all three delete endpoints. All wrapper functions (deleteInventoryItemMetaFieldTypeValue,deleteFormTemplateMetaFieldTypeValue, anddeleteFormTemplateItemMetaFieldTypeValue) construct their DELETE URLs with a trailing slash beforevalues/. The shareddeleteMetaFieldTypeValuefunction ininventory-shared-actions.jsdoes not construct independent URLs—it appends thevalueIdto the pre-constructed URL passed via thesettingsobject (${settings.url}${valueId}), so there is no inconsistency.Likely an incorrect or invalid review comment.
src/components/mui/formik-inputs/additional-input/meta-field-values.js (2)
27-31: Mutation of nested Formik state uses shallow copy correctly.The
onReorderfunction properly creates a shallow copy ofmetaFieldsbefore mutating, then callssetFieldValueto update the form state. This follows Formik best practices.
43-52: Single-default logic is well-implemented.The
handleDefaultChangecorrectly enforces mutual exclusivity by clearing allis_defaultflags before setting the selected one. Good implementation.src/components/mui/formik-inputs/additional-input/additional-input-list.js (2)
26-54: Well-structured deletion flow with proper error handling.The
handleRemovefunction correctly:
- Shows confirmation before deletion
- Handles API deletion with proper
.catch()error logging- Ensures at least one empty field remains after the last deletion
- Supports both persisted (with ID) and new (without ID) items
56-77: Comprehensive validation logic.The
areMetafieldsIncompletefunction properly checks:
- Formik field-level errors
- Required name and type fields
- Option completeness for metafield types that require values
This ensures the Add button is appropriately disabled during incomplete states.
src/components/mui/__tests__/additional-input-list.test.js (1)
1-347: Comprehensive and well-structured test suite.The tests cover:
- Rendering behavior for multiple fields and empty states
- Add functionality with Formik state updates
- Delete flow with confirmation dialog, API calls, and UI updates
- Validation logic for incomplete fields
- Edge case: resetting to default when last item is deleted
Good use of mocked components for isolation and
waitForfor async assertions.src/components/mui/__tests__/meta-field-values.test.js (2)
194-211: Good test for mutual exclusivity of default values.This test correctly verifies that only one value can be marked as default at a time, validating the
handleDefaultChangelogic in the component.
230-267: Thorough API deletion test with proper assertions.The test validates that:
- The mock delete function is called with correct parameters (entityId, fieldId, valueId)
- The UI updates after successful API deletion
src/pages/sponsors_inventory/popup/form-template-popup.js (1)
166-171: Clean refactoring to use AdditionalInputList.The replacement of inline FieldArray logic with the reusable
AdditionalInputListcomponent simplifies the code significantly while maintaining the same functionality. Props are correctly wired for entity ID and delete callbacks.src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
337-342: Consistent integration with AdditionalInputList.The refactoring mirrors the pattern used in
form-template-popup.js, maintaining consistency across the codebase.src/components/mui/formik-inputs/additional-input/additional-input.js (3)
24-33: Well-designed component interface.The
AdditionalInputcomponent has a clear API with props for item data, index, callbacks, and disabled state. This enables good reusability and testability.
96-112: Clean conditional rendering for option-based metafield types.The component correctly renders
MetaFieldValuesonly when the current type is inMETAFIELD_TYPES_WITH_OPTIONS, and properly displays validation errors for the values array.
113-138: Quantity fields correctly rendered for Quantity type.The conditional rendering of
minimum_quantityandmaximum_quantityfields when type is "Quantity" is properly implemented with appropriate number input types.
| ) => { | ||
| const settings = { | ||
| url: `${window.INVENTORY_API_BASE_URL}/api/v1/form-templates/${templateId}/meta-field-types/${metaFieldId}/values`, | ||
| url: `${window.INVENTORY_API_BASE_URL}/api/v1/form-templates/${templateId}/meta-field-types/${metaFieldId}/values/`, |
There was a problem hiding this comment.
Related to trailing slash inconsistency.
This change adds a trailing slash to the values URL, which is inconsistent with the change in src/actions/inventory-shared-actions.js (Line 210) where the trailing slash is removed. Please verify the correct endpoint format to ensure all delete operations use a consistent URL pattern.
🤖 Prompt for AI Agents
In @src/actions/form-template-actions.js around line 318, The URL in the
form-template delete call currently includes a trailing slash (`.../values/`)
while the inventory-shared-actions change removed the trailing slash; update the
URL template string that builds
`${window.INVENTORY_API_BASE_URL}/api/v1/form-templates/${templateId}/meta-field-types/${metaFieldId}/values/`
to remove the trailing slash (`.../values`) so it matches the format used
elsewhere (ensure the code that constructs/uses templateId and metaFieldId is
unchanged), and run or adjust any related callers/tests to confirm all delete
endpoints use the same pattern.
| ) => { | ||
| const settings = { | ||
| url: `${window.INVENTORY_API_BASE_URL}/api/v1/form-templates/${formTemplateId}/items/${formTemplateItemId}/meta-field-types/${metaFieldId}/values`, | ||
| url: `${window.INVENTORY_API_BASE_URL}/api/v1/form-templates/${formTemplateId}/items/${formTemplateItemId}/meta-field-types/${metaFieldId}/values/`, |
There was a problem hiding this comment.
Related to trailing slash inconsistency.
This change adds a trailing slash to the values URL, which is inconsistent with the change in src/actions/inventory-shared-actions.js (Line 210) where the trailing slash is removed. Ensure all meta field value delete endpoints follow the same URL format convention.
🤖 Prompt for AI Agents
In @src/actions/form-template-item-actions.js around line 378, The URL for
deleting meta field values in form-template-item-actions.js currently includes a
trailing slash (`url:
`${window.INVENTORY_API_BASE_URL}/api/v1/form-templates/${formTemplateId}/items/${formTemplateItemId}/meta-field-types/${metaFieldId}/values/``)
which is inconsistent with the style used in inventory-shared-actions.js where
the trailing slash was removed; update this URL to remove the trailing slash
(ending with `/values`) so it matches the convention, and scan other meta field
value delete endpoints (references to metaFieldId, formTemplateId,
formTemplateItemId and the `url` construction) to ensure they all use the same
no-trailing-slash format.
src/components/mui/formik-inputs/additional-input/meta-field-values.js
Outdated
Show resolved
Hide resolved
| onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then( | ||
| () => removeValueFromFields() | ||
| ); |
There was a problem hiding this comment.
Add error handling for the deletion promise.
The promise chain lacks .catch() handling. If onMetaFieldTypeValueDeleted rejects, the error will be silently ignored and the UI won't reflect the failure.
🔎 Proposed fix
onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then(
() => removeValueFromFields()
- );
+ ).catch((err) => {
+ console.error("Error deleting meta field value from API", err);
+ });📝 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.
| onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then( | |
| () => removeValueFromFields() | |
| ); | |
| onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then( | |
| () => removeValueFromFields() | |
| ).catch((err) => { | |
| console.error("Error deleting meta field value from API", err); | |
| }); |
🤖 Prompt for AI Agents
In @src/components/mui/formik-inputs/additional-input/meta-field-values.js
around lines 88-90, The deletion call to onMetaFieldTypeValueDeleted(...)
currently only handles success and will swallow rejections; add error handling
by attaching a .catch(...) to that promise to handle failures (log the error
and/or surface a user-facing message) and consider a .finally(...) if you need
to update UI state regardless; ensure you reference onMetaFieldTypeValueDeleted
and the success callback removeValueFromFields so the catch shows the error
(e.g., process/log or trigger an error toast) and does not leave the UI
inconsistent.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/components/mui/formik-inputs/additional-input/meta-field-values.js:
- Line 22: The sort comparator in the creation of sortedValues assumes every
item in field.values has an order property, which can be undefined for newly
added items; update the comparator used in [...field.values].sort(...) to safely
coerce missing order values (e.g. use (a.order ?? 0) and (b.order ?? 0) or
Number(...) defaults) and include a deterministic tie-breaker (such as falling
back to original index or id) so sorting is stable when order is undefined.
- Around line 82-88: The deletion branch calling
onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id) lacks error
handling; update the call in the component so the promise includes a .catch that
handles rejection (e.g., logs the error via console or a UI error handler and
prevents or reverses calling removeValueFromFields on failure) and optionally
add a .finally if cleanup is required; modify the block around
onMetaFieldTypeValueDeleted, removeValueFromFields, metaFieldValue and field to
ensure failures are surfaced instead of being silently ignored.
🧹 Nitpick comments (2)
src/components/mui/formik-inputs/additional-input/meta-field-values.js (2)
12-18: Add PropTypes validation for component props.The component lacks PropTypes definitions. Based on commit messages mentioning "add missing propTypes", this appears to be a project-wide improvement effort.
🔎 Proposed PropTypes
Add this import at the top:
import React from "react"; +import PropTypes from "prop-types"; import T from "i18n-react/dist/i18n-react";Add PropTypes definition after the component:
MetaFieldValues.propTypes = { field: PropTypes.shape({ id: PropTypes.number, values: PropTypes.arrayOf( PropTypes.shape({ id: PropTypes.number, name: PropTypes.string, value: PropTypes.string, is_default: PropTypes.bool, order: PropTypes.number }) ).isRequired }).isRequired, fieldIndex: PropTypes.number.isRequired, baseName: PropTypes.string, onMetaFieldTypeValueDeleted: PropTypes.func, entityId: PropTypes.oneOfType([PropTypes.number, PropTypes.string]) }; MetaFieldValues.defaultProps = { baseName: "meta_fields", onMetaFieldTypeValueDeleted: null, entityId: null };
91-156: Consider adding ARIA labels to draggable items.The draggable value items lack
aria-labeloraria-describedbyattributes, which could improve screen reader navigation. This is optional but would enhance accessibility.🔎 Optional enhancement
<Grid2 container spacing={2} sx={{ + role: "listitem", + "aria-label": `Meta field value: ${val.name || "unnamed"}`, alignItems: "start", background: snapshot?.isDragging ? "#ebebeb" : "inherit",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/mui/formik-inputs/additional-input/meta-field-values.jssrc/components/mui/formik-inputs/mui-formik-select.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/mui/formik-inputs/mui-formik-select.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/mui/formik-inputs/additional-input/meta-field-values.js (6)
src/components/mui/formik-inputs/additional-input/additional-input-list.js (2)
useFormikContext(18-18)metaFields(20-20)src/components/mui/formik-inputs/mui-formik-select.js (1)
field(20-20)src/components/mui/showConfirmDialog.js (1)
showConfirmDialog(5-42)src/components/mui/formik-inputs/mui-formik-textfield.js (1)
MuiFormikTextField(6-35)src/components/mui/formik-inputs/mui-formik-checkbox.js (1)
MuiFormikCheckbox(11-37)src/components/mui/dnd-list.js (1)
DragAndDropList(16-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
src/components/mui/formik-inputs/additional-input/meta-field-values.js (5)
1-10: LGTM!Import statements are clean and all dependencies are utilized within the component.
24-25: LGTM!Clean helper function for building nested Formik field paths.
27-52: LGTM!The reorder, add value, and default change handlers correctly manage Formik state and enforce the single-default constraint.
54-59: LGTM!Good validation logic using optional chaining and
trim()to check for incomplete values.
144-150: The customonChangehandler does override Formik's field binding, but it remains properly synced.The
MuiFormikCheckboxspreads props after field (line 16), causing the customonChangeto override Formik's automatic change tracking. However,handleDefaultChangecorrectly updates Formik state throughsetFieldValue(baseName, newFields), ensuring the field remains in sync. This pattern is necessary here because the business logic requires setting all otheris_defaultvalues to false—behavior that standard field binding cannot handle. The implementation is correct.
| const { values, setFieldValue } = useFormikContext(); | ||
|
|
||
| const metaFields = values[baseName] || []; | ||
| const sortedValues = [...field.values].sort((a, b) => a.order - b.order); |
There was a problem hiding this comment.
Guard against undefined order values in sort.
The sort assumes all values have an order property, but newly added values may not have this field set initially, leading to unstable sorting behavior.
🔎 Proposed fix
- const sortedValues = [...field.values].sort((a, b) => a.order - b.order);
+ const sortedValues = [...field.values].sort((a, b) => (a.order || 0) - (b.order || 0));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sortedValues = [...field.values].sort((a, b) => a.order - b.order); | |
| const sortedValues = [...field.values].sort((a, b) => (a.order || 0) - (b.order || 0)); |
🤖 Prompt for AI Agents
In @src/components/mui/formik-inputs/additional-input/meta-field-values.js at
line 22, The sort comparator in the creation of sortedValues assumes every item
in field.values has an order property, which can be undefined for newly added
items; update the comparator used in [...field.values].sort(...) to safely
coerce missing order values (e.g. use (a.order ?? 0) and (b.order ?? 0) or
Number(...) defaults) and include a deterministic tie-breaker (such as falling
back to original index or id) so sorting is stable when order is undefined.
| if (field.id && metaFieldValue.id && onMetaFieldTypeValueDeleted) { | ||
| onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then( | ||
| () => removeValueFromFields() | ||
| ); | ||
| } else { | ||
| removeValueFromFields(); | ||
| } |
There was a problem hiding this comment.
Add error handling for the deletion promise.
The promise chain still lacks .catch() handling. If onMetaFieldTypeValueDeleted rejects, the error will be silently ignored and the UI won't reflect the failure.
🔎 Proposed fix
if (field.id && metaFieldValue.id && onMetaFieldTypeValueDeleted) {
onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then(
() => removeValueFromFields()
- );
+ ).catch((err) => {
+ console.error("Error deleting meta field value:", err);
+ // Consider showing a user-facing error message
+ });
} else {
removeValueFromFields();
}📝 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.
| if (field.id && metaFieldValue.id && onMetaFieldTypeValueDeleted) { | |
| onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then( | |
| () => removeValueFromFields() | |
| ); | |
| } else { | |
| removeValueFromFields(); | |
| } | |
| if (field.id && metaFieldValue.id && onMetaFieldTypeValueDeleted) { | |
| onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id).then( | |
| () => removeValueFromFields() | |
| ).catch((err) => { | |
| console.error("Error deleting meta field value:", err); | |
| // Consider showing a user-facing error message | |
| }); | |
| } else { | |
| removeValueFromFields(); | |
| } |
🤖 Prompt for AI Agents
In @src/components/mui/formik-inputs/additional-input/meta-field-values.js
around lines 82 - 88, The deletion branch calling
onMetaFieldTypeValueDeleted(entityId, field.id, metaFieldValue.id) lacks error
handling; update the call in the component so the promise includes a .catch that
handles rejection (e.g., logs the error via console or a UI error handler and
prevents or reverses calling removeValueFromFields on failure) and optionally
add a .finally if cleanup is required; modify the block around
onMetaFieldTypeValueDeleted, removeValueFromFields, metaFieldValue and field to
ensure failures are surfaced instead of being silently ignored.
ref:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.