Skip to content

Improvements: add mobile mode and other features#68

Open
Totremont wants to merge 64 commits into
masterfrom
chat-assistant-5.1.0
Open

Improvements: add mobile mode and other features#68
Totremont wants to merge 64 commits into
masterfrom
chat-assistant-5.1.0

Conversation

@Totremont

@Totremont Totremont commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Close #47 #64 #67

Chat Assistant Features (5.1.0-SNAPSHOT)

FAB (Floating Action Button)

Icon

  • Custom or default iconsetFabIcon(...), with a built-in chatbot icon used by default.

Sizing

  • addFabThemeVariants(LUMO_SMALL, LUMO_LARGE) resizes the FAB (and its icon).
  • Color variants (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.
  • Configurable margin support.

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.
  • Optional resize indicators via setResizeIndicatorsVisible(...), showing arrows that indicate each handle's drag direction.

Sizing & Bounds

  • Configure dimensions with:
    • setWindowWidth(...)
    • setWindowHeight(...)
    • setWindowMinWidth(...)
    • setWindowMaxWidth(...)
    • setWindowMinHeight(...)
    • setWindowMaxHeight(...)
  • Size constraints are respected both when opening the chat and while resizing.
  • Window size is persisted across close/reopen cycles.

Responsive / Mobile Mode

Modes

  • setMode(...) / setMobileMode(...)
    • Mobile: Full-screen dialog.
    • Desktop: Anchored popover.

Auto-Switching

  • Optional breakpoint-based switching using 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.
  • Legacy constructors remain fully supported.

Native Markdown

  • Message Markdown is now rendered using Vaadin's built-in Markdown component.
  • The external markdown-editor dependency has been removed.

Summary by CodeRabbit

Summary

  • New Features

    • Added explicit desktop vs mobile chat modes with responsive auto-switching.
    • Introduced FAB position presets and FAB style variants, plus unread badge color customization.
    • Improved Markdown message rendering using the built-in Markdown component.
  • Bug Fixes

    • Stabilized FAB dragging and resizing, including correct constraint clamping and reliable window size restoration after UI refreshes.
    • Improved open/close and mode synchronization across screen changes.
  • Documentation

    • Expanded the README with builder-based configuration, mobile/responsive guidance, and richer examples.
  • Tests

    • Added logic tests covering parsing, unread clamping, mode/threshold validation, and FAB behavior.

@Totremont
Totremont requested a review from mlopezFC June 30, 2026 15:31
@Totremont Totremont self-assigned this Jun 30, 2026
@Totremont Totremont added the enhancement New feature or request label Jun 30, 2026
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR bumps the project version, replaces MarkdownViewer rendering, adds mode and FAB model types, refactors ChatAssistant sizing and responsive behavior, updates frontend movement/resize support, and expands demos, tests, documentation, and metadata.

Changes

ChatAssistant 5.1 Feature Expansion

Layer / File(s) Summary
Core API, models, and markdown rendering
src/main/java/com/flowingcode/vaadin/addons/chatassistant/...
Adds mode and FAB enums, switches messages to Vaadin Markdown, and refactors ChatAssistant initialization, mode switching, sizing, unread badges, and screen-size listeners.
Frontend movement, resize, and styling
src/main/resources/META-INF/resources/frontend/...
Reworks FAB dragging, mobile observers, overlay sizing, resize constraints, teardown, indicators, and Markdown-related styling.
Demos and validation
src/test/java/com/flowingcode/vaadin/addons/chatassistant/..., src/test/...
Adds FAB, box, and responsive-mode demos; updates messaging demos; and adds logic and integration coverage for the revised behavior.
Documentation and release metadata
README.md, pom.xml, .gitignore, src/main/resources/META-INF/VAADIN/package.properties
Updates version and dependency metadata, expands usage documentation, adds package licensing text, and ignores generated frontend HTML files.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested reviewers: mlopezfc, javier-godoy, paodb

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR adds many unrelated features and demos, including mobile mode, FAB resizing/customization, and builder API, beyond the markdown bug fix. Split the unrelated feature work into separate PRs and keep this one focused on the markdown/lazy-loading rendering fix.
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is related, but 'other features' is vague and doesn't clearly summarize the main changes. Use a more specific title like 'Add mobile mode, FAB customization, and markdown rendering fixes'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The markdown and lazy-loading rendering issue appears addressed by switching to Vaadin Markdown and updating the lazy-loading demo.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chat-assistant-5.1.0

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Compute 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/innerHeight and viewport coordinates. For container-anchored FABs, this can jump or clamp the FAB outside its container. Also, Line 139 mutates position before the sensitivity check, so click-only jitter is later persisted by snapToBoundary().

🐛 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 win

Exercise the new listener bookkeeping in this roundtrip.

This still serializes only the message path. The PR added screenSizeListeners / ScreenSizeListenerEntry state to ChatAssistant, but this test never registers one, so a listener-serialization regression would still pass here. Add at least one addScreenSizeListener(...) before testSerializationOf(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 value

Consider guarding the delayed UI.access against a detached UI.

If the view is detached before the 5-second timer fires, currentUI.access(...) can throw UIDetachedException. For demo robustness, you could capture the optional UI and no-op when absent, or use ui.accessSynchronously-safe handling. Each click also spins up a fresh java.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

📥 Commits

Reviewing files that changed from the base of the PR and between 9345695 and 96035c5.

📒 Files selected for processing (28)
  • .gitignore
  • README.md
  • pom.xml
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatMessage.java
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/ChatAssistantMode.java
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/FabPosition.java
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/Message.java
  • src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js
  • src/main/resources/META-INF/resources/frontend/fc-chat-assistant-resize.js
  • src/main/resources/META-INF/resources/frontend/styles/fc-chat-assistant-style.css
  • src/main/resources/META-INF/resources/frontend/styles/fc-chat-message-styles.css
  • src/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.java
  • src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantBoxDemo.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemoView.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantFabConfigDemo.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantGenerativeDemo.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLazyLoadingDemo.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantMarkdownDemo.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantModeDemo.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/chatassistant/DemoView.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/BasicIT.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/ViewIT.java
  • src/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

Comment thread src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java Outdated
Comment thread src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java Outdated
Comment thread src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java Outdated

@mlopezFC mlopezFC left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @since tags on the new public/protected API (see inline comment on ChatAssistantMode). 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 (parseFabMargin fallbacks, setUnreadMessages clamping to 0–99, size-variant add/remove resizing, addScreenSizeListener argument validation, the setMode open-state reconciliation). Not a blocker — just flagging it as a nice-to-have.
  • Commit atomicity. d38f33b bundles three logical changes (markdown component swap + markdown-editor-addon removal + 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). Running mvn com.spotify.fmt:fmt-maven-plugin:format would normalize these.
  • ChatAssistant.java: chatWindow.setOpenOnClick(false) is set twice (in setUI and again in initializeChatWindow) — harmless redundancy.

What looks good

  • The native Markdown migration is clean — no leftover MarkdownViewer/markdown-editor references, and vaadin-markdown-flow is present for the pinned Vaadin 24.8.0.
  • ScreenSizeListenerEntry implements Serializable and the other new fields are serialization-safe.
  • Live-read isResizable()/shouldDrag() lets setWindowResizable take 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()/addChatSentListener API.

* 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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ChatAssistant methods: setMode/getMode/setMobileMode/isMobileMode/addModeChangedListener, setFabPosition/getFabPosition/resetFabPosition, addFabThemeVariants/removeFabThemeVariants, setFabAnchoredToViewport/isFabAnchoredToViewport, setResizeIndicatorsVisible/isResizeIndicatorsVisible, the setWindowMin*/Max*/Width/Height family, addScreenSizeListener, setUnreadMessages/getUnreadMessages/setUnreadBadgeColors, setMobileModeSwitchingEnabled/isMobileModeSwitchingEnabled/getMobileBreakpoint.

Place it after the main description, before any @param/@return.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Please check if this comment has been addressed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

model_dir="src/main/java/com/flowingcode/vaadin/addons/chatassistant/model"
assistant_file="src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java"

echo '=== Relevant Java source files ==='
fd -t f -e java . src/main/java/com/flowingcode/vaadin/addons/chatassistant | sort

echo
echo '=== Model/event declarations and `@since` context ==='
rg -n -C 8 \
  'public (enum|class) (ChatAssistantMode|FabPosition|ModeChangedEvent|ScreenSizeEvent|ScreenSizeDirection)\b|`@since`\b' \
  "$model_dir" "$assistant_file" || true

echo
echo '=== Requested ChatAssistant method declarations and `@since` context ==='
rg -n -C 10 \
  'public .*\b(setMode|getMode|setMobileMode|isMobileMode|addModeChangedListener|setFabPosition|getFabPosition|resetFabPosition|addFabThemeVariants|removeFabThemeVariants|setFabAnchoredToViewport|isFabAnchoredToViewport|setResizeIndicatorsVisible|isResizeIndicatorsVisible|setWindow(?:Min|Max)?(?:Width|Height)|addScreenSizeListener|setUnreadMessages|getUnreadMessages|setUnreadBadgeColors|setMobileModeSwitchingEnabled|isMobileModeSwitchingEnabled|getMobileBreakpoint)\s*\(' \
  "$assistant_file" || true

Length of output: 78601


@javier-godoy, this is partially addressed, but the comment should remain unresolved.

@since 5.1.0 was added to the enums, events, and most listed APIs. However, it is still missing from these explicitly requested methods in ChatAssistant.java:

  • setWindowMinWidth(String) — Line 984
  • setWindowMinHeight(String) — Line 1017
  • setWindowMaxWidth(String) — Line 1028
  • setWindowMaxHeight(String) — Line 1050
  • setWindowHeight(String) — Line 1072
  • setWindowWidth(String) — Line 1091
  • getUnreadMessages() — Line 1439
  • setUnreadMessages(int) — Line 1449

Also, where present, the new @since tags are generally placed after @param/@return (for example, setMode at Line 1492), while the original request specified placing @since immediately after the description and before other Javadoc tags.

* #%L
* Chat Assistant Add-on
* %%
* Copyright (C) 2023 - 2024 Flowing Code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Please check if this comment has been addressed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Explicitly named files ==='
for file in \
  src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/ChatAssistantMode.java \
  src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/FabPosition.java \
  src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantModeDemo.java
do
  printf '\n--- %s ---\n' "$file"
  if [ -f "$file" ]; then
    rg -n -m1 'Copyright' "$file" || true
  else
    printf '%s\n' 'File not found'
  fi
done

printf '\n%s\n' '=== Frontend JavaScript copyright headers ==='
fd -e js . src/main/resources/META-INF/resources/frontend \
  -x sh -c 'printf "\n--- %s ---\n" "$1"; rg -n -m1 "Copyright" "$1" || true' sh {}

printf '\n%s\n' '=== Distinct copyright years in the affected scope ==='
rg -n 'Copyright \(C\)' \
  src/main/java/com/flowingcode/vaadin/addons/chatassistant/model \
  src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantModeDemo.java \
  src/main/resources/META-INF/resources/frontend \
  | sed -E 's/.*Copyright \(C\) ([0-9]{4} - [0-9]{4}).*/\1/' \
  | sort -u

Length of output: 2058


@javier-godoy, verified: this has been addressed. The affected Java and frontend JavaScript files now consistently use Copyright (C) 2023 - 2026 Flowing Code.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

}
}

