Skip to content

[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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ fun CountryListAdapter(
viewModel.onCountrySelect(country)
},
onRefreshTap = { viewModel.onRefreshTap() },
onFailOtherTap = { viewModel.onFailOtherTap() }
onFailOtherTap = { viewModel.onFailOtherTap() },
onSearchQueryChange = { query ->
viewModel.updateSearchQuery(query)
},
filteredContinents = viewModel.filteredContinents
)

FloatingAlertNotifier(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import androidx.compose.foundation.layout.*
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material.Button
import androidx.compose.material.Text
import androidx.compose.material.TextField
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
Expand All @@ -15,8 +16,8 @@ import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import com.example.domainmodels.Continent
import com.example.domainmodels.Country
import com.example.feature.countrylist.componenets.CountryListButton
import com.example.feature.countrylist.componenets.CountryListList
import com.example.feature.countrylist.components.CountryListButton
import com.example.feature.countrylist.components.CountryListList
import com.example.features.R
import com.example.uicomponents.library.ProgressIndicator
import com.example.viewmodels.CountryListViewModel
Expand All @@ -26,14 +27,30 @@ fun CountryListPage(
listUiState: CountryListViewModel.UiState,
onCountrySelect: ((Country) -> Unit)? = null,
onRefreshTap: (() -> Unit)? = null,
onFailOtherTap: (() -> Unit)? = null
onFailOtherTap: (() -> Unit)? = null,
onSearchQueryChange: ((String) -> Unit)? = null,
filteredContinents: List<Continent> = emptyList()
) {
Box {
ProgressIndicator(isLoading = listUiState.isLoading)
Column(modifier = Modifier.fillMaxHeight()) {
Row(
modifier = Modifier
.fillMaxWidth()
.padding(8.dp),
verticalAlignment = Alignment.CenterVertically
) {
TextField(
value = listUiState.searchQuery,
onValueChange = { onSearchQueryChange?.invoke(it) },
label = { Text("Search countries") },
Copy link
Preview

Copilot AI Jul 4, 2025

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.

modifier = Modifier.fillMaxWidth(),
singleLine = true
)
}
Column(modifier = Modifier.weight(1f)) {
CountryListList(
list = listUiState.continents,
list = filteredContinents,
onClick = onCountrySelect
)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.example.feature.countrylist.componenets
package com.example.feature.countrylist.components

import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.example.feature.countrylist.componenets
package com.example.feature.countrylist.components

import androidx.compose.foundation.ExperimentalFoundationApi
import androidx.compose.foundation.background
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,28 @@ class CountryListViewModel(
val error: CountryListError? = null,
val serverStatus: ServerStatus? = null,
val navigationTarget: Country? = null,
var searchQuery: String = "",
Copy link
Preview

Copilot AI Jul 4, 2025

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.

Suggested change
var searchQuery: String = "",
val searchQuery: String = "",

Copilot uses AI. Check for mistakes.

)

val filteredContinents: List<Continent>
get() {
val query = _state.value?.searchQuery.orEmpty()
return _state.value?.continents
?.map { continent ->
val filteredCountries = continent.countries.filter {
it.countryName.contains(query, ignoreCase = true) ||
it.regionCode.contains(query, ignoreCase = true)
}
continent.copy(countries = filteredCountries)
}
?.filter { it.countries.isNotEmpty() }
?: emptyList()
}

fun updateSearchQuery(query: String) {
_state.onNext(_state.value!!.copy(searchQuery = query))
Copy link
Preview

Copilot AI Jul 4, 2025

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.

Suggested change
_state.onNext(_state.value!!.copy(searchQuery = query))
_state.value?.let { currentState ->
_state.onNext(currentState.copy(searchQuery = query))
}

Copilot uses AI. Check for mistakes.

}

private val _state: BehaviorSubject<UiState> = BehaviorSubject.createDefault(UiState())
val state: Observable<UiState> = _state

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,44 @@ class CountryListViewModelTests: KoinTest {
}
}
}

@Nested
@DisplayName("filteredContinents")
inner class FilteredContinents {
@Test
fun `returns only continents with matching countries`() {
// Arrange: set up continents with countries
val continents = listOf(
com.example.domainmodels.Continent(
name = "Europe",
countries = listOf(
com.example.domainmodels.Country(regionCode = "FR"),
com.example.domainmodels.Country(regionCode = "DE")
)
),
com.example.domainmodels.Continent(
name = "Asia",
countries = listOf(
com.example.domainmodels.Country(regionCode = "JP"),
com.example.domainmodels.Country(regionCode = "CN")
)
)
)
// Set continents in state
val stateField = CountryListViewModel::class.java.getDeclaredField("_state")
Copy link
Preview

Copilot AI Jul 4, 2025

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.

stateField.isAccessible = true
val subject = stateField.get(viewModel) as io.reactivex.rxjava3.subjects.BehaviorSubject<CountryListViewModel.UiState>
subject.onNext(CountryListViewModel.UiState(continents = continents))

// Act: update search query
viewModel.updateSearchQuery("fr")

// Assert: only Europe with France should match
val filtered = viewModel.filteredContinents
filtered.size.shouldBeEqualTo(1)
filtered[0].name.shouldBeEqualTo("Europe")
filtered[0].countries.size.shouldBeEqualTo(1)
filtered[0].countries[0].regionCode.shouldBeEqualTo("FR")
}
}
}