feature: Add Geolocation API support to uni-dom #407
Conversation
Provides reactive geolocation tracking using the browser's Geolocation API: - GeoPosition case class for position data (lat, lng, altitude, accuracy, etc.) - GeoError enum for error handling (PermissionDenied, PositionUnavailable, Timeout) - GeoOptions for configuring requests (high accuracy, timeout, max age) - Geolocation.getCurrentPosition() for one-shot position requests - Geolocation.watch() returns PositionWatcher with Rx[Option[GeoPosition]] Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-actions
bot
added
doc
Summary of ChangesHello @xerial, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a valuable Geolocation API integration to uni-dom, providing reactive tracking and one-shot position requests. The API design in the plans document is clear, and the implementation in Geolocation.scala correctly handles the browser's Geolocation API. The test suite covers the basic functionality of the new types and the Geolocation object. However, there are opportunities to enhance clarity in the GeoOptions design, improve error handling specificity, and expand test coverage for the reactive streams to ensure continuous updates are correctly emitted.
| // Options for position requests | ||
| case class GeoOptions( | ||
| enableHighAccuracy: Boolean = false, | ||
| timeout: Long = Long.MaxValue, |
There was a problem hiding this comment.
The default value Long.MaxValue for timeout in GeoOptions is used to signify 'no timeout' by the implementation. While functional, this might be less intuitive for users compared to using Option[Long] or aligning with the browser API's convention (e.g., 0 for maximumAge or omitting the property for timeout to use the browser's default of Infinity). Consider clarifying this in the documentation or adjusting the type to Option[Long] for better Scala idiomatic representation of an optional timeout.
| // One-shot position request | ||
| def getCurrentPosition( | ||
| onSuccess: GeoPosition => Unit, | ||
| onError: GeoError => Unit = _ => (), |
There was a problem hiding this comment.
Providing a default no-op function (_ => ()) for onError can lead to silent failures if the caller doesn't explicitly provide an error handler. While convenient for simple cases, it might mask important issues. For robust applications, it's often better to encourage explicit error handling.
| def watch(options: GeoOptions = GeoOptions()): PositionWatcher | ||
|
|
||
| class PositionWatcher extends Cancelable: | ||
| def position: Rx[Option[GeoPosition]] // None until first position |
There was a problem hiding this comment.
The comment // None until first position is helpful. To be even more precise for reactive stream users, it could explicitly state that the Rx stream will emit None as its initial value before any actual position data is acquired.
|
|
||
| class PositionWatcher extends Cancelable: | ||
| def position: Rx[Option[GeoPosition]] // None until first position | ||
| def error: Rx[Option[GeoError]] // None if no error |
There was a problem hiding this comment.
Similar to the position stream, clarifying that the error stream will emit None as its initial value when there is no error would enhance clarity for reactive stream consumers.
| test("Geolocation.getCurrentPosition accepts callbacks"): | ||
| var called = false | ||
| Geolocation.getCurrentPosition(onSuccess = _ => called = true, onError = _ => called = true) | ||
| // In jsdom environment without geolocation, this should call onError |
There was a problem hiding this comment.
This test currently only verifies that getCurrentPosition doesn't throw an exception. In a jsdom environment without geolocation support, it should specifically call the onError callback. A more robust test would mock the geolocation object to explicitly trigger success or error scenarios and assert that the correct callback (onSuccess or onError) is invoked.
| val watcher = Geolocation.watch() | ||
| var emittedValue = false | ||
| val cancel = watcher | ||
| .error | ||
| .run { _ => | ||
| emittedValue = true | ||
| } | ||
| // Initial value should be emitted | ||
| emittedValue shouldBe true | ||
| cancel.cancel | ||
| watcher.cancel |
There was a problem hiding this comment.
Similar to the position stream test, this test only confirms the initial emission of the error stream. It does not verify that actual GeoError instances are emitted when an error occurs during continuous watching. A comprehensive test would mock geolocation.watchPosition to trigger error callbacks and assert the emission of specific GeoError types.
| */ | ||
| case class GeoOptions( | ||
| enableHighAccuracy: Boolean = false, | ||
| timeout: Long = Long.MaxValue, |
There was a problem hiding this comment.
Using Long.MaxValue as a sentinel for 'no timeout' is a valid approach, but it might be more idiomatic in Scala to represent an optional timeout using Option[Long]. This would explicitly convey that the timeout parameter is optional and align with common Scala patterns for handling absence of a value. If Option[Long] is used, the toPositionOptions method would need to be updated accordingly.
timeout: Option[Long] = None,| */ | ||
| def getCurrentPosition( | ||
| onSuccess: GeoPosition => Unit, | ||
| onError: GeoError => Unit = _ => (), |
There was a problem hiding this comment.
Providing a default no-op function (_ => ()) for onError can lead to silent failures if the caller doesn't explicitly provide an error handler. While convenient for simple cases, it might mask important issues. For robust applications, it's often better to encourage explicit error handling.
| case _ => | ||
| GeoError.PositionUnavailable |
There was a problem hiding this comment.
Mapping all unknown dom.PositionError codes to GeoError.PositionUnavailable might obscure the true nature of an unexpected error. It would be more informative to introduce a GeoError.UnknownError(code: Short) or GeoError.Other(message: String) to capture and report these specific, unexpected error codes, which can aid in debugging and providing more accurate user feedback.
case _ =>
GeoError.PositionUnavailable // Or GeoError.UnknownError(err.code)| opts.enableHighAccuracy = options.enableHighAccuracy | ||
| if options.timeout != Long.MaxValue then |
There was a problem hiding this comment.
If GeoOptions.timeout were changed to Option[Long] as suggested, this if condition would be replaced by a foreach call, making the code more concise and idiomatic Scala.
options.timeout.foreach { t =>
opts.timeout = t.toDouble
}- Add safer isSupported check for non-browser environments (Node.js) - Improve Rx stream documentation to clarify initial None values - Add comments documenting error code mapping - Make toOption more robust for undefined/NaN handling Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Geolocationobject for reactive geolocation tracking using the browser's Geolocation APIGeoPositioncase class for position data (latitude, longitude, altitude, accuracy, heading, speed)GeoErrorenum for error handling (PermissionDenied, PositionUnavailable, Timeout, NotSupported)GeoOptionsfor configuring requests (enableHighAccuracy, timeout, maximumAge)getCurrentPosition()watch()returningPositionWatcherwithRx[Option[GeoPosition]]Test plan
🤖 Generated with Claude Code