feat: revert "fix: revert new mobile wallet popover P1 (#10422)" #10430
Conversation
📝 WalkthroughWalkthroughAdds a New Wallet Manager feature flag and integrates a new header popover flow gated by the flag. Introduces WalletManagerPopover, WalletButton, PopoverWallet, and PopoverWalletHeader. Updates header routing, compact table views, and related styling. Extends config, preferences, and test mocks to include the flag. Minor unrelated UI control tweak. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Header
participant FeatureFlags as Prefs/FeatureFlags
participant WalletMgr as WalletManagerPopover
participant Button as WalletButton
participant Pop as PopoverWallet
participant Store as Wallet/Portfolio Store
User->>Header: Load app
Header->>Prefs/FeatureFlags: useFeatureFlag('NewWalletManager')
Prefs/FeatureFlags-->>Header: true/false
alt Flag enabled
Header->>WalletMgr: Render
WalletMgr->>Button: Render trigger (connected/locked/loading)
User->>Button: Click
alt Connected and unlocked
Button->>WalletMgr: onOpen
WalletMgr->>Pop: Render PopoverWallet
Pop->>Store: Read wallet info, balances, accounts
User->>Pop: Select tab / Send / Receive / Switch / Disconnect
Pop-->>WalletMgr: onClose (row click or action)
WalletMgr->>Button: Close popover
else Not connected
Button->>WalletMgr: onConnect
WalletMgr->>Store: Dispatch open wallet modal
end
else Flag disabled
Header-->>User: Render existing UserMenu
end
sequenceDiagram
autonumber
actor User
participant Pop as PopoverWallet
participant HeaderComp as PopoverWalletHeader
participant Providers as Wallet Providers
participant Modal as Settings Modal
User->>Pop: Open Wallet Popover
Pop->>HeaderComp: Render wallet header
HeaderComp->>Providers: Resolve provider label/icon (RDNS/MIPD)
User->>HeaderComp: Click kebab (⋯)
alt Settings
HeaderComp->>Modal: Open settings modal
else Switch Provider
HeaderComp->>Providers: Trigger switch flow
else Disconnect
HeaderComp->>Providers: Disconnect wallet
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/pages/Dashboard/components/AccountList/AccountTable.tsx (1)
48-51: Don't mutate selector-derived arrays: copy before sorting.
rowData.sort(...)mutates the array returned by the selector, risking state corruption and subtle UI bugs. Clone before sorting.Apply:
- const sortedRows = useMemo(() => { - return rowData.sort((a, b) => Number(b.fiatAmount) - Number(a.fiatAmount)) - }, [rowData]) + const sortedRows = useMemo(() => { + return [...rowData].sort((a, b) => Number(b.fiatAmount) - Number(a.fiatAmount)) + }, [rowData])
🧹 Nitpick comments (20)
src/components/Layout/Header/DegradedStateBanner.tsx (4)
20-20: Optional: Prefer Chakra's useDisclosure over manual state.Using
useDisclosurereduces boilerplate and aligns with Chakra patterns; you can drop local handlers.Example changes (plus import
useDisclosurefrom@chakra-ui/react):-import { memo, useCallback, useMemo, useState } from 'react' +import { memo, useCallback, useMemo } from 'react'And:
-const [isOpen, setIsOpen] = useState(false) +const { isOpen, onOpen, onClose } = useDisclosure()Then:
-<Popover isOpen={isOpen} onOpen={handleOpen} onClose={handleClose}> +<Popover isOpen={isOpen} onOpen={onOpen} onClose={onClose}>
106-108: Handlers are fine; removable if switching to useDisclosure.If you adopt
useDisclosure, delete these and passonOpen/onClosedirectly.-const handleOpen = useCallback(() => setIsOpen(true), []) -const handleClose = useCallback(() => setIsOpen(false), []) +// handled by useDisclosure
149-149: Controlled Popover: LGTM.This addresses phantom popovers. If issues persist, consider
isLazywithlazyBehavior='unmount'on Popover instead of manual gating.
158-182: Conditional PopoverContent render solves phantom DOM; add a test id for E2E.Helps Ops/QE validate the absence/presence reliably.
-<PopoverContent overflow='hidden'> +<PopoverContent data-testid='degraded-state-popover' overflow='hidden'>Also minor nit: the icon var is named
idMdRefreshIconbut the import isIoMdRefresh. Consider renaming for consistency..env.production (1)
6-6: Sort the new flag above QUICK_BUY to satisfy dotenv-linter.Keeps flags alphabetized and removes the UnorderedKey warning.
Apply:
-VITE_FEATURE_QUICK_BUY=false -VITE_FEATURE_NEW_WALLET_MANAGER=false +VITE_FEATURE_NEW_WALLET_MANAGER=false +VITE_FEATURE_QUICK_BUY=false.env.development (1)
10-10: Sort the new flag above QUICK_BUY to satisfy dotenv-linter.Alphabetize within the feature flags block.
Apply:
-VITE_FEATURE_QUICK_BUY=true -VITE_FEATURE_NEW_WALLET_MANAGER=true +VITE_FEATURE_NEW_WALLET_MANAGER=true +VITE_FEATURE_QUICK_BUY=true.env (1)
54-54: Keep feature flags alphabetized; place NEW_WALLET_MANAGER with other NEW_ flags.*Removes the UnorderedKey warning and keeps grouping consistent.
Apply:
VITE_FEATURE_NEW_WALLET_FLOW=true VITE_FEATURE_NEW_LIMIT_FLOW=true +VITE_FEATURE_NEW_WALLET_MANAGER=false VITE_FEATURE_THORCHAIN_SWAPPER_ACK=false VITE_FEATURE_ACTION_CENTER=true VITE_FEATURE_QUICK_BUY=false -VITE_FEATURE_NEW_WALLET_MANAGER=false VITE_FEATURE_CHATWOOT=truesrc/pages/Dashboard/Dashboard.tsx (1)
53-58: Good color-mode-aware selection state; consider theme tokens over raw colors.If the design system exposes semantic tokens (e.g., text.inverse, bg.accent), prefer those over 'black'/'white' for better theming.
src/pages/Dashboard/components/AccountList/AccountTable.tsx (2)
119-121: Return 0 for equal values in sorters to avoid unstable re-renders.Current sorters return only -1/1. Use a tri-state comparator for stability.
Apply:
- sortType: (a: RowProps, b: RowProps): number => - bnOrZero(a.original.priceChange).gt(bnOrZero(b.original.priceChange)) ? 1 : -1, + sortType: (a: RowProps, b: RowProps): number => { + const av = bnOrZero(a.original.priceChange) + const bv = bnOrZero(b.original.priceChange) + return av.comparedTo?.(bv) ?? (av.gt(bv) ? 1 : av.lt(bv) ? -1 : 0) + }, ... - sortType: (a: RowProps, b: RowProps): number => - bnOrZero(a.original.allocation).gt(bnOrZero(b.original.allocation)) ? 1 : -1, + sortType: (a: RowProps, b: RowProps): number => { + const av = bnOrZero(a.original.allocation) + const bv = bnOrZero(b.original.allocation) + return av.comparedTo?.(bv) ?? (av.gt(bv) ? 1 : av.lt(bv) ? -1 : 0) + },Also applies to: 143-145
38-41: Expose row to onRowClick consumers (and avoid accidental double actions).Forward the clicked row so parents can decide to navigate or intercept. Also prevents parent + local navigation both firing.
Apply:
type AccountTableProps = { forceCompactView?: boolean - onRowClick?: () => void + onRowClick?: (row: Row<AccountRowData>) => void } ... const handleRowClick = useCallback( (row: Row<AccountRowData>) => { vibrate('heavy') - onRowClick?.() + onRowClick?.(row) const { assetId } = row.original const url = assetId ? `/assets/${assetId}` : '' navigate(url) }, - [navigate, onRowClick], + [navigate, onRowClick], )If you keep the current signature, consider renaming the prop to
onTableRowClickto avoid confusion with the table prop of the same name.Also applies to: 185-193
src/components/Layout/Header/NavBar/WalletButton.tsx (3)
44-47: Defensive address validation beforegetAddressto prevent runtime throws.
getAddress()throws on invalid inputs. Guard withisAddresssouseEnsNamenever receives a bad address.Apply:
-import { getAddress } from 'viem' +import { getAddress, isAddress } from 'viem' ... - const { data: ensName } = useEnsName({ - address: walletInfo?.meta?.address ? getAddress(walletInfo.meta.address) : undefined, - }) + const { data: ensName } = useEnsName({ + address: + walletInfo?.meta?.address && isAddress(walletInfo.meta.address) + ? getAddress(walletInfo.meta.address) + : undefined, + })
122-125: Haptic feedback parity for Connect action.You vibrate on menu open; mirror that on connect for consistent UX.
Apply:
- <Button onClick={onConnect} leftIcon={connectIcon}> + <Button + onClick={() => { + vibrate('heavy') + onConnect() + }} + leftIcon={connectIcon} + >
32-39: Consider memoizing the component to reduce header re-renders.Header rerenders are frequent; wrapping in
memois low-risk and aligns with project guidance.Apply:
-import type { FC } from 'react' -import { useCallback, useEffect, useMemo, useState } from 'react' +import type { FC } from 'react' +import { memo, useCallback, useEffect, useMemo, useState } from 'react' ... -export const WalletButton: FC<WalletButtonProps> = ({ +export const WalletButton: FC<WalletButtonProps> = memo(({ isMenuContext = false, isConnected, walletInfo, onConnect, isLoadingLocalWallet, ...otherProps -}) => { +}) => { ... -} +})If typing gets noisy, alternatively export
export const WalletButton = memo((props: WalletButtonProps) => { ... }).src/pages/Home/WatchlistTable.tsx (1)
42-51: Invoke external onRowClick before navigation (minor naming nit)Calling onRowClick?.() before routing is fine. Consider renaming the prop to onAfterRowClick or onClose to reflect intent (no args).
-type WatchlistTableProps = { - forceCompactView?: boolean - onRowClick?: () => void - hideExploreMore?: boolean -} +type WatchlistTableProps = { + forceCompactView?: boolean + onAfterRowClick?: () => void + hideExploreMore?: boolean +} -export const WatchlistTable = ({ - forceCompactView = false, - hideExploreMore = false, - onRowClick, -}: WatchlistTableProps) => { +export const WatchlistTable = ({ + forceCompactView = false, + hideExploreMore = false, + onAfterRowClick, +}: WatchlistTableProps) => { ... - const handleRowClick = useCallback( + const handleRowClick = useCallback( (row: Row<Asset>) => { - onRowClick?.() + onAfterRowClick?.() vibrate('heavy') ... - }, [navigate, onRowClick], + }, [navigate, onAfterRowClick], )src/components/Layout/Header/NavBar/WalletManagerPopover.tsx (2)
23-27: Guard on locked state and use functional setStatePrevent opening when the wallet is locked, and prefer functional updater to avoid stale closures.
- const handleOpen = useCallback(() => { - if (!isConnected) return - setIsOpen(!isOpen) - }, [isConnected, isOpen]) + const handleOpen = useCallback(() => { + if (!isConnected || isLocked) return + setIsOpen(prev => !prev) + }, [isConnected, isLocked])
31-51: Popover content mount strategy — good; add isLazy for extra safetyConditionally rendering PopoverContent fixes the phantom-node issue. Consider adding isLazy to Popover to ensure Chakra defers content mount universally.
- <Popover isOpen={isOpen} onOpen={handleOpen} onClose={handleClose} placement='bottom-end'> + <Popover isOpen={isOpen} onOpen={handleOpen} onClose={handleClose} placement='bottom-end' isLazy>src/components/Layout/Header/NavBar/PopoverWallet.tsx (2)
95-102: Close the popover when launching Send/Receive to avoid lingering popover DOM.Fire
onClose?.()when opening modals so the popover is dismissed immediately. This aligns with the “phantom popover” fix goal.Apply this diff:
const handleSendClick = useCallback(() => { - send.open({}) - }, [send]) + send.open({}) + onClose?.() + }, [send, onClose]) const handleReceiveClick = useCallback(() => { - receive.open({}) - }, [receive]) + receive.open({}) + onClose?.() + }, [receive, onClose])
16-17: Prefer ReactNode import over React namespace type.Avoid relying on the
Reactnamespace. ImportReactNodedirectly.Apply this diff:
-import type { FC } from 'react' +import type { FC, ReactNode } from 'react' @@ type ActionButtonProps = { - icon: React.ReactNode + icon: ReactNode label: string onClick: () => void isDisabled?: boolean }Also applies to: 36-41
src/components/Layout/Header/NavBar/PopoverWalletHeader.tsx (2)
81-86: Localize aria-labels for accessibility.Use translated aria labels instead of hardcoded English.
Apply this diff:
<IconButton - aria-label='Settings' + aria-label={translate('common.settings')} rounded='full' icon={settingsIcon} size='sm' onClick={handleSettingsClick} /> <Menu> <MenuButton as={IconButton} rounded='full' - aria-label='Menu' + aria-label={translate('common.menu')} icon={dotsIcon} size='sm' />Also applies to: 88-94
95-120: Use theme z-index token instead of magic number.Prefer semantic tokens for layering consistency across the app.
Apply this diff:
- <MenuList zIndex={2}> + <MenuList zIndex='dropdown'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
.env(1 hunks).env.development(1 hunks).env.production(1 hunks)src/components/Layout/Header/DegradedStateBanner.tsx(5 hunks)src/components/Layout/Header/Header.tsx(3 hunks)src/components/Layout/Header/NavBar/PopoverWallet.tsx(1 hunks)src/components/Layout/Header/NavBar/PopoverWalletHeader.tsx(1 hunks)src/components/Layout/Header/NavBar/UserMenu.tsx(2 hunks)src/components/Layout/Header/NavBar/WalletButton.tsx(1 hunks)src/components/Layout/Header/NavBar/WalletManagerPopover.tsx(1 hunks)src/components/MarketsTable.tsx(1 hunks)src/config.ts(1 hunks)src/pages/Dashboard/Dashboard.tsx(3 hunks)src/pages/Dashboard/components/AccountList/AccountTable.tsx(8 hunks)src/pages/Home/WatchlistTable.tsx(3 hunks)src/state/slices/preferencesSlice/preferencesSlice.ts(2 hunks)src/test/mocks/store.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
src/state/slices/preferencesSlice/preferencesSlice.tssrc/config.tssrc/components/Layout/Header/NavBar/PopoverWalletHeader.tsxsrc/components/Layout/Header/NavBar/WalletManagerPopover.tsxsrc/components/Layout/Header/DegradedStateBanner.tsxsrc/components/Layout/Header/NavBar/PopoverWallet.tsxsrc/components/Layout/Header/NavBar/WalletButton.tsxsrc/test/mocks/store.tssrc/pages/Dashboard/Dashboard.tsxsrc/components/MarketsTable.tsxsrc/components/Layout/Header/NavBar/UserMenu.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Home/WatchlistTable.tsxsrc/components/Layout/Header/Header.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
src/state/slices/preferencesSlice/preferencesSlice.tssrc/config.tssrc/components/Layout/Header/NavBar/PopoverWalletHeader.tsxsrc/components/Layout/Header/NavBar/WalletManagerPopover.tsxsrc/components/Layout/Header/DegradedStateBanner.tsxsrc/components/Layout/Header/NavBar/PopoverWallet.tsxsrc/components/Layout/Header/NavBar/WalletButton.tsxsrc/test/mocks/store.tssrc/pages/Dashboard/Dashboard.tsxsrc/components/MarketsTable.tsxsrc/components/Layout/Header/NavBar/UserMenu.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Home/WatchlistTable.tsxsrc/components/Layout/Header/Header.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
src/state/slices/preferencesSlice/preferencesSlice.tssrc/config.tssrc/components/Layout/Header/NavBar/PopoverWalletHeader.tsxsrc/components/Layout/Header/NavBar/WalletManagerPopover.tsxsrc/components/Layout/Header/DegradedStateBanner.tsxsrc/components/Layout/Header/NavBar/PopoverWallet.tsxsrc/components/Layout/Header/NavBar/WalletButton.tsxsrc/test/mocks/store.tssrc/pages/Dashboard/Dashboard.tsxsrc/components/MarketsTable.tsxsrc/components/Layout/Header/NavBar/UserMenu.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Home/WatchlistTable.tsxsrc/components/Layout/Header/Header.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.tsx: ALWAYS wrap components in error boundaries
ALWAYS provide user-friendly fallback components in error boundaries
ALWAYS log errors for debugging in error boundaries
ALWAYS use useErrorToast hook for displaying errors
ALWAYS provide translated error messages in error toasts
ALWAYS handle different error types appropriately in error toasts
Missing error boundaries in React components is an anti-pattern
**/*.tsx: ALWAYS use PascalCase for React component names
ALWAYS use descriptive names that indicate the component's purpose
ALWAYS match the component name to the file name
Flag components without PascalCase
Flag default exports for components
Files:
src/components/Layout/Header/NavBar/PopoverWalletHeader.tsxsrc/components/Layout/Header/NavBar/WalletManagerPopover.tsxsrc/components/Layout/Header/DegradedStateBanner.tsxsrc/components/Layout/Header/NavBar/PopoverWallet.tsxsrc/components/Layout/Header/NavBar/WalletButton.tsxsrc/pages/Dashboard/Dashboard.tsxsrc/components/MarketsTable.tsxsrc/components/Layout/Header/NavBar/UserMenu.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Home/WatchlistTable.tsxsrc/components/Layout/Header/Header.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
**/*.{tsx,jsx}: ALWAYS useuseMemofor expensive computations, object/array creations, and filtered data
ALWAYS useuseMemofor derived values and computed properties
ALWAYS useuseMemofor conditional values and simple transformations
ALWAYS useuseCallbackfor event handlers and functions passed as props
ALWAYS useuseCallbackfor any function that could be passed as a prop or dependency
ALWAYS include all dependencies inuseEffect,useMemo,useCallbackdependency arrays
NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary
ALWAYS explain why dependencies are excluded if using eslint disable
ALWAYS use named exports for components
NEVER use default exports for components
KEEP component files under 200 lines when possible
BREAK DOWN large components into smaller, reusable pieces
EXTRACT complex logic into custom hooks
USE local state for component-level state
LIFT state up when needed across multiple components
USE Context for avoiding prop drilling
ALWAYS wrap components in error boundaries for production
ALWAYS handle async errors properly
ALWAYS provide user-friendly error messages
ALWAYS use virtualization for lists with 100+ items
ALWAYS implement proper key props for list items
ALWAYS lazy load heavy components
ALWAYS use React.lazy for code splitting
Components receiving props are wrapped withmemo
Files:
src/components/Layout/Header/NavBar/PopoverWalletHeader.tsxsrc/components/Layout/Header/NavBar/WalletManagerPopover.tsxsrc/components/Layout/Header/DegradedStateBanner.tsxsrc/components/Layout/Header/NavBar/PopoverWallet.tsxsrc/components/Layout/Header/NavBar/WalletButton.tsxsrc/pages/Dashboard/Dashboard.tsxsrc/components/MarketsTable.tsxsrc/components/Layout/Header/NavBar/UserMenu.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Home/WatchlistTable.tsxsrc/components/Layout/Header/Header.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/ButtonWalletPredicate/ButtonWalletPredicate.tsx:7-7
Timestamp: 2025-08-27T09:47:06.275Z
Learning: In shapeshift/web project, NeOMakinG consistently prefers to defer UI/UX improvements and refactoring work (like the Drawer.Close hack fix in ButtonWalletPredicate.tsx) to follow-up PRs rather than expanding the scope of feature PRs, even when the improvements would enhance robustness.
Learnt from: gomesalexandre
PR: shapeshift/web#10206
File: src/config.ts:127-128
Timestamp: 2025-08-07T11:20:44.614Z
Learning: gomesalexandre prefers required environment variables without default values in the config file (src/config.ts). They want explicit configuration and fail-fast behavior when environment variables are missing, rather than having fallback defaults.
Learnt from: gomesalexandre
PR: shapeshift/web#10249
File: src/pages/ThorChainLP/components/ReusableLpStatus/TransactionRow.tsx:447-503
Timestamp: 2025-08-13T17:07:10.763Z
Learning: gomesalexandre prefers relying on TypeScript's type system for validation rather than adding defensive runtime null checks when types are properly defined. They favor a TypeScript-first approach over defensive programming with runtime validations.
Learnt from: gomesalexandre
PR: shapeshift/web#10276
File: src/hooks/useActionCenterSubscribers/useThorchainLpDepositActionSubscriber.tsx:61-66
Timestamp: 2025-08-14T17:51:47.556Z
Learning: gomesalexandre is not concerned about structured logging and prefers to keep console.error usage as-is rather than implementing structured logging patterns, even when project guidelines suggest otherwise.
Learnt from: gomesalexandre
PR: shapeshift/web#10413
File: src/components/Modals/FiatRamps/fiatRampProviders/onramper/utils.ts:29-55
Timestamp: 2025-09-02T14:26:19.028Z
Learning: gomesalexandre prefers to keep preparatory/reference code simple until it's actively consumed, rather than implementing comprehensive error handling, validation, and robustness improvements upfront. They prefer to add these improvements when the code is actually being used in production.
Learnt from: gomesalexandre
PR: shapeshift/web#10276
File: src/pages/ThorChainLP/components/ReusableLpStatus/TransactionRow.tsx:396-402
Timestamp: 2025-08-14T17:55:57.490Z
Learning: gomesalexandre is comfortable with functions/variables that return undefined or true (tri-state) when only the truthy case matters, preferring to rely on JavaScript's truthy/falsy behavior rather than explicitly returning boolean values.
Learnt from: gomesalexandre
PR: shapeshift/web#10206
File: src/lib/moralis.ts:47-85
Timestamp: 2025-08-07T11:22:16.983Z
Learning: gomesalexandre prefers console.error over structured logging for Moralis API integration debugging, as they find it more conventional and prefer to examine XHR requests directly rather than rely on structured logs for troubleshooting.
Learnt from: gomesalexandre
PR: shapeshift/web#10418
File: src/Routes/RoutesCommon.tsx:231-267
Timestamp: 2025-09-03T21:17:27.681Z
Learning: gomesalexandre prefers to keep PR diffs focused and reasonable in size, deferring tangential improvements (like Mixpanel privacy enhancements) to separate efforts rather than expanding the scope of feature PRs.
Learnt from: gomesalexandre
PR: shapeshift/web#10276
File: src/hooks/useActionCenterSubscribers/useThorchainLpActionSubscriber.tsx:46-53
Timestamp: 2025-08-14T19:21:45.426Z
Learning: gomesalexandre does not like code patterns being labeled as "technical debt" and prefers neutral language when discussing existing code patterns that were intentionally moved/maintained for consistency.
📚 Learning: 2025-08-22T13:02:38.078Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/Explore/Explore.tsx:91-92
Timestamp: 2025-08-22T13:02:38.078Z
Learning: In the ShapeShift web codebase, feature flags are consistently used as string literals with the useFeatureFlag hook (e.g., useFeatureFlag('RfoxFoxEcosystemPage')), not as enums. The hook accepts keyof FeatureFlags which provides type safety through the TypeScript type system rather than requiring an enum. This is the established pattern used throughout the entire codebase.
Applied to files:
src/state/slices/preferencesSlice/preferencesSlice.tssrc/components/Layout/Header/Header.tsx
📚 Learning: 2025-07-29T10:27:23.424Z
Learnt from: NeOMakinG
PR: shapeshift/web#10128
File: .cursor/rules/react-best-practices.mdc:8-14
Timestamp: 2025-07-29T10:27:23.424Z
Learning: The ShapeShift team practices aggressive memoization in React components as documented in .cursor/rules/react-best-practices.mdc. They use useMemo for all transformations, derived values, and conditional values, and useCallback for all event handlers and functions that could be passed as props. This approach was adopted after experiencing performance issues ("had hard time") and is their current established practice, though they acknowledge it may evolve in the future.
Applied to files:
src/components/Layout/Header/DegradedStateBanner.tsx
📚 Learning: 2025-08-14T17:54:32.563Z
Learnt from: gomesalexandre
PR: shapeshift/web#10276
File: src/pages/ThorChainLP/components/ReusableLpStatus/ReusableLpStatus.tsx:97-108
Timestamp: 2025-08-14T17:54:32.563Z
Learning: In ReusableLpStatus component (src/pages/ThorChainLP/components/ReusableLpStatus/ReusableLpStatus.tsx), the txAssets dependency is stable from first render because poolAsset, baseAsset, actionSide, and action are all defined first render, making the current txAssetsStatuses initialization pattern safe without needing useEffect synchronization.
Applied to files:
src/components/Layout/Header/DegradedStateBanner.tsx
📚 Learning: 2025-07-30T20:57:48.176Z
Learnt from: NeOMakinG
PR: shapeshift/web#10148
File: src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx:88-91
Timestamp: 2025-07-30T20:57:48.176Z
Learning: In src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx, NeOMakinG prefers keeping the hardcoded overscan value (40) over dynamic calculation based on viewport height, prioritizing code simplicity over precision when the current approach works effectively.
Applied to files:
src/components/MarketsTable.tsx
📚 Learning: 2025-08-03T22:10:38.426Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/react-best-practices.mdc:0-0
Timestamp: 2025-08-03T22:10:38.426Z
Learning: Applies to **/*.{tsx,jsx} : ALWAYS use `useMemo` for conditional values and simple transformations
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
🧬 Code graph analysis (10)
src/state/slices/preferencesSlice/preferencesSlice.ts (1)
src/config.ts (1)
getConfig(202-204)
src/components/Layout/Header/NavBar/PopoverWalletHeader.tsx (8)
src/hooks/useModal/useModal.tsx (1)
useModal(6-24)src/state/store.ts (1)
useAppSelector(143-143)src/state/slices/localWalletSlice/selectors.ts (1)
selectWalletRdns(21-24)src/lib/mipd.ts (1)
useMipdProviders(7-8)src/context/WalletProvider/config.ts (1)
SUPPORTED_WALLETS(339-444)src/components/Layout/Header/NavBar/WalletImage.tsx (1)
WalletImage(10-16)src/pages/Dashboard/components/ProfileAvatar/ProfileAvatar.tsx (1)
ProfileAvatar(11-28)src/components/Text/Text.tsx (1)
Text(19-83)
src/components/Layout/Header/NavBar/WalletManagerPopover.tsx (2)
src/components/Layout/Header/NavBar/WalletButton.tsx (1)
WalletButton(32-126)src/components/Layout/Header/NavBar/PopoverWallet.tsx (1)
PopoverWallet(72-192)
src/components/Layout/Header/DegradedStateBanner.tsx (1)
src/components/Text/Text.tsx (1)
Text(19-83)
src/components/Layout/Header/NavBar/PopoverWallet.tsx (7)
src/hooks/useModal/useModal.tsx (1)
useModal(6-24)src/context/AppProvider/hooks/useDiscoverAccounts.tsx (1)
useDiscoverAccounts(17-150)src/state/store.ts (1)
useAppSelector(143-143)src/state/slices/portfolioSlice/selectors.ts (2)
selectPortfolioTotalUserCurrencyBalance(187-196)selectIsAnyPortfolioGetAccountLoading(1018-1023)src/components/Layout/Header/NavBar/PopoverWalletHeader.tsx (1)
PopoverWalletHeader(40-125)src/pages/Dashboard/components/AccountList/AccountTable.tsx (1)
AccountTable(43-219)src/pages/Home/WatchlistTable.tsx (1)
WatchlistTable(25-75)
src/components/Layout/Header/NavBar/WalletButton.tsx (6)
src/state/store.ts (1)
useAppSelector(143-143)src/state/slices/localWalletSlice/selectors.ts (1)
selectWalletRdns(21-24)src/lib/mipd.ts (1)
useMipdProviders(7-8)src/components/Layout/Header/NavBar/WalletImage.tsx (1)
WalletImage(10-16)src/lib/vibrate.ts (1)
vibrate(6-16)src/components/Text/Text.tsx (2)
RawText(15-17)Text(19-83)
src/components/MarketsTable.tsx (7)
src/theme/theme.tsx (1)
breakpoints(37-44)src/state/store.ts (1)
useAppSelector(143-143)src/state/slices/marketDataSlice/selectors.ts (2)
selectMarketDataUserCurrency(19-55)selectIsAnyMarketDataApiQueryPending(155-156)src/hooks/useModal/useModal.tsx (1)
useModal(6-24)src/state/apis/fiatRamps/hooks.ts (1)
useFetchFiatAssetMarketData(8-20)src/hooks/useInfiniteScroll/useInfiniteScroll.tsx (1)
useInfiniteScroll(11-49)src/components/ReactTable/ReactTableNoPager.tsx (1)
ReactTableNoPager(48-198)
src/pages/Dashboard/components/AccountList/AccountTable.tsx (3)
src/theme/theme.tsx (1)
breakpoints(37-44)src/state/slices/portfolioSlice/selectors.ts (1)
AccountRowData(726-736)src/lib/vibrate.ts (1)
vibrate(6-16)
src/pages/Home/WatchlistTable.tsx (2)
src/lib/vibrate.ts (1)
vibrate(6-16)src/components/MarketsTable.tsx (1)
MarketsTable(38-180)
src/components/Layout/Header/Header.tsx (2)
src/components/Layout/Header/NavBar/WalletManagerPopover.tsx (1)
WalletManagerPopover(11-52)src/components/Layout/Header/NavBar/UserMenu.tsx (1)
UserMenu(62-113)
🪛 dotenv-linter (3.3.0)
.env.development
[warning] 10-10: [UnorderedKey] The VITE_FEATURE_NEW_WALLET_MANAGER key should go before the VITE_FEATURE_QUICK_BUY key
(UnorderedKey)
.env
[warning] 54-54: [UnorderedKey] The VITE_FEATURE_NEW_WALLET_MANAGER key should go before the VITE_FEATURE_OPTIMISM key
(UnorderedKey)
.env.production
[warning] 6-6: [UnorderedKey] The VITE_FEATURE_NEW_WALLET_MANAGER key should go before the VITE_FEATURE_QUICK_BUY key
(UnorderedKey)
⏰ 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 (25)
src/components/Layout/Header/DegradedStateBanner.tsx (1)
50-50: State init is fine.Local
isOpenfixes phantom DOM by making the popover controlled.src/config.ts (1)
189-190: Approve NewWalletManager plumbing Verified the flag insrc/config.ts, wired throughpreferencesSlicestate (NewWalletManager: getConfig().VITE_FEATURE_NEW_WALLET_MANAGER), and consumed viauseFeatureFlag('NewWalletManager')inHeader.tsx; LGTM.src/pages/Dashboard/Dashboard.tsx (1)
103-108: LGTM on Tab styling usage.using _selected plus a memoized style and static unselected style is clean and avoids re-renders.
src/pages/Dashboard/components/AccountList/AccountTable.tsx (2)
59-64: Verify header/compact logic across breakpoints.
isCompactCols = !isLargerThanLg || forceCompactViewanddisplayHeaders = isLargerThanMd && !forceCompactViewmeans headers are hidden whenforceCompactViewis true even on wide screens. Confirm this matches the compact-view UX in Markets/Watchlist tables.Also applies to: 211-211
147-148: Columns memoization deps look correct.
isCompactCols,stackTextAlign, andtextColorfully cover dynamic bits used in the column defs.src/components/Layout/Header/NavBar/WalletButton.tsx (2)
88-91: LGTM on haptics callback.Isolated vibration side-effect in
handleMenuClickkeeps the render tree pure.
20-21: Exportedentriesconstant looks fine.Assuming it feeds connected-route UI, a single-entry array is acceptable.
Confirm no other routes need inclusion after reverting the revert.
src/test/mocks/store.ts (1)
164-165: Mock flag addition is correct.
NewWalletManagerdefaulting to false keeps tests deterministic and aligns with feature gating.Ensure the flag exists in
preferencesSlicetypes and config validation, and update tests that rely on the new wallet manager paths.src/components/Layout/Header/Header.tsx (3)
14-14: Add popover import — matches feature scopeImporting WalletManagerPopover here is appropriate and localized to the header. No issues.
57-57: Feature flag usage follows established string-literal patternuseFeatureFlag('NewWalletManager') aligns with the codebase convention (string literal keys). Good.
123-125: Flag-gated swap between UserMenu and WalletManagerPopover is correctly implemented
Verified presence ofuseFeatureFlag('NewWalletManager')at line 57 and the conditional rendering at line 124 insrc/components/Layout/Header/Header.tsx. Ensure e2e tests cover both branches of this flag.src/state/slices/preferencesSlice/preferencesSlice.ts (2)
92-93: Typed flag added to FeatureFlagsNewWalletManager added to the type—good for compiler coverage.
215-216: Config validator and env variables verified — no further action required.src/pages/Home/WatchlistTable.tsx (2)
19-23: Prop additions enable compact popover UXProps forceCompactView and hideExploreMore, plus onRowClick passthrough, are sensible defaults and backward compatible.
Also applies to: 25-29
67-72: Explore CTA toggleConditional CTA via hideExploreMore matches popover needs. LGTM.
src/components/Layout/Header/NavBar/WalletManagerPopover.tsx (1)
35-40: Consistent data-test attributeKeeping data-test='navigation-wallet-dropdown-button' consistent with UserMenu helps test stability during rollout.
src/components/MarketsTable.tsx (5)
35-36: Prop and responsive compaction are coherentforceCompactView integrates well with header/compact scenarios and responsive breakpoints.
Also applies to: 38-51
56-68: Row click/Trade click event separation looks correctStopping propagation in handleTradeClick prevents accidental row navigation. Good pattern.
Also applies to: 142-150
119-131: Dynamic column visibility is cleanUsing display to collapse columns in compact mode keeps the table simple. LGTM.
Also applies to: 133-141, 143-150
161-179: Table wiring and loading statedisplayHeaders toggling and isLoading passthrough are correct. No issues.
82-94: SparkLine column — verify render performance on large watchlists
MarketsTable.tsx (lines 82–94) renders<SparkLine>directly for every row without memoization. For large tables, wrap this cell in a memoized component (e.g. reuseSparkLineCellinsrc/components/MarketTableVirtualized/SparkLineCell.tsx) or memoize theCellrenderer byassetId. Profile initial render cost in the popover and introduce lazy-loading or memoization if you observe frame drops.src/components/Layout/Header/NavBar/PopoverWallet.tsx (1)
111-191: Solid structure and state management.Nice memoization of tab styles, handlers, and lazy tab panels. Spinner integration tied to discovery/loading states is clean.
src/components/Layout/Header/NavBar/UserMenu.tsx (2)
80-109: WalletButton extraction and menu wiring look good.The trigger works in menu context and preserves the
data-testselector. PassingonClosethrough toWalletConnectedis tidy.
62-107: No tests referencedata-test="navigation-wallet-dropdown-button"—add or update tests to target this selectorSearch found the attribute in UserMenu and WalletManagerPopover but zero test files match it. Ensure existing tests are updated or new ones added to cover the wallet dropdown button.
src/components/Layout/Header/NavBar/PopoverWalletHeader.tsx (1)
69-78: Header composition and label resolution look good.Memoized label/provider resolution and avatar/image usage are clean.
NeOMakinG
left a comment
NeOMakinG
left a comment
There was a problem hiding this comment.
Wallet modal seems to work as expected
And degraded state popover too:
No more ghost elements
It makes me thing we may have this weird behavior in other parts of our apps
…9efe5371beb68860bd
NeOMakinG
deleted the
feat_revert_revert_faed627e730b43505f7a9e9efe5371beb68860bd
branch
... and fix phantom popover
Description
Revert the revert of the wallet drawer, and fix phantom popover.
While at it, also fixes the other phantom popover in app.
See c4bc6e4 for the actual fix in this diff - basically ensures popover is controlled as it should - failure to do so means it will always be displayed, although with its content hidden, but still taking space, and still rugging clicks.
Issue (if applicable)
closes #10285
Risk
See #10395
Testing
popover__body(note double__and notice no phantom elements)Engineering
Operations
Screenshots (if applicable)
Summary by CodeRabbit