Fix broker trade dedupe on financial corrections

[cursor]

Bug and impact

DxFeed's recent PnL mapping correction changed the pnl and commission values used by saveTradesAction when generating deterministic trade IDs. A user who re-synced the same DxFeed history after that change could insert duplicate trades instead of hitting skipDuplicates, double-counting PnL, fees, win rate, and trade counts. Existing imported rows with older financial semantics would also remain stale on re-sync.

Root cause

saveTradesAction regenerated IDs from financial fields (pnl and commission) even for broker-synced trades that have stable external entryId/closeId values. When those financial fields changed, the same logical broker trade got a different primary key.

Fix

• Moved trade persistence identity helpers into lib/trade-persistence.ts.
• Exclude pnl/commission from generated IDs only when stable broker entry/close IDs exist; generic/manual imports keep those fields in the signature to preserve uniqueness.
• Before inserting, look up existing user trades by non-empty broker entryId/closeId/account key and update broker-derived fields instead of inserting a duplicate.
• Preserve user-owned annotation fields such as comments, images, and tags during these broker re-sync updates.
• Added focused Vitest coverage and declared the existing missing vitest dev dependency.

Validation

• npx vitest run lib/trade-persistence.test.ts lib/dxfeed-token.test.ts passed: 2 files, 7 tests.
• DIRECT_URL="postgresql://user:pass@localhost:5432/db" DATABASE_URL="postgresql://user:pass@localhost:5432/db" npx prisma generate && npm run typecheck passed after generating local Prisma types.
• npx eslint lib/trade-persistence.ts lib/trade-persistence.test.ts passed.
• npx eslint server/database.ts lib/trade-persistence.ts lib/trade-persistence.test.ts remains blocked by pre-existing unrelated lint errors in server/database.ts (no-explicit-any and unused variable locations outside this change).

  

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
deltalytix	Ready	Preview, Comment	Jun 8, 2026 7:17am

[github-actions]

🤖 Automated Code Review

PR Review Summary

This PR fixes broker trade re-import behavior: financial corrections no longer mint new rows, user annotations are preserved on update, and persistence logic is extracted into a tested module. The direction is sound and the core design is reasonable.

---

What works well

• Clear separation of concerns — lib/trade-persistence.ts is focused, documented, and unit-tested.
• Correct preservation of user data — getBrokerTradeUpdateData only updates broker-derived fields; comments, tags, and images are excluded.
• Stable broker matching — getStableBrokerTradeKey handles the legacy case where old UUIDs included pnl/commission but broker entryId/closeId are stable.
• Improved ID generation — Excluding financial fields when broker IDs exist prevents duplicate rows on PnL/commission fixes.
• normalizeKeyPart — Trimming/null handling is better than the previous || '' approach.

---

Critical / high-priority issues

1. numberOfTradesAdded ignores updates

Updates are excluded from the returned count:

    return {
      error: result.count === 0 && tradesToUpdate.length === 0 ? 'NO_TRADES_ADDED' : false,
      numberOfTradesAdded: result.count

A re-sync that only updates existing trades reports 0 trades saved. Callers (dxfeed/tradovate sync, import UI) use this for success messaging and logging, so users may see “0 trades imported” on a successful correction sync. Consider numberOfTradesAdded: result.count + tradesToUpdate.length or a separate numberOfTradesUpdated field.

2. Tests are not wired into the project

vitest is added to devDependencies, but there is no test script and no Vitest config. lib/trade-persistence.test.ts will not run in CI or via npm test as-is.

---

Medium issues

3. Over-broad findMany filter (performance + correctness risk)

The lookup uses independent IN clauses:

      const existingBrokerTrades = await prisma.trade.findMany({
        where: {
          userId,
          accountNumber: { in: candidateAccountNumbers },
          entryId: { in: candidateEntryIds },
          closeId: { in: candidateCloseIds },
        },

This matches the Cartesian product of those sets, not specific (accountNumber, entryId, closeId) tuples. Extra rows are fetched and keyed, which can hurt performance on large syncs. Keying mitigates wrong updates today, but this pattern is fragile if the logic changes. Prefer tuple-based filtering (OR of exact combinations) or a raw query.

4. Legacy duplicate broker rows

If duplicate rows already exist for the same broker key (from the old UUID behavior), existingBrokerTradesByKey.set() is last-write-wins. Only one row gets updated; others stay stale. Worth a one-time dedup migration or documenting this limitation.

5. Normalization mismatch between DB lookup and key generation

getStableBrokerTradeKey trims values, but findMany uses raw accountNumber / entryId / closeId from the import batch. Whitespace differences between stored and incoming data can cause a missed match and a duplicate insert.

6. Large update transactions

All updates run in a single prisma.$transaction with no batching. Large re-syncs could hit transaction timeouts or long lock holds. Consider chunking (e.g. 50–100 per transaction).

---

Minor / style

• getBrokerTradeUpdateData — Add an explicit return type (e.g. Prisma.TradeUpdateInput or a picked type) for safer refactors.
• Repeated getStableBrokerTradeKey calls — The same key is computed multiple times per trade in filters/updates; a single precomputed Map<trade, key> would be slightly cleaner and faster.
• Error handling — The thrown "Existing broker trade missing during update preparation" is caught by the outer handler and returned as DATABASE_ERROR, which is acceptable but opaque for debugging.

---

Security

No major concerns. Queries are scoped by userId, and updates target rows resolved from that scoped lookup. User-owned fields are intentionally excluded from broker updates.

---

Verdict

Approve with changes recommended. The main functional gap is underreporting updated trades in the API response. Wire up Vitest before relying on the new tests. The broad findMany and legacy-duplicate edge cases are worth addressing soon, but are not blockers if this is shipping to fix an active duplicate-trade bug.

Suggested test plan before merge:

1. Re-import dxfeed/tradovate trades with corrected PnL → same row updated, annotations preserved.
2. Confirm success UI/logs reflect updates, not only creates.
3. Run vitest locally once a test script is added.
4. Re-import a batch where all trades already exist → should not return NO_TRADES_ADDED.

---

Generated by Cursor CLI workflow — cc34de51a153df847365a5eeabfb14820ef27fe7 Add Vitest for test typechecking · 2 review(s) remaining