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 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 |
|
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 14 minutes and 17 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 selected for processing (2)
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.
Related to #1113