feat(wallet): add ConnectWalletButton and isConnectionRestoring state#7468
feat(wallet): add ConnectWalletButton and isConnectionRestoring state#7468kernelwhisperer wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds restoring-connection detection and UI: new useIsRestoringConnection hook/export, WalletInfo flag, ConnectWalletButton component, Web3Status/Web3StatusInner prop and rendering changes, affiliate CTAs updated, and localization for "Restoring wallet...". ChangesWallet Restoring State Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Deploying swap-dev with
|
| Latest commit: |
00dab29
|
| Status: | 🚫 Build failed. |
![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
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/wallet/containers/ConnectWalletButton/index.tsx (1)
10-10: 💤 Low valueConsider exporting
ConnectWalletButtonPropsThe type is internal but since
ConnectWalletButtonis part of the public module API (re-exported fromwallet/index.ts), exporting its props type allows consumers to build typed wrappers or pass forwarded props without having to re-derive the intersection type.♻️ Proposed change
-type ConnectWalletButtonProps = ButtonPrimaryProps & { children?: ReactNode } +export type ConnectWalletButtonProps = ButtonPrimaryProps & { children?: ReactNode }And add the re-export to
wallet/index.ts:export { ConnectWalletButton } from './containers/ConnectWalletButton' +export type { ConnectWalletButtonProps } from './containers/ConnectWalletButton'As per coding guidelines, "Public module API must be re-exported via the module
index.ts" — the exported component's companion props type is part of that surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-frontend/src/modules/wallet/containers/ConnectWalletButton/index.tsx` at line 10, Export the ConnectWalletButtonProps type so consumers can use its shape: change the declaration of ConnectWalletButtonProps (currently type ConnectWalletButtonProps = ButtonPrimaryProps & { children?: ReactNode }) to an exported type and add a corresponding re-export from the wallet module index (so the public API exposes ConnectWalletButtonProps alongside ConnectWalletButton); update any imports that should rely on the public re-export instead of internal paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/cowswap-frontend/src/modules/wallet/containers/ConnectWalletButton/index.tsx`:
- Around line 17-24: The button currently spreads {...rest} before setting
disabled and onClick so caller-supplied disabled/onClick are ignored; fix
ConnectWalletButton by computing a final disabled value (e.g., const
finalDisabled = isConnectionRestoring || !!rest.disabled) and use
disabled={finalDisabled} on ButtonPrimary, then spread {...rest} (or spread
first and explicitly set disabled={finalDisabled} after) so a caller's
disabled=true is respected; keep the onClick override to call toggleWalletModal
when not restoring but add an inline comment above onClick in
ConnectWalletButton explaining the onClick override is intentional to avoid
future confusion.
---
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/wallet/containers/ConnectWalletButton/index.tsx`:
- Line 10: Export the ConnectWalletButtonProps type so consumers can use its
shape: change the declaration of ConnectWalletButtonProps (currently type
ConnectWalletButtonProps = ButtonPrimaryProps & { children?: ReactNode }) to an
exported type and add a corresponding re-export from the wallet module index (so
the public API exposes ConnectWalletButtonProps alongside ConnectWalletButton);
update any imports that should rely on the public re-export instead of internal
paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e0896c4-d74f-4898-9c78-1c02c5a2b9a8
📒 Files selected for processing (10)
apps/cowswap-frontend/src/locales/en-US.poapps/cowswap-frontend/src/modules/affiliate/containers/AffiliatePartnerOnboard.tsxapps/cowswap-frontend/src/modules/affiliate/containers/AffiliateTraderOnboard.tsxapps/cowswap-frontend/src/modules/wallet/containers/ConnectWalletButton/index.tsxapps/cowswap-frontend/src/modules/wallet/containers/Web3Status/index.tsxapps/cowswap-frontend/src/modules/wallet/index.tsapps/cowswap-frontend/src/modules/wallet/pure/Web3StatusInner/index.tsxlibs/ui/src/pure/Button/index.tsxlibs/wallet/src/api/types.tslibs/wallet/src/wagmi/updater.ts
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
hey @kernelwhisperer , the issue is partially fixed, I think.
The original issue is not 100% addressed:
- open any of affiliate pages
- connect to a wallet
- refresh the page --> the page blinks with the 'connect wallet' banner before the cards with stats are loaded.
|
@elena-zh try now please 🙏 |
|
Hey @kernelwhisperer , kinda... Might it be possible to show a generic loader on the page until the data is fetched? |
|
@elena-zh, there should be only 2 loading states left:
We do this, that's the state number 2
Could you record a video please? |
kernelwhisperer
force-pushed
the
feat/wallet-restoring-spinner
branch
from
658558f to
8192d2b
Compare
I think I found the issue, but I'm not sure. I pushed a new commit: 8192d2b |
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
It looks better now, thank you.
However, I still see 3 different screen states when the page is being loaded https://www.loom.com/share/12868e49884f41dcaef64d1cb4bf24ff
Let's ask @cowprotocol/frontend team for their opinion: if they are Ok with the current implementation, I'll approve the PR.
Those 2 screens, the ones I described above 1.Restoring connection and 2.Fetching data, the reason you might think they are 3 is because the image did not load immediately and caused a layout refresh. |
|
@kernelwhisperer , i DO understand why they're displayed and appear there. My point is a bit different (and the issue I initially reported): I'm not in favor of showing 2-3 different screens while the page content is being loaded. I'm in favor of showing one generic loader that hides all these states, so there is not visible that the page changes 2-3 states while it is loading. |
4 participants
Summary
ConnectWalletButtonwrapper and uses it on both affiliate onboarding screens.Restoring wallet...and disabling clicks while the connection is reconnecting. This avoids the affiliate pages briefly showing an actionableConnect walletCTA during wallet restoration.ButtonPrimaryPropsfrom@cowprotocol/uiso the wrapper can forward normalButtonPrimaryprops directly.To Test
Restoring wallet...Connect walletduring restorationConnect walletAdd codeBackground
Before Wagmi/Viem, this was not practical with ethers.js because we did not have a reliable wallet “reconnecting/restoring” state. Wagmi exposes this via
useConnection().status === 'reconnecting'.Summary by CodeRabbit