Add same-device wallet links to the wallet chooser - #200
Conversation
|
Reviewed the 10 files unique to this branch (#201's four are covered on that PR). The design reasoning in the comments is good — most of this is the code not matching it yet. Three to fix, then smaller things. 1. The new copy is false in its most common case
Also gone: "or visit your wallet website to register" (a first-timer now gets two rows that assume they already have one, and no route to getting one) and the RP's name. 2. The three harness URLs this PR documents show nothing
qrEnabled.value && linkEnabled.value && appLinkEnabled.value ? ... : ''So none render a row, and 3. Activating a row navigates the mediator away with nothing sentBoth anchors are plain I can't say what the RP sees: the popup path detects the dialog leaving by polling Smaller
Checked clean: Pre-existing, not this PR
|
| v-if="collapsible" | ||
| class="wrm-dark-gray" | ||
| style="padding-top: 0.5em"> | ||
| Already have a wallet? Open it here: |
There was a problem hiding this comment.
This text is a little confusing because it can show up when the user "already has a wallet that is in the list above where text appears"
| Already have a wallet? Open it here: | |
| Have a wallet installed on this device? Open it here: |
This suggestion might not be good enough because are Web wallets that only register the web+interaction scheme "installed on the device?" ... perhaps not. We need some better language, we should bring this to others to bikeshed it collectively.
Maybe we want "Don't see your wallet?" to be a simple text that we use for all of the cases -- and then we have buttons for opening a wallet "on this device" and the "QR code" for opening "on another device". That might solve this generally.
There was a problem hiding this comment.
Took the restructure you proposed rather than the inline suggestion, since as you say a web wallet registering only web+interaction is not "installed on the device".
"Don't see your wallet?" is now a single heading over both the same-device rows and the cross-device QR expander, and it renders unconditionally. That is what makes it true in the case you caught: with wallets listed, the rows are for a wallet that is not in the list; with none, there is nothing to see. It no longer asks whether the user has a wallet directly above their wallets.
Grouping the QR under the same heading is the part that fixes "Choose a Wallet" and "Open a wallet..." reading as competing instructions — the chooser asks once, then lists every way to answer: on this device, in a browser, or on another device.
Side effect worth flagging: this reverses the rename in "Also included". The expander used to read "Don't see your wallet?" and I moved it off that string because it sat below a section answering the question. Making it the section heading instead is the better shape.
| <div class="wallet-link-text"> | ||
| <strong>Open my web wallet</strong> | ||
| <div class="wrm-dark-gray wallet-link-caption"> | ||
| A wallet website you already use |
There was a problem hiding this comment.
Similarly, this can show up even if there's a "wallet website you already use in the list above where this appears". So we should bikeshed this a bit too.
There was a problem hiding this comment.
Same fault, and both captions are now deleted rather than reworded. "A wallet website you already use" could appear directly under a list of wallet websites the user already uses, and "An app installed on this device" had the matching problem.
The row titles carry the destination instead:
- Open a wallet app on this device
- Open a wallet website
- Use a wallet on another device
Three parallel phrases under one question, and no low-contrast second line on any of them.
|
Applied in 67070f5. Verified each claim against the code first; all three blocking ones held, and item 3 was worse than latent. 1. The copy was falseConfirmed: Now request-type aware, with the relying party's name and the route to registering a wallet restored:
You were right that two rows assuming an existing wallet, with no route to getting one, is worse for a first-timer than the copy I replaced. 2. The documented harness URLs showed nothingConfirmed — 3. Activation navigated away with nothing sentThis was the one worth catching. The rows now emit the out-of-band response on click, before the navigation. Your reasoning about I did not add While there: Smaller
Not changed, with reasons
Pre-existing
|
Vulnerability resultsNo change from base branch. Total 4 packages affected by 12 known vulnerabilities (0 Critical, 8 High, 4 Medium, 0 Low, 0 Unknown) from 1 ecosystem.
npm audit
npm list
|
dlongley
left a comment
dlongley
left a comment
There was a problem hiding this comment.
I think we just need some config reorg here and then we're good to go. I think the UI improvements are good enough to launch, but we could use some additional iterations after feedback from others to help further improve.
| /* Optional local overrides, loaded last so they win. `configs/local.js` is | ||
| gitignored, so machine-specific settings -- e.g. pointing `server.host` at a | ||
| tunnel hostname so a phone can reach the mediator -- stay out of the tracked | ||
| config. See `configs/local.js.example`. | ||
|
|
||
| This is the development override channel. `@bedrock/config-yaml` (imported | ||
| from `lib/index.js`) is another, but it reads `/etc/bedrock-config/app.yaml` | ||
| or a base64 blob in `BEDROCK_CONFIG`, which suits deployment rather than a | ||
| working copy. | ||
|
|
||
| Assign config values only. The file is imported after `lib/index.js` has | ||
| imported `@bedrock/config-yaml`, so registering a `bedrock.events` handler | ||
| here fails with `"bedrock-config-yaml" must be the last import`. | ||
|
|
||
| `pathToFileURL` because `import()` takes a URL, not a filesystem path: a | ||
| checkout under a directory containing `#` truncates at the fragment, and a | ||
| Windows path parses its drive letter as a URL scheme. */ | ||
| const localConfigPath = path.join(__dirname, 'local.js'); | ||
| if(existsSync(localConfigPath)) { | ||
| await import(pathToFileURL(localConfigPath).href); | ||
| } |
There was a problem hiding this comment.
This bit doesn't look right / isn't "the bedrock way" and invents a new mechanism.
I see the notes here on how this file loads after lib/index.js which imports bedrock/config-yaml, so what should be done is some appropriate reorganization so an optional local config file can be loaded before bedrock/config-yaml, rather than inventing a new and unfamiliar pattern.
cc: @jameseaster there are some "config bootstrap" patterns you've worked with, could you perhaps help here?
There was a problem hiding this comment.
Reorganized in ca8ee68. You were right that the ad-hoc loader was the wrong shape, and the root cause turned out to be upstream of it: authn.io was the only bedrock app importing its environment config from the entry point after lib/index.js. Every other one sets config.paths.config, imports lib/index.js, and loads configs/*.js from handlers in lib/config.js.
So the entry points now do:
config.paths.config = path.join(__dirname, 'configs');
config.paths.envConfig = 'dev.js'; // or 'authn.io.js'
import './lib/index.js';and lib/config.js imports the environment config and then any optional
configs/local.js from handlers it registers. Since those are registered before
lib/index.js imports @bedrock/config-yaml, config-yaml is still last and the
whole "assign values only, do not register handlers" caveat is gone — verified
by having a local config register a bedrock.init handler, which threw before
and works now.
One trap worth recording. Loading only at bedrock.configure looks right and is
not: bedrock reads paths.cache and paths.log while initializing loggers,
right after bedrock-cli.parsed, so file logging silently falls back to
/tmp/bedrock-dev. I hit that and confirmed it by removing the handler again.
Both events now load the same file — ES modules evaluate once, so the second
import is a no-op returning the cached module, which keeps one config file per
environment instead of splitting each into paths/core/rest as some repos do.
Suites unchanged at 305 passed / 40 failed, all 40 the known has no horizontal overflow assertion.
| first-party wizard defines one and this did not, so any path reaching it | ||
| threw. Mirrors that one -- the user has chosen to continue out of band, | ||
| which is a response, not a cancellation. */ | ||
| const crossDevice = async () => { |
There was a problem hiding this comment.
I think this is fine to add -- but I think we never actually hit the code path anymore so it should get cleaned up anyway once we address #157.
There was a problem hiding this comment.
Agreed on both counts. Left as-is here: the binding existed with no handler, so any path that did reach it threw a TypeError, and this branch is the first to give a user a way to reach it. Happy for it to be removed wholesale with the rest of that path under #157 rather than maintained.
| bedrock.events.on('bedrock-cli.parsed', () => _loadConfigs()); | ||
| bedrock.events.on('bedrock.configure', () => _loadConfigs()); |
There was a problem hiding this comment.
@davidlehn can you look at this? It looks like duplication of effort -- what's the right fix to avoid that?
|
@djscruggs One other minor CSS note here -- is that the choices below "Don't see your wallet?" would probably look better if the icons and text were aligned, i.e., each item gets the same space for a centered icon, resulting in the text all being in "vertical alignment" as you look down the options. |
There was a problem hiding this comment.
It might be useful to use a local.example.js name so linter would pick this up, and syntax highlighting would work. Given the short nature of it and it's mostly comments, maybe not a big deal here.
| /* taller hint chooser popup that fits the cross-device QR section, plus | ||
| the same-device wallet link rows above it. | ||
|
|
||
| The extra 20px is a stopgap: the dialog does not size itself to its |
There was a problem hiding this comment.
This looks odd. Is "20px" referring to the change in this PR? That's not going to make sense when reading the merged code.
dlongley
left a comment
dlongley
left a comment
There was a problem hiding this comment.
Approving; this looks good to me, I don't see any config changes like before, so I think we're good to merge -- I'll do that and publish a release. Thanks!
Add "Open a wallet app on this device" and "Open a wallet website" rows to the wallet chooser, shown whenever a relying party supplies an interaction URL. They hand that URL to a wallet on the same device by prefixing it with the `interaction:` and `web+interaction:` schemes, so a wallet that registered a scheme without registering as a credential handler can receive the exchange. Previously such a wallet was unreachable: it never appears in the chooser, and a QR code cannot be scanned by the device displaying it. A single "Don't see your wallet?" heading introduces the rows and the cross-device QR expander together, so the chooser asks the question once and then lists every way to answer it. Each row appears only where it can work: the app row on a phone or tablet, the website row only where `navigator.registerProtocolHandler()` exists. The rows use the same 48px icon slot as the registered wallet list, so every row's text starts on the same vertical line. That also resolves the horizontal overflow the earlier variable-width glyphs caused. Addresses #166.
5c64c24 to
ff34b17
Compare
4 participants
Phase 2 of cross-device-wallet-selection-spec.md, whose Phase 1 (the cross-device QR code) shipped in 7.2.0. Addresses #166.
What this adds
Two rows in the wallet chooser, shown whenever a relying party supplies an
interaction URL:
as
interaction:<url>web+interaction:<url>Both sit under a single "Don't see your wallet?" heading that also covers
the cross-device QR expander, so the chooser asks the question once and then
lists every way to answer it.
They reach a wallet that registered a URL scheme but is not registered as a
credential handler in this browser. Such a wallet was previously unreachable:
it never appears in the chooser, and a QR code cannot be scanned by the device
displaying it — the case a phone always hits.
Why it belongs here and not in coordinator sites
The same argument that moved the QR code into CHAPI. A coordinator site cannot
know which wallets a user has registered, so it cannot decide which link to
show, or whether to show one at all. The chooser can. Coordinators keep a
single button that calls CHAPI and get this behavior — and later fixes —
without changing their code.
Each row appears only where it can work
rather than viewport width: the chooser renders in a 500px popup on the
desktop, the same width range as a phone, so a breakpoint cannot tell them
apart.
navigator.registerProtocolHandler()existing.Nothing can claim a
web+scheme without it, and Safari has never shippedthe API — there the link cannot resolve and the browser reports the address
as invalid, so the row would offer a guaranteed error. This covers every
browser on iOS, since they are all WebKit; feature detection gets that right
without enumerating browsers.
Neither row is a fallback for the other: a native app can only claim the bare
scheme, a web app only the
web+form, and nothing in the browser revealswhich the user registered. Custom-scheme navigation also fails silently, with
no event to detect it, so no fallback can be inferred. The user knows whether
their wallet is an app or a website, so both are offered where both can work
and the copy says what silence means.
Layout
Captures from the
/test/wallet-chooserharness, generated bynpm run gallery. A single "Don't see your wallet?" heading introducesevery way out of the registered list, and each row underneath it names its
destination on one line.
Both link rows available
Chromium at the 500px popup width, with both gates forced on.
Where a row is gated off
registerProtocolHandler()and a desktop is not a phone, so neither row shows. The heading still holds: the QR expander beneath it is a way to answer the question. This is the case that made the heading unconditional.The rows sit between the wallet list and the QR expander, using the same
wrm-itemrow shape as the wallet list — so opening a wallet you already havereads as the same class of action as picking a listed one. The QR stays behind
its expander: it needs a second device, and it is tall enough to push
everything else off a phone screen.
The silent-failure note is not in these shots: it appears only after a row is
activated. It reads "this link" when one row is shown and "these links" when
both are.
With no wallet registered in this browser, the wallet app row is marked
recommended — there is no registered wallet to prefer and the QR needs a
second device, so an installed app is the likeliest thing the user has.
Tapping a row emits nothing and changes no mediator state. A user who taps has
not cancelled, and if nothing claims the scheme the dialog must remain as it
was; dismissal stays with the existing Close button, which resolves
OutOfBandas Phase 1 established.Note on the schemes
interaction:andweb+interaction:are a Digital Bazaar convention, not astandard. Verified against the VCALM Editor's Draft on 2026-08-19: it defines
no interaction URL scheme, treating interaction URLs as plain HTTPS carrying
iuv=1. The names live in two constants inweb/mediator/constants.jsso arename after CCG/VCALM agreement is a one-line change.
This ships experimentally, which the parent spec's Open Question 1 permits
("after agreement or experimentally to gather feedback"). Feedback is what
agreement needs.
Also included
+20px on the cross-device popup height. The dialog does not size itself to
its content, so each section added to it is paid for by hand here; without
it the Close button can scroll out of reach. A stopgap until the dynamic
sizing work lands.
The zero-wallet greeting is cut to one line and points at the QR when no
link rows are available.
The cross-device expander is relabeled "Use a wallet on another device", and
"Don't see your wallet?" is promoted to the heading above it — so it now
introduces the expander and the same-device rows together, rather than
labelling one of the three options.
/test/wallet-choosergainsappandwebparams to force either row onor off, since the real gates depend on the browser, and
jit=Nto renderrecommended wallets — a section no harness state could previously produce.
playwright.config.jstakes anAUTHNIO_TEST_PORToverride, so a localconfigs/local.jsthat moves the dev server off 33443 can still run thesuites.
Config loading moved onto bedrock's config events. The entry points set
config.paths.configandconfig.paths.envConfig, andlib/config.jsimports the environment config plus any optional gitignored
configs/local.jsfrom handlers it registers.authn.iohad been the onlybedrock application importing its environment config from the entry point
after
lib/index.js, which is what forced local overrides to assignvalues without registering handlers —
@bedrock/config-yamlthrows unlessits handler is last. Registering before it removes the constraint.
Both
bedrock-cli.parsedandbedrock.configureload the same file:bedrock reads
paths.cache/paths.logwhile initializing loggers, rightafter the first event, so loading only at
bedrock.configuresends filelogging to bedrock's default directory instead. ES modules evaluate once,
so the repeat import is a cached no-op.
Testing
npm run test:e2eacross chromium, webkit, firefox, iphone, andandroid-pixel: 305 passed, 40 failed. All 40 are the pre-existing
has no horizontal overflowassertion, marked KNOWN FAILING in the spec fileand unchanged by this branch — 8 states × 5 projects. The only reported
offender is
wrm-slide, which comes fromvue-web-request-mediator, not fromanything this branch adds.
The count rose from the earlier 28 because the state table grew, not because
new elements overflow:
STATESnow covers the two same-device link states anda recommended-wallet state, each with both gates forced on so the rows are
exercised on every project rather than only where the browser satisfies them.
Verified by hand on a real iPhone against a tunnelled mediator: the wallet app
row appears, the wallet website row does not, and the rows render correctly in
the first-party popup. That run predates the copy change, so it confirms the
gating and the popup rendering rather than the current strings; the copy itself
is covered by the captures above and by the harness suites, which assert on the
rows in both states.
Not covered
No web wallet registers
web+interaction:yet, so that row is inert where itshows — Chrome logs "the scheme does not have a registered handler". It is
harmless but unexercised; worth deciding whether to ship it now or hold it
until a wallet registers.