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 There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/accounts/Account.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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
| onUnmarkTransactionsAsImported = async (ids: string[]) => { | ||
| this.setState({ workingHard: true }); | ||
|
|
||
| const { data } = await runQuery( | ||
| q('transactions') | ||
| .filter({ id: { $oneof: ids } }) | ||
| .select('*') | ||
| .options({ splits: 'grouped' }), | ||
| ); | ||
|
|
||
| const transactions: TransactionEntity[] = data; | ||
|
|
||
| if (!transactions || transactions.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const importedTransactionIds = transactions | ||
| .filter(t => t.imported_payee) | ||
| .map(t => t.id); | ||
|
|
||
| this.onBatchEdit('imported_payee', importedTransactionIds); | ||
|
|
||
| this.refetchTransactions(); | ||
| }; | ||
|
|
There was a problem hiding this comment.
@joel-jeremy will know better than me on this, is this okay here or should it be moved to packages/desktop-client/src/hooks/useTransactionBatchActions.ts?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx (1)
150-161: Consider using optional chaining for better code style.While the logic is correct, we can improve the code style by using optional chaining as recommended by the linter.
const canUnmarkTransactionsAsImported = useMemo(() => { if (selectedIds.length === 0 || types.preview) { return false; } const transactions = selectedIds.map(id => getTransaction(id)); - const areSomeImportedTransactions = transactions.some( - tx => tx && tx.imported_payee, - ); + const areSomeImportedTransactions = transactions.some( + tx => tx?.imported_payee, + ); return areSomeImportedTransactions; }, [selectedIds, types, getTransaction]);🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/hooks/useTransactionBatchActions.ts
- packages/desktop-client/src/components/accounts/Header.tsx
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx (1)
Learnt from: csenel
PR: actualbudget/actual#3810
File: packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx:150-161
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx`, prefer to keep the implementation of checks consistent with similar patterns elsewhere in the codebase, even if alternative implementations are more concise.
🪛 Biome (1.9.4)
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 (4)
packages/desktop-client/src/components/transactions/SelectedTransactionsButton.tsx (3)
42-42: LGTM!
The new prop type follows the established pattern for batch operations and has a clear, descriptive name.
59-59: LGTM!
The prop is correctly destructured and follows the alphabetical ordering convention.
299-306: LGTM!
The menu item and its handler are implemented consistently with other similar features in the component.
Also applies to: 335-337
packages/desktop-client/src/components/accounts/Account.tsx (1)
1140-1163: 🛠️ Refactor suggestion
Consider optimizing the implementation and adding error handling.
While the implementation works, there are several areas for improvement:
- Add error handling for database operations
- Reset the workingHard state after operations complete
- Optimize the query by filtering imported transactions at the database level
onUnmarkTransactionsAsImported = async (ids: string[]) => {
this.setState({ workingHard: true });
+ try {
const { data } = await runQuery(
q('transactions')
.filter({
id: { $oneof: ids },
+ imported_payee: { $ne: null }
})
.select('*')
.options({ splits: 'grouped' }),
);
const transactions: TransactionEntity[] = data;
if (!transactions || transactions.length === 0) {
+ this.setState({ workingHard: false });
return;
}
- const importedTransactionIds = transactions
- .filter(t => t.imported_payee)
- .map(t => t.id);
+ const importedTransactionIds = transactions.map(t => t.id);
this.onBatchEdit('imported_payee', importedTransactionIds);
this.refetchTransactions();
+ } catch (error) {
+ console.error('Failed to unmark transactions as imported:', error);
+ } finally {
+ this.setState({ workingHard: false });
+ }
};Likely invalid or redundant comment.
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.