feat: add listener for share actions#24
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughAdds a share-action listener API and frontend event dispatch. Java exposes ChangesShare Listener Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/shareeasy/BaseShareEasy.java (1)
180-190: 💤 Low valueConsider validating driverName before dispatching the event.
The frontend should always provide a driver name, but if the event payload is malformed or a future change breaks this contract,
driverNamecould be null. Consider adding a guard to skip calling the listener or log a warning when critical event data is missing.🛡️ Optional defensive check
private void registerShareListener() { if (shareListener == null) { return; } component.getElement().addEventListener("driver-clicked", event -> { JsonObject detail = event.getEventData(); String driverName = getStringOrNull(detail, "event.detail.name"); String link = getStringOrNull(detail, "event.detail.link"); + if (driverName == null) { + // Log warning or skip event if critical data is missing + return; + } shareListener.onShare(new ShareEasyClickEvent(component, driverName, link)); }).addEventData("event.detail.name").addEventData("event.detail.link"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/shareeasy/BaseShareEasy.java` around lines 180 - 190, The registerShareListener method currently dispatches a ShareEasyClickEvent without checking that driverName is present; update registerShareListener to validate driverName (using getStringOrNull result) before calling shareListener.onShare — if driverName is null or empty, skip calling shareListener (and optionally log a warning via a Logger) so malformed events are ignored; keep the rest of the logic (link handling and adding event data) unchanged and reference registerShareListener, shareListener, getStringOrNull, driverName, and ShareEasyClickEvent when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/java/com/flowingcode/vaadin/addons/shareeasy/BaseShareEasy.java`:
- Around line 180-190: The registerShareListener method currently dispatches a
ShareEasyClickEvent without checking that driverName is present; update
registerShareListener to validate driverName (using getStringOrNull result)
before calling shareListener.onShare — if driverName is null or empty, skip
calling shareListener (and optionally log a warning via a Logger) so malformed
events are ignored; keep the rest of the logic (link handling and adding event
data) unchanged and reference registerShareListener, shareListener,
getStringOrNull, driverName, and ShareEasyClickEvent when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dce88ace-9ad6-4728-9cd7-bbfd2987df39
📒 Files selected for processing (10)
README.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/shareeasy/BaseShareEasy.javasrc/main/java/com/flowingcode/vaadin/addons/shareeasy/ShareEasyClickEvent.javasrc/main/java/com/flowingcode/vaadin/addons/shareeasy/ShareEasyClickListener.javasrc/main/java/com/flowingcode/vaadin/addons/shareeasy/enums/Driver.javasrc/main/resources/META-INF/resources/frontend/src/fc-sharee-connector.jssrc/test/java/com/flowingcode/vaadin/addons/shareeasy/NormalModeDemo.javasrc/test/java/com/flowingcode/vaadin/addons/shareeasy/it/ShareEasyElement.javasrc/test/java/com/flowingcode/vaadin/addons/shareeasy/it/ShareEasyNormalModeIT.java
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/flowingcode/vaadin/addons/shareeasy/ShareEasyClickEvent.java`:
- Around line 49-53: The ShareEasyClickEvent constructor should validate
required params: in ShareEasyClickEvent(Component source, String driverName,
String link) add null checks (e.g., Objects.requireNonNull or explicit checks)
for driverName (and optionally source) and throw an IllegalArgumentException
with a clear message if missing so the event cannot be created with a null
driverName; update the constructor to perform these checks before assigning
fields so getDriverName() never returns null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cc97a56-f4fc-483c-bba8-46ea9c196c9e
📒 Files selected for processing (10)
README.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/shareeasy/BaseShareEasy.javasrc/main/java/com/flowingcode/vaadin/addons/shareeasy/ShareEasyClickEvent.javasrc/main/java/com/flowingcode/vaadin/addons/shareeasy/ShareEasyClickListener.javasrc/main/java/com/flowingcode/vaadin/addons/shareeasy/enums/Driver.javasrc/main/resources/META-INF/resources/frontend/src/fc-sharee-connector.jssrc/test/java/com/flowingcode/vaadin/addons/shareeasy/NormalModeDemo.javasrc/test/java/com/flowingcode/vaadin/addons/shareeasy/it/ShareEasyElement.javasrc/test/java/com/flowingcode/vaadin/addons/shareeasy/it/ShareEasyNormalModeIT.java
paodb
force-pushed
the
feature/12-share-listener
branch
from
930d89a to
278285f
Compare
javier-godoy
left a comment
javier-godoy
left a comment
There was a problem hiding this comment.
Code Review Findings
1. Event listeners accumulate on repeated forComponent() calls
File:
Line: 178
In , the is not stored, so if is called multiple times on the same instance, listeners accumulate without deduplication:
component.getElement().addEventListener("driver-clicked", event -> {
// ...
}).addEventData("event.detail.name").addEventData("event.detail.link");
// Registration is discarded - no way to remove old listenerFailure scenario: Calling then leaves both listeners active. Subsequent clicks fire multiple times.
Fix: Store the and remove the previous listener before registering a new one.
2. JsonObject usage not compatible with Vaadin 24-25 json-migration-helper requirement
File:
Line: 185
The code uses and directly:
private static String getStringOrNull(JsonObject data, String key) {
return data.hasKey(key) && data.get(key) instanceof JsonString ? data.getString(key) : null;
}Vaadin 24-25 requires for JsonObject compatibility. This usage may not be properly migrated.
Fix: Verify the json-migration-helper is applied to this code path or refactor to use the migrated JSON API.
github-project-automation
Bot
moved this from To Do
to In Progress
in Flowing Code Addons
javier-godoy
left a comment
javier-godoy
left a comment
There was a problem hiding this comment.
Code Review Findings
1. Event listeners accumulate on repeated forComponent() calls
File: src/main/java/com/flowingcode/vaadin/addons/shareeasy/BaseShareEasy.java
Line: 178
In registerShareListener(), the DomListenerRegistration is not stored, so if forComponent() is called multiple times on the same instance, listeners accumulate without deduplication.
The addEventListener call chain doesn't store the Registration, so the old listener can never be removed. Each call to forComponent() adds another listener.
Failure scenario: Calling forComponent(div1) then forComponent(div2) leaves both listeners active. Subsequent clicks fire multiple times.
Fix: Store the DomListenerRegistration and remove the previous listener before registering a new one.
2. JsonObject usage not compatible with Vaadin 24-25 json-migration-helper requirement
File: src/main/java/com/flowingcode/vaadin/addons/shareeasy/BaseShareEasy.java
Line: 185
The code uses elemental.json.JsonObject and elemental.json.JsonString directly. Vaadin 24-25 requires json-migration-helper for proper JsonObject compatibility. This usage may not be properly migrated for full compatibility.
Fix: Verify the json-migration-helper is applied to this code path or refactor to use the migrated JSON API.
javier-godoy
left a comment
javier-godoy
left a comment
There was a problem hiding this comment.
Comments have been addressed, please squash WIP.
Done @javier-godoy |
paodb
force-pushed
the
feature/12-share-listener
branch
from
6b80e26 to
3c0a5b9
Compare
github-project-automation
Bot
moved this from In Progress
to Pending release
in Flowing Code Addons
|
2 participants
Description
Adds the ability to listen for share actions, as requested in #12.
A new
withShareListener(ShareEasyClickListener)builder method registers alistener that is notified whenever a share driver is clicked. The
ShareEasyClickEventexposes:getDriverName()- the clicked driver's name (e.g."telegram")getDriver()- the resolvedDriverfor default drivers (Optional)getLink()- the share link (the copied URL for the copy driver)getSource()- the component the Share Easy instance is attached toHow it works
This builds on the upstream
driver-clickedevent added in sharee 1.1.24(parsagholipour/sharee#6). The connector:
getNameso the event carries a stabledriver key instead of the bundled (build-dependent) class name;
onClickis fullyoverridden by the add-on - carrying the actual copied URL.
Additional changes
build: bumpwebdrivermanagerto 5.9.2 so the integration tests canresolve a driver for current Chrome versions.
build: set version to 2.2.0-SNAPSHOT.Testing
ShareEasyNormalModeIT.withShareListener_clickDriver_firesEvent.Close #12
Summary by CodeRabbit
"copy".