Skip to content

CP-14671: present perps enable-trading checklist as a formsheet modal route - #3995

Open
bogdandobritoiu wants to merge 5 commits into
mainfrom
CP-14671
Open

CP-14671: present perps enable-trading checklist as a formsheet modal route#3995
bogdandobritoiu wants to merge 5 commits into
mainfrom
CP-14671

Conversation

@bogdandobritoiu

@bogdandobritoiu bogdandobritoiu commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

Ticket: CP-14671

The perps "Set up trading account" checklist (agent approval, builder-fee approval, unified account) shipped in #3984 as an inline half-height RN Modal rendered inside each screen. The design (Figma) specifies the standard stacked K2 formsheet instead. This PR converts it to a proper modal route:

  • New /perpetualsEnableTrading route registered with secondaryModalScreensOptions (stacked-sheet detents), since it always presents on top of the place-order/close/manage modals.
  • New PerpetualsEnableTradingScreen built on ScrollScreen isModal with title/subtitle per design; ports the three-step checklist logic unchanged. Dismissal is the sheet's native swipe/grabber (the design drops the old "Not now" button) plus a once-guarded router.back() when setup completes or is already satisfied.
  • usePerpsEnableTradingGate is now a plain .ts hook: requireTradingEnabled() navigates to the route instead of toggling a locally rendered modal; the enableTradingModal render-prop is removed from PlaceOrder/Close/Manage screens and PerpsEnableTradingModal is deleted.
  • k2-alpine Checklist polish to match design: plain check icon on completed rows, ActivityIndicator replaces the action button while a step is in flight, subtitle2 description.
  • Proactive entry point on the details screen (per design; core-web similarly gates its trade form until setup is done): the footer always shows Short / Long once funded, but for a first-time user the slide opens the enable-trading sheet instead of place-order, carrying the selected side. When the last setup step completes, the sheet router.replaces into place-order with the originally selected Short/Long (so back returns to the details screen, not the sheet). While setup status is still resolving the slide goes straight to place-order so a set-up user is never detoured on cold load (usePerpsEnableTradingGate now exposes isTradingStatusLoading; PerpsProvider now surfaces isLoadingAgent); the submit-time gate remains as the backstop for deep links and cold-load edge cases.

No new dependencies; no breaking changes.

Screenshots/Videos

Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-07-22.at.12.04.19.mov

Testing

Dev Testing

iOS - 9382
Android - 9383

Happy path:

  1. Use a wallet with deposited HL balance that has not completed perps setup and open a market's details screen — the footer shows the normal Short / Long slider.
  2. Slide Short or Long — instead of place-order, the "Set up trading account" formsheet presents stacked on top, matching the Figma.
  3. Complete the three steps in order (each button shows a spinner while in flight, then a green check) — after the last step the sheet closes and place-order opens with the side you originally slid (verify both Short and Long are carried through).
  4. Back from place-order returns to the details screen (not the setup sheet); subsequent slides go straight to place-order.
  5. Backstop path: reach the place-order screen without setup (e.g. deep link) and slide to confirm — the sheet presents at submit time (no side carried), dismisses in place after setup, and the order is not sent until re-submitted.

Edge cases:

  • Swipe the sheet down mid-setup — the order is not submitted; re-submitting reopens the sheet with completed steps still checked.
  • Reject a signature request — a snackbar shows and the step stays incomplete.
  • Open the gate on a wallet that already finished setup — the sheet dismisses itself immediately.

QA Testing

  • Verify all perps order actions (place order, market/limit close, manage TP/SL) still gate on trading setup and present the new formsheet, on both iOS and Android.
  • Verify checklist steps enable strictly in order (each button disabled until the previous step is done) and completed steps show a check.
  • Expected: behavior is identical to the previous inline sheet; only the presentation changed to a full-height stacked formsheet.

Checklist

Please check all that apply (if applicable)

  • I have performed a self-review of my code
  • I have verified the code works
  • I have included screenshots / videos of android and ios
  • I have added testing steps
  • I have added/updated necessary unit tests
  • I have updated the documentation

🤖 Generated with Claude Code

@bogdandobritoiu
bogdandobritoiu requested a review from atn4z7 as a code owner July 20, 2026 20:17
Copilot AI review requested due to automatic review settings July 20, 2026 20:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Converts the perps “Set up trading account” checklist from an inline per-screen React Native Modal to an Expo Router modal route (stacked K2 formsheet), so it can be presented consistently above other perps modals and dismissed via native sheet interactions.

Changes:

  • Added a new (modals)/perpetualsEnableTrading modal route and registered it under secondaryModalScreensOptions.
  • Reworked the enable-trading UI into PerpetualsEnableTradingScreen using ScrollScreen isModal, and updated the enable-trading gate hook to navigate to the route instead of rendering a local modal node.
  • Polished k2-alpine Checklist row visuals (text variant, completion icon, and loading state behavior) to match the design.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/k2-alpine/src/components/Checklist/Checklist.tsx Updates checklist row typography and trailing affordance (check icon / loading indicator / action button).
packages/core-mobile/app/new/routes/(signedIn)/(modals)/perpetualsEnableTrading/index.tsx Exposes the new enable-trading modal route screen as the route default export.
packages/core-mobile/app/new/routes/(signedIn)/(modals)/perpetualsEnableTrading/_layout.tsx Adds a modal stack layout wrapper for the new route, consistent with other perps modal routes.
packages/core-mobile/app/new/routes/(signedIn)/_layout.tsx Registers the new modal route with secondaryModalScreensOptions so it presents above other perps modals.
packages/core-mobile/app/new/features/trade/perpetuals/screens/PerpetualsPlaceOrderScreen.tsx Removes the in-tree modal render-prop and relies on route-based gating instead.
packages/core-mobile/app/new/features/trade/perpetuals/screens/PerpetualsPlaceOrderScreen.test.tsx Updates mocks to match the simplified gate hook API (no enableTradingModal).
packages/core-mobile/app/new/features/trade/perpetuals/screens/PerpetualsManageScreen.tsx Removes the in-tree modal render-prop and relies on route-based gating instead.
packages/core-mobile/app/new/features/trade/perpetuals/screens/PerpetualsEnableTradingScreen.tsx Replaces the old RN Modal implementation with a formsheet-style modal route screen and adds auto-dismiss behavior.
packages/core-mobile/app/new/features/trade/perpetuals/screens/PerpetualsCloseScreen.tsx Removes the in-tree modal render-prop and relies on route-based gating instead.
packages/core-mobile/app/new/features/trade/perpetuals/hooks/usePerpsEnableTradingGate.ts Switches the gate from local modal state to router.navigate('/perpetualsEnableTrading').
Comments suppressed due to low confidence (2)

packages/core-mobile/app/new/features/trade/perpetuals/screens/PerpetualsEnableTradingScreen.tsx:43

  • dismiss() sets dismissedRef.current = true even when router.canGoBack() is false, which can leave the sheet stuck open (and future dismiss attempts become no-ops). Only mark dismissed once you actually dismiss, and prefer router.dismiss() for modal routes when possible.
    packages/core-mobile/app/new/features/trade/perpetuals/hooks/usePerpsEnableTradingGate.ts:53
  • The enable-trading gate now navigates to a modal route when trading isn’t enabled, but there’s no unit test covering the new navigation behavior (e.g., that router.navigate('/perpetualsEnableTrading') is invoked and the handler bails out). Adding a focused hook test (or updating an existing perps screen test) would help prevent regressions that could block order submission.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

Coverage report ✅

2/2 packages passed thresholds
Thresholds are shown inline against each package baseline.

🟢 @avalabs/core-mobile

St. Category Percentage Covered / Total
🟢 Statements 27.83% (+9.83% vs 18% ▲) 12292/44160
🟢 Branches 23.47% (+11.47% vs 12% ▲) 6027/25676
🟢 Functions 22.08% (+9.08% vs 13% ▲) 2252/10195
🟢 Lines 27.91% (+9.91% vs 18% ▲) 11772/42169

🟢 @avalabs/k2-alpine

St. Category Percentage Covered / Total
🟢 Statements 7.62% (+5.62% vs 2% ▲) 270/3539
🟢 Branches 7.02% (+6.02% vs 1% ▲) 151/2148
🟢 Functions 4.52% (+3.52% vs 1% ▲) 37/817
🟢 Lines 7.07% (+5.07% vs 2% ▲) 230/3251
Artifacts and threshold sources
  • @avalabs/core-mobile: summary core-mobile/coverage/coverage-summary.json, thresholds core-mobile/coverage-thresholds.json
  • @avalabs/k2-alpine: summary k2-alpine/coverage/coverage-summary.json, thresholds k2-alpine/coverage-thresholds.json

Source run: Mobile PR

@B0Y3R-AVA B0Y3R-AVA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran an adversarial pass on this (Claude + Codex cross-check). The conversion itself is clean: the rename left no dead refs, all four gate call sites still treat requireTradingEnabled as a sync boolean, and nothing else consumes the k2 Checklist so that change is contained.

Found two real bugs though, both the same shape: the modal used to be local state, now it's global navigation, so async work that finishes after the user has already left can still poke the nav stack. Details inline.

