feat: auto activate subMenu on select root menu #5147
Conversation
|
Warning Rate limit exceeded@mynetfan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughThis pull request introduces a new feature for sidebar menu navigation by adding an Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mynetfan
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: 3
🧹 Nitpick comments (8)
packages/effects/layouts/src/widgets/preferences/blocks/layout/sidebar.vue (1)
30-36: Consider adding aria-label for better accessibilityThe expand-on-hover switch implementation looks good, but could benefit from enhanced accessibility.
<SwitchItem v-model="SidebarExpandOnHover" :disabled="!sidebarEnable || disabled || !sidebarCollapsed" :tip="$t('preferences.sidebar.expandOnHoverTip')" + :aria-label="$t('preferences.sidebar.expandOnHover')" > {{ $t('preferences.sidebar.expandOnHover') }} </SwitchItem>packages/effects/layouts/src/basic/menu/use-extra-menu.ts (3)
18-19: Consider adding JSDoc for the defaultSubMapThe Chinese comment should be translated to English and expanded into proper JSDoc for better maintainability.
-/** 记录当前顶级菜单下哪个子菜单最后激活 */ +/** + * Maps root menu paths to their last activated submenu paths + * Used to restore submenu selection state when returning to a root menu + * @type {Map<string, string>} + */
38-43: Consider memory management for defaultSubMapThe implementation should consider clearing old entries from
defaultSubMapto prevent memory leaks over time, especially in long-running sessions.} else if (preferences.sidebar.autoActivateChild) { + // Prevent unlimited growth of the map + if (defaultSubMap.size > 100) { + const oldestKey = defaultSubMap.keys().next().value; + defaultSubMap.delete(oldestKey); + } await navigation( defaultSubMap.has(menu.path) ? (defaultSubMap.get(menu.path) as string) : menu.path, ); }
38-43: Consider race conditions in navigationThe async navigation and map updates could potentially lead to race conditions. Consider implementing a guard mechanism.
Suggestions:
- Add a navigation lock to prevent concurrent navigation attempts
- Consider using a more robust state management solution for menu state
- Implement proper error handling for navigation failures
Also applies to: 101-101
packages/locales/src/langs/zh-CN/preferences.json (1)
51-52: Consider improving tooltip readabilityThe tooltip text uses backticks inconsistently and could benefit from better formatting.
Consider this improvement:
- "expandOnHoverTip": "鼠标在折叠区域悬浮时，`启用`则展开当前子菜单，`禁用`则展开整个侧边栏" + "expandOnHoverTip": "鼠标在折叠区域悬浮时：\n- 启用：展开当前子菜单\n- 禁用：展开整个侧边栏"packages/locales/src/langs/en-US/preferences.json (2)
49-50: Improve grammar in tooltip textThe tooltip text could be more natural in English.
Consider this improvement:
- "autoActivateChildTip": "`Enabled` to automatically activate the submenu while click menu.", + "autoActivateChildTip": "When enabled, automatically activates the submenu when clicking a menu item."
51-52: Enhance tooltip formatting consistencyThe tooltip uses a mix of newlines and formatting styles. Let's make it consistent with other tooltips.
Consider this improvement:
- "expandOnHoverTip": "When the mouse hovers over menu, \n `Enabled` to expand children menus \n `Disabled` to expand whole sidebar." + "expandOnHoverTip": "When the mouse hovers over a collapsed menu:\n- Enabled: Expands only the current submenu\n- Disabled: Expands the entire sidebar"packages/@core/preferences/src/types.ts (1)
128-129: Enhance JSDoc documentationWhile the comment is in Chinese, it would be beneficial to add more comprehensive documentation including the default behavior.
Consider this improvement:
- /** 点击目录时自动激活子菜单 */ + /** + * Automatically activates child menu items when clicking a directory + * @description When enabled, clicking a menu item will activate either: + * - The last activated submenu, or + * - The first submenu if none was previously activated + * @default false + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/@core/preferences/src/config.ts(1 hunks)packages/@core/preferences/src/types.ts(1 hunks)packages/effects/layouts/src/basic/menu/use-extra-menu.ts(3 hunks)packages/effects/layouts/src/widgets/preferences/blocks/layout/sidebar.vue(2 hunks)packages/effects/layouts/src/widgets/preferences/preferences-drawer.vue(2 hunks)packages/locales/src/langs/en-US/preferences.json(1 hunks)packages/locales/src/langs/zh-CN/preferences.json(1 hunks)
🔇 Additional comments (6)
packages/effects/layouts/src/widgets/preferences/blocks/layout/sidebar.vue (1)
9-9: LGTM: Props and models are well-defined
The new props and models are correctly typed and follow Vue's composition API best practices:
currentLayoutis properly typed as optionalLayoutType- Models use the new
defineModelsyntax for two-way binding
Also applies to: 16-18, 20-20
packages/@core/preferences/src/config.ts (1)
68-68: LGTM: Safe default configuration
The autoActivateChild preference is correctly added with a safe default value of false. This ensures backward compatibility and prevents unexpected behavior for existing users.
packages/effects/layouts/src/basic/menu/use-extra-menu.ts (1)
101-101: 🛠️ Refactor suggestion
Verify path existence before storing in Map
The implementation should verify that the current path exists in the menu structure before storing it.
-if (rootMenuPath) defaultSubMap.set(rootMenuPath, currentPath);
+if (rootMenuPath && findMenu) {
+ defaultSubMap.set(rootMenuPath, currentPath);
+}packages/locales/src/langs/zh-CN/preferences.json (1)
49-50: LGTM: Clear and concise translation for auto-activate feature
The translation accurately reflects the feature's purpose and maintains consistency with the UI terminology.
packages/@core/preferences/src/types.ts (1)
128-129: Verify the property placement
The new property is appropriately placed in the SidebarPreferences interface and correctly typed as boolean.
Let's verify this is the only interface that needs updating:
✅ Verification successful
Property placement is correct and properly integrated
The autoActivateChild property is correctly placed in the SidebarPreferences interface and is well integrated across the codebase:
- Default value is set in
packages/@core/preferences/src/config.ts - Localization strings exist in both en-US and zh-CN locale files
- Property is actively used in the sidebar menu logic (
packages/effects/layouts/src/basic/menu/use-extra-menu.ts) - UI implementation exists in the sidebar preferences component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other preference interfaces that might need the autoActivateChild property
ast-grep --pattern 'interface $INTERFACE {
$$$
activate$_: $_
$$$
}'
Length of output: 76
Script:
#!/bin/bash
# Let's search for any interfaces or types that might be related to sidebar preferences
rg -l "interface.*Sidebar|type.*Sidebar" --type typescript
# Also search for any usage of autoActivateChild
rg "autoActivateChild" --type typescript
Length of output: 171
Script:
#!/bin/bash
# Let's search for any interfaces or types that might be related to sidebar preferences
rg -l "interface.*Sidebar|type.*Sidebar"
# Also search for any usage of autoActivateChild
rg "autoActivateChild"
Length of output: 1145
packages/effects/layouts/src/widgets/preferences/preferences-drawer.vue (1)
306-312: LGTM! Template changes follow Vue.js best practices
The new v-model bindings are properly implemented with:
- Consistent kebab-case prop naming in template
- Appropriate conditional disabling based on layout
- Alphabetical prop ordering
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects/layouts/src/widgets/preferences/blocks/layout/sidebar.vue(2 hunks)packages/effects/layouts/src/widgets/preferences/preferences-drawer.vue(2 hunks)
🔇 Additional comments (4)
packages/effects/layouts/src/widgets/preferences/blocks/layout/sidebar.vue (2)
2-2: LGTM: Props definition looks good
The addition of the optional currentLayout prop of type LayoutType is well-structured and follows Vue's type system conventions.
Also applies to: 9-9
30-36: LGTM: Expand on hover implementation
The implementation of the expand-on-hover feature is well-structured with appropriate disabled state handling based on sidebar state.
packages/effects/layouts/src/widgets/preferences/preferences-drawer.vue (2)
90-93: Maintain consistent naming convention
The same naming inconsistency exists in this file.
306-312: LGTM: Sidebar component integration
The integration of the new preferences with the Sidebar component is well-structured:
- Proper v-model bindings
- Correct prop passing for layout-dependent features
packages/effects/layouts/src/widgets/preferences/blocks/layout/sidebar.vue
Outdated
Show resolved
Hide resolved
packages/effects/layouts/src/widgets/preferences/blocks/layout/sidebar.vue
Show resolved
Hide resolved
Description
双列菜单布局下，点击顶级菜单可以自动激活上一次激活的子菜单。如果所有子菜单都没有被激活过，自动激活第一个子菜单。
以上功能可在偏好设置中开关。
close #4430
另外，补充一个侧边栏偏好设置：鼠标悬停展开
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Localization
Bug Fixes