Merged
Conversation
… sort UI - Create SortOrder enum in domain layer with toGithubParam() method - Update SortBy.toGithubParams() to toGithubSortParam() returning only field name - Update SearchRepository interface and implementation to accept SortOrder - Add SortOrder to SearchState and SearchAction - Handle new sort actions in SearchViewModel - Update SortByBottomSheet with sort order toggle chips - Wire sort button and dialog into SearchRoot - Add string resources for sort order labels Co-authored-by: bilalahmadsheikh <169471620+bilalahmadsheikh@users.noreply.github.com>
Co-authored-by: bilalahmadsheikh <169471620+bilalahmadsheikh@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the search feature’s sorting capabilities by introducing an explicit sort-order (ascending/descending) alongside existing sort-by options, and wires it through the UI → ViewModel → repository → GitHub request layer.
Changes:
- Add
SortOrderdomain model and presentation label mapping for localization. - Update search state/actions/viewmodel and repository interface/impl to accept and apply
sortOrder. - Add UI controls for selecting sort order and localize existing “Close” / “Sort by” strings.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/utils/SortOrderMapper.kt | Adds localized labels for SortOrder. |
| feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/components/SortByBottomSheet.kt | Adds sort-order selection UI alongside sort-by options. |
| feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt | Passes sortOrder into searches and reacts to OnSortOrderSelected. |
| feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchState.kt | Stores selected sort order and sort dialog visibility. |
| feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchRoot.kt | Wires sort dialog visibility and adds a “Sort” chip entry point. |
| feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchAction.kt | Introduces actions for sort order selection and dialog toggling. |
| feature/search/domain/src/commonMain/kotlin/zed/rainxch/domain/repository/SearchRepository.kt | Extends repository API to include sortOrder. |
| feature/search/domain/src/commonMain/kotlin/zed/rainxch/domain/model/SortOrder.kt | Adds SortOrder enum and GitHub query param mapping. |
| feature/search/domain/src/commonMain/kotlin/zed/rainxch/domain/model/SortBy.kt | Refactors GitHub sort param mapping to return sort-only. |
| feature/search/data/src/commonMain/kotlin/zed/rainxch/search/data/repository/SearchRepositoryImpl.kt | Applies sort/order params and adds sortOrder to caching key. |
| core/presentation/src/commonMain/composeResources/values/strings.xml | Adds localized strings for sort order and “Sort” label. |
Comments suppressed due to low confidence (1)
feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/components/SortByBottomSheet.kt:38
SortByBottomSheetusesAlertDialograther than aModalBottomSheet, so the name is misleading (and inconsistent withLanguageFilterBottomSheet). Consider renaming this composable toSortByDialog(or switching implementation to a bottom sheet) to avoid confusing callers and future maintenance issues.
fun SortByBottomSheet(
sortByOptions: List<SortBy>,
selectedSortBy: SortBy,
selectedSortOrder: SortOrder,
onSortBySelected: (SortBy) -> Unit,
onSortOrderSelected: (SortOrder) -> Unit,
onDismissRequest: () -> Unit,
modifier: Modifier = Modifier
) {
AlertDialog(
onDismissRequest = onDismissRequest,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt
Show resolved
Hide resolved
| ): String { | ||
| val queryHash = query.trim().lowercase().hashCode().toUInt().toString(16) | ||
| return "search:$queryHash:${platform.name}:${language.name}:${sortBy.name}:page$page" | ||
| return "search:$queryHash:${platform.name}:${language.name}:${sortBy.name}:${sortOrder.name}:page$page" |
There was a problem hiding this comment.
searchCacheKey(...) includes sortOrder unconditionally, but the request only applies sortOrder when sortBy.toGithubSortParam() is non-null. For SortBy.BestMatch, this will create distinct cache keys (and cache entries / misses) for ascending vs descending even though the upstream request is identical. Consider omitting/normalizing sortOrder in the cache key when sortBy doesn’t produce a sort param.
Suggested change
| return "search:$queryHash:${platform.name}:${language.name}:${sortBy.name}:${sortOrder.name}:page$page" | |
| val effectiveSortOrder = if (sortBy.toGithubSortParam() == null) { | |
| "NONE" | |
| } else { | |
| sortOrder.name | |
| } | |
| return "search:$queryHash:${platform.name}:${language.name}:${sortBy.name}:$effectiveSortOrder:page$page" |
...tation/src/commonMain/kotlin/zed/rainxch/search/presentation/components/SortByBottomSheet.kt
Show resolved
Hide resolved
Comment on lines
+74
to
+91
| Spacer(Modifier.height(4.dp)) | ||
|
|
||
| Row( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.spacedBy(8.dp), | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| SortOrder.entries.forEach { order -> | ||
| FilterChip( | ||
| selected = order == selectedSortOrder, | ||
| onClick = { onSortOrderSelected(order) }, | ||
| label = { | ||
| Text( | ||
| text = stringResource(order.label()), | ||
| style = MaterialTheme.typography.bodyMedium | ||
| ) | ||
| } | ||
| ) |
There was a problem hiding this comment.
Sort order selection is offered even when selectedSortBy is BestMatch, but the data layer only sends the order param when sort is non-null. That means changing sort order while on BestMatch has no effect (but still triggers a search), which is confusing UX. Consider hiding/disable the sort-order chips when selectedSortBy == SortBy.BestMatch (or otherwise indicate it has no effect).
Suggested change
| Spacer(Modifier.height(4.dp)) | |
| Row( | |
| modifier = Modifier.fillMaxWidth(), | |
| horizontalArrangement = Arrangement.spacedBy(8.dp), | |
| verticalAlignment = Alignment.CenterVertically | |
| ) { | |
| SortOrder.entries.forEach { order -> | |
| FilterChip( | |
| selected = order == selectedSortOrder, | |
| onClick = { onSortOrderSelected(order) }, | |
| label = { | |
| Text( | |
| text = stringResource(order.label()), | |
| style = MaterialTheme.typography.bodyMedium | |
| ) | |
| } | |
| ) | |
| if (selectedSortBy != SortBy.BestMatch) { | |
| Spacer(Modifier.height(4.dp)) | |
| Row( | |
| modifier = Modifier.fillMaxWidth(), | |
| horizontalArrangement = Arrangement.spacedBy(8.dp), | |
| verticalAlignment = Alignment.CenterVertically | |
| ) { | |
| SortOrder.entries.forEach { order -> | |
| FilterChip( | |
| selected = order == selectedSortOrder, | |
| onClick = { onSortOrderSelected(order) }, | |
| label = { | |
| Text( | |
| text = stringResource(order.label()), | |
| style = MaterialTheme.typography.bodyMedium | |
| ) | |
| } | |
| ) | |
| } |
Member
|
Hey @bilalahmadsheikh Thank you for your contribution, all the changes look quite good to me but i would like that you would also add localized strings for other languages to so all the users will have sort labels in their local language 👍 |
Member
|
@bilalahmadsheikh When you finish with this please ping me here so i could review and merge 👍 |
Contributor
Author
|
@rainxchzed I have completed. |
Member
|
Cool! Let it get merged then, thanks a lot for your contribution! |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughA new sort order feature is implemented by introducing a Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as SearchUI
participant VM as SearchViewModel
participant Repo as SearchRepository
participant Data as SearchRepositoryImpl
User->>UI: Click sort order button
UI->>VM: dispatch OnToggleSortByDialogVisibility
VM->>VM: toggle isSortByDialogVisible
UI->>UI: display SortByBottomSheet
User->>UI: select sort order (Ascending/Descending)
UI->>VM: dispatch OnSortOrderSelected(sortOrder)
VM->>VM: update selectedSortOrder
VM->>VM: reset to page 1
VM->>VM: cancel debounce timer
VM->>Repo: searchRepositories(..., sortOrder)
Repo->>Data: searchRepositories(..., sortOrder)
Data->>Data: generate cache key with sortOrder
Data->>Data: construct query with sortOrder.toGithubParam()
Data-->>Repo: return search results
Repo-->>VM: emit PaginatedDiscoveryRepositories
VM->>VM: update search results state
VM-->>UI: emit new state
UI-->>User: display filtered results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
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.
fixed import format and added sort order direction
Summary by CodeRabbit