Fix: Duplicate weather API call during startup and refresh#182
Conversation
📝 WalkthroughWalkthrough
ChangesResult-based error handling and storage fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@app/src/main/java/bose/ankush/weatherify/data/repository/WeatherRepositoryImpl.kt`:
- Around line 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.
In
`@network/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepositoryImpl.kt`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b289cbf5-fb4e-4238-89c3-6d1d13476500
📒 Files selected for processing (5)
app/src/main/java/bose/ankush/weatherify/data/repository/WeatherRepositoryImpl.ktnetwork/src/commonMain/kotlin/bose/ankush/network/model/LocationModels.ktnetwork/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepository.ktnetwork/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepositoryImpl.ktstorage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt
| onFailure = { | ||
| // Log error or pass it to UI | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| } catch (e: Exception) { | ||
| throw Exception("Network | Failed to refresh weather data: ${e.message}", e) | ||
| // retryWithExponentialBackoff already normalizes failures to NetworkException. | ||
| Result.failure(e) |
There was a problem hiding this comment.
🧩 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=kotlinRepository: bosankus/Compose-Weatherify
Length of output: 25115
🏁 Script executed:
# Find retryWithExponentialBackoff function definition
rg -n "retryWithExponentialBackoff" --type=kotlin -B2 -A10Repository: 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.ktRepository: 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.ktRepository: 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.
| } 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.
1 participant
Issue
What was changed?
Summary by CodeRabbit
Bug Fixes
Refactor