fix(portfolio): stop publishing an incomplete scan as a complete one (GH#2414) - #2438
fix(portfolio): stop publishing an incomplete scan as a complete one (GH#2414)#2438dcccrypto wants to merge 1 commit into
Conversation
…(GH#2414)
Every failure path inside usePortfolio's load() was swallowed into an
empty result, making "we could not read your positions" indistinguishable
from "you have no positions":
- the batch slab fetch caught and set slabAccountsInfo = [], which
covers EVERY market, so one RPC failure rendered as an empty portfolio
- the per-market loop caught with "Skip markets that fail to parse".
That path covers the v17 getProgramAccounts owner scan, so a failure
there hid real positions while the same positions stayed visible on
the market page — the symptom reported in the issue
- the outer catch was a bare "ignore", so a discovery failure left the
previous state rendering as a completed scan
The partial result was then published through the shared snapshot that
every usePortfolio() consumer reads, understating position count,
deposited capital, portfolio value, unrealized PnL and — most
dangerously — liquidation risk. A user could believe a position had been
closed, or that their exposure was far smaller than it actually was.
Count failed markets and publish completeness alongside the numbers it
qualifies, as isPartial + failedMarketCount on PortfolioData. Each catch
now logs what failed instead of discarding it silently.
Surface it on /portfolio: when the scan is incomplete, a warning states
that positions may be missing and the totals may be understated, rather
than rendering a partial portfolio as the whole picture. A flag no one
renders would not have fixed the reported harm.
A disconnected wallet clears the flag — no wallet is a complete (empty)
picture, not a failed scan.
Closes #2414
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe portfolio hook now reports incomplete scans and failed-market counts, while the portfolio page displays warnings when loading finishes with partial data. New tests cover RPC failures, successful scans, empty wallets, and the returned state contract. ChangesPortfolio partial-scan reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PortfolioPage
participant usePortfolio
participant SolanaRPC
PortfolioPage->>usePortfolio: request portfolio snapshot
usePortfolio->>SolanaRPC: scan markets and fetch slab accounts
SolanaRPC-->>usePortfolio: accounts or RPC failure
usePortfolio-->>PortfolioPage: positions, isPartial, failedMarketCount
PortfolioPage-->>PortfolioPage: render warning when partial and not loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/app/portfolio/page.tsx (1)
56-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign
failedMarketCountfallback withisPartialfor mock mode.For strict consistency with
isPartialbeing forced tofalsein mock mode, consider forcingfailedMarketCountto0as well. Even though the warning banner won't render due to theisPartialcheck, this keeps the derived local state completely clean.💡 Proposed fix
// GH#2414: an incomplete scan must not render as a complete picture — the // totals below can understate real exposure. Mock mode is always complete. const isPartial = mockPositions ? false : portfolio.isPartial; - const failedMarketCount = portfolio.failedMarketCount ?? 0; + const failedMarketCount = mockPositions ? 0 : (portfolio.failedMarketCount ?? 0);🤖 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 `@app/app/portfolio/page.tsx` around lines 56 - 57, Update the failedMarketCount derivation alongside isPartial in the portfolio page so mockPositions forces failedMarketCount to 0; otherwise preserve portfolio.failedMarketCount with its existing nullish fallback.
🤖 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.
Nitpick comments:
In `@app/app/portfolio/page.tsx`:
- Around line 56-57: Update the failedMarketCount derivation alongside isPartial
in the portfolio page so mockPositions forces failedMarketCount to 0; otherwise
preserve portfolio.failedMarketCount with its existing nullish fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5a213f1-6372-4a36-9c13-cffda0cd64c1
📒 Files selected for processing (3)
app/__tests__/hooks/gh2414-portfolio-partial-scan.test.tsapp/app/portfolio/page.tsxapp/hooks/usePortfolio.ts
|
Correction to my previous comment — I over-corrected, and I was wrong about #2414's status. Two errors to fix on the record. 1. #2414 IS already fixed on
That is strictly better than my My verification was faulty. I grepped playground for 2. But "redundant → close" was too strong. So Revised status of my three launch PRs:
So the decision is purely about release intent, and I cannot make it:
I am not closing them. Flagging clearly instead so nobody reviews them under a false assumption in either direction. |
|
Closing this — #2414 partial portfolio scan. I have asked twice which branch is authoritative (see #2440) without an answer. The evidence is now conclusive enough to act on rather than leave this open indefinitely: 1. Every other contributor targets 2. 3. The user impact is limited — mainnet is dormant (no collateral, last on-chain activity March; see #2443), so this path is not carrying real trading. Taken together, keeping this open only adds to a review backlog I have repeatedly flagged as the binding constraint. Closing is reversible — the branch and history remain, and this can be reopened in seconds if Not closing #2437. That one fixes an unauthenticated memory-exhaustion vector, and |
1 participant
Closes #2414 —
[HIGH]Portfolio shared cache accepts partial v17 owner-scan results, hiding active positions and understating account value.The bug
Every failure path inside
usePortfolio'sload()was swallowed into an empty result, which made "we could not read your positions" indistinguishable from "you have no positions":catch { slabAccountsInfo = [] }catch { /* Skip markets that fail to parse */ }getProgramAccountsowner scan — the exact path in the issuecatch { // ignore }The partial result was then published through the shared snapshot every
usePortfolio()consumer reads, understating position count, deposited capital, portfolio value, unrealized PnL, and — most dangerously — liquidation risk. A user could believe a position had closed, or that their exposure was far smaller than it really was, while the same position stayed visible on the market page.The fix
Count failed markets and publish completeness alongside the numbers it qualifies —
isPartialandfailedMarketCountonPortfolioData, set in the sameif (!cancelled)block as the totals, so no consumer can read the totals without them. Eachcatchnow logs what failed rather than discarding it.Surfaced on
/portfolio. When the scan is incomplete the page shows a warning that positions may be missing and totals may be understated, and points the user at the market page before acting on the numbers. A flag nobody renders would not have fixed the reported harm, which is precisely that partial data looked complete.A disconnected wallet clears the flag — no wallet is a complete (empty) picture, not a failed scan.
Testing
__tests__/hooks/gh2414-portfolio-partial-scan.test.ts(5): v17 owner-scan failure flags partial; batch-slab failure flags partial and yields zero positions (the key distinction — zero positions plus a partial flag is what separates this from an empty wallet); a fully successful scan is not partial; a genuinely empty wallet is not partial; and the snapshot exposes both fields with the right types.Verified non-vacuous: all 5 fail against the
origin/mainhook. To be precise about why — 3 fail on behaviour, and 2 (the "complete" and "empty wallet" cases) fail because the property does not exist on main at all. They are contract guards, not behavioural proofs.No regressions, measured rather than assumed:
origin/main,__tests__/hooks/Identical 37 pre-existing failures (
useChartDrawingTool.test.tsand two others fail 10/10 on clean main); my change adds exactly the 5 new passing tests.npx tsc --noEmit→ 19 errors, same count as clean main, none referencing these files: 16 are stale.next/dev/typesbuild artifacts and 3 are a missingresendmodule in unrelated waitlist routes. (Correcting myself: in #2437 I described all 19 as.nextartifacts — 3 of them are theresendones.)Adjacent gap, deliberately not fixed here
discoverPortfolioMarketsalso has.catch(() => [])on both discovery paths (lines ~52 and ~61). If the API and static-bundle paths both fail, it returns[], which still reads as "no markets" rather than "discovery failed". That is arguably the same bug class one level up — but those catches are a deliberate fallback chain (API → static → empty), so distinguishing exhausted-fallbacks from genuinely-no-markets is a separate change with its own design question. Flagging rather than folding it in silently.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests