feat: use formik on additional input list, unify use #744
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRefactors metafield UI by extracting AdditionalInput, AdditionalInputList, and MetaFieldValues into shared MUI/Formik components, removes legacy sponsor-specific implementations, adds centralized Yup validators and constants, updates multiple sponsor/inventory pages to use the new components, and expands test coverage for the new widgets. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as AdditionalInputList
participant Formik
participant Dialog as showConfirmDialog
participant API as onDelete (server)
User->>UI: Click "Delete" on meta-field
UI->>Dialog: showConfirmDialog(...)
Dialog-->>UI: User confirms
UI->>Formik: remove item from values (optimistic/UI update)
alt item has id and onDelete provided
UI->>API: call onDelete(itemId)
API-->>UI: success
UI->>Formik: ensure item removed (sync)
end
UI-->>User: UI reflects deletion (or reset to default empty item)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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.
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
376-381: PropTypes are incomplete.The component uses
onImageDeleted,onMetaFieldTypeDeleted, andonMetaFieldTypeValueDeletedprops (lines 39-41, 166-168, 327-328), but these are not declared in PropTypes.Proposed fix
SponsorItemDialog.propTypes = { open: PropTypes.bool.isRequired, onClose: PropTypes.func.isRequired, onSave: PropTypes.func.isRequired, - entity: PropTypes.object + entity: PropTypes.object, + onImageDeleted: PropTypes.func, + onMetaFieldTypeDeleted: PropTypes.func, + onMetaFieldTypeValueDeleted: PropTypes.func };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/actions/form-template-item-actions.jssrc/actions/inventory-item-actions.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-form-item-list-page/components/item-form.jssrc/pages/sponsors_inventory/popup/sponsor-inventory-popup.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/actions/form-template-item-actions.js
- src/pages/sponsors/sponsor-form-item-list-page/components/item-form.js
- src/i18n/en.json
🧰 Additional context used
🧬 Code graph analysis (1)
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)
⏰ 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 (3)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (2)
4-4: Clean refactoring of imports.The switch from
FieldArraytoFormikProvider+useFormikaligns with the new component-based approach. TheAdditionalInputListcomponent correctly leveragesuseFormikContext()internally, and theFormikProviderwrapper at line 201 ensures proper context propagation.Also applies to: 31-31
324-329: LGTM on AdditionalInputList integration.The component is correctly wired up with the required props (
entityId,name,onDelete,onDeleteValue). The delegation of metafield management to this consolidated component simplifies this file and aligns with the broader refactoring effort across sponsor inventory and sponsor forms.src/actions/inventory-item-actions.js (1)
349-359: Trailing slash is correctly applied for URL path construction.The trailing slash on line 355 is necessary because
deleteMetaFieldTypeValueininventory-shared-actions.jsconstructs the final URL as${settings.url}${valueId}. With the trailing slash, this produces/values/{valueId}as intended. This pattern is consistently applied across all delete operations for metafield values in the three related files (inventory-item-actions.js, form-template-actions.js, and form-template-item-actions.js), while save/update operations correctly omit the trailing slash.
… adjust actions urls Signed-off-by: Tomás Castillo <[email protected]>
Signed-off-by: Tomás Castillo <[email protected]>
chore(sponsor-form-items): refactor yup validations chore(sponsor-form-items): add missing additional fields validations
smarcet
force-pushed
the
feature/formik-additional-input-list
branch
from
e1cd15c to
bd47118
Compare
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 (2)
src/actions/inventory-shared-actions.js (1)
204-216: URL construction missing separator slash causes malformed endpoints.Line 210 constructs the URL as
${settings.url}${valueId}without a separator. Since callers pass URLs ending with/values(form-template-actions.js line 318, form-template-item-actions.js line 378), this produces malformed endpoints like.../values456instead of.../values/456.The inventory-item-actions.js works around this by appending a trailing slash to its URL (line 355:
.../values/), but form-template and form-template-item actions lack this workaround, making their delete calls fail.Fix: Change line 210 to
${settings.url}/${valueId}and remove trailing slashes from the URL settings in callers.src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
319-324: Missing PropTypes for callback props.The component uses
onImageDeleted,onMetaFieldTypeDeleted, andonMetaFieldTypeValueDeletedbut they are not declared in PropTypes.Proposed fix
SponsorItemDialog.propTypes = { open: PropTypes.bool.isRequired, onClose: PropTypes.func.isRequired, onSave: PropTypes.func.isRequired, - entity: PropTypes.object + entity: PropTypes.object, + onImageDeleted: PropTypes.func, + onMetaFieldTypeDeleted: PropTypes.func, + onMetaFieldTypeValueDeleted: PropTypes.func };
🤖 Fix all issues with AI agents
In `@src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js`:
- Around line 66-68: The `code` field is using the function reference
requiredStringValidation instead of invoking it; change the validation mapping
so `code: requiredStringValidation()` (matching how `name` and `description` are
invoked) to ensure the Yup schema is returned and applied for the `code` field
validation.
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js`:
- Around line 64-71: The object assigned to SponsorFormItemPopup.propTypes
mistakenly references the undefined lowercase identifier `propTypes` for several
validators; change those to the imported `PropTypes` (capital P) so lines like
`resetSponsorFormItem: propTypes.func.isRequired`, `saveSponsorFormItem:
propTypes.func.isRequired`, and `updateSponsorFormItem:
propTypes.func.isRequired` become `resetSponsorFormItem:
PropTypes.func.isRequired`, etc., and verify the `PropTypes` import at the top
remains present.
🧹 Nitpick comments (7)
package.json (1)
143-143: Duplicateyupdependency in bothdependenciesanddevDependencies.
yupis listed in both sections at the same version. Since validation is used at runtime (insrc/utils/yup.jsand form schemas), it should only be independencies. The entry indevDependencies(line 173) can be removed.Proposed fix
Remove
yupfrom devDependencies:"lint-staged": "^15.1.0", "prettier": "^2.0.5", - "redux-mock-store": "^1.5.4", - "yup": "^1.4.0" + "redux-mock-store": "^1.5.4" },Also applies to: 173-173
src/utils/constants.js (1)
238-242: Duplicate constant:fieldTypesWithOptionsduplicatesMETAFIELD_TYPES_WITH_OPTIONS.
fieldTypesWithOptions(lines 238-242) contains the same values asMETAFIELD_TYPES_WITH_OPTIONS(lines 194-198). Consider using the existing constant instead to avoid potential inconsistencies if one is updated without the other.#!/bin/bash # Verify usage of both constants to determine which to keep echo "=== Usage of fieldTypesWithOptions ===" rg -n "fieldTypesWithOptions" --type js echo "" echo "=== Usage of METAFIELD_TYPES_WITH_OPTIONS ===" rg -n "METAFIELD_TYPES_WITH_OPTIONS" --type jssrc/utils/yup.js (1)
90-93: Consider renaming:positiveNumberValidationallows zero, which is not strictly positive.The validator uses
.min(0)which allows zero, but zero is not a positive number. The message keyvalidation.number_positivemay also be misleading. Consider renaming tononNegativeIntegerValidationor similar for clarity.src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js (1)
17-17: Unused import:addIssAfterDateFieldValidatoris not needed in this file.The
addIssAfterDateFieldValidatoris imported but only called at module level (line 32). However, this form doesn't use theisAfterDateFieldvalidator in its schema — it has no date fields requiring this validation. Consider removing both the import and the call.Suggested fix
import { - addIssAfterDateFieldValidator, decimalValidation, formMetafieldsValidation, positiveNumberValidation, requiredStringValidation } from "../../../../utils/yup";And remove line 32:
-addIssAfterDateFieldValidator();src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js (1)
73-79: Consider removing unusedsummitTZfrom mapStateToProps.
summitTZis extracted from state but is not used in the component's props or render logic.♻️ Suggested cleanup
const mapStateToProps = ({ sponsorFormItemsListState, - currentSummitState }) => ({ - item: sponsorFormItemsListState.currentItem, - summitTZ: currentSummitState.currentSummit.time_zone_id + item: sponsorFormItemsListState.currentItem });src/components/mui/formik-inputs/additional-input/additional-input.js (2)
113-138: Consider extracting the "Quantity" string to a constant.Line 113 uses a hardcoded string
"Quantity"for comparison. For consistency withMETAFIELD_TYPESandMETAFIELD_TYPES_WITH_OPTIONS, consider defining this as a constant (e.g.,METAFIELD_TYPE_QUANTITY) in the constants file.
47-182: Consider adding PropTypes for documentation.While not strictly required for internal components, adding PropTypes would improve maintainability and provide documentation for the component's expected props (
item,itemIdx,baseName,onAdd,onDelete,onDeleteValue,entityId,isAddDisabled).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
package.jsonsrc/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-pricefield.jssrc/components/mui/formik-inputs/mui-formik-select.jssrc/components/mui/formik-inputs/mui-formik-textfield.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/__test__/inventory-popup.test.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-add-item-from-inventory-popup.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.jssrc/pages/sponsors/sponsor-form-item-list-page/index.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.jssrc/utils/constants.jssrc/utils/yup.js
💤 Files with no reviewable changes (3)
- src/pages/sponsors/components/additional-input-list.js
- src/pages/sponsors/components/additional-input.js
- src/pages/sponsors_inventory/popup/meta-field-values.js
🚧 Files skipped from review as they are similar to previous changes (10)
- src/actions/inventory-item-actions.js
- src/components/mui/tests/meta-field-values.test.js
- src/pages/sponsors_inventory/popup/form-template-popup.js
- src/components/mui/formik-inputs/mui-formik-select.js
- src/components/mui/formik-inputs/additional-input/meta-field-values.js
- src/pages/sponsors_inventory/inventory-list-page.js
- src/components/mui/formik-inputs/additional-input/additional-input-list.js
- src/components/mui/tests/additional-input-list.test.js
- src/components/mui/tests/additional-input.test.js
- src/i18n/en.json
🧰 Additional context used
🧬 Code graph analysis (8)
src/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.js (1)
src/utils/yup.js (6)
requiredStringValidation(87-88)requiredStringValidation(87-88)opensAtValidation(151-154)opensAtValidation(151-154)formMetafieldsValidation(95-149)formMetafieldsValidation(95-149)
src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.js (1)
src/utils/yup.js (6)
requiredStringValidation(87-88)requiredStringValidation(87-88)opensAtValidation(151-154)opensAtValidation(151-154)formMetafieldsValidation(95-149)formMetafieldsValidation(95-149)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js (1)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js (1)
SponsorFormItemForm(34-179)
src/utils/yup.js (1)
src/utils/constants.js (4)
METAFIELD_TYPES(200-208)METAFIELD_TYPES(200-208)fieldTypesWithOptions(238-242)fieldTypesWithOptions(238-242)
src/pages/sponsors/sponsor-form-item-list-page/index.js (1)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js (1)
SponsorFormItemPopup(20-62)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js (2)
src/utils/yup.js (10)
addIssAfterDateFieldValidator(21-40)addIssAfterDateFieldValidator(21-40)requiredStringValidation(87-88)requiredStringValidation(87-88)decimalValidation(45-54)decimalValidation(45-54)positiveNumberValidation(90-93)positiveNumberValidation(90-93)formMetafieldsValidation(95-149)formMetafieldsValidation(95-149)src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
formik(48-79)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (2)
src/utils/yup.js (8)
requiredStringValidation(87-88)requiredStringValidation(87-88)decimalValidation(45-54)decimalValidation(45-54)positiveNumberValidation(90-93)positiveNumberValidation(90-93)formMetafieldsValidation(95-149)formMetafieldsValidation(95-149)src/components/mui/formik-inputs/additional-input/additional-input-list.js (1)
AdditionalInputList(17-96)
src/actions/inventory-shared-actions.js (2)
src/actions/form-template-actions.js (8)
settings(280-284)settings(294-297)settings(304-308)settings(317-320)settings(327-331)settings(336-339)settings(346-349)settings(354-357)src/actions/form-template-item-actions.js (8)
settings(331-335)settings(349-352)settings(363-367)settings(377-380)settings(387-391)settings(400-403)settings(410-413)settings(418-421)
🪛 Biome (2.1.2)
src/utils/yup.js
[error] 108-108: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 119-119: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 127-127: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 132-132: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ 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 (16)
src/components/mui/formik-inputs/mui-formik-textfield.js (1)
37-41: LGTM!Making
labeloptional is appropriate for reusable form components, especially when labels are rendered separately viaInputLabelas seen in the sponsor-inventory-popup usage.src/components/mui/formik-inputs/mui-formik-pricefield.js (1)
34-37: LGTM!The change is consistent with
MuiFormikTextFieldand supports the pattern where labels are rendered externally.src/utils/yup.js (2)
95-149: LGTM! Well-structured conditional validation for metafields.The complex conditional logic correctly handles:
- Name required only when values exist or quantity type with min/max set
- Quantity fields required only for "Quantity" type
- Values array validation only for types in
fieldTypesWithOptionsNote: The Biome static analysis warnings about
thenproperties are false positives — Yup's.when()API usesthenandotherwiseas configuration keys, not Promise-related properties.
151-154: LGTM!Simple and reusable date validation with proper i18n support.
src/pages/sponsors/sponsor-forms-tab/components/customized-form/customized-form.js (2)
16-24: LGTM! Clean consolidation of imports.The refactor correctly centralizes validators and updates the
AdditionalInputListimport path to match the new component location.
59-78: LGTM! Well-structured validation schema.Good use of centralized validators while keeping context-specific logic (like
isAfterDateFieldforexpires_at) inline where appropriate.src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js (2)
34-51: LGTM! Clean validation schema refactor.Good consolidation using centralized validators. The schema is consistent with the form fields and properly validates all required fields.
181-181: LGTM!Component rename to
SponsorFormItemFormis more descriptive and consistent with other form components in the codebase.src/pages/sponsors/sponsor-forms-list-page/components/form-template/form-template-form.js (2)
16-25: LGTM! Consistent import updates.The refactor properly centralizes validators and updates the
AdditionalInputListimport path, maintaining consistency with other forms in this PR.
52-66: LGTM! Well-structured validation schema.Good use of the new centralized validators (
requiredStringValidation,opensAtValidation,formMetafieldsValidation) while keeping the context-specificisAfterDateFieldvalidation inline forexpires_at.src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-add-item-from-inventory-popup.js (2)
28-35: LGTM! Descriptive component rename.The new name
SponsorFormAddItemFromInventoryPopupclearly conveys the component's purpose.
227-233: > Likely an incorrect or invalid review comment.src/pages/sponsors/sponsor-form-item-list-page/components/__test__/inventory-popup.test.js (1)
5-5: LGTM!The import path update correctly reflects the renamed component file (
sponsor-form-add-item-from-inventory-popup). Using the local aliasInventoryPopupminimizes changes to the existing test code while maintaining alignment with the refactored component structure.src/pages/sponsors/sponsor-form-item-list-page/index.js (2)
39-40: LGTM!Import paths correctly updated to reflect the renamed component files.
283-292: LGTM!The JSX usage is correct. Both popup components receive the required props (
formId,open,onClose), and additional props like Redux actions and state are provided via their respectiveconnect()wrappers.src/components/mui/formik-inputs/additional-input/additional-input.js (1)
24-45: LGTM!The component correctly uses Formik's
useFormikContextto access form state, and the field name building pattern withbaseNameanditemIdxenables reusable nested field management. The error display logic properly checks bothtouchedanderrorsstates.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js`:
- Around line 64-71: The propTypes for SponsorFormItemPopup are missing the item
prop used by the component; add an entry for item on
SponsorFormItemPopup.propTypes (e.g., item: PropTypes.object or a more specific
shape) to match how the component destructures and uses item, and mark it
required or optional to match usage (use PropTypes.object.isRequired if item
must always be provided).
🧹 Nitpick comments (2)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js (2)
35-41: Consider adding error handling before closing the dialog.The current implementation closes the dialog regardless of whether the save operation succeeds or fails. Users won't see any feedback if the save fails.
💡 Optional enhancement for error handling
const handleOnSave = (values) => { const save = values.id ? updateSponsorFormItem : saveSponsorFormItem; - save(formId, values).finally(() => { - handleClose(); - }); + save(formId, values) + .then(() => { + handleClose(); + }) + .catch((error) => { + // Error handling is managed by the action/reducer + // Keep dialog open so user can retry + }); };
73-79: Remove unusedsummitTZfrom mapStateToProps.
summitTZis mapped from state but never used in the component.♻️ Proposed fix
const mapStateToProps = ({ - sponsorFormItemsListState, - currentSummitState + sponsorFormItemsListState }) => ({ - item: sponsorFormItemsListState.currentItem, - summitTZ: currentSummitState.currentSummit.time_zone_id + item: sponsorFormItemsListState.currentItem });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js (1)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js (1)
SponsorFormItemForm(34-179)
⏰ 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 (1)
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js (1)
1-18: LGTM!Imports are clean and well-organized. The component and action imports align with the refactored naming convention.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| SponsorFormItemPopup.propTypes = { | ||
| open: PropTypes.bool.isRequired, | ||
| onClose: PropTypes.func.isRequired | ||
| onClose: PropTypes.func.isRequired, | ||
| formId: PropTypes.string.isRequired, | ||
| resetSponsorFormItem: PropTypes.func.isRequired, | ||
| saveSponsorFormItem: PropTypes.func.isRequired, | ||
| updateSponsorFormItem: PropTypes.func.isRequired | ||
| }; |
There was a problem hiding this comment.
Missing item prop in propTypes declaration.
The item prop is destructured in the component (line 22) and used on lines 51 and 59, but it's not declared in propTypes.
🐛 Proposed fix
SponsorFormItemPopup.propTypes = {
open: PropTypes.bool.isRequired,
onClose: PropTypes.func.isRequired,
formId: PropTypes.string.isRequired,
+ item: PropTypes.object,
resetSponsorFormItem: PropTypes.func.isRequired,
saveSponsorFormItem: PropTypes.func.isRequired,
updateSponsorFormItem: PropTypes.func.isRequired
};📝 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.
| SponsorFormItemPopup.propTypes = { | |
| open: PropTypes.bool.isRequired, | |
| onClose: PropTypes.func.isRequired | |
| onClose: PropTypes.func.isRequired, | |
| formId: PropTypes.string.isRequired, | |
| resetSponsorFormItem: PropTypes.func.isRequired, | |
| saveSponsorFormItem: PropTypes.func.isRequired, | |
| updateSponsorFormItem: PropTypes.func.isRequired | |
| }; | |
| SponsorFormItemPopup.propTypes = { | |
| open: PropTypes.bool.isRequired, | |
| onClose: PropTypes.func.isRequired, | |
| formId: PropTypes.string.isRequired, | |
| item: PropTypes.object, | |
| resetSponsorFormItem: PropTypes.func.isRequired, | |
| saveSponsorFormItem: PropTypes.func.isRequired, | |
| updateSponsorFormItem: PropTypes.func.isRequired | |
| }; |
🤖 Prompt for AI Agents
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-popup.js`
around lines 64 - 71, The propTypes for SponsorFormItemPopup are missing the
item prop used by the component; add an entry for item on
SponsorFormItemPopup.propTypes (e.g., item: PropTypes.object or a more specific
shape) to match how the component destructures and uses item, and mark it
required or optional to match usage (use PropTypes.object.isRequired if item
must always be provided).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (2)
319-324: Missing PropTypes for callback props.The component uses
onImageDeleted,onMetaFieldTypeDeleted, andonMetaFieldTypeValueDeletedprops but they're not declared in PropTypes.Proposed fix
SponsorItemDialog.propTypes = { open: PropTypes.bool.isRequired, onClose: PropTypes.func.isRequired, onSave: PropTypes.func.isRequired, + onImageDeleted: PropTypes.func, + onMetaFieldTypeDeleted: PropTypes.func, + onMetaFieldTypeValueDeleted: PropTypes.func, entity: PropTypes.object };
112-118: Read fromformik.values.imagesinstead ofinitialEntity.imagesfor the UploadInputV2 value prop.
getMediaInputValue()reads from the immutableinitialEntity.images, buthandleImageUploadCompleteandhandleRemoveImageupdateformik.values.images. This causes the displayed images to not reflect user additions/removals until the dialog is reopened.Change line 113 from:
initialEntity.images?.length > 0to:
formik.values.images?.length > 0And update line 114 to read from
formik.values.imagesinstead ofinitialEntity.images.
♻️ Duplicate comments (1)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
65-77: Validation schema looks correct.The previous issue where
requiredStringValidationwasn't being invoked for thecodefield has been fixed. All validation functions are now properly called with parentheses.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js
⏰ 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 (2)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (2)
1-37: LGTM!The imports are well-organized.
FormikProvideris correctly added to support context sharing with the newAdditionalInputListcomponent, and the validation utilities are properly imported for the updated schema.
267-272: LGTM!The
AdditionalInputListintegration is clean. It correctly receives the entity ID for deletion callbacks, the field name for Formik binding, and the appropriate handlers for metafield and value deletions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ref:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.