window.addEventListener('resize', () => updateCanDrag());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Please check if this comment has been addressed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

resize_file="src/main/resources/META-INF/resources/frontend/fc-chat-assistant-resize.js"
movement_file="src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js"

echo "== Resize teardown and resource registration =="
rg -n -C 5 --fixed-strings \
  -e "const fetchOverlayTimeout = setTimeout(fetchOverlay, 2000);" \
  -e "window.addEventListener('resize', resizeHandler);" \
  -e "root.__fcCleanups" \
  -e "window.removeEventListener('resize', resizeHandler);" \
  -e "styleObserver?.disconnect();" \
  -e "clearTimeout(fetchOverlayTimeout);" \
  "$resize_file"

echo
echo "== animated-fab disconnected callback =="
rg -n -C 5 --fixed-strings \
  -e "customElements.define('animated-fab'" \
  -e "disconnectedCallback()" \
  -e "__fcCleanups" \
  "$movement_file"

Length of output: 6155


@javier-godoy Yes, this has been addressed.

fc-chat-assistant-resize.js now registers a detach cleanup callback that:

  • Removes its window resize listener.
  • Disconnects styleObserver.
  • Cancels the delayed fetchOverlay timeout.
  • Clears the initialization guard for proper reattachment.

The animated-fab custom element’s disconnectedCallback() executes and clears root.__fcCleanups, so the resize cleanup is actually invoked on detach. The original resource-retention concern no longer applies.


