Skip to content

Fix: Duplicate weather API call during startup and refresh #182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bosankus wants to merge 1 commit into
base: develop
Choose a base branch
from
+56 −26
Open

Fix: Duplicate weather API call during startup and refresh #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions app/src/main/java/bose/ankush/weatherify/data/repository/WeatherRepositoryImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,26 @@ constructor(
val isDataStale = forceRefresh || (currentTime - lastUpdateTime) > ONE_HOUR_IN_MILLIS

if (isDataStale) {
val weatherData = networkRepository.refreshWeatherData(coordinates)

if (weatherData != null) {
val weatherStorageData =
NetworkToStorageMapper.mapWeatherToStorageEntity(weatherData)
val airQualityStorageData =
NetworkToStorageMapper.mapAirQualityToStorageEntity(
weatherData.data?.airQuality,
)
weatherStorage.saveWeatherData(weatherStorageData, airQualityStorageData)
weatherStorage.saveLastWeatherUpdateTime(coordinates, currentTime)
}
networkRepository.refreshWeatherData(coordinates).fold(
onSuccess = { data ->
data?.let {
val weatherStorageData =
NetworkToStorageMapper.mapWeatherToStorageEntity(it)
val airQualityStorageData =
NetworkToStorageMapper.mapAirQualityToStorageEntity(
it.data?.airQuality,
)
weatherStorage.saveWeatherData(
weatherStorageData,
airQualityStorageData
)
weatherStorage.saveLastWeatherUpdateTime(coordinates, currentTime)
}
},
onFailure = {
// Log error or pass it to UI
}
Comment on lines +65 to +67

@coderabbitai coderabbitai Bot Jun 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Empty onFailure silently swallows errors.

The failure branch discards the exception without logging or propagating it. This means:

  • No observability into network failures
  • Users won't know why a refresh produced no change
  • Debugging production issues becomes harder

At minimum, log the error. Consider also propagating to the UI if the method signature allows.

🔧 Proposed fix to add logging 
                     onFailure = {
-                        // Log error or pass it to UI
+                        // TODO: Consider propagating to UI for user feedback
+                        Log.e("WeatherRepository", "Failed to refresh weather data", it)
                     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
onFailure = {
// Log error or pass it to UI
}
onFailure = {
// TODO: Consider propagating to UI for user feedback
Log.e("WeatherRepository", "Failed to refresh weather data", it)
}
🤖 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
`@app/src/main/java/bose/ankush/weatherify/data/repository/WeatherRepositoryImpl.kt`
around lines 65 - 67, The onFailure block in the WeatherRepositoryImpl class is
empty and silently discards errors without any logging or propagation. Replace
the empty onFailure implementation with proper error handling by logging the
exception using an appropriate logging mechanism (such as Log.e or a logger
instance), and include the error message and stack trace for debugging. If the
method signature supports it, consider also propagating the error to the UI
layer through a callback, LiveData, Flow, or similar mechanism so users are
informed of failures.

)
}
}
}
Expand Down
1 change: 1 addition & 0 deletions network/src/commonMain/kotlin/bose/ankush/network/model/LocationModels.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ private object ObjectIdAsStringSerializer : KSerializer<String> {
*/
@Serializable
data class SavedLocation(
@SerialName("_id")
@Serializable(with = ObjectIdAsStringSerializer::class)
val id: String = "",
val userEmail: String = "",
Expand Down
9 changes: 7 additions & 2 deletions network/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ package bose.ankush.network.repository
import bose.ankush.network.model.WeatherForecast

interface WeatherRepository {
/** Fetches fresh weather data from the network. Returns null if offline. */
suspend fun refreshWeatherData(coordinates: Pair<Double, Double>): WeatherForecast?
/**
* Fetches fresh weather data from the network.
*
* Returns [Result.failure] with a [bose.ankush.network.common.NetworkException] when offline
* or the request fails, so callers can surface the actual cause instead of a silent null.
*/
suspend fun refreshWeatherData(coordinates: Pair<Double, Double>): Result<WeatherForecast?>
}
31 changes: 22 additions & 9 deletions network/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepositoryImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,38 @@ package bose.ankush.network.repository

import bose.ankush.network.api.WeatherApiService
import bose.ankush.network.common.NetworkConnectivity
import bose.ankush.network.common.NetworkException
import bose.ankush.network.model.WeatherForecast
import bose.ankush.network.utils.NetworkUtils

class WeatherRepositoryImpl(
private val apiService: WeatherApiService,
private val networkConnectivity: NetworkConnectivity,
) : WeatherRepository {
override suspend fun refreshWeatherData(coordinates: Pair<Double, Double>): WeatherForecast? {
if (!networkConnectivity.isNetworkAvailable()) return null
override suspend fun refreshWeatherData(
coordinates: Pair<Double, Double>,
): Result<WeatherForecast?> {
if (!networkConnectivity.isNetworkAvailable()) {
return Result.failure(
NetworkException(
NetworkException.NETWORK_UNAVAILABLE,
"No internet connection available",
),
)
}

return try {
NetworkUtils.retryWithExponentialBackoff {
apiService.getOneCallWeather(
coordinates.first.toString(),
coordinates.second.toString(),
)
}
Result.success(
NetworkUtils.retryWithExponentialBackoff {
apiService.getOneCallWeather(
coordinates.first.toString(),
coordinates.second.toString(),
)
},
)
} catch (e: Exception) {
throw Exception("Network | Failed to refresh weather data: ${e.message}", e)
// retryWithExponentialBackoff already normalizes failures to NetworkException.
Result.failure(e)
Comment on lines 34 to +36

@coderabbitai coderabbitai Bot Jun 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check what exceptions retryWithExponentialBackoff throws and whether it handles CancellationException

# Find the implementation of retryWithExponentialBackoff
ast-grep --pattern $'suspend fun retryWithExponentialBackoff($$$) {
  $$$
}'

# Also search for any CancellationException handling
rg -n -C5 'CancellationException' --type=kotlin

Repository: bosankus/Compose-Weatherify

Length of output: 25115

🏁 Script executed:

# Find retryWithExponentialBackoff function definition
rg -n "retryWithExponentialBackoff" --type=kotlin -B2 -A10

Repository: bosankus/Compose-Weatherify

Length of output: 10782

🏁 Script executed:

# View the full WeatherRepositoryImpl.kt file to see lines 16-37
cat -n network/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepositoryImpl.kt

Repository: bosankus/Compose-Weatherify

Length of output: 1693

🏁 Script executed:

# View the complete NetworkUtils.kt file
cat -n network/src/commonMain/kotlin/bose/ankush/network/utils/NetworkUtils.kt

Repository: bosankus/Compose-Weatherify

Length of output: 1552

Catching CancellationException breaks structured concurrency.

The broad catch (e: Exception) will also catch CancellationException, which should propagate to allow proper coroutine cancellation. Although retryWithExponentialBackoff currently converts CancellationException to NetworkException, the defensive pattern of explicitly propagating CancellationException should be applied here to match the rest of the codebase (e.g., TokenManager.kt, PaymentViewModel.kt).

🔧 Proposed fix 
         } catch (e: Exception) {
+            if (e is kotlinx.coroutines.CancellationException) throw e
             // retryWithExponentialBackoff already normalizes failures to NetworkException.
             Result.failure(e)
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} catch (e: Exception) {
throw Exception("Network | Failed to refresh weather data: ${e.message}", e)
// retryWithExponentialBackoff already normalizes failures to NetworkException.
Result.failure(e)
} catch (e: Exception) {
if (e is kotlinx.coroutines.CancellationException) throw e
// retryWithExponentialBackoff already normalizes failures to NetworkException.
Result.failure(e)
}
🤖 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
`@network/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepositoryImpl.kt`
around lines 34 - 36, The catch block in WeatherRepositoryImpl.kt that catches
broad Exception type is inadvertently catching CancellationException, which
should propagate to maintain structured concurrency. Modify the catch block to
check if the caught exception is a CancellationException and explicitly rethrow
it before wrapping other exceptions in Result.failure, following the defensive
pattern used in other parts of the codebase like TokenManager.kt and
PaymentViewModel.kt.

}
}
}
9 changes: 6 additions & 3 deletions storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import bose.ankush.storage.room.WeatherEntity
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import kotlinx.coroutines.withContext

class WeatherStorageImpl(
Expand All @@ -19,6 +21,7 @@ class WeatherStorageImpl(
// In-memory per-location timestamp map. Keyed by "lat_lon" string.
// Reset on process restart intentionally — fresh data should be fetched after a cold start.
private val locationTimestamps = mutableMapOf<String, Long>()
private val timestampsMutex = Mutex()

private fun locationKey(coordinates: Pair<Double, Double>) =
"${coordinates.first}_${coordinates.second}"
Expand All @@ -30,13 +33,13 @@ class WeatherStorageImpl(
weatherDatabase.weatherDao().getAirQuality().map { it?.toAirQualityData() }

override suspend fun getLastWeatherUpdateTime(coordinates: Pair<Double, Double>): Long =
locationTimestamps[locationKey(coordinates)] ?: 0L
timestampsMutex.withLock { locationTimestamps[locationKey(coordinates)] ?: 0L }

override suspend fun saveLastWeatherUpdateTime(
coordinates: Pair<Double, Double>,
time: Long,
) {
locationTimestamps[locationKey(coordinates)] = time
timestampsMutex.withLock { locationTimestamps[locationKey(coordinates)] = time }
}

override suspend fun saveWeatherData(
Expand All @@ -55,7 +58,7 @@ class WeatherStorageImpl(
withContext(Dispatchers.IO) {
weatherDatabase.weatherDao().clearAll()
}
locationTimestamps.clear()
timestampsMutex.withLock { locationTimestamps.clear() }
}

private fun List<Weather?>?.toWeatherConditions() =
Expand Down
Loading