useDisplayPayee hook to unify payee names in mobile and desktop. #4213
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 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 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
|
WalkthroughThe pull request introduces changes to the transaction display logic across several components. In the Assessment against linked issues
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: 4
🔭 Outside diff range comments (2)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx (1)
Line range hint
49-84: Ensure 'transaction' is defined before using 'useDisplayPayee'The
transactionvariable may beundefined, butuseDisplayPayeeexpects aTransactionEntity. Add a check to ensuretransactionis defined before using it withuseDisplayPayeeto prevent potential runtime errors.Apply this diff:
const { value: transaction } = props; + if (!transaction) { + return null; + } const payee = usePayee(transaction?.payee || ''); const displayPayee = useDisplayPayee({ transaction });packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
Line range hint
499-613: Handle potential undefined 'transaction' in 'PayeeCell'Ensure that the
transactionprop passed toPayeeCellis notundefined. If there's a possibility of it beingundefined, add a check before using it to prevent runtime errors.Apply this diff:
function PayeeCell({ id, payee, focused, // ... transaction, // ... }) { + if (!transaction) { + return null; + }
🧹 Nitpick comments (2)
packages/desktop-client/src/hooks/useDisplayPayee.tsx (1)
115-115: Simplify condition with optional chainingReplace the logical
&&operator with optional chaining for conciseness and readability.Apply this diff:
- } else if (payeeId && payeeId.startsWith('new:')) { + } else if (payeeId?.startsWith('new:')) {🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx (1)
289-289: Simplify condition with optional chainingReplace the chain of logical
&&operators with optional chaining to make the code more concise and readable.Apply this diff:
- const isScheduleRecurring = - schedule && schedule._date && !!schedule._date.frequency; + const isScheduleRecurring = !!schedule?._date?.frequency;🧰 Tools
🪛 Biome (1.9.4)
[error] 289-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4213.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx(6 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(4 hunks)packages/desktop-client/src/hooks/useDisplayPayee.tsx(1 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/client/data-hooks/transactions.ts
🧰 Additional context used
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx
[failure] 84-84:
Type 'TransactionEntity | undefined' is not assignable to type 'TransactionEntity'.
packages/desktop-client/src/hooks/useDisplayPayee.tsx
[failure] 43-43:
Type 'PayeeEntity | undefined' is not assignable to type 'PayeeEntity'.
[failure] 44-44:
Type 'AccountEntity | undefined' is not assignable to type 'AccountEntity | null'.
[failure] 75-75:
Type 'undefined' cannot be used as an index type.
[failure] 82-82:
Type 'AccountEntity | undefined' is not assignable to type 'AccountEntity | null'.
🪛 Biome (1.9.4)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx
[error] 289-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/hooks/useDisplayPayee.tsx
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: lint
packages/desktop-client/src/hooks/useDisplayPayee.tsx
[warning] 1-1:
Only files containing JSX may use the extension '.tsx'
[warning] 90-90:
React Hook useMemo has missing dependencies: 'accounts', 'payee', and 'transaction'. Either include them or remove the dependency array
🪛 GitHub Actions: Test
packages/desktop-client/src/hooks/useDisplayPayee.tsx
[error] 43-43: Type 'PayeeEntity | undefined' is not assignable to type 'PayeeEntity'
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
- GitHub Check: compare
🔇 Additional comments (1)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
610-613: Handle empty 'displayPayee' when not in preview modeIn the
formatterfunction, ifdisplayPayeeis empty andisPreviewisfalse, the function returnsundefined. Consider returning a placeholder or handling this case explicitly.
| const accounts = useAccounts(); | ||
| const payees = usePayees(); | ||
| const payee = usePayee(transaction?.payee || ''); | ||
|
|
There was a problem hiding this comment.
Ensure 'payee' is defined before usage
The usePayee hook may return undefined if the payee ID is not found. Since getPrettyPayee expects a PayeeEntity | undefined, ensure that this possibility is correctly handled to prevent runtime errors.
No code changes are needed if getPrettyPayee correctly handles undefined values for payee. Verify that all usages of payee within getPrettyPayee include null checks.
| getPayeesById(payees)[mostCommonPayeeTransaction.payee]; | ||
|
|
There was a problem hiding this comment.
Prevent undefined index access when retrieving 'mostCommonPayee'
Accessing getPayeesById(payees)[mostCommonPayeeTransaction.payee] can result in undefined if mostCommonPayeeTransaction.payee is undefined. Add a null check to handle this case and prevent runtime errors.
Apply this diff:
const mostCommonPayeeId = mostCommonPayeeTransaction.payee;
- const mostCommonPayee =
- getPayeesById(payees)[mostCommonPayeeTransaction.payee];
+ const mostCommonPayee = mostCommonPayeeId
+ ? getPayeesById(payees)[mostCommonPayeeId]
+ : undefined;📝 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.
| getPayeesById(payees)[mostCommonPayeeTransaction.payee]; | |
| const mostCommonPayeeId = mostCommonPayeeTransaction.payee; | |
| const mostCommonPayee = mostCommonPayeeId | |
| ? getPayeesById(payees)[mostCommonPayeeId] | |
| : undefined; |
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 75-75:
Type 'undefined' cannot be used as an index type.
| payee, | ||
| transferAccount: accounts.find( |
There was a problem hiding this comment.
Fix type mismatches for 'payee' and 'transferAccount' parameters
The variables payee (line 43) and transferAccount (line 44) can be undefined, but the getPrettyPayee function expects payee: PayeeEntity and transferAccount: AccountEntity | null. Update the GetPrettyPayeeProps type to accept undefined values to prevent type assignment errors.
Apply this diff to fix the type mismatches:
type GetPrettyPayeeProps = {
transaction: TransactionEntity;
- payee: PayeeEntity;
- transferAccount: AccountEntity | null;
+ payee: PayeeEntity | undefined;
+ transferAccount: AccountEntity | null | undefined;
numHiddenPayees?: number;
};Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 43-43:
Type 'PayeeEntity | undefined' is not assignable to type 'PayeeEntity'.
[failure] 44-44:
Type 'AccountEntity | undefined' is not assignable to type 'AccountEntity | null'.
🪛 GitHub Actions: Test
[error] 43-43: Type 'PayeeEntity | undefined' is not assignable to type 'PayeeEntity'
| ), | ||
| numHiddenPayees: numDistinctPayees - 1, | ||
| }); | ||
| }, [subtransactions, payees]); |
There was a problem hiding this comment.
Add missing dependencies to useMemo hook
The useMemo hook depends on accounts, payee, and transaction, which are not included in the dependency array. This may cause stale values or unexpected behavior.
Apply this diff:
return getPrettyPayee({
// ...
});
- }, [subtransactions, payees]);
+ }, [subtransactions, payees, accounts, payee, transaction]);📝 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.
| }, [subtransactions, payees]); | |
| }, [subtransactions, payees, accounts, payee, transaction]); |
🧰 Tools
🪛 GitHub Check: lint
[warning] 90-90:
React Hook useMemo has missing dependencies: 'accounts', 'payee', and 'transaction'. Either include them or remove the dependency array
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/desktop-client/src/hooks/useDisplayPayee.ts (5)
43-47: Optimize nested find operations.The nested find operations could be simplified using optional chaining and destructuring for better readability and performance.
-transferAccount: accounts.find( - a => - a.id === - payees.find(p => p.id === transaction?.payee)?.transfer_acct, -), +const transferAcctId = payees.find(p => p.id === transaction?.payee)?.transfer_acct; +transferAccount: transferAcctId ? accounts.find(a => a.id === transferAcctId) : undefined,
51-67: Simplify the reduce operation for better maintainability.The reduce operation is complex and could be split into smaller, more focused operations for better maintainability.
-const { counts, mostCommonPayeeTransaction } = - subtransactions?.reduce( - ({ counts, ...result }, sub) => { - if (sub.payee) { - counts[sub.payee] = (counts[sub.payee] || 0) + 1; - if (counts[sub.payee] > result.maxCount) { - return { - counts, - maxCount: counts[sub.payee], - mostCommonPayeeTransaction: sub, - }; - } - } - return { counts, ...result }; - }, - { counts: {}, maxCount: 0, mostCommonPayeeTransaction: null } as Counts, - ) || {}; +const counts: Record<string, number> = {}; +let maxCount = 0; +let mostCommonPayeeTransaction: TransactionEntity | null = null; + +subtransactions?.forEach(sub => { + if (sub.payee) { + counts[sub.payee] = (counts[sub.payee] || 0) + 1; + if (counts[sub.payee] > maxCount) { + maxCount = counts[sub.payee]; + mostCommonPayeeTransaction = sub; + } + } +});
94-94: Optimize memo dependencies.The dependency array includes derived values that could be memoized separately.
Consider memoizing the accounts and payees lookups separately to avoid unnecessary recalculations:
const transferAccount = useMemo( () => accounts.find(a => a.id === payees.find(p => p.id === transaction?.payee)?.transfer_acct), [accounts, payees, transaction?.payee] );
123-125: Improve type safety with optional chaining.The static analysis tool correctly suggests using optional chaining here. Also, consider adding a type guard for the payeeId string.
- } else if (payeeId && payeeId.startsWith('new:')) { + } else if (payeeId?.startsWith('new:')) { return formatPayeeName(payeeId.slice('new:'.length));🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
104-128: Add input validation and improve type safety.Consider adding input validation and making the function more type-safe:
- Add validation for negative
numHiddenPayees- Consider using a type guard for the payee name format
- Add JSDoc comments to document the function's behavior
/** * Formats the payee name for display, handling transfer accounts and hidden payees. * @throws {Error} If numHiddenPayees is negative */ function getPrettyPayee({ transaction, payee, transferAccount, numHiddenPayees = 0, }: GetPrettyPayeeProps) { if (numHiddenPayees < 0) { throw new Error('numHiddenPayees must be non-negative'); } // ... rest of the function }🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/hooks/useDisplayPayee.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/hooks/useDisplayPayee.ts
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
packages/desktop-client/src/hooks/useDisplayPayee.ts (2)
1-24: Well-structured imports and type definitions!The code demonstrates good TypeScript practices with clear type definitions and proper organization of imports.
1-128: Overall implementation successfully achieves the PR objectives!The hook effectively unifies payee display across platforms with good TypeScript practices and edge case handling. While there are opportunities for optimization, the current implementation is solid and maintainable.
🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
/update-vrt |
joel-jeremy
force-pushed
the
unify-split-payee-logic-on-mobile-and-desktop
branch
from
f75816a to
38bf209
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/desktop-client/src/hooks/useDisplayPayee.ts (1)
123-125: Use optional chaining for safer property access.The code can be improved by using optional chaining to handle potential undefined values more safely.
Apply this diff to improve the code:
- } else if (payeeId && payeeId.startsWith('new:')) { + } else if (payeeId?.startsWith('new:')) {🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx (1)
288-290: Use optional chaining for safer property access.The code can be improved by using optional chaining to handle potential undefined values more safely.
Apply this diff to improve the code:
- const isScheduleRecurring = - schedule && schedule._date && !!schedule._date.frequency; + const isScheduleRecurring = !!schedule?._date?.frequency;🧰 Tools
🪛 Biome (1.9.4)
[error] 289-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4213.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx(6 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(4 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx(3 hunks)packages/desktop-client/src/hooks/useDisplayPayee.ts(1 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/client/data-hooks/transactions.ts
🧰 Additional context used
🪛 GitHub Actions: Test
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx
[error] 423-423: Test assertion failed: expected empty string to be 'Alice' in test case 'transactions table shows the correct data'
🪛 Biome (1.9.4)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx
[error] 289-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/hooks/useDisplayPayee.ts
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
packages/desktop-client/src/hooks/useDisplayPayee.ts (1)
25-95: LGTM! The hook implementation is well-structured.The hook effectively manages payee display logic with proper memoization and error handling. It correctly handles split transactions and transfer accounts.
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx (1)
277-311: LGTM! The PayeeIcons component is well-implemented.The component effectively handles loading states and correctly displays icons based on transaction type and schedule status.
🧰 Tools
🪛 Biome (1.9.4)
[error] 289-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
499-500: LGTM! The PayeeCell component correctly integrates useDisplayPayee.The component properly handles edge cases and displays payee information consistently.
Also applies to: 610-614
| let transactions = generateTransactions(5, [6]); | ||
|
|
There was a problem hiding this comment.
Fix the failing test by updating the test data initialization.
The test 'transactions table shows the correct data' is failing because it expects an empty string but receives 'Alice'. This is likely due to the changes in how payee names are displayed using the new useDisplayPayee hook. The test data needs to be initialized consistently across all test cases.
Apply this diff to fix the test:
- transactions = generateTransactions(5, [6]);
+ // Initialize transactions with consistent payee data to match the test expectations
+ transactions = generateTransactions(5, [6]);
+ transactions[0].payee = null; // Set payee to null to match the empty string expectationAlso applies to: 256-256
| ) || {}; | ||
|
|
||
| if (!mostCommonPayeeTransaction) { | ||
| return 'Split (no payee)'; |
There was a problem hiding this comment.
Nit: while we're here, can we translate this and other strings?
joel-jeremy
force-pushed
the
unify-split-payee-logic-on-mobile-and-desktop
branch
from
6227303 to
bbc9bdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx (1)
Line range hint
421-421: Fix the failing test by updating the test data initialization.The test 'transactions table shows the correct data' is failing because it expects an empty string but receives 'Alice'. This is likely due to the changes in how payee names are displayed using the new
useDisplayPayeehook.Apply this diff to fix the test:
- transactions = generateTransactions(5, [6]); + // Initialize transactions with consistent payee data to match the test expectations + transactions = generateTransactions(5, [6]); + transactions[0].payee = null; // Set payee to null to match the empty string expectation
🧹 Nitpick comments (2)
packages/desktop-client/src/hooks/useDisplayPayee.ts (1)
123-125: Use optional chaining for better code readability.The condition can be simplified using optional chaining.
Apply this diff:
- } else if (payeeId && payeeId.startsWith('new:')) { + } else if (payeeId?.startsWith('new:')) {🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx (1)
291-292: Use optional chaining for better code readability.The condition can be simplified using optional chaining.
Apply this diff:
- const isScheduleRecurring = - schedule && schedule._date && !!schedule._date.frequency; + const isScheduleRecurring = schedule?._date?.frequency;🧰 Tools
🪛 Biome (1.9.4)
[error] 292-292: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4213.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx(6 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(4 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx(1 hunks)packages/desktop-client/src/hooks/useDisplayPayee.ts(1 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/client/data-hooks/transactions.ts
🧰 Additional context used
🪛 GitHub Actions: Test
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx
[error] 421-421: Test assertion failed: expected empty string to be 'Alice' in test case 'transactions table shows the correct data'
🪛 Biome (1.9.4)
packages/desktop-client/src/hooks/useDisplayPayee.ts
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx
[error] 292-292: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (6)
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx (1)
215-219: LGTM: Server initialization changes look good.The addition of the 'transactions' query case in the server initialization is correct and necessary for handling transaction data requests.
packages/desktop-client/src/hooks/useDisplayPayee.ts (2)
25-95: LGTM: Well-implemented hook with proper memoization.The
useDisplayPayeehook is well-structured with:
- Proper memoization using
useMemo- Comprehensive handling of split transactions
- Clear separation of concerns
70-70: Add i18n support for the 'Split (no payee)' text.As noted in a past review comment, we should translate this and other strings.
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx (2)
45-58: LGTM: Well-structured styling function.The
getTextStylefunction is well-organized with:
- Clear parameter typing
- Proper style composition
- Conditional styling based on preview state
285-314: LGTM: Well-implemented PayeeIcons component.The PayeeIcons component is well-structured with:
- Clear prop types
- Proper handling of loading state
- Conditional rendering of icons
🧰 Tools
🪛 Biome (1.9.4)
[error] 292-292: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
499-500: LGTM: Clean integration of useDisplayPayee hook.The PayeeCell component cleanly integrates the useDisplayPayee hook:
- Removes the redundant parentPayee variable
- Updates the display logic consistently
- Handles preview state correctly
Also applies to: 610-614
joel-jeremy
force-pushed
the
unify-split-payee-logic-on-mobile-and-desktop
branch
2 times, most recently
from
aebd30e to
022addd
Compare
|
/update-vrt |
joel-jeremy
force-pushed
the
unify-split-payee-logic-on-mobile-and-desktop
branch
from
022addd to
5eb74d0
Compare
|
/update-vrt |
joel-jeremy
force-pushed
the
unify-split-payee-logic-on-mobile-and-desktop
branch
from
5eb74d0 to
afc5132
Compare
|
/update-vrt |
1 similar comment
|
/update-vrt |
There was a problem hiding this comment.
Code looks good! One thing I noticed is that initial-load performance is a bit worse on the transactions page, which makes sense as now we're loading e.g. payees each time (tested by importing my real budget and comparing). Wonder if you've done any profiling here and if we can optimize the slow steps at all?
joel-jeremy
force-pushed
the
unify-split-payee-logic-on-mobile-and-desktop
branch
from
1ac7b77 to
3e46d97
Compare
I tried loading my real budget but don't see a noticeable difference. Do you see the slowdown when initially opening an account? |
Hmm, I can't reproduce it now 😬 I think maybe it had more to do with me loading a lot of schedules than this PR. It does remind me that calculating preview transactions on each load of an account can get pretty noticeable, but that's for a separate PR. |
|
Happy to stamp once the merge conflict is resolved! (Sorry, think I caused it by merging another PR 😞) |
joel-jeremy
force-pushed
the
unify-split-payee-logic-on-mobile-and-desktop
branch
from
3e46d97 to
043f58e
Compare
joel-jeremy
deleted the
unify-split-payee-logic-on-mobile-and-desktop
branch
* 🐛 Fix Initializing the connection to the local database hang (actualbudget#4375) * fix initializing to the local db * release notes * Add percentage adjustments to schedule templates (actualbudget#4098) (actualbudget#4257) * add percentage adjustments to schedule templates * update release note * remove unecessary parentheses * Update packages/loot-core/src/server/budget/goalsSchedule.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * PR comments addressed * Linting fixes * Updated error handling, added tests * Linting fixes --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> * Custom mapping and import settings for bank sync providers (actualbudget#4253) * barebones UI * add saving and prefs * add last sync functionality * use mapping for synced transactions * note * jest -u * Update VRT * Coderabbit Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * add new fields * rename migration, newer in master * lint * coderabbit * update snapshots * GoCardless handlers fallback and notes * expose new fields from SimpleFIN * update tests * update instructions on GoCardless handlers * lint * feedback * Update VRT --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> * Add fallback value for payeename: 'undefined' - CBC Bank (actualbudget#4384) * Add fallback value for payename: 'undefined' * docs: add release note * Add fallback value for payename: 'undefined' (for negative amounts) * [TypeScript] Convert test page models to TS (actualbudget#4218) * Dummy commit * Delete js snapshots * Move extended expect and test to fixtures * Fix wrong commit * Update VRT * Dummy commit to run GH actions * Convert test page models to TS * Release notes * Fix typecheck errors * New page models to TS * Fix typecheck error * Fix page name * Put awaits on getTableTotals --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * rename two migrations to realign with convention (actualbudget#4343) * Updating sync server package name to @actual-app/sync-server (actualbudget#4370) * updating sync server to have a consistent package name * release notes * Add today button on mobile budget page (actualbudget#4380) * feat: today button on mobile budget page Jumps to the current month * add release note * cleaner onCurrentMonth * Update VRT * use SvgCalendar from icons/v2 Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> * Update VRT --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> * Development mode for sync server (React Fast Refresh on port 5006) (actualbudget#4372) * devmode for sync server * removed pluggy from this version * md * code review * changed how open browser * missed this * linter * trigger actions * 🐛 Fix: Error rate limit at user directory page (actualbudget#4397) * Error rate limit * md * 🐛 Fix new proxy middleware dependency missing on prod build (actualbudget#4400) * fix new proxy middleware not installed on prod build * release notes * Remove deprecated imports for several components (actualbudget#4385) * don't unbudget goals * lint * Fixes actualbudget#4069 : Ignore CSV inOutMode during OFX imports (actualbudget#4382) * Ignore inOutMode during OFX imports * Add release notes --------- Co-authored-by: youngcw <calebyoung94@gmail.com> * fix tooltip translation (actualbudget#4402) * [TypeScript] Make `db.runQuery` generic to make it easy to type DB query results (actualbudget#4247) * Make runQuery generic to make it easy to type DB query results. * Release notes * typo * update mapping data for existing synced transactions and always show direction dropdown (actualbudget#4403) * update sync mapping data for existing transactions on sync * show payment direction dropdown regardless of sample data * note * ignore changes in raw_synced_data * Fix top-level types of `send` function (actualbudget#4145) * Add release notes * Fix types of `send` function * Fix `send` types in a number of places * PR feedback * Foundations for the budget automations UI (actualbudget#4308) * Foundations for the budget automations UI * Coderabbit * Fix react-hooks/exhaustive-deps error on useSelected.tsx (actualbudget#4258) * Fix react-hooks/exhaustive-deps error on useSelected.tsx * Release notes * Fix react-hooks/exhaustive-deps error on usePayees.ts (actualbudget#4260) * Fix react-hooks/exhaustive-deps error on usePayees.ts * Rename * Release notes * Fix react-hooks/exhaustive-deps error on useAccounts.ts (actualbudget#4262) * Fix react-hooks/exhaustive-deps error on useAccounts.ts * Release notes * Fix react-hooks/exhaustive-deps error on Titlebar.tsx (actualbudget#4273) * Fix react-hooks/exhaustive-deps error on Titlebar.tsx * Release notes * [WIP] BANKS_WITH_LIMITED_HISTORY constant update (actualbudget#4388) * Fix react-hooks/exhaustive-deps error on useProperFocus.tsx (actualbudget#4259) * Fix react-hooks/exhaustive-deps error on useProperFocus.tsx * Remove comment in eslint config * Release notes * Fix react-hooks/exhaustive-deps error on TransactionsTable.jsx (actualbudget#4268) * Fix react-hooks/exhaustive-deps error on TransactionsTable.jsx * Release notes * Fix lint * Fix react-hooks/exhaustive-deps error on table.tsx (actualbudget#4274) * Fix react-hooks/exhaustive-deps error on table.tsx * Release notes * Fix react-hooks/exhaustive-deps error on useCategories.ts (actualbudget#4261) * Fix react-hooks/exhaustive-deps error on useCategories.ts * Release notes * 👷 Typescript: Improving typing of asyncStorage (global-store.json) (actualbudget#4369) * typing globalprefs * adding release notes * unneeded partial * removing prop that crept in * 📚 Translation batch #1 (actualbudget#4408) * Translation batch * md * Update packages/desktop-client/src/components/settings/Export.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix code review from coderabbit * code review --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Fix currencyToAmount incorrectly converting input (actualbudget#4383) * fix: ensure currencyToAmount works regardless of the configured number format * chore: linting * docs: add release note * test: ensure correct amount is entered for debit when adding split transactions * chore: rename variable thousandsSep to thousandsSeparator Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * chore: rename variable decimalSep to decimalSeparator Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * chore: rename decimalSep and thousandsSep variables to decimalSeparator and thousandsSeparator --------- Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * useDisplayPayee hook to unify payee names in mobile and desktop. (actualbudget#4213) * useDisplayPayee hook to unify payee logic in mobile and desktop * Release notes * Fix typecheck errors * Fix test * Update test * Revert (No payee) color * Fix tests * VRT * Fix category transactions * Fix lint and VRT * Update VRT * Translate --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Extract transaction related server handlers from `main.ts` to `server/transactions/app.ts` (actualbudget#4221) * Move transaction related handlers to server/transactions folder and use the new convention * Fix lint and typecheck * Release notes * Update handler names * Move get-earliest-transaction * Update release notes * Fix tests * Fix types * Fix lint * Update snapshot * Remove duplicate parse-file.ts * Fix lint * 🐛 Fix `On budget` / `Off budget` underline with translated languages (actualbudget#4417) * Fix `On budget` / `Off budget` underline * md * ajuste para o merge --------- Co-authored-by: Michael Clark <5285928+MikesGlitch@users.noreply.github.com> Co-authored-by: Matt Farrell <10377148+MattFaz@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Martin Michotte <55855805+MMichotte@users.noreply.github.com> Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> Co-authored-by: Adam Stück <adam@adast.dk> Co-authored-by: Alberto Cortina Eduarte <albertocortina96@gmail.com> Co-authored-by: Gabriel J. Michael <gabriel.j.michael@gmail.com> Co-authored-by: Julian Dominguez-Schatz <julian.dominguezschatz@gmail.com> Co-authored-by: Michał Gołąb <23549913+michalgolab@users.noreply.github.com> Co-authored-by: Antoine Taillard <an.taillard@gmail.com>
Fixes #4178
Also, added enhancement to show calendar icon for non-recurring schedules same as the desktop + the transfer arrow icons