Skip to content

Add return breakdown dialog with PnL history chart - #61

Open
dbigham wants to merge 4 commits into
mainfrom
codex/add-clickable-annualized-return-details-and-chart
Open

Add return breakdown dialog with PnL history chart#61
dbigham wants to merge 4 commits into
mainfrom
codex/add-clickable-annualized-return-details-and-chart

Conversation

@dbigham

@dbigham dbigham commented Oct 5, 2025

Copy link
Copy Markdown
Owner

Summary

  • compute trailing return metrics and P&L history on the server and expose them in the funding summary payload
  • surface the new return data in the client, including sanitization, and wire the annualized return metric to open a detailed dialog
  • introduce a return breakdown dialog with time period controls, CAD/% toggle, and styled chart matching existing visuals

Testing

  • npm run lint

https://chatgpt.com/codex/tasks/task_e_68e279c70b10832d81a0e98a22887154

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

Comment thread server/src/index.js Outdated
Comment on lines +2550 to +2738
if (aggregateTotals.cashFlowsCad.length > 0) {
const performanceSummary = buildFundingPerformanceSummary(
aggregateTotals.cashFlowsCad,
Number.isFinite(aggregateTotals.totalPnlCad) ? aggregateTotals.totalPnlCad : null,
Number.isFinite(aggregateTotals.totalEquityCad) ? aggregateTotals.totalEquityCad : null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Aggregated cash flows mis-handle per-account final equity entries

The all‑accounts summary feeds every account’s cashFlowsCad directly into buildFundingPerformanceSummary. Those per‑account arrays already contain a final positive entry representing that account’s current equity. When concatenated, all but one of those positive entries land in the “contributions” slice inside buildFundingPerformanceSummary, where they are negated and treated as withdrawals. As soon as more than one account is selected, the resulting net‑deposit totals and PnL history become incorrect (final equity for account A is subtracted from the aggregate deposits). The aggregate cash flows should either strip per‑account terminal entries and add one combined terminal balance, or the builder should ignore positive entries before the final one; otherwise the rendered return chart and trailing return numbers for “All accounts” are wrong.

Useful? React with 👍 / 👎.

@dbigham
dbigham force-pushed the main branch 3 times, most recently from c7f2145 to c8636e3 Compare November 15, 2025 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant