feat: markdown editor#194
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Markdown editor and related toolbar/plugins to the design-system, expands global CSS for MDXEditor/popovers, enables rich/plain modes for SocialTextField, guards ActionsOverlay and CommentsList to early-return when nothing to show, and bumps package versions and workspace catalog. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Consumer Component
participant Field as MarkdownEditorField
participant MDX as MDXEditor
participant Plugin as KeyboardCommandsPlugin
participant Toolbar as Toolbar
App->>Field: mount(value, toolbarConfig, handlers)
Field->>MDX: dynamic import (ssr: false) → render InitializedMDXEditor with editorRef
MDX->>Plugin: init(realm) → subscribe rootEditor$
Plugin->>MDX: register KEY_DOWN / PASTE commands
Toolbar->>MDX: user actions → editor commands (bold/italic/insert)
MDX-->>Field: onChange(markdown)
Field-->>App: forward onChange(value)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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: 3
🧹 Nitpick comments (13)
packages/design-system/components/Markdown/highlight.ts (2)
4-8: Global declaration of hljs
Defininghljson the globalwindowobject can lead to potential conflicts or shadowing in certain environments. Consider abstracting the highlight logic instead of adding it to the global scope.
14-16: Conditional attachment to the window object
While checkingtypeof window !== 'undefined'for SSR compatibility is helpful, you may still want to isolatehljsusage in a dedicated utility for improved testability and reduced global pollution.packages/design-system/components/images/LazyLoadImage/utils.ts (1)
3-15: Provide a fallback or error for unrecognized ratio.Currently, this function returns
undefinedif a non-predefined ratio is passed, which may break layouts if the calling code doesn't handleundefined. Consider providing a meaningful fallback or throwing an error for unrecognized ratios.Example refactor:
-export function getRatio(ratio = '1/1') { - return { - '4/3': 'calc(100% / 4 * 3)', - '3/4': 'calc(100% / 3 * 4)', - '6/4': 'calc(100% / 6 * 4)', - '4/6': 'calc(100% / 4 * 6)', - '16/9': 'calc(100% / 16 * 9)', - '9/16': 'calc(100% / 9 * 16)', - '21/9': 'calc(100% / 21 * 9)', - '9/21': 'calc(100% / 9 * 21)', - '1/1': '100%', - }[ratio] +export function getRatio(ratio = '1/1'): string { + const ratioMap: Record<string, string> = { + '4/3': 'calc(100% / 4 * 3)', + '3/4': 'calc(100% / 3 * 4)', + '6/4': 'calc(100% / 6 * 4)', + '4/6': 'calc(100% / 4 * 6)', + '16/9': 'calc(100% / 16 * 9)', + '9/16': 'calc(100% / 9 * 16)', + '21/9': 'calc(100% / 21 * 9)', + '9/21': 'calc(100% / 9 * 21)', + '1/1': '100%', + } + return ratioMap[ratio] || ratioMap['1/1'] }packages/design-system/components/images/LazyLoadImage/types.ts (1)
1-20: Unify ratio definitions with the logic in getRatio.Ensure that the string literals in
LazyLoadImageRatioalways match the explicit keys handled bygetRatioto avoid runtime mismatches. Consider centralizing these values in a single script or shared enum.packages/design-system/components/images/LazyLoadImage/index.tsx (2)
14-36: Provide clearer naming & doc-block for clarity on forwarded ref usage.While using
forwardRefhere is appropriate, adding a short JSDoc-style comment describing the forwarded ref purpose will help other developers understand how to interact with the component.+/** + * LazyLoadImage component uses forwardRef to pass a ref to the underlying container. + * This can be especially useful in scenarios where parent components need direct access + * to DOM elements for measurements or scroll events. + */ const LazyLoadImage = forwardRef<HTMLSpanElement, LazyLoadImageProps>( ... )
41-52: Overlay logic is implemented neatly; consider layering options to handle complex backgrounds.Everything looks organized. If you anticipate gradient overlays or additional styling, expand the overlay logic with more flexible props or a new prop interface (e.g.,
overlayProps).packages/components/modules/messages/SendMessage/index.tsx (1)
6-6: Check if dynamic imports are needed for performance.Importing
MarkdownEditoron demand could improve initial load times if large dependencies are involved. Consider using a dynamic import if performance metrics show the need for lazy loading.packages/design-system/components/Markdown/styled.ts (3)
16-20: Pause on styling<br>as a grid element.
Styling the<br>tag withdisplay: 'grid'and addingcontentcan break standard line-break semantics in certain contexts. Consider verifying it in cross-browser scenarios to ensure it doesn’t create unexpected spacing issues.
42-70: Check blockquote’s large font size for consistency.
AfontSizeof1.5emfor blockquotes may be significantly larger than the surrounding text, which can disrupt reading flow in some designs. Confirm the design intention or consider adjusting it if it looks out of place in the final UI.
106-140: Improve keyboard accessibility for custom checkboxes.
The custom styling for checkboxes is visually appealing, but ensure keyboard navigation, focus styling, and ARIA attributes remain intact. You may need additional:focusor:focus-visiblestyles for better accessibility compliance.packages/design-system/package.json (2)
54-54: Revisit the re-addition of@baseapp-frontend/tsconfig.
Removing and re-adding the same version might indicate a version mismatch or a prior installation issue. Double-check that this dev dependency is at the correct version to maintain consistent type definitions across all packages.
66-70: Ensure correct type definitions batch import.
The type definitions forhighlight.jsandreact-lazy-load-image-componentare pinned to specific versions. If those libraries update, these type definitions might lag behind. Keep an eye on new library releases to maintain compatibility.packages/design-system/components/inputs/SocialTextField/types.ts (1)
15-15: Consider a more robust type strategy to avoid potential mismatches.By using a union type for
FC<TextareaFieldProps> | FC<MarkdownEditorProps>, it’s possible to introduce runtime type mismatches if the parent incorrectly passes markdown-based components expecting text-area props, or vice versa. Consider introducing a safe fallback or a discriminated union to guarantee correct usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
packages/components/modules/messages/MessageUpdate/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(2 hunks)packages/components/modules/messages/SendMessage/index.tsx(2 hunks)packages/design-system/components/Markdown/highlight.ts(1 hunks)packages/design-system/components/Markdown/index.tsx(1 hunks)packages/design-system/components/Markdown/styled.ts(1 hunks)packages/design-system/components/Markdown/types.ts(1 hunks)packages/design-system/components/images/LazyLoadImage/__storybook__/stories.tsx(1 hunks)packages/design-system/components/images/LazyLoadImage/index.tsx(1 hunks)packages/design-system/components/images/LazyLoadImage/types.ts(1 hunks)packages/design-system/components/images/LazyLoadImage/utils.ts(1 hunks)packages/design-system/components/images/index.ts(1 hunks)packages/design-system/components/inputs/MarkdownEditor/MarkdownEditorStyles.css(1 hunks)packages/design-system/components/inputs/MarkdownEditor/index.tsx(1 hunks)packages/design-system/components/inputs/MarkdownEditor/types.ts(1 hunks)packages/design-system/components/inputs/SocialTextField/index.tsx(2 hunks)packages/design-system/components/inputs/SocialTextField/types.ts(1 hunks)packages/design-system/components/inputs/index.ts(1 hunks)packages/design-system/index.ts(1 hunks)packages/design-system/package.json(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/design-system/components/inputs/MarkdownEditor/MarkdownEditorStyles.css
🧰 Additional context used
🪛 GitHub Actions: Main Workflow
packages/design-system/components/inputs/SocialTextField/index.tsx
[error] 51-51: Type mismatch error: The provided type for component props is not assignable to the expected type. This appears to be related to Control and MDEditorProps type definitions.
packages/design-system/components/Markdown/index.tsx
[error] 2-2: CommonJS module imports will produce 'require' calls but referenced file is an ECMAScript module and cannot be imported with 'require'
🪛 Biome (1.9.4)
packages/design-system/components/Markdown/index.tsx
[error] 20-20: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (21)
packages/design-system/components/images/index.ts (1)
3-3: Add new export for LazyLoadImage
This addition looks consistent with the existing pattern of exporting image components in this index file.packages/design-system/components/Markdown/types.ts (1)
1-6: Interface extension approach looks good
By extendingOptionsfromreact-markdownand adding a flexiblesxprop, you keep styling concerns cleanly separated while retaining all base Markdown options.packages/design-system/components/inputs/MarkdownEditor/types.ts (1)
1-11: MarkdownEditorProps type structure
CombiningMDEditorPropsandFormControlalong with optional handlers (e.g.onKeyDown) and placeholders appears both flexible and consistent with common form patterns in this codebase.packages/design-system/components/Markdown/highlight.ts (2)
1-2: Importing Highlight.js and applying a theme
The chosentomorrow-night.csstheme ensures a dark code syntax highlight. This is a sound default for improved readability.
10-12: Whitelisted languages for config
Specifically configuring only necessary languages helps optimize performance while still offering robust highlighting. This targeted approach is good practice.packages/design-system/components/inputs/index.ts (1)
1-2: Re-exporting MarkdownEditor and types looks good.This approach provides a consistent entry point for consumers of the design-system, keeping import statements clean and well-organized.
packages/design-system/components/images/LazyLoadImage/__storybook__/stories.tsx (1)
1-28: Code structure and Storybook usage look great!All newly introduced lines in this Storybook file appear well-structured, with clear separation of the default story variant. The usage of Meta, StoryFn, and StoryObj from
@storybook/reactadheres to best practices, making it straightforward to add additional story variations in the future.packages/design-system/components/inputs/MarkdownEditor/index.tsx (1)
12-36: Integration with@uiw/react-md-editoris properly implemented.The Markdown editor setup is intuitive, especially the usage of
onKeyDownfor custom controls. The container class approach is concise, and the custom placeholder logic intextareaPropsis a nice touch.packages/design-system/index.ts (1)
47-49: Exporting the Markdown component via index.ts is consistent with other exports.This follows the existing pattern and improves discoverability by allowing consumers to import the
Markdowncomponent and its types directly from the design-system index. No further issues identified.packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (2)
4-4: Importing the Markdown component is a good step towards rendering rich text.This addition cleanly reuses the design-system’s Markdown component. Just verify that there aren’t any unused imports left in the file, and ensure that you are bundling only relevant icons.
40-41: Replacing raw text with the Markdown component is an excellent move.You have commented out the raw text usage in
Typographyand introduced<Markdown>. This is a clean approach, preserving formatting while also supporting additional features such as links and bold/italic text.packages/design-system/components/images/LazyLoadImage/index.tsx (2)
54-72: Ensure placeholders exist and are optimized.You’re correctly handling placeholders by providing a transparent image or a blur effect. If you expect large images, consider using a lightweight placeholder (e.g., a small low-resolution or base64-encoded image) to optimize page performance.
86-115: Excellent ratio-based styling approach.Using the
getRatiohelper for aspect ratios is clean. Keep in mind that if you handle videos or unusual image sizes, you may need to expand logic to avoid content shifting.packages/components/modules/messages/SendMessage/index.tsx (1)
128-130: Introducing the MarkdownEditor is a nice enhancement.Passing
MarkdownEditortoSocialTextFieldPropsis straightforward and cleanly encapsulated. Ensure you thoroughly test the user experience, especially around editor initialization times or in lower-bandwidth scenarios.packages/design-system/components/Markdown/styled.ts (2)
3-4: Consider clarifying the fallback for dark mode.
You’re assigninglightModebased ontheme.palette.mode === 'light', but there’s no direct check fortheme.palette.mode === 'dark'. If another mode is introduced or if the theme mode is set differently in some contexts, the style logic may produce unexpected behavior.
72-91: Ensure consistent code highlighting background in dark mode.
backgroundColor: lightMode ? theme.palette.grey[900] : alpha(theme.palette.grey[500], 0.16)yields a dark background even when not in dark mode. Verify that this color difference is intentional for the code blocks in dark themes.packages/components/modules/messages/MessageUpdate/index.tsx (1)
149-151: Confirm fallback behavior for non-Markdown inputs.
SettingTextFieldtoMarkdownEditorwithinSocialTextFieldPropsis a nice integration. However, verify if other areas of the app that rely on plain text editing still work properly, or if they also default to Markdown.packages/design-system/package.json (1)
28-40: Validate newly added Markdown dependencies.
You’ve added@uiw/react-md-editor,highlight.js,react-markdown, and related plugins. Confirm that your bundling process, minification, and tree-shaking properly handle these new libraries to avoid bloating the final bundle.packages/design-system/components/inputs/SocialTextField/types.ts (3)
1-1: Glad to seeFCbeing used for typed functional components.This import from React is standard for ensuring type safety on custom functional components.
5-5: Import ofMarkdownEditorPropslooks correct.The referencing of
MarkdownEditorPropsis necessary for supporting Markdown editor components.
7-7:TextareaFieldPropsimport is correctly placed.No issues with this import; it aligns well with the existing structure.
| @@ -0,0 +1,47 @@ | |||
| import Link from '@mui/material/Link' | |||
| import ReactMarkdown from 'react-markdown' | |||
There was a problem hiding this comment.
Resolve pipeline failure for ESM imports vs. CommonJS.
A pipeline error indicates that CommonJS imports produce require calls, conflicting with ESM modules. Ensure your build config or bundler settings handle ESM imports properly. You may need to adjust your tsconfig.json or Babel configuration to enable ESM for react-markdown or vice versa.
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] 2-2: CommonJS module imports will produce 'require' calls but referenced file is an ECMAScript module and cannot be imported with 'require'
Ronan-Fernandes
force-pushed
the
BA-2167-FE-markdown-editor
branch
from
678baec to
1f141a4
Compare
Ronan-Fernandes
force-pushed
the
BA-2167-FE-markdown-editor
branch
2 times, most recently
from
e801459 to
a21158f
Compare
|
@anicioalexandre and @Ronan-Fernandes does this PR still makes sense? If no, can we close it? If yes, can you please rebase @Ronan-Fernandes ? |
Ronan-Fernandes
force-pushed
the
BA-2167-FE-markdown-editor
branch
from
aed2b4f to
88d0f9d
Compare
|
anicioalexandre
added
needs changes
davidbermudez-tsl
force-pushed
the
BA-2167-FE-markdown-editor
branch
3 times, most recently
from
f756ade to
c6a4b6e
Compare
|
davidbermudez-tsl
force-pushed
the
BA-2167-FE-markdown-editor
branch
2 times, most recently
from
6d9416a to
f437617
Compare
davidbermudez-tsl
force-pushed
the
BA-2167-FE-markdown-editor
branch
from
f437617 to
23225d9
Compare
anicioalexandre
force-pushed
the
BA-2167-FE-markdown-editor
branch
from
8ada307 to
b47c1b8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/components/modules/__shared__/web/SocialInput/index.tsx (1)
103-114:⚠️ Potential issue | 🟡 MinorMove
{...SocialTextFieldProps}beforemode={mode}to avoid silent override.At line 114,
{...SocialTextFieldProps}overrides the explicitmode={mode}prop (line 110) when both providemode. SinceSocialTextFieldPropstype includesmode?: SocialTextFieldMode, a caller could unintentionally override the top-level mode by passingSocialTextFieldProps={{ mode: ... }}.Move the spread before the explicit props to give precedence to the component's top-level interface, or type
SocialTextFieldPropsasOmit<Partial<SocialTextFieldProps>, 'mode'>to prevent the conflict entirely.Proposed fix (option 1: reorder attributes)
<SocialTextField + {...SocialTextFieldProps} inputRef={ref} name={SOCIAL_UPSERT_FORM.body} control={form.control} onKeyDown={handleKeyDown} placeholder={placeholder} autoFocus={autoFocusInput} mode={mode} isReply={isReply} replyTargetName={replyTargetName} onCancelReply={onCancelReply} - {...SocialTextFieldProps}Proposed fix (option 2: type constraint)
- SocialTextFieldProps?: Partial<SocialTextFieldProps> + SocialTextFieldProps?: Omit<Partial<SocialTextFieldProps>, 'mode'>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/__shared__/web/SocialInput/index.tsx` around lines 103 - 114, The spread SocialTextFieldProps is placed after explicit props and can silently override mode; in the SocialInput usage of SocialTextField (props: inputRef/ref, name=SOCIAL_UPSERT_FORM.body, control=form.control, onKeyDown=handleKeyDown, placeholder, autoFocus, mode, isReply, replyTargetName, onCancelReply, ...SocialTextFieldProps) move {...SocialTextFieldProps} before the explicit mode={mode} (and ideally before other explicit props you want to win) so the component's top-level mode prop cannot be overridden, or alternatively change the SocialTextFieldProps type to exclude mode (e.g., Omit<..., 'mode'>) to prevent callers from passing mode via the spread.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/components/modules/__shared__/web/SocialInput/index.tsx`:
- Around line 103-114: The spread SocialTextFieldProps is placed after explicit
props and can silently override mode; in the SocialInput usage of
SocialTextField (props: inputRef/ref, name=SOCIAL_UPSERT_FORM.body,
control=form.control, onKeyDown=handleKeyDown, placeholder, autoFocus, mode,
isReply, replyTargetName, onCancelReply, ...SocialTextFieldProps) move
{...SocialTextFieldProps} before the explicit mode={mode} (and ideally before
other explicit props you want to win) so the component's top-level mode prop
cannot be overridden, or alternatively change the SocialTextFieldProps type to
exclude mode (e.g., Omit<..., 'mode'>) to prevent callers from passing mode via
the spread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fa17b8b-8d0f-42c3-b3c1-bc97a78ddce4
📒 Files selected for processing (5)
packages/components/CHANGELOG.mdpackages/components/modules/__shared__/web/ActionsOverlay/index.tsxpackages/components/modules/__shared__/web/SocialInput/index.tsxpackages/components/modules/__shared__/web/SocialInput/styled.tsxpackages/components/modules/__shared__/web/SocialInput/types.ts
✅ Files skipped from review due to trivial changes (3)
- packages/components/CHANGELOG.md
- packages/components/modules/shared/web/SocialInput/styled.tsx
- packages/components/modules/shared/web/ActionsOverlay/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/design-system/styles/web/material/overrides/components/css-baseline.tsx (1)
46-175: Consider extracting base selector constants to reduce repetition.The repeated long selectors make this block harder to maintain and update safely.
Refactor sketch
export function cssBaseline(theme: Theme): Pick<Components<Theme>, 'MuiCssBaseline'> { + const radixWrapper = '[data-radix-popper-content-wrapper]' + const radixDialog = `${radixWrapper} div[role="dialog"]` + const radixTooltip = `${radixWrapper} div[role="tooltip"]` + return { MuiCssBaseline: { styleOverrides: { @@ - '[data-radix-popper-content-wrapper] div[role="dialog"]': { + [radixDialog]: { @@ - '[data-radix-popper-content-wrapper] div[role="dialog"] form': { + [`${radixDialog} form`]: { @@ - '[data-radix-popper-content-wrapper] div[role="tooltip"]': { + [radixTooltip]: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/design-system/styles/web/material/overrides/components/css-baseline.tsx` around lines 46 - 175, The CSS override block repeats the long selector "[data-radix-popper-content-wrapper] div[role=\"dialog\"]" (and variants) across many rules; extract these base selector strings into constants (e.g., popperDialog, popperDialogForm, popperTooltip) and replace each repeated literal with the corresponding constant/template so the styles for '[data-radix-popper-content-wrapper] div[role="dialog"]', '[data-radix-popper-content-wrapper] div[role="dialog"] form', and '[data-radix-popper-content-wrapper] div[role="tooltip"]' are built from those constants; ensure combined selectors (like the :has(> a) and svg/polygon groups) are composed from the same constants to preserve specificity and update imports/exports if needed so the file (css-baseline.tsx) uses the new constants consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/design-system/styles/web/material/overrides/components/css-baseline.tsx`:
- Around line 46-70: The CSS selectors targeting Radix popper dialogs are too
global; restrict them to the MDXEditor by prefixing each selector that currently
begins with '[data-radix-popper-content-wrapper] div[role="dialog"]' (and its
children: form, form > div:last-of-type, label, etc.) with an MDXEditor-specific
ancestor like '[data-mdx-editor]' or a unique class (e.g., '.mdxEditor') so
these rules only apply to the MDXEditor link UI; update all occurrences in
css-baseline.tsx (the selector strings) to use the new scoped ancestor and keep
the same rule bodies.
- Around line 172-175: Summary: The CSS rule currently hides any svg polygon
inside the popper wrapper, which can unintentionally hide icons; change the
selector to target only the actual arrow element. Update the selector list in
the rule that currently includes 'svg polygon' to a more specific one such as
'svg.popoverArrow polygon' (or the arrow-specific attribute/class used in your
popper implementation) so only the polygon inside the popover arrow SVG is
affected, leaving other SVG icons untouched; adjust the selector string that
begins with '[data-radix-popper-content-wrapper] div[role="dialog"]'
accordingly.
---
Nitpick comments:
In
`@packages/design-system/styles/web/material/overrides/components/css-baseline.tsx`:
- Around line 46-175: The CSS override block repeats the long selector
"[data-radix-popper-content-wrapper] div[role=\"dialog\"]" (and variants) across
many rules; extract these base selector strings into constants (e.g.,
popperDialog, popperDialogForm, popperTooltip) and replace each repeated literal
with the corresponding constant/template so the styles for
'[data-radix-popper-content-wrapper] div[role="dialog"]',
'[data-radix-popper-content-wrapper] div[role="dialog"] form', and
'[data-radix-popper-content-wrapper] div[role="tooltip"]' are built from those
constants; ensure combined selectors (like the :has(> a) and svg/polygon groups)
are composed from the same constants to preserve specificity and update
imports/exports if needed so the file (css-baseline.tsx) uses the new constants
consistently.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1dbd26fe-8b69-4a84-964b-2f22ae4d40bd
📒 Files selected for processing (1)
packages/design-system/styles/web/material/overrides/components/css-baseline.tsx
| '[data-radix-popper-content-wrapper] div[role="dialog"]': { | ||
| backgroundColor: theme.palette.background.paper, | ||
| borderRadius: theme.shape.borderRadius * 2, | ||
| border: 'none', | ||
| boxShadow: theme.customShadows.dialog, | ||
| padding: theme.spacing(3), | ||
| }, | ||
| '[data-radix-popper-content-wrapper] div[role="dialog"] form': { | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| gap: theme.spacing(1), | ||
| overflow: 'hidden', | ||
| padding: 0, | ||
| '& > div': { | ||
| minWidth: 0, | ||
| }, | ||
| }, | ||
| '[data-radix-popper-content-wrapper] div[role="dialog"] label': { | ||
| ...theme.typography.body2, | ||
| color: theme.palette.text.secondary, | ||
| marginBottom: theme.spacing(0.25), | ||
| }, | ||
| // Button container — extra spacing above to separate from inputs | ||
| '[data-radix-popper-content-wrapper] div[role="dialog"] form > div:last-of-type': { | ||
| marginTop: theme.spacing(1), |
There was a problem hiding this comment.
Scope leakage risk: dialog selectors are too global.
These rules apply to any Radix popper dialog in the app, not only MDXEditor link UI. That can unintentionally restyle unrelated popovers/dialogs. Please scope with an MDXEditor-specific ancestor/data attribute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/design-system/styles/web/material/overrides/components/css-baseline.tsx`
around lines 46 - 70, The CSS selectors targeting Radix popper dialogs are too
global; restrict them to the MDXEditor by prefixing each selector that currently
begins with '[data-radix-popper-content-wrapper] div[role="dialog"]' (and its
children: form, form > div:last-of-type, label, etc.) with an MDXEditor-specific
ancestor like '[data-mdx-editor]' or a unique class (e.g., '.mdxEditor') so
these rules only apply to the MDXEditor link UI; update all occurrences in
css-baseline.tsx (the selector strings) to use the new scoped ancestor and keep
the same rule bodies.
| '[data-radix-popper-content-wrapper] div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper] div[role="dialog"] + span, [data-radix-popper-content-wrapper] svg polygon': | ||
| { | ||
| display: 'none', | ||
| }, |
There was a problem hiding this comment.
The svg polygon hide rule is overly broad and can break icons.
This can hide non-arrow SVG content inside the wrapper. Narrow it to the actual arrow element only.
Proposed fix
- '[data-radix-popper-content-wrapper] div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper] div[role="dialog"] + span, [data-radix-popper-content-wrapper] svg polygon':
+ '[data-radix-popper-content-wrapper] div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper] div[role="dialog"] + span':
{
display: 'none',
},📝 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.
| '[data-radix-popper-content-wrapper] div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper] div[role="dialog"] + span, [data-radix-popper-content-wrapper] svg polygon': | |
| { | |
| display: 'none', | |
| }, | |
| '[data-radix-popper-content-wrapper] div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper] div[role="dialog"] + span': | |
| { | |
| display: 'none', | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/design-system/styles/web/material/overrides/components/css-baseline.tsx`
around lines 172 - 175, Summary: The CSS rule currently hides any svg polygon
inside the popper wrapper, which can unintentionally hide icons; change the
selector to target only the actual arrow element. Update the selector list in
the rule that currently includes 'svg polygon' to a more specific one such as
'svg.popoverArrow polygon' (or the arrow-specific attribute/class used in your
popper implementation) so only the polygon inside the popover arrow SVG is
affected, leaving other SVG icons untouched; adjust the selector string that
begins with '[data-radix-popper-content-wrapper] div[role="dialog"]'
accordingly.
c713ca9 to
855f8e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/design-system/components/web/inputs/MarkdownEditorField/types.ts (1)
37-45: ExposeonPasteon the public field props too.
MarkdownEditorPropssupportsonPaste, butMarkdownEditorFieldCorePropsdrops it. Sincepackages/design-system/components/web/inputs/MarkdownEditorField/index.tsx:14-62forwards...propstoForwardRefEditor, this omission is what preventsMarkdownEditorFieldconsumers from wiring paste handling at the public component boundary.♻️ Suggested change
export type MarkdownEditorFieldCoreProps = Omit<MDXEditorProps, 'markdown'> & { value?: string onKeyDown?: KeyboardEventHandler<HTMLTextAreaElement> + onPaste?: ClipboardEventHandler<HTMLTextAreaElement> placeholder?: string onChange?: (value: string) => voidAlso applies to: 47-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/design-system/components/web/inputs/MarkdownEditorField/types.ts` around lines 37 - 45, MarkdownEditorProps declares onPaste but the public/core prop type used by the field drops it, preventing consumers from wiring paste handlers; update the core/public field prop types to include onPaste (e.g., add onPaste?: ClipboardEventHandler<HTMLTextAreaElement> to MarkdownEditorFieldCoreProps and the exported field prop type used by the MarkdownEditorField component) so that when MarkdownEditorField forwards ...props to ForwardRefEditor the onPaste handler is preserved and available to consumers.packages/design-system/components/web/inputs/MarkdownEditorField/Toolbar/index.tsx (1)
91-94: Consider wrapping the full toolbar inDiffSourceToggleWrapperif buttons need to be hidden in source/diff mode.Currently, only
UndoRedois wrapped.DiffSourceToggleWrapperonly controls its direct children, so formatting/list/insert buttons outside the wrapper will remain visible and clickable in source/diff mode. If these buttons should be unavailable in those modes, move the wrapper boundary to include all edit-mode-only controls, or provide aSourceToolbarchild to define what should appear in source mode instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/design-system/components/web/inputs/MarkdownEditorField/Toolbar/index.tsx` around lines 91 - 94, The toolbar only wraps UndoRedo with DiffSourceToggleWrapper so other edit-mode buttons remain active in source/diff mode; expand the wrapper (or create a SourceToolbar child) to encompass all edit-mode-only controls so DiffSourceToggleWrapper controls their visibility — update the JSX around showDiffSourceToggle, DiffSourceToggleWrapper, and UndoRedo (and other button components in this Toolbar) to move the wrapper boundary to include the full set of buttons that should be hidden in source/diff mode, or add a SourceToolbar slot and render that inside DiffSourceToggleWrapper when showDiffSourceToggle is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/components/modules/comments/web/CommentsList/index.tsx`:
- Around line 29-31: The early return in CommentsList that returns EmptyState
when comments.length === 0 prevents CommentsSubscription from mounting, so live
updates for empty threads are missed; change the render so CommentsSubscription
is always mounted inside CommentsList (render EmptyState visually when comments
is empty but still include <CommentsSubscription .../> alongside or above
EmptyState) and ensure CommentsSubscription receives the same props/handlers
(e.g., threadId, onNewComment) it expects so incoming comments are appended and
the UI updates without requiring a reload.
In
`@packages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/plugins/types.ts`:
- Around line 1-6: The props interface KeyboardCommandsPluginProps is using
React synthetic types (ClipboardEventHandler, KeyboardEventHandler) which
mismatch Lexical's native events; change onKeyDown and onPaste to accept native
DOM events (e.g., onKeyDown?: (e: KeyboardEvent) => void and onPaste?: (e:
ClipboardEvent) => void), remove the import of React event handler types, and
update any call sites or component props that assumed the HTMLTextAreaElement
generic so they now accept the native KeyboardEvent and ClipboardEvent
signatures.
In `@packages/design-system/components/web/inputs/MarkdownEditorField/styled.tsx`:
- Around line 28-30: Replace the hardcoded zIndex on
'.mdxeditor-popup-container' in the styled component (styled.tsx) with a
theme-based value: use theme.zIndex.modal + 1 so the popup renders above
Dialog/Modal overlays; update the rule inside the styled component where
'.mdxeditor-popup-container' is defined to reference theme.zIndex.modal + 1
instead of 1000.
In `@packages/design-system/components/web/inputs/SocialTextField/index.tsx`:
- Around line 46-60: renderField's rich-text branch drops shared field state and
masks the mismatch with a cast; update the branch that renders
MarkdownEditorField so it forwards value and error from props (and any other
shared field props) instead of omitting them, remove the unsafe cast to
SocialTextFieldProps['MarkdownEditorFieldProps'], and add an explicit adapter so
the MarkdownEditorField's change callback maps to the top-level onChange (e.g.,
call props.onChange(newValue) inside the adapter). Ensure you reference
renderField, MarkdownEditorField, SocialTextFieldProps and props while making
the change so callers keep the same onChange/value/error contract.
---
Nitpick comments:
In
`@packages/design-system/components/web/inputs/MarkdownEditorField/Toolbar/index.tsx`:
- Around line 91-94: The toolbar only wraps UndoRedo with
DiffSourceToggleWrapper so other edit-mode buttons remain active in source/diff
mode; expand the wrapper (or create a SourceToolbar child) to encompass all
edit-mode-only controls so DiffSourceToggleWrapper controls their visibility —
update the JSX around showDiffSourceToggle, DiffSourceToggleWrapper, and
UndoRedo (and other button components in this Toolbar) to move the wrapper
boundary to include the full set of buttons that should be hidden in source/diff
mode, or add a SourceToolbar slot and render that inside DiffSourceToggleWrapper
when showDiffSourceToggle is true.
In `@packages/design-system/components/web/inputs/MarkdownEditorField/types.ts`:
- Around line 37-45: MarkdownEditorProps declares onPaste but the public/core
prop type used by the field drops it, preventing consumers from wiring paste
handlers; update the core/public field prop types to include onPaste (e.g., add
onPaste?: ClipboardEventHandler<HTMLTextAreaElement> to
MarkdownEditorFieldCoreProps and the exported field prop type used by the
MarkdownEditorField component) so that when MarkdownEditorField forwards
...props to ForwardRefEditor the onPaste handler is preserved and available to
consumers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8160f103-7eb4-4f13-8dea-0f9e9ba63771
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
packages/components/CHANGELOG.mdpackages/components/modules/__shared__/web/ActionsOverlay/index.tsxpackages/components/modules/__shared__/web/SocialInput/index.tsxpackages/components/modules/__shared__/web/SocialInput/styled.tsxpackages/components/modules/__shared__/web/SocialInput/types.tspackages/components/modules/comments/web/CommentsList/EmptyState/index.tsxpackages/components/modules/comments/web/CommentsList/index.tsxpackages/components/modules/comments/web/CommentsList/types.tspackages/components/package.jsonpackages/design-system/CHANGELOG.mdpackages/design-system/components/web/inputs/MarkdownEditorField/ForwardRefEditor/index.tsxpackages/design-system/components/web/inputs/MarkdownEditorField/ForwardRefEditor/styled.tsxpackages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/index.tsxpackages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/plugins/keyboard-commands.tspackages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/plugins/types.tspackages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/types.tspackages/design-system/components/web/inputs/MarkdownEditorField/Toolbar/index.tsxpackages/design-system/components/web/inputs/MarkdownEditorField/Toolbar/types.tspackages/design-system/components/web/inputs/MarkdownEditorField/constants.tspackages/design-system/components/web/inputs/MarkdownEditorField/index.tsxpackages/design-system/components/web/inputs/MarkdownEditorField/styled.tsxpackages/design-system/components/web/inputs/MarkdownEditorField/types.tspackages/design-system/components/web/inputs/SocialTextField/index.tsxpackages/design-system/components/web/inputs/SocialTextField/styled.tsxpackages/design-system/components/web/inputs/SocialTextField/types.tspackages/design-system/components/web/inputs/index.tspackages/design-system/package.jsonpackages/design-system/styles/web/material/overrides/components/css-baseline.tsxpackages/wagtail/CHANGELOG.mdpackages/wagtail/package.jsonpnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (10)
- pnpm-workspace.yaml
- packages/wagtail/CHANGELOG.md
- packages/components/modules/comments/web/CommentsList/EmptyState/index.tsx
- packages/wagtail/package.json
- packages/design-system/components/web/inputs/MarkdownEditorField/ForwardRefEditor/styled.tsx
- packages/components/package.json
- packages/components/CHANGELOG.md
- packages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/types.ts
- packages/design-system/components/web/inputs/MarkdownEditorField/constants.ts
- packages/design-system/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/modules/shared/web/SocialInput/styled.tsx
- packages/components/modules/shared/web/SocialInput/types.ts
- packages/components/modules/shared/web/SocialInput/index.tsx
- packages/components/modules/shared/web/ActionsOverlay/index.tsx
- packages/design-system/styles/web/material/overrides/components/css-baseline.tsx
- packages/design-system/package.json
| if (comments.length === 0) { | ||
| return <EmptyState /> | ||
| } |
There was a problem hiding this comment.
Empty-state early return bypasses live subscriptions.
Because this return runs before Line 70, CommentsSubscription never mounts for empty threads, so real-time incoming comments may not appear until a manual refresh/reload.
Suggested fix
- if (comments.length === 0) {
- return <EmptyState />
- }
+ if (comments.length === 0) {
+ return (
+ <>
+ {subscriptionsEnabled && <CommentsSubscription targetObjectId={target.id} />}
+ <EmptyState />
+ </>
+ )
+ }📝 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 (comments.length === 0) { | |
| return <EmptyState /> | |
| } | |
| if (comments.length === 0) { | |
| return ( | |
| <> | |
| {subscriptionsEnabled && <CommentsSubscription targetObjectId={target.id} />} | |
| <EmptyState /> | |
| </> | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/components/modules/comments/web/CommentsList/index.tsx` around lines
29 - 31, The early return in CommentsList that returns EmptyState when
comments.length === 0 prevents CommentsSubscription from mounting, so live
updates for empty threads are missed; change the render so CommentsSubscription
is always mounted inside CommentsList (render EmptyState visually when comments
is empty but still include <CommentsSubscription .../> alongside or above
EmptyState) and ensure CommentsSubscription receives the same props/handlers
(e.g., threadId, onNewComment) it expects so incoming comments are appended and
the UI updates without requiring a reload.
| import { ClipboardEventHandler, KeyboardEventHandler } from 'react' | ||
|
|
||
| export interface KeyboardCommandsPluginProps { | ||
| onKeyDown?: KeyboardEventHandler<HTMLTextAreaElement> | ||
| onPaste?: ClipboardEventHandler<HTMLTextAreaElement> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @mdxeditor/editor/Lexical integrations, what payload types are passed to KEY_DOWN_COMMAND and PASTE_COMMAND handlers (native DOM events vs React SyntheticEvent)?
💡 Result:
In Lexical (and therefore in @mdxeditor/editor’s Lexical integrations), command handlers receive native DOM events, not React SyntheticEvents:
KEY_DOWN_COMMANDhandler payload:KeyboardEvent(native DOM). [1][2]PASTE_COMMANDhandler payload:ClipboardEvent | InputEvent | KeyboardEvent(all native DOM), depending on how the paste is produced (e.g., a realpasteclipboard event vs. anbeforeinput/input“insertFromPaste” path). [1][2]
Sources: Lexical command type definitions [1] and Lexical’s internal event dispatch (onKeyDown(event: KeyboardEvent, …) and dispatchCommand(editor, PASTE_COMMAND, event) from an InputEvent) [2].
[1] https://mintlify.wiki/facebook/lexical/api/lexical/commands
[2] https://raw.githubusercontent.com/facebook/lexical/refs/heads/main/packages/lexical/src/LexicalEvents.ts
🏁 Script executed:
fd "keyboard-commands" --type fRepository: silverlogic/baseapp-frontend
Length of output: 186
🏁 Script executed:
fd "types\.ts" packages/design-system/components/web/inputs/MarkdownEditorField --type fRepository: silverlogic/baseapp-frontend
Length of output: 425
🏁 Script executed:
cat -n packages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/plugins/keyboard-commands.ts 2>/dev/null || echo "File not found, searching..."Repository: silverlogic/baseapp-frontend
Length of output: 1533
🏁 Script executed:
find packages/design-system -name "keyboard-commands.ts" -o -name "types.ts" | head -20Repository: silverlogic/baseapp-frontend
Length of output: 1425
Use native DOM event types for keyboard/paste plugin props, not React synthetic event types.
The Lexical command pipeline (KEY_DOWN_COMMAND, PASTE_COMMAND) dispatches native DOM events, but this interface currently types them as React synthetic event handlers for a textarea element. This creates a type contract mismatch—consumers will receive native events with different APIs than React's synthetic event interface.
🔧 Proposed fixes
# packages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/plugins/types.ts
-import { ClipboardEventHandler, KeyboardEventHandler } from 'react'
-
export interface KeyboardCommandsPluginProps {
- onKeyDown?: KeyboardEventHandler<HTMLTextAreaElement>
- onPaste?: ClipboardEventHandler<HTMLTextAreaElement>
+ onKeyDown?: (event: globalThis.KeyboardEvent) => void
+ onPaste?: (event: globalThis.ClipboardEvent) => void
}# packages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/plugins/keyboard-commands.ts
-import type { ClipboardEvent, KeyboardEvent } from 'react'
+// Use native DOM events for Lexical command payloads(Then update lines 17 and 28 to omit the generic type parameter, relying on the native DOM event types.)
📝 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.
| import { ClipboardEventHandler, KeyboardEventHandler } from 'react' | |
| export interface KeyboardCommandsPluginProps { | |
| onKeyDown?: KeyboardEventHandler<HTMLTextAreaElement> | |
| onPaste?: ClipboardEventHandler<HTMLTextAreaElement> | |
| } | |
| export interface KeyboardCommandsPluginProps { | |
| onKeyDown?: (event: globalThis.KeyboardEvent) => void | |
| onPaste?: (event: globalThis.ClipboardEvent) => void | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/design-system/components/web/inputs/MarkdownEditorField/InitializedMDXEditor/plugins/types.ts`
around lines 1 - 6, The props interface KeyboardCommandsPluginProps is using
React synthetic types (ClipboardEventHandler, KeyboardEventHandler) which
mismatch Lexical's native events; change onKeyDown and onPaste to accept native
DOM events (e.g., onKeyDown?: (e: KeyboardEvent) => void and onPaste?: (e:
ClipboardEvent) => void), remove the import of React event handler types, and
update any call sites or component props that assumed the HTMLTextAreaElement
generic so they now accept the native KeyboardEvent and ClipboardEvent
signatures.
| '& .mdxeditor-popup-container': { | ||
| zIndex: 1000, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are Material UI's default z-index levels for dialogs/modals and popup-like overlays, and is z-index: 1000 below the modal stack?
💡 Result:
Material UI’s default theme.zIndex scale is: mobileStepper 1000, fab 1050, speedDial 1050, appBar 1100, drawer 1200, modal 1300, snackbar 1400, tooltip 1500. [1]
So Dialogs/Modals sit at z-index 1300 by default, and “popup-like” overlays commonly end up at 1300+ (e.g., snackbars 1400, tooltips 1500). [1]
Yes: z-index: 1000 is below the modal stack (1300). [1]
Sources:
[1] https://mui.com/material-ui/customization/z-index/
🏁 Script executed:
# First, examine the styled.tsx file to understand the context
cat -n packages/design-system/components/web/inputs/MarkdownEditorField/styled.tsxRepository: silverlogic/baseapp-frontend
Length of output: 4376
🏁 Script executed:
# Search for where MarkdownEditorField is used in the codebase
rg -r "MarkdownEditorField" --type tsx --type ts -A 3 | head -50Repository: silverlogic/baseapp-frontend
Length of output: 468
🏁 Script executed:
# Search for where MarkdownEditorField is used in the codebase
rg "MarkdownEditorField" --type tsx --type ts -A 3 | head -80Repository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
# Search for MarkdownEditorField usage
rg "MarkdownEditorField" -A 3 | head -80Repository: silverlogic/baseapp-frontend
Length of output: 5485
🏁 Script executed:
# Search for SocialTextField usage to see if it's used in modals/dialogs
rg "SocialTextField" -B 2 -A 2 | head -100Repository: silverlogic/baseapp-frontend
Length of output: 9595
🏁 Script executed:
# Search for other zIndex usages in the design system to understand conventions
rg "zIndex" packages/design-system/ -B 2 -A 2Repository: silverlogic/baseapp-frontend
Length of output: 3818
Use theme.zIndex for the popup container instead of hardcoding.
The .mdxeditor-popup-container at line 29 is pinned to zIndex: 1000, which equals MUI's mobileStepper level. If this component is ever used inside a Dialog or Modal (z-index 1300 or Drawer at 1200), the popup will be hidden behind the overlay. Since theme is already available in the styled component, use theme.zIndex.modal + 1 to ensure the popup always renders above modal contexts.
♻️ Suggested change
'& .mdxeditor-popup-container': {
- zIndex: 1000,
+ zIndex: theme.zIndex.modal + 1,
},📝 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.
| '& .mdxeditor-popup-container': { | |
| zIndex: 1000, | |
| }, | |
| '& .mdxeditor-popup-container': { | |
| zIndex: theme.zIndex.modal + 1, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/design-system/components/web/inputs/MarkdownEditorField/styled.tsx`
around lines 28 - 30, Replace the hardcoded zIndex on
'.mdxeditor-popup-container' in the styled component (styled.tsx) with a
theme-based value: use theme.zIndex.modal + 1 so the popup renders above
Dialog/Modal overlays; update the rule inside the styled component where
'.mdxeditor-popup-container' is defined to reference theme.zIndex.modal + 1
instead of 1000.
| const renderField = () => { | ||
| if (mode === 'rich-text') { | ||
| const { name, control, helperText, placeholder, onKeyDown } = props | ||
| return ( | ||
| <MarkdownEditorField | ||
| {...({ | ||
| name, | ||
| control, | ||
| helperText, | ||
| placeholder, | ||
| onKeyDown, | ||
| } as SocialTextFieldProps['MarkdownEditorFieldProps'])} | ||
| hasBorder={false} | ||
| {...MarkdownEditorFieldProps} | ||
| /> |
There was a problem hiding this comment.
Rich-text mode drops shared field state.
The rich-text branch only forwards name, control, helperText, placeholder, and onKeyDown. MarkdownEditorField also accepts value and error, so existing callers change behavior when mode === 'rich-text'; the cast on Lines 51-57 is also masking that mismatch from TypeScript. If the top-level onChange contract is meant to remain supported here, it needs an explicit adapter instead of being silently ignored.
🩹 Minimal fix for the dropped props
const renderField = () => {
if (mode === 'rich-text') {
- const { name, control, helperText, placeholder, onKeyDown } = props
+ const { name, control, helperText, placeholder, onKeyDown, value, error } = props
return (
<MarkdownEditorField
- {...({
- name,
- control,
- helperText,
- placeholder,
- onKeyDown,
- } as SocialTextFieldProps['MarkdownEditorFieldProps'])}
+ name={name}
+ control={control}
+ helperText={helperText}
+ placeholder={placeholder}
+ onKeyDown={onKeyDown}
+ value={typeof value === 'string' ? value : undefined}
+ error={error}
hasBorder={false}
{...MarkdownEditorFieldProps}
/>
)
}📝 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.
| const renderField = () => { | |
| if (mode === 'rich-text') { | |
| const { name, control, helperText, placeholder, onKeyDown } = props | |
| return ( | |
| <MarkdownEditorField | |
| {...({ | |
| name, | |
| control, | |
| helperText, | |
| placeholder, | |
| onKeyDown, | |
| } as SocialTextFieldProps['MarkdownEditorFieldProps'])} | |
| hasBorder={false} | |
| {...MarkdownEditorFieldProps} | |
| /> | |
| const renderField = () => { | |
| if (mode === 'rich-text') { | |
| const { name, control, helperText, placeholder, onKeyDown, value, error } = props | |
| return ( | |
| <MarkdownEditorField | |
| name={name} | |
| control={control} | |
| helperText={helperText} | |
| placeholder={placeholder} | |
| onKeyDown={onKeyDown} | |
| value={typeof value === 'string' ? value : undefined} | |
| error={error} | |
| hasBorder={false} | |
| {...MarkdownEditorFieldProps} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/design-system/components/web/inputs/SocialTextField/index.tsx`
around lines 46 - 60, renderField's rich-text branch drops shared field state
and masks the mismatch with a cast; update the branch that renders
MarkdownEditorField so it forwards value and error from props (and any other
shared field props) instead of omitting them, remove the unsafe cast to
SocialTextFieldProps['MarkdownEditorFieldProps'], and add an explicit adapter so
the MarkdownEditorField's change callback maps to the top-level onChange (e.g.,
call props.onChange(newValue) inside the adapter). Ensure you reference
renderField, MarkdownEditorField, SocialTextFieldProps and props while making
the change so callers keep the same onChange/value/error contract.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/design-system/styles/web/material/overrides/components/css-baseline.tsx (2)
51-175:⚠️ Potential issue | 🟠 MajorStill global: scope these Radix selectors to MDXEditor only.
Because this lives under
MuiCssBaseline, every selector in this block is app-wide. As written, any Radixrole="dialog"/role="tooltip"inside[data-radix-popper-content-wrapper]can inherit MDXEditor-specific form, button, and tooltip styling. Prefix the whole block with.mdxeditor-popup-containeror another MDXEditor-only hook before shipping.Scoped selector pattern
- '[data-radix-popper-content-wrapper] div[role="dialog"]': { + '.mdxeditor-popup-container [data-radix-popper-content-wrapper] div[role="dialog"]': { ... }, - '[data-radix-popper-content-wrapper] div[role="tooltip"]': { + '.mdxeditor-popup-container [data-radix-popper-content-wrapper] div[role="tooltip"]': { ... },Apply the same prefix consistently to the rest of this MDXEditor block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/design-system/styles/web/material/overrides/components/css-baseline.tsx` around lines 51 - 175, The Radix popper CSS under MuiCssBaseline (selectors like '[data-radix-popper-content-wrapper] div[role="dialog"]', '[data-radix-popper-content-wrapper] div[role="tooltip"]', and related input/button/link rules) is currently global; scope all these selectors to the MDX editor by prefixing the entire block with an MDX-only hook class (e.g. '.mdxeditor-popup-container') so the styles only apply when the popper is inside the MDX editor; apply the same prefix consistently to every selector in this block (dialog, form, input, button[type="submit"], button[type="reset"], > a, > button, tooltip) and keep the rest of the rules unchanged.
177-180:⚠️ Potential issue | 🟠 MajorStill too broad:
svg polygoncan hide unrelated icons.That selector hides every polygon inside the popper wrapper, not just the arrow. Restrict it to the actual arrow element, or drop it entirely if
svg.popoverArrowalready covers the arrow node.Narrowed selector example
- '[data-radix-popper-content-wrapper] div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper] div[role="dialog"] + span, [data-radix-popper-content-wrapper] svg polygon': + '[data-radix-popper-content-wrapper] div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper] div[role="dialog"] + span':🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/design-system/styles/web/material/overrides/components/css-baseline.tsx` around lines 177 - 180, The selector `[data-radix-popper-content-wrapper] div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper] div[role="dialog"] + span, [data-radix-popper-content-wrapper] svg polygon` is too broad and hides unrelated icons; update the rule in css-baseline.tsx to target only the actual popover arrow element (e.g., keep `svg.popoverArrow` or replace `svg polygon` with a more specific selector that matches the arrow SVG/element class or data attribute), or remove the `svg polygon` part entirely so only the intended arrow node is hidden (refer to the existing `svg.popoverArrow` token and the `display: 'none'` declaration to locate the code to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/design-system/styles/web/material/overrides/components/css-baseline.tsx`:
- Around line 51-175: The Radix popper CSS under MuiCssBaseline (selectors like
'[data-radix-popper-content-wrapper] div[role="dialog"]',
'[data-radix-popper-content-wrapper] div[role="tooltip"]', and related
input/button/link rules) is currently global; scope all these selectors to the
MDX editor by prefixing the entire block with an MDX-only hook class (e.g.
'.mdxeditor-popup-container') so the styles only apply when the popper is inside
the MDX editor; apply the same prefix consistently to every selector in this
block (dialog, form, input, button[type="submit"], button[type="reset"], > a, >
button, tooltip) and keep the rest of the rules unchanged.
- Around line 177-180: The selector `[data-radix-popper-content-wrapper]
div[role="dialog"] svg.popoverArrow, [data-radix-popper-content-wrapper]
div[role="dialog"] + span, [data-radix-popper-content-wrapper] svg polygon` is
too broad and hides unrelated icons; update the rule in css-baseline.tsx to
target only the actual popover arrow element (e.g., keep `svg.popoverArrow` or
replace `svg polygon` with a more specific selector that matches the arrow
SVG/element class or data attribute), or remove the `svg polygon` part entirely
so only the intended arrow node is hidden (refer to the existing
`svg.popoverArrow` token and the `display: 'none'` declaration to locate the
code to change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a4758e9-3dfe-4a73-b2a8-a87ecc87d1d3
📒 Files selected for processing (2)
packages/design-system/components/web/inputs/MarkdownEditorField/styled.tsxpackages/design-system/styles/web/material/overrides/components/css-baseline.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/design-system/components/web/inputs/MarkdownEditorField/styled.tsx
anicioalexandre
force-pushed
the
BA-2167-FE-markdown-editor
branch
from
ab20a7b to
faadd1e
Compare
anicioalexandre
added
approved
anicioalexandre
force-pushed
the
BA-2167-FE-markdown-editor
branch
from
faadd1e to
0bf686d
Compare
|
5 participants
__package_name__package update -v __package_version__Summary by CodeRabbit