Repository navigation
fix: Scan Physical ID above the search bar on Add Credentials - #415
Conversation
Closes #307. Placement. QueryableList rendered its extraSection between the Recent grid and the All heading, so the Scan Physical ID tile sat below recently used credentials rather than at the top of the page as the designs have it. It is the page's own entry point rather than one of the queryable items, so it now renders above the search box, Recent and All. Showing it only when no search is active is unchanged: a search is about the issuer list, and the entry point would be noise in the results. Recent. Every other Recent entry is derived from a stored credential matched against a configuration from /issuer/all. Scan Physical ID is a local route (add/digital-id), not an issuer's credential configuration, so there is nothing to derive it from and it could never appear there. QueryableList gains a recentExtra slot, rendered first in the Recent grid, and AddCredentials fills it once the flow has been used. Starting the flow is what makes it recent, which is the only signal available without tying it to a particular issued credential. Both headings were gated on there being matched recent configurations, so a wallet with no credentials yet would have shown the new entry floating under no heading; they now account for recentExtra too. The usage flag is a per-device convenience in localStorage: no personal data, nothing depends on it, and losing it only means the entry is absent from Recent until the next scan. Both accessors swallow failure, which matters more here than usual -- this test environment has no localStorage object at all, which is why several of the suite's pre-existing failures are modules touching it at import time. Tests live in a new QueryableList.placement.test.tsx rather than QueryableList.test.tsx, because that file imports @/i18n, reads localStorage at module scope and so never loads; a test added there would not have run. It uses the react-i18next stand-in the repo already uses in usePresentCredentialsFlow.test.tsx. The placement assertion was mutation-proved by moving the section back and watching it fail. AddCredentials' own wiring is not unit-tested: AddCredentials.test.jsx is one of the five files that already fail to load. Verified: 1364 tests pass, 0 fail (base: 1353, 0); the same five pre-existing file failures; typecheck 100 errors, exactly base; build succeeds.
🦋 Changeset detectedLatest commit: 2e49787 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
smncd
left a comment
smncd
left a comment
There was a problem hiding this comment.
I think rather than putting the "Scan physical id" entrypoint in the same list as the preconfigured credential offers, it should sit on its own, above the list and searchbar.
So:
| ------------------------------|
| title |
| description |
| ------------------------------|
| Scan Physical ID | <- here, separate from the QueryableList
| ------------------------------|
| search bar |
| recent credential offers |
| all credential offers |
| ------------------------------|
react-router v7's navigate returns a promise. SonarCloud's new_reliability_rating gate flagged it (javascript:S9383); void matches how Credential.tsx already handles it.
Review on #415: the entry point should sit above the list and the search bar rather than inside QueryableList, so it is not presented as one of the preconfigured credential offers. Add Credentials now renders the section itself, between the page description and the list. That makes QueryableList's extraSection prop dead -- it was added for this entry point and had no other caller -- so it goes, along with the tests for it, and QueryableList returns to exactly what it was before. Dropped with it: the Recent entry and the localStorage that tracked whether the flow had been used, which only existed to place the tile in Recent. One deliberate behaviour change: inside QueryableList the section hid as soon as a query was typed. Above the search bar it is not part of what the query filters, so it stays. Covered by a test. The placement test is mutation-proven: moving the section back below the list fails it.
|
Taken — the section now renders in Two consequences worth flagging:
The Recent entry is dropped too, along with the One deliberate behaviour change: inside Verification: full suite 105 files / 1378 passed, no type errors; |
|
jessevanmuijden
changed the title
Closes #307.
1. Placement
QueryableListrendered itsextraSectionbetween the Recent grid and the All heading, so the Scan Physical ID tile sat below recently used credentials instead of at the top of the page.It is the page's own entry point, not one of the queryable items, so it now renders above the search box, Recent and All:
Hiding it while a search is active is unchanged — a search is about the issuer list, and the entry point would only be noise in the results.
2. In Recent
This one needed a mechanism, not a move. Every other Recent entry is derived from a stored credential matched against a configuration from
/issuer/all:Scan Physical ID is a local route (
add/digital-id), not an issuer's credential configuration, so it has no identifier to match and could never have appeared there.QueryableListgains arecentExtraslot, rendered first in the Recent grid, whichAddCredentialsfills once the flow has been used. Starting the flow is what makes it recent — the only signal available without tying the entry to one particular issued credential.It therefore appears twice once used: pinned at the top (always available) and under Recent (recently used). Per the decision on this, that duplication is intended.
A bug this surfaced: both the Recent and All headings were gated purely on there being matched recent configurations, so a wallet with no credentials yet would have shown the new entry floating under no heading at all. Both now account for
recentExtra.The usage flag
A per-device convenience in
localStorage: no personal data, nothing depends on it, and losing it only means the entry is absent from Recent until the next scan. Both accessors swallow failure — which matters more here than usual, because this test environment has nolocalStorageobject at all; that is why several of the suite's pre-existing failures are modules touching it at import time. Covered for a storage that refuses, and for no storage object.Tests, and why they are in a new file
They live in a new
QueryableList.placement.test.tsxrather thanQueryableList.test.tsx, because that file imports@/i18n, which readslocalStorageat module scope and so never loads — a test added there would not have run. The new file uses thereact-i18nextstand-in the repo already uses inusePresentCredentialsFlow.test.tsx.The placement assertion was mutation-proved: moving the section back between Recent and All fails it with "extraSection should precede the search input".
Not unit-tested:
AddCredentials' own wiring, becauseAddCredentials.test.jsxis one of the five files that already fail to load. TheQueryableListcontract and the storage helper are covered; the three lines joining them are not.Verification
Not verified: a click-through in the wrapper app. The entry point only renders when
window.nativeWrapper.startScanPhysicalIdexists, so the visual result needs the native build.