Move Trading CRUD from Beta sidebar to Settings, consolidate Portfolio#214
Merged
Conversation
Trading Accounts (Beta) and Portfolio displayed the same five-account list under two top-level entries, with Trading Accounts re-rendering an aggregate equity banner + per-card sparklines that duplicated Portfolio. Clicking a Trading Accounts card sent users to Portfolio drill-in instead of broker config, blurring the "manage connections" vs "view financial state" line. Disentangle along verbs: - Settings → Trading: broker connection CRUD. UTACard click opens EditUTADialog (was: nav to /uta/:id). Drops PortfolioBanner + sparkline + curve polling; keeps a single per-card equity number as a connection-liveness signal (60s poll vs old 30s). - Portfolio (Beta): aggregate equity + per-UTA drill-in unchanged. Empty state now points to Settings → Trading with an explicit jump button. - Trading-as-Git (Beta): unchanged — orthogonal ops surface. EditUTADialog gains an optional onViewInPortfolio link rendered in the header, so users in config-mode can jump to financial state for the same account without going back through the sidebar. Subsection-header rule for sidebars codified in sections.tsx jsdoc: use headers only when listing items of more than one shape (Portfolio's aggregate + per-instance), not for symmetry across sidebars. Removes the `trading-accounts` ActivitySection / Page / sidebar entry; adds /trading-accounts → /settings/trading redirect. No persistence compat — bump and clear.
Before: fresh-load / deep-link / back-forward to a deep URL only opened the tab; selectedSidebar stayed at whatever was persisted (or null on first run), leaving the secondary sidebar column blank. First-time users landing on /inbox saw ActivityBar + main panel with empty middle space — no left-rail context. This got more visible after Portfolio became a more common bookmark target post-IA- refactor. Add specToSection(spec) and call setSidebar inside useAdopt so URL- driven navigation lands with the matching sidebar already open. Only fires from real URL events (mount + popstate); silent UrlSync writes via replaceState don't re-trigger it, so explicit user sidebar choices still win. uta-detail maps to portfolio (not settings) — its URL lives under /settings/uta/:id for historical reasons but the page is a Portfolio drill-in.
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
2 participants
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.
Summary
Reorganizes the Trading Accounts UI surface: moves broker connection CRUD (add/edit/delete) from a dedicated Beta sidebar into Settings → Trading, and removes portfolio-level aggregates (equity banner, sparklines, 24h deltas) from the Trading page. Those aggregates now live exclusively in Portfolio, where they belong as state/analytics surfaces rather than configuration surfaces.
Key Changes
Settings integration: Trading Accounts CRUD now lives under Settings → Trading (alongside General, AI Provider, etc.) instead of a separate Beta sidebar entry. The
TradingAccountsBetaSidebarcomponent is removed.EditUTADialog promotion: The edit dialog is now the primary CRUD surface for the Trading settings category, mounted directly from TradingPage when a user clicks a card. Adds an optional "View in Portfolio" link to switch context to the Portfolio drill-in for that account.
Portfolio consolidation: Removes
PortfolioBanner(hero equity + 24h delta),Sparklinerendering, and per-UTA equity curves from TradingPage. These now live only in PortfolioPage, which is the correct home for aggregate state and trend visualization.Simplified equity polling: TradingPage now fetches
equity()on a 60s cadence (down from 30s) purely as a liveness signal — to show "this connection returned a real account balance" on each card. Trend data, sparklines, and aggregate PnL are Portfolio's responsibility.URL/sidebar alignment: UrlAdopter now maps
uta-detailspecs to the Portfolio sidebar (not a new Trading sidebar), and sets the correct sidebar context on URL-driven navigation (fresh load, deep link, back-forward). Adds a redirect from/trading-accountsto/settings/tradingfor backward compatibility.Empty state UX: Portfolio's "no accounts" empty state now includes a button that navigates directly to Settings → Trading to add a broker connection.
Implementation Details
EditUTADialognow accepts an optionalonViewInPortfoliocallback; when provided (from Settings), it renders a header link to switch to the Portfolio detail view for that account.summarizeCurve()and related curve-aggregation logic; equity curves are now computed only in PortfolioPage.Metric,Sparkline,fmtPnl,fmtPctSignedfrom TradingPage; these are Portfolio-only concerns.alsoActiveForlogic needed).This change clarifies the responsibility split: Settings is for configuration (CRUD), Portfolio is for state and analytics (read-heavy, trend-focused).
https://claude.ai/code/session_01G5pNede6j7HjyaXkhQRVUy