Add option to unmark imported transactions #3810
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
csenel
changed the title
|
Warning Rate limit exceeded@csenel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces a new method, Additionally, the Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 3
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/hooks/useTransactionBatchActions.ts (2)
117-120: Enhance comment and add validation for imported_payee.While the implementation is correct, consider these improvements:
- Make the comment more specific about the impact: "Unmark as imported transaction by setting imported_payee to empty string, which removes the imported tooltip."
- Add validation to avoid unnecessary updates when the field is already empty.
if (name === 'imported_payee') { - // Unmark as imported transaction by setting imported payee to empty string. + // Unmark as imported transaction by setting imported_payee to empty string, + // which removes the imported tooltip. + if (trans.imported_payee) { valueToSet = ''; + } }
234-235: Consider architectural improvements for better UX.While the implementation is solid, consider these architectural improvements:
- Add a confirmation dialog for bulk unmarking, similar to other batch operations
- Add this action to the transaction context menu for single-transaction operations
- Consider adding a bulk "Mark as imported" operation for symmetry
Would you like help implementing any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3810.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/components/accounts/Account.tsx(2 hunks)packages/desktop-client/src/components/accounts/Header.tsx(3 hunks)packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx(5 hunks)packages/desktop-client/src/hooks/useTransactionBatchActions.ts(2 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx (4)
42-42: LGTM: Prop type definition is well-structured.
The new prop follows the established naming conventions and type patterns in the codebase.
299-306: LGTM: Menu item is well-integrated.
The menu item is properly structured with:
- Correct placement in the menu
- Appropriate conditional rendering
- Proper translation support
335-337: LGTM: Handler implementation is correct.
The new case in the switch statement follows the established pattern and correctly handles the unmark action.
Line range hint 1-361: Verify integration with imported transaction handling.
While the implementation looks good, let's verify the complete flow:
- Ensure this component receives the proper callback from its parent
- Confirm the tooltip is properly hidden when a transaction is unmarked
✅ Verification successful
Based on the search results, I can now provide a final response since I have enough information to verify the implementation:
Implementation is properly integrated and functional
The code shows proper integration of the unmark imported transactions feature:
- The
SelectedTransactionsButtoncomponent correctly checks for imported transactions usingimported_payeefield - The feature is properly connected through the component hierarchy:
Account.tsximplements the handleronUnmarkTransactionsAsImportedHeader.tsxpasses it down to child componentsSelectedTransactionsButton.tsxexposes it in the menu when applicable
The implementation aligns with the user needs as it:
- Only shows the option when there are imported transactions selected
- Handles batch operations correctly
- Properly integrates with the undo notification system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the component is properly connected to handle imported transactions
echo "Checking parent components for proper prop passing..."
rg -l "onUnmarkTransactionsAsImported" --type ts --type tsx
# Verify tooltip implementation
echo "Checking tooltip implementation..."
rg "imported_payee.*tooltip" --type ts --type tsx
Length of output: 363
Script:
#!/bin/bash
# Let's try with correct file type flags and broader search patterns
echo "Checking parent components and handlers..."
rg -t typescript -t tsx "onUnmarkTransactionsAsImported"
echo -e "\nChecking tooltip and imported payee related code..."
rg -t typescript -t tsx "imported_payee"
echo -e "\nChecking transaction list component..."
rg -t typescript -t tsx "SelectedTransactionsButton.*props"
Length of output: 571
Script:
#!/bin/bash
# Search without file type flags since they're not recognized
echo "Checking parent components and handlers..."
rg "onUnmarkTransactionsAsImported"
echo -e "\nChecking imported payee related code..."
rg "imported_payee"
echo -e "\nChecking TransactionList component..."
fd "TransactionList" --exec cat {} \;
Length of output: 43327
🧰 Tools
🪛 Biome
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/accounts/Header.tsx (3)
116-116: LGTM! Type definition is correctly added.
The onUnmarkTransactionsAsImported property is properly integrated into the AccountHeaderProps type definition, maintaining type safety by inheriting from the SelectedTransactionsButton component.
183-183: LGTM! Prop destructuring is properly implemented.
The onUnmarkTransactionsAsImported prop is correctly destructured from the component props, following the established pattern in the codebase.
372-372: LGTM! Prop is correctly passed to child component.
The onUnmarkTransactionsAsImported prop is properly passed to the SelectedTransactionsButton component, enabling the unmarking of imported transactions as intended.
Let's verify the integration with the SelectedTransactionsButton component:
✅ Verification successful
Prop is correctly integrated and handled in the SelectedTransactionsButton component
The verification confirms:
- The prop is properly typed in
SelectedTransactionsButtoncomponent's props interface - The prop is correctly destructured in the component parameters
- The prop is properly invoked with
selectedIdswhen needed - The implementation follows a complete chain from
Accountcomponent throughHeadertoSelectedTransactionsButton
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the SelectedTransactionsButton component accepts and handles the new prop
# Test: Check if SelectedTransactionsButton component properly declares the prop
ast-grep --pattern 'type $_ = {
$$$
onUnmarkTransactionsAsImported?: $_
$$$
}'
# Test: Check if the component implements the unmarking functionality
rg -A 5 'onUnmarkTransactionsAsImported'
Length of output: 5069
packages/desktop-client/src/components/accounts/Account.tsx (1)
1778-1780: LGTM!
The prop is correctly passed to the AccountHeader component, following the existing naming conventions and integration patterns.
packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx
Show resolved
Hide resolved
packages/desktop-client/src/hooks/useTransactionBatchActions.ts
Outdated
Show resolved
Hide resolved
csenel
changed the title
Fixes: #3358
As described in the feature request, a tooltip was added for transactions after v24.9.0. However as someone who imports & exports budget files between servers, most of the historical transactions are marked as imported. Furthermore when you duplicate to create a new transaction, the imported payee field is also kept the same.
With this change, I have added an option to the transaction table to unmark imported transactions (set
imported_payeefield to empty string), which will disable the imported payee tooltip.Please let me know how it looks. This is my first PR ever working on a typescript code base. Even though I have tested the functionality locally, it's likely that I might have missed some things.