Skip to content

Conversation

@bpowers
Copy link
Owner

@bpowers bpowers commented Jan 30, 2026

Summary

This completes the migration of src/app away from Material UI to our own component library in @system-dynamics/diagram.

Changes

  • 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

Test plan

  • yarn build - Full build succeeds
  • yarn tsc - No type errors
  • yarn lint - No lint errors
  • cargo test - All Rust tests pass
  • yarn test in diagram - All 325 tests pass
  • Manual testing of Login, Home, NewProject, NewUser pages

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.
@bpowers
Copy link
Owner Author

bpowers commented Jan 30, 2026

@codex review

@claude
Copy link

claude bot commented Jan 30, 2026

Code Review: PR #220 - Remove MUI dependency, use diagram components

Summary

This 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.

✅ Strengths

Architecture & Design

  • Excellent use of Radix UI: Leveraging accessible, headless primitives is a solid architectural choice that provides WCAG-compliant foundations
  • API compatibility: Components follow MUI patterns (props like variant, color, fullWidth, etc.), making migration straightforward
  • Class components: Consistent with the codebase's preference for class components over hooks
  • Proper TypeScript types: All components are well-typed with clear interfaces
  • CSS Modules: Good isolation with scoped styles

Code Quality

  • Clean separation: New components properly separated from app code
  • Test coverage: TextField has good test coverage (109 lines of tests) including edge cases for event handler chaining
  • No debug artifacts: Clean code with no TODO/FIXME comments or console.log statements
  • Accessibility: Good use of ARIA attributes (aria-label, aria-modal, aria-haspopup, etc.)
  • Build script fix: Updated build:copy-css to handle both *.module.css and reset.css (line changed from *.module.css to *.css)

Specific Implementations

  • TextField (206 lines): Handles both outlined and standard variants with proper focus state management and event handler chaining
  • Dialog: Properly wraps Radix Dialog with overlay, portal, and escape key handling
  • Button: Extended with outlined variant, inherit color, type, component props, and ARIA attributes
  • Menu: Clever positioning using fixed positioning based on anchorEl's bounding rect
  • Accordion: Clean wrapper around Radix with controlled/uncontrolled mode support

⚠️ Issues & Concerns

1. Menu Component - Positioning Fragility (Menu.tsx:28-37)

The Menu component uses a hidden trigger span with fixed positioning based on anchorEl.getBoundingClientRect():

<span
  style={{
    position: 'fixed',
    top: anchorEl?.getBoundingClientRect().bottom ?? 0,
    left: anchorEl?.getBoundingClientRect().left ?? 0,
    width: anchorEl?.getBoundingClientRect().width ?? 0,
    height: 0,
    pointerEvents: 'none',
  }}
/>

Issues:

  • Calls getBoundingClientRect() three times on every render - should memoize
  • No handling for scroll events or window resize - menu won't reposition if the anchorEl moves
  • transformOrigin prop is accepted but never used

Recommendation: Consider using Radix's built-in positioning or add event listeners for scroll/resize.

2. TextField - Input Props Spreading Gotcha (TextField.tsx:93)

The component extracts onFocus/onBlur from inputProps to chain them, then spreads the rest:

const { onFocus: _onFocus, onBlur: _onBlur, ...restInputProps } = inputProps || {};
// ...
<input {...restInputProps} {...rest} />

Issue: The ...rest spread after restInputProps could override inputProps unintentionally. The order should be:

<input {...rest} {...restInputProps} />

This ensures inputProps have final say over base props.

3. Missing Accessibility - Avatar Component (Avatar.tsx:19)

The Avatar always provides an empty alt text when src is present:

{src ? <img src={src} alt={alt || ''} className={styles.image} /> : children}

Issue: If alt is undefined for a decorative image, it should be alt="". But if the avatar is meaningful (user photo), missing alt text creates accessibility issues.

Recommendation: Either require alt when src is provided, or document that avatars are decorative.

4. Dialog - Missing aria-labelledby Connection (Dialog.tsx:11, 58)

The Dialog accepts aria-labelledby prop but never passes it to the RadixDialog.Content:

// Prop exists but is unused
'aria-labelledby'?: string;

Fix needed:

<RadixDialog.Content
  aria-labelledby={props['aria-labelledby']}
  // ...
/>

This breaks accessibility for screen readers.

5. Button - Component Prop Inconsistency (Button.tsx:70-77)

When component="label", the button doesn't receive type, onClick, or aria-owns/aria-haspopup:

if (component === 'label') {
  return (
    <label className={buttonClassName} style={style} aria-label={ariaLabel}>
      {startIcon && <span className={styles.startIcon}>{startIcon}</span>}
      {children}
    </label>
  );
}

Issue: Label elements shouldn't have onClick handlers or button ARIA attributes, but this is inconsistent - a label can't actually be "disabled" either, yet the component applies disabled styles.

Recommendation: Either remove disabled styling for labels or document this edge case.

6. reset.css - Potentially Too Aggressive (reset.css:39-41)

html:focus-within {
  scroll-behavior: smooth;
}

Concern: Smooth scrolling can cause accessibility issues for users with vestibular disorders. Consider using @media (prefers-reduced-motion: no-preference).

7. Missing Tests for New Components

While TextField has good test coverage, the following new components lack tests:

  • Avatar
  • Paper
  • TextLink
  • InputAdornment
  • AppBar/Toolbar
  • ImageList
  • Card/CardContent/CardActions
  • Checkbox
  • FormControlLabel
  • Menu/MenuItem
  • Accordion
  • Dialog

Recommendation: Add basic smoke tests at minimum.

🔍 Minor Issues

8. Checkbox Color Prop (Checkbox.tsx:14)

Accepts color?: 'primary' | 'secondary' but this is unusual for checkboxes in most design systems. Consider if this is actually needed.

9. Typography Module (typography.module.css)

Hard-coded font sizes (3.75rem, 1.5rem, etc.) without CSS custom properties. Consider using CSS variables for consistency and easier theming.

10. Package.json - Missing Peer Dependencies

diagram/package.json adds Radix dependencies as regular dependencies, but these should likely be marked as peer dependencies if diagram is intended to be a shared component library.

🎯 Performance Considerations

Positive:

  • Removing @emotion, @mui reduces bundle size significantly
  • CSS Modules are performant
  • Radix is lightweight compared to MUI

Watch out for:

  • Menu component's multiple getBoundingClientRect() calls
  • No memoization in functional components (though you prefer class components anyway)

🔒 Security

No security concerns identified. No dangerous patterns like dangerouslySetInnerHTML or direct DOM manipulation.

✅ Test Plan Status

From PR description:

  • yarn build succeeds
  • yarn tsc passes
  • yarn lint passes
  • cargo test passes
  • ✅ 325 diagram tests pass
  • ❌ Manual testing incomplete

Recommendation: Complete manual testing before merge, especially for Login, Home, NewProject pages.

📝 Recommendations Summary

Must Fix Before Merge:

  1. Dialog: Pass aria-labelledby to RadixDialog.Content
  2. TextField: Swap spread order ({...rest} {...restInputProps})

Should Fix:
3. Menu: Memoize getBoundingClientRect() calls
4. Avatar: Clarify alt text handling
5. reset.css: Add prefers-reduced-motion check
6. Complete manual testing

Nice to Have:
7. Add tests for new components
8. Menu: Handle scroll/resize for positioning
9. Menu: Use or remove transformOrigin prop
10. Consider CSS custom properties for typography

Overall Assessment

This is a well-executed refactoring that significantly improves the codebase by removing a heavy dependency. The code quality is high, TypeScript usage is solid, and the components follow consistent patterns. The main concerns are around accessibility (Dialog, Avatar), the Menu positioning approach, and missing test coverage for new components.

With the critical fixes applied, this PR is ready to merge. Great work! 🎉

- 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
@bpowers
Copy link
Owner Author

bpowers commented Jan 30, 2026

@codex review

@claude
Copy link

claude bot commented Jan 30, 2026

PR Review: Remove MUI dependency, use diagram components

This 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

  1. Excellent Component Parity: The new components closely match MUI's API patterns, making migration straightforward and reducing the risk of behavioral changes.

  2. Accessibility Improvements: Great use of Radix UI primitives for Dialog, Accordion, Checkbox, and Menu. The aria-labelledby prop passing in Dialog.tsx:35 is correct.

  3. Proper Responsive to Code Review: The latest commit (e23ff06) shows good responsiveness to feedback with accessibility improvements and performance optimizations.

  4. Clean CSS Reset: The reset.css respects user preferences with prefers-reduced-motion check (lines 38-42).

  5. Type Safety: All new components have proper TypeScript interfaces with good type coverage.

🔍 Issues Found

1. 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 () => void to (checked: boolean) => void. This is a breaking change that could cause confusion.

Current code:

<Checkbox checked={this.state.isPublic} onChange={this.handlePublicChecked} />
Publicly accessible

Issue: The label is outside the Checkbox, which breaks the click-to-toggle UX pattern. The entire label area should be clickable.

Recommendation: Either:

  • Use FormControlLabel consistently: <FormControlLabel control={<Checkbox checked={...} onChange={...} />} label="Publicly accessible" />
  • Or document that Checkbox's onChange passes the new checked state directly

2. Performance: Menu Component Recalculates Rect on Every Render (Menu.tsx:25)

const anchorRect = anchorEl?.getBoundingClientRect();

This calls getBoundingClientRect() on every render, even when anchorEl hasn't changed. The latest commit mentions memoization, but this is still happening in the component body.

Fix:

const anchorRect = React.useMemo(
  () => anchorEl?.getBoundingClientRect(),
  [anchorEl]
);

3. TextField Spread Order Could Cause Issues (TextField.tsx:148, 198)

{...rest}
{...restInputProps}

The ...rest spread contains unknown props from TextFieldProps, while restInputProps comes from explicitly typed inputProps. If there's overlap, restInputProps will override, but this might not be the intended behavior.

Recommendation: Document this spread order or remove ...rest if not needed (preferred since TextField should have explicit props).

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:

  • Are h1, h3, h4 intentionally omitted?
  • Should there be body1 in addition to body2?

Consider adding complete typography scale or documenting why only specific variants are needed.

5. Potential Bundle Size Issue: Radix Dependencies

The PR adds 4 new Radix packages (@radix-ui/react-accordion, checkbox, dialog, dropdown-menu). While Radix is generally lightweight, it would be good to:

  • Verify tree-shaking is working properly
  • Check bundle size impact (the PR mentions "reduces bundle size" but no metrics provided)

🎯 Best Practices & Suggestions

  1. Component Documentation: Consider adding JSDoc comments to exported components explaining key differences from MUI (especially where API changed, like Checkbox onChange).

  2. Test Coverage: The PR shows test updates but the test plan says "Manual testing" is incomplete (unchecked). Given this is a large UI migration, automated visual regression tests would be valuable.

  3. Migration Guide: Since this changes component APIs (especially Checkbox), a migration guide or changelog entry would help future developers understand the changes.

  4. CSS Consistency: Some components use inline styles (style prop) while others use CSS modules. The mix of both is fine, but ensure there's a clear pattern (e.g., always prefer CSS modules for component-internal styles, style prop for user overrides).

🔒 Security Considerations

No security concerns identified. The components properly escape user input and don't introduce XSS vulnerabilities.

Performance Considerations

  1. The Menu component's getBoundingClientRect() issue mentioned above
  2. Consider using React.memo for simpler components like Avatar, Paper, TextLink that don't have complex state
  3. The Dialog overlay animation is well-optimized with CSS animations instead of JS

📦 Bundle Size

The 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

  • Formatting: ✅ Consistent and clean
  • Naming: ✅ Clear and follows conventions
  • Structure: ✅ Well-organized component structure
  • TypeScript: ✅ Good type coverage, minimal use of any

🎬 Recommendation

Approve with minor changes. The issues found are relatively minor and can be addressed in follow-up commits or a quick revision:

  1. Fix the Menu memoization issue (performance)
  2. Document or fix the Checkbox usage pattern (usability)
  3. Consider the TextField spread order (potential bug)

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
@bpowers
Copy link
Owner Author

bpowers commented Jan 30, 2026

@codex review

@claude
Copy link

claude bot commented Jan 30, 2026

Pull Request Review: MUI Removal & Custom Component Library Migration

This 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

  1. Excellent architecture choice: Using Radix UI primitives provides accessible, unstyled primitives that you can customize completely while maintaining WCAG compliance
  2. Comprehensive component coverage: All MUI components used in the app have proper replacements
  3. Consistent API surface: New components maintain similar props to MUI, making migration straightforward
  4. Good test coverage: The Drawer component has excellent test coverage (drawer.test.tsx) including accessibility features like focus management
  5. Clean CSS architecture: Using CSS modules for styling instead of CSS-in-JS removes runtime overhead
  6. Proper accessibility: Components use semantic HTML and ARIA attributes correctly

Code Quality Issues

1. Type Safety - Missing or Overly Permissive Types

Menu.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 as unknown as.

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 Issues

Menu.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:

  • useMemo doesn't track window scroll/resize, so menu position won't update
  • Invisible span as trigger is a workaround - Radix expects a proper trigger element
  • Fallback to 0 could cause menus to appear in wrong location

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 Cases

Dialog.tsx: No handling for disableEscapeKeyDown when it's undefined vs false

onEscapeKeyDown={(event) => {
  if (disableEscapeKeyDown) {
    event.preventDefault();
  }
}}

This only prevents escape if explicitly true. If prop is omitted (undefined), escape still works. This is fine if intentional but should be documented.

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 onClick prop is ignored when component="label". This could be confusing.

Performance Considerations

  1. Autocomplete.tsx (Lines 72-79): Position recalculated on every open
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.

  1. TextField counter: Global counter textFieldIdCounter will only increment, never reset. In a long-running SPA, this could theoretically overflow (though extremely unlikely).

Testing Gaps

While the Drawer has excellent tests, I don't see tests for the new components:

  • Accordion
  • Checkbox
  • Dialog
  • Menu
  • Card
  • TextField with new variant support

Recommendation: Add basic smoke tests for new components to ensure they render without errors and handle basic interactions.

Security Concerns

No security issues identified. The components properly escape content and don't introduce XSS vectors.

Documentation & Maintainability

  1. Missing JSDoc: Complex components like TextField and Menu would benefit from JSDoc comments explaining prop behavior
  2. No migration guide: Consider adding a comment or doc noting the MUI→custom component mapping for future developers
  3. reset.css: Good lightweight alternative to CssBaseline, but consider noting which MUI CssBaseline features were intentionally omitted

Minor Issues

  1. Button.tsx: The color='inherit' option is added but only used in text and outlined variants, not contained. Is this intentional?

  2. typography.module.css: New file added but would benefit from seeing its usage patterns to ensure it matches MUI Typography behavior

  3. Autocomplete wrapper div: Has ref={wrapperRef} but it's only used for positioning. This is fine but could use a comment explaining why.

Test Plan Status

According to the PR description:

  • ✅ Build succeeds
  • ✅ No type errors
  • ✅ No lint errors
  • ✅ Rust tests pass
  • ✅ 325 diagram tests pass
  • ❌ Manual testing not complete

Recommendation: Complete manual testing of Login, Home, NewProject, and NewUser pages before merging, particularly:

  • Menu positioning and interaction
  • Form validation error messages (especially given the bugs I found in Login.tsx)
  • Responsive behavior of new components

Verdict

This 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:

  1. Fix the error message bugs in Login.tsx (lines 166-174) - this is a user-facing bug
  2. Review the Menu positioning implementation for robustness
  3. Complete manual testing as noted in the test plan
  4. Consider adding basic tests for the new components

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.
@bpowers
Copy link
Owner Author

bpowers commented Jan 30, 2026

@codex review

@claude
Copy link

claude bot commented Jan 30, 2026

Pull Request Review - PR #220

Summary

This 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:

  1. Excellent use of Radix UI primitives - The components leverage accessible, well-tested primitives from Radix UI (@radix-ui/react-dialog, @radix-ui/react-dropdown-menu, @radix-ui/react-checkbox, @radix-ui/react-accordion) which provide robust accessibility out of the box.

  2. Clean component APIs - The new components maintain MUI-compatible APIs where appropriate (e.g., TextField, Button), making migration straightforward and reducing breaking changes.

  3. Proper accessibility - Good use of ARIA attributes:

    • aria-labelledby passed through in Dialog component (Dialog.tsx:35)
    • aria-label support in buttons, speed dial actions
    • Proper role attributes (dialog, menu, menuitem, presentation)
    • Focus management in Drawer component with focus trapping
  4. Class-based components - Follows the project's preference for class components over hooks, maintaining consistency with CLAUDE.md guidance.

  5. Good separation of concerns - CSS modules for styling, TypeScript for type safety, Radix for behavior/accessibility primitives.

  6. Responsive to review feedback - Commits show iterative improvements based on code review (e23ff06, 7bf0e29).

Minor Issues:

  1. TextField inputProps spread order (src/diagram/components/TextField.tsx:148-149):

    • Fixed in commit e23ff06 - Now correctly spreads {...rest} then {...restInputProps} so inputProps can override.
  2. Menu getBoundingClientRect memoization (src/diagram/components/Menu.tsx:25):

    • Fixed in commit e23ff06 - Now uses React.useMemo to avoid recalculating on every render.
  3. Smooth scrolling accessibility (src/diagram/reset.css:38-43):

    • Fixed in commit e23ff06 - Only enables smooth scrolling when user hasn't requested reduced motion via prefers-reduced-motion media query.

Potential Bugs or Issues ⚠️

1. Missing helperText support in TextField

The TextField component is used with helperText prop in Login.tsx (e.g., lines 242, 277, 289, 327, 336, 349, 414), but the TextField component definition doesn't include helperText in its props interface. This will either:

  • Cause TypeScript errors (if strict mode catches it)
  • Silently fail to render helper text

Recommendation: Add helperText prop to TextField and render it below the input.

2. FormControlLabel lacks proper click handling

In FormControlLabel.tsx, the label wraps the control but doesn't explicitly handle click events. While the native <label> element should work, there's no guaranteed connection between the label and the checkbox since the checkbox is passed as control prop without an explicit id linkage.

Current:

<label className={clsx(styles.formControlLabel, className)} style={style}>
  {control}
  <span className={styles.label}>{label}</span>
</label>

Recommendation: This should work fine for Radix Checkbox, but verify that clicking the label text actually toggles the checkbox in manual testing.

3. TextField autoFocus and onKeyPress props

Login.tsx uses props like autoFocus (line 124, 244, 291, 338, 418) and onKeyPress (line 130) which appear to be missing from the TextField interface in the diff. Verify these are properly supported.

Performance Considerations ✅

  1. Reduced bundle size - Removing MUI, Emotion, and their dependencies will significantly reduce the JavaScript bundle size. This is a major win for initial page load performance.

  2. Memoization - Good use of React.useMemo in Menu component to avoid unnecessary getBoundingClientRect calls.

  3. PureComponent usage - Proper use of React.PureComponent for components with simple prop/state comparisons.

  4. No unnecessary re-renders - The Autocomplete component properly memoizes filtered options (Autocomplete.tsx:56-60).

