Skip to content

Conversation

@pedrouid
Copy link

@pedrouid pedrouid commented Jan 22, 2026

Add NFC tap-to-pay functionality to the POS app, enabling customers to receive payment URLs by tapping their phone on the merchant device.

  • Android: HCE (Host Card Emulation) for NDEF tag emulation
  • iOS: VAS (Value Added Services) via Tap to Pay on iPhone

Includes NFC toggle in settings, status indicator on scan screen, and QR code fallback when NFC is unavailable.

@vercel
Copy link

vercel bot commented Jan 22, 2026

@pedrouid is attempting to deploy a commit to the Reown Team on Vercel.

A member of the Team first needs to authorize it.

@pedrouid pedrouid changed the title add tap-to-pay support for both android and ios via HCE and VAS respe… add tap-to-pay support for both android and ios via HCE and VAS Jan 22, 2026
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pedrouid can you remove this file?

@ignaciosantise
Copy link
Collaborator

@claude review

@claude
Copy link

claude bot commented Jan 22, 2026

Claude encountered an error —— View job

Command failed: git fetch origin --depth=20 pedrouid-nfc-research

I'll analyze this and get back to you.

@ignaciosantise
Copy link
Collaborator

ignaciosantise commented Jan 22, 2026

PR #345 Review Summary

Verdict: COMMENT

This PR adds NFC tap-to-pay functionality to the POS app using HCE (Android) and VAS (iOS). The implementation is well-structured with platform-specific abstractions and graceful fallbacks.


What's Good

  • Clean architecture: Separation of concerns with useNfcCapabilities (detection) and useNfcPayment (transmission) hooks
  • Graceful degradation: Falls back gracefully when native modules aren't available or NFC is unsupported
  • User control: Settings toggle allows users to enable/disable NFC, with proper validation
  • State management: Store migration is properly versioned (v6 → v7) with backwards compatibility
  • Clear UI feedback: NFC status indicator shows HCE/VAS mode when active
  • Good documentation: Comprehensive roadmap.md explaining the three-tier fallback strategy

Concerns

  1. No tests for new hooks - The new useNfcCapabilities and useNfcPayment hooks have no test coverage. These contain complex platform-specific logic and state management that would benefit from unit tests.

  2. Potential stale closure in onNfcReady callback (scan.tsx:48-56) - The nfcMode used inside onNfcReady may be stale when the callback fires:

    onNfcReady: () => {
      const modeLabel = nfcMode === "hce" ? "HCE" : nfcMode === "vas" ? "VAS" : "NFC";
      // nfcMode comes from hook destructuring, may be stale
    }

    Consider passing the mode through the callback parameter or using a ref.

  3. Effect cleanup race condition (use-nfc-payment.ts:639-641) - The cleanup function calls stopNfc() but this is also called conditionally in the main effect body. This could lead to double-stopping.

  4. Missing native module implementations - The hooks reference HceModule and VasModule from NativeModules, but the PR doesn't include the actual native Android/iOS implementations. Are these expected to exist already or be added in a follow-up PR?

  5. Hardcoded color (scan.tsx:95) - The NFC indicator uses a hardcoded green color #4CAF50 instead of using the theme system.


Suggestions (non-blocking)

  • Consider adding a brief loading state or animation when NFC is initializing
  • The showNfcToggle variable in settings.tsx is always true for iOS/Android - the check Platform.OS === "android" || Platform.OS === "ios" is redundant in a React Native context
  • Consider debouncing the updateUrl calls in use-nfc-payment.ts to avoid rapid re-broadcasts if URL changes quickly

Risk Assessment: LOW-MEDIUM

  • Low risk to existing functionality (NFC is additive, QR code remains primary)
  • Medium risk of runtime errors if native modules aren't properly linked
  • The lack of tests makes regression risk higher for future changes

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.

2 participants