Skip to content

Ralph/account page#73

Open
vipineth wants to merge 9 commits intomainfrom
ralph/account-page
Open

Ralph/account page#73
vipineth wants to merge 9 commits intomainfrom
ralph/account-page

Conversation

@vipineth
Copy link
Copy Markdown
Owner

No description provided.

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hypeterminal-main Error Error Apr 13, 2026 10:33am

@claude
Copy link
Copy Markdown

claude Bot commented Apr 13, 2026

PR Review: Ralph/account page

Good overall structure — the account page is well-organized with clear separation of concerns across components. A few issues worth addressing before merge.


Bugs / Correctness

1. TradesTab sends startTime: 0 for "all" range (account-history.tsx:444–448)

const { data, isLoading } = useInfoUserFillsByTime({
  user: address,
  startTime: startTime ?? 0,  // "all" range → startTime is undefined → sends 0
  aggregateByTime: true,
});

When range === "all", getStartTime returns undefined. The ?? 0 coerces it to Unix epoch 0, which probably requests all trades from 1970. Compare to FundingTab which passes startTime directly (correctly letting it be undefined). Either pass undefined, or handle "all" differently.

2. isEven naming is inverted (account-positions.tsx:1786)

<PositionTableRow key={...} isEven={i % 2 === 1} ... />

i % 2 === 1 selects odd indices (rows 1, 3, 5…). If the intent is to stripe every other row, the prop name isEven is misleading and will confuse future readers. Rename to isAlternate or flip the condition.

3. address as string type cast (account-page.tsx:1349)

return <ConnectedAccountView address={address as string} />;

The isConnected check above doesn't narrow address to string in wagmi's types. The cast silences the compiler without fixing the underlying type. Either assert non-null (address!) with a comment explaining why it's safe, or use a type guard.


Token / Design System Inconsistency

account-history.tsx and account-leverage-settings.tsx use tokens from an older system, while account-balances.tsx and account-positions.tsx already use the new tokens. The inconsistency means these sections will look visually different.

Tokens that need updating:

Old (incorrect) New (correct per design-tokens.md)
border-border-200 border-stroke-weak
border-border-200/40 border-stroke-weak/40
bg-surface-execution bg-bg-raised
bg-surface-analysis bg-bg-alternate
text-text-600, text-text-500 text-text-weak
bg-market-up-100 / text-market-up-600 bg-fill-success-weak / text-market-up
bg-market-down-100 / text-market-down-600 bg-fill-error-weak / text-market-down
text-primary-default / bg-primary-muted text-text-brand / bg-fill-brand-weak

Examples in account-history.tsx:

  • Line 341: border-border-200 bg-surface-execution
  • Lines 388–389: bg-primary-default text-text-10 and text-text-600 hover:bg-surface-analysis
  • Line 468: bg-surface-analysis hover:bg-surface-analysis
  • Line 719: text-market-up-600 bg-market-up-100 / text-primary-default bg-primary-muted

4. text-4xs is not a valid token (account-history.tsx:419, 498, 683, 721)
The documented scale is text-3xs (10px) as the minimum. text-4xs doesn't exist in design-tokens.md. These should likely be text-3xs.


Hyperliquid Data Principle

5. Floating-point arithmetic on financial values (account-balances.tsx:82–96, account-page.tsx:1590–1607)

Per hyperliquid.md: "Use big.js inline where needed, then back to string." The SpotRow type stores number and does total - hold, total * midPx, etc. In SpotMetrics:

availableValue += b.coin === DEFAULT_QUOTE_TOKEN ? available : (available / total) * usdValue;

These are display-only values so the precision loss is minor, but for consistency with the codebase principles, financial calculations should use Big(). The aggregated totalValue, availableValue, and inOrderValue accumulate errors across many tokens.


Minor

6. Duplicate Section and MetricCell components across files
account-balances.tsx, account-leverage-settings.tsx, and account-positions.tsx each define local Section and MetricCell components. The Section in account-leverage-settings.tsx uses different tokens (see above), which may explain some visual inconsistency. Once tokens are normalized, consider whether these warrant a shared component in @/components/ui/ — the current approach is fine if intentional but could be a source of future drift.

7. FundingTab total funding calculation

const totalFunding = entries.reduce((acc, f) => acc + toNumberOrZero(f.delta.usdc), 0);

Same float accumulation note as above; low severity for a summary display value.


Overall this is solid work — the component structure is clean, mobile/desktop branching is consistent, and i18n coverage is thorough. Main asks: fix the startTime ?? 0 bug, align tokens in account-history.tsx and account-leverage-settings.tsx to the new system, and swap text-4xs for text-3xs.

@vipineth vipineth changed the base branch from refactor/monorepo to main April 13, 2026 10:09
@claude
Copy link
Copy Markdown

claude Bot commented Apr 13, 2026

Code Review — Account Page

Solid feature addition — the account page covers a lot of ground (positions, balances, history, leverage settings) and the overall structure is clean. A few issues to address:


Bugs / Correctness

1. Skeleton imported directly from boneyard-js/react in 3 files (account-balances.tsx, account-page.tsx, account-positions.tsx)

This PR adds apps/terminal/src/components/ui/skeleton.tsx as the canonical re-export, but then some files still bypass it:

// ❌ account-balances.tsx, account-page.tsx, account-positions.tsx
import { Skeleton } from "boneyard-js/react";

// ✓ account-history.tsx, account-leverage-settings.tsx
import { Skeleton } from "@/components/ui/skeleton";

Use the local wrapper everywhere for consistency.

2. address as string type assertion without narrowing (account-page.tsx)

return <ConnectedAccountView address={address as string} />;

The address from useConnection() is string | undefined. Even though isConnected is checked above, TypeScript can't narrow across the control flow here — if wagmi's type ever changes this will be silently wrong. Use an explicit guard instead:

if (!address) return null;
return <ConnectedAccountView address={address} />;

Hyperliquid Data Principle Violations

3. Floating-point multiplication on API strings (account-balances.tsx lines ~55–59)

const total = toNumberOrZero(b.total);        // string → number
const midPx = toNumberOrZero(mids?.[b.coin]); // string → number
usdValue = midPx > 0 ? total * midPx : ...;  // raw JS multiplication

Per the project rules, use big.js for arithmetic on API strings:

import Big from "big.js";
usdValue = Big(b.total).times(mids[b.coin]).toNumber();

4. Premature .toNumber() conversion in account-positions.tsx

Both PositionTableRow and PositionCard do:

const size = toBig(p.szi)?.toNumber() ?? Number.NaN;
const markPx = toBig(markPxRaw)?.toNumber() ?? Number.NaN;
const entryPx = toBig(p.entryPx)?.toNumber() ?? Number.NaN;
const unrealizedPnl = toBig(p.unrealizedPnl)?.toNumber() ?? Number.NaN;

The formatters (formatToken, formatPrice, formatUSD) all accept Numeric (string | number | null | undefined) — pass the strings directly. Only convert when you actually need to do math (e.g. the liqIsNear comparison). The toBig().toNumber() → formatter round-trip is unnecessary and drops precision.


Code Style Violations

5. Nested ternary in OrderStatusBadge (account-history.tsx)

const colorClass =
    status === "filled"
        ? "text-market-up bg-fill-success-weak"
        : status === "open" || status === "triggered"
            ? "text-text-brand bg-fill-brand-weak"   // ← nested ternary
            : "text-text-weak bg-bg-alternate/50";

Per code style rules, use a helper with early returns or an object map:

function getStatusColorClass(status: string): string {
    if (status === "filled") return "text-market-up bg-fill-success-weak";
    if (status === "open" || status === "triggered") return "text-text-brand bg-fill-brand-weak";
    return "text-text-weak bg-bg-alternate/50";
}

6. Non-token decoration class in ExplorerLink (account-history.tsx)

className="... decoration-dashed decoration-text-500/30"

text-500 is not a design token. Use a token-based color like decoration-text-weak/40.


Design Concerns

7. Duplicate Section and MetricCell components

Section is identically defined in all three of account-balances.tsx, account-positions.tsx, and account-leverage-settings.tsx. MetricCell is defined in both account-balances.tsx and account-positions.tsx (with slightly different prop shapes). Consider extracting both to a shared location under @/components/account/ or @/components/ui/ if they're intended to be reused.


Minor

8. Service worker cache versionASSET_CACHE is keyed to "v1". If you need to bust this cache in future deployments you'll need to bump CACHE_VERSION. Worth documenting the intended invalidation strategy.

9. Error toast + inline error state duplication (account-leverage-settings.tsx) — handleConfirm and handleSwitchMode both call toast.error(...) and surface the error via {error && <div>...}. Users see two feedback signals for the same failure. Pick one (the inline error state is more discoverable in the popover; toasts are fine for success).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant