fix(account): show activity label for accounts supporting trades#1993
fix(account): show activity label for accounts supporting trades#1993xBlaz3kx wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThree view templates switch their account-type conditionals from ChangesAccount Type Gating Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63cdcfb43b
ℹ️ 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".
| <div class="hidden md:flex min-w-0 items-center gap-1 col-span-2"> | ||
| <% if entry.account.investment? && !transaction.transfer? %> | ||
| <%# For investment accounts, show activity label instead of category %> | ||
| <% if entry.account.supports_trades? && !transaction.transfer? %> |
There was a problem hiding this comment.
Guard activity badge until Buy/Sell conversion supports crypto
Rendering investment_activity/quick_edit_badge for all supports_trades? accounts introduces a broken interaction for crypto exchange transactions: the badge’s JS routes Buy/Sell selections to convert_to_trade (app/javascript/controllers/activity_label_quick_edit_controller.js), but TransactionsController#convert_to_trade still rejects non-investment? accounts (app/controllers/transactions_controller.rb). As a result, crypto users now see the activity badge but hit an error path when choosing the two most common labels, which is a regression from the previous category-only UI.
Useful? React with 👍 / 👎.
|
Simple and correct. Two touch-points changed:
Crypto wallet and other crypto subtypes are unaffected since Generated by Claude Code |
Summary
Previously, the account chart view selector (holdings/cash/total value) and the transaction activity badge were gated behind account.investment?. This excluded crypto exchange accounts (Coinbase, Kraken, etc.) which also support trades via supports_trades?.
Changes
Behavior
Summary by CodeRabbit