Security Concerns ✅

No security issues identified. The PR:

  • Uses well-maintained, security-audited Radix UI primitives
  • Doesn't introduce any new XSS vectors
  • Properly handles user input in forms
  • No eval() or dangerous HTML rendering

Test Coverage ⚠️

Test Changes:

  • drawer.test.tsx: +39/-14 lines (expanded tests)
  • snackbar.test.tsx: +2/-9 lines (simplified)
  • speed-dial.test.tsx: +23/-66 lines (simplified)
  • text-field.test.tsx: +3/-19 lines (simplified)

Concerns:

  1. Missing component tests - No test files for new components:

    • Dialog.tsx
    • Menu.tsx
    • Checkbox.tsx
    • FormControlLabel.tsx
    • Card.tsx
    • Accordion.tsx
    • Avatar.tsx
    • AppBar.tsx
    • ImageList.tsx
    • InputAdornment.tsx
    • Paper.tsx
    • TextLink.tsx
    • Toolbar.tsx
  2. Manual testing incomplete - The PR description shows:

    • ✅ yarn build
    • ✅ yarn tsc
    • ✅ yarn lint
    • ✅ cargo test
    • ✅ yarn test in diagram (325 tests pass)
    • ❌ Manual testing of Login, Home, NewProject, NewUser pages NOT CHECKED

Recommendation: Before merging, manually test all UI flows, especially:

  • Login/signup with email
  • Login with Google/Apple
  • Password recovery
  • NewUser dialog
  • NewProject form with checkbox
  • Home page navigation

Additional Observations

Good Practices:

  1. Commit message style - Follows the project's commit message conventions (component: description format)
  2. No backwards compatibility hacks - Clean migration without fallback code, as recommended in CLAUDE.md
  3. Typography utilities - Created typography.module.css for consistent text styling
  4. Reset CSS - Lightweight alternative to MUI's CssBaseline

Dependencies:

Removed from app/package.json:

  • @emotion/react
  • @emotion/styled
  • @mui/material
  • @mui/icons-material

Added to diagram/package.json:

  • @radix-ui/react-accordion: ^1.2.0
  • @radix-ui/react-checkbox: ^1.1.0
  • @radix-ui/react-dialog: ^1.1.0
  • @radix-ui/react-dropdown-menu: ^2.1.0
  • @radix-ui/react-tabs: ^1.1.13

This is a net reduction in dependency weight and complexity.

Recommendations

Before Merging:

  1. ✅ Add helperText prop support to TextField component
  2. ✅ Verify autoFocus and onKeyPress props work on TextField
  3. Complete manual testing of Login, Home, NewProject, and NewUser pages
  4. ⚠️ Consider adding basic smoke tests for new components (at least Dialog, Menu, Checkbox)

Post-Merge (Future Work):

  1. Add comprehensive test coverage for new components
  2. Consider extracting the component library to a separate package if it will be reused
  3. Document component APIs if they deviate from MUI patterns

Overall Assessment

LGTM with minor concerns

This is high-quality work that achieves its goal of removing MUI dependency while maintaining functionality and improving architecture. The code follows project conventions, uses appropriate abstractions, and demonstrates good engineering practices.

The main concern is the incomplete manual testing checklist and missing helperText support in TextField. These should be addressed before merge to avoid runtime issues in the login/signup flows.

Recommendation: Approve with requested changes - Complete manual testing and verify TextField props before merging.

@bpowers
Copy link
Owner Author

bpowers commented Jan 30, 2026

@codex review - helperText, autoFocus, and onKeyPress props were already added to TextField. Please confirm the implementation is correct.

@codecov
Copy link

codecov bot commented Jan 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.82%. Comparing base (eff51dc) to head (d110273).
⚠️ Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. More of your lovely PRs please.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

@bpowers bpowers merged commit 76a79da into main Jan 30, 2026
11 checks passed
@bpowers bpowers deleted the bobby/mui-removal-2 branch January 30, 2026 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants