Repository navigation
fix: apply design nits - #1179
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Low impact] The PR appears safe to merge; no actionable defects were found. SummaryUpdates the Tether illustration and adds “quickly” to the English USDT intro.
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
Reviews (1) · Last reviewed commit: "fix: update tether icon and usdt copy" · Reviewed by Greptile |
There was a problem hiding this comment.
💡 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".
talosmachina
left a comment
There was a problem hiding this comment.
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
ScrollViewcontent 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
left a comment
There was a problem hiding this comment.
Two independent reviews, nothing blocking a merge.
worth doing, does not block
usdt__intro_descriptionchanged 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 sharedUsdtCoinIllustration, whichUsdtWalletScreenalso 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,scaledToFillinto 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,scaledToFitat 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 inUsdtWalletScreen. On the network pickers the illustration is the last item in aScrollView, 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.
@coreyphillips The move is intended: the Wallet USDT frame draws the header coin with the same Since your review, this PR also carries Aldert's other design nits as separate commits (now titled
The description, Preview and QA Notes are updated for the new head 9725012. |
QA on 9725012Debug 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.
ios-preparing2x.mp4 |
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 4 new commits (57105f9, 1fa2bca, 66a2a97, 9725012). One point still blocks a merge.
addressed
usdt__intro_descriptionchanged in English only, against repo policy (Bitkit/Resources/Localization/en.lproj/Localizable.strings:1951). Fixed: All 14 non-English files now carry the adverb inusdt__intro_description(for example de.lproj:1817 "schnell", fr.lproj:1831 "rapidement", ru.lproj:1811 "быстро"). I grepped the key in all 15 locale files, andnode scripts/validate-translations.jsreports 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).
testPreparationUsesAcceptedMethodsBeforeWalletSelectionnow 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 toapp.selectedWalletToPayFrom. The test (BitkitTests/PaykitPaymentRequestServiceTests.swift:15-30) buildsSendConfirmationViewwith no environment. For that case it expects.textSecondary. Readingappoutside a view hierarchy is a fatal error. Even with an environment, no branch returns.textSecondaryany 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 beforeappis read, so this is new. The test needs the new expectation. It also needs an injectedAppViewModel, or the colour choice moved into a static helper that takes the wallet type, the wayconfirmationFeeRateis. 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)
QA on 375538fPushed e16a1a9 and 375538f, answering @ben-kaufman's thread and the preparing-colour point:
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:
|
@coreyphillips Fixed: since 375538f a preparing request returns its colour from its accepted methods before |
coreyphillips
left a comment
There was a problem hiding this comment.
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
preparingAccentColorbeforeappis read, and[usdt, bitcoin]maps to.textSecondaryagain. I rantestPreparationUsesAcceptedMethodsBeforeWalletSelectionat 8fbd4e6 on an iPhone 17e simulator: it passed, and so did all 302 tests inPaykitPaymentRequestServiceTests. - 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
PaymentRequestCreateto iOSPaymentRequestRequestPayment. That is the only line where the iOS and Androidrequest-payment-without-contacts.xmldiffer. - Lightning requests use onchain tint while preparing (Bitkit/Views/Wallets/Send/SendConfirmationView.swift:89-97). Fixed: A request accepting only
btc-lightning-bolt11now gets.purpleAccentfrompreparingAccentColor(SendConfirmationView.swift:106-107) without readingselectedWalletToPayFrom. 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.resetSendStatesets that default (AppViewModel.swift:1366), andhandleScannedDataonly picks the real rail later in preparation (AppViewModel.swift:815). The review is visible throughout, withpreparingRequestset 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 inSendConfirmScreen.kt, so the two apps match. One option is to derive the colour and icon fromacceptedPaymentEndpointIdentifierswhen 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 bypreparingAccentColor(SendConfirmationView.swift:98-113). The third case is unchanged: line 368 picksbolt-hollowonly for purple, so a preparing USDT-only request still shows the tealspeed-normalicon, notcurrency-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.eligibleTargetsis empty, and that list is empty in states where adding a contact is not the answer.eligibleTargetsis 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.openContactsalso setshasSeenContactsIntro. I did not reproduce this on a device; it is from reading the code. One option: route to Add Contact only whencontactsManager.contactsis empty. Otherwise open the request recipient picker, which already showswallet__payment_request_recipient_unavailablefor 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,discoverEligibleTargetsreturns no targets (PaykitPaymentRequestService.swift:797-799), so a user with linked contacts still lands on Add Contact. A failed oroperationInProgressrefresh 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)
QA on 034d204Pushed 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:
ios-j1b2x.mp4Manual tests 1 and 2 (send-review colours) are unchanged since 8fbd4e6; their run is in #1179 (comment). |
coreyphillips
left a comment
There was a problem hiding this comment.
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.contactsis empty. Users with contacts get the recipient picker (373-377). That picker readseligibleTargetsreactively and showswallet__payment_request_recipient_unavailablefor 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
refreshEligibleTargetsawait, so nothing is awaited before routing and there is no shared generation to supersede. The contacts branch callsshowSheetsynchronously (PaymentRequestsView.swift:374).openContactscapturesoriginbefore 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 ifnavigation.pathstill equals theorigincaptured beforehasExistingIdentityForNavigation(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.resetSendStatesets that default (AppViewModel.swift:1366), andhandleScannedDataonly picks the real rail later in preparation (AppViewModel.swift:815). The review is visible throughout, withpreparingRequestset 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 inSendConfirmScreen.kt, so the two apps match. One option is to derive the colour and icon fromacceptedPaymentEndpointIdentifierswhen 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 withaccentColor == .purpleAccent ? "bolt-hollow" : "speed-normal", so a USDT-only request shows the teal speed icon while it prepares. It then switches tocurrency-dollaronce 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)
QA on cf045f9Pushed cf045f9, answering @coreyphillips's two new threads:
Debug build, iOS 27 simulator:
ios-j1c2x.mp4Manual tests 1 and 2 are unchanged since 8fbd4e6; their run is in #1179 (comment). |
coreyphillips
left a comment
There was a problem hiding this comment.
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.hasLoadedbefore routing an empty list toopenContacts. In ContactsManager,hasLoadedis set only by a successful or missing-data load (lines 437, 462) and cleared byreset()(278). A failed load throws at 475 without setting it. So a signed-in user who taps before a successful load gets the picker, andhasSeenContactsIntrois 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.resetSendStatesets that default (AppViewModel.swift:1366), andhandleScannedDataonly picks the real rail later in preparation (AppViewModel.swift:815). The review is visible throughout, withpreparingRequestset 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 inSendConfirmScreen.kt, so the two apps match. One option is to derive the colour and icon fromacceptedPaymentEndpointIdentifierswhen 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 picksaccentColor == .purpleAccent ? "bolt-hollow" : "speed-normal", so a USDT-only request shows the speed icon in teal while it prepares, notcurrency-dollar.
QA on 180987fPushed 180987f, the twin of @coreyphillips's two new Android points:
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:
ios-j1d2x.mp4 |
coreyphillips
left a comment
There was a problem hiding this comment.
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.resetSendStatesets that default (AppViewModel.swift:1366), andhandleScannedDataonly picks the real rail later in preparation (AppViewModel.swift:815). The review is visible throughout, withpreparingRequestset 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 inSendConfirmScreen.kt, so the two apps match. One option is to derive the colour and icon fromacceptedPaymentEndpointIdentifierswhen 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 withaccentColor == .purpleAccent ? "bolt-hollow" : "speed-normal", so it drawsspeed-normal. SendSheet.swift:1027-1034 shows this view while preparing, before the.usdtroute hands off to UsdtSendView withcurrency-dollar, so the icon still swaps from speed to$. Choosingcurrency-dollarwhen the accent is.usdtAccentwould close this. I could not run the test here: this machine has no simulator tooling (xcrun simctlis missing).
QA on 180987f: J1 in fullDebug 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.
The no-profile branch passed earlier on this head; the send-review colour cases are unchanged since their last run. |
|
@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. |
Twin: synonymdev/bitkit-android#1741
Description
tether-coinimage 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.Visualcomponent 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.Visualcomponent, flipped.$icon.currency-dollaricon instead of the speed icon.Out of Scope
Design
Bitkit - Handoff v63:
Preview
Figma (left) and this PR on a simulator (right):
Recordings (2x):
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
request-payment-without-contacts.xml— Request Payment shows with no contacts and opens Add Contact; with contacts it opens the recipient pickerManual Tests
$icon — needs a USDT balance for a quoteAutomated Checks
PaykitPaymentRequestServiceTests.swift— a preparing request takes its colour from its accepted methods: USDT, lightning-only, onchain-only and mixedEvidence
node scripts/validate-translations.jspasses; SwiftFormat is clean;PaykitPaymentRequestServiceTestspasses on 8fbd4e6.Gaps
Models used
claude-opus-5-5(reasoning:Not exposed)claude-opus-5-5(reasoning:Not exposed)The session metadata does not expose the reasoning setting.