feat(search-record): implemented generic search for addresses and ide… #2586
Conversation
📝 WalkthroughWalkthroughAdds a new SearchRecord feature (UI, ViewModel, DI, navigation, strings), core models/repository/local data source, DB entities/DAO/helper methods, and integrates search wiring into authenticated navigation and client screens. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as SearchRecordScreen
participant VM as SearchRecordViewModel
participant Repo as SearchRecordRepository
participant LocalDS as SearchRecordLocalDataSource
participant DAO as ClientDao
participant DB as Database
User->>UI: enter query
UI->>VM: onSearchQueryChanged(query)
Note over VM: Debounce ~300ms
VM->>Repo: searchRecords(recordType, query)
VM->>UI: emit Loading
Repo->>LocalDS: searchRecords(recordType, query)
alt ADDRESS
LocalDS->>DAO: searchAddressesByQuery(query)
else IDENTIFIER
LocalDS->>DAO: searchIdentifiersByQuery(query)
end
DAO->>DB: SQL query (LIKE)
DB-->>DAO: results
DAO-->>LocalDS: Flow<Entities>
LocalDS->>LocalDS: map -> GenericSearchRecord
LocalDS-->>Repo: Flow<List<GenericSearchRecord>>
Repo->>Repo: wrap Result.success / catch -> Result.failure
Repo-->>VM: Flow<Result<List<GenericSearchRecord>>>
VM->>VM: update uiState (Success/NoResults/Error)
VM-->>UI: uiState emission
UI->>User: render results
User->>UI: select record
UI->>Caller: onRecordSelected(record)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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
| return dataManagerIdentifiers.getClientListIdentifiers(clientId) | ||
| .asDataStateFlow() | ||
| .onEach { dataState -> | ||
| if (dataState is DataState.Success) { | ||
| val entities = dataState.data.map { identifier -> | ||
| ClientIdentifierEntity( | ||
| id = identifier.id ?: -1, | ||
| clientId = identifier.clientId ?: clientId.toInt(), | ||
| documentKey = identifier.documentKey ?: "", | ||
| documentTypeName = identifier.documentType?.name ?: "", | ||
| documentTypeId = identifier.documentType?.id ?: -1, | ||
| description = identifier.description ?: "", | ||
| status = identifier.status ?: "", | ||
| ) | ||
| } | ||
| clientDaoHelper.insertIdentifiers(entities) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ClientIdentifiersRepositoryImp.kt" -type fRepository: openMF/android-client
Length of output: 166
🏁 Script executed:
cat -n ./core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt | sed -n '25,60p'Repository: openMF/android-client
Length of output: 2020
🏁 Script executed:
find . -name "ClientDaoHelper.kt" -type fRepository: openMF/android-client
Length of output: 143
🏁 Script executed:
cat -n ./core/database/src/commonMain/kotlin/com/mifos/room/helper/ClientDaoHelper.kt | head -150Repository: openMF/android-client
Length of output: 6745
🏁 Script executed:
cat -n ./core/database/src/commonMain/kotlin/com/mifos/room/helper/ClientDaoHelper.kt | grep -A 10 "insertIdentifiers"Repository: openMF/android-client
Length of output: 673
🏁 Script executed:
cat -n ./core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.ktRepository: openMF/android-client
Length of output: 4123
🏁 Script executed:
rg "onEach.*insertIdentifiers|insertIdentifiers.*onEach" --type kt -B 3 -A 3Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
rg "onEach.*insertIdentifiers|insertIdentifiers.*onEach" --type kotlin -B 3 -A 3Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
rg "fun onEach" --type-list | head -20Repository: openMF/android-client
Length of output: 454
🏁 Script executed:
rg "\.onEach" --max-count 20 -B 2 -A 5Repository: openMF/android-client
Length of output: 13920
🏁 Script executed:
grep -r "\.onEach.*suspend\|onEach.*insert\|onEach.*save" --include="*.kt" -B 2 -A 2 | head -50Repository: openMF/android-client
Length of output: 672
🏁 Script executed:
grep -r "suspend fun insert\|suspend fun save" --include="*.kt" | grep -i identifierRepository: openMF/android-client
Length of output: 396
🏁 Script executed:
cat -n ./core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerCharge.kt | grep -B 5 -A 15 "saveClientCharges"Repository: openMF/android-client
Length of output: 891
🏁 Script executed:
grep -r "\.onEach.*try\|try.*onEach" --include="*.kt" -B 2 -A 5 | head -60Repository: openMF/android-client
Length of output: 914
🏁 Script executed:
find . -name "*.kt" -path "*/repositoryImp/*" -exec grep -l "onEach" {} \; | head -5Repository: openMF/android-client
Length of output: 166
🏁 Script executed:
grep -r "\.onEach.*suspend\|suspend.*onEach" --include="*.kt" -A 8 | head -80Repository: openMF/android-client
Length of output: 47
Wrap the database insert in error handling to prevent flow cancellation.
The insertIdentifiers suspend call can throw (constraint violations, IO errors) and cancel the entire flow, causing the UI to lose the network result. Isolate the side-effect with a try-catch block so local persistence failures don't break the main data path.
🛠️ Suggested resilience guard
.onEach { dataState ->
if (dataState is DataState.Success) {
val entities = dataState.data.map { identifier ->
ClientIdentifierEntity(
id = identifier.id ?: -1,
clientId = identifier.clientId ?: clientId.toInt(),
documentKey = identifier.documentKey ?: "",
documentTypeName = identifier.documentType?.name ?: "",
documentTypeId = identifier.documentType?.id ?: -1,
description = identifier.description ?: "",
status = identifier.status ?: "",
)
}
- clientDaoHelper.insertIdentifiers(entities)
+ try {
+ clientDaoHelper.insertIdentifiers(entities)
+ } catch (_: Throwable) {
+ // TODO: log or report, but don't fail the upstream flow
+ }
}
}🤖 Prompt for AI Agents
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt`
around lines 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.
...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
| ClientAddressEntity::class, | ||
| ClientIdentifierEntity::class, |
There was a problem hiding this comment.
Same migration concern applies to native platform.
The same database schema change without version bump issue noted in the Android variant applies here. Ensure migration strategy is consistent across all platforms.
The entity additions correctly mirror the Android configuration, maintaining KMP platform parity.
🤖 Prompt for AI Agents
In `@core/database/src/nativeMain/kotlin/com/mifos/room/MifosDatabase.kt` around
lines 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.
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/searchrecord/RecordType.kt
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
1 participant
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
Data & Storage
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.