Make ChatAssistant generic to support custom message types and add avatar provider support #38
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Note that 620a778 is a breaking change, so the commit message must be feat!: make ChatAssistant generic to support custom message types
If a reviewer indicates that the message of an initial non-WIP commit requires rewriting, this feedback is addressed during the consolidation process. No amendment or force-push of already reviewed commits is necessary.
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
Outdated
Show resolved
Hide resolved
| @@ -139,6 +146,11 @@ private void initializeFooter() { | |||
| footerContainer = footer; | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
| protected Class<? extends T> getMessageType() { | |||
| return (Class<? extends T>) getClass().getTypeParameters()[0].getBounds()[0]; | |||
There was a problem hiding this comment.
It seems my comment from the previous review didn't posted. This method returns "the upper bound of the first type parameter", which for ChatAssistant is always Message (because T extends Message).
There was a problem hiding this comment.
I removed this method. This was only for throwing an exception in a specific case for a wrong usage of the component.
| @@ -139,6 +146,11 @@ private void initializeFooter() { | |||
| footerContainer = footer; | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
| protected Class<? extends T> getMessageType() { | |||
There was a problem hiding this comment.
Please add javadoc for protected method.
There was a problem hiding this comment.
I removed this method. This was only for throwing an exception in a specific case for a wrong usage of the component.
| @@ -35,6 +36,7 @@ | |||
| @SuppressWarnings("serial") | |||
| @Getter | |||
| @Setter | |||
| @AllArgsConstructor | |||
There was a problem hiding this comment.
Why is there a need for a public all args constructor? The signature of the constructor is (String, boolea, String, String, LocalDateTime), which is cumbersome (that's why this class had a builder).
There was a problem hiding this comment.
The reason was because I couldn't extend the class in the demo and use a builder in there also. Fixed with @SuperBuilder, requesting re-review ...
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomMessage.java (1)
16-16: Consider explicit access modifier for clarity.The
taglinefield implementation is correct and serves its purpose well for demonstrating custom message metadata. For consistency and clarity, consider adding an explicit access modifier.- String tagline; + private String tagline;Since Lombok generates public getters/setters, making the field private follows encapsulation best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/Message.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java(4 hunks)src/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomMessage.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/Message.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/react/animated-fab.tsx:0-0
Timestamp: 2025-07-24T13:41:19.117Z
Learning: In the ChatAssistant animated-fab React component (src/main/resources/META-INF/frontend/react/animated-fab.tsx), the Floating Action Button (FAB) appears empty in the React code but gets its icon content from the Avatar Vaadin component, which is inserted via DOM manipulation from the Java backend's initializeAvatar method.
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-24T13:40:40.471Z
Learning: In ChatAssistant's initializeAvatar method (src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java), direct DOM manipulation using executeJs is intentionally used to move the Avatar component to be a child of a button that is inside a div within the React-controlled web component structure. This approach is necessary because the target elements are created by React, and standard Vaadin component hierarchy methods cannot be used due to ReactAdapterComponent limitations.
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-22T19:52:28.019Z
Learning: When using ReactAdapterComponent with custom React components in ChatAssistant, standard Vaadin component hierarchy methods like add() cannot be used because the component doesn't implement HasComponents, and the DOM structure is controlled by React. Direct DOM manipulation with executeJs may be necessary to integrate Vaadin components (like Avatar) into React-generated DOM structures.
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
🔇 Additional comments (1)
src/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomMessage.java (1)
9-14: LGTM! Proper Lombok annotations for inheritance.The Lombok annotations are correctly configured for a subclass:
@SuperBuilderenables builder pattern inheritance@EqualsAndHashCode(callSuper = true)properly includes parent class fields- Standard
@Getter/@Setterprovide necessary accessorsThis implementation aligns well with the generic
ChatAssistant<T extends Message>design objective.
There was a problem hiding this comment.
LGTM, the changes are approved!
As a final step, could you please consolidate your commits by performing a manual interactive rebase? This is necessary because the PR contains two non-WIP commits and several WIP commits, which prevents us from using the standard "Squash and Merge" or "Rebase and Merge" buttons on GitHub.
Close #36
Close #39
Summary by CodeRabbit
New Features
Bug Fixes
Chores