extend fix splits tool to report splits with mismatched amounts #3970
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
matt-fidd
changed the title
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/settings/FixSplits.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces enhancements to the handling and reporting of split transactions across multiple files in the desktop client and core application. The changes focus on improving the detection and reporting of issues related to split transactions. In the Correspondingly, the Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)packages/desktop-client/src/components/settings/FixSplits.tsx (5)
The change from a single string to an array of strings is a good approach for handling multiple message types, and the destructuring is clean.
The addition of
Good use of theme colors to provide visual distinction between normal and error states.
The layout changes properly implement flex layout with appropriate spacing and alignment, addressing the previous feedback about avoiding
The message construction for mismatched splits is well-structured, but let's verify the date format consistency with other parts of the application. ✅ Verification successfulBased on the search results, I can see that dates in the codebase are consistently handled in ISO format ('YYYY-MM-DD') internally, and when displayed, they are formatted according to the user's preferred date format using utility functions. Let's verify the specific date handling in the FixSplits component and its related utilities: Let's check how the date is being used in the FixSplits component and verify if there are any date formatting utilities being used: Based on the comprehensive analysis of the FixSplits component and date handling in the codebase, I can now generate the final response: Date format is consistent with codebase standards The date handling in the mismatched splits message is correct and consistent with the codebase. The dates are received directly from the server response ( 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for date formatting patterns in the codebase
rg -A 2 "t\.date|date:" packages/desktop-client/src/
Length of output: 35810 Script: #!/bin/bash
# Search for date-related imports and utilities in FixSplits
rg -A 3 "import.*date|formatDate|parseDate" packages/desktop-client/src/components/settings/FixSplits.tsx
# Look for the Transaction type definition that includes the date field
ast-grep --pattern 'type Transaction = {
$$$
date: $_
$$$
}'
Length of output: 938 Script: #!/bin/bash
# Get the full content of FixSplits component to understand the date handling
cat packages/desktop-client/src/components/settings/FixSplits.tsx
# Search for date utility functions in the codebase
rg -g '!*.test.*' -g '!*.d.ts' "export.*function.*date|export.*const.*date" packages/desktop-client/src/
Length of output: 5117 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 (3)
packages/loot-core/src/server/tools/app.ts (1)
60-69: Consider adding an index hint for performance.The query fetches all parent transactions with their splits. As the dataset grows, this could become performance-intensive. Consider adding appropriate database indexes and limiting the result set if possible.
packages/desktop-client/src/components/settings/FixSplits.tsx (2)
38-42: Consider showing more transaction details.The current implementation only shows the date for mismatched splits. Consider adding more identifying information such as the transaction amount, account, or payee to help users locate these transactions more easily.
if (mismatchedSplits.length > 0) { result.push( `Found ${mismatchedSplits.length} split transaction${mismatchedSplits.length > 1 ? 's' : ''} with mismatched amounts:\n` + - mismatchedSplits.map(t => `- ${t.date}`).join('\n'), + mismatchedSplits.map(t => + `- ${t.date} | ${t.payee || 'No payee'} | Amount: ${t.amount} (Expected: ${ + t.subtransactions.reduce((acc, st) => acc + st.amount, 0) + })` + ).join('\n'), ); }
Line range hint
45-57: Improve accessibility for screen readers.Consider adding ARIA labels or roles to improve the screen reader experience for the results message, especially since it contains structured information about mismatched splits.
return ( <Paragraph + role="status" + aria-live="polite" style={{ color: theme.noticeTextLight, marginBottom: 0, marginLeft: '1em', textAlign: 'right', whiteSpace: 'pre-wrap', }} > {result.join('\n')} </Paragraph> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/desktop-client/src/components/settings/FixSplits.tsx(3 hunks)packages/loot-core/src/server/tools/app.ts(2 hunks)packages/loot-core/src/server/tools/types/handlers.d.ts(1 hunks)
🔇 Additional comments (1)
packages/loot-core/src/server/tools/types/handlers.d.ts (1)
1-1: LGTM! Type definitions are well-structured.
The addition of mismatchedSplits: TransactionEntity[] to the return type properly extends the interface to support the new mismatched amounts reporting feature.
Also applies to: 8-8
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/loot-core/src/server/tools/app.ts (1)
70-74:⚠️ Potential issueHandle floating-point arithmetic precision for monetary calculations.
When comparing monetary amounts, floating-point arithmetic can lead to precision issues. Consider using a decimal arithmetic library or rounding to a fixed precision before comparison.
const mismatchedSplits = splitTransactions.filter(t => { - const subValue = t.subtransactions.reduce((acc, st) => acc + st.amount, 0); + const subValue = Number(t.subtransactions.reduce((acc, st) => acc + st.amount, 0).toFixed(2)); + const parentValue = Number(t.amount.toFixed(2)); - return subValue !== t.amount; + return subValue !== parentValue; });
🧹 Nitpick comments (1)
packages/loot-core/src/server/tools/app.ts (1)
59-68: Consider optimizing the query by selecting only required fields.The query currently selects all fields (
select('*')). For better performance, consider selecting only the fields needed for the mismatched splits check (e.g., 'id', 'amount', 'subtransactions').const splitTransactions = ( await runQuery( q('transactions') .options({ splits: 'grouped' }) .filter({ is_parent: true, }) - .select('*'), + .select(['id', 'amount', 'subtransactions']), ) ).data;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3970.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/loot-core/src/server/tools/app.ts(2 hunks)packages/loot-core/src/server/tools/types/handlers.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/tools/types/handlers.d.ts
🔇 Additional comments (2)
packages/loot-core/src/server/tools/app.ts (2)
2-2: LGTM!
The new imports are correctly added and necessary for the query functionality.
Also applies to: 5-5
80-80: LGTM!
The addition of mismatchedSplits to the return object aligns with the PR objective and maintains type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/desktop-client/src/components/settings/FixSplits.tsx (1)
Line range hint
18-72: Consider adding error boundary protectionGiven that this component handles critical data repair operations, consider wrapping it in an error boundary to gracefully handle any rendering errors that might occur due to unexpected data formats or failed operations.
This would help prevent the entire UI from crashing if something goes wrong during the repair process or result rendering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/settings/FixSplits.tsx(3 hunks)
🔇 Additional comments (5)
packages/desktop-client/src/components/settings/FixSplits.tsx (5)
18-19: LGTM: Good refactor to support multiple messages
The change from a single string to an array of strings is a clean approach that makes the message handling more maintainable and extensible.
21-27: LGTM: Comprehensive check for all error conditions
The condition properly includes the new mismatched splits check while maintaining the existing validation logic.
51-54: LGTM: Good use of theme-based error indication
The conditional styling provides clear visual feedback using consistent theme colors.
59-59: LGTM: Clean message rendering
The array join with newlines provides clear visual separation between different types of messages.
38-44: Verify the date format display
The implementation looks good, but we should verify the date format being displayed. The current code uses the raw t.date value without any formatting.
Let's check the date format in the codebase:
Consider using a consistent date formatting function if one exists in the codebase, to maintain uniformity across the application.
✅ Verification successful
Let me search for the specific date format being used in the file's context.
Let me check how the date format is handled in the server-side handler that provides these transactions.
Let me check one final thing to see how the transaction dates are stored and handled in the models.
Let me check the transaction model definition specifically.
Raw date format is appropriate in this context
The current implementation using raw t.date is actually correct because:
- The
TransactionEntitytype shows thatdateis stored as a string in a standardized format - This is an internal diagnostic tool showing dates of transactions that need manual fixing
- The raw format ensures consistency with the database's stored format, which is crucial for manual investigation and fixing of problematic transactions
- The dates will be in ISO format (YYYY-MM-DD) as that's the standard storage format in the database, making it unambiguous for users to locate these transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for date formatting patterns in the codebase
rg -A 2 "\.date.*format|formatDate"
Length of output: 8737
Script:
#!/bin/bash
# Search for date format usage in the FixSplits component and its imports
rg -l "FixSplits" | xargs rg "dateFormat|format.*date"
# Also check the specific file for any date-related imports
cat packages/desktop-client/src/components/settings/FixSplits.tsx
Length of output: 3853
Script:
#!/bin/bash
# Check the server-side handler implementation
ast-grep --pattern 'tools/fix-split-transactions'
# Also check the type definition of the transaction object
rg -A 5 "type.*Transaction" packages/loot-core/src/types/
Length of output: 3435
Script:
#!/bin/bash
# Check the transaction model definition
cat packages/loot-core/src/types/models/index.d.ts
# Also check the fix-split-transactions handler implementation
rg -A 10 "tools/fix-split-transactions" packages/loot-core/src/
Length of output: 2136
Script:
#!/bin/bash
# Check the transaction model definition
cat packages/loot-core/src/types/models/transaction.d.ts
# Also check if there's a common date format utility
rg -A 3 "export.*dateFormat" packages/loot-core/src/
Length of output: 1603
matt-fidd
force-pushed
the
dodgy-splits
branch
from
d681a73 to
9b12556
Compare
No description provided.