feat(search-record): implemented generic search for addresses and ide… #2586
Conversation
📝 WalkthroughWalkthroughAdds a new search-record feature (UI, ViewModel, DI, navigation), new model types and repository/data-source plumbing, database entities/migration for addresses and identifiers, and integrates search flows into client navigation and navbar state. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as SearchRecordScreen
participant VM as SearchRecordViewModel
participant Repo as SearchRecordRepository
participant Local as SearchRecordLocalDataSource
participant Helper as ClientDaoHelper
participant DB as Database
User->>UI: type query
UI->>VM: UpdateSearchQuery(query)
Note over VM: debounce 300ms
VM->>Repo: searchRecords(recordType, query)
Repo->>Local: searchRecords(recordType, query)
alt ADDRESS
Local->>Helper: searchAddressesByQuery(query)
Helper->>DB: query ClientAddress
DB-->>Helper: address rows
Helper-->>Local: Flow<List<ClientAddressEntity>>
else IDENTIFIER
Local->>Helper: searchIdentifiersByQuery(query)
Helper->>DB: query ClientIdentifier
DB-->>Helper: identifier rows
Helper-->>Local: Flow<List<ClientIdentifierEntity>>
end
Local->>Repo: Flow<List<GenericSearchRecord>>
Repo-->>VM: Flow<Result<List<GenericSearchRecord>>>
VM->>UI: state update (results / no-results / error)
User->>UI: select record
UI->>VM: OnRecordSelected(record)
VM-->>UI: Navigate to client screen (based on record.metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/ClientAddressScreen.kt (1)
208-216: Add an accessibility label for the search button.
The new search IconButton has an emptycontentDescription, which makes it invisible to screen readers.🔧 Proposed fix
Icon( painter = painterResource(Res.drawable.search), - contentDescription = "", + contentDescription = stringResource(Res.string.search), )feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt (1)
129-195: UseitemsIndexedinstead offorEachIndexedinside a singleitem {}block.Wrapping
forEachIndexedinside a singleitem {}causes all list items to render eagerly, defeating the purpose ofLazyColumn. For proper lazy loading, useitemsIndexeddirectly.♻️ Suggested refactor
- LazyColumn { - item { - state.clientIdentitiesList.reversed().forEachIndexed { index, item -> + val reversedList = state.clientIdentitiesList.reversed() + LazyColumn { + itemsIndexed(reversedList) { index, item -> // ... item content remains the same ... - Spacer(Modifier.height(DesignToken.spacing.small)) - } + Spacer(Modifier.height(DesignToken.spacing.small)) } }core/database/src/desktopMain/kotlin/com/mifos/room/MifosDatabase.kt (1)
83-151: Room schema change requires version bump and migration.Adding
ClientAddressEntityandClientIdentifierEntityat lines 118–119 modifies the database schema. WithVERSIONstill at 1 andautoMigrationsempty, existing installs will fail schema validation. Increment the DB version and add an appropriate migration (auto or manual) consistent with your Room setup.Example adjustment (align with your migration strategy)
+import androidx.room.AutoMigration ... `@Database`( entities = [ ... ], - version = MifosDatabase.VERSION, + version = MifosDatabase.VERSION, exportSchema = false, - autoMigrations = [], + autoMigrations = [ + AutoMigration(from = 1, to = 2), + ], ) ... companion object { - const val VERSION = 1 + const val VERSION = 2 }
🤖 Fix all issues with AI agents
In
`@cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt`:
- Around line 373-387: The current routing in searchRecordNavigation uses
fragile string literals ("Address", "Identifiers") to match record.type; change
these comparisons to use the RecordType enum displayName (e.g., compare
record.type to RecordType.Address.displayName and
RecordType.Identifiers.displayName) so lookups stay correct if the enum's
display names change, then call navController.navigateToClientAddressRoute(id =
clientId) and navController.navigateToClientIdentifiersListScreen(clientId =
clientId) accordingly; update the when/if branches in the onRecordSelected
lambda (inside searchRecordNavigation) to use RecordType.displayName constants
for robust comparisons.
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt`:
- Around line 35-51: The database insert in the onEach handler of the flow
returned by dataManagerIdentifiers.getClientListIdentifiers (chained with
asDataStateFlow and onEach) must be guarded so persistence failures don't cancel
the entire flow: wrap the clientDaoHelper.insertIdentifiers(entities) call
inside a try-catch that catches Throwable, logs or records the error (using
existing logging facilities) and swallows it (does not rethrow) so the
DataState.Success continues to emit to the UI; keep the construction of
ClientIdentifierEntity and the insert call but isolate only the side-effect in
the try-catch within the same onEach block.
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/CreateNewClientRepositoryImp.kt`:
- Around line 73-95: getAddresses currently only calls
clientDaoHelper.insertAddresses when dataManagerClient.getClientAddresses
returns a non-empty list, which leaves stale addresses in the DB if the server
returns an empty list; modify getAddresses to remove existing addresses for that
client before inserting new ones by calling
clientDaoHelper.deleteAddressesByClientId(clientId) (or equivalent) prior to
insertAddresses, or if you prefer to only delete when the server explicitly
returns an empty list call deleteAddressesByClientId when addresses.isEmpty();
locate symbols getAddresses, dataManagerClient.getClientAddresses,
clientDaoHelper.insertAddresses and add the deleteAddressesByClientId call
accordingly to ensure cached addresses are replaced rather than retained.
- Around line 76-98: The current catch block around mapping addresses and
calling clientDaoHelper.insertAddresses swallows CancellationException (breaking
structured coroutine cancellation); update the error handling in the try/catch
so that if the caught exception is a CancellationException you rethrow it
immediately (e.g., if (e is CancellationException) throw e) and only handle/log
non-cancellation exceptions (e.g., call e.printStackTrace() or logger). Ensure
this change is applied where RoomAddressEntity objects are created and
clientDaoHelper.insertAddresses(...) is invoked (and note that suspend functions
like getClientAddresses() and insertAddresses() can throw
CancellationException).
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/SearchRecordRepositoryImpl.kt`:
- Around line 23-34: The current searchRecords implementation catches all
Exceptions and turns CancellationException into a failure; update the
searchRecords function to preserve coroutine cancellation by operating on the
Flow returned from localDataSource.searchRecords(recordType, query) and use Flow
operators: map to wrap emitted records into Result.success and catch to emit
Result.failure for non-cancellation exceptions, but rethrow
CancellationException (and other Throwable cancellation types) instead of
emitting them as failures; ensure you reference the searchRecords method and
localDataSource.searchRecords call when making the change.
In `@core/database/src/androidMain/kotlin/com/mifos/room/MifosDatabase.kt`:
- Around line 119-120: The database schema was changed by adding
ClientAddressEntity and ClientIdentifierEntity to the `@Database` on MifosDatabase
while leaving VERSION = 1 and no migrations; update the schema handling by
incrementing VERSION to 2 and add a proper Room Migration (create a Migration
object and register it via addMigrations(...) on the Room database builder) to
migrate existing data, or alternatively (only if acceptable) call
fallbackToDestructiveMigration() on the builder or declare an autoMigration in
the `@Database` autoMigrations array; ensure the change is applied for all
occurrences where VERSION and the database builder are defined (references to
MifosDatabase.VERSION, the `@Database` annotation, and the Room.databaseBuilder
call).
In
`@core/database/src/commonMain/kotlin/com/mifos/room/helper/ClientDaoHelper.kt`:
- Around line 366-371: The insertIdentifiers function currently deletes
identifiers only for identifiers.first().clientId which causes data
inconsistency when the list contains multiple clientIds; update
insertIdentifiers to either (A) enforce a precondition that all items share the
same clientId (throw IllegalArgumentException or rename to
replaceIdentifiersForClient) OR (B) iterate the input, compute the distinct
clientIds (e.g., identifiers.map { it.clientId }.distinct()), call
clientDao.deleteIdentifiersByClientId for each distinct id before calling
clientDao.insertIdentifiers, ensuring you reference insertIdentifiers and
clientDao.deleteIdentifiersByClientId to locate and change the logic.
In `@core/database/src/nativeMain/kotlin/com/mifos/room/MifosDatabase.kt`:
- Around line 120-121: The native MifosDatabase schema was changed by adding
ClientAddressEntity and ClientIdentifierEntity but the migration/version bump
wasn’t applied; update the native database configuration in MifosDatabase to
match the Android migration strategy: increment the database schema version
constant and register the same Migration object(s) used on Android (or implement
equivalent migrations) so the new entities are created without data loss,
ensuring MifosDatabase, the database builder/initialization and any migration
registration points include the new version and migration logic.
In
`@core/model/src/commonMain/kotlin/com/mifos/core/model/objects/searchrecord/RecordType.kt`:
- Around line 12-15: The enum RecordType has inconsistent displayName values:
ADDRESS is "Address" (singular) while IDENTIFIER is "Identifiers" (plural);
update one or both to be consistent—either change ADDRESS.displayName to
"Addresses" or change IDENTIFIER.displayName to "Identifier" (or pick the
preferred singular/plural form for the UI) so that the displayName values for
RecordType::ADDRESS and RecordType::IDENTIFIER follow the same pluralization
rule.
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt`:
- Around line 203-214: SearchRecordScreen is comparing record.type to localized
strings using stringResource(), which will break in other locales; change the
comparison to use the canonical RecordType enum
(com.mifos.core.model.objects.searchrecord.RecordType) or backend constant
values instead of stringResource(), e.g., map record.type to RecordType (or
compare to RecordType.NAME) and switch on that to decide whether to render
AddressRecordCard, IdentifierRecordCard, or GenericRecordCard so the logic is
locale-independent.
- Around line 233-241: The map of address labels in SearchRecordScreen (the
addressList assignment) uses hardcoded English strings; extract each label into
string resources (e.g., address_line_1, address_line_2, address_line_3, city,
province/state, country, postal_code) and replace the literal keys with
localized lookups (e.g., stringResource(R.string.address_line_1) or
context.getString(R.string.address_line_1) depending on your Compose/Platform
API). Add the new keys to your strings resource files for each platform and
update the addressList map to reference the resource lookups instead of raw
literals.
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt`:
- Around line 85-91: The code in SearchRecordViewModel uses
Res.string.error_searching_records but Res is not imported; add the missing
import for the Res class alongside the other imports at the top of the file so
references in SearchRecordViewModel (the result.onFailure block that updates
SearchRecordUiState via _uiState.update) compile correctly.
🧹 Nitpick comments (7)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/utils/IdentifierStatus.kt (1)
14-28: Consider refactoring to avoid duplicatelowercase()call and improve readability.The
status.lowercase()is called twice (lines 16 and 18-19). Store the result in a local variable and use awhenexpression for cleaner idiomatic Kotlin.♻️ Suggested refactor
fun getClientIdentifierStatus(status: String?): Status? { - return if (status != null) { - if (status.lowercase().endsWith("inactive")) { - Status.Inactive - } else if (status.lowercase() - .endsWith("active") - ) { - Status.Active - } else { - Status.Pending - } - } else { - null + if (status == null) return null + val lowerStatus = status.lowercase() + return when { + lowerStatus.endsWith("inactive") -> Status.Inactive + lowerStatus.endsWith("active") -> Status.Active + else -> Status.Pending } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
201-207: Centralize search category labels.
Using string literals for categories risks drift and makes refactors harder. Consider constants (or an enum) for category values.♻️ Suggested refactor
+private const val SEARCH_CATEGORY_ADDRESS = "Address" +private const val SEARCH_CATEGORY_IDENTIFIER = "Identifier" ... onNavigateToSearch = { - onNavigateToSearch("Address") + onNavigateToSearch(SEARCH_CATEGORY_ADDRESS) }, ... onNavigateToSearch = { - onNavigateToSearch("Identifier") + onNavigateToSearch(SEARCH_CATEGORY_IDENTIFIER) },Also applies to: 335-337
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt (1)
216-219: Consider using a parameterized string resource for proper localization.String concatenation with
+can break in languages with different word orders. A parameterized string resource (e.g.,"%s items") would handle this correctly.feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt (2)
41-41: Consider localizing the search label.The search label uses hardcoded English text. For proper i18n support, consider using a string resource with a placeholder.
💡 Suggested approach
Define a string resource with a placeholder in
strings.xml:<string name="search_label_format">Search %1$s</string>Then use it in the ViewModel:
// You may need to expose this differently since ViewModel shouldn't directly access Compose resources val searchLabelRes: StringResource = Res.string.search_label_format val searchLabelArg: String = recordType.displayName
54-69: Consider using Flow'sdebounceoperator for cleaner implementation.The manual debounce with
Jobcancellation works but could be simplified using Kotlin Flow's built-indebounceoperator.♻️ Simplified approach using Flow debounce
private fun observeSearchQuery() { viewModelScope.launch { _searchQuery .debounce(SEARCH_DEBOUNCE_DELAY_MS) .collectLatest { query -> if (query.isBlank()) { _uiState.update { SearchRecordUiState.EmptyQuery } } else { performSearch(query) } } } }Note: You'd also need to handle the immediate blank-query case separately if you want instant feedback when the user clears the search field.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationViewModel.kt (1)
49-61: Route matching viacontains()is fragile.String-based route detection can cause false positives if route names change or overlap (e.g., a route named
ClientIdentifiersListDetailswould also match). Consider using sealed classes for routes or an enum-based approach to make this more type-safe and maintainable.♻️ Suggested improvement
- private fun updateRouteState(route: String?) { - val safeRoute = route ?: "" - val shouldShowTopBar = !safeRoute.contains("SearchRecord") - val searchType = when { - safeRoute.contains("ClientIdentifiersList") -> RecordType.IDENTIFIER.name - else -> RecordType.ADDRESS.name - } + private fun updateRouteState(route: String?) { + val safeRoute = route ?: "" + val shouldShowTopBar = !safeRoute.endsWith("SearchRecord") + val searchType = when { + safeRoute.endsWith("ClientIdentifiersList") -> RecordType.IDENTIFIER.name + else -> RecordType.ADDRESS.name + }Or better yet, define route constants in a shared location and match against them.
core/database/src/commonMain/kotlin/com/mifos/room/dao/ClientDao.kt (1)
182-193: SQL comment inside query annotation is unusual.The inline SQL comment on line 185 (
-- Added this line...) works but is atypical for Room queries. Consider removing it to keep the query clean, or move the explanation to a KDoc comment above the method.♻️ Suggested cleanup
+ /** + * Searches identifiers by query string. + * Note: Includes CAST(id AS TEXT) to allow searching by numeric ID. + */ `@Query`( """ SELECT * FROM ClientIdentifier - WHERE CAST(id AS TEXT) LIKE :query -- Added this line to search by numeric ID + WHERE CAST(id AS TEXT) LIKE :query OR documentKey LIKE :query OR description LIKE :query OR documentTypeName LIKE :query ORDER BY documentKey LIMIT 50 """, ) fun searchIdentifiersByQuery(query: String): Flow<List<ClientIdentifierEntity>>
...on/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt
Show resolved
Hide resolved
...ta/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt
Show resolved
Hide resolved
...data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/CreateNewClientRepositoryImp.kt
Show resolved
Hide resolved
...data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/CreateNewClientRepositoryImp.kt
Outdated
Show resolved
Hide resolved
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/SearchRecordRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/searchrecord/RecordType.kt
Outdated
Show resolved
Hide resolved
...ure/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt
Show resolved
Hide resolved
...ure/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt
Show resolved
Hide resolved
.../search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt`:
- Around line 34-53: In getClientListIdentifiers, the onEach side-effect
swallows all Throwables when calling clientDaoHelper.insertIdentifiers, which
hides CancellationException; change the error handling so CancellationException
is not suppressed (e.g., catch Throwable around
clientDaoHelper.insertIdentifiers but rethrow if it is CancellationException, or
catch Exception instead of Throwable), ensuring structured coroutine
cancellation is preserved for getClientListIdentifiers /
dataManagerIdentifiers.getClientListIdentifiers / onEach /
clientDaoHelper.insertIdentifiers.
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt`:
- Around line 201-211: The items call in LazyColumn currently uses a non-unique
key via items(records, key = { it.id }) which can collide between different
record types; change the key to a composite that includes the record type so
keys are unique across types (for example use record.type combined with
record.id) where the items invocation and SearchRecordItem rendering occur to
prevent Compose recomposition issues.
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt`:
- Around line 30-49: The view model currently builds a hardcoded, non-localized
label in SearchRecordViewModel via the searchLabel property using "Search
${recordType.displayName}"; replace this with a localized, parameterized string
resource (e.g., "search_record_label_format" with a %1$s placeholder) and stop
constructing the final text in the ViewModel. Either move label construction
into the UI layer where stringResource() is available and derive the formatted
label from recordType.displayName there, or inject a string-resolver into
SearchRecordViewModel (e.g., a ResourceProvider) and use it to format the
localized string for searchLabel instead of hardcoding "Search ".
🧹 Nitpick comments (6)
feature/search-record/src/commonMain/composeResources/values/strings.xml (2)
22-22: Inconsistent naming convention.This string uses
error_searching_recordswhile all other strings follow thesearch_record_*prefix pattern. Consider renaming for consistency.Suggested fix
- <string name="error_searching_records">An error occurred while searching</string> + <string name="search_record_error_searching">An error occurred while searching</string>
30-30: Missing trailing newline.The file should end with a newline character after the closing
</resources>tag for POSIX compliance and to avoid potential issues with version control diffs.feature/search-record/build.gradle.kts (1)
26-30: Consider removing direct dependency oncore.datafrom feature module.In clean architecture, feature modules typically depend on
core.domainfor repository interfaces and use cases, whilecore.data(containing implementations) is wired via dependency injection. While some feature modules in this project do depend oncore.datadirectly (auth, center, client, data-table, groups, settings), most others function without it. Review whether the direct dependency oncore.datais necessary for this feature.cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt (2)
231-240: Consider breaking long lines for better readability.Line 233 is quite long. While functional, it could be formatted across multiple lines for improved readability and maintainability.
♻️ Suggested formatting
- Image(modifier = Modifier.fillMaxSize(), contentScale = ContentScale.Crop, painter = painterResource(Res.drawable.drawer_profile_header), contentDescription = stringResource(Res.string.cmp_navigation_profile_header)) + Image( + modifier = Modifier.fillMaxSize(), + contentScale = ContentScale.Crop, + painter = painterResource(Res.drawable.drawer_profile_header), + contentDescription = stringResource(Res.string.cmp_navigation_profile_header), + )
264-274: Empty click handlers for search and notification icons.
onSearchIconClickandonNotificationIconClickare empty lambdas. If these icons are visible but non-functional, consider either:
- Implementing the handlers (e.g.,
onSearchIconClickcould navigate to the search screen)- Hiding these icons until functionality is ready
This may be intentional for incremental development, but leaving visible non-functional UI elements can confuse users.
feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt (1)
102-105: Minor race condition betweenclearSearch()andobserveSearchQuery().Setting
_searchQueryto""triggersobserveSearchQuery()which sets state toEmptyQuery, but then line 104 immediately sets state toIdle. Due to coroutine scheduling, the final state could be either. Currently both states renderSearchRecordEmptyState()so this is functionally harmless, but it introduces subtle inconsistency.💡 Suggested fix
Either remove the explicit state update (letting the observer handle it):
fun clearSearch() { _searchQuery.update { "" } - _uiState.update { SearchRecordUiState.Idle } }Or unify
IdleandEmptyQueryif they serve the same purpose.
...ta/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt
Show resolved
Hide resolved
...ure/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt
Show resolved
Hide resolved
.../search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt
Show resolved
Hide resolved
|
|
||
| kotlin { | ||
| sourceSets { | ||
| commonMain.dependencies { |
There was a problem hiding this comment.
You don't need to import compose libraries and core libraries, they are already provided by the mifos.cmp.feature plugin.
There was a problem hiding this comment.
oh got it. I was just following the pattern I saw in other feature modules. I will remove the redundant dependencies
| searchRecordNavigation( | ||
| onBackClick = { navController.popBackStack() }, | ||
| onRecordSelected = { record -> | ||
| val clientId = record.metadata["clientId"]?.toIntOrNull() |
There was a problem hiding this comment.
Don't use hardcoded clientId it may cause typo errors in the future, so use enums or constants instead.
| class SearchRecordViewModel( | ||
| savedStateHandle: SavedStateHandle, | ||
| private val repository: SearchRecordRepository, | ||
| ) : ViewModel() { |
There was a problem hiding this comment.
Use BaseViewModel approach, take reference from feature:client module viewModels
| */ | ||
| package com.mifos.core.model.objects.searchrecord | ||
|
|
||
| enum class RecordType(val displayName: String) { |
There was a problem hiding this comment.
I suggest you don't use displayName constructor parameter in enum class. make you enum class simple with add all the necessary enum constants.
eg
enum class RecordType{
ADDRESS,
IDENTIFIER
}And for handling display name showing on top of the screen, implement like this logic in SearchRecordViewNodel
eg.
private val route = savedStateHandle.toRoute<RecordType>()
init {
mutableStateFlow.update {
it.copy(
displayName = when(route){
RecordType.ADDRESS -> "Address"
RecordType.IDENTIFIER -> "Identifiers"
}
)
}
}
There was a problem hiding this comment.
Using this approach typos error not occur
kartikey004
force-pushed
the
feature/search-record
branch
from
10800d1 to
032e5b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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/clientAddress/ClientAddressScreen.kt (1)
199-208:⚠️ Potential issue | 🟡 MinorAdd a meaningful
contentDescriptionfor accessibility.The search icon's
contentDescriptionis an empty string, which negatively impacts screen reader users. Consider using the string resource that appears to be imported (Res.drawable.search) or a dedicated accessibility string.♿ Proposed fix for accessibility
IconButton( onClick = { onAction(ClientAddressAction.NavigateToSearch) }, ) { Icon( painter = painterResource(Res.drawable.search), - contentDescription = "", + contentDescription = stringResource(Res.string.search), ) }Note: Ensure
Res.string.searchexists or add an appropriate string resource for the search action description.
🤖 Fix all issues with AI agents
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt`:
- Around line 40-49: The Long→Int fallback conversion at the
ClientIdentifiersRepositoryImp mapping (usage of identifier.clientId ?:
clientId.toInt()) is intentional but should be made explicit and safe: update
the mapping to either (1) add a concise inline comment referencing why the
interface parameter is Long and why truncation is acceptable for now, and keep
the existing toInt() fallback; or (preferred) replace the fallback with a
safe/clamped conversion such as using clientId.coerceIn(Int.MIN_VALUE.toLong(),
Int.MAX_VALUE.toLong()).toInt() before assigning to
ClientIdentifierEntity.clientId to avoid silent overflow; refer to
ClientIdentifiersRepositoryImp, the identifier.clientId usage, and
ClientIdentifierEntity.clientId when making the change.
In
`@core/database/src/commonMain/kotlin/com/mifos/room/helper/ClientDaoHelper.kt`:
- Around line 370-374: The current insertIdentifiers in ClientDaoHelper deletes
identifiers per clientId then inserts, which risks data loss if insert fails;
replace this with an atomic operation by adding a DAO-level method (e.g.,
clientDao.replaceIdentifiersForClients(clientIds: List<Long>, identifiers:
List<ClientIdentifierEntity)) that is annotated with `@Transaction` and performs
the delete-by-clientIds and insertIdentifiers together, then call that new
method from insertIdentifiers (or alternatively wrap the existing two-step call
in a try/catch and implement a rollback/recovery path), referencing
clientDao.deleteIdentifiersByClientId, clientDao.insertIdentifiers, and the
helper method insertIdentifiers to locate and change the code.
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/navigation/SearchRecordNavigation.kt`:
- Around line 20-23: SearchRecordRoute is `@Serializable` but its RecordType enum
lacks the `@Serializable` annotation causing runtime failures; add the
kotlinx.serialization.Serializable import and annotate enum class RecordType
with `@Serializable` (the enum values ADDRESS, IDENTIFIER remain unchanged) so
serialization of SearchRecordRoute<RecordType> works correctly.
🧹 Nitpick comments (8)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListViewModel.kt (1)
295-296: Remove the unusedisSearchBarActiveproperty fromClientIdentifiersListState.The property is defined but never referenced in the UI or updated when
ToggleSearchis triggered. The action directly emits aNavigateToSearchevent instead of toggling this state property. Removing it will reduce clutter and avoid confusion about the UI's actual search behavior.core/ui/src/commonMain/kotlin/com/mifos/core/ui/utils/IdentifierStatus.kt (1)
14-24: Consider refactoring for improved readability and avoiding repeated allocationsWhile
lowercase()on Kotlin is actually locale-safe (it uses Unicode case-mapping rules and doesn't have the Turkish I pitfall), the current code makes repeated calls tolowercase(), which allocates unnecessary intermediate strings. Refactoring withendsWith(ignoreCase = true)is a cleaner, more direct approach:Suggested refactoring (optional)
fun getClientIdentifierStatus(status: String?): Status? { - return if (status != null) { - if (status.lowercase().endsWith("inactive")) { - Status.Inactive - } else if (status.lowercase() - .endsWith("active") - ) { - Status.Active - } else { - Status.Pending - } - } else { - null - } + return when { + status == null -> null + status.endsWith("inactive", ignoreCase = true) -> Status.Inactive + status.endsWith("active", ignoreCase = true) -> Status.Active + else -> Status.Pending + } }cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationViewModel.kt (1)
49-61: Route matching viacontains()is fragile.The string-based route checks (
safeRoute.contains("SearchRecord"),safeRoute.contains("ClientIdentifiersList")) may match unintended routes if naming conventions change or if route names partially overlap. Consider using the route's qualified type or comparing against route class names from the navigation graph for more robust matching.feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt (1)
40-48: Hardcoded display title strings break internationalization.Similar to the previously addressed
searchLabelissue,displayTitleuses hardcoded strings"Address"and"Identifiers". Consider using string resources for consistency with the i18n approach applied elsewhere.🌐 Suggested approach
Use localized string resources similar to how
error_searching_recordsis referenced:init { mutableStateFlow.update { it.copy( displayTitle = when (recordType) { - RecordType.ADDRESS -> "Address" - RecordType.IDENTIFIER -> "Identifiers" + RecordType.ADDRESS -> // Use Res.string.search_record_address_title + RecordType.IDENTIFIER -> // Use Res.string.search_record_identifiers_title }, ) } observeSearchQuery() }Note: You'll need to resolve string resources appropriately (e.g., via
getString()if available in the ViewModel context, or move title derivation to the UI layer).cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt (1)
378-395: Consider handling unknown record types explicitly.The
whenblock handlesADDRESSandIDENTIFIERtypes but silently ignores any otherrecord.typevalues. If a newRecordTypeis added in the future, this could lead to silent navigation failures. Consider logging or handling unknown types explicitly.🔧 Suggested improvement
if (clientId != null) { when { record.type.equals(RecordType.ADDRESS.name, ignoreCase = true) -> { navController.navigateToClientAddressRoute(id = clientId) } record.type.equals(RecordType.IDENTIFIER.name, ignoreCase = true) -> { navController.navigateToClientIdentifiersListScreen(clientId = clientId) } + + else -> { + // Log or handle unknown record type + } } }core/database/src/commonMain/kotlin/com/mifos/room/entities/client/ClientIdentifierEntity.kt (1)
16-35: Consider adding an index onclientIdfor query performance.The DAO queries identifiers by
clientId(getIdentifiersByClientId,deleteIdentifiersByClientId), but no index is defined on this column. For tables with many records, this could impact query performance.📊 Proposed index addition
`@Entity`( tableName = "ClientIdentifier", - indices = [], + indices = [Index(value = ["clientId"])], inheritSuperIndices = false, primaryKeys = [], foreignKeys = [], ignoredColumns = [], )Note: You would also need to update
MIGRATION_1_2to create the index on existing databases:CREATE INDEX IF NOT EXISTS `index_ClientIdentifier_clientId` ON `ClientIdentifier` (`clientId`)core/database/src/commonMain/kotlin/com/mifos/room/dao/ClientDao.kt (1)
185-196: Remove SQL comment from production query.Line 188 contains a SQL comment (
-- Added this line to search by numeric ID) which is unusual in production code and adds noise. The query intent is clear from context.🧹 Proposed cleanup
`@Query`( """ SELECT * FROM ClientIdentifier - WHERE CAST(id AS TEXT) LIKE :query -- Added this line to search by numeric ID + WHERE CAST(id AS TEXT) LIKE :query OR documentKey LIKE :query OR description LIKE :query OR documentTypeName LIKE :query ORDER BY documentKey LIMIT 50 """, ) fun searchIdentifiersByQuery(query: String): Flow<List<ClientIdentifierEntity>>core/data/src/commonMain/kotlin/com/mifos/core/data/datasource/SearchRecordLocalDataSourceImpl.kt (1)
25-33: Guard blank queries to avoid full‑table scans.
Right now a blank query becomes"%%"in downstream search, which can return all rows. If the UI expects “no query → no results,” short‑circuit here.🔧 Suggested refactor
override fun searchRecords( recordType: RecordType, query: String, ): Flow<List<GenericSearchRecord>> { + if (query.isBlank()) return flowOf(emptyList()) return when (recordType) { RecordType.ADDRESS -> searchAddressesLocal(query) RecordType.IDENTIFIER -> searchIdentifiersLocal(query) else -> flowOf(emptyList()) } }
...ta/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt
Show resolved
Hide resolved
core/database/src/commonMain/kotlin/com/mifos/room/helper/ClientDaoHelper.kt
Outdated
Show resolved
Hide resolved
...rd/src/commonMain/kotlin/com/mifos/feature/searchrecord/navigation/SearchRecordNavigation.kt
Show resolved
Hide resolved
| import com.mifos.core.model.objects.searchrecord.GenericSearchRecord | ||
| import org.jetbrains.compose.resources.StringResource | ||
|
|
||
| sealed interface SearchRecordUiState { |
There was a problem hiding this comment.
don't use UiState approach now you are using BaseViewModel approach so that use dialogState approach take reference from other viewModel how there will be loading, error and success state handle.
we have shifted from UiState to dialogState so please follow this approach.
reference :
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt`:
- Around line 315-327: The identifyDocuments prop is incorrectly set to
record.name (duplicating type) when it should be the document identifier; update
the MifosActionsIdentifierListingComponent call to pass
record.metadata[Constants.DOCUMENT_KEY] (or another specific metadata key that
represents the document identifier) for the identifyDocuments parameter instead
of record.name, ensuring identifyDocuments uniquely identifies the document
while leaving type as record.name and keeping other props (id, key, status,
description, etc.) unchanged.
🧹 Nitpick comments (2)
core/database/src/commonMain/kotlin/com/mifos/room/helper/ClientDaoHelper.kt (1)
370-395: ReplaceprintStackTrace()with structured logging.The backup/restore pattern addresses the atomicity concern raised previously. However,
restoreException.printStackTrace()on line 390 is not suitable for production. If the restore fails, this critical data loss event would only appear in stderr and could be easily missed.Consider using a proper logging framework to record the failure with appropriate severity.
♻️ Suggested improvement
} catch (restoreException: Exception) { - restoreException.printStackTrace() + // TODO: Use proper logging (e.g., Timber, Napier, or platform logger) + // Log.e("ClientDaoHelper", "Failed to restore identifiers after insert failure", restoreException) + throw IllegalStateException("Failed to restore identifiers after insert failure", restoreException) }Alternatively, if silently failing the restore is acceptable, at minimum use structured logging rather than
printStackTrace().feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt (1)
40-50: Consider extracting display titles for localization.The
displayTitlevalues are hardcoded in the ViewModel ("Address" and "Identifiers"). While the final label formatting usesstringResourcein the UI layer, these base titles should ideally come from string resources as well for full i18n support.Additionally, there's a pluralization inconsistency: "Address" (singular) vs "Identifiers" (plural).
♻️ Suggested approach
Pass the
RecordTypeto the state and resolve the display name in the UI layer using string resources:init { mutableStateFlow.update { it.copy( - displayTitle = when (recordType) { - RecordType.ADDRESS -> "Address" - RecordType.IDENTIFIER -> "Identifiers" - }, + recordType = recordType, ) } observeSearchQuery() }Then in
SearchRecordScreen, use:val displayTitle = when (state.recordType) { RecordType.ADDRESS -> stringResource(Res.string.search_record_address) RecordType.IDENTIFIER -> stringResource(Res.string.search_record_identifiers) }
...ure/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt
Show resolved
Hide resolved
| */ | ||
| package com.mifos.feature.searchrecord | ||
|
|
||
| import com.mifos.core.model.objects.searchrecord.GenericSearchRecord |
There was a problem hiding this comment.
don't create separate file, implement this in viewModel
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt`:
- Around line 59-67: The ClearSearch handler in SearchRecordViewModel currently
resets searchQuery, searchRecords, and isNoResultsFound but does not reset
dialogState, which can leave a loading dialog visible; update the
SearchRecordAction.ClearSearch branch where mutableStateFlow.update is called to
also clear or set dialogState to the idle/hidden value used in your state model
(e.g., null or DialogState.Hidden) so the loading indicator is dismissed when a
user clears the search.
- Around line 89-96: When query.isBlank() in SearchRecordViewModel, you
currently clear searchRecords and cancel searchJob but forget to reset
dialogState; update the same mutableStateFlow.update block (or immediately
after) to also clear or reset dialogState (e.g., set it to the default/empty
state used elsewhere) so any loading indicator is removed when the query becomes
blank and the searchJob is cancelled.
🧹 Nitpick comments (1)
feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt (1)
41-49: HardcodeddisplayTitlebreaks internationalization.The
displayTitleis set using hardcoded strings "Address" and "Identifiers", which won't be localized. Consider using string resources, similar to howerror_searching_recordsis handled elsewhere.🌐 Suggested fix
Add string resources for the display titles and use them:
mutableStateFlow.update { it.copy( displayTitle = when (recordType) { - RecordType.ADDRESS -> "Address" - RecordType.IDENTIFIER -> "Identifiers" + RecordType.ADDRESS -> Res.string.record_type_address + RecordType.IDENTIFIER -> Res.string.record_type_identifiers }, ) }You'll need to change
displayTitleinSearchRecordStatefromStringtoStringResourceand resolve it in the UI layer usingstringResource().
.../search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt
Show resolved
Hide resolved
.../search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordViewModel.kt
Show resolved
Hide resolved
3 participants
Fixes - Jira-#628
I have added a generic search module. Search is powered by the local Room database for instant results and uses a unified
GenericSearchRecordmodel so different record types (currently Address and Identifier) can be handled by a single reusable UI.Video:
Search_Address.webm
Search_Identifier.mp4
Summary by CodeRabbit
New Features
Chores
Localization