🧬 Code Graph Analysis Results

Relevant snippet: src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js

Lines 97-104

resizeHandler = (_) => {
        screenWidth = window.innerWidth;
        screenHeight = window.innerHeight;

        // The popover content part clamps itself to the viewport (max-height/width: 100%), so no
        // manual container shrinking is needed here. Just keep the FAB within the new screen bounds.
        snapToBoundary();
    }

Lines 177-190

function stopDragging(e) {
        if(isDragging) {
            isDragging = false;
            item.style.transition = snapTransition + ', ' + sizeTransition;
            fab.classList.remove('dragging');
            if (item.hasPointerCapture(e.pointerId)) {
                item.releasePointerCapture(e.pointerId);
            }
            snapToBoundary();
            if (isClickOnlyEvent()) {
                root.$server?.onClick();
            }
        }
    }

Comment on lines +1381 to +1383
boolean movablePreference = this.fabMovable;
setFabMovable(false);
this.fabMovable = movablePreference;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Please check if this comment has been addressed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javier-godoy, this has been addressed.

ChatAssistant now preserves the desktop preference separately in desktopFabMovablePreference. Entering MOBILE calls setFabMovable(false) without restoring fabMovable, so isFabMovable() correctly returns false during mobile mode. The saved preference is restored only when switching back to DESKTOP.

