Skip to content

fix: apply design nits - #1179

Merged
ovitrif merged 13 commits into
masterfrom
ovi/tether-icon
Oct 31, 2026
Merged

ovitrif merged 13 commits into
masterfrom
ovi/tether-icon

Conversation

@ovitrif

@ovitrif ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Twin: synonymdev/bitkit-android#1741

Description

  • Replaces the 3D Tether image (tether-coin image set) with the updated render from Figma, which fixes the brand mark's accuracy. It shows on the USDT intro sheet, the USDT wallet header and the USDT network pickers.
  • Places that image as the Figma Visual component does: 256 pt square, 32 pt down inside its 256 pt box. The old 304.76 × 228.57 crop was tuned to the old render's padding and would draw the new render about 20% too large.
    • On the USDT wallet header this moves the coin about 30 pt down, to where the Wallet USDT frame draws it: the same Visual component, flipped.
  • Adds "quickly" to the USDT intro description, in English and every locale: "Enable USDT (Tether) to quickly send and receive digital dollars with Bitkit."
  • Colours send reviews by the paying rail on every send flow, as in the Payment Request frames: lightning purple with the bolt icon, onchain orange with the speed icon, USDT teal with the $ icon.
    • While a payment request prepares, its colour follows the rail it accepts, not the last send's wallet: lightning-only purple, onchain-only orange, USDT teal, and neutral when it accepts both bitcoin rails.
    • The USDT review's Show Details button uses the new currency-dollar icon instead of the speed icon.
  • Always shows Request Payment on Subscriptions › Payments. With no profile, or once contacts have loaded and are empty, it opens Add Contact, through the same route as the Contacts intro: the Add Contact sheet when signed in, otherwise profile setup first. Otherwise it opens the recipient picker, which says when none of them can receive a request. The empty state now also reads "Request Payment", as in Figma.

Out of Scope

Design

Bitkit - Handoff v63:

Preview

Figma (left) and this PR on a simulator (right):

Intro Wallet Network Payments

Recordings (2x):

USDT intro, wallet, network picker Request Payment with no contacts
ios-flow2x.mp4
ios-request2x.mp4

QA Notes

Setup

For J1's contact steps: create a Pubky profile in the app on two devices (staging homeserver), then add each other through Add Contact.
Debug build on regtest with the default USDT endpoints, fresh wallet with Paykit on and no contacts.

Journeys

  • J1: new request-payment-without-contacts.xml — Request Payment shows with no contacts and opens Add Contact; with contacts it opens the recipient picker

Manual Tests

  • 1. Open a Paykit payment request while it prepares → lightning-only purple, onchain-only orange, neutral when it accepts both — needs a linked Paykit contact sending a request
  • 2. Review a USDT send → Show Details shows the teal $ icon — needs a USDT balance for a quote

Automated Checks

  • updated PaykitPaymentRequestServiceTests.swift — a preparing request takes its colour from its accepted methods: USDT, lightning-only, onchain-only and mixed

Evidence

  • On 2953738, Debug build, iOS 27 simulator: the intro sheet, USDT wallet header and receive network picker show the new render at the Figma size (pentagon ≈193 pt wide, flush 32 pt above the intro headline), and the intro shows the new copy.
  • On 180987f: J1 passes in full on a device, with two profiles linked as private Paykit contacts: no profile opens profile setup; signed in with no contacts opens the Add Contact sheet; with a saved contact the recipient picker lists it (QA).
  • On 375538f: manual tests pass for every rail, staged with a local debug-only hook (QA).
  • node scripts/validate-translations.js passes; SwiftFormat is clean; PaykitPaymentRequestServiceTests passes on 8fbd4e6.

Gaps

Models used

  • Planning/scoping: claude-opus-5-5 (reasoning: Not exposed)
  • Implementation: claude-opus-5-5 (reasoning: Not exposed)
  • Review: Not performed

The session metadata does not expose the reasoning setting.

@ovitrif ovitrif added this to the 3.1.0 milestone Oct 30, 2026
@ovitrif ovitrif self-assigned this Oct 30, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T21:22:59.904503Z 2953738 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@greptile-apps

greptile-apps Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge; no actionable defects were found.

Summary

Updates the Tether illustration and adds “quickly” to the English USDT intro.

  • USDT screens show the updated Tether coin at its new size and position.
  • The USDT intro says users can quickly send and receive digital dollars.

No actionable issues were found. Image viewing was unavailable, so the artwork itself could not be checked. Native iOS screen capture was also unavailable in this Linux environment.

Scope acknowledged by ovitrif:

  • Translated copy stays unchanged; this change is limited to English.
  • The wallet header caller’s screen positioning stays unchanged.

Reviews (1) · Last reviewed commit: "fix: update tether icon and usdt copy" · Reviewed by Greptile

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2953738b4d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ovitrif
ovitrif requested review from a team, ben-kaufman, coreyphillips and pwltr and removed request for a team and pwltr October 30, 2026 21:23

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No findings. Swaps the tether-coin image set, draws UsdtCoinIllustration as a 256 pt scaledToFit frame offset 32 pt down, and adds "quickly" to the English intro copy. Reviewed 8d7313d, light tier, static: iOS does not build on this reviewer's Linux host, so this rests on the code and CI.

What I checked, and 2 candidates I ruled out

Read in full: UsdtWalletScreen.swift, UsdtNetworkPicker.swift, UsdtIntroSheet.swift, the Localizable.strings hunk
Call sites traced: all three UsdtCoinIllustration callers (wallet header, network picker, intro sheet)
Asset: byte-identical to the Android twin's tether_coin.png; 1024x1024, opaque bounds 771x721+125+174, so the 32 pt offset lands the art flush with the frame bottom
CI: SwiftFormat and translation validation passed; unit and integration tests were still pending at review time

Ruled out

  • Offset drawing outside the ScrollView content in the network picker: the part pushed past the frame is the PNG's transparent bottom margin.
  • Translations not updated: the other locales keep the old, still accurate wording, and the PR scopes it out explicitly.

Merge confidence: 4/5, no findings, but the test runs had not finished and the app was not built here.

Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the review session's metadata does not report it)

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two independent reviews, nothing blocking a merge.

worth doing, does not block

  • usdt__intro_description changed in English only, against repo policy (Bitkit/Resources/Localization/en.lproj/Localizable.strings:1951)

nits

  • Wallet header coin moves about 30 pt down, but the description says unchanged (Bitkit/Views/Wallets/UsdtWalletScreen.swift:22-25). The Out of Scope note says the wallet header illustration's screen position is unchanged. It is not, because the new .offset(y: 32) sits inside the shared UsdtCoinIllustration, which UsdtWalletScreen also renders under its own .offset(x: 97, y: 4). I measured the pentagon's opaque bounds in both PNGs and applied each version's layout math: - master: 1000 px image, scaledToFill into 304.76 × 228.57, clipped, then centred in 256. The pentagon spans y ≈ 45 to 221 in the 256 box. - this PR: 1024 px image, scaledToFit at 256, plus the 32 pt offset. The pentagon spans y ≈ 75 to 256. Horizontal placement is within about 2 pt of master. In the PR's Wallet screenshot the coin is still left of and less cropped than in the Figma frame, so it is worth checking that this shift is intended. If it is, say so in the description. If the wallet header should stay put, compensate in UsdtWalletScreen. On the network pickers the illustration is the last item in a ScrollView, so the 32 pt drawn below the layout box can be clipped. I checked the new PNG: every row below the pentagon (rows 896 and down) has alpha 2 or less, so nothing visible is lost.

@ovitrif ovitrif changed the title fix: update tether icon and usdt copy fix: apply design nits Oct 30, 2026
@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Wallet header coin moves about 30 pt down, but the description says unchanged

@coreyphillips The move is intended: the Wallet USDT frame draws the header coin with the same Visual component (flipped), so the shared 32 pt offset puts it where Figma has it. The description now says so instead of "unchanged". Translations for "quickly" are in 57105f9, all 14 locales.

Since your review, this PR also carries Aldert's other design nits as separate commits (now titled fix: apply design nits):

  • 1fa2bca: send reviews take the paying rail's colour (no grey while a payment request prepares), and the USDT review's Show Details uses a new $ icon.
  • 66a2a97: Request Payment always shows on Subscriptions › Payments and opens Add Contact when no contact can receive a request.

The description, Preview and QA Notes are updated for the new head 9725012.

@ovitrif
ovitrif requested a review from coreyphillips October 30, 2026 21:44
@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

QA on 9725012

