-
Notifications
You must be signed in to change notification settings - Fork 2
[Summit prep] Add a search feature #19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a search feature to the country list by extending the ViewModel state, filtering logic, tests, and UI.
- Introduces
searchQuery
andupdateSearchQuery()
inCountryListViewModel
- Adds a computed
filteredContinents
property and corresponding unit test - Updates the Compose UI (
CountryListPage
) to show aTextField
and uses filtered data - Fixes package typos in component imports and wires the adapter to the new search APIs
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
viewmodels/src/test/kotlin/com/example/viewmodels/CountryListViewModelTests.kt | Adds FilteredContinents nested tests to verify search behavior |
viewmodels/src/main/java/com/example/viewmodels/CountryListViewModel.kt | Adds searchQuery to UiState , filteredContinents getter, and updateSearchQuery method |
feature/src/main/java/com/example/feature/countrylist/components/CountrySelectingList.kt | Corrects package name from componenets to components |
feature/src/main/java/com/example/feature/countrylist/components/CountrySelectingButton.kt | Corrects package name from componenets to components |
feature/src/main/java/com/example/feature/countrylist/CountryListPage.kt | Imports TextField , adds search bar UI, passes filteredContinents |
feature/src/main/java/com/example/feature/countrylist/CountryListAdapter.kt | Propagates onSearchQueryChange and binds filteredContinents |
Comments suppressed due to low confidence (1)
viewmodels/src/test/kotlin/com/example/viewmodels/CountryListViewModelTests.kt:113
- [nitpick] This test verifies filtering by region code but not by
countryName
; consider adding a test where the search query matches the country name to ensure full coverage of the filter logic.
viewModel.updateSearchQuery("fr")
} | ||
|
||
fun updateSearchQuery(query: String) { | ||
_state.onNext(_state.value!!.copy(searchQuery = query)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using !!
on _state.value
can throw a NullPointerException if value
is ever null; consider using a safe call (value?.copy(...)
) or ensuring the subject always has a non-null value before updating.
_state.onNext(_state.value!!.copy(searchQuery = query)) | |
_state.value?.let { currentState -> | |
_state.onNext(currentState.copy(searchQuery = query)) | |
} |
Copilot uses AI. Check for mistakes.
@@ -20,8 +20,28 @@ class CountryListViewModel( | |||
val error: CountryListError? = null, | |||
val serverStatus: ServerStatus? = null, | |||
val navigationTarget: Country? = null, | |||
var searchQuery: String = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Defining searchQuery
as a var
in a data-style state class breaks immutability; consider changing it to val
and enforcing updates only via copy()
to keep UiState
pure.
var searchQuery: String = "", | |
val searchQuery: String = "", |
Copilot uses AI. Check for mistakes.
) | ||
) | ||
// Set continents in state | ||
val stateField = CountryListViewModel::class.java.getDeclaredField("_state") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The test uses reflection to access a private field _state
; consider exposing a test-only setter or using a public API to inject state, reducing coupling to the implementation details.
Copilot uses AI. Check for mistakes.
TextField( | ||
value = listUiState.searchQuery, | ||
onValueChange = { onSearchQueryChange?.invoke(it) }, | ||
label = { Text("Search countries") }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label string is hardcoded; for localization support, consider moving it into strings.xml
and referring to it via a resource ID.
Copilot uses AI. Check for mistakes.
No description provided.