Improvements: add mobile mode and other features#68
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe PR bumps the project version, updates documentation and metadata, adds new chat assistant mode/FAB model types, refactors core chat rendering and sizing behavior, refreshes frontend movement/resize support, and expands demos and tests. ChangesChatAssistant 5.1 Feature Expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js (1)
72-84: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCompute drag candidates against the FAB’s local bounds before mutating position.
Initial/reset placement uses offset-parent bounds, but drag/snap still use
window.innerWidth/innerHeightand viewport coordinates. For container-anchored FABs, this can jump or clamp the FAB outside its container. Also, Line 139 mutatespositionbefore the sensitivity check, so click-only jitter is later persisted bysnapToBoundary().🐛 Proposed fix
function fcChatAssistantBounds(item) { if (getComputedStyle(item).position === 'fixed' || !item.offsetParent) { - return { width: window.innerWidth, height: window.innerHeight }; + return { left: 0, top: 0, width: window.innerWidth, height: window.innerHeight }; } const rect = item.offsetParent.getBoundingClientRect(); - return { width: rect.width, height: rect.height }; + return { left: rect.left, top: rect.top, width: rect.width, height: rect.height }; } ... - let screenWidth = window.innerWidth; - let screenHeight = window.innerHeight; + let bounds = fcChatAssistantBounds(item); ... - screenWidth = window.innerWidth; - screenHeight = window.innerHeight; + bounds = fcChatAssistantBounds(item); ... - const xMax = Math.max(margin, screenWidth - itemRect.width - margin); - const yMax = Math.max(margin, screenHeight - itemRect.height - margin); + const xMax = Math.max(margin, bounds.width - itemRect.width - margin); + const yMax = Math.max(margin, bounds.height - itemRect.height - margin); ... - position.x = screenWidth - e.clientX - (itemRect.width / 2); - position.y = screenHeight - e.clientY - (itemRect.height / 2); + const nextX = bounds.left + bounds.width - e.clientX - (itemRect.width / 2); + const nextY = bounds.top + bounds.height - e.clientY - (itemRect.height / 2); // Do not move if delta is below sensitivity - if (isClickOnlyEvent()) { + if (Math.abs(nextX - initialPosition.x) < sensitivity + && Math.abs(nextY - initialPosition.y) < sensitivity) { return; } + position.x = nextX; + position.y = nextY; updatePosition();Also applies to: 103-115, 138-145
🤖 Prompt for 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. In `@src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js` around lines 72 - 84, The drag/snap logic in fc-chat-assistant-movement.js is still using window.innerWidth/window.innerHeight and viewport coordinates, which can push a container-anchored FAB outside its local bounds; update the candidate and boundary calculations in the movement handlers and snapToBoundary() to use the FAB’s local offset-parent bounds consistently, matching the initial/reset placement logic. Also adjust the drag update path so the position is not mutated until after the sensitivity check in the pointer/mouse move handler, preventing click jitter from being committed by snapToBoundary().
🧹 Nitpick comments (2)
src/test/java/com/flowingcode/vaadin/addons/chatassistant/test/SerializationTest.java (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the new listener bookkeeping in this roundtrip.
This still serializes only the message path. The PR added
screenSizeListeners/ScreenSizeListenerEntrystate toChatAssistant, but this test never registers one, so a listener-serialization regression would still pass here. Add at least oneaddScreenSizeListener(...)beforetestSerializationOf(chatAssistant).🤖 Prompt for 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. In `@src/test/java/com/flowingcode/vaadin/addons/chatassistant/test/SerializationTest.java` around lines 46 - 50, The roundtrip in SerializationTest.testSerialization only covers the message path and does not exercise the new screen-size listener state added to ChatAssistant. Update the test to register at least one listener via ChatAssistant.addScreenSizeListener(...) before calling testSerializationOf(chatAssistant), so ScreenSizeListenerEntry and screenSizeListeners are included in serialization coverage.src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java (1)
97-108: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding the delayed
UI.accessagainst a detached UI.If the view is detached before the 5-second timer fires,
currentUI.access(...)can throwUIDetachedException. For demo robustness, you could capture the optional UI and no-op when absent, or useui.accessSynchronously-safe handling. Each click also spins up a freshjava.util.Timer(non-daemon) thread; reusing a single scheduler would be cleaner.🤖 Prompt for 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. In `@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java` around lines 97 - 108, The delayed UI update in ChatAssistantDemo should be made safe for detached views: the TimerTask currently calls UI.access on the captured UI without checking whether it is still attached, so guard this path using the existing UI capture in the click handler and skip the update when the UI is no longer available. While touching the same block, avoid creating a new java.util.Timer per click by centralizing scheduling/reusing a single scheduler for the delayed message logic.
🤖 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/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java`:
- Around line 294-295: The repeated CSS/dimension literals in the ChatAssistant
setters are triggering duplication warnings, so extract the shared values into
constants and reuse them across the affected setters. Update the relevant
methods in ChatAssistant to reference a single source for the --fc-min-width,
--fc-min-height, and "width" strings so the dimension configuration stays
aligned and the static-analysis gate passes.
- Around line 134-135: The unread badge reset path is using a different default
text color than the initial badge style, so make the fallback in ChatAssistant
consistent. Update the DEFAULT_UNREAD_BADGE_COLOR used by
setUnreadBadgeColors(null, null) to match the initial unread badge text color
value, and verify the reset behavior in the unread badge styling logic remains
aligned with the existing default constants.
- Around line 451-471: Restore the per-UI singleton check in
ChatAssistant.onAttach so a Vaadin UI cannot end up with multiple ChatAssistant
instances registering competing listeners. Add a UI-scoped guard around the
existing addComponentRefreshedListener calls in ChatAssistant, and ensure that
any state used to track the active instance is cleared or updated when the
component is detached or the UI is reused. Keep the fix localized to
ChatAssistant.onAttach/onDetach and the related instance-tracking logic so only
one ChatAssistant can be active per UI at runtime.
- Around line 770-783: Separate the persisted FAB movable preference from the
runtime mobile override in ChatAssistant. Update setFabMovable(boolean) and the
mobile-mode handling around fabMovable so the stored preference is not
overwritten by the temporary mobile-state toggle, and isFabMovable() always
reflects the user’s preference rather than the effective drag state. Ensure the
mobile-mode path only adds or removes the client attribute for actual dragging
behavior without mutating the saved preference, and keep the relevant fab
element attribute updates consistent with that separation.
In
`@src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js`:
- Around line 302-308: The bulk teardown in fcChatAssistantScreenSizeOffAll only
disconnects observers and leaves the per-key refresh guards behind, which can
block re-registration after detach/reattach. Update
fcChatAssistantScreenSizeOffAll to clear the same screen-size guard entries that
fcChatAssistantScreenSizeOff removes, alongside disconnecting each observer, so
ChatAssistant.onAttach can safely re-register listeners after a full teardown.
In `@src/main/resources/META-INF/resources/frontend/fc-chat-assistant-resize.js`:
- Around line 29-32: The resize handler in fc-chat-assistant-resize.js is
caching overlay/content-part nodes too aggressively, so after the popover closes
and reopens it can keep targeting detached DOM references. Update fetchOverlay()
in the resize handle initialization path to re-resolve the current overlay and
the [part="content"] element on each open (or whenever the existing nodes are no
longer connected), and make sure shouldDrag(), setContentWidth(), and
setContentHeight() always operate on the refreshed nodes rather than stale
closures.
In
`@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantFabConfigDemo.java`:
- Around line 63-75: The color selection buttons in ChatAssistantFabConfigDemo
should replace the current FAB color theme instead of accumulating multiple
variants. Update the handlers for the Success, Error, and Contrast buttons to
first remove the other color-related variants via
chatAssistant.removeFabThemeVariants(...) and then apply the chosen one with
chatAssistant.addFabThemeVariants(...), keeping the existing clearColors action
as-is. This ensures the demo always reflects the currently selected color
deterministically.
---
Outside diff comments:
In
`@src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js`:
- Around line 72-84: The drag/snap logic in fc-chat-assistant-movement.js is
still using window.innerWidth/window.innerHeight and viewport coordinates, which
can push a container-anchored FAB outside its local bounds; update the candidate
and boundary calculations in the movement handlers and snapToBoundary() to use
the FAB’s local offset-parent bounds consistently, matching the initial/reset
placement logic. Also adjust the drag update path so the position is not mutated
until after the sensitivity check in the pointer/mouse move handler, preventing
click jitter from being committed by snapToBoundary().
---
Nitpick comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java`:
- Around line 97-108: The delayed UI update in ChatAssistantDemo should be made
safe for detached views: the TimerTask currently calls UI.access on the captured
UI without checking whether it is still attached, so guard this path using the
existing UI capture in the click handler and skip the update when the UI is no
longer available. While touching the same block, avoid creating a new
java.util.Timer per click by centralizing scheduling/reusing a single scheduler
for the delayed message logic.
In
`@src/test/java/com/flowingcode/vaadin/addons/chatassistant/test/SerializationTest.java`:
- Around line 46-50: The roundtrip in SerializationTest.testSerialization only
covers the message path and does not exercise the new screen-size listener state
added to ChatAssistant. Update the test to register at least one listener via
ChatAssistant.addScreenSizeListener(...) before calling
testSerializationOf(chatAssistant), so ScreenSizeListenerEntry and
screenSizeListeners are included in serialization coverage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c05eb13-8aae-47cb-a9dc-aaf4a8211467
📒 Files selected for processing (28)
.gitignoreREADME.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatMessage.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/model/ChatAssistantMode.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/model/FabPosition.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/model/Message.javasrc/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.jssrc/main/resources/META-INF/resources/frontend/fc-chat-assistant-resize.jssrc/main/resources/META-INF/resources/frontend/styles/fc-chat-assistant-style.csssrc/main/resources/META-INF/resources/frontend/styles/fc-chat-message-styles.csssrc/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.javasrc/test/java/com/flowingcode/vaadin/addons/DemoLayout.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantBoxDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantFabConfigDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantGenerativeDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLazyLoadingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantMarkdownDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantModeDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomChatMessage.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomMessage.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/DemoView.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/it/BasicIT.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/it/ViewIT.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/test/SerializationTest.java
💤 Files with no reviewable changes (7)
- src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/Message.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/BasicIT.java
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomChatMessage.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomMessage.java
- src/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/DemoView.java
mlopezFC
left a comment
mlopezFC
left a comment
There was a problem hiding this comment.
Code Review
Solid, well-documented feature work: FAB drag/corner positioning, resizable chat window with min/max bounds, responsive desktop/mobile modes with opt-in breakpoint switching, screen-size listeners, and the migration to the native Vaadin Markdown component. The Java↔JS contract is clean and the inline comments explaining the popover/overlay sizing are genuinely helpful. Version bump 5.0.2 → 5.1.0 (MINOR) is correctly aligned with the feat: commits.
Most of the substantive points are left as inline comments. A few cross-cutting notes below.
Should address
- Missing
@sincetags on the new public/protected API (see inline comment onChatAssistantMode). FC add-on conventions require it for all new API elements.
Suggestions (non-blocking)
- Test coverage. There are no unit tests for the new pure-Java logic (
parseFabMarginfallbacks,setUnreadMessagesclamping to 0–99, size-variant add/remove resizing,addScreenSizeListenerargument validation, thesetModeopen-state reconciliation). Not a blocker — just flagging it as a nice-to-have. - Commit atomicity.
d38f33bbundles three logical changes (markdown component swap +markdown-editor-addonremoval + pom version bump). Ideally these would be separate commits (in particular a standalone version-bump commit). Suggestion only.
Minor / style
- Google Java Style: 11 occurrences of
if(/else if(without a space and 2 trailing-whitespace lines (ChatAssistant.java:922,940). Runningmvn com.spotify.fmt:fmt-maven-plugin:formatwould normalize these. ChatAssistant.java:chatWindow.setOpenOnClick(false)is set twice (insetUIand again ininitializeChatWindow) — harmless redundancy.
What looks good
- The native
Markdownmigration is clean — no leftoverMarkdownViewer/markdown-editorreferences, andvaadin-markdown-flowis present for the pinned Vaadin 24.8.0. ScreenSizeListenerEntry implements Serializableand the other new fields are serialization-safe.- Live-read
isResizable()/shouldDrag()letssetWindowResizabletake effect without re-init. - Auto-switching is opt-in to avoid a breaking change — good backward-compat judgment, well documented.
- README rewrite is a real improvement and drops the stale
toggle()/addChatSentListenerAPI.
| * The display mode of the chat assistant: {@link #MOBILE} opens the chat window as a full-screen | ||
| * dialog, while {@link #DESKTOP} opens it as an anchored popover. | ||
| */ | ||
| public enum ChatAssistantMode { |
There was a problem hiding this comment.
Missing @since tag. FlowingCode add-on conventions require @since on every new public/protected API element. This applies across the whole PR, not just this enum — please add @since 5.1.0 to:
- both new enums (
ChatAssistantMode,FabPosition), - the new events/enums (
ModeChangedEvent,ScreenSizeEvent,ScreenSizeDirection), - and the new
ChatAssistantmethods:setMode/getMode/setMobileMode/isMobileMode/addModeChangedListener,setFabPosition/getFabPosition/resetFabPosition,addFabThemeVariants/removeFabThemeVariants,setFabAnchoredToViewport/isFabAnchoredToViewport,setResizeIndicatorsVisible/isResizeIndicatorsVisible, thesetWindowMin*/Max*/Width/Heightfamily,addScreenSizeListener,setUnreadMessages/getUnreadMessages/setUnreadBadgeColors,setMobileModeSwitchingEnabled/isMobileModeSwitchingEnabled/getMobileBreakpoint.
Place it after the main description, before any @param/@return.
| * #%L | ||
| * Chat Assistant Add-on | ||
| * %% | ||
| * Copyright (C) 2023 - 2024 Flowing Code |
There was a problem hiding this comment.
Inconsistent copyright years across new files. This enum says 2023 - 2024, FabPosition also 2023 - 2024, ChatAssistantModeDemo says 2023 - 2025, and the new JS files say 2023 - 2026. Running mvn license:update-file-header -Dlicense.canUpdateCopyright -Dlicense.canUpdateDescription will normalize them to the current year.
| } | ||
| } | ||
|
|
||
| window.addEventListener('resize', () => updateCanDrag()); |
There was a problem hiding this comment.
Listeners/observers are never cleaned up on detach. Unlike fc-chat-assistant-movement.js (which patches disconnectedCallback to remove its resize listener), this script adds a window resize listener here, a MutationObserver (styleObserver), and a setTimeout(fetchOverlay, 2000) that are never removed/disconnected. The init guard lives on the durable resizer Div so it won't re-add on reopen, but if the component is detached these closures (capturing item/container/overlay) stay alive. Consider mirroring the movement.js cleanup pattern (disconnect the observer and remove the resize listener on disconnect).
| boolean movablePreference = this.fabMovable; | ||
| setFabMovable(false); | ||
| this.fabMovable = movablePreference; |
There was a problem hiding this comment.
isFabMovable() reports the preference, not the effective state, in mobile mode. Here the movable attribute is turned off (setFabMovable(false)) but this.fabMovable is restored to the preference, so while in MOBILE the FAB isn't draggable yet isFabMovable() returns true. It's documented as "preserving the preference," but a caller querying the state mid-mobile gets a misleading answer. Consider exposing an effective-state notion, or a @code-level note on the getter clarifying it returns the desktop preference.
| public void testSerialization() throws ClassNotFoundException, IOException { | ||
| try { | ||
| ChatAssistant chatAssistant = new ChatAssistant(); | ||
| ChatAssistant<Message> chatAssistant = new ChatAssistant<Message>(); |
There was a problem hiding this comment.
Minor: the style guide prefers diamond syntax — new ChatAssistant<>() instead of new ChatAssistant<Message>().
Swap the markdown-editor-addon MarkdownViewer for Vaadin's built-in Markdown component in ChatMessage.
Add the directional resize handles, window sizing with clamped min/max bounds applied to the popover content part, size persistence across close/reopen, the screen-size threshold tracking, and the resize direction indicators.
Replace the stale markdown-editor rule with rules that collapse the rendered markdown's outer block margins and tighten inter-block gaps.
Close #64 Add a Lombok @builder constructor and extend the component with: - FAB icon (default inline chatbot icon, custom overloads), sizing, and theme variants (color pass-through; LUMO_SMALL/LARGE drive the diameter). - FAB placement: setFabPosition/resetFabPosition, margin, setFabMovable, and setFabAnchoredToViewport for bounded placement. - Window control: setWindowResizable, setResizeIndicatorsVisible, initial size, and min/max bounds honored on open and while resizing; the window shrinks to its min height and clamps to the overlay. - Display mode: setMode/setMobileMode with a full-screen mobile dialog, breakpoint auto-switching (opt-in), and addModeChangedListener. - addScreenSizeListener for chat-window size threshold crossings.
Commented basic/lazy-loading/markdown/generative demos using the current API.
A FAB configuration demo (custom icon, size and color variants, section titles, movable/resizable/indicator toggles with notifications) and an in-a-box demo with a container-anchored FAB positioned via setFabPosition.
Desktop/mobile modes with breakpoint auto-switching, manual setMode, a mode-changed listener with notification, FAB position reset, and a chat window screen-size threshold listener.
Expand the feature list and replace the outdated getting-started example with current builder/setter-based tutorials covering messaging, FAB styling, window sizing, and responsive mobile mode.
Introduce a FabVariant enum for FAB theming. Make isFabMovable() report the effective current state. Enforce a single ChatAssistant per UI. Use --lumo-warning-contrast-color as the badge text. Extract the --fc-min/max-* CSS property names into constants. Drop the duplicate setOpenOnClick(false). Add @SInCE 5.1.0 tags to the new public API.
Cover parseFabMargin fallbacks, setUnreadMessages clamping to 0-99, addScreenSizeListener argument validation, and the FabVariant to ButtonVariant mapping, in the lightweight style of SerializationTest.
Expose width, height, maxWidth and maxHeight on the ChatAssistant builder so the initial window size and its max bounds can be configured at construction time.
Totremont
force-pushed
the
chat-assistant-5.1.0
branch
from
96035c5 to
2025dd3
Compare
|
To split the markdown migration and the version bump I had to create 3 commits and do a force push. The other changes are left as WIP |
javier-godoy
left a comment
javier-godoy
left a comment
There was a problem hiding this comment.
Add-ons compatible with Vaadin 25 must support both Lumo and Aura themes.
github-project-automation
Bot
moved this from To Do
to In Progress
in Flowing Code Addons
Make the FAB color variants cross-theme: SUCCESS and ERROR now also carry an Aura accent CSS class (aura-accent-green/red) alongside the Lumo theme variant, since Aura styles accent colors via class rather than the theme attribute; PRIMARY stays on the cross-theme primary token and LUMO_CONTRAST is documented as Lumo-only. Replace the hardcoded --lumo-* tokens on the unread badge and in fc-chat-assistant-style.css (shadows, border radius, contrast) with cross-theme fallback chains (var(--lumo-x, var(--aura-y, <literal>))), mirroring the pattern already used in fc-chat-message-styles.css.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js (1)
103-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClamp anchored-container FABs against their container, not the viewport.
fcChatAssistantBounds()handles fixed vs offset-parent geometry, butsnapToBoundary()still usesscreenWidth/screenHeight. ForsetFabAnchoredToViewport(false), a container resize can leave a corner-positioned FAB outside its container.Proposed fix
function snapToBoundary() { // Get current dimensions to account for transforms const itemRect = fab.getBoundingClientRect(); + const bounds = fcChatAssistantBounds(item); - const xMax = Math.max(margin, screenWidth - itemRect.width - margin); - const yMax = Math.max(margin, screenHeight - itemRect.height - margin); + const xMax = Math.max(margin, bounds.width - itemRect.width - margin); + const yMax = Math.max(margin, bounds.height - itemRect.height - margin);🤖 Prompt for 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. In `@src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js` around lines 103 - 115, The snapToBoundary() logic is still clamping against screenWidth and screenHeight, so anchored FABs can escape their container when setFabAnchoredToViewport(false) is used. Update snapToBoundary() in fc-chat-assistant-movement.js to use the container-aware bounds from fcChatAssistantBounds() (or the same offset-parent geometry it returns) instead of viewport dimensions, and keep the existing position.x/position.y clamping and updatePosition() flow intact.
🧹 Nitpick comments (2)
src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantMarkdownDemo.java (1)
54-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a Java text block for the sample Markdown string.
Same as noted elsewhere: this multi-line concatenation could be a text block for readability.
🤖 Prompt for 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. In `@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantMarkdownDemo.java` around lines 54 - 58, The sample Markdown string in ChatAssistantMarkdownDemo is built with multi-line concatenation, which should be simplified for readability. Update the message.setValue usage to use a Java text block instead of repeated string concatenation, keeping the same Markdown content and preserving the existing sample formatting.Source: Linters/SAST tools
src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLazyLoadingDemo.java (1)
132-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider Java text blocks for the multi-line dialogue strings.
SonarCloud flags several multi-line
+-concatenated strings here (e.g. lines 139-149, 173-178, 191-193, 199-202, 209-215, 240-243, 248-258) that could be simplified with text blocks ("""...""") for readability.🤖 Prompt for 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. In `@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLazyLoadingDemo.java` around lines 132 - 260, The multi-line dialogue literals in createMessages() are still built with repeated string concatenation, which makes the sample data hard to read and maintain. Update the affected Message.builder().content(...) calls in ChatAssistantLazyLoadingDemo to use Java text blocks for each multi-line Hamlet excerpt, keeping the same text and line breaks while removing the chained + concatenations. Focus on the long content strings used for Claudius and Hamlet entries so the sample conversation remains identical but much clearer.Source: Linters/SAST tools
🤖 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/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java`:
- Around line 928-930: `isFabMovable()` is currently reporting the raw
`fabMovable` flag even when container-anchored mode disables dragging, so align
it with the actual drag state used by the client script. Update
`ChatAssistant.isFabMovable()` to also account for the anchored condition
managed by `setFabAnchoredToViewport(boolean)` and the `anchored` client
attribute, ensuring it only returns true when dragging is actually possible.
In
`@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java`:
- Around line 98-128: The ChatAssistantDemo click handler creates a new Timer
for each "Chat With Thinking" action, but the Timer is never canceled after the
one-shot task finishes, leaving a live background thread behind. Update the
logic around the delayed message task in the chatWithThinking listener to retain
the Timer instance and call cancel() (and purge if needed) once currentUI.access
updates delayedMessage and chatAssistant.updateMessage complete, so the timer
thread terminates after the scheduled run.
In
`@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantGenerativeDemo.java`:
- Around line 111-146: The async demo in ChatAssistantGenerativeDemo is blocking
the shared ForkJoinPool.commonPool() by using CompletableFuture.runAsync without
an executor together with the sleep inside the delayed message flow. Update the
Chat With Generative Thinking click handler to run the background work on a
dedicated executor or UI-friendly task runner instead of the common pool, and
keep the UI updates through currentUI.access as they are. Ensure the streaming
logic around streamWords and delayedMessage remains the same, but remove any
blocking waits from the shared pool path.
---
Outside diff comments:
In
`@src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js`:
- Around line 103-115: The snapToBoundary() logic is still clamping against
screenWidth and screenHeight, so anchored FABs can escape their container when
setFabAnchoredToViewport(false) is used. Update snapToBoundary() in
fc-chat-assistant-movement.js to use the container-aware bounds from
fcChatAssistantBounds() (or the same offset-parent geometry it returns) instead
of viewport dimensions, and keep the existing position.x/position.y clamping and
updatePosition() flow intact.
---
Nitpick comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLazyLoadingDemo.java`:
- Around line 132-260: The multi-line dialogue literals in createMessages() are
still built with repeated string concatenation, which makes the sample data hard
to read and maintain. Update the affected Message.builder().content(...) calls
in ChatAssistantLazyLoadingDemo to use Java text blocks for each multi-line
Hamlet excerpt, keeping the same text and line breaks while removing the chained
+ concatenations. Focus on the long content strings used for Claudius and Hamlet
entries so the sample conversation remains identical but much clearer.
In
`@src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantMarkdownDemo.java`:
- Around line 54-58: The sample Markdown string in ChatAssistantMarkdownDemo is
built with multi-line concatenation, which should be simplified for readability.
Update the message.setValue usage to use a Java text block instead of repeated
string concatenation, keeping the same Markdown content and preserving the
existing sample formatting.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f425be89-a84e-4f7f-8535-20d6b08edbc8
⛔ Files ignored due to path filters (2)
src/main/resources/META-INF/resources/icons/chatbot.svgis excluded by!**/*.svgsrc/test/resources/META-INF/resources/chatbot.svgis excluded by!**/*.svg
📒 Files selected for processing (35)
.gitignoreREADME.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatMessage.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/model/ChatAssistantMode.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/model/FabPosition.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/model/FabVariant.javasrc/main/java/com/flowingcode/vaadin/addons/chatassistant/model/Message.javasrc/main/resources/META-INF/VAADIN/package.propertiessrc/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.jssrc/main/resources/META-INF/resources/frontend/fc-chat-assistant-resize.jssrc/main/resources/META-INF/resources/frontend/styles/fc-chat-assistant-style.csssrc/main/resources/META-INF/resources/frontend/styles/fc-chat-message-styles.csssrc/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.javasrc/test/java/com/flowingcode/vaadin/addons/DemoLayout.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantBoxDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantFabConfigDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantGenerativeDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLazyLoadingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLogicTest.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantMarkdownDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantModeDemo.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomChatMessage.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomMessage.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/DemoView.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/it/BasicIT.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/it/ViewIT.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/it/po/ChatAssistantElement.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/it/po/ChatBubbleElement.javasrc/test/java/com/flowingcode/vaadin/addons/chatassistant/test/SerializationTest.javasrc/test/resources/META-INF/frontend/styles/chat-assistant-styles-demo.css
💤 Files with no reviewable changes (1)
- src/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.java
✅ Files skipped from review due to trivial changes (14)
- .gitignore
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/po/ChatAssistantElement.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/DemoView.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/po/ChatBubbleElement.java
- src/test/resources/META-INF/frontend/styles/chat-assistant-styles-demo.css
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/AbstractViewTest.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomChatMessage.java
- src/main/resources/META-INF/VAADIN/package.properties
- src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/Message.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/BasicIT.java
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
- README.md
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLogicTest.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/CustomMessage.java
🚧 Files skipped from review as they are similar to previous changes (12)
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/test/SerializationTest.java
- src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/ChatAssistantMode.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemoView.java
- src/main/resources/META-INF/resources/frontend/styles/fc-chat-message-styles.css
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/ViewIT.java
- src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/FabPosition.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantBoxDemo.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantFabConfigDemo.java
- src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantModeDemo.java
- src/main/resources/META-INF/resources/frontend/styles/fc-chat-assistant-style.css
- pom.xml
- src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatMessage.java
isFabMovable() now returns fabMovable && fabAnchoredToViewport, matching the client drag gate (movable && anchored). A container-anchored FAB cannot be dragged, so the getter no longer reports true in that configuration; it already returned false in mobile mode. Adds a unit test covering the anchored/movable combinations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChatAssistantDemo replaces the leaked java.util.Timer (non-daemon, never cancelled) with a daemon-threaded ScheduledExecutorService that is shut down on detach. ChatAssistantGenerativeDemo runs its streaming task on a dedicated daemon executor instead of the shared ForkJoinPool.commonPool(), so its blocking sleeps no longer tie up common-pool threads; the executor is shut down on detach. Streaming behaviour and UI.access updates are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrite the javadoc on isFabMovable, setResizeIndicatorsVisible and the FabVariant enum so it describes behaviour from the caller's point of view rather than leaking client-side implementation details (the movable/anchored attributes, the Aura CSS-class-vs-theme-attribute mechanism, and Lumo design tokens). No behavioural change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make the ButtonVariant mapping and Aura CSS class private to FabVariant and expose intent-level applyColorTo(Button)/removeColorFrom(Button) instead of the toButtonVariant()/getAuraClass() accessors, so the underlying ButtonVariant and theme-class details no longer leak into the public API. ChatAssistant delegates color application to the variant; size handling is unchanged. Tests now verify the applied theme variant and Aura class through a real Button. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
| @@ -86,101 +117,208 @@ public class ChatAssistant<T extends Message> extends Div { | |||
| protected final Div resizerTopLeft = new Div(); | |||
|
|
|||
| protected static final int DEFAULT_FAB_SIZE = 60; | |||
| protected static final int DEFAULT_FAB_SMALL_SIZE = 50; | |||
| protected static final int DEFAULT_FAB_LARGE_SIZE = 72; | |||
| protected static final boolean DEFAULT_FAB_ANCHORED_TO_VIEWPORT = true; | |||
| protected static final boolean DEFAULT_RESIZE_INDICATORS_VISIBLE = false; | |||
| protected static final boolean DEFAULT_WINDOW_RESIZABLE = true; | |||
| protected static final boolean DEFAULT_FAB_MOVABLE = true; | |||
| protected static final ChatAssistantMode DEFAULT_MODE = ChatAssistantMode.DESKTOP; | |||
| protected static final int DEFAULT_MOBILE_BREAKPOINT = 768; | |||
| protected static final int DEFAULT_FAB_ICON_SIZE = 40; | |||
| protected static final int DEFAULT_FAB_MARGIN = 25; | |||
| protected static final int DEFAULT_RESIZER_SIZE = 25; | |||
| protected static final int DEFAULT_MAX_RESIZER_SIZE = 200; | |||
| protected static final int DEFAULT_DRAG_SENSITIVITY = 25; | |||
| protected static final FabPosition DEFAULT_POSITION = FabPosition.BOTTOM_RIGHT; | |||
| protected static final String DEFAULT_UNREAD_BADGE_BACKGROUND = "var(--lumo-warning-color)"; | |||
| protected static final String DEFAULT_UNREAD_BADGE_COLOR = "var(--lumo-warning-text-color)"; | |||
|
|
|||
| protected static final int DEFAULT_CONTENT_MIN_WIDTH = 150; | |||
| protected static final int DEFAULT_CONTENT_MIN_HEIGHT = 150; | |||
| protected static final String DEFAULT_POPOVER_TAG = "fc-chat-assistant-popover"; | |||
| protected static final String DEFAULT_DIALOG_TAG = "fc-chat-assistant-dialog"; | |||
| protected static final String DEFAULT_FAB_CLASS = "fc-chat-assistant-fab"; | |||
| protected static final String DEFAULT_RESIZE_CLASS = "fc-chat-assistant-resize"; | |||
| protected static final String RESIZE_INDICATOR_VISIBLE_CLASS = | |||
| "fc-chat-assistant-resize-indicator-visible"; | |||
| protected static final String DEFAULT_UNREAD_BADGE_CLASS = "fc-chat-assistant-unread-badge"; | |||
| protected static final String DEFAULT_FAB_ICON_SRC = loadDefaultFabIconSrc(); | |||
|
|
|||
| protected final VirtualList<T> content = new VirtualList<>(); | |||
| protected final List<T> messages = new ArrayList<>(); | |||
|
|
|||
| protected Component headerComponent; | |||
| protected Component footerContainer; | |||
| protected MessageInput messageInput; | |||
| protected Span whoIsTyping; | |||
| protected Registration defaultSubmitListenerRegistration; | |||
| protected int unreadMessages = 0; | |||
|
|
|||
| private int screenSizeKeySeq = 0; | |||
| private final Map<Integer, ScreenSizeListenerEntry> screenSizeListeners = new HashMap<>(); | |||
There was a problem hiding this comment.
Do we need to introduce all this protected API in a non-final class? Can it be made private instead?
| * @param minHeight the chat window minimum height ({@code null} keeps the default) | ||
| */ | ||
| @Builder | ||
| public ChatAssistant( |
There was a problem hiding this comment.
Please avoid a public constructor with so many arguments. Builder constructor can be private. Also: use boolean instead of Boolean so that caller cannot set them as null, which in turn avoids the need to defaulting null to a known value.
| icon.getStyle() | ||
| .setWidth(Math.min(size, DEFAULT_FAB_SIZE) + "px") | ||
| .setHeight(Math.min(size, DEFAULT_FAB_SIZE) + "px"); | ||
| this.fabIcon = icon instanceof SvgIcon ? (SvgIcon) icon : null; |
There was a problem hiding this comment.
Storing the same component as fabIcon and fabIconComponent seems confusing. Store it once and apply an instanceof check where needed
| for (ButtonVariant variant : variants) { | ||
| if (isSizeVariant(variant)) { | ||
| activeSizeVariant = variant; | ||
| setFabSize(variant == ButtonVariant.LUMO_LARGE ? DEFAULT_FAB_LARGE_SIZE : DEFAULT_FAB_SMALL_SIZE); |
There was a problem hiding this comment.
The logic is correct only if isSizeVariant accepts either LUMO_LARGE or LUMO_SMALL, thus it makes sense to inline it in the conditional.
| resizer.add(arrow); | ||
| } | ||
|
|
||
| protected void setResizerClass(Div resizer, String direction, boolean resizable) { |
There was a problem hiding this comment.
I'm not sure if this method must be protected
|
|
||
| /** | ||
| * Sets the number of unread messages to be displayed in the chat assistant. | ||
| * Sets the number of unread messages shown on the FAB badge. The value is clamped to the 0–99 |
There was a problem hiding this comment.
This change improves the documentation. Don't piggyback in unrelated commit.
| * @param background the background color of the unread badge | ||
| * @param color the text color of the unread badge | ||
| */ | ||
| public void setUnreadBadgeColors(String background, String color) { |
There was a problem hiding this comment.
This method seems to correspond to another feature.
| * #%L | ||
| * Chat Assistant Add-on | ||
| * %% | ||
| * Copyright (C) 2023 - 2025 Flowing Code |
There was a problem hiding this comment.
Please verify copyright year in modified sources.
| * #%L | ||
| * Chat Assistant Add-on | ||
| * %% | ||
| * Copyright (C) 2023 - 2025 Flowing Code |
There was a problem hiding this comment.
Please verify copyright year in modified sources
| public void testSerialization() throws ClassNotFoundException, IOException { | ||
| try { | ||
| ChatAssistant chatAssistant = new ChatAssistant(); | ||
| ChatAssistant<Message> chatAssistant = new ChatAssistant<Message>(); |
There was a problem hiding this comment.
The type argument in RHS of the assignment is not needed. Our style guide prefers diamond constructor invocation.
javier-godoy
left a comment
javier-godoy
left a comment
There was a problem hiding this comment.
🤖 Automated code review by Claude Code
15 verified findings from an xhigh-effort multi-agent review of this PR's diff against master (correctness regressions first, then maintainability cleanups). Each inline comment was produced by one finder agent and independently verified by a separate agent. CONFIRMED = reproduced from the code; PLAUSIBLE = mechanism verified but needs a runtime check. This is machine-generated — please confirm before acting.
| UI ui = attachEvent.getUI(); | ||
| Object registered = ComponentUtil.getData(ui, UI_INSTANCE_KEY); | ||
| if (registered != null && registered != this) { | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (correctness)
onAttach throws if another ChatAssistant is already registered on the UI
onAttach registers this instance under UI_INSTANCE_KEY and throws IllegalStateException("Only one ChatAssistant is supported per UI") if another instance is already registered. No such single-instance restriction existed in 5.0.x.
Failure scenario: Attach-before-detach route navigation (two routes each holding a ChatAssistant under the same parent layout), or a ChatAssistant inside a Dialog opened over a view that also has one: the second onAttach sees registered != this and throws, crashing the navigation / dialog open with a server error.
Introduced in 657f684 — WIP: address FAB API and behavior review feedback.
Note: this precondition was not part of the original feature commit — it was added a day later in the review-feedback round, so it may not have had the same scrutiny.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
| * @param width the width as an absolute CSS length | ||
| * @since 5.1.0 | ||
| */ | ||
| public void setWindowWidth(String width) { |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (correctness)
setWindowWidth/Height(String) no longer accept relative CSS units
The new path stores the raw value and the resize script's clamp() does parseFloat(value) then re-appends 'px', so relative units like '50%' / '30vw' are silently reinterpreted as pixels and clamped to the min bound. Master called overlay.setWidth(width), which accepted any CSS length.
Failure scenario: setWindowWidth("50%") previously opened the window at half the viewport width. Now clamp parses 50 from "50%", and because the default min width is 150px it returns "150px" — the window renders as a fixed 150px box instead of 50% of the viewport.
Introduced in 9570271 — feat: add FAB, window, and mode configuration to ChatAssistant.
Two-commit interaction: the server-side rerouting is here (9570271); the parseFloat(...)+'px' clamp that drops the unit lives in fcChatAssistantApplyConstraints, added in 1d79af0 ("add chat window resize and sizing frontend").
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
| String maxWidth, | ||
| String maxHeight) { | ||
| if (messages != null) { | ||
| this.messages.addAll(messages); |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (correctness)
(List, boolean) constructor copies the list instead of holding the caller's reference
The constructor now does this.messages.addAll(messages) into an owned internal ArrayList instead of retaining the caller's reference (master did this.messages = messages), so the ListDataProvider is no longer backed by the caller's list.
Failure scenario: A caller keeps a reference (List shared = new ArrayList<>(); new ChatAssistant<>(shared, false);), later appends to shared, then triggers a refresh (sendMessage(...) / refreshContent()). Previously refreshAll re-read the shared list and rendered the appended messages; now they never appear.
Introduced in 9570271 — feat: add FAB, window, and mode configuration to ChatAssistant.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
| .executeJs( | ||
| "window.fcChatAssistantScreenSize($0, $1, $2, $3, $4);", | ||
| this.getElement(), | ||
| overlay, |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (correctness)
Screen-size listener observes the popover overlay, never the mobile Dialog
addScreenSizeListener always observes the popover 'overlay' Div, but in MOBILE mode the chat content lives in mobileChatWindow (a Dialog) and the popover/overlay is never opened or sized, so the ResizeObserver watches an empty, closed surface reporting 0x0 (which notify() ignores).
Failure scenario: An app registers addScreenSizeListener(500, null, listener) to react to chat-window width. On a phone/narrow viewport (MOBILE mode) the listener callback is never invoked — not even the documented once-per-open initial delivery — so size-dependent behavior silently never runs while the chat is open on mobile.
Introduced in 9570271 — feat: add FAB, window, and mode configuration to ChatAssistant.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
| protected static final String CSS_MIN_HEIGHT = "--fc-min-height"; | ||
| protected static final String CSS_MAX_WIDTH = "--fc-max-width"; | ||
| protected static final String CSS_MAX_HEIGHT = "--fc-max-height"; | ||
| protected static final String DEFAULT_FAB_ICON_SRC = loadDefaultFabIconSrc(); |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (correctness)
Default FAB icon loaded in a static-final initializer that can throw
DEFAULT_FAB_ICON_SRC = loadDefaultFabIconSrc() runs at class init; a missing/unreadable /META-INF/resources/icons/chatbot.svg makes getResourceAsStream return null, in.readAllBytes() NPEs, and the rethrown IllegalStateException escapes as ExceptionInInitializerError (then NoClassDefFoundError on every later reference).
Failure scenario: In a repackaged/shaded uber-jar or classloader-isolation setup that strips META-INF/resources, every new ChatAssistant(...) in the JVM fails with an error that never mentions the icon — even for callers that always supply their own FAB icon. Master's lazy new SvgIcon("/icons/chatbot.svg") had no such class-load-time failure.
Introduced in 9570271 — feat: add FAB, window, and mode configuration to ChatAssistant.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
| protected final Dialog mobileChatWindow = new Dialog(); | ||
| protected final Div overlay = new Div(); | ||
| protected final VerticalLayout container = new VerticalLayout(); | ||
|
|
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (cleanup)
Eight resizer Divs hardcoded as separate fields, enumerated across three sites
resizerTop..resizerBottomLeft plus their direction strings are spelled out in applyGenericResizerStyle calls (405-467), setResizerClass calls (855-862), and the resize registrations (1235-1306). A single Map<String,Div> (direction -> resizer) would let all three sites iterate one collection.
Failure scenario: Maintainability: adding or renaming a handle requires touching all three enumeration sites in lockstep; missing one leaves a handle unstyled, non-toggleable, or unregistered.
Anchored here; the finding is at line 121 of the head file.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
| * while the window is closed (it no-ops until the content part exists, then applies on the next | ||
| * open). | ||
| */ | ||
| private void applyWindowConstraints() { |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (cleanup)
Each window setter spins a 20x100ms DOM-polling loop against a closed popover at construction
setUI calls all six size/bound setters (483-500); each issues an executeJs running fcChatAssistantApplyConstraints -> fcChatAssistantContentPart, which retries up to 20x at 100ms. At initial attach the popover is closed, so a fully-configured builder starts up to 6 concurrent ~2s polling loops (~120 wasted DOM queries) that never find the content part; the size is re-applied anyway on first open.
Failure scenario: Efficiency: skip the constraint push until the popover is open, or coalesce into one call.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
|
|
||
| // Resolves the popover overlay's [part='content'] element, retrying briefly because the overlay is | ||
| // (re)created lazily when the popover opens. | ||
| function fcChatAssistantContentPart(popoverTag, callback, attempts = 0) { |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (cleanup)
fcChatAssistantContentPart re-implements overlay + [part=content] resolution already in fetchOverlay
Both fetchOverlay (298-312) and fcChatAssistantContentPart (391-401) hardcode the same selectors ('.'+popoverTag shadowRoot -> vaadin-popover-overlay -> [part=content], with the getElementsByClassName fallback). Extract one shared resolveOverlay(popoverTag) helper.
Failure scenario: Maintainability: if the popover's DOM structure or tag changes, both copies must be updated; updating only one leaves either resize dragging or constraint application resolving a stale/null overlay.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
| * | ||
| * @param icon the icon component, it cannot be null | ||
| */ | ||
| public void setFabIcon(Component icon) { |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (cleanup)
setFabIcon(Component) and setFabIcon(Component,int) duplicate the assignment + setIcon logic
Both methods repeat this.fabIcon = icon instanceof SvgIcon ? ... : null; this.fabIconComponent = icon; applyIconSize(...); fab.setIcon(icon); differing only in the size clamp. The no-size overload could delegate to setFabIcon(icon, getFabIconSize()) (watch the edge case: getFabIconSize() can be 0 when fabSize<=20, which trips the size<=0 guard).
Failure scenario: Maintainability: a future change to icon assignment applied to one overload but not the other diverges the two entry points.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
| (max-height/width: 100%), so the content shrinks with the popover instead of overflowing. | ||
| The min/max bounds are applied as inline styles from JS (fcChatAssistantApplyConstraints), sourced from | ||
| the component's configured values, so they are intentionally not hardcoded here. */ | ||
| vaadin-popover-overlay.fc-chat-assistant-popover::part(content) { |
There was a problem hiding this comment.
🤖 Claude Code review — CONFIRMED (cleanup)
Popover and dialog ::part(content) rule bodies duplicated verbatim across V24/V25 selectors
The popover ::part(content) declaration block is repeated for vaadin-popover-overlay... and vaadin-popover..., and likewise the dialog pair, to cover both Vaadin 24 and 25 selectors. Combining the selectors with a comma keeps a single declaration block.
Failure scenario: Maintainability: a future tweak to the flex layout must be duplicated across both blocks; editing one leaves the other Vaadin version with the old layout.
🤖 Generated by Claude Code (Opus 4.8). Automated — verify before acting; PLAUSIBLE items need a runtime check.
|
Full review of the current branch state (24b1b98), cross-checked against all prior review threads. Skipping items already raised in javier-godoy's reviews — these are additional findings. Blockers1. All frontend detach cleanup is dead code. 2. FAB drag/snap still ignores container bounds (raised twice by CodeRabbit, still open). 3. README sample doesn't compile. 4.
Should fix
NitsFormatter-mangled comments in |
4 participants
Close #47 #64 #67
Chat Assistant Features (5.1.0-SNAPSHOT)
FAB (Floating Action Button)
Icon
setFabIcon(...), with a built-in chatbot icon used by default.Sizing
addFabThemeVariants(LUMO_SMALL, LUMO_LARGE)resizes the FAB (and its icon).LUMO_SUCCESS,LUMO_ERROR,LUMO_CONTRAST) restyle the FAB, with the icon automatically matching the button color.Positioning
setFabPosition(FabPosition)places the FAB in any of the four screen corners.resetFabPosition()restores the default position.Dragging
setFabMovable(...)allows the FAB to be dragged by the user.Bounded Placement
setFabAnchoredToViewport(false)positions the FAB inside a container instead of floating over the viewport.Unread badge colors
setUnreadBadgeColors(background, color)to change the badge background and text colors.Chat Window
Resizing
setWindowResizable(...)enables resizing using eight drag handles.setResizeIndicatorsVisible(...), showing arrows that indicate each handle's drag direction.Sizing & Bounds
setWindowWidth(...)setWindowHeight(...)setWindowMinWidth(...)setWindowMaxWidth(...)setWindowMinHeight(...)setWindowMaxHeight(...)Responsive / Mobile Mode
Modes
setMode(...)/setMobileMode(...)Auto-Switching
mobileBreakpoint.Listeners
addModeChangedListener(...)for mobile/desktop mode changes.addScreenSizeListener(...)for detecting when the chat window crosses a configured size threshold.Construction & Miscellaneous
Builder API
ChatAssistant.builder()provides a declarative configuration API.Native Markdown
markdown-editordependency has been removed.Summary by CodeRabbit
Summary
New Features
Bug Fixes
Documentation
Tests