Conversation
Initial SDK and dependency updates
Improve null handling with backend data
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
app/build.gradle (1)
100-102: Align the Moshi artifacts to one version.
moshiis now1.15.2, but the companion Moshi modules in the same block are still on1.14.0. Square’s current setup examples keepmoshiandmoshi-kotlinon the same version, so I’d bumpmoshi-kotlinandmoshi-adapterstogether here to avoid dependency drift. (github.com)♻️ Proposed fix
- implementation("com.squareup.moshi:moshi-kotlin:1.14.0") - implementation 'com.squareup.moshi:moshi-adapters:1.14.0' + implementation("com.squareup.moshi:moshi-kotlin:1.15.2") + implementation 'com.squareup.moshi:moshi-adapters:1.15.2'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/build.gradle` around lines 100 - 102, The Moshi dependency versions are inconsistent: com.squareup.moshi:moshi is at 1.15.2 while com.squareup.moshi:moshi-kotlin and com.squareup.moshi:moshi-adapters remain at 1.14.0; update the moshi companion artifacts (moshi-kotlin and moshi-adapters) to 1.15.2 so all com.squareup.moshi artifacts use the same version, and keep the existing dependency declaration style consistent with the surrounding lines.app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/EateryDetailViewModel.kt (1)
85-90:WhileSubscribednever really disengages here.
openEatery()starts collectingeateryFlowinviewModelScopeduringinit, so this upstream stays subscribed for the full ViewModel lifetime anyway. If the goal was lifecycle-aware sharing, movestateInto the public state exposed to the UI; otherwise a plainFlowis simpler here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/EateryDetailViewModel.kt` around lines 85 - 90, The eateryFlow is being stateIn'd inside the ViewModel (eateryFlow) while openEatery() also collects it in viewModelScope, so SharingStarted.WhileSubscribed never disengages; either remove stateIn and keep eateryFlow as a plain Flow returned from eateryRepository.getEateryFlow(eateryId) (and let collectors call stateIn/collect as needed), or move the stateIn call out of the private eateryFlow and apply it where you expose state to the UI (e.g., the public StateFlow property) so lifecycle-aware sharing actually happens; update references to eateryFlow and openEatery() accordingly.app/src/main/java/com/cornellappdev/android/eatery/ui/screens/SearchScreen.kt (1)
254-257: Key the recent-search collectors byeateryId.These
collectAsStateWithLifecycle()calls live inside a plainforEach, so whenrecentSearchesreorders, Compose keeps them position-based. Wrapping each block inkey(eateryId)or moving this toitems(..., key = { it })keeps the subscription/state attached to the right eatery. Worth sanity-checking by adding a new recent search while this list is visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/android/eatery/ui/screens/SearchScreen.kt` around lines 254 - 257, The recentSearches.forEach loop creates Compose state via searchViewModel.observeEatery(...).collectAsStateWithLifecycle() without stable keys, so when recentSearches reorders subscriptions stay tied to positions; wrap each eater block in key(eateryId) (or move the list to a LazyColumn/LazyRow and use items(recentSearches, key = { it })) so the collectAsStateWithLifecycle() call remains attached to the correct eateryId (update the block that contains recentSearches.forEach and the call to collectAsStateWithLifecycle()).app/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.kt (1)
210-213: Clear the persisted GET PIN on logout too.
GETAccountRepository.linkGETAccount()stores both the session ID and PIN, but logout only clears the session ID and login flag. Leaving the PIN in preferences after logout keeps reusable auth material on disk longer than necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.kt` around lines 210 - 213, logout() currently clears only the session ID and login flag while GETAccountRepository.linkGETAccount() persists a GET PIN; update logout() to also remove the stored PIN by calling the GETAccountRepository method that clears the PIN (e.g., getAccountRepository.clearGETPin()) — if such a method doesn’t exist, add a clearGETPin/clearPin method to GETAccountRepository that removes the PIN from preferences and invoke it from logout() alongside getAccountRepository.clearSessionId() and getAccountRepository.setIsLoggedIn(false).app/src/main/java/com/cornellappdev/android/eatery/ui/components/login/AccountPage.kt (1)
433-442: TODO: Deposit formatting not yet implemented.The TODO comment indicates that deposit transactions (positive amounts) should display with a "+" prefix and green color, but currently all non-zero amounts are formatted as negative with red. This affects user perception of account credits.
Would you like me to help implement the deposit formatting logic, or should I open an issue to track this task?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/android/eatery/ui/components/login/AccountPage.kt` around lines 433 - 442, The current formatting in the block that computes (amtString, amtColor) incorrectly treats all non-zero monetary amounts as negatives; update the logic in the expression using isMealSwipes, amount, and epsilonEqual to distinguish positive (deposits) from negative (charges): keep the existing meal-swipe branch, keep zero mapping to "$0.00" to Black, but for amount > 0 (use a robust check like !amount.epsilonEqual(0.0) && amount > 0) set amtString to "+$%.2f".format(abs(amount)) and amtColor to Green; for amount < 0 keep "-$%.2f".format(abs(amount)) to Red. Ensure you reference the same constants Red, Green, Black and the same tuple (amtString, amtColor) assignment so other code reads unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/cornellappdev/android/eatery/data/models/User.kt`:
- Around line 58-59: The model is turning missing financial fields into real
values; change User.kt so `@Json`(name = "laundry_balance") val laundryBalance:
Double? defaults to null (not 0.0) and `@Json`(name = "transactions") val
transactions: List<Transaction>? defaults to null (not emptyList()), and then
update any consumers of User.laundryBalance and User.transactions to handle the
nullable states (e.g., show loading/error state or explicit "unknown" UI) so
absent backend data isn't misrepresented as $0.00 or no transactions.
In
`@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/GETAccountRepository.kt`:
- Around line 20-24: The generated PIN can be 1–3 digits because
Random.nextInt(10000) yields 0..9999; change PIN generation in
GETAccountRepository (where Random.nextInt and userPreferencesRepository.setPin
and LoginRequest(pin.toString(), sessionId) are used) to guarantee four digits
by generating in the 1000–9999 range (e.g., Random.nextInt(9000) + 1000) so
setPin and the LoginRequest receive a valid 4-digit value; alternatively, if
leading-zero PINs are allowed, persist and send the PIN as a zero-padded String
(format to 4 digits) instead of an Int.
In
`@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/RepositoryRequestUtils.kt`:
- Around line 9-40: Both safeNetworkRequest and tryRequestWithTokenRefresh
currently catch Exception (in their catch blocks) which swallows
CancellationException; update both functions (safeNetworkRequest and
tryRequestWithTokenRefresh) to rethrow CancellationException immediately (e.g.,
if (exception is CancellationException) throw exception) before calling
mapExceptionToNetworkError or returning Result.Error so structured concurrency
is preserved and cancelled coroutines aren't treated as network errors during
token refresh or retries.
In
`@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.kt`:
- Around line 164-173: The current catch around networkApi.getFinancials blindly
retries with the same session and ignores getAccountRepository.refreshLogin()
result; change it to only handle the expired-session case (detect the specific
exception/response from networkApi.getFinancials rather than catch Exception),
call getAccountRepository.refreshLogin(pin) and check its Result<Unit> (if
Result.Error then rethrow/return immediately and do NOT retry getFinancials),
and otherwise call networkApi.getFinancials again with the new
SessionID(getAccountRepository.getSessionId()); also remove the broad catch and
rely on the centralized retry behavior in RepositoryRequestUtils.kt for
non-session errors.
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt`:
- Around line 74-75: When starting a new login (where isLoginState is true) you
need to reset the backing flows so previous session filters don't persist:
update the LoginViewModel logic that builds ProfileUiState (the branch that sets
accountFilter and filterText) to also set _queryFlow.value = "" and
_accountTypeFilterFlow.value = <default TransactionAccountType> (e.g.,
TransactionAccountType.ALL or whatever the default is) whenever isLoginState is
true; apply the same change to the second occurrence around the 142-145 block so
both places clear _queryFlow and _accountTypeFilterFlow when a new login starts.
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/SearchViewModel.kt`:
- Line 42: eateryFlowCache is a plain mutableMap causing a race when
observeEatery is called concurrently; replace mutableMapOf with a
java.util.concurrent.ConcurrentHashMap and change observeEatery to use
computeIfAbsent to atomically insert the StateFlow produced by
eateryRepository.getEateryFlow(...).stateIn(...). Do the same for any other
similar caches in this file (e.g., the eateries flow cache used around the other
observe method) so all cache insertions use computeIfAbsent on a
ConcurrentHashMap to avoid duplicate flows.
---
Nitpick comments:
In `@app/build.gradle`:
- Around line 100-102: The Moshi dependency versions are inconsistent:
com.squareup.moshi:moshi is at 1.15.2 while com.squareup.moshi:moshi-kotlin and
com.squareup.moshi:moshi-adapters remain at 1.14.0; update the moshi companion
artifacts (moshi-kotlin and moshi-adapters) to 1.15.2 so all com.squareup.moshi
artifacts use the same version, and keep the existing dependency declaration
style consistent with the surrounding lines.
In
`@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.kt`:
- Around line 210-213: logout() currently clears only the session ID and login
flag while GETAccountRepository.linkGETAccount() persists a GET PIN; update
logout() to also remove the stored PIN by calling the GETAccountRepository
method that clears the PIN (e.g., getAccountRepository.clearGETPin()) — if such
a method doesn’t exist, add a clearGETPin/clearPin method to
GETAccountRepository that removes the PIN from preferences and invoke it from
logout() alongside getAccountRepository.clearSessionId() and
getAccountRepository.setIsLoggedIn(false).
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/components/login/AccountPage.kt`:
- Around line 433-442: The current formatting in the block that computes
(amtString, amtColor) incorrectly treats all non-zero monetary amounts as
negatives; update the logic in the expression using isMealSwipes, amount, and
epsilonEqual to distinguish positive (deposits) from negative (charges): keep
the existing meal-swipe branch, keep zero mapping to "$0.00" to Black, but for
amount > 0 (use a robust check like !amount.epsilonEqual(0.0) && amount > 0) set
amtString to "+$%.2f".format(abs(amount)) and amtColor to Green; for amount < 0
keep "-$%.2f".format(abs(amount)) to Red. Ensure you reference the same
constants Red, Green, Black and the same tuple (amtString, amtColor) assignment
so other code reads unchanged.
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/screens/SearchScreen.kt`:
- Around line 254-257: The recentSearches.forEach loop creates Compose state via
searchViewModel.observeEatery(...).collectAsStateWithLifecycle() without stable
keys, so when recentSearches reorders subscriptions stay tied to positions; wrap
each eater block in key(eateryId) (or move the list to a LazyColumn/LazyRow and
use items(recentSearches, key = { it })) so the collectAsStateWithLifecycle()
call remains attached to the correct eateryId (update the block that contains
recentSearches.forEach and the call to collectAsStateWithLifecycle()).
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/EateryDetailViewModel.kt`:
- Around line 85-90: The eateryFlow is being stateIn'd inside the ViewModel
(eateryFlow) while openEatery() also collects it in viewModelScope, so
SharingStarted.WhileSubscribed never disengages; either remove stateIn and keep
eateryFlow as a plain Flow returned from
eateryRepository.getEateryFlow(eateryId) (and let collectors call
stateIn/collect as needed), or move the stateIn call out of the private
eateryFlow and apply it where you expose state to the UI (e.g., the public
StateFlow property) so lifecycle-aware sharing actually happens; update
references to eateryFlow and openEatery() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb159ade-8104-43cd-ab73-78090136280d
📒 Files selected for processing (24)
app/build.gradleapp/src/main/java/com/cornellappdev/android/eatery/data/MoshiAdapters.ktapp/src/main/java/com/cornellappdev/android/eatery/data/models/User.ktapp/src/main/java/com/cornellappdev/android/eatery/data/repositories/AuthTokenRepository.ktapp/src/main/java/com/cornellappdev/android/eatery/data/repositories/GETAccountRepository.ktapp/src/main/java/com/cornellappdev/android/eatery/data/repositories/RepositoryRequestUtils.ktapp/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.ktapp/src/main/java/com/cornellappdev/android/eatery/di/NetworkingModule.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/components/login/AccountPage.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/components/login/LoginToast.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/screens/FavoritesScreen.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/screens/HomeScreen.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/screens/NearestScreen.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/screens/ProfileScreen.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/screens/SearchScreen.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/EateryDetailViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/FavoritesViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/HomeViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/NearestViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/SearchViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/state/DisplayTransaction.ktbuild.gradlegradle/wrapper/gradle-wrapper.properties
💤 Files with no reviewable changes (3)
- app/src/main/java/com/cornellappdev/android/eatery/di/NetworkingModule.kt
- app/src/main/java/com/cornellappdev/android/eatery/ui/components/login/LoginToast.kt
- app/src/main/java/com/cornellappdev/android/eatery/data/MoshiAdapters.kt
Comment on lines
+58
to
+59
| @Json(name = "laundry_balance") val laundryBalance: Double? = 0.0, | ||
| @Json(name = "transactions") val transactions: List<Transaction> = emptyList(), |
There was a problem hiding this comment.
Don’t turn missing financial data into real values.
Defaulting laundryBalance to 0.0 and transactions to emptyList() makes an absent payload look like a legitimate $0.00 balance or “no transactions” state. That masks backend/schema regressions and can surface incorrect account data in the UI.
🛠️ Proposed fix
- `@Json`(name = "laundry_balance") val laundryBalance: Double? = 0.0,
- `@Json`(name = "transactions") val transactions: List<Transaction> = emptyList(),
+ `@Json`(name = "laundry_balance") val laundryBalance: Double? = null,
+ `@Json`(name = "transactions") val transactions: List<Transaction>? = null,📝 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
| @Json(name = "laundry_balance") val laundryBalance: Double? = 0.0, | |
| @Json(name = "transactions") val transactions: List<Transaction> = emptyList(), | |
| `@Json`(name = "laundry_balance") val laundryBalance: Double? = null, | |
| `@Json`(name = "transactions") val transactions: List<Transaction>? = null, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/cornellappdev/android/eatery/data/models/User.kt`
around lines 58 - 59, The model is turning missing financial fields into real
values; change User.kt so `@Json`(name = "laundry_balance") val laundryBalance:
Double? defaults to null (not 0.0) and `@Json`(name = "transactions") val
transactions: List<Transaction>? defaults to null (not emptyList()), and then
update any consumers of User.laundryBalance and User.transactions to handle the
nullable states (e.g., show loading/error state or explicit "unknown" UI) so
absent backend data isn't misrepresented as $0.00 or no transactions.
Comment on lines
+20
to
+24
| val pin = Random.nextInt(10000) | ||
| userPreferencesRepository.setPin(pin) | ||
| return tryRequestWithResult { | ||
| networkApi.authorizeUser( | ||
| loginRequest = LoginRequest(pin.toString(), sessionId) |
There was a problem hiding this comment.
Guarantee a four-digit GET PIN.
Random.nextInt(10000) can generate 0..999, so pin.toString() here stores/sends 1-3 digit values. If this contract is meant to be a fixed 4-digit PIN, some generated credentials will be invalid; if leading-zero PINs are valid, the PIN needs to be persisted as a zero-padded String instead of an Int.
💡 Minimal fix if leading-zero PINs are not required
- val pin = Random.nextInt(10000)
+ val pin = Random.nextInt(1000, 10000)📝 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
| val pin = Random.nextInt(10000) | |
| userPreferencesRepository.setPin(pin) | |
| return tryRequestWithResult { | |
| networkApi.authorizeUser( | |
| loginRequest = LoginRequest(pin.toString(), sessionId) | |
| val pin = Random.nextInt(1000, 10000) | |
| userPreferencesRepository.setPin(pin) | |
| return tryRequestWithResult { | |
| networkApi.authorizeUser( | |
| loginRequest = LoginRequest(pin.toString(), sessionId) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/GETAccountRepository.kt`
around lines 20 - 24, The generated PIN can be 1–3 digits because
Random.nextInt(10000) yields 0..9999; change PIN generation in
GETAccountRepository (where Random.nextInt and userPreferencesRepository.setPin
and LoginRequest(pin.toString(), sessionId) are used) to guarantee four digits
by generating in the 1000–9999 range (e.g., Random.nextInt(9000) + 1000) so
setPin and the LoginRequest receive a valid 4-digit value; alternatively, if
leading-zero PINs are allowed, persist and send the PIN as a zero-padded String
(format to 4 digits) instead of an Int.
Comment on lines
+9
to
+40
| internal suspend fun <T> safeNetworkRequest(request: suspend () -> T): Result<T> { | ||
| return try { | ||
| Result.Success(request()) | ||
| } catch (exception: Exception) { | ||
| Result.Error(mapExceptionToNetworkError(exception)) | ||
| } | ||
| } | ||
|
|
||
| internal suspend fun <T> tryRequestWithTokenRefresh( | ||
| request: suspend () -> T, | ||
| refreshTokens: suspend () -> Result<Unit> | ||
| ): Result<T> { | ||
| return try { | ||
| Result.Success(request()) | ||
| } catch (initialException: Exception) { | ||
| // Only attempt refresh for auth-related errors | ||
| val initialError = mapExceptionToNetworkError(initialException) | ||
| if (initialError !is NetworkError.Unauthorized) { | ||
| return Result.Error(initialError) | ||
| } | ||
| when (val refreshResult = refreshTokens()) { | ||
| is Result.Success -> { | ||
| try { | ||
| Result.Success(request()) | ||
| } catch (retryException: Exception) { | ||
| Result.Error(mapExceptionToNetworkError(retryException)) | ||
| } | ||
| } | ||
|
|
||
| is Result.Error -> Result.Error(refreshResult.error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the file
find . -name "RepositoryRequestUtils.kt" -type fRepository: cuappdev/eatery-blue-android
Length of output: 168
🏁 Script executed:
# Also check Kotlin's coroutine exception hierarchy basics by examining imports and usage in the repo
fd -e kt -e kts | head -20 | xargs grep -l "CancellationException" 2>/dev/null | head -5Repository: cuappdev/eatery-blue-android
Length of output: 54
🏁 Script executed:
# Search for usage of safeNetworkRequest and tryRequestWithTokenRefresh
rg "safeNetworkRequest|tryRequestWithTokenRefresh" --type kt -A 2 -B 2Repository: cuappdev/eatery-blue-android
Length of output: 98
🏁 Script executed:
cat -n ./app/src/main/java/com/cornellappdev/android/eatery/data/repositories/RepositoryRequestUtils.ktRepository: cuappdev/eatery-blue-android
Length of output: 2341
🏁 Script executed:
# Search for usage of these functions without file type restrictions
rg "safeNetworkRequest|tryRequestWithTokenRefresh" -A 2 -B 2Repository: cuappdev/eatery-blue-android
Length of output: 4002
🏁 Script executed:
# Check if CancellationException is imported or used anywhere in the project
rg "CancellationException" -A 2 -B 2Repository: cuappdev/eatery-blue-android
Length of output: 54
🌐 Web query:
Kotlin CancellationException catch Exception structured concurrency best practices 2025
💡 Result:
Best practices (structured concurrency + CancellationException + catch (Exception))
- Don’t “consume”
CancellationException
- In Kotlin coroutines, cancellation is signaled by throwing
CancellationException. If you catch it and don’t rethrow, you can break cooperative cancellation and keep work running after the parent scope is cancelled. Android’s coroutine best practices explicitly warn against consumingCancellationException. [1] - JetBrains’ inspection guidance likewise says it must not be swallowed/handled in coroutine context (you may do extra actions, but rethrow). [2]
- If you must catch broad exceptions, rethrow cancellation immediately
try {
doWork()
} catch (e: CancellationException) {
throw e // always rethrow
} catch (e: Exception) {
// handle real failure
}This “always rethrow CancellationException” pattern is shown in kotlinx.coroutines docs (e.g., produce). [3]
- Avoid
runCatching { ... }aroundsuspendwork (or make it cancellation-aware)
runCatchingcatches broadly (including coroutine control-flow exceptions), which can accidentally trapCancellationExceptionand prevent cancellation from propagating. This is a commonly flagged pitfall. [4]
- Prefer structured concurrency scopes and the right supervision model
- Use
coroutineScope {}to tie child lifetimes to the parent (default: a child failure cancels the parent/siblings). - Use
supervisorScope {}/SupervisorJob()when you intentionally want child failures not to cancel siblings, and handle failures per-child. Kotlin’s exception-handling docs explain how supervision changes cancellation propagation in a coroutine hierarchy. [5]
- Do cleanup in
finally(and only useNonCancellablefor truly required cleanup)
- Let cancellation propagate, and put resource cleanup in
finally. If cleanup must complete even when cancelled, wrap just the cleanup inwithContext(NonCancellable) { ... }(don’t “mask” cancellation for the main work). (This aligns with the general guidance to not block cancellation while still ensuring cleanup.) [1][2]
Sources: Android Developers coroutines best practices [1], JetBrains Inspectopedia cancellation handling inspection [2], kotlinx.coroutines produce docs [3], runCatching/cancellation pitfall write-up [4], Kotlin coroutine exception-handling docs (supervision/structured concurrency) [5].
Rethrow coroutine cancellation in both helpers.
catch (Exception) here also catches CancellationException. Turning cancellation into Result.Error breaks structured concurrency: cancelled work can keep running, retry token refresh, and surface a bogus network error after the caller's scope is already cancelled. Please rethrow cancellation before mapping exceptions.
🛠️ Proposed fix
import com.cornellappdev.android.eatery.data.models.NetworkError
import com.cornellappdev.android.eatery.data.models.Result
+import kotlinx.coroutines.CancellationException
import retrofit2.HttpException
import java.io.IOException
import java.net.SocketTimeoutException
internal suspend fun <T> safeNetworkRequest(request: suspend () -> T): Result<T> {
return try {
Result.Success(request())
+ } catch (exception: CancellationException) {
+ throw exception
} catch (exception: Exception) {
Result.Error(mapExceptionToNetworkError(exception))
}
}
@@
): Result<T> {
return try {
Result.Success(request())
+ } catch (initialException: CancellationException) {
+ throw initialException
} catch (initialException: Exception) {
// Only attempt refresh for auth-related errors
val initialError = mapExceptionToNetworkError(initialException)
if (initialError !is NetworkError.Unauthorized) {
return Result.Error(initialError)
@@
is Result.Success -> {
try {
Result.Success(request())
+ } catch (retryException: CancellationException) {
+ throw retryException
} catch (retryException: Exception) {
Result.Error(mapExceptionToNetworkError(retryException))
}
}📝 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
| internal suspend fun <T> safeNetworkRequest(request: suspend () -> T): Result<T> { | |
| return try { | |
| Result.Success(request()) | |
| } catch (exception: Exception) { | |
| Result.Error(mapExceptionToNetworkError(exception)) | |
| } | |
| } | |
| internal suspend fun <T> tryRequestWithTokenRefresh( | |
| request: suspend () -> T, | |
| refreshTokens: suspend () -> Result<Unit> | |
| ): Result<T> { | |
| return try { | |
| Result.Success(request()) | |
| } catch (initialException: Exception) { | |
| // Only attempt refresh for auth-related errors | |
| val initialError = mapExceptionToNetworkError(initialException) | |
| if (initialError !is NetworkError.Unauthorized) { | |
| return Result.Error(initialError) | |
| } | |
| when (val refreshResult = refreshTokens()) { | |
| is Result.Success -> { | |
| try { | |
| Result.Success(request()) | |
| } catch (retryException: Exception) { | |
| Result.Error(mapExceptionToNetworkError(retryException)) | |
| } | |
| } | |
| is Result.Error -> Result.Error(refreshResult.error) | |
| } | |
| } | |
| internal suspend fun <T> safeNetworkRequest(request: suspend () -> T): Result<T> { | |
| return try { | |
| Result.Success(request()) | |
| } catch (exception: CancellationException) { | |
| throw exception | |
| } catch (exception: Exception) { | |
| Result.Error(mapExceptionToNetworkError(exception)) | |
| } | |
| } | |
| internal suspend fun <T> tryRequestWithTokenRefresh( | |
| request: suspend () -> T, | |
| refreshTokens: suspend () -> Result<Unit> | |
| ): Result<T> { | |
| return try { | |
| Result.Success(request()) | |
| } catch (initialException: CancellationException) { | |
| throw initialException | |
| } catch (initialException: Exception) { | |
| // Only attempt refresh for auth-related errors | |
| val initialError = mapExceptionToNetworkError(initialException) | |
| if (initialError !is NetworkError.Unauthorized) { | |
| return Result.Error(initialError) | |
| } | |
| when (val refreshResult = refreshTokens()) { | |
| is Result.Success -> { | |
| try { | |
| Result.Success(request()) | |
| } catch (retryException: CancellationException) { | |
| throw retryException | |
| } catch (retryException: Exception) { | |
| Result.Error(mapExceptionToNetworkError(retryException)) | |
| } | |
| } | |
| is Result.Error -> Result.Error(refreshResult.error) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/RepositoryRequestUtils.kt`
around lines 9 - 40, Both safeNetworkRequest and tryRequestWithTokenRefresh
currently catch Exception (in their catch blocks) which swallows
CancellationException; update both functions (safeNetworkRequest and
tryRequestWithTokenRefresh) to rethrow CancellationException immediately (e.g.,
if (exception is CancellationException) throw exception) before calling
mapExceptionToNetworkError or returning Result.Error so structured concurrency
is preserved and cancelled coroutines aren't treated as network errors during
token refresh or retries.
Comment on lines
164
to
173
| try { | ||
| financials = networkApi.getFinancials( | ||
| sessionId = SessionID(authTokenRepository.getSessionId()) | ||
| sessionId = SessionID(getAccountRepository.getSessionId()) | ||
| ) | ||
| } catch (_: Exception) { | ||
| val pin = authTokenRepository.getPin() | ||
| authTokenRepository.refreshLogin(pin = pin) | ||
| val pin = getAccountRepository.getPin() ?: throw IllegalStateException() | ||
| getAccountRepository.refreshLogin(pin = pin) | ||
| financials = networkApi.getFinancials( | ||
| sessionId = SessionID(authTokenRepository.getSessionId()) | ||
| sessionId = SessionID(getAccountRepository.getSessionId()) | ||
| ) |
There was a problem hiding this comment.
Don't retry /financials after a failed refreshLogin().
refreshLogin() returns Result<Unit>, but its result is ignored here, so a refresh error still falls through to the second getFinancials() call with the stale session ID. The broad catch (_: Exception) also swallows non-session failures and bypasses the narrower retry policy already centralized in RepositoryRequestUtils.kt; this path should only run for the specific expired-session case, and it should stop immediately on Result.Error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.kt`
around lines 164 - 173, The current catch around networkApi.getFinancials
blindly retries with the same session and ignores
getAccountRepository.refreshLogin() result; change it to only handle the
expired-session case (detect the specific exception/response from
networkApi.getFinancials rather than catch Exception), call
getAccountRepository.refreshLogin(pin) and check its Result<Unit> (if
Result.Error then rethrow/return immediately and do NOT retry getFinancials),
and otherwise call networkApi.getFinancials again with the new
SessionID(getAccountRepository.getSessionId()); also remove the broad catch and
rely on the centralized retry behavior in RepositoryRequestUtils.kt for
non-session errors.
Comment on lines
+74
to
+75
| accountFilter = if (isLoginState) TransactionAccountType.BRBS else accountFilter, | ||
| filterText = if (isLoginState) "" else query, |
There was a problem hiding this comment.
Reset the previous transaction filters when a new login starts.
Lines 74-75 only mask filterText and accountFilter in ProfileUiState; the backing _queryFlow and _accountTypeFilterFlow still keep the last session's values. If a user logs out and logs back in, the new account page can come up pre-filtered and look empty.
💡 Minimal fix
fun onLoginPressed() {
+ _queryFlow.value = ""
+ _accountTypeFilterFlow.value = TransactionAccountType.BRBS
clearError()
_isLoginLoadingFlow.value = true
}Also applies to: 142-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt`
around lines 74 - 75, When starting a new login (where isLoginState is true) you
need to reset the backing flows so previous session filters don't persist:
update the LoginViewModel logic that builds ProfileUiState (the branch that sets
accountFilter and filterText) to also set _queryFlow.value = "" and
_accountTypeFilterFlow.value = <default TransactionAccountType> (e.g.,
TransactionAccountType.ALL or whatever the default is) whenever isLoginState is
true; apply the same change to the second occurrence around the 142-145 block so
both places clear _queryFlow and _accountTypeFilterFlow when a new login starts.
| val error: NetworkUiError? = null | ||
| ) | ||
|
|
||
| private val eateryFlowCache = mutableMapOf<Int, StateFlow<EateryApiResponse<Eatery>>>() |
There was a problem hiding this comment.
Potential race condition on eateryFlowCache access.
mutableMapOf is not thread-safe. If observeEatery is called concurrently from different coroutines (e.g., during rapid UI composition), getOrPut may create duplicate flows for the same eateryId.
Consider using a ConcurrentHashMap or synchronizing access:
🛡️ Proposed fix using ConcurrentHashMap
- private val eateryFlowCache = mutableMapOf<Int, StateFlow<EateryApiResponse<Eatery>>>()
+ private val eateryFlowCache = java.util.concurrent.ConcurrentHashMap<Int, StateFlow<EateryApiResponse<Eatery>>>()Note: ConcurrentHashMap.getOrPut extension in Kotlin is still not atomic for the compute, so for strict atomicity you could use computeIfAbsent:
fun observeEatery(eateryId: Int): StateFlow<EateryApiResponse<Eatery>> =
eateryFlowCache.computeIfAbsent(eateryId) {
eateryRepository.getEateryFlow(eateryId).stateIn(
scope = viewModelScope,
started = SharingStarted.WhileSubscribed(5_000),
initialValue = EateryApiResponse.Pending
)
}Also applies to: 206-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/SearchViewModel.kt`
at line 42, eateryFlowCache is a plain mutableMap causing a race when
observeEatery is called concurrently; replace mutableMapOf with a
java.util.concurrent.ConcurrentHashMap and change observeEatery to use
computeIfAbsent to atomically insert the StateFlow produced by
eateryRepository.getEateryFlow(...).stateIn(...). Do the same for any other
similar caches in this file (e.g., the eateries flow cache used around the other
observe method) so all cache insertions use computeIfAbsent on a
ConcurrentHashMap to avoid duplicate flows.
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
2 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.
Overview
This PR releases the transaction error-handling fix, the SDK update, and minor Gradle updates to prod.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores