feat: Integrate NFC card reading and enhance icon animation - #486
feat: Integrate NFC card reading and enhance icon animation#486manideepk90 wants to merge 3 commits into
Conversation
| open Validation | ||
|
|
||
| // Import NFC EMV components | ||
| open NfcEmvButton |
There was a problem hiding this comment.
rename to NFCEMVButton, its cleaner
| "final-form": "^5.0.0", | ||
| "react-final-form": "^7.0.0", | ||
| "react-native-inappbrowser-reborn": "^3.7.0", | ||
| "react-native-nfc-emv": "link:../../../library/react-native-nfc-emv", |
| }, | ||
| dependencies: { | ||
| 'react-native-nfc-emv': { | ||
| root: path.join(__dirname, '..', '..', '..', 'library', 'react-native-nfc-emv'), |
There was a problem hiding this comment.
also if you don't need react native build system here + if this feature doesn't require any special entitlements/ permissions or takes too much space then it can also be shipped with the SDK with merchant config to toggle
| // Mask card number for display | ||
| let maskCardNumber = (pan: string): string => { | ||
| if pan->String.length < 8 { | ||
| pan |
| } else { | ||
| let first4 = pan->String.slice(~start=0, ~end=4) | ||
| let last4 = pan->String.slice(~start=-4, ~end=pan->String.length) | ||
| `${first4} **** **** ${last4}` |
There was a problem hiding this comment.
isn't it better to just have last 4, seems more standard + will cover other edge cases where its 15, 19 (Amex)
756fe71 to
80dbed5
Compare
|
@hyperswitch-client-core-review-bot please review this PR |
| } | ||
| } catch { | ||
| | ex => { | ||
| Console.log2("TapCardModule not available:", ex) |
There was a problem hiding this comment.
Console.log2 appears at lines 46 and 74. Only src/utility/logics/Window.res is allowlisted for console logging.
Remove these calls or route through the proper telemetry/logging infrastructure.
| if error.code !== "USER_CANCELLED" { | ||
| cleanup() | ||
| // Small delay before retry to avoid rapid retries | ||
| let _ = setTimeout(() => { |
There was a problem hiding this comment.
setTimeout at line 116 creates a retry timer that is never cleared. If the component unmounts during the 500ms delay, the callback will still execute and potentially call a stale function reference.
Store the timer ID and clear it in the cleanup effect, following the pattern in PaymentHook.res:42-53.
| "final-form": "^5.0.0", | ||
| "react-final-form": "^7.0.0", | ||
| "react-native-inappbrowser-reborn": "^3.7.0", | ||
| "react-native-nfc-emv": "link:../../../library/react-native-nfc-emv", |
There was a problem hiding this comment.
link:../../../library/react-native-nfc-emv will break builds for anyone without this exact directory structure.
Publish the package to a registry (npm/GitHub Packages) or use a git dependency with commit/tag pinning before merging.
| } | ||
|
|
||
| // Format expiry date from YYMMDD to MM/YY | ||
| let formatExpiryDate = (expiry: string): string => { |
There was a problem hiding this comment.
💡 formatExpiryDate (lines 242-254) and maskCardNumber (lines 257-265) are defined but never referenced anywhere in the codebase.
Remove dead code or integrate these utilities if they serve a purpose.
| } | ||
|
|
||
| // Store reference to startNfcListening for retry logic | ||
| React.useEffect1(() => { |
There was a problem hiding this comment.
💡 startListeningRef changes every render due to the recursive function reference, causing unnecessary effect re-runs.
Wrap startNfcListening in React.useCallback with stable dependencies, then store only the stable callback in the ref.
hs-agent
left a comment
hs-agent
left a comment
There was a problem hiding this comment.
Verdict: 🔄 Request Changes
The NFC card reading feature is well-structured overall, but there are several blocking issues that need addressing before merge:
- Console.log calls violate the internal_linter allowlist policy
- Missing setTimeout cleanup creates potential memory leaks and stale callback execution
- Local path dependency will break CI/builds for anyone without the exact filesystem layout
The dead code and effect stability issues should also be cleaned up for production readiness.
4 participants
Type of Change
What & Why
This pull request introduces NFC card reading support to the React Native app by integrating the
react-native-nfc-emvlibrary and implementing UI and logic for tap-to-read card functionality. The most significant changes include dependency and configuration updates, a new NFC module abstraction, a new UI button for NFC, and updates to the card entry flow to support NFC-based input.NFC Integration and Configuration:
react-native-nfc-emvlibrary as a dependency and configured it inreact-native.config.jsto enable native NFC EMV features in the app. [1] [2]NFC Module Abstraction:
NfcEmvModule.res, a ReScript module that wraps the native NFC EMV API, providing promise-based methods for checking availability, permissions, listening for card data, and handling errors, as well as utility functions for formatting and masking card data.UI Components and Card Entry Flow:
NfcEmvButton.res, a new component that manages NFC listening state, handles permission requests, displays a blinking NFC icon while listening, and manages success/error flows for reading card data via NFC.CardElement.resto integrate NFC card reading:These changes collectively enable users to tap their card on an NFC reader to autofill card details in the payment form, improving both usability and speed for supported devices.
Screenshots / Recordings
Affected Area & Impact
Android PR / status (if any):
iOS PR / status (if any):
Shared Codebase PR / status (if any):
Testing
Notes:
Checklist