feat(card): integrate card holder and secrets details#3729
feat(card): integrate card holder and secrets details#3729esaugomez31 wants to merge 10 commits intofeat--home-add-card-accountfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
esaugomez31
changed the title
|
@grimen @blink-claw-bot please Review |
blink-claw-bot
left a comment
blink-claw-bot
left a comment
There was a problem hiding this comment.
Code Review: feat(card): integrate card holder and secrets details
Overview
This PR integrates real card holder and encrypted secrets data into the card flow, replacing hardcoded/empty values. The changes are well-structured with comprehensive test coverage and follow good security practices for handling sensitive financial data.
✅ Security Assessment
AES-GCM Decryption Implementation
The new decryptAesGcm utility is correctly implemented:
- ✅ Proper key derivation from hex
- ✅ Correct IV handling from base64
- ✅ Authentication tag extraction (last 16 bytes)
- ✅ Secure algorithm choice (aes-128-gcm)
- ✅ Proper buffer handling and UTF-8 conversion
Card Secrets Security Flow
The useCardSecrets hook follows excellent security practices:
- ✅ RSA-OAEP session key encryption before transmission
- ✅ Client-side AES-GCM decryption (secrets never transmitted in plaintext)
- ✅ Biometric authentication gate on card details screen
- ✅ Error handling doesn't leak sensitive information
- ✅ No secrets stored in AsyncStorage or persistent state
- ✅ Secrets are ephemeral (stored only in React state)
Authentication & Access Control
- ✅ Biometric gate properly implemented for sensitive data access
- ✅ GraphQL queries properly parameterized (no injection risks)
- ✅ Session-based encryption prevents replay attacks
📝 Code Quality
Architecture & Design
- ✅ Clean separation: UI components → hooks → GraphQL → crypto utilities
- ✅ Proper abstraction:
useCardHoldercentralizes holder name resolution - ✅ Navigation cleanup: Replaced display data passing with API resolution
- ✅ Consistent patterns: Follows existing Apollo/GraphQL patterns
Error Handling
- ✅ Comprehensive error handling in
useCardSecrets - ✅ Toast notifications for user feedback
- ✅ Graceful fallbacks (empty strings when data unavailable)
- ✅ Loading states properly managed
Test Coverage
Outstanding test coverage across all new functionality:
- ✅
useCardHolderhook: 140 test lines covering all scenarios - ✅
useCardSecretshook: 174 test lines with comprehensive mocking - ✅ Crypto utilities: 69 additional test lines for
decryptAesGcm - ✅ Component tests updated for new props/behavior
- ✅ Screen integration tests updated
🔍 Detailed Review
New Hooks
useCardHolder (app/hooks/use-card-holder.ts):
- ✅ Proper GraphQL query implementation
- ✅ Cache-first fetch policy (good for performance)
- ✅ Conditional execution (skip when no cardId)
- ✅ Safe string concatenation for fullName
useCardSecrets (app/screens/card-screen/hooks/use-card-secrets.ts):
- ✅ Excellent async error handling
- ✅ Proper state management (loading/error/data)
- ✅ Secure key generation and encryption flow
- ✅ Clean separation of concerns
UI Components
BlinkCard updates:
- ✅ Smooth integration of
useCardHolderhook - ✅ Proper prop handling (cardId vs holderName precedence)
- ✅ Uppercase formatting for visual consistency
Card Details Screen:
- ✅ Proper integration of sensitive data display
- ✅ Copy-to-clipboard functionality with proper error handling
- ✅ Conditional rendering based on data availability
Navigation Changes
- ✅ Breaking change handled well: Replaced
holderNamewithcardIdin navigation params - ✅ All affected screens and tests updated consistently
- ✅ Better architecture: screens resolve data from API instead of passing through navigation
🚨 Minor Issues
Nit: Potential Memory Optimization
In useCardSecrets.ts, the secrets are stored in component state indefinitely. Consider:
// Could add a cleanup mechanism
useEffect(() => {
return () => {
// Clear secrets from memory when component unmounts
setSecrets(undefined)
}
}, [])Nit: Error Message Consistency
In useCardSecrets.ts line 73, consider using a consistent error message format:
const message = err instanceof Error ? err.message : "Unknown error occurred"Nit: GraphQL Generated Files
The app/graphql/generated.ts changes look correct (auto-generated), but ensure the schema changes are intentional and match backend implementation.
🎯 Recommendations
- Monitor Performance: With the new
useCardHolderhook being called in multiple places, monitor for N+1 query scenarios - Consider Caching: The holder name data is relatively static - consider longer cache TTL if appropriate
- Expiry Date: Good that the TODO for expiry date is documented - ensure backend implementation aligns with the UI expectations
🏁 Verdict
This is excellent work with:
- ✅ Proper security implementation for sensitive financial data
- ✅ Comprehensive test coverage
- ✅ Clean architecture and code organization
- ✅ Consistent error handling
- ✅ Proper navigation parameter cleanup
The security implementation is particularly noteworthy - the client-side decryption, session key encryption, and biometric authentication create a robust security model for handling card secrets.
Recommendation: APPROVE ✅
The minor nits above are suggestions for future consideration and don't block this PR.
🤖 Review by Claude Sonnet 4 via Blink-Claw-Bot
There was a problem hiding this comment.
Utility libraries unit tests should test actual values rather than mocked, looks like too much magic here?
There was a problem hiding this comment.
Done. Rewrote tests using real node:crypto as backend, no more mocked cipher/decipher. Tests now do actual encrypt then decrypt roundtrips verifying real values (simple strings, unicode, card numbers, CVCs, wrong key rejection, tampered data detection).
| } | ||
| }, [authenticated, card?.id, secrets, fetchSecrets]) | ||
|
|
||
| const copyAction = useCallback( |
There was a problem hiding this comment.
Seems very universal, so would make sense to have a dedicated hook useCopyToClipboard.
There was a problem hiding this comment.
useClipboard already exists as the universal clipboard hook. Instead of creating a duplicate hook, simplified copyAction to a plain handleCopy function that calls copyToClipboard directly. ActionField already handles the undefined case internally (disables action when value is null), so no wrapper needed.
3 participants
Summary
Integrates
cardHolderandcardSecretsEncryptedGraphQL queries into the card flow, replacing hardcoded/empty values with real data from the API.Changes
BlinkCardcomponent now resolves the holder name internally viauseCardHolderhook when receiving acardIdprop — eliminates duplicate hook calls across screensholderNamestring param withcardIdacross all navigation routes — screens no longer pass display data through navigation, they resolve it from the APIdecryptAesGcmutility to complement the existingencryptAesGcmScreens updated
cardIdinstead ofholderNameto status screenPending
Tests
All new hooks, utilities, and screen modifications are covered by tests.