Conversation
…io fetch fails When `fetchAndUpdatePortfolio` threw, the catch block recorded a diagnostic but still called `markLoaded()`, leaving the portfolio refs `undefined`. The page's display logic only substitutes "—" when positions exist with missing prices, so an outright load failure fell through to formatted zeros: "$0.00 Supplied / $0.00 Borrowed / $0.00 Net Worth". For an account with real deposits and borrows this is indistinguishable from an empty account, and the recorded diagnostics were consumed nowhere in the UI. Track an explicit `hasPositionsFetchError` flag, set on a thrown fetch and cleared on every fresh attempt, and expose `hasPortfolioLoadError` — true only when a fetch failed AND there is no (stale) portfolio to fall back on, so a transient background-refresh failure keeps the last-good figures on screen while a genuine cold-load failure surfaces an error. The portfolio page renders a dismissable-free error banner with a Retry action and shows "—"/"-" for the totals and performance figures instead of zeros. Mirrors the existing "Position data is incomplete" handling on the per-position page. Adds a unit test covering the failed-fetch path.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: euler-xyz/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a ChangesPortfolio load error handling
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PortfolioPage as portfolio.vue
participant Composable as useEulerAccount
participant API as fetchPortfolio
PortfolioPage->>Composable: updatePositions()
Composable->>Composable: clear hasPositionsFetchError
Composable->>API: fetchPortfolio()
API-->>Composable: throws error
Composable->>Composable: set hasPositionsFetchError = true
Composable->>Composable: compute hasPortfolioLoadError (no fallback portfolio)
Composable-->>PortfolioPage: hasPortfolioLoadError = true
PortfolioPage->>User: show error banner + placeholders
User->>PortfolioPage: click Retry
PortfolioPage->>Composable: retryPortfolioLoad -> updatePositions(fresh, preempt)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pages/portfolio.vue (1)
137-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlaceholder character inconsistent between
-and—.
portfolioNetApyDisplay/portfolioRoeDisplay(and theirdata-valuebindings at Lines 275, 298) fall back to a hyphen'-', whiletotalSuppliedDisplay/totalBorrowedDisplay/netAssetValueDisplay(and Lines 326, 349, 372) fall back to an em dash'—'. Minor visual inconsistency across the same error state.Also applies to: 275-372
🤖 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 `@pages/portfolio.vue` around lines 137 - 167, The placeholder shown for missing/error portfolio values is inconsistent between the display computeds in portfolio.vue: portfolioNetApyDisplay and portfolioRoeDisplay use a hyphen while totalSuppliedDisplay, totalBorrowedDisplay, and netAssetValueDisplay use an em dash. Update the relevant computed fallback values and their related data-value bindings so the same placeholder character is used consistently across all portfolio metrics, matching the existing error state handling in those computed properties.
🤖 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 `@composables/useEulerAccount.ts`:
- Line 129: The fetch flow in useEulerAccount’s fetchAndUpdatePortfolio is
clearing hasPositionsFetchError too early, which makes portfolio.vue drop its
error/placeholder state before new data is available. Remove the synchronous
reset at the start of fetchAndUpdatePortfolio and only set
hasPositionsFetchError.value = false on the success path, alongside the other
successful portfolio assignments, so the previous error state remains visible
until a fetch actually completes.
---
Nitpick comments:
In `@pages/portfolio.vue`:
- Around line 137-167: The placeholder shown for missing/error portfolio values
is inconsistent between the display computeds in portfolio.vue:
portfolioNetApyDisplay and portfolioRoeDisplay use a hyphen while
totalSuppliedDisplay, totalBorrowedDisplay, and netAssetValueDisplay use an em
dash. Update the relevant computed fallback values and their related data-value
bindings so the same placeholder character is used consistently across all
portfolio metrics, matching the existing error state handling in those computed
properties.
🪄 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: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e52b2701-dd2c-4573-a7bd-427e4ae25f98
📒 Files selected for processing (3)
composables/useEulerAccount.tspages/portfolio.vuetests/composables/useEulerAccount.test.ts
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Leonard review — changes requested
Reviewed head 6a970c665e54f247a0cd1d0ac96dc019ebbe00b5.
This is the right direction: a cold portfolio fetch failure should not be rendered as a confident empty account. The new composable flag and portfolio-page placeholders cover the initial thrown-fetch path, and the focused test exercises that path.
One blocking edge remains, matching CodeRabbit's major comment after verification:
composables/useEulerAccount.ts:hasPositionsFetchErroris cleared synchronously at the start of each new fetch attempt. After a cold-load failure, clicking Retry or waiting for a refresh immediately dropshasPortfolioLoadErrorwhileportfoliois stillundefined, so the portfolio cards can show0.00%/$0again for the duration of the retry. That reintroduces the exact misleading-zero state this PR is trying to remove, just during the retry window. Keep the error/placeholder state until a new portfolio result is actually confirmed, then clear the error on the success path.
Validation performed:
npx vitest run tests/composables/useEulerAccount.test.ts— passed.npm run typecheck— passed.npm run build— passed, with existing Vite/Rollup chunk/dynamic-import warnings only.- Local throwaway regression test for “failed cold load → pending retry” — failed as expected on this head:
hasPortfolioLoadErrorbecamefalsewhileportfoliowas stillundefined.
Scalability / maintainability hygiene:
- Scope is appropriately centered in
useEulerAccount, rather than a page-only formatting patch. That is the right reusable layer for portfolio consumers. - I checked sibling portfolio/error surfaces: position pages already have separate
hasQueryFailurehandling; this PR’s shared portfolio-load state is specifically for account portfolio data and does not need to be copied into position-card components. - The remaining issue is also best fixed in the shared composable, not with more page-level guards.
Bot feedback:
- Verified CodeRabbit’s major retry-window finding as valid and blocking.
- The placeholder-character nit is minor and not merge-blocking.
Browser/mobile smoke:
- Incomplete for the isolated portfolio-fetch failure. A local headed/Xvfb attempt with forced RPC failures exercised a broader prerequisite failure mode rather than just
fetchPortfolio, so I did not use or post those screenshots as evidence. The blocking retry invariant is covered more directly by the composable-level regression test above.
Clear the portfolio fetch error only after a successful refresh confirms replacement data, and cover the pending retry window with a composable regression test.
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Leonard review — current head 95a23fc2f70ab476c08c776d44a0f371284b9c35
Verdict: no blocking findings. Submitting as a comment review, not approval.
The previous blocker on head 6a970c66 is resolved: hasPositionsFetchError is no longer cleared at fetch start, so a retry/background refresh keeps the cold-load error state visible until a successful portfolio response installs real data. The new regression test covers that exact pending-retry window.
What changed:
useEulerAccountnow tracks thrown portfolio fetches separately from partial per-position pricing diagnostics.hasPortfolioLoadErroris only true when the latest fetch threw and no stale portfolio exists to fall back to.pages/portfolio.vueshows an explicit portfolio-load error banner plus placeholders instead of confident$0/0.00%values.- Retry uses a fresh preemptive portfolio refresh.
Validation performed:
- Diff and surrounding-code review for
composables/useEulerAccount.ts,pages/portfolio.vue, andtests/composables/useEulerAccount.test.ts. - Focused regression:
npx vitest run tests/composables/useEulerAccount.test.ts— passed, including cold-load failure and pending-retry persistence. npm run typecheck— passed.npm run lint -- composables/useEulerAccount.ts pages/portfolio.vue tests/composables/useEulerAccount.test.ts— exited 0; only pre-existing repo-wideanywarnings surfaced outside this PR’s changed files.npm run build— passed; build emitted existing chunk/sourcemap-style warnings only.- Headed Chromium/Xvfb smoke against a local production build:
/portfolio?network=1&spy=0x68e7...8264on desktop and mobile rendered the portfolio shell/tabs without material page errors. This was route/mobile coverage, not wallet/signing coverage.
Scalability / maintainability hygiene pass:
- Sibling portfolio value/performance surfaces on the top-level portfolio page are covered by the shared
hasPortfolioLoadErrorgate, not separate one-off guards per metric. - Position-detail
hasQueryFailurehandling and portfolio-item query-failure handling are related but not the same data boundary: they cover incomplete position/vault data after a portfolio exists, while this PR covers the cold portfolio-fetch failure before there is any portfolio to render. I do not see a missed sibling surface that would still show the same cold-load$0bug. - The retry-persistence invariant is now in a focused composable test, which is the right level for this state machine.
Bot-feedback pass:
- CodeRabbit’s earlier major concern about clearing the error at fetch start was valid on the old head and is fixed here.
- Its remaining placeholder-character nit is low-value/non-blocking; I don’t see it changing correctness or user safety for this fix.
Coverage caveats:
- I did not run wallet/signing smoke; this PR changes read-only portfolio load/error presentation only.
- I did not post screenshots: the meaningful error state was validated through a controlled composable failure/retry test rather than a synthetic browser network-failure screenshot, and the browser pass was only route/mobile sanity coverage.
|
🚅 Deployed to the euler-lite-pr-718 environment in euler-lite(dev,PR previews)
|
2 participants
Problem
When
fetchAndUpdatePortfolioinuseEulerAccountthrows, the catch block records a diagnostic but still callsmarkLoaded(), leavingvisiblePortfolio/allPortfolioundefined. The portfolio page only substitutes "—" when positions exist with missing prices, so an outright load failure falls through to formatted zeros:For an account with real deposits and borrows this is indistinguishable from an empty account, and the recorded
portfolioDiagnosticsare consumed nowhere in the UI — the failure is fully swallowed. In a lending context, showing a confident $0.00 where a user has funds can drive wrong decisions.Fix
hasPositionsFetchErrorflag inuseEulerAccount, set when a fetch throws and cleared at the start of every fresh attempt.hasPortfolioLoadError, computed as "a fetch failed and there is no portfolio to fall back on". Gating on the absence of data means a transient background-refresh failure keeps the last-good figures on screen, while a genuine cold-load failure lets the UI show an error.—/-for the value and performance figures instead of zeros. Thedata-valuecapture attributes reflect the error too, so parity/e2e captures don't record misleading zeros.This mirrors the existing
hasQueryFailure→ "Position data is incomplete" handling already used on the per-position page.Testing
tests/composables/useEulerAccount.test.tscovering the failed-fetch path (assertshasPortfolioLoadErroris set, totals stay zero, and an error-severity diagnostic is present).npx vitest run tests/composables/useEulerAccount.test.ts— passes.Summary by CodeRabbit