fix(performance): eliminate N+1 query in TagsController#index#2787
fix(performance): eliminate N+1 query in TagsController#index#2787sentry[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70b2a9d8a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def index | ||
| @tags = Current.family.tags.alphabetically | ||
| @tags = Current.family.tags.includes(:transactions).alphabetically |
There was a problem hiding this comment.
Avoid loading every tagged transaction
For families where a tag is attached to many transactions, includes(:transactions) makes the tags settings page instantiate every matching Transaction record even though the partial only needs tag.transactions.any? to choose the delete flow. That replaces a set of indexed existence checks with loading the full tagged transaction history, so large accounts can see worse latency/memory or timeouts; preload/count taggings or expose a boolean instead.
Useful? React with 👍 / 👎.
ReviewVerified the fix targets the right spot: Scoped to exactly the one line that needs it, no behavior change otherwise. Looks correct. One minor, non-blocking note: Generated by Claude Code |
This PR resolves an N+1 query issue identified in
TagsController#index.The problem occurred because the
app/views/tags/_tag.html.erbpartial was callingtag.transactions.any?for each tag without thetransactionsassociation being eager-loaded. This resulted in a separate database query for every tag to check for associated transactions.The fix involves modifying the
indexaction inTagsControllerto eager-load thetransactionsassociation. By changingCurrent.family.tags.alphabeticallytoCurrent.family.tags.includes(:transactions).alphabetically, all necessary transaction data is fetched in a single query, thereby eliminating the N+1 query.Fixes SURE-APP-168