feat: multi-broker architecture with Degiro CSV parser#12
Merged
Conversation
- Merge auto-tag + release into one workflow (like CashPilot/LynxPrompt) - Tag, sync package.json, generate changelog, create release — all in one job - Uses only GITHUB_TOKEN (no PAT secret required) - CLI reads version from package.json (single source of truth) - Update ROADMAP to reflect v0.2.0 state
- Add BrokerParser interface (detect + parse → Statement) - Add parser registry with auto-detection (detectBroker, getBroker) - Wrap IBKR parser with BrokerParser interface - Implement Degiro parser: Transactions CSV (trades) + Account CSV (dividends/withholdings), multi-language (ES/EN/NL/DE), auto-detect delimiter (comma/semicolon), EU number format support - Update CLI: add --broker flag, auto-detect broker from file content - Update ROADMAP with broker tax integration research findings: Degiro/Scalable (high priority, no AEAT integration) vs Trade Republic/XTB (low priority, already report to AEAT) - 99 tests passing (24 new: 18 Degiro parser, 6 registry)
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a broker-agnostic parsing layer with a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli/index.ts (1)
156-186:⚠️ Potential issue | 🟠 MajorDon't treat broker files without holdings data as “no Modelo 720”.
This path now accepts any detected broker file, but the new Degiro support in this PR only parses transactions/account movements. Those inputs can reach the
openPositions = []branch and print “No es necesario presentar Modelo 720”, which is a false negative for users who still hold positions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/index.ts` around lines 156 - 186, The code assumes an empty statement.openPositions means "no Modelo 720" which is wrong for parsers that only produce transactions (e.g., the new Degiro parser); after parser.parse(...) check for parsers/outputs that indicate transactions-only (e.g., statement.transactions?.length > 0 or parser.name === "degiro" or a parser.capabilities flag) and treat an empty openPositions as "unknown" rather than "below threshold": instead of directly printing "Posiciones... No es necesario...", surface a clear warning/error asking the user to provide holdings data or run a balance-reconstruction path (or exit non-zero), so update the logic around statement.openPositions and the generateModelo720 call to skip the false-negative path when transactions exist but openPositions is empty..github/workflows/release.yml (1)
91-91:⚠️ Potential issue | 🟡 MinorGit log filtering logic won't exclude both
featandfixcommits.Multiple
--invert-grep --greppairs don't stack. The current command won't properly filter out both feat and fix commits from "Other Changes".🔧 Fix using extended regex
- OTHERS=$(git log $PREV_TAG..$VERSION --pretty=format:"- %s" --invert-grep --grep="^feat" --invert-grep --grep="^fix" 2>/dev/null | head -10 || true) + OTHERS=$(git log $PREV_TAG..$VERSION --pretty=format:"- %s" --invert-grep --extended-regexp --grep="^(feat|fix)" 2>/dev/null | head -10 || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml at line 91, The git log command assigned to OTHERS won't exclude both feat and fix because multiple --invert-grep/--grep pairs don't combine; update the OTHERS assignment to use a single --invert-grep with an alternation regex (e.g., --invert-grep --grep="^(feat|fix)" or the POSIX equivalent) so both commit types are filtered out from PREV_TAG..VERSION; modify the line that defines OTHERS (the git log invocation) to use that single combined grep pattern.
🧹 Nitpick comments (3)
tests/parsers/degiro.test.ts (1)
46-71: Detection tests look solid.Consider adding a detection test for semicolon-delimited CSV (
TRANSACTIONS_CSV_SEMICOLON) to ensure delimiter doesn't affect header recognition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/parsers/degiro.test.ts` around lines 46 - 71, Add a test case to the degiroParser.detect suite that verifies semicolon-delimited CSV is recognized: call degiroParser.detect with the TRANSACTIONS_CSV_SEMICOLON fixture and expect true, similar to the existing TRANSACTIONS_CSV_ES/EN tests; place it alongside the other detect tests to ensure delimiter variations don't break header recognition..github/workflows/release.yml (1)
27-43: Patch version extraction assumes well-formed semver.If an existing tag lacks a patch component (e.g.,
v1.0),cut -d. -f3returns empty and$((PATCH + 1))fails. This is unlikely with thev0.0.0default, but worth noting.🛡️ Optional: Add fallback for empty PATCH
PATCH=$(echo "$VERSION" | cut -d. -f3) + PATCH=${PATCH:-0} NEW_PATCH=$((PATCH + 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 27 - 43, The current tag parsing (variables VERSION, PATCH, NEW_PATCH) can fail if the tag lacks a third component; modify the extraction so PATCH defaults to 0 when empty/non-numeric (e.g., use parameter expansion or a numeric fallback for PATCH after VERSION is split), then compute NEW_PATCH as PATCH+1 and form NEW_TAG as before; update the logic around VERSION, MAJOR, MINOR, PATCH, NEW_PATCH and NEW_TAG to ensure PATCH is set to 0 if cut -f3 yields empty or non-numeric input (refer to variables VERSION, PATCH, NEW_PATCH, NEW_TAG, LATEST).src/parsers/degiro.ts (1)
237-239:parseFloatused for sign determination.This is only for comparing signs to determine buy/sell direction, not for financial calculations. The monetary values remain as strings. Acceptable, but if extreme precision is needed, consider using
Decimal.jshere too. As per coding guidelines: "Check that all string-to-number conversions go through Decimal.js."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/degiro.ts` around lines 237 - 239, The code uses parseFloat on value and quantity to set isSell (valueNum and qtyNum); per guidelines, all string-to-number conversions must go through Decimal.js. Replace parseFloat usage with Decimal (e.g., new Decimal(value).gt(0) / new Decimal(quantity).lt(0) or Decimal(value).isPositive()/isNegative()) to compute isSell, and add the necessary Decimal import/require if missing; leave the original money strings untouched for arithmetic elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 45-62: The workflow currently runs the "Create tag" step (git tag
-a "${{ steps.version.outputs.new_tag }}") before the "Sync package.json
version" step which can leave an annotated tag pointing to a commit that doesn't
include the updated package.json; move the package.json sync logic so the "Sync
package.json version" step runs before the "Create tag" step, ensure npm version
"$VERSION" --no-git-tag-version, git add/commit/push happen first, and then
create and push the annotated tag using the same ${{
steps.version.outputs.new_tag }} so the tag and committed package.json are in
sync (update any step names or ordering referencing "Create tag" and "Sync
package.json version" accordingly).
In `@src/cli/index.ts`:
- Around line 190-191: The current write to opts.output uses writeFileSync(...,
{ encoding: "latin1" }) which yields ISO-8859-1 bytes; instead install and
import iconv-lite and transcode the output720 buffer/string to ISO-8859-15 (aka
"ISO-8859-15" or "latin9") before writing. Locate the write in src/cli/index.ts
where output720 is written and replace the encoding option with writing the
result of iconv.encode(output720, "ISO-8859-15") (or equivalent API), ensuring
the output is written as a Buffer so the resulting file conforms to Modelo 720
encoding requirements.
In `@src/parsers/degiro.ts`:
- Around line 11-12: Update the top docstring to remove the outdated claim that
"Account CSV support is planned for a future version" and instead reflect that
account CSV parsing is implemented; specifically edit the parser description
that currently references Transactions-only to mention support for both
Transactions CSV and Account CSV (see parseAccountCsv function) or remove the
future-planned sentence entirely so the docstring accurately matches the
implemented parseAccountCsv behavior.
In `@src/parsers/index.ts`:
- Around line 27-29: The getBroker function currently does substring matching on
brokerParsers even when name is empty; update getBroker to first reject empty or
whitespace-only names (e.g., if (!name || name.trim().length === 0) return
undefined) before computing lower and performing brokerParsers.find((p) =>
p.name.toLowerCase().includes(lower)), so that getBroker("") returns undefined
rather than the first parser.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Line 91: The git log command assigned to OTHERS won't exclude both feat and
fix because multiple --invert-grep/--grep pairs don't combine; update the OTHERS
assignment to use a single --invert-grep with an alternation regex (e.g.,
--invert-grep --grep="^(feat|fix)" or the POSIX equivalent) so both commit types
are filtered out from PREV_TAG..VERSION; modify the line that defines OTHERS
(the git log invocation) to use that single combined grep pattern.
In `@src/cli/index.ts`:
- Around line 156-186: The code assumes an empty statement.openPositions means
"no Modelo 720" which is wrong for parsers that only produce transactions (e.g.,
the new Degiro parser); after parser.parse(...) check for parsers/outputs that
indicate transactions-only (e.g., statement.transactions?.length > 0 or
parser.name === "degiro" or a parser.capabilities flag) and treat an empty
openPositions as "unknown" rather than "below threshold": instead of directly
printing "Posiciones... No es necesario...", surface a clear warning/error
asking the user to provide holdings data or run a balance-reconstruction path
(or exit non-zero), so update the logic around statement.openPositions and the
generateModelo720 call to skip the false-negative path when transactions exist
but openPositions is empty.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 27-43: The current tag parsing (variables VERSION, PATCH,
NEW_PATCH) can fail if the tag lacks a third component; modify the extraction so
PATCH defaults to 0 when empty/non-numeric (e.g., use parameter expansion or a
numeric fallback for PATCH after VERSION is split), then compute NEW_PATCH as
PATCH+1 and form NEW_TAG as before; update the logic around VERSION, MAJOR,
MINOR, PATCH, NEW_PATCH and NEW_TAG to ensure PATCH is set to 0 if cut -f3
yields empty or non-numeric input (refer to variables VERSION, PATCH, NEW_PATCH,
NEW_TAG, LATEST).
In `@src/parsers/degiro.ts`:
- Around line 237-239: The code uses parseFloat on value and quantity to set
isSell (valueNum and qtyNum); per guidelines, all string-to-number conversions
must go through Decimal.js. Replace parseFloat usage with Decimal (e.g., new
Decimal(value).gt(0) / new Decimal(quantity).lt(0) or
Decimal(value).isPositive()/isNegative()) to compute isSell, and add the
necessary Decimal import/require if missing; leave the original money strings
untouched for arithmetic elsewhere.
In `@tests/parsers/degiro.test.ts`:
- Around line 46-71: Add a test case to the degiroParser.detect suite that
verifies semicolon-delimited CSV is recognized: call degiroParser.detect with
the TRANSACTIONS_CSV_SEMICOLON fixture and expect true, similar to the existing
TRANSACTIONS_CSV_ES/EN tests; place it alongside the other detect tests to
ensure delimiter variations don't break header recognition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f4811c0-f992-4ce5-a8c7-8f73329eaa03
📒 Files selected for processing (12)
.github/workflows/auto-tag.yml.github/workflows/release.ymlROADMAP.mdsrc/cli/index.tssrc/index.tssrc/parsers/degiro.tssrc/parsers/ibkr.tssrc/parsers/index.tssrc/types/broker.tssrc/types/index.tstests/parsers/degiro.test.tstests/parsers/registry.test.ts
💤 Files with no reviewable changes (1)
- .github/workflows/auto-tag.yml
Comment on lines
+45
to
+62
| - name: Create tag | ||
| run: | | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git tag -a "${{ steps.version.outputs.new_tag }}" -m "Release ${{ steps.version.outputs.new_tag }}" | ||
| git push origin "${{ steps.version.outputs.new_tag }}" | ||
|
|
||
| - name: Sync package.json version | ||
| run: | | ||
| VERSION="${{ steps.version.outputs.new_tag }}" | ||
| VERSION="${VERSION#v}" | ||
| CURRENT=$(node -p "require('./package.json').version") | ||
| if [ "$CURRENT" != "$VERSION" ]; then | ||
| npm version "$VERSION" --no-git-tag-version | ||
| git add package.json package-lock.json | ||
| git commit -m "chore: bump version to ${{ steps.version.outputs.new_tag }} [skip ci]" | ||
| git push origin main | ||
| fi |
Contributor
There was a problem hiding this comment.
Tag is created before package.json is synced—consider reordering.
If the workflow fails after creating the tag (line 49-50) but before syncing package.json (line 52-62), you'll have a published tag with a stale package.json version. Users running declarenta --version would see the old version.
Consider: create the tag after package.json is updated and committed, so both are in sync or neither is published.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 45 - 62, The workflow currently
runs the "Create tag" step (git tag -a "${{ steps.version.outputs.new_tag }}")
before the "Sync package.json version" step which can leave an annotated tag
pointing to a commit that doesn't include the updated package.json; move the
package.json sync logic so the "Sync package.json version" step runs before the
"Create tag" step, ensure npm version "$VERSION" --no-git-tag-version, git
add/commit/push happen first, and then create and push the annotated tag using
the same ${{ steps.version.outputs.new_tag }} so the tag and committed
package.json are in sync (update any step names or ordering referencing "Create
tag" and "Sync package.json version" accordingly).
Comment on lines
190
to
+191
| if (opts.output) { | ||
| writeFileSync(opts.output, content, { encoding: "latin1" }); | ||
| writeFileSync(opts.output, output720, { encoding: "latin1" }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Node.js fs.writeFileSync support ISO-8859-15 directly, and is "latin1" equivalent to ISO-8859-1 rather than ISO-8859-15?
💡 Result:
No, Node.js fs.writeFileSync does not support ISO-8859-15 directly as a built-in encoding option. The fs module uses the same character encodings as the Buffer class, which explicitly supports 'latin1' as ISO-8859-1 (a single-byte mapping of Unicode code points U+0000 to U+00FF, truncating higher characters). Yes, "latin1" is equivalent to ISO-8859-1, not ISO-8859-15 (which differs in positions like the euro symbol and differs from ISO-8859-1 in byte mappings for some characters). Node.js official docs list supported encodings as 'utf8', 'utf16le', 'latin1' (ISO-8859-1), 'ascii', 'binary' (alias for latin1), 'base64', 'base64url', 'hex'. No mention of ISO-8859-15 in any official source. For ISO-8859-15, third-party libraries like iconv-lite are required.
Citations:
- 1: https://nodejs.org/api/buffer.html
- 2: https://stackoverflow.com/questions/14551608/list-of-encodings-that-node-js-supports
- 3: https://nodejs.org/docs/latest-v15.x/api/buffer.html
- 4: https://nodejs.org/api/buffer.md
- 5: https://nodejs.org/docs/latest-v18.x/api/buffer.html
🏁 Script executed:
# Read the file at the specified lines
head -n 195 src/cli/index.ts | tail -n 10Repository: GeiserX/DeclaRenta
Length of output: 419
🏁 Script executed:
# Check package.json for encoding-related dependencies
cat package.json | grep -A 20 '"dependencies"' | head -n 30Repository: GeiserX/DeclaRenta
Length of output: 451
🏁 Script executed:
# Search for ISO-8859-15 references in codebase
rg "8859-15|iso-8859-15|ISO-8859-15" -iRepository: GeiserX/DeclaRenta
Length of output: 696
🏁 Script executed:
# Search for iconv or encoding handling
rg "iconv|encode|transcode" -i --type ts | head -n 20Repository: GeiserX/DeclaRenta
Length of output: 44
🏁 Script executed:
# Look for Modelo 720 related code
rg "modelo.*720|720.*modelo" -i --type ts | head -n 20Repository: GeiserX/DeclaRenta
Length of output: 1296
Use ISO-8859-15 encoding for Modelo 720 output, not latin1.
Node.js's writeFileSync(..., { encoding: "latin1" }) uses ISO-8859-1 semantics, which produces incorrect byte sequences for Modelo 720 files that must comply with ISO-8859-15 encoding (as documented in src/generators/modelo720.ts comments and ROADMAP). Since Node.js doesn't natively support ISO-8859-15, add a library like iconv-lite to the dependencies and transcode the output before writing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/index.ts` around lines 190 - 191, The current write to opts.output
uses writeFileSync(..., { encoding: "latin1" }) which yields ISO-8859-1 bytes;
instead install and import iconv-lite and transcode the output720 buffer/string
to ISO-8859-15 (aka "ISO-8859-15" or "latin9") before writing. Locate the write
in src/cli/index.ts where output720 is written and replace the encoding option
with writing the result of iconv.encode(output720, "ISO-8859-15") (or equivalent
API), ensuring the output is written as a Buffer so the resulting file conforms
to Modelo 720 encoding requirements.
Comment on lines
+27
to
+29
| export function getBroker(name: string): BrokerParser | undefined { | ||
| const lower = name.toLowerCase(); | ||
| return brokerParsers.find((p) => p.name.toLowerCase().includes(lower)); |
Contributor
There was a problem hiding this comment.
Reject empty broker names before substring matching.
getBroker("") currently returns the first registered parser because every string includes "". That makes --broker "" resolve to a real parser instead of failing, and the partial-name lookup will get more ambiguous as the registry grows.
♻️ Minimal fix
export function getBroker(name: string): BrokerParser | undefined {
- const lower = name.toLowerCase();
- return brokerParsers.find((p) => p.name.toLowerCase().includes(lower));
+ const query = name.trim().toLowerCase();
+ if (!query) return undefined;
+ return (
+ brokerParsers.find((p) => p.name.toLowerCase() === query) ??
+ brokerParsers.find((p) => p.name.toLowerCase().includes(query))
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parsers/index.ts` around lines 27 - 29, The getBroker function currently
does substring matching on brokerParsers even when name is empty; update
getBroker to first reject empty or whitespace-only names (e.g., if (!name ||
name.trim().length === 0) return undefined) before computing lower and
performing brokerParsers.find((p) => p.name.toLowerCase().includes(lower)), so
that getBroker("") returns undefined rather than the first parser.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
==========================================
+ Coverage 56.97% 65.78% +8.81%
==========================================
Files 17 20 +3
Lines 1083 1438 +355
Branches 174 284 +110
==========================================
+ Hits 617 946 +329
- Misses 465 491 +26
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Handle real 19-column Transactions CSV format (Número, Bolsa de, Valor local + Valor + Tipo de cambio columns) - Add "número"/"number" to quantity header detection - Parse FX rate from CSV, default to "1" for EUR trades - Skip zero-price rows (rights assignments, non-tradeable conversions) - Add real Degiro CSV fixture from mxmhos/degiro (GitHub) - 7 new integration tests against real export data - 106 tests passing total
Handle dual Account CSV layouts: old format with "Importe" column vs real export with "Variación" header mapping to currency position. Add "Retención del dividendo" withholding pattern. Validate against real Degiro Account export fixture (4 dividends + 4 withholdings).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/parsers/degiro.ts`:
- Around line 335-379: The parser currently drops the "value date" / "Fecha
valor" column which causes parseAccountCsv() to set both booking and settlement
dates from the same "Fecha/Date" column; update the AccountColumns type to
include a separate settleDate (or valueDate) field, update
resolveAccountColumns() to locate the "Fecha valor"/"Value date" header (e.g.,
findColumn(headers, ["Fecha valor","Value date"]) and assign it to that new
field), and then modify parseAccountCsv() to use the new settleDate/valueDate
column when populating settleDate (leaving dateTime populated from the original
date column); ensure isDegiroAccount() still checks the new settleDate/valueDate
column as required.
- Around line 200-206: The parser currently extracts Degiro's broker-supplied FX
column into the fxRate variable via findColumn and assigns it directly to
trade.fxRateToBase and cashTransaction.fxRateToBase; instead stop populating
fxRateToBase from parsed FX data so downstream logic can rely on getEcbRate() to
supply the canonical rate. Remove or skip setting fxRateToBase where it's
assigned (the assignments around trade.fxRateToBase and
cashTransaction.fxRateToBase), keep the fxRate detection (findColumn) if needed
for diagnostics but do not persist its value into parsed trade or cash
transaction objects so getEcbRate() remains the authoritative source.
- Around line 89-110: The current parseNumber normalizes numeric strings but the
code later (checks that compare parsed strings to "0" or "0.0000" and subsequent
arithmetic) must use Decimal instead of string comparisons; import Decimal from
"decimal.js", convert parseNumber(...) results into new
Decimal(parseNumber(value)) before performing zero checks or math, replace
string equality checks like parsed === "0" or parsed === "0.0000" with
Decimal.equals(0) / decimal.isZero(), and ensure any arithmetic on values
previously done with string-to-number casting now uses Decimal methods so all
monetary calculations (those using parseNumber outputs and the logic around
them) follow the Decimal.js guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 231af07d-1b45-4850-81da-bd9f70ec1b98
⛔ Files ignored due to path filters (2)
tests/fixtures/degiro-account-real.csvis excluded by!**/*.csvtests/fixtures/degiro-transactions-real.csvis excluded by!**/*.csv
📒 Files selected for processing (2)
src/parsers/degiro.tstests/parsers/degiro.test.ts
Comment on lines
+89
to
+110
| function parseNumber(val: string): string { | ||
| const trimmed = val.trim(); | ||
| if (!trimmed) return "0"; | ||
|
|
||
| // If it has both dot and comma, the last one is the decimal separator | ||
| const lastDot = trimmed.lastIndexOf("."); | ||
| const lastComma = trimmed.lastIndexOf(","); | ||
|
|
||
| if (lastComma > lastDot) { | ||
| // EU format: dots are thousands, comma is decimal | ||
| return trimmed.replace(/\./g, "").replace(",", "."); | ||
| } | ||
| if (lastDot > lastComma && lastComma >= 0) { | ||
| // US/UK format: commas are thousands, dot is decimal | ||
| return trimmed.replace(/,/g, ""); | ||
| } | ||
| // Only one separator or none — comma might be decimal (no thousands) | ||
| if (lastComma >= 0 && lastDot < 0) { | ||
| return trimmed.replace(",", "."); | ||
| } | ||
| return trimmed; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/parsers/degiro.ts | head -150Repository: GeiserX/DeclaRenta
Length of output: 6053
🏁 Script executed:
cat -n src/parsers/degiro.ts | sed -n '280,295p'Repository: GeiserX/DeclaRenta
Length of output: 689
🏁 Script executed:
grep -n "import.*Decimal" src/parsers/degiro.tsRepository: GeiserX/DeclaRenta
Length of output: 44
🏁 Script executed:
rg "import.*Decimal" src/parsers/Repository: GeiserX/DeclaRenta
Length of output: 44
🏁 Script executed:
rg "Decimal" src/parsers/degiro.tsRepository: GeiserX/DeclaRenta
Length of output: 44
🏁 Script executed:
cat -n src/parsers/degiro.ts | sed -n '1,30p'Repository: GeiserX/DeclaRenta
Length of output: 1540
🏁 Script executed:
rg "decimal\.js" src/ --type tsRepository: GeiserX/DeclaRenta
Length of output: 566
🏁 Script executed:
rg "Decimal" src/ --type ts -lRepository: GeiserX/DeclaRenta
Length of output: 255
🏁 Script executed:
grep -n "parseFloat\|parseInt" src/parsers/*.ts | head -20Repository: GeiserX/DeclaRenta
Length of output: 194
🏁 Script executed:
cat -n src/parsers/degiro.ts | sed -n '280,310p'Repository: GeiserX/DeclaRenta
Length of output: 1350
Replace string comparisons with Decimal for zero-value filtering.
parseNumber() normalizes "0,00" and "0.00" to "0.00", but line 281 checks only for exact "0" and line 284 checks for "0" or "0.0000". This allows normalized zero values to leak through. Additionally, lines 287–289 violate the guideline requiring all string-to-number conversions use Decimal.js for monetary calculations.
Import Decimal and replace the zero/sign checks:
Diff
+import Decimal from "decimal.js";
import type { BrokerParser, Statement } from "../types/broker.js";
...
- if (!isin || quantity === "0") continue;
+ const quantityDec = new Decimal(quantity || "0");
+ const priceDec = new Decimal(price || "0");
+ const valueDec = new Decimal(value || "0");
+
+ if (!isin || quantityDec.isZero()) continue;
// Skip zero-price rows (rights assignments, non-tradeable conversions)
- if (price === "0" || price === "0.0000") continue;
+ if (priceDec.isZero()) continue;
// Determine buy/sell: negative local value = you paid = buy; or negative quantity = sell
- const valueNum = parseFloat(value);
- const qtyNum = parseFloat(quantity);
- const isSell = valueNum > 0 || qtyNum < 0;
+ const isSell = valueDec.gt(0) || quantityDec.lt(0);📝 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.
Suggested change
| function parseNumber(val: string): string { | |
| const trimmed = val.trim(); | |
| if (!trimmed) return "0"; | |
| // If it has both dot and comma, the last one is the decimal separator | |
| const lastDot = trimmed.lastIndexOf("."); | |
| const lastComma = trimmed.lastIndexOf(","); | |
| if (lastComma > lastDot) { | |
| // EU format: dots are thousands, comma is decimal | |
| return trimmed.replace(/\./g, "").replace(",", "."); | |
| } | |
| if (lastDot > lastComma && lastComma >= 0) { | |
| // US/UK format: commas are thousands, dot is decimal | |
| return trimmed.replace(/,/g, ""); | |
| } | |
| // Only one separator or none — comma might be decimal (no thousands) | |
| if (lastComma >= 0 && lastDot < 0) { | |
| return trimmed.replace(",", "."); | |
| } | |
| return trimmed; | |
| } | |
| import Decimal from "decimal.js"; | |
| import type { BrokerParser, Statement } from "../types/broker.js"; | |
| // ... existing code ... | |
| const quantityDec = new Decimal(quantity || "0"); | |
| const priceDec = new Decimal(price || "0"); | |
| const valueDec = new Decimal(value || "0"); | |
| if (!isin || quantityDec.isZero()) continue; | |
| // Skip zero-price rows (rights assignments, non-tradeable conversions) | |
| if (priceDec.isZero()) continue; | |
| // Determine buy/sell: negative local value = you paid = buy; or negative quantity = sell | |
| const isSell = valueDec.gt(0) || quantityDec.lt(0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parsers/degiro.ts` around lines 89 - 110, The current parseNumber
normalizes numeric strings but the code later (checks that compare parsed
strings to "0" or "0.0000" and subsequent arithmetic) must use Decimal instead
of string comparisons; import Decimal from "decimal.js", convert
parseNumber(...) results into new Decimal(parseNumber(value)) before performing
zero checks or math, replace string equality checks like parsed === "0" or
parsed === "0.0000" with Decimal.equals(0) / decimal.isZero(), and ensure any
arithmetic on values previously done with string-to-number casting now uses
Decimal methods so all monetary calculations (those using parseNumber outputs
and the logic around them) follow the Decimal.js guidelines.
Comment on lines
+200
to
+206
| // FX rate column (only in 19-col format) | ||
| const fxRate = findColumn(headers, [ | ||
| "Tipo de cambio", | ||
| "Exchange rate", | ||
| "Wechselkurs", | ||
| "Wisselkoers", | ||
| ]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and check the specific lines mentioned
wc -l src/parsers/degiro.tsRepository: GeiserX/DeclaRenta
Length of output: 87
🏁 Script executed:
# Examine the code around the specified line ranges
sed -n '195,210p' src/parsers/degiro.tsRepository: GeiserX/DeclaRenta
Length of output: 493
🏁 Script executed:
# Check lines 277-280
sed -n '275,285p' src/parsers/degiro.tsRepository: GeiserX/DeclaRenta
Length of output: 507
🏁 Script executed:
# Check lines 351-352
sed -n '348,355p' src/parsers/degiro.tsRepository: GeiserX/DeclaRenta
Length of output: 511
🏁 Script executed:
# Check lines 409-435
sed -n '405,440p' src/parsers/degiro.tsRepository: GeiserX/DeclaRenta
Length of output: 1123
🏁 Script executed:
# Search for fxRateToBase in degiro.ts to see where FX rates are assigned
rg "fxRateToBase" src/parsers/degiro.ts -A 2 -B 2Repository: GeiserX/DeclaRenta
Length of output: 357
🏁 Script executed:
# Check if getEcbRate function is available and used elsewhere
rg "getEcbRate" src/ -lRepository: GeiserX/DeclaRenta
Length of output: 188
🏁 Script executed:
# Look for the function signature of getEcbRate
rg "getEcbRate" src/ -B 2 -A 5Repository: GeiserX/DeclaRenta
Length of output: 6257
Remove Degiro's FX rates from the normalized statement.
Both trade and cash transaction objects persist broker-supplied rates into fxRateToBase, embedding execution rates into downstream tax calculations. The codebase already uses getEcbRate() to override these; don't bake the wrong rates in at parse time.
Affected code at lines 309-311 (trade) and 418 (cash transaction) where fxRateToBase is set from parsed FX data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parsers/degiro.ts` around lines 200 - 206, The parser currently extracts
Degiro's broker-supplied FX column into the fxRate variable via findColumn and
assigns it directly to trade.fxRateToBase and cashTransaction.fxRateToBase;
instead stop populating fxRateToBase from parsed FX data so downstream logic can
rely on getEcbRate() to supply the canonical rate. Remove or skip setting
fxRateToBase where it's assigned (the assignments around trade.fxRateToBase and
cashTransaction.fxRateToBase), keep the fxRate detection (findColumn) if needed
for diagnostics but do not persist its value into parsed trade or cash
transaction objects so getEcbRate() remains the authoritative source.
Comment on lines
+335
to
+379
| interface AccountColumns { | ||
| date: number; | ||
| product: number; | ||
| isin: number; | ||
| description: number; | ||
| fx: number; | ||
| currency: number; | ||
| amount: number; | ||
| orderId: number; | ||
| } | ||
|
|
||
| function resolveAccountColumns(headers: string[]): AccountColumns { | ||
| const date = findColumn(headers, ["Fecha", "Date", "Datum"]); | ||
| const product = findColumn(headers, ["Producto", "Product", "Produkt"]); | ||
| const isin = findColumn(headers, ["ISIN"]); | ||
| const description = findColumn(headers, DESCRIPTION_HEADERS); | ||
| const fx = findColumn(headers, ["Tipo de cambio", "Tipo", "FX", "Wisselkoers", "Wechselkurs"]); | ||
|
|
||
| // Degiro Account CSV has two known layouts: | ||
| // Old: ..., Tipo de cambio, [empty], Importe, [empty], Saldo, ... | ||
| // → "Importe" = amount col, currency at fx+1 | ||
| // New (real export): ..., Tipo, Variación, [empty], Saldo, ... | ||
| // → "Variación" col holds CURRENCY in data, amount is at Variación+1 | ||
|
|
||
| let currency: number; | ||
| let amount = findColumn(headers, ["Importe", "Amount", "Change", "Mutatie", "Änderung"]); | ||
|
|
||
| if (amount >= 0) { | ||
| // Old format: amount found directly, currency is before it | ||
| currency = findColumn(headers, ["Divisa tipo de cambio", "Currency", "Währung", "Valuta"]); | ||
| if (currency < 0 && fx >= 0) currency = fx + 1; | ||
| } else { | ||
| // New format: "Variación" header = currency position, amount = next column | ||
| const varCol = findColumn(headers, ["Variación", "Variation"]); | ||
| if (varCol >= 0) { | ||
| currency = varCol; | ||
| amount = varCol + 1; | ||
| } else { | ||
| currency = fx >= 0 ? fx + 1 : -1; | ||
| } | ||
| } | ||
|
|
||
| const orderId = findColumn(headers, ["ID Orden", "Order ID", "Auftrags-ID"]); | ||
|
|
||
| return { date, product, isin, description, fx, currency, amount, orderId }; |
Contributor
There was a problem hiding this comment.
Keep the account value date instead of dropping it.
isDegiroAccount() requires Fecha valor / Value date, but AccountColumns never stores that column and parseAccountCsv() sets both dateTime and settleDate from Fecha/Date. If booking date and value date differ, dividends and withholdings will be normalized onto the wrong settlement day.
Minimal fix
interface AccountColumns {
date: number;
+ valueDate: number;
product: number;
isin: number;
description: number;
fx: number;
currency: number;
amount: number;
orderId: number;
}
function resolveAccountColumns(headers: string[]): AccountColumns {
const date = findColumn(headers, ["Fecha", "Date", "Datum"]);
+ const valueDate = findColumn(headers, VALUE_DATE_HEADERS);
const product = findColumn(headers, ["Producto", "Product", "Produkt"]);
const isin = findColumn(headers, ["ISIN"]);
const description = findColumn(headers, DESCRIPTION_HEADERS);
const fx = findColumn(headers, ["Tipo de cambio", "Tipo", "FX", "Wisselkoers", "Wechselkurs"]);
...
- return { date, product, isin, description, fx, currency, amount, orderId };
+ return { date, valueDate, product, isin, description, fx, currency, amount, orderId };
}
...
- const tradeDate = convertDate(dateStr);
+ const bookingDate = convertDate(dateStr);
+ const settleDate =
+ cols.valueDate >= 0 ? convertDate(fields[cols.valueDate] ?? "") : bookingDate;
...
- dateTime: tradeDate,
- settleDate: tradeDate,
+ dateTime: bookingDate,
+ settleDate,Also applies to: 402-435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parsers/degiro.ts` around lines 335 - 379, The parser currently drops the
"value date" / "Fecha valor" column which causes parseAccountCsv() to set both
booking and settlement dates from the same "Fecha/Date" column; update the
AccountColumns type to include a separate settleDate (or valueDate) field,
update resolveAccountColumns() to locate the "Fecha valor"/"Value date" header
(e.g., findColumn(headers, ["Fecha valor","Value date"]) and assign it to that
new field), and then modify parseAccountCsv() to use the new
settleDate/valueDate column when populating settleDate (leaving dateTime
populated from the original date column); ensure isDegiroAccount() still checks
the new settleDate/valueDate column as required.
10 tasks
GeiserX
added a commit
that referenced
this pull request
Apr 15, 2026
* ci: single-workflow release pipeline (no PAT needed) - Merge auto-tag + release into one workflow (like CashPilot/LynxPrompt) - Tag, sync package.json, generate changelog, create release — all in one job - Uses only GITHUB_TOKEN (no PAT secret required) - CLI reads version from package.json (single source of truth) - Update ROADMAP to reflect v0.2.0 state * feat: multi-broker architecture with Degiro CSV parser - Add BrokerParser interface (detect + parse → Statement) - Add parser registry with auto-detection (detectBroker, getBroker) - Wrap IBKR parser with BrokerParser interface - Implement Degiro parser: Transactions CSV (trades) + Account CSV (dividends/withholdings), multi-language (ES/EN/NL/DE), auto-detect delimiter (comma/semicolon), EU number format support - Update CLI: add --broker flag, auto-detect broker from file content - Update ROADMAP with broker tax integration research findings: Degiro/Scalable (high priority, no AEAT integration) vs Trade Republic/XTB (low priority, already report to AEAT) - 99 tests passing (24 new: 18 Degiro parser, 6 registry) * fix: validate Degiro parser against real CSV export - Handle real 19-column Transactions CSV format (Número, Bolsa de, Valor local + Valor + Tipo de cambio columns) - Add "número"/"number" to quantity header detection - Parse FX rate from CSV, default to "1" for EUR trades - Skip zero-price rows (rights assignments, non-tradeable conversions) - Add real Degiro CSV fixture from mxmhos/degiro (GitHub) - 7 new integration tests against real export data - 106 tests passing total * fix: Degiro Account CSV parser for real export format Handle dual Account CSV layouts: old format with "Importe" column vs real export with "Variación" header mapping to currency position. Add "Retención del dividendo" withholding pattern. Validate against real Degiro Account export fixture (4 dividends + 4 withholdings). * docs: update Degiro parser docstring to reflect Account CSV support
This was referenced Apr 15, 2026
This was referenced Apr 24, 2026
GeiserX
added a commit
that referenced
this pull request
Apr 28, 2026
* ci: single-workflow release pipeline (no PAT needed) - Merge auto-tag + release into one workflow (like CashPilot/LynxPrompt) - Tag, sync package.json, generate changelog, create release — all in one job - Uses only GITHUB_TOKEN (no PAT secret required) - CLI reads version from package.json (single source of truth) - Update ROADMAP to reflect v0.2.0 state * feat: multi-broker architecture with Degiro CSV parser - Add BrokerParser interface (detect + parse → Statement) - Add parser registry with auto-detection (detectBroker, getBroker) - Wrap IBKR parser with BrokerParser interface - Implement Degiro parser: Transactions CSV (trades) + Account CSV (dividends/withholdings), multi-language (ES/EN/NL/DE), auto-detect delimiter (comma/semicolon), EU number format support - Update CLI: add --broker flag, auto-detect broker from file content - Update ROADMAP with broker tax integration research findings: Degiro/Scalable (high priority, no AEAT integration) vs Trade Republic/XTB (low priority, already report to AEAT) - 99 tests passing (24 new: 18 Degiro parser, 6 registry) * fix: validate Degiro parser against real CSV export - Handle real 19-column Transactions CSV format (Número, Bolsa de, Valor local + Valor + Tipo de cambio columns) - Add "número"/"number" to quantity header detection - Parse FX rate from CSV, default to "1" for EUR trades - Skip zero-price rows (rights assignments, non-tradeable conversions) - Add real Degiro CSV fixture from mxmhos/degiro (GitHub) - 7 new integration tests against real export data - 106 tests passing total * fix: Degiro Account CSV parser for real export format Handle dual Account CSV layouts: old format with "Importe" column vs real export with "Variación" header mapping to currency position. Add "Retención del dividendo" withholding pattern. Validate against real Degiro Account export fixture (4 dividends + 4 withholdings). * docs: update Degiro parser docstring to reflect Account CSV support
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/types/broker.ts): commondetect()+parse()contract for all broker parserssrc/parsers/index.ts):detectBroker()auto-detection,getBroker()lookup, extensible arraysrc/parsers/degiro.ts): full implementation supporting:1.234,56) and standard format--broker <name>flag, auto-detection fallback, improved error messagesBroker research highlights
Test plan
Summary by CodeRabbit
New Features
Documentation
Tests
Chores