Skip to content

fix: align paykit auth sheet with figma - #880

Open
jvsena42 wants to merge 4 commits into
masterfrom
fix/paykit-auth-figma-parity
Open

jvsena42 wants to merge 4 commits into
masterfrom
fix/paykit-auth-figma-parity

Conversation

@jvsena42

@jvsena42 jvsena42 commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

iOS port of synonymdev/bitkit-android#1421.

This PR aligns the Paykit authorization sheet with the Auth (paykit) flow in Figma.

Description

  • Earn consent: uses the three-coin illustration from the frame (coin-stack-4).
  • Authorize: names the requester in the lead sentence (app.paykit.server is requesting permission…) and removes the separate Requester ID line. The old sentence stays as the fallback when a request carries no client id.
  • Authorize: groups the content under REQUESTED PERMISSIONS, DETAILS and BEFORE YOU CONTINUE headings with 32pt between sections, removes the divider under the permissions, and uses the Figma copy for the Paykit details and trust warning.
  • Profile card: replaces the centered 96pt avatar card with the compact row (48pt avatar, truncated key above the name). This applies to every Pubky auth dialog. Its placeholder avatar, shown when the profile has no picture, is grey like the app's other avatar placeholders; it was Pubky green.
  • Authorizing: the label uses the 15pt button text style.
  • Success: names the requester in the summary sentence, insets the text 32pt from the sheet edges and draws the check illustration at the frame's scale. The content scrolls when a long requester id does not fit, so the OK button stays on screen.
  • The requester id and service name are shown literally: <accent> tags in them are removed before the sentence is styled, so a request cannot inject its own emphasis.
  • Permission row: folder icon is 16pt.

Out of Scope

  • PubkyAuthApprovalSheet.swift: the AUTHORIZATION RELAY section and the relay line on the Earn screen are iOS additions that the Figma frames do not show; they are kept.
  • SheetIntro.swift: the Earn screen keeps the shared intro component's sizing and side insets instead of the frame's.
  • Localizable.strings: the Figma title reads "Authorization Succesful"; the correctly spelled title is kept. New and changed strings are English only.
  • Authorize paykit FaceID frame: Face ID is the system prompt and is unchanged.

Design

Preview

iPhone 17 simulator, fresh wallet and Bitkit-created Pubky profile, combined paykit-access-v1.watch-only-account-v1 claim.

iOS and Figma

QA Notes

Journeys

N/A — no journey added or updated. The route and identifiers are unchanged, so wallet-leg.xml and paykit-only-approval.xml still drive this sheet as written.

Manual Tests

  • Open a bitkit://pubky-auth/setup URL with caps=/pub/paykit/:rw, a cid and x-bitkit-claim=paykit-access-v1.watch-only-account-v1 → the Earn, Authorize, Authorizing and Success screens match the image above — needs a valid auth URL fixture.

Automated Checks

  • updated PubkyAuthApprovalSheetTests.swift — accent tags in a requester id are removed, including nested ones
  • ran PubkyAuthApprovalSheetTests, swiftformat --lint and node scripts/validate-translations.js — pass locally; a 253-character and a tag-wrapped cid were checked on the iPhone 17 simulator.

Base automatically changed from codex/paykit-shared-runtime-local-20260930 to master October 7, 2026 17:01
@jvsena42
jvsena42 force-pushed the fix/paykit-auth-figma-parity branch from 685a89e to a06a143 Compare October 8, 2026 09:11
@jvsena42
jvsena42 marked this pull request as ready for review October 8, 2026 09:37
@greptile-apps

greptile-apps Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Updates text and layout of an authorization dialog.

The PR appears safe to merge; no actionable issues were established.

What we checked:

  • New text still appears: LocalizationHelper.getString uses the English resource when the selected language lacks a key. The new keys are present in English.

Summary

Updates the Pubky authorization sheet’s copy and layout to match the supplied design.

  • Names the requester in approval and success text.
  • Adds section headings, adjusts spacing and illustrations, and uses a compact profile card.
  • No actionable issues were established.
  • jvsena42 explicitly keeps English-only strings, the authorization relay sections, shared intro sizing, the correctly spelled success title, and the existing Face ID prompt.

Reviews (1) · Last reviewed commit: "fix: align paykit auth sheet with figma" · Reviewed by Greptile

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No findings. Aligns the Pubky auth sheet with the Figma frames: names the requester in the lead and success sentences, adds the DETAILS and BEFORE YOU CONTINUE headings, and switches to the compact profile row. Reviewed a06a143, full tier (an approval sheet is a security path), reasoned from the code and CI: iOS does not build on this box.

What I checked, and 3 candidates I ruled out

