fix(ChatMessage/ChatMessages): handle generic message#5259
fix(ChatMessage/ChatMessages): handle generic message#5259
Conversation
commit: |
benjamincanac
left a comment
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) |
zAlweNy26
force-pushed
the
chat-message-generics
branch
from
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 |
zAlweNy26
force-pushed
the
chat-message-generics
branch
from
7abafbb to
03f44b2
Compare
benjamincanac
force-pushed
the
v4
branch
2 times, most recently
from
60e4da6 to
dc8040f
Compare
zAlweNy26
force-pushed
the
chat-message-generics
branch
from
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 |
zAlweNy26
force-pushed
the
chat-message-generics
branch
from
d8cf45b to
92b170b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/components/ChatMessages.vue (1)
62-69:⚠️ Potential issue | 🟠 MajorSlot generic propagation is still defaulting instead of deriving from
T[number].
ChatMessageSlots[K]here uses default generic params, so slot props (likecontent.metadata) can still collapse tounknownrather than the concrete message union type.🔧 Proposed fix
+type SlotBase<T extends UIMessage[]> = + T[number] extends UIMessage<infer M, infer D, infer U> + ? ChatMessageSlots<M, D, U> + : ChatMessageSlots + -type ExtendSlotWithVersion<K extends keyof ChatMessageSlots, T extends UIMessage[] = UIMessage[]> - = ChatMessageSlots[K] extends (props: infer P) => any +type ExtendSlotWithVersion<K extends keyof SlotBase<T>, T extends UIMessage[] = UIMessage[]> + = SlotBase<T>[K] extends (props: infer P) => any ? (props: P & { message: T[number] }) => any - : ChatMessageSlots[K] + : SlotBase<T>[K] export type ChatMessagesSlots<T extends UIMessage[] = UIMessage[]> = { - [K in keyof ChatMessageSlots]: ExtendSlotWithVersion<K, T> + [K in keyof SlotBase<T>]: ExtendSlotWithVersion<K, T> } & {🤖 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 62 - 69, The slot mapping currently indexes ChatMessageSlots without propagating the message generic, causing slot prop types to default to unknown; update ExtendSlotWithVersion and ChatMessagesSlots to index the genericized slot type (use ChatMessageSlots<T>), e.g. change the constraint to K extends keyof ChatMessageSlots<T> and replace ChatMessageSlots[K] with ChatMessageSlots<T>[K] so the slot props derive from T[number] instead of the default generic.
🧹 Nitpick comments (1)
src/runtime/components/ChatMessage.vue (1)
3-3: Split type imports into separate declarations.Line [3] combines multiple type imports; this repo requires one
import typedeclaration per imported type.♻️ Proposed fix
-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 3, The import line that combines multiple types into one "import type" declaration must be split into separate declarations: replace the single combined import for UIDataTypes, UIMessage, and UITools with three distinct "import type" statements (one for UIDataTypes, one for UIMessage, and one for UITools) so each type has its own import; update the import at the top of ChatMessage.vue that currently references UIDataTypes, UIMessage, UITools to use individual import type lines.
🤖 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 62-69: The slot mapping currently indexes ChatMessageSlots without
propagating the message generic, causing slot prop types to default to unknown;
update ExtendSlotWithVersion and ChatMessagesSlots to index the genericized slot
type (use ChatMessageSlots<T>), e.g. change the constraint to K extends keyof
ChatMessageSlots<T> and replace ChatMessageSlots[K] with ChatMessageSlots<T>[K]
so the slot props derive from T[number] instead of the default generic.
---
Nitpick comments:
In `@src/runtime/components/ChatMessage.vue`:
- Line 3: The import line that combines multiple types into one "import type"
declaration must be split into separate declarations: replace the single
combined import for UIDataTypes, UIMessage, and UITools with three distinct
"import type" statements (one for UIDataTypes, one for UIMessage, and one for
UITools) so each type has its own import; update the import at the top of
ChatMessage.vue that currently references UIDataTypes, UIMessage, UITools to use
individual import type lines.
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 🙏 |
benjamincanac
changed the title
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 |
3 participants
🔗 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