[SDK] Fix showQrModal option not respected on desktop web #7711
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: aa6cffc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a QR code overlay modal for WalletConnect flows on desktop web, with dynamic UI creation and cancellation handling. It expands the WalletConnect options to support an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (hooks.tsx)
participant useConnect Hook
participant createWallet (WalletConnect)
participant QR Overlay
User->>UI (hooks.tsx): Initiate wallet connect
UI (hooks.tsx)->>useConnect Hook: connect()
useConnect Hook->>createWallet (WalletConnect): connect()
alt Injected provider available
createWallet (WalletConnect)-->>useConnect Hook: Connect via injected provider
else No injected provider
createWallet (WalletConnect)->>QR Overlay: Show QR modal
QR Overlay-->>User: Display QR code
User->>QR Overlay: Scan or Cancel
alt User scans QR
QR Overlay-->>createWallet (WalletConnect): Success
createWallet (WalletConnect)-->>QR Overlay: Destroy overlay
createWallet (WalletConnect)-->>useConnect Hook: Connected
else User cancels
QR Overlay-->>createWallet (WalletConnect): onCancel callback
createWallet (WalletConnect)-->>useConnect Hook: cancelConnection()
useConnect Hook-->>UI (hooks.tsx): Cancelled
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Playground
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
joaquim-verges
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
apps/playground-web/src/components/sign-in/hooks.tsx (1)
24-34: Good fallback logic, but remove debug logging.The conditional logic properly checks for injected provider availability and falls back to WalletConnect with QR modal. The cancellation handling is correctly wired.
Consider removing the console.log for production:
onCancel: () => { - console.log("onCancel"); connectMutation.cancelConnection(); },packages/thirdweb/src/wallets/create-wallet.ts (1)
305-386: Excellent QR overlay integration with room for improvement.The implementation correctly handles the QR overlay lifecycle with proper cleanup on both success and error. The dynamic import approach is efficient.
Consider these improvements:
- More robust browser detection:
- if ( - typeof window !== "undefined" && - typeof document !== "undefined" - ) { + if (typeof window !== "undefined" && typeof document !== "undefined" && document.body) {
- Better error isolation:
try { const { createQROverlay } = await import( "./wallet-connect/qr-overlay.js" ); // Clean up any existing overlay if (qrOverlay) { qrOverlay.destroy(); } // Create new QR overlay qrOverlay = createQROverlay(uri, { theme: wcOptions.walletConnect?.qrModalOptions ?.themeMode ?? "dark", qrSize: 280, showCloseButton: true, onCancel: () => { wcOptions.walletConnect?.onCancel?.(); }, }); } catch (error) { console.error( "Failed to create QR overlay:", error, ); + // Still call the original onDisplayUri if provided + wcOptions.walletConnect?.onDisplayUri?.(uri); }packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (4)
6-39: Prefer type aliases over interfaces for structural typesPer the coding guidelines, type aliases should be preferred over interfaces except for nominal shapes. These types represent structural configurations and API contracts, not nominal types.
-export interface QROverlayOptions { +export type QROverlayOptions = { /** * Custom styles to apply to the overlay */ overlayStyles?: Partial<CSSStyleDeclaration>; /** * Custom styles to apply to the modal container */ modalStyles?: Partial<CSSStyleDeclaration>; /** * QR code size in pixels */ qrSize?: number; /** * Show close button */ showCloseButton?: boolean; /** * Custom close button text */ closeButtonText?: string; /** * Theme preference */ theme?: "light" | "dark"; /** * Custom container element to append the overlay to */ container?: HTMLElement; /** * Callback called when user cancels the overlay (closes without connecting) */ onCancel?: () => void; -} +}; -export interface QROverlay { +export type QROverlay = { /** * Remove the overlay from the DOM */ destroy: () => void; /** * Hide the overlay (without removing from DOM) */ hide: () => void; /** * Show the overlay */ show: () => void; -} +};Also applies to: 41-54
193-204: Improve error handling for clipboard operationWhen clipboard access fails, the user isn't notified. Consider showing an error message to improve UX.
copyButton.addEventListener("click", async () => { try { await navigator.clipboard.writeText(uri); const originalText = copyButton.textContent; copyButton.textContent = "Copied!"; setTimeout(() => { copyButton.textContent = originalText; }, 2000); } catch (err) { console.error("Failed to copy URI:", err); + const originalText = copyButton.textContent; + copyButton.textContent = "Copy failed"; + copyButton.style.color = "#ff0000"; + setTimeout(() => { + copyButton.textContent = originalText; + copyButton.style.color = theme === "dark" ? "#ffffff" : "#000000"; + }, 2000); } });
310-328: Simplify error handling and add user feedback for fallbackThe error handling uses both try-catch and .catch() which is redundant. Also, the fallback QR pattern might confuse users who try to scan it.
try { // Try to dynamically import a QR code library if available // This allows the overlay to work with or without a QR code dependency - const QRCode = await import("qrcode").catch(() => null); + const QRCode = await import("qrcode"); - if (QRCode) { - await QRCode.toCanvas(canvas, text, { - width: size, - margin: 2, - color: { - dark: "#000000", - light: "#ffffff", - }, - }); - return; - } + await QRCode.toCanvas(canvas, text, { + width: size, + margin: 2, + color: { + dark: "#000000", + light: "#ffffff", + }, + }); + return; } catch (error) { - console.warn("QR code library not available, using fallback"); + console.warn("QR code library not available, using fallback", error); } // Fallback: Create a more sophisticated placeholder ctx.fillStyle = "#ffffff"; ctx.fillRect(0, 0, size, size); + + // Add text to indicate this is a placeholder + ctx.fillStyle = "#000000"; + ctx.font = `${size / 15}px Arial`; + ctx.textAlign = "center"; + ctx.textBaseline = "middle"; + ctx.fillText("QR Placeholder", size / 2, size / 2);
377-386: Extract magic numbers as named constantsThe function uses magic numbers (9, 16) that represent QR code structure. These should be named constants for clarity.
+const FINDER_PATTERN_SIZE = 9; // 7x7 pattern + 1 module separator +const QR_GRID_SIZE = 25; + function isInFinderPattern(i: number, j: number): boolean { return ( // Top-left finder pattern - (i < 9 && j < 9) || + (i < FINDER_PATTERN_SIZE && j < FINDER_PATTERN_SIZE) || // Top-right finder pattern - (i >= 16 && j < 9) || + (i >= QR_GRID_SIZE - FINDER_PATTERN_SIZE && j < FINDER_PATTERN_SIZE) || // Bottom-left finder pattern - (i < 9 && j >= 16) + (i < FINDER_PATTERN_SIZE && j >= QR_GRID_SIZE - FINDER_PATTERN_SIZE) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/social-dogs-bet.md(1 hunks)apps/playground-web/src/components/sign-in/hooks.tsx(2 hunks)apps/playground-web/src/components/sign-in/modal.tsx(0 hunks)packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts(1 hunks)packages/thirdweb/src/wallets/create-wallet.ts(2 hunks)packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts(1 hunks)packages/thirdweb/src/wallets/wallet-connect/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/playground-web/src/components/sign-in/modal.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Files:
packages/thirdweb/src/react/core/hooks/wallets/useConnect.tspackages/thirdweb/src/wallets/wallet-connect/types.tspackages/thirdweb/src/wallets/create-wallet.tsapps/playground-web/src/components/sign-in/hooks.tsxpackages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/react/core/hooks/wallets/useConnect.tspackages/thirdweb/src/wallets/wallet-connect/types.tspackages/thirdweb/src/wallets/create-wallet.tsapps/playground-web/src/components/sign-in/hooks.tsxpackages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/wallet-connect/types.tspackages/thirdweb/src/wallets/create-wallet.tspackages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
apps/{dashboard,playground-web}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
apps/{dashboard,playground-web}/**/*.{ts,tsx}: Import UI primitives from@/components/ui/*(Button, Input, Select, Tabs, Card, Sidebar, Badge, Separator) in dashboard and playground apps
UseNavLinkfor internal navigation with automatic active states in dashboard and playground apps
Use Tailwind CSS only – no inline styles or CSS modules
Usecn()from@/lib/utilsfor conditional class logic
Use design system tokens (e.g.,bg-card,border-border,text-muted-foreground)
Server Components (Node edge): Start files withimport "server-only";
Client Components (browser): Begin files with'use client';
Always callgetAuthToken()to retrieve JWT from cookies on server side
UseAuthorization: Bearerheader – never embed tokens in URLs
Return typed results (e.g.,Project[],User[]) – avoidany
Wrap client-side data fetching calls in React Query (@tanstack/react-query)
Use descriptive, stablequeryKeysfor React Query cache hits
ConfigurestaleTime/cacheTimein React Query based on freshness (default ≥ 60s)
Keep tokens secret via internal API routes or server actions
Never importposthog-jsin server components
Files:
apps/playground-web/src/components/sign-in/hooks.tsx
⏰ 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). (8)
- GitHub Check: Build Packages
- GitHub Check: Size
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
.changeset/social-dogs-bet.md (1)
1-6: LGTM! Changeset format is correct.The changeset properly documents the patch fix with appropriate metadata and clear description.
packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts (1)
82-85: LGTM! Clean cancellation implementation.The
cancelConnectionfunction correctly resets the connection state. The implementation usesuseCallbackappropriately with proper dependencies.packages/thirdweb/src/wallets/wallet-connect/types.ts (1)
160-172: LGTM! Well-documented callback type addition.The
onCancelcallback is properly typed as optional with comprehensive documentation. The distinction between user-initiated vs programmatic cancellation is clear and helpful.apps/playground-web/src/components/sign-in/hooks.tsx (1)
9-9: LGTM! Clean import addition.The import correctly adds
injectedProvideralongside the existingcreateWalletimport.packages/thirdweb/src/wallets/create-wallet.ts (2)
20-20: LGTM! Proper type import.The QROverlay type import is correctly added for the overlay management functionality.
303-303: Good overlay lifecycle management.The
qrOverlayvariable properly tracks the overlay instance for cleanup purposes.
| return { | ||
| connect: handleConnection, | ||
| error, | ||
| isConnecting, | ||
| cancelConnection, | ||
| } as const; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify no race conditions with ongoing connections.
The return object correctly exposes the new cancelConnection function. Ensure that calling cancelConnection during an active connection properly handles cleanup and doesn't cause race conditions with the ongoing handleConnection process.
🏁 Script executed:
#!/bin/bash
# Search for any cleanup logic in wallet connection flows that might need coordination with cancelConnection
ast-grep --pattern 'const $_ = useConnect($_)'Length of output: 913
🏁 Script executed:
#!/bin/bash
# Locate cancelConnection definition
grep -R "cancelConnection" -n packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts
# Inspect handleConnection implementation for cancellation logic
grep -R "handleConnection" -n packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts
# Search for AbortController usage to manage cancellation
grep -R "AbortController" -n packages/thirdweb/src/react/core/hooks/wallets/useConnect.tsLength of output: 423
🏁 Script executed:
#!/bin/bash
# Display the relevant portion containing handleConnection and cancelConnection implementations
sed -n '1,120p' packages/thirdweb/src/react/core/hooks/wallets/useConnect.tsLength of output: 2792
Coordinate cancellation to avoid racing state updates
The current cancelConnection (lines 82–85) only flips the React state but doesn’t actually stop the in-flight promise in handleConnection (lines 53–76). This means a cancelled connection can still resolve or reject later and overwrite your status flags. To fix:
• In packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts
– Introduce an AbortController (or a ref-based isCancelled flag) at the hook level.
– Pass its signal into both walletOrFn() and connect(...) so that pending work can be aborted.
– In cancelConnection, call controller.abort() (or set the flag) before resetting state.
Example diff:
+ const controllerRef = useRef<AbortController>();
const handleConnection = useCallback(
async (walletOrFn: Wallet | (() => Promise<Wallet>)) => {
+ const controller = new AbortController();
+ controllerRef.current = controller;
setError(null);
setConnectionStatus("connecting");
+ if (controller.signal.aborted) return null;
if (typeof walletOrFn !== "function") {
- const account = await connect(walletOrFn, options);
+ const account = await connect(walletOrFn, { ...options, signal: controller.signal });
setConnectionStatus("connected");
return account;
}
setIsConnecting(true);
try {
- const w = await walletOrFn();
+ const w = await Promise.race([
+ walletOrFn(),
+ new Promise((_, rej) =>
+ controller.signal.addEventListener("abort", () => rej(new Error("cancelled")))
+ ),
+ ]);
- const account = await connect(w, options);
+ const account = await connect(w, { ...options, signal: controller.signal });
setConnectionStatus("connected");
return account;
} catch (e) {
console.error(e);
setError(e as Error);
setConnectionStatus("disconnected");
} finally {
setIsConnecting(false);
}
return null;
},
[connect, options, setConnectionStatus],
);
const cancelConnection = useCallback(() => {
+ controllerRef.current?.abort();
setIsConnecting(false);
setConnectionStatus("disconnected");
}, [setConnectionStatus]);This ensures that calling cancelConnection truly aborts the ongoing flow and prevents late-settling promises from clobbering the UI.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| connect: handleConnection, | |
| error, | |
| isConnecting, | |
| cancelConnection, | |
| } as const; | |
| // Introduce a ref to hold the current AbortController | |
| const controllerRef = useRef<AbortController>(); | |
| const handleConnection = useCallback( | |
| async (walletOrFn: Wallet | (() => Promise<Wallet>)) => { | |
| // Create a fresh controller for this invocation | |
| const controller = new AbortController(); | |
| controllerRef.current = controller; | |
| setError(null); | |
| setConnectionStatus("connecting"); | |
| // If already aborted, bail out | |
| if (controller.signal.aborted) return null; | |
| if (typeof walletOrFn !== "function") { | |
| // Pass the abort signal into connect | |
| const account = await connect(walletOrFn, { ...options, signal: controller.signal }); | |
| setConnectionStatus("connected"); | |
| return account; | |
| } | |
| setIsConnecting(true); | |
| try { | |
| // Race wallet factory against an abort event | |
| const w = await Promise.race([ | |
| walletOrFn(), | |
| new Promise<never>((_, rej) => | |
| controller.signal.addEventListener("abort", () => rej(new Error("cancelled"))) | |
| ), | |
| ]); | |
| const account = await connect(w, { ...options, signal: controller.signal }); | |
| setConnectionStatus("connected"); | |
| return account; | |
| } catch (e) { | |
| console.error(e); | |
| setError(e as Error); | |
| setConnectionStatus("disconnected"); | |
| } finally { | |
| setIsConnecting(false); | |
| } | |
| return null; | |
| }, | |
| [connect, options, setConnectionStatus], | |
| ); | |
| const cancelConnection = useCallback(() => { | |
| // Abort any in-flight connection attempt | |
| controllerRef.current?.abort(); | |
| setIsConnecting(false); | |
| setConnectionStatus("disconnected"); | |
| }, [setConnectionStatus]); | |
| return { | |
| connect: handleConnection, | |
| error, | |
| isConnecting, | |
| cancelConnection, | |
| } as const; |
🤖 Prompt for AI Agents
In packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts around lines 53
to 92, the cancelConnection function only updates React state but does not stop
the ongoing async connection process, causing potential race conditions. To fix
this, add an AbortController or a ref-based cancellation flag at the hook level,
pass its signal or flag into walletOrFn() and connect() calls to allow aborting
the async operations, and modify cancelConnection to call controller.abort() or
set the cancellation flag before resetting state. This will ensure that
cancelling truly aborts the connection flow and prevents stale promise
resolutions from updating state.
| copyButton.textContent = "Copied!"; | ||
| setTimeout(() => { | ||
| copyButton.textContent = originalText; | ||
| }, 2000); | ||
| } catch (err) { | ||
| console.error("Failed to copy URI:", err); | ||
| } | ||
| }); | ||
|
|
||
| // Assemble the modal | ||
| qrContainer.appendChild(title); | ||
| qrContainer.appendChild(qrCanvas); | ||
| qrContainer.appendChild(copyButton); | ||
| modal.appendChild(qrContainer); | ||
| overlay.appendChild(modal); | ||
|
|
||
| // Add CSS animations | ||
| const style = document.createElement("style"); | ||
| style.textContent = ` | ||
| @keyframes fadeIn { | ||
| from { opacity: 0; } | ||
| to { opacity: 1; } | ||
| } | ||
| @keyframes fadeOut { | ||
| from { opacity: 1; } | ||
| to { opacity: 0; } | ||
| } | ||
| @keyframes scaleIn { | ||
| from { transform: scale(0.9); opacity: 0; } | ||
| to { transform: scale(1); opacity: 1; } | ||
| } | ||
| @keyframes scaleOut { | ||
| from { transform: scale(1); opacity: 1; } | ||
| to { transform: scale(0.9); opacity: 0; } | ||
| } | ||
| `; | ||
| document.head.appendChild(style); | ||
|
|
||
| // Event handlers | ||
| const handleEscapeKey = (event: KeyboardEvent) => { | ||
| if (event.key === "Escape") { | ||
| destroyOverlay(true); | ||
| } | ||
| }; | ||
|
|
||
| const handleOverlayClick = (event: MouseEvent) => { | ||
| if (event.target === overlay) { | ||
| destroyOverlay(true); | ||
| } | ||
| }; | ||
|
|
||
| // Add event listeners | ||
| document.addEventListener("keydown", handleEscapeKey); | ||
| overlay.addEventListener("click", handleOverlayClick); | ||
|
|
||
| // Append to container | ||
| container.appendChild(overlay); | ||
|
|
||
| function destroyOverlay(userInitiated = false) { | ||
| document.removeEventListener("keydown", handleEscapeKey); | ||
| overlay.removeEventListener("click", handleOverlayClick); | ||
|
|
||
| // Call onCancel callback only if user initiated the close action | ||
| if (userInitiated && onCancel) { | ||
| onCancel(); | ||
| } | ||
|
|
||
| // Animate both overlay and modal out | ||
| overlay.style.animation = "fadeOut 200ms ease-in"; | ||
| modal.style.animation = "scaleOut 200ms ease-in"; | ||
|
|
||
| const cleanup = () => { | ||
| if (overlay.parentNode) { | ||
| overlay.parentNode.removeChild(overlay); | ||
| } | ||
| if (style.parentNode) { | ||
| style.parentNode.removeChild(style); | ||
| } | ||
| }; | ||
|
|
||
| overlay.addEventListener("animationend", cleanup, { once: true }); | ||
|
|
||
| // Fallback cleanup in case animation doesn't fire | ||
| setTimeout(cleanup, 250); | ||
| } | ||
|
|
||
| function hideOverlay() { | ||
| overlay.style.display = "none"; | ||
| } | ||
|
|
||
| function showOverlay() { | ||
| overlay.style.display = "flex"; | ||
| } | ||
|
|
||
| return { | ||
| destroy: () => destroyOverlay(false), // Programmatic cleanup, don't call onCancel | ||
| hide: hideOverlay, | ||
| show: showOverlay, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function violates single-responsibility principle
This function is 237 lines long and handles multiple responsibilities including DOM creation, styling, event handling, animations, and lifecycle management. Per the coding guidelines, each file should contain "one stateless, single-responsibility function for clarity."
Consider breaking this into smaller, focused functions:
- DOM element creation functions
- Style management utilities
- Event handler setup/cleanup
- Animation management
- QR overlay lifecycle controller
The function should be decomposed into smaller utilities. Here's a suggested structure:
// Separate style constants
const OVERLAY_STYLES = {
light: { /* ... */ },
dark: { /* ... */ }
} as const;
// DOM creation utilities
function createOverlayElement(theme: "light" | "dark"): HTMLDivElement { /* ... */ }
function createModalElement(theme: "light" | "dark"): HTMLDivElement { /* ... */ }
function createCloseButton(theme: "light" | "dark", text: string): HTMLButtonElement { /* ... */ }
// Animation utilities
function injectAnimationStyles(): HTMLStyleElement { /* ... */ }
function animateOut(overlay: HTMLElement, modal: HTMLElement): Promise<void> { /* ... */ }
// Event management
function setupEventListeners(/* ... */): () => void { /* ... */ }
// Main orchestrator (much smaller)
export function createQROverlay(uri: string, options: QROverlayOptions = {}): QROverlay {
// Coordinate the smaller functions
}🤖 Prompt for AI Agents
In packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts from lines 59 to
296, the createQROverlay function is too large and handles multiple concerns
like DOM creation, styling, event handling, animations, and lifecycle
management. To fix this, refactor by extracting smaller, focused functions for
each responsibility: create separate utilities for DOM element creation
(overlay, modal, buttons), style management (constants or functions for styles),
event listener setup and cleanup, animation injection and control, and lifecycle
methods. Then have the main createQROverlay function orchestrate these smaller
functions to improve clarity and maintainability.
| const style = document.createElement("style"); | ||
| style.textContent = ` | ||
| @keyframes fadeIn { | ||
| from { opacity: 0; } | ||
| to { opacity: 1; } | ||
| } | ||
| @keyframes fadeOut { | ||
| from { opacity: 1; } | ||
| to { opacity: 0; } | ||
| } | ||
| @keyframes scaleIn { | ||
| from { transform: scale(0.9); opacity: 0; } | ||
| to { transform: scale(1); opacity: 1; } | ||
| } | ||
| @keyframes scaleOut { | ||
| from { transform: scale(1); opacity: 1; } | ||
| to { transform: scale(0.9); opacity: 0; } | ||
| } | ||
| `; | ||
| document.head.appendChild(style); |
There was a problem hiding this comment.
Prevent style element duplication
Multiple calls to createQROverlay will inject duplicate style elements into the document head. Consider checking if the styles already exist or using a shared style element.
// Add CSS animations
-const style = document.createElement("style");
-style.textContent = `
+const existingStyle = document.getElementById("walletconnect-qr-overlay-styles");
+if (!existingStyle) {
+ const style = document.createElement("style");
+ style.id = "walletconnect-qr-overlay-styles";
+ style.textContent = `
@keyframes fadeIn {
from { opacity: 0; }
to { opacity: 1; }
}
@keyframes fadeOut {
from { opacity: 1; }
to { opacity: 0; }
}
@keyframes scaleIn {
from { transform: scale(0.9); opacity: 0; }
to { transform: scale(1); opacity: 1; }
}
@keyframes scaleOut {
from { transform: scale(1); opacity: 1; }
to { transform: scale(0.9); opacity: 0; }
}
`;
-document.head.appendChild(style);
+ document.head.appendChild(style);
+}Also update the cleanup logic:
const cleanup = () => {
if (overlay.parentNode) {
overlay.parentNode.removeChild(overlay);
}
- if (style.parentNode) {
- style.parentNode.removeChild(style);
- }
+ // Don't remove shared styles as other overlays might be using them
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const style = document.createElement("style"); | |
| style.textContent = ` | |
| @keyframes fadeIn { | |
| from { opacity: 0; } | |
| to { opacity: 1; } | |
| } | |
| @keyframes fadeOut { | |
| from { opacity: 1; } | |
| to { opacity: 0; } | |
| } | |
| @keyframes scaleIn { | |
| from { transform: scale(0.9); opacity: 0; } | |
| to { transform: scale(1); opacity: 1; } | |
| } | |
| @keyframes scaleOut { | |
| from { transform: scale(1); opacity: 1; } | |
| to { transform: scale(0.9); opacity: 0; } | |
| } | |
| `; | |
| document.head.appendChild(style); | |
| // Add CSS animations | |
| const existingStyle = document.getElementById("walletconnect-qr-overlay-styles"); | |
| if (!existingStyle) { | |
| const style = document.createElement("style"); | |
| style.id = "walletconnect-qr-overlay-styles"; | |
| style.textContent = ` | |
| @keyframes fadeIn { | |
| from { opacity: 0; } | |
| to { opacity: 1; } | |
| } | |
| @keyframes fadeOut { | |
| from { opacity: 1; } | |
| to { opacity: 0; } | |
| } | |
| @keyframes scaleIn { | |
| from { transform: scale(0.9); opacity: 0; } | |
| to { transform: scale(1); opacity: 1; } | |
| } | |
| @keyframes scaleOut { | |
| from { transform: scale(1); opacity: 1; } | |
| to { transform: scale(0.9); opacity: 0; } | |
| } | |
| `; | |
| document.head.appendChild(style); | |
| } | |
| const cleanup = () => { | |
| if (overlay.parentNode) { | |
| overlay.parentNode.removeChild(overlay); | |
| } | |
| // Don't remove shared styles as other overlays might be using them | |
| }; |
🤖 Prompt for AI Agents
In packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts around lines 214
to 233, the code injects a style element with keyframe animations each time
createQROverlay is called, causing duplicate style elements in the document
head. To fix this, check if the style element with these keyframes already
exists before creating and appending a new one, or create a shared style element
once and reuse it. Also, update the cleanup logic to avoid removing shared
styles that might be used elsewhere.
d392f9c to
b7e1e42
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (4)
59-62: Return type annotation is present.The function signature correctly includes the explicit return type annotation
: QROverlayas required by TypeScript coding guidelines.
63-296: Function still violates single-responsibility principle.This function remains 230+ lines long and handles multiple responsibilities including DOM creation, styling, event handling, animations, and lifecycle management. The previous suggestion to break this into smaller, focused functions is still relevant.
214-233: Style element duplication issue persists.Multiple calls to
createQROverlaywill still inject duplicate style elements into the document head. The previous suggestion to check for existing styles or use a shared style element remains valid.
302-306: Missing return type annotation on async function.The
generateQRCodefunction is missing an explicit return type annotation. It should bePromise<void>as suggested in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.changeset/social-dogs-bet.md(1 hunks)apps/playground-web/src/app/wallets/sign-in/headless/page.tsx(2 hunks)apps/playground-web/src/components/sign-in/hooks.tsx(2 hunks)apps/playground-web/src/components/sign-in/modal.tsx(0 hunks)packages/thirdweb/package.json(1 hunks)packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts(1 hunks)packages/thirdweb/src/wallets/create-wallet.ts(2 hunks)packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts(1 hunks)packages/thirdweb/src/wallets/wallet-connect/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/playground-web/src/components/sign-in/modal.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/thirdweb/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/social-dogs-bet.md
- packages/thirdweb/src/wallets/wallet-connect/types.ts
- packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts
- apps/playground-web/src/components/sign-in/hooks.tsx
- packages/thirdweb/src/wallets/create-wallet.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Files:
apps/playground-web/src/app/wallets/sign-in/headless/page.tsxpackages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
apps/playground-web/src/app/wallets/sign-in/headless/page.tsxpackages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
apps/{dashboard,playground-web}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
apps/{dashboard,playground-web}/**/*.{ts,tsx}: Import UI primitives from@/components/ui/*(Button, Input, Select, Tabs, Card, Sidebar, Badge, Separator) in dashboard and playground apps
UseNavLinkfor internal navigation with automatic active states in dashboard and playground apps
Use Tailwind CSS only – no inline styles or CSS modules
Usecn()from@/lib/utilsfor conditional class logic
Use design system tokens (e.g.,bg-card,border-border,text-muted-foreground)
Server Components (Node edge): Start files withimport "server-only";
Client Components (browser): Begin files with'use client';
Always callgetAuthToken()to retrieve JWT from cookies on server side
UseAuthorization: Bearerheader – never embed tokens in URLs
Return typed results (e.g.,Project[],User[]) – avoidany
Wrap client-side data fetching calls in React Query (@tanstack/react-query)
Use descriptive, stablequeryKeysfor React Query cache hits
ConfigurestaleTime/cacheTimein React Query based on freshness (default ≥ 60s)
Keep tokens secret via internal API routes or server actions
Never importposthog-jsin server components
Files:
apps/playground-web/src/app/wallets/sign-in/headless/page.tsx
packages/thirdweb/src/wallets/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
⏰ 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). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/playground-web/src/app/wallets/sign-in/headless/page.tsx (1)
80-80: LGTM: Correct addition of cancelConnection.The destructuring correctly includes the new
cancelConnectionfunction from theuseConnecthook, enabling cancellation functionality as intended.packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (3)
6-54: Well-designed interface definitions.The
QROverlayOptionsandQROverlayinterfaces are clearly defined with comprehensive documentation and appropriate type annotations. Good use of optional properties and clear method signatures.
307-322: Good implementation of dynamic QR code generation.The dynamic import approach keeps the bundle lean while providing proper QR code functionality. Error handling with canvas context check and appropriate QR code configuration options are well implemented.
177-177: Adequate error handling for QR generation.The error handling with
.catch(console.error)is appropriate for this utility function, providing visibility into QR generation failures without breaking the overlay functionality.
joaquim-verges
force-pushed
the
Fix_showQrModal_option_not_respected_on_desktop_web
branch
from
b7e1e42 to
946c877
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (5)
59-296: Function remains too large despite previous feedbackThis 237-line function still violates the single-responsibility principle mentioned in coding guidelines. The function handles DOM creation, styling, event management, animations, and lifecycle - multiple distinct concerns that should be separated.
The previous review comment suggesting decomposition into smaller utilities hasn't been addressed. Consider the suggested refactoring approach to improve maintainability.
214-233: Style duplication issue persistsMultiple calls to
createQROverlaywill still inject duplicate style elements into the document head. The previous suggestion to check for existing styles hasn't been implemented.Apply this fix to prevent style duplication:
-// Add CSS animations -const style = document.createElement("style"); -style.textContent = ` +// Add CSS animations +const existingStyle = document.getElementById("walletconnect-qr-overlay-styles"); +if (!existingStyle) { + const style = document.createElement("style"); + style.id = "walletconnect-qr-overlay-styles"; + style.textContent = ` @keyframes fadeIn { from { opacity: 0; } to { opacity: 1; } } @keyframes fadeOut { from { opacity: 1; } to { opacity: 0; } } @keyframes scaleIn { from { transform: scale(0.9); opacity: 0; } to { transform: scale(1); opacity: 1; } } @keyframes scaleOut { from { transform: scale(1); opacity: 1; } to { transform: scale(0.9); opacity: 0; } } `; -document.head.appendChild(style); + document.head.appendChild(style); +}
302-306: Add missing return type annotationThe async function lacks an explicit return type annotation as required by coding guidelines.
async function generateQRCode( text: string, canvas: HTMLCanvasElement, size: number, -) { +): Promise<void> {
312-321: Add error handling for dynamic importThe dynamic import could fail if the qrcode dependency isn't available, but there's no fallback handling.
+ try { - const { toCanvas } = await import("qrcode"); - await toCanvas(canvas, text, { - width: size, - margin: 2, - color: { - dark: "#000000", - light: "#ffffff", - }, - }); - return; + const { toCanvas } = await import("qrcode"); + await toCanvas(canvas, text, { + width: size, + margin: 2, + color: { + dark: "#000000", + light: "#ffffff", + }, + }); + } catch (error) { + console.warn("QR code library not available, falling back to placeholder"); + // Draw a simple placeholder + ctx.fillStyle = "#f0f0f0"; + ctx.fillRect(0, 0, size, size); + ctx.fillStyle = "#666"; + ctx.font = "16px Arial"; + ctx.textAlign = "center"; + ctx.fillText("QR Code", size / 2, size / 2); + }
272-274: Remove shared style cleanup to prevent breaking other overlaysThe cleanup function should not remove shared styles that might be used by other overlay instances.
- if (style.parentNode) { - style.parentNode.removeChild(style); - } + // Don't remove shared styles as other overlays might be using them
🧹 Nitpick comments (1)
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (1)
193-204: Consider fallback for clipboard APIThe clipboard API may not be available in all browser contexts (e.g., non-HTTPS contexts). Consider adding a fallback.
copyButton.addEventListener("click", async () => { try { - await navigator.clipboard.writeText(uri); + if (navigator.clipboard) { + await navigator.clipboard.writeText(uri); + } else { + // Fallback for older browsers or non-HTTPS contexts + const textArea = document.createElement("textarea"); + textArea.value = uri; + document.body.appendChild(textArea); + textArea.select(); + document.execCommand('copy'); + document.body.removeChild(textArea); + } const originalText = copyButton.textContent; copyButton.textContent = "Copied!"; setTimeout(() => { copyButton.textContent = originalText; }, 2000); } catch (err) { console.error("Failed to copy URI:", err); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.changeset/social-dogs-bet.md(1 hunks)apps/dashboard/package.json(1 hunks)apps/playground-web/src/app/wallets/sign-in/headless/page.tsx(2 hunks)apps/playground-web/src/components/sign-in/hooks.tsx(2 hunks)apps/playground-web/src/components/sign-in/modal.tsx(0 hunks)packages/thirdweb/package.json(1 hunks)packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts(1 hunks)packages/thirdweb/src/wallets/create-wallet.ts(2 hunks)packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts(1 hunks)packages/thirdweb/src/wallets/wallet-connect/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/playground-web/src/components/sign-in/modal.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/thirdweb/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- .changeset/social-dogs-bet.md
- apps/dashboard/package.json
- packages/thirdweb/src/wallets/wallet-connect/types.ts
- apps/playground-web/src/app/wallets/sign-in/headless/page.tsx
- packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts
- apps/playground-web/src/components/sign-in/hooks.tsx
- packages/thirdweb/src/wallets/create-wallet.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Files:
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
⏰ 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). (8)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: Build Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (2)
6-39: Well-structured type definitions with appropriate interface usageThe type definitions follow TypeScript best practices by using interfaces for nominal shapes (
QROverlay) and clear option patterns. The optional properties and documentation are well-defined.
255-262: Good separation of user-initiated vs programmatic cleanupThe distinction between user-initiated and programmatic destruction with appropriate onCancel callback handling is well implemented.
joaquim-verges
force-pushed
the
Fix_showQrModal_option_not_respected_on_desktop_web
branch
from
a202c79 to
7b6b0e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (4)
59-296: Function still violates single-responsibility principle.This function remains too large at 237 lines and handles multiple concerns including DOM creation, styling, event handling, animations, and lifecycle management. The previous suggestion to decompose into smaller, focused functions should still be addressed.
214-233: Style element duplication issue remains unaddressed.Multiple calls to
createQROverlaywill still inject duplicate style elements into the document head. The previous suggestion to check for existing styles or use a shared style element should be implemented.
302-306: Missing explicit return type annotation.The async function is missing an explicit return type annotation as required by the TypeScript coding guidelines.
312-321: Missing error handling for dynamic import.The dynamic import of the qrcode library lacks error handling, which could cause unhandled exceptions if the import fails. This was previously flagged and should be addressed with appropriate try-catch blocks and fallback behavior.
🧹 Nitpick comments (1)
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (1)
79-79: Consider making z-index configurable.The hardcoded z-index of 9999 could conflict with other modals or overlays in the application. Consider making this configurable through the options interface.
Add z-index to the options interface and use it in the overlay styling:
interface QROverlayOptions { // ... existing options + zIndex?: number; } // In createQROverlay function: const { // ... existing destructuring + zIndex = 9999, } = options; // In overlay styling: - z-index: 9999; + z-index: ${zIndex};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.changeset/social-dogs-bet.md(1 hunks)apps/dashboard/package.json(2 hunks)apps/playground-web/src/app/wallets/sign-in/headless/page.tsx(2 hunks)apps/playground-web/src/components/sign-in/hooks.tsx(2 hunks)apps/playground-web/src/components/sign-in/modal.tsx(0 hunks)packages/thirdweb/package.json(2 hunks)packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts(1 hunks)packages/thirdweb/src/wallets/create-wallet.ts(2 hunks)packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts(1 hunks)packages/thirdweb/src/wallets/wallet-connect/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/playground-web/src/components/sign-in/modal.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/thirdweb/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- .changeset/social-dogs-bet.md
- apps/dashboard/package.json
- packages/thirdweb/src/react/core/hooks/wallets/useConnect.ts
- packages/thirdweb/src/wallets/create-wallet.ts
- packages/thirdweb/src/wallets/wallet-connect/types.ts
- apps/playground-web/src/app/wallets/sign-in/headless/page.tsx
- apps/playground-web/src/components/sign-in/hooks.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Files:
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts
⏰ 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). (8)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts (1)
6-54: Well-designed interfaces with clear contracts.The interface definitions are properly structured with comprehensive JSDoc documentation and appropriate typing. The separation of concerns between configuration options and the returned overlay object follows good TypeScript practices.
| copyButton.addEventListener("click", async () => { | ||
| try { | ||
| await navigator.clipboard.writeText(uri); | ||
| const originalText = copyButton.textContent; | ||
| copyButton.textContent = "Copied!"; | ||
| setTimeout(() => { | ||
| copyButton.textContent = originalText; | ||
| }, 2000); | ||
| } catch (err) { | ||
| console.error("Failed to copy URI:", err); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add feature detection for clipboard API.
The code uses navigator.clipboard.writeText() without checking if the API is available. This could throw errors in browsers that don't support the Clipboard API or in insecure contexts (non-HTTPS).
Consider adding feature detection:
copyButton.addEventListener("click", async () => {
try {
+ if (!navigator.clipboard || !navigator.clipboard.writeText) {
+ throw new Error("Clipboard API not available");
+ }
await navigator.clipboard.writeText(uri);
const originalText = copyButton.textContent;
copyButton.textContent = "Copied!";
setTimeout(() => {
copyButton.textContent = originalText;
}, 2000);
} catch (err) {
- console.error("Failed to copy URI:", err);
+ console.error("Failed to copy URI:", err);
+ // Fallback: could select text or show manual copy instructions
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| copyButton.addEventListener("click", async () => { | |
| try { | |
| await navigator.clipboard.writeText(uri); | |
| const originalText = copyButton.textContent; | |
| copyButton.textContent = "Copied!"; | |
| setTimeout(() => { | |
| copyButton.textContent = originalText; | |
| }, 2000); | |
| } catch (err) { | |
| console.error("Failed to copy URI:", err); | |
| } | |
| }); | |
| copyButton.addEventListener("click", async () => { | |
| try { | |
| if (!navigator.clipboard || !navigator.clipboard.writeText) { | |
| throw new Error("Clipboard API not available"); | |
| } | |
| await navigator.clipboard.writeText(uri); | |
| const originalText = copyButton.textContent; | |
| copyButton.textContent = "Copied!"; | |
| setTimeout(() => { | |
| copyButton.textContent = originalText; | |
| }, 2000); | |
| } catch (err) { | |
| console.error("Failed to copy URI:", err); | |
| // Fallback: could select text or show manual copy instructions | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/thirdweb/src/wallets/wallet-connect/qr-overlay.ts around lines 193
to 204, the code uses navigator.clipboard.writeText() without checking if the
Clipboard API is supported, which can cause errors in unsupported browsers or
insecure contexts. Add a feature detection check before calling writeText by
verifying if navigator.clipboard and navigator.clipboard.writeText exist. If
unsupported, handle the fallback gracefully, such as showing an error message or
disabling the copy button.
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7711 +/- ##
==========================================
- Coverage 56.26% 56.21% -0.05%
==========================================
Files 908 908
Lines 58177 58236 +59
Branches 4208 4206 -2
==========================================
+ Hits 32731 32738 +7
- Misses 25337 25389 +52
Partials 109 109
🚀 New features to boost your workflow:
|
joaquim-verges
force-pushed
the
Fix_showQrModal_option_not_respected_on_desktop_web
branch
from
7b6b0e2 to
a32d97e
Compare
github-actions
bot
added
the
Portal
joaquim-verges
force-pushed
the
Fix_showQrModal_option_not_respected_on_desktop_web
branch
from
a32d97e to
aa6cffc
Compare
joaquim-verges
deleted the
Fix_showQrModal_option_not_respected_on_desktop_web
branch
Fixes BLD-2
PR-Codex overview
This PR focuses on improving the
showQrModalfunctionality for wallet connections in desktop web applications. It introduces new options for handling cancellations and updates dependencies related to QR code generation.Detailed summary
cancelConnectioncallback inuseConnecthook.onCanceloption for wallet connection to handle user cancellations.createQROverlayfunctionality for displaying QR codes.package.jsonforqrcodeand related types.Summary by CodeRabbit
New Features
Bug Fixes
Improvements