feat(android): migrate XML layouts to Jetpack Compose#943
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 33 minor |
| ErrorProne | 3 high |
| Complexity | 8 medium |
🟢 Metrics 78 complexity · -1 duplication
Metric Results Complexity 78 Duplication -1
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
AchoArnold
changed the title
Greptile SummaryThis PR migrates all three Android activity UIs (Login, Main, Settings) from XML layouts to Jetpack Compose, introducing ViewModels backed by
Confidence Score: 3/5The overall migration is well-structured, but the login flow has a regression that can leave the UI permanently unresponsive when a user enters a malformed server URL. The weakened URL validation in android/app/src/main/java/com/httpsms/ui/login/LoginViewModel.kt — the URL validation and exception-handling around the URI() / network call block need attention before shipping. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant A as LoginActivity
participant VM as LoginViewModel
participant API as HttpSmsApiService
A->>VM: initialize(context, defaultServerUrl)
VM-->>A: uiState (StateFlow)
A->>A: "setContent { LoginScreen(viewModel) }"
note over A: LaunchedEffect(uiState.loginSuccess)
A->>VM: login(context, countryCode, callbacks)
VM->>VM: validate phone numbers
VM->>VM: validate serverUrl (prefix check only ⚠️)
VM->>+API: updateFcmToken(phone1, SIM1, fcmToken)
API-->>-VM: Pair(apiError?, urlError?)
VM->>VM: save Settings (apiKey, serverUrl, phones)
VM->>VM: "uiState.loginSuccess = true"
note over A: LaunchedEffect fires → redirectToMain()
A->>A: finish() + startActivity(MainActivity)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant A as LoginActivity
participant VM as LoginViewModel
participant API as HttpSmsApiService
A->>VM: initialize(context, defaultServerUrl)
VM-->>A: uiState (StateFlow)
A->>A: "setContent { LoginScreen(viewModel) }"
note over A: LaunchedEffect(uiState.loginSuccess)
A->>VM: login(context, countryCode, callbacks)
VM->>VM: validate phone numbers
VM->>VM: validate serverUrl (prefix check only ⚠️)
VM->>+API: updateFcmToken(phone1, SIM1, fcmToken)
API-->>-VM: Pair(apiError?, urlError?)
VM->>VM: save Settings (apiKey, serverUrl, phones)
VM->>VM: "uiState.loginSuccess = true"
note over A: LaunchedEffect fires → redirectToMain()
A->>A: finish() + startActivity(MainActivity)
Reviews (1): Last reviewed commit: "feat: migrate XML layouts to Jetpack Com..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Simple URL validation (can be improved) | ||
| if (!serverUrl.startsWith("http")) { | ||
| _uiState.value = _uiState.value.copy( | ||
| isLoading = false, | ||
| serverUrlError = "Server URL [$serverUrl] is invalid" | ||
| ) | ||
| return@launch | ||
| } | ||
|
|
||
| if (!serverUrl.startsWith("https")) { | ||
| _uiState.value = _uiState.value.copy( | ||
| isLoading = false, | ||
| serverUrlError = "Server URL [$serverUrl] must be HTTPS" | ||
| ) | ||
| return@launch | ||
| } | ||
|
|
||
| val authResult = withContext(Dispatchers.IO) { | ||
| val service = HttpSmsApiService(apiKey, URI(serverUrl)) | ||
| val e164Phone1 = PhoneNumberValidator.formatE164(phone1, countryCode) | ||
| val response1 = service.updateFcmToken(e164Phone1, Constants.SIM1, Settings.getFcmToken(context) ?: "") | ||
|
|
||
| if (response1.second != null || response1.third != null) { | ||
| return@withContext Pair(response1.second, response1.third) | ||
| } | ||
|
|
||
| if (currentState.isDualSim) { | ||
| val e164Phone2 = PhoneNumberValidator.formatE164(phone2, countryCode) | ||
| val response2 = service.updateFcmToken(e164Phone2, Constants.SIM2, Settings.getFcmToken(context) ?: "") | ||
| return@withContext Pair(response2.second, response2.third) | ||
| } | ||
|
|
||
| Pair(null, null) | ||
| } |
There was a problem hiding this comment.
The original validation used URLUtil.isValidUrl() + URLUtil.isHttpsUrl(), which verify full URL structure. The replacement (startsWith("http") / startsWith("https")) only checks the scheme prefix, so a URL like "https://not valid" (containing a space) passes both checks. When URI(serverUrl) is then constructed inside withContext(Dispatchers.IO), it throws URISyntaxException. That exception is not caught anywhere in the coroutine, so it propagates silently — but isLoading was already set to true before the launch and is never reset, leaving the progress indicator spinning indefinitely.
To fix, wrap the network block in a try/catch (at minimum) and reset isLoading = false in the catch clause, or restore full URL structural validation before reaching the URI(...) call.
| import androidx.compose.material.icons.automirrored.filled.ArrowBack | ||
| import androidx.compose.material.icons.filled.ArrowBack |
There was a problem hiding this comment.
ArrowBack import (deprecated on Android) is unused — only Icons.AutoMirrored.Filled.ArrowBack is referenced in the code below. Removing it avoids a compiler/IDE warning.
| import androidx.compose.material.icons.automirrored.filled.ArrowBack | |
| import androidx.compose.material.icons.filled.ArrowBack | |
| import androidx.compose.material.icons.automirrored.filled.ArrowBack |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| composeOptions { | ||
| kotlinCompilerExtensionVersion = "1.5.15" | ||
| } |
There was a problem hiding this comment.
org.jetbrains.kotlin.plugin.compose Gradle plugin (Kotlin 2.0+), the compose compiler is bundled with the Kotlin plugin and the composeOptions { kotlinCompilerExtensionVersion } block is no longer needed. Including version 1.5.15 here alongside the 2.0.21 plugin can produce a build warning and is at minimum confusing.
| composeOptions { | |
| kotlinCompilerExtensionVersion = "1.5.15" | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| implementation("androidx.compose.material3:material3") | ||
| implementation("androidx.activity:activity-compose:1.9.3") | ||
| implementation("androidx.lifecycle:lifecycle-viewmodel-compose:2.8.7") | ||
| implementation("androidx.navigation:navigation-compose:2.8.3") |
There was a problem hiding this comment.
navigation-compose is added as a dependency but is never used in this PR — all three activities still navigate via explicit Intents. This unused artifact increases build time and APK size needlessly. Remove it if Compose Navigation is not yet planned, or leave a comment explaining its intent.
| implementation("androidx.navigation:navigation-compose:2.8.3") |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| import com.httpsms.ui.theme.Blue500 | ||
| import com.httpsms.ui.theme.Pink500 | ||
| import androidx.compose.foundation.layout.* |
There was a problem hiding this comment.
layout.* import duplicates the specific layout imports already declared above it (lines 6–11). The redundant wildcard can cause IDE lint warnings and makes the dependency footprint of this file harder to read at a glance.
| import com.httpsms.ui.theme.Blue500 | |
| import com.httpsms.ui.theme.Pink500 | |
| import androidx.compose.foundation.layout.* | |
| import com.httpsms.ui.theme.Blue500 | |
| import com.httpsms.ui.theme.Pink500 |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
This PR migrates the httpSMS Android application's UI from legacy XML layouts to Jetpack Compose.
Key Changes
Verification