fix(search): use of xml strings in search filter instead of hardcoded values#2621
Open
amanna13 wants to merge 2 commits intoopenMF:developmentfrom
Open
fix(search): use of xml strings in search filter instead of hardcoded values#2621amanna13 wants to merge 2 commits intoopenMF:developmentfrom
amanna13 wants to merge 2 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughRefactors search filter options to use localized string resources: replaces string-array usage with per-option label/value resources (default locale) and per-option labels in Spanish; updates FilterOption to hold StringResource references and modifies UI components and ViewModel to resolve those resources at runtime. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt (1)
171-195:⚠️ Potential issue | 🟠 MajorAdd the missing
FilterOption.Centersdata object and imports — XML string resources are currently unused.Both
values/feature_search_strings.xmlandvalues-es/feature_search_strings.xmldefinefeature_search_filter_options_centers_labelandfeature_search_filter_options_centers_value(Spanish: "Centros"), but no correspondingdata object Centersexists in theFilterOptionsealed class. These resources are dead code. Either add the missing Centers data object with its imports, or remove the unused resource strings from both XML locale files.➕ Proposed addition of the missing
Centersdata objectIn
SearchViewModel.kt, add the missing import and data object:import androidclient.feature.search.generated.resources.feature_search_filter_options_groups_value +import androidclient.feature.search.generated.resources.feature_search_filter_options_centers_label +import androidclient.feature.search.generated.resources.feature_search_filter_options_centers_value import androidclient.feature.search.generated.resources.feature_search_filter_options_loans_labeldata object Groups : FilterOption( labelRes = Res.string.feature_search_filter_options_groups_label, valueRes = Res.string.feature_search_filter_options_groups_value, ) + data object Centers : FilterOption( + labelRes = Res.string.feature_search_filter_options_centers_label, + valueRes = Res.string.feature_search_filter_options_centers_value, + ) + data object LoanAccounts : FilterOption(- val values = listOf(Clients, Groups, LoanAccounts, SavingsAccounts) + val values = listOf(Clients, Groups, Centers, LoanAccounts, SavingsAccounts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt` around lines 171 - 195, Add the missing FilterOption.Centers data object and corresponding import usage so the XML string resources are used: inside the sealed class FilterOption add a data object Centers with labelRes = Res.string.feature_search_filter_options_centers_label and valueRes = Res.string.feature_search_filter_options_centers_value, and include Centers in the companion object values list (FilterOption.values). Alternatively, if you prefer to delete unused resources instead, remove feature_search_filter_options_centers_label and feature_search_filter_options_centers_value from both locale XMLs.
🧹 Nitpick comments (1)
feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt (1)
108-110: Consider replacingvalueResAPI strings with constants to decouple the ViewModel from the UI resource layer.
getString(it.valueRes)resolves a locale string resource inside the ViewModel just to get an API parameter like"clients"or"loans". These values are not display text and must never vary by locale. Callingorg.jetbrains.compose.resources.getString(a Compose Multiplatform UI primitive) in the ViewModel breaks architectural separation and introduces an unnecessary suspend-point for what are effectively compile-time constants.A cleaner approach is to add a plain
val value: StringtoFilterOptiondirectly:♻️ Proposed refactor to remove API-value resource resolution from the ViewModel
-sealed class FilterOption(val labelRes: StringResource, val valueRes: StringResource) { +sealed class FilterOption(val labelRes: StringResource, val value: String) { data object Clients : FilterOption( labelRes = Res.string.feature_search_filter_options_clients_label, - valueRes = Res.string.feature_search_filter_options_clients_value, + value = "clients", ) data object Groups : FilterOption( labelRes = Res.string.feature_search_filter_options_groups_label, - valueRes = Res.string.feature_search_filter_options_groups_value, + value = "groups", ) data object LoanAccounts : FilterOption( labelRes = Res.string.feature_search_filter_options_loans_label, - valueRes = Res.string.feature_search_filter_options_loans_value, + value = "loans", ) data object SavingsAccounts : FilterOption( labelRes = Res.string.feature_search_filter_options_savings_label, - valueRes = Res.string.feature_search_filter_options_savings_value, + value = "savingsaccounts", ) }Then in
getSearchResult():- resources = state.value.selectedFilter?.let { getString(it.valueRes) }, + resources = state.value.selectedFilter?.value,This also lets you drop the
getStringandStringResourceimports from the ViewModel and removes the*_valueXML string resources entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt` around lines 108 - 110, The ViewModel is resolving UI string resources (getString(it.valueRes)) for API parameters; add a plain String property to the FilterOption data/class (e.g., val value: String) and populate it where FilterOption instances are created, then update SearchViewModel.getSearchResult()/search invocation to pass state.value.selectedFilter?.value to searchRepository.searchResources instead of getString(it.valueRes); remove the getString import and any StringResource usage plus the corresponding *_value XML resources since API values are now compile-time constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/search/src/commonMain/composeResources/values/feature_search_strings.xml`:
- Around line 26-35: The API parameter string resources
(feature_search_filter_options_clients_value,
feature_search_filter_options_groups_value,
feature_search_filter_options_centers_value,
feature_search_filter_options_loans_value,
feature_search_filter_options_savings_value) should be marked non-translatable
to prevent translators from altering request parameters; update each of those
<string> entries to include translatable="false" while leaving the corresponding
*_label entries unchanged so UI text remains translatable.
---
Outside diff comments:
In
`@feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt`:
- Around line 171-195: Add the missing FilterOption.Centers data object and
corresponding import usage so the XML string resources are used: inside the
sealed class FilterOption add a data object Centers with labelRes =
Res.string.feature_search_filter_options_centers_label and valueRes =
Res.string.feature_search_filter_options_centers_value, and include Centers in
the companion object values list (FilterOption.values). Alternatively, if you
prefer to delete unused resources instead, remove
feature_search_filter_options_centers_label and
feature_search_filter_options_centers_value from both locale XMLs.
---
Duplicate comments:
In
`@feature/search/src/commonMain/composeResources/values-es/feature_search_strings.xml`:
- Around line 26-35: The localized file incorrectly translates the API token
strings — update the value entries for
feature_search_filter_options_clients_value,
feature_search_filter_options_groups_value,
feature_search_filter_options_centers_value,
feature_search_filter_options_loans_value, and
feature_search_filter_options_savings_value so they remain the original API
identifiers (clients, groups, centers, loans, savingsaccounts) rather than
localized text; locate these string resources in the feature_search_strings.xml
and replace the translated values with the exact API tokens to ensure the app
sends the correct unlocalized values.
---
Nitpick comments:
In
`@feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt`:
- Around line 108-110: The ViewModel is resolving UI string resources
(getString(it.valueRes)) for API parameters; add a plain String property to the
FilterOption data/class (e.g., val value: String) and populate it where
FilterOption instances are created, then update
SearchViewModel.getSearchResult()/search invocation to pass
state.value.selectedFilter?.value to searchRepository.searchResources instead of
getString(it.valueRes); remove the getString import and any StringResource usage
plus the corresponding *_value XML resources since API values are now
compile-time constants.
feature/search/src/commonMain/composeResources/values/feature_search_strings.xml
Outdated
Show resolved
Hide resolved
therajanmaurya
requested changes
Feb 22, 2026
| <item>Savings Accounts</item> | ||
| </string-array> | ||
|
|
||
| <string-array name="feature_search_filter_option_values"> |
Member
There was a problem hiding this comment.
Why removed string array?
Member
Author
There was a problem hiding this comment.
The string-array was originally introduced during "Migrate Search Fragment To Compose"
The filter options are mainly structured data of the
- label (these are localized)
- values (these are the constants - localization not needed)
I believe using string-array in this context would introduce additional complexity. It would be depended on the positional indexing between the two arrays.
amanna13
force-pushed
the
refactor/xml-string-search
branch
from
9ddea74 to
60f0b72
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt (1)
171-195: Consider keeping API parameter values as plainStringconstants rather thanStringResource.
valueResentries are all markedtranslatable="false"— they are effectively compile-time constants. Resolving them viagetString()(a suspend call) on every search invocation adds unnecessary async overhead for values that never change. Labels genuinely need localization and should stay asStringResource; values do not.♻️ Suggested refactor
-sealed class FilterOption(val labelRes: StringResource, val valueRes: StringResource) { +sealed class FilterOption(val labelRes: StringResource, val value: String) { data object Clients : FilterOption( labelRes = Res.string.feature_search_filter_options_clients_label, - valueRes = Res.string.feature_search_filter_options_clients_value, + value = "clients", ) data object Groups : FilterOption( labelRes = Res.string.feature_search_filter_options_groups_label, - valueRes = Res.string.feature_search_filter_options_groups_value, + value = "groups", ) data object LoanAccounts : FilterOption( labelRes = Res.string.feature_search_filter_options_loans_label, - valueRes = Res.string.feature_search_filter_options_loans_value, + value = "loans", ) data object SavingsAccounts : FilterOption( labelRes = Res.string.feature_search_filter_options_savings_label, - valueRes = Res.string.feature_search_filter_options_savings_value, + value = "savingsaccounts", ) }Then in
getSearchResult():-resources = state.value.selectedFilter?.let { getString(it.valueRes) }, +resources = state.value.selectedFilter?.value,This also removes the need to import
StringResource,getString, and the four*_valueresource references inSearchViewModel.kt, and allows the*_valuestring resources in the XML (and their ES counterparts) to be removed entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt` around lines 171 - 195, The FilterOption sealed class currently stores API parameter values as StringResource causing suspend getString() calls; change the constructor to accept a plain String for the API value (e.g., rename valueRes -> value or similar) and update the data objects Clients, Groups, LoanAccounts, SavingsAccounts to supply compile-time string constants (e.g., "clients", "groups", "loanAccounts", "savingsAccounts"). Then update getSearchResult() to use the new plain String property instead of calling getString(), and remove now-unused imports (StringResource, getString) and the corresponding *_value XML resources. Ensure references to FilterOption.value in other code are updated to the new property name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/search/src/commonMain/composeResources/values/feature_search_strings.xml`:
- Around line 30-31: The EN/ES string resources
feature_search_filter_options_centers_label and
feature_search_filter_options_centers_value are unused because FilterOption in
SearchViewModel.kt only defines Clients, Groups, LoanAccounts, and
SavingsAccounts; either add a new FilterOption data object named Centers to the
sealed class (and update any imports/usages that populate/filter options) so the
resources are consumed, or remove both resource entries from the locale files;
reference the sealed class FilterOption and the file SearchViewModel.kt when
making the change and ensure any UI code that builds the filter list is updated
to include FilterOption.Centers if you add it.
---
Nitpick comments:
In
`@feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt`:
- Around line 171-195: The FilterOption sealed class currently stores API
parameter values as StringResource causing suspend getString() calls; change the
constructor to accept a plain String for the API value (e.g., rename valueRes ->
value or similar) and update the data objects Clients, Groups, LoanAccounts,
SavingsAccounts to supply compile-time string constants (e.g., "clients",
"groups", "loanAccounts", "savingsAccounts"). Then update getSearchResult() to
use the new plain String property instead of calling getString(), and remove
now-unused imports (StringResource, getString) and the corresponding *_value XML
resources. Ensure references to FilterOption.value in other code are updated to
the new property name.
feature/search/src/commonMain/composeResources/values/feature_search_strings.xml
Show resolved
Hide resolved
|
Member
|
Fix conflicts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes - Jira-#712
Screen_recording_20260222_133757.mp4
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
Refactor
Localization