Conversation
kvietelaitis
commented
Mar 26, 2026
- Added correct navigation for all transaction / document screens
- Fixed permission prompts
- Random optimizations
There was a problem hiding this comment.
Pull request overview
This PR refactors dashboard navigation and some UI/state handling, aiming to make the Documents/Transactions flows navigable as standalone back-stack screens and to streamline recent data rendering and permission prompting.
Changes:
- Added dedicated navigation routes for Documents and Transactions and updated list screens to use back navigation via
NavController.popBackStack(). - Updated Home state/model to expose “recent” documents/transactions and a “has more” flag, moving list slicing/filtering into the ViewModel.
- Adjusted Authenticate screen to request permissions before starting NFC engagement, plus assorted UI refactors/optimizations.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/java/com/k689/identid/ui/dashboard/transactions/list/TransactionsScreen.kt | Switches back handling to popBackStack() and adopts ToolbarConfig title toolbar. |
| app/src/main/java/com/k689/identid/ui/dashboard/home/HomeViewModel.kt | Adds recent docs/tx state + navigation events for “See all…”, and computes recent lists in ViewModel. |
| app/src/main/java/com/k689/identid/ui/dashboard/home/HomeScreen.kt | Uses new recent state, refactors quick actions, and tweaks effect/permissions handling. |
| app/src/main/java/com/k689/identid/ui/dashboard/documents/list/DocumentsScreen.kt | Switches to backable toolbar + popBackStack(), changes add-document sheet component usage. |
| app/src/main/java/com/k689/identid/ui/dashboard/dashboard/DashboardScreen.kt | Removes/comment-outs bottom tab NavHost, leaving only HomeScreen rendered. |
| app/src/main/java/com/k689/identid/ui/dashboard/authenticate/AuthenticateScreen.kt | Adds runtime BT permission request gating before enabling NFC engagement. |
| app/src/main/java/com/k689/identid/navigation/routes/dashboard/Graph.kt | Registers new composable destinations for Documents and Transactions routes. |
| app/src/main/java/com/k689/identid/navigation/RouterContract.kt | Adds DashboardScreens.Documents and DashboardScreens.Transactions. |
| app/src/main/java/com/k689/identid/interactor/dashboard/DocumentsInteractor.kt | Optimizes revoked-document lookup by precomputing revoked IDs set. |
| GoogleSansFlex-VariableFont_GRAD,ROND,opsz,slnt,wdth,wght.ttf | Adds a font asset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import com.k689.identid.extension.ui.finish | ||
| import com.k689.identid.extension.ui.openAppSettings | ||
| import com.k689.identid.extension.ui.openBleSettings | ||
| import com.k689.identid.navigation.DashboardScreens |
There was a problem hiding this comment.
New DashboardScreens import appears unused in this file. Please remove it to avoid warnings (and potential build failures if warnings are treated as errors).
| import com.k689.identid.navigation.DashboardScreens |
| private fun handleNavigationEffect( | ||
| navigationEffect: Effect.Navigation, | ||
| navController: NavController, | ||
| context: Context, | ||
| ) { | ||
| when (navigationEffect) { | ||
| is Effect.Navigation.Pop -> { | ||
| context.finish() | ||
| navController.popBackStack() | ||
| } |
There was a problem hiding this comment.
handleNavigationEffect(...) no longer uses context after switching from context.finish() to navController.popBackStack(). Please remove the unused context parameter (and update call sites), and also drop the now-unused finish import in this file to keep compilation clean if warnings are treated as errors.
| IconButton(onClick = onAuthenticateClick) { | ||
| Icon( | ||
| imageVector = Icons.Default.MobileFriendly, | ||
| contentDescription = "Authenticate", |
There was a problem hiding this comment.
contentDescription is hardcoded ("Authenticate") even though there is a dedicated string resource (home_screen_content_description_authenticate). Please use stringResource(...) so accessibility text is localizable and consistent.
| contentDescription = "Authenticate", | |
| contentDescription = stringResource(R.string.home_screen_content_description_authenticate), |
| @@ -129,8 +120,7 @@ internal fun DashboardScreen( | |||
| viewModel.setEvent(event) | |||
| }, | |||
| ) | |||
| } | |||
| } | |||
| }*/ | |||
There was a problem hiding this comment.
Large blocks of navigation code are currently commented out inside DashboardScreen. This makes the file hard to maintain and risks leaving dead logic (e.g., Effect.SwitchTab) behind. Please either restore the navigation host properly or remove the commented code and the associated unused dependencies/logic.
| }, | ||
| toolBarConfig = | ||
| ToolbarConfig( | ||
| title = stringResource(R.string.documents_screen_title), |
There was a problem hiding this comment.
By removing the custom TopBar(...) from ContentScreen and switching to a plain ToolbarConfig(title=...), the UI no longer exposes the + action that triggers Event.AddDocumentPressed (the only place that event is sent is inside the old TopBar). This makes it impossible to open the “Add document” bottom sheet from the Documents screen. Please reintroduce an add action (e.g., via ToolbarConfig.actions or by keeping the custom top bar).
| title = stringResource(R.string.documents_screen_title), | |
| title = stringResource(R.string.documents_screen_title), | |
| actions = | |
| listOf { | |
| WrapIconButton( | |
| icon = AppIcons.Add, | |
| contentDescription = stringResource(R.string.documents_screen_title), | |
| onClick = { viewModel.setEvent(Event.AddDocumentPressed) }, | |
| ) | |
| }, |
| ), | ||
| onEventSent = onEventSent, | ||
| hostTab = BottomNavigationItem.Documents.route.lowercase(), | ||
| // hostTab = BottomNavigationItem.Documents.route.lowercase(), | ||
| ) |
There was a problem hiding this comment.
There is commented-out code (hostTab = BottomNavigationItem.Documents...) left in this call. Since BottomNavigationItem is now only referenced in a comment, it also leaves an effectively-unused import. Please remove the commented line and the unused import, or reintroduce the needed parameter in the bottom-sheet API if it’s still required.
| private fun handleNavigationEffect( | ||
| navigationEffect: Effect.Navigation, | ||
| navController: NavController, | ||
| context: Context, | ||
| ) { | ||
| when (navigationEffect) { | ||
| is Effect.Navigation.Pop -> { | ||
| context.finish() | ||
| navController.popBackStack() |
There was a problem hiding this comment.
handleNavigationEffect(...) still takes a context: Context parameter, but after changing Pop to navController.popBackStack() the context is unused. Please remove the unused parameter (and update call sites) and drop the now-unused finish import to keep the file warning-free.
| IconButton(onClick = onSignDocumentClick) { | ||
| Icon( | ||
| imageVector = Icons.Default.Edit, | ||
| contentDescription = "Sign document", |
There was a problem hiding this comment.
contentDescription is hardcoded ("Sign document") even though there is a dedicated string resource (home_screen_content_description_sign_document). Please use stringResource(...) so accessibility text is localizable and consistent.
| contentDescription = "Sign document", | |
| contentDescription = stringResource(R.string.home_screen_content_description_sign_document), |
| DisposableEffect(context, permissionsState.allPermissionsGranted) { | ||
| val activity = context as? ComponentActivity | ||
| activity?.let { | ||
| viewModel.setEvent(Event.NfcEngagement(it, true)) | ||
|
|
||
| // ONLY start NFC engagement if permissions are granted | ||
| if (permissionsState.allPermissionsGranted && activity != null) { | ||
| viewModel.setEvent(Event.NfcEngagement(activity, true)) | ||
| } | ||
|
|
||
| onDispose { | ||
| activity?.let { | ||
| viewModel.setEvent(Event.NfcEngagement(it, false)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
NFC engagement is toggled via DisposableEffect(...), so it will remain enabled when the app is backgrounded while this composable stays in composition (no onDispose), which is inconsistent with how other NFC engagement screens are handled. Consider switching to lifecycle-driven toggling (enable on ON_RESUME, disable on ON_PAUSE) like ProximityQRScreen does (ui/proximity/qr/ProximityQRScreen.kt:108-130), and keep the permission gating in the resume handler.
| HomeScreen( | ||
| hostNavController, | ||
| homeViewModel, | ||
| onDashboardEventSent = { event -> | ||
| viewModel.setEvent(event) | ||
| }, | ||
| ) | ||
| /* composable(BottomNavigationItem.Documents.route) { |
There was a problem hiding this comment.
NavHost/composable(...) setup for bottomNavigationController was removed (and destinations are now commented out). This means bottomNavigationController never gets a graph, so any later bottomNavigationController.navigate(...) calls (e.g., from Effect.SwitchTab) will throw IllegalStateException: NavController graph has not been set or become a no-op. Either restore the NavHost with the tab destinations, or remove the bottomNavigationController + Effect.SwitchTab handling and migrate all tab switching to hostNavController routes.