feat: send flow #3539
feat: send flow #3539
Conversation
…ow-with-contact-search
…//github.com/tiankii/blink-mobile into feat--redesign-send-flow-with-contact-search
…ow-with-contact-search
…ow-with-contact-search
WalkthroughThis PR introduces a new PhoneInput React Native component with country picker, integrates phone number validation into the send bitcoin destination flow with new validation states, extends localization keys for phone-related UI elements, adds a scan icon to the destination screen navigation, updates GraphQL input types, and adds a grey6 color token to the theme. Changes
Sequence DiagramsequenceDiagram
actor User
participant DSScreen as Send Bitcoin<br/>Destination Screen
participant PhoneInput as PhoneInput<br/>Component
participant CountryPicker as Country<br/>Picker
participant Input as Phone<br/>Input
participant Validator as Phone<br/>Validator
participant Reducer as Redux<br/>Reducer
User->>DSScreen: Tap Phone Input
DSScreen->>PhoneInput: Render PhoneInput
PhoneInput->>PhoneInput: Detect device location<br/>(useDeviceLocation)
PhoneInput->>CountryPicker: Initialize with default country
User->>CountryPicker: Select Country
CountryPicker->>PhoneInput: handleCountrySelect
PhoneInput->>Input: Set focus & country
User->>Input: Enter phone number
Input->>PhoneInput: setPhoneNumber (onChange)
PhoneInput->>PhoneInput: Parse & format number<br/>(libphonenumber-js)
PhoneInput->>PhoneInput: Compute phoneInputInfo<br/>(useMemo)
PhoneInput->>DSScreen: onChangeInfo callback
DSScreen->>Validator: Validate phone number
alt Valid Phone Number
Validator->>Reducer: Dispatch SetValid
Reducer->>DSScreen: Update destinationState
DSScreen->>DSScreen: Filter & show contacts
else Invalid Phone
Validator->>Reducer: Dispatch SetPhoneInvalid
Reducer->>DSScreen: Update destinationState
DSScreen->>DSScreen: Show error message
else Phone Not Allowed
Validator->>Reducer: Dispatch SetPhoneNotAllowed
Reducer->>DSScreen: Update destinationState
DSScreen->>DSScreen: Show error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/screens/send-bitcoin-screen/destination-information.tsx (1)
50-117: Bug inLnurlError/LnurlUnsupportedhandling—only one case will ever matchIn the
Invalidswitch, this line:case InvalidDestinationReason.LnurlError || InvalidDestinationReason.LnurlUnsupported:evaluates the
||expression once at runtime, so the case effectively becomes justcase InvalidDestinationReason.LnurlError:;LnurlUnsupportedwill never hit this branch and will fall through to the default.Recommend splitting the cases explicitly:
- case InvalidDestinationReason.LnurlError || - InvalidDestinationReason.LnurlUnsupported: + case InvalidDestinationReason.LnurlError: + case InvalidDestinationReason.LnurlUnsupported: return { error: translate.SendBitcoinDestinationScreen.lnAddressError(), adviceTooltip: { text: translate.SendBitcoinDestinationScreen.lnAddressAdvice(), }, }This preserves the intended behavior for both reasons.
🧹 Nitpick comments (27)
app/rne-theme/colors.ts (1)
32-32: Newgrey6token looks consistent with existing paletteThe additional
grey6entries for light and dark themes are wired consistently with the rest of the greys and with the type declarations inthemed.d.ts. Just make sure any new usages are checked for contrast against backgrounds where accessibility matters.Also applies to: 81-81
app/navigation/stack-param-lists.ts (1)
45-45: Clarify semantics ofscanPressed?: numberThe new
scanPressed?: numberprop is type‑safe, but its meaning isn’t obvious (timestamp vs counter vs boolean flag). Consider either renaming (e.g.,scanPressedAtorscanEventId) or adding a short doc comment where it’s used so future readers know how to set and compare it.app/config/appinfo.ts (1)
16-16: ReorderedLNURL_DOMAINSis fine; consider addressing the FIXMEThe domain list contents are unchanged, so behavior should remain the same unless some code depends on array index order. Given the existing FIXME, it would be good (when convenient) to centralize this in
globals.lightningAddressDomainAliasesto avoid future drift between duplicated domain lists.app/screens/send-bitcoin-screen/payment-destination/lnurl.ts (1)
3-3: Phone-number detection is reasonable; consider explicit default country / intentUsing
libphonenumber-js/mobileplusisValid()to gate out phone‑like usernames from intra‑ledger resolution is a good way to keep LNURL phone flows and username flows separate.A couple of points to double‑check:
parsePhoneNumber()(default export) returns aPhoneNumberinstance orundefinedwhen it can’t parse the input, and the throwing variant isparsePhoneNumberWithError(), so thetry/catcharoundparsePhoneNumberis mainly defensive rather than required. (github.com)- Because you don’t pass a
defaultCountryor options, only numbers thatlibphonenumber-jscan recognize as full international numbers (typically with+and country code) will be treated as valid. If you expect local, digits‑only usernames (e.g.,"503…"without a+) to be recognized as phone numbers too, you may want to:
- Pass a
defaultCountry/ options, or- Adjust the helper to match the exact handle format you use for phone‑based lightning addresses.
If the product intent is “only treat E.164‑style handles as phone numbers and leave all other numeric locals as regular usernames”, then the current implementation is fine; otherwise, it’s worth aligning the helper with the desired handle format so some phone‑style usernames don’t slip through and get resolved as intra‑ledger.
Also applies to: 130-137, 148-148
app/i18n/raw-i18n/translations/es.json (5)
2188-2188: Unify tone: prefer “Introduce…” to match the rest of ES copyMany strings use informal “tú” (e.g., “Estás convirtiendo”, “Añade una nota”). Suggest reverting to “Introduce un destino válido” for consistency.
- "enterValidDestination": "Introduzca un destino válido", + "enterValidDestination": "Introduce un destino válido",
2212-2215: Minor fixes: keep tone consistent for phone validationKeep “Introduce …” to align with nearby copy. The other two lines look good.
- "invalidPhoneNumber": "Introduzca un número de teléfono válido", + "invalidPhoneNumber": "Introduce un número de teléfono válido",
2225-2225: Match existing validation phrasing (“Se requiere …”)Other validations use “Se requiere …” (e.g., “Se requiere cantidad”). Suggest aligning.
- "destinationRequired": "Destino requerido", + "destinationRequired": "Se requiere destino",
2228-2228: Placeholder capitalizationUse sentence case to match Spanish style (and other placeholders).
- "placeholder": "Factura o Dirección", + "placeholder": "Factura o dirección",
2252-2256: Fix untranslated and unclear strings in destination flow
- pendingDecryptionMessage is still in English — translate.
- orBySMS likely means “by phone number/SMS”; current “O Número De Teléfono” is odd-cased. Prefer “O por número de teléfono” (or “O por SMS” if the UI truly routes via SMS).
- orSaved is ambiguous; if it means saved contacts/recipients, make it explicit.
Please confirm the intent of “orBySMS” (phone number vs SMS) and “orSaved” (saved contacts vs saved recipients).
- "pendingDecryptionMessage": "Encrypted message. Waiting for payment confirmation.", + "pendingDecryptionMessage": "Mensaje cifrado. Esperando la confirmación del pago.", "destinationScreenTitle": "Enviar a", - "orBySMS": "O Número De Teléfono", - "orSaved": "O Guardado" + "orBySMS": "O por número de teléfono", + "orSaved": "O contactos guardados"If “orBySMS” truly refers to SMS, use:
- "orBySMS": "O Número De Teléfono", + "orBySMS": "O por SMS",app/i18n/raw-i18n/source/en.json (2)
2224-2227: Destination-required and placeholder changes—verify they match actual behaviorRenaming to
destinationRequiredand changing the placeholder to “Invoice or Address” make sense given the new separate phone and contacts flows, but they also implicitly suggest usernames are no longer valid here. Please double‑check that:
- All code paths now use
destinationRequiredinstead of any previous key.- The destination input truly only accepts invoices/addresses; if usernames are still valid, consider updating the placeholder to reflect that.
2251-2254: New send‑flow labels are reasonable, but might be tightened
destinationScreenTitle,orBySMS, andorSavedmap well to the new design. If you iterate on copy later, you might consider slightly more explicit variants like “Or mobile number” / “Or saved contact” for extra clarity, but this is non‑blocking.app/components/phone-input/phone-input.tsx (3)
66-68:countryCodeinitialization relies solely on device locationIf
useDeviceLocationfails or is slow,countryCodestaysundefined, andphoneInputInforemainsnull, meaningonChangeInfowill receivenulluntil something else sets the country. The UI still falls back toDEFAULT_COUNTRY_CODEfor the picker, so state and visuals can be out of sync.Not strictly broken, but consider defaulting
countryCodetoDEFAULT_COUNTRY_CODEon mount (or whendetectedCountryCodeis absent) so:
phoneInputInfois available immediately, and- state matches the flag shown in the picker.
97-108: Focus restoration via timeouts—consider more robust handlingUsing hard‑coded timeouts (100ms/300ms) to refocus the TextInput after country selection/close works but is brittle across devices and animations. If
react-native-country-picker-modalexposes callbacks or props that better signal when the modal is actually dismissed, prefer those over arbitrary delays to avoid occasional missed or premature focus.
120-135:phoneInputInfocontract—raw vs formatted values
formattedPhoneNumberusesAsYouTypewhilerawPhoneNumberis the unmodifiedvalue. This is sensible and gives callers both views; just ensure downstream consumers never assumerawPhoneNumberis digits‑only. If they need a normalized E.164 string, they should derive it fromparsePhoneNumberinstead of relying on either of these fields.app/screens/send-bitcoin-screen/destination-information.tsx (1)
150-169:adviceTooltipis now dead code—either re‑enable or removeThe
adviceTooltiprendering block is commented out, butdestinationStateToInformationstill computesadviceTooltipfor several paths. This leaves unused data and can confuse future maintainers about whether advice tooltips are intentionally hidden.Either:
- Re‑enable the block if the design still calls for advice bubbles, or
- Remove
adviceTooltipfrom the return type and strip the associated branches.This will keep the view and mapping logic consistent.
app/i18n/i18n-types.ts (1)
8880-8886: Consider reusing a generic “paste” key if one exists
SettingsScreen.pastemakes sense if this label is settings‑specific. If there is already a genericcommon.paste(or similar), consider wiring the UI to that instead to avoid duplicate translation keys.app/navigation/root-navigator.tsx (2)
6-7: Header scan action wiring looks correct; consider simplifying navigation usage and improving accessibility.The new
ScanIconheader button correctly callsnavigation.setParams({ scanPressed: Date.now() })with a type-safeStackNavigationProp<RootStackParamList, "sendBitcoinDestination">, and the dynamicdestinationScreenTitle()is aligned with the new flow.Two small improvements you might consider:
- Instead of using
useNavigationat the navigator level, use theoptions={({ navigation }) => ({ ... })}form forsendBitcoinDestination. That avoids relying on a top-level hook and ties the navigation instance directly to that screen’s options:- <RootNavigator.Screen - name="sendBitcoinDestination" - component={SendBitcoinDestinationScreen} - options={{ - title: LL.SendBitcoinScreen.destinationScreenTitle(), - headerRight: () => ( - <TouchableOpacity - onPress={() => navigation.setParams({ scanPressed: Date.now() })} - style={styles.SendBitcoinScreenScanIcon} - > - <ScanIcon fill={colors.black} /> - </TouchableOpacity> - ), - }} - /> + <RootNavigator.Screen + name="sendBitcoinDestination" + component={SendBitcoinDestinationScreen} + options={({ navigation }) => ({ + title: LL.SendBitcoinScreen.destinationScreenTitle(), + headerRight: () => ( + <TouchableOpacity + onPress={() => + navigation.setParams({ scanPressed: Date.now() }) + } + style={styles.SendBitcoinScreenScanIcon} + accessibilityRole="button" + accessibilityLabel={LL.HomeScreen.scan()} // or a dedicated string + > + <ScanIcon fill={colors.black} /> + </TouchableOpacity> + ), + })} + />
- Add
accessibilityLabel(and optionallytestID) on theTouchableOpacityso the scan action is reachable via screen readers and easier to target in tests.Also applies to: 58-63, 105-107, 117-118, 183-195
754-769: New scan icon style is fine; only minor naming nit if you care about consistency.
SendBitcoinScreenScanIcon: { marginRight: 20 }is functionally fine and gives the header-right icon breathing room. If you want to keep style keys consistent with others in this file (bottomNavigatorStyle,headerStyle, etc.), you could optionally rename it to something likesendBitcoinScreenScanIconand update usages, but this is purely cosmetic.app/screens/send-bitcoin-screen/send-bitcoin-reducer.ts (1)
10-12: Phone-specific destination states are a good addition; align state reset behavior to avoid stale destinations.Adding
PhoneInvalid/PhoneNotAllowedand the corresponding actions cleanly extends the destination state machine for phone flows.One nuance: in the new cases you do:
case SendBitcoinActions.SetPhoneInvalid: return { ...state, destinationState: DestinationState.PhoneInvalid, } case SendBitcoinActions.SetPhoneNotAllowed: return { ...state, destinationState: DestinationState.PhoneNotAllowed, }Whereas in
SetInvalid/SetRequiresUsernameConfirmation/SetConfirmedyou construct a fresh state object without spreadingstate, which implicitly clearsdestination,validDestination,invalidDestination, andconfirmationUsernameType.If
SetPhoneInvalid/SetPhoneNotAllowedcan be dispatched after a previously valid destination, this pattern will leave those old fields populated while the state reads as phone-invalid, which may be confusing or lead to subtle bugs if any logic keys offdestinationrather than onlydestinationState.To keep invariants simpler, consider clearing derived fields when entering these phone error states, e.g.:
case SendBitcoinActions.SetPhoneInvalid: - return { - ...state, - destinationState: DestinationState.PhoneInvalid, - } + return { + unparsedDestination: state.unparsedDestination, + destinationState: DestinationState.PhoneInvalid, + destination: undefined, + validDestination: undefined, + invalidDestination: undefined, + confirmationUsernameType: undefined, + } case SendBitcoinActions.SetPhoneNotAllowed: - return { - ...state, - destinationState: DestinationState.PhoneNotAllowed, - } + return { + unparsedDestination: state.unparsedDestination, + destinationState: DestinationState.PhoneNotAllowed, + destination: undefined, + validDestination: undefined, + invalidDestination: undefined, + confirmationUsernameType: undefined, + }This would mirror how other non-valid states are handled and make the state shape easier to reason about.
Also applies to: 30-40, 45-92, 156-165
app/i18n/en/index.ts (1)
2241-2255: Destination and phone-related copy updates look consistent with the new flow; ensure all call sites use the renamed key.
SendBitcoinDestinationScreen.enterValidDestination→"Enter a valid destination"matches the more generic UX.- New
invalidPhoneNumber/phoneNotAllowedmessages are clear and align with the newPhoneInvalid/PhoneNotAllowedstates in the reducer.- In
SendBitcoinScreen, the rename fromdestinationIsRequiredtodestinationRequiredplus the placeholder change to"Invoice or Address"fit the updated input semantics.- New keys
destinationScreenTitle: "Send to",orBySMS: "Or Mobile Number", andorSaved: "Or Saved"correctly support the redesigned destination screen and header.Just verify that all usages of the old
destinationIsRequiredkey have been updated (TypeScript/typesafe-i18n should catch any leftovers at compile time).Also applies to: 2277-2279, 2289-2293, 2323-2325
app/screens/send-bitcoin-screen/send-bitcoin-destination-screen.tsx (7)
115-123: Consider simplifying redundant phone validation logic.The
isValidPhoneNumbercheck on line 117 already validates the phone number. The subsequentparsePhoneNumberandisValid()call on lines 118-119 is redundant sinceisValidPhoneNumberinternally does the same validation.const isPhoneNumber = (handle: string): boolean => { try { - if (isValidPhoneNumber(handle)) return true - const parsed = parsePhoneNumber(handle) - return parsed?.isValid() ?? false + return isValidPhoneNumber(handle) } catch { return false } }
905-922: Review suppressed exhaustive-deps warning.The
eslint-disablecomment indicates missing dependencies. The effect readsactiveInputRef.currentanddestinationState.destinationStatebut they're not in the dependency array. While refs don't trigger re-runs, readingdestinationState.destinationStatewithout including it as a dependency could lead to stale closure issues where the effect doesn't re-run when the destination state changes.Consider adding
destinationState.destinationStateto the dependency array or restructuring the logic.
961-961: Remove unnecessary statickeyprop.The
key={1}prop on a single component outside of a list serves no purpose and could be confusing. React'skeyprop is only meaningful for reconciling lists of sibling elements.<PhoneInput - key={1} rightIcon={
991-991: Consider explicitundefinedinstead offalsefor conditional style.When the condition is false, this expression evaluates to
falsewhich gets passed as a style prop. While React Native handles this gracefully, usingundefinedis more explicit.- inputContainerStyle={activeInputRef.current === "phone" && inputContainerStyle} + inputContainerStyle={activeInputRef.current === "phone" ? inputContainerStyle : undefined}
254-259: SimplifyListEmptyContentlogic.Multiple branches now return empty fragments (
<></>), making the conditional structure unnecessarily complex. Consider simplifying since only the loading state produces meaningful content.- } else if (allContacts.length > 0) { - ListEmptyContent = <></> - } else { - ListEmptyContent = <></> - } + } else { + ListEmptyContent = <></> + }
810-829: Consider reducing prop count forPhoneInputSection.The component accepts 19 props, which suggests it might benefit from consolidation (e.g., grouping related state into an object or using a custom hook to encapsulate phone input logic). This would improve maintainability as the component evolves.
That said, for an internal component, the current structure is functional.
170-170: Using ref for UI-affecting state may cause stale renders.
activeInputRefis used to conditionally render UI elements (lines 663-665, 671-674, 695-706, 716-720, 798, etc.), but ref changes don't trigger re-renders. The current implementation works becauseonFocusedInputcallsresetInput(), which updates state and forces a re-render. However, this indirect coupling is fragile.Consider using
useStateforactiveInputinstead, which would make the render dependency explicit and prevent potential bugs if the reset logic changes.- const activeInputRef = useRef<TInputType>("search") + const [activeInput, setActiveInput] = useState<TInputType>("search")Then update
onFocusedInputand all usages accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/components/phone-input/index.ts(1 hunks)app/components/phone-input/phone-input.tsx(1 hunks)app/config/appinfo.ts(1 hunks)app/graphql/generated.ts(1 hunks)app/i18n/en/index.ts(5 hunks)app/i18n/i18n-types.ts(12 hunks)app/i18n/raw-i18n/source/en.json(5 hunks)app/i18n/raw-i18n/translations/es.json(5 hunks)app/navigation/root-navigator.tsx(6 hunks)app/navigation/stack-param-lists.ts(1 hunks)app/rne-theme/colors.ts(2 hunks)app/rne-theme/themed.d.ts(1 hunks)app/screens/send-bitcoin-screen/destination-information.tsx(2 hunks)app/screens/send-bitcoin-screen/payment-destination/lnurl.ts(3 hunks)app/screens/send-bitcoin-screen/send-bitcoin-destination-screen.tsx(22 hunks)app/screens/send-bitcoin-screen/send-bitcoin-reducer.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app/screens/send-bitcoin-screen/send-bitcoin-destination-screen.tsx (5)
app/graphql/generated.ts (3)
UserContact(2541-2559)Icon(817-869)Icon(871-871)app/components/phone-input/phone-input.tsx (2)
PhoneInputInfo(23-28)PhoneInput(45-194)app/screens/send-bitcoin-screen/send-bitcoin-reducer.ts (5)
SendBitcoinActions(30-40)SendBitcoinActions(42-43)DestinationState(3-12)DestinationState(14-14)SendBitcoinDestinationState(21-28)app/screens/send-bitcoin-screen/destination-information.tsx (1)
DestinationInformation(135-172)app/i18n/i18n-react.tsx (1)
useI18nContext(14-14)
app/screens/send-bitcoin-screen/destination-information.tsx (1)
app/screens/send-bitcoin-screen/send-bitcoin-reducer.ts (2)
DestinationState(3-12)DestinationState(14-14)
app/navigation/root-navigator.tsx (1)
app/navigation/stack-param-lists.ts (1)
RootStackParamList(17-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Check Code
- GitHub Check: Tests
- GitHub Check: Audit
🔇 Additional comments (26)
app/rne-theme/themed.d.ts (1)
31-32: Theme typings correctly exposegrey6Adding
grey6to theColorsinterface aligns the type surface with the new tokens incolors.ts. No issues here.app/components/phone-input/index.ts (1)
1-1: Barrel export forphone-inputis straightforwardRe-exporting from
"./phone-input"simplifies imports for consumers and matches common module layout patterns. Looks good.app/graphql/generated.ts (1)
454-469: New requiredisL2Verifiedfield inCardConsumerApplicationCreateInputThe new non-nullable
isL2Verified: Scalars["Boolean"]["input"]field is now required. Ensure that any client mutations usingCardConsumerApplicationCreateInputare updated to pass this flag. Verify this file was regenerated via codegen rather than edited manually.app/i18n/raw-i18n/translations/es.json (1)
2714-2716: LGTM: new common keys“profiles”: “Perfiles” and “paste”: “Pegar” are correct and consistent.
app/i18n/raw-i18n/source/en.json (3)
2187-2187: Generic fallback message for invalid destination looks fineThe updated
enterValidDestinationstring is clear and matches its use as a generic fallback when more specific reasons are not available. No issues from a UX or i18n standpoint.
2211-2213: Phone error strings are clear and specific
pastedClipboardSuccess,invalidPhoneNumber, andphoneNotAllowedread well and should surface unambiguously in the UI. Wording is concise and consistent with nearby messages.
2713-2715: New common stringsprofilesandpasteBoth labels are simple and consistent with existing naming. No further changes needed.
app/components/phone-input/phone-input.tsx (4)
139-170: CountryPicker configuration looks consistent with supported countriesPassing
countryCodes={allSupportedCountries}and rendering the flag + calling code based on the selected country aligns well with the new GraphQL‑driven country list and theming. Once the loading/null‑guard onallSupportedCountriesis fixed, this block should behave correctly.
171-191: Input wiring and test id look solidThe main
Inputis correctly wired withtestProps("telephoneNumber"), phone keyboard, and the customsetPhoneNumberhandler, and supports all expected callbacks (onFocus,onBlur,onSubmitEditing,rightIcon). Besides the disabled behavior noted earlier, this is ready to use.
110-118:isDisabledonly affects styles—field remains interactiveThe
isDisabledprop currently only lowers opacity viastyles.disabledInput, but both theCountryPickertrigger and theInputremain fully interactive. This can confuse users and automated tests expecting a non‑editable field.Consider wiring
isDisabledthrough to behavior:- const setPhoneNumber = (number: string) => { - if (!keepCountryCode) { + const setPhoneNumber = (number: string) => { + if (isDisabled) return + if (!keepCountryCode) { const parsedPhoneNumber = parsePhoneNumber(number, countryCode) @@ - onChangeText(number) + onChangeText(number) }And on the UI side:
- <CountryPicker + <CountryPicker + disabled={isDisabled} @@ - <Input + <Input {...testProps("telephoneNumber")} ref={phoneInputRef} @@ - onChangeText={setPhoneNumber} + onChangeText={setPhoneNumber} + editable={!isDisabled}(or equivalent, depending on the APIs you prefer). This keeps visuals and interactivity aligned.
70-78: The current code is safe and does not have a crash risk
data?.globals?.supportedCountries.map(...)safely short-circuits toundefinedwhensupportedCountriesis absent, and the|| []fallback handles this correctly. Optional chaining prevents the crash you described—there is no need to change this code.The dependency array could be more precise (
[data?.globals?.supportedCountries]instead of[data?.globals]) for performance optimization, but the current dependency is functionally safe.app/screens/send-bitcoin-screen/destination-information.tsx (1)
39-48: New phone‑related destination states are correctly surfacedThe
PhoneInvalidandPhoneNotAllowedbranches cleanly map to the new i18n keys and are checked before the genericInvalidstate, which avoids accidentally falling back to “enter a valid destination”. This integrates well with the new phone flow.app/i18n/i18n-types.ts (11)
6982-6986: Docstring forenterValidDestinationis fine and keeps types unchangedThe updated description text is clear and the
enterValidDestination: stringtype remains consistent with the rest of the i18n shape.
7088-7098: Phone-number validation error keys are well named and scoped
invalidPhoneNumberandphoneNotAllowedasstringfields fit nicely with the surrounding error messages and read cleanly for the send-flow validation states.
7133-7138:destinationRequiredkey adds the missing validation stateAdding
destinationRequired: stringalongsidedestinationcompletes the destination field’s validation surface for this screen.
7145-7150: Placeholder copy change to “Invoice or Address” looks correctOnly the doc comment is updated; the
placeholder: stringtype remains unchanged and aligned with usage.
7248-7260: New destination screen copy keys align with the redesigned send flow
destinationScreenTitle,orBySMS, andorSavedare clearly named and group all the “send to / or …” variants in one place for the destination screen.
16370-16374: Function wrapper forenterValidDestinationmatches the string keyThe comment and
enterValidDestination: () => LocalizedStringfunction signature are consistent with the string definition above.
16464-16474: Function types for phone validation messages mirror the string keys
invalidPhoneNumberandphoneNotAllowednow exist both as raw strings and as() => LocalizedStringfunctions, keeping the typesafe-i18n contract in sync.
16507-16512:destinationRequiredfunction type is consistent with the new string fieldThe
destinationRequired: () => LocalizedStringentry correctly mirrors the string key and follows the existing naming pattern.
16519-16524: Placeholder docstring update mirrors the string-side copy changeThe function
placeholder: () => LocalizedStringremains unchanged; only the description text is updated to “Invoice or Address”, staying consistent with the string definition.
16620-16634: New destination screen function keys line up with the string keys
destinationScreenTitle,orBySMS, andorSavedas() => LocalizedStringkeep parity with the string keys and give the send flow all necessary localized labels for the new UI sections.
18226-18232: Settingspastefunction mirrors the new string key
paste: () => LocalizedStringcorrectly pairs withSettingsScreen.pasteon the string side; no type or naming issues here.app/i18n/en/index.ts (1)
2817-2818: New commonpastelabel is fine and consistent.The
common.paste: "Paste"string is straightforward and matches existing capitalization of other common actions; no issues here.app/screens/send-bitcoin-screen/send-bitcoin-destination-screen.tsx (2)
493-512: Effect may re-run unintentionally whenparseValidPhonereference changes.The
parseValidPhonecallback depends ondefaultPhoneInputInfo, which changes during user interaction. When it changes, this effect will re-run. Although the early return onroute.params?.paymentshould prevent issues in most cases, ifroute.params?.paymentis truthy and hasn't changed, the effect logic will execute again unnecessarily.Consider memoizing the phone parsing logic differently or using a ref for the parser to avoid unnecessary effect triggers.
329-337: Verify if blocking numeric inputs in search mode is intended.The
isInt(rawInput)check will reject purely numeric strings like "12345" asPhoneNotAllowedeven when entered in the search input. If numeric usernames are valid in the system, this could incorrectly block legitimate destinations.
|
|
||
| const activeInputRef = useRef<TInputType>("search") | ||
|
|
||
| const [rawPhoneNumber, setRawPhoneNumber] = useState<string>("null") |
There was a problem hiding this comment.
Bug: State initialized with string literal "null" instead of empty string.
The rawPhoneNumber state is initialized with the string "null", which will likely cause display issues in the phone input and incorrect validation behavior.
- const [rawPhoneNumber, setRawPhoneNumber] = useState<string>("null")
+ const [rawPhoneNumber, setRawPhoneNumber] = useState<string>("")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [rawPhoneNumber, setRawPhoneNumber] = useState<string>("null") | |
| const [rawPhoneNumber, setRawPhoneNumber] = useState<string>("") |
🤖 Prompt for AI Agents
In app/screens/send-bitcoin-screen/send-bitcoin-destination-screen.tsx around
line 172, the rawPhoneNumber state is incorrectly initialized to the literal
string "null"; change its initial value to an empty string (use
useState<string>("")) so the phone input and validation treat it as empty rather
than the text "null", and verify any downstream checks expecting null/undefined
are updated to handle an empty string.
| setTimeout(() => { | ||
| setKeepCountryCode(true) | ||
| }, 100) |
There was a problem hiding this comment.
Timing hack with setTimeout may cause race conditions.
Using setTimeout with a 100ms delay to reset keepCountryCode is fragile and may not work reliably across all devices or under varying load conditions. This pattern appears again in handlePastePhone (lines 881-883).
Consider using a callback-based approach or leveraging React's state update batching to synchronize these state changes more reliably.
There was a problem hiding this comment.
The setTimeout with 100ms delay for keepCountryCode is intentionally benign and does not pose a race condition risk.
Using React's state batching or callbacks would add unnecessary complexity for what is essentially a "fire-and-forget" UI polish feature. The current implementation is simple, readable, and fit-for-purpose.
If this were controlling validation logic, API calls, or navigation—then yes, a more robust approach would be needed. But for a UI smoothing timeout, this pattern is appropriate.
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.