✨ Filter account with 'on budget' or 'off budget' #3891
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
|
@coderabbitai review |
|
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/loot-core/src/server/accounts/transaction-rules.tsOops! 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 several modifications across multiple components and files, primarily focusing on enhancing the handling of budget-related operations, specifically On the server side, 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: 0
🧹 Outside diff range and nitpick comments (8)
packages/desktop-client/src/components/filters/subfieldToOptions.ts (1)
30-36: LGTM! Consider enhancing type safety.The implementation for account budget filtering is clean and follows the existing pattern. For better type safety, consider defining an enum or const array for valid subfield values.
+const ACCOUNT_SUBFIELDS = ['on-budget', 'off-budget'] as const; +type AccountSubfield = typeof ACCOUNT_SUBFIELDS[number]; + case 'account': - if (['on-budget', 'off-budget'].includes(subfield)) { + if (ACCOUNT_SUBFIELDS.includes(subfield as AccountSubfield)) {packages/loot-core/src/types/models/rule.d.ts (1)
62-62: LGTM! Consider adding JSDoc documentation.The new optional
onBudgetproperty is well-placed and correctly typed. Consider adding JSDoc documentation to explain its purpose and usage.+ /** Filters accounts based on their budget status */ onBudget?: boolean;packages/desktop-client/src/components/filters/FilterExpression.tsx (1)
87-115: Simplify the conditional rendering logicThe current implementation has some redundancy and could be more maintainable.
Consider applying this refactor to simplify the code:
- {field === 'account' && options?.onBudget !== undefined ? ( + {field === 'account' && 'onBudget' in options ? ( <> <Text>is</Text>{' '} - {options?.onBudget !== undefined && options?.onBudget ? ( - <Text style={{ color: theme.pageTextPositive }}> - (on budget) - </Text> - ) : ( - <Text style={{ color: theme.pageTextPositive }}> - (off budget) - </Text> - )} + <Text style={{ color: theme.pageTextPositive }}> + ({options.onBudget ? 'on' : 'off'} budget) + </Text> </> ) : (Changes made:
- Replaced
options?.onBudget !== undefinedwith'onBudget' in optionsfor clearer property existence check- Simplified the nested conditional by combining the text content
- Reduced duplicate JSX structure
packages/desktop-client/src/components/util/GenericInput.jsx (1)
103-124: Consider enhancing the disabled inputs for better UX and accessibility.The implementation for handling budget status is clean and aligns well with the PR objectives. However, there are a few suggestions to improve the user experience:
Consider these enhancements:
case 'on-budget': - content = <Input disabled value="On budget" />; + content = ( + <Input + disabled + value="On budget" + aria-label="Account is on budget" + title="This account is on budget" + /> + ); break; case 'off-budget': - content = <Input disabled value="Off budget" />; + content = ( + <Input + disabled + value="Off budget" + aria-label="Account is off budget" + title="This account is off budget" + /> + ); break;The changes add:
aria-labelfor screen readerstitlefor tooltip on hoverpackages/desktop-client/src/components/filters/FiltersMenu.jsx (3)
120-150: Consider enhancing the account filter button UXThe implementation looks good with clean state management and proper styling. However, consider these UX improvements:
- Add visual indicators (icons) alongside text to make the states more distinguishable
- Consider using a dropdown instead of cycling through states for more explicit state selection
Example implementation:
<Button variant="bare" style={{ color: theme.pageTextPositive, flexGrow: 0 }} onPress={() => { - if (subfield === undefined || subfield === 'account') { - setSubfield('on-budget'); - } else if (subfield === 'on-budget') { - setSubfield('off-budget'); - } else { - setSubfield('account'); - } }} > + <View style={{ flexDirection: 'row', alignItems: 'center' }}> + {/* Add appropriate icons based on your icon library */} + <Icon name={subfield === 'on-budget' ? 'check-circle' : subfield === 'off-budget' ? 'circle-slash' : 'filter'} /> + <Text style={{ marginLeft: 5 }}> {subfield === undefined || subfield === 'account' ? 'All' : subfield === 'on-budget' ? 'On budget' : 'Off budget'} + </Text> + </View> </Button>
166-231: Simplify the conditional rendering logicThe operation buttons implementation is solid, but the visibility condition could be simplified for better maintainability.
Consider these improvements:
- {(field !== 'account' || - subfield === undefined || - subfield === 'account') && ( + {(!isAccountFilter || isDefaultAccountFilter) && (Add these constants at the top of the component:
const isAccountFilter = field === 'account'; const isDefaultAccountFilter = subfield === undefined || subfield === 'account';Also consider:
- Extracting the magic number 3 into a named constant for the operation buttons split
- Adding a comment explaining why the operations are split into two groups
Line range hint
120-231: Add tests for the new account filtering functionalityWhile the implementation looks good, it would benefit from test coverage for:
- Account filter state transitions
- Operation buttons visibility conditions
- Integration with the filter system
Would you like me to help create test cases for these scenarios?
packages/loot-core/src/server/accounts/transaction-rules.ts (1)
390-391: LGTM! Consider adding a comment for clarity.The implementation correctly handles the account filtering based on budget status. The logic using
!options.onBudgetappropriately maps between the UI's "on budget" concept and the database's "offbudget" field.Consider adding a brief comment explaining the negation logic:
} else if (options?.onBudget !== undefined && field === 'account') { + // Convert UI's "onBudget" to DB's "offbudget" field (they're opposites) return { 'account.offbudget': !options.onBudget };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3891.mdis excluded by!**/*.md
📒 Files selected for processing (7)
packages/desktop-client/src/components/filters/FilterExpression.tsx(1 hunks)packages/desktop-client/src/components/filters/FiltersMenu.jsx(2 hunks)packages/desktop-client/src/components/filters/subfieldFromFilter.ts(1 hunks)packages/desktop-client/src/components/filters/subfieldToOptions.ts(1 hunks)packages/desktop-client/src/components/util/GenericInput.jsx(1 hunks)packages/loot-core/src/server/accounts/transaction-rules.ts(1 hunks)packages/loot-core/src/types/models/rule.d.ts(1 hunks)
🔇 Additional comments (6)
packages/desktop-client/src/components/filters/subfieldFromFilter.ts (1)
26-30: LGTM! Consider adding JSDoc documentation.
The implementation is clean and follows the existing patterns. The null checks and type safety are properly handled.
Consider adding JSDoc documentation to describe the new account filtering behavior:
export function subfieldFromFilter({
field,
options,
value,
}: Pick<RuleConditionEntity, 'field' | 'options' | 'value'>) {
+ /**
+ * Processes filter conditions for different fields
+ * For account field:
+ * - Returns 'on-budget' when options.onBudget is true
+ * - Returns 'off-budget' when options.onBudget is false
+ * - Falls back to 'account' if onBudget is undefined
+ */Let's verify the integration with related components mentioned in the AI summary:
✅ Verification successful
Integration verified, implementation is correct and well-integrated
The verification shows proper integration across the codebase:
FilterExpression.tsxcorrectly renders the budget status with appropriate stylingsubfieldToOptions.tshandles the conversion logic for 'on-budget'/'off-budget' values- The account field handling is properly implemented in filter-related components
- The changes align with the existing budget-related functionality across the application
The suggested JSDoc documentation would still be a nice addition, but it's a minor enhancement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of account budget filtering across components
# Check FilterExpression.tsx for proper rendering of budget status
echo "Checking FilterExpression.tsx for budget status rendering:"
rg -A 5 "options\?.onBudget" "packages/desktop-client/src/components/filters/FilterExpression.tsx"
# Check subfieldToOptions.ts for corresponding conversion logic
echo "Checking subfieldToOptions.ts for account field handling:"
rg -A 5 "case 'account':" "packages/desktop-client/src/components/filters/subfieldToOptions.ts"
# Check GenericInput.jsx for conditional rendering
echo "Checking GenericInput.jsx for account field handling:"
rg -A 5 "field === 'account'" "packages/desktop-client/src/components/GenericInput.jsx"
Length of output: 1380
Script:
#!/bin/bash
# Let's verify the integration with a broader search for account-related components
# Check for any GenericInput component with .tsx extension
echo "Checking for GenericInput component:"
fd -e tsx -e js GenericInput
# Search for account field handling in all filter-related files
echo "Checking filter components for account field handling:"
fd -e tsx -e ts "Filter" packages/desktop-client/src/components/filters/ --exec rg -l "field === 'account'"
# Look for budget-related rendering in components
echo "Checking components for budget status rendering:"
rg -l "onBudget" packages/desktop-client/src/components/
Length of output: 2322
packages/desktop-client/src/components/filters/subfieldToOptions.ts (1)
30-36: Verify integration with the filtering system.
Let's verify that this implementation correctly integrates with the filter expression rendering and menu components.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the integration has been thoroughly verified:
Account budget filtering integration is properly implemented
The implementation correctly integrates with the filtering system across components:
FilterExpression.tsxproperly renders the budget status with appropriate stylingFiltersMenu.jsximplements the expected toggle functionality between 'on-budget' and 'off-budget' states- The conversion between subfields and options is consistently handled in both
subfieldFromFilter.tsandsubfieldToOptions.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of account budget filtering across components
# Check FilterExpression for proper rendering of budget status
echo "Checking FilterExpression for budget status rendering..."
rg -A 5 "options\?\.onBudget" "packages/desktop-client/src/components/filters/"
# Check FiltersMenu for budget toggle functionality
echo "Checking FiltersMenu for budget toggle..."
rg -A 5 "'on-budget'|'off-budget'" "packages/desktop-client/src/components/filters/"
# Verify consistent usage of account subfields
echo "Checking for consistent subfield usage..."
ast-grep --pattern 'case "account": { $$$ }'
Length of output: 4117
packages/loot-core/src/types/models/rule.d.ts (1)
62-62: Verify the property usage across the codebase.
Let's ensure the new onBudget property is consistently handled across the codebase.
✅ Verification successful
Based on the search results, I can now provide a final response. The onBudget property is properly integrated across the codebase:
The onBudget property is correctly implemented and used consistently
The property is used in several key areas:
- In rule type definitions (
packages/loot-core/src/types/models/rule.d.ts) - In transaction rule processing (
packages/loot-core/src/server/accounts/transaction-rules.ts) - In filter components for account conditions (
packages/desktop-client/src/components/filters/*) - In YNAB4 import functionality (
packages/loot-core/src/server/importers/ynab4.ts)
The implementation is consistent with the codebase's handling of account-related filtering and rule conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of onBudget property across the codebase
# Expected results: Find all references to ensure proper implementation
# Check for direct property access
echo "Checking direct property access..."
rg "onBudget" --type ts --type tsx
# Check for specific account field conditions that might need updating
echo "Checking account field conditions..."
ast-grep --pattern 'field: "account"'
# Look for potential test files that should be updated
echo "Checking test files..."
fd "test|spec" -e ts -e tsx | xargs rg "account.*condition"
Length of output: 381
Script:
#!/bin/bash
# Let's try a different approach to find references to onBudget
# Search for onBudget in all files (not limiting to specific extensions)
echo "Searching for onBudget references..."
rg "onBudget"
# Search for rule type definitions and interfaces
echo "Searching for rule type definitions..."
rg "interface.*Rule|type.*Rule"
# Search for rule-related files
echo "Looking for rule-related files..."
fd "rule" -t f
Length of output: 29284
packages/desktop-client/src/components/filters/FilterExpression.tsx (1)
87-115: Verify integration with FiltersMenu component
The new account budget filtering feature requires proper integration between components.
Let's verify the integration:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Integration between filter components is properly implemented
The account budget filtering feature shows consistent implementation across components:
FiltersMenu.jsxcorrectly handles the account filter UI with "All", "On budget", and "Off budget" optionssubfieldToOptions.tsproperly converts the subfield toonBudgetoptionsubfieldFromFilter.tscorrectly processes theonBudgetoptionFilterExpression.tsxproperly renders the filter state based on the passed options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of account budget filtering across components
# Check for account filter handling in FiltersMenu
echo "Checking FiltersMenu for account filter configuration:"
rg -A 10 "field === 'account'" packages/desktop-client/src/components/filters/
# Check for consistent option handling
echo "Checking for onBudget option handling:"
rg "onBudget" packages/desktop-client/src/components/filters/
Length of output: 6447
packages/desktop-client/src/components/util/GenericInput.jsx (1)
111-122: LGTM! Default case maintains existing functionality.
The default case preserves the original AccountAutocomplete behavior while maintaining consistency with the component's patterns.
packages/loot-core/src/server/accounts/transaction-rules.ts (1)
390-391: Verify test coverage for the new condition.
Since this is a WIP PR and introduces a new condition type, please ensure:
- Test cases cover both "on budget" and "off budget" scenarios
- Edge cases are handled (undefined options, null values)
Let's check for existing test coverage:
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Test coverage exists and implementation is correct
The new condition for account budget status is properly implemented and tested:
- The condition is handled in
conditionsToAQLfunction which converts rule conditions to AQL queries - The implementation correctly translates
onBudgetoption toaccount.offbudgetfield with inverted boolean value - The test file contains comprehensive test coverage for various condition types, including:
- Basic condition validation
- Rule application
- Complex queries with multiple conditions
- Edge cases with null values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for relevant test files and their content
fd -e test.ts -e spec.ts | xargs rg -l "conditionsToAQL|onBudget|offbudget"
# Search for existing test scenarios
rg -A 5 "describe.*account.*budget"
Length of output: 384
Script:
#!/bin/bash
# Let's check the content of the transaction-rules test file specifically
cat packages/loot-core/src/server/accounts/transaction-rules.test.ts
# Also check the implementation file for context
cat packages/loot-core/src/server/accounts/transaction-rules.ts
Length of output: 52423
lelemm
changed the title
|
I think the new button feels out of place. Another idea would be to make another pill after the has tag(s) e.g.
|
lelemm
changed the title
I will try that |
lelemm
force-pushed
the
account_inoutbudget
branch
from
6140638 to
1b0bebf
Compare
lelemm
force-pushed
the
account_inoutbudget
branch
from
1b0bebf to
45015ab
Compare
done |
lelemm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
packages/desktop-client/src/components/filters/updateFilterReducer.ts (2)
22-24: LGTM! Consider refactoring the growing list of conditions.The addition of budget-related operations is implemented correctly and follows the existing pattern. However, the growing list of conditions in the if statement might benefit from being refactored into a set or array for better maintainability.
Consider this refactoring:
+ const VALUE_RESET_OPERATIONS = new Set([ + 'contains', + 'matches', + 'is', + 'doesNotContain', + 'isNot', + 'hasTags', + 'onBudget', + 'offBudget' + ]); if ( (type === 'id' || type === 'string') && - (action.op === 'contains' || - action.op === 'matches' || - action.op === 'is' || - action.op === 'doesNotContain' || - action.op === 'isNot' || - action.op === 'hasTags' || - action.op === 'onBudget' || - action.op === 'offBudget') + VALUE_RESET_OPERATIONS.has(action.op) ) {
Line range hint
26-28: Update the comment to reflect all operations.The comment "Clear out the value if switching between contains or is/oneof" doesn't fully reflect all the operations that trigger value reset, including the newly added budget operations.
Consider updating the comment to:
- // Clear out the value if switching between contains or - // is/oneof for the id or string type + // Clear out the value for operations that don't require a value + // (e.g., contains, is, hasTags, budget status) for id or string typespackages/desktop-client/src/components/util/GenericInput.jsx (3)
35-35: Consider using a more descriptive prop name.The prop name
opis terse. Consider renaming it to something more descriptive likeoperationorfilterOperationto improve code readability.- op = undefined, + filterOperation = undefined,
104-125: Enhance maintainability and accessibility of the account filter UI.While the implementation is functional, consider the following improvements:
- Extract the text constants to avoid duplication and enable easier localization
- Add proper styling for the disabled state to match the application's design system
- Enhance accessibility by adding appropriate ARIA labels
+ const BUDGET_STATUS = { + ON_BUDGET: 'on budget', + OFF_BUDGET: 'off budget' + }; switch (op) { case 'onBudget': - content = <Input disabled value="on budget" />; + content = ( + <Input + disabled + value={BUDGET_STATUS.ON_BUDGET} + aria-label="Account is on budget" + style={{ opacity: 0.7 }} + /> + ); break; case 'offBudget': - content = <Input disabled value="off budget" />; + content = ( + <Input + disabled + value={BUDGET_STATUS.OFF_BUDGET} + aria-label="Account is off budget" + style={{ opacity: 0.7 }} + /> + ); break;
104-125: Implementation supports filtering functionality but consider UI feedback.The current implementation correctly handles the account budget status filtering logic. However, note that there's ongoing discussion about using pills instead of buttons in the UI. This implementation is flexible enough to support either approach, as it focuses on the underlying logic.
packages/loot-core/src/shared/rules.ts (2)
71-71: LGTM! Consider adding documentation.The changes correctly prevent budget operations from being used with inappropriate fields. The type constraints are properly maintained.
Consider adding a comment explaining why these operations are disallowed for payee and category fields, to help future maintainers understand the business logic.
payee: { type: 'id', + // Budget operations are only applicable to accounts disallowedOps: new Set(['onBudget', 'offBudget']) },Also applies to: 76-80
Line range hint
1-400: Consider implementing an enum for operation types.The addition of new operations suggests this system might grow. Consider introducing an enum for operation types to:
- Centralize operation definitions
- Provide type safety
- Make it easier to maintain operation-specific logic
Example implementation:
enum RuleOperation { OnBudget = 'onBudget', OffBudget = 'offBudget', // ... other operations }packages/loot-core/src/server/accounts/rules.ts (1)
953-954: Consider adjusting operation scoresBoth operations are assigned a score of 0, which puts them at the lowest priority in rule evaluation. Consider if these operations should have higher scores, similar to other boolean operations like
is(score: 10).- onBudget: 0, - offBudget: 0, + onBudget: 10, + offBudget: 10,packages/desktop-client/src/components/modals/EditRuleModal.jsx (1)
Line range hint
1-1068: Consider breaking down the component for better maintainabilityThe file is quite large and handles multiple concerns. Consider the following improvements:
- Extract the condition and action editors into separate files
- Create custom hooks for managing rule state and validation
- Consider using a reducer for complex state updates in conditions and actions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3891.mdis excluded by!**/*.md
📒 Files selected for processing (9)
packages/desktop-client/src/components/filters/FilterExpression.tsx(1 hunks)packages/desktop-client/src/components/filters/FiltersMenu.jsx(2 hunks)packages/desktop-client/src/components/filters/updateFilterReducer.ts(1 hunks)packages/desktop-client/src/components/modals/EditRuleModal.jsx(2 hunks)packages/desktop-client/src/components/util/GenericInput.jsx(2 hunks)packages/loot-core/src/server/accounts/rules.ts(2 hunks)packages/loot-core/src/server/accounts/transaction-rules.ts(1 hunks)packages/loot-core/src/shared/rules.ts(3 hunks)packages/loot-core/src/types/models/rule.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/desktop-client/src/components/filters/FilterExpression.tsx
- packages/desktop-client/src/components/filters/FiltersMenu.jsx
- packages/loot-core/src/server/accounts/transaction-rules.ts
- packages/loot-core/src/types/models/rule.d.ts
🔇 Additional comments (3)
packages/loot-core/src/shared/rules.ts (1)
25-26: LGTM! Verify operation usage.
The new operations are correctly added to the 'id' type operations list. The naming convention is consistent with the codebase.
Let's verify these operations are only used with account fields:
packages/loot-core/src/server/accounts/rules.ts (1)
204-205:
Update TypeScript types for new operations
The new operations are added to string literal arrays but might not be reflected in the TypeScript types. This could cause type checking issues.
Add the new operations to the RuleConditionOp type in the types definition file:
type RuleConditionOp =
| 'is'
| 'isNot'
// ... existing operations ...
| 'onBudget'
| 'offBudget';Also applies to: 953-954
✅ Verification successful
The previous command failed because the TypeScript file type wasn't recognized. Let's try a broader search to find the type definitions and relevant files.
TypeScript types are already up to date
The RuleConditionOp type in packages/loot-core/src/types/models/rule.d.ts already includes both 'onBudget' and 'offBudget' operations. These operations are also properly included in the BaseConditionEntity type for the 'account' field. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the operations are properly typed
rg -t typescript "type.*RuleConditionOp.*="
Length of output: 80
Script:
#!/bin/bash
# Search for type definitions using ripgrep without file type restriction
rg "type.*RuleConditionOp.*="
# Also search for enum definitions as an alternative way to define these operations
rg "enum.*RuleConditionOp"
# Search for files that might contain type definitions
fd "types?.ts"
Length of output: 153
Script:
#!/bin/bash
# Let's check the content of the type definition file to see if it includes the new operations
cat packages/loot-core/src/types/models/rule.d.ts
Length of output: 3558
packages/desktop-client/src/components/modals/EditRuleModal.jsx (1)
265-265: LGTM: Consistent prop passing for operation-specific input handling
The addition of the op prop to GenericInput components in both ConditionEditor and ActionEditor is well-implemented and maintains consistency across the codebase.
Also applies to: 465-465
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/accounts/rules.ts (1)
528-528: Remove unnecessary whitespaceRemove trailing whitespace on this line.
🧰 Tools
🪛 GitHub Check: lint
[warning] 528-528:
Delete········
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/accounts/rules.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/server/accounts/rules.ts
[warning] 528-528:
Delete ········
🔇 Additional comments (1)
packages/loot-core/src/server/accounts/rules.ts (1)
204-205: LGTM!
The new operations are correctly added to the CONDITION_TYPES.id.ops array and their scoring in OP_SCORES is consistent with similar operations.
Also applies to: 959-960
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/loot-core/src/types/models/transaction.d.ts (2)
29-29: LGTM with architectural considerations.The type definition is correct and follows the existing patterns for internal properties. However, consider these architectural implications:
- Storing the full account entity in each transaction increases memory usage.
- Having both
accountID and_accountentity could lead to data synchronization challenges.If this is a performance optimization to avoid repeated account lookups during rule processing, consider alternative approaches like:
- Caching account entities separately
- Using a normalized data structure
29-29: Add JSDoc comments to document the property's purpose.Since this is an internal property used specifically for rule processing, it would be helpful to add documentation explaining:
- Its purpose in relation to budget filtering
- When it gets populated
- That it's for internal use only
Consider adding a comment like this:
+ /** Internal property populated during rule processing to support budget status filtering. + * Contains the full account entity corresponding to the `account` ID. + * @internal + */ _account?: AccountEntity;packages/loot-core/src/server/accounts/transaction-rules.ts (2)
558-563: Add null safety checks for account field accessThe implementation looks good but could benefit from additional safety checks to handle cases where the account object might be undefined.
Consider this safer implementation:
case 'onBudget': - return { 'account.offbudget': false }; + return { + $and: [ + { account: { $ne: null } }, + { 'account.offbudget': false } + ] + }; case 'offBudget': - return { 'account.offbudget': true }; + return { + $and: [ + { account: { $ne: null } }, + { 'account.offbudget': true } + ] + };
858-864: Add error handling for account retrievalConsider adding error handling for the account retrieval operation to gracefully handle potential database errors.
Consider this implementation:
if (trans.account) { - const account = await getAccount(trans.account); - if (account) { - r._account = account; - } + try { + const account = await getAccount(trans.account); + if (account) { + r._account = account; + } + } catch (error) { + console.error('Failed to retrieve account:', error); + // Optionally: throw new Error('Failed to prepare transaction: ' + error.message); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/loot-core/src/server/accounts/rules.ts(3 hunks)packages/loot-core/src/server/accounts/transaction-rules.ts(3 hunks)packages/loot-core/src/server/db/index.ts(1 hunks)packages/loot-core/src/types/models/transaction.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/accounts/rules.ts
🔇 Additional comments (2)
packages/loot-core/src/server/db/index.ts (2)
476-478: Implementation aligns well with PR objectives.
The function follows the established patterns in the codebase and provides the necessary functionality for filtering accounts based on budget status.
476-478: Verify error handling in calling code.
Let's verify that all callers of this function properly handle the case when no account is found.
✅ Verification successful
Error handling is properly implemented in the codebase
The verification shows that getAccount is used in two locations:
- In
transaction-rules.ts, proper error handling is implemented with a null check:
const account = await getAccount(trans.account);
if (account) {
r._account = account;
}- In
TransactionEdit.jsx, the code also handles the null case appropriately:
const account = getAccount(transaction);
const isOffBudget = account && !!account.offbudget;Both usages properly handle the case when no account is found through null checks, making the implementation safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getAccount function and verify error handling
# Find direct function calls
echo "Finding direct function calls to getAccount:"
rg -A 5 "getAccount\(" --type ts --type tsx --type js --type jsx
# Find potential error handling
echo -e "\nChecking for error handling patterns:"
rg -A 5 "getAccount\(" --type ts --type tsx --type js --type jsx | rg -A 5 "(if.*null|try.*catch|\?\.|\|\|)"
Length of output: 423
Script:
#!/bin/bash
# Let's try a different approach using ast-grep to find function calls
# and ripgrep without file type restrictions
# Find direct function calls to getAccount
echo "Finding direct function calls to getAccount:"
rg "getAccount\(" -A 5
# Use ast-grep to find function calls with potential error handling
echo -e "\nChecking for function calls using AST:"
ast-grep --pattern 'getAccount($$$)'
# Look for await expressions since it's an async function
echo -e "\nChecking for await expressions:"
ast-grep --pattern 'await getAccount($$$)'
Length of output: 2361
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/accounts/transaction-rules.ts (1)
860-866: Add error handling for account retrieval.While the account preparation logic is well-structured, consider adding error handling for the
getAccountcall to gracefully handle potential database errors.if (trans.account) { - const account = await getAccount(trans.account); - if (account) { - r._account = account; - } + try { + const account = await getAccount(trans.account); + if (account) { + r._account = account; + } + } catch (error) { + console.error('Failed to fetch account:', error); + // Optionally: r._account = null; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/accounts/transaction-rules.ts(4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/accounts/transaction-rules.ts (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3619
File: packages/loot-core/src/server/accounts/transaction-rules.ts:795-801
Timestamp: 2024-11-10T16:45:31.225Z
Learning: In the `finalizeTransactionForRules` function within `packages/loot-core/src/server/accounts/transaction-rules.ts`, potential race conditions when inserting payees are handled in the method that calls this function, so additional safeguards within `finalizeTransactionForRules` are unnecessary.
🔇 Additional comments (2)
packages/loot-core/src/server/accounts/transaction-rules.ts (2)
559-564: LGTM: Budget filter conditions are correctly implemented.
The new filter conditions for onBudget and offBudget are properly implemented using the account.offbudget field with appropriate boolean values.
846-846: Type definition properly added as previously suggested.
The _account property has been correctly added to the TransactionForRules type interface as suggested in a previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/rules/ConditionExpression.tsx (2)
52-56: Improve type safety for operation handlingThe type assertion
(op as string)suggests that we should better type theopprop. Consider defining a union type for valid operations.+type Operation = 'onbudget' | 'offbudget' | string; type ConditionExpressionProps = { field: unknown; - op: unknown; + op: Operation; value: unknown; options: unknown; prefix?: string; style?: CSSProperties; inline?: boolean; };Then simplify the condition:
- {!['onbudget', 'offbudget'].includes( - (op as string)?.toLocaleLowerCase(), - ) && ( + {!['onbudget', 'offbudget'].includes(op.toLowerCase()) && (
52-56: LGTM! The conditional rendering logic is well-implementedThe implementation correctly handles the special cases for budget-related operations, aligning with the PR objectives to support filtering accounts based on budget status.
Consider extracting the budget operation constants to improve maintainability:
+const BUDGET_OPERATIONS = ['onbudget', 'offbudget'] as const; export function ConditionExpression({ // ... }: ConditionExpressionProps) { return ( // ... - {!['onbudget', 'offbudget'].includes( + {!BUDGET_OPERATIONS.includes( (op as string)?.toLocaleLowerCase(), ) && (
…into account_inoutbudget
|
Check out the changed code please @matt-fidd |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/accounts/sync.ts (2)
526-528: Consider caching the accounts map.The accounts map is created by fetching all accounts from the database. Since this operation could be expensive for large datasets, consider caching the map or passing it as a parameter to avoid multiple database calls.
526-528: Consider adding type guards for account entities.While the code uses TypeScript, it would be beneficial to add runtime type checks for the account entities to ensure data integrity, especially since this is dealing with financial data.
Here's a suggested type guard implementation:
function isValidAccountEntity(account: any): account is AccountEntity { return ( account && typeof account.id === 'string' && typeof account.type === 'string' && typeof account.balances === 'object' ); } function createAccountsMap(accounts: any[]): Map<string, AccountEntity> { const validAccounts = accounts.filter(isValidAccountEntity); if (validAccounts.length !== accounts.length) { console.warn('Some accounts were filtered out due to invalid data'); } return new Map(validAccounts.map(account => [account.id, account])); }Also applies to: 679-681
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/loot-core/src/server/accounts/sync.ts(2 hunks)packages/loot-core/src/shared/rules.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/shared/rules.ts
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/accounts/sync.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/server/accounts/sync.ts:28-46
Timestamp: 2024-11-10T16:45:31.225Z
Learning: Matt prefers to defer adding proper types for transactions interfaces until another PR.
🔇 Additional comments (1)
packages/loot-core/src/server/accounts/sync.ts (1)
536-536: Verify error handling in runRules function.
The runRules function is called with the new accountsMap parameter, but there's no visible error handling if the rules execution fails. Consider adding try-catch blocks to handle potential errors gracefully.
Let's verify the error handling in the runRules function:
Also applies to: 684-684
Co-authored-by: Matt Fiddaman <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/server/accounts/transaction-rules.ts (3)
278-291: Consider memoizing the accounts map.The accounts map is created on every call to
runRuleswhen not provided. Consider caching this map to avoid repeated database queries, especially if this function is called frequently.+let cachedAccountsMap: Map<string, AccountEntity> | null = null; + export async function runRules( trans, accounts: Map<string, AccountEntity> = null, ) { let accountsMap = null; if (accounts === null) { + if (cachedAccountsMap === null) { accountsMap = new Map( (await db.getAccounts()).map(account => [account.id, account]), ); + cachedAccountsMap = accountsMap; + } else { + accountsMap = cachedAccountsMap; + } } else { accountsMap = accounts; }
626-633: Consider using Promise.all for better performance.The code fetches accounts and then maps them into a Map structure sequentially. This could be optimized by running these operations in parallel.
- const accounts: AccountEntity[] = await db.getAccounts(); - const transactionsForRules = await Promise.all( - transactions.map(transactions => - prepareTransactionForRules( - transactions, - new Map(accounts.map(account => [account.id, account])), - ), - ), - ); + const [accounts, transactionsForRules] = await Promise.all([ + db.getAccounts(), + Promise.all( + transactions.map(async transactions => { + const accountsMap = new Map( + (await db.getAccounts()).map(account => [account.id, account]) + ); + return prepareTransactionForRules(transactions, accountsMap); + }), + ), + ]);
864-864: Consider adding JSDoc comments for better documentation.The
TransactionForRulestype andprepareTransactionForRulesfunction would benefit from JSDoc comments explaining their purpose and parameters.+/** + * Extended transaction type that includes additional properties for rule processing + */ export type TransactionForRules = TransactionEntity & { payee_name?: string; _account?: AccountEntity; }; +/** + * Prepares a transaction for rule processing by enriching it with additional data + * @param trans The transaction to prepare + * @param accounts Optional map of accounts for faster lookups + * @returns Promise<TransactionForRules> The prepared transaction + */ export async function prepareTransactionForRules( trans: TransactionEntity, accounts: Map<string, AccountEntity> | null = null, ): Promise<TransactionForRules> {Also applies to: 869-869
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/accounts/transaction-rules.ts(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/loot-core/src/server/accounts/transaction-rules.ts
[error] 880-880: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
packages/loot-core/src/server/accounts/transaction-rules.ts (1)
571-576: LGTM: New budget filter conditions are well implemented.
The implementation of onBudget and offBudget cases in conditionsToAQL is clean and straightforward.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/accounts/transaction-rules.ts (2)
278-291: Consider memoizing the accounts map.The implementation looks correct, but there's an opportunity for optimization. If this function is called frequently, creating a new Map on each invocation could impact performance.
Consider this optimization:
+ let cachedAccountsMap: Map<string, AccountEntity> | null = null; + export async function runRules( trans, accounts: Map<string, AccountEntity> | null = null, ) { - let accountsMap = null; if (accounts === null) { + if (cachedAccountsMap === null) { accountsMap = new Map( (await db.getAccounts()).map(account => [account.id, account]) ); + cachedAccountsMap = accountsMap; + } else { + accountsMap = cachedAccountsMap; + } } else { accountsMap = accounts; }
626-633: Consider batching account retrieval.The implementation is correct but could be optimized. Currently, we're retrieving all accounts even if we only need a subset.
Consider filtering accounts based on the transaction's account IDs:
- const accounts: AccountEntity[] = await db.getAccounts(); + const accountIds = new Set(transactions.map(t => t.account)); + const accounts: AccountEntity[] = await db.all( + 'SELECT * FROM accounts WHERE id IN (?)', + [Array.from(accountIds)] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/accounts/transaction-rules.ts(6 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/accounts/transaction-rules.ts (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3619
File: packages/loot-core/src/server/accounts/transaction-rules.ts:795-801
Timestamp: 2024-11-10T16:45:31.225Z
Learning: In the `finalizeTransactionForRules` function within `packages/loot-core/src/server/accounts/transaction-rules.ts`, potential race conditions when inserting payees are handled in the method that calls this function, so additional safeguards within `finalizeTransactionForRules` are unnecessary.
🪛 Biome (1.9.4)
packages/loot-core/src/server/accounts/transaction-rules.ts
[error] 880-880: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
packages/loot-core/src/server/accounts/transaction-rules.ts (4)
16-16: LGTM! Import changes look good.
The addition of AccountEntity type import and getAccount function import aligns well with the new account-related functionality.
Also applies to: 20-20
571-576: LGTM! New budget filter conditions look good.
The implementation of onBudget and offBudget cases in conditionsToAQL is clean and straightforward.
864-864: LGTM! Type definition update looks good.
The addition of the optional _account property to TransactionForRules type is well-documented.
879-886: 🛠️ Refactor suggestion
Use optional chaining for safer property access.
The code could benefit from using optional chaining when accessing properties.
Apply this change:
- if (trans.account) {
- if (accounts !== null && accounts.has(trans.account)) {
- r._account = accounts.get(trans.account);
+ if (trans?.account) {
+ if (accounts?.has(trans.account)) {
+ r._account = accounts?.get(trans.account);
} else {
r._account = await getAccount(trans.account);
}
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 880-880: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Just a small quality of life for filtering on/off budget accounts:
Actual.-.Google.Chrome.2024-11-26.09-57-18.mp4