android icon indicating copy to clipboard operation
android copied to clipboard

Option to sort vaults by vault name or cloud

Open Vladyslav-Soldatenko opened this issue 2 months ago • 4 comments

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 image image

Vladyslav-Soldatenko avatar Nov 01 '25 09:11 Vladyslav-Soldatenko

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 01 '25 09:11 CLAassistant

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 cachedVaults lifecycle (population, refresh, potential staleness).
  • Integration of SortOverrideConfirmationDialog.Callback through VaultListActivity to the presenter (wiring and lifecycle).
  • Persistence flow in onSortOverrideConfirmed() (reindexing and SaveVaultsUseCase usage).
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 01 '25 10:11 coderabbitai[bot]

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:

  1. 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/Name each would employ a comparator and disable DnD behavior while Custom would retain the current behavior.
  2. Building on that we could then switch to a less intrusive component such as menu.

JaniruTEC avatar Nov 01 '25 14:11 JaniruTEC

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"

Vladyslav-Soldatenko avatar Nov 02 '25 10:11 Vladyslav-Soldatenko