feat: edit global sponsor page template#793
Conversation
📝 WalkthroughWalkthroughAdds page-template Redux state and normalization, introduces FILE/URL document-module type handling and conditional UI, refactors several Formik components to field-level APIs, wires media file types from Redux into module forms, and updates tests and constants/translations accordingly. Changes
Sequence DiagramsequenceDiagram
actor User
participant List as PageTemplateListPage
participant API
participant Redux as Redux Store
participant Reducer as PageTemplateReducer
participant Popup as PageTemplatePopup
participant Module as DocumentModule
User->>List: Click Edit
List->>API: getPageTemplate(id)
API-->>Redux: RECEIVE_PAGE_TEMPLATE(payload)
Redux->>Reducer: Normalize template (modules, files, types)
Reducer-->>Redux: Update pageTemplateState
Redux-->>Popup: Provide pageTemplate prop
Popup->>Module: Render module with type (FILE/URL)
User->>Module: Select download type
Module->>Module: Conditionally render file upload or URL input
User->>Popup: Submit
Popup->>API: Save/Update template
API-->>Redux: PAGE_TEMPLATE_ADDED/UPDATED
Popup->>Redux: resetPageTemplateForm
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/formik-inputs/mui-formik-datepicker.js (1)
8-20:⚠️ Potential issue | 🟡 Minor*Prevent “undefined ” when
labelis omitted.With
labeloptional, the required label string should be built only when a label exists.🛠️ Suggested fix
- const requiredLabel = `${label} *`; + const finalLabel = label ? (required ? `${label} *` : label) : undefined; ... - label: required ? requiredLabel : label, + label: finalLabel,Also applies to: 46-49
🤖 Fix all issues with AI agents
In `@src/actions/page-template-actions.js`:
- Around line 164-171: Guard against indexing module.file by checking its type
before accessing [0]; in the PAGES_MODULE_KINDS.DOCUMENT branch inside the logic
that handles PAGE_MODULES_DOWNLOAD.FILE, replace the direct module.file[0] usage
with a safe assignment that handles arrays, objects and undefined (e.g., if
Array.isArray(module.file) use module.file[0] || null, else if module.file is an
object use module.file, otherwise null) so you don't throw when module.file is
undefined or overwrite existing file objects; update the code that sets
normalizedModule.file and the related deletes accordingly.
In `@src/components/forms/media-upload-form.js`:
- Around line 106-109: The map over mediaFileTypes can throw when mediaFileTypes
is undefined; update the mapping in media-upload-form.js (the mediaFileTypesDDL
creation and the other mapping around line ~191) to safely handle that by
defaulting to an empty array before mapping (e.g., use (mediaFileTypes || []) or
Array.isArray check) so the code never calls .map on undefined and returns an
empty dropdown list until props load.
In `@src/components/inputs/formik-text-editor.js`:
- Around line 10-18: The TextEditorV3 field isn't wiring onBlur to Formik so the
field never becomes touched; add an onBlur handler on the TextEditorV3 that
marks the Formik field as touched (use helpers.setTouched(true)) so validation
runs during interaction; keep the existing onChange (normalizeHtmlString and
helpers.setValue) and add onBlur={() => helpers.setTouched(true)} (or an
event-aware variant that calls helpers.setTouched(true) and forwards the event)
to the TextEditorV3 props to match how other wrappers like MuiFormikTextField
behave.
In `@src/pages/sponsors-global/page-templates/page-template-popup/index.js`:
- Around line 90-99: The yup.when() uses the object syntax which triggers
Biome's noThenProperty lint; update the schemas for external_url and file in
this module to use the functional form of when() instead of the object form
(refer to symbols external_url, file and the discriminators
PAGE_MODULES_DOWNLOAD.URL / PAGE_MODULES_DOWNLOAD.FILE and the yup schema where
they are defined) so that the conditional validation is expressed as
yup.string().when("type", (type, schema) => type === PAGE_MODULES_DOWNLOAD.URL ?
schema.required(T.translate("validation.required")) : schema.nullable()) and
similarly for file with .when("type", (type, schema) => type ===
PAGE_MODULES_DOWNLOAD.FILE ? schema.min(1,
T.translate("validation.file_required")) : schema.nullable()).
In
`@src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-document-download-module.js`:
- Around line 13-26: The download type can be undefined for new/legacy document
modules causing both URL and FILE inputs to stay hidden; update the logic that
derives downloadTypeVal (currently using buildFieldName("type") and useField) to
default to PAGE_MODULES_DOWNLOAD.URL when field.value is missing (e.g., set
downloadTypeVal = field.value || PAGE_MODULES_DOWNLOAD.URL or read via
getIn(values, buildFieldName("type")) || PAGE_MODULES_DOWNLOAD.URL) so the
conditional checks against PAGE_MODULES_DOWNLOAD.URL/FILE show the correct input
by default.
In
`@src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-media-request-module.js`:
- Around line 86-96: The renderValue callback for MuiFormikSelect uses an
undefined identifier placeholder which causes a ReferenceError; update
renderValue in page-template-media-request-module.js (the MuiFormikSelect
instance with name={buildFieldName("file_type_id")}) to reference the correct
placeholder source (e.g. props.placeholder or a locally defined placeholder
variable) or fall back to a default string (e.g. "Select...") so renderValue
uses a defined value when selected is empty.
In
`@src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js`:
- Around line 182-184: The mapStateToProps function currently returns undefined
which triggers a react-redux error; either make mapStateToProps return a plain
object (e.g., change the body of mapStateToProps to "return {}") or remove the
empty function and pass null as the first arg to connect; update the connect
call that wires getAllMediaFileTypes and PageModules accordingly so connect
receives a valid first parameter instead of an undefined mapStateToProps.
In `@src/reducers/sponsors_inventory/page-template-reducer.js`:
- Around line 56-68: The reducer assumes payload.response.modules is always an
array and calls .map, which will throw if modules is null/undefined; before
mapping in the block that builds entity from payload.response, ensure modules is
defaulted to an empty array (or check for truthiness) so entity.modules =
(entity.modules || []) .map(...) safely executes and still applies the
upload_deadline transformation (referencing entity, payload.response, modules,
tmpModule, upload_deadline, MILLISECONDS_IN_SECOND, and moment).
🧹 Nitpick comments (1)
src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js (1)
40-42: AddgetAllMediaFileTypesto the effect dependency list.
This avoids stale closures and satisfies React Hooks’ exhaustive-deps expectations.Suggested fix
- useEffect(() => { - getAllMediaFileTypes(); - }, []); + useEffect(() => { + getAllMediaFileTypes(); + }, [getAllMediaFileTypes]);
| if (module.kind === PAGES_MODULE_KINDS.DOCUMENT) { | ||
| if (module.type === PAGE_MODULES_DOWNLOAD.FILE) { | ||
| normalizedModule.file = module.file[0] || null; | ||
| delete normalizedModule.external_url; | ||
| } else { | ||
| delete normalizedModule.file; | ||
| delete normalizedModule.file_id; | ||
| } |
There was a problem hiding this comment.
Guard module.file before indexing.
module.file[0] will throw if module.file is undefined or not an array, and can drop existing file objects.
🛠️ Suggested fix
- if (module.type === PAGE_MODULES_DOWNLOAD.FILE) {
- normalizedModule.file = module.file[0] || null;
+ if (module.type === PAGE_MODULES_DOWNLOAD.FILE) {
+ const fileValue = Array.isArray(module.file)
+ ? module.file[0]
+ : module.file || null;
+ normalizedModule.file = fileValue;
delete normalizedModule.external_url;
} else {📝 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 (module.kind === PAGES_MODULE_KINDS.DOCUMENT) { | |
| if (module.type === PAGE_MODULES_DOWNLOAD.FILE) { | |
| normalizedModule.file = module.file[0] || null; | |
| delete normalizedModule.external_url; | |
| } else { | |
| delete normalizedModule.file; | |
| delete normalizedModule.file_id; | |
| } | |
| if (module.kind === PAGES_MODULE_KINDS.DOCUMENT) { | |
| if (module.type === PAGE_MODULES_DOWNLOAD.FILE) { | |
| const fileValue = Array.isArray(module.file) | |
| ? module.file[0] | |
| : module.file || null; | |
| normalizedModule.file = fileValue; | |
| delete normalizedModule.external_url; | |
| } else { | |
| delete normalizedModule.file; | |
| delete normalizedModule.file_id; | |
| } |
🤖 Prompt for AI Agents
In `@src/actions/page-template-actions.js` around lines 164 - 171, Guard against
indexing module.file by checking its type before accessing [0]; in the
PAGES_MODULE_KINDS.DOCUMENT branch inside the logic that handles
PAGE_MODULES_DOWNLOAD.FILE, replace the direct module.file[0] usage with a safe
assignment that handles arrays, objects and undefined (e.g., if
Array.isArray(module.file) use module.file[0] || null, else if module.file is an
object use module.file, otherwise null) so you don't throw when module.file is
undefined or overwrite existing file objects; update the code that sets
normalizedModule.file and the related deletes accordingly.
| const mediaFileTypesDDL = mediaFileTypes.map((mft) => ({ | ||
| value: mft.id, | ||
| label: mft.name | ||
| })); |
There was a problem hiding this comment.
Guard against undefined mediaFileTypes before .map().
If the prop is briefly undefined while loading, this will throw and break the form.
🛠️ Suggested fix
- const mediaFileTypesDDL = mediaFileTypes.map((mft) => ({
+ const mediaFileTypesDDL = (mediaFileTypes || []).map((mft) => ({
value: mft.id,
label: mft.name
}));Also applies to: 191-191
🤖 Prompt for AI Agents
In `@src/components/forms/media-upload-form.js` around lines 106 - 109, The map
over mediaFileTypes can throw when mediaFileTypes is undefined; update the
mapping in media-upload-form.js (the mediaFileTypesDDL creation and the other
mapping around line ~191) to safely handle that by defaulting to an empty array
before mapping (e.g., use (mediaFileTypes || []) or Array.isArray check) so the
code never calls .map on undefined and returns an empty dropdown list until
props load.
| <TextEditorV3 | ||
| name={name} | ||
| id={name} | ||
| value={values[name]} | ||
| value={field.value} | ||
| onChange={(e) => { | ||
| const stringValue = normalizeHtmlString(e.target.value); | ||
| setFieldValue(name, stringValue); | ||
| helpers.setValue(stringValue); | ||
| }} | ||
| onBlur={() => setFieldTouched(name, true)} | ||
| error={touched?.[name] && errors?.[name] ? errors?.[name] : ""} | ||
| error={meta.touched && meta.error} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/inputs/formik-text-editor.jsRepository: fntechgit/summit-admin
Length of output: 947
🏁 Script executed:
fd TextEditorV3 -t js --exec head -20 {} \;Repository: fntechgit/summit-admin
Length of output: 235
🏁 Script executed:
rg "useField|useFormik|connect" src/components/inputs/formik-text-editor.js -A 5Repository: fntechgit/summit-admin
Length of output: 334
🏁 Script executed:
rg "TextEditorV3" -t jsx -t js -A 3 src/Repository: fntechgit/summit-admin
Length of output: 93
🏁 Script executed:
rg "useField" -t jsx -t js -B 2 -A 10 src/components/inputs/Repository: fntechgit/summit-admin
Length of output: 93
🏁 Script executed:
fd formik -t js src/components/inputs/ --exec basename {} \;Repository: fntechgit/summit-admin
Length of output: 235
🏁 Script executed:
rg "TextEditorV3" src/ -A 3Repository: fntechgit/summit-admin
Length of output: 18232
🏁 Script executed:
rg "useField" src/components/inputs/ -B 2 -A 10Repository: fntechgit/summit-admin
Length of output: 1367
🏁 Script executed:
find src/components/inputs -name "*formik*"Repository: fntechgit/summit-admin
Length of output: 109
🏁 Script executed:
rg "useField|useFormik" src/components/inputs/ -A 8Repository: fntechgit/summit-admin
Length of output: 1022
🏁 Script executed:
find src/components/inputs -name "formik-*" -exec basename {} \;Repository: fntechgit/summit-admin
Length of output: 87
🏁 Script executed:
rg "helpers\." src/components/inputs/ -B 3 -A 2Repository: fntechgit/summit-admin
Length of output: 534
🏁 Script executed:
rg "FormikTextEditor" src/ -B 2 -A 3Repository: fntechgit/summit-admin
Length of output: 12133
🏁 Script executed:
find src -name "*.test.*" -o -name "*.spec.*" | head -20Repository: fntechgit/summit-admin
Length of output: 1365
🏁 Script executed:
rg "field.onBlur" src/Repository: fntechgit/summit-admin
Length of output: 152
🏁 Script executed:
cat -n src/components/mui/formik-inputs/mui-formik-textfield.js | head -50Repository: fntechgit/summit-admin
Length of output: 1576
🏁 Script executed:
rg "onBlur" src/components/mui/formik-inputs/ -B 2 -A 2Repository: fntechgit/summit-admin
Length of output: 502
Wire onBlur so Formik can mark the field as touched.
Without onBlur, the field's touched state never updates during editing. Validation errors will only appear on submit, not during user interaction. Other Formik field wrappers in the codebase (e.g., MuiFormikTextField) follow the same pattern.
🛠️ Suggested fix
onChange={(e) => {
const stringValue = normalizeHtmlString(e.target.value);
helpers.setValue(stringValue);
}}
+ onBlur={field.onBlur}
error={meta.touched && meta.error}📝 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.
| <TextEditorV3 | |
| name={name} | |
| id={name} | |
| value={values[name]} | |
| value={field.value} | |
| onChange={(e) => { | |
| const stringValue = normalizeHtmlString(e.target.value); | |
| setFieldValue(name, stringValue); | |
| helpers.setValue(stringValue); | |
| }} | |
| onBlur={() => setFieldTouched(name, true)} | |
| error={touched?.[name] && errors?.[name] ? errors?.[name] : ""} | |
| error={meta.touched && meta.error} | |
| <TextEditorV3 | |
| name={name} | |
| id={name} | |
| value={field.value} | |
| onChange={(e) => { | |
| const stringValue = normalizeHtmlString(e.target.value); | |
| helpers.setValue(stringValue); | |
| }} | |
| onBlur={field.onBlur} | |
| error={meta.touched && meta.error} |
🤖 Prompt for AI Agents
In `@src/components/inputs/formik-text-editor.js` around lines 10 - 18, The
TextEditorV3 field isn't wiring onBlur to Formik so the field never becomes
touched; add an onBlur handler on the TextEditorV3 that marks the Formik field
as touched (use helpers.setTouched(true)) so validation runs during interaction;
keep the existing onChange (normalizeHtmlString and helpers.setValue) and add
onBlur={() => helpers.setTouched(true)} (or an event-aware variant that calls
helpers.setTouched(true) and forwards the event) to the TextEditorV3 props to
match how other wrappers like MuiFormikTextField behave.
| external_url: yup.string().when("type", { | ||
| is: PAGE_MODULES_DOWNLOAD.URL, | ||
| then: (schema) => schema.required(T.translate("validation.required")), | ||
| otherwise: (schema) => schema.nullable() | ||
| }), | ||
| file: yup.array().when("type", { | ||
| is: PAGE_MODULES_DOWNLOAD.FILE, | ||
| then: (schema) => schema.min(1, T.translate("validation.file_required")), | ||
| otherwise: (schema) => schema.nullable() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the code at the specified lines
find . -name "index.js" -path "*/sponsors-global/page-templates/page-template-popup/*" | head -5Repository: fntechgit/summit-admin
Length of output: 137
🏁 Script executed:
# Search for yup usage in the repository to understand context
rg "yup\.string\(\)\.when" --type js -A 3 | head -30Repository: fntechgit/summit-admin
Length of output: 552
🌐 Web query:
Biome lint rule yup.when then property suspicious
💡 Result:
Biome’s lint/suspicious/noThenProperty rule flags any object/class member named then (to prevent accidental “thenables” that can confuse await / dynamic import() semantics). It’s enabled by default and has no autofix. [1]
That collides with Yup’s object-form when() API, which uses { is, then, otherwise }. [2]
Options to resolve it
1) Prefer Yup’s function-form when() (no then property)
Yup supports when(keys, (values, schema) => Schema) as an alternative to the { then, otherwise } object form. [2]
count: yup.number().when('isBig', ([isBig], schema) =>
isBig ? schema.min(5) : schema.min(0)
),2) Suppress the rule for that line/block
Biome supports targeted suppression comments using the rule’s diagnostic category. [3]
// biome-ignore lint/suspicious/noThenProperty: Yup's API requires `then`/`otherwise` keys
count: yup.number().when('isBig', {
is: true,
then: (schema) => schema.min(5),
otherwise: (schema) => schema.min(0),
});3) Disable the rule in biome.json (project-wide)
{
"linter": {
"rules": {
"suspicious": {
"noThenProperty": "off"
}
}
}
}(That configuration key is the one documented for the rule.) [1]
Sources: Biome rule docs [1], Yup when() API docs (object + function overloads) [2], Biome suppression syntax [3].
🏁 Script executed:
# Search for functional form of yup.when() usage in the codebase
rg "\.when\(" --type js -A 2 | grep -A 2 "=>" | head -40Repository: fntechgit/summit-admin
Length of output: 2238
🏁 Script executed:
# Check if there are examples of yup.when with functional signature
rg "when\(\s*['\"][^'\"]+['\"],\s*\(" --type js -B 1 -A 3 | head -50Repository: fntechgit/summit-admin
Length of output: 48
Replace yup.when() object syntax with functional form to resolve Biome noThenProperty lint error.
Biome's noThenProperty rule (enabled by default) flags then properties on objects, which conflicts with Yup's object-form when() API. Use the functional signature instead.
Suggested fix
- external_url: yup.string().when("type", {
- is: PAGE_MODULES_DOWNLOAD.URL,
- then: (schema) => schema.required(T.translate("validation.required")),
- otherwise: (schema) => schema.nullable()
- }),
- file: yup.array().when("type", {
- is: PAGE_MODULES_DOWNLOAD.FILE,
- then: (schema) => schema.min(1, T.translate("validation.file_required")),
- otherwise: (schema) => schema.nullable()
- })
+ external_url: yup.string().when("type", (type, schema) =>
+ type === PAGE_MODULES_DOWNLOAD.URL
+ ? schema.required(T.translate("validation.required"))
+ : schema.nullable()
+ ),
+ file: yup.array().when("type", (type, schema) =>
+ type === PAGE_MODULES_DOWNLOAD.FILE
+ ? schema.min(1, T.translate("validation.file_required"))
+ : schema.nullable()
+ )📝 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.
| external_url: yup.string().when("type", { | |
| is: PAGE_MODULES_DOWNLOAD.URL, | |
| then: (schema) => schema.required(T.translate("validation.required")), | |
| otherwise: (schema) => schema.nullable() | |
| }), | |
| file: yup.array().when("type", { | |
| is: PAGE_MODULES_DOWNLOAD.FILE, | |
| then: (schema) => schema.min(1, T.translate("validation.file_required")), | |
| otherwise: (schema) => schema.nullable() | |
| }) | |
| external_url: yup.string().when("type", (type, schema) => | |
| type === PAGE_MODULES_DOWNLOAD.URL | |
| ? schema.required(T.translate("validation.required")) | |
| : schema.nullable() | |
| ), | |
| file: yup.array().when("type", (type, schema) => | |
| type === PAGE_MODULES_DOWNLOAD.FILE | |
| ? schema.min(1, T.translate("validation.file_required")) | |
| : schema.nullable() | |
| ) |
🧰 Tools
🪛 Biome (2.3.14)
[error] 92-92: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 97-97: Do not add then to an object.
(lint/suspicious/noThenProperty)
🤖 Prompt for AI Agents
In `@src/pages/sponsors-global/page-templates/page-template-popup/index.js` around
lines 90 - 99, The yup.when() uses the object syntax which triggers Biome's
noThenProperty lint; update the schemas for external_url and file in this module
to use the functional form of when() instead of the object form (refer to
symbols external_url, file and the discriminators PAGE_MODULES_DOWNLOAD.URL /
PAGE_MODULES_DOWNLOAD.FILE and the yup schema where they are defined) so that
the conditional validation is expressed as yup.string().when("type", (type,
schema) => type === PAGE_MODULES_DOWNLOAD.URL ?
schema.required(T.translate("validation.required")) : schema.nullable()) and
similarly for file with .when("type", (type, schema) => type ===
PAGE_MODULES_DOWNLOAD.FILE ? schema.min(1,
T.translate("validation.file_required")) : schema.nullable()).
| const typeFieldName = buildFieldName("type"); | ||
| const [field] = useField(typeFieldName); | ||
| const downloadTypeVal = field.value; | ||
|
|
||
| const downloadTypeOptions = [ | ||
| { | ||
| value: PAGE_MODULES_DOWNLOAD.FILE, | ||
| label: T.translate("page_template_list.page_crud.upload_file") | ||
| }, | ||
| { | ||
| value: PAGE_MODULES_DOWNLOAD.URL, | ||
| label: T.translate("page_template_list.page_crud.input_url_link") | ||
| } | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-document-download-module.jsRepository: fntechgit/summit-admin
Length of output: 3699
🏁 Script executed:
rg "PAGE_MODULES_DOWNLOAD" src/utils/constants.js -A 5 -B 2Repository: fntechgit/summit-admin
Length of output: 174
🏁 Script executed:
# Check if there's any default initialization for the type field
rg "type.*FILE|type.*URL" src/pages/sponsors-global/page-templates/page-template-popup/modules/ -B 3 -A 3 --type js
# Also check the Formik form initialization to see if defaults are set anywhere
rg "initialValues|type:" src/pages/sponsors-global/page-templates/page-template-popup/ -B 2 -A 2 --type js | head -100Repository: fntechgit/summit-admin
Length of output: 9064
🏁 Script executed:
# Check if there's initialization for download modules like there is for media modules
rg "PAGE_MODULES_DOWNLOAD" src/pages/sponsors-global/page-templates/page-template-popup/ -B 5 -A 5 --type js | head -150
# Check page-template-media-request-module more closely to see the full pattern
sed -n '1,50p' src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-media-request-module.jsRepository: fntechgit/summit-admin
Length of output: 11889
🏁 Script executed:
# Check initialization of document modules (not media)
rg "PAGES_MODULE_KINDS.DOCUMENT" src/pages/sponsors-global/page-templates/page-template-popup/ -B 5 -A 10 --type jsRepository: fntechgit/summit-admin
Length of output: 28282
Default or infer type for document modules to prevent hidden inputs.
New document modules created via "Add Document" lack a type field (it's not included in the initialization object), and legacy modules from before type was introduced similarly have no value. Since conditional rendering uses strict equality checks (downloadTypeVal === PAGE_MODULES_DOWNLOAD.URL and downloadTypeVal === PAGE_MODULES_DOWNLOAD.FILE), both the URL input and file upload inputs remain hidden when downloadTypeVal is undefined, preventing any interaction.
Compare this to page-template-media-request-module.js, which correctly handles this: const mediaType = getIn(values, buildFieldName("type")) || PAGE_MODULES_MEDIA_TYPES.FILE;
Apply the same pattern to document modules: default to PAGE_MODULES_DOWNLOAD.URL when type is missing.
🤖 Prompt for AI Agents
In
`@src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-document-download-module.js`
around lines 13 - 26, The download type can be undefined for new/legacy document
modules causing both URL and FILE inputs to stay hidden; update the logic that
derives downloadTypeVal (currently using buildFieldName("type") and useField) to
default to PAGE_MODULES_DOWNLOAD.URL when field.value is missing (e.g., set
downloadTypeVal = field.value || PAGE_MODULES_DOWNLOAD.URL or read via
getIn(values, buildFieldName("type")) || PAGE_MODULES_DOWNLOAD.URL) so the
conditional checks against PAGE_MODULES_DOWNLOAD.URL/FILE show the correct input
by default.
| <MuiFormikSelect | ||
| name={buildFieldName("file_type_id")} | ||
| queryFunction={queryMediaFileTypes} | ||
| formatOption={(item) => ({ | ||
| value: item.id, | ||
| label: `${item.name} (${item.allowed_extensions?.join(", ")})` | ||
| })} | ||
| /> | ||
| renderValue={(selected) => { | ||
| if (!selected || selected === "") { | ||
| return <span style={{ color: "#aaa" }}>{placeholder}</span>; | ||
| } | ||
| const selectedOption = fileTypeOptions.find( | ||
| (t) => t.value === selected | ||
| ); | ||
| return selectedOption?.label || selected; | ||
| }} |
There was a problem hiding this comment.
Fix placeholder ReferenceError in renderValue.
placeholder is not defined in scope, so this will crash when the select renders.
Suggested fix
+ const fileTypePlaceholder = T.translate(
+ "page_template_list.page_crud.allowed_formats"
+ );
...
- renderValue={(selected) => {
+ renderValue={(selected) => {
if (!selected || selected === "") {
- return <span style={{ color: "#aaa" }}>{placeholder}</span>;
+ return (
+ <span style={{ color: "#aaa" }}>
+ {fileTypePlaceholder}
+ </span>
+ );
}
const selectedOption = fileTypeOptions.find(
(t) => t.value === selected
);
return selectedOption?.label || selected;
}}🤖 Prompt for AI Agents
In
`@src/pages/sponsors-global/page-templates/page-template-popup/modules/page-template-media-request-module.js`
around lines 86 - 96, The renderValue callback for MuiFormikSelect uses an
undefined identifier placeholder which causes a ReferenceError; update
renderValue in page-template-media-request-module.js (the MuiFormikSelect
instance with name={buildFieldName("file_type_id")}) to reference the correct
placeholder source (e.g. props.placeholder or a locally defined placeholder
variable) or fall back to a default string (e.g. "Select...") so renderValue
uses a defined value when selected is empty.
src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js
Outdated
Show resolved
Hide resolved
| const entity = { ...payload.response }; | ||
|
|
||
| entity.modules = entity.modules.map((module) => { | ||
| const tmpModule = { | ||
| ...module, | ||
| ...(module.upload_deadline | ||
| ? { | ||
| upload_deadline: moment( | ||
| module.upload_deadline * MILLISECONDS_IN_SECOND | ||
| ) | ||
| } | ||
| : {}) | ||
| }; |
There was a problem hiding this comment.
Guard against missing modules to avoid a crash.
If the API ever returns modules: null/undefined, .map will throw.
Suggested fix
- entity.modules = entity.modules.map((module) => {
+ const modules = Array.isArray(entity.modules) ? entity.modules : [];
+ entity.modules = modules.map((module) => {🤖 Prompt for AI Agents
In `@src/reducers/sponsors_inventory/page-template-reducer.js` around lines 56 -
68, The reducer assumes payload.response.modules is always an array and calls
.map, which will throw if modules is null/undefined; before mapping in the block
that builds entity from payload.response, ensure modules is defaulted to an
empty array (or check for truthiness) so entity.modules = (entity.modules || [])
.map(...) safely executes and still applies the upload_deadline transformation
(referencing entity, payload.response, modules, tmpModule, upload_deadline,
MILLISECONDS_IN_SECOND, and moment).
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js (2)
179-181: AddgetAllMediaFileTypesto PropTypes.The component now receives
getAllMediaFileTypesas a required prop viaconnect, but PropTypes only definesname.Suggested fix
PageModules.propTypes = { - name: PropTypes.string + name: PropTypes.string, + getAllMediaFileTypes: PropTypes.func.isRequired };🤖 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/page-template-modules-form.js` around lines 179 - 181, PageModules currently lists only name in PageModules.propTypes but also receives getAllMediaFileTypes via connect; update PageModules.propTypes to include getAllMediaFileTypes (as a function) and mark it required so the connected prop is validated, i.e. add getAllMediaFileTypes: PropTypes.func.isRequired to the PageModules.propTypes object.
40-42: Consider addinggetAllMediaFileTypesto the dependency array.The
useEffectreferencesgetAllMediaFileTypesbut the dependency array is empty. While this works because Redux-bound action creators are stable, adding it to the deps array silences ESLint warnings and follows the rules of hooks:Suggested fix
useEffect(() => { getAllMediaFileTypes(); - }, []); + }, [getAllMediaFileTypes]);🤖 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/page-template-modules-form.js` around lines 40 - 42, The useEffect that calls getAllMediaFileTypes should include getAllMediaFileTypes in its dependency array to satisfy the rules of hooks and silence ESLint; update the useEffect in which getAllMediaFileTypes is referenced so the dependency array contains getAllMediaFileTypes, and if that action creator is not stable (causing unwanted re-runs) ensure it is memoized (e.g., via useCallback or by using the stable bound action from Redux) or, if the current behavior is intentionally one-time, add a precise eslint-disable comment only on that line with a short justification.src/pages/sponsors-global/page-templates/page-template-popup/page-template-module-form.test.js (1)
434-435: Consider consistent delete button selectors across tests.The tests use two different strategies to select delete buttons:
- Line 434:
getByTestId("delete-module-btn")(single module)- Lines 481-482, 519, 564:
getAllByTestId("DeleteIcon")then.closest("button")(multiple modules)Since
data-testid="delete-module-btn"doesn't include an index,getAllByTestId("delete-module-btn")would work for multi-module tests and be more consistent:- const deleteButtons = screen.getAllByTestId("DeleteIcon"); - await userEvent.click(deleteButtons[0].closest("button")); + const deleteButtons = screen.getAllByTestId("delete-module-btn"); + await userEvent.click(deleteButtons[0]);Also applies to: 481-482, 519-520, 564-565
🤖 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/page-template-module-form.test.js` around lines 434 - 435, Replace mixed test selectors with a consistent data-testid for delete buttons: use data-testid="delete-module-btn" everywhere and update tests that currently query DeleteIcon + .closest("button") to instead use getAllByTestId("delete-module-btn") and select the desired index (or getByTestId("delete-module-btn") for single-module cases). Locate usages of getByTestId("delete-module-btn") and the getAllByTestId("DeleteIcon") patterns in the test file (e.g., the click interactions around deleteButton and the blocks using .closest("button")) and switch them to the unified getAllByTestId/getByTestId approach so multi-module tests can target specific delete buttons by index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/pages/sponsors-global/page-templates/page-template-popup/page-template-module-form.test.js`:
- Around line 434-435: Replace mixed test selectors with a consistent
data-testid for delete buttons: use data-testid="delete-module-btn" everywhere
and update tests that currently query DeleteIcon + .closest("button") to instead
use getAllByTestId("delete-module-btn") and select the desired index (or
getByTestId("delete-module-btn") for single-module cases). Locate usages of
getByTestId("delete-module-btn") and the getAllByTestId("DeleteIcon") patterns
in the test file (e.g., the click interactions around deleteButton and the
blocks using .closest("button")) and switch them to the unified
getAllByTestId/getByTestId approach so multi-module tests can target specific
delete buttons by index.
In
`@src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js`:
- Around line 179-181: PageModules currently lists only name in
PageModules.propTypes but also receives getAllMediaFileTypes via connect; update
PageModules.propTypes to include getAllMediaFileTypes (as a function) and mark
it required so the connected prop is validated, i.e. add getAllMediaFileTypes:
PropTypes.func.isRequired to the PageModules.propTypes object.
- Around line 40-42: The useEffect that calls getAllMediaFileTypes should
include getAllMediaFileTypes in its dependency array to satisfy the rules of
hooks and silence ESLint; update the useEffect in which getAllMediaFileTypes is
referenced so the dependency array contains getAllMediaFileTypes, and if that
action creator is not stable (causing unwanted re-runs) ensure it is memoized
(e.g., via useCallback or by using the stable bound action from Redux) or, if
the current behavior is intentionally one-time, add a precise eslint-disable
comment only on that line with a short justification.
1 participant
https://app.clickup.com/t/86b79917k
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests