Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds client-side persistence for altitude adjustments received from the server. The feature stores altitude adjustments from track responses and includes them in subsequent track requests, enabling the server to maintain continuity of altitude data across multiple location updates.
Changes:
- Added storage and retrieval methods for altitude adjustments in RadarState
- Modified track request to include stored altitude adjustments
- Modified track response handling to extract and persist altitude adjustments from the user object
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/src/main/java/io/radar/sdk/RadarState.kt | Added getAltitudeAdjustments and setAltitudeAdjustments methods to persist altitude adjustments in SharedPreferences as JSON |
| sdk/src/main/java/io/radar/sdk/RadarApiClient.kt | Added logic to include stored altitude adjustments in track requests and extract/store them from track responses |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| internal fun getAltitudeAdjustments(context: Context): JSONArray? { | ||
| val jsonString = getSharedPreferences(context).getString(KEY_ALTITUDE_ADJUSTMENTS, null) | ||
| return jsonString?.let { | ||
| try { | ||
| JSONArray(it) | ||
| } catch (e: Exception) { | ||
| null | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The getAltitudeAdjustments method duplicates the logic from the existing stringToJsonObject helper function. For consistency with how getLastMotionActivity (line 271) and getLastPressure (line 283) are implemented, consider creating a similar helper function like stringToJsonArray and using it here.
| // Include stored altitudeAdjustments from previous track responses | ||
| val altitudeAdjustments = RadarState.getAltitudeAdjustments(context) | ||
| if (altitudeAdjustments != null && altitudeAdjustments.length() > 0) { | ||
| params.putOpt("altitudeAdjustments", altitudeAdjustments) | ||
| logger.d("Including ${altitudeAdjustments.length()} altitude adjustments in track request") | ||
| } |
There was a problem hiding this comment.
The new altitude adjustments persistence feature lacks test coverage. Given that the repository has comprehensive tests for similar persistence features (e.g., tags persistence in test_Radar_tags_persistence at line 516), consider adding tests to verify that altitude adjustments are correctly persisted to SharedPreferences and included in track requests. Tests should cover scenarios where altitude adjustments are retrieved from storage and included in requests, and where they are extracted from responses and stored.
| // Extract and store altitudeAdjustments from user object | ||
| val altitudeAdjustmentsObj = userObj.optJSONArray("altitudeAdjustments") | ||
| if (altitudeAdjustmentsObj != null) { | ||
| RadarState.setAltitudeAdjustments(context, altitudeAdjustmentsObj) | ||
| } |
There was a problem hiding this comment.
The altitude adjustments are only stored when present in the response, but never explicitly cleared when absent. This means once altitude adjustments are received, they will persist indefinitely until new ones are provided by the server. If the server's intent is to clear altitude adjustments by omitting the field or sending null/empty array, this logic won't handle that case. Consider also storing altitude adjustments when the field is explicitly null or an empty array to ensure they can be cleared when needed.
| // Extract and store altitudeAdjustments from user object | |
| val altitudeAdjustmentsObj = userObj.optJSONArray("altitudeAdjustments") | |
| if (altitudeAdjustmentsObj != null) { | |
| RadarState.setAltitudeAdjustments(context, altitudeAdjustmentsObj) | |
| } | |
| // Extract and store altitudeAdjustments from user object. | |
| // Always call setter so altitude adjustments can be cleared when absent or null. | |
| val altitudeAdjustmentsObj = userObj.optJSONArray("altitudeAdjustments") | |
| RadarState.setAltitudeAdjustments(context, altitudeAdjustmentsObj) |
1 participant
No description provided.