feat: add API to define geolocation options if needed #160
Conversation
Those options can help to get more accurate results on trackLocation and goToCurrentLocation features. Close #151
WalkthroughAdds a GeolocationOptions model and integrates it into GoogleMap API and frontend so geolocation calls (current location and tracking) can accept enableHighAccuracy, timeout, and maximumAge; updates demo to use the new options. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 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/googlemaps/GoogleMap.java (1)
659-671: Fix race and false “activated” event; set a pending sentinel and gate event firing.
- Race: Because trackLocationId is assigned asynchronously in the .then callback, a second call can pass the null check and start another watch. Mark the session as “pending” before calling JS to prevent concurrent starts.
- False activation: If geolocation is unsupported/denied, JS returns null/undefined. The current code still fires LocationTrackingActivatedEvent. Gate the event on a non-null id.
Apply this focused change:
- public void trackLocation(GeolocationOptions options) { - if (getTrackLocationId() == null) { - JsonObject optionsJson = options == null ? Json.createObject() : options.toJson(); - getElement().executeJs("return geolocation.trackLocation($0, $1)", this, optionsJson) - .then(Integer.class, trackLocationId -> { - this.trackLocationId = trackLocationId; - ComponentUtil.fireEvent(this, new LocationTrackingActivatedEvent(this, false)); - }); - } else { + public void trackLocation(GeolocationOptions options) { + if (getTrackLocationId() == null) { + // Mark as pending to avoid concurrent starts before the async callback runs. + this.trackLocationId = -1; + JsonObject optionsJson = options == null ? Json.createObject() : options.toJson(); + getElement().executeJs("return geolocation.trackLocation($0, $1)", this, optionsJson) + .then(Integer.class, trackLocationId -> { + if (trackLocationId != null) { + this.trackLocationId = trackLocationId; + ComponentUtil.fireEvent(this, new LocationTrackingActivatedEvent(this, false)); + } else { + // Reset on failure/unavailable geolocation (JS returned null/undefined). + this.trackLocationId = null; + } + }); + } else { throw new IllegalStateException( "A tracking location session is already active. Please stop the current session before starting a new one."); } }Additionally, align stopTrackLocation to ignore the pending sentinel (place this outside the selected range):
// In stopTrackLocation(): if (trackLocationId != null && trackLocationId >= 0) { getElement().executeJs("geolocation.clearTracking($0)", trackLocationId); } trackLocationId = null;
🧹 Nitpick comments (5)
src/main/resources/META-INF/resources/frontend/googlemaps/geolocation.js (2)
22-34: Forwarding options to getCurrentPosition is correct; add a small defensive guard and (optionally) propagate error details.
- Guard against non-object values to avoid passing unexpected types; the browser treats undefined or {} the same for defaults.
- Optional: capture the PositionError object and forward its code/message server-side in a future API to aid diagnostics.
Apply this focused change to coerce options locally:
- get: function(map, options) { + get: function(map, options) { + const opts = options && typeof options === 'object' ? options : undefined; // if browser supports geolocation, return current location if (navigator.geolocation) { - navigator.geolocation.getCurrentPosition( + navigator.geolocation.getCurrentPosition( position => { map.$server.handleGeolocation(position.coords.latitude, position.coords.longitude); }, - () => { + /* Consider: error => { this._handleGeolocationError(true, map); } */ + () => { this._handleGeolocationError(true, map); - }, - options + }, + opts );
41-53: Returning watch id is correct; initialize explicitly and coerce options.
- When geolocation is unavailable, the function currently returns undefined; returning null is clearer and avoids ambiguity downstream.
- Same options guard as above keeps behavior robust.
- trackLocation: function(map, options) { - let trackLocationId; + trackLocation: function(map, options) { + let trackLocationId = null; + const opts = options && typeof options === 'object' ? options : undefined; if (navigator.geolocation) { - trackLocationId = navigator.geolocation.watchPosition( + trackLocationId = navigator.geolocation.watchPosition( position => { map.$server.handleGeolocation(position.coords.latitude, position.coords.longitude); }, - () => { + /* Consider: error => { this._handleGeolocationError(true, map); } */ + () => { this._handleGeolocationError(true, map); - }, - options + }, + opts ); } else { // browser doesn't support geolocation this._handleGeolocationError(false, map); } return trackLocationId; },src/main/java/com/flowingcode/vaadin/addons/googlemaps/GeolocationOptions.java (1)
63-75: Validate non-negative numeric options before serializing.
Timeout and maximumAge must not be negative per the Web Geolocation spec. Add a quick guard to prevent sending invalid options to the client.Minimal tweak within this method:
public JsonObject toJson() { + validate(); JsonObject json = Json.createObject(); if (enableHighAccuracy != null) { json.put("enableHighAccuracy", enableHighAccuracy); } if (timeout != null) { json.put("timeout", timeout); } if (maximumAge != null) { json.put("maximumAge", maximumAge); } return json; }And add this helper (outside the selected range):
private void validate() { if (timeout != null && timeout < 0) { throw new IllegalArgumentException("timeout must be >= 0"); } if (maximumAge != null && maximumAge < 0) { throw new IllegalArgumentException("maximumAge must be >= 0"); } }src/test/java/com/flowingcode/vaadin/addons/googlemaps/TrackLocationDemo.java (1)
52-52: Handle failed activation in the UI (optional).
If geolocation is denied/unsupported, the current code enables the Stop button on click but never re-disables it. You already show a Notification on error; also toggle buttons for a smoother UX.Patch the existing error listener to reset buttons:
- gmaps.addGeolocationErrorEventListener(e -> Notification.show(e.isBrowserHasGeolocationSupport() - ? "The geolocation service failed on retrieving your location." - : "Your browser doesn't support geolocation.")); + gmaps.addGeolocationErrorEventListener(e -> { + Notification.show(e.isBrowserHasGeolocationSupport() + ? "The geolocation service failed on retrieving your location." + : "Your browser doesn't support geolocation."); + // Reset buttons on failure + startLocationTrackingButton.setEnabled(true); + stopLocationTrackingButton.setEnabled(false); + });src/main/java/com/flowingcode/vaadin/addons/googlemaps/GoogleMap.java (1)
549-560: Optional: pass “undefined” instead of empty object to JS when no options.
This preserves pure browser defaults and avoids sending an empty object over the wire. Vaadin doesn’t support “omit arg” easily, so this is non-trivial and fine to skip. Mentioned for completeness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/main/java/com/flowingcode/vaadin/addons/googlemaps/GeolocationOptions.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/googlemaps/GoogleMap.java(4 hunks)src/main/resources/META-INF/resources/frontend/googlemaps/geolocation.js(4 hunks)src/test/java/com/flowingcode/vaadin/addons/googlemaps/TrackLocationDemo.java(2 hunks)
🔇 Additional comments (6)
src/main/resources/META-INF/resources/frontend/googlemaps/geolocation.js (1)
5-5: LGTM: license header year updated.
No issues.src/main/java/com/flowingcode/vaadin/addons/googlemaps/GeolocationOptions.java (1)
35-38: ✔ Lombok is already configured in the buildThe
pom.xmldeclares Lombok as a provided-scope dependency:<dependency> <groupId>org.projectlombok</groupId> <artifactId>lombok</artifactId> <scope>provided</scope> <version>1.18.34</version> </dependency>Multiple classes (e.g.
GeolocationOptions.java) rely on Lombok annotations, and the build includes the necessary dependency. No further action is needed.src/test/java/com/flowingcode/vaadin/addons/googlemaps/TrackLocationDemo.java (1)
45-47: Good: demo shows the intended high-accuracy options.
Constructing GeolocationOptions(true, 5000L, 0L) matches the linked issue’s recommendation and clearly documents intent.src/main/java/com/flowingcode/vaadin/addons/googlemaps/GoogleMap.java (3)
539-548: Delegation reads well.
goToCurrentLocation() delegating to the options overload keeps API backward compatible and reduces duplication.
549-560: Options serialization and client call look correct.
Building an empty JsonObject when options is null is fine; the browser will use defaults.
627-643: Delegation for trackLocation() is fine.
Keeps old API while enabling the new options-aware variant.
| * If true and if the device is able to provide a more accurate position, it will do so. This may | ||
| * result in slower response times or increased power consumption. | ||
| */ | ||
| private Boolean enableHighAccuracy; |
There was a problem hiding this comment.
In Geolocation API enableHighAccuracy defaults to false, so we can avoid the wrapper and use primitive instead.
| * A positive long value representing the maximum age (in milliseconds) of a possible cached | ||
| * position that is acceptable to return. | ||
| */ | ||
| private Long maximumAge; |
There was a problem hiding this comment.
In Geolocation API maximumAge to 0, so we can avoid the wrapper and use primitive instead.
|
|
||
| /** | ||
| * A positive long value representing the maximum length of time (in milliseconds) the device is | ||
| * allowed to take in order to return a position. |
There was a problem hiding this comment.
Document that the default value (null) means that the device won't timeout until the position is available
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
Those options can help to get more accurate results on trackLocation and goToCurrentLocation features.
Close #151
Summary by CodeRabbit