Conversation
WalkthroughProject version and Vaadin upgraded; FlowingCode snapshots repo and two dependencies (json-migration-helper, lombok) added; several calendar classes annotated with Lombok Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
55-95: Approve the compatibility approach, but address the code style issues.The conditional initialization handling both
_boundOverlayRenderer(older Vaadin) and event-based (newer Vaadin) approaches is well-structured. The__fcWrappedguards prevent duplicate wrapping.However, please address the static analysis findings:
- Line 76: Replace
const self=this;with arrow functions or bind- Line 89: Replace
var calendarwithconstApply these changes:
- const self=this; - this.addEventListener('opened-changed', ev=>{ - if (ev.detail.value && !self._overlayContent._monthScroller.__fcWrapped) { + this.addEventListener('opened-changed', ev=>{ + if (ev.detail.value && !this._overlayContent._monthScroller.__fcWrapped) { this._overlayContent._monthScroller.__fcWrapped = true; - const updateElement = self._overlayContent._monthScroller._updateElement; - self._overlayContent._monthScroller._updateElement = (element, index) => { + const updateElement = this._overlayContent._monthScroller._updateElement; + this._overlayContent._monthScroller._updateElement = (element, index) => { updateElement(element,index); if (element instanceof HTMLElement) { this._updateMonthStyles(element); } }; - const createElement = self._overlayContent._monthScroller._createElement; - self._overlayContent._monthScroller._createElement = () => { - var calendar = createElement(); + const createElement = this._overlayContent._monthScroller._createElement; + this._overlayContent._monthScroller._createElement = () => { + const calendar = createElement(); setTimeout(()=>this._updateMonthStyles(calendar)); return calendar; }; } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pom.xml(4 hunks)src/main/java/com/flowingcode/addons/ycalendar/AbstractCalendarComponent.java(2 hunks)src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java(2 hunks)src/main/java/com/flowingcode/addons/ycalendar/InlineDatePicker.java(3 hunks)src/main/java/com/flowingcode/addons/ycalendar/YearMonthField.java(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js(1 hunks)src/main/resources/META-INF/frontend/fc-month-calendar/fc-month-calendar.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-24T17:59:52.000Z
Learnt from: javier-godoy
Repo: FlowingCode/YearMonthCalendarAddon PR: 96
File: src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js:128-132
Timestamp: 2025-04-24T17:59:52.000Z
Learning: In the YearMonthCalendarAddon project, the DatePicker component uses custom formatting functions (`__formatDate` and `__parseDate`) rather than `this.locale` for date formatting and localization.
Applied to files:
src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.javasrc/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.jssrc/main/java/com/flowingcode/addons/ycalendar/YearMonthField.javasrc/main/java/com/flowingcode/addons/ycalendar/InlineDatePicker.javasrc/main/java/com/flowingcode/addons/ycalendar/AbstractCalendarComponent.java
📚 Learning: 2025-04-24T18:03:00.778Z
Learnt from: javier-godoy
Repo: FlowingCode/YearMonthCalendarAddon PR: 96
File: src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js:128-132
Timestamp: 2025-04-24T18:03:00.778Z
Learning: The YearMonthCalendarAddon only supports Gregorian calendars and does not support non-Gregorian calendars such as Hijrah, as documented in issue #20.
Applied to files:
src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.javasrc/main/java/com/flowingcode/addons/ycalendar/YearMonthField.javapom.xml
🧬 Code graph analysis (2)
src/main/java/com/flowingcode/addons/ycalendar/YearMonthField.java (2)
src/main/java/com/flowingcode/addons/ycalendar/AbstractCalendarComponent.java (1)
SuppressWarnings(40-93)src/main/java/com/flowingcode/addons/ycalendar/InlineDatePicker.java (1)
SuppressWarnings(40-151)
src/main/java/com/flowingcode/addons/ycalendar/InlineDatePicker.java (2)
src/main/java/com/flowingcode/addons/ycalendar/AbstractCalendarComponent.java (1)
SuppressWarnings(40-93)src/main/java/com/flowingcode/addons/ycalendar/YearMonthField.java (1)
SuppressWarnings(37-115)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
[failure] 76-76: Do not assign this to self.
[failure] 89-89: Unexpected var, use let or const instead.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-vaadin25
🔇 Additional comments (9)
src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java (1)
22-22: LGTM! JsonMigration integration looks correct.The import and wrapping of the
fetchStylesresult withJsonMigration.convertToClientCallableResult()aligns with the broader migration pattern across the codebase. The method signature remains unchanged, maintaining API compatibility.Also applies to: 92-92
src/main/resources/META-INF/frontend/fc-month-calendar/fc-month-calendar.js (1)
119-143: LGTM! Guarded styling injection ensures shadow DOM readiness.Moving the style injection to
connectedCallbackwith a guard checking for existing styles (data="fc-component-styles") is a solid pattern. Waiting forupdateCompleteensures the shadow DOM is ready before injecting styles, preventing timing-related issues.src/main/java/com/flowingcode/addons/ycalendar/InlineDatePicker.java (1)
22-23: LGTM! JsonMigration extension method integration is consistent.The addition of
@ExtensionMethod(JsonMigration.class)and updated imports align with the broader migration pattern across all calendar components. This enables extension methods while maintaining backward compatibility.Also applies to: 38-38, 45-45
src/main/java/com/flowingcode/addons/ycalendar/AbstractCalendarComponent.java (1)
22-23: LGTM! Base class migration is consistent with the pattern.The
@ExtensionMethod(JsonMigration.class)annotation and updated imports follow the same migration pattern applied across the component hierarchy. This maintains consistency throughout the codebase.Also applies to: 34-34, 41-41
src/main/java/com/flowingcode/addons/ycalendar/YearMonthField.java (1)
22-23: LGTM! Consistent JsonMigration integration.The changes mirror those in
AbstractCalendarComponentandInlineDatePicker, maintaining a uniform migration pattern across all components.Also applies to: 35-35, 40-40
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
100-103: LGTM! Fallback query pattern is robust.The
refreshAllmethod now usesthis._overlayContentwith a fallback toquerySelector, ensuring compatibility across different initialization states.pom.xml (3)
7-7: Clarify the Vaadin version target.The PR title mentions "Vaadin 25" but Line 12 updates the main Vaadin version to 24.9.5. The v25 profile (Line 524) does reference Vaadin 25.0.0-beta5. Please clarify which version this PR primarily targets.
Also applies to: 12-12
94-103: LGTM! Snapshots repository properly configured.The FlowingCode Snapshots repository is correctly configured with snapshots enabled and releases disabled, which is appropriate for pulling the json-migration-helper SNAPSHOT dependency.
132-136: Based on my verification, I have enough information to rewrite the review comment. The artifact is confirmed available, and I can provide informed guidance on the SNAPSHOT dependency in context.
Confirm SNAPSHOT dependency is appropriate for this project phase.
The
json-migration-helper0.0.1-SNAPSHOT artifact is available in the FlowingCode Snapshots repository. Since the project itself is in SNAPSHOT phase (version 4.6.0-SNAPSHOT) and properly configured to use snapshot repositories, using a SNAPSHOT version of an internal library is acceptable during development. However, ensure that a stable release version is published before releasing this project to production, and plan to migrate away from the SNAPSHOT dependency at that time.
javier-godoy
force-pushed
the
vaadin25
branch
from
9679983 to
8ff9403
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)
pom.xml (1)
153-157: Based on my verification of the pom.xml file, I can now provide the rewritten review comment:
Duplicate
commons-demodependency with version override in v25 profileWith profile
v25active, the main<dependencies>section (lines 153-157) declarescom.flowingcode.vaadin.addons.demo:commons-demowith test scope and no explicit version (inheriting 4.0.0 from<dependencyManagement>), while the v25 profile (lines 551-556) declares the same dependency with version 5.0.0-SNAPSHOT. This creates redundancy and potential confusion about which version is active.When -Pv25 is used, Maven will resolve to 5.0.0-SNAPSHOT due to profile-level precedence, but the main dependency declaration becomes superfluous. Consider:
- Remove the base dependency declaration (lines 153-157) and rely solely on the profile-scoped override, or
- Define the version override in
<dependencyManagement>using a profile-specific property to keep declarations centralized.
🧹 Nitpick comments (2)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (2)
63-90: Replacevarwithconstand dropself = thisto satisfy Sonar and modern JS styleTwo small cleanups will align this block with Sonar and modern JavaScript:
- Use
constforcalendar(both in the renderer and in theopened-changedbranch) instead ofvar.- Remove
const self = this;and consistently usethisinside the arrow callbacks, since arrow functions already capture lexicalthis.Suggested diff:
- const self=this; - this.addEventListener('opened-changed', ev=>{ - if (ev.detail.value && !self._overlayContent._monthScroller.__fcWrapped) { - this._overlayContent._monthScroller.__fcWrapped = true; - const updateElement = self._overlayContent._monthScroller._updateElement; + this.addEventListener('opened-changed', ev=>{ + if (ev.detail.value && !this._overlayContent._monthScroller.__fcWrapped) { + this._overlayContent._monthScroller.__fcWrapped = true; + const updateElement = this._overlayContent._monthScroller._updateElement; - self._overlayContent._monthScroller._updateElement = (element, index) => { + this._overlayContent._monthScroller._updateElement = (element, index) => { updateElement(element,index); if (element instanceof HTMLElement) { this._updateMonthStyles(element); } }; - const createElement = self._overlayContent._monthScroller._createElement; - self._overlayContent._monthScroller._createElement = () => { - var calendar = createElement(); + const createElement = this._overlayContent._monthScroller._createElement; + this._overlayContent._monthScroller._createElement = () => { + const calendar = createElement(); setTimeout(()=>this._updateMonthStyles(calendar)); return calendar; }; } });And in the bound-renderer branch:
- const createElement = this._overlayContent._monthScroller._createElement; + const createElement = this._overlayContent._monthScroller._createElement; this._overlayContent._monthScroller.__fcWrapped = true; this._overlayContent._monthScroller._createElement = () => { - var calendar = createElement(); + const calendar = createElement(); calendar.addEventListener('dom-change',ev=>{
100-103:refreshAlloverlayContent fallback looks good; consider a light guard on_monthScrollerUsing
const overlayContent = this._overlayContent || this.querySelector("vaadin-date-picker-overlay-content");is a sensible way to support both the private field and the DOM element. To make this a bit more defensive against internal changes, you might add a short guard before dereferencing
_monthScroller, e.g.:- const overlayContent = this._overlayContent || this.querySelector("vaadin-date-picker-overlay-content"); - if (overlayContent) { - overlayContent._monthScroller.querySelectorAll("vaadin-month-calendar").forEach(calendar=>this._updateMonthStyles(calendar)); + const overlayContent = this._overlayContent || this.querySelector("vaadin-date-picker-overlay-content"); + const scroller = overlayContent && overlayContent._monthScroller; + if (scroller) { + scroller.querySelectorAll("vaadin-month-calendar") + .forEach(calendar => this._updateMonthStyles(calendar)); }Optional, but it would make
refreshAllmore robust if Vaadin tweaks overlay internals again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pom.xml(4 hunks)src/main/java/com/flowingcode/addons/ycalendar/AbstractCalendarComponent.java(2 hunks)src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java(2 hunks)src/main/java/com/flowingcode/addons/ycalendar/InlineDatePicker.java(3 hunks)src/main/java/com/flowingcode/addons/ycalendar/YearMonthField.java(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js(1 hunks)src/main/resources/META-INF/frontend/fc-month-calendar/fc-month-calendar.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/resources/META-INF/frontend/fc-month-calendar/fc-month-calendar.js
- src/main/java/com/flowingcode/addons/ycalendar/YearMonthField.java
- src/main/java/com/flowingcode/addons/ycalendar/AbstractCalendarComponent.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: javier-godoy
Repo: FlowingCode/YearMonthCalendarAddon PR: 96
File: src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js:128-132
Timestamp: 2025-04-24T17:59:52.000Z
Learning: In the YearMonthCalendarAddon project, the DatePicker component uses custom formatting functions (`__formatDate` and `__parseDate`) rather than `this.locale` for date formatting and localization.
📚 Learning: 2025-04-24T18:03:00.778Z
Learnt from: javier-godoy
Repo: FlowingCode/YearMonthCalendarAddon PR: 96
File: src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js:128-132
Timestamp: 2025-04-24T18:03:00.778Z
Learning: The YearMonthCalendarAddon only supports Gregorian calendars and does not support non-Gregorian calendars such as Hijrah, as documented in issue #20.
Applied to files:
pom.xmlsrc/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java
📚 Learning: 2025-04-24T17:59:52.000Z
Learnt from: javier-godoy
Repo: FlowingCode/YearMonthCalendarAddon PR: 96
File: src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js:128-132
Timestamp: 2025-04-24T17:59:52.000Z
Learning: In the YearMonthCalendarAddon project, the DatePicker component uses custom formatting functions (`__formatDate` and `__parseDate`) rather than `this.locale` for date formatting and localization.
Applied to files:
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.jssrc/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.javasrc/main/java/com/flowingcode/addons/ycalendar/InlineDatePicker.java
🪛 GitHub Check: SonarCloud Code Analysis
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
[failure] 89-89: Unexpected var, use let or const instead.
[failure] 76-76: Do not assign this to self.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-vaadin25
🔇 Additional comments (6)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
55-95: Overlay wrapping logic for month styles looks consistent; please verify_overlayContent._monthScrolleravailability across Vaadin versionsThe new split between bound (
_boundOverlayRenderer) and unbound overlay paths correctly ensures that newly created or updated month calendars go through_updateMonthStyles, and the__fcWrappedguard prevents double-wrapping. The approach looks sound for preserving behavior across Vaadin 24/25, but it assumes thatthis._overlayContentandthis._overlayContent._monthScrollerare always present by the time the renderer oropened-changedlistener runs. Please double-check this assumption against Vaadin 25’s internals (and any lazy overlay creation cases) so we don’t hit null/undefined at runtime.src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java (1)
22-22: JsonMigration wrapping offetchStylesresult looks correctUsing
JsonMigration.convertToClientCallableResult(result)keeps the existing aggregation logic while routing the payload through the new migration helper; this is consistent with the rest of the PR and should be safe from a server‑side perspective. Please just verify that this is the intended helper for@ClientCallableJSON payloads across Vaadin 24/25 injson-migration-helper.Also applies to: 84-93
src/main/java/com/flowingcode/addons/ycalendar/InlineDatePicker.java (2)
22-24: Lombok@ExtensionMethod(JsonMigration.class)integration is consistentThe added JsonMigration/JsonSerializer imports together with
@ExtensionMethod(JsonMigration.class)give this component the same JSON‑migration capabilities as the other calendar classes; no issues from a structure/usage standpoint.Also applies to: 38-46
91-99: Switch to externalJsonSerializerinsetI18nlooks appropriateReplacing the internal Vaadin
JsonSerializerwithcom.flowingcode.vaadin.jsonmigration.JsonSerializer.toJson(i18n)keeps the semantics of writing a JSON i18n object to the element while routing through the migration helper; null‑guard is preserved.pom.xml (2)
7-13: Core version and Vaadin bump align with Vaadin 24.9.5 baselineUpdating the artifact to
4.6.0-SNAPSHOTandvaadin.versionto24.9.5is consistent with adding a Vaadin 25 profile and the rest of the PR; nothing problematic spotted here.
94-103: Snapshot repo and json-migration/Lombok dependencies look coherentAdding the FlowingCode Snapshots repository plus the
json-migration-helpersnapshot and Lombok (scopeprovided) matches the new JsonMigration usage in the Java sources and is a reasonable setup for this PR. Keep in mind that shipping with snapshot dependencies is a process decision; verify this is acceptable for your release strategy.Also applies to: 132-142
github-project-automation
bot
moved this from To Do
to In Progress
in Flowing Code Addons
github-project-automation
bot
moved this from In Progress
to Pending release
in Flowing Code Addons
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.