public void testSerialization() throws ClassNotFoundException, IOException {
try {
ChatAssistant chatAssistant = new ChatAssistant();
ChatAssistant<Message> chatAssistant = new ChatAssistant<Message>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the style guide prefers diamond syntax — new ChatAssistant<>() instead of new ChatAssistant<Message>().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Please check if this comment has been addressed.

Totremont added 22 commits July 1, 2026 12:58
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 and others added 24 commits July 20, 2026 12:48
resize.js disconnectedCallback tore down the window resize listener, style
observer and overlay-lookup timeout but never cleared the per-direction
init guard on the durable resizer Div (nor the server refresh guard), so on
reattach fcChatAssistantResize early-returned and the resize handlers/
indicators were never re-established. Clear both guards on disconnect (which
only fires on a genuine detach, not on popover reopen).
Addresses PR #68 review r3546279335.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The all-args builder constructor was public with many boxed Boolean flags
that had to be null-defaulted in setUI. Make it private (construction is via
ChatAssistant.builder() or the public legacy constructors) and switch the
flags to primitive boolean so callers cannot pass null. A small partial
ChatAssistantBuilder seeds the real defaults for the flags whose default is
not false, and setUI no longer null-coalesces. Addresses PR #68 review
r3545748934.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
applyGenericResizerStyle and setResizerClass are internal helpers used only
within ChatAssistant to build/toggle the resize handles; they need not be
part of the protected extension surface. Make them private.
Addresses PR #68 review r3546022009.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
add/removeFabThemeVariants branched on variant.isSizeVariant() and then
assumed the variant was LARGE or SMALL (variant == LARGE ? ... : SMALL).
Inline the explicit SMALL/LARGE check so that assumption is provable at the
call site and a future non-size variant cannot fall into the size branch.
isSizeVariant() remains as public API. Addresses PR #68 review r3545985941.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the redundant explicit type argument on the right-hand side; the target
type already provides it, per the style guide's diamond preference.
Addresses PR #68 review r3546149078.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The popover ::part(content) declaration block was duplicated across the
Vaadin 24 (vaadin-popover-overlay) and Vaadin 25 (vaadin-popover) selectors,
and likewise for the dialog pair. Combine each pair into a single
comma-separated rule (valid under lightningcss) so future tweaks live in one
place. Addresses PR #68 review r3546279367.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fetchOverlay and fcChatAssistantContentPart each hardcoded the same overlay
selector chain (popover class -> shadowRoot -> vaadin-popover-overlay, with a
getElementsByClassName fallback). Extract a single resolveOverlay(popoverTag)
helper so the DOM structure is described once.
Addresses PR #68 review r3546279356.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both setFabIcon(Component) and setFabIcon(Component, int) repeated the icon
assignment, fab.setIcon and applyIconSize sequence. Extract a private
applyFabIcon(icon, size) helper. The no-size overload keeps its own path
(getFabIconSize() can be 0, which is valid here but rejected by the sized
overload's size > 0 guard). Addresses PR #68 review r3546279361.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Each of the six window size/bound setters ran an executeJs that polled the
popover content part up to 20x100ms; at construction the popover is closed,
so a fully-configured builder spun ~6 pointless ~2s polling loops. Store the
desired size in the --fc-* custom properties on the durable overlay Div from
Java and only push to the client when the popover is open (the open listener
already re-applies everything via fcChatAssistantRestoreWindowSize). The
now-unused fcChatAssistantSetWindowSize JS helper is removed.
Addresses PR #68 review r3546279350.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The eight resizer Divs were separate fields enumerated across three sites
(edge styling, resizable class toggling, and client registration). Replace
them with a single Map<String,Div> keyed by direction plus a
RESIZER_DIRECTIONS list, and iterate that one collection at each site, so
adding or renaming a handle is a single-line change. Per-direction edge
styling moves into applyResizerEdgeStyle. Addresses PR #68 review r3546279344.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The root <animated-fab> element has no backing web component, so the
root.disconnectedCallback assigned by the movement and resize modules was
never invoked by the browser: the window resize listeners, style observer,
overlay-lookup timeout and screen-size/mobile observers leaked on detach and
the init guards stayed set (blocking reattach). Register a minimal
animated-fab custom element whose disconnectedCallback runs teardown
callbacks accumulated on root.__fcCleanups, and have both modules push their
cleanup there.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
setFabIcon(Component) now calls setFabIcon(icon, getFabIconSize()) instead of
duplicating the assignment/install/size logic. getFabIconSize() returns at
least 1px so it never trips the sized overload's size > 0 guard when fabSize
is small.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pointermove mutated position before the sensitivity check returned, so a
click with tiny movement was committed by stopDragging -> snapToBoundary ->
updatePosition, shifting the FAB. Compute a candidate position and only
commit it (and update the DOM) once the move exceeds the drag sensitivity.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
parseFabMargin accepted a negative value (e.g. "-5"), which would pull the
FAB off the edge. Fall back to the default for negatives, and cover it with
a test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
streamWords swallowed InterruptedException (re-set the flag but kept
emitting), so after onDetach's executor.shutdownNow() the loop kept calling
ui.access on a detached view. Replace the stream with a plain loop whose
Thread.sleep propagates the interrupt and aborts streaming.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
addFabThemeVariants takes FabVariant, not ButtonVariant. Use
FabVariant.LARGE/LUMO_CONTRAST in the snippet and correct the feature-list
wording so the sample compiles.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove @SInCE 5.1.0 from the String window-size setters, getUnreadMessages
and setUnreadMessages(int) (all present since 5.0.0), and add @SInCE 5.1.0 to
setWindowResizable/isWindowResizable, which are new in 5.1.0 (also normalizing
the malformed isWindowResizable javadoc). Verified against tag
chat-assistant-addon-5.0.0.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FabVariant.applyColorTo/removeColorFrom were public methods that mutated a
Button, leaking the apply logic. Make FabVariant a data-only enum exposing
getButtonVariant()/getAuraClass(), and apply/remove the color from private
ChatAssistant methods. Tests now assert the variant's data mapping.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the Lombok-internals explanation on ChatAssistantBuilder, the
breaking-change rationale on the mobile breakpoint, and the redundant
single-instance comment in onAttach (the code and warning already say it).
Reflow comments that had been wrapped into orphaned one-word lines (these
render verbatim in the @demosource viewer).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The minWidth/minHeight fields were only ever assigned; the real state is the
--fc-min-width/--fc-min-height custom properties on the overlay Div (read by
fcChatAssistantApplyConstraints). Remove the dead fields and set the custom
properties directly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The component root tag is animated-fab, but the ViewIT selector, the
ChatAssistantElement page object and the demo CSS still referenced the old
chat-bot tag. Update them so the ITs resolve the component (and, with the
custom-element registration, ViewIT's upgrade assertion now holds).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Register an addScreenSizeListener before serializing so the screen-size
listener map (ScreenSizeListenerEntry) is exercised by the round-trip.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the redundant String.valueOf(...) in the int window-size setters
(int + "px" already yields a String), and add a literal fallback to the
who-is-typing color var chain so it degrades when neither the Lumo nor
Vaadin token is defined.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@Totremont

Copy link
Copy Markdown
Contributor Author

@scardanzan Thanks for the thorough pass — all addressed on top of the existing WIP history (commits noted):

Blockers

  1. Dead detach cleanup — <animated-fab> is now a registered custom element whose disconnectedCallback runs teardown from root.__fcCleanups, so listener/observer cleanup and guard clearing actually run (ff5ee1d).
  2. FAB drag/snap container bounds — no change needed: dragging is gated on movable && anchored (movement.js:131/174), so it's disabled when unanchored; verified the FAB/window stay in-bounds.
  3. README sample — FabVariant instead of ButtonVariant (9c182e3).
  4. @since tags — removed the wrong 5.1.0 tags on the 5.0.0 API and added the missing ones (047d970).

Should fix
5. Click jitter — sub-threshold moves are no longer committed to position (39c1edb).
6. Reattach init guards — resolved by the custom-element teardown above (ff5ee1d).
7. FabVariant.applyColorTo/removeColorFrom — moved into private ChatAssistant methods; the enum is now data-only (f463c95).
8. SerializationTest — now registers a screen-size listener (16ce7b0).
9. GenerativeDemo — aborts the stream on interrupt (1f76604).
10. IT tag chat-botanimated-fab (fa0c653).
11. Assorted — isBlank/isEmpty (a59143e); parseFabMargin negatives (14799ca); write-only minWidth/minHeight removed (67998fc); malformed * */ and empty @param (047d970, 9761b99). setFabMovable javadoc already matches the mobile behavior (8206630). getMobileBreakpoint has no setter — left as-is; can add setMobileBreakpoint if you want it reconfigurable.

Nits — reflowed the formatter-mangled comments (06b0a72); String.valueOf(x)+"px" simplification and the CSS literal fallback (5042e78). The uncancellable retry loop is now cleared on detach via the teardown (ff5ee1d). Left for now (happy to do any): the unreferenced SVG clipPaths, the snapToBoundary mid-transition measure (no visible bounds issue in testing), and the redundant v25 jetty.version (to avoid touching pom.xml alongside an unrelated pom change).

@Totremont
Totremont requested a review from javier-godoy July 20, 2026 21:36

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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)

91-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use container-aware bounds for drag and snap pointermove and snapToBoundary() still clamp against window.innerWidth/innerHeight, so an anchored FAB can be dragged or snapped outside its offset parent. Reuse fcChatAssistantBounds(item) here too.

🤖 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 91 - 92, Update the drag and snap boundary calculations in the
pointermove handler and snapToBoundary() to use fcChatAssistantBounds(item)
instead of window.innerWidth/innerHeight. Ensure both movement clamping and
boundary snapping respect the anchored item’s offset parent dimensions.
🧹 Nitpick comments (2)
src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js (2)

107-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the root.__fcCleanups lazy-init assignment out of the push() call expression.

Both files repeat (root.__fcCleanups = root.__fcCleanups || []).push(...), flagged by SonarCloud in each location. Splitting the assignment onto its own line improves readability without behavior change.

  • src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js#L107-L114: split the lazy-init assignment from the .push(...) call.
  • src/main/resources/META-INF/resources/frontend/fc-chat-assistant-resize.js#L317-L323: apply the same split here.
♻️ Proposed fix (apply to both sites)
-    (root.__fcCleanups = root.__fcCleanups || []).push(() => {
+    root.__fcCleanups = root.__fcCleanups || [];
+    root.__fcCleanups.push(() => {
         ...
     });
🤖 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 107 - 114, Split the lazy initialization of root.__fcCleanups from
the .push(...) call in the cleanup registration block of
fc-chat-assistant-movement.js, assigning the array on its own statement before
pushing the cleanup callback. Apply the same change in
fc-chat-assistant-resize.js at the cited site; preserve behavior and callback
contents.

Source: Linters/SAST tools


210-220: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

IntersectionObserver for deferred corner placement isn't torn down on detach.

If the FAB never reaches a non-zero size (e.g., stays in a hidden tab) before the component is detached, this observer is never disconnected and keeps observing the detached fab node indefinitely. Consider pushing observer.disconnect() into root.__fcCleanups alongside the other teardown callbacks for consistency.

♻️ Proposed fix
     } else {
         const observer = new IntersectionObserver((_, obs) => {
             if (fcChatAssistantSize(fab).width > 0) {
                 obs.disconnect();
                 applyCorner();
             }
         });
         observer.observe(fab);
+        (root.__fcCleanups = root.__fcCleanups || []).push(() => observer.disconnect());
     }
🤖 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 210 - 220, Register the deferred IntersectionObserver’s disconnect
callback in root.__fcCleanups when it is created, alongside the existing
teardown callbacks. Keep the current disconnect-and-applyCorner behavior when
the FAB becomes visible, while ensuring the observer is also disconnected if the
component is detached first.
🤖 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/test/java/com/flowingcode/vaadin/addons/chatassistant/it/ViewIT.java`:
- Around line 61-62: Update the assertion in ViewIT around the animated-fab
lookup to stop using hasBeenUpgradedToCustomElement, which requires a shadow
root. Assert that the animated-fab custom element is upgraded or that its
initialization side effect occurs, while preserving the existing element
selection.

---

Outside diff comments:
In
`@src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js`:
- Around line 91-92: Update the drag and snap boundary calculations in the
pointermove handler and snapToBoundary() to use fcChatAssistantBounds(item)
instead of window.innerWidth/innerHeight. Ensure both movement clamping and
boundary snapping respect the anchored item’s offset parent dimensions.

---

Nitpick comments:
In
`@src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js`:
- Around line 107-114: Split the lazy initialization of root.__fcCleanups from
the .push(...) call in the cleanup registration block of
fc-chat-assistant-movement.js, assigning the array on its own statement before
pushing the cleanup callback. Apply the same change in
fc-chat-assistant-resize.js at the cited site; preserve behavior and callback
contents.
- Around line 210-220: Register the deferred IntersectionObserver’s disconnect
callback in root.__fcCleanups when it is created, alongside the existing
teardown callbacks. Keep the current disconnect-and-applyCorner behavior when
the FAB becomes visible, while ensuring the observer is also disconnected if the
component is detached first.
🪄 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: 013770e9-2b0e-4d44-8873-d99fb40b470c

📥 Commits

Reviewing files that changed from the base of the PR and between 24b1b98 and 5042e78.

📒 Files selected for processing (15)
  • README.md
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatMessage.java
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/model/FabVariant.java
  • src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js
  • src/main/resources/META-INF/resources/frontend/fc-chat-assistant-resize.js
  • src/main/resources/META-INF/resources/frontend/styles/fc-chat-assistant-style.css
  • src/main/resources/META-INF/resources/frontend/styles/fc-chat-message-styles.css
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantFabConfigDemo.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantGenerativeDemo.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLogicTest.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/ViewIT.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/po/ChatAssistantElement.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/test/SerializationTest.java
  • src/test/resources/META-INF/frontend/styles/chat-assistant-styles-demo.css
🚧 Files skipped from review as they are similar to previous changes (9)
  • src/test/resources/META-INF/frontend/styles/chat-assistant-styles-demo.css
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/po/ChatAssistantElement.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/test/SerializationTest.java
  • src/main/resources/META-INF/resources/frontend/styles/fc-chat-message-styles.css
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLogicTest.java
  • src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantFabConfigDemo.java
  • src/main/resources/META-INF/resources/frontend/styles/fc-chat-assistant-style.css
  • src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatMessage.java
  • README.md

Comment on lines +61 to +62
TestBenchElement element = $("animated-fab").first();
MatcherAssert.assertThat(element, hasBeenUpgradedToCustomElement);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for animated-fab definition to see if it attaches a shadow root
rg 'customElements\.define' -A 15 -g '*.js'
# Search for attachShadow
rg 'attachShadow' -g '*.js'

Repository: FlowingCode/ChatAssistant

Length of output: 163


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the component definition and nearby implementation details.
rg -n "animated-fab|customElements\.define|attachShadow|shadowRoot" .

# If a likely Java/TS source file is found, show the relevant section with line numbers.
# (This is a read-only inspection; no files are modified.)

Repository: FlowingCode/ChatAssistant

Length of output: 2041


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== ChatAssistant.java =="
sed -n '70,110p' src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java

echo
echo "== fc-chat-assistant-movement.js =="
sed -n '1,80p' src/main/resources/META-INF/resources/frontend/fc-chat-assistant-movement.js

echo
echo "== ViewIT.java =="
sed -n '1,90p' src/test/java/com/flowingcode/vaadin/addons/chatassistant/it/ViewIT.java

Repository: FlowingCode/ChatAssistant

Length of output: 7401


Stop requiring a shadow root hereanimated-fab is registered as a minimal HTMLElement with no attachShadow(), so this matcher will fail for the current implementation. Assert the custom-element upgrade or its init side effects instead.

🤖 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/it/ViewIT.java`
around lines 61 - 62, Update the assertion in ViewIT around the animated-fab
lookup to stop using hasBeenUpgradedToCustomElement, which requires a shadow
root. Assert that the animated-fab custom element is upgraded or that its
initialization side effect occurs, while preserving the existing element
selection.

@javier-godoy javier-godoy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending comments:

To be verified: Add-ons compatible with Vaadin 25 must support both Lumo and Aura themes.

Since this PR accumulated several WIP commits, I would like to revisit it after @mlopezFC and @scardanzan complete their review, and all the WIP commits are rebased (which is also an opportunity for rebasing the other non-atomic commits).

@scardanzan

Copy link
Copy Markdown
Member

@Totremont

  1. The integration-test fix introduced a new failure — CodeRabbit flagged this and it's still open. ViewIT now targets animated-fab, but the hasBeenUpgradedToCustomElement matcher asserts the element has a shadowRoot with children (ViewIT.java:45). The new animated-fab is a minimal HTMLElement with no attachShadow(), so componentWorks() will fail. Needs a different assertion (upgrade check or an init side-effect).

  2. My drag/bounds blocker was resolved by design, not by the fix I suggested. The author's reasoning checks out: dragging is gated on movable && anchored (movement.js:146), and the anchored attribute is removed when setFabAnchoredToViewport(false) (ChatAssistant.java:930) — so a container-bounded FAB simply isn't draggable, and the viewport-vs-container math never runs. No bug, but it means "movable + not-anchored" is silently a no-op; worth a Javadoc note.

Still open

  1. @ since placement: new tags sit after @param (e.g. setMode), but FC convention puts @ since right after the description — CodeRabbit noted this. Minor.
  2. Note CodeRabbit is wrong on its other @ since complaint: it re-flags 8 window/unread methods as "still missing @ since", but those existed in 5.0.0/5.0.1 (verified against the tags), so leaving them untagged is correct.
  3. (List, boolean) constructor still copies via this.messages.addAll (line 280) — javier's automated review flagged this as a regression vs master (callers mutating their own list no longer see updates); not in the author's response, appears unaddressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Rendering problems

4 participants