Add Wallet portfolio scene and chart/service refactor#1768
Add Wallet portfolio scene and chart/service refactor#1768
Conversation
Introduce a new WalletPortfolioScene and WalletPortfolioSceneViewModel with PortfolioService to fetch and combine asset charts using ChartService and PriceService. Add portfolio sheet presentation to WalletNavigationStack and a trigger in WalletScene/WalletSceneViewModel (isPresentingPortfolio, onSelectPortfolio). Refactor chart-related APIs: ChartValuesViewModel gains a static priceChange factory and priceModel, ChartView switches to selecting ChartDateValue, and PerpetualPortfolioSceneViewModel updated to use the new factory. Add Balance.total computed property and use it in BalanceViewModel; add subtitle action support to WalletHeaderView with a new system image constant. Update package dependencies to include PriceService and adjust test expectations accordingly.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, dedicated portfolio scene within the wallet tab, enabling users to visualize a combined chart of their assets over various timeframes. This feature required the creation of new UI components, view models, and a service layer responsible for aggregating financial data from multiple sources. Concurrently, core charting components were refactored to enhance flexibility and data handling, and the main wallet view was updated to provide a clear entry point to this new portfolio functionality. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Wallet Portfolio feature and includes several refactorings for chart-related components. The changes are generally well-structured and follow good practices. I've identified a couple of areas for improvement: one related to simplifying the view's asynchronous logic for better robustness and another concerning a performance optimization in the portfolio calculation logic.
| let price = chart.prices | ||
| .min(by: { abs($0.timestamp - point.timestamp) < abs($1.timestamp - point.timestamp) })? | ||
| .value ?? .zero |
There was a problem hiding this comment.
Finding the closest price by iterating through all prices with .min(by:) has a time complexity of O(N) for each point in the timeline. For large charts, this can be a performance bottleneck.
Assuming chart.prices is sorted by timestamp, you can optimize this search to O(log N) by implementing a binary search to find the price with the closest timestamp. This would make the combine function significantly more efficient.
| .task { | ||
| await model.fetch() | ||
| } | ||
| .onChange(of: model.selectedPeriod) { | ||
| Task { await model.fetch() } | ||
| } |
There was a problem hiding this comment.
The combination of .task for the initial fetch and .onChange for subsequent fetches can be simplified and made more robust by using the .task(id:) modifier. This modifier automatically handles cancellation and re-running the task when the identified value changes, and it also runs the task when the view first appears. This would replace both the .task and .onChange blocks with a single, cleaner block of code.
| .task { | |
| await model.fetch() | |
| } | |
| .onChange(of: model.selectedPeriod) { | |
| Task { await model.fetch() } | |
| } | |
| .task(id: model.selectedPeriod) { | |
| await model.fetch() | |
| } |
Remove the old local PortfolioService and add a new public PortfolioService in Packages/FeatureServices/PriceService that delegates to GemAPIPortfolioService. Update WalletPortfolioSceneViewModel to accept an injected PortfolioService and use it to fetch portfolio assets, while obtaining exchange rates from PriceService to build chart values. Wire the new service through the app: add portfolioService to Environment, AppResolver, and ServicesFactory (constructed with apiService), and pass it into WalletNavigationStack when creating the view model. These changes centralize portfolio fetching via the shared PriceService package and enable DI across the app.
Render portfolio all-time high/low and introduce view model to format them. Changes: add AllTimeValueViewModel to format all-time high/low list items; update WalletPortfolioScene to display a new Section using model.allTimeValues; change WalletPortfolioSceneViewModel state from chart array to PortfolioAssets, update fetch to store the full PortfolioAssets result, add allTimeValues computed property, wire AllTimeValueViewModel, and adjust chartModel to build charts from PortfolioAssets (using price service rate with a 1.0 fallback).
Introduce an optional subtitleImage (Image?) on HeaderViewModel with a default nil implementation. Update WalletHeaderView to render model.subtitleImage when present instead of checking onSubtitleAction and using a hardcoded system image. Implement subtitleImage in WalletHeaderViewModel (now returning chartLineUptrendXyaxis) and remove the previous pnl-based switch and subtitleImageOffset property to simplify image handling.
Introduce WalletPortfolioData to bundle chart and portfolio assets, and update WalletPortfolioSceneViewModel to use it as state. Pass Wallet into the view model to use the wallet name for the navigation title. Move chart construction into fetch() (apply price rate, build ChartValuesViewModel) and set state to include the combined WalletPortfolioData. Replace the previous private chartModel helper and adjust formatter usage: WalletPortfolioSceneViewModel now creates CurrencyFormatter instances for price and percent and uses the refactored AllTimeValueViewModel which now accepts injected formatters. Update WalletNavigationStack to pass the wallet and adapt to the new AllTimeValueViewModel initializer signature.
Refactor chart header and list handling: add ChartListView and ChartListViewable protocol to centralize the chart header UI and fetching logic. Rename ChartPriceViewModel/ChartPriceView to ChartHeaderViewModel/ChartHeaderView and add optional headerValue to support showing a primary header amount. Update ChartValuesViewModel to carry headerValue and provide header view models; adapt ChartView and CandlestickChartView to use ChartHeaderView. Make scene view models conform to ChartListViewable, wire ObservableQuery for live assets/total in WalletPortfolioSceneViewModel, and replace inline list headers with ChartListView in several scenes. Update testkit and tests (add ChartHeaderViewModelTests, update mocks), and remove the old ChartPriceViewModelTests. Also remove the now-unused assets parameter from WalletNavigationStack.
Clean up unused imports and dead code: remove the `Style` import from several Swift files and the `Charts` import from ChartScene.swift, and delete unused localized title properties (`emptyTitle`, `errorTitle`) from ChartSceneViewModel. This reduces compiler warnings and tidies MarketInsight, Perpetuals, and WalletTab feature modules.
1 participant
Introduce a new WalletPortfolioScene and WalletPortfolioSceneViewModel with PortfolioService to fetch and combine asset charts using ChartService and PriceService. Add portfolio sheet presentation to WalletNavigationStack and a trigger in WalletScene/WalletSceneViewModel (isPresentingPortfolio, onSelectPortfolio). Refactor chart-related APIs: ChartValuesViewModel gains a static priceChange factory and priceModel, ChartView switches to selecting ChartDateValue, and PerpetualPortfolioSceneViewModel updated to use the new factory. Add Balance.total computed property and use it in BalanceViewModel; add subtitle action support to WalletHeaderView with a new system image constant. Update package dependencies to include PriceService and adjust test expectations accordingly.
closes #1705