♻️ make templates classy and replace 🍝 code #3754
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. |
…tual into youngcw/classy-templates
…tual into youngcw/classy-templates
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 |
youngcw
changed the title
|
@UnderKoen Thanks for the review! I added most of the suggestions. A few that didn't make much difference and might break the flow other places I left out. |
@deathblade666 I think I fixed this. Can you try again? |
Yep, looks good |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/budget/categoryTemplate.ts (1)
366-366: Simplify boolean assignment by removing unnecessary ternary operatorOn line 366, the assignment
this.limitHold = t.limit.hold ? true : false;uses an unnecessary ternary operator. You can simplify this expression by directly assigning the boolean value.Apply this diff to simplify the code:
- this.limitHold = t.limit.hold ? true : false; + this.limitHold = Boolean(t.limit.hold);🧰 Tools
🪛 Biome
[error] 366-366: 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/categoryTemplate.ts(1 hunks)packages/loot-core/src/server/budget/goaltemplates.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/budget/goaltemplates.ts
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/budget/categoryTemplate.ts
[error] 366-366: 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 (3)
packages/loot-core/src/server/budget/categoryTemplate.ts (3)
60-60: Clarify the necessity of running limits in runAll
There's a TODO comment on line 60: "does this need to run limits? maybe pass in option to ignore previous balance?" Please verify whether limits should be applied within the runAll method and consider implementing the necessary logic.
Would you like assistance in determining if limits should be incorporated here or if an option to ignore the previous balance is needed?
103-103: Implement logic for 'by' templates in runTemplatesForPriority
The TODO comment indicates that the logic for 'by' templates within the runTemplatesForPriority method is not yet implemented. This functionality is essential for handling 'by' type templates correctly.
This issue has been previously noted, and the past review comment is still valid.
241-252: Use a Set to collect unique priorities more efficiently
Currently, you're collecting priorities into an array and then filtering to get unique items. Using a Set would improve performance and code clarity.
This suggestion has been made previously and is still applicable.
| ); | ||
| sum += await getSheetValue(sheetName, `sum-amount-${this.categoryID}`); | ||
| } | ||
| return -Math.round(sum / template.amount); |
There was a problem hiding this comment.
Correct division to use numMonths instead of amount in runAverage
In the runAverage method, you're dividing by template.amount, but it seems you should be dividing by template.numMonths to compute the average correctly.
Apply this diff to fix the issue:
- return -Math.round(sum / template.amount);
+ return -Math.round(sum / template.numMonths);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return -Math.round(sum / template.amount); | |
| return -Math.round(sum / template.numMonths); |
There was a problem hiding this comment.
@youngcw could you check this? I think it's possibly a copy/paste error from the old file
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const incomeCat = (await db.getCategories()).find( | ||
| c => c.is_income && c.name.toLowerCase() === cat, | ||
| ); | ||
| monthlyIncome = await getSheetValue( | ||
| sheetName, | ||
| `sum-amount-${incomeCat.id}`, | ||
| ); |
There was a problem hiding this comment.
Handle potential undefined income categories to prevent runtime errors
In the runPercentage method, if the specified income category is not found, incomeCat will be undefined, leading to a runtime error when accessing incomeCat.id.
Add a check to handle the case when incomeCat is undefined and throw a meaningful error.
Apply this diff to fix the issue:
const incomeCat = (await db.getCategories()).find(
c => c.is_income && c.name.toLowerCase() === cat,
);
+ if (!incomeCat) {
+ throw new Error(`Income category '${cat}' not found`);
+ }
monthlyIncome = await getSheetValue(
sheetName,
`sum-amount-${incomeCat.id}`,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const incomeCat = (await db.getCategories()).find( | |
| c => c.is_income && c.name.toLowerCase() === cat, | |
| ); | |
| monthlyIncome = await getSheetValue( | |
| sheetName, | |
| `sum-amount-${incomeCat.id}`, | |
| ); | |
| const incomeCat = (await db.getCategories()).find( | |
| c => c.is_income && c.name.toLowerCase() === cat, | |
| ); | |
| if (!incomeCat) { | |
| throw new Error(`Income category '${cat}' not found`); | |
| } | |
| monthlyIncome = await getSheetValue( | |
| sheetName, | |
| `sum-amount-${incomeCat.id}`, | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/loot-core/src/server/budget/categoryTemplate.ts (4)
241-252: Simplify priorities initialization using SetThe current implementation can be simplified using Set for uniqueness and better readability.
- const p = []; - this.templates.forEach(t => { - if (t.priority != null) { - p.push(t.priority); - } - }); - //sort and reduce to unique items - this.priorities = p - .sort(function (a, b) { - return a - b; - }) - .filter((item, idx, curr) => curr.indexOf(item) === idx); + this.priorities = [...new Set( + this.templates + .filter(t => t.priority != null) + .map(t => t.priority) + )].sort((a, b) => a - b);
273-316: SplitcheckByAndScheduleAndSpendinto smaller, focused methodsThe method handles multiple concerns: schedule validation, priority validation, and date validation. Consider splitting it into separate methods for better maintainability.
+ private static async validateScheduleNames(templates, scheduleNames) { + templates + .filter(t => t.type === 'schedule') + .forEach(t => { + if (!scheduleNames.includes(t.name.trim())) { + throw new Error(`Schedule ${t.name.trim()} does not exist`); + } + }); + } + private static validatePriorities(templates, lowestPriority) { + templates + .filter(t => t.type === 'schedule' || t.type === 'by') + .forEach(t => { + if (t.priority !== lowestPriority) { + throw new Error( + `Schedule and By templates must be the same priority level. Fix by setting all Schedule and By templates to priority level ${lowestPriority}` + ); + } + }); + } + private static validateTargetDates(templates, month) { + templates + .filter(t => t.type === 'by' || t.type === 'spend') + .forEach(t => { + const range = monthUtils.differenceInCalendarMonths( + `${t.month}`, + month + ); + if (range < 0 && !(t.repeat || t.annual)) { + throw new Error( + `Target month has passed, remove or update the target month` + ); + } + }); + }
368-368: Simplify boolean assignmentThe ternary operator is unnecessary for boolean assignment.
- this.limitHold = t.limit.hold ? true : false; + this.limitHold = !!t.limit.hold;🧰 Tools
🪛 Biome
[error] 368-368: 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)
539-539: Implement schedule template processingThe
runSchedulemethod is currently commented out. This could lead to incomplete template processing.Would you like assistance in implementing the schedule template processing logic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/loot-core/src/server/budget/categoryTemplate.ts(1 hunks)packages/loot-core/src/server/budget/goalsSchedule.ts(1 hunks)packages/loot-core/src/server/budget/goaltemplates.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/loot-core/src/server/budget/goalsSchedule.ts
- packages/loot-core/src/server/budget/goaltemplates.ts
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/budget/categoryTemplate.ts
[error] 368-368: 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 (4)
packages/loot-core/src/server/budget/categoryTemplate.ts (4)
203-204: 🛠️ Refactor suggestion
Initialize goal-related fields with proper types
The fields isLongGoal and goalAmount are initialized to null, which could cause type issues. Consider using proper initial values that match their intended types.
- private isLongGoal: boolean = null; //defaulting the goals to null so templates can be unset
- private goalAmount: number = null;
+ private isLongGoal: boolean = false;
+ private goalAmount: number = 0;Likely invalid or redundant comment.
482-488:
Add null check for income category
The runPercentage method should validate that the income category exists before accessing its properties.
const incomeCat = (await db.getCategories()).find(
c => c.is_income && c.name.toLowerCase() === cat,
);
+ if (!incomeCat) {
+ throw new Error(`Income category '${cat}' not found`);
+ }
monthlyIncome = await getSheetValue(
sheetName,
`sum-amount-${incomeCat.id}`,
);Likely invalid or redundant comment.
494-503:
Fix average calculation to use correct denominator
The runAverage method incorrectly uses template.amount instead of template.numMonths for calculating the average.
- return -Math.round(sum / template.amount);
+ return -Math.round(sum / template.numMonths);Likely invalid or redundant comment.
60-60: Address TODO comment regarding limits handling
The comment raises an important question about limits handling that needs to be addressed.
Would you like me to help implement the limits handling logic or create an issue to track this?
|
Looks good! |
|
Happy to approve once the conflict and the comment I've tagged you in is resolved, code all looks good! |
…tual into youngcw/classy-templates
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/loot-core/src/server/budget/categoryTemplate.ts (5)
12-28: Add JSDoc documentation for public APIWhile the usage instructions are helpful, the class would benefit from proper JSDoc documentation for its public methods, including parameter types, return types, and examples.
Example documentation:
/** * Manages budget templates for a specific category * @class CategoryTemplate */ export class CategoryTemplate { /** * Initialize a new CategoryTemplate instance * @param {Template[]} templates - Templates for this category * @param {string} categoryID - Category identifier * @param {string} month - Target month in YYYY-MM format * @returns {Promise<CategoryTemplate>} Initialized template instance * @throws {Error} When template validation fails */ static async init(templates: Template[], categoryID: string, month: string)
54-62: Address TODO comment and improve error handlingThe TODO comment about running limits needs to be addressed. Additionally, consider adding error handling for edge cases:
- When
availableis negative- When priority calculations overflow
Would you like me to help implement these improvements or create an issue to track this work?
66-150: Simplify template processing logicThe
runTemplatesForPrioritymethod is quite complex with multiple responsibilities. Consider:
- Extracting the template type processing into a strategy pattern
- Moving the available funds calculation into a separate method
- Adding early returns to reduce nesting
Example refactor:
private async processTemplate(template: Template, context: ProcessContext): Promise<number> { const processor = this.templateProcessors.get(template.type); if (!processor) { throw new Error(`Unknown template type: ${template.type}`); } return processor.process(template, context); }
368-368: Simplify boolean expressionThe boolean literal in the conditional expression can be simplified.
Apply this diff:
- this.limitHold = t.limit.hold ? true : false; + this.limitHold = !!t.limit.hold;🧰 Tools
🪛 Biome
[error] 368-368: 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)
505-537: Simplify date calculations inrunBymethodThe date calculation logic in
runByis complex and could be simplified by extracting the repeat logic into a separate method.Example refactor:
private calculateTargetMonth(template: Template): string { if (!template.repeat && !template.annual) { return template.month; } const repeat = template.annual ? (template.repeat || 1) * 12 : template.repeat; let targetMonth = template.month; while (monthUtils.differenceInCalendarMonths(targetMonth, this.month) < 0) { targetMonth = monthUtils.addMonths(targetMonth, repeat); } return targetMonth; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/loot-core/src/server/budget/categoryTemplate.ts(1 hunks)packages/loot-core/src/server/budget/goaltemplates.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/budget/goaltemplates.ts
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/budget/categoryTemplate.ts
[error] 368-368: 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 (2)
packages/loot-core/src/server/budget/categoryTemplate.ts (2)
1-2: Consider enabling TypeScript strict mode
The @ts-strict-ignore directive suggests potential type safety issues. Consider addressing these issues to enable strict mode, which would provide better type safety and catch potential runtime errors during compilation.
482-488:
Add null check for income category
The incomeCat could be undefined if no matching category is found, which would cause a runtime error when accessing incomeCat.id.
Apply this diff:
const incomeCat = (await db.getCategories()).find(
c => c.is_income && c.name.toLowerCase() === cat,
);
+ if (!incomeCat) {
+ throw new Error(`Income category '${cat}' not found`);
+ }
monthlyIncome = await getSheetValue(Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/budget/goaltemplates.ts (1)
224-240: Consider optimizing priority level processing.The nested loops for processing priority levels could become a performance bottleneck with many categories.
Consider:
- Processing priority levels in parallel where possible
- Implementing early termination conditions
- Caching intermediate results for categories with multiple priority levels
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/budget/goaltemplates.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/server/budget/goaltemplates.ts
[warning] 48-48:
Replace ⏎····month,⏎····true,⏎····categoryTemplates,⏎····categories,⏎·· with month,·true,·categoryTemplates,·categories
[warning] 218-218:
'priorities' is constant
🪛 GitHub Check: typecheck
packages/loot-core/src/server/budget/goaltemplates.ts
[failure] 218-218:
Cannot assign to 'priorities' because it is a constant.
🪛 Biome
packages/loot-core/src/server/budget/goaltemplates.ts
[error] 218-218: Can't assign priorities because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🔇 Additional comments (5)
packages/loot-core/src/server/budget/goaltemplates.ts (5)
7-8: LGTM! Type safety improvements and clean function signatures.
The changes improve type safety by:
- Adding explicit Promise return types
- Importing the new CategoryTemplate class
- Properly structuring async function signatures
Also applies to: 11-14, 18-22
171-172: Optimize sequential async operations.
Multiple sequential await calls inside a loop can impact performance.
241-261: LGTM! Well-structured template finalization.
The limit application and remainder distribution logic is clear and well-organized:
- Applies limits to all categories
- Distributes remaining budget based on weights
- Collects final values for batch updates
262-268: LGTM! Clean error handling and success reporting.
The function properly:
- Batches budget and goal updates
- Returns a clear success message with category count
Line range hint 72-107: Verify database query performance.
While the function works correctly, the database query might benefit from an index on the goal_def column to optimize template retrieval.
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 218-218:
Cannot assign to 'priorities' because it is a constant.
🪛 GitHub Check: lint
[warning] 218-218:
'priorities' is constant
🪛 Biome
[error] 218-218: Can't assign priorities because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
|
@joel-jeremy @matt-fidd This should be ready. Test your templates again if you have them since the merge in of master broke a bunch of things that I had to fix |
Matches limit additions in actualbudget/actual#3754. Original PR with descriptions is actualbudget/actual#3714 Tweaks a few other things to retain accuracy.
An attempt to rewrite the goal template processing to not be so messy. Im moving a lot of the functions into a class that will be used for each category and that way all the calculations can stay local and have minimal variables passed around. There will still be level above this so that things that require some checks of the full budget can still be done ex. balances at priority levels, remainder amounts, recapturing funds from limits, etc
This also should hopefully make #1686 easier. While also making future hooks into goals be easier to do. For example showing changes before applying, calculating full amounts for planning, etc.
This changes the following:
up totargets #3711Future:
Add tests to all the template processing. I need to remove the existing ones for now to get CI/CD happy and the old tests wont work anymore with the new structure.
The schedule template is a beast. I just used the existing function in its own file since it is so involved. Maybe that could be reviewed at some point to see if we can simplify that. If sub categories ever get added limiting some templates to one per category/sub-category would be good.
Finish the runAll method for use with running a single category, all priorities all at once. Could maybe be used for applying a single category, or for future hooks like gathering full budget amounts for planning.