save csv inOutMode settings #3543
Conversation
WalkthroughThe pull request introduces changes to 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 (
|
✅ 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 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
|
![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
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/modals/ImportTransactionsModal.jsx (1)
877-883: LGTM! Consider adding a default value forinOutMode.The changes look good and align with the PR objectives. The new state variables
inOutModeandoutValueare correctly initialized using user preferences.A minor suggestion:
- Consider adding a default value for
inOutModein caseprefs[\csv-in-out-mode-${accountId}`]` is undefined. This would ensure a consistent initial state.Example:
const [inOutMode, setInOutMode] = useState( String(prefs[`csv-in-out-mode-${accountId}`] ?? true) !== 'false' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/modals/ImportTransactionsModal.jsx (2 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
🔇 Additional comments (3)
packages/loot-core/src/types/prefs.d.ts (1)
Line range hint
1-67: Summary: Changes look good and address the PR objectivesThe additions to the
SyncedPrefstype in this file are appropriate and directly address the PR objectives of remembering the "Select column to specify if amount goes in/out" setting for CSV imports. The implementation is consistent with the existing preference structure and naming conventions.Remember to update any relevant documentation or user guides to reflect these new preferences and their functionality.
packages/desktop-client/src/components/modals/ImportTransactionsModal.jsx (2)
1352-1353: LGTM! Preferences are correctly saved.These changes ensure that the user's preferences for
inOutModeandoutValueare saved, which is consistent with the behavior of other import settings and aligns with the PR objectives.
877-883: Summary: Implementation successfully addresses the PR objectives.The changes effectively implement the "Select column to specify if amount goes in/out" feature:
- New state variables
inOutModeandoutValueare introduced and initialized with user preferences.- These preferences are correctly saved when importing transactions.
These modifications ensure that the setting is remembered between imports, aligning with the behavior of other import settings and addressing the issue described in #3542.
The implementation is consistent with the existing code style and patterns. Great job on improving the user experience for CSV imports!
Also applies to: 1352-1353
matt-fidd
force-pushed
the
save-inout-mode
branch
from
a423070
to
c420fad
Compare
matt-fidd
force-pushed
the
save-inout-mode
branch
from
c420fad
to
f0194e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (2)
645-646: LGTM: Saving new CSV import preferences.The addition of saving preferences for
csv-in-out-modeandcsv-out-valueis correct and aligns with the PR objectives. This ensures that the user's choices for these settings are remembered for future imports.For consistency with other preference saving lines, consider using template literals:
-savePrefs({ [`csv-in-out-mode-${accountId}`]: String(inOutMode) }); -savePrefs({ [`csv-out-value-${accountId}`]: String(outValue) }); +savePrefs({ [`csv-in-out-mode-${accountId}`]: `${inOutMode}` }); +savePrefs({ [`csv-out-value-${accountId}`]: `${outValue}` });This change makes the code more consistent with other preference saving lines in the file.
Issues Found: Inconsistent Usage of
inOutModeandoutValueThe verification script identified instances where
splitModeis used without the accompanyinginOutModeandoutValueparameters. This inconsistency may lead to inaccurate import previews.
packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx
- Line: const isSplit = !splitMode;
packages/desktop-client/src/components/modals/ImportTransactionsModal/FieldMappings.jsx
- Line: splitMode,
packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.js
- Lines:
- splitMode,
- if (splitMode) {
packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.jsx
- Line: splitMode,
Please update these instances to include
inOutModeandoutValuewhere necessary to ensure consistency across the codebase.🔗 Analysis chain
Line range hint
285-286: LGTM: Updated getImportPreview function.The addition of
inOutModeandoutValueparameters to thegetImportPreviewfunction and their subsequent use inparseAmountFieldsensures that the import preview accurately reflects the user's chosen CSV import settings.To ensure that these changes are correctly propagated throughout the codebase, please run the following verification script:
This script will help identify any instances where
inOutModeandoutValuemight need to be added for consistency.Also applies to: 287-288
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that inOutMode and outValue are consistently used in relevant functions # Check for usage in getImportPreview and parseAmountFields echo "Checking usage in getImportPreview and parseAmountFields:" rg --type javascript 'function (getImportPreview|parseAmountFields).*\{' -A 10 | rg '(inOutMode|outValue)' # Check for any missed occurrences where these parameters should be added echo "\nChecking for potential missed occurrences:" rg --type javascript 'function .*\{' -A 10 | rg 'splitMode' | rg -v '(inOutMode|outValue)'Length of output: 493
Script:
#!/bin/bash # Description: Verify that inOutMode and outValue are consistently used in relevant functions # Check for usage in getImportPreview and parseAmountFields echo "Checking usage in getImportPreview and parseAmountFields:" rg --type js 'function (getImportPreview|parseAmountFields)\s*\(' -A 10 | rg '(inOutMode|outValue)' # Check for any missed occurrences where these parameters should be added echo "\nChecking for potential missed occurrences:" rg --type js 'function\s+\w+\s*\(' -A 10 | rg 'splitMode' | rg -v '(inOutMode|outValue)'Length of output: 1117
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (2 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/types/prefs.d.ts
🧰 Additional context used
🔇 Additional comments (2)
packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (2)
171-176: LGTM: New state variables for CSV import settings.The addition of
inOutModeandoutValuestate variables, initialized fromprefs, aligns well with the PR objective of remembering the "Select column to specify if amount goes in/out" setting. This change will help maintain consistency in the user's import preferences across sessions.
Line range hint
1-1068: Overall assessment: Changes look good and address the PR objectives.The implemented changes effectively address the issue of remembering the "Select column to specify if amount goes in/out" setting for CSV imports. The new state variables, preference saving, and updates to the import preview function are well-integrated into the existing codebase.
A few minor suggestions were made for consistency and verification, but overall, the changes are approved and ready for merging, pending the results of the verification script.
matt-fidd
changed the title
Fixes #3542