feat: show DemoSource based on Vaadin version #134
Conversation
WalkthroughThe PR adds conditional source-tab filtering based on a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pom.xml (1)
130-141: Remove the unnecessary JNA exclusion from webdrivermanager 6.3.2.The Java 17 runtime satisfies the Java 11+ requirement for WebDriverManager 6.x. The
setup()API remains supported, so existing test code is compatible. However, the JNA exclusion is no longer needed in 6.x since WebDriverManager 6 does not pull in JNA as a direct dependency. Remove the<exclusions>block:Exclusion to remove
<exclusions> <exclusion> <groupId>net.java.dev.jna</groupId> <artifactId>jna</artifactId> </exclusion> </exclusions>
🤖 Fix all issues with AI agents
In
`@src/main/java/com/flowingcode/vaadin/addons/demo/DemoSourceConditionHelper.java`:
- Around line 68-84: The compare method currently returns 0 when all shared
components match, ignoring extra components; update compare(String a, String b)
(which uses split() and local arrays aa, bb) so after the for-loop it inspects
any remaining components in the longer array: parse each remaining part to int
and if any remaining int != 0 return +1 if aa has the extra non-zero part or -1
if bb does; if all remaining parts are zero return 0. This ensures "24.3" <
"24.3.1" and handles trailing zeros correctly.
In `@src/test/java/com/flowingcode/vaadin/addons/demo/MultiSourceDemo.java`:
- Around line 32-38: Update the mismatched show-source comments so they reflect
the actual `@DemoSource` annotations: replace the references to
multi-source-demo.css on the show-source comment lines with the correct resource
names used by the annotations (condition-true.css and condition-false.css) or
alternatively change the `@DemoSource` annotations to reference
multi-source-demo.css if that was intended; ensure the show-source comments and
the `@DemoSource` entries (the annotations on the MultiSourceDemo class:
`@DemoSource`("/src/test/resources/.../multi-source-demo.css"),
`@DemoSource`(value="/src/test/resources/.../condition-true.css", condition =
"vaadin ge 14"), and
`@DemoSource`(value="/src/test/resources/.../condition-false.css", condition =
"vaadin eq 0")) are consistent.
🧹 Nitpick comments (4)
src/main/java/com/flowingcode/vaadin/addons/demo/DemoSource.java (1)
80-81: Add Javadoc for the newconditionattribute.Other attributes in this annotation have Javadoc explaining their purpose and usage. Consider documenting the expected format of the condition expression (e.g.,
"vaadin ge 14","flow lt 24.0"), and what happens when the condition evaluates to false.📝 Suggested documentation
+ /** + * A condition expression that determines when this source should be displayed. + * The condition is evaluated against version properties (e.g., "vaadin", "flow"). + * Supported operators: eq, ne, lt, le, gt, ge. + * Example: "vaadin ge 14" shows the source when Vaadin version is 14 or higher. + * If empty (default), the source is always displayed. + */ String condition() default "";src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoViewIT.java (2)
40-40: Unused fieldviewer.The
viewerfield is declared and checked inopen()but never actually assigned or used elsewhere in the class. Consider removing it if it's not needed, or implement its intended usage.♻️ Suggested fix
- private SourceCodeViewerElement viewer; - protected void open(Class<? extends Component> clazz) { - if (viewer != null) { - throw new IllegalStateException(); - } getDriver().get(getURL(clazz.getAnnotation(Route.class).value()));
88-90: Consider usingMatcherAssert.assertThatinstead of deprecatedAssert.assertThat.
Assert.assertThatis deprecated in JUnit 4.13+. UsingMatcherAssert.assertThatfrom Hamcrest is the recommended approach.♻️ Suggested fix
+import static org.hamcrest.MatcherAssert.assertThat; ... private void assertThat(Matcher<SearchContext> matcher) { - Assert.assertThat(getDriver(), matcher); + assertThat(getDriver(), matcher); }src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
183-203: Redundant assignment on line 200.
currentLayout = null;is redundant at line 200. At this point,currentLayoutis alreadynulleither becausesourceTabs.isEmpty()was true (line 183 condition not entered) or becausecurrentLayout.isEmpty()was true (set to null at line 187).♻️ Suggested fix
setupDemoHelperButton(currentLayout.getContent().getPrimaryComponent().getClass()); } else { - currentLayout = null; demo.getElement().getStyle().set("height", "100%"); setupDemoHelperButton(content.getClass()); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
pom.xmlsrc/main/java/com/flowingcode/vaadin/addons/demo/DemoSource.javasrc/main/java/com/flowingcode/vaadin/addons/demo/DemoSourceConditionHelper.javasrc/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.javasrc/main/java/com/flowingcode/vaadin/addons/demo/SourceCodeTab.javasrc/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.javasrc/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.javasrc/test/java/com/flowingcode/vaadin/addons/demo/MultiSourceDemo.javasrc/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoViewIT.javasrc/test/resources/META-INF/resources/frontend/condition-false.csssrc/test/resources/META-INF/resources/frontend/condition-true.css
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoViewIT.java (1)
src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoView.java (1)
Route(48-49)
🔇 Additional comments (18)
src/test/resources/META-INF/resources/frontend/condition-true.css (1)
1-1: LGTM!Simple test resource that clearly documents its conditional display purpose. Consider adding a trailing newline for consistency with standard file conventions.
src/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.java (1)
57-59: LGTM!Clean delegation method that enables the caller to check if all conditional sources have been filtered out. This integrates well with the conditional source filtering feature.
src/test/resources/META-INF/resources/frontend/condition-false.css (1)
1-1: LGTM!Appropriate test resource for validating that conditional sources with a false condition are properly hidden. Consider adding a trailing newline for consistency.
src/main/java/com/flowingcode/vaadin/addons/demo/SourceCodeTab.java (1)
39-39: LGTM!The new
conditionfield is correctly added as an optional field. Lombok's@Builderwill generate the appropriate builder method, and the field integrates well with the existing class structure.src/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.java (3)
46-50: Good defensive programming with the early return pattern.The defensive copy before filtering and the early return when empty are well-implemented. The
isEmpty()method provides a clean way for callers to check if the viewer has content.
79-81: LGTM!The
isEmpty()method correctly indicates when no tabs are available after filtering.
147-154: LGTM!Good null-safe handling in
getSourcePosition(). ReturningSourcePosition.SECONDARYas the default when empty is a reasonable fallback for the UI layout.src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoViewIT.java (1)
92-135: Good test coverage for the conditional source feature.The tests comprehensively cover all scenarios: no source, single source, multi-source, and both conditional true/false cases. The use of custom Hamcrest matchers makes the assertions readable and maintainable.
src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
253-256: LGTM!The condition propagation from the annotation to the builder is correctly implemented, using an empty string check to avoid setting unnecessary values.
src/main/java/com/flowingcode/vaadin/addons/demo/DemoSourceConditionHelper.java (2)
29-66: LGTM on the eval() structure.The condition evaluation logic is well-structured with clear error handling for invalid formats and unknown operators. Returning
truefor null conditions is a sensible default that allows unconditional sources to pass through.
86-92: LGTM!Good input validation with a clear regex pattern and descriptive error message.
src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoView.java (6)
1-32: LGTM!The license header, imports, and class declaration are well-structured. The
@SuppressWarnings("serial")at the class level appropriately covers all inner classes extending Vaadin components.
34-40: Good test coverage.The constructor registers all five demo variants, covering the key scenarios: no source, single source, multiple sources, and conditional sources (both true and false conditions).
42-46: Clean base class for test demos.Using
getClass().getSimpleName()correctly returns the concrete subclass name at runtime, making each demo view self-identifying. The abstract modifier appropriately prevents direct instantiation of the base class.
48-53: LGTM!The
TabbedDemoViewNoSourcecorrectly tests the no-source scenario, whileTabbedDemoViewSingleSourcevalidates basic source display. Route paths follow a clear naming convention.
55-58: LGTM!Good test case for multiple source tabs using the repeatable
@DemoSourceannotation with different source classes.
60-66: Well-designed conditional test cases.The conditions are appropriately chosen for deterministic integration testing:
"vaadin ge 14"reliably evaluates to true for any modern Vaadin environment (14+)"vaadin eq 0"reliably evaluates to false since no Vaadin version is 0This ensures consistent test behavior across environments.
pom.xml (1)
8-8: LGTM!The minor version bump from 5.0.1-SNAPSHOT to 5.1.0-SNAPSHOT appropriately reflects the addition of new conditional DemoSource functionality per semantic versioning conventions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/main/java/com/flowingcode/vaadin/addons/demo/DemoSourceConditionHelper.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/addons/demo/MultiSourceDemo.java
Outdated
Show resolved
Hide resolved
javier-godoy
force-pushed
the
feat-conditional-demo-source
branch
from
f96e62e to
e11ddd5
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
249-251: Bug:annotation.caption()should beannotation.language().The language builder is incorrectly using
caption()instead oflanguage(). This would cause the language setting to be ignored and use the caption value instead.🐛 Proposed fix
if (!annotation.language().equals(DemoSource.DEFAULT_VALUE)) { - builder.language(annotation.caption()); + builder.language(annotation.language()); }
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/demo/DemoSource.java (1)
80-81: Consider adding Javadoc for theconditionelement.Other annotation elements have Javadoc comments describing their purpose. Adding documentation here would improve consistency and clarify the expected syntax (e.g.,
"vaadin ge 24").📝 Suggested Javadoc
+ /** + * A condition expression to control the visibility of this source tab. + * Uses the same syntax as conditional directives: {`@code` variable operator value} + * (e.g., {`@code` "vaadin ge 24"}). An empty string means no condition (always visible). + */ String condition() default "";src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
183-203: Redundant assignment on line 200.In the
elsebranch,currentLayoutis alreadynull(since we only enter this branch whencurrentLayout == null), so the assignment is unnecessary.♻️ Suggested fix
} else { - currentLayout = null; demo.getElement().getStyle().set("height", "100%"); setupDemoHelperButton(content.getClass()); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/demo/DemoSource.javasrc/main/java/com/flowingcode/vaadin/addons/demo/DemoSourceConditionHelper.javasrc/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.javasrc/main/java/com/flowingcode/vaadin/addons/demo/SourceCodeTab.javasrc/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.javasrc/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.javasrc/test/java/com/flowingcode/vaadin/addons/demo/MultiSourceDemo.javasrc/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoViewIT.javasrc/test/resources/META-INF/resources/frontend/condition-false.csssrc/test/resources/META-INF/resources/frontend/condition-true.css
🚧 Files skipped from review as they are similar to previous changes (8)
- src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoViewIT.java
- src/test/resources/META-INF/resources/frontend/condition-true.css
- src/test/resources/META-INF/resources/frontend/condition-false.css
- pom.xml
- src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoView.java
- src/main/java/com/flowingcode/vaadin/addons/demo/DemoSourceConditionHelper.java
- src/test/java/com/flowingcode/vaadin/addons/demo/MultiSourceDemo.java
- src/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: javier-godoy
Repo: FlowingCode/CommonsDemo PR: 134
File: src/main/java/com/flowingcode/vaadin/addons/demo/DemoSourceConditionHelper.java:68-84
Timestamp: 2026-01-15T16:34:56.307Z
Learning: In `DemoSourceConditionHelper.java`, the `compare()` method implements a prefix-based set membership test, not strict version ordering. The LHS is always a `major.minor.patch` version, and the RHS can be a prefix. The method returns 0 when all shared components match, which correctly implements the semantics of "Does the LHS belong to the set of releases starting with the prefix defined by the RHS?"
🔇 Additional comments (4)
src/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.java (1)
57-59: LGTM!The
isEmpty()method correctly delegates to the underlyingMultiSourceCodeViewer, providing a clean way for callers (likeTabbedDemo) to check if any source tabs remain after condition-based filtering.src/main/java/com/flowingcode/vaadin/addons/demo/SourceCodeTab.java (1)
39-39: LGTM!The new
conditionfield is appropriately placed among the optional fields and will be properly handled by Lombok's@Builderand@Getterannotations.README.md (1)
182-191: LGTM!Clear documentation with a practical example that demonstrates the
conditionattribute usage. This aligns well with the existing conditional code documentation style.src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
253-256: LGTM!The condition handling follows the same pattern as other optional builder fields and correctly passes the condition through to the
SourceCodeTab.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.