Conversation
1. 按钮 -> Tooltip 2. DeviceSnapshotGroups 被“去业务化” 3. useDevicePlus 删除两个功能 4. HomeSnapshotGroups 删除重复“查看” 5. useHomePlus 6. Router 清理
📝 WalkthroughWalkthrough将 Changes快照操作 Prop 上提
Plus 包装页删除与路由精简
测试基础设施与合约测试更新
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5a0e66443
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ActionCard.vue`:
- Around line 108-116: The `<a>` tag with `target="_blank"` is missing the
`rel="noopener noreferrer"` attribute, which creates a security vulnerability
allowing the new page to access the original page through `window.opener`
(reverse tabnabbing attack). Add `rel="noopener noreferrer"` to the anchor tag
that opens the preview URL to prevent this security risk.
In `@src/components/plus/home/HomeSnapshotGroups.vue`:
- Around line 318-322: The ActionCard component in the HomeSnapshotGroups.vue
file has a default showPreview=true, which re-exposes the view/jump feature that
was meant to be removed as part of this change. Add an explicit
:showPreview="false" prop to the ActionCard component (lines 318-322) to ensure
the preview entrance is disabled and consistent with the goal of removing the
jump capability.
In `@src/test/setup.ts`:
- Around line 178-184: The mock for useEventListener in the vi.mock call for
`@vueuse/core` is currently returning undefined by default, but the real API
returns a cleanup function. Update the useEventListener mock to return a no-op
function (the stop function) instead of undefined, so that if tested code calls
the return value expecting a cleanup function, it will receive a valid function
instead of throwing an error. Change useEventListener from vi.fn() to vi.fn(()
=> vi.fn()) to maintain API contract consistency with the actual `@vueuse/core`
behavior.
- Around line 87-155: The component stubs in this file (NCheckbox, NInput, and
NSelect) are using the modelValue/update:modelValue two-way binding pattern,
which does not match the actual Naive UI v2.x API contracts. Update the
NCheckbox stub to use a checked property and emit update:checked events instead
of modelValue. Update the NInput stub to use a value property and emit
update:value events instead of modelValue. Update the NSelect stub to use a
value property and emit update:value events instead of modelValue. Ensure all
prop definitions, emits declarations, and onChange handlers are aligned with
their respective event names to match the production Naive UI component
behavior.
In `@src/test/utils.ts`:
- Around line 122-130: The `mountWithStubs` function has an option merge order
issue where spreading `...options` at the end overwrites the previously
constructed `global.stubs` object. When a caller provides `options.global`, the
default stubs for `NTooltip/NButton/SvgIcon` are lost. To fix this, restructure
the merge logic by first spreading `options`, then explicitly constructing the
`global` property to deeply merge user-provided stubs with the default stubs,
ensuring that default stubs like `NTooltip`, `NButton`, and `SvgIcon` are
preserved when users pass their own `global.stubs`.
- Around line 39-41: The custom render branch in the createStubComponent setup()
function is directly returning the result of options.render(renderProps) as a
VNode, but Vue 3's setup() must return a render function, not a VNode directly.
This causes reactive state changes to not trigger re-renders. Fix this by
wrapping the options.render(renderProps) call in a function (e.g., an arrow
function) so that setup() returns a render function instead of a VNode, making
it consistent with the default render function logic in the subsequent else
branch.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c976d11c-fc7b-4ebf-9eac-e03453ab3d02
⛔ Files ignored due to path filters (2)
package.jsonis excluded by none and included by nonevitest.config.tsis excluded by none and included by none
📒 Files selected for processing (14)
src/components/ActionCard.vuesrc/components/plus/device/DeviceSnapshotGroups.vuesrc/components/plus/home/HomeSnapshotGroups.vuesrc/composables/plus/useDevicePlus.tssrc/composables/plus/useHomePlus.tssrc/router.tssrc/test/setup.tssrc/test/utils.tssrc/types/plus/index.d.tssrc/views/DevicePage.vuesrc/views/plus/DevicePage.vuesrc/views/plus/SelectorPage.vuesrc/views/plus/SvgPage.vuesrc/views/plus/__tests__/adapter.contract.test.ts
💤 Files with no reviewable changes (4)
- src/views/plus/SvgPage.vue
- src/views/plus/SelectorPage.vue
- src/composables/plus/useHomePlus.ts
- src/composables/plus/useDevicePlus.ts
📜 Review details
🔇 Additional comments (6)
src/components/plus/device/DeviceSnapshotGroups.vue (1)
11-18: LGTM!Also applies to: 239-253, 302-303
src/views/DevicePage.vue (1)
537-538: LGTM!src/views/plus/DevicePage.vue (1)
68-74: LGTM!Also applies to: 80-81
src/router.ts (1)
100-100: LGTM!Also applies to: 105-105
src/types/plus/index.d.ts (1)
10-10: LGTM!src/views/plus/__tests__/adapter.contract.test.ts (1)
4-4: LGTM!Also applies to: 14-19, 39-44, 47-48, 65-67, 84-92, 107-107, 117-130, 150-150, 161-161, 174-174
target="_blank" 缺少 rel，存在反向标签劫持风险。
- 修正 NCheckbox 测试桩为 checked/update:checked - 修正 NInput、NSelect 测试桩为 value/update:value - 修复 createStubComponent 返回值错误 - 为 useEventListener 补充清理函数模拟 - 优化 mountWithStubs 的 global 合并逻辑 - 在测试桩中放宽类型以兼容 Vue 3.5 的 slots 类型限制
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/setup.ts`:
- Around line 47-52: The NModal stub component in the setup function declares a
`show` prop with default false but never uses it in the render logic, causing
the modal content to always render regardless of the prop value. This differs
from the real naive-ui NModal behavior where show=false should hide content,
allowing visibility-related bugs to go undetected in tests. To fix, modify the
setup function to accept the props parameter (instead of ignoring it with _:
unknown), then conditionally render the modal div and its content only when
props.show is true, ensuring the stub behaves consistently with the production
NModal component.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0e1667b-7176-4394-b62a-5def0f8ac5d4
📒 Files selected for processing (3)
src/components/ActionCard.vuesrc/test/setup.tssrc/test/utils.ts
📜 Review details
🔇 Additional comments (2)
src/test/utils.ts (1)
20-33: LGTM!Also applies to: 105-114
src/components/ActionCard.vue (1)
110-110: LGTM!
当一个被选中的快照被删除后，checkedRowKeys 仍然包含已删除的 ID，导致批量操作栏显示过时的计数，后续批量删除可能尝试删除已不存在的快照
fix: 根据 props.show 的值来决定是否渲染
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests