feat: add scoped import filters #227
Conversation
- Add bash language identifier to code block in README.md - Fix Prettier formatting issues in src/utils/scope.ts
- Remove misleading 'Parsed account mapping' log from AccountMap.loadFromConfig() - Add accurate 'Filtered account mapping' log in Importer showing only processed accounts - Add early validation to skip transaction processing when no valid account mappings exist - Remove problematic account filtering logic from scope mechanism (handled by Importer) - Improve user experience with clear warnings for invalid account filters - Optimize performance by avoiding unnecessary API calls for invalid filters
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduces repeatable, case-insensitive CLI flags (--server, --budget, --account), adds a Scope type and applyScope(config, scope) to filter servers/budgets/accounts, updates import command to load full config then apply scope, adjusts mapping logs, and early-exits when scope or account mappings yield no work. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as ImportCommand
participant Loader as ConfigLoader
participant Scope as scope.applyScope
participant AM as AccountMap.getMap
participant Importer as Importer
U->>CLI: run import --server... --budget... --account...
CLI->>Loader: load full Config
Loader-->>CLI: Config
CLI->>Scope: applyScope(Config, Scope{servers,budgets,accounts})
Scope-->>CLI: Scoped Config (servers -> budgets -> account mappings)
alt no servers after scope
CLI-->>U: log "no servers after scope" & exit
else servers present
CLI->>AM: getMap(scopedConfig, accountRefs)
AM-->>CLI: accountMapping (logged with strike-through for filtered refs)
alt accountRefs provided AND mapping empty
CLI-->>U: warn "no matching accounts" & skip import
else
CLI->>Importer: import(scopedConfig, dates, accountMapping)
Importer-->>U: import logs/summary
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/Importer.ts (1)
207-218: Bug: async predicate in Array.filter causes no filtering.Array.filter ignores the resolved value of Promises; returning a Promise keeps all items. Make the predicate synchronous.
- createTransactions = createTransactions.filter( - async (transaction) => { - const transactionExists = existingActualTransactions.some( - (existingTransaction) => - existingTransaction.imported_id === - transaction.imported_id - ); - - return !transactionExists; - } - ); + createTransactions = createTransactions.filter((transaction) => { + const transactionExists = existingActualTransactions.some( + (existingTransaction) => + existingTransaction.imported_id === transaction.imported_id + ); + return !transactionExists; + });src/commands/import.command.ts (1)
129-141: Typo: 'to' description bound to 'from'.The second describe should target 'to', not 'from'.
- .string('to') - .describe( - 'from', - `Import transactions up to this date (${DATE_FORMAT})` - ); + .string('to') + .describe( + 'to', + `Import transactions up to this date (${DATE_FORMAT})` + );
🧹 Nitpick comments (7)
src/index.ts (2)
39-56: Clarify option descriptions to reflect accepted identifiers.Align help text with actual matching (server URL/name; budget syncId/name/id; account refs).
- description: 'Limit to server name(s)', + description: 'Limit to server name(s) or URL(s)', ... - description: 'Limit to budget id/name(s)', + description: 'Limit to budget syncId/name/id(s)', ... - description: 'Limit to account name(s)', + description: 'Limit to account reference(s): name, number/IBAN, or UUID',
57-65: Prefer option-level coercion over global middleware.Use yargs .coerce('server'|'budget'|'account', splitArgs) to avoid mutating argv globally and keep typing localized. Middleware works, but coerce is simpler and more explicit.
Example:
.option('server', { /* ... */ }) .coerce('server', splitArgs) .option('budget', { /* ... */ }) .coerce('budget', splitArgs) .option('account', { /* ... */ }) .coerce('account', splitArgs)README.md (1)
80-97: Add a brief note on what each flag matches.Suggest appending a one-liner: server matches name or URL; budget matches syncId/name/id; account matches name/number(IBAN)/UUID. Helps users craft filters correctly.
src/utils/Importer.ts (1)
139-149: Empty mapping with no filters silently does nothing — log and return.If mapping is empty and no accountRefs were provided, consider logging and exiting early to avoid a silent no-op.
if (accountMapping.size === 0) { if (accountRefs && accountRefs.length > 0) { this.logger.warn( 'No valid account mappings found for the specified account filters. Skipping transaction processing.' ); return; } - // If no account filters specified, continue with all accounts + // No account filters specified and no mappings configured + this.logger.info('No account mappings configured for this budget. Nothing to import.'); + return; }src/commands/import.command.ts (2)
25-28: Fix grammar in error message.- 'No Actual servers configured. Refer to the docs on how to a new server with in the configuration file.' + 'No Actual servers configured. Refer to the docs on how to add a new server to the configuration file.'
11-11: Use type-only import for Scope (minor).Avoids emitting a runtime import for a type.
-import { applyScope, Scope } from '../utils/scope.js'; +import { applyScope } from '../utils/scope.js'; +import type { Scope } from '../utils/scope.js';src/utils/scope.ts (1)
9-20: Defensively trim keys before lowercasing.Handles stray whitespace even if upstream trimming changes.
-const toKey = (value: string) => value.toLowerCase(); +const toKey = (value: string) => value.trim().toLowerCase();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)src/commands/import.command.ts(1 hunks)src/index.ts(2 hunks)src/utils/AccountMap.ts(0 hunks)src/utils/Importer.ts(1 hunks)src/utils/scope.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/utils/AccountMap.ts
🔇 Additional comments (5)
src/index.ts (1)
17-28: Robust arg normalization (comma/whitespace/empties). LGTM.src/utils/Importer.ts (1)
151-159: Good: emits visible, scoped mapping preview.src/commands/import.command.ts (2)
38-44: Scope summary logging is clear and helpful.
56-56: Passing scoped account refs downstream — looks right.src/utils/scope.ts (1)
43-73: Scope application logic is clean and composable. LGTM.
NikxDa
left a comment
NikxDa
left a comment
There was a problem hiding this comment.
Hey Jens, I've added some comments to this that I hope will help you get an understanding of what this feature needs to be. Happy to guide you a bit more if you have any questions :)
|
Interestingly, CodeRabbit didn't pick up on the logical flaws of the configuration types, but it did find some smaller nitpicks. I do think it adds value, but it cannot be relied on fully. |
- Move server, budget, and account options from global to import command only - Remove comma-separated argument splitting to prevent issues with account names containing commas - Update README examples to use repeated flags instead of comma separation - Add early exit when no valid account mappings are found - Improve account mapping logging to show all mappings with filtered ones indicated - Simplify scope filtering logic with linear approach instead of complex matcher system - Remove type additions that would be stripped by validation - Use array type coercion for CLI options to ensure proper type handling
- Remove unused ActualBudgetConfig import from scope.ts - Apply consistent code formatting across modified files
|
@CodeRabbit fullreview |
|
@1cu: I'll perform a full review of the PR. I'll pay special attention to the configuration types and logic, as mentioned in the earlier feedback about logical flaws that may have been missed. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)src/commands/import.command.ts(2 hunks)src/utils/AccountMap.ts(2 hunks)src/utils/Importer.ts(1 hunks)src/utils/scope.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/commands/import.command.ts (3)
src/utils/scope.ts (2)
Scope(3-7)applyScope(20-47)src/utils/config.ts (1)
getConfig(100-133)src/utils/shared.ts (1)
DATE_FORMAT(4-4)
src/utils/scope.ts (1)
src/utils/config.ts (2)
Config(89-89)ActualServerConfig(87-87)
🪛 GitHub Actions: ci
src/utils/scope.ts
[error] 1-1: ESLint: 'ActualBudgetConfig' is defined but never used. (no-unused-vars)
🔇 Additional comments (8)
src/utils/Importer.ts (1)
139-151: LGTM!The early exit logic correctly handles the case when no valid account mappings are found for specified filters, preventing unnecessary processing. The delegation of account mapping logging to
AccountMap.getMap()improves separation of concerns.src/utils/AccountMap.ts (1)
30-46: LGTM!The filtered account logging with strikethrough notation effectively addresses the previous review feedback. It shows all account mappings while visually distinguishing which accounts are filtered out, providing clear visibility into what will be processed.
src/commands/import.command.ts (5)
17-21: LGTM!The scope construction from CLI arguments is clean and straightforward, properly handling optional filter arrays.
23-36: LGTM!The two-phase approach (load full config, then apply scope) with early exit when no servers remain after filtering is the correct pattern. This ensures that invalid filters are caught early without attempting unnecessary operations.
38-43: LGTM!The scope summary provides clear visibility into which filters are active, using "ALL" as a sensible default when no filter is specified. This helps users understand what will be processed.
45-47: LGTM!Creating
PayeeTransformerafter applying the scope is correct, as the scoped config should be used for all downstream operations. This ensures the transformer only operates on the filtered configuration.
142-159: LGTM!The CLI option definitions correctly use
array: trueand provide clear descriptions. The use of aliases (-s,-b,-a) follows CLI conventions for shorter alternatives.src/utils/scope.ts (1)
3-7: Drop schema validation suggestion forScope—it isn’t part ofconfigSchema.
Scoperepresents CLI-provided filters applied at runtime, not fields parsed from the config file via Zod, so no schema updates are needed.Likely an incorrect or invalid review comment.
| type Account = { | ||
| id: string; | ||
| name: string; | ||
| type: string; | ||
| offbudget: boolean; | ||
| closed: boolean; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify Account type is not duplicating existing types.
This internal Account type may duplicate types already defined in ActualApi or the Actual Budget SDK. Reusing existing types ensures consistency and reduces maintenance burden.
Check if this type already exists:
🏁 Script executed:
#!/bin/bash
# Search for Account type definitions in ActualApi and related files
rg -n "type Account = " --type=ts
# Check what ActualApi returns from getAccounts
ast-grep --pattern $'getAccounts(): $_ {
$$$
}'
# Also search for Account interface definitions
rg -n "interface Account" --type=ts -C 3Length of output: 94
Reuse existing Account type instead of redefining
The Account type is already defined in src/types/actual-app__api.d.ts (line 289). Remove the inline definition in src/utils/AccountMap.ts and import it to maintain a single source of truth.
🤖 Prompt for AI Agents
In src/utils/AccountMap.ts around lines 6 to 12, remove the inline Account type
definition and import the existing Account type from
src/types/actual-app__api.d.ts instead; update the top of the file to add the
import (e.g. import { Account } from 'src/types/actual-app__api' or the correct
relative path) and ensure any references use that imported type so there is a
single source of truth.
There was a problem hiding this comment.
@NikxDa - I discovered an issue while implementing the CodeRabbit suggestion to fix the Account type duplication.
Problem: The build is now failing with duplicate CreateTransaction type definitions in src/types/actual-app__api.d.ts:
- Line 266:
type CreateTransaction = Modify<BaseTransaction, ...> - Line 339:
type CreateTransaction = Pick<BaseTransaction, 'account' | 'date'> & ...
Root Cause: The duplicate definitions have been there since commit f57ea7a (May 2023), but they were hidden by skipLibCheck: true in tsconfig.json. When I added export to the Account type to fix the CodeRabbit suggestion, it changed how TypeScript processes the types file, revealing this latent issue.
Impact:
- Build was working before our changes
- Adding
export type Account = {made TypeScript process the file more strictly - Now the duplicate
CreateTransactiondefinitions are caught and causing build failures
Options:
- Remove the duplicate
CreateTransactionat line 339 (recommended) - Revert the Account type changes and keep the inline definition
- Find a different approach that doesn't change the module structure
Question: Did I get this right? Should I fix the duplicate definition, or would you prefer a different approach for the Account type issue?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/utils/scope.ts (2)
9-22: Handle empty and whitespace-only strings in filter matching.The function doesn't filter out empty or whitespace-only strings, which can cause false matches when comparing empty filter values against empty candidate values.
Apply this diff:
const matchesFilter = ( candidates: Array<string | undefined>, filterValues?: Array<string> ) => { if (!filterValues || filterValues.length === 0) { return true; } - const normalizedFilter = filterValues.map((v) => v.toLowerCase()); + const normalizedFilter = filterValues + .filter((v) => v.trim().length > 0) + .map((v) => v.toLowerCase()); return candidates.some( (candidate) => - candidate && normalizedFilter.includes(candidate.toLowerCase()) + candidate && + candidate.trim().length > 0 && + normalizedFilter.includes(candidate.toLowerCase()) ); };
24-51: Implement missing account filtering.The
Scopetype includes anaccountsfield (Line 6), and the PR objectives state that the--accountflag should filter by account references. However,applyScopenever usesscope.accountsto filter account mappings within budgets.After filtering budgets by
syncId(lines 31-34), add logic to filter eachbudget.accountMappingso only entries whose keys or values matchscope.accountsare retained. Remove budgets that end up with emptyaccountMapping:.map((server) => { const budgets = server.budgets.filter((budget) => { // Simple budget filtering by syncId return matchesFilter([budget.syncId], scope.budgets); - }); + }) + .map((budget) => { + // Filter account mappings if scope.accounts is provided + if (!scope.accounts || scope.accounts.length === 0) { + return budget; + } + + const filteredMapping: Record<string, string> = {}; + for (const [moneyMoneyRef, actualRef] of Object.entries(budget.accountMapping)) { + // Check if either key or value matches the account filters + if ( + matchesFilter([moneyMoneyRef, actualRef], scope.accounts) + ) { + filteredMapping[moneyMoneyRef] = actualRef; + } + } + + return { + ...budget, + accountMapping: filteredMapping, + }; + }) + .filter((budget) => Object.keys(budget.accountMapping).length > 0); if (budgets.length === 0) { return null; }
🧹 Nitpick comments (1)
src/utils/AccountMap.ts (1)
6-12: Reuse existing Account type instead of redefining.The
Accounttype is already defined in the Actual Budget SDK types. Redefining it here creates maintenance overhead and risks type drift.Based on past review comments, the Account type exists in
src/types/actual-app__api.d.ts. Import and reuse that type:import { Account as MonMonAccount, getAccounts } from 'moneymoney'; import ActualApi from './ActualApi.js'; import { ActualBudgetConfig } from './config.js'; import Logger from './Logger.js'; +import { Account } from '../types/actual-app__api.js'; -type Account = { - id: string; - name: string; - type: string; - offbudget: boolean; - closed: boolean; -}; - export class AccountMap {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/AccountMap.ts(2 hunks)src/utils/scope.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/scope.ts (1)
src/utils/config.ts (2)
Config(89-89)ActualServerConfig(87-87)
🔇 Additional comments (1)
src/utils/AccountMap.ts (1)
26-75: LGTM! Clear logging of filtered vs. included accounts.The logging approach with strikethrough effectively communicates which accounts are filtered out versus processed. The case-insensitive matching and handling of optional fields (like
accountNumber) are correctly implemented.
- Add account filtering logic to scope.ts to filter budget.accountMapping based on scope.accounts - Handle empty strings in filter values to prevent false matches - Remove budgets with empty account mappings after filtering - Add comment to Account type in AccountMap.ts explaining its relationship to actual-app__api.d.ts - Improve code formatting for better readability Addresses CodeRabbit comments about missing account filtering logic and empty string handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/AccountMap.ts (1)
6-14: Document the reason for type duplication.The comment mentions this matches the type from
actual-app__api.d.ts, but doesn't explain why it's duplicated locally. Based on past discussions, importing from the types file causes build failures due to duplicateCreateTransactiondefinitions. Consider updating the comment to clarify this is a workaround.-// Account type matching the structure from @actual-app/api -// This matches the Account type defined in src/types/actual-app__api.d.ts +// Local Account type definition to avoid build failures. +// Duplicates the Account type from src/types/actual-app__api.d.ts because +// importing it directly triggers TypeScript to process that file more strictly, +// exposing duplicate CreateTransaction definitions that cause build failures. type Account = { id: string; name: string; type: string; offbudget: boolean; closed: boolean; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/AccountMap.ts(2 hunks)src/utils/scope.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/scope.ts (1)
src/utils/config.ts (2)
Config(89-89)ActualServerConfig(87-87)
🔇 Additional comments (3)
src/utils/AccountMap.ts (1)
28-77: LGTM! Strike-through approach addresses previous feedback.The filtered account logging with strike-through formatting provides clear visibility into which accounts are being processed versus skipped, addressing the concern raised in past reviews about removing the account mapping log entirely.
src/utils/scope.ts (2)
1-26: LGTM! Edge cases properly handled.The
matchesFilterhelper correctly handles empty filter values and empty candidates, addressing edge cases identified in past reviews. The case-insensitive matching implementation is sound.
28-86: LGTM! Account filtering now complete.The
applyScopefunction correctly implements the three-level filtering hierarchy (servers → budgets → accounts). The account filtering logic (lines 40-64) addresses the concern raised in past reviews about missing account filtering support.The implementation properly:
- Filters servers by URL (ActualServerConfig has no name field, so URL-only filtering is correct)
- Filters budgets by syncId
- Filters account mappings by checking both MoneyMoney references and Actual account names
- Cleans up empty budgets and servers
2 participants
PR Summary: Add Scope Filtering for Import Operations
Overview
This PR adds scope filtering capabilities to the
actual-monmonCLI tool, allowing users to selectively import transactions by server, budget, and account criteria.Changes
New Features
--server/-sto filter by server URL or name--budget/-bto filter by budget syncId, name, or ID--account/-ato filter by account referencesTechnical Changes
splitArgs()functionsrc/utils/scope.tsTest Cases
Summary by CodeRabbit
New Features
Documentation