Skip to content

Fix ATAS locale-formatted number parsing#164

Draft
cursor[bot] wants to merge 1 commit into
mainfrom
cursor/critical-bug-investigation-4c6a
Draft

Fix ATAS locale-formatted number parsing#164
cursor[bot] wants to merge 1 commit into
mainfrom
cursor/critical-bug-investigation-4c6a

Conversation

@cursor

@cursor cursor Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix ATAS import parsing for locale-formatted PnL, prices, and signed volumes.
  • Reuse the existing localized number parser so values like 1 234,56 € are saved as 1234.56 instead of being truncated.
  • Skip ATAS rows with unparseable required numeric fields instead of saving corrupted trade economics.
  • Add focused parser coverage for European currency and comma-decimal price formats.

Bug and Impact

French/European ATAS workbook exports can produce strings such as 1 234,56 €. The ATAS importer stripped commas before parseFloat, turning that value into 1 and saving incorrect PnL. The same parsing style affected price and volume fields, which could corrupt saved trade prices/quantities and side detection.

Root Cause

The recent XLSX parsing path emits formatted strings (raw: false), but atas-processor.tsx still used US-centric comma stripping and parseFloat.

Validation

  • node --loader ts-node/esm --experimental-specifier-resolution=node --test lib/ninjatrader-number-parser.test.ts
  • npm run typecheck
Open in Web View Automation 

Co-authored-by: Hugo Demenez <hugodemenez@users.noreply.github.com>
@vercel

vercel Bot commented May 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
deltalytix Ready Ready Preview, Comment May 28, 2026 11:11am

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Automated Code Review

Findings

  • High – Numeric cell values now fail parsing and can silently drop valid trades
    In lib/ninjatrader-number-parser.ts, parseLocalizedNumber rejects non-string inputs (typeof rawValue !== 'string').
    In app/[locale]/dashboard/components/import/atas/atas-processor.tsx, it is now used for quantity, pnl, price, and close volume; on parse error, rows are skipped (shouldSkipTrade = true / return).
    If spreadsheet parsing yields numbers (common for XLSX numeric cells), previously valid rows (handled via parseFloat(String(...))) will now be skipped. This is a behavioral regression and data-loss risk during import.

  • Medium – Error handling is inconsistent for side detection parsing
    originalOpenVolume / originalCloseVolume use parseLocalizedNumber(...).value without checking .error, unlike quantity/PNL/price paths.
    This can mask malformed values and default side inference to "long" unexpectedly, reducing import correctness and debuggability.

  • Low – Test coverage misses regression case for numeric inputs
    Added tests in lib/ninjatrader-number-parser.test.ts cover localized string formats well, but not numeric (non-string) input handling. A test asserting behavior for raw numeric values (e.g. 1234.56) would likely catch the regression above.

Open Questions / Assumptions

  • Assumed csvData may contain numeric values (not only strings), which is typical when parsed from Excel sources.
  • Assumed skipping a trade on parse failure is intended for malformed input, but not intended for valid numeric inputs.

Review Summary

The PR improves locale-aware number parsing and adds helpful tests, but introduces a critical import regression: numeric cell values can now be treated as invalid and skipped. Fixing parseLocalizedNumber to accept number | string (or coercing via String(rawValue) before validation) and adding numeric-input tests would make this merge-safe.


Generated by Cursor CLI workflow

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