Extract payees related server handlers from main.ts to server/payees/app.ts #4443
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. |
joel-jeremy
changed the title
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
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 |
|
Warning Rate limit exceeded@joel-jeremy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
WalkthroughThe changes refactor and enhance the payee functionality across several files. In the database module, the 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: 1
🧹 Nitpick comments (4)
packages/loot-core/src/server/payees/app.ts (4)
9-19: Consider explicitly typing function return values.
Explicitly defining the return types of functions used inPayeesHandlerscan enhance code clarity and reduce potential confusion.
44-46: Clarify naming forsyncGetOrphanedPayees.
The term “sync” could be misinterpreted as synchronous. Consider a more descriptive function name or an explanatory comment.
48-59: Anticipate performance constraints ingetPayeeRuleCounts.
Manually iterating over rules can be expensive at scale. Consider delegating the count logic to the database or caching logic if performance becomes a concern.
109-115: Add a docstring for clarity.
A brief docstring explaininggetPayeeRulesaids future maintainers in understanding the function’s purpose and returned data shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4443.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/loot-core/src/server/db/index.ts(1 hunks)packages/loot-core/src/server/main.ts(3 hunks)packages/loot-core/src/server/payees/app.ts(1 hunks)packages/loot-core/src/types/handlers.d.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (13)
packages/loot-core/src/types/handlers.d.ts (2)
7-7: Good addition for modular payee handling.
This import cleanly separates payee-related handler logic.
32-33: Interface extension aligns with new PayeesHandlers.
IncludingPayeesHandlersinHandlerscodifies the newly modular payee functionality.packages/loot-core/src/server/db/index.ts (1)
519-519: Great improvement in type safety.
DeclaringidasDbPayee['id']helps ensure consistent usage across the codebase.packages/loot-core/src/server/main.ts (4)
45-45: Modular import promotes better payee separation.
ImportingpayeesAppis consistent with removing payee-handling logic from this file.
65-65: Verify concurrency usage with batchMessages.
Ensure the code properly handles and sequences batched DB updates to avoid data inconsistencies.
71-71: Importing undo functions is consistent.
These additions support advanced undo/redo operations; looks properly aligned with the existing pattern.
439-439: Consolidation of payee routes.
CombiningpayeesAppfinalizes the modular approach, grouping payee-related endpoints logically.packages/loot-core/src/server/payees/app.ts (6)
1-8: Imports look consistent.
All the imported modules are used appropriately. No concerns here.
21-30: Well-structured method registrations.
The usage ofcreateAppcoupled with method definitions is straightforward and organized. Each method is clearly registered with its key, which aligns with best practices.
32-34: Handle potential DB insertion errors.
Consider adding error handling arounddb.insertPayeeto gracefully report or manage insertion failures.
36-42: Standard retrieval methods look solid.
ThegetCommonPayeesandgetPayeesfunctions are concise and appear correct for data retrieval.
61-74: Guard against merging payee into itself.
Verify how the function behaves ifmergeIdscontainstargetIdor if a payee does not exist.
100-107: Confirm orphaned payee assumptions.
Ensuredb.getOrphanedPayeesconsistently returns valid IDs and handle any edge cases involving an empty or erroneous outcome.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/loot-core/src/server/models.ts (2)
109-116: Consider enhancing type safety in fromDb method.While the implementation is functionally correct, consider replacing the type assertion with a type guard to ensure type safety at runtime.
- return convertFromSelect( + const converted = convertFromSelect( schema, schemaConfig, 'payees', payee, - ) as PayeeEntity; + ); + if (!isPayeeEntity(converted)) { + throw new Error('Invalid PayeeEntity conversion'); + } + return converted;Consider adding a type guard function:
function isPayeeEntity(obj: unknown): obj is PayeeEntity { return obj !== null && typeof obj === 'object' && 'name' in obj; }
117-123: Consider enhancing type safety in toDb method.Similar to fromDb, consider adding type guards and error handling for better runtime safety.
- return ( + const converted = ( update ? convertForUpdate(schema, schemaConfig, 'payees', payee) : convertForInsert(schema, schemaConfig, 'payees', payee) - ) as DbPayee; + ); + if (!isDbPayee(converted)) { + throw new Error('Invalid DbPayee conversion'); + } + return converted;Consider adding a type guard function:
function isDbPayee(obj: unknown): obj is DbPayee { return obj !== null && typeof obj === 'object' && 'id' in obj; }Also, consider adding JSDoc comments to document the conversion process:
/** * Converts a PayeeEntity to a DbPayee format * @param payee - The payee entity to convert * @param options - Options for the conversion * @param options.update - If true, converts for update operation, otherwise for insert * @returns The converted DbPayee object * @throws {Error} If conversion results in invalid DbPayee */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/loot-core/src/server/db/index.ts(4 hunks)packages/loot-core/src/server/models.ts(2 hunks)packages/loot-core/src/server/payees/app.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/loot-core/src/server/db/index.ts
- packages/loot-core/src/server/payees/app.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
- GitHub Check: release-notes
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (1)
packages/loot-core/src/server/models.ts (1)
8-15: LGTM!The new imports are well-organized and provide the necessary utilities for the payee model enhancements.
|
Looks like per-payee category learning settings aren't working here, they appear to be always disabled and can't be enabled. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/loot-core/src/server/payees/app.ts (1)
80-98: 🛠️ Refactor suggestion**Reinforce atomicity for batch changes. **
As noted in past reviews, partially failed batch operations can leave the data in an inconsistent state. Consider introducing a transaction mechanism or a rollback strategy if any individual operation fails.
🧹 Nitpick comments (6)
packages/loot-core/src/server/payees/app.ts (4)
1-10: Favor explicit imports for clarity and maintainability.While the imports here are consolidated from multiple modules, consider selectively importing only what is necessary (e.g.,
insertPayee,getPayees) if the underlying modules export multiple functions. This reduces the chance of namespace clashes and clarifies usage.
34-36: Check for input validations.
createPayeedoes not currently validatename. Consider adding checks to ensurenameis not empty or invalid. This can help prevent introducing accidental blank or invalid payees into the database.
38-50: Potential for large data returns on get operations.
getCommonPayees,getPayees, andgetOrphanedPayeesreturn arrays that may become large for existing databases. Depending on usage patterns, consider adding pagination or filters to avoid performance bottlenecks for large datasets.
52-63: Be mindful of performance in counting rules.
getPayeeRuleCountsiterates over all rules in memory. If the set of rules grows large, this approach might be slow or memory-intensive. Consider storing this count in the database or indexing it if performance becomes an issue.packages/desktop-client/src/components/payees/ManagePayees.tsx (1)
106-108: Verify orphaned payees implementationThe filter logic here correctly handles the updated type of
orphanedPayeesas it only uses the ID field. However, since theorphanedPayeestype changed to only include theidfield, the mapping expression could be simplified.Consider simplifying this expression:
- filtered = filtered.filter(p => - orphanedPayees.map(o => o.id).includes(p.id), - ); + filtered = filtered.filter(p => + orphanedPayees.some(o => o.id === p.id), + );Using
some()instead ofmap().includes()is more efficient as it stops iteration as soon as a match is found.packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx (1)
133-136: Simplify orphan ID lookupsSince orphans now only contain ID fields, the current implementation using
map().includes()is functional but can be optimized.Consider simplifying these lookups:
- const targetIdIsOrphan = orphans.map(o => o.id).includes(targetId); - const mergeIdsOrphans = mergeIds.filter(m => - orphans.map(o => o.id).includes(m), - ); + const targetIdIsOrphan = orphans.some(o => o.id === targetId); + const mergeIdsOrphans = mergeIds.filter(m => + orphans.some(o => o.id === m), + );Using
some()is more efficient as it stops iteration as soon as a match is found.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/desktop-client/src/components/payees/ManagePayees.tsx(1 hunks)packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx(1 hunks)packages/loot-core/src/server/payees/app.ts(1 hunks)packages/loot-core/src/shared/util.ts(2 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (11)
packages/loot-core/src/server/payees/app.ts (5)
11-21: Great use of a dedicated handlers type.defining
PayeesHandlersprovides clarity around the shape of payees-related operations and makes it easier to keep track of available endpoints.
23-33: Modular payees app setup looks solid.Exporting
appas the primary interface ensures a clear delineation of payee-specific behavior. This is aligned with the PR's goal of extracting payee logic out ofmain.ts.
65-78: Undo mechanism integrates well, but watch for partial failures.
withUndois a strong approach to revert merges if needed. However, if partial failures occur duringdb.mergePayees, ensure proper rollback or error handling is in place to avoid inconsistent states.
100-107: Efficient approach to checking orphaned payees.
checkOrphanedPayeesis straightforward and usesSetfor quick lookups. This is efficient for fairly large lists of IDs.
109-115: Serialization of rules is clear and consistent.Returning
rule.serialize()ensures a uniform structure and hides internal details from consumers. This helps maintain a stable contract for payee rules.packages/loot-core/src/types/server-handlers.d.ts (1)
7-7: Removal of payee references in server-handlers aligns with the new modular payees app.This change cleans up the interface by deferring payee operations to the dedicated
PayeesHandlers. This promotes better separation of concerns.packages/desktop-client/src/components/payees/ManagePayees.tsx (1)
73-73: Type restriction improves type safetyThe type change from
PayeeEntity[]toArray<Pick<PayeeEntity, 'id'>>restricts theorphanedPayeesproperty to only include theidfield of payee entities. This is a good optimization that aligns with how orphaned payees are actually used in the component (only the IDs are necessary as seen on line 107).packages/loot-core/src/shared/util.ts (2)
48-48: Type refinement for deleted items improves data efficiencyChanging
deleted: Partial<T>[]todeleted: Pick<T, 'id'>[]in theDifftype definition ensures that deleted items only contain the ID field, which is all that's needed for deletion operations. This is a good optimization that reduces the memory footprint when handling deletions.
132-134: Implementation aligns with type definitionThe implementation of
diffItemshas been updated to match the refined type definition, ensuring that deleted items only include the ID field. This change is consistent with the type change on line 48 and provides a more efficient data structure for deleted items.packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx (2)
30-30: State type update for consistencyThe state type for
orphanshas been updated to match the prop type change inManagePayees.tsx. This ensures type consistency between components and aligns with the data optimization pattern used throughout the payee management components.
127-129:❓ Verification inconclusive
Verify payees update behavior
The removal of
await dispatch(getPayees());after updating payees withpayees-batch-changechanges the component's behavior. The component now relies on theapplyChangesfunction to update the orphans state locally without refetching all payees from the server.Make sure this change is intentional and doesn't lead to stale data. This adjustment might be related to performance optimization, but ensure that payee data remains in sync between components.
🏁 Script executed:
#!/bin/bash # Check for other instances where getPayees() is called after batch changes rg -A 3 "payees-batch-change" --glob "*.ts*"Length of output: 1729
Action Required: Confirm Local Sync is Sufficient After Batch Update
In the file
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx, the batch update now executes:await send('payees-batch-change', changes); setOrphans(applyChanges(changes, orphans));The previous call to
dispatch(getPayees())has been removed, so the component now relies onapplyChangesto update the local orphan state without refetching all payees from the server. Our grep search for "payees-batch-change" confirms that no subsequentgetPayees()call exists in the client code, which suggests that this change is intentional and likely aimed at optimizing performance.Please verify that:
- The local state update using
applyChangessufficiently synchronizes the payee data across components.- There is no risk of stale data due to the absence of a full refetch, particularly if other parts of the application depend on more comprehensive payee data.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx (2)
742-753:⚠️ Potential issueFix failing test due to change in ordering
The pipeline shows a test failure in this section. The test expects payees to be ordered as
['Alice-payee-item', 'Bob-payee-item', ...]but is receiving['Bob-payee-item', ...]instead. This suggests the ordering logic might be affected by the type change from integers to booleans.The payee ordering might have changed because of the update to boolean types. You'll need to either:
- Update the test expectation to match the new ordering:
- ).toStrictEqual([ - 'Alice-payee-item', - 'Bob-payee-item', - 'This guy on the side of the road-payee-item', - ]); + ).toStrictEqual([ + 'Bob-payee-item', + 'Alice-payee-item', + 'This guy on the side of the road-payee-item', + ]);
- Or ensure the sorting logic maintains the same order despite the type change
🧰 Tools
🪛 GitHub Actions: Test
[error] 743-743: AssertionError: expected [ 'Bob-payee-item', …(2) ] to strictly equal [ 'Alice-payee-item', …(2) ]
[error] 743-743: AssertionError: expected [ 'Bob-payee-item', …(2) ] to strictly equal [ 'Alice-payee-item', …(2) ]
748-752:⚠️ Potential issueFix favorites test expectations
The payee favorite test expectations also need to be updated based on the changes you made to the favorite values above. Since Alice's favorite status was changed from
truetofalse, the test should be updated accordingly.- // @ts-expect-error fix me - expect(renderedPayees).payeesToHaveFavoriteStars([ - 'Alice-payee-item', - 'Bob-payee-item', - ]); + // @ts-expect-error fix me + expect(renderedPayees).payeesToHaveFavoriteStars([ + 'Bob-payee-item', + ]);
🧹 Nitpick comments (3)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx (1)
42-42: Update favorite field from integer to boolean representationThis change converts the
favoritefield from an integer representation to a boolean, aligning with similar changes across the codebase. Based on the retrieved learning, there was previously a requirement that this field use integer values, but the codebase now appears to be transitioning to booleans for better type safety.The ternary operator can be simplified for better readability.
- favorite: options?.favorite ? true : false, + favorite: !!options?.favorite,🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: 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)
packages/loot-core/src/server/undo.ts (1)
94-105: Add error handling for metaFunc invocationIntroducing the optional
metaFuncparameter is a helpful addition that enables the generation of custom metadata. For improved robustness, consider the following:
- Guard against errors thrown by
metaFuncso that an exception doesn't leave the system in a partially undone state.- Include documentation or in-code comments describing how to utilize
metaFunceffectively.- Consider validating or type-narrowing
metaFunc's return value to ensure consistency and prevent unexpected behaviors in downstream logic.packages/loot-core/src/server/payees/app.ts (1)
42-45: Consider validating payee creation parameters.
createPayeeinserts a new payee without validation (e.g., empty names, duplicates). You might add a sanity check to ensure thenameis valid and unique if required by business logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx(1 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx(1 hunks)packages/desktop-client/src/components/payees/ManagePayees.tsx(3 hunks)packages/desktop-client/src/components/payees/PayeeTableRow.tsx(1 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx(1 hunks)packages/loot-core/src/server/aql/schema/index.ts(1 hunks)packages/loot-core/src/server/db/index.ts(6 hunks)packages/loot-core/src/server/models.ts(4 hunks)packages/loot-core/src/server/payees/app.ts(1 hunks)packages/loot-core/src/server/undo.ts(1 hunks)packages/loot-core/src/types/models/payee.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-11-10T16:45:25.627Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-11-10T16:45:25.627Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-11-10T16:45:25.627Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-11-10T16:45:25.627Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
🪛 Biome (1.9.4)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx
[error] 42-42: 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)
🪛 GitHub Actions: Test
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx
[error] 743-743: AssertionError: expected [ 'Bob-payee-item', …(2) ] to strictly equal [ 'Alice-payee-item', …(2) ]
[error] 743-743: AssertionError: expected [ 'Bob-payee-item', …(2) ] to strictly equal [ 'Alice-payee-item', …(2) ]
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (27)
packages/loot-core/src/server/aql/schema/index.ts (1)
66-66:❓ Verification inconclusive
Added new boolean field to payees schema
Adding the
learn_categoriesfield of type boolean to the payees schema is a clean addition that will enable per-payee category learning settings, which was mentioned as not working in the PR comments.Let's validate how this field will be used in the application by searching for references to it:
🏁 Script executed:
#!/bin/bash # Description: Find references to the new learn_categories field throughout the codebase # First, look for direct references to the field in code echo "Searching for direct references to learn_categories..." rg -p "learn_categories" --type ts --type tsxLength of output: 192
Validation of
learn_categoriesField Addition and Its Usage
- The new boolean field
learn_categoriesis correctly added in
packages/loot-core/src/server/aql/schema/index.ts(line 66) as:learn_categories: f('boolean'),- The initial search for references using
rg -p "learn_categories" --type ts --type tsxresulted in an error (unrecognized file type: tsx) rather than valid findings.- To verify the usages of this field throughout the codebase, please re-run the search with explicit file globs. For example, try the following command:
rg 'learn_categories' -g '*.ts' -g '*.tsx'- Once you confirm that the field is or isn’t referenced elsewhere as intended, update or verify its integration accordingly.
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx (1)
320-320:❓ Verification inconclusive
Update favorite field from integer to boolean representation
Changed
favorite: 0tofavorite: falsein the payee object, which is consistent with the schema change to use boolean types for fields likefavoriteandlearn_categoriesinstead of integers (0/1).Let's check for any other locations that might still be using the old integer representation:
🏁 Script executed:
#!/bin/bash # Description: Check for places still using integer values for favorite field echo "Searching for potential integer assignments to favorite field..." rg -p "favorite: [01]" --type ts --type tsxLength of output: 212
Action: Verify "favorite" Field Update in All TS/TSX Files
The update inpackages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx—changingfavorite: 0tofavorite: false—correctly reflects the schema change to use booleans. However, our initial verification command errored on TSX files (message: "unrecognized file type: tsx"). Please re-run the verification using a modified script that explicitly targets TSX files. For example:#!/bin/bash echo "Searching for potential integer assignments to favorite field in TS files..." rg "favorite:\s*[01]" -g "*.ts" echo "Searching for potential integer assignments to favorite field in TSX files..." rg "favorite:\s*[01]" -g "*.tsx"After confirming that no other integer assignments exist, you can be confident that the update is complete.
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx (3)
59-59: Update favorite field from integer to booleanChanged
favorite: 1tofavorite: truefor consistency with the schema changes that use boolean types instead of integers.
68-68: Update favorite field from integer to booleanChanged
favorite: 0tofavorite: falsefor consistency with the schema changes that use boolean types instead of integers.
64-64:❓ Verification inconclusive
Update favorite field from integer to boolean WITH VALUE CHANGE
Changed
favorite: 1tofavorite: false- this changes both the type (from integer to boolean) AND the value (from favorited to not favorited). Verify if this change in value is intentional as it could affect test outcomes.This change might impact the test expectations. Let's check if this value change affects any test cases:
🏁 Script executed:
#!/bin/bash # Description: Check test cases that might be affected by the value change # Look for test cases involving Alice payee echo "Checking test cases involving Alice payee..." rg -p "Alice.*favorite" -A 5 -B 5 "TransactionsTable.test.tsx"Length of output: 228
Action Required: Confirm the Favorite Field Update Impact
The change fromfavorite: 1tofavorite: falsealters both the data type and the value. Our initial search did not locate any test cases for"Alice payee"in the expected test file path, which suggests one of the following:
- The test file path might not be accurate (i.e., it might have been moved or renamed).
- The search criteria could be too narrow, requiring manual verification of the test cases that rely on this field.
Next Steps:
- Verify that the file at
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsxexists and is the correct target for testing this change.- Confirm that test assertions expecting a numerical value (
1) have been updated to align with the new booleanfalsevalue.Please manually inspect the affected test cases to ensure that this intentional change is properly reflected in the test expectations.
packages/loot-core/src/types/models/payee.d.ts (1)
7-8: Validate boolean usage for future database consistencySwitching from
1 | 0tobooleanfor thefavoriteandlearn_categoriesfields is more idiomatic and clearer in TypeScript. However, verify that the database or any code expecting numeric fields is updated accordingly to avoid a mismatch when reading or writing these values.packages/loot-core/src/server/models.ts (5)
55-66: Confirm 'id' enforcement for account updatesBy using
Partial<DbAccount>and only requiring['name', 'offbudget', 'closed']during updates, there's a risk of no 'id' check for identifying which account to update. Ensure that elsewhere in the code or database layer, theidfield is properly enforced for updates to avoid unintended writes or conflicts.
69-83: Verify usage of numeric vs. boolean for 'hidden' in categoriesMapping
hiddento1or0ensures compatibility with the underlying database schema. Confirm that any references tohiddenoutside this model do not treat it as a boolean to prevent type mismatches.
87-99: Preserve consistency for 'hidden' in category groupsThe pattern of converting
hiddento a numeric form matches the approach used for categories, maintaining uniformity. This appears correct for meeting DB expectations.
107-113: Confirm boolean handling intoDbmethodThe approach of using
convertForInsertvs.convertForUpdateis solid for distinct creation and update flows. If the database still stores certain payee fields as 0/1, verify that this transformation seamlessly handles boolean inputs (e.g.,favorite,learn_categories).
114-120: Avoid type mismatches infromDbmethodWhen converting DB data back to
PayeeEntity, ensure that numeric fields (if any still exist in the database) are correctly turned into booleans. This will prevent runtime issues where code assumes thatfavoriteorlearn_categoriesare strictly boolean.packages/desktop-client/src/components/payees/ManagePayees.tsx (3)
73-73: Ensure broader code references remain consistent with the narrowed type.Changing
orphanedPayeestoArray<Pick<PayeeEntity, 'id'>>is fine for the current usage. However, if other parts of the code attempt to access additional payee fields (e.g.,name), they will break due to the narrowed type.
154-172: Verify boolean usage forfavorite.The code now toggles
favoritewith booleans (trueorfalse). Note that past learnings in the repo suggest the database might storefavoriteas1or0. Confirm that these changes align with the underlying data model to avoid potential type or runtime mismatches.
174-198: Check alignment oflearn_categoriestoggling.As with
favorite,learn_categoriesis now toggled as a boolean. Validate that the rest of the codebase (including database schemas and prior usage) expects booleans instead of integers to prevent data consistency errors.packages/desktop-client/src/components/payees/PayeeTableRow.tsx (1)
183-186: Confirm boolean vs. numeric expectations infavoriteandlearn_categories.Here,
favoriteandlearn_categoriesare updated using!payee.<field>(boolean toggling). Verify that this usage does not conflict with older code or database logic that may still rely on integer (0/1) values.packages/loot-core/src/server/payees/app.ts (4)
73-81: Add error checks for merging payees.
mergePayeesdoes not validate whether the target ID is distinct from the merge IDs or whetherdb.mergePayeessucceeds. Consider adding checks or error handling to address edge cases like self-merge or an invalid target.
83-109: Ensure full transactional coverage of batch changes.Although
batchMessagesgroups the operations, partial failures could leave data in an inconsistent state. A fully transactional approach (e.g., database-level transaction or an explicit rollback strategy) may be warranted if multiple operations can fail mid-batch.
111-118: LGTM for orphaned payees check.Implementation correctly intersects the provided IDs with orphaned IDs from the database. No issues found.
120-126: No concerns with retrieving rules for a payee.
getPayeeRulesreturns serialized rule objects, consistent with the code's usage ofrules.getRulesForPayee(...). Looks good.packages/loot-core/src/server/db/index.ts (8)
19-19: Well-chosen dependency import for type safety.The addition of the
WithRequiredtype import enhances type safety across the codebase by allowing functions to specify which properties are required in parameter objects.
518-520: Type signature improvement enhances parameter validation.Adding the
WithRequired<Partial<DbPayee>, 'name'>type to theinsertPayeefunction ensures that thenameproperty is always provided while keeping other properties optional. This prevents accidentally creating payees without names.
522-522: Good type annotation for variable clarity.Explicitly typing
idasDbPayee['id']improves code clarity and ensures type consistency throughout the function. This makes the code more maintainable and helps catch potential type-related bugs during compilation.
554-554: Appropriate type constraint for update operation.The
WithRequired<Partial<DbPayee>, 'id'>type ensures that theidproperty is always provided for the update operation, which is essential sinceidis used to identify the record to update.
599-599: Return type annotation enhances API clarity.Adding the explicit return type
Promise<DbPayee[]>togetPayees()improves documentation and makes the API contract clearer. This helps consumers of this function understand what to expect without having to examine the implementation.
608-608: Explicit return type adds valuable context.The explicit return type
Promise<DbPayee[]>forgetCommonPayees()clearly communicates that the function returns the same type asgetPayees(), making the API more predictable and easier to understand.
650-650: Precise return type specification.The detailed return type
Promise<Array<Pick<DbPayee, 'id'>>>correctly indicates that the function returns only theidfield fromDbPayeeobjects. This precision helps downstream consumers understand exactly what data is available.
654-654: Accurate return type annotation.The return type
Promise<Array<DbPayee['id']>>correctly represents that this function returns an array of ID values rather than objects with ID properties. This helps distinguish it fromsyncGetOrphanedPayees().
| validate(payee: Partial<DbPayee>, { update }: { update?: boolean } = {}) { | ||
| requiredFields('payee', payee, update ? [] : ['name'], update); | ||
| return payee as DbPayee; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure 'id' is required for payee updates
Currently, the validate function passes an empty array of required fields when update is true. Typically, updates rely on an existing record's id. Consider adding id as a required field to prevent accidental creation or mismatch of payees during updates.
joel-jeremy
force-pushed
the
server-payees-handlers
branch
from
4995ae2 to
6ecbbbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/loot-core/src/server/payees/app.ts (1)
60-71: Potential large-scale performance pitfall.
getPayeeRuleCounts()iterates in memory viarules.getRules(). If these arrays grow large, consider indexing or an aggregated DB solution. Otherwise, this is acceptable for smaller datasets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4443.mdis excluded by!**/*.md
📒 Files selected for processing (16)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx(1 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx(1 hunks)packages/desktop-client/src/components/payees/ManagePayees.tsx(3 hunks)packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx(1 hunks)packages/desktop-client/src/components/payees/PayeeTableRow.tsx(1 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx(1 hunks)packages/loot-core/src/server/aql/schema/index.ts(1 hunks)packages/loot-core/src/server/db/index.ts(6 hunks)packages/loot-core/src/server/main.ts(3 hunks)packages/loot-core/src/server/models.ts(4 hunks)packages/loot-core/src/server/payees/app.ts(1 hunks)packages/loot-core/src/server/undo.ts(1 hunks)packages/loot-core/src/shared/util.ts(2 hunks)packages/loot-core/src/types/handlers.d.ts(2 hunks)packages/loot-core/src/types/models/payee.d.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/loot-core/src/server/undo.ts
- packages/loot-core/src/types/models/payee.d.ts
- packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx
- packages/desktop-client/src/components/payees/PayeeTableRow.tsx
- packages/desktop-client/src/components/payees/ManagePayees.tsx
- packages/loot-core/src/server/aql/schema/index.ts
- packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
- packages/loot-core/src/types/handlers.d.ts
- packages/loot-core/src/shared/util.ts
- packages/loot-core/src/server/db/index.ts
🧰 Additional context used
🧠 Learnings (1)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-11-10T16:45:25.627Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
🪛 Biome (1.9.4)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx
[error] 42-42: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (24)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx (1)
42-42:❓ Verification inconclusive
Simplify boolean conversion and verify consistency with data model.
The ternary operation to convert to boolean is unnecessary and can be simplified. Additionally, there might be a data model consistency issue based on the retrieved learning.
- favorite: options?.favorite ? true : false, + favorite: Boolean(options?.favorite),The retrieved learning indicates that the
favoritefield previously expected integer values (1or0), not boolean values. This change to boolean appears to be part of a broader update to the data model across multiple files (based on the AI summary). Please verify that all consuming components and backend handlers correctly handle this type change and that any database layer properly converts between the two representations if needed.
🏁 Script executed:
#!/bin/bash # Find any instances where favorite is still used with numeric values echo "Searching for instances where favorite is assigned numeric values..." rg -p "\bfavorite\s*[:=]\s*[01]\b" --type ts --type tsx # Find the type definition for PayeeEntity echo "Looking for PayeeEntity type definition..." rg -p "interface\s+PayeeEntity" --type tsLength of output: 451
Simplify and Standardize
favoriteField Conversion – Verify Data Model Consistency
- Refactor: Replace
to simplify the boolean conversion.- favorite: options?.favorite ? true : false, + favorite: Boolean(options?.favorite),- Caution: The PayeeEntity type (located in
packages/loot-core/src/types/models/payee.d.ts) appears to have historically representedfavoriteas numeric (using1/0). Ensure that all consuming components, backend handlers, and any database layers are updated to handle a boolean value, or that necessary conversions are in place.- Action: Please verify across the codebase that the change in type representation does not introduce inconsistencies.
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: 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)
packages/loot-core/src/server/main.ts (4)
45-45: Good addition for modular payee functionality.Importing the
payeesAppneatly supports the new payee-specific workflow and aligns well with the objective of separating payee logic from the main file.
64-64: Ensure proper coordination when batching messages.While adding
batchMessagesimproves performance by grouping sync operations, verify that any operation within this batch is safe to retry or rollback if an error occurs.
70-70: Nice improvement for undo/redo logic.Importing
clearUndo, undo, redo, withUndoindicates a more robust approach to transaction-like modifications. This fosters better user experience by allowing easy revert of recent changes.
1424-1424: Consistent with the new payees module.Including
payeesAppinapp.combinefulfills the refactoring goal of modularizing payee handlers. No concerns here.packages/loot-core/src/server/payees/app.ts (11)
1-10: Imports and initialization look consistent.All required modules (database, mutator, rules, etc.) are imported. Ensure all references (e.g.,
payeeModel,rules, etc.) are used as intended without unused dependencies.
11-21: Well-defined handler interface.Defining a dedicated
PayeesHandlerstype clarifies the contract for payee operations. This is a maintainable approach.
23-41: Encapsulated payee methods align with app structure.Registering payee methods in a separate app is a clean modular design. The usage of
mutatorandundoableconsistently follows the project’s patterns for reversible operations.
42-44: Simple payee creation logic.
createPayeedirectly inserts a new record into the DB with minimal overhead. Verify external callers handle user input sanitation.
46-49: Returning domain entities vs. raw DB data.The decision to map DB rows to
payeeModel.fromDbensures consistent entity shapes. This helps maintain a clear separation between DB and UI layers.
51-54: Consistent pattern for getPayees.Same approach as
getCommonPayees, providing domain-level shapes. No issues found.
56-58: Efficient retrieval of orphaned payees.
getOrphanedPayeesis straightforward. Confirm thatdb.syncGetOrphanedPayees()handles edge cases like an empty list.
73-81: Merge logic is concise.
mergePayeesleveragesdb.mergePayeeswithin an undoable scope. Check for edge cases:
- Merging nonexistent payees
- Attempting to merge into a deleted payee
83-109: ** Refactor suggestion: Ensure transactional batch updates.**As previously noted, consider wrapping the batch operation in a database transaction if partial writes or DB-level rollback is required. Currently,
batchMessagesalone might not guarantee an atomic rollback of DB statements on failure.
111-118: Straightforward orphan check logic.
checkOrphanedPayeesuses a set comparison with DB results. This approach is efficient and readable.
120-126: Correct usage of rule serialization.Returning
rule.serialize()ensures the client gets consistent data shape for payee-specific rules.packages/loot-core/src/server/models.ts (4)
1-9: Reorganized imports for payee logic.These changes ensure that
PayeeEntityis properly handled alongside the existing transformations (convertForInsert, etc.). No issues found.
55-65: Account model validation.Allowing partial DB account objects in the
validatefunction is flexible, but be sure to confirm that fields likename,offbudget, etc. are always properly set or defaulted for new inserts.
68-83: Category model updates.Requiring
cat_groupfor new categories but ignoring it for updates is sensible. Also ensuringhiddenis converted to numeric is consistent with DB expectations.
102-120: ** Revisit requirement of 'id' for updates.**As previously suggested, requiring
'id'whenupdateis true can prevent incorrect or conflicting payee records. Currently, no fields are enforced inrequiredFields('payee', payee, update ? [] : ['name'], update).packages/loot-core/src/types/server-handlers.d.ts (4)
7-7: Import statement correctly updated to remove payee-related types.The import statement has been modified to only include
CategoryEntityandCategoryGroupEntity, removing the payee-related types as part of the extraction of payee functionality to a dedicated module.
13-273:✅ Verification successful
Verify the PayeesHandlers type is properly defined.
The PR summary mentions extracting payees handlers to a dedicated module, but we should ensure that a proper type definition for
PayeesHandlersexists and is exported from an appropriate location.Let's check if a
PayeesHandlerstype has been defined and is being used correctly:
🏁 Script executed:
#!/bin/bash # Check if PayeesHandlers type has been defined echo "Searching for PayeesHandlers type definition..." rg -t ts "interface PayeesHandlers" packages/loot-core/src/ # Check if the type is being imported and used in the app.ts file echo "Checking if the type is being used in app.ts..." rg -t ts "PayeesHandlers" packages/loot-core/src/server/payees/app.tsLength of output: 420
PayeesHandlers type is correctly defined and utilized
The
PayeesHandlerstype is defined as a type alias inpackages/loot-core/src/server/payees/app.tsand is properly used when initializing the app withcreateApp<PayeesHandlers>(). No further changes are needed.
13-273:✅ Verification successful
LGTM: Payee handlers successfully removed from ServerHandlers interface.
The
ServerHandlersinterface no longer contains the payee-related methods that were extracted to the new payees module. This refactoring improves code organization by grouping related functionality together.Let me verify that all the payee-related handlers have been properly moved to the new location:
🏁 Script executed:
#!/bin/bash # Verify that payee handlers exist in the new location echo "Checking for payee handlers in the new location..." rg -t ts "payee-create|common-payees-get|payees-get|payees-get-rule-counts|payees-merge|payees-batch-change|payees-check-orphaned|payees-get-orphaned|payees-get-rules" packages/loot-core/src/server/payees/app.ts # Verify that payeesApp is properly imported and combined in main.ts echo "Checking that payeesApp is properly imported and combined in main.ts..." rg -t ts "import.*payeesApp.*from.*./payees/app" packages/loot-core/src/server/main.ts rg -t ts "app.combine.*payeesApp" packages/loot-core/src/server/main.tsLength of output: 1557
LGTM: Payee handlers have been successfully relocated.
The
ServerHandlersinterface has no longer been cluttered with payee-related methods. Our verification confirms that all payee handlers (e.g.,payee-create,common-payees-get, etc.) are now correctly implemented in the new module atpackages/loot-core/src/server/payees/app.tsand that the new module is properly imported inpackages/loot-core/src/server/main.ts.
13-273:❓ Verification inconclusive
Check for any missing payee-related functionality.
Given the comment from matt-fidd about per-payee category learning settings not working as intended, we should verify that all necessary payee functionality has been properly migrated.
Let's check if all payee-related functionality has been properly migrated, particularly focusing on category learning settings:
🏁 Script executed:
#!/bin/bash # Search for payee category learning settings in the codebase echo "Searching for payee category learning settings..." rg -t ts "category.*learning|learning.*category" --context 2 packages/loot-core/src/ # Look for UI components related to per-payee category learning echo "Looking for UI components related to per-payee category learning..." rg -t tsx "category.*learning|learning.*category" --context 2 packages/loot-core/src/Length of output: 779
Verify Payee Category Learning Functionality Migration
The initial search in TypeScript files returned one occurrence of
"payee-category-learning"inpackages/loot-core/src/client/modals/modalsSlice.ts, which suggests that a reference exists. However, our subsequent attempt to search UI components in.tsxfiles resulted in a file-type error. Since the intended UI aspects for per-payee category learning might reside in.tsxfiles, please manually verify that all relevant UI components or other UI references have been properly migrated and integrated. Additionally, confirm that the absence of payee-related handlers inserver-handlers.d.tsis expected.
- File Reference:
packages/loot-core/src/client/modals/modalsSlice.tscontains the"payee-category-learning"reference.- Action Needed:
- Manually verify the existence and correctness of UI components in
.tsxfiles related to per-payee category learning.- Confirm that the server-side functionality (or intentional omission thereof) aligns with the intended payee migration.
joel-jeremy
force-pushed
the
server-payees-handlers
branch
from
6ecbbbf to
afafeca
Compare
This show now be fixed. |
Related to #1113