refactor: replace sharedPrefs with data store. #7
Conversation
ardevd
commented
Dec 1, 2025
ardevd
commented
- disk io on background thread.
- improved data consistency
- reactive
- disk io on background thread. - improved data consistency - reactive
There was a problem hiding this comment.
Pull request overview
This PR refactors authentication storage from SharedPreferences to DataStore Preferences, introducing reactive data access patterns with Kotlin Flow while maintaining backward compatibility through blocking methods for legacy call sites.
- Migrates
TokenManagerto use DataStore with Flow-based API for reactive data access - Updates
MainActivityandLoginFragmentto use coroutines for async DataStore operations - Adds blocking wrapper methods for synchronous access in network interceptors
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Adds DataStore Preferences library dependency declaration (version 1.0.0) |
| app/build.gradle.kts | Includes DataStore Preferences library in project dependencies |
| app/src/main/java/net/ardevd/tagius/core/data/TokenManager.kt | Replaces SharedPreferences with DataStore, introduces Flow-based API and blocking wrapper methods |
| app/src/main/java/net/ardevd/tagius/features/auth/ui/LoginFragment.kt | Wraps credential save operation in coroutine to support async DataStore writes |
| app/src/main/java/net/ardevd/tagius/MainActivity.kt | Uses Flow to reactively check authentication state during activity creation |
| app/src/main/java/net/ardevd/tagius/core/network/RetrofitClient.kt | Switches to blocking server URL retrieval for synchronous Retrofit initialization |
| app/src/main/java/net/ardevd/tagius/core/network/AuthInterceptor.kt | Switches to blocking token retrieval for synchronous request interception |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val authInterceptor = AuthInterceptor(tokenManager) | ||
|
|
||
| val baseUrl = tokenManager.getServerUrl() | ||
| val baseUrl = tokenManager.getServerUrlBlocking() |
There was a problem hiding this comment.
Using runBlocking in buildApiService() can potentially block the calling thread. If getInstance() is called from the main thread (which is common during initialization), this will cause disk I/O on the main thread, negating the benefits of DataStore's background thread operations.
Consider restructuring the initialization to use coroutines, or implement a lazy initialization pattern that doesn't require synchronous access.
| import android.content.SharedPreferences | ||
| import androidx.core.content.edit |
There was a problem hiding this comment.
The imports for SharedPreferences and androidx.core.content.edit are no longer used after migrating to DataStore and should be removed.
| import android.content.SharedPreferences | |
| import androidx.core.content.edit |
| lifecycleScope.launch { | ||
| val token = tokenManager.authTokenFlow.first() | ||
| if (savedInstanceState == null) { | ||
| if (!token.isNullOrBlank()) { | ||
| supportFragmentManager.beginTransaction() | ||
| .replace(R.id.fragment_container, RecordsListFragment()) | ||
| .commit() | ||
| } else { | ||
| // Not logged in? Go to Login | ||
| binding.topAppBar.isVisible = false | ||
|
|
||
| supportFragmentManager.beginTransaction() | ||
| .replace(R.id.fragment_container, LoginFragment()) | ||
| .commit() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The coroutine launched in onCreate is not guaranteed to complete before the activity starts. This creates a potential race condition where the UI could be in an inconsistent state if the activity lifecycle progresses before fragment transactions complete.
Consider using lifecycleScope.launch with Dispatchers.Main.immediate or restructure to ensure the initial fragment is set synchronously, then update based on auth state.
| viewLifecycleOwner.lifecycleScope.launch { | ||
| TokenManager(requireContext()).saveConnectionDetails(url, token) | ||
| RetrofitClient.reset() | ||
|
|
||
| parentFragmentManager.beginTransaction() | ||
| .replace(R.id.fragment_container, RecordsListFragment()) | ||
| .commit() | ||
| parentFragmentManager.beginTransaction() | ||
| .replace(R.id.fragment_container, RecordsListFragment()) | ||
| .commit() | ||
|
|
||
| requireActivity().findViewById<View>(R.id.topAppBar).isVisible = true | ||
| requireActivity().findViewById<View>(R.id.topAppBar).isVisible = true | ||
| } |
There was a problem hiding this comment.
Fragment transactions and view updates (lines 109-113) are executed inside a coroutine without ensuring they run on the main thread. While lifecycleScope defaults to Dispatchers.Main, explicitly using withContext(Dispatchers.Main) for UI operations would make the threading requirements clearer and more maintainable.
Additionally, there's no error handling for the saveConnectionDetails call. If DataStore write fails, the user would be navigated to the next screen despite credentials not being saved.
| fun getToken(): String? { | ||
| return prefs.getString(KEY_AUTH_TOKEN, "") | ||
| fun getServerUrlBlocking(): String = runBlocking { | ||
| serverUrlFlow.first() |
There was a problem hiding this comment.
The getServerUrlBlocking() method returns only the base URL without the API path suffix /timetagger/api/v2/. The old implementation appended this path (line 35 in diff: return baseUrl + "timetagger/api/v2/"), but the new implementation only returns the stored URL. This will cause API calls to fail since they'll be directed to the wrong endpoints.
Either restore the API path suffix in this method, or ensure the full path is stored when saveConnectionDetails is called.
| serverUrlFlow.first() | |
| val baseUrl = serverUrlFlow.first() | |
| (if (baseUrl.endsWith("/")) baseUrl else "$baseUrl/") + "timetagger/api/v2/" |
| val originalRequest = chain.request() | ||
|
|
||
| val token = tokenManager.getToken() | ||
| val token = tokenManager.getTokenBlocking() |
There was a problem hiding this comment.
Using runBlocking inside intercept() will block the OkHttp thread pool. Since intercept() is called on OkHttp's background threads during network requests, this introduces unnecessary blocking that defeats the purpose of migrating to DataStore.
Consider caching the token value in memory and refreshing it when needed, or restructure the code to avoid needing synchronous access in the interceptor.
| val token = tokenManager.getTokenBlocking() | |
| val token = tokenManager.cachedToken |
