Sorted tags alphabetically #57
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @shrestha-bishal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've updated the application to sort tags alphabetically across various views, improving the user experience by presenting tags in a consistent and easily navigable order. This change affects how tags are displayed on the main tags page and within transaction forms where tags can be selected. Additionally, I've made a minor adjustment to the Docker development environment's port configuration.
Highlights
- Custom Alphanumeric Tag Sorting: I've implemented a custom alphanumeric sorting logic for tags, ensuring they are displayed alphabetically, handling both text and numerical parts correctly.
- Tags Index Page Sorting: This new sorting is applied to the tags listed on the TagsController index page, providing a consistent order for all tags.
- Refactored Tag Sorting Logic: I've refactored the tag sorting logic into a reusable
set_tagsprivate method withinTransactionsControllerto ensure consistent sorting for tag selection in transaction forms and views. - Updated Transaction Forms: The transaction forms (
_form.html.erbandshow.html.erb) have been updated to utilize the pre-sorted@tagsinstance variable for tag selection dropdowns, ensuring users always see tags in the desired order. - Docker Port Mapping Adjustment: I've adjusted the Docker development environment port mapping from
3000:3000to3001:3000in.devcontainer/docker-compose.yml.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request implements a natural sort order for tags, which is a great usability improvement. My review focuses on two main points: first, the new sorting logic is duplicated and has performance implications that can be addressed by moving it to the Tag model. Second, there's an unrelated port change in the Docker configuration that might be unintentional. Addressing these points will improve the maintainability and performance of the code.
There was a problem hiding this comment.
Pull Request Overview
This PR implements natural sorting for tags throughout the application, replacing the previous alphabetical sorting with a more user-friendly approach that properly handles tags containing numbers.
- Adds a new
sorted_naturallymethod to the Tag model that sorts tags by separating alphabetic and numeric components - Updates all tag display locations to use natural sorting instead of alphabetical sorting
- Removes unnecessary blank line in the tags controller
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/models/tag.rb | Adds sorted_naturally method with natural sorting logic |
| app/views/transactions/show.html.erb | Updates tag selection to use natural sorting |
| app/views/transactions/_form.html.erb | Updates tag selection to use natural sorting |
| app/controllers/tags_controller.rb | Updates tag index to use natural sorting and removes blank line |
|
Fixed a silly linter error so tests pass now, want to act on any of the Copilot comments @shrestha-bishal ? A couple of them are interesting ones! |
Sure ill have a look into it. Thanks |
- Handle multiple numeric segments in tag names (e.g. "abc123def456") - Ensure case-insensitive sorting of non-numeric parts - Prevent nil errors with safe string handling
…a-bishal/sure-finance into sort-tags-alphabetically
@jjmata fixed it and all checks passed. good to merge. Thanks |
|
The new commit also fixes the sorting issue in rule creation window on rule category and the value options. |
|
Want to give this one a look @Oaluiser / add a review? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces natural sorting for tags and applies deterministic ordering across tags, categories, rule filters, and executors. Updates controllers and views to use the new tag sorting. Adds SQL and Ruby implementations for natural sort in Tag. Ensures options and registries return consistently ordered arrays. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant VC as View/Controller
participant Tag as Tag Model
participant Rule as Rule Registry
rect rgba(220,235,255,0.3)
note over VC,Tag: Tags index / transaction selects
User->>VC: Request tags / open form
VC->>Tag: Current.family.tags.sorted_naturally
Tag-->>VC: Array of tags in natural order
VC-->>User: Rendered list/select with natural ordering
end
rect rgba(235,255,220,0.3)
note over VC,Rule: Rule config options
User->>VC: Open rule editor
VC->>Rule: condition_filters / action_executors
Rule-->>VC: Arrays sorted by label (case-insensitive)
VC-->>User: Deterministically ordered UI options
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/rule/action_executor/set_transaction_category.rb (1)
10-26: Guard against nil category to avoid NoMethodError on category.idIf value is nil/blank or the category ID doesn’t exist, category will be nil and category.id will raise. A no-op early return keeps rule execution safe.
def execute(transaction_scope, value: nil, ignore_attribute_locks: false) - category = family.categories.find_by_id(value) + return if value.blank? + category = family.categories.find_by(id: value) + return unless categoryMinor readability tweak (optional):
- scope = transaction_scope - - unless ignore_attribute_locks - scope = scope.enrichable(:category_id) - end + scope = ignore_attribute_locks ? transaction_scope : transaction_scope.enrichable(:category_id)
♻️ Duplicate comments (1)
app/models/tag.rb (1)
11-18: Fix SQL: empty-string cast to INTEGER will raise and breaks tags with no digitsFor names without digits, REGEXP_REPLACE(name, '\D+', '') returns an empty string; casting '' to INTEGER raises in PostgreSQL. Also, add a case-insensitive prefix key to stabilize ordering. Replace the cast with NULLIF/COALESCE to avoid runtime errors.
Apply this diff:
scope :sorted_naturally_db, -> { order( Arel.sql( - "REGEXP_REPLACE(name, '\\d+$', '') ASC, " \ - "CAST(REGEXP_REPLACE(name, '\\D+', '') AS INTEGER) ASC" + "LOWER(REGEXP_REPLACE(name, '\\d+$', '')) ASC, " \ + "COALESCE(NULLIF(REGEXP_REPLACE(name, '\\D+', ''), '')::INTEGER, 0) ASC, " \ + "LOWER(name) ASC" ) ) }Notes:
- This still won’t perfectly “natural sort” multi-number segments (e.g., foo12bar3 vs foo12bar10), but it’s deterministic, safe, and performant for most lists. If you need true multi-token natural sorting at the DB layer, we can discuss a regexp_split_to_array-based approach or a function-based index.
🧹 Nitpick comments (4)
app/models/rule/action_executor/set_transaction_tags.rb (1)
7-7: Consider natural sort for rules to match the rest of the UIElsewhere tags are shown with natural ordering; rules UI currently uses pure alphabetical. If you want a consistent experience, switch to the same scope.
- family.tags.order(:name).pluck(:name, :id) + family.tags.sorted_naturally_db.pluck(:name, :id)app/models/rule/action_executor/set_transaction_category.rb (1)
6-8: Deterministic, alphabetical options for categories — good moveOrdering by name before pluck aligns the UI with the rest of the PR’s deterministic ordering goal.
If you want case-insensitive stability across DB collations, consider ordering by LOWER(name):
- family.categories.order(:name).pluck(:name, :id) + family.categories.order(Arel.sql('LOWER(name) ASC'), :name).pluck(:name, :id)Optionally, a composite index on (family_id, name) will keep this snappy at scale.
app/models/rule/registry/transaction_resource.rb (2)
6-12: Sorting condition filters for deterministic UI — thumbs upThis removes order drift between environments. To be extra defensive if any label could be nil, coerce to string:
- ].sort_by { |filter| filter.label.downcase } + ].sort_by { |filter| filter.label.to_s.downcase }
22-28: Executor list ordering is now stable; consider DRYing the sort keyBoth places sort by label.downcase. Extracting a tiny helper keeps them in sync.
Example:
class Rule::Registry::TransactionResource < Rule::Registry + LABEL_KEY = ->(o) { o.label.to_s.downcase } def condition_filters - [ ... ].sort_by { |filter| filter.label.downcase } + [ ... ].sort_by(&LABEL_KEY) end def action_executors enabled_executors = [ ... ] if ai_enabled? enabled_executors << Rule::ActionExecutor::AutoCategorize.new(rule) enabled_executors << Rule::ActionExecutor::AutoDetectMerchants.new(rule) end - enabled_executors.sort_by { |executor| executor.label.downcase } + enabled_executors.sort_by(&LABEL_KEY) endOptional: add an assertion in tests that the returned labels are already sorted to prevent regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/controllers/tags_controller.rb(1 hunks)app/models/rule/action_executor/set_transaction_category.rb(1 hunks)app/models/rule/action_executor/set_transaction_tags.rb(1 hunks)app/models/rule/registry/transaction_resource.rb(2 hunks)app/models/tag.rb(2 hunks)app/views/transactions/_form.html.erb(1 hunks)app/views/transactions/show.html.erb(1 hunks)
| def index | ||
| @tags = Current.family.tags.alphabetically | ||
|
|
||
| @tags = Current.family.tags.sorted_naturally |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the scope to keep it relational (no eager loading)
Switch to the DB scope so this remains chainable and avoids loading into Ruby.
- @tags = Current.family.tags.sorted_naturally
+ @tags = Current.family.tags.sorted_naturally_db📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @tags = Current.family.tags.sorted_naturally | |
| @tags = Current.family.tags.sorted_naturally_db |
🤖 Prompt for AI Agents
In app/controllers/tags_controller.rb around line 5, the current call loads Tag
records into Ruby (non-relational) via a method that eagerly materializes
results; replace it with a chainable ActiveRecord scope so the result stays an
ActiveRecord::Relation. Use the DB scope version of the natural sort by either
merging the Tag.sorted_naturally scope into Current.family.tags
(Current.family.tags.merge(Tag.sorted_naturally)) or by applying
Tag.sorted_naturally.where(family: Current.family), and ensure you do not call
any loading methods like to_a, load, or similar.
| def self.sorted_naturally | ||
| all.to_a.sort_by do |tag| | ||
| tag.name.to_s.scan(/\d+|\D+/).map { |s| s =~ /\d+/ ? s.to_i : s.downcase } | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make natural sort chainable and consistent (avoid loading entire table and ambiguity between scope vs class method)
self.sorted_naturally returns an Array (loads all records) and is a class method, not a scope. Calling Current.family.tags.sorted_naturally relies on Rails’ method_missing scoping semantics and returns a Ruby array, which:
- Breaks chainability (can’t paginate, further filter, etc.).
- Loads all matching rows into memory.
- Diverges from the DB-level variant.
Prefer a scope for use in relations and keep the Ruby method only as an explicit fallback.
Two options:
Option A (recommended): consolidate on DB scope and remove/rename the Ruby method.
- def self.sorted_naturally
- all.to_a.sort_by do |tag|
- tag.name.to_s.scan(/\d+|\D+/).map { |s| s =~ /\d+/ ? s.to_i : s.downcase }
- end
- end
+ # Optional: keep a clearly named Ruby-only helper if ever needed explicitly
+ def self.sorted_naturally_ruby(collection = all)
+ collection.to_a.sort_by do |tag|
+ tag.name.to_s.scan(/\d+|\D+/).map { |s| s =~ /\d+/ ? s.to_i : s.downcase }
+ end
+ endOption B: keep both, but add a scope alias for consistent call sites:
+ scope :sorted_naturally, -> { sorted_naturally_db }If you’d like, I can follow through with a repo-wide rename to use sorted_naturally_db (or the alias) wherever relations are expected.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def self.sorted_naturally | |
| all.to_a.sort_by do |tag| | |
| tag.name.to_s.scan(/\d+|\D+/).map { |s| s =~ /\d+/ ? s.to_i : s.downcase } | |
| end | |
| end | |
| # Optional: keep a clearly named Ruby-only helper if ever needed explicitly | |
| def self.sorted_naturally_ruby(collection = all) | |
| collection.to_a.sort_by do |tag| | |
| tag.name.to_s.scan(/\d+|\D+/).map { |s| s =~ /\d+/ ? s.to_i : s.downcase } | |
| end | |
| end |
| <%= f.fields_for :entryable do |ef| %> | ||
| <%= ef.select :tag_ids, | ||
| Current.family.tags.alphabetically.pluck(:name, :id), | ||
| Current.family.tags.sorted_naturally.pluck(:name, :id), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer DB-level sort for options to avoid loading all records and to keep consistent ordering
Using the scope keeps pluck on the database and avoids Ruby-level sorting overhead.
- Current.family.tags.sorted_naturally.pluck(:name, :id),
+ Current.family.tags.sorted_naturally_db.pluck(:name, :id),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Current.family.tags.sorted_naturally.pluck(:name, :id), | |
| Current.family.tags.sorted_naturally_db.pluck(:name, :id), |
|
|
||
| <%= ef.select :tag_ids, | ||
| Current.family.tags.alphabetically.pluck(:name, :id), | ||
| Current.family.tags.sorted_naturally.pluck(:name, :id), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align with DB scope for consistent, efficient natural ordering
Same reasoning as in _form: keep sorting in SQL and let pluck run on the DB.
- Current.family.tags.sorted_naturally.pluck(:name, :id),
+ Current.family.tags.sorted_naturally_db.pluck(:name, :id),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Current.family.tags.sorted_naturally.pluck(:name, :id), | |
| Current.family.tags.sorted_naturally_db.pluck(:name, :id), |
🤖 Prompt for AI Agents
app/views/transactions/show.html.erb around line 80: the view currently calls
sorted_naturally in Ruby before pluck, causing sorting to happen in memory;
change this to use an ActiveRecord/SQL ordering so the DB performs the natural
sort and then pluck runs on the DB. Replace the in-memory sort with the model
scope or an order(...) that implements natural ordering (or ensure
sorted_naturally is an ActiveRecord scope) so you call
.order(...)/.sorted_naturally_scope.pluck(:name, :id) instead of sorting the
array after loading.
2 participants
Before:
After:
Summary by CodeRabbit
New Features
Bug Fixes