One more thing worth eyeballing when you do the device recordings: this is the first secondary sheet we present on top of another secondary sheet (perpetualsClose). Every other secondaryModalScreensOptions screen opens from a primary modal. Probably fine, but nobody has seen the stacked detents/gestures on a real device yet.

return
}
dismissedRef.current = true
router.canGoBack() && router.back()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dismissedRef only gets set by our own dismiss() calls. Nothing flips it when the user swipe-dismisses the sheet natively.

So: tap "Enable unified account", swipe the sheet down while the tx is in flight. When the promise resolves, dismiss() still runs and router.back() fires against a stack where this sheet is already gone, popping the place-order/close/manage screen underneath it. The old code just did setVisible(false), harmless after unmount, so this is new with the route conversion.

Android makes it worse: a surplus back() can close the app (there's a comment about exactly that in (signedIn)/_layout.tsx), and navigationGuard patches push/navigate/replace but not back().

I'd set dismissedRef from a beforeRemove listener, or check navigation.isFocused() before calling back(). Dismissing by route identity instead of raw back() would also do it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 89547fd. A beforeRemove listener now marks the sheet dismissed on any removal (same pattern as PerpetualsOnboardingScreen), so a continuation resolving after a native swipe-dismiss no-ops instead of firing back() — or, since the slide-to-order change, replace()ing the underlying screen into place-order, which made this worse than the original report.

const { submitOrder } = usePerpsOrderSubmit()
const { requireTradingEnabled, enableTradingModal } =
usePerpsEnableTradingGate()
const { requireTradingEnabled } = usePerpsEnableTradingGate()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same shape of bug on the caller side (the code is unchanged in this PR but the gate's behavior changed under it). In handleConfirm, the geo re-check awaits before the gate, and the gate now navigates instead of setting local state. If the user swipe-dismisses the place-order sheet while the re-check is in flight (it can look hung), the continuation still runs and router.navigate('/perpetualsEnableTrading') opens the setup sheet over whatever screen they're on now, after they already bailed. Previously setVisible(true) on the unmounted screen was a no-op.

A mounted/focused check after the await covers it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 89547fd, in the gate hook rather than this call site: requireTradingEnabled() now checks navigation.isFocused() before presenting the sheet (it still returns false, so the submit aborts either way). Hook-level covers all four call sites — place-order is the only one with an await before the gate today, but close/manage won't regress if one grows an async precheck.

/>
)
) : loading ? (
<ActivityIndicator size="small" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc nits while you're in here: the loading docstring on ChecklistItem still says the spinner shows "in place of the action label" (it now replaces the whole button), and the done docstring still names Icons.Action.CheckCircle as the default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 89547fddone now names Icons.Navigation.Check and loading says the spinner replaces the button.

…tinue into place order

For a first-time user (funded, one-time setup incomplete) the market details
Short/Long slide now presents the enable-trading formsheet instead of place
order, carrying the selected side; when the last setup step completes the
sheet replaces itself with place order for that side. While setup status is
still loading the slide goes straight to place order so set-up users are
never detoured; the submit-time gate stays as the deep-link backstop.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 22, 2026 09:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

packages/core-mobile/app/new/features/trade/perpetuals/screens/PerpetualsEnableTradingScreen.tsx:53

  • dismissedRef.current is set to true before any navigation occurs. If coin/side aren’t present and router.canGoBack() is false, dismiss() won’t actually dismiss the sheet but will permanently no-op on subsequent calls (including the completion effect). Consider only flipping the guard once a navigation action is taken.

…creens

Review fixes (PR #3995): the sheet was local state before the route
conversion, so async continuations finishing after the user left were
harmless; as navigation they poke the stack. Mark the sheet dismissed via a
beforeRemove listener so a swipe-dismiss mid-step can't fire a stray
back()/replace(), and have requireTradingEnabled skip presenting the sheet
when its caller is no longer focused (place-order awaits a geo re-check
before gating). Also refresh stale Checklist docstrings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 22, 2026 09:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

…ailed dismisses

Review follow-ups (PR #3995): add a focused hook test for
usePerpsEnableTradingGate (navigation on incomplete setup, focus bail-out,
builder-fee-undefined semantics, status-loading composition), and only latch
the sheet's dismiss guard when a navigation actually fires so a failed
dismiss stays retryable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 22, 2026 09:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

# Conflicts:
#	packages/core-mobile/app/new/features/trade/perpetuals/screens/PerpetualsPlaceOrderScreen.tsx
Copilot AI review requested due to automatic review settings July 22, 2026 21:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

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.

3 participants