Skip to content

[#772] Simplify Reader holdings + portfolio boxes, add trade history#778

Merged
realproject7 merged 3 commits intomainfrom
task/772-reader-cost-pnl
Apr 2, 2026
Merged

[#772] Simplify Reader holdings + portfolio boxes, add trade history#778
realproject7 merged 3 commits intomainfrom
task/772-reader-cost-pnl

Conversation

@realproject7
Copy link
Copy Markdown
Owner

Summary

  • Holdings cards: 4-box grid → 2 boxes (Value with cost-basis % change, Balance). Removed PnL and First Traded boxes
  • Trade history: Added HoldingRecentTrades component showing last 5 buy/sell transactions below each holdings card
  • Portfolio dashboard: 4-box grid → 2 boxes (Value in USD with portfolio-level cost-basis % change, Holdings count). Removed PLOT and Best 24h boxes

Fixes #772

Test plan

  • Holdings cards show 2 boxes: Value (with cost % change) and Balance
  • Recent 5 transactions appear below each holdings card
  • Portfolio dashboard shows 2 boxes: Value (with portfolio % change) and Holdings
  • PnL, First Traded, PLOT, Best 24h boxes are gone
  • npm run build passes

🤖 Generated with Claude Code

Holdings cards: replace 4-box grid with 2 boxes (Value with cost-basis
% change, Balance). Remove PnL and First Traded boxes. Add recent 5
transactions list below each card via HoldingRecentTrades component.

Portfolio dashboard: replace 4-box grid with 2 boxes (Value in USD
with portfolio-level cost-basis % change, Holdings count). Remove
PLOT and Best 24h boxes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 2, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
plotlink Ignored Ignored Apr 2, 2026 7:56pm

Request Review

Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

T2b APPROVE

Clean simplification matching issue scope:

  • Portfolio: 4→2 boxes. Cost-basis % calculation correctly aggregates current vs entry across all holdings.
  • Holdings: 4→2 boxes. Value now shows cost-basis % instead of 24h price change — more meaningful for individual positions.
  • HoldingRecentTrades: Good component isolation. Query is well-scoped (per address+storyline, limit 5, 60s stale time). Gracefully returns null when loading/empty. Buy/sell mapped from mint/burn event types.

No concerns.

Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Summary

The simplified Reader layout is close, but the new portfolio-level cost-basis percentage can be materially wrong for users who hold tokens without a tracked entry price.

Findings

  • [medium] portfolioCostPct adds every holding's current USD value into the numerator, but only adds holdings with a known entryPrice into the cost basis denominator. That means any transferred / legacy / otherwise untracked holding is treated as having zero cost and inflates the portfolio percentage, even though the per-holding cards correctly hide the percent when cost basis is unknown.
    • File: src/app/profile/[address]/page.tsx:1450
    • Suggestion: When entryPrice is missing, exclude that holding from both totalCurrentUsd and totalCostUsd for the portfolio percentage calculation, or return null unless all included holdings have a cost basis.

Decision

Requesting changes because the current portfolio-level % can be misleadingly high and does not correctly represent total cost-basis performance.

…folio cost %

Only include holdings with known entryPrice in both current value and
cost basis calculations, preventing transferred/untracked holdings
from inflating the portfolio percentage.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Summary

The portfolio percentage calculation no longer treats unknown-cost holdings as zero-cost, but the summary card still mixes two different populations in the same metric display.

Findings

  • [medium] The portfolio summary still renders Value from totalValue, which includes all holdings, while portfolioCostPct now includes only holdings with a known entryPrice. That means the UI can show a total portfolio dollar value for all positions with a percentage that only describes a subset of them, which is still misleading for mixed portfolios.
    • File: src/app/profile/[address]/page.tsx:1443
    • File: src/app/profile/[address]/page.tsx:1473
    • Suggestion: Either compute the displayed USD value from the same subset used for portfolioCostPct, or suppress the % unless the displayed value and cost basis are derived from the same set of holdings.

Decision

Requesting changes because the summary card still pairs a full-portfolio dollar value with a partial-portfolio percentage, which is not a coherent cost-basis metric.

…ave entry prices

Portfolio cost % is now hidden unless every holding has a known entry
price, ensuring the displayed percentage always describes the full
Value amount shown.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: APPROVE

Summary

The follow-up change resolves the remaining portfolio summary issue by hiding the cost-basis percentage unless all holdings have a known entry price, so the displayed % now always describes the same full portfolio value shown on the card.

Findings

  • None.

Decision

Approving because the Reader holdings simplification, recent-trades list, and portfolio summary now match issue #772 without the misleading mixed-population percentage from the earlier revision.

@realproject7 realproject7 merged commit 86537fc into main Apr 2, 2026
5 checks passed
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.

[Enhancement] Reader tab: simplify holdings + portfolio boxes, add trade history

2 participants