-
Notifications
You must be signed in to change notification settings - Fork 4
86b7yv08q - chore: add validation rate inventory items #746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughCentralizes numeric validators into shared Yup utilities, replaces text price inputs with a price-specific input, adds a keydown handler to block "e", "E", "+", "-" in the price field, and updates the sponsor-inventory popup to use the price field and trigger form validation on value changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/utils/yup.js:
- Around line 44-52: The test "max-decimals" in decimalValidation can mis-handle
very large numbers because value.toString() may produce scientific notation
(e.g., "1e+21") and fail the regex; update the test callback in
decimalValidation to first ensure the number is finite and below a safe upper
bound (e.g., Math.abs(value) < 1e21) and return false for values at/above that
threshold (or handle scientific notation explicitly), then run the existing
regex on value.toString() for the normal range—this keeps the two-decimal check
intact while avoiding false negatives for huge numbers.
🧹 Nitpick comments (2)
src/components/mui/formik-inputs/mui-formik-pricefield.js (1)
16-21: Simplify event handling and consider blocking additional characters.The
preventDefault()andstopImmediatePropagation()methods should be called on the synthetic eventerather thane.nativeEvent. React's synthetic event system handles cross-browser compatibility.Additionally, consider blocking other problematic characters like
+and-that can also be entered in number inputs, which may not be appropriate for price fields.🔎 Proposed refactor
onKeyDown={(e) => { - if (e.key === "e" || e.key === "E") { - e.nativeEvent.preventDefault(); - e.nativeEvent.stopImmediatePropagation(); + if (["e", "E", "+", "-"].includes(e.key)) { + e.preventDefault(); + e.stopPropagation(); } }}src/pages/sponsors/sponsor-form-item-list-page/components/item-form.js (1)
130-161: Consider applying the same key blocking to quantity fields for consistency.The quantity fields use
type="number"but don't include theonKeyDownhandler to block 'e', 'E', '+', and '-' like theMuiFormikPriceFieldcomponent does. While validation will catch invalid input, preventing these characters from being typed would provide a more consistent user experience across all numeric inputs.🔎 Optional enhancement
You could either:
- Add the same
onKeyDownhandler to theseMuiFormikTextFieldcomponents, or- Create a reusable
MuiFormikIntegerFieldcomponent similar toMuiFormikPriceFieldthat includes this behavior by default.Example for option 1:
<Grid2 size={4}> <MuiFormikTextField name="quantity_limit_per_show" label={T.translate( "sponsor_form_item_list.edit_item.quantity_limit_per_show" )} fullWidth type="number" inputProps={{ min: 0 }} + onKeyDown={(e) => { + if (["e", "E", "+", "-"].includes(e.key)) { + e.preventDefault(); + } + }} /> </Grid2>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/mui/formik-inputs/mui-formik-pricefield.jssrc/pages/sponsors/sponsor-form-item-list-page/components/item-form.jssrc/utils/yup.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). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
src/utils/yup.js (2)
2-2: LGTM!The import of the translation utility is necessary for the validation error messages below.
41-42: LGTM!The
numberValidationfunction provides a clean, reusable validator with consistent error messaging.src/pages/sponsors/sponsor-form-item-list-page/components/item-form.js (2)
16-20: LGTM!The refactoring to import validation functions from a shared utility module improves code reusability and maintainability.
48-60: LGTM!The validation schema correctly applies
decimalValidation()for rate fields andnumberValidation()with integer constraints for quantity fields. The validator usage is appropriate for each field type.
smarcet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niko-exo please review comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js:
- Around line 153-158: Remove the debug log and meaningless cleanup in the
useEffect: delete the console.log("ERRORES", formik.errors) and the empty return
() => {}; if you require immediate validation on value changes, await
formik.validateForm() inside the effect and then read the resolved errors (e.g.,
const errors = await formik.validateForm()) instead of accessing formik.errors
synchronously; otherwise drop the entire useEffect since Formik already
validates on change via validateOnChange.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (2)
src/pages/sponsors/sponsor-form-item-list-page/components/item-form.js (1)
formik(36-63)src/components/mui/formik-inputs/mui-formik-pricefield.js (1)
MuiFormikPriceField(8-32)
⏰ 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 (3)
1-1: LGTM!The imports are well-organized. The validation utilities are correctly centralized in
utils/yup, aligning with the pattern initem-form.js.Also applies to: 33-33, 38-38
73-84: LGTM!The validation schema correctly uses the centralized
decimalValidation()andnumberValidation()utilities, consistent with the pattern initem-form.js.
311-335: No changes needed —MuiFormikPriceFieldcorrectly uses Formik context.The
MuiFormikPriceFieldcomponents are correct without theformik={formik}prop. The underlyingMuiFormikTextFielduses theuseField(name)hook, which automatically accesses the Formik context fromFormikProvider. Theformikprop passed toMuiFormikTextFieldin other usages (lines 272, 283, 344) is unnecessary and unused by the component — it's simply spread into props without being referenced.Both approaches work identically. No action required.
smarcet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
304-308: Add theformikprop for consistency.While the
FormikProvidermakes the formik context available, all otherMuiFormikTextFieldusages in this file explicitly passformik={formik}(see lines 265-270, 276-281, 337-342, etc.). For consistency with the established pattern, add the prop to these threeMuiFormikPriceFieldinstances.♻️ Proposed fix for consistency
<MuiFormikPriceField variant="outlined" name="early_bird_rate" + formik={formik} fullWidth />Apply similar changes to
standard_rate(line 314) andonsite_rate(line 324).Also applies to: 314-318, 324-328
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js (1)
src/components/mui/formik-inputs/mui-formik-pricefield.js (1)
MuiFormikPriceField(8-32)
⏰ 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 (3)
33-33: LGTM!The import of
MuiFormikPriceFieldis correctly structured and will enable specialized price input handling with key blocking and proper formatting.
73-76: LGTM!The validation schema correctly applies
decimalValidation()for monetary rate fields andnumberValidation()for integer quantities. The use of centralized validators improves maintainability.
38-38: Import validation functions are properly implemented.Both
numberValidationanddecimalValidationare correctly exported fromsrc/utils/yup.js. ThedecimalValidationfunction properly validates monetary values with up to 2 decimal places using the regex pattern/^\d+(\.\d{1,2})?$/and enforces non-negative values. ThenumberValidationfunction provides general number validation with appropriate type error handling.
ref: https://app.clickup.com/t/86b7yv08q
86b7yv08q - chore: add validation rate inventory items
Changelog
eandEon price inputs.Links
86b7yv08q - Add numeric validation for rate on inventory items ( edit / new )
Evidence
2026-01-06_14-18-23.mp4
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.