Skip to content

fix(accounts): clamp breached balances at accepted drawdown floor - #246

Closed
hugodemenez wants to merge 3 commits into
betafrom
cursor/issue-86-accepted-loss-floor-eb88
Closed

hugodemenez wants to merge 3 commits into
betafrom
cursor/issue-86-accepted-loss-floor-eb88

Conversation

@hugodemenez

@hugodemenez hugodemenez commented Jun 24, 2026 •

Copy link
Copy Markdown
Owner

Problem

Fixes #86 — after a prop-firm account breaches its drawdown, balances could keep displaying losses below the accepted loss floor, and continued trading after a bust was ambiguous.

Changes

Drawdown floor display

  • Extract shared drawdown helpers in lib/account-drawdown.ts
  • Clamp displayed balances at the drawdown floor across metrics, daily stats, and charts
  • Propagate clamped balance into subsequent days/events for consistency

Post-breach lifecycle

  • Add bursted and breachDate fields on Account
  • Detect when trades exist after a raw drawdown breach (lib/account-breach.ts)
  • Prompt the user via AccountBreachPrompt:
    • Yes, set reset date → persists resetDate, clears bursted
    • No, mark as bursted → persists bursted + breachDate, metrics ignore post-breach trades
    • Ask me later → dismisses until next session
  • Show bursted state on account card and table

Quality

  • Unit tests for drawdown floor, breach detection, and bursted trade filtering
  • bun run test script + GitHub Actions test workflow

Testing

  • bun run test (29 tests)
  • bun run typecheck
  • OPENAI_API_KEY=dummy bun run build

Migration

Run bunx prisma db push (or deploy migration) to add bursted and breachDate columns.

Open in Web Open in Cursor 

Clamp displayed balances at the drawdown floor across account metrics,
daily stats, and equity charts so post-breach losses are not over-reported.

- Extract shared drawdown helpers in lib/account-drawdown.ts
- Apply floor clamp in computeAccountMetrics and dailyMetrics
- Align trade-progress-chart, account-equity-chart, and account-table
- Add unit tests and vitest path alias config

Closes #86

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

vercel Bot commented Jun 24, 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 Jun 24, 2026 10:23pm

@cursor cursor Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Not approving: Cursor Bugbot is not configured on this PR, and the completed cursor-review automation flagged unresolved medium findings (inconsistent post-breach balance propagation across charts vs metrics). Human verification is needed before merge. No reviewers assigned — the only repo collaborator is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

Address PR review feedback:
- Feed clamped balance into the next day/event in daily metrics and
  equity chart, matching trade-progress-chart behavior
- Add propagation regression test for post-breach daily balances
- Add bun run test script and GitHub Actions test workflow

Co-authored-by: Hugo Demenez <hugodemenez@users.noreply.github.com>
cursor[bot]
cursor Bot approved these changes Jun 24, 2026 •
When drawdown is breached and later trades exist, prompt the user to either
set a reset date for a new evaluation or mark the account as bursted.

- Add bursted and breachDate fields to Account schema
- Detect post-breach trades with lib/account-breach.ts
- Exclude post-breach trades from metrics when bursted
- Add AccountBreachPrompt dialog in accounts overview
- Show bursted state on account card and table

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

Copy link
Copy Markdown
Contributor

🤖 Automated Code Review

PR Review Summary

This PR adds prop-firm drawdown handling: balance clamping at the drawdown floor, persisted bursted / breachDate fields, a user prompt when trades continue after breach, and solid unit test coverage with a new CI workflow. The refactor into lib/account-drawdown.ts and reuse across metrics/charts is a good direction.


Critical

1. Missing Prisma migration

schema.prisma adds bursted and breachDate, but the diff has no prisma/migrations/ SQL file. Deployments that use migrations (not only db push) will fail or silently miss these columns until a migration is added.

2. Breach detection ignores payouts

detectAccountBreach walks trades only; computeAccountMetrics subtracts payouts when computing balance. A payout-driven breach (no post-breach trades) would not trigger the confirmation prompt, while metrics would still show a breach. Breach detection should use the same balance model as metrics (including payouts).

3. Silent save failures in AccountBreachPrompt

handleMarkBursted and handleConfirmReset use try/finally with no catch. If saveAccount throws, the dialog closes and the user gets no feedback. Add error handling (toast + keep dialog open).


Medium

4. Impure useMemo + sessionStorage

nextPrompt reads sessionStorage inside useMemo without it in deps. Dismiss works via local state, but the memo can be stale until accounts/trades change. Prefer useState/useEffect for dismiss tracking, or a dedicated dismiss state in deps.

5. No validation on reset date

The calendar allows any date. Users can pick a date before the breach or unrelated to the first post-breach trade. Constrain to >= firstPostBreachTradeDate (or >= breachDate) and disable invalid dates.

6. bursted without breachDate is a no-op for filtering

filterTradesForBurstedAccount returns all trades when bursted is true but breachDate is missing. Post-breach trades stay in metrics until both are set. Consider requiring breachDate when marking bursted, or deriving it from detection.

7. markBursted does not clear resetDate

Confirming reset clears breachDate; marking bursted leaves an existing resetDate, which can leave inconsistent account state.


Minor / Best Practices

8. Error handling & UX

  • Dismiss is session-only (sessionStorage); users are re-prompted each new session — likely intentional, but worth documenting.
  • Only the first breached account in array order is prompted; others wait — acceptable, but consider queueing.

9. Performance

  • AccountBreachPrompt runs detectAccountBreach for every account over the full trades array on each change. Fine for typical use; consider scoping trades per account if this grows.

10. CI workflow

  • New .github/workflows/test.yml is good. Pinning bun-version (instead of latest) would improve reproducibility.

11. TypeScript / React

  • Tests use as any — acceptable in tests, but typed fixtures would be stronger.
  • AlertDialogCancel + custom onClick={dismissPrompt} may double-fire with default close behavior — verify in UI.
  • Drawdown logic extraction and account-table.tsx using precomputed metric.totalBalance are solid refactors.

12. Security

  • No new auth concerns; persistence goes through existing setupAccountAction + getUserId(). No sensitive data in sessionStorage.

What looks good

  • Shared computeDrawdownLevel / clampBalanceAtDrawdownFloor used in metrics, trade progress chart, and equity chart.
  • Unit tests for drawdown floor, trailing drawdown, and breach detection edge cases.
  • i18n for EN/FR.
  • UI surfaces bursted state on cards and table views.
  • account-table.tsx deduplication by using dailyMetrics.totalBalance instead of recomputing running balance inline.

Verdict

Do not merge until a Prisma migration is added and payout-aware breach detection is aligned with metrics (or explicitly documented as trade-only). Add save error handling before shipping the prompt UX. The core drawdown clamping and test infrastructure are well done.


Generated by Cursor CLI workflow — 82ad8e7e3c2fe7147fa80b1184f090444bc55d12 feat(accounts): prompt for reset or bursted after post-breach trades · 0 review(s) remaining

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not approving: Cursor Bugbot is not configured on this PR, and the latest cursor-review on head (82ad8e7) reports critical findings (missing Prisma migration, payout-aware breach detection, silent save failures). Human verification is needed before merge. No reviewers assigned — the only repo collaborator is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

This branch was successfully deployed

1 active deployment
Preview — 82ad8e7e Deployed Jun 24, 2026 by vercel[bot]
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.

2 participants