Skip to content

Conversation

@NeOMakinG
Copy link
Collaborator

@NeOMakinG NeOMakinG commented Jul 29, 2025

Description

  • Going back from create wallet wasn't working
  • Responsive height of wallet modal was too hight we couldn't access the close and back buttons
  • Clicking on manage accounts was leaving the wallet dialog behind
  • Switch mobile wallet dialog to the global modal context
  • Adapt all the handlers so it opens the modal accordingly on both login screen and in app
  • Keep the responsive drawer if it's not the mobile app
  • Close the drawer on disconnect

Issue (if applicable)

Found why trying to reproduce #10102 which I couldn't reproduce

Risk

Low

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

  • Open the wallet modal using Expo
  • Try to click on create a new wallet
    • Click on go back
  • Click on manage accounts and see the behind drawer is closed
  • Using the responsive and not the mobile app, open the wallet modal, notice any part of this is working as expected and you can access the top button (back and close)

Engineering

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

Mobile app bugs

https://jam.dev/c/72337213-7533-4109-9584-a7c60222853a

responsive bugs

https://jam.dev/c/72337213-7533-4109-9584-a7c60222853a

Desktop:
image

Mobile dialog changes test:
https://jam.dev/c/06d776d7-a8b5-48d5-8754-d6c0c0865c85

Summary by CodeRabbit

  • New Features

    • Added support for setting a default route when opening the mobile wallet dialog, allowing for more flexible navigation.
    • Introduced a new default route option in the wallet dialog routes.
    • Added back navigation capability to the Manage Accounts modal.
  • Enhancements

    • Improved dialog responsiveness with adaptive minimum and maximum height settings for better display across devices.
    • Updated redirection logic in wallet creation flows for more dynamic navigation.
    • Centralized mobile wallet dialog control using a global modal system for consistent behavior across the app.
    • Updated user header interactions to open the appropriate modal or drawer based on platform context.
  • Bug Fixes

    • Fixed an issue where the dialog did not close when managing accounts from the saved wallets menu; the dialog now closes as expected.

@NeOMakinG NeOMakinG requested a review from a team as a code owner July 29, 2025 18:33
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 29, 2025

📝 Walkthrough

Walkthrough

The changes introduce a new Default route to the mobile wallet dialog, refactor the routing logic to allow dynamic redirection, and enhance responsiveness in wallet view containers. The dialog's open state is managed via a global modal hook instead of local state. The dashboard and history pages update their user header click handlers to open the wallet dialog modal on mobile. The manage accounts modal gains a back button capability, and modal registration and typing are extended to include the mobile wallet dialog.

Changes

Cohort / File(s) Change Summary
Mobile Wallet Dialog Routing Refactor
src/components/MobileWalletDialog/routes/CreateWallet/CreateWalletRouter.tsx, src/components/MobileWalletDialog/types.ts
Added Default route to MobileWalletDialogRoutes enum. Refactored CreateWalletRouter and CreateWalletRoutes to accept a new onRedirectToDefaultRoute prop, lifting redirection logic and making it dynamic based on the defaultRoute.
Mobile Wallet Dialog Modal Integration
src/components/MobileWalletDialog/MobileWalletDialog.tsx, src/context/ModalProvider/ModalContainer.tsx, src/context/ModalProvider/types.ts
Refactored MobileWalletDialog to use useModal hook for open state and closing. Registered mobileWalletDialog modal in modal container and updated modal types to include it.
Saved Wallets Dialog Behavior
src/components/MobileWalletDialog/routes/SavedWallets.tsx
Modified the manage accounts menu item click handler to close the dialog immediately after opening the account management modal. Added a disconnect click handler that closes the dialog after disconnecting.
Manage Accounts Modal Back Navigation
src/components/Modals/ManageAccounts/ManageAccountsModal.tsx
Added optional onBack prop with a back button in the modal header that triggers onBack callback and closes the modal.
Wallet Views Responsiveness
src/context/WalletProvider/NewWalletViews/NewWalletViewsSwitch.tsx
Replaced fixed minimum height for the wallet views container with responsive min/max height values for improved UI adaptability across screen sizes.
Dashboard & History Header Mobile Drawer Updates
src/pages/Dashboard/components/DashboardHeader/DashboardHeader.tsx, src/pages/History/History.tsx
Removed direct rendering of MobileWalletDialog in mobile drawer. Introduced useModal('mobileWalletDialog') hook and updated user header click handlers to open the wallet dialog modal on mobile devices, otherwise open existing drawers.
MobileConnect Modal State Refactor
src/pages/ConnectWallet/MobileConnect.tsx
Replaced local state and useDisclosure with useModal('mobileWalletDialog') hook to control mobile wallet dialog modal. Removed direct rendering of MobileWalletDialog.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant DashboardHeader
    participant MobileWalletDialog
    participant CreateWalletRouter
    participant CreateWalletRoutes

    User->>DashboardHeader: Click user header (mobile)
    DashboardHeader->>MobileWalletDialog: open modal via useModal
    MobileWalletDialog->>CreateWalletRouter: pass defaultRoute
    CreateWalletRouter->>CreateWalletRoutes: pass onRedirectToDefaultRoute callback
    CreateWalletRoutes->>CreateWallet: handleRedirectToHome = onRedirectToDefaultRoute
    CreateWallet-->>CreateWalletRoutes: User triggers redirect
    CreateWalletRoutes-->>CreateWalletRouter: call onRedirectToDefaultRoute
    CreateWalletRouter-->>MobileWalletDialog: navigate to defaultRoute
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~15 minutes

Suggested reviewers

  • premiumjibles
  • 0xApotheosis

Poem

A rabbit hops through routes anew,
With dialogs now more smart and true.
Responsive heights and paths that flex,
No more hardcoded redirect specs!
On mobile screens, the wallet's saved—
A smoother hop, a bug well-braved.
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4dfe09 and e592d1c.

📒 Files selected for processing (1)
  • src/pages/History/History.tsx (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10128
File: .cursor/rules/error-handling.mdc:266-274
Timestamp: 2025-07-29T10:35:22.059Z
Learning: NeOMakinG prefers less nitpicky suggestions on documentation and best practices files, finding overly detailed suggestions on minor implementation details (like console.error vs logger.error) too granular for cursor rules documentation.
src/pages/History/History.tsx (1)

Learnt from: NeOMakinG
PR: #10139
File: src/components/MultiHopTrade/components/TradeConfirm/components/ExpandableStepperSteps.tsx:109-115
Timestamp: 2025-07-29T15:04:28.083Z
Learning: In src/components/MultiHopTrade/components/TradeConfirm/components/ExpandableStepperSteps.tsx, the component is used under an umbrella that 100% of the time contains the quote, making the type assertion activeTradeQuote?.steps[currentHopIndex] as TradeQuoteStep safe. Adding conditional returns before hooks would violate React's Rules of Hooks.

⏰ 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). (1)
  • GitHub Check: Install and Cache
🔇 Additional comments (4)
src/pages/History/History.tsx (4)

55-55: LGTM: Proper integration with global modal system.

The addition of the mobileWalletDialog modal hook follows the established pattern and correctly integrates with the global modal context refactor.


62-62: LGTM: Correct mobile app handling.

The change to return null for mobile apps is appropriate since the mobile wallet dialog is now handled through the global modal context instead of being rendered within the drawer.


68-71: LGTM: Clean separation of mobile and non-mobile behavior.

The handleUserHeaderClick callback correctly implements the conditional logic to use the global modal for mobile apps while maintaining existing drawer behavior for non-mobile. The useCallback implementation with proper dependencies is well done.


87-87: LGTM: Proper callback integration.

The update to use handleUserHeaderClick correctly connects the MobileUserHeader to the new conditional modal handling logic, maintaining the component interface while enabling the global modal behavior for mobile apps.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch blue-bar-android

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Collaborator

@premiumjibles premiumjibles left a comment

Choose a reason for hiding this comment

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

Personally happy with this behaviour and will also make a different task i'm working on easier to do with removing background scrolling with the drawers open on mobile 😁 .

63b961f6-5ca4-45ef-b7a1-74d4a6d6b72b.mp4

Do just want to flag @0xApotheosis to make sure you're cool with this. This change results in the manage accounts modal no longer going back to the saved wallets menu when you close it. You mentioned wanting to maintain this behaviour the other day.

@0xApotheosis
Copy link
Member

Hmm, good callout @premiumjibles.

I think this behaviour is ok if we can add a back button to the "Manage Accounts" action sheet that goes back to the saved wallets menu?

@NeOMakinG
Copy link
Collaborator Author

@0xApotheosis @premiumjibles I have updated the PR so it adds a back button to the management dialog if it comes from the mobile dialog, for this I had to change the dialog and move it to the global modal context, so we can open it from anywhere in the app

I also fixed a few behavior like closing the dialog on disconnect, and adapted all handlers so it works with the new modal handling, I did more tests but I would appreciate some more eyes again even if it's approved already as I changed a lot of things after the first review pass

Copy link
Collaborator

@premiumjibles premiumjibles left a comment

Choose a reason for hiding this comment

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

Looks great nice one @NeOMakinG 🤌

9da129e9-5fbb-4ec8-bd37-85c9cc57a3c2.mp4

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.

4 participants