Repository navigation
fix: align paykit auth sheet with figma - #1421
Conversation
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
a2e9569 to
18cbbec
Compare
18cbbec to
355a467
Compare
ed9dc03 to
1dcb6ad
Compare
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete pull request diff against merge base 50c9bc34, at 1dcb6ad. No earlier assessment baseline was available.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
The grant, local unlock, and capabilities path are unchanged. Consent copy matches the linked Figma frames and the comparison revision 12e44e9 on bitkit-ios#880. wallet-leg.xml, paykit-only-approval.xml, and paykit-reconnect.xml still match the Paykit details sentence and the existing test tags. An earlier note about a non-scrolling footer is already addressed here: the warning and profile scroll with the details when the column is taller than the sheet (thread).
Validation: Inspected PubkyAuthApprovalSheetTest (accent-tag stripping only). I did not run unit tests or a device. On this commit, GitHub CI build, lint, and the local e2e matrix 37923940835 succeeded. Those e2e jobs do not open this sheet. Device testing: not performed in this review. iOS was comparison-only, not a full review.
Non-blocking consistency suggestions: The Authorize frame stacks Before you continue and the profile directly under the details, with the spare space above the buttons. This sheet, like iOS, expands a spacer so that group sits on the bottom when the content is short. iOS still joins every service name with "and"; Android still shows only the first. The success check is scaled uniformly in a 256dp slot; the Authorized frame places a 274×206 image inside a 256×192 frame.
Suggested additional test cases:
- Android, dev build,
pubkyauth://signin_grantwhosecidis longer than the sheet text width and has no spaces: open Authorize and Success. The requester is fully visible or ellipsized, and the permission sentence stays fully visible. - Android, same flow, with a
cidthat contains an encoded newline plus a second sentence, and one that contains a bidi override: the sheet shows one consent sentence in the app's word order, with no extra line. - Android earn consent on a phone-width sheet: the coin stays clear of the title, matching the Earn frame.
Findings
- [MEDIUM] Isolate requester text in the consent sentence — inline at
app/src/main/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalSheet.kt:510. - [LOW] Clip the earn coin to its illustration frame — inline at
app/src/main/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalSheet.kt:298.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1dcb6ad to
fd722c3
Compare
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Aligns the Pubky auth approval sheet with the Paykit auth frames: the requester named in the lead and success sentences, sectioned permissions, details and trust warning, a compact profile card, and request text rendered literally. Reviewed fd722c3, full tier (the sheet is on the auth approval path), reasoned from the code and CI: per the repo's agent rules the Android branch is not built on the reviewing machine, so the layout was not run on a device here.
What I checked, and 5 candidates I ruled out
Read in full: PubkyAuthApprovalSheet.kt, PubkyAuthApprovalSheetTest.kt, the strings.xml hunk, ui/utils/Text.kt (withAccent, removeAccentTags)
Call sites traced: clientId and serviceName from PubkyRepo.parseAuthUrl through PubkyAuthRequest.parse / parseSignup / parseGrantSignup into PubkyAuthApprovalViewModel.load
CI: build, build-local, lint, detekt, Greptile green; 7 e2e-tests-local shards still pending at review time
Ruled out
- Markup injection through the request text: hidden characters are dropped before the accent strip, and the strip repeats until stable, so a tag split by a zero-width or nested inside another cannot survive into
withAccentBoldBright. A fragment likex</accnext to the template's own</accent>cannot open a new span; the regex is non-greedy and at worst bolds the fragment inside the requester's own span. Covered by the two new tests. - Signup requests losing the sentence:
parseSignuppassesclientId = "", which takes theprofile__auth_approval_service/success_detailfallback, so the signup flow keeps the old wording. - Requester id is caller-chosen:
cidis whatever the requesting app puts in the URL; the sheet already showed it before this PR asRequester ID, and naming it in the lead sentence is the linked frame's choice, so not a regression introduced here. - Removed or reworded strings with stale translations: no
values-*locale carriesprofile__auth_approval_requester,paykit_access_title,paykit_access_descriptionortrust_warning, so nothing dangles. - Journeys asserting the old text or tags:
paykit-only-approval.xml,paykit-reconnect.xml,wallet-leg.xmland the twopubky-authjourneys assert onPubkyAuthPaykitAccess,PubkyAuthAuthorize,PubkySignupHomeserverandPubkyAuthOK, all kept, and their prose (no spending keys shared, profile name and key shown) still matches the new copy and card.
Merge confidence: 4/5, the e2e shards were still running and the layout change was not exercised on a device by this review.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at fd722c33 after the comparison base changed and no verified baseline was available to inherit. This pass covers the complete diff from merge base 3cac93b0.
No new actionable code findings.
The grant path is unchanged: local PIN or biometric unlock still runs, and approval still submits the raw client id, capabilities, and companion claim. Literal display filtering applies only to the requester id and service name in the authorize and success sentences. asLiteralRequestText drops control, format, line-separator, and paragraph-separator characters, then removes <accent> tags, including tags rebuilt by nesting or by deleting invisible characters. A Java replica of that filter produced the same results as the new PubkyAuthApprovalSheetTest cases. String.format leaves % inside those values as literal text.
The open consent-sentence thread describes newline and bidi-override injection from 1dcb6ad. At this revision those characters are removed before the sentence is styled. The sentence uses unbounded soft-wrapped body text, so an overlong id is a wrapping run. This review did not render that layout. The author reports a 229-character id wrapping on a Pixel 9 at this commit. The open coin thread assumes a solid 329dp bitmap. The opaque pixels of coin_stack_4.png end about 21dp above the bottom of the 256dp slot after the 329/256 scale and 7dp downward shift, and the 32dp spacer sits below that.
iOS 508c58f0 on bitkit-ios#880 uses the same character filter and sentence structure. That comparison is limited to this sheet.
Non-blocking consistency suggestions: Android still names only the first service in the lead sentence. iOS joins every service name with "and". Both platforms list every permission path. The Android first-service choice is unchanged by this layout.
Validation: Gradle and a device were not run here. GitHub unit tests and lint succeeded on this revision. wallet-leg.xml, paykit-only-approval.xml, and paykit-reconnect.xml still match the Paykit details wording and the existing test tags. Local e2e shards were still running and do not open this sheet.
Device testing: not performed in this review.
Ready for device testing.
piotr-iohk
left a comment
There was a problem hiding this comment.
Tested a local build of fd722c3 on an Android emulator. Focused smoke passed: Earn → Authorize → Success → OK, cancellation, reopening with a fresh requester, and a 253-character requester at font scale 2.0. Verified the requester, permissions, details and trust warning remained readable by scrolling, and the controls remained usable. Restored the normal text setting afterward.
Authorization used a controlled local relay. This was a focused UI smoke; physical hardware and PIN/biometric prompts were not tested.
This PR aligns the Paykit authorization sheet with the Auth (paykit) flow in Figma.
Description
app.paykit.server is requesting permission…) and removes the separateRequester IDline. The old sentence stays as the fallback when a request carries no client id.REQUESTED PERMISSIONS,DETAILSandBEFORE YOU CONTINUEheadings with 32dp between sections, removes the divider under the permissions, uses the Figma copy for the Paykit details and trust warning, and sets both to the 15sp body style.<accent>tags, line breaks, bidi controls and other invisible characters in them are removed before the sentence is styled, so a request cannot inject emphasis, add lines or reorder the sentence. A long id wraps in full.Out of Scope
strings.xml: the Figma title reads "Authorization Succesful"; the existing correctly spelled "Authorization Successful" is kept.Authorize paykit FaceIDframe: the Face ID prompt is the iOS system dialog; Android keeps its own biometric/PIN prompt.Type.kt: body styles keep the app-wide 0.4sp letter spacing where the frames show 0.bitkit-ios: ported in fix: align paykit auth sheet with figma bitkit-ios#880.Design
Preview
Pixel 9 emulator, dev build, combined
paykit-access-v1.watch-only-account-v1claim.QA Notes
Journeys
N/A — no journey added or updated. The route and test tags are unchanged, so
wallet-leg.xmlandpaykit-only-approval.xmlstill drive this sheet as written.Manual Tests
pubkyauth://signin_grantURL withcaps=/pub/paykit/:rw, acidandx-bitkit-claim=paykit-access-v1.watch-only-account-v1→ the Earn, Authorize, Authorizing and Success screens match the Figma frames above — needs a valid auth URL fixture.DETAILSsection — needs a valid auth URL fixture.Automated Checks
PubkyAuthApprovalSheetTest.kt— accent tags in a requester id are removed, including nested ones, and so are line breaks and bidi controlsjust compile,just test,just lint— pass locally.