Option to sort vaults by vault name or cloud
Hi guys.
This is my proposal for #347 issue. Currently the two ways are supported: alphabetical and by CloudType enum. Since this action is kinda destructive (since the user could have their own sorting via DnD) there is confirmation modal with proper text. We can, of course, add asc/desc option, but I didn't want to complicate things initially as even such sorting is better than none. Feel free to leave comments
Walkthrough
This pull request adds a vault sorting feature to the presentation layer. It introduces a new enum VaultListSortOption with NAME and LOCATION. VaultListPresenter gains a cachedVaults state, locale-aware nameComparator() and locationComparator(), and a public onSortOverrideConfirmed() to reorder, reindex, persist (via SaveVaultsUseCase), and refresh the view. A new SortOverrideConfirmationDialog presents radio options and a Callback; VaultListActivity implements the callback and delegates selection to the presenter. UI resources added: menu item, dialog layout, and strings.
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- Presenter sorting logic and the
cachedVaultslifecycle (population, refresh, potential staleness). - Integration of
SortOverrideConfirmationDialog.CallbackthroughVaultListActivityto the presenter (wiring and lifecycle). - Persistence flow in
onSortOverrideConfirmed()(reindexing andSaveVaultsUseCaseusage). - Comparator implementations for locale-aware name sorting and location sorting, including null/edge-case handling.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | ⚠️ Warning | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title Check | ✅ Passed | The pull request title "Option to sort vaults by vault name or cloud" directly aligns with the changeset's primary objective. The changes introduce a sorting feature that allows users to sort vaults by two criteria: vault name (alphabetically) and cloud location (by CloudType), which is exactly what the title conveys. The title is concise, specific, and not vague or generic, making it clear to reviewers scanning the history what the main change entails. |
| Description Check | ✅ Passed | The pull request description is clearly related to the changeset. It references issue #347, explicitly mentions the two supported sorting methods (alphabetical and by CloudType enum), explains the inclusion of a confirmation modal due to the destructive nature of the action, and provides design rationale for not including ascending/descending options initially. The description also includes screenshots demonstrating the UI implementation, which support understanding of the changes made throughout the codebase. |
✨ Finishing touches
- [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
Hey, thank you for your PR! :grin: I can't say much about the code at this point, but I'd like to put two notes up for discussion:
- I think it'd be easier (as in less complexity, less dialogs and less destructive operations) to apply the sorting dynamically while populating the list view. The user could then pick between location/name/custom-sorting.
Location/Nameeach would employ a comparator and disable DnD behavior whileCustomwould retain the current behavior. - Building on that we could then switch to a less intrusive component such as menu.
Hi, @JaniruTEC. Could you clarify one thing: when user selects "custom", do you want to just unlock DnD or you want to keep previous "custom" sorting saved somewhere when user selects "Location/Name" and restore it when users moves back to custom?
- If we decide NOT to keep previous custom sorting when user moves to non-custom sort, than the operation is quite destructive still (and possible some warning won't hurt).
- If we decide to keep previous custom sorting order, then it raises quite a lot of questions like "what if user has some custom sorting, moved to non-custom, deleted X vaults and added Y, where should they be placed when user moves back to custom order"