minor UI fixes after QA review #396
Conversation
WalkthroughAdds wallet portfolio fetching state and refetch propagation from AppWrapper to Search via new props. Updates Search to display loading and trigger refetch for wallet portfolio. Enhances PreviewSell with a click-outside-to-close behavior using a ref and document listener. No public API changes except new optional Search props. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant AppWrapper
participant Search
participant WalletQuery as useGetWalletPortfolioQuery
AppWrapper->>WalletQuery: subscribe { data, isFetching, refetch }
AppWrapper->>Search: props { walletPortfolioFetching, refetchWalletPortfolio }
User->>Search: Click Refresh
Search->>WalletQuery: refetch()
WalletQuery-->>AppWrapper: isFetching = true
AppWrapper-->>Search: walletPortfolioFetching = true
Note over Search: Refresh shows loading/disabled
WalletQuery-->>AppWrapper: data loaded, isFetching = false
AppWrapper-->>Search: walletPortfolioFetching = false
sequenceDiagram
autonumber
actor User
participant PreviewSell
participant Document
PreviewSell->>PreviewSell: mount, set ref on wrapper
PreviewSell->>Document: addEventListener('mousedown', handler)
User->>Document: mousedown outside wrapper
Document->>PreviewSell: handler detects outside
PreviewSell->>PreviewSell: closePreview()
PreviewSell-->>Document: cleanup listener on unmount
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks (2 warnings, 1 inconclusive)❌ Failed checks (2 warnings, 1 inconclusive)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/apps/pulse/components/Sell/PreviewSell.tsx (2)
80-96: Harden outside-click handling for touch, portals, and in-flight actions.Current listener is
mousedownand always closes, which can miss touch on some devices and can close during an in‑flight sell. Consider:
- Use
pointerdownfor unified mouse/touch.- Ignore while
isExecutingto prevent accidental close mid-flow.- Use
event.composedPath()(fallback tocontains) to better handle shadow DOM/portals.- (Optional) use capture to avoid interference from
stopPropagation.Proposed diff:
- // Click outside to close functionality - useEffect(() => { - const handleClickOutside = (event: MouseEvent) => { - if ( - previewModalRef.current && - !previewModalRef.current.contains(event.target as Node) - ) { - closePreview(); - } - }; - - document.addEventListener('mousedown', handleClickOutside); - return () => { - document.removeEventListener('mousedown', handleClickOutside); - }; - }, [closePreview]); + // Click outside to close functionality + useEffect(() => { + const handlePointerDown = (event: PointerEvent) => { + if (isExecuting || event.defaultPrevented) return; + const el = previewModalRef.current; + if (!el) return; + const path = typeof event.composedPath === 'function' ? event.composedPath() : []; + const clickedInside = path.length ? path.includes(el) : el.contains(event.target as Node); + if (!clickedInside) closePreview(); + }; + // capture=true to survive stopPropagation in descendants + document.addEventListener('pointerdown', handlePointerDown, true); + return () => { + document.removeEventListener('pointerdown', handlePointerDown, true); + }; + }, [closePreview, isExecuting]);
235-238: Add basic dialog a11y attributes.Mark the container as a dialog for assistive tech; optional: manage focus when opened.
- <div - ref={previewModalRef} - className="flex flex-col w-full max-w-[446px] bg-[#1E1D24] border border-white/5 rounded-[10px] p-6" - > + <div + ref={previewModalRef} + role="dialog" + aria-modal="true" + aria-label="Sell preview" + className="flex flex-col w-full max-w-[446px] bg-[#1E1D24] border border-white/5 rounded-[10px] p-6" + >I can also wire focus management (set initial focus, trap within dialog, restore on close) if you’d like.
src/apps/pulse/components/Search/Search.tsx (2)
45-48: TyperefetchWalletPortfolioto match RTK Query’s return (promise)
Typing it as() => Promise<unknown>is more accurate and helps downstream callersawaitit if needed.interface SearchProps { @@ - refetchWalletPortfolio?: () => void; + refetchWalletPortfolio?: () => Promise<unknown>; }
300-305: Guard undefined handler and include initial load in disabled/spinner statesRefresh.tsx defines onClick as optional (onClick?: () => void); use optional chaining for the handler and include the initial-loading flag in isLoading/disabled to avoid triggering refetch during initial load.
File: src/apps/pulse/components/Search/Search.tsx (lines 300-305)
- <Refresh - isLoading={walletPortfolioFetching} - onClick={refetchWalletPortfolio} - disabled={!refetchWalletPortfolio || walletPortfolioFetching} - /> + <Refresh + isLoading={walletPortfolioLoading || walletPortfolioFetching} + onClick={() => refetchWalletPortfolio?.()} + disabled={ + !refetchWalletPortfolio || + walletPortfolioLoading || + walletPortfolioFetching + } + />src/apps/pulse/components/App/AppWrapper.tsx (1)
57-62: Only passrefetchwhen a wallet address exists
Prevents a no-op refetch when the query is skipped and simplifies disabling logic in the child.- walletPortfolioFetching={walletPortfolioFetching} + walletPortfolioFetching={walletPortfolioFetching} walletPortfolioError={!!walletPortfolioError} - refetchWalletPortfolio={refetchWalletPortfolio} + refetchWalletPortfolio={accountAddress ? refetchWalletPortfolio : undefined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/pulse/components/Search/tests/__snapshots__/Search.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/apps/pulse/components/App/AppWrapper.tsx(2 hunks)src/apps/pulse/components/Search/Search.tsx(3 hunks)src/apps/pulse/components/Sell/PreviewSell.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
PR: pillarwallet/x#275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Search/Search.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Search/Search.tsx (1)
src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-34)
🔇 Additional comments (4)
src/apps/pulse/components/Sell/PreviewSell.tsx (2)
1-1: Import update looks good.
useRefis required for the outside-click handling introduced below.
50-50: Ref setup is appropriate.Using a typed
useRef<HTMLDivElement>(null)for the modal root is correct for containment checks.src/apps/pulse/components/Search/Search.tsx (1)
70-73: Destructuring new props — looks good
Props are optional and safely passed down.src/apps/pulse/components/App/AppWrapper.tsx (1)
21-26: Good: exposeisFetchingfor UI refresh controls
SurfacingisFetchingaswalletPortfolioFetchingaligns with RTK Query semantics.
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit