Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved TransactionType and its Moshi adapter; introduced DisplayTransaction UI model; reshaped transaction models and nullability; added GetAccountRepository and repository network helpers; refactored token/session flows and updated viewmodels/composables to use DisplayTransaction. Changes
Sequence DiagramsequenceDiagram
participant UI as UI Layer
participant VM as ViewModel
participant Repo as UserRepository
participant GET as GetAccountRepository
participant Auth as AuthTokenRepository
participant Net as NetworkApi
UI->>VM: subscribe uiState / request transactions
VM->>Repo: fetch user & financials
Repo->>GET: getSessionId / isLoggedIn (prefs)
Repo->>Auth: tryRequestWithTokenRefresh(..., refreshTokens)
Auth->>Net: refreshTokens() (if needed)
Net-->>Auth: tokens
Auth-->>Repo: refresh result
Repo->>Net: fetch financials (network call)
Net-->>Repo: financials payload (nullable)
Repo->>Repo: filter, parse dates (String.toLocalDateTime), map to Transaction -> DisplayTransaction
Repo-->>VM: validated DisplayTransaction list
VM-->>UI: emit uiState with DisplayTransaction list
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/components/login/AccountPage.kt`:
- Around line 435-443: The current fallback always formats non-zero amounts as
negative red; change the logic in the amtString/amtColor assignment to use the
numeric sign of amount (abs only for magnitude) so positives render as "+$%.2f"
with Green and negatives as "-$%.2f" with Red, while preserving the meal-swipes
branch (isMealSwipes) and the zero case (amount.epsilonEqual(0.0)); also
reintroduce the Green color import if you use the sign-based fallback.
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt`:
- Around line 136-142: Replace the composite key currently passed to
DisplayTransaction.id with the backend transaction's tenderId: when building
DisplayTransaction (the block that constructs id from date, location, amount,
mappedAccountType), use transaction.tenderId as the primary id; if tenderId is
null/empty, produce a deterministic unique fallback by using mapIndexedNotNull
(or mapIndexed) to incorporate the source index into the id so each item in the
snapshot is unique and prevents LazyColumn key collisions. Ensure references to
DisplayTransaction and the Transaction model (tenderId) are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab742e90-7778-4d43-b41c-dba6a1363c07
📒 Files selected for processing (8)
app/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/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/screens/ProfileScreen.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/state/DisplayTransaction.kt
💤 Files with no reviewable changes (2)
- app/src/main/java/com/cornellappdev/android/eatery/di/NetworkingModule.kt
- app/src/main/java/com/cornellappdev/android/eatery/data/MoshiAdapters.kt
app/src/main/java/com/cornellappdev/android/eatery/ui/components/login/AccountPage.kt
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
AndrewCheung360
approved these changes
Mar 19, 2026
Member
AndrewCheung360
left a comment
AndrewCheung360
left a comment
There was a problem hiding this comment.
Left some minor comments to look over, but as long as it's been tested, I'll just give approval preemptively
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt (2)
116-120: Consider precomputing repeated values.
query.lowercase()andLocalDateTime.now().minusDays(30)are evaluated for each transaction. For larger lists, extracting these to local variables avoids redundant allocations and computation.♻️ Optional optimization
) { loadedUser, query, accountFilter -> if (loadedUser == null) return@combine emptyList() + val lowerQuery = query.lowercase() + val cutoff = LocalDateTime.now().minusDays(30) loadedUser.transactions.filter { - it.location.lowercase().contains(query.lowercase()) - && it.accountType == accountFilter - && it.date >= LocalDateTime.now().minusDays(30) + it.location.lowercase().contains(lowerQuery) + && it.accountType == accountFilter + && it.date >= cutoff }.map { it.toDisplayTransaction() } }🤖 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 116 - 120, The filtering currently calls query.lowercase() and LocalDateTime.now().minusDays(30) for every transaction inside loadedUser.transactions.filter; refactor the block to compute val lowerQuery = query.lowercase() (or use locale if needed) and val cutoff = LocalDateTime.now().minusDays(30) once before calling loadedUser.transactions.filter, then use lowerQuery and cutoff inside the predicate (preserve accountFilter and the toDisplayTransaction() mapping).
124-127: Consider locale-aware date formatting for internationalization.The hardcoded pattern
"h:mm a · EEEE, MMMM d"assumes US locale conventions. If the app supports multiple locales, consider usingDateTimeFormatter.ofLocalizedDateTime()or passing aLocaleparameter.🤖 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 124 - 127, The LocalDateTime.formatDate() helper currently uses a hardcoded pattern ("h:mm a · EEEE, MMMM d") which is not locale-aware; update formatDate() to use a locale-sensitive formatter (e.g., DateTimeFormatter.ofLocalizedDateTime(...) and call localizedBy(Locale.getDefault()) or accept a Locale parameter) instead of DateTimeFormatter.ofPattern, so displayed dates follow the device/app locale; modify the LocalDateTime.formatDate() implementation accordingly (or add an overload that takes a Locale) to ensure internationalized formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt`:
- Around line 116-120: The filtering currently calls query.lowercase() and
LocalDateTime.now().minusDays(30) for every transaction inside
loadedUser.transactions.filter; refactor the block to compute val lowerQuery =
query.lowercase() (or use locale if needed) and val cutoff =
LocalDateTime.now().minusDays(30) once before calling
loadedUser.transactions.filter, then use lowerQuery and cutoff inside the
predicate (preserve accountFilter and the toDisplayTransaction() mapping).
- Around line 124-127: The LocalDateTime.formatDate() helper currently uses a
hardcoded pattern ("h:mm a · EEEE, MMMM d") which is not locale-aware; update
formatDate() to use a locale-sensitive formatter (e.g.,
DateTimeFormatter.ofLocalizedDateTime(...) and call
localizedBy(Locale.getDefault()) or accept a Locale parameter) instead of
DateTimeFormatter.ofPattern, so displayed dates follow the device/app locale;
modify the LocalDateTime.formatDate() implementation accordingly (or add an
overload that takes a Locale) to ensure internationalized formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6ed460f-7613-47b5-9be0-faca3087fc8e
📒 Files selected for processing (5)
app/src/main/java/com/cornellappdev/android/eatery/data/models/User.ktapp/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/components/login/AccountPage.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/state/DisplayTransaction.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/state/DisplayTransaction.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.kt
- app/src/main/java/com/cornellappdev/android/eatery/ui/components/login/AccountPage.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/cornellappdev/android/eatery/data/repositories/GetAccountRepository.kt (1)
44-46: Silent default values may mask missing session state.Returning
""for missing session ID and0for missing PIN makes it impossible for callers to distinguish between "not set" and "set to empty/zero." Consider returning nullable types or throwing if these are required preconditions.💡 Alternative approach
- suspend fun getSessionId(): String = userPreferencesRepository.sessionIdFlow.firstOrNull() ?: "" + suspend fun getSessionId(): String? = userPreferencesRepository.sessionIdFlow.firstOrNull() - suspend fun getPin(): Int = userPreferencesRepository.pinFlow.firstOrNull() ?: 0 + suspend fun getPin(): Int? = userPreferencesRepository.pinFlow.firstOrNull()Or throw if these are preconditions:
suspend fun getSessionId(): String = userPreferencesRepository.sessionIdFlow.firstOrNull() ?: throw IllegalStateException("Session ID not available")🤖 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 44 - 46, The current getSessionId() and getPin() functions return silent defaults ("" and 0) which mask missing state; update getSessionId and getPin in GetAccountRepository to surface absence by either changing their return types to nullable (String? and Int?) and returning userPreferencesRepository.sessionIdFlow.firstOrNull() / pinFlow.firstOrNull(), or keep non-nullable and throw an explicit exception (e.g., IllegalStateException) when firstOrNull() is null; update all callers of getSessionId and getPin to handle the nullable return or catch/propagate the thrown exception accordingly so missing session/PIN is no longer indistinguishable from empty/zero.
🤖 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/repositories/RepositoryRequestUtils.kt`:
- Around line 17-36: Capture the original exception thrown by request() and map
it to a network error first (e.g., call mapExceptionToNetworkError on the caught
exception) and only attempt refreshTokens() if that mapped error indicates an
authentication problem; if it is not an auth error, return Result.Error<T> with
the mapped original error immediately. If you do call refreshTokens() and it
returns Result.Error<Unit>, convert/propagate that error as a Result.Error<T>
(do not return the Result<Unit> directly) so the function consistently returns
Result<T>. Keep the existing retry block that maps retryException via
mapExceptionToNetworkError and returns Result.Error<T> on retry failure; update
tryRequestWithTokenRefresh, request, refreshTokens, and the places where you
check the mapped error to implement this flow.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/android/eatery/data/repositories/GetAccountRepository.kt`:
- Around line 44-46: The current getSessionId() and getPin() functions return
silent defaults ("" and 0) which mask missing state; update getSessionId and
getPin in GetAccountRepository to surface absence by either changing their
return types to nullable (String? and Int?) and returning
userPreferencesRepository.sessionIdFlow.firstOrNull() / pinFlow.firstOrNull(),
or keep non-nullable and throw an explicit exception (e.g.,
IllegalStateException) when firstOrNull() is null; update all callers of
getSessionId and getPin to handle the nullable return or catch/propagate the
thrown exception accordingly so missing session/PIN is no longer
indistinguishable from empty/zero.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 581e3274-070e-4fc0-a6ed-97f8a57df05a
📒 Files selected for processing (5)
app/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/ui/viewmodels/LoginViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/cornellappdev/android/eatery/data/repositories/UserRepository.kt
app/src/main/java/com/cornellappdev/android/eatery/data/repositories/RepositoryRequestUtils.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/main/java/com/cornellappdev/android/eatery/ui/screens/SearchScreen.kt (1)
259-261: RedundantinitialValueparameter.Since
observeEateryreturns aStateFlowwhich already has an initial value ofEateryApiResponse.Pending, specifyinginitialValueincollectAsStateWithLifecycleis redundant. The overload withoutinitialValuecan be used forStateFlow.♻️ Suggested simplification
val eateryResponse = - searchViewModel.observeEatery(eateryId).collectAsStateWithLifecycle( - initialValue = EateryApiResponse.Pending - ).value + searchViewModel.observeEatery(eateryId).collectAsStateWithLifecycle().value🤖 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 259 - 261, The call to collectAsStateWithLifecycle passes a redundant initialValue because searchViewModel.observeEatery(eateryId) already returns a StateFlow seeded with EateryApiResponse.Pending; remove the initialValue = EateryApiResponse.Pending argument from the collectAsStateWithLifecycle invocation so it uses the StateFlow's own initial value (locate the expression using observeEatery and collectAsStateWithLifecycle in SearchScreen.kt and delete the initialValue parameter).app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/SearchViewModel.kt (1)
33-34: Consider thread-safety and memory growth foreateryFlowCache.The
mutableMapOfis not thread-safe. WhilegetOrPutis typically called from the main thread via composables, concurrent calls could lead to duplicateStateFlowcreations. Additionally, the cache grows indefinitely without eviction.Consider using a
ConcurrentHashMapor synchronizing access. For memory, theWhileSubscribed(5_000)policy helps, but stale entries remain in the map.♻️ Suggested improvement using ConcurrentHashMap
+import java.util.concurrent.ConcurrentHashMap + `@HiltViewModel` class SearchViewModel `@Inject` constructor( private val userPreferencesRepository: UserPreferencesRepository, private val eateryRepository: EateryRepository, private val userRepository: UserRepository ) : ViewModel() { - private val eateryFlowCache = mutableMapOf<Int, StateFlow<EateryApiResponse<Eatery>>>() + private val eateryFlowCache = ConcurrentHashMap<Int, StateFlow<EateryApiResponse<Eatery>>>()🤖 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` around lines 33 - 34, Replace the non-thread-safe mutableMapOf eateryFlowCache with a ConcurrentHashMap<Int, StateFlow<EateryApiResponse<Eatery>>> and switch usages that create flows (currently using getOrPut) to computeIfAbsent so duplicate StateFlow creation is avoided under concurrency; additionally implement eviction: when creating the StateFlow (the one using WhileSubscribed(5_000)), observe its subscriptionCount in a coroutine and remove its entry from the ConcurrentHashMap after subscriptionCount drops to zero for the desired idle period (or use a simple LRU/size-bound policy) so stale entries don't grow unbounded. Ensure references to eateryFlowCache and the flow-creation function (the code path that currently calls getOrPut) are updated accordingly.app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt (1)
106-112: Consider precomputing filter values for slight efficiency gain.
LocalDateTime.now().minusDays(30)andquery.lowercase()are evaluated for each transaction during filtering. For large lists, extracting these before the filter would avoid repeated allocations.♻️ Suggested optimization
) { loadedUser, query, accountFilter -> if (loadedUser == null) return@combine emptyList() + val cutoffDate = LocalDateTime.now().minusDays(30) + val lowerQuery = query.lowercase() loadedUser.transactions.filter { - it.location.lowercase().contains(query.lowercase()) + it.location.lowercase().contains(lowerQuery) && it.accountType == accountFilter - && it.date >= LocalDateTime.now().minusDays(30) + && it.date >= cutoffDate }.map { it.toDisplayTransaction() }🤖 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 106 - 112, The filter repeatedly computes query.lowercase() and LocalDateTime.now().minusDays(30) for every transaction; precompute these once before filtering in the combine lambda inside LoginViewModel (the block that takes loadedUser, query, accountFilter) — e.g., assign val lowerQuery = query.lowercase() and val cutoff = LocalDateTime.now().minusDays(30) and then use lowerQuery and cutoff inside loadedUser.transactions.filter { ... } so the expensive/allocating calls are not executed per-transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/screens/SearchScreen.kt`:
- Around line 259-261: The call to collectAsStateWithLifecycle passes a
redundant initialValue because searchViewModel.observeEatery(eateryId) already
returns a StateFlow seeded with EateryApiResponse.Pending; remove the
initialValue = EateryApiResponse.Pending argument from the
collectAsStateWithLifecycle invocation so it uses the StateFlow's own initial
value (locate the expression using observeEatery and collectAsStateWithLifecycle
in SearchScreen.kt and delete the initialValue parameter).
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt`:
- Around line 106-112: The filter repeatedly computes query.lowercase() and
LocalDateTime.now().minusDays(30) for every transaction; precompute these once
before filtering in the combine lambda inside LoginViewModel (the block that
takes loadedUser, query, accountFilter) — e.g., assign val lowerQuery =
query.lowercase() and val cutoff = LocalDateTime.now().minusDays(30) and then
use lowerQuery and cutoff inside loadedUser.transactions.filter { ... } so the
expensive/allocating calls are not executed per-transaction.
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/SearchViewModel.kt`:
- Around line 33-34: Replace the non-thread-safe mutableMapOf eateryFlowCache
with a ConcurrentHashMap<Int, StateFlow<EateryApiResponse<Eatery>>> and switch
usages that create flows (currently using getOrPut) to computeIfAbsent so
duplicate StateFlow creation is avoided under concurrency; additionally
implement eviction: when creating the StateFlow (the one using
WhileSubscribed(5_000)), observe its subscriptionCount in a coroutine and remove
its entry from the ConcurrentHashMap after subscriptionCount drops to zero for
the desired idle period (or use a simple LRU/size-bound policy) so stale entries
don't grow unbounded. Ensure references to eateryFlowCache and the flow-creation
function (the code path that currently calls getOrPut) are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d590c00-5ba6-4e18-81f7-0e26bb3a971b
📒 Files selected for processing (4)
app/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/LoginViewModel.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/SearchViewModel.kt
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/src/main/java/com/cornellappdev/android/eatery/ui/screens/NearestScreen.kt (2)
104-108:⚠️ Potential issue | 🟡 MinorPotential NPE with force-unwrapped
eatery.id.Same issue as in FavoritesScreen - using
eatery.id!!as a key can crash if an eatery has a null ID.🛡️ Suggested safer key
items( items = nearestEateries, key = { eatery -> - eatery.id!! + eatery.id ?: eatery.hashCode() }) { eatery ->🤖 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/NearestScreen.kt` around lines 104 - 108, The key provider for the LazyColumn items uses a force-unwrapped ID (items(... key = { eatery -> eatery.id!! })) which can throw an NPE; update the key logic in NearestScreen to use a null-safe fallback (e.g., key = { eatery -> eatery.id ?: eatery.hashCode() } or combine a stable field like name/index) so it never force-unwraps, mirroring the safer approach used in FavoritesScreen.
88-89:⚠️ Potential issue | 🟡 MinorIncorrect empty state message for "Nearest to You" screen.
The message says "You currently have no favorite eateries!" but this is the NearestScreen, which displays eateries sorted by proximity. The message should reflect the actual content (e.g., "No eateries found nearby" or indicate a location/data loading issue).
🤖 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/NearestScreen.kt` around lines 88 - 89, The empty-state message in NearestScreen is incorrect; locate the Text composable inside the NearestScreen composable (the Text whose text is "You currently have no favorite eateries!") and update its string to a context-appropriate message such as "No eateries found nearby" (or a localized string resource like R.string.no_eateries_nearby), ensuring the displayed message matches the NearestScreen's purpose and data-loading/location states.app/src/main/java/com/cornellappdev/android/eatery/ui/screens/SearchScreen.kt (1)
237-239:⚠️ Potential issue | 🟡 MinorPotential NPE with force-unwrapped
eatery.id.Using
eatery.id!!as a key in the favorites LazyRow can crash if an eatery has a null ID.🛡️ Suggested safer key
items(items = favorites, key = { eatery -> - eatery.id!! + eatery.id ?: eatery.hashCode() }) { eatery ->🤖 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 237 - 239, The key selector in the favorites LazyRow uses a force-unwrapped ID (items(items = favorites, key = { eatery -> eatery.id!! })) which can NPE; change the key to a null-safe value such as using eatery.id ?: fallback (e.g., eatery.name, eatery.slug, or eatery.hashCode()) so the key generation never uses !!; update the items(...) key lambda to return that safe fallback to avoid crashes while keeping key uniqueness.app/src/main/java/com/cornellappdev/android/eatery/ui/screens/FavoritesScreen.kt (1)
203-207:⚠️ Potential issue | 🟡 MinorPotential NPE with force-unwrapped
eatery.id.Using
eatery.id!!as a key assumes the ID is always non-null. If an eatery with a null ID enters this list, the app will crash. Consider using a safe fallback or filtering out eateries with null IDs upstream.🛡️ Suggested safer key
items( items = favoriteEateries, key = { eatery -> - eatery.id!! + eatery.id ?: eatery.hashCode() }) { eatery ->🤖 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/FavoritesScreen.kt` around lines 203 - 207, The use of a force-unwrapped key eatery.id!! inside the composable items call (items(items = favoriteEateries, key = { eatery -> eatery.id!! })) can crash if an Eatery has a null id; update the items key to use a safe fallback (e.g., eatery.id ?: eatery.hashCode().toString() or another stable unique fallback) or filter out null-id entries from favoriteEateries before rendering so the items key is never null; modify the items(...) invocation and/or the producer of favoriteEateries accordingly (refer to favoriteEateries and the items { eatery -> ... } key lambda).
🧹 Nitpick comments (2)
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/SearchViewModel.kt (2)
132-132: Verify ordering ofreversed().take(10).distinct().The current order applies
distinct()aftertake(10), which means if there are duplicates within the first 10 reversed items, you'll get fewer than 10 results. If the intent is to show up to 10 unique recent searches (most recent first), consider:recents.reversed().distinct().take(10)This ensures deduplication happens before limiting, giving you the 10 most recent unique searches.
🤖 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 132, The assignment to recentSearches in SearchViewModel uses recents.reversed().take(10).distinct(), which can yield fewer than 10 entries when duplicates appear in the first 10; change the call order to perform deduplication before limiting by using recents.reversed().distinct().take(10) so you get up to 10 most-recent unique searches (update the expression that sets recentSearches in SearchViewModel accordingly).
42-42: Consider thread-safety and memory management foreateryFlowCache.The
mutableMapOfcache storesStateFlowinstances indefinitely. Two potential concerns:
- Memory: The cache grows without bounds as users browse eateries. Consider using a size-limited cache or weak references.
- Thread-safety: While ViewModel operations typically run on Main,
getOrPutis not atomic. IfobserveEateryis called concurrently with the same ID, duplicate flows could be created.♻️ Thread-safe alternative using ConcurrentHashMap
+import java.util.concurrent.ConcurrentHashMap - private val eateryFlowCache = mutableMapOf<Int, StateFlow<EateryApiResponse<Eatery>>>() + private val eateryFlowCache = ConcurrentHashMap<Int, StateFlow<EateryApiResponse<Eatery>>>()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 an unbounded, non-thread-safe mutableMap that can leak memory and create duplicate StateFlows when observeEatery (and related logic around lines 206-213) is called concurrently; replace the map with a thread-safe bounded cache (e.g., a ConcurrentHashMap or an LRU size-limited cache holding StateFlow<EateryApiResponse<Eatery>> or WeakReferences to those flows) and atomically insert-or-retrieve entries (or synchronize access) so getOrPut-style creation of flows is safe under concurrency and the cache cannot grow unbounded.
🤖 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/ui/viewmodels/LoginViewModel.kt`:
- Around line 125-127: onLoginPressed currently only flips _isLoginLoadingFlow
to true but leaves any previous NetworkUiError live; modify onLoginPressed to
also clear the stored network error (e.g., set _networkUiErrorFlow /
_networkUiError.value to null or call the ViewModel's error-clear helper) so a
new login attempt doesn't show stale errors while loading. Target the
onLoginPressed method and the backing error flow/variable (e.g.,
_networkUiErrorFlow or NetworkUiError) and ensure the error is cleared before
setting _isLoginLoadingFlow to true.
- Around line 43-83: The UI shows the login screen whenever loadedUser == null,
which flashes during async session restore; add a session-restoring flag and use
it in the uiState combine: introduce a MutableStateFlow<Boolean> (e.g.
_isRestoringSession = MutableStateFlow(true)), include it in the combine
alongside userRepository.loadedUser and compute isLoginState as
(!isRestoringSession && loadedUser == null), then set _isRestoringSession.value
= false after init's restore flow finishes (e.g. after getFinancials() returns
or when getAccountRepository.isLoggedIn() check completes) so the UI only shows
login once restore is done; update references in uiState construction (symbols:
_isRestoringSession, uiState, userRepository.loadedUser, getFinancials(), init).
---
Outside diff comments:
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/screens/FavoritesScreen.kt`:
- Around line 203-207: The use of a force-unwrapped key eatery.id!! inside the
composable items call (items(items = favoriteEateries, key = { eatery ->
eatery.id!! })) can crash if an Eatery has a null id; update the items key to
use a safe fallback (e.g., eatery.id ?: eatery.hashCode().toString() or another
stable unique fallback) or filter out null-id entries from favoriteEateries
before rendering so the items key is never null; modify the items(...)
invocation and/or the producer of favoriteEateries accordingly (refer to
favoriteEateries and the items { eatery -> ... } key lambda).
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/screens/NearestScreen.kt`:
- Around line 104-108: The key provider for the LazyColumn items uses a
force-unwrapped ID (items(... key = { eatery -> eatery.id!! })) which can throw
an NPE; update the key logic in NearestScreen to use a null-safe fallback (e.g.,
key = { eatery -> eatery.id ?: eatery.hashCode() } or combine a stable field
like name/index) so it never force-unwraps, mirroring the safer approach used in
FavoritesScreen.
- Around line 88-89: The empty-state message in NearestScreen is incorrect;
locate the Text composable inside the NearestScreen composable (the Text whose
text is "You currently have no favorite eateries!") and update its string to a
context-appropriate message such as "No eateries found nearby" (or a localized
string resource like R.string.no_eateries_nearby), ensuring the displayed
message matches the NearestScreen's purpose and data-loading/location states.
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/screens/SearchScreen.kt`:
- Around line 237-239: The key selector in the favorites LazyRow uses a
force-unwrapped ID (items(items = favorites, key = { eatery -> eatery.id!! }))
which can NPE; change the key to a null-safe value such as using eatery.id ?:
fallback (e.g., eatery.name, eatery.slug, or eatery.hashCode()) so the key
generation never uses !!; update the items(...) key lambda to return that safe
fallback to avoid crashes while keeping key uniqueness.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/SearchViewModel.kt`:
- Line 132: The assignment to recentSearches in SearchViewModel uses
recents.reversed().take(10).distinct(), which can yield fewer than 10 entries
when duplicates appear in the first 10; change the call order to perform
deduplication before limiting by using recents.reversed().distinct().take(10) so
you get up to 10 most-recent unique searches (update the expression that sets
recentSearches in SearchViewModel accordingly).
- Line 42: eateryFlowCache is an unbounded, non-thread-safe mutableMap that can
leak memory and create duplicate StateFlows when observeEatery (and related
logic around lines 206-213) is called concurrently; replace the map with a
thread-safe bounded cache (e.g., a ConcurrentHashMap or an LRU size-limited
cache holding StateFlow<EateryApiResponse<Eatery>> or WeakReferences to those
flows) and atomically insert-or-retrieve entries (or synchronize access) so
getOrPut-style creation of flows is safe under concurrency and the cache cannot
grow unbounded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6948e272-6db4-4826-83d7-620c40148e48
📒 Files selected for processing (10)
app/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/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.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/cornellappdev/android/eatery/ui/screens/ProfileScreen.kt
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/LoginViewModel.kt
Show resolved
Hide resolved
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 prevents app crashes by making response data fields nullable and handling null data appropriately in the UI.
Changes Made
TransactionTypeDisplayTransactiondata structure which avoids nullable fieldsNext Steps
Summary by CodeRabbit
Refactor
Bug Fixes
UI
Performance