test(wallet): cover wallet page; a11y: focus rings, icon labels, skip link - #614
Open
JosephOladele wants to merge 1 commit into
Open
Conversation
… link - Add tests for the wallet page's loading spinner, "set up" (400) state, loaded wallet address, and the copy-to-clipboard confirmation. (kellymusk#473) - Add focus-visible rings to the two inputs missing one entirely (landing Pricing's country <select>, AmountWidget's amount <input>), and give the default Button variant a ring-offset so its focus ring isn't the same hue as its own background — confirmed via live keyboard nav that the ring was nearly invisible on primary-colored buttons (e.g. "Sign in") before this. Everything else audited (Input, Select, links, other Button variants) already pairs outline-none with a focus-visible alternative. (kellymusk#474) - Add aria-label to the three icon-only buttons found on audit that had none: the send flow's header back button, its QR-scan button, and its numpad backspace key. The two examples named in the issue itself (charge page's backspace, wallet page's copy button) already had accessible names — this covers what was actually still missing repo-wide. (kellymusk#475) - Add a visually-hidden "Skip to main content" link as the first focusable element in app/(app)/layout.tsx, targeting a new main#main-content. (kellymusk#476) Along the way, fixed components/session-provider.tsx: a bad merge between the Freighter-auth branch and the session-persistence branch had left duplicate imports, a duplicate Session type, and two interleaved, broken signOut implementations — the file didn't compile, which blocked every test here (including the wallet page test, since it depends on useAuthenticatedSession). Reconciled to the cookie-session restoration path plus Freighter sign-in. app/(app)/transactions/page.tsx has the same bad-merge pattern (duplicate imports, dozens of now-undefined names) and doesn't compile, so keyboard-nav verification on the transactions page specifically wasn't possible; kellymusk#589 and kellymusk#472 already track downstream symptoms of it. Everything it shares with the other three pages (Button, Input, Select) is already fixed here. Closes kellymusk#473 Closes kellymusk#474 Closes kellymusk#475 Closes kellymusk#476
|
@JosephOladele is attempting to deploy a commit to the kelly musk's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@JosephOladele Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four issues, all assigned to me:
app/(app)/wallet/page.tsxhad no tests. Added coverage for the loading spinner, the "set up your payment address" state whenGET /walletreturns 400, the loaded wallet-address view, and the copy-to-clipboard button's confirmation state.outline-noneusage across the app plus the login/charge/transactions/withdraw pages' full component trees. Found and fixed two inputs with no focus-visible alternative at all (landingPricing's country<select>,AmountWidget's amount<input>). Also found — via actually tabbing through the login page in a browser, not just static review — that the defaultButtonvariant's focus ring (ring-ring/50) is nearly invisible against its ownbg-primarybackground on primary-colored buttons like "Sign in"; addedring-offset-2/ring-offset-backgroundso the ring is visually separated regardless of button color. Everything else (Input,Select, links, otherButtonvariants) already pairedoutline-nonewith a workingfocus-visible:alternative.<Button>/<button>in the app for icon-only content with no accessible name. The two examples the issue names (charge page's backspace key, wallet page's copy button) already have visible text/aria-labels. Found three that didn't, all in the send flow: the header back button, the QR-scan button, and the numpad backspace key — addedaria-labelto each.app/(app)/layout.tsx, visible on focus, targeting a newmain#main-content.Closes #473
Closes #474
Closes #475
Closes #476
A blocking bug found along the way
components/session-provider.tsxdidn't compile ondev— a bad merge between the Freighter-auth PR and the session-persistence PR left duplicate imports, a duplicate localSessiontype shadowing the one imported from@/lib/api, and two interleavedsignOutimplementations (the second literally opening inside the first's unclosed body). This blocks anything importinguseAuthenticatedSession, including the wallet page under test here. Reconciled it to the cookie-basedapi.getSession()/api.logout()restoration path (the direction issue #582 already points toward) plus the Freighter sign-in flow.What I couldn't verify
app/(app)/transactions/page.tsxhas the exact same bad-merge pattern (duplicate imports, dozens of now-undefined names) and doesn't compile at all, so I couldn't keyboard-test it directly for a11y: add visible focus indicators to all interactive elements #474. Issues Fix: transactions list capped at 50 with no pagination — older payments are inaccessible #589 and test(transactions): write tests for transaction list page #472 already track downstream symptoms of this (missing pagination, no tests). Everything that page shares with login/charge/withdraw (Button,Input,Select) is already fixed in this PR, so it'll be correct once that separate compile bug is fixed.Test plan
npx jest— 44/44 passing, including the pre-push hook's own full run#main-content, hidden while session isn't ready), 3 for the two send-flow icon-button labels + backspace key, 2 for the focus-ring className presence on the two fixed inputsnpx eslinton every changed file — clean (one pre-existingjsx-a11y/label-has-associated-controlerror insend-page-client.tsxpredates this PR, confirmed viagit stash, not touched here — different a11y concern than the three issues above)npx tsc --noEmit— no new errors/loginpage confirming every focusable element gets a clearly visible ring, confirmed thePricingselect andAmountWidgetinput rings render correctly. Before thering-offsetfix, the "Sign in" button's ring was the same hue as its own background and hard to see; after, there's a clear gap around the button regardless of its color.