Hl react optimisation v2

[vipineth]

No description provided.

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
hypeterminal-main	Ready	Preview, Comment	Apr 27, 2026 3:23pm

[claude]

Code Review — Hl react optimisation v2

Overview

This PR delivers a focused UI polish pass across the trading interface:

• Modal component consolidation — moves the header divider and padding into ModalHeader defaults, removing boilerplate from every call site
• Side indicator redesign — replaces pill badges (background + colored text) with thin vertical bars, a cleaner visual pattern
• Tooltip arrow fix — corrects how BaseTooltip.Arrow is used
• TP/SL modal header refactor — simplifies the asset/badge combo into a title + description hierarchy

---

Positive observations

• Consistent cleanup: removing getSideClass from the helper layer and replacing with a clean isLong local constant follows the "inline simple logic" rule.
• aria-hidden="true" on purely decorative elements and the improved aria-label strings are solid accessibility improvements.
• The tooltip arrow fix is correct — applying className directly to BaseTooltip.Arrow is the proper Base UI usage; the old pattern (wrapping a div) was leaving the arrow unpinned.
• i18n files are updated in all locales; obsoleted strings are correctly marked with the obsolete marker.

---

Issues

1. Border opacity regression (packages/ui/src/modal.tsx)

All call sites previously used className="border-b border-stroke-weak/40" — a 40% opacity tint. The new default applies border-stroke-weak at full opacity (or whatever alpha the token natively carries). If --color-stroke-weak does not already encode that alpha, every modal header border will appear noticeably stronger than before.

Recommendation: Confirm border-stroke-weak already carries the intended alpha, or add an opacity modifier to the built-in class (border-stroke-weak/40). Alternatively, expose it as a separate design token so intent is explicit.

2. Modal padding reduction changes public API (packages/ui/src/modal.tsx)

ModalContent went from px-6 py-4 to p-4 (horizontal padding 24px to 16px) and ModalFooter from px-6 pb-6 to p-4. These are significant visual changes for all existing modals across the app. Any call site that relied on the 24px horizontal gutter now has 16px.

This also affects position-limit-close-modal.tsx and position-tpsl-modal.tsx where a px-6 wrapper was changed to px-4 — worth auditing that these outside-ModalContent containers still align correctly with the updated ModalContent padding.

3. Order type label lost its container (apps/terminal/src/components/trade/positions/orders-tab.tsx)

The typeConfig.class values are now "text-success", "text-warning", "text-fg-muted" — colored text only. The removal of px-1.5 py-0.5 rounded-8 from the wrapping span may cause the TP/Stop labels to look visually unanchored in the table cell. If intentional, this is fine, but please verify the visual result.

4. PositionTpSlModal — asset identity lost from header

The old header showed the coin logo + name via AssetDisplay plus a directional badge — allowing the user to confirm which position they are editing before interacting. The new header shows "Manage TP/SL" + "Long BTC position" as plain text. This removes the visual confirmation pattern (icon + badge color), which is a usability regression in a destructive-adjacent context (managing stop-losses).

Suggestion: Consider keeping at minimum the AssetDisplay inside the ModalDescription so the coin icon is still visible.

---

Minor notes

• The divider prop on ModalHeader (default true) is well-designed API, but since no call site currently passes divider={false}, consider whether any modal intentionally needs no divider in the future.
• order.side === "B" is used directly in three files. The existing isLongOrder(order) helper in open-orders.ts does exactly this. Given the project's "inline simple logic" rule this is acceptable, but both now coexist — if the rule means inline, isLongOrder itself should arguably be removed to avoid a confusing split.
• New i18n strings for non-English locales ("Long N position", "Short N position", "Manage TP/SL") have empty translations — expected for new strings, but worth flagging for translators.

[claude]

Code Review

Overview

This PR refactors UI styling across order/position panels and modals. Main changes:

• Replaces badge/pill side-indicators (long/short labels with backgrounds) with a thin colored vertical bar
• Consolidates ModalHeader border into the component itself via a divider prop (default true)
• Standardizes modal padding from px-6 pt-6 to p-4 across the board
• Removes helper functions getSideClass, isLongOrder from open-orders.ts
• Fixes tooltip arrow positioning with data-[side=*] classes

---

Issues

1. Border opacity regression in ModalHeader

All call sites previously used border-stroke-weak/40 (40% alpha), but the new default in modal.tsx is border-stroke-weak (full opacity). This is a subtle but real visual difference — the divider will appear slightly stronger than before.

// old at call sites
<ModalHeader className="border-b border-stroke-weak/40">

// new in component (full opacity)
divider && "border-b border-stroke-weak"

If the intent is a consistent stronger border, that's fine — but it should be explicit rather than accidental.

---

2. Accessibility regression in position-tpsl-modal.tsx

AssetDisplay was moved out of ModalTitle, making the modal title just "Manage TP/SL" instead of including the coin name. Screen readers use the dialog title to describe the modal — the previous version was more descriptive (AssetDisplay coin + "Long/Short").

// before — title included the coin
<ModalTitle className="flex items-center gap-1">
    <AssetDisplay coin={position.coin} />
    <Badge ...>{position.isLong ? "Long" : "Short"}</Badge>
</ModalTitle>

// after — coin is outside the title
<ModalTitle>{t`Manage TP/SL`}</ModalTitle>
<div className="flex items-center gap-1.5">
    <span ... />
    <AssetDisplay coin={position.coin} />
    ...
</div>

Consider moving AssetDisplay back into ModalTitle, or adding a visually-hidden aria-label on the dialog element.

---

3. divider prop is unused as false

The prop is added with a default of true but no call site passes divider={false}. The prop is fine as an escape hatch, but if none of the removed className overrides actually needed to opt out, consider whether it's needed at all right now.

---

4. Order type label loses visual container

In orders-tab.tsx, the order-type chip (TP, Stop, etc.) loses its pill background:

// before — pill with bg tint
<span className={cn("text-2xs px-1.5 py-0.5 rounded-8 uppercase whitespace-nowrap", typeConfig.class)}>

// after — plain colored text
<span className={cn("text-2xs uppercase whitespace-nowrap", typeConfig.class)}>

And ORDER_TYPE_CONFIG classes changed from "bg-success-soft text-success" → "text-success". This makes TP/Stop labels less visually distinct at a glance in a dense table. Worth verifying this is intentional.

---

Positives

• Tooltip arrow fix (tooltip.tsx) is clean — passing className directly to BaseTooltip.Arrow and using data-[side=*] for positioning is the correct approach.
• biome-ignore comment in analysis-section.tsx is acceptable since linter suppressions require a reason string; this doesn't violate the no-comments rule.
• i18n is properly updated with new strings and deprecated old ones marked with #~.
• getSideClass removal is a clean simplification — the logic was trivial and is now inlined correctly.
• Improved aria-label strings (e.g. "Switch to BTC market, long order") are a nice accessibility win.

---

Summary

The refactor is directionally good — consistent spacing, simpler side indicators, and a cleaner ModalHeader API. The two things worth addressing before merge: (1) confirm the border opacity change from /40 to full is intentional, and (2) consider the accessibility impact of removing the coin name from the TP/SL modal title.

[claude]

Code Review: hl-react optimisation v2

Overview

This PR makes three main changes:

1. Modal layout standardisation — bakes divider border and consistent p-4 padding into ModalHeader, ModalContent, and ModalFooter in packages/ui, removing ~9 call-site overrides
2. Side indicator redesign — replaces colored pill badges (long/short/buy/sell) with a thin 4×0.5 vertical stripe, removes getSideClass / isLongOrder helpers
3. Tooltip arrow positioning bug fix — adds data-[side=*] classes and simplifies Arrow rendering

---

Issues

🔴 Design token violation in ModalHeader

// packages/ui/src/modal.tsx
divider && "border-b border-stroke-weak/40"

The style guide explicitly states: "Avoid opacity modifiers on border-stroke-weak for hover emphasis — switch to border-stroke-strong instead." Using /40 on border-stroke-weak violates this. Use border-stroke-weak directly or border-stroke-strong for a heavier divider.

---

🔴 ORDER_TYPE_CONFIG.default.class changes CSS property type

- default: { class: "bg-surface/50" },
+ default: { class: "text-fg-muted" },

The class was applied as a background color. Changing it to a text utility silently changes its semantics. Any call site that relies on it as a background will produce incorrect styling. Verify all usages of ORDER_TYPE_CONFIG.default.class are still correct after this change.

---

🟡 Global padding reduction is a silent breaking change

ModalContent and ModalFooter padding changed from px-6/pb-6 to p-4 at the design-system level, reducing horizontal padding from 1.5rem to 1rem for all modals across the app. The PR only audits its own touched files. Other modals (bridge, history, etc.) will silently reflow. Consider verifying these visually or making this a documented intentional design change.

---

🟡 Desktop orders views lose text side labels

