feat: add treasury portfolio page#32
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds a new ChangesTreasury Portfolio Feature
Dashboard Back Link Rollout
Docs and Mobile Table Styling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PortfolioPage
participant AuthSession
participant TreasurySnapshot
User->>PortfolioPage: request /portfolio
PortfolioPage->>AuthSession: serialize session and check permissions
AuthSession-->>PortfolioPage: session state
PortfolioPage->>TreasurySnapshot: getTreasuryBalanceSnapshot()
TreasurySnapshot-->>PortfolioPage: snapshot or failure state
PortfolioPage-->>User: render access gate or portfolio dashboard
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new member-only Treasury Portfolio page and wires it into the existing dashboard experience, while also standardizing “back to dashboard” navigation across major subpages and tightening responsive table layouts.
Changes:
- Added a new
/portfoliopage with allocation, rebalance guidance, asset/account breakdowns, and an interactive allocation chart. - Introduced a reusable
DashboardBackLinkcomponent and refactored multiple dashboard subpages to use it. - Updated responsive table CSS + added E2E coverage for the new portfolio route and overflow at a narrow viewport; expanded README and removed the old project spec doc.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/responsive-dashboard.spec.ts | Adds /portfolio to smoke checks and a 350px overflow regression test. |
| src/components/treasury/treasury-dashboard.tsx | Adds a “View Portfolio” CTA linking to /portfolio. |
| src/components/portfolio/asset-allocation-chart.tsx | New interactive pie chart + legend for allocation breakdown. |
| src/components/dashboard-back-link.tsx | New reusable “Dashboard” back-link component. |
| src/components/app-header.tsx | Adds “Portfolio” to the main DAO navigation links. |
| src/app/rips/page.tsx | Uses DashboardBackLink and adjusts layout spacing. |
| src/app/reports/page.tsx | Uses DashboardBackLink for consistent navigation. |
| src/app/raids/page.tsx | Uses DashboardBackLink for consistent navigation. |
| src/app/proposals/page.tsx | Uses DashboardBackLink for consistent navigation. |
| src/app/portfolio/page.tsx | New server-rendered portfolio page with allocation/rebalance logic and tables. |
| src/app/membership/page.tsx | Uses DashboardBackLink for consistent navigation. |
| src/app/globals.css | Tweaks responsive table grid spacing and adds overflow-prevention styling. |
| src/app/admin/treasury-accounts/page.tsx | Uses DashboardBackLink for consistent navigation. |
| src/app/admin/quarters/page.tsx | Uses DashboardBackLink for consistent navigation. |
| src/app/admin/providers/page.tsx | Uses DashboardBackLink for consistent navigation. |
| README.md | Expands product scope/architecture/permissions documentation. |
| PROJECT_SPEC.md | Removes the previous project spec document. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/globals.css (1)
328-331: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid stacking padding on
mobile-card-table-lglink cells
.mobile-card-table-lg td, thaddslg:px-4 lg:py-3, and directa.blockcells add anotherlg:px-3 lg:py-3on top. That makes those cells noticeably larger than the rest of the table; drop one layer if that isn’t intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/globals.css` around lines 328 - 331, The `mobile-card-table-lg` table cells are stacking padding between the shared `td, th` rule and the direct `a.block` cell styles, making link cells larger than the rest. Update the padding in `globals.css` so the `mobile-card-table-lg` link cells only use one padding layer, adjusting either the shared `mobile-card-table-lg td, th` rule or the direct `a.block` rule to keep spacing consistent.
🧹 Nitpick comments (3)
src/app/portfolio/page.tsx (1)
63-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate formatting helpers across files.
formatCurrencyandformatPercenthere are near-identical to the implementations insrc/components/portfolio/asset-allocation-chart.tsx(lines 20-34). Consider extracting to a shared util module to avoid drift if formatting rules change later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/portfolio/page.tsx` around lines 63 - 84, `formatCurrency` and `formatPercent` in `src/app/portfolio/page.tsx` duplicate the same formatting logic used in `asset-allocation-chart.tsx`, so extract the shared number-formatting helpers into a common utility module and have both `page.tsx` and `AssetAllocationChart` import them. Keep the existing behavior intact while centralizing the implementations to prevent drift if formatting rules change later.src/components/portfolio/asset-allocation-chart.tsx (1)
105-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutating outer
cursorvariable during render map is fragile.
cursoris incrementally mutated as a side effect inside.map, relying onArray.prototype.map's guaranteed sequential execution order. This works correctly today but is an unusual pattern for a render function; usingreduceto carry cumulative angle state would be clearer and safer against future refactors (e.g., someone reordering/parallelizing the map).♻️ Possible refactor sketch
- const visibleRows = rows.filter((row) => row.usdValue > 0); - let cursor = 0; + const visibleRows = rows.filter((row) => row.usdValue > 0); + const slices = visibleRows.reduce<{ cursor: number; items: Array<{ row: AssetAllocationChartRow; startAngle: number; endAngle: number }> }>( + (acc, row) => { + const startAngle = acc.cursor; + const endAngle = startAngle + row.percent * 3.6; + acc.items.push({ row, startAngle, endAngle }); + acc.cursor = endAngle; + return acc; + }, + { cursor: 0, items: [] }, + ).items;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/portfolio/asset-allocation-chart.tsx` around lines 105 - 167, The render logic in asset-allocation-chart.tsx mutates the outer cursor variable inside the visibleRows.map loop, which is fragile and harder to maintain. Refactor the pie-slice rendering in the AssetAllocationChart component to carry cumulative angle state explicitly instead of side effects, ideally by using reduce or a precomputed accumulator before rendering each slice. Keep the existing behavior for getPieSlicePath, getTooltipPosition, and the row.key/tooltip handlers intact while removing the cursor mutation from the render path.src/app/globals.css (1)
226-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
min-width: 0on all.container-customchildren.This applies to every direct child regardless of layout context (flex/grid/block), which could unintentionally shrink intrinsic-sized elements that aren't participating in a flex/grid overflow scenario. Given the stated intent is overflow protection, this is likely acceptable, but consider scoping it to flex/grid containers if unintended shrinking of unrelated children surfaces.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/globals.css` around lines 226 - 229, The `.container-custom > *` selector applies `min-width: 0` to every direct child, which can unintentionally affect non-flex/grid content. Update the rule in `globals.css` so the overflow-protection behavior is scoped only to the flex/grid contexts that need it, using the `.container-custom` selector and its relevant child/layout wrappers rather than all direct children.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/portfolio/page.tsx`:
- Around line 125-127: The `isOutOfRange` logic in `page.tsx` treats a zero
`safeTotalUsd` as “within tolerance,” which makes `AllocationFlag` show a
healthy state while balances are unavailable. Update the allocation state
calculation so `safeTotalUsd === 0` is handled as an unavailable/loading case
rather than falling through to `false`, and keep the
`AllocationFlag`/`rebalanceCopy` messaging consistent with that state.
- Around line 129-142: The rebalance message in portfolio/page.tsx hardcodes
wETH even though the non-stable bucket is derived from `otherUsd` and may
include any non-stable asset. Update the `rebalanceCopy` logic to reference the
actual asset(s) represented by the non-stable holdings instead of a fixed token
name, using the existing portfolio data/model in this component to determine the
label. Keep the `rebalanceCopy` branches inside the same rebalance calculation
flow, but make the buy/sell text dynamic so it accurately reflects what the
treasury actually holds.
---
Outside diff comments:
In `@src/app/globals.css`:
- Around line 328-331: The `mobile-card-table-lg` table cells are stacking
padding between the shared `td, th` rule and the direct `a.block` cell styles,
making link cells larger than the rest. Update the padding in `globals.css` so
the `mobile-card-table-lg` link cells only use one padding layer, adjusting
either the shared `mobile-card-table-lg td, th` rule or the direct `a.block`
rule to keep spacing consistent.
---
Nitpick comments:
In `@src/app/globals.css`:
- Around line 226-229: The `.container-custom > *` selector applies `min-width:
0` to every direct child, which can unintentionally affect non-flex/grid
content. Update the rule in `globals.css` so the overflow-protection behavior is
scoped only to the flex/grid contexts that need it, using the
`.container-custom` selector and its relevant child/layout wrappers rather than
all direct children.
In `@src/app/portfolio/page.tsx`:
- Around line 63-84: `formatCurrency` and `formatPercent` in
`src/app/portfolio/page.tsx` duplicate the same formatting logic used in
`asset-allocation-chart.tsx`, so extract the shared number-formatting helpers
into a common utility module and have both `page.tsx` and `AssetAllocationChart`
import them. Keep the existing behavior intact while centralizing the
implementations to prevent drift if formatting rules change later.
In `@src/components/portfolio/asset-allocation-chart.tsx`:
- Around line 105-167: The render logic in asset-allocation-chart.tsx mutates
the outer cursor variable inside the visibleRows.map loop, which is fragile and
harder to maintain. Refactor the pie-slice rendering in the AssetAllocationChart
component to carry cumulative angle state explicitly instead of side effects,
ideally by using reduce or a precomputed accumulator before rendering each
slice. Keep the existing behavior for getPieSlicePath, getTooltipPosition, and
the row.key/tooltip handlers intact while removing the cursor mutation from the
render path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36792112-facf-4dc0-bfc3-a2959ab1ccc2
📒 Files selected for processing (17)
PROJECT_SPEC.mdREADME.mdsrc/app/admin/providers/page.tsxsrc/app/admin/quarters/page.tsxsrc/app/admin/treasury-accounts/page.tsxsrc/app/globals.csssrc/app/membership/page.tsxsrc/app/portfolio/page.tsxsrc/app/proposals/page.tsxsrc/app/raids/page.tsxsrc/app/reports/page.tsxsrc/app/rips/page.tsxsrc/components/app-header.tsxsrc/components/dashboard-back-link.tsxsrc/components/portfolio/asset-allocation-chart.tsxsrc/components/treasury/treasury-dashboard.tsxtests/e2e/responsive-dashboard.spec.ts
💤 Files with no reviewable changes (1)
- PROJECT_SPEC.md
Member
Author
|
Review follow-up posted after e27569a:
Validation: pnpm lint, pnpm build, pnpm test:e2e --grep responsive dashboard smoke|350px, and git diff --check all passed locally before the commit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new reusable
DashboardBackLinkcomponent and refactors all major dashboard subpages to use it for consistent navigation back to the main dashboard. It also makes several improvements to the mobile and desktop table layouts for better readability and alignment, and updates documentation to clarify the product scope and architecture. Additionally, a new "Portfolio" link is added to the main navigation. The most important changes are grouped and summarized below:Navigation and Component Reuse
DashboardBackLinkcomponent for consistent "Back to Dashboard" navigation and refactored all relevant pages (proposals,reports,rips,membership,admin/providers,admin/quarters,admin/treasury-accounts,raids) to use it instead of duplicating link code. This improves maintainability and UI consistency. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17]UI/UX Improvements
.mobile-card-tableand.mobile-card-table-lgclasses inglobals.css. This results in better readability and alignment across devices. [1] [2] [3] [4].container-customhavemin-width: 0to prevent layout overflow issues.Navigation Structure
app-header, making it easier for users to access the portfolio view.Documentation
README.mdto include detailed product scope, architecture, and permissions sections, clarifying the app's purpose, structure, and security practices.Minor UI Cleanups
ripspage for more consistent spacing.Summary by CodeRabbit