Repository navigation
feat: 通知模块自定义加强 - #7807
feat: 通知模块自定义加强#7807
Conversation
|
📝 WalkthroughWalkthroughAdds notification click handling and navigation across multiple layout components. Refactors the notification widget to emit click events and accept slot-based content/action renderers. Extends notification item types with arbitrary fields. Also exports the Download icon and VbenIconButton. Changes
Sequence DiagramsequenceDiagram
actor User
participant NotifComponent as Notification Component
participant LayoutHandler as Layout (handleClick)
participant Router as Router
participant Window as Window/Tab
User->>NotifComponent: Click notification item
NotifComponent->>LayoutHandler: emit onClick(item)
alt item.link exists
LayoutHandler->>LayoutHandler: navigateTo(link, query?, state?)
alt External link (http/https)
LayoutHandler->>Window: window.open(link, '_blank')
Window->>Window: Open in new tab
else Internal link
LayoutHandler->>Router: router.push({path, query, state})
Router->>Router: Navigate internally
end
else No link
LayoutHandler->>LayoutHandler: No action
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 (2)
apps/web-tdesign/src/layouts/basic.vue (1)
153-176: Consider extracting notification navigation into a shared helper.This handler is duplicated across multiple layout variants in this PR; centralizing it will prevent behavior drift and simplify future changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-tdesign/src/layouts/basic.vue` around lines 153 - 176, Extract the duplicated notification navigation logic (current functions handleClick and navigateTo) into a shared helper module and replace the per-layout copies with imports; create a utility (e.g., export function navigateToNotification or reuse navigateTo) that accepts (link: string, query?: Record<string, any>, state?: Record<string, any>, router?: Router) or uses the app router, preserves the NotificationItem type, handles external links with window.open and internal links with router.push({ path: link, query: query || {}, state }), export it from a central utils file and update each layout to import and call this helper from their click handlers instead of duplicating the logic so behavior stays consistent.packages/effects/layouts/src/widgets/notification/types.ts (1)
15-16: Avoidanyindex signature on a shared contract.
[key: string]: anydisables useful type checks and can mask typos. Preferunknown(or a typed generic extension) so consumers narrow intentionally.Type-safe alternative
interface NotificationItem { @@ - query?: Record<string, any>; - state?: Record<string, any>; + query?: Record<string, unknown>; + state?: Record<string, unknown>; /** 业务字段 */ - [key: string]: any; + [key: string]: unknown; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/layouts/src/widgets/notification/types.ts` around lines 15 - 16, Replace the unsafe index signature "[key: string]: any" in packages/effects/layouts/src/widgets/notification/types.ts with a type-safe alternative: either change it to "[key: string]: unknown" so callers must explicitly narrow values, or make the surrounding type generic (e.g., <T = unknown> and use "[key: string]: T") to allow typed extensions; update any usages that relied on implicit any to perform proper type narrowing/casting and add a short verification test or type assertion to confirm consumers still compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-tdesign/src/layouts/basic.vue`:
- Around line 165-168: The external-link branch uses window.open(link, '_blank')
which exposes window.opener; update the call that opens external URLs (the
window.open invocation that receives the variable link) to include the noopener
and noreferrer flags (e.g. pass the third argument or use a programmatic anchor
with rel="noopener noreferrer") so opened pages cannot access window.opener;
apply the same change wherever window.open(link, '_blank') appears in
layout/adapters to consistently protect external navigation.
In `@packages/effects/layouts/src/widgets/notification/notification.vue`:
- Around line 46-49: handleViewAll currently only emits 'viewAll' so if no
parent listener exists the button is a no-op; update handleViewAll to detect a
parent listener and perform a sensible fallback when none exists: inside the
function use getCurrentInstance() to check instance.vnode.props?.onViewAll (or
onView-all) and if present emit('viewAll') as before, otherwise call close() and
perform the default behavior (e.g. useRouter().push('/notifications') or another
app-wide fallback) so clicking still does something; reference handleViewAll,
emit('viewAll'), close(), getCurrentInstance(), and useRouter() when making the
change.
- Around line 84-86: The VbenScrollbar is currently shown when notifications is
falsy due to the condition v-if="!notifications || notifications.length > 0";
update that condition to only render the list when there are items (e.g., use
notifications && notifications.length > 0 or notifications?.length > 0) so
VbenScrollbar and the <ul> inside it are only shown when the notifications array
has items, allowing the existing v-else no-data state to display when
notifications is missing or empty; change the conditional on the VbenScrollbar
element (and keep the existing v-for on notifications and :key using item.id ??
item.title) accordingly.
---
Nitpick comments:
In `@apps/web-tdesign/src/layouts/basic.vue`:
- Around line 153-176: Extract the duplicated notification navigation logic
(current functions handleClick and navigateTo) into a shared helper module and
replace the per-layout copies with imports; create a utility (e.g., export
function navigateToNotification or reuse navigateTo) that accepts (link: string,
query?: Record<string, any>, state?: Record<string, any>, router?: Router) or
uses the app router, preserves the NotificationItem type, handles external links
with window.open and internal links with router.push({ path: link, query: query
|| {}, state }), export it from a central utils file and update each layout to
import and call this helper from their click handlers instead of duplicating the
logic so behavior stays consistent.
In `@packages/effects/layouts/src/widgets/notification/types.ts`:
- Around line 15-16: Replace the unsafe index signature "[key: string]: any" in
packages/effects/layouts/src/widgets/notification/types.ts with a type-safe
alternative: either change it to "[key: string]: unknown" so callers must
explicitly narrow values, or make the surrounding type generic (e.g., <T =
unknown> and use "[key: string]: T") to allow typed extensions; update any
usages that relied on implicit any to perform proper type narrowing/casting and
add a short verification test or type assertion to confirm consumers still
compile.
🪄 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: c77d4fc0-4ca3-46fa-91f8-9ce970badb30
📒 Files selected for processing (10)
apps/web-antd/src/layouts/basic.vueapps/web-antdv-next/src/layouts/basic.vueapps/web-ele/src/layouts/basic.vueapps/web-naive/src/layouts/basic.vueapps/web-tdesign/src/layouts/basic.vuepackages/@core/base/icons/src/lucide.tspackages/effects/common-ui/src/components/index.tspackages/effects/layouts/src/widgets/notification/notification.vuepackages/effects/layouts/src/widgets/notification/types.tsplayground/src/layouts/basic.vue
| if (link.startsWith('http://') || link.startsWith('https://')) { | ||
| // 外部链接，在新标签页打开 | ||
| window.open(link, '_blank'); | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the specific file
fd -t f 'basic.vue' | grep -E 'apps/web-tdesign'Repository: vbenjs/vue-vben-admin
Length of output: 103
🏁 Script executed:
# Read the file to see the context around lines 165-168
cat -n apps/web-tdesign/src/layouts/basic.vue | sed -n '155,180p'Repository: vbenjs/vue-vben-admin
Length of output: 738
🏁 Script executed:
# Search for all window.open calls in the codebase to see if this is a widespread issue
rg 'window\.open' apps/web-tdesign --type vue -B 2 -A 2Repository: vbenjs/vue-vben-admin
Length of output: 92
🏁 Script executed:
# Search for window.open calls without file type restriction
rg 'window\.open' apps/web-tdesign -B 2 -A 2Repository: vbenjs/vue-vben-admin
Length of output: 423
🏁 Script executed:
# Also check if there are similar patterns in other layout files
rg 'window\.open' --type-list | head -20Repository: vbenjs/vue-vben-admin
Length of output: 454
🏁 Script executed:
# Search more broadly for window.open in the entire codebase
rg 'window\.open\s*\(' -B 1 -A 1Repository: vbenjs/vue-vben-admin
Length of output: 3120
Protect external navigation from opener abuse with noopener,noreferrer.
window.open(link, '_blank') exposes window.opener to the opened page, creating a security vulnerability. Add the noopener,noreferrer flags to prevent opener access.
Corrected implementation
- window.open(link, '_blank');
+ const newWindow = window.open(link, '_blank', 'noopener,noreferrer');
+ if (newWindow) {
+ newWindow.opener = null;
+ }Note: This same pattern appears across multiple layout and adapter files in the codebase and should be addressed consistently.
📝 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.
| if (link.startsWith('http://') || link.startsWith('https://')) { | |
| // 外部链接，在新标签页打开 | |
| window.open(link, '_blank'); | |
| } else { | |
| if (link.startsWith('http://') || link.startsWith('https://')) { | |
| // 外部链接，在新标签页打开 | |
| const newWindow = window.open(link, '_blank', 'noopener,noreferrer'); | |
| if (newWindow) { | |
| newWindow.opener = null; | |
| } | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-tdesign/src/layouts/basic.vue` around lines 165 - 168, The
external-link branch uses window.open(link, '_blank') which exposes
window.opener; update the call that opens external URLs (the window.open
invocation that receives the variable link) to include the noopener and
noreferrer flags (e.g. pass the third argument or use a programmatic anchor with
rel="noopener noreferrer") so opened pages cannot access window.opener; apply
the same change wherever window.open(link, '_blank') appears in layout/adapters
to consistently protect external navigation.
| const handleViewAll = () => { | ||
| emit('viewAll'); | ||
| close(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
viewAll became emit-only; one known consumer currently has a no-op button.
This refactor makes “View all” fully dependent on parent wiring. In the provided snippet apps/web-antd/src/layouts/basic.vue:185-191, <Notification> has no @view-all listener, so clicking the button does nothing in that app variant. Please align all consumers with the new contract (or provide a temporary fallback path).
Also applies to: 165-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects/layouts/src/widgets/notification/notification.vue` around
lines 46 - 49, handleViewAll currently only emits 'viewAll' so if no parent
listener exists the button is a no-op; update handleViewAll to detect a parent
listener and perform a sensible fallback when none exists: inside the function
use getCurrentInstance() to check instance.vnode.props?.onViewAll (or
onView-all) and if present emit('viewAll') as before, otherwise call close() and
perform the default behavior (e.g. useRouter().push('/notifications') or another
app-wide fallback) so clicking still does something; reference handleViewAll,
emit('viewAll'), close(), getCurrentInstance(), and useRouter() when making the
change.
| <VbenScrollbar v-if="!notifications || notifications.length > 0"> | ||
| <ul class="flex! max-h-90 w-full flex-col"> | ||
| <template v-for="item in notifications" :key="item.id ?? item.title"> |
There was a problem hiding this comment.
Fix the empty-state condition to avoid blank panel rendering.
At Line 84, v-if="!notifications || notifications.length > 0" renders the list container when notifications is falsy, which skips the v-else no-data state and can show an empty panel.
Proposed fix
- <VbenScrollbar v-if="!notifications || notifications.length > 0">
+ <VbenScrollbar v-if="notifications && notifications.length > 0">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects/layouts/src/widgets/notification/notification.vue` around
lines 84 - 86, The VbenScrollbar is currently shown when notifications is falsy
due to the condition v-if="!notifications || notifications.length > 0"; update
that condition to only render the list when there are items (e.g., use
notifications && notifications.length > 0 or notifications?.length > 0) so
VbenScrollbar and the <ul> inside it are only shown when the notifications array
has items, allowing the existing v-else no-data state to display when
notifications is missing or empty; change the conditional on the VbenScrollbar
element (and keep the existing v-for on notifications and :key using item.id ??
item.title) accordingly.
There was a problem hiding this comment.
Notifications default value is array
There was a problem hiding this comment.
@doraemonxxx, you're absolutely right! Since notifications is declared with withDefaults using notifications: () => [], it will never be undefined or falsy. That means !notifications is always false, so the condition !notifications || notifications.length > 0 effectively reduces to notifications.length > 0 — which behaves correctly and the v-else no-data state will still display when the array is empty. My concern was unfounded. Apologies for the noise! 🙏
(ᴗ_ ᴗ。)
|
Hi. You can remove the checking |
通知模块增加样式自定义 & 基础模块只做包装与暴漏，具体实现交由实际app
Summary by CodeRabbit
New Features
Chores