Debug build, iOS 27 simulator. Both cases were staged with a local debug-only hook: a preparing request in the real send sheet, and a fixed quote in the real USDT review. The hook is not part of this PR.

  • 1. Payment request while it prepares: lightning shows the purple swipe, bolt and FROM/FOR icons, onchain the orange ones with the speed icon; no grey state.
  • 2. USDT review: Show Details shows the teal $ icon next to the teal swipe.
  • J1 (request-payment-without-contacts.xml), on 66a2a97: Request Payment shows on the empty Payments tab; with no profile it opens profile setup, the step before Add Contact.

ios-preparing2x.mp4

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rechecked after 4 new commits (57105f9, 1fa2bca, 66a2a97, 9725012). One point still blocks a merge.

addressed

  • usdt__intro_description changed in English only, against repo policy (Bitkit/Resources/Localization/en.lproj/Localizable.strings:1951). Fixed: All 14 non-English files now carry the adverb in usdt__intro_description (for example de.lproj:1817 "schnell", fr.lproj:1831 "rapidement", ru.lproj:1811 "быстро"). I grepped the key in all 15 locale files, and node scripts/validate-translations.js reports 0 errors.
  • Wallet header coin moves about 30 pt down, but the description says unchanged (Bitkit/Views/Wallets/UsdtWalletScreen.swift:22-25). Fixed: The updated description explicitly acknowledges the intended 30 pt downward shift. I checked Bitkit/Views/Wallets/UsdtWalletScreen.swift:22-25 and :119-124, confirming that the shared offset reaches the flipped wallet header.

new since the last review

needs changing before merge

  • Preparing-request accent change crashes an existing unit test (BitkitTests/PaykitPaymentRequestServiceTests.swift:15-30). testPreparationUsesAcceptedMethodsBeforeWalletSelection now crashes the test host. accentColor (SendConfirmationView.swift:88-97) no longer returns early for a preparing request that is not USDT-only. A [usdt, bitcoin] request therefore falls through to app.selectedWalletToPayFrom. The test (BitkitTests/PaykitPaymentRequestServiceTests.swift:15-30) builds SendConfirmationView with no environment. For that case it expects .textSecondary. Reading app outside a view hierarchy is a fatal error. Even with an environment, no branch returns .textSecondary any more. The test is not in the skip list in .github/workflows/unit-tests.yml, so the unit suite fails (Run Tests was still pending on 9725012 when I looked). I confirmed it by running only this test at HEAD 9725012 on an iPhone 17e simulator (Xcode 27.1): xcodebuild test -project Bitkit.xcodeproj -scheme Bitkit -only-testing:BitkitTests/PaykitPaymentRequestServiceTests/testPreparationUsesAcceptedMethodsBeforeWalletSelection ... SwiftUICore/EnvironmentObject.swift:93: Fatal error: No ObservableObject of type AppViewModel found. ** TEST FAILED ** (exit 65) On master the same case returns before app is read, so this is new. The test needs the new expectation. It also needs an injected AppViewModel, or the colour choice moved into a static helper that takes the wallet type, the way confirmationFeeRate is. The PR's Automated Checks says N/A, but this test covers the changed behaviour.

worth doing, does not block

  • Preparing review uses the default wallet's colour, not the paying rail (Bitkit/Views/Wallets/Send/SendConfirmationView.swift:88-97)
  • Request Payment opens Add Contact for users who already have contacts (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:366-373)
  • Lightning requests use onchain tint while preparing (Bitkit/Views/Wallets/Send/SendConfirmationView.swift:89-97)

nits

  • New journey's identifier differs from Android and is not in the naming table (journeys/subscriptions/request-payment-without-contacts.xml:10)

@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

QA on 375538f

Pushed e16a1a9 and 375538f, answering @ben-kaufman's thread and the preparing-colour point:

  • e16a1a9: when no target is eligible yet but saved contacts exist, Request Payment refreshes eligibility before routing, so Add Contact opens only once eligibility is known to be empty.
  • 375538f: while a request prepares, the colour comes from the request's accepted methods, not the last send's wallet: lightning-only purple with the bolt, onchain-only orange with the speed icon, mixed or unknown neutral until the rail is chosen.

Debug build, iOS 27 simulator, cases staged with a local debug-only hook (not part of this PR) and the pay-from wallet left on onchain:

  • 1. Preparing request: lightning-only purple with the bolt, onchain-only orange, mixed neutral.
  • 2. USDT review: Show Details shows the teal $ icon next to the teal swipe.
Preparing and USDT review J1
ios-preparing22x.mp4
ios-j12x.mp4
  • J1 (request-payment-without-contacts.xml): with no contacts, Request Payment opens profile setup, the step before Add Contact.

@ovitrif

ovitrif commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Preparing-request accent change crashes an existing unit test

@coreyphillips Fixed: since 375538f a preparing request returns its colour from its accepted methods before app is read, so the test no longer touches the environment, and [usdt, bitcoin] is .textSecondary again. 8fbd4e6 extends testPreparationUsesAcceptedMethodsBeforeWalletSelection with the lightning-only, onchain-only and both-bitcoin cases; it passes locally on the head. Your other four threads are answered and resolved, and the simulator QA for the new head is in #1179 (comment).

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rechecked after 4 new commits (e16a1a9, 375538f, a2bf9a8, 8fbd4e6). Nothing blocking remains.

addressed

  • Preparing-request accent change crashes an existing unit test (BitkitTests/PaykitPaymentRequestServiceTests.swift:15-30). Fixed: SendConfirmationView.swift:89-90 now returns the static preparingAccentColor before app is read, and [usdt, bitcoin] maps to .textSecondary again. I ran testPreparationUsesAcceptedMethodsBeforeWalletSelection at 8fbd4e6 on an iPhone 17e simulator: it passed, and so did all 302 tests in PaykitPaymentRequestServiceTests.
  • New journey's identifier differs from Android and is not in the naming table (journeys/subscriptions/request-payment-without-contacts.xml:10). Fixed: journeys/README.md:180 now maps Android PaymentRequestCreate to iOS PaymentRequestRequestPayment. That is the only line where the iOS and Android request-payment-without-contacts.xml differ.
  • Lightning requests use onchain tint while preparing (Bitkit/Views/Wallets/Send/SendConfirmationView.swift:89-97). Fixed: A request accepting only btc-lightning-bolt11 now gets .purpleAccent from preparingAccentColor (SendConfirmationView.swift:106-107) without reading selectedWalletToPayFrom. The summary icons and swipe use that colour (lines 354 and 395), and mixed bitcoin requests are neutral, so there is no orange flash. The new ([bitcoin], .purpleAccent) case passes at 8fbd4e6.

still open

  • Preparing review uses the default wallet's colour, not the paying rail (Bitkit/Views/Wallets/Send/SendConfirmationView.swift:88-97). While a payment request prepares, the review takes its colour from app.selectedWalletToPayFrom, which is still the default .onchain. resetSendState sets that default (AppViewModel.swift:1366), and handleScannedData only picks the real rail later in preparation (AppViewModel.swift:815). The review is visible throughout, with preparingRequest set and a loading swipe. From the code, I expect three visible mismatches: - A request that will be paid over Lightning shows an orange swipe and summary with the speed icon, then turns purple with the bolt. - A mixed USDT and bitcoin request that resolves to USDT shows orange before the USDT review. - A USDT-only request shows teal with the speed icon (SendConfirmationView.swift:353), not the $ icon the description promises. The grey this replaces was the deliberate "rail not known yet" state, which is what the test above is named for. I did not see this on a device, because I had no linked Paykit contact to send a request; it is from reading the code. The Android twin makes the same change in SendConfirmScreen.kt, so the two apps match. One option is to derive the colour and icon from acceptedPaymentEndpointIdentifiers when they fix the rail (Lightning only, onchain only, USDT only) and keep a neutral state for mixed requests. Manual test 1 should also look for an orange to purple flash. Lightning-only and mixed requests are fixed by preparingAccentColor (SendConfirmationView.swift:98-113). The third case is unchanged: line 368 picks bolt-hollow only for purple, so a preparing USDT-only request still shows the teal speed-normal icon, not currency-dollar.
  • Request Payment opens Add Contact for users who already have contacts (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:366-373). The footer now opens Add Contact whenever paymentRequests.eligibleTargets is empty, and that list is empty in states where adding a contact is not the answer. eligibleTargets is filled over the network at the end of a full Paykit refresh (AppScene.swift:1518-1519). It is [] at launch and after a reset (PaykitPaymentRequestService.swift:1740, 2283). It also stays [] while the user's own private payment publishing is off, or when they have no receivable endpoint (PaykitPaymentRequestService.swift:798-799). Before this PR the button was hidden in those states. Now two cases go wrong: - A user with linked contacts who taps before discovery finishes gets the Add Contact sheet. - A user with private sharing off always gets it, and adding a contact does not help them. openContacts also sets hasSeenContactsIntro. I did not reproduce this on a device; it is from reading the code. One option: route to Add Contact only when contactsManager.contacts is empty. Otherwise open the request recipient picker, which already shows wallet__payment_request_recipient_unavailable for an empty list (CreatePaymentRequestView.swift:190-198). The refresh on tap (PaymentRequestsView.swift:367-369) helps a user who taps before a successful discovery. It does not cover private sharing: with the Private payments toggle off, discoverEligibleTargets returns no targets (PaykitPaymentRequestService.swift:797-799), so a user with linked contacts still lands on Add Contact. A failed or operationInProgress refresh also leaves the list empty (lines 1621-1633), so Add Contact can still open while eligibility is unknown.

