Repository navigation
fix: 修复在vue-router 的模式hash复制路径不对 - #7667
Conversation
|
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced the top-level Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/`@core/ui-kit/menu-ui/src/components/menu-item.vue:
- Around line 90-93: The router-link :to binding is using
item.parentPaths.at(-1) (the parent's path) instead of the menu item's own path;
update the :to expression in menu-item.vue to use the menu item's path
(item.path or props.path as defined around line 58) and still append the query
via qs.stringify(item?.query) so the final URL is (item.path OR props.path) +
(item?.query ? `?${qs.stringify(item?.query)}` : ''); ensure you replace
references to item.parentPaths.at(-1) with item.path (or props.path)
consistently.
- Around line 95-134: The template currently nests <li> inside an <a> (see the
anchor using :href and `@click.prevent.stop`="handleClick"), which is invalid
HTML; change the structure so the <li> is the outer element and contains the
link element (router-link or <a>) inside it, move the :href and `@click` handler
(handleClick) onto the inner link, and ensure all children like VbenTooltip,
MenuBadge, VbenIcon, and slots remain within the <li>'s content but nested under
the link where appropriate; keep role="menuitem" and class bindings (b(),
is(...), rootMenu.theme, nsMenu usage) on the <li> and preserve conditional
rendering (showTooltip, collapseShowTitle) and props passed to MenuBadge.
- Around line 87-95: The current template always uses <router-link>, which is
incorrect for external URLs; compute the link target (the concatenation of
(item.parentPaths.at(-1) ?? '') and the query string from item.query) and
conditionally render either a plain <a> (for external URLs detected via
isHttpUrl from use-navigation.ts) or the existing <router-link> (for internal
routes); keep the same `@click.prevent.stop`="handleClick" behavior, import/use
isHttpUrl and ensure the external branch binds href to the full URL while the
internal branch uses :to as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 457703fd-bb27-4327-8dc0-a31bc2fd0d85
📒 Files selected for processing (1)
packages/@core/ui-kit/menu-ui/src/components/menu-item.vue
| <router-link | ||
| v-slot="{ href }" | ||
| custom | ||
| :to=" | ||
| (item.parentPaths.at(-1) ?? '') + | ||
| (item?.query ? `?${qs.stringify(item?.query)}` : '') | ||
| " | ||
| @click.prevent.stop="handleClick" | ||
| > | ||
| <li | ||
| :class="[ | ||
| rootMenu.theme, | ||
| b(), | ||
| is('active', active), | ||
| is('disabled', disabled), | ||
| is('collapse-show-title', collapseShowTitle), | ||
| ]" | ||
| role="menuitem" | ||
| > | ||
| <!-- --> | ||
| <VbenTooltip | ||
| v-if="showTooltip" | ||
| :content-class="[rootMenu.theme]" | ||
| side="right" | ||
| <a :href="href" @click.prevent.stop="handleClick"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if external URLs are used in menu configurations
rg -n 'https?://' --glob '*menu*' --glob '!node_modules'
# Check for any isExternal or isExternalLink utility functions
rg -n 'isExternal|externalLink|isHttp' --type ts --type vue
# Look for menu path definitions to understand path formats
ast-grep --pattern 'path: $_'Repository: vbenjs/vue-vben-admin
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Find the menu-item.vue file and examine it
find . -name "menu-item.vue" -type f | head -5
# Search for MenuItemProps interface definition
rg -n "MenuItemProps|interface.*Menu.*Prop" --type ts
# Look for external URL handling patterns in menu system
rg -n "http|external|isHttp" -i --type ts --type js | grep -i menu | head -20
# Check the actual menu-item.vue file location
find . -path "*/menu-ui/src/components/menu-item.vue"Repository: vbenjs/vue-vben-admin
Length of output: 1036
🏁 Script executed:
#!/bin/bash
# Read the menu-item.vue file to understand full context
cat -n packages/@core/ui-kit/menu-ui/src/components/menu-item.vue | head -150Repository: vbenjs/vue-vben-admin
Length of output: 4455
🏁 Script executed:
#!/bin/bash
# Search for MenuItemProps definition
rg -n "export.*interface.*MenuItemProps|type.*MenuItemProps" -A 10Repository: vbenjs/vue-vben-admin
Length of output: 4489
🏁 Script executed:
#!/bin/bash
# View MenuItemProps interface definition
cat -n packages/@core/ui-kit/menu-ui/src/types.ts | sed -n '81,120p'Repository: vbenjs/vue-vben-admin
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# View use-navigation.ts to understand external URL handling pattern
cat -n packages/effects/layouts/src/basic/menu/use-navigation.tsRepository: vbenjs/vue-vben-admin
Length of output: 2367
🏁 Script executed:
#!/bin/bash
# Check if there's an isHttpUrl function and understand its implementation
rg -n "export.*isHttpUrl|function isHttpUrl" -A 3Repository: vbenjs/vue-vben-admin
Length of output: 356
🏁 Script executed:
#!/bin/bash
# Check the rootMenu context and how handleMenuItemClick is implemented
rg -n "handleMenuItemClick" -A 5 -B 2Repository: vbenjs/vue-vben-admin
Length of output: 2681
🏁 Script executed:
#!/bin/bash
# Look for the Menu component implementation to understand rootMenu context
find . -name "*.vue" -o -name "*.ts" | xargs rg -l "handleMenuItemClick.*=" | head -5Repository: vbenjs/vue-vben-admin
Length of output: 109
🏁 Script executed:
#!/bin/bash
# View the Menu component to see how it handles menu item clicks
cat -n packages/@core/ui-kit/menu-ui/src/components/menu.vue | head -200Repository: vbenjs/vue-vben-admin
Length of output: 6264
🏁 Script executed:
#!/bin/bash
# View more of menu.vue handleMenuItemClick to see if it delegates to navigation
cat -n packages/@core/ui-kit/menu-ui/src/components/menu.vue | sed -n '247,260p'Repository: vbenjs/vue-vben-admin
Length of output: 526
🏁 Script executed:
#!/bin/bash
# Search for any context where menu-item.vue is used to see how navigation is expected to work
rg -n "Menu|MenuItem" packages/effects/layouts/src/basic/menu/ -A 2 -B 2 | head -50Repository: vbenjs/vue-vben-admin
Length of output: 4316
🏁 Script executed:
#!/bin/bash
# Search for how the Menu component's 'select' event is handled
rg -n "@select|@.*select" packages/effects/layouts/src/basic/menu/ -A 3Repository: vbenjs/vue-vben-admin
Length of output: 728
🏁 Script executed:
#!/bin/bash
# Find where Menu component is used and how it integrates with navigation
rg -n "Menu|select" packages/effects/layouts/src/basic/layout.vue -A 2 -B 2 | head -80Repository: vbenjs/vue-vben-admin
Length of output: 1949
🏁 Script executed:
#!/bin/bash
# Check if there's a layout component that uses the menu
find packages/effects/layouts -name "*.vue" | xargs grep -l "Menu" | head -5Repository: vbenjs/vue-vben-admin
Length of output: 322
🏁 Script executed:
#!/bin/bash
# View the handleMenuSelect function in menu.vue
cat -n packages/effects/layouts/src/basic/menu/menu.vueRepository: vbenjs/vue-vben-admin
Length of output: 1304
🏁 Script executed:
#!/bin/bash
# View the handleMenuSelect in useMixedMenu
cat -n packages/effects/layouts/src/basic/menu/use-mixed-menu.ts | grep -A 20 "handleMenuSelect"Repository: vbenjs/vue-vben-admin
Length of output: 1210
🏁 Script executed:
#!/bin/bash
# Look for the complete implementation of handleMenuSelect and related functions
cat -n packages/effects/layouts/src/basic/menu/use-mixed-menu.ts | sed -n '1,150p'Repository: vbenjs/vue-vben-admin
Length of output: 5190
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or examples showing external URLs in menu paths
rg -n "http|external" packages/@core/ui-kit/menu-ui/ -A 2 -B 2Repository: vbenjs/vue-vben-admin
Length of output: 1106
🏁 Script executed:
#!/bin/bash
# Search for how MenuRecordRaw is defined and if it has link/external properties
rg -n "interface MenuRecordRaw|type MenuRecordRaw" -A 20Repository: vbenjs/vue-vben-admin
Length of output: 1403
🏁 Script executed:
#!/bin/bash
# View complete MenuRecordRaw interface definition
cat -n packages/@core/base/typings/src/menu-record.tsRepository: vbenjs/vue-vben-admin
Length of output: 1775
🏁 Script executed:
#!/bin/bash
# Check the router-link behavior with external URLs and verify if there's any special handling
rg -n "router-link|routerLink" packages/@core/ui-kit/menu-ui/src/components/ -A 3 -B 1Repository: vbenjs/vue-vben-admin
Length of output: 673
🏁 Script executed:
#!/bin/bash
# Look for any conditional rendering based on external URLs in the codebase
rg -n "isHttpUrl|isExternal" packages/ -l | head -10Repository: vbenjs/vue-vben-admin
Length of output: 366
Use conditional rendering to handle external URLs correctly in menu items.
router-link is designed for internal routes and cannot properly handle external URLs (e.g., http://, https://) in the :to prop. While navigation currently works through handleClick delegation to the useNavigation hook, using router-link with external paths is incorrect and confusing.
The system supports external URLs (as evidenced by isHttpUrl() checks in use-navigation.ts), so this should be handled explicitly with conditional rendering:
- Use a plain
<a>tag for external URLs - Use
router-linkonly for internal routes
The .stop modifier on the click handler is appropriate and prevents unintended event bubbling to parent menu items.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/`@core/ui-kit/menu-ui/src/components/menu-item.vue around lines 87 -
95, The current template always uses <router-link>, which is incorrect for
external URLs; compute the link target (the concatenation of
(item.parentPaths.at(-1) ?? '') and the query string from item.query) and
conditionally render either a plain <a> (for external URLs detected via
isHttpUrl from use-navigation.ts) or the existing <router-link> (for internal
routes); keep the same `@click.prevent.stop`="handleClick" behavior, import/use
isHttpUrl and ensure the external branch binds href to the full URL while the
internal branch uses :to as before.
| :to=" | ||
| (item.parentPaths.at(-1) ?? '') + | ||
| (item?.query ? `?${qs.stringify(item?.query)}` : '') | ||
| " |
There was a problem hiding this comment.
Critical: Wrong path used in router-link binding.
The :to binding uses item.parentPaths.at(-1) which retrieves the parent's path, not the menu item's own path. Based on line 58, the menu item's path is stored in item.path (or props.path). This will cause navigation to the wrong route.
🐛 Proposed fix
:to="
- (item.parentPaths.at(-1) ?? '') +
+ props.path +
(item?.query ? `?${qs.stringify(item?.query)}` : '')
"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/`@core/ui-kit/menu-ui/src/components/menu-item.vue around lines 90 -
93, The router-link :to binding is using item.parentPaths.at(-1) (the parent's
path) instead of the menu item's own path; update the :to expression in
menu-item.vue to use the menu item's path (item.path or props.path as defined
around line 58) and still append the query via qs.stringify(item?.query) so the
final URL is (item.path OR props.path) + (item?.query ?
`?${qs.stringify(item?.query)}` : ''); ensure you replace references to
item.parentPaths.at(-1) with item.path (or props.path) consistently.
| <a :href="href" @click.prevent.stop="handleClick"> | ||
| <li | ||
| :class="[ | ||
| rootMenu.theme, | ||
| b(), | ||
| is('active', active), | ||
| is('disabled', disabled), | ||
| is('collapse-show-title', collapseShowTitle), | ||
| ]" | ||
| role="menuitem" | ||
| > | ||
| <template #trigger> | ||
| <div :class="[nsMenu.be('tooltip', 'trigger')]"> | ||
| <VbenIcon :class="nsMenu.e('icon')" :icon="menuIcon" fallback /> | ||
| <slot></slot> | ||
| <span v-if="collapseShowTitle" :class="nsMenu.e('name')"> | ||
| <slot name="title"></slot> | ||
| </span> | ||
| </div> | ||
| </template> | ||
| <slot name="title"></slot> | ||
| </VbenTooltip> | ||
| <div v-show="!showTooltip" :class="[e('content')]"> | ||
| <MenuBadge | ||
| v-if="rootMenu.props.mode !== 'horizontal'" | ||
| class="right-2" | ||
| v-bind="props" | ||
| /> | ||
| <VbenIcon :class="nsMenu.e('icon')" :icon="menuIcon" /> | ||
| <slot></slot> | ||
| <slot name="title"></slot> | ||
| </div> | ||
| </li> | ||
| </a> | ||
| <!-- --> | ||
| <VbenTooltip | ||
| v-if="showTooltip" | ||
| :content-class="[rootMenu.theme]" | ||
| side="right" | ||
| > | ||
| <template #trigger> | ||
| <div :class="[nsMenu.be('tooltip', 'trigger')]"> | ||
| <VbenIcon :class="nsMenu.e('icon')" :icon="menuIcon" fallback /> | ||
| <slot></slot> | ||
| <span v-if="collapseShowTitle" :class="nsMenu.e('name')"> | ||
| <slot name="title"></slot> | ||
| </span> | ||
| </div> | ||
| </template> | ||
| <slot name="title"></slot> | ||
| </VbenTooltip> | ||
| <div v-show="!showTooltip" :class="[e('content')]"> | ||
| <MenuBadge | ||
| v-if="rootMenu.props.mode !== 'horizontal'" | ||
| class="right-2" | ||
| v-bind="props" | ||
| /> | ||
| <VbenIcon :class="nsMenu.e('icon')" :icon="menuIcon" /> | ||
| <slot></slot> | ||
| <slot name="title"></slot> | ||
| </div> | ||
| </li> | ||
| </a> |
There was a problem hiding this comment.
Invalid HTML: <a> should not contain <li>.
The current structure places <li> inside <a>, which violates HTML semantics. Per spec, <li> must be a direct child of <ul> or <ol>. This can cause accessibility issues with screen readers and may result in unexpected rendering behavior.
The structure should be inverted: <li> wrapping <router-link> and <a>.
🛠️ Proposed fix
- <router-link
- v-slot="{ href }"
- custom
- :to="
- (item.parentPaths.at(-1) ?? '') +
- (item?.query ? `?${qs.stringify(item?.query)}` : '')
- "
- >
- <a :href="href" `@click.prevent.stop`="handleClick">
- <li
- :class="[
- rootMenu.theme,
- b(),
- is('active', active),
- is('disabled', disabled),
- is('collapse-show-title', collapseShowTitle),
- ]"
- role="menuitem"
- >
+ <li
+ :class="[
+ rootMenu.theme,
+ b(),
+ is('active', active),
+ is('disabled', disabled),
+ is('collapse-show-title', collapseShowTitle),
+ ]"
+ role="menuitem"
+ >
+ <router-link
+ v-slot="{ href }"
+ custom
+ :to="props.path + (item?.query ? `?${qs.stringify(item?.query)}` : '')"
+ >
+ <a :href="href" `@click.prevent`="handleClick">
<!-- tooltip and content sections remain the same -->
- </li>
- </a>
- </router-link>
+ </a>
+ </router-link>
+ </li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/`@core/ui-kit/menu-ui/src/components/menu-item.vue around lines 95 -
134, The template currently nests <li> inside an <a> (see the anchor using :href
and `@click.prevent.stop`="handleClick"), which is invalid HTML; change the
structure so the <li> is the outer element and contains the link element
(router-link or <a>) inside it, move the :href and `@click` handler (handleClick)
onto the inner link, and ensure all children like VbenTooltip, MenuBadge,
VbenIcon, and slots remain within the <li>'s content but nested under the link
where appropriate; keep role="menuitem" and class bindings (b(), is(...),
rootMenu.theme, nsMenu usage) on the <li> and preserve conditional rendering
(showTooltip, collapseShowTitle) and props passed to MenuBadge.
|
如何复现 |
|
你直接在演示网站右键复制会少一个#，没有开启404页面重置跟目录就会打不开页面 |
- 引入 isHttpUrl 工具函数用于判断 URL 类型 - 添加 isHttp 计算属性检测父路径是否为 HTTP 链接 - 修改菜单项渲染逻辑，对外部链接直接使用原地址跳转 - 调整 HTML 结构，将链接属性移到正确位置 - 确保内部路由和外部链接都能正常工作
Description
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