feat: migrate main menu to mui#912
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughMenu components refactored to Material UI, menu data moved to ChangesMenu System Modernization
Sequence Diagram(s)sequenceDiagram
actor User
participant Menu as Menu (index.js)
participant Exp as ExpandableItem
participant Sub as SubMenuItem
participant MI as MenuItem
participant Router as history
User->>Menu: open menu / click section
Menu->>Exp: render section header with defaultOpen
User->>Exp: click header
Exp->>Exp: toggle open state / show Collapse
Exp->>Sub: render children list
User->>MI: click nested item
MI->>Menu: onMenuItemClick(url)
Menu->>Router: history.push('/app/' + url)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
santipalenque
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/menu/expandable-item.js`:
- Line 14: The ExpandableItem component currently initializes state with
useState(defaultOpen) but never updates when the prop changes, so add an effect
to sync the local open state when the defaultOpen prop updates: in the
ExpandableItem component (where useState(defaultOpen) is used) import and use
useEffect to call setOpen(defaultOpen) whenever defaultOpen changes; apply the
same fix for the similar instance around lines 25-29 (the other
useState(defaultOpen) usage) so route-driven changes to defaultOpen
re-open/collapse the item as expected.
In `@src/components/menu/index.js`:
- Around line 33-35: Guard access to currentSummit before calling
getSummitItems: change the eager call const summitItems =
getSummitItems(currentSummit.id) to a safe conditional such as const summitItems
= currentSummit ? getSummitItems(currentSummit.id) : [] (or use
currentSummit?.id inside a conditional) so the function is not invoked when
currentSummit is null/undefined; apply the same guard to the other occurrence
that builds summit-related items (the block referenced around lines 96-103) to
prevent runtime errors before summit data is loaded.
- Around line 36-40: The menu currently only closes via navigation
(onMenuItemClick) or onMouseLeave, which leaves touch users stuck; create a
single closeMenu handler (e.g., function closeMenu() { setMenuOpen(false); })
and use it everywhere: replace direct setMenuOpen(false) in onMenuItemClick with
closeMenu, attach closeMenu to the burger toggle's close action, add a backdrop
element with onClick/onTouchStart={closeMenu} to detect taps outside, and add
onTouchStart/onClick={closeMenu} to the explicit close button or drawer header;
also keep onMouseLeave as-is. Update references: onMenuItemClick, setMenuOpen,
history.push, and the burger toggle/button and any backdrop/drawer components
shown around lines 36 and 77-89.
- Around line 17-24: The project is using MUI v6 imports (e.g., Box, Divider,
IconButton from "@mui/material" in src/components/menu/index.js) while your
React version is 16.13.1 and thus incompatible; fix by either upgrading React
and ReactDOM in package.json to ^17.0.0 (or ^18/^19) and running npm/yarn
install and rebuilding, or pinning `@mui/material` to a v5 compatible release
(downgrade MUI in package.json) and reinstalling; after changing, verify
peerDependencies, run the app/tests, and update any MUI v6-specific APIs in
components like SubMenuItem, MenuItem, ExpandableItem if you upgrade/downgrade.
In `@src/components/menu/sub-menu-item.js`:
- Around line 20-24: The submenu component is creating an accordion even when
all children are filtered out; after computing const _subItems =
subItems.filter(...) in sub-menu-item.js, guard against an empty list by not
rendering the parent group when _subItems.length === 0 (i.e., return null / skip
render for the SubMenuItem/RenderSubMenu function). Apply the same empty-check
in the other related block (the alternative rendering branch around lines 30-45)
so parent menu groups with no visible children are not shown.
🪄 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: 82f333a7-3997-405f-9e7e-bd2a32e8abae
📒 Files selected for processing (6)
src/components/menu/expandable-item.jssrc/components/menu/index.jssrc/components/menu/menu-definition.jssrc/components/menu/menu-item.jssrc/components/menu/menu.module.lesssrc/components/menu/sub-menu-item.js
| * limitations under the License. | ||
| * */ | ||
|
|
||
| import React, { useState } from "react"; |
There was a problem hiding this comment.
Sync open when defaultOpen changes.
defaultOpen is route-derived in src/components/menu/sub-menu-item.js:26-33, but useState(defaultOpen) only applies on the first mount. After navigation, the active section can stay collapsed because this component never re-reads the updated prop.
Suggested fix
-import React, { useState } from "react";
+import React, { useEffect, useState } from "react";
@@
const [open, setOpen] = useState(defaultOpen);
+
+ useEffect(() => {
+ setOpen(defaultOpen);
+ }, [defaultOpen]);Also applies to: 25-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/menu/expandable-item.js` at line 14, The ExpandableItem
component currently initializes state with useState(defaultOpen) but never
updates when the prop changes, so add an effect to sync the local open state
when the defaultOpen prop updates: in the ExpandableItem component (where
useState(defaultOpen) is used) import and use useEffect to call
setOpen(defaultOpen) whenever defaultOpen changes; apply the same fix for the
similar instance around lines 25-29 (the other useState(defaultOpen) usage) so
route-driven changes to defaultOpen re-open/collapse the item as expected.
| import Box from "@mui/material/Box"; | ||
| import Divider from "@mui/material/Divider"; | ||
| import IconButton from "@mui/material/IconButton"; | ||
| import MenuIcon from "@mui/icons-material/Menu"; | ||
| import ChevronRightIcon from "@mui/icons-material/ChevronRight"; | ||
| import SubMenuItem from "./sub-menu-item"; | ||
| import MenuItem from "./menu-item"; | ||
| import ExpandableItem from "./expandable-item"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the peer dependency requirements for @mui/material6.4.3 and@mui/icons-material 6.4.3, and do they support React 16.13.1?
💡 Result:
The peer dependency requirements for @mui/material 6.4.3 and @mui/icons-material 6.4.3 are react (^17.0.0 || ^18.0.0 || ^19.0.0) and react-dom (^17.0.0 || ^18.0.0 || ^19.0.0). @mui/icons-material also has a peer dependency on @mui/material ^6.0.0 (matching the version series). They do not support React 16.13.1, as MUI v6 requires React ^17.0.0 or higher. React 16 support was dropped starting with v6; previous versions (v5) supported React >=16.8.0.
Citations:
- 1: https://mui.com/material-ui/getting-started/installation/
- 2: https://www.npmjs.com/package/@mui/icons-material?activeTab=dependents
- 3: https://v6.mui.com/material-ui/getting-started/supported-platforms
- 4: https://mui.com/material-ui/migration/upgrade-to-v6.md
Upgrade React to ^17.0.0 or higher before using MUI v6.
@mui/material 6.4.3 requires React ^17.0.0, ^18.0.0, or ^19.0.0 and does not support React 16.13.1. MUI v6 dropped React 16 support entirely. Either downgrade MUI to v5 (which supports React >=16.8.0) or upgrade your React version to at least 17.0.0. This is a blocking dependency incompatibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/menu/index.js` around lines 17 - 24, The project is using MUI
v6 imports (e.g., Box, Divider, IconButton from "@mui/material" in
src/components/menu/index.js) while your React version is 16.13.1 and thus
incompatible; fix by either upgrading React and ReactDOM in package.json to
^17.0.0 (or ^18/^19) and running npm/yarn install and rebuilding, or pinning
`@mui/material` to a v5 compatible release (downgrade MUI in package.json) and
reinstalling; after changing, verify peerDependencies, run the app/tests, and
update any MUI v6-specific APIs in components like SubMenuItem, MenuItem,
ExpandableItem if you upgrade/downgrade.
fb14a4f to
371fab2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/menu/index.js (1)
90-94: ⚡ Quick winAdd
aria-labelto the menu toggle button for accessibility.The burger button lacks an accessible label for screen readers. Users relying on assistive technology won't understand the button's purpose.
♿ Suggested fix
<IconButton onClick={() => (menuOpen ? closeMenu() : setMenuOpen(true))} + aria-label={menuOpen ? "Close menu" : "Open menu"} > <MenuIcon sx={{ fontSize: "2.5rem", color: "#555555" }} /> </IconButton>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/menu/index.js` around lines 90 - 94, The IconButton used as the menu toggle lacks an accessible label; update the IconButton element (the one rendering MenuIcon and using menuOpen / closeMenu / setMenuOpen) to include an aria-label that reflects state, e.g. aria-label={menuOpen ? 'Close menu' : 'Open menu'} so screen readers convey the button purpose (optionally also add aria-expanded={menuOpen} for state announcement).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/menu/index.js`:
- Around line 90-94: The IconButton used as the menu toggle lacks an accessible
label; update the IconButton element (the one rendering MenuIcon and using
menuOpen / closeMenu / setMenuOpen) to include an aria-label that reflects
state, e.g. aria-label={menuOpen ? 'Close menu' : 'Open menu'} so screen readers
convey the button purpose (optionally also add aria-expanded={menuOpen} for
state announcement).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e5d05f8-1f06-4b46-8d0b-0366d1cc14da
📒 Files selected for processing (2)
src/components/menu/index.jssrc/components/menu/sub-menu-item.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/menu/menu-definition.js`:
- Around line 140-144: The menu entry uses kebab-case ("attendee-list") while
all other menu.name values use snake_case, which breaks i18n lookups via
T.translate(`menu.${name}`); update the menu item's name property to use
snake_case (e.g., change the "name" value to "attendee_list") in the
menu-definition.js entry for the attendees route and verify any references to
that name (translation keys and tests) are updated to match the new key.
🪄 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: aba29a2a-2583-4c93-b092-32f0938e7825
📒 Files selected for processing (6)
src/components/menu/expandable-item.jssrc/components/menu/index.jssrc/components/menu/menu-definition.jssrc/components/menu/menu-item.jssrc/components/menu/menu.module.lesssrc/components/menu/sub-menu-item.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/menu/expandable-item.js
- src/components/menu/sub-menu-item.js
- src/components/menu/index.js
| { | ||
| name: "attendee-list", | ||
| linkUrl: `summits/${summitId}/attendees`, | ||
| accessRoute: "attendees" | ||
| }, |
There was a problem hiding this comment.
Inconsistent naming convention: attendee-list uses kebab-case.
All other name values use snake_case (e.g., speaker_list, badge_checkin, ticket_list), but this one uses kebab-case. Since name is used for i18n translation lookups via T.translate(\menu.${name}`)`, this inconsistency may require a separate translation key or could indicate a typo.
Proposed fix
{
- name: "attendee-list",
+ name: "attendee_list",
linkUrl: `summits/${summitId}/attendees`,
accessRoute: "attendees"
},📝 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.
| { | |
| name: "attendee-list", | |
| linkUrl: `summits/${summitId}/attendees`, | |
| accessRoute: "attendees" | |
| }, | |
| { | |
| name: "attendee_list", | |
| linkUrl: `summits/${summitId}/attendees`, | |
| accessRoute: "attendees" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/menu/menu-definition.js` around lines 140 - 144, The menu
entry uses kebab-case ("attendee-list") while all other menu.name values use
snake_case, which breaks i18n lookups via T.translate(`menu.${name}`); update
the menu item's name property to use snake_case (e.g., change the "name" value
to "attendee_list") in the menu-definition.js entry for the attendees route and
verify any references to that name (translation keys and tests) are updated to
match the new key.
santipalenque
force-pushed
the
feature/migrate-menu-to-mui
branch
from
371fab2 to
c3d0a00
Compare
1 participant
https://app.clickup.com/t/86b9n80f4
Summary by CodeRabbit
New Features
UI/Design Changes
Behavior
Tests