new since the last review

worth doing, does not block

  • Request Payment waits on a refresh with no loading state and no stale-tap guard (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:366-383)
  • Eligibility refresh can navigate after leaving Payments (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:367-370)

@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

QA on 034d204

Pushed 034d204, answering @coreyphillips's two new threads: Request Payment no longer waits on an eligibility refresh. It routes on the saved contacts already on the device: no saved contacts opens Add Contact; otherwise it opens the recipient picker, which updates as eligibility loads and says when no contact can receive a request. With no await before routing, a double tap or leaving the screen can't race a refresh any more.

Debug build, iOS 27 simulator:

  • J1 (request-payment-without-contacts.xml): with no contacts, Request Payment opens profile setup, the step before Add Contact.
  • PaykitPaymentRequestServiceTests passes (302 tests).

ios-j1b2x.mp4

Manual tests 1 and 2 (send-review colours) are unchanged since 8fbd4e6; their run is in #1179 (comment).

@ovitrif
ovitrif requested a review from coreyphillips October 30, 2026 22:34

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rechecked after 1 new commit (034d204). Nothing blocking remains.

addressed

  • Request Payment opens Add Contact for users who already have contacts (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:366-373). Fixed: PaymentRequestsView.swift:366 now sends the user to Add Contact only when contactsManager.contacts is empty. Users with contacts get the recipient picker (373-377). That picker reads eligibleTargets reactively and shows wallet__payment_request_recipient_unavailable for an empty list (CreatePaymentRequestView.swift:166-203). Undiscovered eligibility and private sharing off no longer lead to Add Contact.
  • Request Payment waits on a refresh with no loading state and no stale-tap guard (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:366-383). Fixed: 034d204 removes the refreshEligibleTargets await, so nothing is awaited before routing and there is no shared generation to supersede. The contacts branch calls showSheet synchronously (PaymentRequestsView.swift:374). openContacts captures origin before its own await and checks it afterwards (ContactsIntroView.swift:44-46), the same as the Contacts intro.
  • Eligibility refresh can navigate after leaving Payments (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:367-370). Fixed: The await that let the task outlive the Payments screen is gone. showSheet(.receive) now runs in the same turn as the tap. The Add Contact branch only navigates if navigation.path still equals the origin captured before hasExistingIdentityForNavigation (ContactsIntroView.swift:44-46).

still open

  • Preparing review uses the default wallet's colour, not the paying rail (Bitkit/Views/Wallets/Send/SendConfirmationView.swift:88-97). While a payment request prepares, the review takes its colour from app.selectedWalletToPayFrom, which is still the default .onchain. resetSendState sets that default (AppViewModel.swift:1366), and handleScannedData only picks the real rail later in preparation (AppViewModel.swift:815). The review is visible throughout, with preparingRequest set and a loading swipe. From the code, I expect three visible mismatches: - A request that will be paid over Lightning shows an orange swipe and summary with the speed icon, then turns purple with the bolt. - A mixed USDT and bitcoin request that resolves to USDT shows orange before the USDT review. - A USDT-only request shows teal with the speed icon (SendConfirmationView.swift:353), not the $ icon the description promises. The grey this replaces was the deliberate "rail not known yet" state, which is what the test above is named for. I did not see this on a device, because I had no linked Paykit contact to send a request; it is from reading the code. The Android twin makes the same change in SendConfirmScreen.kt, so the two apps match. One option is to derive the colour and icon from acceptedPaymentEndpointIdentifiers when they fix the rail (Lightning only, onchain only, USDT only) and keep a neutral state for mixed requests. Manual test 1 should also look for an orange to purple flash. The colours are fixed. preparingAccentColor (SendConfirmationView.swift:98-113) gives lightning-only purple, onchain-only orange, USDT-only teal and mixed neutral, and the updated test covers all five cases. The third mismatch is still there. Line 368 picks the icon with accentColor == .purpleAccent ? "bolt-hollow" : "speed-normal", so a USDT-only request shows the teal speed icon while it prepares. It then switches to currency-dollar once UsdtSendView takes over.

