feat(auth): offer 'Open Dash Wallet' on desktop, not just touch devices - #354
feat(auth): offer 'Open Dash Wallet' on desktop, not just touch devices#354PastaPastaPasta wants to merge 2 commits into
Conversation
The wallet QR login gated its deep link behind a coarse-pointer check, so desktop users only ever saw the QR code. That is backwards for the wallet the copy actually names: Dash Evo Tool is a desktop app running on the same machine as the browser, so the user was told to scan a code with a phone to reach a wallet sitting one click away. KeyExchangeQR now renders the 'Open Dash Wallet' deep link on every device and points the desktop instructions at a local wallet first. Because browsers stay silent when no app is registered for the scheme, a fallback hint appears once the link has been clicked, explaining the likely cause and pointing back at the QR code and Copy URI. The hint resets whenever a new request URI is generated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
⛔ Final review complete — 1 blocking finding(s) (commit f5efa76) |
Deploying yappr with
|
| Latest commit: |
f5efa76
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b373260a.yappr.pages.dev |
| Branch Preview URL: | https://t3code-add-open-dash-wallet.yappr.pages.dev |
Deploying yappr-v2 with
|
| Latest commit: |
f5efa76
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2fdf9b80.yappr-v2.pages.dev |
| Branch Preview URL: | https://t3code-add-open-dash-wallet.yappr-v2.pages.dev |
On a phone the deep link already hands the URI straight to the wallet, so Copy URI sits underneath it offering the strictly worse version of the same thing: copy this, go find your wallet, find its paste entry point. That manual path is what the deep link exists to avoid, and on a small screen it is pure clutter. Copy URI is now desktop-only, and the no-handler fallback hint no longer suggests copying on touch devices, where the QR (scannable by a second device) is the remaining escape hatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Follow-up per review feedback: on touch devices the deep link now replaces Copy URI rather than sitting above it. On a phone the deep link already hands the URI straight to the wallet, so Copy URI offered the strictly worse version of the same action — copy this, go find your wallet, find its paste entry point. That manual path is exactly what the deep link exists to avoid, and on a small screen it was just clutter.
Verification status, stated plainly: lint and build pass, and the desktop layout was confirmed live in a browser. I was not able to visually confirm the touch layout — the preview automation stopped being able to re-open the wallet modal partway through the session (viewport emulation timed out, and the modal would not mount on retry). The touch branch is gated on the same 🤖 Posted autonomously by Claude on behalf of pasta. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — GLM Flash + Sol
The desktop deep link, device-specific instructions, and request-reset behavior are consistent and preserve static-export compatibility. However, removing Copy URI on touch devices creates a blocking mobile authentication regression when no custom-scheme handler is registered; the existing login smoke path can also cover the new desktop controls.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-5.6-sol (agent: phase2-reviewer, role: general); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
Review provenance
- Phase 1 reviewers (GLM Flash):
glm-5.3-flash— general (completed); agentphase1-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Phase 2 reviewers (Sol):
gpt-5.6-sol— general (completed); agentphase2-reviewer
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `components/auth/key-exchange-qr.tsx`:
- [BLOCKING] components/auth/key-exchange-qr.tsx:108: Preserve the same-device fallback after a touch launch attempt
The `!isTouchDevice` condition removes Copy URI from every coarse-pointer device, including after the user clicks the deep link and the component warns that no wallet may be registered for `dash-key:` or `dash-st:`. In that case the link cannot transfer the request, and a phone cannot scan the QR code on its own screen. The pre-PR component and the earlier on-device-link change explicitly retained clipboard-based manual entry as the mobile fallback, so users whose wallet accepts a pasted URI but has no registered protocol handler now need a second device or cannot complete login or key registration. Keep the initial touch layout uncluttered, but reveal Copy URI after `launchAttempted` and update the touch fallback text to mention it.
- [SUGGESTION] components/auth/key-exchange-qr.tsx:72-79: Add smoke coverage for the desktop wallet deep link
The PR's primary desktop behavior has no automated regression coverage even though `e2e/smoke/public-pages.spec.ts` already opens `/login/`. Extend that test to click “Login with Wallet (QR),” wait for the locally generated request, assert that “Open Dash Wallet” has a `dash-key:` href, and confirm that “Copy URI” is present in the desktop project. URI generation completes before response polling and requires no write credentials, so this exercises the new rendering without performing a Platform write.
| )} | ||
| </button> | ||
| {/* Copy button — desktop only; the deep link supersedes it on touch */} | ||
| {!isTouchDevice && ( |
There was a problem hiding this comment.
🔴 Blocking: Preserve the same-device fallback after a touch launch attempt
The !isTouchDevice condition removes Copy URI from every coarse-pointer device, including after the user clicks the deep link and the component warns that no wallet may be registered for dash-key: or dash-st:. In that case the link cannot transfer the request, and a phone cannot scan the QR code on its own screen. The pre-PR component and the earlier on-device-link change explicitly retained clipboard-based manual entry as the mobile fallback, so users whose wallet accepts a pasted URI but has no registered protocol handler now need a second device or cannot complete login or key registration. Keep the initial touch layout uncluttered, but reveal Copy URI after launchAttempted and update the touch fallback text to mention it.
| {!isTouchDevice && ( | |
| {(!isTouchDevice || launchAttempted) && ( |
source: ['claude', 'codex']
| <a | ||
| href={uri} | ||
| onClick={() => setLaunchAttempted(true)} | ||
| className={cn(buttonVariants({ size: 'lg' }), 'w-full gap-2')} | ||
| > | ||
| <ArrowTopRightOnSquareIcon className="w-5 h-5" /> | ||
| Open Dash Wallet | ||
| </a> |
There was a problem hiding this comment.
🟡 Suggestion: Add smoke coverage for the desktop wallet deep link
The PR's primary desktop behavior has no automated regression coverage even though e2e/smoke/public-pages.spec.ts already opens /login/. Extend that test to click “Login with Wallet (QR),” wait for the locally generated request, assert that “Open Dash Wallet” has a dash-key: href, and confirm that “Copy URI” is present in the desktop project. URI generation completes before response polling and requires no write credentials, so this exercises the new rendering without performing a Platform write.
source: ['codex']
2 participants
Problem
On
yap.pr/staging/login, the Login with Wallet modal on desktop shows only a QR code, a countdown, and Copy URI. The copy tells you to "scan with a compatible Dash wallet, such as Dash Evo Tool" — but Dash Evo Tool is a desktop app, typically running on the very machine displaying the QR. The user is told to pick up a phone to reach a wallet that is one click away.KeyExchangeQRalready had an "Open in Wallet App" deep link, but it was gated behindwindow.matchMedia('(pointer: coarse)'), so it only ever rendered on phones and tablets.Change
dash-key:/dash-st:URI itself) on every device, not just coarse-pointer ones. Desktop OSes register custom URI schemes the same way mobile does.The same component backs the key-registration step, so that flow gets the deep link on desktop too.
Verification
npm run lint— clean (no new warnings; the remaining ones are pre-existing in unrelated files)npm run build— succeeds, static export intactnpm run devand drove the real modal in a browser: the button renders above the QR with the livedash-key:…?n=t&v=1URI, desktop wording reads "Open a wallet on this computer, such as Dash Evo Tool, or scan the QR code with a wallet on your phone", and the fallback hint appears after clicking.No e2e run — this change touches neither feeds, posts, nor the auth paths the suite exercises (it is presentation-only within the QR component).
🤖 Generated with Claude Code