feat: enhance PerpTradingForm with slider for size adjustment and improved price handling #8505
Conversation
…roved price handling
originalix
requested review from
ezailWang,
revan-zhang,
sidmorizon and
huhuanming
as code owners
WalkthroughAdds token-switch guarding, reference-price derivation, slider-driven size mechanics, and derived trade calculations to PerpTradingForm. Refactors price input behavior for market vs limit, updates available-to-trade display handling, and renders size sliders for mobile and desktop with proper initialization and synchronization after token changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Form as PerpTradingForm
participant Token as TokenState
participant Price as MarketData/Price
Note over Form,Token: Initial load
Form->>Token: Read selected token
Form->>Form: Initialize tokenRef (first load)
Form->>Price: Read mark/limit price
Form->>Form: Compute referencePrice, totals, sliderConfig
Note over User,Form: Token switch
User->>Form: Change token
Form->>Form: Set switching=true (guard updates)
Form->>Token: Update selected token
Token-->>Form: Sync completes
Form->>Price: Fetch new price
Form->>Form: Update referencePrice, derived values
Form->>Form: Set switching=false
sequenceDiagram
autonumber
actor User
participant UI as Slider/Inputs
participant Form as PerpTradingForm
Note over User,UI: Adjust amount via slider
User->>UI: Drag size slider
UI->>Form: handleSizeSliderChange(value)
Form->>Form: Clamp by sliderStep & decimals, update size
Form->>Form: Recompute totalValue, marginRequired, sliderConfig
Form-->>UI: Update slider controlledValue and displayed amounts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx (3)
85-117: Simplify token-switch effect; remove redundant stateThe three‑step/ref bookkeeping is more complex than needed. You can key off the token identity and update price once markPx is ready, without tokenSwitchingRef or the “synced” check.
Suggested simplification within this effect:
-useEffect(() => { - const prevToken = prevTokenRef.current; - const hasTokenChanged = - currentTokenName && prevToken && prevToken !== currentTokenName; - const isDataSynced = prevToken === currentTokenName; - const shouldUpdatePrice = - tokenSwitchingRef.current === currentTokenName && - formData.type === 'limit' && - currentTokenName && - tokenInfo?.markPx && - isDataSynced; - if (hasTokenChanged) { - tokenSwitchingRef.current = currentTokenName; - prevTokenRef.current = currentTokenName; - return; - } - if (shouldUpdatePrice && tokenInfo.markPx) { - updateForm({ price: formatPriceToSignificantDigits(tokenInfo.markPx) }); - tokenSwitchingRef.current = false; - } - if (!prevToken && currentTokenName) { - prevTokenRef.current = currentTokenName; - } -}, [currentTokenName, tokenInfo?.markPx, formData.type, updateForm]); +useEffect(() => { + if (formData.type !== 'limit') return; + const tokenKey = currentTokenName || ''; + if (!tokenKey || !tokenInfo?.markPx) return; + if (prevTokenRef.current !== tokenKey) { + prevTokenRef.current = tokenKey; + updateForm({ price: formatPriceToSignificantDigits(tokenInfo.markPx) }); + } +}, [currentTokenName, tokenInfo?.markPx, formData.type, updateForm]);Optional: remove tokenSwitchingRef entirely since it becomes unused.
367-382: Unify TP/SL price formatting with desktopMobile passes referencePrice.toFixed() (implicit precision). Desktop passes a pre‑formatted string. Use the same referencePriceString to avoid mismatched precision.
Apply:
- <TpslInput - price={referencePrice.toFixed()} + <TpslInput + price={referencePriceString} side={formData.side} szDecimals={tokenInfo?.szDecimals ?? 2} leverage={leverage}
420-428: Use NumberSizeableText for currency formatting (mobile)Align with desktop and other rows; avoid manual “$” + toFixed.
Apply:
- <SizableText fontSize={10}> - ${marginRequired.toFixed(2)} - </SizableText> + <NumberSizeableText + fontSize={10} + formatter="value" + formatterOptions={{ currency: '$' }} + > + {marginRequired.toNumber()} + </NumberSizeableText>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx (9)
11-11: LGTM: Slider importGood addition.
232-236: LGTM: Step resolutionReasonable cap on precision and numerically stable step.
237-248: LGTM: Slider change handlerProperly floors to szDecimals and guards invalid input.
338-348: LGTM: Mobile slider wiringConfig, step and disabled conditions look correct (after clamping fix above).
513-514: Decimals for PriceInput may be pxDecimals, not szDecimalsFlagging here too for visibility; see earlier comment on lines 122‑136.
524-535: LGTM: Desktop slider wiringMatches mobile; will be correct after clamping fix.
556-567: LGTM: Desktop TP/SL uses pre‑formatted priceGood to reuse the formatted reference string.
122-136: No pxDecimals field found; szDecimals is correct
310-329: Verify PriceInput disabled parsing and decimal prop
- Disabled inputs still render any string; confirm the market-price label won’t trigger validation or onChange
- Component only accepts szDecimals; ensure you’re passing price decimals (pxDecimals) instead of size decimals (szDecimals)
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx
Show resolved
Hide resolved
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx
Show resolved
Hide resolved
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx
Show resolved
Hide resolved
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx
Show resolved
Hide resolved
# Conflicts: # packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
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)
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx (1)
423-423: Use referencePriceString; avoid BigNumber.toFixed() default.toFixed() defaults can produce overly long strings. Use the already formatted string.
- price={referencePrice.toFixed()} + price={referencePriceString}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unittest (20.x)
- GitHub Check: lint (20.x)
🔇 Additional comments (7)
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx (7)
148-164: Reference price derivation looks good.
269-273: Step calculation looks solid.Clamped decimals and a positive step. Good.
102-140: Token switch effect: verify no stale price on fast switches.You set prevTokenRef immediately, so isDataSynced is effectively always true on the next run. Please sanity-check rapid symbol switches in limit mode to ensure we never momentarily set a stale price from the previous token.
252-259: Clamp slider value to [0, maxSize].Negative sizes can slip through and break the Slider. Clamp both ends.
return { price: effectivePrice, leverage: safeLeverage, maxSize, currentValue: currentValueNum, - controlledValue: maxSize > 0 ? Math.min(currentValueNum, maxSize) : 0, + controlledValue: + maxSize > 0 ? Math.max(0, Math.min(currentValueNum, maxSize)) : 0, isValid: !!effectivePrice && effectivePrice.gt(0) && maxSize > 0, };
171-179: Stop pre-formatting “Available”; return numeric only.Drop the fixed 2 decimals. Let UI format it consistently.
- const { availableToTradeDisplay, availableToTradeValue } = useMemo(() => { + const { availableToTradeValue } = useMemo(() => { const _availableToTrade = activeAssetData?.availableToTrade || [0, 0]; const value = _availableToTrade[formData.side === 'long' ? 0 : 1] || 0; const valueBN = new BigNumber(value); return { - availableToTradeDisplay: valueBN.toFixed(2, BigNumber.ROUND_DOWN), availableToTradeValue: valueBN.toNumber(), }; }, [formData.side, activeAssetData?.availableToTrade]);
326-327: Use NumberSizeableText for “Available” (mobile).Avoid pre-formatted strings; format at render.
- <SizableText size="$bodySmMedium" color="$text"> - ${availableToTradeDisplay} - </SizableText> + <NumberSizeableText + size="$bodySmMedium" + color="$text" + formatter="value" + formatterOptions={{ currency: '$' }} + > + {availableToTradeValue} + </NumberSizeableText>
517-518: Use NumberSizeableText for “Available” (desktop).Match mobile and keep formatting consistent.
- <SizableText size="$bodySmMedium" color="$text"> - ${availableToTradeDisplay} - </SizableText> + <NumberSizeableText + size="$bodySmMedium" + color="$text" + formatter="value" + formatterOptions={{ currency: '$' }} + > + {availableToTradeValue} + </NumberSizeableText>
packages/kit/src/views/Perp/components/TradingPanel/panels/PerpTradingForm.tsx
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes