diagram: remove MUI and Emotion dependencies #217
Conversation
Replace all MUI components in the diagram package with custom implementations: SvgIcon, Button, IconButton, TextField, Snackbar, Tabs (Radix), SpeedDial, Drawer, and Autocomplete (Downshift). MUI's Card, Paper, CardContent, CardActions, and Typography are replaced with plain divs with CSS module classes that replicate MUI's elevation shadows, padding, and layout. The global MuiAutocomplete CSS selector is removed in favor of a scoped CSS module class in the new Autocomplete component. The app package gains explicit MUI dependencies (previously inherited via hoisting from diagram), and the website drops its MUI/Emotion/ThemeProvider usage entirely.
Code Review: Remove MUI and Emotion Dependencies from Diagram PackageThis is an excellent refactoring that successfully eliminates MUI dependencies from the ✅ StrengthsArchitecture & Design
Code Quality
Accessibility
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 76.79% 76.81% +0.01%
==========================================
Files 69 69
Lines 20523 20523
==========================================
+ Hits 15761 15765 +4
+ Misses 4762 4758 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddec24f8dc
ℹ️ 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".
| <button | ||
| className={clsx(styles.actionButton, styles.actionButtonOpen, className)} | ||
| onClick={onClick as any} | ||
| aria-label={title} |
There was a problem hiding this comment.
The new SpeedDialAction only forwards the action’s onClick handler and never triggers the parent onClose, so selecting Stock/Flow/etc. does not clear dialOpen anymore. In Editor, dialOpen is only toggled by the FAB click or handleDialClose (which ignores mouseLeave/blur), so tool selection now leaves the speed dial open until the user clicks the FAB again or presses Escape. This is a behavior regression from MUI’s SpeedDialAction, which closes after action clicks, and it keeps the overlay covering the canvas unexpectedly.
Useful? React with 👍 / 👎.
- SpeedDial: Close dial on action click by injecting _onActionClick callback from parent to children via cloneElement. Add 'actionClick' to CloseReason type. Editor.tsx now handles this to close the dial while keeping the selected tool. - SpeedDial: Fix type safety by changing onClick to accept HTMLButtonElement instead of HTMLDivElement, removing 'as any' casts. - Tabs: Replace _tabValue cloneElement hack with React Context for passing tab index from parent to child. - TextField: Add id/htmlFor connection between labels and inputs for accessibility. Generate unique IDs when not provided. - TextField: Fix border width layout shift by using box-shadow for the thicker focus/error indicator instead of changing border-width. - Drawer: Add role="dialog", aria-modal="true", and tabIndex for accessibility. Implement focus trap (Tab cycles within panel when open) and focus management (save/restore focus on open/close).
|
@codex review |
PR Review: Remove MUI and Emotion dependencies from @system-dynamics/diagramOverviewThis PR successfully removes Material-UI and Emotion dependencies from the ✅ StrengthsArchitecture & Design
Implementation Quality
🔍 Issues & ConcernsCritical Issues1. Autocomplete: Inconsistent React paradigm (src/diagram/components/Autocomplete.tsx:27)The Autocomplete component is a function component with hooks, while the codebase standard (per CLAUDE.md) is to use class components and avoid hooks. This is inconsistent with all other components in this PR. Recommendation: Refactor to a class component, or provide justification for why hooks are required here (likely due to Downshift's 2. Missing unit tests (Test plan checklist item incomplete)The PR description includes a test plan with an incomplete item:
Recommendation: Add comprehensive unit tests before merging, especially for:
Per CLAUDE.md's TDD principle, tests should have been written first to guide implementation. High-Priority Issues3. TextField: Missing accessibility labels (src/diagram/components/TextField.tsx:148-152)The outlined variant's label is not properly associated with the input when shrunk. The label uses Recommendation: Add 4. Drawer: Global event listener not scoped (src/diagram/components/Drawer.tsx:24)The keydown listener is added to the document in Recommendation: Add/remove the listener in componentDidUpdate(prevProps: DrawerProps) {
if (this.props.open && !prevProps.open) {
document.addEventListener('keydown', this.handleKeyDown);
// existing focus management...
} else if (!this.props.open && prevProps.open) {
document.removeEventListener('keydown', this.handleKeyDown);
// existing focus restoration...
}
}5. Autocomplete: Potential stale closure in dropdown positioning (src/diagram/components/Autocomplete.tsx:74-83)The Recommendation: Add window resize and scroll event listeners, or use a library like Medium-Priority Issues6. Hardcoded color values instead of CSS variables (Multiple CSS files)Many CSS files use hardcoded color values:
Some CSS variables are used ( Recommendation: Extract all color values to CSS variables for maintainability and potential theming support. 7. Button: Missing ripple effect (src/diagram/components/Button.tsx)MUI buttons have a ripple effect on click. The custom Button lacks this, which may be a noticeable visual regression. Recommendation: Either add a ripple effect or acknowledge this as an intentional simplification in the PR description. 8. SnackbarContent: Fragile prop filtering (src/diagram/components/Snackbar.tsx:76-82)The component manually filters out Recommendation: Use a whitelist approach or TypeScript's 9. TextField: Global ID counter potential collision (src/diagram/components/TextField.tsx:11-12)The Recommendation: Use a more robust ID generation strategy, like Low-Priority / Style Issues10. Missing CSS variable definitionsThe code references Recommendation: Ensure these are defined in a global stylesheet or document where they should be set. 11. SpeedDial: Unused prop type (src/diagram/components/SpeedDial.tsx:14)The Recommendation: Remove if not needed, or implement if this was an oversight. 📊 Security & Performance
🧪 Test CoverageStatus: ❌ Incomplete The PR test plan explicitly notes that unit tests are not yet added. Per CLAUDE.md:
Required before merge:
📦 Dependencies
🎯 Recommendations SummaryMust fix before merge:
Should fix before merge: Nice to have: ✨ ConclusionThis is a well-structured PR that successfully removes MUI dependencies with clean, maintainable custom components. The CSS module approach is excellent, and the component APIs are thoughtfully designed. However, the PR should not be merged until unit tests are added per the project's TDD requirements and CLAUDE.md guidelines. The implementation quality is high, but the missing tests and a few architectural issues (particularly around Autocomplete's hook usage and Drawer's event listener efficiency) should be addressed. Overall assessment: Approve with required changes ✅ (pending test coverage) Review generated following CLAUDE.md conventions and development standards |
Code Review: PR #217 - Remove MUI and Emotion DependenciesSummaryThis PR successfully removes Material-UI and Emotion dependencies from StrengthsArchitecture & Design✅ Clean separation of concerns: Custom components are properly organized in Component Quality✅ Accessibility: Good ARIA attributes (aria-label, aria-modal, aria-describedby, aria-expanded) Critical Issues1. Missing Test Coverage
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3be20b51e
ℹ️ 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".
| inputValue, | ||
| selectedItem: value || null, | ||
| onInputValueChange: ({ inputValue: newInputValue }) => { | ||
| setInputValue(newInputValue || ''); |
There was a problem hiding this comment.
value changes
In src/diagram/components/Autocomplete.tsx, inputValue is only updated from onInputValueChange, while selectedItem is controlled by the value prop. If a parent updates value programmatically (for example, selection changes outside the dropdown), the text field can keep the old inputValue and keep filtering against stale text because nothing re-syncs inputValue to value. Consider syncing inputValue in an effect when value changes or exposing a controlled inputValue prop so the displayed input stays consistent with the selected item.
Useful? React with 👍 / 👎.
src/diagram/components/Snackbar.tsx
Outdated
| componentDidUpdate(prevProps: SnackbarProps) { | ||
| if (this.props.open && !prevProps.open && this.props.autoHideDuration && this.props.onClose) { | ||
| this.startTimer(); | ||
| } |
There was a problem hiding this comment.
In src/diagram/components/Snackbar.tsx, the auto-hide timer only starts in componentDidUpdate when open transitions from false to true. If the Snackbar mounts with open={true} (e.g., the screen loads with existing errors), the timer never starts and onClose will not fire, so autoHideDuration is effectively ignored. Starting the timer in componentDidMount when open is already true would align with expected Snackbar behavior.
Useful? React with 👍 / 👎.
Fix several issues identified in the PR review:
- Autocomplete: Add useEffect to sync inputValue when value prop changes
externally. Previously, if a parent updated the value programmatically,
the text field would show stale text.
- Snackbar: Start auto-hide timer in componentDidMount when open={true},
not just on transitions. Also simplify update logic to always clear
timer first, preventing race conditions on rapid open/close cycles.
- SpeedDial: Replace unsafe type assertions with proper type guards using
React.isValidElement<T>() generic parameter instead of casting after.
- Drawer: Expand focus trap selector to include 'a' and '[contenteditable]'
elements that were previously missed.
Add comprehensive tests for these components covering:
- Autocomplete: External value sync, filtering, clearOnEscape
- Snackbar: Timer behavior on mount, transitions, rapid toggles
- Drawer: Focus management, focus trap with various element types
- SpeedDial: Rendering, interactions, icon transitions
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6509d4f07
ℹ️ 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".
src/diagram/components/Snackbar.tsx
Outdated
| componentDidUpdate(_prevProps: SnackbarProps) { | ||
| // Always clear first to prevent race conditions on rapid open/close | ||
| this.clearTimer(); | ||
|
|
||
| // Start new timer if now open | ||
| if (this.props.open) { | ||
| this.startTimer(); |
There was a problem hiding this comment.
Right now componentDidUpdate clears and restarts the timer on every update, even when open stays true and only unrelated parent state changes. In the editor, frequent re-renders (selection updates, mouse moves, etc.) will keep resetting the timeout so the snackbar never auto-dismisses despite autoHideDuration being set. MUI’s Snackbar only restarts on open transitions or duration changes; this version should similarly gate the timer reset on open/autoHideDuration changes to avoid an effectively infinite timeout.
Useful? React with 👍 / 👎.
The previous implementation reset the auto-hide timer on every componentDidUpdate call, which meant unrelated parent re-renders would continuously reset the countdown, effectively preventing the snackbar from ever auto-dismissing in practice. Now the timer only restarts when `open` or `autoHideDuration` actually change. Added a test that verifies unrelated re-renders don't reset the timer, ensuring the snackbar dismisses after the configured duration regardless of how often the parent re-renders.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4f51159c7
ℹ️ 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".
| <input | ||
| id={inputId} | ||
| className={styles.standardInput} | ||
| value={value} | ||
| onChange={onChange} | ||
| type={type} | ||
| placeholder={placeholder} | ||
| onFocus={this.handleFocus} | ||
| onBlur={this.handleBlur} | ||
| {...inputProps} | ||
| {...rest} |
There was a problem hiding this comment.
In TextField, inputProps is spread after the component’s own onFocus/onBlur handlers. When inputProps contains these handlers (e.g., Downshift’s getInputProps used by Autocomplete), it overwrites the internal handlers so isFocused never updates. This prevents the floating label and focus/error styles from activating while the field is focused, which is visible any time TextField is used via Autocomplete with a label or outlined variant. Consider chaining inputProps.onFocus/onBlur with handleFocus/handleBlur instead of letting them override.
Useful? React with 👍 / 👎.
When TextField is used with inputProps that include onFocus/onBlur handlers (e.g., from Downshift's getInputProps in Autocomplete), those handlers were overwriting the internal handlers that track isFocused state. This meant the floating label and focus styles wouldn't work properly when used via Autocomplete. Now the internal handlers call any external handlers from inputProps after updating internal state, ensuring both the component's focus tracking and any external handlers work correctly together. Added comprehensive tests for TextField including handler chaining.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
PR Review: Remove MUI and Emotion dependenciesSummaryExcellent refactoring with comprehensive test coverage and strong accessibility. Code quality is high with only minor issues identified. Strengths1. Excellent Test Coverage
2. Strong Accessibility
3. Good Event Handler Design
Issues Found1. TextField Handler Extraction (Priority: High)File: src/diagram/components/TextField.tsx:82 Line 82 extracts handlers into unused variables (_onFocus, _onBlur), but lines 126-127 and 167-168 reference this.props.inputProps directly. Fix: Use the extracted values consistently: const { onFocus: externalOnFocus, onBlur: externalOnBlur, ...restInputProps } = inputProps || {};
handleFocus = (event) => {
this.setState({ isFocused: true });
externalOnFocus?.(event);
};2. Drawer Event Listener (Priority: Medium)File: src/diagram/components/Drawer.tsx:24 Attaches global keydown listener in componentDidMount regardless of open state. Consider only adding when open=true and managing in componentDidUpdate for better performance. 3. Duplicated CSS Shadows (Priority: Low)File: src/diagram/Editor.module.css:22, 68 MUI elevation shadows duplicated. Consider CSS custom properties for maintainability. Performance & Security
VerdictAPPROVE with minor changes requested High-quality work demonstrating excellent engineering practices. Issues identified are minor and don't block merging, though fixing the TextField handler extraction improves code clarity. Great migration work! 🚀 |
Summary
@system-dynamics/diagramwith custom implementations using CSS modules, Radix Tabs, and Downshift@system-dynamics/app(previously inherited via hoisting)Details
New custom components in
src/diagram/components/:MUI's Card, Paper, CardContent, CardActions, and Typography replaced with plain
<div>elements styled via CSS module classes.Test plan
yarn tscpassesyarn lintpassesyarn buildsucceeds (app, web component, and website)@muior@emotionimports remain insrc/diagram/