new since the last review

worth doing, does not block

  • Changelog fragment still says Add Contact opens when no contact can receive (changelog.d/next/1179.fixed.md:1)
  • Request Payment opens Add Contact while saved contacts are still loading (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:366)

Comment thread changelog.d/next/1179.fixed.md Resolved
@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

QA on cf045f9

Pushed cf045f9, answering @coreyphillips's two new threads:

  • Add Contact opens only when there is no signed-in profile, or once contacts have loaded and are empty; while contacts load or after a failed load, the recipient picker opens instead.
  • The changelog fragment now says Add Contact opens "when you have no contacts".

Debug build, iOS 27 simulator:

  • J1 (request-payment-without-contacts.xml): with no profile, Request Payment opens profile setup, the step before Add Contact.

ios-j1c2x.mp4

Manual tests 1 and 2 are unchanged since 8fbd4e6; their run is in #1179 (comment).

@ovitrif
ovitrif requested a review from coreyphillips October 30, 2026 22:48

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rechecked after 1 new commit (cf045f9). Nothing blocking remains.

addressed

  • Changelog fragment still says Add Contact opens when no contact can receive (changelog.d/next/1179.fixed.md:1). Fixed: changelog.d/next/1179.fixed.md:1 now reads "opening Add Contact when you have no contacts". That matches the routing at PaymentRequestsView.swift:366: Add Contact (or profile setup first) opens with no profile or with an empty loaded list, and saved contacts get the picker.
  • Request Payment opens Add Contact while saved contacts are still loading (Bitkit/Views/PaymentRequests/PaymentRequestsView.swift:366). Fixed: PaymentRequestsView.swift:366 now requires contactsManager.hasLoaded before routing an empty list to openContacts. In ContactsManager, hasLoaded is set only by a successful or missing-data load (lines 437, 462) and cleared by reset() (278). A failed load throws at 475 without setting it. So a signed-in user who taps before a successful load gets the picker, and hasSeenContactsIntro is not touched.

still open

  • Preparing review uses the default wallet's colour, not the paying rail (Bitkit/Views/Wallets/Send/SendConfirmationView.swift:88-97). While a payment request prepares, the review takes its colour from app.selectedWalletToPayFrom, which is still the default .onchain. resetSendState sets that default (AppViewModel.swift:1366), and handleScannedData only picks the real rail later in preparation (AppViewModel.swift:815). The review is visible throughout, with preparingRequest set and a loading swipe. From the code, I expect three visible mismatches: - A request that will be paid over Lightning shows an orange swipe and summary with the speed icon, then turns purple with the bolt. - A mixed USDT and bitcoin request that resolves to USDT shows orange before the USDT review. - A USDT-only request shows teal with the speed icon (SendConfirmationView.swift:353), not the $ icon the description promises. The grey this replaces was the deliberate "rail not known yet" state, which is what the test above is named for. I did not see this on a device, because I had no linked Paykit contact to send a request; it is from reading the code. The Android twin makes the same change in SendConfirmScreen.kt, so the two apps match. One option is to derive the colour and icon from acceptedPaymentEndpointIdentifiers when they fix the rail (Lightning only, onchain only, USDT only) and keep a neutral state for mixed requests. Manual test 1 should also look for an orange to purple flash. The colour part is fixed: preparingAccentColor (SendConfirmationView.swift:98-113) gives lightning-only purple, onchain-only orange, USDT-only teal and mixed neutral, and the test covers all of these. The third mismatch remains. Line 368 still picks accentColor == .purpleAccent ? "bolt-hollow" : "speed-normal", so a USDT-only request shows the speed icon in teal while it prepares, not currency-dollar.

@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

QA on 180987f

Pushed 180987f, the twin of @coreyphillips's two new Android points:

  • Opening the recipient picker from Request Payment now also starts an eligibility refresh, without waiting on it, so the list fills sooner after launch.
  • J1 gains the saved-contacts steps: add a contact with a private connection, then Request Payment opens the recipient picker and lists it.

The picker itself still has no loading state, as before this PR; that is tracked in #1180 (3.2.0).

Debug build, iOS 27 simulator:

  • J1 (request-payment-without-contacts.xml), partly run: on a wallet with no profile, Request Payment opens profile setup, the first step before Add Contact. The Add Contact sheet step and the saved-contacts steps were not run: they need a signed-in profile and a second identity with a private connection. Earlier QA comments ticked J1 after only these first steps; this is the accurate status.

ios-j1d2x.mp4

@ovitrif
ovitrif requested a review from coreyphillips October 30, 2026 22:57

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rechecked after 1 new commit (180987f). Nothing blocking remains.

still open

  • Preparing review uses the default wallet's colour, not the paying rail (Bitkit/Views/Wallets/Send/SendConfirmationView.swift:88-97). While a payment request prepares, the review takes its colour from app.selectedWalletToPayFrom, which is still the default .onchain. resetSendState sets that default (AppViewModel.swift:1366), and handleScannedData only picks the real rail later in preparation (AppViewModel.swift:815). The review is visible throughout, with preparingRequest set and a loading swipe. From the code, I expect three visible mismatches: - A request that will be paid over Lightning shows an orange swipe and summary with the speed icon, then turns purple with the bolt. - A mixed USDT and bitcoin request that resolves to USDT shows orange before the USDT review. - A USDT-only request shows teal with the speed icon (SendConfirmationView.swift:353), not the $ icon the description promises. The grey this replaces was the deliberate "rail not known yet" state, which is what the test above is named for. I did not see this on a device, because I had no linked Paykit contact to send a request; it is from reading the code. The Android twin makes the same change in SendConfirmScreen.kt, so the two apps match. One option is to derive the colour and icon from acceptedPaymentEndpointIdentifiers when they fix the rail (Lightning only, onchain only, USDT only) and keep a neutral state for mixed requests. Manual test 1 should also look for an orange to purple flash. Two of the three mismatches are fixed. preparingAccentColor (SendConfirmationView.swift:98-113) now gives Lightning-only requests purple with the bolt, onchain-only orange, and mixed requests the neutral grey. The test at PaykitPaymentRequestServiceTests.swift:15-38 covers those cases. The third mismatch remains. A USDT-only request is teal while it prepares, but line 368 picks the icon with accentColor == .purpleAccent ? "bolt-hollow" : "speed-normal", so it draws speed-normal. SendSheet.swift:1027-1034 shows this view while preparing, before the .usdt route hands off to UsdtSendView with currency-dollar, so the icon still swaps from speed to $. Choosing currency-dollar when the accent is .usdtAccent would close this. I could not run the test here: this machine has no simulator tooling (xcrun simctl is missing).

@ovitrif

ovitrif commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

QA on 180987f: J1 in full

Debug build, iOS 27 simulator, regtest dev build against the staging Pubky homeserver. Setup: a new Pubky profile created in the app on each device (Android "QaAndroid", iOS "QaIos"), then each added the other through the normal Add Contact flow, giving a private Paykit connection.

  • J1 (request-payment-without-contacts.xml), every step:
    • Signed in with no contacts: Request Payment on the empty Payments tab opens Contacts with the Add Contact sheet.
    • After adding QaAndroid as a contact from that sheet: Request Payment opens the recipient picker, which lists QaAndroid.
Signed in, no contacts → Add Contact Saved contact → recipient picker
ios-j1full2x.mp4
ios-j1saved2x.mp4

The no-profile branch passed earlier on this head; the send-review colour cases are unchanged since their last run.

@ovitrif

ovitrif commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

@coreyphillips Merging this PR on your approval of head 180987f, with a device run of its cases on that head, checks green and no open threads. The disabled Show Details pill's icon on a preparing USDT-only request is left for a follow-up.

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