MIFOSAC-617: Add missing Pinpoint location feature #2561
Conversation
This commit introduces a new "Pinpoint Location" feature accessible from the client profile screen. Key changes include: - Added a "Pinpoint Location" action item to the client profile actions list. - Implemented the navigation flow to the new pinpoint location screen. - Updated the default server tenant from "dev" to "default". - Increased the map zoom level on the pinpoint location screen for a more focused view.
Increased the default zoom level on the client pinpoint map for better initial visibility.
This commit refactors the `PinpointClientScreen` to improve user experience and introduce new functionality. Key changes include: - A `MifosBreadcrumbNavBar` has been added for better navigation context. - The UI now displays a title "Client Locations" and an "Add" icon to initiate the process of adding a new location. - An empty state card (`MifosEmptyCard`) is shown when there are no client locations, improving clarity. - The logic for displaying the permission dialog is now triggered for both updating an existing address and adding a new one. - Removed the specific error state for "no pinpoint found" in the ViewModel, as the UI now handles the empty list case gracefully. - Added Moko permissions library dependencies.
WalkthroughAdds a client pinpoint-location feature with UI, navigation and permission wiring, new profile action, moko-permissions dependency, and changes the default server tenant from "dev" to "default". Changes
Sequence DiagramsequenceDiagram
participant User
participant ClientProfile as ClientProfileScreen
participant NavGraph as Navigation
participant PinpointScreen as PinpointClientScreen
participant Permissions as MokoPermissions
participant MapDialog as Map Dialog
User->>ClientProfile: tap "Pinpoint location" action
ClientProfile->>NavGraph: pinpointLocation(clientId)
NavGraph->>PinpointScreen: navigate to pinpoint route (navController)
User->>PinpointScreen: tap "Add/Update location"
PinpointScreen->>Permissions: request location permission
alt permission granted
Permissions-->>PinpointScreen: granted
PinpointScreen->>MapDialog: open map dialog
User->>MapDialog: select location
MapDialog-->>PinpointScreen: return location coords
PinpointScreen->>PinpointScreen: emit/update locations state
else permission denied
Permissions-->>PinpointScreen: denied
PinpointScreen->>PinpointScreen: show permission dialog/guide
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinPointClientViewModel.kt (1)
58-62: Race condition: refresh indicator dismissed prematurely.The
_isRefreshingflag is set tofalseimmediately after callinggetClientPinpointLocations, but since that method launches an async coroutine (line 64), the actual data fetch hasn't completed yet. Users won't see the refresh indicator during the actual loading process.🔎 Suggested fix to properly manage refresh state
Either rely on the existing
Loadingstate from_pinPointClientUiStatefor the refresh indicator, or await the completion:Option 1: Remove the refresh flag and use the existing Loading state
- // for refresh feature - private val _isRefreshing = MutableStateFlow(false) - val isRefreshing = _isRefreshing.asStateFlow() - - fun refreshPinpointLocations(clientId: Int) { - _isRefreshing.value = true - getClientPinpointLocations(clientId = clientId) - _isRefreshing.value = false - } + fun refreshPinpointLocations(clientId: Int) { + getClientPinpointLocations(clientId = clientId) + }Option 2: Properly synchronize the refresh flag
- fun refreshPinpointLocations(clientId: Int) { - _isRefreshing.value = true - getClientPinpointLocations(clientId = clientId) - _isRefreshing.value = false - } + fun refreshPinpointLocations(clientId: Int) = viewModelScope.launch { + _isRefreshing.value = true + getClientPinpointLocations(clientId = clientId).join() + _isRefreshing.value = false + } - fun getClientPinpointLocations(clientId: Int) = viewModelScope.launch { + private fun getClientPinpointLocations(clientId: Int) = viewModelScope.launch {
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileScreen.kt (1)
50-63: Consider reordering parameters for better API ergonomics.The
pinpointLocationparameter (required, no default) is placed aftermodifierandviewModel(which have defaults). This works with named arguments but could be cleaner if required parameters are grouped before optional ones.🔎 Suggested parameter ordering
@Composable internal fun ClientProfileScreen( notes: (Int) -> Unit, navController: NavController, documents: (Int) -> Unit, viewAssociatedAccounts: (Int) -> Unit, identifiers: (Int) -> Unit, onNavigateBack: () -> Unit, navigateToClientDetailsScreen: (Int) -> Unit, viewAddress: (Int) -> Unit, + pinpointLocation: (Int) -> Unit, modifier: Modifier = Modifier, viewModel: ClientProfileViewModel = koinViewModel(), - pinpointLocation: (Int) -> Unit, ) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
build.gradle.kts(0 hunks)core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ServerConfig.kt(1 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinPointClientViewModel.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt(9 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileNavigation.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileScreen.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/components/ClientProfileActions.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt(3 hunks)gradle/libs.versions.toml(2 hunks)
💤 Files with no reviewable changes (1)
- build.gradle.kts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (15)
gradle/libs.versions.toml (1)
168-168: No action required. Version 0.20.1 is the latest release, with last release on Aug 28, 2025, and moko-permissions has no vulnerabilities.core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ServerConfig.kt (1)
34-34: No changes needed. The tenant change from "dev" to "default" is correct and aligns with Mifos X standards. "default" is the default TenantId for a MifosX instance, and this change ensures both the DEFAULT and LOCALHOST configurations use the proper production-ready tenant value. No "dev" tenant exists elsewhere in the codebase, and the default configuration uses "default" as the tenant.id.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/components/ClientProfileActions.kt (2)
75-79: LGTM!The
PinpointLocationaction item follows the established pattern of other action items in this sealed class, with proper resource references for title, subtitle, and icon.
88-88: LGTM!The
PinpointLocationitem is correctly added to theclientsActionItemslist, making it visible in the client profile UI.feature/client/src/commonMain/composeResources/values/strings.xml (1)
239-240: LGTM!The string resources follow the established naming convention and are placed consistently with other client profile action item strings.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileNavigation.kt (1)
34-34: LGTM!The
pinpointLocationcallback is correctly added to the navigation destination and properly propagated toClientProfileScreen, following the same pattern as other action callbacks.Also applies to: 45-45
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileScreen.kt (1)
94-97: LGTM!The
PinpointLocationaction is correctly handled in the event flow, following the same pattern as other action items with the client ID fallback.feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (3)
160-163: LGTM!The
navControlleris correctly passed toclientPinPointRouteto enable breadcrumb navigation inPinpointClientScreen.
191-191: LGTM!The
pinpointLocationcallback is correctly wired to use the existingnavigateClientPinPointScreennavigation function, completing the feature integration.
439-451: LGTM!The
clientPinPointRoutefunction is correctly updated to accept and pass thenavControllertoPinpointClientScreen, enabling the breadcrumb navigation bar as shown in the relevant code snippets.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt (3)
84-128: LGTM!The
navControllerparameter addition and wiring to the inner composable follows the established pattern. ViewModel interactions for CRUD operations are properly delegated.
130-264: LGTM!The permission-driven flow is correctly implemented: checking location permissions before displaying the map dialog. The
MifosBreadcrumbNavBarintegration provides good navigation context. State management for dialogs is clean.
400-417: LGTM!Preview correctly updated with
rememberNavController()to satisfy the newnavControllerparameter requirement.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinPointClientViewModel.kt (2)
41-41: LGTM: Constructor parameter refactor.Removing
private valis appropriate sincestateHandleis only used during initialization (line 44) and doesn't need to be stored as a class property.
73-75: The UI layer properly handles empty pinpoint location lists. When an empty list is returned from the API as a success state, thePinPointClientContentcomposable displaysMifosEmptyCardwith the message "No location data found". This is semantically correct and provides appropriate user feedback. The change does not mask API errors, which are separately handled by theDataState.Errorbranch.
...client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt
Show resolved
Hide resolved
...client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt
Show resolved
Hide resolved
...client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt
Show resolved
Hide resolved
Refactored PinpointClientScreen to utilize string resources for UI text and content descriptions, enhancing internationalization support. Also, corrected a typo in the `libs.versions.toml` file, changing `permissions-gallary` to `permissions-gallery`.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
gradle/libs.versions.toml (1)
420-420: Fix the typo in the library key name.The library key
moko-permissions-gallaryis misspelled. It should bemoko-permissions-galleryfor consistency with other library keys and to match the actual artifact name pattern.🔎 Proposed fix
-moko-permissions-gallary = {group ="dev.icerock.moko", name="permissions-gallery", version.ref = "mokoPermissions"} +moko-permissions-gallery = {group ="dev.icerock.moko", name="permissions-gallery", version.ref = "mokoPermissions"}Note: After making this change, you'll need to update all references to
moko-permissions-gallaryin build files to use the correctedmoko-permissions-gallerykey.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt (1)
290-296: Consider IconButton for better accessibility.The Add icon uses a
clickablemodifier on a basicIconcomposable. While functional and properly accessible viacontentDescription, usingIconButtonwould provide better UX:
- Larger minimum touch target (48dp)
- Built-in ripple effect
- Better keyboard/focus navigation support
🔎 Proposed refactor
-Icon( - imageVector = MifosIcons.Add, - contentDescription = stringResource(Res.string.feature_client_add_location), - modifier = Modifier.clickable { - onClickAddLocation() - }.size(DesignToken.sizes.iconAverage), -) +IconButton( + onClick = { onClickAddLocation() }, + modifier = Modifier.size(DesignToken.sizes.iconAverage), +) { + Icon( + imageVector = MifosIcons.Add, + contentDescription = stringResource(Res.string.feature_client_add_location), + ) +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
feature/client/src/commonMain/composeResources/values/strings.xml(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt(9 hunks)gradle/libs.versions.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/composeResources/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
MifosEmptyCard(28-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (9)
gradle/libs.versions.toml (1)
168-169: Version 0.20.1 is valid and current.Version 0.20.1 is the latest stable release of moko-permissions, released in August 2025 and available on Maven Central. No known security vulnerabilities exist for this version.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt (8)
15-21: Past i18n issues resolved.The string resources added here (
feature_client_add_location,feature_client_client_locations,feature_client_no_location_data_found) properly address the hardcoded string issues flagged in previous reviews. Well done!
88-131: LGTM: NavController integration.The NavController parameter is correctly added and threaded through to support breadcrumb navigation. Since this is an internal function, the breaking change is appropriately scoped.
212-221: Toolbar Add button correctly resets state.This implementation properly clears
addressToUpdate = nullalong with settingupdateMode = falsebefore triggering the permission dialog.
226-226: Breadcrumb navigation integrated correctly.The breadcrumb component is properly placed within the scaffold content and receives the NavController.
276-297: Content header properly localized and accessible.The refactored UI correctly uses
stringResourcefor both the "Client Locations" text (line 285) and the Add icon'scontentDescription(line 292), resolving the i18n issues from previous reviews.
212-221: Verify duplicate Add location entry points are intentional.There are now two UI elements that trigger "Add location":
- Toolbar IconButton (lines 212-221)
- Content header Icon (lines 290-296)
Both invoke the same add-location flow. This could be intentional UX design (providing multiple convenient entry points) or unintended redundancy. Please confirm this duplication is by design.
Also applies to: 290-296
301-304: Empty state properly localized.The empty state message now correctly uses
stringResource, resolving the hardcoded string issue from the previous review.
405-420: Preview updated correctly.The preview function properly supplies
navController = rememberNavController()to match the updated function signature.
...client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt (1)
291-297: Consider expanding the touch target for better accessibility.The Add icon currently has a touch target equal to
DesignToken.sizes.iconAverage. For better accessibility and UX, consider wrapping the icon in anIconButton(similar to the toolbar at line 212) or applying a minimum touch target size of 48dp.🔎 Optional refactor using IconButton
-Icon( - imageVector = MifosIcons.Add, - contentDescription = stringResource(Res.string.feature_client_add_location), - modifier = Modifier.clickable { - onClickAddLocation() - }.size(DesignToken.sizes.iconAverage), -) +IconButton( + onClick = { onClickAddLocation() } +) { + Icon( + imageVector = MifosIcons.Add, + contentDescription = stringResource(Res.string.feature_client_add_location), + modifier = Modifier.size(DesignToken.sizes.iconAverage), + ) +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
MifosEmptyCard(28-49)
🔇 Additional comments (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt (6)
15-15: Excellent additions to support the new features.The new imports properly support:
- String resources for i18n compliance (addressing previous hardcoded string issues)
- NavController integration for breadcrumb navigation
- UI enhancements (header, Add icon, empty state)
Also applies to: 17-17, 21-21, 29-29, 32-32, 37-37, 65-66, 72-73, 76-77
90-90: NavController integration is clean and consistent.The NavController parameter is properly threaded through the composable hierarchy and used exclusively for breadcrumb navigation, which provides good UX for tracking navigation history.
Also applies to: 99-99, 136-136, 226-226
236-245: Excellent fix for the state management issue.The previous major issue regarding missing
addressToUpdate = nullin theonClickAddLocationcallback has been properly addressed. Both the toolbar Add button and the content Add icon now consistently reset state before requesting permissions, preventing the state corruption bug described in earlier reviews.
286-286: All previous i18n and accessibility issues successfully resolved.The hardcoded strings and missing contentDescription from previous reviews have been properly fixed:
- "Client Locations" title now uses string resource
- Add icon contentDescription now uses string resource
- Empty state message now uses string resource
This ensures proper localization support and screen reader accessibility.
Also applies to: 293-293, 304-304
277-318: Well-structured UI improvements.The content restructure provides excellent UX:
- Clear header with title and prominent Add action
- Proper empty state handling with
MifosEmptyCard- Clean separation of concerns with conditional rendering
- Consistent use of design tokens for spacing and sizing
The implementation follows Compose best practices and maintains consistency with the rest of the codebase.
419-419: Preview correctly provides NavController.Using
rememberNavController()in the preview is the appropriate way to satisfy the new NavController requirement for rendering the breadcrumb navigation component.
3 participants
Fixes - Jira-#617
VID_20251220_200053.mp4
This pull request introduces a new "Pinpoint Location" feature to the client profile, allowing users to check or update a client's location. The changes span UI strings, navigation, action item definitions, and view logic to support this new functionality.
Feature: Pinpoint Location in Client Profile
Added new action item:
PinpointLocationaction inClientProfileActionItem, including title, subtitle, and icon resources, and updated the list of client action items to include it. [1] [2]Navigation and event handling:
clientProfileDestination,ClientProfileScreen, andclientNavGraph) to support the newpinpointLocationcallback, wiring it through navigation and screen components. [1] [2] [3] [4]PinpointLocationaction in theClientProfileScreenby invoking thepinpointLocationcallback when the action is selected.Other minor change
ServerConfigfrom"dev"to"default".Summary by CodeRabbit
New Features
UI
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.