-
Notifications
You must be signed in to change notification settings - Fork 17
app: remove MUI dependency, use diagram components #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This completes the migration of src/app away from Material UI to our own component library in @system-dynamics/diagram. The change: - Adds Radix UI primitives (@radix-ui/react-accordion, checkbox, dialog, dropdown-menu) to diagram package for accessible interactive components - Creates replacement components: Avatar, Paper, TextLink, InputAdornment, AppBar, Toolbar, ImageList, Card/CardContent/CardActions, Checkbox, FormControlLabel, Menu/MenuItem, Accordion, Dialog - Extends existing TextField and Button components with missing props - Adds new icons: AccountCircle, ExpandMore, Apple, Email, Check - Creates reset.css as a lightweight CssBaseline replacement - Creates typography.module.css for heading/text utility classes - Removes @emotion/react, @emotion/styled, @mui/material, @mui/icons-material from app dependencies The components follow MUI's API patterns for easier migration while being implemented with simpler native HTML/CSS or Radix primitives. This reduces bundle size and removes the heavy MUI dependency from the app.
|
@codex review |
Code Review: PR #220 - Remove MUI dependency, use diagram componentsSummaryThis PR successfully completes the migration from Material UI to a custom component library built on Radix UI primitives. The changes reduce bundle size and dependencies while maintaining API compatibility for easier migration. ✅ StrengthsArchitecture & Design
Code Quality
Specific Implementations
|
- Dialog: Pass aria-labelledby prop to RadixDialog.Content for screen reader accessibility - TextField: Fix spread order so inputProps overrides base props correctly - Menu: Memoize getBoundingClientRect() call to avoid 3x per render - reset.css: Only enable smooth scrolling when user hasn't requested reduced motion via prefers-reduced-motion media query
|
@codex review |
PR Review: Remove MUI dependency, use diagram componentsThis is a significant refactoring that successfully removes Material UI from the app package and consolidates components in the diagram package. Overall, this is high-quality work with good attention to detail. Here are my findings: ✅ Strengths
🔍 Issues Found1. Critical: Checkbox API Inconsistency (NewProject.tsx:241, NewUser.tsx:46)The Checkbox component is being used directly without FormControlLabel, but the event handler signature changed from Current code: <Checkbox checked={this.state.isPublic} onChange={this.handlePublicChecked} />
Publicly accessibleIssue: The label is outside the Checkbox, which breaks the click-to-toggle UX pattern. The entire label area should be clickable. Recommendation: Either:
2. Performance: Menu Component Recalculates Rect on Every Render (Menu.tsx:25)const anchorRect = anchorEl?.getBoundingClientRect();This calls Fix: const anchorRect = React.useMemo(
() => anchorEl?.getBoundingClientRect(),
[anchorEl]
);3. TextField Spread Order Could Cause Issues (TextField.tsx:148, 198){...rest}
{...restInputProps}The Recommendation: Document this spread order or remove 4. Missing Typography Utilities (typography.module.css)The typography module only exports specific heading levels (2, 5, 6) and subtitle variants. This seems incomplete. Questions:
Consider adding complete typography scale or documenting why only specific variants are needed. 5. Potential Bundle Size Issue: Radix DependenciesThe PR adds 4 new Radix packages (@radix-ui/react-accordion, checkbox, dialog, dropdown-menu). While Radix is generally lightweight, it would be good to:
🎯 Best Practices & Suggestions
🔒 Security ConsiderationsNo security concerns identified. The components properly escape user input and don't introduce XSS vulnerabilities. ⚡ Performance Considerations
📦 Bundle SizeThe PR removes heavy dependencies (@emotion, @mui/material, @mui/icons-material) and adds lighter Radix primitives. This should reduce bundle size, but metrics would be helpful to verify the claimed improvement. ✏️ Code Quality
🎬 RecommendationApprove with minor changes. The issues found are relatively minor and can be addressed in follow-up commits or a quick revision:
The core refactoring is solid and achieves the goal of removing MUI dependency. Great work on maintaining API compatibility while simplifying the implementation! |
- Menu: Use React.useMemo to memoize getBoundingClientRect() call so it's not recalculated on every render when anchorEl hasn't changed - NewProject: Wrap Checkbox with FormControlLabel so the entire label area is clickable, improving UX consistency
|
@codex review |
Pull Request Review: MUI Removal & Custom Component Library MigrationThis is a well-executed migration that successfully removes the Material-UI dependency and replaces it with custom components built on Radix UI primitives. The overall approach is sound and demonstrates good architectural judgment. Here's my detailed feedback: Strengths
Code Quality Issues1. Type Safety - Missing or Overly Permissive TypesMenu.tsx (Line 73-76): Type casting could mask issues onSelect={(event) => {
if (onClick) {
onClick(event as unknown as React.MouseEvent); // Risky double cast
}
}}Recommendation: Consider using a proper event adapter or constraining the Radix event type rather than Login.tsx (Lines 166-174): Error messages don't match validation const fullName = this.state.fullName.trim();
if (!fullName) {
this.setState({ emailError: 'Enter your email address to continue' }); // Should be fullNameError
return;
}
const password = this.state.password.trim();
if (!password) {
this.setState({ passwordError: 'Enter your email address to continue' }); // Wrong message
return;
}Recommendation: Fix error messages to match what's actually being validated. 2. Component Implementation IssuesMenu.tsx (Lines 25-39): Positioning approach is fragile const anchorRect = React.useMemo(() => anchorEl?.getBoundingClientRect(), [anchorEl]);
// Later...
<span
style={{
position: 'fixed',
top: anchorRect?.bottom ?? 0,
left: anchorRect?.left ?? 0,
width: anchorRect?.width ?? 0,
height: 0,
pointerEvents: 'none',
}}
/>Issues:
Recommendation: Either use a ResizeObserver/scroll listener to update position, or refactor to use Radix's positioning more idiomatically. TextField.tsx (Lines 88-93): Potential controlled/uncontrolled component issue const inputId = id || generatedId;
const hasValue = value !== undefined && value !== null && value !== '';
const shouldShrink = isFocused || hasValue;
const { onFocus: _onFocus, onBlur: _onBlur, ...restInputProps } = inputProps || {};The component doesn't distinguish between controlled (value prop provided) and uncontrolled (no value prop) modes explicitly. MUI's TextField handles this more carefully. Recommendation: Consider tracking whether the component is controlled in constructor and warning on mode changes (like React does). 3. Missing Edge CasesDialog.tsx: No handling for onEscapeKeyDown={(event) => {
if (disableEscapeKeyDown) {
event.preventDefault();
}
}}This only prevents escape if explicitly Button.tsx (Lines 70-77): Label component doesn't handle onClick if (component === 'label') {
return (
<label className={buttonClassName} style={style} aria-label={ariaLabel}>
{startIcon && <span className={styles.startIcon}>{startIcon}</span>}
{children}
</label>
);
}The Performance Considerations
React.useEffect(() => {
if (isOpen && wrapperRef.current) {
const rect = wrapperRef.current.getBoundingClientRect();
setDropdownPosition({...});
}
}, [isOpen]);This is fine, but could benefit from a scroll listener to update position if the page scrolls while dropdown is open.
Testing GapsWhile the Drawer has excellent tests, I don't see tests for the new components:
Recommendation: Add basic smoke tests for new components to ensure they render without errors and handle basic interactions. Security ConcernsNo security issues identified. The components properly escape content and don't introduce XSS vectors. Documentation & Maintainability
Minor Issues
Test Plan StatusAccording to the PR description:
Recommendation: Complete manual testing of Login, Home, NewProject, and NewUser pages before merging, particularly:
VerdictThis is solid work that successfully achieves the goal of removing MUI. The new component library is well-structured and maintainable. The issues I've identified are mostly minor fixes and polish items rather than fundamental problems. Recommended actions before merge:
The bundle size reduction and removal of @emotion/react + @emotion/styled dependencies are significant wins for performance. Great work! Let me know if you'd like me to elaborate on any of these points or if you'd like help addressing any of the issues. |
The fullName and password validation errors were using the wrong error field and message text. Fixed to use the correct error state fields (fullNameError, passwordError) and appropriate messages.
|
@codex review |
Pull Request Review - PR #220SummaryThis PR successfully removes Material UI (MUI) as a dependency from the app, replacing it with a custom component library built on Radix UI primitives. This is an excellent architectural improvement that reduces bundle size and removes the heavy MUI/Emotion dependency chain. Code Quality & Best Practices ✅Strengths:
Minor Issues:
Potential Bugs or Issues
|
|
@codex review - helperText, autoFocus, and onKeyPress props were already added to TextField. Please confirm the implementation is correct. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
==========================================
+ Coverage 76.79% 76.82% +0.02%
==========================================
Files 69 69
Lines 20523 20523
==========================================
+ Hits 15761 15767 +6
+ Misses 4762 4756 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
This completes the migration of src/app away from Material UI to our own component library in @system-dynamics/diagram.
Changes
Test plan
yarn build- Full build succeedsyarn tsc- No type errorsyarn lint- No lint errorscargo test- All Rust tests passyarn testin diagram - All 325 tests pass