XTB: Accept new export format#196
Conversation
|
""" WalkthroughThe pull request updates the XTB documentation and CSV conversion logic to accommodate changes in the XTB cash operations export template. The README now provides detailed, step-by-step instructions for exporting an Excel file, navigating to the “CASH OPERATION HISTORY” tab, and post-processing the resulting CSV file by removing extraneous lines. In the converter tests, the expected structure of the CSV has been updated by modifying delimiters and adjusting column counts. The CSV parsing logic in the Assessment against linked issues
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (1)
164-164: Minor grammatical improvement suggestion.The sentence is missing an article before "header line".
-Open the CSV file in an editor to clean it up. Remove all lines above header line (`ID,Type,Time,Comment,Symbol,Amount,,,,`), and remove the `Total` line as well. The file can now be imported into the tool. +Open the CSV file in an editor to clean it up. Remove all lines above the header line (`ID,Type,Time,Comment,Symbol,Amount,,,,`), and remove the `Total` line as well. The file can now be imported into the tool.🧰 Tools
🪛 LanguageTool
[uncategorized] ~164-~164: You might be missing the article “the” here.
Context: ... to clean it up. Remove all lines above header line (`ID,Type,Time,Comment,Symbol,Amou...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
src/converters/xtbConverter.ts (3)
180-200: Improved error handling with better variable initialization.The changes to variable initialization and match handling significantly improve the robustness of the code. However, consider using optional chaining as suggested by the static analysis.
- if (match && match[1] && match[2]) { + if (match && match?.[1] && match?.[2]) { - } else if (match && match[3]) { + } else if (match && match?.[3]) {🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
305-310: Consider optimizing the dividend tax record lookup logic.The current condition
idx > 0 && records.length - 1 > idx + 1will always fail on the first record due to the first part, but still evaluates the second part. Consider restructuring for better efficiency.- if (idx > 0 && records.length - 1 > idx + 1) { + if (idx > 0 && idx + 1 < records.length) {
316-316: Use consistent optional chaining.The code uses optional chaining for
previousRecord?.typebut not for the ID comparison.- if (previousRecord?.type.toLocaleLowerCase().indexOf("tax") > -1 && currentRecordId + 1 === previousRecord?.id) { + if (previousRecord?.type.toLocaleLowerCase().indexOf("tax") > -1 && currentRecordId + 1 === previousRecord.id) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsamples/xtb-export.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
GitVersion.yml(1 hunks)README.md(1 hunks)package.json(1 hunks)src/converters/xtbConverter.test.ts(5 hunks)src/converters/xtbConverter.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/converters/xtbConverter.ts
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
README.md
[uncategorized] ~164-~164: You might be missing the article “the” here.
Context: ... to clean it up. Remove all lines above header line (`ID,Type,Time,Comment,Symbol,Amou...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (10)
package.json (1)
3-3: LGTM! Version increment aligns with PR objectives.The version change from "0.29.1" to "0.29.2" correctly implements a patch version increment for the bug fix that accepts the new XTB CSV export format.
GitVersion.yml (1)
1-1: LGTM! Version update is consistent.The next-version update from "0.29.1" to "0.29.2" is consistent with the version change in package.json, indicating proper synchronized versioning across configuration files.
README.md (1)
162-164: Excellent update to the XTB export instructions.The new instructions are much more detailed and clearly guide users through the process of exporting and preparing the XTB data for import, including critical steps like:
- Navigating to the correct tab in the Excel file
- Setting the separation character to a comma
- Cleaning up the CSV file by removing extraneous lines
This update directly addresses the bug mentioned in the PR objectives about accepting the new XTB export format.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~164-~164: You might be missing the article “the” here.
Context: ... to clean it up. Remove all lines above header line (`ID,Type,Time,Comment,Symbol,Amou...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
src/converters/xtbConverter.test.ts (3)
37-37: Updated test expectations for the new format.The assertion has been updated to expect 36 activities instead of the previous number, correctly reflecting the changes in the XTB export format structure.
67-68: Test data format successfully updated to match new CSV structure.The test cases have been properly updated to use comma-delimited data instead of the previous semicolon format. The header line and example data now match the new XTB export format with 10 columns in the header.
Also applies to: 86-88, 104-106, 128-130
94-94: Error message properly updated to reflect new column count.The error message now correctly references the expected 10 columns versus 12 received, matching the new CSV structure.
src/converters/xtbConverter.ts (4)
26-26: Change of delimiter correctly implements support for the new XTB export format.The change from semicolon to comma delimiter aligns with the PR objective to accept the new XTB export format.
47-47: Good addition for interest detection.Adding support for "free-funds interest" increases the robustness of the code to handle variations in the text of the records.
67-71: Robust date format handling for different locales.The date conversion code correctly handles US date formats (MM/DD/YYYY) and converts them to the expected format (DD.MM.YYYY) for consistent processing.
281-286: CSV header updates properly handle the new format.The added columns with their comments clearly explain their purpose in the parsing process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/converters/xtbConverter.ts (2)
180-200: More robust data extraction and validationThe initialization of variables to zero and additional validation is a good improvement. This prevents processing records with insufficient data and provides helpful logging.
Consider simplifying the optional chaining expressions:
-if (match && match?.[1] && match?.[2]) { +if (match?.[1] && match?.[2]) { -} else if (match && match?.[3]) { +} else if (match?.[3]) {This simplification addresses the static analysis warnings while maintaining the same functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
158-162: Incorrect comment for fee recordLine 161 contains a misleading comment that refers to an "interest record" when this code block is actually handling fee records.
- // Add interest record to export. + // Add fee record to export.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)src/converters/xtbConverter.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🪛 Biome (1.9.4)
src/converters/xtbConverter.ts
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
src/converters/xtbConverter.ts (4)
26-26: Delimiter updated to support new CSV formatThe change from semicolon to comma delimiter aligns with the PR objective to support the new XTB export format.
47-47: Enhanced condition for interest type detectionAdded support for "free-funds interest" in addition to "free funds interests" to ensure all interest transactions are properly categorized.
67-71: US date format conversion addedGood addition to handle US date format (MM/DD/YYYY) and convert it to DD.MM.YYYY format. The detection relies on the presence of "/" in the date string, which seems reasonable for the XTB export format.
281-286: Updated headers to match new export formatThe additional columns (c1-c4) have been properly included in the header mapping to support the new CSV format.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/converters/xtbConverter.ts (1)
180-200: Improved data parsing robustnessThe parsing logic now initializes variables to zero and verifies successful matches before proceeding, preventing potential undefined value errors and adding better error handling.
However, there's a static analysis hint about using optional chaining that should be addressed:
- if (match && match?.[1] && match?.[2]) { + if (match?.at(1) && match?.at(2)) {- } else if (match && match?.[3]) { + } else if (match?.at(3)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
samples/xtb-export.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
README.md(1 hunks)src/converters/xtbConverter.test.ts(5 hunks)src/converters/xtbConverter.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/converters/xtbConverter.test.ts (3)
src/converters/xtbConverter.ts (1)
XtbConverter(11-323)src/securityService.ts (1)
SecurityService(13-370)src/testing/yahooFinanceServiceMock.ts (1)
YahooFinanceServiceMock(12-51)
🪛 GitHub Actions: Test Converters
src/converters/xtbConverter.test.ts
[error] 94-94: Expected: 'An error ocurred while parsing! Details: Invalid Record Length: columns length is 10; got 12 on line 2'. Received: 'An error ocurred while parsing! Details: Invalid Record Length: columns length is 10, got 12 on line 2'.
[error] 80-80: Exceeded timeout of 5000 ms for a test while waiting for done() to be called. Add a timeout value to this test to increase the timeout.
[error] 142-142: Expected: '[i] No result found for action buy; symbol SPYL.DE and comment OPEN BUY 8 @ 11.2835! Please add this manually..'. Received: '[i] No result found for action buy, symbol SPYL.DE and comment OPEN BUY 8 @ 11.2835! Please add this manually..'.
[error] 124-124: Exceeded timeout of 5000 ms for a test while waiting for done() to be called. Add a timeout value to this test to increase the timeout.
🪛 Biome (1.9.4)
src/converters/xtbConverter.ts
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
README.md (1)
162-164: Detailed XTB export instructions improve user experienceThe updated instructions provide a clearer step-by-step process for exporting XTB data, specifying the semicolon delimiter and necessary file clean-up steps. This aligns well with the changes made to the converter logic.
src/converters/xtbConverter.test.ts (2)
37-37: Updated expected activities countThe expected activities count has been updated from 34 to 36, which reflects the improved parsing logic in the XtbConverter implementation.
67-68: Test data format now matches implementation expectationsThe test CSV data structure has been updated to include semicolon delimiters and the additional empty columns needed by the parser.
Also applies to: 86-87, 104-105, 128-129
src/converters/xtbConverter.ts (3)
47-47: Added support for alternative interest type notationThe interest type detection now includes "free-funds interest" alongside "free funds interests", improving the converter's compatibility with different export formats.
67-71: Added US date format supportNew functionality to handle US date formats (MM/DD/YYYY), converting them to the expected DD.MM.YYYY format. This enhances the converter's robustness across different regional settings.
282-286: Added placeholder columns for correct parsingThe header mapping now includes additional columns (c1-c4) that are required for parsing but not used in the data processing. This ensures compatibility with the new export format.
dickwolff
force-pushed
the
feat/Xtb-New-Format
branch
from
9288ff7 to
af311c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsamples/xtb-export.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
README.md(1 hunks)src/converters/xtbConverter.test.ts(5 hunks)src/converters/xtbConverter.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- src/converters/xtbConverter.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/converters/xtbConverter.ts
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
src/converters/xtbConverter.ts (6)
47-47: Support added for alternative interest label format.Good addition to handle the new "free-funds interest" format alongside "free funds interests", improving the converter's robustness to variations in the XTB export format.
50-50: Handling of typo in dividend record type.Nice defensive programming to account for the "divident" typo in the export format. This will make the converter more resilient to variations in the source data.
67-71: US date format handling enhancement.Good addition to convert US date formats (MM/DD/YYYY) to European format (DD.MM.YYYY) which addresses the requirements for the new export format. Using dayjs for this conversion ensures reliable date handling.
195-200: Good validation and error handling for missing match data.The check for zero values and subsequent skipping of records with appropriate logging is a robust approach. This prevents processing incomplete or invalid records that could cause issues downstream.
281-286: Updated CSV headers to accommodate new format.Good adaptation to the new CSV structure by adding placeholders for additional columns that are required for proper parsing. The comments clearly indicate these are not used functionally but are needed for structural integrity.
25-30:Details
❓ Verification inconclusive
Verify CSV delimiter and parsing configuration.
The code uses semicolon (
;) as the delimiter while the AI summary mentions a change from semicolon to comma (,) in the test file. Please ensure this is intentional and that the delimiter used here matches what's expected in the new export format.
🏁 Script executed:
#!/bin/bash # Check if there's any mention of CSV delimiter in other parts of the codebase rg -i "delimiter|separator" --glob "*.ts" --glob "*.md" -A 1 -B 1Length of output: 4646
CSV Delimiter Discrepancy in xtbConverter.ts
The file
src/converters/xtbConverter.tscurrently configures the CSV parser to use a semicolon (;), whereas the export tests now indicate that a comma (,) is expected for the new format. Notably, other converter files use both delimiters—which suggests that each format might differ, or this might be an oversight. Please verify whether the XTB export format should adopt the comma delimiter like some other converters, or if it should continue using a semicolon. Update either the converter configuration or the test expectations/documentation accordingly.
dickwolff
force-pushed
the
feat/Xtb-New-Format
branch
from
37e0e0b to
3d6a599
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
162-163: Improve readability by breaking out XTB export steps
The current paragraph contains multiple actions that could be clearer as discrete steps. Consider refactoring into a numbered list or bullet points, for example:1. Click “Export button” to download the Excel file. 2. Open it in Excel and select the **CASH OPERATION HISTORY** tab. 3. Export that tab as a CSV file, using `;` (semicolon) as the separator.This will make it easier for users to follow each step.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
samples/xtb-export.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
README.md(1 hunks)src/converters/xtbConverter.test.ts(5 hunks)src/converters/xtbConverter.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/converters/xtbConverter.test.ts
- src/converters/xtbConverter.ts
🔇 Additional comments (1)
README.md (1)
164-164: Verify header cleanup instructions against converter expectations
You specify the header line as:ID;Type;Time;Comment;Symbol;Amount;;;;Please confirm that the number of trailing semicolons (4 empty columns) exactly matches what
XtbConverteris looking for. It may also help to present this header in a fenced code block for visual clarity.
…ted info and added support for usage of english in csv records.
Extracted all the match() calls in a single static method so that the parsing behavior is consistent. I was running into an issue for which all the `untiPrice` for a revolut trading report in EUR were NaN.
Bumps the npm_and_yarn group with 2 updates: and [brace-expansion](https://github.com/juliangruber/brace-expansion). Updates `brace-expansion` from 2.0.1 to 2.0.2 - [Release notes](https://github.com/juliangruber/brace-expansion/releases) - [Commits](juliangruber/brace-expansion@v2.0.1...v2.0.2) Updates `brace-expansion` from 1.1.11 to 2.0.2 - [Release notes](https://github.com/juliangruber/brace-expansion/releases) - [Commits](juliangruber/brace-expansion@v2.0.1...v2.0.2) --- updated-dependencies: - dependency-name: brace-expansion dependency-version: 2.0.2 dependency-type: indirect dependency-group: npm_and_yarn - dependency-name: brace-expansion dependency-version: 2.0.2 dependency-type: indirect dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps the dependencies group with 5 updates in the / directory: | Package | From | To | | --- | --- | --- | | [tsx](https://github.com/privatenumber/tsx) | `4.19.4` | `4.20.3` | | [@types/cacache](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/cacache) | `17.0.2` | `19.0.0` | | [jest](https://github.com/jestjs/jest/tree/HEAD/packages/jest) | `29.7.0` | `30.0.0` | | [@types/jest](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jest) | `29.5.14` | `30.0.0` | | [ts-jest](https://github.com/kulshekhar/ts-jest) | `29.3.4` | `29.4.0` | Updates `tsx` from 4.19.4 to 4.20.3 - [Release notes](https://github.com/privatenumber/tsx/releases) - [Changelog](https://github.com/privatenumber/tsx/blob/master/release.config.cjs) - [Commits](privatenumber/tsx@v4.19.4...v4.20.3) Updates `@types/cacache` from 17.0.2 to 19.0.0 - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/cacache) Updates `jest` from 29.7.0 to 30.0.0 - [Release notes](https://github.com/jestjs/jest/releases) - [Changelog](https://github.com/jestjs/jest/blob/main/CHANGELOG.md) - [Commits](https://github.com/jestjs/jest/commits/v30.0.0/packages/jest) Updates `@types/jest` from 29.5.14 to 30.0.0 - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jest) Updates `ts-jest` from 29.3.4 to 29.4.0 - [Release notes](https://github.com/kulshekhar/ts-jest/releases) - [Changelog](https://github.com/kulshekhar/ts-jest/blob/main/CHANGELOG.md) - [Commits](kulshekhar/ts-jest@v29.3.4...v29.4.0) --- updated-dependencies: - dependency-name: tsx dependency-version: 4.20.3 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: dependencies - dependency-name: "@types/cacache" dependency-version: 19.0.0 dependency-type: direct:development update-type: version-update:semver-major dependency-group: dependencies - dependency-name: jest dependency-version: 30.0.0 dependency-type: direct:development update-type: version-update:semver-major dependency-group: dependencies - dependency-name: "@types/jest" dependency-version: 30.0.0 dependency-type: direct:development update-type: version-update:semver-major dependency-group: dependencies - dependency-name: ts-jest dependency-version: 29.4.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: dependencies ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps the dependencies group with 2 updates in the / directory: [dotenv](https://github.com/motdotla/dotenv) and [jest](https://github.com/jestjs/jest/tree/HEAD/packages/jest). Updates `dotenv` from 16.5.0 to 16.6.0 - [Changelog](https://github.com/motdotla/dotenv/blob/master/CHANGELOG.md) - [Commits](motdotla/dotenv@v16.5.0...v16.6.0) Updates `jest` from 30.0.0 to 30.0.3 - [Release notes](https://github.com/jestjs/jest/releases) - [Changelog](https://github.com/jestjs/jest/blob/main/CHANGELOG.md) - [Commits](https://github.com/jestjs/jest/commits/v30.0.3/packages/jest) --- updated-dependencies: - dependency-name: dotenv dependency-version: 16.6.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: dependencies - dependency-name: jest dependency-version: 30.0.3 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: dependencies ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps the dependencies group with 2 updates: [dotenv](https://github.com/motdotla/dotenv) and [jest](https://github.com/jestjs/jest/tree/HEAD/packages/jest). Updates `dotenv` from 16.6.0 to 17.0.1 - [Changelog](https://github.com/motdotla/dotenv/blob/master/CHANGELOG.md) - [Commits](motdotla/dotenv@v16.6.0...v17.0.1) Updates `jest` from 30.0.3 to 30.0.4 - [Release notes](https://github.com/jestjs/jest/releases) - [Changelog](https://github.com/jestjs/jest/blob/main/CHANGELOG.md) - [Commits](https://github.com/jestjs/jest/commits/v30.0.4/packages/jest) --- updated-dependencies: - dependency-name: dotenv dependency-version: 17.0.1 dependency-type: direct:production update-type: version-update:semver-major dependency-group: dependencies - dependency-name: jest dependency-version: 30.0.4 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: dependencies ... Signed-off-by: dependabot[bot] <support@github.com>
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
README.md (2)
8-8: Comma missing after introductory adverbMinor grammar nit: add a comma after “Currently” to separate the introductory adverbial phrase.
-This tool allows you to convert CSV transaction exports to an import file that can be read by [Ghostfolio](https://github.com/ghostfolio/ghostfolio/). Currently there is support for 22 brokers: +This tool allows you to convert CSV transaction exports to an import file that can be read by [Ghostfolio](https://github.com/ghostfolio/ghostfolio/). Currently, there is support for 22 brokers:
162-165: Consider converting the multi-sentence instructions into an ordered list for clarityThe current paragraph is dense and easy to mis-scan, especially given the multi-step flow (download, open Excel, pick tab, export to CSV, set separator, manual cleanup). An ordered list makes each mandatory step explicit and reduces user error.
-Login to your XTB account and from the top bar click on "Account history", then "Cash operations". Click the "Export button" to download the Excel file. Open the file in Excel and go to the "CASH OPERATION HISTORY" tab. Then export this tab to a CSV file (and set the separation character to `;` (semicolon)). - -Open the CSV file in an editor to clean it up. Remove all lines above the header line (`ID;Type;Time;Comment;Symbol;Amount;;;;`), and remove the `Total` line as well. The file can now be imported into the tool. +1. Log in to your XTB account and choose **Account history → Cash operations**. +2. Click **Export** to download the Excel file. +3. Open the file in Excel, select the **CASH OPERATION HISTORY** tab, and **export only this tab** to a CSV file. + • Set the field separator to **`;` (semicolon)**. +4. Open the resulting CSV in a text editor and + • delete every line above the header + `ID;Type;Time;Comment;Symbol;Amount;;;;` + • delete the final **Total** line. +5. Save the cleaned file and import it into Export-to-Ghostfolio.No functional impact—purely readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
package-lock.jsonis excluded by!**/package-lock.jsonsamples/freetrade-export.csvis excluded by!**/*.csvsamples/revolut-invest-export.csvis excluded by!**/*.csvsamples/schwab-export.csvis excluded by!**/*.csvsamples/traderepublic-export.csvis excluded by!**/*.csv
📒 Files selected for processing (14)
.github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/workflows/docker.yml(1 hunks).github/workflows/frameworkTesting.yml(1 hunks)GitVersion.yml(1 hunks)README.md(2 hunks)package.json(2 hunks)src/converters/freetradeConverter.test.ts(1 hunks)src/converters/freetradeConverter.ts(2 hunks)src/converters/revolutConverter.test.ts(1 hunks)src/converters/revolutConverter.ts(3 hunks)src/converters/schwabConverter.test.ts(1 hunks)src/converters/schwabConverter.ts(3 hunks)src/converters/tradeRepublicConverter.test.ts(1 hunks)src/converters/tradeRepublicConverter.ts(3 hunks)
✅ Files skipped from review due to trivial changes (6)
- GitVersion.yml
- src/converters/schwabConverter.test.ts
- .github/workflows/frameworkTesting.yml
- .github/workflows/docker.yml
- .github/ISSUE_TEMPLATE/bug_report.md
- package.json
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~8-~8: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...s://github.com/ghostfolio/ghostfolio/). Currently there is support for 22 brokers: - [Av...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🔇 Additional comments (15)
src/converters/revolutConverter.test.ts (2)
38-38: Activity count increase needs justification.The expected activity count changed from 5 to 6. While this aligns with the refactored parsing logic, it would be helpful to understand what specific record is now being processed correctly that wasn't before.
Can you confirm which specific record type or scenario is now being processed correctly that accounts for the additional activity?
39-42: Good addition of NaN validation.The forEach loop validating that
unitPriceandquantityare not NaN is a great addition. This ensures the refactoredparseNumericValuemethod is working correctly and catches any parsing failures.src/converters/revolutConverter.ts (3)
74-74: Good refactoring to eliminate duplicate parsing logic.Replacing the inline parsing with
this.parseNumericValue(columnValue)is a good improvement that follows the DRY principle.
98-100: Consistent application of the new parsing method.The consistent use of
this.parseNumericValue()for price, value, and fees parsing is good. This ensures all numeric parsing follows the same logic and error handling.
238-252: Well-implemented parseNumericValue method.The new
parseNumericValuemethod is well-implemented with:
- Proper handling of empty strings
- Regex-based extraction of numeric values
- Comma removal for proper parsing
- NaN validation
- Descriptive error messages
This consolidates the parsing logic effectively and improves maintainability.
src/converters/tradeRepublicConverter.ts (3)
59-59: LGTM: Enhanced transaction type recognitionGood addition of English "sell" term alongside Dutch "verkoop" for better language support.
62-62: LGTM: Enhanced transaction type recognitionGood addition of English "buy" term alongside Dutch "aankoop" for better language support.
172-172: LGTM: Extended ignored record typesGood addition of English transaction types ("interest", "removal", "deposit") to the ignored list, maintaining consistency with the bilingual approach.
src/converters/schwabConverter.ts (4)
40-46: LGTM: Enhanced buy transaction classificationGood addition of "stock div dist" and "spin-off" as buy actions. The logic expansion improves transaction type recognition.
49-55: LGTM: Enhanced sell transaction classificationGood addition of multiple sell action types including "reverse split", "stock merger", "name change", and "conversion". This improves comprehensive transaction handling.
62-63: LGTM: Reordered dividend checksThe reordering of dividend type checks appears to be for better pattern matching priority.
90-99: LGTM: Added correction for inconsistent sell recordsGood addition of logic to correct sell records with negative quantities by converting them to buy records. This handles data inconsistencies gracefully.
src/converters/freetradeConverter.test.ts (1)
38-38: ✔️ Confirmed activity count updated to include FREESHARE_ORDER and PROPERTY entriesThe sample export (
samples/freetrade-export.csv) contains two new records—one FREESHARE_ORDER and one PROPERTY—so increasing the expected activities from 7 to 9 is correct. No further changes required.src/converters/freetradeConverter.ts (2)
33-33: LGTM: Enhanced dividend classificationGood addition of "property" to dividend classification. This expands the types of dividend-related transactions that are properly categorized.
135-135: LGTM: Enhanced order type recognitionGood addition of "freeshare_order" to be treated the same as "order" records. This ensures freeshare transactions are properly processed using the buy/sell logic.
| accountId: process.env.GHOSTFOLIO_ACCOUNT_ID, | ||
| comment: "", | ||
| fee: 0, | ||
| fee: Math.abs(record.costs), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the fee calculation change
The fee field was changed from a fixed zero to Math.abs(record.costs). This is a significant change that will affect all exported activities.
Please verify this change is intentional and correct by running the following script to understand the impact:
🏁 Script executed:
#!/bin/bash
# Description: Check for any existing tests or documentation about fee handling in TradeRepublic converter
# Look for fee-related tests
rg -A 5 -B 5 "fee.*0" src/converters/tradeRepublicConverter.test.ts
# Look for any comments about fee handling
rg -A 3 -B 3 "fee|cost" src/converters/tradeRepublicConverter.tsLength of output: 1032
Add tests and documentation for the new fee calculation
We’ve replaced the hard-coded zero fee with Math.abs(record.costs), but there are no existing tests or comments covering this change. Please ensure this update is fully validated:
- Add unit tests in
src/converters/tradeRepublicConverter.test.tsto cover:- Negative, positive, and zero values for
record.costs - Each transaction type (e.g. trade, dividend) to confirm fees are calculated as expected
- Negative, positive, and zero values for
- Update or add inline documentation in
src/converters/tradeRepublicConverter.ts:- Explain that CSV “costs” may be negative and why
Math.absis used - Clarify expected behavior when
costsis zero
- Explain that CSV “costs” may be negative and why
- Run the full converter against sample TradeRepublic exports to manually verify fee values match source statements
🤖 Prompt for AI Agents
In src/converters/tradeRepublicConverter.ts at line 130, the fee calculation was
changed to use Math.abs(record.costs) instead of a hard-coded zero, but there
are no tests or documentation for this update. Add unit tests in
src/converters/tradeRepublicConverter.test.ts to cover negative, positive, and
zero values of record.costs across all transaction types to verify correct fee
calculation. Also, update or add inline comments in tradeRepublicConverter.ts
explaining why Math.abs is used (due to possible negative CSV costs) and the
expected behavior when costs are zero. Finally, run the full converter on sample
TradeRepublic exports to manually confirm fee values match the source
statements.
| expect(actualExport).toBeTruthy(); | ||
| expect(actualExport.activities.length).toBeGreaterThan(0); | ||
| expect(actualExport.activities.length).toBe(4); | ||
| expect(actualExport.activities.length).toBe(6); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify test expectation aligns with converter changes
The expected activity count increased from 4 to 6. This should align with the enhanced transaction type recognition and fee handling changes in the converter.
Please verify this change is correct by running the following script:
🏁 Script executed:
#!/bin/bash
# Description: Analyze the sample file to understand the activity count change
# Check the sample file structure
head -10 samples/traderepublic-export.csv 2>/dev/null || echo "Sample file not found"
# Look for any test data or fixtures that might explain the count change
fd -e csv -e json traderepublic samples/ test/Length of output: 814
🏁 Script executed:
#!/bin/bash
# Locate the TradeRepublicConverter implementation
rg --files-with-matches "class TradeRepublicConverter" -n src
# Show the mapping of transaction types within the converter
rg -n "transactionType" -n src/converters/TradeRepublicConverter.tsLength of output: 253
🏁 Script executed:
#!/bin/bash
# Count transaction types in the sample export
tail -n +2 samples/traderepublic-export.csv | cut -d';' -f2 | sort | uniq -c
# Check converter for English “Buy” and “Sell” handlers
rg -n "case 'Buy'" -n src/converters/tradeRepublicConverter.ts || echo "No 'Buy' case found"
rg -n "case 'Sell'" -n src/converters/tradeRepublicConverter.ts || echo "No 'Sell' case found"Length of output: 444
Missing English transaction type handlers in TradeRepublicConverter
The converter only maps the Dutch types (“Aankoop”, “Verkoop”, “Dividend”, etc.), so the two “Buy” and “Sell” lines in the sample CSV aren’t recognized. As a result, the test change from 4→6 activities will fail until “Buy”/“Sell” are handled.
Please update src/converters/tradeRepublicConverter.ts to include:
switch (transactionType) {
case 'Aankoop':
+ case 'Buy':
return TransactionType.Buy;
case 'Verkoop':
+ case 'Sell':
return TransactionType.Sell;
case 'Dividend':
return TransactionType.Dividend;
// … other cases …
}This ensures the two English transactions are converted and the test passes.
📝 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.
| expect(actualExport.activities.length).toBe(6); | |
| // file: src/converters/tradeRepublicConverter.ts | |
| switch (transactionType) { | |
| case 'Aankoop': | |
| case 'Buy': | |
| return TransactionType.Buy; | |
| case 'Verkoop': | |
| case 'Sell': | |
| return TransactionType.Sell; | |
| case 'Dividend': | |
| return TransactionType.Dividend; | |
| // … other cases … | |
| } |
🤖 Prompt for AI Agents
In src/converters/tradeRepublicConverter.ts around the code handling transaction
type mapping, the converter currently only recognizes Dutch transaction types
and misses the English equivalents "Buy" and "Sell". Update the mapping logic to
include these English transaction types so that "Buy" maps to the same internal
type as "Aankoop" and "Sell" maps to the same as "Verkoop". This will ensure the
converter correctly processes English transaction types and the test expecting 6
activities will pass.
| // Check if the record has a CUSIP, try to get it by name instead. | ||
| if (record.symbol.match(/^[A-Z0-9]{8}[A-Z0-9*@#]$/)) { | ||
| const name = record.description.split(/XXX|0\%/)[0].trim(); | ||
| this.progress.log(`[i] CUSIP "${record.symbol}" detected, trying by name "${name}"\n`); | ||
| record.symbol = ""; | ||
| record.description = name; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify CUSIP regex pattern and name extraction logic
The CUSIP detection logic looks for 8 alphanumeric characters followed by a check digit. However, there are some concerns:
- The regex
^[A-Z0-9]{8}[A-Z0-9*@#]$may not handle all valid CUSIP formats - The name extraction using
split(/XXX|0\%/)[0].trim()assumes specific delimiters
Please verify the CUSIP pattern and name extraction logic:
🏁 Script executed:
#!/bin/bash
# Description: Verify CUSIP pattern usage and validate the extraction logic
# Look for CUSIP examples in test data or documentation
rg -A 3 -B 3 "CUSIP|cusip" src/ test/ samples/
# Check for any existing CUSIP handling patterns
ast-grep --pattern 'match(/.*CUSIP.*/)'Length of output: 972
Refine CUSIP validation and name‐extraction logic
The current check in src/converters/schwabConverter.ts (lines 161–167):
// Check if the record has a CUSIP, try to get it by name instead.
if (record.symbol.match(/^[A-Z0-9]{8}[A-Z0-9*@#]$/)) {
const name = record.description.split(/XXX|0\%/)[0].trim();
this.progress.log(`[i] CUSIP "${record.symbol}" detected, trying by name "${name}"\n`);
record.symbol = "";
record.description = name;
}has two issues:
- The regex allows
*,@,#and doesn’t enforce a full 9‐character alphanumeric CUSIP or validate its check digit. - Splitting on
/XXX|0\%/may miss other suffix patterns in descriptions.
Please revise as follows:
- Replace the regex with
/^[A-Z0-9]{9}$/(or implement proper check-digit validation per the CUSIP specification). - Remove non‐standard symbols (
*,@,#) from the allowed character set. - Make the name‐extraction more robust—either parameterize valid delimiters or build a parser that handles all known description‐suffix patterns.
- Add unit tests demonstrating both valid and invalid CUSIPs and covering the various description formats your data includes.
🤖 Prompt for AI Agents
In src/converters/schwabConverter.ts around lines 161 to 167, the CUSIP
validation regex currently allows non-standard symbols and does not enforce a
full 9-character alphanumeric pattern or check digit validation. Replace the
regex with /^[A-Z0-9]{9}$/ to strictly match 9 alphanumeric characters only.
Remove the characters *, @, and # from the allowed set. Improve the
name-extraction logic by making the delimiter pattern configurable or by
implementing a parser that handles all known suffix patterns in descriptions
instead of splitting only on /XXX|0%/. Additionally, add unit tests that cover
valid and invalid CUSIP cases and various description formats to ensure
robustness.
|
3 participants
Fixes
Checklist
package.jsonRelated issue (if applicable)
Fixes #190