Improve Mobile detection in WC connection #8415
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
WalkthroughisMobile adds a viewport-width heuristic (window.innerWidth < 640). WalletConnect QR overlay gets a desktop-specific flow: dynamic import of overlay module, cleanup of any existing overlay, configurable overlay creation, and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / WalletConnect flow
participant Env as Runtime (browser)
participant OverlayModule as QR Overlay Module (dynamic)
rect rgba(0,128,0,0.06)
Note right of App: Desktop QR overlay path (new)
end
App->>Env: detect isMobile()
alt isMobile === false (desktop)
App->>Env: dynamic import OverlayModule
activate OverlayModule
OverlayModule-->>App: overlay factory
App->>OverlayModule: remove existing overlay (if any)
App->>OverlayModule: create overlay with {theme,size,onCancel}
OverlayModule-->>App: overlay instance / success
else isMobile === true (mobile)
Note right of App: use existing mobile flow (no desktop overlay)
end
par Error handling
OverlayModule--xApp: import/creation error
App->>App: handle error / fallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
size-limit report 📦
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8415 +/- ##
=======================================
Coverage 54.84% 54.85%
=======================================
Files 919 919
Lines 60846 60853 +7
Branches 4141 4142 +1
=======================================
+ Hits 33372 33378 +6
- Misses 27372 27373 +1
Partials 102 102
🚀 New features to boost your workflow:
|
Merge activity
|
<!--
## title your PR with this format: "[SDK/Dashboard/Portal] Feature/Fix: Concise title for the changes"
If you did not copy the branch name from Linear, paste the issue tag here (format is TEAM-0000):
## Notes for the reviewer
Anything important to call out? Be sure to also clarify these in your comments.
## How to test
Unit tests, playground, etc.
-->
<!-- start pr-codex -->
---
## PR-Codex overview
This PR enhances the `isMobile` function to include viewport detection for mobile devices and refines the QR overlay creation logic for desktop environments by ensuring existing overlays are cleaned up before creating new ones.
### Detailed summary
- Added viewport detection in `isMobile` function to return `true` if `window.innerWidth` is less than 640.
- Refactored QR overlay creation in `create-wallet.ts` to ensure existing overlays are destroyed before creating a new one.
> ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}`
<!-- end pr-codex -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved mobile device detection based on viewport size.
* Enhanced QR code overlay display with better error handling and resource cleanup.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
bot
force-pushed
the
11-14-improve_mobile_detection_in_wc_connection
branch
from
8a83711 to
304700f
Compare
![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 (2)
packages/thirdweb/src/utils/web/isMobile.ts (1)
58-81: Viewport-based fallback broadens “mobile” to narrow desktop windows; consider scope and add testsThe new
isMobileViewportfallback runs only after OS and touch/mobile-API checks, so the primary new cases that become “mobile” are environments where:
- OS is not Android/iOS,
- no touch/mobile APIs are detected, but
window.innerWidth < 640.That effectively treats narrow desktop browser windows as mobile. If that’s intentional (for WalletConnect UX, responsive layouts, etc.), it’s fine, but it does change semantics for every existing
isMobile()caller (including the session handler and WC flows increate-wallet.ts). Please double-check that all current usages are okay with “narrow viewport” being equivalent to mobile, or consider constraining this heuristic to the specific call sites that need it (or gating it behind an option).Also, the new viewport branch isn’t covered by tests (per codecov). It would be good to add unit tests that stub
window.innerWidtharound edge values (e.g. 639/640) to lock in the intended behavior.packages/thirdweb/src/wallets/create-wallet.ts (1)
352-371: QR overlay lifecycle is well-handled; consider adding tests for overlay creation/cleanupThe QR overlay management is carefully done:
- Destroys any existing
qrOverlaybefore creating a new one to avoid stacking overlays.- Cleans up the overlay in both the success path (lines 384–388) and error path (lines 403–407).
onCanceldelegates to any user-providedwalletConnect.onCancel, preserving existing hooks.- Dynamic import of
createQROverlayis scoped to the desktop branch and wrapped in atry/catch, so failures degrade gracefully.Given the new logic is not covered by tests (per codecov), it would be ideal to add tests that simulate
onDisplayUribeing called on desktop and verify that:
createQROverlayis invoked with the expected options (theme, size,onCancel).destroy()is called on successful connect and on error.You can mock
./wallet-connect/qr-overlay.jsandconnectWCto exercise these branches without relying on real DOM or WalletConnect behavior.Also applies to: 384-407
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/thirdweb/src/utils/web/isMobile.ts(2 hunks)packages/thirdweb/src/wallets/create-wallet.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 8106
File: packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx:482-485
Timestamp: 2025-09-23T19:56:43.668Z
Learning: In packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx, the EmbedContainer uses width: "100vw" intentionally rather than "100%" - this is by design for the bridge widget embedding use case.
🧬 Code graph analysis (1)
packages/thirdweb/src/wallets/create-wallet.ts (1)
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (1)
createQROverlay(59-296)
🪛 GitHub Check: codecov/patch
packages/thirdweb/src/wallets/create-wallet.ts
[warning] 345-349: packages/thirdweb/src/wallets/create-wallet.ts#L345-L349
Added lines #L345 - L349 were not covered by tests
[warning] 352-354: packages/thirdweb/src/wallets/create-wallet.ts#L352-L354
Added lines #L352 - L354 were not covered by tests
[warning] 357-371: packages/thirdweb/src/wallets/create-wallet.ts#L357-L371
Added lines #L357 - L371 were not covered by tests
packages/thirdweb/src/utils/web/isMobile.ts
[warning] 78-79: packages/thirdweb/src/utils/web/isMobile.ts#L78-L79
Added lines #L78 - L79 were not covered by tests
⏰ 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). (3)
- GitHub Check: Size
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/thirdweb/src/wallets/create-wallet.ts (1)
321-373: Remove concern aboutsessionHandlervsonDisplayUriconflict; only viewport-based UX change warrants attentionThe core concern in the original review is incorrect. When
create-wallet.tsprovides a customonDisplayUricallback (which it does),connectWCcompletely bypasses the fallbacksessionHandlerfor URI display. The logic is:At
connectWCline 94:if (!onDisplayUri && sessionHandler)— sinceonDisplayUriis provided bycreate-wallet.ts, this condition is false and thedeeplinkHandlerwrapper is never used. The custom callback has full control;sessionHandleris only used for session initialization/recovery, not for URI handling. No conflict or duplicate handling occurs.The only valid observation from the original review is that
isMobile()now includes a viewport-width heuristic, so narrow desktop windows will show the mobile deeplink UI instead of QR. That's an intentional UX shift worth confirming, but not a correctness issue.Likely an incorrect or invalid review comment.
graphite-app
bot
deleted the
11-14-improve_mobile_detection_in_wc_connection
branch
PR-Codex overview
This PR enhances the
isMobiledetection logic and refines the QR overlay creation process in the wallet functionality. It adds viewport checks for mobile devices and improves error handling when creating QR overlays.Detailed summary
isMobile.ts, added a check for mobile viewport size (less than 640px).create-wallet.ts, refined the QR overlay creation logic:Summary by CodeRabbit
Bug Fixes
New Features