Token and Session Management#106
Merged
isiahpwilliams merged 11 commits intomainfrom Mar 25, 2026
Merged
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:
📝 WalkthroughWalkthroughAdds reactive session management and automatic token refresh: introduces SessionManager, TokenAuthenticator, moves auth utilities to a dedicated package, provides named Apollo clients ("main" and "refresh"), updates DI and repositories to use Changes
Sequence DiagramssequenceDiagram
participant Client as App (OkHttp)
participant TokenAuth as TokenAuthenticator
participant TokenMgr as TokenManager
participant ApolloRefresh as ApolloClient (Refresh)
participant Server as GraphQL Server
participant SessionMgr as SessionManager
Client->>Server: Request with Authorization: Bearer <access>
Server-->>Client: 401 Unauthorized
Client->>TokenAuth: OkHttp calls authenticate(response)
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: return original request with Authorization: Bearer <newAccessToken>
Client->>Server: Retry request with new token
Server-->>Client: 200 OK
else no refresh token or refresh failed
TokenAuth->>SessionMgr: logout()
SessionMgr-->>TokenMgr: clearTokens()
TokenAuth-->>Client: null (stop retries)
end
sequenceDiagram
participant User as User
participant LoginVM as LoginViewModel
participant UserRepo as UserInfoRepository
participant ApolloMain as ApolloClient (Main)
participant Server as GraphQL Server
participant SessionMgr as SessionManager
participant TokenMgr as TokenManager
User->>LoginVM: Submit netId
LoginVM->>UserRepo: loginUser(netId)
rect rgba(100,200,150,0.5)
UserRepo->>ApolloMain: LoginUserMutation
ApolloMain->>Server: POST GraphQL mutation
Server-->>ApolloMain: { accessToken, refreshToken, user }
end
UserRepo->>UserRepo: getUserByNetId(), parse userId (toIntOrNull)
UserRepo->>SessionMgr: startSession(userId, name, email, access, refresh)
SessionMgr->>TokenMgr: saveUserSession(userId, name, email)
SessionMgr->>TokenMgr: saveTokens(access, refresh)
SessionMgr->>SessionMgr: emit isLoggedIn = true
UserRepo-->>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 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
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. |
| @@ -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
Contributor
Author
There was a problem hiding this comment.
I think clearing the data fields makes sense since if you don't have tokens, you would get logged out and hence, you wouldn't use the user data anyway
Member
There was a problem hiding this comment.
I think it makes sense to clear data fields, mainly just a minor naming mismatch with clearTokens.
| ) : 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.
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
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
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt (2)
104-108:⚠️ Potential issue | 🟡 MinorSame string interpolation bug as above.
'$userInfo.id'should be'${userInfo.id}'to correctly log the ID value.🐛 Proposed fix
val id = userInfo.id.toIntOrNull() if (id == null) { - Log.e("UserInfoRepository", "Failed to log in: non-numeric user ID '$userInfo.id'") + 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, In UserInfoRepository (around the login path), the Log.e call incorrectly interpolates userInfo.id as '$userInfo.id' which logs the literal string; change the interpolation to use '${userInfo.id}' so the actual ID value is logged (update the Log.e message that currently reads "Failed to log in: non-numeric user ID '$userInfo.id'" to use the corrected '${userInfo.id}' form).
51-55:⚠️ Potential issue | 🟡 MinorString interpolation bug: logs object reference instead of property value.
'$userFields.id'interpolatesuserFields.toString()followed by the literal string.id, not the actual ID value. This was flagged in a previous review but the fix is incorrect.🐛 Proposed fix
val id = userFields.id.toIntOrNull() if (id == null) { - Log.e("UserInfoRepository", "Failed to set goal: non-numeric user ID '$userFields.id'") + 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.e call is interpolating the object reference instead of the actual ID string ("'$userFields.id'"); update the log to include the real value by using the ID variable or Kotlin string template syntax (e.g., ${userFields.id} or $id) so Log.e("UserInfoRepository", "Failed to set goal: non-numeric user ID '${userFields.id}'") prints the actual value; change the string in the Log.e invocation near userFields.id/id in UserInfoRepository.kt accordingly.
🧹 Nitpick comments (3)
app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.kt (1)
105-106: Avoid redundant state collection.
rootNavigationUiStateis already collected at line 80. UserootNavigationUiState.isLoggedIninstead of callingcollectUiStateValue()again.♻️ Proposed fix
- val isLoggedIn = rootNavigationViewModel.collectUiStateValue().isLoggedIn + val isLoggedIn = rootNavigationUiState.isLoggedIn🤖 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 105 - 106, The code redundantly re-collects navigation state by calling collectUiStateValue() again to set isLoggedIn; instead use the already-collected rootNavigationUiState and read its isLoggedIn property. Replace the usage that assigns isLoggedIn from collectUiStateValue() with rootNavigationUiState.isLoggedIn so only the previously-collected state (rootNavigationUiState) is referenced and avoid duplicate state collection.app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt (1)
127-131: Misleading log message:idisInt?, not a string that failed parsing.The error message says "non-numeric user ID" but
idis already typed asInt?. Whenidisnull, the log will always show'null'. Consider a clearer message:✏️ Suggested improvement
suspend fun uploadGoal(id:Int?, goal: Int, manualToken: String? = null): Boolean { if (id == null) { - Log.e("UserInfoRepository", "Failed to set goal: non-numeric user ID '$id'") + Log.e("UserInfoRepository", "Failed to set goal: user ID is null") 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 127 - 131, The log message in UserInfoRepository.uploadGoal incorrectly implies a parsing error ("non-numeric user ID") even though id is an Int?; change the error text to clearly state the ID is missing/null (e.g., "Failed to set goal: user ID is null") and still include the id value for context, so update the Log.e call in uploadGoal to a clear, accurate message referencing the id variable.app/src/main/java/com/cornellappdev/uplift/di/AppModule.kt (1)
52-63: Consider adding timeouts to the main OkHttpClient.The refresh client has explicit timeouts, but the main client does not. Without timeouts, requests could hang indefinitely if the backend becomes unresponsive, which would also block the
runBlockingcall inTokenAuthenticator.♻️ Proposed fix
`@Provides` `@Singleton` `@Named`("main") fun provideOkHttpClient( authInterceptor: AuthInterceptor, tokenAuthenticator: TokenAuthenticator ): OkHttpClient { return OkHttpClient.Builder() .addInterceptor(authInterceptor) .authenticator(tokenAuthenticator) + .connectTimeout(30, TimeUnit.SECONDS) + .readTimeout(30, 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 52 - 63, The main OkHttpClient provided by provideOkHttpClient (named "main") lacks timeouts, so requests can hang and block TokenAuthenticator's runBlocking; update provideOkHttpClient to set appropriate connectTimeout, readTimeout, and writeTimeout (and optionally callTimeout) on the OkHttpClient.Builder before build(), keeping the existing .addInterceptor(authInterceptor) and .authenticator(tokenAuthenticator) configuration so the main client will fail fast when backend is unresponsive.
🤖 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/TokenAuthenticator.kt`:
- Around line 15-19: TokenAuthenticator uses synchronized(this) to guard token
refresh but is not annotated `@Singleton`, so multiple instances can bypass the
lock; annotate the TokenAuthenticator class with `@Singleton` (and add the
javax.inject.Singleton import) so Hilt provides a single instance across the
app, preserving the synchronized(this) protection for methods interacting with
TokenManager/SessionManager/apolloClient used in authenticate().
- Around line 76-83: The responseCount function fails to advance through the
Response chain causing an infinite loop or incorrect count; fix it by iterating
the traversal variable (response) inside the while loop (e.g., set response =
response.priorResponse each iteration) or rewrite to loop while current != null
and increment a counter, ensuring responseCount correctly returns the number of
prior responses used by the retry limiting logic in TokenAuthenticator.
In `@app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.kt`:
- Around line 83-84: Remove the unused local variable by deleting the line that
defines "val sessionManager = rootNavigationViewModel.sessionManager"; if logout
functionality is intended instead of removing, replace the unused extraction
with an explicit call such as "rootNavigationViewModel.sessionManager.logout()"
(or invoke the appropriate method on sessionManager) and ensure any necessary
imports are present.
- Around line 107-113: LaunchedEffect currently triggers navigation on initial
composition when isLoggedIn is false, duplicating the NavHost's startDestination
behavior; to fix, track the previous login state using a rememberSaveable var
(e.g., prevIsLoggedIn) and only call
navController.navigate(UpliftRootRoute.Onboarding) inside LaunchedEffect when
the state transitions from logged-in to logged-out (prevIsLoggedIn == true &&
!isLoggedIn) or otherwise skip the initial-run navigation; update prevIsLoggedIn
after handling so future logout transitions still navigate, and keep
ONBOARDING_FLAG checks intact.
---
Duplicate comments:
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt`:
- Around line 104-108: In UserInfoRepository (around the login path), the Log.e
call incorrectly interpolates userInfo.id as '$userInfo.id' which logs the
literal string; change the interpolation to use '${userInfo.id}' so the actual
ID value is logged (update the Log.e message that currently reads "Failed to log
in: non-numeric user ID '$userInfo.id'" to use the corrected '${userInfo.id}'
form).
- Around line 51-55: The Log.e call is interpolating the object reference
instead of the actual ID string ("'$userFields.id'"); update the log to include
the real value by using the ID variable or Kotlin string template syntax (e.g.,
${userFields.id} or $id) so Log.e("UserInfoRepository", "Failed to set goal:
non-numeric user ID '${userFields.id}'") prints the actual value; change the
string in the Log.e invocation near userFields.id/id in UserInfoRepository.kt
accordingly.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt`:
- Around line 127-131: The log message in UserInfoRepository.uploadGoal
incorrectly implies a parsing error ("non-numeric user ID") even though id is an
Int?; change the error text to clearly state the ID is missing/null (e.g.,
"Failed to set goal: user ID is null") and still include the id value for
context, so update the Log.e call in uploadGoal to a clear, accurate message
referencing the id variable.
In `@app/src/main/java/com/cornellappdev/uplift/di/AppModule.kt`:
- Around line 52-63: The main OkHttpClient provided by provideOkHttpClient
(named "main") lacks timeouts, so requests can hang and block
TokenAuthenticator's runBlocking; update provideOkHttpClient to set appropriate
connectTimeout, readTimeout, and writeTimeout (and optionally callTimeout) on
the OkHttpClient.Builder before build(), keeping the existing
.addInterceptor(authInterceptor) and .authenticator(tokenAuthenticator)
configuration so the main client will fail fast when backend is unresponsive.
In `@app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.kt`:
- Around line 105-106: The code redundantly re-collects navigation state by
calling collectUiStateValue() again to set isLoggedIn; instead use the
already-collected rootNavigationUiState and read its isLoggedIn property.
Replace the usage that assigns isLoggedIn from collectUiStateValue() with
rootNavigationUiState.isLoggedIn so only the previously-collected state
(rootNavigationUiState) is referenced and avoid duplicate state collection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d55ebb5-59a5-480c-9cc2-7f51747fd50c
📒 Files selected for processing (7)
app/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 as they are similar to previous changes (2)
- app/src/main/java/com/cornellappdev/uplift/data/auth/SessionManager.kt
- app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt
| ) | ||
| .execute() | ||
| if (manualToken != null) { | ||
| call.addHttpHeader("Authorization", "Bearer $manualToken") |
Member
There was a problem hiding this comment.
Is this needed anymore if we have the interceptor?
Contributor
Author
There was a problem hiding this comment.
yes, because if you start the session after we try and update the goal, it won't work since we need the access tokens stored first
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 and created an application scope for project [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