[Goals] fix limits #3829
[Goals] fix limits #3829
Conversation
✅ 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 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
|
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 |
WalkthroughThe pull request introduces significant modifications to the Possibly related PRs
Suggested labels
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 (4)
packages/loot-core/src/server/budget/goaltemplates.ts (2)
2-2: Remove commented-out import statement.Instead of commenting out the unused import, it should be removed entirely for better code cleanliness.
-//import { t } from 'i18next';
194-195: Consider future internationalization needs.While removing problematic translations resolves the immediate crashes, this change might impact non-English users. Consider:
- Adding a TODO comment to track the need for proper internationalization
- Creating a centralized message constant file for easier future translation integration
Would you like me to help create a proposal for a more maintainable internationalization solution?
Also applies to: 201-202, 249-252
packages/loot-core/src/server/budget/categoryTemplate.ts (2)
51-53: Consider adding documentation forgetLimitExcessmethodAdding a JSDoc comment to the
getLimitExcess()method would improve maintainability by explaining its purpose and usage.
365-365: Simplify boolean assignmentThe assignment of
this.limitHoldcan be simplified by directly using the boolean value without a ternary operator.Apply this diff to simplify the code:
-this.limitHold = t.limit.hold ? true : false; +this.limitHold = !!t.limit.hold;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3829.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/loot-core/src/server/budget/categoryTemplate.ts(6 hunks)packages/loot-core/src/server/budget/goaltemplates.ts(4 hunks)
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/budget/categoryTemplate.ts
[error] 364-364: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (9)
packages/loot-core/src/server/budget/goaltemplates.ts (2)
169-169: LGTM! Correctly implements early limit handling.
This change aligns with the PR objectives by adding limit excess to the available budget during the initial data gathering phase, ensuring excess funds are available for redistribution to other categories.
Line range hint 1-252: Verify complete removal of i18next usage.
Let's ensure all i18next dependencies have been consistently removed throughout the codebase to prevent potential issues.
packages/loot-core/src/server/budget/categoryTemplate.ts (7)
19-22: Documentation updates look good
The inclusion of limitExcess in the documentation enhances clarity about the data gathered.
74-74: Ensure limitMet is properly set before the early return
The added check if (this.limitMet) return 0; prevents further processing when the limit is met. Please verify that this.limitMet is correctly updated in all relevant code paths prior to this check to avoid unintended behavior.
143-149: Verify limit enforcement logic for edge cases
The limit enforcement adjusts toBudget when the limit is exceeded. Ensure that scenarios where fromLastMonth exceeds limitAmount are correctly handled, and toBudget does not become negative unintentionally.
200-201: Initialization of limitMet and limitExcess
The new private variables limitMet and limitExcess are appropriately initialized to default values.
341-357: Validate limit period parameters to prevent errors
In the 'daily' and 'weekly' limit calculations, ensure that t.limit.start and related parameters are properly initialized and validated. This prevents potential issues like infinite loops or incorrect limit amounts due to invalid or missing data.
358-362: Limit period handling for 'monthly' is correct
The calculation for 'monthly' limits correctly sets this.limitAmount based on the provided amount.
367-377: Verify handling of negative budget adjustments
When the limit is already met and this.limitHold is false, negative values are assigned to this.toBudgetAmount and this.fullAmount. Please ensure that the system correctly handles negative budget amounts and that this logic aligns with the intended financial calculations.
|
Fixed all the translation errors here: #3830 It's to do with the loot-core server files not having access to the translations |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/budget/actions.ts (1)
1-2: Consider removing@ts-strict-ignoredirective.The presence of this directive suggests potential type-safety issues that should be addressed. Since you're adding new TypeScript code, it would be beneficial to fix the underlying type issues instead of ignoring them.
packages/loot-core/src/server/budget/categoryTemplate.ts (1)
382-382: Simplify boolean assignmentThe ternary operator for boolean assignment can be simplified.
- this.limitHold = t.limit.hold ? true : false; + this.limitHold = !!t.limit.hold;🧰 Tools
🪛 Biome
[error] 382-382: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/loot-core/src/server/budget/actions.ts(2 hunks)packages/loot-core/src/server/budget/categoryTemplate.ts(8 hunks)
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/budget/categoryTemplate.ts
[error] 382-382: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (8)
packages/loot-core/src/server/budget/actions.ts (2)
17-23: LGTM! The new getSheetBoolean function is well-implemented.
The function follows good practices:
- Handles type safety with proper TypeScript types
- Consistent with existing
getSheetValuepattern - Safely defaults to
falsefor non-boolean values
17-23: Verify the integration points of the new boolean getter.
Since this function is part of the limits refactoring effort, let's verify all its usage points to ensure proper integration.
✅ Verification successful
Let me gather more context about the usage in categoryTemplate.ts to understand the integration better.
Let me check the test files to verify if this functionality is properly tested.
Let me check one final thing about the sheet module that getSheetBoolean depends on.
Let me try one last search to locate the sheet module.
Based on all the gathered information, I can now provide the final response:
The new getSheetBoolean function is properly integrated and safe to use
The function is:
- Used specifically in
categoryTemplate.tsfor handling carryover status - Part of a well-structured sheet access layer
- Follows the existing pattern of sheet cell access methods
- Has a safe fallback to
falsefor non-boolean values
The single usage point in categoryTemplate.ts shows proper integration within the budget limits refactoring effort, where it's used alongside other sheet access methods like getSheetValue to determine carryover status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getSheetBoolean to verify integration points
# Search for direct function calls
echo "Direct function calls:"
rg "getSheetBoolean\(" --type ts
# Search for potential interface implementations
echo "\nPotential interface implementations:"
ast-grep --pattern 'interface $_ {
$$$
getSheetBoolean
$$$
}'
Length of output: 435
Script:
#!/bin/bash
# Get context of the usage in categoryTemplate.ts
echo "Usage context in categoryTemplate.ts:"
rg -B 5 -A 5 "getSheetBoolean\(" packages/loot-core/src/server/budget/categoryTemplate.ts
# Check if there are any tests for this functionality
echo "\nRelated test files:"
fd ".*test.*" --type f | rg -i "budget|category"
Length of output: 836
Script:
#!/bin/bash
# Check test coverage for getSheetBoolean functionality
echo "Checking base.test.ts for getSheetBoolean tests:"
rg -B 3 -A 3 "getSheetBoolean|carryover-" packages/loot-core/src/server/budget/base.test.ts
echo "\nChecking template-notes.test.ts:"
rg -B 3 -A 3 "getSheetBoolean|carryover-" packages/loot-core/src/server/budget/template-notes.test.ts
# Also check the implementation of getCell which getSheetBoolean depends on
echo "\nChecking getCell implementation:"
rg -B 3 -A 3 "getCell.*=|export.*getCell" packages/loot-core/src/server/budget/
Length of output: 1825
Script:
#!/bin/bash
# Check the sheet module implementation
echo "Checking sheet module exports and getCell implementation:"
rg -B 2 -A 2 "export.*getCell|getCell.*=" packages/loot-core/src/server/budget/sheet.ts
# Also check for any existing boolean cell handling
echo "\nChecking for boolean cell handling:"
rg -B 2 -A 2 "boolean.*cell|cell.*boolean" packages/loot-core/src/server/budget/
Length of output: 468
Script:
#!/bin/bash
# Find the sheet module location
echo "Looking for sheet module:"
fd "sheet.ts" --type f
# Search for getCell implementation across all files
echo "\nSearching for getCell implementation:"
ast-grep --pattern 'export $_ getCell($_, $_) {
$$$
}'
Length of output: 791
packages/loot-core/src/server/budget/categoryTemplate.ts (6)
41-50: LGTM: Improved handling of negative balances with carryover flag
The new carryover check ensures proper handling of negative balances from the previous month, aligning with the PR's goal of better funds management.
64-66: LGTM: Added getter for limit excess
Clean implementation of the getter method for accessing limit excess, which is essential for the new limit-first approach.
87-87: LGTM: Early return prevents unnecessary processing
Good optimization to skip processing when the limit is already met.
359-379: LGTM: Comprehensive limit period handling
Well-structured implementation of different limit periods with proper date calculations.
384-394: LGTM: Proper handling of limit excess
The implementation correctly:
- Checks if the limit is already met from previous month
- Handles excess funds based on the hold flag
- Adjusts budget amounts accordingly
This aligns perfectly with the PR's goal of applying limits at the beginning of the process.
156-167: Verify edge case handling in limit check
The limit check logic looks correct, but please verify the behavior when:
- Multiple templates are processed in the same priority level
- The limit is hit mid-priority
✅ Verification successful
Let me search for test files and implementation details related to priority processing and limit handling. The previous command failed because the --type=test flag wasn't supported. I've modified the approach to search for test files with common extensions and look for both limit and priority-related test cases.
Let me try a different approach by first finding test files using fd and then searching for relevant test cases and implementation. The previous commands failed because of incorrect file pattern syntax.
Based on the search results, I can now provide a final response about the limit check behavior in the code:
Limit check implementation is robust for multiple templates and mid-priority scenarios
The code correctly handles both scenarios:
-
Multiple templates: The code explicitly prevents multiple limit templates per category with the check
if (this.limitCheck) { throw new Error('Only oneup toallowed per category'); } -
Mid-priority limit handling: The implementation properly manages limits across priority levels by:
- Checking limit status before processing each priority (
if (this.limitMet) return 0;) - Tracking running totals across priorities (
toBudget + this.toBudgetAmount + this.fromLastMonth) - Adjusting available amounts when limit is hit (
available = available + orig - toBudget)
- Checking limit status before processing each priority (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering limit handling in priority processing
rg -l "runTemplatesForPriority.*limit" --type=test
Length of output: 81
Script:
#!/bin/bash
# Search for test files containing limit-related test cases
rg -l "limit.*test|test.*limit" ".*test.*\.(ts|js|tsx|jsx)$"
# Search for test files containing priority-related test cases
rg -l "priority.*test|test.*priority" ".*test.*\.(ts|js|tsx|jsx)$"
# Search for actual implementation of priority processing with limits
ast-grep --pattern 'function $_($$priority$$) {
$$$
limit
$$$
}'
Length of output: 338
Script:
#!/bin/bash
# Find test files first
fd "test.*\.(ts|js|tsx|jsx)$"
# Search for limit-related test cases
rg -C 5 "limit" --type ts --type js
# Look for priority processing implementation
rg -C 5 "runTemplatesForPriority" --type ts --type js
Length of output: 83445
since limits were being applied at the end of the processing chain, the funds pulled from categories because of the limit never had a chance to be used in other underfunded categories.
Ive reworked the limits to apply at the beginning, to return any excess from the previous month; and every priority pass, to not pull in unneeded funds. Goals for the categories with limits should also set correctly now. This also now handles rollover funds properly.
To test: