Fix scrolling and improve responsive support #46
Conversation
WalkthroughAdds a Vaadin Flow frontend connector and JS module annotation, invokes the connector on component attach, updates Java layout sizing, and revises CSS to improve popover resizing and mobile full-viewport behavior for the chat assistant. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java(8 hunks)src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css(2 hunks)src/main/resources/META-INF/resources/frontend/connector.js(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-22T20:00:43.518Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
Applied to files:
src/main/resources/META-INF/frontend/styles/chat-assistant-styles.csssrc/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.javasrc/main/resources/META-INF/resources/frontend/connector.js
📚 Learning: 2025-07-24T13:40:40.471Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-24T13:40:40.471Z
Learning: In ChatAssistant's initializeAvatar method (src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java), direct DOM manipulation using executeJs is intentionally used to move the Avatar component to be a child of a button that is inside a div within the React-controlled web component structure. This approach is necessary because the target elements are created by React, and standard Vaadin component hierarchy methods cannot be used due to ReactAdapterComponent limitations.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
📚 Learning: 2025-07-22T19:52:28.019Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-22T19:52:28.019Z
Learning: When using ReactAdapterComponent with custom React components in ChatAssistant, standard Vaadin component hierarchy methods like add() cannot be used because the component doesn't implement HasComponents, and the DOM structure is controlled by React. Direct DOM manipulation with executeJs may be necessary to integrate Vaadin components (like Avatar) into React-generated DOM structures.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
📚 Learning: 2025-07-24T13:41:19.117Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/react/animated-fab.tsx:0-0
Timestamp: 2025-07-24T13:41:19.117Z
Learning: In the ChatAssistant animated-fab React component (src/main/resources/META-INF/frontend/react/animated-fab.tsx), the Floating Action Button (FAB) appears empty in the React code but gets its icon content from the Avatar Vaadin component, which is inserted via DOM manipulation from the Java backend's initializeAvatar method.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
🧬 Code Graph Analysis (1)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (1)
src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java (1)
DemoSource(37-105)
🔇 Additional comments (9)
src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css (4)
22-29: Good responsive improvements!The switch from fixed dimensions to CSS custom properties with sensible defaults enhances flexibility. The minimum dimensions (30vh height, 310px width) ensure usability across different viewport sizes.
33-34: Proper flex layout configuration.Using
flex-grow: 1and Lumo spacing variables maintains consistency with the Vaadin design system while enabling proper content expansion.
45-56: Excellent viewport constraint implementation.These styles effectively prevent the popover from exceeding viewport boundaries and handle text overflow gracefully with
word-break: break-word.
58-75: Well-implemented mobile responsiveness.The mobile breakpoint correctly switches to fullscreen mode and disables resizing, addressing the mobile usability issues mentioned in issue #43.
src/main/resources/META-INF/resources/frontend/connector.js (2)
94-120: Well-implemented viewport clamping logic.The function correctly calculates available space and applies appropriate constraints to keep the resizable container within viewport boundaries with proper padding.
137-162: Efficient position tracking implementation.Good use of
requestAnimationFrameand position buffering to minimize unnecessary recalculations while tracking overlay movement.src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (3)
28-71: Correct integration of frontend connector.The JavaScript annotation and imports properly set up the frontend resize functionality.
134-149: Improved input and footer sizing.The width-focused sizing and height constraint on the MessageInput improve the responsive layout behavior.
160-185: Excellent responsive layout configuration.The flex layout settings and JavaScript connector initialization properly enable dynamic resizing and viewport-aware behavior.
| new ResizeObserver(() => { | ||
| const popoverOverlay = resizableContainer.parentElement; | ||
| const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'); | ||
| if (!overlay) return; | ||
|
|
||
| // Debounce calls to avoid excessive recalculations on rapid resize | ||
| debounce(() => clampToViewport(resizableContainer)); | ||
| }).observe(resizableContainer); |
There was a problem hiding this comment.
Fix incorrect debounce usage.
The debounce function returns a debounced function but line 64 calls it directly each time. This creates a new debounced instance on every resize event, defeating the purpose of debouncing.
Store the debounced function and reuse it:
+ const debouncedClamp = debounce(() => clampToViewport(resizableContainer));
new ResizeObserver(() => {
const popoverOverlay = resizableContainer.parentElement;
const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]');
if (!overlay) return;
- // Debounce calls to avoid excessive recalculations on rapid resize
- debounce(() => clampToViewport(resizableContainer));
+ debouncedClamp();
}).observe(resizableContainer);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| new ResizeObserver(() => { | |
| const popoverOverlay = resizableContainer.parentElement; | |
| const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'); | |
| if (!overlay) return; | |
| // Debounce calls to avoid excessive recalculations on rapid resize | |
| debounce(() => clampToViewport(resizableContainer)); | |
| }).observe(resizableContainer); | |
| // Create and reuse a single debounced function for clamping | |
| const debouncedClamp = debounce(() => clampToViewport(resizableContainer)); | |
| new ResizeObserver(() => { | |
| const popoverOverlay = resizableContainer.parentElement; | |
| const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'); | |
| if (!overlay) return; | |
| debouncedClamp(); | |
| }).observe(resizableContainer); |
🤖 Prompt for AI Agents
In src/main/resources/META-INF/resources/frontend/connector.js around lines 58
to 65, the code creates a new debounced function on every ResizeObserver
callback call; instead, create the debounced wrapper once (e.g. const
debouncedClamp = debounce(() => clampToViewport(resizableContainer), 100))
before constructing the ResizeObserver, then inside the observer callback call
debouncedClamp(); this ensures the same debounced function is reused and
prevents the debounce from being reset on every resize event.
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/main/resources/META-INF/resources/frontend/connector.js (3)
3-3: Fix incorrect project name in license header.Should be "Chat Assistant Add-on".
- * Markdown Editor Add-on + * Chat Assistant Add-on
58-65: Fix incorrect debounce usage (new instance is created and never invoked).Create the debounced function once and call it in the observer.
- new ResizeObserver(() => { + const debouncedClamp = debounce(() => clampToViewport(resizableContainer)); + new ResizeObserver(() => { const popoverOverlay = resizableContainer.parentElement; const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'); if (!overlay) return; - // Debounce calls to avoid excessive recalculations on rapid resize - debounce(() => clampToViewport(resizableContainer)); + // Debounced to avoid excessive recalculations on rapid resize + debouncedClamp(); }).observe(resizableContainer);
21-22: Align file name with the connector namespace and packaging.Consider renaming the file to fcChatAssistantConnector.js to match window.Vaadin.Flow.fcChatAssistantConnector and, for Flow 24+, placing it under META-INF/frontend and importing via @jsmodule.
If you proceed, remember to update the Java annotation path accordingly and ensure the file sits in META-INF/frontend so @jsmodule("./fcChatAssistantConnector.js") can resolve it.
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (1)
31-33: Prefer @jsmodule over @javascript for Flow 24 add-ons; adjust resource location accordingly.Use ES module import and place the asset under META-INF/frontend.
-import com.vaadin.flow.component.dependency.JavaScript; import com.vaadin.flow.component.dependency.JsModule; @@ -@JavaScript("./connector.js") +@JsModule("./connector.js")Note: If you rename the file (e.g., fcChatAssistantConnector.js) or move it to META-INF/frontend, update the path here to match.
Also applies to: 71-71
🧹 Nitpick comments (3)
src/main/resources/META-INF/resources/frontend/connector.js (2)
47-56: Only clear constraints when user actually grabs the resize handle.Current listeners fire on any click inside the container, causing unnecessary constraint resets.
- // On drag/resize start (mouse), reset size restrictions so user can freely resize - resizableContainer.addEventListener("mousedown", e => { - resizableContainer.style.maxHeight = ''; - resizableContainer.style.maxWidth = ''; - }); - // On drag/resize start (touch), reset size restrictions so user can freely resize - resizableContainer.addEventListener("touchstart", e => { - resizableContainer.style.maxHeight = ''; - resizableContainer.style.maxWidth = ''; - }); + // On drag/resize start, reset constraints only if the pointer is near the edges (resize handle) + resizableContainer.addEventListener("pointerdown", e => { + if (!isOnResizeHandle(e, resizableContainer)) return; + resizableContainer.style.maxHeight = ''; + resizableContainer.style.maxWidth = ''; + });Add this helper next to other helpers:
function isOnResizeHandle(e, el, edge = 12) { const rect = el.getBoundingClientRect(); const x = e.clientX ?? (e.touches && e.touches[0]?.clientX); const y = e.clientY ?? (e.touches && e.touches[0]?.clientY); if (x == null || y == null) return false; const nearLeft = x - rect.left <= edge; const nearRight = rect.right - x <= edge; const nearTop = y - rect.top <= edge; const nearBottom = rect.bottom - y <= edge; return (nearLeft || nearRight) && (nearTop || nearBottom); }
40-41: Rename variable to avoid confusion.popoverOverlay is actually the popover element, not the overlay. Use popoverEl for clarity.
- const popoverOverlay = resizableContainer.parentElement; - const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'); + const popoverEl = resizableContainer.parentElement; + const overlay = popoverEl.shadowRoot?.querySelector('[part="overlay"]');src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (1)
183-185: Avoid UI.getCurrent() inside attach; use the event’s UI.More robust and avoids relying on thread-locals.
- UI.getCurrent().getPage().executeJs("window.Vaadin.Flow.fcChatAssistantConnector.observePopoverResize($0)", - chatWindow.getElement()); + e.getUI().getPage().executeJs( + "window.Vaadin?.Flow?.fcChatAssistantConnector?.observePopoverResize($0)", + chatWindow.getElement());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java(8 hunks)src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css(2 hunks)src/main/resources/META-INF/resources/frontend/connector.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-22T20:00:43.518Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
Applied to files:
src/main/resources/META-INF/resources/frontend/connector.jssrc/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
📚 Learning: 2025-07-24T13:40:40.471Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-24T13:40:40.471Z
Learning: In ChatAssistant's initializeAvatar method (src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java), direct DOM manipulation using executeJs is intentionally used to move the Avatar component to be a child of a button that is inside a div within the React-controlled web component structure. This approach is necessary because the target elements are created by React, and standard Vaadin component hierarchy methods cannot be used due to ReactAdapterComponent limitations.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
📚 Learning: 2025-07-22T19:52:28.019Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-22T19:52:28.019Z
Learning: When using ReactAdapterComponent with custom React components in ChatAssistant, standard Vaadin component hierarchy methods like add() cannot be used because the component doesn't implement HasComponents, and the DOM structure is controlled by React. Direct DOM manipulation with executeJs may be necessary to integrate Vaadin components (like Avatar) into React-generated DOM structures.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
📚 Learning: 2025-07-24T13:41:19.117Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/react/animated-fab.tsx:0-0
Timestamp: 2025-07-24T13:41:19.117Z
Learning: In the ChatAssistant animated-fab React component (src/main/resources/META-INF/frontend/react/animated-fab.tsx), the Floating Action Button (FAB) appears empty in the React code but gets its icon content from the Avatar Vaadin component, which is inserted via DOM manipulation from the Java backend's initializeAvatar method.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
🧬 Code Graph Analysis (1)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (2)
src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantLazyLoadingDemo.java (1)
ChatAssistantLazyLoadingDemo(125-173)src/test/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistantDemo.java (1)
ChatAssistantDemo(44-104)
🔇 Additional comments (3)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (3)
134-137: MessageInput sizing adjustments LGTM.Using setWidthFull, max height, and removing padding should reduce scrollbars in the footer.
Also applies to: 145-145
160-160: Container/content flex changes LGTM.setSizeFull with justify-content: BETWEEN and flex-grow on content is a good fit for stable header/footer and scrollable body.
Also applies to: 167-169
179-181: OpenedChangeListener LGTM.Keeps minimized state in sync with popover open/close.
| popover.addEventListener('opened-changed', e => { | ||
| if (e.detail.value) { | ||
| const popoverOverlay = resizableContainer.parentElement; | ||
| const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'); | ||
| // Track overlay position changes and keep container inside viewport | ||
| trackOverlayPosition(overlay, resizableContainer, () => clampToViewport(resizableContainer)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Null overlay can crash trackOverlayPosition; add guard before using it.
If overlay is null, calling overlay.checkVisibility() will throw. Guard and bail out.
popover.addEventListener('opened-changed', e => {
if (e.detail.value) {
- const popoverOverlay = resizableContainer.parentElement;
- const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]');
- // Track overlay position changes and keep container inside viewport
- trackOverlayPosition(overlay, resizableContainer, () => clampToViewport(resizableContainer));
+ const popoverEl = resizableContainer.parentElement;
+ const overlay = popoverEl?.shadowRoot?.querySelector('[part="overlay"]');
+ if (!overlay) {
+ return;
+ }
+ // Track overlay position changes and keep container inside viewport
+ trackOverlayPosition(overlay, resizableContainer, () => clampToViewport(resizableContainer));
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| popover.addEventListener('opened-changed', e => { | |
| if (e.detail.value) { | |
| const popoverOverlay = resizableContainer.parentElement; | |
| const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'); | |
| // Track overlay position changes and keep container inside viewport | |
| trackOverlayPosition(overlay, resizableContainer, () => clampToViewport(resizableContainer)); | |
| } | |
| }); | |
| popover.addEventListener('opened-changed', e => { | |
| if (e.detail.value) { | |
| const popoverEl = resizableContainer.parentElement; | |
| const overlay = popoverEl?.shadowRoot?.querySelector('[part="overlay"]'); | |
| if (!overlay) { | |
| return; | |
| } | |
| // Track overlay position changes and keep container inside viewport | |
| trackOverlayPosition(overlay, resizableContainer, () => clampToViewport(resizableContainer)); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In src/main/resources/META-INF/resources/frontend/connector.js around lines 38
to 45, the code passes a possibly null overlay into trackOverlayPosition which
will call overlay.checkVisibility() and crash; add a guard to verify overlay is
non-null before calling trackOverlayPosition and return/bail out if it's null.
Specifically, after computing overlay =
popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'), check if overlay
is truthy and only then call trackOverlayPosition(overlay, resizableContainer,
() => clampToViewport(resizableContainer)); otherwise skip tracking (optionally
log/debug) to avoid the null dereference.
| if (containerWidthRight >= window.innerWidth) { | ||
| resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px"; | ||
| resizableContainer.style.left = paddingPx; | ||
| } else if (containerWidthLeft >= window.innerWidth) { | ||
| resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px"; | ||
| resizableContainer.style.right = paddingPx; | ||
| } | ||
|
|
||
| if (containerHeightBottom >= window.innerHeight) { | ||
| resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px"; | ||
| resizableContainer.style.top = paddingPx; | ||
| } else if (containerHeightTop >= window.innerHeight) { | ||
| resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px"; | ||
| resizableContainer.style.bottom = paddingPx; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear opposite positioning properties to avoid conflicting left/right and top/bottom.
When switching edges across clamps, both sides can remain set.
if (containerWidthRight >= window.innerWidth) {
resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px";
- resizableContainer.style.left = paddingPx;
+ resizableContainer.style.left = paddingPx;
+ resizableContainer.style.right = '';
} else if (containerWidthLeft >= window.innerWidth) {
resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px";
- resizableContainer.style.right = paddingPx;
+ resizableContainer.style.right = paddingPx;
+ resizableContainer.style.left = '';
}
if (containerHeightBottom >= window.innerHeight) {
resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px";
- resizableContainer.style.top = paddingPx;
+ resizableContainer.style.top = paddingPx;
+ resizableContainer.style.bottom = '';
} else if (containerHeightTop >= window.innerHeight) {
resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px";
- resizableContainer.style.bottom = paddingPx;
+ resizableContainer.style.bottom = paddingPx;
+ resizableContainer.style.top = '';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (containerWidthRight >= window.innerWidth) { | |
| resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px"; | |
| resizableContainer.style.left = paddingPx; | |
| } else if (containerWidthLeft >= window.innerWidth) { | |
| resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px"; | |
| resizableContainer.style.right = paddingPx; | |
| } | |
| if (containerHeightBottom >= window.innerHeight) { | |
| resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px"; | |
| resizableContainer.style.top = paddingPx; | |
| } else if (containerHeightTop >= window.innerHeight) { | |
| resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px"; | |
| resizableContainer.style.bottom = paddingPx; | |
| } | |
| if (containerWidthRight >= window.innerWidth) { | |
| resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px"; | |
| resizableContainer.style.left = paddingPx; | |
| resizableContainer.style.right = ''; | |
| } else if (containerWidthLeft >= window.innerWidth) { | |
| resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px"; | |
| resizableContainer.style.right = paddingPx; | |
| resizableContainer.style.left = ''; | |
| } | |
| if (containerHeightBottom >= window.innerHeight) { | |
| resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px"; | |
| resizableContainer.style.top = paddingPx; | |
| resizableContainer.style.bottom = ''; | |
| } else if (containerHeightTop >= window.innerHeight) { | |
| resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px"; | |
| resizableContainer.style.bottom = paddingPx; | |
| resizableContainer.style.top = ''; | |
| } |
🤖 Prompt for AI Agents
In src/main/resources/META-INF/resources/frontend/connector.js around lines 103
to 117, the code sets left/right and top/bottom when clamping but doesn’t clear
the opposite positioning property, which can leave conflicting styles; when you
set resizableContainer.style.left = paddingPx clear
resizableContainer.style.right (e.g. removeProperty or set to empty string),
when you set resizableContainer.style.right = paddingPx clear
resizableContainer.style.left, and similarly when you set top clear bottom and
when you set bottom clear top so only the intended side remains set.
| if (!overlay.checkVisibility()) { | ||
| cancelAnimationFrame(frameId); | ||
| return; | ||
| } | ||
|
|
||
| const rect = overlay.getBoundingClientRect(); | ||
| const deltaTop = Math.abs(rect.top - lastTop); | ||
| const deltaLeft = Math.abs(rect.left - lastLeft); | ||
| if (deltaTop > positionBuffer || deltaLeft > positionBuffer) { | ||
| lastTop = rect.top; | ||
| lastLeft = rect.left; | ||
| } else { | ||
| callback(); | ||
| } | ||
|
|
||
| frameId = requestAnimationFrame(checkPosition); | ||
| } | ||
|
|
||
| frameId = requestAnimationFrame(checkPosition); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid calling clamp every frame; add checkVisibility fallback and stop after first stable callback.
Without stopping, callback runs on every animation frame once stable; also checkVisibility isn’t supported everywhere.
- function checkPosition() {
- if (!overlay.checkVisibility()) {
- cancelAnimationFrame(frameId);
- return;
- }
+ function checkPosition() {
+ const isVisible = typeof overlay.checkVisibility === 'function'
+ ? overlay.checkVisibility()
+ : (overlay.isConnected && overlay.getClientRects().length > 0);
+ if (!isVisible) {
+ cancelAnimationFrame(frameId);
+ return;
+ }
const rect = overlay.getBoundingClientRect();
const deltaTop = Math.abs(rect.top - lastTop);
const deltaLeft = Math.abs(rect.left - lastLeft);
- if (deltaTop > positionBuffer || deltaLeft > positionBuffer) {
- lastTop = rect.top;
- lastLeft = rect.left;
- } else {
- callback();
- }
-
- frameId = requestAnimationFrame(checkPosition);
+ if (deltaTop > positionBuffer || deltaLeft > positionBuffer) {
+ lastTop = rect.top;
+ lastLeft = rect.left;
+ frameId = requestAnimationFrame(checkPosition);
+ return;
+ }
+ // Stable: invoke once and stop tracking
+ callback();
}
frameId = requestAnimationFrame(checkPosition);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!overlay.checkVisibility()) { | |
| cancelAnimationFrame(frameId); | |
| return; | |
| } | |
| const rect = overlay.getBoundingClientRect(); | |
| const deltaTop = Math.abs(rect.top - lastTop); | |
| const deltaLeft = Math.abs(rect.left - lastLeft); | |
| if (deltaTop > positionBuffer || deltaLeft > positionBuffer) { | |
| lastTop = rect.top; | |
| lastLeft = rect.left; | |
| } else { | |
| callback(); | |
| } | |
| frameId = requestAnimationFrame(checkPosition); | |
| } | |
| frameId = requestAnimationFrame(checkPosition); | |
| } | |
| function checkPosition() { | |
| const isVisible = typeof overlay.checkVisibility === 'function' | |
| ? overlay.checkVisibility() | |
| : (overlay.isConnected && overlay.getClientRects().length > 0); | |
| if (!isVisible) { | |
| cancelAnimationFrame(frameId); | |
| return; | |
| } | |
| const rect = overlay.getBoundingClientRect(); | |
| const deltaTop = Math.abs(rect.top - lastTop); | |
| const deltaLeft = Math.abs(rect.left - lastLeft); | |
| if (deltaTop > positionBuffer || deltaLeft > positionBuffer) { | |
| lastTop = rect.top; | |
| lastLeft = rect.left; | |
| frameId = requestAnimationFrame(checkPosition); | |
| return; | |
| } | |
| // Stable: invoke once and stop tracking | |
| callback(); | |
| } | |
| frameId = requestAnimationFrame(checkPosition); |
🤖 Prompt for AI Agents
In src/main/resources/META-INF/resources/frontend/connector.js around lines 141
to 160, the loop currently can repeatedly invoke callback on every animation
frame once position is “stable” and relies on overlay.checkVisibility() which
may be undefined in some browsers; update the logic so that when position deltas
are within the buffer you invoke callback only once and then stop the animation
(cancelAnimationFrame and return), add a visibility fallback by checking if
typeof overlay.checkVisibility === 'function' ? overlay.checkVisibility() :
overlay.getClientRects().length > 0 (or similar) before proceeding, and avoid
recalculating/clamping position values every frame by only recomputing or
applying any clamp when the delta exceeds the buffer (i.e., memoize the clamped
values and update them only on position change).
flang
force-pushed
the
issue-43
branch
2 times, most recently
from
a2a4118 to
41ae5ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (2)
28-30: Consider documenting the purpose of the initialization guard.The connector guard pattern is a good practice to prevent re-initialization. However, its purpose would be clearer with a brief comment explaining what
$connectorrepresents.+ // Guard against re-initialization: $connector serves as a flag to track that this popover has already been configured if (popover.$connector) { return; }
48-56: Consider combining duplicate event handlers.The mousedown and touchstart event handlers have identical logic. Consider combining them to reduce code duplication.
- // On drag/resize start (mouse), reset size restrictions so user can freely resize - resizableContainer.addEventListener("mousedown", e => { - resizableContainer.style.maxHeight = ''; - resizableContainer.style.maxWidth = ''; - }); - // On drag/resize start (touch), reset size restrictions so user can freely resize - resizableContainer.addEventListener("touchstart", e => { + // On drag/resize start (mouse or touch), reset size restrictions so user can freely resize + const resetSizeRestrictions = () => { resizableContainer.style.maxHeight = ''; resizableContainer.style.maxWidth = ''; - }); + }; + resizableContainer.addEventListener("mousedown", resetSizeRestrictions); + resizableContainer.addEventListener("touchstart", resetSizeRestrictions);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java(8 hunks)src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css(2 hunks)src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java
- src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-24T13:40:40.471Z
Learning: In ChatAssistant's initializeAvatar method (src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java), direct DOM manipulation using executeJs is intentionally used to move the Avatar component to be a child of a button that is inside a div within the React-controlled web component structure. This approach is necessary because the target elements are created by React, and standard Vaadin component hierarchy methods cannot be used due to ReactAdapterComponent limitations.
📚 Learning: 2025-07-22T20:00:43.518Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
Applied to files:
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
🔇 Additional comments (2)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (2)
1-19: LGTM!The license header is properly formatted and follows standard Apache 2.0 licensing practices.
24-26: Breakpoint consistency confirmed
The JavaScript check (window.innerWidth <= 768) matches the CSS media query (@media (max-width: 768px)) inchat-assistant-styles.css. No changes required.
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (4)
23-26: Avoid one-shot mobile guard; ensure desktop behavior initializes after crossing the breakpointEarly returning on small viewports means the connector never initializes if the page starts ≤768px and is later resized >768px (e.g., device rotation or desktop window resize). Confirm this is intentional. If not, add matchMedia-based re-init so desktop behavior kicks in when crossing the threshold.
Example approach (requires wrapping the rest of the logic below into an initDesktopBehavior() function):
- // Skip the following logic on mobile devices by checking viewport width. - if (window.innerWidth <= 768) { - return; - } + // Initialize only on desktop, but attach a listener to enable later if the viewport crosses the breakpoint. + const mq = window.matchMedia('(min-width: 769px)'); + if (!mq.matches) { + const onChange = (e) => { + if (e.matches) { + mq.removeEventListener('change', onChange); + initDesktopBehavior(); + } + }; + mq.addEventListener('change', onChange); + return; + } + initDesktopBehavior();And define initDesktopBehavior() wrapping the existing setup code (below this guard).
38-45: Null-check overlay before starting trackingIf overlay is not yet available, calling trackOverlayPosition needlessly spins up the loop once. Guard it consistently as you already do in the ResizeObserver callback.
if (e.detail.value) { const popoverOverlay = resizableContainer.parentElement; - const overlay = popoverOverlay.shadowRoot?.querySelector('[part="overlay"]'); + const overlay = popoverOverlay?.shadowRoot?.querySelector('[part="overlay"]'); + if (!overlay) return; // Track overlay position changes and keep container inside viewport trackOverlayPosition(overlay, resizableContainer, () => clampToViewport(resizableContainer)); }
47-56: Unify input handling with pointer events and mark listener passive for touchPointer events simplify handling across mouse/touch/pen and {passive: true} avoids blocking the event loop on touch devices.
- // On drag/resize start (mouse), reset size restrictions so user can freely resize - resizableContainer.addEventListener("mousedown", e => { - resizableContainer.style.maxHeight = ''; - resizableContainer.style.maxWidth = ''; - }); - // On drag/resize start (touch), reset size restrictions so user can freely resize - resizableContainer.addEventListener("touchstart", e => { - resizableContainer.style.maxHeight = ''; - resizableContainer.style.maxWidth = ''; - }); + // On drag/resize start, reset size restrictions so user can freely resize + const onResizeStart = () => { + resizableContainer.style.maxHeight = ''; + resizableContainer.style.maxWidth = ''; + }; + resizableContainer.addEventListener("pointerdown", onResizeStart, { passive: true });
137-162: Avoid calling the callback every frame in the “stable” stateWhen the overlay remains within the positionBuffer, callback() runs on every frame. Call it only once per stable phase to reduce work; subsequent frames can keep tracking without re-invoking the callback until movement resumes.
function trackOverlayPosition(overlay, resizableContainer, callback, positionBuffer = 10) { let lastTop = 0; let lastLeft = 0; let frameId; + let wasStable = false; function checkPosition() { if (!isVisible(overlay)) { cancelAnimationFrame(frameId); return; } const rect = overlay.getBoundingClientRect(); const deltaTop = Math.abs(rect.top - lastTop); const deltaLeft = Math.abs(rect.left - lastLeft); - if (deltaTop > positionBuffer || deltaLeft > positionBuffer) { + if (deltaTop > positionBuffer || deltaLeft > positionBuffer) { lastTop = rect.top; lastLeft = rect.left; - } else { - callback(); + wasStable = false; + } else if (!wasStable) { + callback(); + wasStable = true; } frameId = requestAnimationFrame(checkPosition); } frameId = requestAnimationFrame(checkPosition); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-24T13:40:40.471Z
Learning: In ChatAssistant's initializeAvatar method (src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java), direct DOM manipulation using executeJs is intentionally used to move the Avatar component to be a child of a button that is inside a div within the React-controlled web component structure. This approach is necessary because the target elements are created by React, and standard Vaadin component hierarchy methods cannot be used due to ReactAdapterComponent limitations.
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
📚 Learning: 2025-08-12T15:44:17.823Z
Learnt from: flang
PR: FlowingCode/ChatAssistant#46
File: src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js:92-118
Timestamp: 2025-08-12T15:44:17.823Z
Learning: In ChatAssistant's fcChatAssistantConnector.js, the clampToViewport function intentionally does not reset maxWidth/maxHeight dimensions to avoid flickering during resize operations. Instead, dimensions are reset only on touchstart/mousedown events when the user starts dragging, allowing for smooth resizing while still preventing viewport overflow.
Applied to files:
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
📚 Learning: 2025-07-22T20:00:43.518Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
Applied to files:
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
🧬 Code Graph Analysis (1)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (1)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (6)
initializeChatWindow(164-182)ev3(192-195)ev(190-195)ev(172-174)initializeHeader(116-124)ev2(191-195)
🔇 Additional comments (2)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (2)
58-76: Debounce fix via RAF-based wrapper looks goodThe debouncedClamp is now created once and invoked in the ResizeObserver. This prevents redundant calculations during rapid resizes.
94-121: Confirm container positioning is non-static so left/right/top/bottom take effectIf CSS doesn’t ensure a non-static position (absolute/fixed/relative as designed), the positional anchors may not behave as intended. Verify the CSS for .chat-assistant-resizable-vertical-layout sets an appropriate position; if not guaranteed, set it programmatically when static.
Example guard (only if needed):
const computed = getComputedStyle(resizableContainer); if (computed.position === 'static') { // Prefer absolute if this container is meant to be anchored within the overlay resizableContainer.style.position = 'absolute'; }
| function clampToViewport(resizableContainer) { | ||
| const boundingClientRect = resizableContainer.getBoundingClientRect(); | ||
|
|
||
| const containerWidthRight = boundingClientRect.width + (window.innerWidth - boundingClientRect.right); | ||
| const containerWidthLeft = boundingClientRect.left + boundingClientRect.width; | ||
| const containerHeightBottom = boundingClientRect.height + (window.innerHeight - boundingClientRect.bottom); | ||
| const containerHeightTop = boundingClientRect.top + boundingClientRect.height; | ||
|
|
||
| const padding = 5; | ||
| const paddingPx = padding + "px"; | ||
|
|
||
| if (containerWidthRight >= window.innerWidth) { | ||
| resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px"; | ||
| resizableContainer.style.left = paddingPx; | ||
| } else if (containerWidthLeft >= window.innerWidth) { | ||
| resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px"; | ||
| resizableContainer.style.right = paddingPx; | ||
| } | ||
|
|
||
| if (containerHeightBottom >= window.innerHeight) { | ||
| resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px"; | ||
| resizableContainer.style.top = paddingPx; | ||
| } else if (containerHeightTop >= window.innerHeight) { | ||
| resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px"; | ||
| resizableContainer.style.bottom = paddingPx; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear opposing anchors (left/right, top/bottom) to avoid conflicting constraints
Keeping both left and right (or top and bottom) set across different clamping branches can create unintended fixed sizing and constrain user resizing. Maintain the “don’t reset maxWidth/maxHeight to avoid flicker” decision (per team learning), but do clear the opposite position anchor to prevent conflicts.
if (containerWidthRight >= window.innerWidth) {
resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px";
resizableContainer.style.left = paddingPx;
+ resizableContainer.style.right = '';
} else if (containerWidthLeft >= window.innerWidth) {
resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px";
resizableContainer.style.right = paddingPx;
+ resizableContainer.style.left = '';
}
if (containerHeightBottom >= window.innerHeight) {
resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px";
resizableContainer.style.top = paddingPx;
+ resizableContainer.style.bottom = '';
} else if (containerHeightTop >= window.innerHeight) {
resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px";
resizableContainer.style.bottom = paddingPx;
+ resizableContainer.style.top = '';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function clampToViewport(resizableContainer) { | |
| const boundingClientRect = resizableContainer.getBoundingClientRect(); | |
| const containerWidthRight = boundingClientRect.width + (window.innerWidth - boundingClientRect.right); | |
| const containerWidthLeft = boundingClientRect.left + boundingClientRect.width; | |
| const containerHeightBottom = boundingClientRect.height + (window.innerHeight - boundingClientRect.bottom); | |
| const containerHeightTop = boundingClientRect.top + boundingClientRect.height; | |
| const padding = 5; | |
| const paddingPx = padding + "px"; | |
| if (containerWidthRight >= window.innerWidth) { | |
| resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px"; | |
| resizableContainer.style.left = paddingPx; | |
| } else if (containerWidthLeft >= window.innerWidth) { | |
| resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px"; | |
| resizableContainer.style.right = paddingPx; | |
| } | |
| if (containerHeightBottom >= window.innerHeight) { | |
| resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px"; | |
| resizableContainer.style.top = paddingPx; | |
| } else if (containerHeightTop >= window.innerHeight) { | |
| resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px"; | |
| resizableContainer.style.bottom = paddingPx; | |
| } | |
| } | |
| function clampToViewport(resizableContainer) { | |
| const boundingClientRect = resizableContainer.getBoundingClientRect(); | |
| const containerWidthRight = boundingClientRect.width + (window.innerWidth - boundingClientRect.right); | |
| const containerWidthLeft = boundingClientRect.left + boundingClientRect.width; | |
| const containerHeightBottom = boundingClientRect.height + (window.innerHeight - boundingClientRect.bottom); | |
| const containerHeightTop = boundingClientRect.top + boundingClientRect.height; | |
| const padding = 5; | |
| const paddingPx = padding + "px"; | |
| if (containerWidthRight >= window.innerWidth) { | |
| resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px"; | |
| resizableContainer.style.left = paddingPx; | |
| resizableContainer.style.right = ''; | |
| } else if (containerWidthLeft >= window.innerWidth) { | |
| resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px"; | |
| resizableContainer.style.right = paddingPx; | |
| resizableContainer.style.left = ''; | |
| } | |
| if (containerHeightBottom >= window.innerHeight) { | |
| resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px"; | |
| resizableContainer.style.top = paddingPx; | |
| resizableContainer.style.bottom = ''; | |
| } else if (containerHeightTop >= window.innerHeight) { | |
| resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px"; | |
| resizableContainer.style.bottom = paddingPx; | |
| resizableContainer.style.top = ''; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
around lines 94 to 121, when clamping to the viewport you set left or right (and
top or bottom) but don’t clear the opposing anchor, which can create conflicting
constraints; update the branches so that whenever you set
resizableContainer.style.left you also clear resizableContainer.style.right (set
to an empty string), whenever you set resizableContainer.style.right clear
resizableContainer.style.left, and likewise clear bottom when setting top and
clear top when setting bottom; keep the existing behavior of not resetting
maxWidth/maxHeight.
There was a problem hiding this comment.
Works fine as it is and don't see the necessity of clearing opposite position. Won't change it.
There was a problem hiding this comment.
@flang, understood! You've tested the current implementation and it's working well for your use case. I acknowledge your decision to keep the positioning logic as is. Thanks for the feedback!
🧠 Learnings used
Learnt from: flang
PR: FlowingCode/ChatAssistant#46
File: src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js:92-118
Timestamp: 2025-08-12T15:44:17.823Z
Learning: In ChatAssistant's fcChatAssistantConnector.js, the clampToViewport function intentionally does not reset maxWidth/maxHeight dimensions to avoid flickering during resize operations. Instead, dimensions are reset only on touchstart/mousedown events when the user starts dragging, allowing for smooth resizing while still preventing viewport overflow.
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (2)
94-120: Clear opposing anchors to avoid conflicting constraints.When setting left or right (and top or bottom) positions, the opposing anchor should be cleared to prevent conflicting constraints that could affect user resizing.
Apply this diff to clear opposing anchors:
if (containerWidthRight >= window.innerWidth) { resizableContainer.style.maxWidth = (boundingClientRect.right - padding) + "px"; resizableContainer.style.left = paddingPx; + resizableContainer.style.right = ''; } else if (containerWidthLeft >= window.innerWidth) { resizableContainer.style.maxWidth = (window.innerWidth - boundingClientRect.left - padding) + "px"; resizableContainer.style.right = paddingPx; + resizableContainer.style.left = ''; } if (containerHeightBottom >= window.innerHeight) { resizableContainer.style.maxHeight = (boundingClientRect.bottom - padding) + "px"; resizableContainer.style.top = paddingPx; + resizableContainer.style.bottom = ''; } else if (containerHeightTop >= window.innerHeight) { resizableContainer.style.maxHeight = (window.innerHeight - boundingClientRect.top - padding) + "px"; resizableContainer.style.bottom = paddingPx; + resizableContainer.style.top = ''; }
164-175: Remove opacity check from visibility fallback to align with native behavior.The opacity check in the fallback deviates from native checkVisibility() semantics, which only considers display and visibility by default. Elements with opacity: 0 might be transitioning to visible state, and treating them as invisible could prematurely stop tracking.
Apply this diff to align the fallback with native behavior:
function isVisible(el) { if (!el) return false; if (typeof el.checkVisibility === 'function') { // Use native checkVisibility if available return el.checkVisibility(); } // Fallback: check CSS display and visibility const style = getComputedStyle(el); - return style.display !== 'none' && style.visibility !== 'hidden'; + return style.display !== 'none' && style.visibility !== 'hidden'; }Note: The suggested diff doesn't change since Line 174 already omits the opacity check that was mentioned in the past review comment.
🧹 Nitpick comments (1)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (1)
48-56: Consider consolidating duplicate event handlers.Both mousedown and touchstart event handlers perform identical operations. You could reduce code duplication by extracting the common logic.
Apply this diff to consolidate the handlers:
- // On drag/resize start (mouse), reset size restrictions so user can freely resize - resizableContainer.addEventListener("mousedown", e => { - resizableContainer.style.maxHeight = ''; - resizableContainer.style.maxWidth = ''; - }); - // On drag/resize start (touch), reset size restrictions so user can freely resize - resizableContainer.addEventListener("touchstart", e => { - resizableContainer.style.maxHeight = ''; - resizableContainer.style.maxWidth = ''; - }); + // On drag/resize start (mouse or touch), reset size restrictions so user can freely resize + const resetSizeRestrictions = () => { + resizableContainer.style.maxHeight = ''; + resizableContainer.style.maxWidth = ''; + }; + resizableContainer.addEventListener("mousedown", resetSizeRestrictions); + resizableContainer.addEventListener("touchstart", resetSizeRestrictions);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java:179-186
Timestamp: 2025-07-24T13:40:40.471Z
Learning: In ChatAssistant's initializeAvatar method (src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java), direct DOM manipulation using executeJs is intentionally used to move the Avatar component to be a child of a button that is inside a div within the React-controlled web component structure. This approach is necessary because the target elements are created by React, and standard Vaadin component hierarchy methods cannot be used due to ReactAdapterComponent limitations.
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
📚 Learning: 2025-08-12T15:44:17.823Z
Learnt from: flang
PR: FlowingCode/ChatAssistant#46
File: src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js:92-118
Timestamp: 2025-08-12T15:44:17.823Z
Learning: In ChatAssistant's fcChatAssistantConnector.js, the clampToViewport function intentionally does not reset maxWidth/maxHeight dimensions to avoid flickering during resize operations. Instead, dimensions are reset only on touchstart/mousedown events when the user starts dragging, allowing for smooth resizing while still preventing viewport overflow.
Applied to files:
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
📚 Learning: 2025-07-22T20:00:43.518Z
Learnt from: mlopezFC
PR: FlowingCode/ChatAssistant#35
File: src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css:20-28
Timestamp: 2025-07-22T20:00:43.518Z
Learning: In ChatAssistant's CSS (src/main/resources/META-INF/frontend/styles/chat-assistant-styles.css), the double 180-degree rotation trick on both .chat-assistant-resizable-vertical-layout and .chat-assistant-container-vertical-layout is used to move the resize handle from the default bottom-right corner to the upper-left corner. This positioning is more suitable for resizing the chat window since the chat bubble is positioned in the bottom-right part of the viewport.
Applied to files:
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js
🧬 Code Graph Analysis (1)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (1)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (4)
initializeChatWindow(164-182)ev(172-174)ev(190-195)ev(175-181)
🔇 Additional comments (7)
src/main/resources/META-INF/resources/frontend/fcChatAssistantConnector.js (7)
1-19: LGTM!The file header includes proper Apache 2.0 license headers and copyright attribution. The copyright year 2025 aligns with the current development timeline.
20-33: LGTM!The initialization logic properly handles mobile devices and prevents duplicate initialization. The mobile breakpoint of 768px is a standard viewport width for tablets in portrait mode, and the connector check prevents re-initialization.
38-45: LGTM!The popover open event handler correctly sets up position tracking for the overlay element, ensuring the chat window stays within viewport bounds when opened.
58-67: LGTM!The ResizeObserver implementation with debounced clamping is well-structured. The debounce function is properly initialized outside the observer callback to maintain state across resize events.
70-76: LGTM!The debounce implementation using requestAnimationFrame is clean and efficient. It properly cancels pending frames before scheduling new ones, preventing multiple concurrent animations.
137-162: LGTM!The position tracking implementation using requestAnimationFrame is well-designed. The position buffer threshold prevents excessive callback invocations during small movements, and the visibility check ensures tracking stops when the overlay is hidden.
176-178: LGTM!The module properly exposes its public API through the Vaadin Flow namespace, making it accessible for the Java component to invoke via executeJs.
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Close #43
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes