Draft: implementation client auth and jwt#3
Open
ParovozikThomas wants to merge 2 commits intomainfrom
Open
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces authentication infrastructure for the mobile app, including Retrofit-based network API endpoints for registration and login, DataStore-based local token persistence via TokenStorage, an AuthRepository coordinating API calls and storage operations, and UI screens for login and registration with associated ViewModels and navigation routing. Changes
Sequence DiagramssequenceDiagram
participant User
participant LoginScreen
participant LoginViewModel
participant AuthRepository
participant AuthApi
participant TokenStorage
participant NavController
User->>LoginScreen: Enters credentials & taps Login
LoginScreen->>LoginViewModel: onLoginClicked()
LoginViewModel->>LoginViewModel: Validate inputs
LoginViewModel->>AuthRepository: login(email, password)
AuthRepository->>AuthApi: login(LoginRequest)
AuthApi->>AuthApi: POST /api/auth/login
AuthApi-->>AuthRepository: LoginResponse (tokens, expiries)
AuthRepository->>TokenStorage: saveToken(accessToken, refreshToken, expiredAt, refreshExpiredAt)
TokenStorage->>TokenStorage: Persist to DataStore
AuthRepository-->>LoginViewModel: Result.Success
LoginViewModel->>LoginViewModel: Emit NavigateToHome event
LoginScreen->>NavController: Navigate to Home
NavController-->>User: Display Home Screen
sequenceDiagram
participant User
participant RegisterScreen
participant RegisterViewModel
participant AuthRepository
participant AuthApi
participant TokenStorage
participant NavController
User->>RegisterScreen: Enters registration data & selects role
RegisterScreen->>RegisterViewModel: onRegisterClicked()
RegisterViewModel->>RegisterViewModel: Validate email, password, username, role
RegisterViewModel->>AuthRepository: register(email, password, username, role)
AuthRepository->>AuthApi: register(RegisterRequest)
AuthApi->>AuthApi: POST /api/auth/register
AuthApi-->>AuthRepository: LoginResponse (tokens, expiries)
AuthRepository->>TokenStorage: saveToken(...)
TokenStorage->>TokenStorage: Persist to DataStore
AuthRepository-->>RegisterViewModel: Result.Success
RegisterViewModel->>RegisterViewModel: Emit NavigateToHome event
RegisterScreen->>NavController: Navigate to Home
NavController-->>User: Display Home Screen
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mobile/app/build.gradle.kts`:
- Around line 46-65: Dependencies are using dynamic '+' selectors which cause
non-reproducible builds; replace each dynamic version with a fixed explicit
version string for the following coordinates:
androidx.navigation:navigation-compose (replace $nav_version+ with a pinned
nav_version value or explicit version), com.squareup.retrofit2:retrofit,
com.squareup.okhttp3:okhttp, com.squareup.retrofit2:converter-gson,
com.squareup.okhttp3:logging-interceptor,
androidx.datastore:datastore-preferences,
org.jetbrains.kotlinx:kotlinx-coroutines-android, and
androidx.lifecycle:lifecycle-viewmodel-compose — update the build.gradle.kts
entries for these artifacts to remove the '+' wildcard and use concrete versions
(or a locked version variable) so builds are deterministic.
In `@mobile/app/src/main/java/com/smartjam/app/data/api/AuthApi.kt`:
- Around line 12-16: The two Retrofit endpoints in AuthApi (the `@POST`
annotations on the login and refresh functions) use inconsistent path styles;
make them consistent by updating the `@POST` values so both use the same
convention (either both start with "/" or both omit it)—e.g., change
`@POST`("/api/auth/login") to `@POST`("api/auth/login") or change
`@POST`("api/auth/refresh") to `@POST`("/api/auth/refresh")—ensuring the login and
refresh annotations match.
In `@mobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.kt`:
- Line 12: The BASE_URL constant is malformed (uses "http:/" instead of
"http://"); update the constant named BASE_URL in NetworkModule.kt to use a
properly formatted URL with the HTTP scheme (e.g., "http://10.0.2.2:8000/") so
Retrofit can construct requests correctly.
- Around line 33-35: The HttpLoggingInterceptor is currently set to Level.BODY
unconditionally; change it to enable detailed logging only in debug builds by
wrapping the level assignment with a BuildConfig.DEBUG check (e.g., if
(BuildConfig.DEBUG) { logging.level = Level.BODY } else { logging.level =
Level.NONE }) and redact the Authorization header on the interceptor (call
logging.redactHeader("Authorization")) before adding it to the OkHttpClient in
NetworkModule (references: HttpLoggingInterceptor, level, redactHeader,
BuildConfig.DEBUG, Authorization).
In `@mobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.kt`:
- Around line 27-33: The saveToken suspend function currently writes JWTs
directly to DataStore keys (ACCESS_TOKEN, REFRESH_TOKEN, ACCESS_EXPIRED_AT,
REFRESH_EXPIRED_AT); change it to encrypt tokens before writing and decrypt on
read using a secure Android-backed key (e.g., Android
Keystore/EncryptedSharedPreferences or an encryption helper using MasterKey), so
modify saveToken and corresponding read methods to call the encrypt/decrypt
helper around ACCESS_TOKEN and REFRESH_TOKEN while leaving timestamps as-is;
ensure keys and helper are initialized once and handle failures gracefully
(throw or clear tokens) in TokenStorage.kt.
- Around line 62-71: The expiry check in isRefreshTokenExpired() currently uses
(currentTime > expires) which treats the token as valid during the exact expiry
second; change the comparison to (currentTime >= expires) so the token is
considered expired at the boundary. Locate the suspend fun
isRefreshTokenExpired() and update the final return expression to use >=; also
scan the analogous function isAccessTokenExpired() and apply the same >=
boundary check for consistency.
- Line 17: The TokenStorage class currently stores an Activity Context; change
the constructor to capture an application Context (e.g. val appContext =
context.applicationContext) and replace every use of context.dataStore with
appContext.dataStore (affecting saveToken(), the Flow property initializers,
clearTokens(), isAccessTokenExpired(), and isRefreshTokenExpired()). Also update
the expiry checks in isAccessTokenExpired() and isRefreshTokenExpired() to use
currentTime >= expires for a conservative expiration comparison. Ensure all
references use the new appContext variable.
In `@mobile/app/src/main/java/com/smartjam/app/data/model/LoginResponce.kt`:
- Line 3: The data class name LoginResponce is misspelled; rename the type to
LoginResponse and update all references (constructor calls, imports,
serialization annotations, DAO/DTO usages) to the new symbol to avoid breaking
changes—specifically change the class declaration for LoginResponce to
LoginResponse and update usages across the codebase (e.g., any places
referencing LoginResponce, JSON mappers, Room/Parcel implementations) to refer
to LoginResponse so the public API is consistent.
In `@mobile/app/src/main/java/com/smartjam/app/data/model/LoginState.kt`:
- Around line 3-7: LoginState currently mixes domain transport fields and UI
concerns; change it to a pure data/credentials model by removing isLoading and
error (keep email and password, optionally rename to AuthCredentials or
Credentials), then introduce a separate UI-state class in the UI/ViewModel layer
(e.g., LoginViewState or ViewModel-specific state) that holds isLoading and
error. Update all usages that read/write isLoading/error to reference the new
UI-state in the ViewModel and adjust any serialization or mapping code to
convert between LoginState/AuthCredentials and the ViewModel UI state.
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt`:
- Around line 29-30: The catch blocks in the suspend functions login and
refreshToken are catching broad Exception (which includes CancellationException)
and thus converting or handling cancellations incorrectly; update both functions
(login(...) and refreshToken(...)) to first catch and rethrow
CancellationException (e.g., catch (e: CancellationException) { throw e })
before the generic catch, and in refreshToken ensure clearTokens() is only
invoked for non-cancellation errors (i.e., placed inside the generic Exception
catch block or guarded so cancellation does not trigger clearTokens()); also
ensure login does not wrap CancellationException in Result.failure but lets it
propagate.
In `@mobile/app/src/main/java/com/smartjam/app/MainActivity.kt`:
- Around line 39-45: The callbacks onNavigateToHome and onNavigateToRegister
currently only print logs and must perform real navigation; replace the printlns
with calls that trigger the app's navigation flow (e.g., use the NavController
navigate() to go to the home destination or registration screen or call the
activity's startActivity/finish as appropriate). Locate the usages of
onNavigateToHome and onNavigateToRegister in MainActivity (inside the composable
or view setup) and invoke the appropriate navigation method
(navController.navigate("home") or navController.navigate("register") or
startActivity(intentForRegister)) and ensure any required parameters
(navController instance or Context) are available/passed into the lambda.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginScreen.kt`:
- Around line 168-170: The Google auth button currently wired as
AppleGlassButton with a no-op onClick should not be clickable; update the
AppleGlassButton usage in LoginScreen.kt (the AppleGlassButton instance for
"Google") to either render as disabled (set its enabled/disabled state or use a
disabled variant) or replace its onClick with a callback that surfaces a clear
“Coming soon” feedback (e.g., trigger a Toast/snackbar event or call
showComingSoon()) so users get immediate feedback instead of a silent no-op;
ensure the change targets the AppleGlassButton instantiation and its onClick
handler so accessibility/state reflects the non-functional auth option.
- Around line 49-52: The LoginScreen composable should not use an unsafe default
viewModel() parameter because LoginViewModel requires AuthRepository; remove the
default in the LoginScreen signature (change "viewModel: LoginViewModel =
viewModel()" to a required parameter "viewModel: LoginViewModel") and update all
call sites (including the preview that calls LoginScreen) to pass an explicit
LoginViewModel (use LoginViewModelFactory in production and a test/dummy
LoginViewModel or mocked repository for the preview), or alternatively provide a
safe factory-aware default by calling viewModel(factory =
LoginViewModelFactory(...)) inside the composable if you supply a valid
AuthRepository there—update LoginScreen, the preview, and any callers
accordingly (refer to LoginScreen, LoginViewModel, and LoginViewModelFactory).
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.kt`:
- Around line 53-56: The onLoginClicked handler currently allows concurrent
submissions; prevent parallel login requests by adding a guard (e.g., an
isLoading flag in _state or a private nullable Job loginJob) and early-return if
a login is already in progress. In onLoginClicked check _state.value.isLoading
(or loginJob?.isActive) and return immediately if true; when starting the
coroutine via viewModelScope.launch set isLoading = true (or assign loginJob)
and ensure in a finally block you set isLoading = false (or clear loginJob) so
the button and state are correctly re-enabled and race conditions are avoided.
- Around line 57-60: The validation branch sets _state.value =
_state.value.copy(errorMessage = "Fill in all fields") when currentPassword or
currentEmail is blank but does not stop execution, so viewModelScope.launch
still runs; update the login method in LoginViewModel to return immediately
after setting the error (e.g., add an early return) so that
viewModelScope.launch is only entered when inputs are valid, ensuring symbols
involved are currentPassword, currentEmail, _state, and viewModelScope.launch.
- Around line 63-71: authRepository.login(...) returns a Result<Unit> and you
must check it before sending LoginEvent.NavigateToHome; update LoginViewModel to
capture the Result from authRepository.login(currentEmail, currentPassword),
call result.onSuccess { eventChannel.send(LoginEvent.NavigateToHome) } and
result.onFailure { _state.value = _state.value.copy(errorMessage = it.message ?:
"Unknown error") }, instead of relying on the try/catch to catch thrown
exceptions; ensure you reference authRepository.login,
LoginEvent.NavigateToHome, and _state when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98d59073-94b5-43df-bdbd-def9a288895c
📒 Files selected for processing (13)
mobile/app/build.gradle.ktsmobile/app/src/main/java/com/smartjam/app/MainActivity.ktmobile/app/src/main/java/com/smartjam/app/data/api/AuthApi.ktmobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.ktmobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.ktmobile/app/src/main/java/com/smartjam/app/data/model/LoginRequest.ktmobile/app/src/main/java/com/smartjam/app/data/model/LoginResponce.ktmobile/app/src/main/java/com/smartjam/app/data/model/LoginState.ktmobile/app/src/main/java/com/smartjam/app/data/model/RefreshRequest.ktmobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.ktmobile/app/src/main/java/com/smartjam/app/ui/navigation/NavGraph.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginScreen.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.kt
Comment on lines
+46
to
+65
| implementation("androidx.navigation:navigation-compose:$nav_version+") | ||
|
|
||
| //network | ||
| implementation("com.squareup.retrofit2:retrofit:2.11.+") | ||
| implementation("com.squareup.okhttp3:okhttp:4.12.+") | ||
|
|
||
| //serialization | ||
| implementation("com.squareup.retrofit2:converter-gson:2.11.+") | ||
|
|
||
| //logging | ||
| implementation("com.squareup.okhttp3:logging-interceptor:4.12.+") | ||
|
|
||
| //database | ||
| implementation("androidx.datastore:datastore-preferences:1.1.+") | ||
|
|
||
| //coroutines | ||
| implementation("org.jetbrains.kotlinx:kotlinx-coroutines-android:1.9.+") | ||
|
|
||
| //ne pon | ||
| implementation("androidx.lifecycle:lifecycle-viewmodel-compose:2.8.+") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant lines
cat -n mobile/app/build.gradle.kts | sed -n '40,70p'Repository: Satlykovs/SmartJam
Length of output: 1219
🏁 Script executed:
# Search for all dynamic selectors in the build.gradle.kts file
rg -n '\+\"' mobile/app/build.gradle.ktsRepository: Satlykovs/SmartJam
Length of output: 134
🏁 Script executed:
# Correct regex to find all dynamic selectors (escape the + properly)
rg -n 'implementation.*\+' mobile/app/build.gradle.ktsRepository: Satlykovs/SmartJam
Length of output: 636
🏁 Script executed:
# Also verify the suggested versions exist by checking if they're reasonable
# Let's look at the complete dependencies block to understand context
cat -n mobile/app/build.gradle.kts | sed -n '42,100p'Repository: Satlykovs/SmartJam
Length of output: 1902
Pin all dependency versions; remove dynamic + selectors.
Lines 46, 49, 50, 53, 56, 59, 62, and 65 use dynamic version selectors (+), which creates non-reproducible builds and risks introducing untested behavior in networking/auth code paths.
Suggested change
- implementation("androidx.navigation:navigation-compose:$nav_version+")
+ implementation("androidx.navigation:navigation-compose:$nav_version")
- implementation("com.squareup.retrofit2:retrofit:2.11.+")
- implementation("com.squareup.okhttp3:okhttp:4.12.+")
+ implementation("com.squareup.retrofit2:retrofit:2.11.0")
+ implementation("com.squareup.okhttp3:okhttp:4.12.0")
- implementation("com.squareup.retrofit2:converter-gson:2.11.+")
+ implementation("com.squareup.retrofit2:converter-gson:2.11.0")
- implementation("com.squareup.okhttp3:logging-interceptor:4.12.+")
+ implementation("com.squareup.okhttp3:logging-interceptor:4.12.0")
- implementation("androidx.datastore:datastore-preferences:1.1.+")
+ implementation("androidx.datastore:datastore-preferences:1.1.1")
- implementation("org.jetbrains.kotlinx:kotlinx-coroutines-android:1.9.+")
+ implementation("org.jetbrains.kotlinx:kotlinx-coroutines-android:1.9.0")
- implementation("androidx.lifecycle:lifecycle-viewmodel-compose:2.8.+")
+ implementation("androidx.lifecycle:lifecycle-viewmodel-compose:2.8.7")📝 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
| implementation("androidx.navigation:navigation-compose:$nav_version+") | |
| //network | |
| implementation("com.squareup.retrofit2:retrofit:2.11.+") | |
| implementation("com.squareup.okhttp3:okhttp:4.12.+") | |
| //serialization | |
| implementation("com.squareup.retrofit2:converter-gson:2.11.+") | |
| //logging | |
| implementation("com.squareup.okhttp3:logging-interceptor:4.12.+") | |
| //database | |
| implementation("androidx.datastore:datastore-preferences:1.1.+") | |
| //coroutines | |
| implementation("org.jetbrains.kotlinx:kotlinx-coroutines-android:1.9.+") | |
| //ne pon | |
| implementation("androidx.lifecycle:lifecycle-viewmodel-compose:2.8.+") | |
| implementation("androidx.navigation:navigation-compose:$nav_version") | |
| //network | |
| implementation("com.squareup.retrofit2:retrofit:2.11.0") | |
| implementation("com.squareup.okhttp3:okhttp:4.12.0") | |
| //serialization | |
| implementation("com.squareup.retrofit2:converter-gson:2.11.0") | |
| //logging | |
| implementation("com.squareup.okhttp3:logging-interceptor:4.12.0") | |
| //database | |
| implementation("androidx.datastore:datastore-preferences:1.1.1") | |
| //coroutines | |
| implementation("org.jetbrains.kotlinx:kotlinx-coroutines-android:1.9.0") | |
| //ne pon | |
| implementation("androidx.lifecycle:lifecycle-viewmodel-compose:2.8.7") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/build.gradle.kts` around lines 46 - 65, Dependencies are using
dynamic '+' selectors which cause non-reproducible builds; replace each dynamic
version with a fixed explicit version string for the following coordinates:
androidx.navigation:navigation-compose (replace $nav_version+ with a pinned
nav_version value or explicit version), com.squareup.retrofit2:retrofit,
com.squareup.okhttp3:okhttp, com.squareup.retrofit2:converter-gson,
com.squareup.okhttp3:logging-interceptor,
androidx.datastore:datastore-preferences,
org.jetbrains.kotlinx:kotlinx-coroutines-android, and
androidx.lifecycle:lifecycle-viewmodel-compose — update the build.gradle.kts
entries for these artifacts to remove the '+' wildcard and use concrete versions
(or a locked version variable) so builds are deterministic.
Comment on lines
+12
to
+16
| @POST("/api/auth/login") | ||
| suspend fun login(@Body request: LoginRequest): LoginResponce | ||
|
|
||
| @POST("api/auth/refresh") | ||
| suspend fun refresh(@Body request: RefreshRequest): LoginResponce |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n mobile/app/src/main/java/com/smartjam/app/data/api/AuthApi.ktRepository: Satlykovs/SmartJam
Length of output: 637
🏁 Script executed:
fd -t f -e "kt" mobile/app/src/main/java/com/smartjam/app/data/api/ -exec grep -l "@POST" {} \;Repository: Satlykovs/SmartJam
Length of output: 302
🏁 Script executed:
fd -t f -e "kt" "mobile/app/src/main/java/com/smartjam/app/data/api/"Repository: Satlykovs/SmartJam
Length of output: 571
🏁 Script executed:
fd . 'mobile/app/src/main/java/com/smartjam/app/data/api/'Repository: Satlykovs/SmartJam
Length of output: 191
🏁 Script executed:
cat -n mobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.ktRepository: Satlykovs/SmartJam
Length of output: 1353
Normalize Retrofit endpoint paths (leading slash consistency).
Line 12 uses absolute path (/api/auth/login) while line 15 uses relative path (api/auth/refresh). Mixing styles is error-prone—choose one convention for all endpoints to avoid subtle bugs if the base URL configuration changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/data/api/AuthApi.kt` around lines
12 - 16, The two Retrofit endpoints in AuthApi (the `@POST` annotations on the
login and refresh functions) use inconsistent path styles; make them consistent
by updating the `@POST` values so both use the same convention (either both start
with "/" or both omit it)—e.g., change `@POST`("/api/auth/login") to
`@POST`("api/auth/login") or change `@POST`("api/auth/refresh") to
`@POST`("/api/auth/refresh")—ensuring the login and refresh annotations match.
|
|
||
|
|
||
| object NetworkModule { | ||
| private const val BASE_URL = "http:/10.0.2.2:8000/" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the file exists and read the relevant content
if [ -f "mobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.kt" ]; then
echo "=== File content around line 12 ==="
sed -n '10,15p' "mobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.kt"
echo ""
echo "=== Extracting BASE_URL ==="
grep -n "BASE_URL" "mobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.kt"
else
echo "File not found"
fiRepository: Satlykovs/SmartJam
Length of output: 362
Fix malformed BASE_URL (http:/ → http://).
Line 12 contains an invalid URL format with only a single slash after the scheme. Retrofit requires properly formatted URLs with :// for the HTTP scheme, and this malformed URL will cause request construction to fail.
Suggested change
- private const val BASE_URL = "http:/10.0.2.2:8000/"
+ private const val BASE_URL = "http://10.0.2.2:8000/"📝 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
| private const val BASE_URL = "http:/10.0.2.2:8000/" | |
| private const val BASE_URL = "http://10.0.2.2:8000/" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.kt` at line
12, The BASE_URL constant is malformed (uses "http:/" instead of "http://");
update the constant named BASE_URL in NetworkModule.kt to use a properly
formatted URL with the HTTP scheme (e.g., "http://10.0.2.2:8000/") so Retrofit
can construct requests correctly.
Comment on lines
+33
to
+35
| HttpLoggingInterceptor().apply { | ||
| level = HttpLoggingInterceptor.Level.BODY | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'HttpLoggingInterceptor|Level\.BODY|BuildConfig\.DEBUG|redactHeader' mobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.ktRepository: Satlykovs/SmartJam
Length of output: 210
Do not enable HttpLoggingInterceptor.Level.BODY unconditionally.
This can leak credentials and auth tokens into logs. Restrict logging to debug builds and redact sensitive headers like Authorization.
Suggested fix
private val okHttpClient by lazy {
OkHttpClient.Builder()
- .addInterceptor(loggingInterceptor)
+ .apply {
+ if (BuildConfig.DEBUG) addInterceptor(loggingInterceptor)
+ }
.connectTimeout(30, TimeUnit.SECONDS)
.readTimeout(30, TimeUnit.SECONDS)
.writeTimeout(30, TimeUnit.SECONDS)
.build()
}
private val loggingInterceptor by lazy {
HttpLoggingInterceptor().apply {
- level = HttpLoggingInterceptor.Level.BODY
+ level = if (BuildConfig.DEBUG) HttpLoggingInterceptor.Level.BODY
+ else HttpLoggingInterceptor.Level.NONE
+ redactHeader("Authorization")
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/data/api/NetworkModule.kt` around
lines 33 - 35, The HttpLoggingInterceptor is currently set to Level.BODY
unconditionally; change it to enable detailed logging only in debug builds by
wrapping the level assignment with a BuildConfig.DEBUG check (e.g., if
(BuildConfig.DEBUG) { logging.level = Level.BODY } else { logging.level =
Level.NONE }) and redact the Authorization header on the interceptor (call
logging.redactHeader("Authorization")) before adding it to the OkHttpClient in
NetworkModule (references: HttpLoggingInterceptor, level, redactHeader,
BuildConfig.DEBUG, Authorization).
| private val Context.dataStore : DataStore<Preferences> by preferencesDataStore( | ||
| name = "auth_preferences" | ||
| ) | ||
| class TokenStorage(private val context: Context) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the TokenStorage class and examine it
find . -name "TokenStorage.kt" -type f | head -20Repository: Satlykovs/SmartJam
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Verify TokenStorage Context retention and instantiation patterns
rg -nP 'class TokenStorage\(.*Context\)|TokenStorage\(context = this\)|TokenStorage\(this\)' mobile/app/src/main/java/com/smartjam/appRepository: Satlykovs/SmartJam
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Let's also search more broadly for TokenStorage instantiations
rg -n 'TokenStorage(' mobile/app/src/main/java/com/smartjam/app -A 2 -B 2Repository: Satlykovs/SmartJam
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Read the TokenStorage.kt file to see full implementation
cat -n mobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.ktRepository: Satlykovs/SmartJam
Length of output: 3914
🏁 Script executed:
#!/bin/bash
# Also check MainActivity to understand the instantiation context better
cat -n mobile/app/src/main/java/com/smartjam/app/MainActivity.kt | head -50Repository: Satlykovs/SmartJam
Length of output: 2103
Use applicationContext instead of storing Activity Context in TokenStorage.
Line 17 stores the passed Context directly. At line 26 in MainActivity, TokenStorage(context = this) retains an Activity reference that can outlive the Activity's lifecycle.
Apply context.applicationContext throughout the class—replace all 6 usages of context.dataStore (lines 28, 36, 39, 42, 45, 49, 58, 67) with appContext.dataStore:
Suggested changes
-class TokenStorage(private val context: Context) {
+class TokenStorage(context: Context) {
+ private val appContext = context.applicationContextReplace all occurrences of context.dataStore with appContext.dataStore in:
- Line 28:
saveToken()method - Lines 36, 39, 42, 45: Flow property initializers
- Line 49:
clearTokens()method - Lines 58, 67:
isAccessTokenExpired()andisRefreshTokenExpired()methods
Minor: Lines 62 and 71 use currentTime > expires—consider currentTime >= expires for conservative expiry (tokens valid only before expiry time, not through it).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.kt` at line
17, The TokenStorage class currently stores an Activity Context; change the
constructor to capture an application Context (e.g. val appContext =
context.applicationContext) and replace every use of context.dataStore with
appContext.dataStore (affecting saveToken(), the Flow property initializers,
clearTokens(), isAccessTokenExpired(), and isRefreshTokenExpired()). Also update
the expiry checks in isAccessTokenExpired() and isRefreshTokenExpired() to use
currentTime >= expires for a conservative expiration comparison. Ensure all
references use the new appContext variable.
mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginScreen.kt
Show resolved
Hide resolved
Comment on lines
+168
to
+170
| AppleGlassButton( | ||
| onClick = { /* TODO: Google Auth */ }, | ||
| text = "Google", |
There was a problem hiding this comment.
Avoid shipping a clickable no-op auth action.
Line 169 exposes a Google button that does nothing. Either hide/disable it for now or surface a clear “coming soon” feedback path.
I can draft a minimal fallback implementation (disabled state or toast/event path) and open a follow-up issue template if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginScreen.kt`
around lines 168 - 170, The Google auth button currently wired as
AppleGlassButton with a no-op onClick should not be clickable; update the
AppleGlassButton usage in LoginScreen.kt (the AppleGlassButton instance for
"Google") to either render as disabled (set its enabled/disabled state or use a
disabled variant) or replace its onClick with a callback that surfaces a clear
“Coming soon” feedback (e.g., trigger a Toast/snackbar event or call
showComingSoon()) so users get immediate feedback instead of a silent no-op;
ensure the change targets the AppleGlassButton instantiation and its onClick
handler so accessibility/state reflects the non-functional auth option.
Comment on lines
+53
to
+56
| fun onLoginClicked() { | ||
| val currentEmail = _state.value.emailInput | ||
| val currentPassword = _state.value.passwordInput | ||
|
|
There was a problem hiding this comment.
Guard against parallel login submissions.
Rapid repeated taps can launch concurrent login requests and race state/events.
🔧 Proposed fix
fun onLoginClicked() {
+ if (_state.value.isLoading) return
+
val currentEmail = _state.value.emailInput
val currentPassword = _state.value.passwordInputAlso applies to: 60-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.kt`
around lines 53 - 56, The onLoginClicked handler currently allows concurrent
submissions; prevent parallel login requests by adding a guard (e.g., an
isLoading flag in _state or a private nullable Job loginJob) and early-return if
a login is already in progress. In onLoginClicked check _state.value.isLoading
(or loginJob?.isActive) and return immediately if true; when starting the
coroutine via viewModelScope.launch set isLoading = true (or assign loginJob)
and ensure in a finally block you set isLoading = false (or clear loginJob) so
the button and state are correctly re-enabled and race conditions are avoided.
Comment on lines
+57
to
+60
| if (currentPassword.isBlank() || currentEmail.isBlank()){ | ||
| _state.value = _state.value.copy(errorMessage = "Fill in all fields") | ||
| } | ||
| viewModelScope.launch { |
There was a problem hiding this comment.
Return early after validation failure.
Line 58 sets an error, but execution continues into the login coroutine at Line 60.
🔧 Proposed fix
if (currentPassword.isBlank() || currentEmail.isBlank()){
_state.value = _state.value.copy(errorMessage = "Fill in all fields")
+ return
}
viewModelScope.launch {📝 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
| if (currentPassword.isBlank() || currentEmail.isBlank()){ | |
| _state.value = _state.value.copy(errorMessage = "Fill in all fields") | |
| } | |
| viewModelScope.launch { | |
| if (currentPassword.isBlank() || currentEmail.isBlank()){ | |
| _state.value = _state.value.copy(errorMessage = "Fill in all fields") | |
| return | |
| } | |
| viewModelScope.launch { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.kt`
around lines 57 - 60, The validation branch sets _state.value =
_state.value.copy(errorMessage = "Fill in all fields") when currentPassword or
currentEmail is blank but does not stop execution, so viewModelScope.launch
still runs; update the login method in LoginViewModel to return immediately
after setting the error (e.g., add an early return) so that
viewModelScope.launch is only entered when inputs are valid, ensuring symbols
involved are currentPassword, currentEmail, _state, and viewModelScope.launch.
Comment on lines
+63
to
+71
| try { | ||
| authRepository.login(currentEmail, currentPassword) | ||
|
|
||
| eventChannel.send(LoginEvent.NavigateToHome) | ||
| } catch (e: Exception){ | ||
| _state.value = _state.value.copy( | ||
| errorMessage = e.message?: "Unknown error" | ||
| ) | ||
| } finally { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the LoginViewModel.kt file
find . -type f -name "LoginViewModel.kt" | head -5Repository: Satlykovs/SmartJam
Length of output: 140
🏁 Script executed:
# Read the LoginViewModel.kt file to see the actual code at lines 63-71
wc -l mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.ktRepository: Satlykovs/SmartJam
Length of output: 141
🏁 Script executed:
# Get the code around lines 63-71
sed -n '50,85p' mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.ktRepository: Satlykovs/SmartJam
Length of output: 1139
🏁 Script executed:
# Search for authRepository definition and login method signature
rg -nP --type=kotlin 'fun login|authRepository.*=|class.*Repository' mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.ktRepository: Satlykovs/SmartJam
Length of output: 44
🏁 Script executed:
# Look for the Repository interface/class definition to check login return type
find . -type f -name "*Repository.kt" -o -name "*repository.kt" | grep -i authRepository: Satlykovs/SmartJam
Length of output: 141
🏁 Script executed:
# Read the AuthRepository.kt file to check the login method signature
cat -n ./mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.ktRepository: Satlykovs/SmartJam
Length of output: 3654
🏁 Script executed:
# Also search for any Result type imports or usage in LoginViewModel
rg -nP --type=kotlin 'import.*Result|sealed class Result' mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.ktRepository: Satlykovs/SmartJam
Length of output: 44
🏁 Script executed:
# Check what Result type is used in the repository
rg -nP --type=kotlin 'import.*Result|suspend fun login' ./mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.ktRepository: Satlykovs/SmartJam
Length of output: 135
Handle Result from repository before navigating.
authRepository.login() returns Result<Unit> (line 18 of AuthRepository.kt) but the return value is never checked before navigation. This means failed login attempts will still trigger LoginEvent.NavigateToHome unconditionally. The catch block at line 68 cannot handle Result.failure() since the repository wraps exceptions in Result rather than throwing them.
🔧 Proposed fix
- try {
- authRepository.login(currentEmail, currentPassword)
-
- eventChannel.send(LoginEvent.NavigateToHome)
- } catch (e: Exception){
- _state.value = _state.value.copy(
- errorMessage = e.message?: "Unknown error"
- )
- } finally {
+ try {
+ authRepository.login(currentEmail, currentPassword).fold(
+ onSuccess = {
+ eventChannel.send(LoginEvent.NavigateToHome)
+ },
+ onFailure = { e ->
+ _state.value = _state.value.copy(
+ errorMessage = e.message ?: "Unknown error"
+ )
+ }
+ )
+ } finally {
_state.value = _state.value.copy(isLoading = 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
| try { | |
| authRepository.login(currentEmail, currentPassword) | |
| eventChannel.send(LoginEvent.NavigateToHome) | |
| } catch (e: Exception){ | |
| _state.value = _state.value.copy( | |
| errorMessage = e.message?: "Unknown error" | |
| ) | |
| } finally { | |
| try { | |
| authRepository.login(currentEmail, currentPassword).fold( | |
| onSuccess = { | |
| eventChannel.send(LoginEvent.NavigateToHome) | |
| }, | |
| onFailure = { e -> | |
| _state.value = _state.value.copy( | |
| errorMessage = e.message ?: "Unknown error" | |
| ) | |
| } | |
| ) | |
| } finally { | |
| _state.value = _state.value.copy(isLoading = false) | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 67-67: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.kt`
around lines 63 - 71, authRepository.login(...) returns a Result<Unit> and you
must check it before sending LoginEvent.NavigateToHome; update LoginViewModel to
capture the Result from authRepository.login(currentEmail, currentPassword),
call result.onSuccess { eventChannel.send(LoginEvent.NavigateToHome) } and
result.onFailure { _state.value = _state.value.copy(errorMessage = it.message ?:
"Unknown error") }, instead of relying on the try/catch to catch thrown
exceptions; ensure you reference authRepository.login,
LoginEvent.NavigateToHome, and _state when making the change.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
mobile/app/src/main/java/com/smartjam/app/data/api/AuthApi.kt (1)
13-20:⚠️ Potential issue | 🟡 MinorUse one Retrofit path style for all auth endpoints.
register/loginuse absolute-style paths, butrefreshis relative. Retrofit resolves those differently against the base URL, so one endpoint can drift if the base path changes.Proposed fix
- `@POST`("api/auth/refresh") + `@POST`("/api/auth/refresh")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/app/src/main/java/com/smartjam/app/data/api/AuthApi.kt` around lines 13 - 20, The auth endpoints in AuthApi.kt use mixed Retrofit path styles—register and login use absolute paths ("/api/auth/register", "/api/auth/login") while refresh uses a relative path ("api/auth/refresh")—which can cause inconsistent URL resolution; update the refresh endpoint to use the same absolute-style path as the others (match "/api/auth/refresh") so all methods (register, login, refresh) use a consistent path format and avoid base-URL drift.mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt (1)
16-29:⚠️ Potential issue | 🔴 CriticalRe-throw
CancellationExceptionbefore generic failure handling.These suspend functions catch
Exception, which also catches coroutine cancellation. That turns normal cancellation intoResult.failure(...), andrefreshToken()additionally clears stored credentials during cancellation.Proposed fix
import com.smartjam.app.data.model.RegisterRequest +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.first @@ suspend fun register(email: String, password: String, username: String, role: String): Result<Unit> { return try { val response = authApi.register(RegisterRequest(email, password, username, role)) @@ Result.success(Unit) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Result.failure(e) } } @@ return try { val response = authApi.login(LoginRequest(email, password)) @@ ) Result.success(Unit) - } catch (e: Exception){ + } catch (e: CancellationException) { + throw e + } catch (e: Exception){ Result.failure(e) } } @@ - } catch (e: Exception){ + } catch (e: CancellationException) { + throw e + } catch (e: Exception){ tokenStorage.clearTokens() return false } }Also applies to: 32-45, 48-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt` around lines 16 - 29, The suspend functions (e.g., register and refreshToken) currently catch Exception and swallow coroutine cancellation; update their error handling to rethrow CancellationException before the generic failure handling: in each catch(e: Exception) block, add if (e is CancellationException) throw e else return Result.failure(e). For refreshToken specifically, ensure you rethrow CancellationException before any cleanup that clears stored credentials so cancellation won't trigger credential clearing. Locate these changes in the functions named register and refreshToken (and other similar suspend methods in the file) and apply the same pattern consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt`:
- Around line 48-74: The refreshToken() currently clears stored credentials for
any exception; change it so tokens are only cleared when the refresh token is
missing/expired or explicitly rejected by the server. Concretely: remove
tokenStorage.clearTokens() from the broad catch block and instead check
tokenStorage.isRefreshTokenExpired() and whether refreshToken (from
tokenStorage.refreshToken.first()) is null and clear there, and when
authApi.refresh(RefreshRequest(...)) fails inspect the error (e.g.,
HttpException or the API error code) and only call tokenStorage.clearTokens()
for server responses that mean invalid/ revoked token (HTTP 401/400 or API error
indicating invalid_refresh_token); for transient network errors/timeouts/5xx
simply return false (or propagate as needed) without clearing tokens. Use the
existing symbols refreshToken(), tokenStorage.isRefreshTokenExpired(),
tokenStorage.refreshToken.first(), authApi.refresh(), tokenStorage.clearTokens()
to locate and apply the change.
In `@mobile/app/src/main/java/com/smartjam/app/ui/navigation/NavGraph.kt`:
- Around line 27-35: SmartJamNavGraph always uses Screen.Login.route as the
NavHost startDestination; change it to decide the start destination from
persisted auth state by first showing a small auth/loading gate inside
SmartJamNavGraph that queries AuthRepository (e.g., getStoredSession(),
isTokenValid(), refreshToken()) and only then calls NavHost with
startDestination = Screen.Home.route if the stored session is valid/refresh
succeeds, otherwise startDestination = Screen.Login.route; implement a transient
Loading/Checking UI while the async check runs and ensure errors fall back to
the login route.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterScreen.kt`:
- Around line 132-143: The IME action and the register button call
viewModel.onRegisterClicked() even when a registration is already in-flight;
update both the KeyboardActions(onDone = { ... }) in the AppleGlassTextField and
the register Button's onClick handler (also referenced around lines 161-165) to
first check state.isLoading and return/do nothing if true (or skip calling
viewModel.onRegisterClicked), and also optionally bind the Button.enabled state
to !state.isLoading so UI prevents duplicate submissions; locate the handlers by
the symbols AppleGlassTextField, KeyboardActions, viewModel.onRegisterClicked
and the register Button to apply the guard.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt`:
- Around line 71-89: RegisterViewModel currently sets hardcoded Russian error
strings in the validation block (checks around username/email/password matching
using emailRegex, passwordRegex and _state.update), which breaks localization;
extract each message into Android string resources (e.g.
error_username_required, error_invalid_email, error_password_requirements,
error_password_mismatch) and change RegisterViewModel to emit resource
identifiers or a sealed ValidationError instead of raw strings, then inject a
StringProvider or resolve resources in the UI layer to convert those IDs/sealed
errors into localized strings; update the validation branches (the blocks that
call _state.update { it.copy(errorMessage = "...") }) to supply the resource
ID/enum value and remove hardcoded text.
- Around line 113-123: Replace the manual RegisterViewModelFactory by using
dependency injection: annotate RegisterViewModel with `@HiltViewModel` and add an
`@Inject` constructor that accepts AuthRepository, register AuthRepository as an
injectable binding (or provide it via a `@Module`), remove usages of
RegisterViewModelFactory and obtain the ViewModel via hiltViewModels() (or the
ViewModel lazy delegate) in your Fragment/Activity which must be annotated with
`@AndroidEntryPoint`; ensure Hilt is enabled in the application (Application
annotated with `@HiltAndroidApp`) and delete the RegisterViewModelFactory class
once all usages are migrated.
- Around line 37-38: The channel is created with zero buffer
(Channel<RegisterEvent>()), which can suspend or lose events during config
changes; change the initialization of eventChannel to use a buffered capacity
(e.g., Channel<RegisterEvent>(capacity = Channel.BUFFERED) or Channel.BUFFERED)
and adjust any places that call send() to use trySend() (or ensure send() is
called from a coroutine) so events emitted from the ViewModel (eventChannel) are
not dropped when no collector is currently active; keep events =
eventChannel.receiveAsFlow() as the consumer side.
- Line 40: The current passwordRegex in RegisterViewModel (`passwordRegex`)
restricts passwords to only letters and digits; update it to require at least
one letter and one digit but allow special characters by replacing the character
class [A-Za-z\\d]{8,} with a wildcard length check, e.g. use the pattern
"^(?=.*[A-Za-z])(?=.*\\d).{8,}$" (remember to escape backslashes in the Kotlin
string) so inputs like "P@ssw0rd!" are accepted while preserving the
letter+digit+min-length rules.
---
Duplicate comments:
In `@mobile/app/src/main/java/com/smartjam/app/data/api/AuthApi.kt`:
- Around line 13-20: The auth endpoints in AuthApi.kt use mixed Retrofit path
styles—register and login use absolute paths ("/api/auth/register",
"/api/auth/login") while refresh uses a relative path ("api/auth/refresh")—which
can cause inconsistent URL resolution; update the refresh endpoint to use the
same absolute-style path as the others (match "/api/auth/refresh") so all
methods (register, login, refresh) use a consistent path format and avoid
base-URL drift.
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt`:
- Around line 16-29: The suspend functions (e.g., register and refreshToken)
currently catch Exception and swallow coroutine cancellation; update their error
handling to rethrow CancellationException before the generic failure handling:
in each catch(e: Exception) block, add if (e is CancellationException) throw e
else return Result.failure(e). For refreshToken specifically, ensure you rethrow
CancellationException before any cleanup that clears stored credentials so
cancellation won't trigger credential clearing. Locate these changes in the
functions named register and refreshToken (and other similar suspend methods in
the file) and apply the same pattern consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 883e0308-9ca9-40e7-a2dd-1009ef8c04fe
📒 Files selected for processing (9)
mobile/app/src/main/java/com/smartjam/app/MainActivity.ktmobile/app/src/main/java/com/smartjam/app/data/api/AuthApi.ktmobile/app/src/main/java/com/smartjam/app/data/model/LoginResponse.ktmobile/app/src/main/java/com/smartjam/app/data/model/RegisterRequest.ktmobile/app/src/main/java/com/smartjam/app/domain/data/UserRole.ktmobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.ktmobile/app/src/main/java/com/smartjam/app/ui/navigation/NavGraph.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterScreen.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt
Comment on lines
+48
to
+74
| suspend fun refreshToken(): Boolean { | ||
| return try{ | ||
| if (tokenStorage.isRefreshTokenExpired()){ | ||
| tokenStorage.clearTokens() | ||
| return false | ||
| } | ||
|
|
||
| val refreshToken = tokenStorage.refreshToken.first() | ||
|
|
||
| if (refreshToken == null){ | ||
| return false | ||
| } | ||
| val responce = authApi.refresh(RefreshRequest(refreshToken)) | ||
|
|
||
| tokenStorage.saveToken( | ||
| accessToken = responce.accessToken, | ||
| refreshToken = responce.refreshToken, | ||
| accessExpiredAt = responce.accessExpiresAt, | ||
| refreshExpiredAt = responce.refreshExpiredAt | ||
| ) | ||
|
|
||
| return true | ||
|
|
||
| } catch (e: Exception){ | ||
| tokenStorage.clearTokens() | ||
| return false | ||
| } |
There was a problem hiding this comment.
Do not log the user out on every refresh failure.
refreshToken() clears stored credentials for any exception, including transient offline/timeout/5xx failures. That makes a temporary backend issue indistinguishable from an actually invalid refresh token and forces unnecessary re-authentication. Only clear tokens when the refresh token is missing, expired, or explicitly rejected by the server.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt`
around lines 48 - 74, The refreshToken() currently clears stored credentials for
any exception; change it so tokens are only cleared when the refresh token is
missing/expired or explicitly rejected by the server. Concretely: remove
tokenStorage.clearTokens() from the broad catch block and instead check
tokenStorage.isRefreshTokenExpired() and whether refreshToken (from
tokenStorage.refreshToken.first()) is null and clear there, and when
authApi.refresh(RefreshRequest(...)) fails inspect the error (e.g.,
HttpException or the API error code) and only call tokenStorage.clearTokens()
for server responses that mean invalid/ revoked token (HTTP 401/400 or API error
indicating invalid_refresh_token); for transient network errors/timeouts/5xx
simply return false (or propagate as needed) without clearing tokens. Use the
existing symbols refreshToken(), tokenStorage.isRefreshTokenExpired(),
tokenStorage.refreshToken.first(), authApi.refresh(), tokenStorage.clearTokens()
to locate and apply the change.
Comment on lines
+27
to
+35
| @Composable | ||
| fun SmartJamNavGraph( | ||
| navController: NavHostController, | ||
| authRepository: AuthRepository | ||
| ) { | ||
| NavHost( | ||
| navController = navController, | ||
| startDestination = Screen.Login.route | ||
| ) { |
There was a problem hiding this comment.
Choose the start destination from persisted auth state.
NavHost always boots into login_screen, so users with a valid stored session are still dropped onto the auth flow after app restart. That defeats the TokenStorage/JWT persistence added in this PR. Add an auth gate/loading state first, then start at home_screen when the stored session is still valid (or refresh succeeds).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/app/src/main/java/com/smartjam/app/ui/navigation/NavGraph.kt` around
lines 27 - 35, SmartJamNavGraph always uses Screen.Login.route as the NavHost
startDestination; change it to decide the start destination from persisted auth
state by first showing a small auth/loading gate inside SmartJamNavGraph that
queries AuthRepository (e.g., getStoredSession(), isTokenValid(),
refreshToken()) and only then calls NavHost with startDestination =
Screen.Home.route if the stored session is valid/refresh succeeds, otherwise
startDestination = Screen.Login.route; implement a transient Loading/Checking UI
while the async check runs and ensure errors fall back to the login route.
Comment on lines
+132
to
+143
| AppleGlassTextField( | ||
| value = state.repeatPasswordInput, | ||
| onValueChange = { viewModel.onRepeatPasswordChanged(it) }, | ||
| hint = "Повторите пароль", | ||
| icon = Icons.Default.Lock, | ||
| visualTransformation = PasswordVisualTransformation(), | ||
| keyboardOptions = KeyboardOptions( | ||
| keyboardType = KeyboardType.Password, | ||
| imeAction = ImeAction.Done | ||
| ), | ||
| keyboardActions = KeyboardActions(onDone = { viewModel.onRegisterClicked() }) | ||
| ) |
There was a problem hiding this comment.
Block duplicate register submissions while isLoading is true.
Both the IME action and the button still invoke onRegisterClicked() during an in-flight request. For a non-idempotent register endpoint, that can send duplicate creates.
Proposed fix
AppleGlassTextField(
value = state.repeatPasswordInput,
onValueChange = { viewModel.onRepeatPasswordChanged(it) },
hint = "Повторите пароль",
icon = Icons.Default.Lock,
visualTransformation = PasswordVisualTransformation(),
keyboardOptions = KeyboardOptions(
keyboardType = KeyboardType.Password,
imeAction = ImeAction.Done
),
- keyboardActions = KeyboardActions(onDone = { viewModel.onRegisterClicked() })
+ keyboardActions = KeyboardActions(
+ onDone = {
+ if (!state.isLoading) viewModel.onRegisterClicked()
+ }
+ )
)
@@
GoldenStringsButton(
text = if (state.isLoading) "Создание..." else "Зарегистрироваться",
- onClick = { viewModel.onRegisterClicked() },
+ onClick = {
+ if (!state.isLoading) viewModel.onRegisterClicked()
+ },
modifier = Modifier.fillMaxWidth()
)Also applies to: 161-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterScreen.kt`
around lines 132 - 143, The IME action and the register button call
viewModel.onRegisterClicked() even when a registration is already in-flight;
update both the KeyboardActions(onDone = { ... }) in the AppleGlassTextField and
the register Button's onClick handler (also referenced around lines 161-165) to
first check state.isLoading and return/do nothing if true (or skip calling
viewModel.onRegisterClicked), and also optionally bind the Button.enabled state
to !state.isLoading so UI prevents duplicate submissions; locate the handlers by
the symbols AppleGlassTextField, KeyboardActions, viewModel.onRegisterClicked
and the register Button to apply the guard.
Comment on lines
+37
to
+38
| private val eventChannel = Channel<RegisterEvent>() | ||
| val events = eventChannel.receiveAsFlow() |
There was a problem hiding this comment.
Channel with RENDEZVOUS capacity may lose events.
Using Channel<RegisterEvent>() creates a channel with zero buffer. If send() is called when no collector is active (e.g., during configuration change), the coroutine suspends and the event may be lost when the ViewModel is cleared.
🔧 Proposed fix: use a buffered channel
- private val eventChannel = Channel<RegisterEvent>()
+ private val eventChannel = Channel<RegisterEvent>(Channel.BUFFERED)📝 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
| private val eventChannel = Channel<RegisterEvent>() | |
| val events = eventChannel.receiveAsFlow() | |
| private val eventChannel = Channel<RegisterEvent>(Channel.BUFFERED) | |
| val events = eventChannel.receiveAsFlow() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt`
around lines 37 - 38, The channel is created with zero buffer
(Channel<RegisterEvent>()), which can suspend or lose events during config
changes; change the initialization of eventChannel to use a buffered capacity
(e.g., Channel<RegisterEvent>(capacity = Channel.BUFFERED) or Channel.BUFFERED)
and adjust any places that call send() to use trySend() (or ensure send() is
called from a coroutine) so events emitted from the ViewModel (eventChannel) are
not dropped when no collector is currently active; keep events =
eventChannel.receiveAsFlow() as the consumer side.
| private val eventChannel = Channel<RegisterEvent>() | ||
| val events = eventChannel.receiveAsFlow() | ||
| private val emailRegex = "^[A-Za-z0-9+_.-]+@[A-Za-z0-9.-]+\$".toRegex() | ||
| private val passwordRegex = "^(?=.*[A-Za-z])(?=.*\\d)[A-Za-z\\d]{8,}\$".toRegex() |
There was a problem hiding this comment.
Password regex blocks special characters.
The regex ^(?=.*[A-Za-z])(?=.*\d)[A-Za-z\d]{8,}$ only permits letters and digits. Users entering passwords with special characters (e.g., P@ssw0rd!) will be rejected, which is counterintuitive for password security best practices.
🔧 Proposed fix: allow special characters
- private val passwordRegex = "^(?=.*[A-Za-z])(?=.*\\d)[A-Za-z\\d]{8,}\$".toRegex()
+ private val passwordRegex = "^(?=.*[A-Za-z])(?=.*\\d).{8,}\$".toRegex()📝 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
| private val passwordRegex = "^(?=.*[A-Za-z])(?=.*\\d)[A-Za-z\\d]{8,}\$".toRegex() | |
| private val passwordRegex = "^(?=.*[A-Za-z])(?=.*\\d).{8,}\$".toRegex() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt`
at line 40, The current passwordRegex in RegisterViewModel (`passwordRegex`)
restricts passwords to only letters and digits; update it to require at least
one letter and one digit but allow special characters by replacing the character
class [A-Za-z\\d]{8,} with a wildcard length check, e.g. use the pattern
"^(?=.*[A-Za-z])(?=.*\\d).{8,}$" (remember to escape backslashes in the Kotlin
string) so inputs like "P@ssw0rd!" are accepted while preserving the
letter+digit+min-length rules.
Comment on lines
+71
to
+89
| if (currentState.usernameInput.isBlank()) { | ||
| _state.update { it.copy(errorMessage = "Введите имя пользователя") } | ||
| return | ||
| } | ||
|
|
||
| if (!emailRegex.matches(currentState.emailInput)) { | ||
| _state.update { it.copy(errorMessage = "Некорректный формат email") } | ||
| return | ||
| } | ||
|
|
||
| if (!passwordRegex.matches(currentState.passwordInput)) { | ||
| _state.update { it.copy(errorMessage = "Пароль: мин. 8 символов, латинские буквы и цифры") } | ||
| return | ||
| } | ||
|
|
||
| if (currentState.passwordInput != currentState.repeatPasswordInput) { | ||
| _state.update { it.copy(errorMessage = "Пароли не совпадают") } | ||
| return | ||
| } |
There was a problem hiding this comment.
Hardcoded Russian strings break localization.
Error messages are hardcoded in Russian. For proper i18n support, extract these to Android string resources and access via application context or a resource provider interface.
🔧 Suggested approach
- Define strings in
res/values/strings.xml:
<string name="error_username_required">Введите имя пользователя</string>
<string name="error_invalid_email">Некорректный формат email</string>
<!-- etc. -->- Inject a string resource provider into the ViewModel or return error codes/sealed classes instead of raw strings, letting the UI layer resolve them to localized strings.
Also applies to: 106-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt`
around lines 71 - 89, RegisterViewModel currently sets hardcoded Russian error
strings in the validation block (checks around username/email/password matching
using emailRegex, passwordRegex and _state.update), which breaks localization;
extract each message into Android string resources (e.g.
error_username_required, error_invalid_email, error_password_requirements,
error_password_mismatch) and change RegisterViewModel to emit resource
identifiers or a sealed ValidationError instead of raw strings, then inject a
StringProvider or resolve resources in the UI layer to convert those IDs/sealed
errors into localized strings; update the validation branches (the blocks that
call _state.update { it.copy(errorMessage = "...") }) to supply the resource
ID/enum value and remove hardcoded text.
Comment on lines
+113
to
+123
| class RegisterViewModelFactory( | ||
| private val authRepository: AuthRepository | ||
| ) : ViewModelProvider.Factory { | ||
| @Suppress("UNCHECKED_CAST") | ||
| override fun <T : androidx.lifecycle.ViewModel> create(modelClass: Class<T>): T { | ||
| if (modelClass.isAssignableFrom(RegisterViewModel::class.java)) { | ||
| return RegisterViewModel(authRepository) as T | ||
| } | ||
| throw IllegalArgumentException("Unknown ViewModel class") | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using dependency injection for ViewModel creation.
The manual factory pattern works but adds boilerplate. If the project grows, consider using Hilt with @HiltViewModel or the viewModelFactory DSL for cleaner ViewModel instantiation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt`
around lines 113 - 123, Replace the manual RegisterViewModelFactory by using
dependency injection: annotate RegisterViewModel with `@HiltViewModel` and add an
`@Inject` constructor that accepts AuthRepository, register AuthRepository as an
injectable binding (or provide it via a `@Module`), remove usages of
RegisterViewModelFactory and obtain the ViewModel via hiltViewModels() (or the
ViewModel lazy delegate) in your Fragment/Activity which must be annotated with
`@AndroidEntryPoint`; ensure Hilt is enabled in the application (Application
annotated with `@HiltAndroidApp`) and delete the RegisterViewModelFactory class
once all usages are migrated.
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
1 participant
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.
No description provided.