[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 8 commits intocraftercms:developfrom
Conversation
…le to edit source, and move 'learn more' link to alert icon
WalkthroughAdds a new FieldInformationDialog component that displays metadata for a ContentTypeField and integrates it into FormsEngineField; FormsEngineField now opens the dialog from its menu, adjusts header/help/inheritance UI, and adds inline edit/info actions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Field as FormsEngineField
participant Dialog as FieldInformationDialog
participant Descriptors as ControlDescriptors
User->>Field: Click "Field Information" menu option
Field->>Field: dispatch pushDialog({ component: FieldInformationDialog, props: { field } })
Field->>Dialog: Render with `field` prop (open)
Dialog->>Descriptors: Retrieve descriptor for `field.type` (fallback to inputDescriptor)
Descriptors-->>Dialog: Return descriptor + sections
Dialog->>Dialog: Build Basic Properties + descriptor sections, translate labels, format values (booleans → Chips)
Dialog-->>User: Display modal with field metadata
User->>Dialog: Close modal
Dialog->>Field: onClose -> dialog closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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!
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 |
|
| const isValid = props.isValid ?? (validityData.state === 'hasData' ? validityData?.data.isValid : true); | ||
| const handleCloseMenu = () => setOpenMenu(false); | ||
| const handleRollback = () => formApi.rollbackField(field.id); | ||
| const fieldInformationDialogState = useEnhancedDialogState(); |
There was a problem hiding this comment.
Could we pop out the info dialog so there's a single instance? Controls would simply trigger open with the event.
| 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
| function Body(props) { | ||
| const { field } = props; | ||
| const { formatMessage } = useIntl(); | ||
|
|
||
| // retrieve control descriptor for field type | ||
| const descriptor = controlDescriptors[field.type] ?? inputDescriptor; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Body is missing prop typings.
function Body(props) has an implicit-any props parameter, which defeats TypeScript checking on the field destructure and on field.type/field.properties/field.validations usage below. Please type it explicitly:
-function Body(props) {
- const { field } = props;
+interface BodyProps {
+ field: ContentTypeField;
+}
+
+function Body({ field }: BodyProps) {🤖 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 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.
| <MenuItem | ||
| onClick={() => { | ||
| dispatch( | ||
| pushDialog({ | ||
| component: createComponentId('FieldInformationDialog'), | ||
| props: { field } | ||
| }) | ||
| ); | ||
| }} | ||
| > | ||
| <ListItemText> | ||
| <FormattedMessage defaultMessage="Rollback changes" /> | ||
| <FormattedMessage defaultMessage="Field Information" /> | ||
| </ListItemText> | ||
| </MenuItem> |
There was a problem hiding this comment.
Multiple clicks push duplicate FieldInformationDialog instances onto the stack.
Every click on this menu item dispatches pushDialog without any dedupe. Unlike CodeEditorDialog/FormsEngineDialog/LegacyFormDialog, the reducer does not special‑case FieldInformationDialog, so opening the menu twice (e.g., on two different fields or the same field) stacks multiple dialogs. This is the concern reviewer rart raised earlier about keeping a single instance.
Consider one of:
- Only dispatch
pushDialogif noFieldInformationDialogis currently open, and otherwise update itsprops(e.g., via anupdateDialog-style action) so the same instance is reused across fields. - Or extend the
pushDialogreducer dedupe to treatFieldInformationDialogsimilarly to the other dialogs that already get merged.
🤖 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 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.
2 participants
craftercms/craftercms#8424
Summary by CodeRabbit
New Features
Improvements
Bug Fixes