Skip to content

fix(fx): skip all CASH trades in auto-convert accounts#170

Merged
GeiserX merged 1 commit into
mainfrom
fix/fx-autoconvert-skip-manual-cash
May 22, 2026
Merged

fix(fx): skip all CASH trades in auto-convert accounts#170
GeiserX merged 1 commit into
mainfrom
fix/fx-autoconvert-skip-manual-cash

Conversation

@GeiserX
Copy link
Copy Markdown
Owner

@GeiserX GeiserX commented May 22, 2026

Summary

  • In auto-convert accounts (AFx/FXCONV markers detected), manual CASH trades on IDEALFX are cleanup conversions of leftover USD — not independent FX events
  • Previously these created orphan disposals with UNKNOWN lot IDs and gain=0, confusing users (showed "1146.90 EUR" FX transmission value with zero net gain)
  • Now ALL CASH-category trades are skipped when detectAutoConvert() returns true, eliminating UNKNOWN lots entirely

Context

User elmasvital reported that monodivisa toggle had no effect and UNKNOWN lot IDs appeared. Their account is a hybrid: auto-convert for stock purchases (AFx markers) but they also manually sold USD back on IDEALFX. The manual sells found no lots (the corresponding buys were all AFx, which we correctly skip), producing confusing zero-gain entries.

Test plan

  • Verified with real user XML (hybrid account): FX disposals → 0, no UNKNOWN
  • 1047 tests pass, including the non-autoconvert FX gains test (manual-only accounts still produce correct FX events)
  • Type check clean, lint clean

Summary by CodeRabbit

Bug Fixes

  • Fixed foreign exchange event generation in auto-convert accounts to skip all cash-related trades when auto-conversion is detected, preventing duplicate FX events from automatic currency conversions.

Review Change Stack

In auto-convert accounts (AFx/FXCONV markers present), manual CASH
trades on IDEALFX are cleanup conversions of leftover USD from
dividends/commissions. The underlying FCY was created by the
auto-convert mechanism, never tracked as a lot. Processing these
manual sells creates orphan disposals with UNKNOWN lot IDs and
gain=0, confusing users (e.g. showing "1146.90 EUR" transmission
value with zero net gain).

Fix: when detectAutoConvert() returns true, skip ALL CASH-category
trades in extractFxEvents(), not just those with AFx/FXCONV markers.
This eliminates UNKNOWN lots entirely for auto-convert accounts.

Verified with real user data (hybrid account with both AFx and manual
CASH trades): FX disposals drop to 0, matching competitor output.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 20157e0d-415a-4914-82eb-f6a252e1836b

📥 Commits

Reviewing files that changed from the base of the PR and between b62eaf8 and dc1e7bb.

📒 Files selected for processing (2)
  • src/engine/fx-fifo.ts
  • tests/engine/fx-fifo.test.ts

📝 Walkthrough

Walkthrough

When autoConvert is true, FxFifoEngine.extractFxEvents now skips all CASH trades before checking for FX-conversion markers. Test cases verify that auto-convert suppression produces zero extracted FX events, including manual conversions and mixed scenario trades.

Changes

Auto-convert CASH-trade suppression

Layer / File(s) Summary
CASH-trade auto-convert suppression
src/engine/fx-fifo.ts, tests/engine/fx-fifo.test.ts
When autoConvert is true, all CASH trades are skipped in extractFxEvents before FX-conversion checks. Two test cases updated to assert zero FX events are generated in auto-convert scenarios, including manual conversions and mixed trades.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • GeiserX/DeclaRenta#131: Strengthens autoConvert detection logic and threads it into extractFxEvents, where the main PR then applies the CASH-trade suppression.
  • GeiserX/DeclaRenta#161: Lightyear parser emits non-EUR FX conversions as CASH trades, which trigger the main PR's auto-convert CASH-trade skip path.
  • GeiserX/DeclaRenta#124: Prior FxFifoEngine auto-convert logic adjustments affecting CASH-trade handling and FX-event suppression.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and directly describes the main change: skipping all CASH trades in auto-convert accounts, which is the core fix implemented in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/fx-autoconvert-skip-manual-cash

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.61%. Comparing base (89700cc) to head (dc1e7bb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #170   +/-   ##
=======================================
  Coverage   97.61%   97.61%           
=======================================
  Files          38       38           
  Lines        7786     7787    +1     
  Branches     1594     1595    +1     
=======================================
+ Hits         7600     7601    +1     
  Misses        185      185           
  Partials        1        1           
Files with missing lines Coverage Δ
src/engine/fx-fifo.ts 99.17% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GeiserX GeiserX merged commit 7eb3ac5 into main May 22, 2026
5 checks passed
@GeiserX GeiserX deleted the fix/fx-autoconvert-skip-manual-cash branch May 22, 2026 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant