DEGIRO: Add Polish language support to V3 and improve error handling#326
DEGIRO: Add Polish language support to V3 and improve error handling#326WarpPL wants to merge 15 commits into
Conversation
* Implement filtering for Polish deposit and transfer records * Enhance transaction fee detection for Polish and other languages * Improve error handling in test cases for better clarity * Add warnings for partial-fill orders in DEGIRO processing
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a pre-scan in the DEGIRO converter to group fills by Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Hi! First contribution here. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/securityService.test.ts (1)
555-570:⚠️ Potential issue | 🟡 MinorEnv restoration is unsafe: assigning
undefinedstringifies to"undefined", and cleanup is skipped on assertion failure.If
ISIN_OVERRIDE_FILEwas not previously set,oldEnvisundefinedandprocess.env.ISIN_OVERRIDE_FILE = oldEnvresults in the literal string"undefined"rather than removing the key. Additionally, the restoration runs inline after the assertions, so a failingexpect(or any throw) leaves the env var polluted for subsequent tests in the same worker. Same applies to lines 576-577/615 and the pre-existing block at 516-540.🛡️ Suggested pattern
- const oldEnv = process.env.ISIN_OVERRIDE_FILE; - process.env.ISIN_OVERRIDE_FILE = "isin-overrides-nonexistent.txt"; - jest.resetModules(); - const { SecurityService } = require("./securityService"); - const yahooFinanceMock = new YahooFinanceServiceMock(); - const sut = new SecurityService(yahooFinanceMock); - - // Act - const cache = await sut.loadCache(); - - // Assert - expect(cache[0]).toBe(0); - expect(cache[1]).toBe(0); - expect(cache[2]).toBe(0); - - process.env.ISIN_OVERRIDE_FILE = oldEnv; + const oldEnv = process.env.ISIN_OVERRIDE_FILE; + process.env.ISIN_OVERRIDE_FILE = "isin-overrides-nonexistent.txt"; + try { + jest.resetModules(); + const { SecurityService } = require("./securityService"); + const yahooFinanceMock = new YahooFinanceServiceMock(); + const sut = new SecurityService(yahooFinanceMock); + + // Act + const cache = await sut.loadCache(); + + // Assert + expect(cache[0]).toBe(0); + expect(cache[1]).toBe(0); + expect(cache[2]).toBe(0); + } finally { + if (oldEnv === undefined) { + delete process.env.ISIN_OVERRIDE_FILE; + } else { + process.env.ISIN_OVERRIDE_FILE = oldEnv; + } + }Alternatively, hoist the save/restore into
beforeEach/afterEachfor theloadCache()describe block to DRY up the three call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/securityService.test.ts` around lines 555 - 570, The test mutates process.env.ISIN_OVERRIDE_FILE unsafely and doesn't guarantee cleanup on failure; change the test setup to save and restore the env var in a safe afterEach (or wrap the mutate/assert in try/finally) so the original value is restored correctly: when oldEnv is undefined delete process.env.ISIN_OVERRIDE_FILE instead of assigning undefined (to avoid the literal "undefined"), and ensure restoration runs in afterEach (or finally) for the tests that instantiate SecurityService, call sut.loadCache(), and use jest.resetModules() so failing assertions won't leave the env polluted.
🧹 Nitpick comments (3)
src/converters/degiroConverterV3.ts (2)
337-340: Redundant patterns — already caught by existing generic terms.
isIgnoredRecordusesindexOf-based substring matching."fx credit".toLowerCase()is already matched by"credit"(line 309), and"fx withdrawal"by"withdrawal"(line 301). These two entries are dead code and can be removed to keep the list lean."hong kong stamp duty"is genuinely needed because"stamp duty"was added toisTransactionFeeRecord— keep that one.🧹 Proposed cleanup
// FX records - these are paired with trade records and should be ignored - "fx credit", - "fx withdrawal", "hong kong stamp duty"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV3.ts` around lines 337 - 340, The isIgnoredRecord list contains redundant entries: because matching is done via case-insensitive substring checks (indexOf on .toLowerCase()), remove the "fx credit" and "fx withdrawal" strings from the array in degiroConverterV3.ts since "credit" and "withdrawal" are already present (keep "hong kong stamp duty" as-is because it is not covered by the generic "stamp duty" in isTransactionFeeRecord).
497-511: Edge case: regex is not anchored, could match a separator-shaped token inside the product name.Example trace on
"Kupno 100 ACME-1,234 Fund@5,0 EUR":
beforeAt = "Kupno 100 ACME-1,234 Fund"- The thousands-separator regex first matches
"1,234"(a 1-digit number +,+ 3 digits) and returns1234instead of the real quantity100.Real DEGIRO product names rarely contain such tokens, so this is unlikely in practice, but the current fallback order means a 1-3 digit quantity without a separator loses to any later separator-shaped token in the name. A small hardening is to prefer the first numeric token and only treat it as grouped if the adjacent characters form a valid thousands group:
🛡️ Optional hardening
- // Match: 1-3 leading digits + one or more groups of - // (space|dot|comma|NBSP|narrow NBSP + exactly 3 digits) - // This handles all locale thousands-separator variants while ignoring decimal separators. - const withSeparator = beforeAt.match(/(\d{1,3}(?:[,. \u00A0\u202F]\d{3})+)/); - if (withSeparator) { - return parseInt(withSeparator[0].replace(/[,. \u00A0\u202F]/g, ""), 10); - } - - // Fallback: plain integer (no separator). - const plain = beforeAt.match(/(\d+)/); - return plain ? parseInt(plain[0], 10) : 0; + // Find the first numeric token; expand it if it's immediately followed by + // one or more groups of (separator + exactly 3 digits). + const first = beforeAt.match(/\d{1,3}(?:[,. \u00A0\u202F]\d{3})+|\d+/); + if (!first) return 0; + return parseInt(first[0].replace(/[,. \u00A0\u202F]/g, ""), 10);This uses a single alternation anchored by the first numeric occurrence, so a plain
100wins over any later1,234appearing in the product name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV3.ts` around lines 497 - 511, In parseQuantityFromDescription, the current unanchored grouped-number regex can match a later thousands-formatted token in the product name (e.g., "1,234") and override the true leading quantity; change the logic to locate the first numeric token and prefer that: find the first occurrence of a digit sequence (e.g., match /\d{1,3}/), then check at that same position whether it is part of a valid grouped-thousands pattern (the existing grouped form used by withSeparator) and parse that if so, otherwise parse the first plain integer; update references in parseQuantityFromDescription accordingly so the grouped check only applies to the first numeric token rather than any match in the whole string.src/converters/degiroConverterV3.test.ts (1)
72-72:done.fail()is a jasmine2-ism and is not supported byjest-circus(Jest 30's default runner).The new callbacks in this PR correctly use
done(err || new Error(...)), but several existing sites still calldone.fail("Should not succeed!"). In Jest 30 withjest-circus,done.failisundefined, so if an unexpected success ever triggers those branches the test would throwTypeError: done.fail is not a functioninstead of a readable assertion failure.Replace with
done(new Error(...))for consistency and Jest 30 compatibility at lines 72, 90, 110, 134, and 211.Example fix
- sut.readAndProcessFile(tempFileName, () => { done.fail("Should not succeed!"); }, (err: Error) => { + sut.readAndProcessFile(tempFileName, () => { done(new Error("Should not succeed!")); }, (err: Error) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV3.test.ts` at line 72, The test uses jasmine2's done.fail which is undefined under jest-circus; locate the failing callbacks where sut.readAndProcessFile (and other test callbacks in this file) call done.fail("Should not succeed!") and replace each with done(new Error("Should not succeed!")) so unexpected success reports a proper Error; update all occurrences referenced (around the calls to sut.readAndProcessFile and similar test callback invocations at the noted sites) to use done(new Error(...)) for Jest 30 compatibility.
🤖 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/converters/degiroConverterV3.ts`:
- Around line 535-554: The entry "gebühr" in the transactionFeeRecordType array
is too broad and can accidentally match generic German fee phrases; update the
transactionFeeRecordType (used in degiroConverterV3.ts) to a more specific
German identifier such as "transaktionsgebühr" (or another sample-specific
phrase) instead of the generic "gebühr", and/or ensure the isPlatformFees logic
in processFileContents is extended to include any German platform-fee variants
(so platform fees are detected before transactionFeeRecordType runs); modify the
transactionFeeRecordType array and, if chosen, add the new platform keywords to
the isPlatformFees checks to preserve correct classification.
---
Outside diff comments:
In `@src/securityService.test.ts`:
- Around line 555-570: The test mutates process.env.ISIN_OVERRIDE_FILE unsafely
and doesn't guarantee cleanup on failure; change the test setup to save and
restore the env var in a safe afterEach (or wrap the mutate/assert in
try/finally) so the original value is restored correctly: when oldEnv is
undefined delete process.env.ISIN_OVERRIDE_FILE instead of assigning undefined
(to avoid the literal "undefined"), and ensure restoration runs in afterEach (or
finally) for the tests that instantiate SecurityService, call sut.loadCache(),
and use jest.resetModules() so failing assertions won't leave the env polluted.
---
Nitpick comments:
In `@src/converters/degiroConverterV3.test.ts`:
- Line 72: The test uses jasmine2's done.fail which is undefined under
jest-circus; locate the failing callbacks where sut.readAndProcessFile (and
other test callbacks in this file) call done.fail("Should not succeed!") and
replace each with done(new Error("Should not succeed!")) so unexpected success
reports a proper Error; update all occurrences referenced (around the calls to
sut.readAndProcessFile and similar test callback invocations at the noted sites)
to use done(new Error(...)) for Jest 30 compatibility.
In `@src/converters/degiroConverterV3.ts`:
- Around line 337-340: The isIgnoredRecord list contains redundant entries:
because matching is done via case-insensitive substring checks (indexOf on
.toLowerCase()), remove the "fx credit" and "fx withdrawal" strings from the
array in degiroConverterV3.ts since "credit" and "withdrawal" are already
present (keep "hong kong stamp duty" as-is because it is not covered by the
generic "stamp duty" in isTransactionFeeRecord).
- Around line 497-511: In parseQuantityFromDescription, the current unanchored
grouped-number regex can match a later thousands-formatted token in the product
name (e.g., "1,234") and override the true leading quantity; change the logic to
locate the first numeric token and prefer that: find the first occurrence of a
digit sequence (e.g., match /\d{1,3}/), then check at that same position whether
it is part of a valid grouped-thousands pattern (the existing grouped form used
by withSeparator) and parse that if so, otherwise parse the first plain integer;
update references in parseQuantityFromDescription accordingly so the grouped
check only applies to the first numeric token rather than any match in the whole
string.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cacf291a-5511-4cbc-96cc-3a75c56d16fc
📒 Files selected for processing (4)
README.mdsrc/converters/degiroConverterV3.test.tssrc/converters/degiroConverterV3.tssrc/securityService.test.ts
Use a unique `E2G_CACHE_FOLDER` per test in `securityService.test.ts` and restore env/cleanup afterward to prevent parallel test interference.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/converters/degiroConverterV3.test.ts (1)
408-468: Good coverage on partial-fill warning and locale quantity parsing.The partial-fill test asserts both the
MISSING:andDETAILS:segments of the warning — that's the right level of specificity (string contains, not full equality), so formatting tweaks to the warning won't make this test brittle. The parametrizedcasestable for quantity parsing exercises every locale separator branch (space/NBSP/narrow-NBSP/dot/comma/plain) and explicitly guards against the unit-price-after-@regression. Solid.One small observation: the "price with comma decimal" case (
"Kupno 100 ACME@1,234 EUR"→ 100) demonstrates thesplit("@")[0]safeguard works, but all expected quantities in the table are ≤ 10000. If you want to harden against the edge case mentioned onparseQuantityFromDescription, consider adding a case with a product name that embeds a\d{1,3} \d{3}pattern (e.g."Kupno 5 MSCI World 2 000 Index@1,0 USD"→ 5) to pin the expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV3.test.ts` around lines 408 - 468, Add a new parametrized test case to the cases array in src/converters/degiroConverterV3.test.ts to cover product names that include a thousands-style number so parseQuantityFromDescription doesn't pick the embedded number as quantity; specifically add an entry like description: "Kupno 5 MSCI World 2 000 Index@1,0 USD" with expectedQty: 5, ensuring the test still exercises DeGiroConverterV3.processFileContents and asserts activity.quantity equals 5 to lock in the split("@")[0] safeguard.src/converters/degiroConverterV3.ts (2)
72-104: Pre-scan grouping and warning logic look correct.Verified both CSV orderings (fee before fills and fills before fee):
fills[0]is always the first buy/sell row in CSV order, which is exactly the row that ends up paired with the fee and exported (the duplicate-skip guard at lines 122–130 drops the rest by matchingorderId). SoEXPORTED = firstFillQtyandMISSING = totalQty - firstFillQtyare accurate in both cases.Minor nit: on line 84,
fillsByOrderId.get(r.orderId)returnsT | undefinedto TypeScript even though it's guaranteed defined here. Ifstrict/noUncheckedIndexedAccessever gets tightened, consider using the return value ofsetor a local variable to avoid the implicit non-null assumption:♻️ Optional readability tweak
- if (!fillsByOrderId.has(r.orderId)) fillsByOrderId.set(r.orderId, []); - fillsByOrderId.get(r.orderId).push(r); + const existing = fillsByOrderId.get(r.orderId); + if (existing) { + existing.push(r); + } else { + fillsByOrderId.set(r.orderId, [r]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV3.ts` around lines 72 - 104, Pre-scan uses fillsByOrderId.get(r.orderId) and then fillsByOrderId.get(...).push(...), which yields a T | undefined type; change the bucket creation to use a local variable and set it when missing to avoid the implicit non-null assumption: inside the loop that builds fillsByOrderId (referencing fillsByOrderId, r.orderId, isBuyOrSellRecord, isIgnoredRecord), do let bucket = fillsByOrderId.get(r.orderId); if (!bucket) { bucket = []; fillsByOrderId.set(r.orderId, bucket); } then push into bucket; this removes the undefined typing without relying on non-null assertions and keeps the later logic that reads fillsByOrderId (and parseQuantityFromDescription) unchanged.
491-505: Quantity regex can be fooled by numeric patterns inside product names.The regex
\d{1,3}(?:[,. \u00A0\u202F]\d{3})+scans the entire substring before@and returns the first match. For a pathological but not impossible description like"Kupno 5 Fund ABC 123 456 XYZ@..."(company/index name containing a<1–3 digits> <3 digits>sequence), it would match"123 456"and return123456instead of5. Real DEGIRO product names such as"MSCI 2 000"or"CAC 40 100"variants could theoretically trigger this.Two low-cost hardening options — pick whichever matches sample data:
- Anchor the quantity to the first number after a leading action verb (e.g.
Kupno|Koop|Kauf|Buy|Sell|Sprzedaz|Verkoop|Acquisto|Vendita).- Require a whitespace boundary before the captured digit group and prefer the earliest match (current behavior is earliest, but without left-boundary enforcement, a digit group inside a word like
"Fund123"slips past).♻️ Proposed refactor (option 2, minimal change)
- // Match: 1-3 leading digits + one or more groups of - // (space|dot|comma|NBSP|narrow NBSP + exactly 3 digits) - // This handles all locale thousands-separator variants while ignoring decimal separators. - const withSeparator = beforeAt.match(/(\d{1,3}(?:[,. \u00A0\u202F]\d{3})+)/); + // Match: whitespace-anchored 1-3 leading digits + one or more groups of + // (space|dot|comma|NBSP|narrow NBSP + exactly 3 digits). + // The leading boundary prevents matching digits inside product names. + const withSeparator = beforeAt.match(/(?:^|\s)(\d{1,3}(?:[,. \u00A0\u202F]\d{3})+)(?=\s|$)/); if (withSeparator) { - return parseInt(withSeparator[0].replace(/[,. \u00A0\u202F]/g, ""), 10); + return parseInt(withSeparator[1].replace(/[,. \u00A0\u202F]/g, ""), 10); }Not a blocker — current test cases all pass and real-world descriptions rarely hit this. Flagging as a robustness improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV3.ts` around lines 491 - 505, The regex in parseQuantityFromDescription can match digit groups inside product names; change the separators to require a left word boundary so we only match standalone numbers: update the grouped-separator regex to include a left boundary (e.g. /\b(\d{1,3}(?:[,. \u00A0\u202F]\d{3})+)/) and the fallback plain-integer regex to /\b(\d+)/ so the function returns the earliest standalone numeric token (still using the existing replace/parseInt logic) rather than digits embedded in words like product codes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/converters/degiroConverterV3.test.ts`:
- Around line 408-468: Add a new parametrized test case to the cases array in
src/converters/degiroConverterV3.test.ts to cover product names that include a
thousands-style number so parseQuantityFromDescription doesn't pick the embedded
number as quantity; specifically add an entry like description: "Kupno 5 MSCI
World 2 000 Index@1,0 USD" with expectedQty: 5, ensuring the test still
exercises DeGiroConverterV3.processFileContents and asserts activity.quantity
equals 5 to lock in the split("@")[0] safeguard.
In `@src/converters/degiroConverterV3.ts`:
- Around line 72-104: Pre-scan uses fillsByOrderId.get(r.orderId) and then
fillsByOrderId.get(...).push(...), which yields a T | undefined type; change the
bucket creation to use a local variable and set it when missing to avoid the
implicit non-null assumption: inside the loop that builds fillsByOrderId
(referencing fillsByOrderId, r.orderId, isBuyOrSellRecord, isIgnoredRecord), do
let bucket = fillsByOrderId.get(r.orderId); if (!bucket) { bucket = [];
fillsByOrderId.set(r.orderId, bucket); } then push into bucket; this removes the
undefined typing without relying on non-null assertions and keeps the later
logic that reads fillsByOrderId (and parseQuantityFromDescription) unchanged.
- Around line 491-505: The regex in parseQuantityFromDescription can match digit
groups inside product names; change the separators to require a left word
boundary so we only match standalone numbers: update the grouped-separator regex
to include a left boundary (e.g. /\b(\d{1,3}(?:[,. \u00A0\u202F]\d{3})+)/) and
the fallback plain-integer regex to /\b(\d+)/ so the function returns the
earliest standalone numeric token (still using the existing replace/parseInt
logic) rather than digits embedded in words like product codes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2374e8b4-a7b2-4c1a-a915-06b4e9d09614
📒 Files selected for processing (4)
README.mdsrc/converters/degiroConverterV3.test.tssrc/converters/degiroConverterV3.tssrc/securityService.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/securityService.test.ts
* Include deposit, withdrawal, and fee transactions in Polish
…ngifies to "undefined", and cleanup is skipped on assertion failure.
The unanchored regex could match a thousands-separated number embedded in the product name instead of the actual quantity.
|
Resolved all nitpicks and actionable comments issued by CodeRabbit. The PR is ready to review. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/converters/degiroConverterV3.ts`:
- Around line 501-512: parseQuantityFromDescription currently returns 0 on parse
failure which hides bad rows and causes division-by-zero in mapRecordToActivity;
change parseQuantityFromDescription to throw a descriptive Error (including the
original description string and maybe the record id/context) when neither the
separator nor plain-int regex matches so failures surface immediately; update
any callers (e.g., mapRecordToActivity) to let the exception propagate (or catch
and rethrow/wrap) rather than relying on a zero sentinel value.
- Around line 344-361: The matching logic in findMatchByOrderId treats
missing/undefined orderId values as equal; update it so orderId-based matching
only runs when currentRecord.orderId is present and non-empty. Concretely, in
findMatchByOrderId check if currentRecord.orderId is truthy before building
candidates (or add a guard that both r.orderId and currentRecord.orderId are
truthy in the filter), and if it's falsy return undefined so ISIN fallback can
run; keep the existing uses of isBuyOrSellRecord and isTransactionFeeRecord
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0d44cf4-9192-49c5-bceb-eb52fc6f33fd
📒 Files selected for processing (3)
src/converters/degiroConverterV3.test.tssrc/converters/degiroConverterV3.tssrc/securityService.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/securityService.test.ts
…seable quantity - findMatchByOrderId: return undefined early when currentRecord has no orderId, avoiding undefined===undefined matches between unrelated same-day no-order rows - findMatchByOrderId: scope the buy/sell preference to fee records only - processFileContents: skip buy/sell records with unparseable quantity (qty=0) with a warning instead of producing an activity with unitPrice=NaN in the export - degiro-export.csv sample test: update expected count 27→26 reflecting that "STOCK DIVIDEND: Koop 999 @ 0 EUR" now correctly surfaces as unparseable
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/converters/degiroConverterV3.ts (1)
310-350:⚠️ Potential issue | 🟠 MajorNarrow the generic
creditignore pattern.The new
fx creditentry is effectively shadowed by the existing broad"credit"match. SinceisIgnoredRecord()runs before conversion, a valid trade whose description contains a product name like “Credit …” can be silently skipped.🐛 Proposed fix
- "credit", "prelievo", "creditering", "debitering",Keep the specific entries with known DEGIRO cash-flow wording, such as
fx credit, instead of matching every occurrence ofcredit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV3.ts` around lines 310 - 350, The ignoredRecordTypes array in isIgnoredRecord() contains a too-broad "credit" entry that shadows specific patterns like "fx credit" and causes valid trade rows to be skipped; remove the generic "credit" token (or replace it with a more specific pattern such as " fx credit", "crediting", or other exact DEGIRO cash-flow phrases) and keep only the precise entries (e.g., "fx credit", "fx withdrawal", "credito", "creditering") so that legitimate trade descriptions containing the substring "credit" are not ignored.
♻️ Duplicate comments (1)
src/converters/degiroConverterV3.ts (1)
213-247:⚠️ Potential issue | 🟠 MajorValidate the matched fill before calling
combineRecords.Line 216 only guards the current row. If a fee row appears before an unparseable buy/sell fill,
matchingRecordcan still be passed intocombineRecords(), wheremapRecordToActivity()divides by0and producesunitPrice: NaN.Move the quantity check after matching and apply it to whichever side is the buy/sell record.
🐛 Proposed fix
- // Look ahead in the remaining records if there is one with the same orderId. - // Guard against division-by-zero in mapRecordToActivity: - // skip with a warning rather than producing an invalid activity (unitPrice: NaN). - if (this.isBuyOrSellRecord(record) && this.parseQuantityFromDescription(record.description) === 0) { - this.progress.log(`[w] Could not parse share quantity from: "${record.description}". Division by zero. Skipping record — add this activity manually.\n`); - bar1.increment(); - continue; - } - // Look ahead in the remaining records if there is one with the same orderId. let matchingRecord = this.findMatchByOrderId(record, records.slice(idx + 1)); // If there was no match by orderId, and there was no orderId present on the current record, look ahead in the remaining records to find a match by ISIN + Product. if (!matchingRecord && !record.orderId) { matchingRecord = this.findMatchByIsin(record, records.slice(idx + 1)); } + + // Guard against division-by-zero in mapRecordToActivity: + // skip with a warning rather than producing an invalid activity (unitPrice: NaN). + const buySellRecord = this.isBuyOrSellRecord(record) + ? record + : matchingRecord && this.isBuyOrSellRecord(matchingRecord) + ? matchingRecord + : undefined; + + if (buySellRecord && this.parseQuantityFromDescription(buySellRecord.description) === 0) { + this.progress.log(`[w] Could not parse share quantity from: "${buySellRecord.description}". Division by zero. Skipping record — add this activity manually.\n`); + bar1.increment(); + continue; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV3.ts` around lines 213 - 247, The zero-quantity guard should run after you find a matching record so you validate the actual fill (either the current record or the matchingRecord) before calling combineRecords; change the logic in the block that sets matchingRecord (found via findMatchByOrderId / findMatchByIsin) to check whichever of the two records returns true for isBuyOrSellRecord and then call parseQuantityFromDescription on that record, and if it equals 0 log the same warning via this.progress.log, call bar1.increment(), and continue instead of calling combineRecords/mapRecordToActivity; ensure you still handle standalone records (mapRecordToActivity/mapDividendRecord) as before and only skip paired processing when the parsed quantity is zero.
🤖 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/converters/degiroConverterV3.ts`:
- Around line 517-528: The parseQuantityFromDescription function can mis-parse
quantities when the thousands-separator regex partially consumes digits from a
numeric product name (e.g., "Buy 5 2000 Index..."); update the
thousands-separator match in parseQuantityFromDescription to forbid trailing
digits immediately after the captured number (use a negative lookahead such as
(?!\d) or an appropriate lookahead asserting end/space/@) so the regex only
matches a full grouped number and falls back to the plain integer case when the
next token contains extra digits.
---
Outside diff comments:
In `@src/converters/degiroConverterV3.ts`:
- Around line 310-350: The ignoredRecordTypes array in isIgnoredRecord()
contains a too-broad "credit" entry that shadows specific patterns like "fx
credit" and causes valid trade rows to be skipped; remove the generic "credit"
token (or replace it with a more specific pattern such as " fx credit",
"crediting", or other exact DEGIRO cash-flow phrases) and keep only the precise
entries (e.g., "fx credit", "fx withdrawal", "credito", "creditering") so that
legitimate trade descriptions containing the substring "credit" are not ignored.
---
Duplicate comments:
In `@src/converters/degiroConverterV3.ts`:
- Around line 213-247: The zero-quantity guard should run after you find a
matching record so you validate the actual fill (either the current record or
the matchingRecord) before calling combineRecords; change the logic in the block
that sets matchingRecord (found via findMatchByOrderId / findMatchByIsin) to
check whichever of the two records returns true for isBuyOrSellRecord and then
call parseQuantityFromDescription on that record, and if it equals 0 log the
same warning via this.progress.log, call bar1.increment(), and continue instead
of calling combineRecords/mapRecordToActivity; ensure you still handle
standalone records (mapRecordToActivity/mapDividendRecord) as before and only
skip paired processing when the parsed quantity is zero.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e355fd4-af4f-49b6-95b1-b9b7a304416f
📒 Files selected for processing (3)
GitVersion.ymlsrc/converters/degiroConverterV3.test.tssrc/converters/degiroConverterV3.ts
✅ Files skipped from review due to trivial changes (1)
- GitVersion.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/converters/degiroConverterV3.test.ts
WarpPL
changed the title
|
Hi @dickwolff ! |
- Verify correct fee aggregation for transactions involving DEGIRO fees and various local taxes across different exchanges. - Ensure accurate handling of multiple fills and their associated costs in the exported activities. - Implement tests for dividend reversal scenarios to ensure no duplicates are created when DEGIRO re-books dividends.
|
Hi @dickwolff ! |
1 participant
DEGIRO V3: Polish locale, partial-fill merging, dividend storno
What changed
Polish locale support
depozyt,przelew,wpłata/wypłata,opłata abonamentu,zmiana produktui/lub,opłata transakcyjna,podatek dywidendowy,podatek od transakcji(catches French/Italian variants too)Quantity parsing rewritten
Previous regex broke on space-separated thousands (e.g.
Kupno 2 083 iShares…). NewparseQuantityFromDescriptionhandles space/NBSP/dot/comma separators and ignores the unit price after@.FX and stamp-duty rows filtered
FX Credit,FX Withdrawal,Hong Kong Stamp Dutyadded to ignored records — they were previously exported as unknown activities.Partial-fill orders merged
When DEGIRO splits one order into multiple fills, all fills are now merged into one activity with total quantity and weighted-average unit price. A log line is printed with the merge details.
Dividend storno suppressed
When a dividend and its full reversal appear in the same file (same ISIN, date, time), both pairs are suppressed and no activity is produced. Isolated reversals (different date) are still caught by the existing fallback.
Fee-pairing fix
findMatchByOrderIdcould pair fill+fill instead of fill+fee for partial-fill orders, misclassifying fills as dividends. Fixed.Date comparison fix
dayjs().isSame(..., 'day')replaced with string comparison to avoid timezone shifts.Tests
35 tests pass. New cases: Polish records, FX filtering, partial-fill merge, dividend storno (same-date and different-date), 10 quantity-parsing variants.