Add Vaadin 25 support while maintaining Vaadin 24 compatibility#28
Open
javier-godoy wants to merge 7 commits intomasterfrom
Open
Add Vaadin 25 support while maintaining Vaadin 24 compatibility#28javier-godoy wants to merge 7 commits intomasterfrom
javier-godoy wants to merge 7 commits intomasterfrom
Conversation
Extract JavaScript DOM traversal into LoginOverlayUtils helper methods. Update LoginLayout and ReplaceableLoginOverlay to use these methods, and add removeForgotPassword() extracted from replaceForgotPassword(). Co-Authored-By: Javier Godoy <11554739+javier-godoy@users.noreply.github.com>
Add Vaadin 25 fallback logic to all LoginOverlayUtils helper methods.
When getElementById('vaadinLoginOverlayWrapper') returns null (Vaadin 25),
fall back to querying the vaadin-login-overlay shadow DOM for the wrapper.
Close #23
Co-Authored-By: Javier Godoy <11554739+javier-godoy@users.noreply.github.com>
Use demo source condition attribute to select between Vaadin 24 & Vaadin 25 styles.
Add testbench-rpc dependency to be able to retrieve Vaadin version.
WalkthroughThis PR adds Vaadin 25+ compatibility to the ExtendedLoginAddon by introducing version-conditional DOM manipulation utilities, updating integration tests with version detection logic, and adding Vaadin 25-specific styling. Core components now use centralized JavaScript utility methods instead of inline scripts, and test infrastructure includes RPC-based version checking. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip Migrating from UI to YAML configuration.Use the |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.java (1)
76-83: Consider more specific validation feedback for demo clarity.The current validation conflates two failure cases under one message:
- Password is null/blank → "Passwords do not match."
- Passwords actually don't match → "Passwords do not match."
For a demo showcasing form validation patterns, slightly more specific messages could be helpful, though the current implementation is functional.
♻️ Optional: More specific validation messages
private void onAccept() { String passwordValue = this.password.getValue(); String confirmPasswordValue = this.confirmPassword.getValue(); - if (passwordValue != null && !passwordValue.isBlank() && passwordValue.equals(confirmPasswordValue)) { + if (passwordValue == null || passwordValue.isBlank()) { + Notification.show("Password cannot be empty."); + } else if (!passwordValue.equals(confirmPasswordValue)) { + Notification.show("Passwords do not match."); + } else { Notification.show("Password changed."); - } else { - Notification.show("Passwords do not match."); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.java` around lines 76 - 83, The onAccept method currently treats a blank/null password and a mismatched confirmation the same; update onAccept to first check that password.getValue() is non-null and not blank and show a specific message like "Password cannot be blank" (or set a validation error) if it fails, then separately check password.equals(confirmPasswordValue) and show "Passwords do not match" only in that case; locate the onAccept method and the variables password, confirmPassword, passwordValue and confirmPasswordValue to implement the two-step validation and corresponding Notification messages.src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java (1)
53-53: Tighten the RPC proxy field contract.Subclasses rely on this proxy, so leaving it package-private and mutable is looser than necessary. Marking it
protected finalmakes the intended extension point explicit and prevents accidental reassignment.♻️ Proposed tweak
- ServerVersionCallables $server = createCallableProxy(ServerVersionCallables.class); + protected final ServerVersionCallables $server = createCallableProxy(ServerVersionCallables.class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java` at line 53, The field holding the RPC proxy (ServerVersionCallables $server) is currently package-private and mutable; change its declaration to protected final to tighten the contract so subclasses can use but not reassign it. Locate the declaration of ServerVersionCallables $server in AbstractViewTest (the line with createCallableProxy(ServerVersionCallables.class)) and update its modifier to protected final, ensuring any initialization remains compatible with the constructor or field initializer that calls createCallableProxy.src/main/java/com/flowingcode/vaadin/addons/extendedlogin/ReplaceableLoginOverlay.java (1)
58-77: You can merge brand clear+append into a singleexecuteJscall.This would avoid two client round-trips and remove timing coupling between separate async snippets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/ReplaceableLoginOverlay.java` around lines 58 - 77, The code issues two separate getElement().executeJs calls to first clear and then append to the brand element causing extra round-trips and timing coupling; merge them into a single executeJs invocation in ReplaceableLoginOverlay that uses LoginOverlayUtils.getOverlayWrapperScript to both clear (replaceChildren()) and append ($0) the passed withElement in one script, keeping the same parameter (withElement) and call site so the operation is atomic and reduces client-server round-trips.src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginOverlayUtils.java (1)
39-54: Consider extracting shared overlay-wrapper lookup to one internal template.The Vaadin 24/25 wrapper resolution block is duplicated in two methods. Centralizing it would reduce drift risk when compatibility logic changes again.
Also applies to: 88-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginOverlayUtils.java` around lines 39 - 54, Extract the duplicated Vaadin 24/25 overlay-wrapper lookup into a single internal template/constant and reuse it from getOverlayWrapperScript and the other method that contains the same block (the overlay-close/cleanup script method), so the JS snippet is defined once and then injected (e.g., via String.format or text block concatenation) into both getOverlayWrapperScript and the sibling method; update both methods to reference the new shared symbol (e.g., OVERLAY_WRAPPER_LOOKUP or buildOverlayWrapperLookup()) and remove the duplicated JS block from each method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayout.java`:
- Around line 54-65: Guard the usage of the content field in the onAttach logic
by checking content for null before executing the form replacement JS and DOM
manipulations: wrap the block that calls
this.getElement().executeJs(LoginOverlayUtils.getLoginFormWrapperScript(...)),
this.getElement().appendChild(content.getElement()),
content.getElement().setAttribute("slot","form"), and
content.getElement().executeJs(...) with an early-return or conditional (e.g.,
if (content == null) return; or if (content != null) { ... }); ensure you
reference the same symbols (content, this.getElement(),
LoginOverlayUtils.getLoginFormWrapperScript, formWrapper) so the DOM/script
operations only run when content is present.
In
`@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ServerVersionCallables.java`:
- Around line 20-29: The ServerVersionCallables interface is currently in the
integration-test (.it) package and is referenced by TestExtendedLoginOverlayView
and TestLoginLayout, which are included in the demo JAR; move the contract to a
neutral test-support package so the demo classifier can load the tests without
missing references. Create a new package (e.g.,
com.flowingcode.vaadin.addons.extendedlogin.testsupport) and relocate the
ServerVersionCallables interface there (keep the same interface name and
getMajorVersion() signature), then update imports in
TestExtendedLoginOverlayView and TestLoginLayout to point to the new package;
alternatively, if you prefer not to move files, ensure the interface is
explicitly included in the demo JAR by adjusting the build configuration to not
exclude it.
In `@src/test/resources/META-INF/frontend/styles/extended-login-styles-v25.css`:
- Line 1: The comment-whitespace-inside stylelint violations are caused by
comments that lack required spaces; update the comment tokens in this file by
inserting a space after the opening /* and before the closing */ — e.g., change
the leading comment token that is currently "/*-" to include a space ("/* -")
and change "/*Demo styles*/" to "/* Demo styles */" so they comply with the
comment-whitespace-inside rule; locate these comments in the CSS and adjust
their spacing accordingly.
---
Nitpick comments:
In
`@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginOverlayUtils.java`:
- Around line 39-54: Extract the duplicated Vaadin 24/25 overlay-wrapper lookup
into a single internal template/constant and reuse it from
getOverlayWrapperScript and the other method that contains the same block (the
overlay-close/cleanup script method), so the JS snippet is defined once and then
injected (e.g., via String.format or text block concatenation) into both
getOverlayWrapperScript and the sibling method; update both methods to reference
the new shared symbol (e.g., OVERLAY_WRAPPER_LOOKUP or
buildOverlayWrapperLookup()) and remove the duplicated JS block from each
method.
In
`@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/ReplaceableLoginOverlay.java`:
- Around line 58-77: The code issues two separate getElement().executeJs calls
to first clear and then append to the brand element causing extra round-trips
and timing coupling; merge them into a single executeJs invocation in
ReplaceableLoginOverlay that uses LoginOverlayUtils.getOverlayWrapperScript to
both clear (replaceChildren()) and append ($0) the passed withElement in one
script, keeping the same parameter (withElement) and call site so the operation
is atomic and reduces client-server round-trips.
In
`@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java`:
- Line 53: The field holding the RPC proxy (ServerVersionCallables $server) is
currently package-private and mutable; change its declaration to protected final
to tighten the contract so subclasses can use but not reassign it. Locate the
declaration of ServerVersionCallables $server in AbstractViewTest (the line with
createCallableProxy(ServerVersionCallables.class)) and update its modifier to
protected final, ensuring any initialization remains compatible with the
constructor or field initializer that calls createCallableProxy.
In
`@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.java`:
- Around line 76-83: The onAccept method currently treats a blank/null password
and a mismatched confirmation the same; update onAccept to first check that
password.getValue() is non-null and not blank and show a specific message like
"Password cannot be blank" (or set a validation error) if it fails, then
separately check password.equals(confirmPasswordValue) and show "Passwords do
not match" only in that case; locate the onAccept method and the variables
password, confirmPassword, passwordValue and confirmPasswordValue to implement
the two-step validation and corresponding Notification messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe931338-0769-4cc1-960e-95dcf11c9083
📒 Files selected for processing (15)
pom.xmlsrc/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayout.javasrc/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginOverlayUtils.javasrc/main/java/com/flowingcode/vaadin/addons/extendedlogin/ReplaceableLoginOverlay.javasrc/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/ExtendedLoginOverlayDemo.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayoutDemo.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestExtendedLoginOverlayView.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayout.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ExtendedLoginOverlayIT.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/LoginLayoutIT.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ServerVersionCallables.javasrc/test/resources/META-INF/frontend/styles/extended-login-styles-v25.css
src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayout.java
Show resolved
Hide resolved
Comment on lines
+20
to
+29
| package com.flowingcode.vaadin.addons.extendedlogin.it; | ||
|
|
||
| /** | ||
| * Interface for callable methods to retrieve server version information. This | ||
| * is used to determine whether certain tests should be executed based on the | ||
| * server version. | ||
| */ | ||
| public interface ServerVersionCallables { | ||
|
|
||
| int getMajorVersion(); |
There was a problem hiding this comment.
Move this shared RPC contract out of the .it package.
The demo-jar profile still excludes **/it/*, but TestExtendedLoginOverlayView and TestLoginLayout now implement this interface and are not excluded. That leaves the demo classifier with classes that reference a missing interface, which can break classloading as soon as those demo classes are loaded. Put the contract in a neutral test-support package, or explicitly keep it in the demo JAR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ServerVersionCallables.java`
around lines 20 - 29, The ServerVersionCallables interface is currently in the
integration-test (.it) package and is referenced by TestExtendedLoginOverlayView
and TestLoginLayout, which are included in the demo JAR; move the contract to a
neutral test-support package so the demo classifier can load the tests without
missing references. Create a new package (e.g.,
com.flowingcode.vaadin.addons.extendedlogin.testsupport) and relocate the
ServerVersionCallables interface there (keep the same interface name and
getMajorVersion() signature), then update imports in
TestExtendedLoginOverlayView and TestLoginLayout to point to the new package;
alternatively, if you prefer not to move files, ensure the interface is
explicitly included in the demo JAR by adjusting the build configuration to not
exclude it.
Member
There was a problem hiding this comment.
It's needed for integration tests. So the package is correct.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify the actual demo-jar build configuration before responding.
🏁 Script executed:
#!/bin/bash
# Check demo-jar profile exclusions in pom.xml
grep -A 50 "demo-jar" pom.xml | grep -A 30 "excludes\|exclude\|include"
Length of output: 1127
@paodb, understood that ServerVersionCallables belongs in the .it package for integration tests — the package placement itself isn't the concern.
The specific risk is a demo-JAR classloading issue: the demo-jar profile excludes **/it/* (which removes ServerVersionCallables from the artifact), but TestLoginLayout and TestExtendedLoginOverlayView — which both implements ServerVersionCallables — are not excluded by any of the current patterns (**/test/*, **/it/*, or the named .class exclusions). Since the #if/#endif directives are display-only and don't affect bytecode, implements ServerVersionCallables is always compiled in. If either of those views gets classloaded in the demo context, the JVM will throw a NoClassDefFoundError for the missing interface.
If you've verified the demo JAR works correctly (e.g., those views aren't actually loaded in the demo context, or the demo JAR isn't deployed independently), happy to defer to your judgment.
🧠 Learnings used
Learnt from: javier-godoy
Repo: FlowingCode/ExtendedLoginAddon PR: 27
File: src/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.java:41-45
Timestamp: 2026-03-09T12:28:46.811Z
Learning: Guideline: In this repository, the // `#if` vaadin eq <version>, // `#else`, and // `#endif` directives are demo-display controls used by the demo framework with DemoSource(condition = "..."). They are NOT build-time or runtime conditionals and do not affect compilation or execution. When reviewing code in test sources that uses these directives, treat them as display-only annotations; do not remove or rely on them for conditional compilation. If you see code wrapped by these directives, understand that both branches may be present for demo purposes and are intended to be active in runtime; use this knowledge when assessing correctness and maintainability.
Member
Author
There was a problem hiding this comment.
@coderabbitai @paodb The demo JAR IS deployed independently.
There was a problem hiding this comment.
🧩 Analysis chain
@javier-godoy, thanks for confirming — that makes the classloading issue a real problem. Let me verify all the usages before suggesting the fix.
🏁 Script executed:
#!/bin/bash
# Check demo-jar exclusion patterns
grep -A 60 "demo-jar" pom.xml | grep -E "exclud|includ"
echo "---"
# Find all usages of ServerVersionCallables
rg -n "ServerVersionCallables" --type java
Length of output: 1308
@javier-godoy, thanks for confirming — the issue is real. Here's the precise breakdown:
**/it/*excludesServerVersionCallables.classfrom the demo JAR — confirmed by the exclusion patterns.**/test/*matches a package literally namedtest, not class names that begin withTest— soTestLoginLayoutandTestExtendedLoginOverlayVieware included in the demo JAR.- Both implement
ServerVersionCallables, so when either view is classloaded in the deployed demo, the JVM throwsNoClassDefFoundError.
The cleanest fix is to move ServerVersionCallables from the it package to com.flowingcode.vaadin.addons.extendedlogin (the same package as its implementing classes). AbstractViewTest (in it) can still import it from the parent package.
- Move
ServerVersionCallables.javato theextendedloginpackage - Update its
packagedeclaration - Update the import in
AbstractViewTest.java - Update the imports in
TestLoginLayout.javaandTestExtendedLoginOverlayView.java
🧠 Learnings used
Learnt from: javier-godoy
Repo: FlowingCode/ExtendedLoginAddon PR: 27
File: src/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.java:41-45
Timestamp: 2026-03-09T12:28:46.811Z
Learning: Guideline: In this repository, the // `#if` vaadin eq <version>, // `#else`, and // `#endif` directives are demo-display controls used by the demo framework with DemoSource(condition = "..."). They are NOT build-time or runtime conditionals and do not affect compilation or execution. When reviewing code in test sources that uses these directives, treat them as display-only annotations; do not remove or rely on them for conditional compilation. If you see code wrapped by these directives, understand that both branches may be present for demo purposes and are intended to be active in runtime; use this knowledge when assessing correctness and maintainability.
Failed to handle agent chat message. Please try again.
src/test/resources/META-INF/frontend/styles/extended-login-styles-v25.css
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds Vaadin 25 compatibility while maintaining support for Vaadin 24, along with several refactorings and demo improvements. (Rebased from #27)
Changes:
Vaadin 24 & 25 compatibility
Code improvements
Demo improvements
Summary by CodeRabbit
New Features
Tests
Chores