Skip to content

Conversation

@paynezhuang
Copy link
Contributor

  1. 支持混合布局菜单联动点击(可选项). 支持 左侧混合-顶部优先, 顶部混合-侧边优先, 顶部混合-顶部优先
  2. 修复 左侧混合-顶部优先 布局,tab 选中后无法激活左侧菜单
IINA 2025-12-03 22 59 39

Copilot AI review requested due to automatic review settings December 3, 2025 15:16
Copy link

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 adds a new autoSelectFirstMenu feature for hybrid layout modes that automatically navigates to the deepest child menu when clicking a first-level menu item. The feature is optional and can be toggled via a new setting in the theme drawer.

  • Adds autoSelectFirstMenu boolean setting to control auto-navigation behavior
  • Implements deepest menu navigation logic for three hybrid layout modes
  • Adds UI toggle with tooltip in theme settings drawer

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/typings/app.d.ts Adds type definitions for the new autoSelectFirstMenu setting and i18n keys
src/theme/settings.ts Sets default value of autoSelectFirstMenu to false
src/locales/langs/zh-cn.ts Adds Chinese translations for the setting label and tooltip
src/locales/langs/en-us.ts Adds English translations for the setting label and tooltip
src/layouts/modules/theme-drawer/modules/layout/modules/sider-settings.vue Adds UI toggle for the setting with tooltip, shown only for hybrid layouts
src/layouts/modules/global-menu/modules/vertical-hybrid-header-first.vue Implements conditional navigation logic based on setting; handles both auto-select and manual expansion cases
src/layouts/modules/global-menu/modules/top-hybrid-sidebar-first.vue Adds handleSelectMenu to trigger deepest menu navigation when setting is enabled
src/layouts/modules/global-menu/modules/top-hybrid-header-first.vue Adds handleSelectMenu to trigger deepest menu navigation when setting is enabled
src/layouts/modules/global-menu/context/index.ts Implements core logic: getDeepestLevelMenuKey to find deepest menu, activeDeepestLevelMenuKey to navigate to it, and updates route watcher to track child menus

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +141 to +144
// if there are child level menus, get the active second level menu key
if (hasChildLevelMenus.value) {
getActiveSecondLevelMenuKey();
}
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The condition if (hasChildLevelMenus.value) checks whether the currently active second-level menu (based on the old route) has children, before updating the active second-level menu key for the new route. This creates a dependency on stale state.

The getActiveSecondLevelMenuKey() should be called whenever the route changes, regardless of whether the previous menu had children. Consider removing the conditional check and always calling getActiveSecondLevelMenuKey() after getActiveFirstLevelMenuKey().

Suggested change
// if there are child level menus, get the active second level menu key
if (hasChildLevelMenus.value) {
getActiveSecondLevelMenuKey();
}
// always get the active second level menu key when the route changes
getActiveSecondLevelMenuKey();

Copilot uses AI. Check for mistakes.
Comment on lines +141 to +144
// if there are child level menus, get the active second level menu key
if (hasChildLevelMenus.value) {
getActiveSecondLevelMenuKey();
}
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The comment "if there are child level menus, get the active second level menu key" is misleading. The condition checks whether the OLD active second-level menu has children (using stale state), not whether we need to update the active second-level menu for the NEW route.

Update the comment to clarify this is checking for child level menus before updating, or remove the condition altogether as suggested in the related bug comment.

Suggested change
// if there are child level menus, get the active second level menu key
if (hasChildLevelMenus.value) {
getActiveSecondLevelMenuKey();
}
getActiveSecondLevelMenuKey();

Copilot uses AI. Check for mistakes.
function handleSelectMenu(key: RouteKey) {
handleSelectFirstLevelMenu(key);
// if there are second level menus, select the deepest one by default
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The comment states "if there are second level menus, select the deepest one by default", but this is inaccurate. The activeDeepestLevelMenuKey() function only navigates to the deepest menu when themeStore.sider.autoSelectFirstMenu is enabled. When disabled, the function does nothing.

Update the comment to accurately reflect that navigation depends on the autoSelectFirstMenu setting, e.g., "if there are second level menus and autoSelectFirstMenu is enabled, select the deepest one".

Suggested change
// if there are second level menus, select the deepest one by default
// if there are second level menus and autoSelectFirstMenu is enabled, select the deepest one

Copilot uses AI. Check for mistakes.
const deepestLevelMenuKey = getDeepestLevelMenuKey();
if (!deepestLevelMenuKey) return;

// select the deepest second level menu
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The comment "select the deepest second level menu" is misleading. The function actually navigates to the deepest menu at any level (could be second, third, fourth level, etc.), not specifically the second level.

Consider updating to "navigate to the deepest menu" or "select and navigate to the deepest level menu" for clarity.

Suggested change
// select the deepest second level menu
// select and navigate to the deepest level menu

Copilot uses AI. Check for mistakes.
function handleSelectMenu(key: RouteKey) {
handleSelectFirstLevelMenu(key);
// if there are second level menus, select the deepest one by default
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The comment states "if there are second level menus, select the deepest one by default", but this is inaccurate. The activeDeepestLevelMenuKey() function only navigates to the deepest menu when themeStore.sider.autoSelectFirstMenu is enabled. When disabled, the function does nothing.

Update the comment to accurately reflect that navigation depends on the autoSelectFirstMenu setting, e.g., "if there are second level menus and autoSelectFirstMenu is enabled, select the deepest one".

Suggested change
// if there are second level menus, select the deepest one by default
// if there are second level menus and autoSelectFirstMenu is enabled, select the deepest one

Copilot uses AI. Check for mistakes.
@honghuangdc honghuangdc merged commit 9401925 into soybeanjs:main Dec 4, 2025
7 checks passed
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