Improvements: add mobile mode and other features#68
Open
Totremont wants to merge 30 commits into
Open
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 (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughVersion bumped to 5.1.0-SNAPSHOT. The PR replaces the markdown editor dependency with Vaadin Markdown, adds new FAB and mode enums, refactors ChatAssistant for desktop/mobile mode switching and sizing, updates frontend movement/resize behavior and styles, and expands demos, tests, and README coverage. 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
reviewed
Jun 30, 2026
mlopezFC
left a comment
mlopezFC
left a comment
Member
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 { |
Member
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 |
Member
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()); |
Member
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).
Comment on lines
+1381
to
+1383
| boolean movablePreference = this.fabMovable; | ||
| setFabMovable(false); | ||
| this.fabMovable = movablePreference; |
Member
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>(); |
Member
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
Contributor
Author
|
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
requested changes
Jul 1, 2026
javier-godoy
left a comment
javier-godoy
left a comment
Member
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>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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