Preload transfer counterparty associations on transactions index#2819
Preload transfer counterparty associations on transactions index#2819bittensorrider wants to merge 25 commits into
Conversation
Only run the hourly Cloudflare preview cleanup on we-promise/sure, where the required secrets exist.
Transfer#categorizable? walks inflow_transaction.entry.account during list render, which N+1'd transactions, entries, and accounts per transfer row. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe transactions index now eager-loads nested transfer counterparty transaction, entry, and account associations. An integration test verifies transfer rendering without repeated per-transfer queries. ChangesTransfer eager loading
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/controllers/transactions_controller_test.rb`:
- Around line 735-746: Update the transaction index test around the six
create_transfer calls to retain the created transfer records, then assert that
their DOM IDs or an equivalent transfer-row marker are present in the GET
transactions_url response before validating captured SQL queries. Keep the
existing query assertions unchanged.
- Around line 750-755: Broaden the SQL regression assertions in the transaction
list-render test so they detect any per-transfer lazy loads for transactions,
entries, and accounts, regardless of equality versus IN predicates, key
formatting, or adapter-generated whitespace. Prefer normalizing captured queries
before counting association loads, while preserving the expectation that each
association is eager-loaded without extra per-row queries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cd94a79-e4ae-4c5c-97d7-9555e260aac4
📒 Files selected for processing (2)
app/controllers/transactions_controller.rbtest/controllers/transactions_controller_test.rb
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa8f2bd728
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/controllers/transactions_controller_test.rb (1)
781-792: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid classifying single-record
IN (?)queries as lazy loads.All six transfers use the same
to_account, so the nested account preload can legitimately generateaccounts.id IN (?). Sincesingle_record_lookupscurrently matches a single-valueIN (...), the test may fail even when eager loading is correct. Match onlycolumn = ?lookups, or use multiple counterparty accounts and assert the preload includes the expected IDs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/transactions_controller_test.rb` around lines 781 - 792, The single_record_lookups matcher currently treats single-value IN queries as lazy loads, causing valid nested preloads to be misclassified. Update single_record_lookups to match only column = ? lookups, or adjust the test data to use multiple counterparty accounts and assert the preload contains their expected IDs; preserve batch preload detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/controllers/transactions_controller_test.rb`:
- Around line 781-792: The single_record_lookups matcher currently treats
single-value IN queries as lazy loads, causing valid nested preloads to be
misclassified. Update single_record_lookups to match only column = ? lookups, or
adjust the test data to use multiple counterparty accounts and assert the
preload contains their expected IDs; preserve batch preload detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8dc74d4-f9d6-4393-be75-0dbab8336312
📒 Files selected for processing (2)
app/controllers/transactions_controller.rbtest/controllers/transactions_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/transactions_controller.rb
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
The nested includes correctly targets what Transfer#categorizable?/#payment? actually walk (inflow_transaction.entry.account) for both the transfer_as_inflow and transfer_as_outflow sides, and skipping outflow_transaction per the inline comment looks right given the kind-check happens against the current Transaction, not the transfer object.
The regression test is a good addition — asserting on normalized SQL patterns for the three specific by-id lookups (transactions.id =, entries.entryable_id =, accounts.id =) rather than just a total query count makes it robust to unrelated query-count churn elsewhere on the page, and the comment distinguishing lazy = ? loads from legitimate nested-preload IN (...) is a nice touch to avoid false positives.
No concerns — this is exactly the class of "avoid N+1 in a global/high-traffic view" fix the project prioritizes.
Generated by Claude Code
|
@jjmata Thanks for the review. Glad this matches the intended preload path. |
2 participants
Summary
TransactionsController#indexwhere transfer rows repeatedly loadedtransactions→entries→accountsby idTransfer#categorizable?/#payment?walkinflow_transaction.entry.accountduring category menu render without nested includesFix
Fixes #2818
Test plan
/transactionswith several transfer rows; confirm list/category UI still renderstest/controllers/transactions_controller_test.rbWHERE "transactions"."id" = ?/entryable_id/accounts.idduring list renderSummary by CodeRabbit