Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 4 个问题，并给出了一些整体反馈：
- 在
resolveBinding中，你有时会构造{ component: result }/{ component: binding, props: {} }，但在其他地方（以及Tooltip.vue中）代码期望的是compData.comp，所以这些分支应该设置comp而不是component，以避免在运行时向<component :is="compData.comp">传入undefined。 UserInfoTooltip.vue在 setup 阶段只执行了一次把userProp解析为userInfo（以及titleClass）的逻辑，所以之后如果user这个 prop 发生变化，tooltip 不会更新；建议基于userProp使用一个computed（并基于它派生titleClass），这样内容就能随着响应式变化保持同步。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- In `resolveBinding` you sometimes construct `{ component: result }` / `{ component: binding, props: {} }` but elsewhere (and in `Tooltip.vue`) you expect `compData.comp`, so these branches should set `comp` instead of `component` to avoid `undefined` being passed to `<component :is="compData.comp">` at runtime.
- `UserInfoTooltip.vue` resolves `userProp` into `userInfo` (and `titleClass`) once at setup time, so if the `user` prop changes later the tooltip won't update; consider using a `computed` based on `userProp` (and deriving `titleClass` from it) so content stays in sync with reactive changes.
## Individual Comments
### Comment 1
<location> `src/renderer/src/function/utils/appUtil.ts:2035-2044` </location>
<code_context>
+ | (() => T | VueCompData<T> )
+ | ((eventData: {x: number, y: number}) => T | VueCompData<T> )
+
+function resolveBinding<T extends Component>(binding: VTooltipBinding<T>, eventData: {x: number, y: number}): VueCompData<T> {
+ if (typeof binding === 'function') {
+ // eslint-disable-next-line multiline-ternary
+ const result = binding.length === 0
+ // eslint-disable-next-line multiline-ternary
+ ? (binding as () => T | VueCompData<T>)()
+ : (binding as (eventData: {x: number, y: number}) => T | VueCompData<T>)(eventData)
+ if ('comp' in result) return result
+ return { component: result } as unknown as VueCompData<T>
+ } else if ('comp' in binding) {
+ return binding
+ } else {
+ return { component: binding, props: {} } as unknown as VueCompData<T>
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Returned tooltip data uses `component` key but the rest of the code expects `comp`, which will break tooltip rendering.
In `resolveBinding`, the returned objects use `{ component: ... }`, but `VueCompData<T>` and the tooltip pipeline (e.g. `Tooltip.vue` reading `compData.comp`) expect `comp`. This means `compData.comp` will be `undefined` for the function and non-`comp` branches, breaking rendering at runtime. Please update both `component` occurrences to `comp`, e.g.:
```ts
if ('comp' in result) return result
return { comp: result } as unknown as VueCompData<T>
...
} else {
return { comp: binding, props: {} } as unknown as VueCompData<T>
}
```
</issue_to_address>
### Comment 2
<location> `src/renderer/src/components/tooltip/UserInfoTooltip.vue:1-7` </location>
<code_context>
-\<!--
- * @FileDescription: 群成员消息悬浮窗
- * @Author: Mr.Lee
</code_context>
<issue_to_address>
**issue (bug_risk):** Leading backslash before the HTML comment will render a stray `\` in the DOM and is unnecessary.
Starting the template with `\<!--` makes the backslash render as a literal character before the HTML comment, which can introduce an unwanted `\` node and confuse tooling. This should just be a standard HTML comment:
```vue
<!--
* @FileDescription: 群成员消息悬浮窗
...
-->
```
</issue_to_address>
### Comment 3
<location> `src/renderer/src/function/elements/vueComp.ts:43` </location>
<code_context>
+ [K in keyof E]?: E[K]
+} : never
+
+type SetOptional<O extends Record<string, unknown>, T> = {
+ [K in keyof O as T extends O[K] ? K : never]?: O[K]
+} & {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the VueCompData type by removing the SetOptional/CreateOptionalKey helpers and using simpler optional properties and Partial-typed emits instead.
You can drop a good chunk of the type-level machinery without losing any runtime or useful typing functionality.
Specifically, `SetOptional` and `CreateOptionalKey` only control whether `model`/`props` appear as required vs optional based on emptiness. That adds a lot of indirection for a small ergonomic gain. You can also simplify `GetEmitArgs` to just use `Partial`.
Concrete changes:
```ts
// Remove these helpers entirely
// type SetOptional<...> = ...
// type CreateOptionalKey<...> = ...
type GetEmitArgs<T extends Component> = Partial<GetEmit<T>>
export type VueCompData<T extends Component> = {
comp: T
// keeps model typed, but always optional at the top level
model?: GetModel<T>
// keeps props typed, but always optional
props?: GetProps<T>
// same emit typing, just simpler
emit?: GetEmitArgs<T>
}
```
This preserves:
- `model` strongly typed from `GetModel<T>`
- `props` strongly typed from `GetProps<T>`
- `emit` strongly typed from the derived emit map
while removing:
- `SetOptional`
- `CreateOptionalKey`
- the non-obvious conditional logic around “empty” objects vs `undefined`
The resulting `VueCompData<T>` shape is immediately clear from its definition, and the type error surface is much smaller and easier to reason about.
</issue_to_address>
### Comment 4
<location> `src/renderer/src/function/utils/appUtil.ts:1958` </location>
<code_context>
}
}}
+function createVLongHover(): Directive<HTMLElement, undefined> {
+ const {
+ handle: userHoverHandle,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the shared long-hover listener setup into a reusable helper so both vLongHover and vTooltip can use it without cross-calling each other or duplicating logic.
You can keep all current behavior but reduce complexity and coupling by extracting the long‑hover wiring into a shared helper, instead of having `vTooltip` call `vLongHover.mounted/unmounted` via `as any` and managing a second `AbortController`.
### 1. Factor out shared long‑hover setup
Move the event wiring that `createVLongHover` currently does into a reusable function:
```ts
function setupLongHover(
el: HTMLElement,
handlers: {
onHover: (eventData: { x: number; y: number }) => void
onEnd: () => void
}
): AbortController {
const { handle: userHoverHandle, handleEnd: userHoverEnd } = useStayEvent(
(event: MouseEvent) => ({ x: event.clientX, y: event.clientY }),
{
onFit: (eventData, ctx: HTMLElement) => handlers.onHover(eventData),
onLeave: () => handlers.onEnd(),
},
495,
)
const controller = new AbortController()
const options = { signal: controller.signal }
el.addEventListener(
'mouseenter',
(event) => {
userHoverHandle(event, el)
},
options,
)
el.addEventListener(
'mousemove',
(event) => {
userHoverHandle(event, el)
},
options,
)
el.addEventListener(
'mouseleave',
(event) => {
userHoverEnd(event)
},
options,
)
return controller
}
```
Then `vLongHover` becomes just a thin wrapper that dispatches events, with no duplication:
```ts
function createVLongHover(): Directive<HTMLElement, undefined> {
return {
mounted(el) {
const controller = setupLongHover(el, {
onHover(eventData) {
el.dispatchEvent(new CustomEvent('v-long-hover', { detail: eventData }))
},
onEnd() {
el.dispatchEvent(new CustomEvent('v-long-hover-end'))
},
})
;(el as any)._vLongHoverController = controller
},
unmounted(el) {
const controller = (el as any)._vLongHoverController
if (!controller) return
controller.abort()
delete (el as any)._vLongHoverController
},
}
}
```
And `vTooltip` can use the same helper directly, without cross‑directive calls or a second controller:
```ts
export const vTooltip = {
mounted<T extends Component>(
el: HTMLElement,
binding: DirectiveBinding<VTooltipBinding<T>> & { modifiers: { debug?: boolean } },
) {
let tooltip: TooltipController | undefined
const controller = setupLongHover(el, {
onHover(detail) {
const compData = resolveBinding(binding.value, detail)
tooltip = addTooltip(compData, { x: detail.x, y: detail.y })
},
onEnd() {
if (binding.modifiers?.debug) return
tooltip?.close()
tooltip = undefined
},
})
;(el as any)._vTooltipController = controller
},
unmounted(el: HTMLElement) {
const controller = (el as any)._vTooltipController
if (!controller) return
controller.abort()
delete (el as any)._vTooltipController
},
}
```
This keeps:
- The same external `v-long-hover` events.
- All tooltip behavior, including `debug` modifier.
- Support for all existing binding shapes via your current `resolveBinding`.
But it:
- Removes the `as any` cross‑directive calls.
- Centralizes the stay/hover logic in one place.
- Avoids layering two separate `AbortController`‑backed listener sets on the same element.帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈改进后续的代码审查。
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- In
resolveBindingyou sometimes construct{ component: result }/{ component: binding, props: {} }but elsewhere (and inTooltip.vue) you expectcompData.comp, so these branches should setcompinstead ofcomponentto avoidundefinedbeing passed to<component :is="compData.comp">at runtime. UserInfoTooltip.vueresolvesuserPropintouserInfo(andtitleClass) once at setup time, so if theuserprop changes later the tooltip won't update; consider using acomputedbased onuserProp(and derivingtitleClassfrom it) so content stays in sync with reactive changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resolveBinding` you sometimes construct `{ component: result }` / `{ component: binding, props: {} }` but elsewhere (and in `Tooltip.vue`) you expect `compData.comp`, so these branches should set `comp` instead of `component` to avoid `undefined` being passed to `<component :is="compData.comp">` at runtime.
- `UserInfoTooltip.vue` resolves `userProp` into `userInfo` (and `titleClass`) once at setup time, so if the `user` prop changes later the tooltip won't update; consider using a `computed` based on `userProp` (and deriving `titleClass` from it) so content stays in sync with reactive changes.
## Individual Comments
### Comment 1
<location> `src/renderer/src/function/utils/appUtil.ts:2035-2044` </location>
<code_context>
+ | (() => T | VueCompData<T> )
+ | ((eventData: {x: number, y: number}) => T | VueCompData<T> )
+
+function resolveBinding<T extends Component>(binding: VTooltipBinding<T>, eventData: {x: number, y: number}): VueCompData<T> {
+ if (typeof binding === 'function') {
+ // eslint-disable-next-line multiline-ternary
+ const result = binding.length === 0
+ // eslint-disable-next-line multiline-ternary
+ ? (binding as () => T | VueCompData<T>)()
+ : (binding as (eventData: {x: number, y: number}) => T | VueCompData<T>)(eventData)
+ if ('comp' in result) return result
+ return { component: result } as unknown as VueCompData<T>
+ } else if ('comp' in binding) {
+ return binding
+ } else {
+ return { component: binding, props: {} } as unknown as VueCompData<T>
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Returned tooltip data uses `component` key but the rest of the code expects `comp`, which will break tooltip rendering.
In `resolveBinding`, the returned objects use `{ component: ... }`, but `VueCompData<T>` and the tooltip pipeline (e.g. `Tooltip.vue` reading `compData.comp`) expect `comp`. This means `compData.comp` will be `undefined` for the function and non-`comp` branches, breaking rendering at runtime. Please update both `component` occurrences to `comp`, e.g.:
```ts
if ('comp' in result) return result
return { comp: result } as unknown as VueCompData<T>
...
} else {
return { comp: binding, props: {} } as unknown as VueCompData<T>
}
```
</issue_to_address>
### Comment 2
<location> `src/renderer/src/components/tooltip/UserInfoTooltip.vue:1-7` </location>
<code_context>
-\<!--
- * @FileDescription: 群成员消息悬浮窗
- * @Author: Mr.Lee
</code_context>
<issue_to_address>
**issue (bug_risk):** Leading backslash before the HTML comment will render a stray `\` in the DOM and is unnecessary.
Starting the template with `\<!--` makes the backslash render as a literal character before the HTML comment, which can introduce an unwanted `\` node and confuse tooling. This should just be a standard HTML comment:
```vue
<!--
* @FileDescription: 群成员消息悬浮窗
...
-->
```
</issue_to_address>
### Comment 3
<location> `src/renderer/src/function/elements/vueComp.ts:43` </location>
<code_context>
+ [K in keyof E]?: E[K]
+} : never
+
+type SetOptional<O extends Record<string, unknown>, T> = {
+ [K in keyof O as T extends O[K] ? K : never]?: O[K]
+} & {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the VueCompData type by removing the SetOptional/CreateOptionalKey helpers and using simpler optional properties and Partial-typed emits instead.
You can drop a good chunk of the type-level machinery without losing any runtime or useful typing functionality.
Specifically, `SetOptional` and `CreateOptionalKey` only control whether `model`/`props` appear as required vs optional based on emptiness. That adds a lot of indirection for a small ergonomic gain. You can also simplify `GetEmitArgs` to just use `Partial`.
Concrete changes:
```ts
// Remove these helpers entirely
// type SetOptional<...> = ...
// type CreateOptionalKey<...> = ...
type GetEmitArgs<T extends Component> = Partial<GetEmit<T>>
export type VueCompData<T extends Component> = {
comp: T
// keeps model typed, but always optional at the top level
model?: GetModel<T>
// keeps props typed, but always optional
props?: GetProps<T>
// same emit typing, just simpler
emit?: GetEmitArgs<T>
}
```
This preserves:
- `model` strongly typed from `GetModel<T>`
- `props` strongly typed from `GetProps<T>`
- `emit` strongly typed from the derived emit map
while removing:
- `SetOptional`
- `CreateOptionalKey`
- the non-obvious conditional logic around “empty” objects vs `undefined`
The resulting `VueCompData<T>` shape is immediately clear from its definition, and the type error surface is much smaller and easier to reason about.
</issue_to_address>
### Comment 4
<location> `src/renderer/src/function/utils/appUtil.ts:1958` </location>
<code_context>
}
}}
+function createVLongHover(): Directive<HTMLElement, undefined> {
+ const {
+ handle: userHoverHandle,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the shared long-hover listener setup into a reusable helper so both vLongHover and vTooltip can use it without cross-calling each other or duplicating logic.
You can keep all current behavior but reduce complexity and coupling by extracting the long‑hover wiring into a shared helper, instead of having `vTooltip` call `vLongHover.mounted/unmounted` via `as any` and managing a second `AbortController`.
### 1. Factor out shared long‑hover setup
Move the event wiring that `createVLongHover` currently does into a reusable function:
```ts
function setupLongHover(
el: HTMLElement,
handlers: {
onHover: (eventData: { x: number; y: number }) => void
onEnd: () => void
}
): AbortController {
const { handle: userHoverHandle, handleEnd: userHoverEnd } = useStayEvent(
(event: MouseEvent) => ({ x: event.clientX, y: event.clientY }),
{
onFit: (eventData, ctx: HTMLElement) => handlers.onHover(eventData),
onLeave: () => handlers.onEnd(),
},
495,
)
const controller = new AbortController()
const options = { signal: controller.signal }
el.addEventListener(
'mouseenter',
(event) => {
userHoverHandle(event, el)
},
options,
)
el.addEventListener(
'mousemove',
(event) => {
userHoverHandle(event, el)
},
options,
)
el.addEventListener(
'mouseleave',
(event) => {
userHoverEnd(event)
},
options,
)
return controller
}
```
Then `vLongHover` becomes just a thin wrapper that dispatches events, with no duplication:
```ts
function createVLongHover(): Directive<HTMLElement, undefined> {
return {
mounted(el) {
const controller = setupLongHover(el, {
onHover(eventData) {
el.dispatchEvent(new CustomEvent('v-long-hover', { detail: eventData }))
},
onEnd() {
el.dispatchEvent(new CustomEvent('v-long-hover-end'))
},
})
;(el as any)._vLongHoverController = controller
},
unmounted(el) {
const controller = (el as any)._vLongHoverController
if (!controller) return
controller.abort()
delete (el as any)._vLongHoverController
},
}
}
```
And `vTooltip` can use the same helper directly, without cross‑directive calls or a second controller:
```ts
export const vTooltip = {
mounted<T extends Component>(
el: HTMLElement,
binding: DirectiveBinding<VTooltipBinding<T>> & { modifiers: { debug?: boolean } },
) {
let tooltip: TooltipController | undefined
const controller = setupLongHover(el, {
onHover(detail) {
const compData = resolveBinding(binding.value, detail)
tooltip = addTooltip(compData, { x: detail.x, y: detail.y })
},
onEnd() {
if (binding.modifiers?.debug) return
tooltip?.close()
tooltip = undefined
},
})
;(el as any)._vTooltipController = controller
},
unmounted(el: HTMLElement) {
const controller = (el as any)._vTooltipController
if (!controller) return
controller.abort()
delete (el as any)._vTooltipController
},
}
```
This keeps:
- The same external `v-long-hover` events.
- All tooltip behavior, including `debug` modifier.
- Support for all existing binding shapes via your current `resolveBinding`.
But it:
- Removes the `as any` cross‑directive calls.
- Centralizes the stay/hover logic in one place.
- Avoids layering two separate `AbortController`‑backed listener sets on the same element.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
对了，记住密码好像也坏了...太晚了，没排查那里有问题... |
|
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
2 participants
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.
顺便反馈一个bug，为毛llbt的动画表情尺寸这么大？这分明是常规图片的尺寸啊，class也没有face。快去修bug去
Summary by Sourcery
引入一个支持长按悬停的新通用提示系统，并将其应用到用户信息和表情预览中。
新功能：
优化改进：
<script setup>以集成新的提示系统。Original summary in English
Summary by Sourcery
Introduce a new generic tooltip system with long-hover support and apply it to user info and emoji previews.
New Features:
Enhancements:
<script setup>for new tooltip integration.