The pill showing "long" / "short" / "buy" / "sell" is removed from orders-tab.tsx, orders-history-tab.tsx, and twap-tab.tsx. The thin color stripe is the only direction signal remaining for sighted users. This is a regression for users with red/green color blindness. The mobile component (mobile-orders-history-tab.tsx) still renders getSideLabel() text — consider keeping text labels on desktop too, or at minimum adding a title attribute to the stripe element.

---

🟡 aria-label on ModalPopup may conflict with dialog title

<ModalPopup aria-label={t`Manage TP/SL for ${position.coin}...`}>

ModalPopup renders a div, not the Radix Dialog.Root. The accessible name for the dialog is already derived from <ModalTitle> via aria-labelledby. An aria-label on an inner div won't override the dialog's accessible name and may confuse screen readers. If the goal is a richer dialog label, set it on the <Modal> (Dialog.Root) element instead.

---

Positive Changes

• ModalHeader divider prop — the divider={true} default cleanly replaces 9+ identical className="border-b border-stroke-weak/40" overrides. Good abstraction.
• Tooltip arrow fix — using data-[side=*] positioning classes and removing the nested <div> is correct and much cleaner.
• Improved aria-label text on market switch buttons (e.g. "Switch to BTC market, long order") is a meaningful accessibility improvement.
• positions-tab.tsx icon/color polish — text-success/70 for TP prices and larger hover-visible pencil icon are subtle but nice improvements.
• i18n cleanup — retiring "buy" / "sell" lowercase strings and properly obsoleting them is correct housekeeping.

---

Minor Notes

• The biome-ignore comment in analysis-section.tsx is a lint suppression, not a regular comment, so it's acceptable — but adding a follow-up task to eliminate the useLayoutEffect dep issue would be cleaner long-term.
• New translation strings ("Long position", "Manage TP/SL", etc.) have empty msgstr in all non-EN locales — expected, but flag for translators before shipping.

[claude]

Code Review

Overview

Large structural refactor with 8,722 additions / 6,989 deletions across ~100 files. The core themes are:

1. Config decomposition — monolithic constants.ts (605 lines) split into ~15 focused domain files (app.ts, bridge.ts, chart.ts, trade.ts, layout.ts, etc.)
2. Component decomposition — bridge-tab.tsx (1047 lines → 8 files), trade-form-fields.tsx, positions-tab.tsx, token-selector.tsx all broken into smaller single-responsibility units
3. Hook extraction — useKlineOrderOverlays, useKlinePositionOverlays pulled out of KlineChart
4. Mobile UX — new MobilePositionCard, MetricCell, AppShellSkeleton
5. Magic number naming — 500 → INITIAL_CANDLE_COUNT, 86_400_000 → MS_PER_DAY, 0.1 → LIQ_WARNING_PROXIMITY, 30_000 → TAB_RESTORE_THRESHOLD_MS

The direction is clearly right — this is a meaningful improvement to maintainability.

---

Bugs

symbol?: string vs symbol: string type mismatch (kline-chart.tsx → overlay hooks)

KlineChart declares symbol?: string (optional), but both useKlineOrderOverlays and useKlinePositionOverlays expect symbol: string:

// kline-chart.tsx
interface Props {
  symbol?: string;   // optional
  ...
}

// use-kline-order-overlays.ts
interface Params {
  symbol: string;    // required
}

When symbol is undefined, the hooks receive undefined where string is expected. TypeScript may not surface this in all strict configurations. The hooks should accept symbol: string | undefined and guard with an early return inside the effect.

AssetSelectionScreen never shows selected state (asset-selection-screen.tsx:138)

{tokens.map((token) => (
  <TokenRow key={...} token={token} isSelected={false} onSelect={onSelect} />
))}

isSelected is always false, so the brand-border selection style (border-stroke-brand-strong bg-brand/5) is permanently dead code. The selected token should be tracked in BridgeTab state and passed down, or the prop should be removed.

---

Code Style

memo with manual comparator contradicts project rules (token-selector-content.tsx)

const TokenSelectorRow = memo(function TokenSelectorRow(...) { ... }, rowPropsEqual);

Project guidelines: "React 19 Compiler: Don't add manual useMemo/useCallback for performance — the compiler handles memoization automatically." This applies equally to React.memo. If the virtual list is slow without it, that's worth a note in the PR, but it should be called out explicitly.

chartRef in useEffect deps is unnecessary

Both overlay hooks include chartRef in the dependency array. A RefObject has stable identity and never changes — its .current does, but refs are not dependencies of effects. This is harmless but adds noise and could mislead readers into thinking the effect re-runs on chart mount.

// Both hooks
}, [openOrdersEvent, symbol, chartRef]); // chartRef is never a new object

renderTpSlContent function inside component (position-row.tsx:80)

This renders a non-trivial JSX subtree (3 branches). Per project rules, single-helper functions are fine, but this is complex enough that it reads like a missing sub-component. Not a blocker, but worth noting.

---

Architectural Observations

Shared position-dialog-types.ts is a good call — TpSlPositionData, LimitClosePositionData, and TpSlOrderInfo were duplicated across positions-tab.tsx and mobile-positions-tab.tsx. Centralising them in position-dialog-types.ts removes that duplication cleanly.

Shared Placeholder component (positions/placeholder.tsx) — similarly good; it was inlined in multiple tab files.

bridge-tab.tsx screen machine — the BridgeTab state machine (select → amount → confirm → executing → success/error) is clear and the guard conditions (if (!address), if (bridge.status === "error")) are handled in priority order. One edge case: if bridge.status flips to "idle" while on "executing" the effect redirects to "confirm", which could be confusing if the bridge fails silently with no status transition to "error". Worth testing that path.

labelTypographyClass shared from field-label.tsx — exporting a CSS class string from a component file for reuse works, but couples consumers to the component module. If this needs to be shared more broadly, a dedicated typography-tokens.ts in lib/ui/ would be cleaner. Minor.

---

Positives

• Config decomposition is thorough and well-named — the naming rule (no token stutter) is respected throughout
• Bridge flow decomposition is dramatically easier to read than the 1047-line monolith it replaced
• TradeFormScale, TradeFormTif, TradeFormTpSl, TradeFormTwap extractions reduce trade-form-fields.tsx appropriately without premature abstraction
• AppShellSkeleton gives the app a proper skeleton before hydration
• Magic number replacement is consistently applied across the chart module
• All new components use function declarations (project style ✓)
• No hardcoded hex colors, cn() used throughout ✓

---

Summary

The refactor is solid. Two real issues to address before merging:

1. Fix the symbol type mismatch in the kline overlay hooks
2. Either wire up the isSelected prop in AssetSelectionScreen or remove the dead prop/styling

The memo concern is a style/guidelines conflict worth a quick decision from the team.

[claude]

Code Review — Hl React Optimisation v2

This is a substantial refactor (~8.7k additions / ~7k deletions) focused on component decomposition, code organisation, and reducing render surface. Here's a breakdown by area.

---

Overview

The PR does three main things:

1. Splits monolithic components — bridge-tab.tsx (1047 lines deleted) → 8 focused files; positions-tab.tsx (363 → 64 lines) → PositionRow, Placeholder, position-dialog-types; trade-form-fields.tsx → sub-components (TradeFormScale, TradeFormTif, TradeFormTpSl, TradeFormTwap); deposit-modal.tsx → deposit/ folder
2. Extracts chart overlays — useKlineOrderOverlays and useKlinePositionOverlays custom hooks pulled out of kline-chart.tsx; toolbar extracted to KlineToolbar
3. Adds AppShellSkeleton for improved loading UX and MobilePositionCard for the mobile positions view

---

Positives

• Decomposition is well-scoped. Each extracted component has a single responsibility. PositionRow, Placeholder, and position-dialog-types are clean separations that will make the table easy to maintain.
• KlineToolbar extraction is clean — the interval/chart-type UI now lives in its own file and the hooks (useKlineOrderOverlays, useKlinePositionOverlays) correctly encapsulate side-effect logic without leaking chart state.
• position-dialog-types.ts is a nice move — shared data shapes for TpSlPositionData / LimitClosePositionData are now importable without pulling in a whole component.
• AppShellSkeleton avoids layout shift on load and follows the existing design-token conventions.
• Bridge screen decomposition is dramatically cleaner — each screen (select → amount → confirm → executing → success → error) is now its own file. The state machine in BridgeTab is easy to follow.
• SSR guard respected — document.addEventListener in kline-chart.tsx is inside a useEffect, which is correct per the SSR rules.

---

Issues & Suggestions

token-selector-content.tsx — large Props interface passed as spread

// token-selector.tsx
const contentProps = { ... }; // ~20 fields
<TokenSelectorContent {...contentProps} />

The Props interface in token-selector-content.tsx has 20+ fields, all threaded through a spread from TokenSelector. Consider co-locating useTokenSelector state access inside TokenSelectorContent directly (it already imports the hook's return type). The intermediate contentProps object buys nothing and makes the prop surface harder to trace.

getMaxLeverage called twice in the same render

// token-selector-content.tsx:149
{getMaxLeverage(market) && <span>{getMaxLeverage(market)}x</span>}

Call it once and store the result: const maxLev = getMaxLeverage(market);. Minor, but repeated calls accumulate across virtualised rows.

useKlineOrderOverlays / useKlinePositionOverlays — missing chartRef in dep arrays

// use-kline-order-overlays.ts:56
}, [openOrdersEvent, symbol]);

chartRef is a ref so its .current never triggers the dep array, but chartRef itself (the RefObject identity) is an external value that should still be listed. The linter may flag this depending on config.

kline-chart.tsx — activeInterval/activeChartType in the chart init useEffect

// kline-chart.tsx:121
}, [symbol, activeInterval, activeChartType, yAxisInside, theme]);

The chart is fully disposed and re-initialised every time the interval or chart type changes. That's a heavy operation (network re-fetch + klinecharts dispose/init cycle). The previous implementation likely updated the chart in place. Confirm this is intentional — if not, the interval change should call chart.applyNewData without re-running the init effect.

mobile-positions-tab.tsx — duplicate close logic vs positions-tab.tsx

handleClosePosition in MobilePositionsTab is nearly identical to the desktop version, including the toast, closingKeyRef, placeOrder, and buildOrderPlan calls. The only difference is setMobileActiveTab. This is a prime candidate for a shared hook (e.g. usePositionClose) to avoid the two implementations drifting. The desktop version also has a handleReverse that the mobile version is missing — worth aligning.

PositionRow — toBig called repeatedly per render

// position-row.tsx:51-70
const size = toBig(p.szi)?.toNumber() ?? NaN;
const markPx = toBig(markPxRaw)?.toNumber() ?? NaN;
const entryPx = toBig(p.entryPx)?.toNumber() ?? NaN;
const unrealizedPnl = toBig(p.unrealizedPnl)?.toNumber() ?? NaN;
const roe = toBig(p.returnOnEquity)?.toNumber() ?? NaN;
const cumFunding = toBig(p.cumFunding.sinceOpen)?.toNumber() ?? NaN;
const liqPx = toBig(p.liquidationPx)?.toNumber();

Seven Big() allocations per row per render. This is fine at low position counts, but the React 19 compiler won't eliminate object allocations. Consider whether these can stay as strings passed directly to formatters (per the Hyperliquid data principle — "strings flow from API to display unchanged").

deposit-modal.tsx — template literal string for min-height

<div className={`flex flex-col ${DEPOSIT_TAB_MIN_HEIGHT}`}>

DEPOSIT_TAB_MIN_HEIGHT is a string constant ("min-h-72"). Use cn() here for consistency with the rest of the codebase: cn("flex flex-col", DEPOSIT_TAB_MIN_HEIGHT).

TradeFormScale — Number(e.target.value) || 4 silently substitutes 4

// trade-form-scale.tsx:48
onChange={(e) => setScaleLevels(Number(e.target.value) || 4)}

If a user types 0 or an empty string, this defaults to 4 silently. Prefer explicit handling: let the store or validation layer decide the fallback, not the input handler.

BridgeTab — LiFi init side effect on every mount

// bridge/bridge-tab.tsx:26-30
useEffect(() => {
  if (!lifiInitialized.current) {
    initLiFi();
    lifiInitialized.current = true;
  }
}, []);

lifiInitialized is a component-local ref, so if BridgeTab is unmounted and remounted (e.g. tab switch), the guard still fires because the ref is fresh. If initLiFi is idempotent this is harmless, but if not (e.g. registers event listeners), it will double-initialise. Consider moving the guard to module scope.

---

Style / Convention Notes

• A few new files use useMemo for tpSlOrdersByCoin (mobile-positions-tab.tsx:57, positions-tab.tsx:67). This is correct — it's an array transformation that warrants memoisation.
• AppShellSkeleton hardcodes ["chart", "trade", "book", "positions", "account"] as tab names. If this list diverges from the actual tabs it will silently show the wrong skeleton. Consider deriving from a shared constant.
• token-selector-content.tsx uses hardcoded "All", "Perp", "Spot", "HIP-3" labels without t\...`` wrappers (lines 14–18). The rest of the file is properly internationalised.

---

Summary

The decomposition work is solid and the new component boundaries make sense. The main risks are: (1) the chart interval-change triggering a full dispose+init cycle, (2) duplicate close logic between desktop and mobile positions, and (3) the LiFi init guard being mount-local. Everything else is polish.