feat: add new formik input for file size, update page media module#791
feat: add new formik input for file size, update page media module#791
Conversation
…media Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Formik-bound file-size input component that displays megabytes and stores bytes, a BYTES_PER_MB constant, tests for MB↔bytes behavior, replaces a numeric text field with the new component in the media request module, and enforces MB-aligned validation for FILE type max_file_size. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Field as MuiFormikFilesizeField
participant Formik
participant Module as MediaRequestModule
participant API
User->>Field: types "10"
Field->>Formik: setFieldValue("max_file_size", 10 * BYTES_PER_MB)
Note right of Formik: value stored as bytes
User->>Formik: submit form
Formik->>Module: onSubmit(payload with bytes)
Module->>API: send payload
API-->>Module: response
Module-->>User: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/mui/formik-inputs/mui-formik-file-size-field.js`:
- Line 19: initialValues[name] is incorrect for nested/formik paths like
"modules[0].max_file_size"; use Formik's getIn to read nested values. Replace
the emptyValue computation to use getIn(initialValues, name) (from
useFormikContext) so emptyValue becomes null when the nested initial value is
null and "" otherwise; update any references in mui-formik-file-size-field.js
(the emptyValue variable and its usage) accordingly.
- Around line 14-17: displayValue computation does not guard against undefined
leading to NaN; update the conditional used to compute displayValue in
mui-formik-file-size-field.js (the expression using field.value, BYTES_PER_MB
and Math.floor) to explicitly check for undefined (e.g., ensure field.value is
not null, not undefined, and not an empty string) before performing
Math.floor(field.value / BYTES_PER_MB) so that when field.value is undefined you
return "" instead of computing and producing NaN.
🧹 Nitpick comments (2)
src/components/mui/formik-inputs/mui-formik-file-size-field.js (2)
48-52: Prefere.preventDefault()overe.nativeEvent.preventDefault().Using
e.nativeEventis unconventional. React's synthetic event methods (e.preventDefault(),e.stopPropagation()) are sufficient and more idiomatic.Proposed fix
onKeyDown={(e) => { if (BLOCKED_KEYS.includes(e.key)) { - e.nativeEvent.preventDefault(); - e.nativeEvent.stopImmediatePropagation(); + e.preventDefault(); + e.stopPropagation(); } }}
33-61: Consider addingrequiredprop support for consistency.Looking at
MuiFormikTextFieldin the relevant snippets, it supports arequiredprop that appends*to the label. This component doesn't forward that behavior explicitly, though it may work via...propsspread.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
src/components/mui/__tests__/mui-formik-file-size-field.test.js
Outdated
Show resolved
Hide resolved
| name={name} | ||
| label={label} | ||
| type="number" | ||
| value={displayValue} |
There was a problem hiding this comment.
do we need this? formik will pick up the value with the name
There was a problem hiding this comment.
this is required to display a simpler value to the user (MB) instead of the value sent and received from the API (bytes)
…constant and tests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
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-global/page-templates/page-template-popup/index.js (1)
91-103:⚠️ Potential issue | 🔴 CriticalFix Biome lint error by using Yup's functional
whensignature.The code triggers Biome's
noThenPropertyrule, which flags objects with athenproperty to prevent accidental "thenable" behavior. Refactor to use Yup v1.4.0's functionalwhen(key, (values, schema) => schema)signature. Note: values are passed as an array, so use destructuring([type], schema)not(type, schema).🛠️ Corrected refactor (functional when with array destructuring)
- max_file_size: yup.number().when("type", { - is: PAGE_MODULES_MEDIA_TYPES.FILE, - then: (schema) => - schema - .min(1, T.translate("validation.number_positive")) - .required(T.translate("validation.required")), - otherwise: (schema) => schema.nullable() - }), - file_type_id: yup.object().when("type", { - is: PAGE_MODULES_MEDIA_TYPES.FILE, - then: (schema) => schema.required(T.translate("validation.required")), - otherwise: (schema) => schema.nullable() - }) + max_file_size: yup.number().when("type", ([type], schema) => + type === PAGE_MODULES_MEDIA_TYPES.FILE + ? schema + .min(1, T.translate("validation.number_positive")) + .required(T.translate("validation.required")) + : schema.nullable() + ), + file_type_id: yup.object().when("type", ([type], schema) => + type === PAGE_MODULES_MEDIA_TYPES.FILE + ? schema.required(T.translate("validation.required")) + : schema.nullable() + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors-global/page-templates/page-template-popup/index.js` around lines 91 - 103, Refactor the Yup .when calls for max_file_size and file_type_id to use the functional signature when("type", ([type], schema) => ...) instead of the object form to satisfy Biome's noThenProperty rule: inside the handler check if [type] === PAGE_MODULES_MEDIA_TYPES.FILE and return schema.min(1).required(...) for max_file_size or schema.required(...) for file_type_id, otherwise return schema.nullable(); keep the same validation messages via T.translate and preserve the symbols max_file_size, file_type_id, PAGE_MODULES_MEDIA_TYPES.FILE and the existing schema methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pages/sponsors-global/page-templates/page-template-popup/index.js`:
- Around line 91-103: Refactor the Yup .when calls for max_file_size and
file_type_id to use the functional signature when("type", ([type], schema) =>
...) instead of the object form to satisfy Biome's noThenProperty rule: inside
the handler check if [type] === PAGE_MODULES_MEDIA_TYPES.FILE and return
schema.min(1).required(...) for max_file_size or schema.required(...) for
file_type_id, otherwise return schema.nullable(); keep the same validation
messages via T.translate and preserve the symbols max_file_size, file_type_id,
PAGE_MODULES_MEDIA_TYPES.FILE and the existing schema methods.
src/pages/sponsors-global/page-templates/page-template-popup/index.js
Outdated
Show resolved
Hide resolved
src/pages/sponsors-global/page-templates/page-template-popup/index.js
Outdated
Show resolved
Hide resolved
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/i18n/en.json`:
- Around line 97-98: The validation message key "mib_aligned" uses "MiB" while
the UI label uses "MB"; update the "mib_aligned" value to match the UI
terminology (e.g., change the string to "Must be an MB aligned value") so labels
and validation are consistent, and scan for any other i18n keys using "MiB" to
align them with the UI term if present.
| "boolean": "Must be a boolean.", | ||
| "mib_aligned": "Must be a MiB aligned value" |
There was a problem hiding this comment.
Align MB/MiB wording with the UI label.
Line 98 says “MiB” while the UI label says “MB” (Line 3976). This mixed terminology can confuse users—please pick one term across label + validation.
📝 Suggested wording fix (if you keep “MB” in the label)
- "mib_aligned": "Must be a MiB aligned value"
+ "mib_aligned": "Must be an MB-aligned value."📝 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.
| "boolean": "Must be a boolean.", | |
| "mib_aligned": "Must be a MiB aligned value" | |
| "boolean": "Must be a boolean.", | |
| "mib_aligned": "Must be an MB-aligned value." |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/i18n/en.json` around lines 97 - 98, The validation message key
"mib_aligned" uses "MiB" while the UI label uses "MB"; update the "mib_aligned"
value to match the UI terminology (e.g., change the string to "Must be an MB
aligned value") so labels and validation are consistent, and scan for any other
i18n keys using "MiB" to align them with the UI term if present.
3 participants
ref: https://app.clickup.com/t/86b8gnk8b
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Tests
Chores