feat: add GeolocationClient port for external test drivers#24211
Merged
Conversation
Carve a small GeolocationClient port between the Geolocation facade and
the underlying delivery mechanism. The production implementation,
BrowserGeolocationClient, carries the existing wire behavior unchanged:
same executeJs expressions, same DOM event names, same target elements.
The facade and GeolocationTracker now delegate to the port instead of
calling executeJs directly.
Two methods are added as public framework-internal entry points so
external browserless test drivers can replace the production client and
inspect tracker handles without reflection:
- Geolocation.setClient(GeolocationClient) swaps the active client,
closing the previous one and re-wiring the availability subscription.
- GeolocationTracker.handle() exposes the active WatchHandle (or null
when stopped) so test drivers can push synthetic position/error
updates to a specific tracker.
The port uses SerializableConsumer rather than java.util.function.Consumer
to keep listener lambdas serializable. Geolocation.availabilitySubscription
is transient because the Registration lambda's nested SerializedLambda
does not deserialize cleanly; the underlying listener stays in the
client's listener list, and setClient() unconditionally closes the prior
client to drop any orphaned wiring.
Wire protocol unchanged; the existing GeolocationTest cases verify this.
Verify the four invariants of the Geolocation.setClient and
GeolocationTracker.handle() seam:
- setClient routes Geolocation.get(...) through the installed client.
- setClient closes the previous client when replacing it.
- track(...) returns a tracker whose handle() comes from the client
that was active at track() time.
- handle() returns null after stop().
Uses an in-test FakeClient implementing GeolocationClient directly, with
no external test infrastructure.
3 tasks
GeolocationClient, Geolocation#setClient, and GeolocationTracker#handle are now package-private. The split-package test driver in browserless-test lives in the same package and can access them directly without the SPI being exposed as public API.
github-project-automation
Bot
moved this to 🔎Iteration reviews
in Vaadin Flow | Hilla | Kits ongoing work
mcollovati
requested changes
Apr 29, 2026
mcollovati
reviewed
Apr 29, 2026
| } else if (result.error() != null) { | ||
| future.complete(result.error()); | ||
| } else { | ||
| LOGGER.debug( |
Collaborator
There was a problem hiding this comment.
I guess here we should complete the future exceptionally.
| LOGGER.debug( | ||
| "Geolocation get() returned neither position nor error"); | ||
| } | ||
| }, err -> LOGGER.debug("Client-side geolocation.get failed: {}", |
Collaborator
There was a problem hiding this comment.
I guess here we should complete the future exceptionally.
Drop redundant availabilitySubscription field — BrowserGeolocationClient.close() clears its listeners on its own. Eagerly install the production client via setClient(...) instead of a lazy accessor; pass the bootstrap-seeded availability into the BrowserGeolocationClient constructor so the SPI doesn't read framework internals.
When the browser response has neither a position nor an error, or when executeJs fails, the CompletableFuture from BrowserGeolocationClient.get(...) was previously orphaned. Callers chained via .exceptionally(...) got nothing and .thenAccept(...) silently never fired. Complete the future exceptionally so failures surface.
3 tasks
mcollovati
requested changes
Apr 30, 2026
| } | ||
| }, err -> LOGGER.debug("Client-side geolocation.get failed: {}", | ||
| err)); | ||
| client.get(options).thenAccept(callback); |
Collaborator
There was a problem hiding this comment.
This code now silently ignores the potential exception instead of logging it.
Member
Author
There was a problem hiding this comment.
Good catch — switched to whenComplete so an exceptionally completed future logs a warning instead of being swallowed silently.
Replace thenAccept with whenComplete so an exceptionally completed future from the client logs a warning instead of being silently swallowed.
mcollovati
reviewed
May 4, 2026
| client.get(options).thenAccept(callback); | ||
| client.get(options).whenComplete((outcome, error) -> { | ||
| if (error != null) { | ||
| LOGGER.warn("Geolocation get() failed", error); |
Collaborator
There was a problem hiding this comment.
IIRC we usually use DEBUG level in such cases to avoid flooding the log
|
mcollovati
approved these changes
May 4, 2026
4 tasks
mshabarov
moved this from 🔎Iteration reviews
to Done
in Vaadin Flow | Hilla | Kits ongoing work
Collaborator
|
This ticket/PR has been released with Vaadin 25.2.0-alpha6. |
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
4 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.
Summary
GeolocationClientport behind theGeolocationfacade andGeolocationTracker. Production wire behavior moves to a newBrowserGeolocationClient;executeJsexpressions, DOM event names and target elements are unchanged.Geolocation.setClient(GeolocationClient)andGeolocationTracker.handle()as public framework-internal entry points so external browserless test drivers (e.g.vaadin/browserless-test) can swap the production client and reach the activeWatchHandlewithout reflection.SerializableConsumer<T>for the port's listener types and markGeolocation.availabilitySubscriptiontransientso dev-mode UI/session serialization keeps working.Adds the API surface needed by the Geolocation browserless testing PRD requirement. The actual test driver lives in
vaadin/browserless-test(separate PR) — keeping it out offlowavoids dragging Selenium / TestBench into the dependency tree of consumers that only want browserless unit tests.Test plan
mvn test -pl :flow-server -Dtest=GeolocationTest— 33 wire-protocol assertions still pass (production behavior preserved byte-for-byte)mvn test -pl :flow-server -Dtest=GeolocationClientSeamTest— 4 new tests pin thesetClient+handle()contractmvn test -pl :flow-server -Dtest=SerializationTest,FlowClassesSerializableTest— dev-mode UI/session serialization still passesvaadin/browserless-test(feat/geolocation-test-support) consumes this branch; ran against use-cases/geolocation, all 7 PRD use cases testable browserlessly (12 tests, 0 failures)