Read in full: PubkyAuthApprovalSheet.swift; traced config.request.clientID back to Paykit.parsePubkyAuthUrl in PubkyAuthRequest.swift; t(_:variables:) in LocalizeHelpers.swift
CI: validate (translations), Greptile and change detection green; Run Tests, Run Integration Tests and build-local were still pending at review time

Ruled out

  • Requester id lost from the screen: the separate Requester ID line goes, but the id now appears in the lead sentence whenever it is non-empty, and the fallback sentence covers the empty case, so a user still sees who is asking before approving.
  • Removed string keys still referenced: no Swift reference to pubky_auth__requester or pubky_auth__paykit_access_title remains, and the other localizations carry no copy of them.
  • Missing illustration: coin-stack-4.imageset exists in Assets.xcassets/Illustrations.

Merge confidence: 4/5, no findings and copy or layout a unit test would not apply to, but the test and build jobs were still pending and the branch was not built here.

@jvsena42 jvsena42 self-assigned this Oct 8, 2026
@jvsena42
jvsena42 added this pull request to stack #895 October 8, 2026 10:33
@jvsena42
jvsena42 requested review from a team, ben-kaufman and coreyphillips and removed request for a team October 8, 2026 11:52
@coreyphillips

Copy link
Copy Markdown
Contributor

Two independent reviews.

worth doing, does not block

  • Success screen now renders an unbounded, requester-supplied client id (Bitkit/Views/Sheets/PubkyAuthApproval/PubkyAuthApprovalSheet.swift:331). The success sentence now interpolates config.request.clientID through pubky_auth__success_named, but successContent is not in a ScrollView. It stacks the text, a fixed 256pt check scaled to about 274pt, and the OK button. The old requester line was lineLimit(1) with tail truncation. The new text wraps without limit, and the cid comes straight from the auth URL. A long cid could push the OK button (PubkyAuthOK) off screen. I did not find a length cap on cid in PubkyAuthRequest.parse, but I did not check whether Paykit's parser enforces one, so this is unconfirmed. The authorize screen is scrollable, so it is not affected.
  • Requester IDs are parsed as accent markup (Bitkit/Views/Sheets/PubkyAuthApproval/PubkyAuthApprovalSheet.swift:303). Requester IDs can inject <accent> tags into the authorization copy. The pinned ClientId validation accepts any nonempty value up to 253 bytes, while AccentedText interprets these tags instead of displaying the ID literally. This affects both authorization and success text. Escape markup tokens or compose emphasized spans without parsing request data.
  • Changelog fragment omits the pull request reference (changelog.d/next/paykit-auth-figma-parity.changed.md:1). The fragment filename does not follow the required <issue-or-pr>.<category>.md convention. Rename it to 880.changed.md so the collected release entry retains its pull request reference.

nits

  • Changelog fragment is not named after the PR number (changelog.d/next/paykit-auth-figma-parity.changed.md). The fragment is changelog.d/next/paykit-auth-figma-parity.changed.md, but the convention in .agents/commands/pr.md (section 8b) and AGENTS.md is <issue-or-pr>.<category>.md. Every other fragment in the directory follows it. It should be 880.changed.md.

the reviewers disagree, your call

  • Requester is no longer shown when a request has no permissions (Bitkit/Views/Sheets/PubkyAuthApproval/PubkyAuthApprovalSheet.swift:240). Objection: Paykit rejects empty or invalid capability lists before this view is shown, while direct signup requests have an empty client ID. The claimed production state, a nonempty client ID with no parsed permissions, is not reachable.

coreyphillips
coreyphillips previously approved these changes Oct 8, 2026
@jvsena42

jvsena42 commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Went through the review.

  • Unbounded client id on the success screen: fixed in 508f705. The success content is in a scroll view with the OK button outside it. Checked on the iPhone 17 simulator with a 253-character cid: the text scrolls and PubkyAuthOK stays on screen.
  • Requester ids parsed as accent markup: fixed in 508f705. pubkyAuthLiteralText removes <accent> tags from the client id and service name, repeatedly so nested tags cannot rebuild one. Covered in PubkyAuthApprovalSheetTests.swift. Android got the same fix in fix: align paykit auth sheet with figma bitkit-android#1421.
  • Changelog fragment name: fixed in 3d58d12, now 880.changed.md.
  • Requester not shown with no permissions: not changed. As the objection says, a nonempty client id with no parsed permissions is rejected before the sheet opens.

jvsena42 and others added 4 commits October 9, 2026 08:24
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>
@jvsena42
jvsena42 force-pushed the fix/paykit-auth-figma-parity branch from 3d58d12 to 12e44e9 Compare October 9, 2026 11:26

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: Full review of the PR diff from merge base 33982a09, at 12e44e9.

2 actionable findings — resolve or provide an evidence-backed rebuttal.

The named authorize and success sentences can rewrite a requester id or service name that contains another {placeholder}. Paykit accepts those strings, and this sheet no longer has a separate requester line. The matching Android sheet in bitkit-android#1421 at 1dcb6ad formats with %s and leaves the same strings unchanged. Accent-tag removal matches AccentedText, but the new unit test is nested inside XCTAssertThrowsErrorAsync and is not an XCTest method.

Validation: Run Tests passed on 12e44e9. This review did not rerun that suite. Device testing was not performed. The Figma frames linked on the PR were not re-rendered here. iOS still shows the authorization relay block and uses the shared intro sizing; that is intentional in this PR.

Recommended before device testing: show placeholder-bearing requester ids and service names literally, and attach the accent-tag test to PubkyAuthApprovalSheetTests.

Suggested additional test cases

  • iOS, fresh wallet, valid auth URL with cid={service} and caps=/pub/paykit/:rw. Open the authorize sheet. The lead sentence shows {service} as the requester, not paykit.
  • Same setup with cid=bank.com and caps=/pub/{clientId}/:rw. The service portion shows {clientId}, and the permission row shows /pub/{clientId}.
  • Run testRequestTextLosesAccentMarkupEvenWhenTagsAreNested as a method of PubkyAuthApprovalSheetTests. The nested-tag assertions execute and pass.

Findings

  • [MEDIUM] Consent sentence rewrites requester ids that contain placeholders — inline at Bitkit/Views/Sheets/PubkyAuthApproval/PubkyAuthApprovalSheet.swift:333.
  • [LOW] Accent-tag test is nested where XCTest will not run it — inline at BitkitTests/PubkyAuthApprovalSheetTests.swift:625.

t("pubky_auth__description_prefix") + "<accent>" + serviceText + "</accent>" + t("pubky_auth__description_suffix"),
requesterText.isEmpty
? t("pubky_auth__description_prefix") + "<accent>" + serviceText + "</accent>" + t("pubky_auth__description_suffix")
: t("pubky_auth__description_named", variables: ["clientId": requesterText, "service": serviceText]),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Consent sentence rewrites requester ids that contain placeholders

Trigger: a Paykit auth URL whose client id or service path contains another consent placeholder, such as cid={service} with caps=/pub/paykit/:rw, or cid=bank.com with caps=/pub/{clientId}/:rw. ClientId::new in pubky-common 0.15.0 (paykit-rs 2ea4b2b) accepts any non-empty string up to 253 characters, and storage paths allow { in a segment.

Mechanism: descriptionText and successDescriptionText pass those values through t(), which replaces {clientId}, {service}, and {pubky} with replacingOccurrences and then keeps scanning the result for the remaining keys. Dictionary order is not fixed. pubkyAuthLiteralText only removes <accent> tags, so a value such as {service} is inserted and can be replaced by the service name. The same pass can turn a service name of {clientId} into the client id, and on the success sentence a client id containing {pubky} can include the truncated pubky.

Consequence: this path would name a different requester or service than the grant. The lead sentence is now the only client-id display, so a cid of {service} can read as paykit while the authorized client id remains {service}. The permission row still shows the raw path, but not the client id.

Expected behavior: show the requester id and service name literally, including {service}, {clientId}, and {pubky}. Substitute each template placeholder from the original string only, without scanning inserted values. Android already does that with positional %s arguments.

Evidence basis: source analysis of t() and the two call sites at 12e44e9, plus the same replacement sequence checked outside the app. The iOS UI was not run. Run Tests does not cover this input.

XCTFail("Expected expression to throw", file: file, line: line)
} catch {}

func testRequestTextLosesAccentMarkupEvenWhenTagsAreNested() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Accent-tag test is nested where XCTest will not run it

Trigger: PubkyAuthApprovalSheetTests runs in CI or locally.

Mechanism: testRequestTextLosesAccentMarkupEvenWhenTagsAreNested is declared inside the file-private function XCTAssertThrowsErrorAsync, after PubkyAuthApprovalSheetTests has already closed. Nothing calls that nested function. XCTest only runs methods on the test class, so the three XCTAssertEqual checks are never executed. The function still typechecks, which is why the suite can pass.

Consequence: a regression in pubkyAuthLiteralText would not fail CI. The PR presents this test as coverage for nested <accent> tags.

Expected behavior: declare the test as a method on PubkyAuthApprovalSheetTests so the nested-tag cases run with the class.

Evidence basis: source analysis of BitkitTests/PubkyAuthApprovalSheetTests.swift at 12e44e9. The suite was not rerun here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants