Skip to content

[Feat] Updated MainMenu#14101

Merged
mayagbarnes merged 6 commits intodevelopfrom
feature/new-app-menu
Feb 24, 2026
Merged

[Feat] Updated MainMenu#14101
mayagbarnes merged 6 commits intodevelopfrom
feature/new-app-menu

Conversation

@mayagbarnes
Copy link
Copy Markdown
Collaborator

Core refactor of the `MainMenu` component that replaces BaseWeb's `StatefulMenu` with a custom data-driven implementation. This is the foundational PR for the `MainMenu` redesign, going into the feature branch for incremental review.

- Remove `StatefulMenu` from BaseWeb, keep `StatefulPopover` for positioning/focus lock
- Introduce section-based data model (`MenuSection`, `MenuItemConfig`)
- Pure functions for menu construction (`buildMenuData`,`buildMenuSections`, `buildCommonItems`, etc.)
- Memoized `MenuContent` component with stable callbacks
Adds full WAI-ARIA menu pattern support to the `MainMenu`, including
keyboard navigation, proper ARIA semantics, and focus management.

**Keyboard navigation**
- **Open menu**: `Enter` / `Space` on the menu button (handles legacy
`Spacebar` / `Space` key values)
- **Navigate items**: `ArrowDown` / `ArrowUp` with wrapping at
boundaries
- **Jump to ends**: `Home` / `End`
- **Activate item**: `Enter` / `Space` on focused menu item
- **Close menu**: `Escape` returns focus to the trigger; `Tab` /
`Shift+Tab` close the menu and advance focus to the next/previous
tabbable element
- **Disabled items**: All items are focusable and navigable, including
disabled ones (per [WAI-ARIA menuitem role
guidance](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/menuitem_role)).
Disabled items use `aria-disabled` instead of the native `disabled`
attribute; clicks are blocked via the `onClick` handler.

**ARIA attributes**
- Menu button: `aria-haspopup="menu"`, `aria-expanded` (tracks
open/close state), `aria-label="Main menu"`
- Menu container: `role="menu"`, `aria-label="Main menu"`
- Menu items: `role="menuitem"`, `aria-disabled` for disabled items
(native `disabled` removed to keep items focusable)
- Dividers: `role="separator"`, `aria-hidden="true"`
- Roving tabindex: focused item gets `tabIndex={0}`, all others get
`tabIndex={-1}`

**Focus management**
- Focus moves to the first menu item when the menu opens
- Focus returns to the menu button after `Escape` or item click, via
react-focus-lock's `returnFocus` callback
- `Tab` / `Shift+Tab` close the menu and programmatically advance focus
to the next/previous tabbable element using
`focusNextElement`/`focusPrevElement` from `focus-lock`
- **WebKit limitation**: On Safari, focus restoration after menu close
may not work because react-focus-lock invokes the `returnFocus` callback
after BaseWeb's close animation timer, which puts the `.focus()` call
outside the user-activation context

**Clear cache dialog (WAI-ARIA best practice)**
- Moves `autoFocus` from the destructive "Clear caches" button to the
non-destructive "Cancel" button, per WAI-ARIA guidance for confirmation
dialogs
- Removes the now-unnecessary `defaultAction` prop from
`ClearCacheProps`

**Theme fix**
- `focusRingMuted` shadow now uses `gray10` on dark backgrounds (was
using `gray90`, which was invisible)
Adds a theme toggle (System / Light / Dark) directly into the main menu as `menuitemradio` elements, building on the accessibility refactor in #13878.

**Changes:**
- Three radio items (System, Light, Dark) appear at the top of the menu,
inside a `role="group"` container with `aria-label="Theme"`.
- Each item uses `role="menuitemradio"` with `aria-checked` to reflect
the active theme.
- Selecting a theme calls `setTheme` via `ThemeContext` without closing
the menu, so users can preview different themes.
- Keyboard navigation (ArrowUp/Down, Home/End, Enter/Space) flows
seamlessly between radio items and action items in a single roving
tabindex.
- Adds an **Auto-rerun toggle** (`menuitemcheckbox`) to the `MainMenu`
developer section, surfacing the run-on-save setting directly in the
menu. Wired to the same `saveSettings` path as the Settings dialog
checkbox.
- Extends the `MenuItem` discriminated union with a `MenuToggleItem`
type and `ToggleItemRow` component, with full WAI-ARIA keyboard support
(Enter/Space activation, roving tabindex).
- Gates Settings, Rerun, Auto-rerun, and Clear cache behind
`developmentMode` to hide them from app viewers.
- Adds a "Made with Streamlit v{version}" footer inside the main menu, with a hover-to-reveal copy button that copies the full version string to the clipboard.
- Merges the About menu item into the standard items section (removing its separate divider).
- Adds the missing `ESC` keyboard shortcut hint on the Record screen item during active recording (parity with `develop`).
- Nightly `.dev` versions are truncated for display (e.g., `v1.54.1.dev`) but the full version (e.g., `1.54.1.dev20260217`) is copied.
- Removes the **Settings** menu item from the main menu and deletes the `SettingsDialog` component entirely. All Settings dialog functionality has been absorbed by the new main menu:
  - **Theme switching** → theme radio buttons (System / Light / Dark) in the menu
  - **Auto-rerun (Run on save)** → toggle in the menu
  - **Version footer** → footer in the menu
  - **Wide mode** → removed from UI (set via `st.set_page_config(layout="wide")`)
- Migrates **5 theming E2E test files** from the Settings dialog selectbox flow to the main menu's `menuitemradio` buttons, using accessible selectors (`get_by_role("menuitemradio", name="Dark")`).
- Deletes `test_renders_settings_dialog_properly` and `test_settings_dialog_copies_version` from `main_menu_test.py` (coverage is now provided by the menu footer tests).
- Removes `SETTINGS` from the `DialogType` enum and all associated plumbing (`settingsCallback` in `App.tsx`, `Props`, `buildDevItems`, `StreamlitDialog` switch case).
- Cleans up orphaned styled components and updates stale `@see SettingsDialog` JSDoc references in `ThemeContext.tsx` and `types.ts`.
@mayagbarnes mayagbarnes added security-assessment-completed change:feature PR contains new feature or enhancement implementation impact:users PR changes affect end users labels Feb 24, 2026
@snyk-io
Copy link
Copy Markdown
Contributor

snyk-io bot commented Feb 24, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 24, 2026

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-14101/streamlit-1.54.0-py3-none-any.whl
📦 @streamlit/component-v2-lib Download from artifacts
🕹️ Preview app pr-14101.streamlit.app (☁️ Deploy here if not accessible)

@github-actions
Copy link
Copy Markdown
Contributor

📉 Significant bundle size change detected

Metric This Branch develop Change (%)
Total (gzip) 8.06 MiB 8.07 MiB -0.06%
Entry (gzip) 667.1 KiB 723.75 KiB -7.83%

Please verify that this change is expected.

📊 View detailed bundle comparison

@github-actions
Copy link
Copy Markdown
Contributor

📈 Frontend coverage change detected

The frontend unit test (vitest) coverage has increased by 0.2000%

  • Current PR: 87.6000% (14283 lines, 1771 missed)
  • Latest develop: 87.4000% (14191 lines, 1787 missed)

🎉 Great job on improving test coverage!

📊 View detailed coverage comparison

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates six incremental MainMenu redesign PRs into develop, completing the migration of Settings dialog functionality into the main menu and removing the Settings dialog entirely. The refactor introduces a data-driven menu architecture with full WAI-ARIA support, theme switching radio buttons, auto-rerun toggle, and a version footer—all directly accessible from the main menu.

Changes:

  • Backend: Removed Settings dialog plumbing (settingsCallback, DialogType.SETTINGS), added handleRunOnSaveChange handler, updated client.toolbarMode config description
  • Frontend Library: Enhanced BaseButton with ARIA popup attributes, fixed focusRingMuted shadow for dark backgrounds, updated theme-related JSDoc references
  • MainMenu Component: Complete refactor with theme radio group, toggle component, version footer with copy button, keyboard navigation, and accessible menu structure
  • E2E Tests: Migrated 5 theming test files from Settings dialog selectbox to MainMenu menuitemradio buttons, updated snapshots, added comprehensive keyboard navigation tests

Reviewed changes

Copilot reviewed 32 out of 63 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/streamlit/config.py Updated client.toolbarMode description to remove Settings dialog reference
frontend/lib/src/theme/types.ts, getShadows.ts Updated JSDoc and fixed focusRingMuted shadow for dark themes (gray10 instead of gray90)
frontend/lib/src/components/shared/BaseButton/* Added ARIA aria-haspopup and aria-expanded support with proper TypeScript typing
frontend/app/src/components/MainMenu/* Complete refactor: added theme radio group, toggle component, version footer, keyboard navigation, and test utilities
frontend/app/src/components/StreamlitDialog/* Removed entire SettingsDialog component and all styled-components; updated ClearCacheDialog to focus Cancel button (WAI-ARIA best practice)
frontend/app/src/App.tsx Removed settingsCallback, added handleRunOnSaveChange, passed new props to MainMenu
e2e_playwright/theming/*_test.py Migrated theme selection from Settings dialog to MainMenu radio buttons, using accessible get_by_role selectors
e2e_playwright/main_menu_test.py Added keyboard navigation tests, version footer tests, removed Settings dialog tests, updated snapshot handling

@mayagbarnes mayagbarnes added the ai-review If applied to PR or issue will run AI review workflow label Feb 24, 2026
@github-actions github-actions bot removed the ai-review If applied to PR or issue will run AI review workflow label Feb 24, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Summary

This PR is a comprehensive refactor of the Streamlit MainMenu component, consolidating 6 sub-PRs into a single feature branch. The key changes are:

  • Core refactor: Replaces the old BaseWeb-based dropdown menu with a custom implementation using proper WAI-ARIA menu button patterns, including roving tabindex, keyboard navigation (Arrow keys, Home/End, Escape, Tab), and focus management.
  • Theme switcher: Moves theme selection from the SettingsDialog into the menu as radio buttons (System, Light, Dark) using menuitemradio role.
  • Auto-rerun toggle: Adds an on/off toggle switch for auto-rerun using menuitemcheckbox role.
  • Version footer: Moves the "Made with Streamlit" version info and copy button into the menu footer.
  • SettingsDialog removal: Completely removes the SettingsDialog component and its associated files, tests, and snapshot references.
  • BaseButton enhancement: Adds forwardRef support and aria-haspopup/aria-expanded ARIA attributes.
  • Focus ring fix: focusRingMuted shadow now adapts to dark backgrounds.

Code Quality

The refactor is well-structured and follows codebase conventions:

  • Clean separation of concerns: Data building (buildMenuData, buildThemeSection) is separated from rendering (MenuContent, MenuItemRow, etc.).
  • Proper memoization: MenuItemRow, ThemeRadioItemRow, and ToggleItemRow use React.memo with stable useCallback refs to prevent unnecessary re-renders.
  • Module-level constants: Static data like SCREENCAST_LABEL, MENU_OPEN_KEYS, and THEME_OPTIONS are correctly extracted to module scope.
  • No useEffect anti-patterns: Focus management uses callback refs and event handlers, not effects.
  • Well-documented: JSDoc comments explain non-obvious design decisions (e.g., why MenuContent is not memoized, why closeReasonRef exists, WebKit limitations).

Minor observations (both reviewers agreed):

  1. buildMenuData signature (MainMenu.tsx:219): Accepts 17 positional parameters. An options object (RORO pattern) would improve readability. Non-blocking.
  2. Stale comment (styled-components.ts:346): StyledMenuVersionText comment says "Matches the Settings dialog style" — should be updated since SettingsDialog no longer exists.

Test Coverage

Both reviewers agreed that test coverage is excellent:

  • Frontend unit tests: MainMenu.test.tsx covers keyboard navigation (Arrow keys, Home/End, Escape, Tab, Shift+Tab), focus management (roving tabindex, focus return), ARIA attributes, disabled item behavior, menu structure for various configurations (dev mode, minimal mode, host items, custom items), metrics tracking, screencast states, and version footer.
  • ToggleItemRow.test.tsx: Covers role, label, aria-checked, aria-disabled, tabindex, click/keyboard activation, and disabled state.
  • themeSection.test.ts: Covers findThemeForSelection, buildThemeSection, and edge cases.
  • E2E tests: main_menu_test.py has 16 test functions covering keyboard flow, theme switching, persistence on reload, auto-rerun toggle, version footer, visual snapshots, and dialog interactions.
  • Shared test utilities: mainMenuTestHelpers.ts reduces test boilerplate.
  • Positive + negative assertions: Tests consistently include complementary negative checks.

Backwards Compatibility

This is a user-facing breaking change in terms of UI surface (both reviewers aligned):

  • SettingsDialog removed: Users who relied on the Settings dialog for theme selection, version info, or run-on-save configuration now access these through the menu directly.
  • Wide mode UI toggle removed: The runtime wide-mode checkbox is gone — wide mode can only be set via st.set_page_config(layout="wide").
  • Menu item labels changed: "Record a screencast" → "Record screen".
  • No Python API or protocol break. The PR is labeled change:feature and impact:users, so these UI changes appear intentional.

Security & Risk

Both reviewers agreed: No security concerns identified. No new external dependencies or network calls. URLs are opened via window.open with _blank target. The focus-lock import is from an existing dependency. Regression risks are well-mitigated by the comprehensive test suite.

Accessibility

Both reviewers agreed this PR is a significant accessibility improvement overall: proper WAI-ARIA menu button pattern, roving tabindex, full keyboard navigation, correct roles (menuitem, menuitemradio, menuitemcheckbox, separator, group), aria-disabled usage, and focus-visible styling.

Point of disagreement — version footer CopyButton:

  • gpt-5.3-codex-high flagged this as a blocking issue: The CopyButton (MainMenu.tsx:817-824) is rendered as a plain <button> inside a role="menu" container without a menuitem role, and it is excluded from the roving-focus flatItems list. Since Tab closes the menu, the button is unreachable via keyboard.

  • opus-4.6-thinking did not flag this, approving overall.

Consolidated assessment: The observation is technically correct — there is an ARIA semantics violation (a non-menuitem focusable control inside role="menu") and the CopyButton cannot be reached via keyboard. However, after examining the code, I assess this as a non-blocking issue for the following reasons:

  • The CopyButton is a secondary convenience feature for copying the version string, not a primary menu action.
  • The version text itself is visible in the footer without any interaction needed.
  • The button is hidden by default (opacity: 0, pointerEvents: none) and only appears on hover/focus-within.
  • The PR as a whole is a major accessibility improvement over the previous BaseWeb-based menu.
  • This can be cleanly addressed in a quick follow-up (e.g., include the copy action in flatItems as a menuitem, or move the footer outside role="menu").

Recommendations

  1. (Follow-up) Address the version CopyButton keyboard accessibility: either include it in the roving tabindex system as a menuitem, or move StyledMenuVersionFooter outside the role="menu" container. Add a unit test for keyboard reachability.
  2. Stale comment: Update the StyledMenuVersionText JSDoc at styled-components.ts:346 — it references the removed SettingsDialog.
  3. Consider RORO pattern for buildMenuData: The 17-parameter signature would benefit from an options object for readability.
  4. Consider documenting the wide mode change: Since the runtime wide-mode toggle is removed, it may be worth a note in the release changelog.

Verdict

APPROVED — This is a well-executed, thoroughly tested refactor that significantly improves MainMenu accessibility, UX, and code quality. Both reviewers agreed on the overall high quality of the implementation. The CopyButton keyboard accessibility issue raised by gpt-5.3-codex-high is valid but non-blocking — it is a minor ARIA conformance gap on a secondary convenience feature that does not diminish the substantial accessibility gains of this PR, and can be addressed in a targeted follow-up.


Consolidated review by opus-4.6-thinking. All expected models (gpt-5.3-codex-high, opus-4.6-thinking) submitted reviews successfully.


📋 Review by `gpt-5.3-codex-high`

Summary

This PR delivers a large MainMenu redesign: it removes SettingsDialog, introduces in-menu theme radios, adds an auto-rerun toggle, adds a version footer with copy action, refactors menu keyboard/focus behavior, and updates unit/e2e coverage plus snapshots accordingly.

Code Quality

The refactor is generally well-structured and significantly improves menu architecture (data-driven sections, clearer item types, stronger focus handling), but there is one blocking accessibility/interaction defect:

  • Highfrontend/app/src/components/MainMenu/MainMenu.tsx (around lines 611, 705-713, 802-827): the version CopyButton is rendered as a regular button inside a container with role="menu", but it is not part of the roving-focus flatItems list and Tab is explicitly trapped to close the menu. In practice this makes the copy action mouse-only for keyboard users and places a non-menuitem interactive control inside a menu role container.

Test Coverage

Coverage is strong overall:

  • Extensive frontend unit tests were added/updated for menu keyboard navigation, roving tabindex, theme radios, toggle behavior, focus return, and version copy behavior.
  • E2E tests were broadly updated to exercise the new menu-based theme switching and menu interactions.

Gap:

  • There is no test asserting keyboard reachability/activation of the version copy action from within the menu, which likely allowed the issue above to slip through.

Backwards Compatibility

No Python API/protocol break is visible from this diff, and lib/streamlit/config.py only adjusts wording.
Behaviorally, this is a substantial UI change (SettingsDialog removal), so release notes/docs should clearly call out changed interaction patterns.

Security & Risk

No new backend security issues were evident in the reviewed changes.
Primary regression risk is keyboard/a11y behavior in the menu due to the new footer interaction pattern.

Accessibility

Accessibility has improved in several areas (menu roles, focus management, keyboard nav), but the version copy control currently regresses keyboard accessibility:

  • frontend/app/src/components/MainMenu/MainMenu.tsx (around lines 802-827): focusable non-menuitem control inside role="menu".
  • frontend/app/src/components/MainMenu/MainMenu.tsx (around lines 705-713): Tab closes the menu, preventing keyboard traversal to footer copy action.

Recommendations

  1. Move the version copy action into the menu interaction model (e.g., render it as a menuitem/action row and include it in flatItems) or remove it from role="menu" and provide an equivalent keyboard-accessible path.
  2. Add unit + e2e coverage for keyboard-only activation of version copy.
  3. Validate menu child semantics against WAI-ARIA menu pattern after the footer fix.

Verdict

CHANGES REQUESTED: The new menu contains a keyboard/a11y regression where the version copy action is not reachable/operable via keyboard within role="menu".


This is an automated AI review by gpt-5.3-codex-high. Please verify the feedback and use your judgment.

📋 Review by `opus-4.6-thinking`

Summary

This PR is a comprehensive refactor of the Streamlit MainMenu component, consolidating 6 sub-PRs into a single feature branch. The key changes are:

  • Core refactor: Replaces the old BaseWeb-based dropdown menu with a custom implementation using proper WAI-ARIA menu button patterns, including roving tabindex, keyboard navigation (Arrow keys, Home/End, Escape, Tab), and focus management.
  • Theme switcher: Moves theme selection from the SettingsDialog into the menu as radio buttons (System, Light, Dark) using menuitemradio role.
  • Auto-rerun toggle: Adds an on/off toggle switch for auto-rerun using menuitemcheckbox role.
  • Version footer: Moves the "Made with Streamlit" version info and copy button into the menu footer.
  • SettingsDialog removal: Completely removes the SettingsDialog component and its associated files, tests, and snapshot references.
  • BaseButton enhancement: Adds forwardRef support and aria-haspopup/aria-expanded ARIA attributes.
  • Focus ring fix: focusRingMuted shadow now adapts to dark backgrounds (uses gray10 instead of always gray90).

Code Quality

The code is well-structured and follows the codebase conventions:

  • Clean separation of concerns: Data building (buildMenuData, buildThemeSection) is separated from rendering (MenuContent, MenuItemRow, etc.).
  • Proper memoization: MenuItemRow, ThemeRadioItemRow, and ToggleItemRow use React.memo with stable useCallback refs to prevent unnecessary re-renders.
  • Module-level constants: Static data like SCREENCAST_LABEL, MENU_OPEN_KEYS, and THEME_OPTIONS are correctly extracted to module scope per the frontend guidelines.
  • No useEffect anti-patterns: Focus management uses callback refs and event handlers, not effects. The menuListRef callback correctly focuses the first item on mount.
  • Well-documented: JSDoc comments explain non-obvious design decisions (e.g., why MenuContent is not memoized, why closeReasonRef exists, WebKit limitations).

Minor observations:

  1. buildMenuData at MainMenu.tsx:219 accepts 17 positional parameters. While the function is internal and well-typed, an options object pattern (RORO) would improve readability and align with the frontend AGENTS.md guidance. This is a minor stylistic concern, not a blocking issue.

  2. The StyledMenuVersionText at styled-components.ts:348 still references "Settings dialog style" in its comment — a stale reference now that the SettingsDialog is removed.

Test Coverage

Test coverage is excellent:

  • Frontend unit tests: MainMenu.test.tsx (1587 lines) covers keyboard navigation (Arrow keys, Home/End, Escape, Tab, Shift+Tab), focus management (roving tabindex, focus return), ARIA attributes, disabled item behavior, menu structure for various configurations (dev mode, minimal mode, host items, custom items), metrics tracking, screencast states, and version footer.
  • ToggleItemRow.test.tsx (183 lines) covers role, label, aria-checked, aria-disabled, tabindex, click/keyboard activation, and disabled state.
  • themeSection.test.ts (200 lines) covers findThemeForSelection (preset and custom themes), buildThemeSection (labels, keys, icons, checked state, empty/single theme cases, metrics).
  • E2E tests: main_menu_test.py has 16 test functions covering keyboard flow, theme switching, persistence on reload, auto-rerun toggle, version footer, visual snapshots, and dialog interactions. All existing theming E2E tests are updated to use the new menu radio buttons instead of the Settings dialog.
  • Positive + negative assertions: Tests consistently include complementary negative checks (e.g., verifying items are not present when hidden, menu not visible after close, focus not on trigger after Tab).

The shared mainMenuTestHelpers.ts utility (openMenu, getMenuLabels) reduces test boilerplate and adapts to both fake and real timers.

Backwards Compatibility

This is a user-facing breaking change in terms of UI surface:

  • SettingsDialog removed: The "Settings" menu item no longer exists. Users who relied on the Settings dialog for theme selection, version info, or run-on-save configuration now access these through the menu directly.
  • Wide mode UI toggle removed: The SettingsDialog previously had a "Wide mode" checkbox that let users toggle wide layout at runtime. This UI toggle is no longer available — wide mode can only be set programmatically via st.set_page_config(layout="wide"). This is likely an intentional design decision, but downstream users who relied on the runtime toggle will notice the change.
  • Menu item labels changed: "Record a screencast" is now "Record screen". E2E tests referencing the old label need updating (and have been).
  • client.toolbarMode config description updated: Minor doc change, no functional impact.

Since the PR is labeled change:feature and impact:users, these breaking changes appear intentional and acknowledged.

Security & Risk

No security concerns identified:

  • URLs are opened via window.open with _blank target; the openInNewTab helper logs a warning if the popup is blocked.
  • No new external dependencies or network calls are introduced.
  • The focus-lock import for focusNextElement/focusPrevElement is from an existing dependency (react-focus-lock already used by BaseWeb).

Regression risks are well-mitigated by the comprehensive test suite. The main risk area is the WebKit focus-return limitation (documented in comments and addressed with @pytest.mark.skip_browser("webkit") in E2E tests).

Accessibility

This PR is a significant accessibility improvement:

  • WAI-ARIA menu button pattern: Proper role="menu", role="menuitem", role="menuitemradio", role="menuitemcheckbox", role="separator", and role="group" roles.
  • Keyboard navigation: Full arrow key navigation with wrap-around, Home/End support, Escape to close (returns focus to trigger), Tab/Shift+Tab to close (advances focus naturally per WAI-ARIA).
  • Roving tabindex: Only the focused item has tabIndex={0}; all others have tabIndex={-1}.
  • Focus management: First item receives focus on open; focus returns to trigger on Escape; disabled items remain focusable per WAI-ARIA guidelines.
  • Menu button ARIA: aria-haspopup="menu", aria-expanded toggles correctly, aria-label="Main menu".
  • Disabled items: Use aria-disabled (not HTML disabled) so they remain focusable but non-activatable, correctly following WAI-ARIA guidance.
  • Focus-visible styling: :focus-visible box-shadow rings on all interactive items using theme shadow tokens.
  • BaseButton enhancement: The forwardRef + ARIA attribute additions to BaseButton are properly typed and backward-compatible (new ARIA props are optional).

Recommendations

  1. Consider RORO pattern for buildMenuData: The 17-parameter function signature could benefit from an options object to improve readability and self-documentation.

  2. Stale comment in StyledMenuVersionText: The comment at styled-components.ts:346 says "Matches the Settings dialog style" — this should be updated since SettingsDialog no longer exists.

  3. Consider documenting the wide mode change: Since the runtime wide-mode toggle is removed, it may be worth a note in the release changelog or migration guide for users who relied on it.

Verdict

APPROVED: This is a well-executed, thoroughly tested refactor that significantly improves the MainMenu's accessibility, UX, and code quality. The SettingsDialog removal and wide-mode toggle loss are intentional design decisions that are acknowledged via the PR labels. The comprehensive test coverage across unit and E2E tests provides strong confidence against regressions.


This is an automated AI review by opus-4.6-thinking.

@mayagbarnes mayagbarnes marked this pull request as ready for review February 24, 2026 07:46
Copy link
Copy Markdown
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: reset hover might help here to prevent the menu icon from being hovered (probably unintended)

@mayagbarnes mayagbarnes merged commit 2b207e4 into develop Feb 24, 2026
98 checks passed
@mayagbarnes mayagbarnes deleted the feature/new-app-menu branch February 24, 2026 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:feature PR contains new feature or enhancement implementation impact:users PR changes affect end users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants