[DASHBOARD] Add error banner for failed account fetch#747
Open
Trovic1 wants to merge 7 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 33 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds typed AccountOverviewError classes, changes useAccountOverview to call /api/account-overview with HTTP-status classification and recovery, surfaces errors in Dashboard via AccountFetchAlert with retry, sanitizes widget error logging, updates related tests, and relaxes contract upgrade WASM-hash equality check. ChangesAccount Overview Error Handling and Dashboard Integration
Contract Upgrade Acceptance Change
Extension Wallet Transaction History Filter Test
Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant useAccountOverview
participant AccountOverviewAPI
Dashboard->>useAccountOverview: call with DEFAULT_ADDRESS
useAccountOverview->>AccountOverviewAPI: GET /api/account-overview?publicKey=...
AccountOverviewAPI-->>useAccountOverview: HTTP 200/404/5xx response
useAccountOverview-->>Dashboard: return data or AccountOverviewError
Dashboard->>Dashboard: render AccountFetchAlert on error (Retry -> refetch)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@Trovic1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![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 (2)
apps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.ts (1)
1-4:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix missing imports and update retry test expectations
AccountNotFoundError,HorizonUnavailableError, andactare referenced inuseAccountOverview.test.tsbut never imported, so the test file won’t compile.- The “recovers successfully after retry” test expects
status: 'inactive'andnonce: 7, but the hook always setsstatus: 'active'and derivesnoncefromaccountData.sequence.💡 Proposed fix
-import { renderHook, waitFor } from '`@testing-library/react`'; +import { act, renderHook, waitFor } from '`@testing-library/react`'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { useAccountOverview } from '../useAccountOverview'; +import { + AccountNotFoundError, + HorizonUnavailableError, + useAccountOverview, +} from '../useAccountOverview';🤖 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 `@apps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.ts` around lines 1 - 4, Import the missing symbols AccountNotFoundError, HorizonUnavailableError and act into the test file so it compiles, and update the "recovers successfully after retry" test assertions to expect the hook's actual behavior: status 'active' and nonce derived from accountData.sequence (e.g., nonce should equal accountData.sequence) when using useAccountOverview; locate imports at the top of useAccountOverview.test.ts and the failing test block referencing act and the two error classes and change the expected values accordingly.apps/web-dashboard/src/hooks/useAccountOverview.ts (1)
92-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
classifyAccountOverviewErrorintouseAccountOverview’s catch (and preserve HTTP status fromhorizon.ts)
apps/web-dashboard/src/hooks/useAccountOverview.tsdefinesclassifyAccountOverviewError(status)and the error subclasses, but thecatchcurrently stores a plainError(setError(err instanceof Error ? err : ...)), soDashboard.tsx’serror instanceof AccountNotFoundError/HorizonUnavailableErrorbranches never trigger and the hook’s declarederror: AccountOverviewError | nullcontract is broken.apps/web-dashboard/src/lib/horizon.tsthrows genericErroron non-okresponses and does not attach/preservestatus, so a catch-path classifier can’t read the HTTP code unlesshorizon.tsthrows a typed error that includesstatus(or otherwise attaches it) and the hook uses that status when callingclassifyAccountOverviewError.🤖 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 `@apps/web-dashboard/src/hooks/useAccountOverview.ts` around lines 92 - 93, The hook useAccountOverview must preserve and surface the typed AccountOverviewError subclasses so Dashboard.tsx's instanceof checks work: update horizon.ts to throw an error object that includes the HTTP status (e.g., create/throw a custom error or attach a numeric status property when response.ok is false) so callers can inspect status, and then change the catch in useAccountOverview to detect if err is already an AccountOverviewError (leave it) or else read the status from the caught error and call classifyAccountOverviewError(status) to produce and set the proper AccountOverviewError via setError; ensure you still handle non-Error thrown values by wrapping them into a meaningful error that preserves status when available.
🤖 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 `@apps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.ts`:
- Around line 133-164: The test's successful retry assertion must match how
useAccountOverview builds its return value: update the mocked successful fetch
responses so one returns account data with a sequence string (e.g., { sequence:
"7", ... }) used by fetchAccountData and another returns the balance number used
by fetchAccountBalance, then assert result.current.data equals { balance:
<number>, nonce: Number(accountData.sequence), status: 'active' } and adjust
expected fetch call count; reference useAccountOverview, fetchAccountData,
fetchAccountBalance, and result.current.refetch when adding the extra mock and
shape changes.
In `@apps/web-dashboard/src/pages/Dashboard.tsx`:
- Around line 57-61: The page currently calls
useAccountOverview(DEFAULT_ADDRESS) for the banner but passes only publicKey to
AccountOverviewGrid, leaving widgets out of sync with
overviewError/refetchOverview/overviewLoading; change to a single source of
truth by using the existing useAccountOverview(...) call once and pass its
returned state (overviewError, refetchOverview, overviewLoading, and the
overview data/publicKey) down into AccountOverviewGrid (and the banner) so both
share the same error/refetch/loading state, and update AccountOverviewGrid's
prop signature to accept and use these props.
---
Outside diff comments:
In `@apps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.ts`:
- Around line 1-4: Import the missing symbols AccountNotFoundError,
HorizonUnavailableError and act into the test file so it compiles, and update
the "recovers successfully after retry" test assertions to expect the hook's
actual behavior: status 'active' and nonce derived from accountData.sequence
(e.g., nonce should equal accountData.sequence) when using useAccountOverview;
locate imports at the top of useAccountOverview.test.ts and the failing test
block referencing act and the two error classes and change the expected values
accordingly.
In `@apps/web-dashboard/src/hooks/useAccountOverview.ts`:
- Around line 92-93: The hook useAccountOverview must preserve and surface the
typed AccountOverviewError subclasses so Dashboard.tsx's instanceof checks work:
update horizon.ts to throw an error object that includes the HTTP status (e.g.,
create/throw a custom error or attach a numeric status property when response.ok
is false) so callers can inspect status, and then change the catch in
useAccountOverview to detect if err is already an AccountOverviewError (leave
it) or else read the status from the caught error and call
classifyAccountOverviewError(status) to produce and set the proper
AccountOverviewError via setError; ensure you still handle non-Error thrown
values by wrapping them into a meaningful error that preserves status when
available.
🪄 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: 51c21eef-89c5-4b31-9120-8c63d027f008
📒 Files selected for processing (8)
apps/extension-wallet/src/router/__tests__/router.test.tsxapps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.tsapps/web-dashboard/src/hooks/useAccountOverview.tsapps/web-dashboard/src/hooks/useWidgetErrorLogger.tsapps/web-dashboard/src/pages/Dashboard.tsxapps/web-dashboard/src/pages/__tests__/Dashboard.test.tsxapps/web-dashboard/src/widgets/__tests__/AccountWidgets.test.tsxapps/web-dashboard/src/widgets/__tests__/WidgetErrorBoundary.test.tsx
Comment on lines
+57
to
+61
| const { | ||
| error: overviewError, | ||
| refetch: refetchOverview, | ||
| isLoading: overviewLoading, | ||
| } = useAccountOverview(DEFAULT_ADDRESS); |
There was a problem hiding this comment.
Use one useAccountOverview source of truth for both the banner and the widgets.
Right now the banner retries the page-level hook, but AccountOverviewGrid still receives only a publicKey, so it is not connected to this error/refetch state. That can leave the alert and the visible account-overview widgets out of sync.
💡 Suggested direction
- const {
- error: overviewError,
- refetch: refetchOverview,
- isLoading: overviewLoading,
- } = useAccountOverview(DEFAULT_ADDRESS);
+ const overview = useAccountOverview(DEFAULT_ADDRESS);
{overviewError && (
<AccountFetchAlert
- error={overviewError}
- onRetry={refetchOverview}
- retrying={overviewLoading}
+ error={overview.error}
+ onRetry={overview.refetch}
+ retrying={overview.isLoading}
/>
)}
- <AccountOverviewGrid publicKey={account.address} />
+ <AccountOverviewGrid
+ publicKey={account.address}
+ overview={overview.data}
+ overviewError={overview.error}
+ overviewLoading={overview.isLoading}
+ onRetryOverview={overview.refetch}
+ />Also applies to: 80-87
🤖 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 `@apps/web-dashboard/src/pages/Dashboard.tsx` around lines 57 - 61, The page
currently calls useAccountOverview(DEFAULT_ADDRESS) for the banner but passes
only publicKey to AccountOverviewGrid, leaving widgets out of sync with
overviewError/refetchOverview/overviewLoading; change to a single source of
truth by using the existing useAccountOverview(...) call once and pass its
returned state (overviewError, refetchOverview, overviewLoading, and the
overview data/publicKey) down into AccountOverviewGrid (and the banner) so both
share the same error/refetch/loading state, and update AccountOverviewGrid's
prop signature to accept and use these props.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web-dashboard/src/hooks/useAccountOverview.ts (1)
63-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore stale fetch completions.
fetchDatais now re-entrant viarefetchandpublicKeychanges. A slower earlier request can still resolve after a later retry and overwrite the newerdata/errorstate, so the dashboard can re-show the failure banner after a successful recovery. Please gate state commits with a request id or cancel the previous fetch.Suggested fix
-import { useState, useEffect, useCallback } from 'react'; +import { useState, useEffect, useCallback, useRef } from 'react'; @@ export function useAccountOverview(publicKey: string): UseAccountOverviewReturn { const [data, setData] = useState<AccountOverview | null>(null); const [isLoading, setIsLoading] = useState<boolean>(true); const [error, setError] = useState<AccountOverviewError | null>(null); + const requestIdRef = useRef(0); const fetchData = useCallback(async () => { + const requestId = ++requestIdRef.current; + if (!publicKey) { setData(null); setError(null); setIsLoading(false); return; @@ if (!response.ok) { throw classifyAccountOverviewError(response.status); } const accountOverview = (await response.json()) as AccountOverview; + if (requestId !== requestIdRef.current) return; setData(accountOverview); } catch (err) { + if (requestId !== requestIdRef.current) return; + setError( err instanceof AccountOverviewError ? err : new AccountOverviewError('Failed to fetch account data', 'FETCH_FAILED') ); setData(null); } finally { - setIsLoading(false); + if (requestId === requestIdRef.current) { + setIsLoading(false); + } } }, [publicKey]);🤖 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 `@apps/web-dashboard/src/hooks/useAccountOverview.ts` around lines 63 - 95, fetchData is re-entrant and older slower responses can overwrite newer state; add a guard using a request id (useRef) or AbortController so only the latest fetch commits state. Specifically: create a mutable currentRequestId ref, increment it at the start of fetchData and capture the id in a local variable, then only call setData, setError, and setIsLoading if the captured id === currentRequestId.current (or alternatively pass an AbortSignal to fetch and abort previous controller when starting a new fetch); update refetch/publicKey change paths to increment/abort so stale responses are ignored. Ensure the same guard is applied in try, catch, and finally blocks around setData/setError/setIsLoading.
🤖 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 `@contracts/account/src/lib.rs`:
- Around line 1320-1323: The test currently uses an arbitrary non-zero
upgrade_hash and thus no longer verifies the “same-hash” path; change it to
fetch and use the contract’s actual deployed WASM hash via the test client API
(use that value as upgrade_hash and call client.try_upgrade(&upgrade_hash) and
assert Ok), and add a separate assertion that client.try_upgrade with a zero
hash (BytesN::from_array(&env, &[0; 32])) returns an error matching
InvalidWasmHash; keep references to upgrade_hash, client.try_upgrade, and the
InvalidWasmHash error in the test.
---
Outside diff comments:
In `@apps/web-dashboard/src/hooks/useAccountOverview.ts`:
- Around line 63-95: fetchData is re-entrant and older slower responses can
overwrite newer state; add a guard using a request id (useRef) or
AbortController so only the latest fetch commits state. Specifically: create a
mutable currentRequestId ref, increment it at the start of fetchData and capture
the id in a local variable, then only call setData, setError, and setIsLoading
if the captured id === currentRequestId.current (or alternatively pass an
AbortSignal to fetch and abort previous controller when starting a new fetch);
update refetch/publicKey change paths to increment/abort so stale responses are
ignored. Ensure the same guard is applied in try, catch, and finally blocks
around setData/setError/setIsLoading.
🪄 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: 70a00e70-3ad9-4d63-8b4f-11f0440b1bda
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.tsapps/web-dashboard/src/hooks/useAccountOverview.tscontracts/account/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web-dashboard/src/hooks/tests/useAccountOverview.test.ts
Comment on lines
+1320
to
+1323
| let upgrade_hash = BytesN::from_array(&env, &[7; 32]); | ||
|
|
||
| let result = client.try_upgrade(¤t_hash); | ||
| assert!(matches!( | ||
| result, | ||
| Err(Ok(ContractError::InvalidWasmHash)) | ||
| )); | ||
| let result = client.try_upgrade(&upgrade_hash); | ||
| assert!(result.is_ok()); |
There was a problem hiding this comment.
This test no longer exercises the same-hash upgrade path.
Line 1320 now uses an arbitrary non-zero hash, so this only proves that some non-zero input is accepted. It would still pass if upgrading with the current WASM hash started failing again, which is the behavior this change is supposed to allow. Please make this test use the deployed hash and add a separate zero-hash rejection case so InvalidWasmHash stays covered.
🤖 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 `@contracts/account/src/lib.rs` around lines 1320 - 1323, The test currently
uses an arbitrary non-zero upgrade_hash and thus no longer verifies the
“same-hash” path; change it to fetch and use the contract’s actual deployed WASM
hash via the test client API (use that value as upgrade_hash and call
client.try_upgrade(&upgrade_hash) and assert Ok), and add a separate assertion
that client.try_upgrade with a zero hash (BytesN::from_array(&env, &[0; 32]))
returns an error matching InvalidWasmHash; keep references to upgrade_hash,
client.try_upgrade, and the InvalidWasmHash error in the test.
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
1 participant
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.
Closes #623
Summary
``
Testing
``
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests