Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds support for a new Wealthfolio Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #8 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 493 516 +23
Branches 106 113 +7
=========================================
+ Hits 493 516 +23
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (2)
src/formats/GenericFormat.ts (1)
150-187: Consider adding "futures" as an alias for instruments.The
mapInstrumentTypeimplementation is well-structured with good normalization (trim + lowercase) and comprehensive alias coverage. The typo handling for "mutial fund" (Line 160) is a nice touch for user-friendly input tolerance.One minor consideration: futures contracts are a common instrument type that could map to either
Option(derivatives) or a separate category. If Wealthfolio'sInstrumentTypeenum doesn't have a specific futures type, you might consider adding "futures" or "future" as an alias mapping to the most appropriate existing type.tests/formats/GenericFormat.test.ts (1)
641-664: Consider adding "InstrumentType" to the schema assertion.The
getExpectedSchematest verifies expected column names but doesn't include the newly addedInstrumentTypecolumn in the assertion array.🔧 Proposed fix
expect(columnNames).toEqual( expect.arrayContaining([ "Date", "TransactionType", "TransactionSubtype", + "InstrumentType", "Symbol", "ISIN", "CUSIP",
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e69f2399-ea96-4103-97fc-517fc16edb00
⛔ Files ignored due to path filters (1)
examples/sample-generic.csvis excluded by!**/*.csv
📒 Files selected for processing (12)
ChangeLog.mdREADME.mddocs/format-plugin-development-guide.mddocs/generic-format-user-guide.mddocs/technical-information.mdsrc/core/BaseFormat.tssrc/core/FieldRequirements.tssrc/formats/GenericFormat.tstests/core/BaseFormat.test.tstests/core/Converter.test.tstests/core/FieldRequirements.test.tstests/formats/GenericFormat.test.ts
This field was added to Wealthfolio in March 2026 and is optional. - Add `instrumentType` property to `WealthfolioRecord` and corresponding enum for type safety. - Add validation for `instrumentType` field: optional when symbol is present, ignored otherwise. - Update the Generic plugin to support the new field. - Update documentation, samples, and tests.
|
1 participant
Description
instrumentTypeproperty toWealthfolioRecordand corresponding enum for type safety.instrumentTypefield: optional when symbol is present, ignored otherwise.Type of Change
Checklist
Additional Notes
This field was added to Wealthfolio in March 2026 and is optional (see afadil/wealthfolio#666).
By submitting this pull request, I confirm that I have read and understood the contribution guidelines and that my contributions are my own work.