fix: Ghostfolio v3 compat, date preservation, rmSync safety - #334
fix: Ghostfolio v3 compat, date preservation, rmSync safety#334dominatos wants to merge 8 commits into
Conversation
- Auth: use POST /api/v1/auth/anonymous with accessToken body (Ghostfolio v3+)
- Dates: map timeUTC header to time so timestamps are preserved
- rmSync: use { force: true } to avoid ENOENT when input already cleaned
|
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:
WalkthroughThe converter normalizes Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ghostfolioService.ts (1)
55-55: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInfinite loop due to postfix increment operator.
The postfix increment (
retryCount++) evaluates to the current value ofretryCountbefore incrementing it. As a result, the recursive calls always receive the original value (e.g.,0), meaning the termination conditionretryCount === 3is never met. This results in an infinite recursion loop if authentication continuously fails.Replace
retryCount++withretryCount + 1to correctly increment the counter for the next recursive call.
src/ghostfolioService.ts#L55-L55: ReplaceretryCount++withretryCount + 1in thevalidatemethod.src/ghostfolioService.ts#L110-L110: ReplaceretryCount++withretryCount + 1in theimportmethod.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ghostfolioService.ts` at line 55, Replace the postfix retryCount++ argument with retryCount + 1 in both the validate method at src/ghostfolioService.ts:55-55 and the import method at src/ghostfolioService.ts:110-110, ensuring recursive calls advance the retry counter and reach the termination condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ghostfolioService.ts`:
- Around line 137-144: Update the authentication flow around bearerResponse and
cachedBearerToken to verify bearerResponse.ok before parsing its JSON. Handle
unsuccessful responses by raising or propagating an authentication error, and
only assign bearer.authToken after confirming the response succeeded and
contains a valid token.
---
Outside diff comments:
In `@src/ghostfolioService.ts`:
- Line 55: Replace the postfix retryCount++ argument with retryCount + 1 in both
the validate method at src/ghostfolioService.ts:55-55 and the import method at
src/ghostfolioService.ts:110-110, ensuring recursive calls advance the retry
counter and reach the termination condition.
🪄 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: fa754b7e-af6c-4526-b851-cb57d429578f
📒 Files selected for processing (3)
src/converters/abstractconverter.tssrc/ghostfolioService.tssrc/watcher.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
- ghostfolioService: POST auth, response.ok check, retryCount termination - abstractconverter: timeUTC → time header mapping for Trading212 - Bump GitVersion to 0.34.2
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/abstractconverter.test.ts (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpose the protected method via the test subclass to avoid
anycasting.Using
(converter as any).processHeaders(...)circumvents TypeScript's type checking. SinceTestConverteris already a dedicated test subclass, you can safely expose the protected method through a public wrapper. This maintains type safety if the signature ofprocessHeadersever changes.You can then update the test cases to use
converter.exposeProcessHeaders(csvContent).♻️ Proposed refactor
class TestConverter extends AbstractConverter { constructor(securityService: SecurityService) { super(securityService); } isIgnoredRecord(record: any): boolean { return false; } processFileContents(input: string, successCallback: CallableFunction, errorCallback: CallableFunction): void { // not needed for header tests } + + public exposeProcessHeaders(csvFile: string, splitChar = ","): string[] { + return this.processHeaders(csvFile, splitChar); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/abstractconverter.test.ts` around lines 5 - 16, Expose AbstractConverter’s protected processHeaders method through a public exposeProcessHeaders wrapper on TestConverter, preserving its typed signature and forwarding arguments. Update the header tests to call converter.exposeProcessHeaders(csvContent) instead of casting converter to any and invoking processHeaders directly.src/ghostfolioService.test.ts (2)
113-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid filesystem side-effects and cross-platform issues by mocking
fs.Hardcoding
/tmp/can cause tests to fail on Windows environments where this directory might not exist. Additionally, if theexpectassertion fails, the test will abort before reachingfs.unlinkSync, leaving a lingering file on the disk.Consider mocking
fs.readFileSyncto avoid filesystem side-effects entirely, which removes the need for file creation and deletion.♻️ Proposed refactor
- const tmpFile = "/tmp/test-validate.json"; - fs.writeFileSync(tmpFile, JSON.stringify({ activities: [] })); + const tmpFile = "dummy-test-validate.json"; + const fsSpy = jest.spyOn(fs, "readFileSync").mockReturnValue(JSON.stringify({ activities: [] })); // Mock fetch: auth always succeeds, validate always returns 401 mockFetch.mockImplementation(async (url: string) => { if (url.includes("/auth/anonymous")) { return { ok: true, json: async () => ({ authToken: "token" }) }; } // validate endpoint - always 401 to trigger retry return { ok: true, status: 401, json: async () => ({}) }; }); // Act & Assert - should throw after 3 retries (retryCount 0→1→2→3) await expect(service.validate(tmpFile, 0)) .rejects.toThrow("Failed to validate export file because of authentication error"); // Auth should have been called 3 times (retryCount 0→1→2, then 3 throws before auth) const authCalls = mockFetch.mock.calls.filter( (call: any) => call[0].includes("/auth/anonymous") ); expect(authCalls.length).toBe(3); // Cleanup - fs.unlinkSync(tmpFile); + fsSpy.mockRestore();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ghostfolioService.test.ts` around lines 113 - 137, Update the validate retry test to mock fs.readFileSync with the expected export contents instead of creating a hardcoded /tmp/test-validate.json file. Remove the writeFileSync and unlinkSync setup/cleanup while preserving the existing authentication retry assertions and validate expectation.
32-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover mock setup and comments.
The second
mockFetch.mockResolvedValueOnceand the comments above it are remnants of an older test strategy that tried to triggerauthenticateviavalidate. Sinceauthenticate(true)is now called directly and only makes a singlefetchrequest, this setup is unused and can be confusing.♻️ Proposed refactor
- // Act - trigger authenticate via validate (which calls authenticate on 401) - // First call: validate gets 401, triggers authenticate - mockFetch.mockResolvedValueOnce({ - ok: true, - status: 201, - json: async () => ({}) - }); - - // We need to call validate to trigger authenticate - // But validate reads a file, so let's test authenticate indirectly - // by checking the fetch call format -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ghostfolioService.test.ts` around lines 32 - 43, Remove the unused second mockFetch.mockResolvedValueOnce setup and the outdated comments describing indirect authenticate triggering in the affected test. Keep the direct authenticate(true) invocation and its single fetch mock unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/abstractconverter.test.ts`:
- Around line 5-16: Expose AbstractConverter’s protected processHeaders method
through a public exposeProcessHeaders wrapper on TestConverter, preserving its
typed signature and forwarding arguments. Update the header tests to call
converter.exposeProcessHeaders(csvContent) instead of casting converter to any
and invoking processHeaders directly.
In `@src/ghostfolioService.test.ts`:
- Around line 113-137: Update the validate retry test to mock fs.readFileSync
with the expected export contents instead of creating a hardcoded
/tmp/test-validate.json file. Remove the writeFileSync and unlinkSync
setup/cleanup while preserving the existing authentication retry assertions and
validate expectation.
- Around line 32-43: Remove the unused second mockFetch.mockResolvedValueOnce
setup and the outdated comments describing indirect authenticate triggering in
the affected test. Keep the direct authenticate(true) invocation and its single
fetch mock unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 819f4296-efd2-429b-8e5f-91a027a8ac97
📒 Files selected for processing (3)
GitVersion.ymlsrc/abstractconverter.test.tssrc/ghostfolioService.test.ts
…ock fs instead of temp file, remove unused mocks
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ce schema requirements
feat: auto-detect Ghostfolio version, fall back to pre-v3 GET if POST…
1 participant
Fixes three bugs that break the converter with Ghostfolio v3.30.0+ and Trading212 CSV exports.
Problem
GET /api/v1/auth/anonymous/{secret}toPOST /api/v1/auth/anonymouswith{ accessToken }body. The old GET format returns 404, which triggers infinite retry loops that exhaust the 4GB heap (OOM crash).processHeaders()camelizesTime (UTC)totimeUTC, but the converter accessesrecord.time. Sincerecord.timeis undefined,dayjs(undefined)falls back to today's date for every activity. This means buy/sell/dividend dates are all lost.fs.rmSync(filePath)throws ENOENT when the orchestrator script has already removed the input file before the container finishes processing.retryCount++(postfix) passes the original value to recursive calls, so theretryCount === 3termination is never reached.authenticate()doesn't checkresponse.okbefore parsing JSON, so a failed auth (wrong secret, network error) silently setscachedBearerTokento undefined.Changes
src/ghostfolioService.tsretryCount + 1src/converters/abstractconverter.tstimeUTCheader totimeinprocessHeaders()src/watcher.ts{ force: true }on bothrmSynccallsTesting
Verified against a Trading212 CSV with 970 rows (buys, sells, dividends, interest):
Added
Fixes
Checklist
Related issue (if applicable)
Fixes #..