[8424] Field information dialog, help icon in inheritance Alert updates#4800
[8424] Field information dialog, help icon in inheritance Alert updates#4800jvega190 wants to merge 7 commits intocraftercms:developfrom
Conversation
…le to edit source, and move 'learn more' link to alert icon
WalkthroughAdds a new FieldInformationDialog component that shows ContentTypeField metadata and integrates it into FormsEngineField; FormsEngineField now opens the dialog from its menu and reorganizes header, help, and inheritance UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Field as FormsEngineField
participant Dialog as FieldInformationDialog
participant Descriptors as ControlDescriptors
User->>Field: Select "Field Information" menu item
Field->>Field: useEnhancedDialogState.open()
Field->>Dialog: Render with `field` prop (open=true)
Dialog->>Descriptors: Request descriptor for field.type
Descriptors-->>Dialog: Return descriptor (or fallback)
Dialog->>Dialog: Build Basic Properties and descriptor sections
Dialog->>Dialog: Translate labels and format values (booleans → Chips)
Dialog-->>User: Display modal with field information
User->>Dialog: Close modal
Dialog->>Field: onClose -> useEnhancedDialogState.close()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🤖 Fix all issues with AI agents
In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx`:
- Around line 76-77: The help text is being injected via dangerouslySetInnerHTML
(in FieldInformationDialog.tsx where renderHtml renders <Typography
dangerouslySetInnerHTML={{ __html: value ?? '-' }} />) which is an XSS risk;
sanitize the HTML before injecting—import a sanitizer (e.g., DOMPurify) and
replace the raw assignment with something like const safeHtml =
DOMPurify.sanitize(value ?? '-'); then pass safeHtml to dangerouslySetInnerHTML.
Apply the same fix for the identical pattern in FormsEngineField.tsx (around the
render that uses dangerouslySetInnerHTML at the noted location) so all
helpText/html rendering is sanitized centrally before rendering.
- Around line 88-116: The Card rendered inside descriptor.sections?.map(...) is
missing a key prop; add a stable unique key to the Card (e.g., use section.id if
available, otherwise a safe fallback like section.title) so React can properly
reconcile; update the map callback that renders <Card> (inside
FieldInformationDialog, referencing descriptor.sections, section and fieldName)
to pass that key prop to the Card element.
In `@ui/app/src/components/FormsEngine/components/FormsEngineField.tsx`:
- Around line 230-243: The IconButton currently uses a placeholder href="/" with
component="a" and target="_blank" which will open the site root; update the
IconButton in FormsEngineField.tsx (the IconButton / href / component="a" /
target="_blank" usage) to be inert until a real docs URL is available — either
remove href and component="a" (render a plain IconButton with title/tooltip) or
change to href="#" and add an onClick handler that calls event.preventDefault()
(and remove target="_blank"); keep the title/InfoOutlineIcon/formatMessage as-is
so the button remains accessible.
- Around line 276-281: In the FormattedMessage inside FormsEngineField (the
defaultMessage text for the inherited value case), correct the typo by changing
"overriden" to "overridden" so the user-facing string reads "Inherited value
from {label} is overridden"; update the defaultMessage in the FormattedMessage
component used around itemsByPath[sourceMap[fieldId]]?.label.
🧹 Nitpick comments (3)
ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx (2)
50-51: Add a type annotation forBodyprops.The
Bodycomponent'spropsparameter is implicitlyany. Consider typing it for consistency with the rest of the codebase.Proposed fix
-function Body(props) { - const { field } = props; +function Body(props: { field: ContentTypeField }) { + const { field } = props;
94-102: Reassigningvaluefrom a primitive to a JSX element may cause type confusion.
valuestarts as a property/validation value (string, boolean, number, etc.), then is conditionally reassigned to a<Chip>ReactNode. This works at runtime but muddies the type. Also, on line 109,value ? value : '-'will treat0,false(after the boolean branch), and""as missing — thoughfalseis already handled,0and""would incorrectly show'-'.Suggested approach
{section.fields?.map((fieldName) => { - let value = field.properties?.[fieldName]?.value ?? field.validations?.[fieldName]?.value; - if (typeof value === 'boolean') { - value = value ? ( - <Chip label="true" color="success" size="small" /> - ) : ( - <Chip label="false" color="default" size="small" /> - ); - } + const rawValue = field.properties?.[fieldName]?.value ?? field.validations?.[fieldName]?.value; + let display: ReactNode; + if (typeof rawValue === 'boolean') { + display = rawValue ? ( + <Chip label="true" color="success" size="small" /> + ) : ( + <Chip label="false" color="default" size="small" /> + ); + } else { + display = rawValue != null && rawValue !== '' ? String(rawValue) : '-'; + } return ( <Grid container spacing={2} key={fieldName} sx={{ mt: 0.5 }}> <Grid size={{ xs: 4 }}> <Typography color="text.secondary">{getPossibleTranslation(fieldName, formatMessage)}</Typography> </Grid> <Grid size={{ xs: 8 }}> - <Typography>{value ? value : '-'}</Typography> + <Typography>{display}</Typography> </Grid> </Grid> ); })}ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
190-197: Inline array pattern for conditional menu items.Returning
[<Divider>, <MenuItem>]from a conditional expression works but is slightly unusual. A<Fragment>wrapper would be more idiomatic and avoids the need forkeyprops on each element:Suggested alternative
- {hasChanges && [ - <Divider key="rollback-divider" />, - <MenuItem key="rollback-action" onClick={handleRollback}> - <ListItemText> - <FormattedMessage defaultMessage="Rollback changes" /> - </ListItemText> - </MenuItem> - ]} + {hasChanges && ( + <> + <Divider /> + <MenuItem onClick={handleRollback}> + <ListItemText> + <FormattedMessage defaultMessage="Rollback changes" /> + </ListItemText> + </MenuItem> + </> + )}
| {renderHtml ? ( | ||
| <Typography dangerouslySetInnerHTML={{ __html: value ?? '-' }} /> |
There was a problem hiding this comment.
XSS risk with dangerouslySetInnerHTML for help text.
Both static analysis tools flag this. The helpText value is injected as raw HTML without sanitization. Even if the content is admin-authored via content type definitions, sanitizing with a library like DOMPurify would add defense-in-depth. The same pattern also appears in FormsEngineField.tsx line 220.
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 76-76: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.3.14)
[error] 77-77: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx`
around lines 76 - 77, The help text is being injected via
dangerouslySetInnerHTML (in FieldInformationDialog.tsx where renderHtml renders
<Typography dangerouslySetInnerHTML={{ __html: value ?? '-' }} />) which is an
XSS risk; sanitize the HTML before injecting—import a sanitizer (e.g.,
DOMPurify) and replace the raw assignment with something like const safeHtml =
DOMPurify.sanitize(value ?? '-'); then pass safeHtml to dangerouslySetInnerHTML.
Apply the same fix for the identical pattern in FormsEngineField.tsx (around the
render that uses dangerouslySetInnerHTML at the noted location) so all
helpText/html rendering is sanitized centrally before rendering.
There was a problem hiding this comment.
@rart should we add a lib like DOMPurify for this and other many usages like this one?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx`:
- Line 79: The JSX rendering uses a falsy check that treats legitimate values
like 0, "" or false as missing (e.g., in FieldInformationDialog's Typography
where it currently uses value ? value : '-'); change this to a nullish check so
only null or undefined are treated as absent (use the nullish coalescing
approach for the same expression and update the identical occurrence around the
other Typography instance referenced on line 109) so valid falsy values are
preserved.
- Around line 104-107: The displayed field label is using the raw key variable
fieldName (e.g., "maxlength") so getPossibleTranslation(formatMessage) returns
it verbatim; update the render in FieldInformationDialog to pass the actual
MessageDescriptor (descriptor.fields[fieldName].name) — or fallback safely if
descriptor or descriptor.fields[fieldName] is missing — into
getPossibleTranslation/formatMessage so the UI shows the translated field name
instead of the raw id.
In `@ui/app/src/components/FormsEngine/components/FormsEngineField.tsx`:
- Line 52: Import and component name are wrong: replace the import of
InfoOutlineIcon from '@mui/icons-material/InfoOutline' with InfoOutlinedIcon
from '@mui/icons-material/InfoOutlined' and update the component usage in
FormsEngineField (change any <InfoOutlineIcon /> instances to <InfoOutlinedIcon
/>) so the symbol names (InfoOutlinedIcon) match MUI v7's outlined icon exports.
- Around line 216-222: The dangerouslySetInnerHTML usage in FormsEngineField
(rendering field.helpText) is an XSS risk; install and import a sanitizer like
DOMPurify and replace the raw usage with a sanitized value (e.g., const
safeHelpText = DOMPurify.sanitize(field.helpText || '') and pass that to
dangerouslySetInnerHTML). Update the FormsEngineField component to import
DOMPurify, sanitize field.helpText before rendering, and consider adding/using a
shared sanitizeHtml helper for reuse across similar components (SiteTools,
GlobalApp, FieldInformationDialog) to keep behavior consistent.
🧹 Nitpick comments (5)
ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx (3)
50-51: Add a type annotation forBodyprops.The
propsparameter is untyped, which loses type safety. A simple inline type or a pick from the dialog props would suffice.Proposed fix
-function Body(props) { - const { field } = props; +function Body(props: { field: ContentTypeField }) { + const { field } = props;
65-66: Variabledescriptorshadows the outer scope.The
.mapcallback parameterdescriptorshadows thedescriptordeclared on line 55. Consider renaming it (e.g.,fieldDescriptororcommonField) to avoid confusion.
88-89: Prefer a stable key over array index.Descriptor sections have an
idproperty (e.g.,'properties','constraints'). Usingsection.idas the key is more robust than the array index.Proposed fix
- {descriptor.sections?.map((section, index) => ( - <Card key={index}> + {descriptor.sections?.map((section) => ( + <Card key={section.id}>ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (2)
244-259: Nit: Unnecessary Fragment wrapper around single element.The
actioncontains a single<IconButton>wrapped in<>...</>. The Fragment can be removed for cleaner JSX.♻️ Proposed simplification
action={ - <> - <IconButton - color="inherit" - size="small" - sx={{ px: 0.5, minWidth: 0 }} - onClick={() => { - globalApi.pushForm({ - update: { path: sourceMap[fieldId] } - }); - }} - title={formatMessage({ defaultMessage: 'Edit' })} - > - <EditOutlined fontSize="small" /> - </IconButton> - </> + <IconButton + color="inherit" + size="small" + sx={{ px: 0.5, minWidth: 0 }} + onClick={() => { + globalApi.pushForm({ + update: { path: sourceMap[fieldId] } + }); + }} + title={formatMessage({ defaultMessage: 'Edit' })} + > + <EditOutlined fontSize="small" /> + </IconButton> }
204-204: Use JSX children for consistency (Biome lint:noChildrenProp).Line 204 passes
childrenas a prop while the otherListItemTextusages (lines 186–188, 193–195) use JSX children. Aligning with the JSX form is more idiomatic and silences the Biome lint error.♻️ Proposed fix
-<MenuItem key={option.id} onClick={(e) => onMenuOptionClick?.(e, option.id, handleCloseMenu)}> - <ListItemText children={option.text} /> -</MenuItem> +<MenuItem key={option.id} onClick={(e) => onMenuOptionClick?.(e, option.id, handleCloseMenu)}> + <ListItemText>{option.text}</ListItemText> +</MenuItem>
ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx
Outdated
Show resolved
Hide resolved
ui/app/src/components/FormsEngine/components/FormsEngineField.tsx
Outdated
Show resolved
Hide resolved
1 participant
craftercms/craftercms#8424
Summary by CodeRabbit
New Features
Improvements