feat: add skipped records report with reasons - #323
Open
dominatos wants to merge 2 commits into
Open
Conversation
WalkthroughAdds skipped-record tracking and reporting and updates configuration: introduces a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.sample:
- Around line 1-39: Add the missing INPUT_FILE variable back into .env.sample
because src/manual.ts still reads process.env.INPUT_FILE and passes it into
createAndRunConverter(); update .env.sample to include a descriptive line like
"INPUT_FILE=/path/to/input-file" (or an empty placeholder) and a short comment
explaining it's used by the manual entrypoint so developers don't get an
undefined path at runtime; leave this in place until src/manual.ts is changed to
stop using process.env.INPUT_FILE or createAndRunConverter() is made robust to
undefined inputs.
In `@src/converter.ts`:
- Around line 100-106: The CSV generation in converter.ts currently only quotes
fields (escapedRaw/escapedReason) but doesn't neutralize formula-injection risk
from untrusted skipped.rawLine and skipped.reason; update the logic where
escapedRaw and escapedReason are created (the block building csvLines from
skippedRecords) to first escape internal quotes, then if the resulting cell
begins with any of = + - @ prefix a single quote (') or a safe character to
neutralize spreadsheet formulas, and finally wrap in quotes as before so both
rawLine and reason are sanitized before being written to csvLines.
In `@src/converters/abstractconverter.ts`:
- Around line 12-13: The skippedRecords accumulator (private skippedRecords:
SkippedRecord[]) is never reset between runs, so getSkippedRecords() returns
stale entries; fix by clearing this.skippedRecords = [] at the start of the
conversion entry point(s) (e.g., in AbstractConverter.convert / the method that
begins a conversion run) so each run starts with an empty array; also ensure any
other conversion entry helper referenced around the area noted (lines ~118-124)
similarly resets skippedRecords before processing.
In `@src/converters/degiroConverterV2.ts`:
- Around line 26-27: The current use of rawLines = input.split(...) and deriving
rawLine/lineNumber from idx is incorrect when CSV fields can contain embedded
newlines; instead, modify the parsing in degiroConverterV2 so each parsed record
carries its original source slice or start/end index (do not rely on rawLines or
input.split). Update the parser (or wrap the CSV parse loop) to record for each
output row a rawText or sourceStart/sourceEnd and then use that stored
rawText/source indices for skipped-record reporting (replace uses of rawLine,
lineNumber, and idx at the current reporting sites including the later
occurrences around the logic referenced at 76-78). Ensure downstream code that
reports skipped records reads the stored per-record source info rather than
recomputing via input.split.
🪄 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: c7ec475b-4be4-4cce-bbb2-96097610f2b7
📒 Files selected for processing (5)
.env.samplesrc/converter.tssrc/converters/abstractconverter.tssrc/converters/degiroConverterV2.tssrc/models/skippedRecord.ts
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/converters/abstractconverter.ts (1)
128-129: Avoid exposing mutable internal state viagetSkippedRecords().Returning the backing array directly allows external mutation of converter internals. Prefer returning a defensive copy (and optionally a readonly type).
♻️ Suggested refinement
- public getSkippedRecords(): SkippedRecord[] { - return this.skippedRecords; - } + public getSkippedRecords(): ReadonlyArray<SkippedRecord> { + return [...this.skippedRecords]; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/abstractconverter.ts` around lines 128 - 129, getSkippedRecords currently returns the internal mutable array skippedRecords directly, allowing callers to mutate converter internals; change getSkippedRecords to return a defensive copy (e.g., return [...this.skippedRecords]) or a readonly view (e.g., return this.skippedRecords.slice() typed as ReadonlyArray<SkippedRecord>) so external code cannot modify the backing skippedRecords array inside the class (update the getSkippedRecords method in the AbstractConverter/abstractconverter implementation).
🤖 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/abstractconverter.ts`:
- Around line 128-129: getSkippedRecords currently returns the internal mutable
array skippedRecords directly, allowing callers to mutate converter internals;
change getSkippedRecords to return a defensive copy (e.g., return
[...this.skippedRecords]) or a readonly view (e.g., return
this.skippedRecords.slice() typed as ReadonlyArray<SkippedRecord>) so external
code cannot modify the backing skippedRecords array inside the class (update the
getSkippedRecords method in the AbstractConverter/abstractconverter
implementation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb1fa593-c570-4bc4-81d0-ac589839e422
📒 Files selected for processing (3)
.env.samplesrc/converter.tssrc/converters/abstractconverter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/converter.ts
When converting a CSV file, some records may be skipped because: - The record type is intentionally ignored (e.g. deposits, withdrawals) - No matching security symbol could be found - The record could not be matched to a valid transaction type - An error occurred looking up the security This change introduces a skipped records report: - Adds `SkippedRecord` model with line number, raw CSV line, and reason - Adds `addSkippedRecord()` and `getSkippedRecords()` to AbstractConverter - Updates all skip/continue paths in converters to call `addSkippedRecord()` - After conversion, if any records were skipped, writes a `skipped-records-<converter>-<timestamp>.csv` file to the output folder - Controlled by env var `GHOSTFOLIO_SHOW_SKIPPED` (default: true)
dominatos
force-pushed
the
feature/skipped-records-report
branch
from
April 6, 2026 00:04
6c0f3d2 to
aed1039
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/converters/abstractconverter.ts (1)
12-13:⚠️ Potential issue | 🟡 MinorReset
skippedRecordsat the start of each conversion run.Line 119 says this returns skips from the “last conversion run”, but
readAndProcessFile(Line 35) does not clear prior entries. If the same converter instance is reused, reports will include stale rows.💡 Suggested fix
public readAndProcessFile(inputFile: string, successCallback: CallableFunction, errorCallback: CallableFunction) { + this.skippedRecords = []; + // If the file does not exist, throw error. if (!fs.existsSync(inputFile)) { return errorCallback(new Error(`File ${inputFile} does not exist!`)); }Also applies to: 119-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/abstractconverter.ts` around lines 12 - 13, The skippedRecords array on the converter instance is not cleared between runs, causing getSkippedRecords (lines ~119-124) to return stale entries; fix this by resetting the private property skippedRecords = [] at the start of each conversion run inside readAndProcessFile (or any public method that begins a run) so every invocation begins with an empty skippedRecords collection and subsequent pushes only reflect the current run.src/converters/degiroConverterV2.ts (1)
26-27:⚠️ Potential issue | 🟠 MajorDon’t derive source rows via
input.split(/\r?\n/)+idx.This breaks as soon as a valid CSV field contains embedded newlines:
rawLineandlineNumberno longer match the actual parsed record, so skipped-record reports become inaccurate.Also applies to: 76-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/degiroConverterV2.ts` around lines 26 - 27, Don’t split the input with input.split(/\r?\n/) to compute source rows because embedded newlines in quoted CSV fields break line mapping; instead derive the row/line number from the CSV parser metadata when processing each parsed record (e.g. use the parser's info/lines or record-level metadata your CSV library exposes) in the degiro conversion flow (replace uses of rawLines and rawLines[idx] in the functions in degiroConverterV2.ts, including the earlier instance around the rawLines declaration and the later uses around the record-processing logic referenced at 76-78), and use that parser-provided line/record index in skipped-record reports so they remain correct when fields contain embedded newlines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/converters/abstractconverter.ts`:
- Around line 12-13: The skippedRecords array on the converter instance is not
cleared between runs, causing getSkippedRecords (lines ~119-124) to return stale
entries; fix this by resetting the private property skippedRecords = [] at the
start of each conversion run inside readAndProcessFile (or any public method
that begins a run) so every invocation begins with an empty skippedRecords
collection and subsequent pushes only reflect the current run.
In `@src/converters/degiroConverterV2.ts`:
- Around line 26-27: Don’t split the input with input.split(/\r?\n/) to compute
source rows because embedded newlines in quoted CSV fields break line mapping;
instead derive the row/line number from the CSV parser metadata when processing
each parsed record (e.g. use the parser's info/lines or record-level metadata
your CSV library exposes) in the degiro conversion flow (replace uses of
rawLines and rawLines[idx] in the functions in degiroConverterV2.ts, including
the earlier instance around the rawLines declaration and the later uses around
the record-processing logic referenced at 76-78), and use that parser-provided
line/record index in skipped-record reports so they remain correct when fields
contain embedded newlines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37fec02a-86ca-457e-b71b-9380e7b51688
📒 Files selected for processing (5)
.env.samplesrc/converter.tssrc/converters/abstractconverter.tssrc/converters/degiroConverterV2.tssrc/models/skippedRecord.ts
✅ Files skipped from review due to trivial changes (1)
- src/models/skippedRecord.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.sample
- src/converter.ts
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
When converting a CSV file, some records may be skipped for various reasons. This PR introduces a skipped records report to give users visibility into what was skipped and why.
Changes
SkippedRecordmodel with line number, raw CSV line, and reasonaddSkippedRecord()andgetSkippedRecords()methods toAbstractConverteraddSkippedRecord()skipped-records-<converter>-<timestamp>.csvfile to the output folderGHOSTFOLIO_SHOW_SKIPPED(default:true)Reasons a record may be skipped
Testing
Converters updated to report skipped records with descriptive reasons. Output CSV is only written when at least one record was skipped.