[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 12 commits into
Conversation
…le to edit source, and move 'learn more' link to alert icon
|
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:
WalkthroughAdds a new ChangesFieldInformationDialog and FormsEngineField Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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> + </> + )}
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>
|
Please share a screenshot of how this is looking |
|
| FolderMoveAlertDialog: lazy(() => import('../components/FolderMoveAlertDialog')), | ||
| FormEngineControls: lazy(() => import('../components/FormEngineControls')), | ||
| FormsEngine: lazy(() => import('../components/FormsEngine/FormsEngine')), | ||
| FieldInformationDialog: lazy(() => import('../components/FormsEngine/components/FieldInformationDialog')), |
There was a problem hiding this comment.
@rart by adding this we can simply use the dialog's system. Would it make sense to include components inside other components folder (in the script) so we can do this?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx (1)
79-80:⚠️ Potential issue | 🟠 MajorXSS:
helpHTML is still rendered unsanitized.Per the prior discussion where
@rartagreed to add DOMPurify,field.help(rendered here viadangerouslySetInnerHTML) is still injected without sanitization. Please sanitize before injecting, e.g.:- <Typography dangerouslySetInnerHTML={{ __html: value ?? '-' }} /> + <Typography dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(value ?? '-') }} />The same sanitization should be applied to
FormsEngineField.tsxline 229 and any otherdangerouslySetInnerHTMLsites. If you'd like, I can open a tracking issue for introducing a sharedsanitizeHtmlhelper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx` around lines 79 - 80, FieldInformationDialog.tsx is still injecting unsanitized HTML via the renderHtml branch (Typography using dangerouslySetInnerHTML) which allows XSS; update the render path to sanitize the HTML (e.g., import and use a shared sanitizeHtml helper that wraps DOMPurify) before passing it to dangerouslySetInnerHTML, and apply the same change to the other site mentioned (FormsEngineField component where dangerouslySetInnerHTML is used) so all uses of dangerouslySetInnerHTML call sanitizeHtml(value or field.help) and not raw content.
🧹 Nitpick comments (3)
ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
253-269: Unnecessary fragment around a single child.The
actionprop only wraps oneIconButton; the surrounding<>...</>is redundant and can be removed.♻️ Proposed simplification
- action={ - <> - <IconButton + 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> - </> + > + <EditOutlined fontSize="small" /> + </IconButton> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/components/FormsEngineField.tsx` around lines 253 - 269, Remove the redundant fragment wrapping the single IconButton in the action prop of the FormsEngineField component: locate the action={ <> <IconButton ... /> </> } JSX (the IconButton that calls globalApi.pushForm with update: { path: sourceMap[fieldId] }) and replace it with the IconButton directly (action={ <IconButton ... /> }) so there is no unnecessary <>...</> wrapper.ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx (2)
68-87: Variable name shadowing reduces readability.The inner
descriptorinsideObject.values(commonControlFieldsDescriptors).map((descriptor) => ...)shadows the outerdescriptor(the control type descriptor computed on line 58). Later on line 91 the outerdescriptor.sectionsis used, and on line 110descriptor.fields?.[fieldName]?.nameis used — so both are live in the file concurrently. Rename the inner one for clarity (e.g.,fieldDescriptor) to make it obvious whichdescriptoris being referenced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx` around lines 68 - 87, The inner map callback in FieldInformationDialog.tsx shadows the outer control-type `descriptor`; rename the inner parameter (e.g., from `descriptor` to `fieldDescriptor`) in the Object.values(commonControlFieldsDescriptors).map(...) and update all uses inside that block (`descriptor.id`, `descriptor.name`, etc.) to `fieldDescriptor.id`, `fieldDescriptor.name` so the outer `descriptor` (used later for `descriptor.sections` and `descriptor.fields?.[fieldName]?.name`) remains unshadowed and the intent is clear.
28-28: Inconsistent MUI import style.Every other MUI component in this file is imported via its subpath (
@mui/material/Box,@mui/material/Card, etc.), butChipuses the barrel import. This defeats the tree‑shaking benefit of subpath imports and is inconsistent with the rest of the file and the surrounding codebase.-import { Chip } from '@mui/material'; +import Chip from '@mui/material/Chip';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx` at line 28, The import for Chip uses the barrel import "@mui/material" which is inconsistent with other subpath imports in FieldInformationDialog.tsx; replace the barrel import line importing Chip with a subpath import from "@mui/material/Chip" (keeping the existing named symbol Chip) so tree-shaking and import style match other components in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx`:
- Around line 53-58: The Body component has an untyped props parameter causing
implicit any for field; define an explicit props interface (e.g., BodyProps with
a typed field property matching your form field descriptor used elsewhere) and
annotate the component signature (change function Body(props) to function
Body(props: BodyProps) or function Body({ field }: BodyProps)). Ensure the Field
type includes the properties used (type, properties, validations) or import the
existing Field/FormField type, so usages of field.type and
controlDescriptors[field.type] are fully typed.
In `@ui/app/src/components/FormsEngine/components/FormsEngineField.tsx`:
- Around line 185-198: The FieldInformation menu currently always dispatches
pushDialog(createComponentId('FieldInformationDialog'), props: { field }) which
allows duplicate dialogs; change the onClick handler in FormsEngineField (the
MenuItem callback that calls dispatch(pushDialog(...))) to first check the
dialogs state (e.g., via the existing selector that returns the dialogs stack or
getDialogs) for an entry whose component equals
createComponentId('FieldInformationDialog'), and if found dispatch an
updateDialog-style action to replace its props with { field } instead of
pushing, otherwise dispatch pushDialog as before; alternatively, add logic in
the reducer handling pushDialog to dedupe by component id for
'FieldInformationDialog' like CodeEditorDialog/FormsEngineDialog do so the same
instance is reused.
---
Duplicate comments:
In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx`:
- Around line 79-80: FieldInformationDialog.tsx is still injecting unsanitized
HTML via the renderHtml branch (Typography using dangerouslySetInnerHTML) which
allows XSS; update the render path to sanitize the HTML (e.g., import and use a
shared sanitizeHtml helper that wraps DOMPurify) before passing it to
dangerouslySetInnerHTML, and apply the same change to the other site mentioned
(FormsEngineField component where dangerouslySetInnerHTML is used) so all uses
of dangerouslySetInnerHTML call sanitizeHtml(value or field.help) and not raw
content.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx`:
- Around line 68-87: The inner map callback in FieldInformationDialog.tsx
shadows the outer control-type `descriptor`; rename the inner parameter (e.g.,
from `descriptor` to `fieldDescriptor`) in the
Object.values(commonControlFieldsDescriptors).map(...) and update all uses
inside that block (`descriptor.id`, `descriptor.name`, etc.) to
`fieldDescriptor.id`, `fieldDescriptor.name` so the outer `descriptor` (used
later for `descriptor.sections` and `descriptor.fields?.[fieldName]?.name`)
remains unshadowed and the intent is clear.
- Line 28: The import for Chip uses the barrel import "@mui/material" which is
inconsistent with other subpath imports in FieldInformationDialog.tsx; replace
the barrel import line importing Chip with a subpath import from
"@mui/material/Chip" (keeping the existing named symbol Chip) so tree-shaking
and import style match other components in this file.
In `@ui/app/src/components/FormsEngine/components/FormsEngineField.tsx`:
- Around line 253-269: Remove the redundant fragment wrapping the single
IconButton in the action prop of the FormsEngineField component: locate the
action={ <> <IconButton ... /> </> } JSX (the IconButton that calls
globalApi.pushForm with update: { path: sourceMap[fieldId] }) and replace it
with the IconButton directly (action={ <IconButton ... /> }) so there is no
unnecessary <>...</> wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 386b7066-5450-4f3d-ab47-487eed186f75
📒 Files selected for processing (3)
ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsxui/app/src/components/FormsEngine/components/FormsEngineField.tsxui/app/src/env/studioUI.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx (1)
93-95: ⚡ Quick winUse a stable section key instead of array index.
key={index}can cause reconciliation issues if section ordering changes. Prefer a stable identifier such assection.id(with a safe fallback only if needed).Proposed change
- {descriptor.sections?.map((section, index) => ( - <Card key={index}> + {descriptor.sections?.map((section, index) => ( + <Card key={section.id ?? index}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx` around lines 93 - 95, The Card elements in FieldInformationDialog.tsx use key={index} when rendering descriptor.sections in the map, which can break reconciliation; update the map to use a stable identifier such as section.id for the React key (e.g., key={section.id}) and only fall back to a deterministic alternative if id is absent (for example a concatenation of another stable field like section.name or a generated stable id), ensuring the key is unique and stable across renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx`:
- Around line 93-95: The Card elements in FieldInformationDialog.tsx use
key={index} when rendering descriptor.sections in the map, which can break
reconciliation; update the map to use a stable identifier such as section.id for
the React key (e.g., key={section.id}) and only fall back to a deterministic
alternative if id is absent (for example a concatenation of another stable field
like section.name or a generated stable id), ensuring the key is unique and
stable across renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf7b5225-a44b-4290-b8d3-e26e1115fd17
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the new FieldInformationDialog component
cat -n ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsx
echo "---SEPARATOR---"
# Read the modified FormsEngineField
cat -n ui/app/src/components/FormsEngine/components/FormsEngineField.tsx
echo "---SEPARATOR---"
# Check studioUI.ts around the new entry
rg -n "FieldInformationDialog" ui/app/src/env/studioUI.ts -C 3
echo "---SEPARATOR---"
# Check how pushDialog / createComponentId are used elsewhere to confirm pattern correctness
rg -n "createComponentId" ui/app/src/components/FormsEngine/components/FormsEngineField.tsx
echo "---SEPARATOR---"
# Confirm FieldInformationDialog is registered as a component id
rg -rn "FieldInformationDialog" ui/app/src/
echo "---SEPARATOR---"
# Check if there are any TypeScript issues with the props passed to FieldInformationDialog
rg -n "FieldInformationDialogProps\|FieldInformationDialog" ui/app/src/components/FormsEngine/components/FieldInformationDialog.tsxLength of output: 16155 🏁 Script executed: #!/bin/bash
# Read the actual FieldInformationDialog component file
cat -n ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx
echo "---SEPARATOR---"
cat -n ui/app/src/components/FieldInformationDialog/index.ts
echo "---SEPARATOR---"
# Exact lines around FieldInformationDialog in studioUI.ts
sed -n '135,150p' ui/app/src/env/studioUI.ts
echo "---SEPARATOR---"
# Check the exact line in FormsEngineField.tsx around createComponentId
grep -n "createComponentId" ui/app/src/components/FormsEngine/components/FormsEngineField.tsxLength of output: 8380
✅ What looks good
🔶 Minor issues to address before merging
🔵 Nit (no action required)
Overall this is ready for human review once the alphabetical ordering is fixed and the |
…refactor FormsEngineField component structure for improved readability and functionality.
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
✅ Ready for human review, pending these items:
Everything else looks solid:
Once you address (or consciously accept) items 1 & 2, this is good to hand off. 🐇 ✅ Action performedFull review finished. |
|
@CodeRabbit it seems like you checked against a previous version. Also, the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== FieldInformationDialog.tsx ==="
fd "FieldInformationDialog.tsx" ui/app/src --exec cat -n {}
echo "=== FieldInformationDialog index ==="
fd "index.ts" ui/app/src/components/FieldInformationDialog --exec cat -n {}
echo "=== FormsEngineField.tsx ==="
cat -n ui/app/src/components/FormsEngine/components/FormsEngineField.tsx
echo "=== studioUI.ts around FieldInformationDialog ==="
grep -n "FieldInformationDialog\|FolderBrowser\|FormsEngine" ui/app/src/env/studioUI.ts
echo "=== studioUI.ts full entry list (just component lines) ==="
grep -n "lazy(" ui/app/src/env/studioUI.ts | head -80Length of output: 26672
✅ Everything looks goodAll three previously raised concerns are already resolved in this version:
A few things specifically verified in the current code:
This is ready to hand off for human review. 🐇 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx`:
- Around line 82-83: The Typography component that renders HTML via
dangerouslySetInnerHTML is missing the component="section" prop, which can cause
invalid DOM nesting when the help text contains block-level elements like
paragraphs or lists. Add component="section" to the Typography component on line
83 that renders the value with dangerouslySetInnerHTML to properly contain the
rich HTML content, following the same pattern used in FormsEngineField.tsx for
rendering field.helpText.
In `@ui/app/src/components/FormsEngine/components/FormsEngineField.tsx`:
- Around line 234-259: The two icon-only IconButton components in the
inheritance alert (the one with InfoOutlinedIcon at line 234 and the one with
EditOutlined at line 248) currently rely only on the title prop for
accessibility. Add an explicit aria-label prop to both IconButton components
using the same formatMessage calls as their respective title props to ensure
reliable naming for assistive technology. The first IconButton should have
aria-label={formatMessage({defaultMessage: 'Learn more about content
inheritance'})} and the second should have
aria-label={formatMessage({defaultMessage: 'Edit'})}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46338ff9-ea27-419b-b434-91f9866fa5b8
📒 Files selected for processing (4)
ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsxui/app/src/components/FieldInformationDialog/index.tsui/app/src/components/FormsEngine/components/FormsEngineField.tsxui/app/src/env/studioUI.ts
| {renderHtml ? ( | ||
| <Typography dangerouslySetInnerHTML={{ __html: value ?? '-' }} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
cat -n ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx | sed -n '75,95p'Repository: craftercms/studio-ui
Length of output: 976
🏁 Script executed:
ast-grep outline ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx --view expandedRepository: craftercms/studio-ui
Length of output: 589
🏁 Script executed:
cat -n ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx | sed -n '1,100p'Repository: craftercms/studio-ui
Length of output: 4522
🏁 Script executed:
# Check for Typography component import and usage
rg "Typography" ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx -A 1 -B 1Repository: craftercms/studio-ui
Length of output: 1441
🏁 Script executed:
# Check where renderHtml and value are defined
rg "renderHtml|value" ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx -B 3 -A 1 | head -60Repository: craftercms/studio-ui
Length of output: 1261
🏁 Script executed:
# Check what basicPropsMap maps to, especially for help
rg "basicPropsMap|helpText" ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx -B 2 -A 2Repository: craftercms/studio-ui
Length of output: 440
🏁 Script executed:
# Check the ContentTypeField model to understand what field.helpText contains
find ui/app/src -name "*.ts" -o -name "*.tsx" | xargs grep -l "ContentTypeField" | head -5Repository: craftercms/studio-ui
Length of output: 487
🏁 Script executed:
# Look for helpText field definition in models
rg "helpText" ui/app/src/models --type ts --type tsx -B 2 -A 2Repository: craftercms/studio-ui
Length of output: 186
🏁 Script executed:
# Check if there are any examples or tests showing what help text content looks like
rg "dangerouslySetInnerHTML|helpText.*html|help.*<" ui/app/src --type ts --type tsx -B 2 -A 2 | head -40Repository: craftercms/studio-ui
Length of output: 186
🏁 Script executed:
# Find ContentTypeField definition
rg "interface ContentTypeField|type ContentTypeField" ui/app/src --type ts -A 15Repository: craftercms/studio-ui
Length of output: 8964
🏁 Script executed:
# Look for where helpText is used or defined
rg "helpText" ui/app/src -B 2 -A 2 | head -50Repository: craftercms/studio-ui
Length of output: 4319
🏁 Script executed:
# Check if help text can contain HTML by looking at RTE or rich text usage
rg "richText|rte|RTE|html.*help|help.*html|dangerouslySetInnerHTML" ui/app/src --type ts -B 2 -A 2 | head -60Repository: craftercms/studio-ui
Length of output: 5149
🏁 Script executed:
# Check Typography default component mapping in MUI
web_search "Material-UI Typography default component variant rendering"Repository: craftercms/studio-ui
Length of output: 206
🏁 Script executed:
# Check the exact implementation in FormsEngineField to compare
cat -n ui/app/src/components/FormsEngine/components/FormsEngineField.tsx | sed -n '70,140p'Repository: craftercms/studio-ui
Length of output: 2756
🏁 Script executed:
# Verify if helpText in the model can contain HTML
cat -n ui/app/src/models/ContentType.ts | sed -n '1,150p'Repository: craftercms/studio-ui
Length of output: 4784
🏁 Script executed:
# Get more context around the dangerouslySetInnerHTML usage in FormsEngineField
cat -n ui/app/src/components/FormsEngine/components/FormsEngineField.tsx | sed -n '155,185p'Repository: craftercms/studio-ui
Length of output: 1409
🏁 Script executed:
# Search for all instances of dangerouslySetInnerHTML to see the pattern used elsewhere
rg "dangerouslySetInnerHTML" ui/app/src -B 2 -A 2Repository: craftercms/studio-ui
Length of output: 3354
Add component="section" to Typography to properly contain rich HTML help text.
Line 83 renders help text via dangerouslySetInnerHTML into Typography without specifying a component. Since help text is a string field that can contain HTML, this may inject block-level elements (paragraphs, lists, etc.) into an inline-by-default component, causing invalid DOM nesting. The codebase already follows this pattern elsewhere—FormsEngineField.tsx renders the same field.helpText with component="section" for the same purpose.
Proposed fix
- <Typography dangerouslySetInnerHTML={{ __html: value ?? '-' }} />
+ <Typography component="section" dangerouslySetInnerHTML={{ __html: 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.
| {renderHtml ? ( | |
| <Typography dangerouslySetInnerHTML={{ __html: value ?? '-' }} /> | |
| {renderHtml ? ( | |
| <Typography component="section" dangerouslySetInnerHTML={{ __html: value ?? '-' }} /> |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 82-82: 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
(react-unsafe-html-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/app/src/components/FieldInformationDialog/FieldInformationDialog.tsx`
around lines 82 - 83, The Typography component that renders HTML via
dangerouslySetInnerHTML is missing the component="section" prop, which can cause
invalid DOM nesting when the help text contains block-level elements like
paragraphs or lists. Add component="section" to the Typography component on line
83 that renders the value with dangerouslySetInnerHTML to properly contain the
rich HTML content, following the same pattern used in FormsEngineField.tsx for
rendering field.helpText.
| <IconButton | ||
| href="/" | ||
| size="small" | ||
| color="inherit" | ||
| target="_blank" | ||
| component="a" | ||
| title={formatMessage({ defaultMessage: 'Learn more about content inheritance' })} | ||
| sx={{ p: 0 }} | ||
| > | ||
| <InfoOutlinedIcon /> | ||
| </IconButton> | ||
| } | ||
| action={ | ||
| <> | ||
| <Button | ||
| <IconButton | ||
| color="inherit" | ||
| size="small" | ||
| sx={{ px: 0.5, minWidth: 0 }} | ||
| onClick={() => { | ||
| globalApi.pushForm({ | ||
| readonly: true, | ||
| update: { path: sourceMap[fieldId] } | ||
| }); | ||
| }} | ||
| title={formatMessage({ defaultMessage: 'Edit' })} | ||
| > | ||
| View | ||
| </Button> | ||
| {/* TODO: Create or link to content inheritance article */} | ||
| <IconButton | ||
| href="/" | ||
| size="small" | ||
| color="inherit" | ||
| target="_blank" | ||
| component="a" | ||
| title={formatMessage({ defaultMessage: 'Learn more about content inheritance' })} | ||
| > | ||
| <HelpOutlineRounded fontSize="small" /> | ||
| <EditOutlined fontSize="small" /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add aria-label to icon-only actions in the inheritance alert.
Line 234 and Line 248 use icon-only IconButtons with title, but explicit aria-label is needed for reliable assistive-tech naming.
Proposed fix
<IconButton
href="/"
size="small"
color="inherit"
target="_blank"
component="a"
+ aria-label={formatMessage({ defaultMessage: 'Learn more about content inheritance' })}
title={formatMessage({ defaultMessage: 'Learn more about content inheritance' })}
sx={{ p: 0 }}
>
<InfoOutlinedIcon />
</IconButton>
@@
<IconButton
color="inherit"
size="small"
sx={{ px: 0.5, minWidth: 0 }}
+ aria-label={formatMessage({ defaultMessage: 'Edit inherited source content' })}
onClick={() => {
globalApi.pushForm({
update: { path: sourceMap[fieldId] }
});
}}
title={formatMessage({ defaultMessage: 'Edit' })}
>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/app/src/components/FormsEngine/components/FormsEngineField.tsx` around
lines 234 - 259, The two icon-only IconButton components in the inheritance
alert (the one with InfoOutlinedIcon at line 234 and the one with EditOutlined
at line 248) currently rely only on the title prop for accessibility. Add an
explicit aria-label prop to both IconButton components using the same
formatMessage calls as their respective title props to ensure reliable naming
for assistive technology. The first IconButton should have
aria-label={formatMessage({defaultMessage: 'Learn more about content
inheritance'})} and the second should have
aria-label={formatMessage({defaultMessage: 'Edit'})}.
2 participants
craftercms/craftercms#8424
Summary by CodeRabbit
New Features
Improvements
Bug Fixes