Open
Conversation
📝 WalkthroughWalkthroughIntroduces reactive session management and token refresh: adds SessionManager and TokenAuthenticator, migrates auth utilities to a dedicated package, adds a named refresh ApolloClient, updates DI to use Changes
Sequence DiagramssequenceDiagram
participant Client as Client (App)
participant TokenAuth as TokenAuthenticator
participant TokenMgr as TokenManager
participant ApolloRefresh as ApolloClient (Refresh)
participant Server as Server
participant SessionMgr as SessionManager
Client->>Server: Request with access token
Server-->>Client: 401 Unauthorized
Client->>TokenAuth: OkHttp invokes authenticate()
TokenAuth->>TokenMgr: getRefreshToken()
TokenMgr-->>TokenAuth: refresh token or null
alt refresh token present
rect rgba(200, 150, 255, 0.5)
TokenAuth->>ApolloRefresh: RefreshAccessTokenMutation (Authorization: Bearer <refresh>)
ApolloRefresh->>Server: POST GraphQL mutation
Server-->>ApolloRefresh: { newAccessToken }
end
TokenAuth->>TokenMgr: saveTokens(newAccessToken, existingRefresh)
TokenAuth->>Client: Retry original request with new access token
Client->>Server: Request with new token
Server-->>Client: Success
else no refresh token or refresh failed
TokenAuth->>SessionMgr: logout()
SessionMgr-->>TokenAuth: session cleared
TokenAuth-->>Client: null (no retry)
end
sequenceDiagram
participant User as User
participant LoginVM as LoginViewModel
participant UserInfoRepo as UserInfoRepository
participant ApolloMain as ApolloClient (Main)
participant Server as Server
participant SessionMgr as SessionManager
participant TokenMgr as TokenManager
User->>LoginVM: Submit netId
LoginVM->>UserInfoRepo: loginUser(netId)
rect rgba(100, 200, 150, 0.5)
UserInfoRepo->>ApolloMain: LoginUserMutation
ApolloMain->>Server: POST GraphQL mutation
Server-->>ApolloMain: tokens + user data
end
UserInfoRepo->>UserInfoRepo: getUserByNetId(), validate tokens
UserInfoRepo->>UserInfoRepo: parse userId (toIntOrNull)
UserInfoRepo->>SessionMgr: startSession(userId, name, email, access, refresh)
SessionMgr->>TokenMgr: saveUserSession(userId, name, email)
SessionMgr->>TokenMgr: saveTokens(access, refresh)
SessionMgr->>SessionMgr: set isLoggedIn = true
SessionMgr-->>UserInfoRepo: return
UserInfoRepo-->>LoginVM: true
LoginVM-->>User: Login complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (8)
app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/onboarding/ProfileCreationViewModel.kt (1)
96-98:navigateToHome()appears unused after this change.Since navigation is now reactive via
SessionManager, this private method may be dead code. Consider removing it if no longer needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/onboarding/ProfileCreationViewModel.kt` around lines 96 - 98, The private method navigateToHome() (which calls rootNavigationRepository.navigate(UpliftRootRoute.Home)) appears to be unused now that navigation is driven reactively via SessionManager; remove this dead method and any now-unused imports or references to it, or if it's referenced by tests or external callers keep it and instead convert its callers to rely on SessionManager; alternatively annotate with `@Suppress`("unused") if you must keep it for reflection/tests—prefer deletion if there are no usages.app/src/main/java/com/cornellappdev/uplift/data/repositories/TokenManager.kt (1)
62-64: Consider renaming or scopingclearTokens()to match its behavior.
clearTokens()callsclear()which removes all preferences including user session data (user_id,username,user_email). If this is intentional, consider renaming toclearAll()orclearSession(). If only tokens should be cleared, update to remove specific keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/data/repositories/TokenManager.kt` around lines 62 - 64, The clearTokens() function currently calls sharedPreferences?.edit { clear() } which wipes all stored prefs (including user_id, username, user_email); either rename the method to reflect clearing everything (e.g., clearAll() or clearSession()) or limit its behavior to only token-related keys by editing sharedPreferences to remove the specific token keys (use sharedPreferences?.edit { remove("access_token"); remove("refresh_token") } or equivalent). Update the function name and any callers if you choose renaming, or replace the clear() call with targeted remove(...) calls inside TokenManager.clearTokens() to preserve other user session data.app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.kt (2)
81-83: Simplify SessionManager access - redundant ViewModel instantiation.The current pattern calls
hiltViewModel<RootNavigationViewModel>()twice unnecessarily. Theletblock assigns toitwhich is never used.♻️ Proposed fix
- sessionManager: SessionManager = hiltViewModel<RootNavigationViewModel>().let { - hiltViewModel<RootNavigationViewModel>().sessionManager - } + sessionManager: SessionManager = rootNavigationViewModel.sessionManager🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.kt` around lines 81 - 83, The sessionManager initialization redundantly calls hiltViewModel<RootNavigationViewModel>() twice and uses a useless let block; replace this with a single hiltViewModel<RootNavigationViewModel>() invocation and read the sessionManager from that instance (e.g., obtain the RootNavigationViewModel once and use its sessionManager property). Update the expression that sets sessionManager to reference the single ViewModel instance (RootNavigationViewModel) instead of calling hiltViewModel twice or using an unused it.
111-117: Potential duplicate navigation logic withstartDestination.Both
RootNavigationViewModel.startDestinationand thisLaunchedEffectreact toisLoggedInstate. On app startup, both may attempt to set the initial route, potentially causing redundant navigation. Consider guarding against navigating to Onboarding if already there:♻️ Suggested guard
LaunchedEffect(isLoggedIn) { - if (!isLoggedIn && ONBOARDING_FLAG) { + val currentRoute = navController.currentDestination?.route + if (!isLoggedIn && ONBOARDING_FLAG && currentRoute != UpliftRootRoute.Onboarding::class.qualifiedName) { navController.navigate(UpliftRootRoute.Onboarding) { popUpTo(0) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.kt` around lines 111 - 117, The LaunchedEffect block in MainNavigationWrapper (reacting to isLoggedIn and ONBOARDING_FLAG) can duplicate navigation already handled by RootNavigationViewModel.startDestination; update the LaunchedEffect to first check the current navController destination and only call navController.navigate(UpliftRootRoute.Onboarding) if the current route is not already Onboarding (e.g., inspect navController.currentBackStackEntry?.destination?.route or a similar marker) so you avoid redundant navigation attempts when startDestination has already set the route.app/src/main/java/com/cornellappdev/uplift/data/repositories/SessionManager.kt (1)
17-22: Consider adding input validation tostartSession.The method accepts parameters directly without validation. Depending on how this is called, consider guarding against invalid inputs (e.g., empty tokens, negative userId) to fail fast rather than persisting invalid session state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/data/repositories/SessionManager.kt` around lines 17 - 22, Validate inputs in startSession: check userId > 0, name and email are not blank, and access/refresh tokens are not null/blank before calling tokenManager.saveTokens or tokenManager.saveUserSession and before setting _isLoggedIn; if any check fails, do not persist anything and fail fast (e.g., throw IllegalArgumentException or return a failure result) so invalid session state is never stored. Ensure these checks live inside startSession and reference tokenManager.saveTokens, tokenManager.saveUserSession, and _isLoggedIn to prevent partial updates.app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt (1)
50-59: Potential startup flicker and repeated DataStore reads.The
startDestinationinitializes toOnboarding(whenONBOARDING_FLAGis true) before the collector runs and potentially updates it toHome. This could cause a brief navigation flicker on app startup.Additionally,
getSkipFromDataStore()is called on everyisLoggedInemission, which may involve I/O. Consider caching this value or reading it once during initialization.♻️ Suggested optimization
viewModelScope.launch { + val hasSkipped = userInfoRepository.getSkipFromDataStore() sessionManager.isLoggedIn.collect { loggedIn -> - val hasSkipped = userInfoRepository.getSkipFromDataStore() val shouldShowHome = loggedIn || hasSkipped || !ONBOARDING_FLAG applyMutation { copy( startDestination = if (shouldShowHome) UpliftRootRoute.Home else UpliftRootRoute.Onboarding ) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt` around lines 50 - 59, The startup flicker and repeated DataStore reads are caused by computing startDestination before knowing the user's skip flag and by calling userInfoRepository.getSkipFromDataStore() on every sessionManager.isLoggedIn emission; fix it in RootNavigationViewModel by reading and caching the skip value once (e.g., call userInfoRepository.getSkipFromDataStore() once during init or use first()) and computing the initial startDestination using ONBOARDING_FLAG, the cached hasSkipped, and the current session state, then launch the collector (viewModelScope.launch { sessionManager.isLoggedIn.collect { ... } }) to only update on real changes while referencing the cached skip value; update applyMutation(copy(startDestination = ...)) to use this cached value so you avoid repeated I/O and eliminate the brief navigation flicker.app/src/main/java/com/cornellappdev/uplift/di/AppModule.kt (1)
21-32: LGTM - Consider adding timeout configuration.The separation of "refresh" and "main"
ApolloClientinstances correctly prevents authentication loops. The refresh client intentionally omits interceptors.As an optional improvement, consider configuring timeouts on the
OkHttpClientto prevent indefinite hangs during network issues:♻️ Optional timeout configuration
fun provideRefreshApolloClient(): ApolloClient { // This client does NOT have interceptors to avoid loops - val okHttpClient = OkHttpClient.Builder().build() + val okHttpClient = OkHttpClient.Builder() + .connectTimeout(30, java.util.concurrent.TimeUnit.SECONDS) + .readTimeout(30, java.util.concurrent.TimeUnit.SECONDS) + .build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/di/AppModule.kt` around lines 21 - 32, The provideRefreshApolloClient() currently builds an OkHttpClient without any timeouts; update the OkHttpClient.Builder() in provideRefreshApolloClient to configure sensible timeouts (e.g., connectTimeout, readTimeout, writeTimeout and callTimeout) to avoid indefinite hangs during network issues—use OkHttpClient.Builder().connectTimeout(..., TimeUnit.SECONDS).readTimeout(..., TimeUnit.SECONDS).writeTimeout(..., TimeUnit.SECONDS).callTimeout(..., TimeUnit.SECONDS) (or the Kotlin Duration overloads) and ensure TimeUnit is imported; keep this change confined to the provideRefreshApolloClient OkHttpClient construction so the refresh client still has no interceptors.app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/onboarding/LoginViewModel.kt (1)
44-61: LGTM with a note on error handling.The refactored login flow correctly delegates navigation to session state. When
loginUserreturnstrue, theSessionManagerwill updateisLoggedIn, triggering navigation viaMainNavigationWrapper.Consider addressing the TODO comments (Lines 26, 40, 56) to provide user feedback on authentication failures instead of silently signing out.
Would you like me to open an issue to track adding user-facing error handling (e.g., toast messages) for the login failure cases?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/onboarding/LoginViewModel.kt` around lines 44 - 61, The current branches (userInfoRepository.hasUser / userInfoRepository.hasFirebaseUser / else) silently sign out or log an error without user feedback; replace the TODOs by emitting a user-facing error event from LoginViewModel (e.g., a LiveData/StateFlow like loginError or single-shot UI event) when loginUser returns false and in the else branch, and use that event to show a toast/snackbar or error screen instead of only calling userInfoRepository.signOut; update calls surrounding userInfoRepository.loginUser(netId) and userInfoRepository.hasFirebaseUser() and rootNavigationRepository.navigate(UpliftRootRoute.ProfileCreation) to trigger navigation only on success while reporting clear error messages via the new ViewModel event so the UI can present feedback.
🤖 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/uplift/data/repositories/TokenAuthenticator.kt`:
- Around line 58-61: The catch block in TokenAuthenticator (the catch (e:
Exception) that returns null during token refresh) is silently swallowing
exceptions; update it to log the caught exception with context (e.g., using
Log.e or Timber.e or your project's logger) so network/server errors during
refresh are recorded, then continue to return null; locate the catch in the
TokenAuthenticator class (the token refresh/authentication method) and replace
the empty swallow with a descriptive error log including e.
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/TokenManager.kt`:
- Line 74: getUserId() currently returns sharedPreferences?.getInt("user_id",
-1) which yields -1 when the key is absent instead of null; update
TokenManager.getUserId() to check sharedPreferences?.contains("user_id") (or
read the int and map the sentinel -1 to null) and return null when the key
doesn't exist, otherwise return the stored Int value; reference
sharedPreferences and the "user_id" key when making this change so callers
receive null for "no user" rather than -1.
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt`:
- Around line 99-111: The session start block uses userInfo.id.toIntOrNull() ?:
-1 and logs a generic error using loginResponse.errors; change it to explicitly
handle a null or non-parsable userId and distinguish failure reasons: before
calling sessionManager.startSession, validate that userInfo is non-null and that
userInfo.id parses to Int (using userInfo.id.toIntOrNull() and bail with a clear
Log.e mentioning "missing userInfo" or "invalid user id" instead of -1), and
when tokens are missing log whether access/refresh tokens are absent versus
userInfo being null (include loginResponse.errors only when applicable). Update
the flow around sessionManager.startSession and the Log.e calls to produce
precise, separate log messages for missing tokens, null userInfo, and invalid
user id.
- Around line 58-64: The code currently calls sessionManager.startSession with
userId = id.toIntOrNull() ?: -1 which silently uses -1 on parse failure; update
the logic in the surrounding function (the caller performing id.toIntOrNull and
the call to sessionManager.startSession) to validate the id first: if
id.toIntOrNull() is null, do not call sessionManager.startSession and
return/propagate an error (e.g., return false or throw a descriptive exception)
instead of starting a session with -1, otherwise pass the valid parsed Int into
sessionManager.startSession; adjust any callers or return value handling
accordingly so invalid IDs fail early.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/SessionManager.kt`:
- Around line 17-22: Validate inputs in startSession: check userId > 0, name and
email are not blank, and access/refresh tokens are not null/blank before calling
tokenManager.saveTokens or tokenManager.saveUserSession and before setting
_isLoggedIn; if any check fails, do not persist anything and fail fast (e.g.,
throw IllegalArgumentException or return a failure result) so invalid session
state is never stored. Ensure these checks live inside startSession and
reference tokenManager.saveTokens, tokenManager.saveUserSession, and _isLoggedIn
to prevent partial updates.
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/TokenManager.kt`:
- Around line 62-64: The clearTokens() function currently calls
sharedPreferences?.edit { clear() } which wipes all stored prefs (including
user_id, username, user_email); either rename the method to reflect clearing
everything (e.g., clearAll() or clearSession()) or limit its behavior to only
token-related keys by editing sharedPreferences to remove the specific token
keys (use sharedPreferences?.edit { remove("access_token");
remove("refresh_token") } or equivalent). Update the function name and any
callers if you choose renaming, or replace the clear() call with targeted
remove(...) calls inside TokenManager.clearTokens() to preserve other user
session data.
In `@app/src/main/java/com/cornellappdev/uplift/di/AppModule.kt`:
- Around line 21-32: The provideRefreshApolloClient() currently builds an
OkHttpClient without any timeouts; update the OkHttpClient.Builder() in
provideRefreshApolloClient to configure sensible timeouts (e.g., connectTimeout,
readTimeout, writeTimeout and callTimeout) to avoid indefinite hangs during
network issues—use OkHttpClient.Builder().connectTimeout(...,
TimeUnit.SECONDS).readTimeout(..., TimeUnit.SECONDS).writeTimeout(...,
TimeUnit.SECONDS).callTimeout(..., TimeUnit.SECONDS) (or the Kotlin Duration
overloads) and ensure TimeUnit is imported; keep this change confined to the
provideRefreshApolloClient OkHttpClient construction so the refresh client still
has no interceptors.
In `@app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.kt`:
- Around line 81-83: The sessionManager initialization redundantly calls
hiltViewModel<RootNavigationViewModel>() twice and uses a useless let block;
replace this with a single hiltViewModel<RootNavigationViewModel>() invocation
and read the sessionManager from that instance (e.g., obtain the
RootNavigationViewModel once and use its sessionManager property). Update the
expression that sets sessionManager to reference the single ViewModel instance
(RootNavigationViewModel) instead of calling hiltViewModel twice or using an
unused it.
- Around line 111-117: The LaunchedEffect block in MainNavigationWrapper
(reacting to isLoggedIn and ONBOARDING_FLAG) can duplicate navigation already
handled by RootNavigationViewModel.startDestination; update the LaunchedEffect
to first check the current navController destination and only call
navController.navigate(UpliftRootRoute.Onboarding) if the current route is not
already Onboarding (e.g., inspect
navController.currentBackStackEntry?.destination?.route or a similar marker) so
you avoid redundant navigation attempts when startDestination has already set
the route.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt`:
- Around line 50-59: The startup flicker and repeated DataStore reads are caused
by computing startDestination before knowing the user's skip flag and by calling
userInfoRepository.getSkipFromDataStore() on every sessionManager.isLoggedIn
emission; fix it in RootNavigationViewModel by reading and caching the skip
value once (e.g., call userInfoRepository.getSkipFromDataStore() once during
init or use first()) and computing the initial startDestination using
ONBOARDING_FLAG, the cached hasSkipped, and the current session state, then
launch the collector (viewModelScope.launch { sessionManager.isLoggedIn.collect
{ ... } }) to only update on real changes while referencing the cached skip
value; update applyMutation(copy(startDestination = ...)) to use this cached
value so you avoid repeated I/O and eliminate the brief navigation flicker.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/onboarding/LoginViewModel.kt`:
- Around line 44-61: The current branches (userInfoRepository.hasUser /
userInfoRepository.hasFirebaseUser / else) silently sign out or log an error
without user feedback; replace the TODOs by emitting a user-facing error event
from LoginViewModel (e.g., a LiveData/StateFlow like loginError or single-shot
UI event) when loginUser returns false and in the else branch, and use that
event to show a toast/snackbar or error screen instead of only calling
userInfoRepository.signOut; update calls surrounding
userInfoRepository.loginUser(netId) and userInfoRepository.hasFirebaseUser() and
rootNavigationRepository.navigate(UpliftRootRoute.ProfileCreation) to trigger
navigation only on success while reporting clear error messages via the new
ViewModel event so the UI can present feedback.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/onboarding/ProfileCreationViewModel.kt`:
- Around line 96-98: The private method navigateToHome() (which calls
rootNavigationRepository.navigate(UpliftRootRoute.Home)) appears to be unused
now that navigation is driven reactively via SessionManager; remove this dead
method and any now-unused imports or references to it, or if it's referenced by
tests or external callers keep it and instead convert its callers to rely on
SessionManager; alternatively annotate with `@Suppress`("unused") if you must keep
it for reflection/tests—prefer deletion if there are no usages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75c38f1e-58e7-412b-b55b-2f88669bd909
📒 Files selected for processing (17)
app/build.gradleapp/src/main/graphql/User.graphqlapp/src/main/java/com/cornellappdev/uplift/data/repositories/CapacityRemindersRepository.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/CheckInRepository.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/PopularTimesRepository.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/ProfileRepository.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/ReportRepository.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/SessionManager.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/TokenAuthenticator.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/TokenManager.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/UpliftApiRepository.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.ktapp/src/main/java/com/cornellappdev/uplift/di/AppModule.ktapp/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/onboarding/LoginViewModel.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/onboarding/ProfileCreationViewModel.kt
app/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.kt
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/uplift/data/repositories/TokenManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/uplift/data/repositories/SessionManager.kt
Outdated
Show resolved
Hide resolved
Member
|
For the AuthInterceptor code which isn't here, since we are adding authenticator with retries, we might want to change addHeader to just header to make sure we replace the header with the old access token. |
app/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.kt
Outdated
Show resolved
Hide resolved
| private val tokenManager: TokenManager | ||
| ) { | ||
| // A reactive flow that the UI can collect | ||
| private val _isLoggedIn = MutableStateFlow(tokenManager.getAccessToken() != null) |
Member
There was a problem hiding this comment.
This is only initialized once, so if the token stuff changes outside of SessionManager, this state can become outdated. I think you can either make sure to enforce that it is only updated via the session manager (which you might be doing already) or expose a flow from token manager.
| @@ -62,4 +62,15 @@ class TokenManager @Inject constructor(@ApplicationContext private val context: | |||
| fun clearTokens() { | |||
| sharedPreferences?.edit { clear() } | |||
| } | |||
|
|
|||
| fun saveUserSession(userId: Int, username: String, userEmail: String) { | |||
Member
There was a problem hiding this comment.
Nit: It's probably fine to have this here, but the naming of this class implies mainly handling tokens so having other user data stuff here might be a little confusing
Member
There was a problem hiding this comment.
Also, the above clearTokens function will now clear these data fields as well, which could be the intended behavior anyways but same "issue" as above
| import javax.inject.Inject | ||
| import javax.inject.Named | ||
|
|
||
| class TokenAuthenticator @Inject constructor( |
Member
There was a problem hiding this comment.
We might want to consider having a guard based on number of retries to prevent infinite retries
| ) : Authenticator { | ||
|
|
||
| override fun authenticate(route: Route?, response: Response): Request? { | ||
| val refreshToken = tokenManager.getRefreshToken() ?: return null |
Member
There was a problem hiding this comment.
This is probably out of scope of this PR, but for refresh tokens, if we know when refresh tokens expire from the backend and if backend has some endpoint that allows us to get a new refresh token aside from forcing the user to log in, it would probably be better UX to get the new refresh token before it expires and keep the user logged in for longer and then fall back to logging the user out for cases where maybe the user hasn't opened the app in a long time.
| val accessToken = loginData.accessToken | ||
| val refreshToken = loginData.refreshToken | ||
| tokenManager.saveTokens(accessToken, refreshToken) | ||
| sessionManager.startSession( |
Member
There was a problem hiding this comment.
Might make sense to start the session after the if (!skip) logic below since if uploadGoal ends up returning null, we could end up returning false for the createUser function but still preserving the partial login state from the session manager.
| gymDetailViewModel: GymDetailViewModel = hiltViewModel(), | ||
| classDetailViewModel: ClassDetailViewModel = hiltViewModel(), | ||
| rootNavigationViewModel: RootNavigationViewModel = hiltViewModel(), | ||
| sessionManager: SessionManager = hiltViewModel<RootNavigationViewModel>().let { |
Member
There was a problem hiding this comment.
we might be able to just declare val sessionManager = rootNavigationViewModel.sessionManager outside of the parameters
| copy( | ||
| startDestination = if (hasSkipped || hasUser || !ONBOARDING_FLAG) UpliftRootRoute.Home else UpliftRootRoute.Onboarding | ||
| ) | ||
| sessionManager.isLoggedIn.collect { loggedIn -> |
Member
There was a problem hiding this comment.
This could be part of a separate or same PR or documented as a git issue, but it would probably be better UX if we can have it so if the user is logged in, they see the splash screen to home screen directly instead of splash screen to login screen briefly then home screen.
Screen_recording_20260323_214956.webm
|
|
||
| systemUiController.setStatusBarColor(PRIMARY_YELLOW) | ||
|
|
||
| val isLoggedIn by sessionManager.isLoggedIn.collectAsState() |
Member
There was a problem hiding this comment.
since we're getting the session manager from the root navigation vm, I think it might make more sense to just add isLoggedIn as part of the vm's uiState?
Member
AndrewCheung360
left a comment
AndrewCheung360
left a comment
There was a problem hiding this comment.
Left some initial comments for you to look at for some possible improvements, but overall nice work!
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/src/main/java/com/cornellappdev/uplift/data/auth/AuthInterceptor.kt (1)
4-4: Redundant import:TokenManageris in the same package.Since
TokenManageris now incom.cornellappdev.uplift.data.auth(same as this file), the explicit import is unnecessary.🧹 Proposed fix
import android.util.Log -import com.cornellappdev.uplift.data.auth.TokenManager import okhttp3.Interceptor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/data/auth/AuthInterceptor.kt` at line 4, The import of TokenManager at the top of AuthInterceptor.kt is redundant because TokenManager resides in the same package; remove the explicit import statement referencing TokenManager so the file relies on the package scope, keeping class AuthInterceptor unchanged.app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt (1)
63-74: Session starts before goal upload - partial state on failure.If
uploadGoal()fails (line 71-73), the function returnsfalsebut the session is already active (started at line 63). This leaves the user in a partially logged-in state where tokens are saved but onboarding isn't complete. Consider movingstartSession()after the goal upload succeeds, or handling the rollback on failure.♻️ Proposed restructure
- sessionManager.startSession( - userId = id, - name = name, - email = email, - access = accessToken, - refresh = refreshToken - ) if (!skip) { if (!uploadGoal(id, goal)) { return false } } else { Log.d("UserInfoRepository", "Skipping goal upload") } + sessionManager.startSession( + userId = id, + name = name, + email = email, + access = accessToken, + refresh = refreshToken + ) storeUserFields(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt` around lines 63 - 74, The current flow calls sessionManager.startSession(...) before uploadGoal(...), which can leave tokens saved if uploadGoal fails; change the order so uploadGoal(id, goal) is attempted first (when skip is false) and only call sessionManager.startSession(...) after uploadGoal returns true, or if you prefer to keep the current order implement a rollback by calling sessionManager.clearSession()/endSession and removing stored tokens when uploadGoal(...) returns false; update the logic around startSession, uploadGoal, and sessionManager to ensure no active session exists on uploadGoal failure.app/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.kt (1)
16-20: Consider adding@Singletonannotation for consistency and clarity.
TokenAuthenticatoris implicitly scoped to the singleton component since it's only injected inprovideOkHttpClient()(a@Singletonprovider). While the code works correctly as-is, explicitly annotating@Singletonwould improve consistency withTokenManagerandSessionManager, making the scoping intent clearer to future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.kt` around lines 16 - 20, The TokenAuthenticator class lacks an explicit scope annotation; add the `@Singleton` annotation to the TokenAuthenticator class declaration so its scope is consistent with TokenManager and SessionManager and matches the singleton provider that injects it (see TokenAuthenticator, TokenManager, SessionManager, and the provideOkHttpClient provider).
🤖 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/uplift/data/auth/AuthInterceptor.kt`:
- Around line 17-20: The interceptor currently uses addHeader("Authorization",
"Bearer $token") inside chain.request().newBuilder().apply which appends a new
Authorization header on retries; change that call to header("Authorization",
"Bearer $token") so the header is set/replaced instead of duplicated (locate the
addHeader usage in AuthInterceptor and swap to header within the request builder
logic).
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt`:
- Around line 104-108: Logging uses the nullified parsed variable `id` instead
of the original string; in the method in UserInfoRepository where you parse `val
id = userInfo.id.toIntOrNull()` (e.g., the login routine), update the error log
to include the original `userInfo.id` string (and keep the same descriptive
message) so the log shows the actual input that failed to parse rather than the
null `id` variable.
- Around line 51-55: The log currently prints the parsed `id` variable (which is
null on parse failure) making the message unhelpful; update the error logging in
UserInfoRepository where you call `userFields.id.toIntOrNull()` (the block that
returns false) to log the original string `userFields.id` (and optionally a
short context message) instead of `id` so the malformed input value is visible
in the log.
---
Nitpick comments:
In `@app/src/main/java/com/cornellappdev/uplift/data/auth/AuthInterceptor.kt`:
- Line 4: The import of TokenManager at the top of AuthInterceptor.kt is
redundant because TokenManager resides in the same package; remove the explicit
import statement referencing TokenManager so the file relies on the package
scope, keeping class AuthInterceptor unchanged.
In `@app/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.kt`:
- Around line 16-20: The TokenAuthenticator class lacks an explicit scope
annotation; add the `@Singleton` annotation to the TokenAuthenticator class
declaration so its scope is consistent with TokenManager and SessionManager and
matches the singleton provider that injects it (see TokenAuthenticator,
TokenManager, SessionManager, and the provideOkHttpClient provider).
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt`:
- Around line 63-74: The current flow calls sessionManager.startSession(...)
before uploadGoal(...), which can leave tokens saved if uploadGoal fails; change
the order so uploadGoal(id, goal) is attempted first (when skip is false) and
only call sessionManager.startSession(...) after uploadGoal returns true, or if
you prefer to keep the current order implement a rollback by calling
sessionManager.clearSession()/endSession and removing stored tokens when
uploadGoal(...) returns false; update the logic around startSession, uploadGoal,
and sessionManager to ensure no active session exists on uploadGoal failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30aaaa26-59f2-4393-b46b-1177035e00ef
📒 Files selected for processing (8)
app/src/main/java/com/cornellappdev/uplift/data/auth/AuthInterceptor.ktapp/src/main/java/com/cornellappdev/uplift/data/auth/SessionManager.ktapp/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.ktapp/src/main/java/com/cornellappdev/uplift/data/auth/TokenManager.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.ktapp/src/main/java/com/cornellappdev/uplift/di/AppModule.ktapp/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt
- app/src/main/java/com/cornellappdev/uplift/di/AppModule.kt
Comment on lines
+51
to
+55
| val id = userFields.id.toIntOrNull() | ||
| if (id == null) { | ||
| Log.e("UserInfoRepository", "Failed to set goal: non-numeric user ID '$id'") | ||
| return false | ||
| } |
There was a problem hiding this comment.
Log the original string value, not the null result.
When toIntOrNull() fails, id is null, so the log message always shows "non-numeric user ID 'null'" which doesn't help identify the malformed input. Log the original userFields.id instead.
🔧 Proposed fix
val id = userFields.id.toIntOrNull()
if (id == null) {
- Log.e("UserInfoRepository", "Failed to set goal: non-numeric user ID '$id'")
+ Log.e("UserInfoRepository", "Failed to set goal: non-numeric user ID '${userFields.id}'")
return false
}📝 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 id = userFields.id.toIntOrNull() | |
| if (id == null) { | |
| Log.e("UserInfoRepository", "Failed to set goal: non-numeric user ID '$id'") | |
| return false | |
| } | |
| val id = userFields.id.toIntOrNull() | |
| if (id == null) { | |
| Log.e("UserInfoRepository", "Failed to set goal: non-numeric user ID '${userFields.id}'") | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt`
around lines 51 - 55, The log currently prints the parsed `id` variable (which
is null on parse failure) making the message unhelpful; update the error logging
in UserInfoRepository where you call `userFields.id.toIntOrNull()` (the block
that returns false) to log the original string `userFields.id` (and optionally a
short context message) instead of `id` so the malformed input value is visible
in the log.
Comment on lines
+104
to
+108
| val id = userInfo.id.toIntOrNull() | ||
| if (id == null) { | ||
| Log.e("UserInfoRepository", "Failed to log in: non-numeric user ID '$id'") | ||
| return false | ||
| } |
There was a problem hiding this comment.
Same logging issue: log the original string value.
Similar to createUser, this logs the already-null id variable rather than the original userInfo.id string.
🔧 Proposed fix
val id = userInfo.id.toIntOrNull()
if (id == null) {
- Log.e("UserInfoRepository", "Failed to log in: non-numeric user ID '$id'")
+ Log.e("UserInfoRepository", "Failed to log in: non-numeric user ID '${userInfo.id}'")
return false
}📝 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 id = userInfo.id.toIntOrNull() | |
| if (id == null) { | |
| Log.e("UserInfoRepository", "Failed to log in: non-numeric user ID '$id'") | |
| return false | |
| } | |
| val id = userInfo.id.toIntOrNull() | |
| if (id == null) { | |
| Log.e("UserInfoRepository", "Failed to log in: non-numeric user ID '${userInfo.id}'") | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt`
around lines 104 - 108, Logging uses the nullified parsed variable `id` instead
of the original string; in the method in UserInfoRepository where you parse `val
id = userInfo.id.toIntOrNull()` (e.g., the login routine), update the error log
to include the original `userInfo.id` string (and keep the same descriptive
message) so the log shows the actual input that failed to parse rather than the
null `id` variable.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.kt (2)
23-26: Add a retry count guard to prevent infinite refresh loops.If the refreshed token is also rejected by the server (e.g., account revoked, permissions changed), this authenticator will continuously refresh and retry without limit. OkHttp's
Response.priorResponsecan be used to count attempts.🛡️ Proposed fix
+ private fun responseCount(response: Response): Int { + var count = 1 + var prior = response.priorResponse + while (prior != null) { + count++ + prior = prior.priorResponse + } + return count + } + override fun authenticate(route: Route?, response: Response): Request? { + // Prevent infinite retry loops + if (responseCount(response) >= 3) { + Log.w("TokenAuthenticator", "Max retry attempts reached, giving up") + return null + } + val refreshToken = tokenManager.getRefreshToken() ?: return 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/uplift/data/auth/TokenAuthenticator.kt` around lines 23 - 26, The authenticate method in TokenAuthenticator currently refreshes and retries indefinitely; add a retry-count guard by walking response.priorResponse to count previous attempts and bail out when a max retry threshold (e.g., MAX_REFRESH_ATTEMPTS) is reached; if the count exceeds the limit, return null to avoid further retries. Implement the guard at the start of authenticate (before calling tokenManager.getRefreshToken/refreshToken) and reference the authenticate method and tokenManager.refreshToken/tokenManager.getRefreshToken so the flow stops and no new request is built when the max attempts are exceeded.
6-7: Redundant same-package imports.
SessionManagerandTokenManagerare in the same package (com.cornellappdev.uplift.data.auth) as this file, so these imports are unnecessary in Kotlin.🧹 Proposed fix
import com.apollographql.apollo.ApolloClient import com.cornellappdev.uplift.RefreshAccessTokenMutation -import com.cornellappdev.uplift.data.auth.SessionManager -import com.cornellappdev.uplift.data.auth.TokenManager import kotlinx.coroutines.runBlocking🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.kt` around lines 6 - 7, The file TokenAuthenticator.kt has redundant same-package imports for SessionManager and TokenManager; remove the import lines for com.cornellappdev.uplift.data.auth.SessionManager and com.cornellappdev.uplift.data.auth.TokenManager from the top of TokenAuthenticator.kt so the classes are referenced directly (they’re in the same package), and run a quick build or linter to ensure no other unused-import warnings remain.
🤖 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/uplift/data/auth/TokenAuthenticator.kt`:
- Around line 23-26: The authenticate method in TokenAuthenticator currently
refreshes and retries indefinitely; add a retry-count guard by walking
response.priorResponse to count previous attempts and bail out when a max retry
threshold (e.g., MAX_REFRESH_ATTEMPTS) is reached; if the count exceeds the
limit, return null to avoid further retries. Implement the guard at the start of
authenticate (before calling tokenManager.getRefreshToken/refreshToken) and
reference the authenticate method and
tokenManager.refreshToken/tokenManager.getRefreshToken so the flow stops and no
new request is built when the max attempts are exceeded.
- Around line 6-7: The file TokenAuthenticator.kt has redundant same-package
imports for SessionManager and TokenManager; remove the import lines for
com.cornellappdev.uplift.data.auth.SessionManager and
com.cornellappdev.uplift.data.auth.TokenManager from the top of
TokenAuthenticator.kt so the classes are referenced directly (they’re in the
same package), and run a quick build or linter to ensure no other unused-import
warnings remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cca1dd5-8f2c-45ec-a5d6-a63ba479c178
📒 Files selected for processing (3)
app/src/main/java/com/cornellappdev/uplift/data/auth/AuthInterceptor.ktapp/src/main/java/com/cornellappdev/uplift/data/auth/TokenAuthenticator.ktapp/src/main/java/com/cornellappdev/uplift/di/AppModule.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/cornellappdev/uplift/data/auth/AuthInterceptor.kt
- app/src/main/java/com/cornellappdev/uplift/di/AppModule.kt
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
Handled user token expiration and created way to manage the user's session
Changes Made
Created a token manager to encrypt and store access and refresh tokens for users [TokenManager.kt]
Created a token authenticator to attach access token header to all mutations [TokenAuthenticator.kt]
Created a session manager to automatically refresh expired tokens and log users out when their tokens aren't able to be refreshed [SessionManager.kt]
Made another apollo client to enable token refresh [AppModule.kt]
Added name flag to Apollo Client fields of multiple repositories to distinguish between the two
Test Coverage
Was able to test by deleting my account and going through onboarding again to test whether the access tokens were available or not with the refactor changes
Related PRs or Issues (delete if not applicable)
Improved on Token management component of "Goals For Onboarding" PR
Screenshots (delete if not applicable)
Log Cat Testing
Summary by CodeRabbit
New Features
Improvements