Authentication UI and Homescreen fixes#17
Conversation
kvietelaitis
commented
Mar 26, 2026
- Added new UI for authentication
- Fixed the recent transaction modal (Height and added all transactions button)
There was a problem hiding this comment.
Pull request overview
Updates the dashboard authentication experience by introducing a new “present in person” QR/NFC-based UI, and adjusts the Home screen recent-transactions bottom sheet to better constrain height and provide a path to the full transactions list.
Changes:
- Added a new Authenticate screen flow that generates/displays a proximity QR code, shows an NFC hint, and offers switching to QR scan.
- Updated the Home screen bottom sheet to show up to 5 recent transactions and include a “show all transactions” action, plus adjusted sheet height behavior.
- Minor UI text/typography adjustments (new localized “OR” string; biometric PIN header typography tweak).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/values/strings.xml | Adds a new string used as a separator (“OR”) in the authentication UI. |
| app/src/main/res/values-lt/strings.xml | Updates LT wording for proximity title and adds LT translation for the new “OR” separator. |
| app/src/main/java/com/k689/identid/ui/dashboard/home/HomeScreen.kt | Refactors recent-transactions sheet content into a LazyColumn, limits items shown, adds “show all” action, adjusts sheet height. |
| app/src/main/java/com/k689/identid/ui/dashboard/authenticate/AuthenticateViewModel.kt | Reworks Authenticate VM into a proximity QR/NFC-driven engagement flow with error handling and navigation. |
| app/src/main/java/com/k689/identid/ui/dashboard/authenticate/AuthenticateScreen.kt | New authentication UI layout showing QR, NFC hint, and action to open QR scan; enables NFC engagement. |
| app/src/main/java/com/k689/identid/ui/common/biometric/BiometricScreen.kt | Tweaks the typography style used above the PIN field. |
Comments suppressed due to low confidence (1)
app/src/main/java/com/k689/identid/ui/common/biometric/BiometricScreen.kt:305
- This text style change is internally inconsistent:
styleis based onheadlineSmall, butfontSizeis explicitly set toheadlineMedium.fontSize, which negates the intended size change and makes the typography harder to reason about. Consider either usingheadlineMediumagain, or remove the separatefontSizeoverride / switch it toheadlineSmall.fontSize.
style =
MaterialTheme.typography.headlineSmall.copy(
color = MaterialTheme.colorScheme.onSurface,
),
fontSize = MaterialTheme.typography.headlineMedium.fontSize,
textAlign = TextAlign.Center,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val allRecentTransactions = | ||
| state.allTransactions | ||
| .filter { !it.isPending } | ||
| .reversed() |
There was a problem hiding this comment.
state.allTransactions.filter { !it.isPending }.reversed() relies on the Room SELECT * order (which is undefined) and then reverses it, so the “recent” list order can be effectively random. Sort transactions deterministically (e.g., by creation timestamp) before taking the first 5, ideally at the data source/viewmodel level where the timestamp is available.
| .reversed() | |
| .sortedByDescending { it.hashCode() } |
| .padding(vertical = 64.dp), | ||
| ) { | ||
| Text( | ||
| text = "No recent transactions", |
There was a problem hiding this comment.
Hardcoded UI text "No recent transactions" should use the existing string resource (R.string.home_screen_no_recent_transactions) to keep localization consistent.
| text = "No recent transactions", | |
| text = stringResource(R.string.home_screen_no_recent_transactions), |
| DisposableEffect(context) { | ||
| val activity = context as? ComponentActivity | ||
| activity?.let { | ||
| viewModel.setEvent(Event.NfcEngagement(it, true)) | ||
| } | ||
| onDispose { | ||
| activity?.let { | ||
| viewModel.setEvent(Event.NfcEngagement(it, false)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
NFC engagement is enabled/disabled via DisposableEffect, which won’t disable NFC when the app goes to background while this screen remains composed. In ProximityQRScreen the code uses lifecycle-aware ON_RESUME/ON_PAUSE toggling. Consider switching to LifecycleEffect (or equivalent) so NFC is reliably disabled on pause.
| import com.k689.identid.ui.component.ListItemTrailingContentDataUi | ||
| import com.k689.identid.ui.component.LargeActionFooter | ||
| import com.k689.identid.ui.component.content.ContentScreen | ||
| import com.k689.identid.ui.component.content.ContentTitle |
There was a problem hiding this comment.
Unused import ContentTitle remains after refactor; please remove it to avoid lint/format checks failing.
| import com.k689.identid.ui.component.content.ContentTitle |
| Text( | ||
| text = "Show all transactions", | ||
| style = MaterialTheme.typography.labelLarge, | ||
| color = MaterialTheme.colorScheme.primary, | ||
| fontWeight = FontWeight.Bold, | ||
| ) |
There was a problem hiding this comment.
Hardcoded UI text "Show all transactions" should be moved to a string resource for localization (and reuse if needed elsewhere).
| import androidx.compose.foundation.layout.size | ||
| import androidx.compose.foundation.layout.statusBarsPadding | ||
| import androidx.compose.foundation.layout.width | ||
| import androidx.compose.foundation.layout.wrapContentHeight |
There was a problem hiding this comment.
Unused import wrapContentHeight was added but is not referenced anywhere in this file; please remove it to avoid lint/format checks failing.
| import androidx.compose.foundation.layout.wrapContentHeight |
| is Event.Init -> { | ||
| if (interactor == null) { | ||
| initializeConfig() | ||
| generateQrCode() | ||
| } | ||
| } |
There was a problem hiding this comment.
Event.Init only runs generateQrCode() when interactor == null, but the error retry handler uses onRetry = { setEvent(Event.Init) }. After an error, interactor is already non-null, so retry becomes a no-op and the screen can’t recover. Remove the guard or make Init restart the engagement (cancel any existing job and re-run generateQrCode()).
| is Event.OpenScanQr -> { | ||
| setEffect { | ||
| Effect.Navigation.SwitchScreen( | ||
| screenRoute = | ||
| generateComposableNavigationLink( | ||
| screen = CommonScreens.QrScan, | ||
| arguments = | ||
| generateComposableArguments( | ||
| mapOf( | ||
| QrScanUiConfig.serializedKeyName to | ||
| uiSerializer.toBase64( | ||
| QrScanUiConfig( | ||
| title = resourceProvider.getString(R.string.presentation_qr_scan_title), | ||
| subTitle = resourceProvider.getString(R.string.presentation_qr_scan_subtitle), | ||
| qrScanFlow = QrScanFlow.Presentation, | ||
| ), | ||
| QrScanUiConfig.Parser, | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Event.OpenScanQr navigates away without calling cleanUp(). Unlike ProximityQRViewModel, this can leave the QR/NFC presentation flow running in the background while the user is in the scan flow (active job + open Koin presentation scope). Consider calling cleanUp() before navigating, or explicitly justify why both flows should run concurrently.
| is Event.NfcEngagement -> { | ||
| interactor?.toggleNfcEngagement( | ||
| event.componentActivity, | ||
| event.enable, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Event.NfcEngagement is dropped if it is handled before initializeConfig() runs because interactor is nullable and setEvent(...) is asynchronous. This can result in NFC engagement never being enabled. Make NFC toggling resilient (e.g., initialize the interactor on-demand in NfcEngagement, or store the desired toggle state and apply it once the interactor is ready).
| Modifier | ||
| .fillMaxWidth() | ||
| .wrapContentHeight() | ||
| .padding(bottom = SPACING_EXTRA_SMALL.dp), |
There was a problem hiding this comment.
SimplifiedNFCFooter takes paddingValues but never uses it, which is misleading and will trigger an unused-parameter warning. Either remove the parameter or apply the relevant insets (e.g., bottom padding) inside the footer.
| .padding(bottom = SPACING_EXTRA_SMALL.dp), | |
| .padding(bottom = paddingValues.calculateBottomPadding() + SPACING_EXTRA_SMALL.dp), |