fix(ChatMessage/ChatMessages): handle generic message#5259
fix(ChatMessage/ChatMessages): handle generic message#5259
Conversation
commit: |
benjamincanac
left a comment
There was a problem hiding this comment.
I need to look more closely but I'm not sure the generic will work this way 🤔 @sandros94 do you have an opinion on this?
|
Hi @benjamincanac, thanks for the response. I tested it, and it seems to work. I only have some doubts about the implementation I did. I could have probably used the |
Somehow I've missed this notification.
@zAlweNy26 I will leave a couple of reviews with what should be the required changes (tho I'm on mobile, thus I cannot test them directly) |
4f3231c to
68029e1
Compare
|
@benjamincanac @sandros94 should I open a PR also in the chat template to fix the error? Or this PR should be retro-compatible? |
|
Also I noticed a missing prop in the |
7abafbb to
03f44b2
Compare
60e4da6 to
dc8040f
Compare
03f44b2 to
7710d6f
Compare
|
Hi @benjamincanac, any update on this PR? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces TypeScript generics to ChatMessage.vue and ChatMessages.vue and propagates them through props, slots, and the script-setup signatures. ChatMessage gains generics TMetadata, TDataParts extends UIDataTypes, TTools extends UITools; ChatMessageProps, actions.onClick typing, and ChatMessageSlots are updated and metadata is passed into the content slot with a guarded text-part render (part.type === 'text' && 'text' in part). ChatMessages is made generic over T extends UIMessage[], adds SlotBase and ExtendSlotWithVersion types, and updates props, slots, and template slot bindings accordingly. Three docs/example files switch several Tailwind utilities to use trailing Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/runtime/components/ChatMessages.vue`:
- Around line 62-68: The slot props are losing the concrete generics because
ChatMessageSlots is being referenced with its defaults; change
ExtendSlotWithVersion to first extract the element type from T (T[number]) and
infer its generic parameters (e.g. metadata, data types, tools) and then
instantiate ChatMessageSlots with those inferred generics before extracting
props. In other words, derive the message element type (like Msg = T[number])
and use a conditional infer (e.g. Msg extends UIMessage<InferMeta, InferData,
InferTools> ? ChatMessageSlots<InferMeta, InferData, InferTools>[K] :
ChatMessageSlots[K]) so the resulting slot prop type includes the actual
metadata/data/tools instead of the defaults for slots such as content.metadata.
|
I just realized that all most of my reviews I left back in october are still pending because I've never submitted them... How did I manage to do such a mistake 🥲 |
|
@zAlweNy26 I'll be back to Nuxt UI for type-related fixes in a few days, I'll also be available on discord so we can revisit this one together. I've submitted the reviews even if most are outdated just for backlog |
d8cf45b to
92b170b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/runtime/components/ChatMessages.vue (1)
312-314: Consider documenting the type cast rationale.The
(slotData as any)cast is a pragmatic workaround for Vue's template type limitations with generics. While functionally correct, you might consider adding a brief comment explaining why this cast is necessary to help future maintainers understand it's intentional rather than a type safety oversight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/ChatMessages.vue` around lines 312 - 314, Add a brief inline comment above the template cast explaining why the cast is necessary: note Vue's template typing limitations with generics require using `(slotData as any)` when iterating `getProxySlots()` so that `slotData` can be forwarded to `<slot>` while preserving `ChatMessagesSlots<T>` usage; reference the cast and the surrounding symbols (`getProxySlots()`, `slotData`, `ChatMessagesSlots<T>`, and the `<slot :name="...">` line) so future maintainers know this is an intentional workaround rather than an unchecked type omission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/components/ChatMessages.vue`:
- Around line 312-314: Add a brief inline comment above the template cast
explaining why the cast is necessary: note Vue's template typing limitations
with generics require using `(slotData as any)` when iterating `getProxySlots()`
so that `slotData` can be forwarded to `<slot>` while preserving
`ChatMessagesSlots<T>` usage; reference the cast and the surrounding symbols
(`getProxySlots()`, `slotData`, `ChatMessagesSlots<T>`, and the `<slot
:name="...">` line) so future maintainers know this is an intentional workaround
rather than an unchecked type omission.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/app/components/content/examples/chat/ChatPaletteContentSearchExample.vuedocs/app/components/content/examples/chat/ChatPaletteModalExample.vuedocs/app/components/search/SearchChat.vuesrc/runtime/components/ChatMessage.vuesrc/runtime/components/ChatMessages.vue
|
@zAlweNy26 sorry if it took me so long to review this 🙏 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/components/ChatMessages.vue (1)
67-70:⚠️ Potential issue | 🟠 Major
ExtendSlotWithVersioncan fail to augment slot props because slots are optional.
SlotBase<T>[K]is optional (... | undefined), so the conditional may not match the function signature and can fall back toSlotBase<T>[K]unchanged. That drops the added{ message: T[number] }in type space.🔧 Suggested fix
type ExtendSlotWithVersion<K extends keyof SlotBase<T>, T extends UIMessage[] = UIMessage[]> - = SlotBase<T>[K] extends (props: infer P) => VNode[] + = NonNullable<SlotBase<T>[K]> extends (props: infer P) => VNode[] ? (props: P & { message: T[number] }) => VNode[] : SlotBase<T>[K]In TypeScript, if `type S = { content?: (props: { id: string }) => any }`, what does `type R = S['content'] extends (props: infer P) => any ? P : never` resolve to? Does optionality (`| undefined`) prevent `infer P` from matching?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/ChatMessages.vue` around lines 67 - 70, ExtendSlotWithVersion fails when SlotBase<T>[K] is optional (includes undefined); change the conditional to first remove undefined/null (e.g., via NonNullable or an equivalent) before inferring props so the infer P branch matches and the augmented prop { message: T[number] } is applied. Update the type definition for ExtendSlotWithVersion to use NonNullable<SlotBase<T>[K]> in the conditional inference, referencing SlotBase, ExtendSlotWithVersion and UIMessage so slot props are properly augmented even when slots are optional.
🧹 Nitpick comments (1)
src/runtime/components/ChatMessage.vue (1)
4-4: Split the'ai'type import into separateimport typestatements.This line groups multiple type imports in one statement, which conflicts with the repo import rule.
♻️ Suggested change
-import type { UIDataTypes, UIMessage, UITools } from 'ai' +import type { UIDataTypes } from 'ai' +import type { UIMessage } from 'ai' +import type { UITools } from 'ai'As per coding guidelines
**/*.{ts,tsx,vue}: Always use separateimport type { X }statements for type imports in TypeScript/Vue files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/ChatMessage.vue` at line 4, The import grouping of types from 'ai' in ChatMessage.vue must be split into separate import type statements; replace the single line "import type { UIDataTypes, UIMessage, UITools } from 'ai'" with three separate lines each using "import type" for UIDataTypes, UIMessage, and UITools respectively so the code follows the repo rule (refer to the symbols UIDataTypes, UIMessage, UITools and the ChatMessage.vue import area).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/runtime/components/ChatMessages.vue`:
- Around line 67-70: ExtendSlotWithVersion fails when SlotBase<T>[K] is optional
(includes undefined); change the conditional to first remove undefined/null
(e.g., via NonNullable or an equivalent) before inferring props so the infer P
branch matches and the augmented prop { message: T[number] } is applied. Update
the type definition for ExtendSlotWithVersion to use NonNullable<SlotBase<T>[K]>
in the conditional inference, referencing SlotBase, ExtendSlotWithVersion and
UIMessage so slot props are properly augmented even when slots are optional.
---
Nitpick comments:
In `@src/runtime/components/ChatMessage.vue`:
- Line 4: The import grouping of types from 'ai' in ChatMessage.vue must be
split into separate import type statements; replace the single line "import type
{ UIDataTypes, UIMessage, UITools } from 'ai'" with three separate lines each
using "import type" for UIDataTypes, UIMessage, and UITools respectively so the
code follows the repo rule (refer to the symbols UIDataTypes, UIMessage, UITools
and the ChatMessage.vue import area).
|
@benjamincanac CI is correctly failing because the template still destructure the duplicated Template should be updated to be |
|
@sandros94 I'm not sure we should introduce a breaking change to add generics 🤔 |
Well, I could hardcode types to take into account the duplicated message. Tho, IMO, I would fix as it is just wasted memory and DOM if not always properly destructured. The |
|
Hi, may I ask what happened to this PR? It would be cool to have the messages correctly typed. Also the |
🔗 Linked issue
❓ Type of change
📚 Description
With this PR, I wanted to add generics in both
ChatMessageandChatMessagesso that the correct type can be inferred inside slots (like the metadata of every message)📝 Checklist