Fix mobile demo app document registration #1182
Conversation
WalkthroughAdds registeredAt to DocumentMetadata, implements extractNameFromDocument, updates MRZ parsing for TD1/TD3 and name extraction, exports new utility, enhances demo app: new picker components, header rightAction, documents hook (sorting, clearAll), registration hook one-shot completion, documents list and register screen logic/UI updates, WebSocket adapter overhaul, demo dependencies and Podfile tweak. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Screen
participant SDK as mobile-sdk-alpha
participant Client as SelfClient
participant Store as DocumentStore
UI->>SDK: extractNameFromDocument(selfClient, documentId)
SDK->>Client: loadDocument(documentId)
Client->>Store: get document
Store-->>Client: document
Client-->>SDK: document
alt Aadhaar
SDK->>SDK: parse extractedFields.name
SDK-->>UI: { firstName, lastName }
else MRZ
SDK->>SDK: extractNameFromMRZ(document.mrz)
SDK-->>UI: { firstName, lastName } or null
else Other/invalid
SDK-->>UI: null
end
sequenceDiagram
autonumber
actor User
participant Hook as useDocuments
participant Catalog as CatalogProvider
participant Storage as DocumentStorage
User->>Hook: clearAllDocuments()
Hook->>Catalog: snapshot catalog
Hook->>Catalog: clear catalog (optimistic)
par delete files
Hook->>Storage: delete all document blobs
and delete meta
Hook->>Storage: remove metadata entries
end
alt deletion ok
Hook->>Hook: refresh()
Hook-->>User: done
else deletion fails
Hook->>Catalog: rollback to snapshot
Hook-->>User: error
end
sequenceDiagram
autonumber
actor App
participant Adapter as WsAdapter
participant WS as WebSocket
App->>Adapter: connect(url, {signal, headers})
alt WebSocket available
Adapter->>WS: new WebSocket(url)
Adapter->>Adapter: bind onmessage/onerror/onclose
Adapter-->>App: WsConn { send, close, onMessage... }
opt AbortSignal
App->>Adapter: signal.abort
Adapter->>WS: close()
end
else WebSocket missing
Adapter-->>App: throw Error("Provide a WebSocket implementation")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ument; adopt useDocuments/useRegistration and ScreenLayout
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/mobile-sdk-alpha/src/processing/mrz.ts (1)
65-71: Remove MRZ logging immediately.
console.log('validateTD1Format', lines);dumps raw MRZ lines, exposing passports/IDs in logs. That’s a direct PII leak and violates our “never log sensitive data” rule. Drop the log (or fully anonymize/redact before logging) before shipping.
🧹 Nitpick comments (2)
packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx (1)
57-75: Reduce type assertions and improve type safety.The heavy use of
anytype assertions in the event attachment logic weakens type safety and could mask runtime type errors. The addEventListener checks could also be simplified.Consider refactoring to:
- const attach = (event: 'message' | 'error' | 'close', handler: (payload?: any) => void) => { - if (typeof socket.addEventListener === 'function') { - if (event === 'message') { - (socket.addEventListener as any)('message', handler as any); - } else if (event === 'error') { - (socket.addEventListener as any)('error', handler as any); - } else { - (socket.addEventListener as any)('close', handler as any); - } - } else { - if (event === 'message') { - (socket as any).onmessage = handler; - } else if (event === 'error') { - (socket as any).onerror = handler; - } else { - (socket as any).onclose = handler; - } - } - }; + const attach = (event: 'message' | 'error' | 'close', handler: (e: any) => void) => { + if ('addEventListener' in socket && typeof socket.addEventListener === 'function') { + socket.addEventListener(event, handler); + } else { + // Fallback to onX properties + socket[`on${event}` as 'onmessage' | 'onerror' | 'onclose'] = handler; + } + };packages/mobile-sdk-alpha/src/documents/utils.ts (1)
59-73: Consider edge cases in name splitting logic.The space-based splitting for Aadhaar names handles 1-2 parts but may not handle:
- Names with multiple spaces (e.g., " FIRST LAST ")
- Single-word names correctly (currently returns empty lastName)
- Titles or honorifics (e.g., "Mr. FIRST LAST")
Improve robustness:
const name = document.extractedFields?.name; if (name && typeof name === 'string') { // Aadhaar name is typically "FIRSTNAME LASTNAME" format - const parts = name.trim().split(/\s+/); + const cleanName = name.trim().replace(/\s+/g, ' '); // normalize spaces + const parts = cleanName.split(' '); if (parts.length >= 2) { const firstName = parts[0]; const lastName = parts.slice(1).join(' '); return { firstName, lastName }; - } else if (parts.length === 1) { - return { firstName: parts[0], lastName: '' }; + } else if (parts.length === 1 && parts[0]) { + // For single-name case, treat as lastName (common in India) + return { firstName: '', lastName: parts[0] }; } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/mobile-sdk-demo/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
common/src/utils/types.ts(1 hunks)packages/mobile-sdk-alpha/src/documents/utils.ts(3 hunks)packages/mobile-sdk-alpha/src/index.ts(1 hunks)packages/mobile-sdk-alpha/src/processing/mrz.ts(2 hunks)packages/mobile-sdk-alpha/src/types/ui.ts(1 hunks)packages/mobile-sdk-demo/package.json(1 hunks)packages/mobile-sdk-demo/src/components/AlgorithmCountryFields.tsx(1 hunks)packages/mobile-sdk-demo/src/components/PickerField.tsx(1 hunks)packages/mobile-sdk-demo/src/components/ScreenLayout.tsx(1 hunks)packages/mobile-sdk-demo/src/components/StandardHeader.tsx(2 hunks)packages/mobile-sdk-demo/src/hooks/useDocuments.ts(2 hunks)packages/mobile-sdk-demo/src/hooks/useRegistration.ts(4 hunks)packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx(2 hunks)packages/mobile-sdk-demo/src/screens/DocumentsList.tsx(7 hunks)packages/mobile-sdk-demo/src/screens/GenerateMock.tsx(6 hunks)packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx(8 hunks)packages/mobile-sdk-demo/src/screens/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
common/src/utils/types.tspackages/mobile-sdk-alpha/src/types/ui.tspackages/mobile-sdk-demo/src/components/AlgorithmCountryFields.tsxpackages/mobile-sdk-demo/src/components/StandardHeader.tsxpackages/mobile-sdk-demo/src/screens/RegisterDocument.tsxpackages/mobile-sdk-demo/src/components/ScreenLayout.tsxpackages/mobile-sdk-alpha/src/processing/mrz.tspackages/mobile-sdk-demo/src/hooks/useRegistration.tspackages/mobile-sdk-demo/src/providers/SelfClientProvider.tsxpackages/mobile-sdk-demo/src/screens/DocumentsList.tsxpackages/mobile-sdk-demo/src/screens/GenerateMock.tsxpackages/mobile-sdk-alpha/src/documents/utils.tspackages/mobile-sdk-demo/src/components/PickerField.tsxpackages/mobile-sdk-demo/src/hooks/useDocuments.tspackages/mobile-sdk-demo/src/screens/index.tspackages/mobile-sdk-alpha/src/index.ts
common/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
common/src/**/*.{ts,tsx,js,jsx}: Review shared utilities for:
- Reusability and modular design
- Type safety and error handling
- Side-effect management
- Documentation and naming clarity
Files:
common/src/utils/types.ts
packages/mobile-sdk-alpha/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/mobile-sdk-alpha/AGENTS.md)
packages/mobile-sdk-alpha/**/*.{ts,tsx}: Use strict TypeScript type checking across the codebase
Follow ESLint TypeScript-specific rules
Avoid introducing circular dependencies
Files:
packages/mobile-sdk-alpha/src/types/ui.tspackages/mobile-sdk-alpha/src/processing/mrz.tspackages/mobile-sdk-alpha/src/documents/utils.tspackages/mobile-sdk-alpha/src/index.ts
packages/mobile-sdk-alpha/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
packages/mobile-sdk-alpha/**/*.{ts,tsx,js,jsx}: Review alpha mobile SDK code for:
- API consistency with core SDK
- Platform-neutral abstractions
- Performance considerations
- Clear experimental notes or TODOs
Files:
packages/mobile-sdk-alpha/src/types/ui.tspackages/mobile-sdk-alpha/src/processing/mrz.tspackages/mobile-sdk-alpha/src/documents/utils.tspackages/mobile-sdk-alpha/src/index.ts
packages/mobile-sdk-alpha/src/processing/**
📄 CodeRabbit inference engine (.cursor/rules/mobile-sdk-migration.mdc)
Place MRZ processing helpers in packages/mobile-sdk-alpha/src/processing/
Files:
packages/mobile-sdk-alpha/src/processing/mrz.ts
packages/mobile-sdk-alpha/src/index.ts
📄 CodeRabbit inference engine (.cursor/rules/mobile-sdk-migration.mdc)
Re-export new SDK modules via packages/mobile-sdk-alpha/src/index.ts
Files:
packages/mobile-sdk-alpha/src/index.ts
🧠 Learnings (3)
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Verify extractMRZInfo() using published sample MRZ strings (e.g., ICAO examples)
Applied to files:
packages/mobile-sdk-alpha/src/processing/mrz.tspackages/mobile-sdk-alpha/src/index.ts
📚 Learning: 2025-08-24T18:54:04.809Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursor/rules/mobile-sdk-migration.mdc:0-0
Timestamp: 2025-08-24T18:54:04.809Z
Learning: Applies to packages/mobile-sdk-alpha/src/processing/** : Place MRZ processing helpers in packages/mobile-sdk-alpha/src/processing/
Applied to files:
packages/mobile-sdk-alpha/src/processing/mrz.ts
📚 Learning: 2025-08-24T18:54:04.809Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursor/rules/mobile-sdk-migration.mdc:0-0
Timestamp: 2025-08-24T18:54:04.809Z
Learning: Applies to packages/mobile-sdk-alpha/src/index.ts : Re-export new SDK modules via packages/mobile-sdk-alpha/src/index.ts
Applied to files:
packages/mobile-sdk-alpha/src/index.ts
🧬 Code graph analysis (7)
packages/mobile-sdk-demo/src/components/AlgorithmCountryFields.tsx (1)
packages/mobile-sdk-demo/src/components/PickerField.tsx (1)
PickerField(12-43)
packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx (5)
app/src/providers/passportDataProvider.tsx (1)
setSelectedDocument(736-746)packages/mobile-sdk-alpha/src/documents/utils.ts (1)
extractNameFromDocument(48-85)packages/mobile-sdk-demo/src/components/ScreenLayout.tsx (1)
ScreenLayout(19-26)packages/mobile-sdk-demo/src/utils/document.ts (1)
humanizeDocumentType(7-16)packages/mobile-sdk-demo/src/components/LogsPanel.tsx (1)
LogsPanel(14-32)
packages/mobile-sdk-demo/src/components/ScreenLayout.tsx (2)
packages/mobile-sdk-demo/src/components/SafeAreaScrollView.tsx (1)
SafeAreaScrollView(13-27)packages/mobile-sdk-demo/src/components/StandardHeader.tsx (1)
StandardHeader(15-28)
packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx (2)
packages/mobile-sdk-alpha/src/types/public.ts (1)
WsConn(204-210)packages/mobile-sdk-demo/src/utils/secureStorage.ts (1)
getOrCreateSecret(180-185)
packages/mobile-sdk-demo/src/screens/DocumentsList.tsx (4)
packages/mobile-sdk-demo/src/hooks/useDocuments.ts (1)
useDocuments(17-95)packages/mobile-sdk-alpha/src/documents/utils.ts (1)
extractNameFromDocument(48-85)packages/mobile-sdk-demo/src/utils/document.ts (1)
humanizeDocumentType(7-16)packages/mobile-sdk-demo/src/components/ScreenLayout.tsx (1)
ScreenLayout(19-26)
packages/mobile-sdk-demo/src/screens/GenerateMock.tsx (1)
packages/mobile-sdk-demo/src/components/AlgorithmCountryFields.tsx (1)
AlgorithmCountryFields(13-46)
packages/mobile-sdk-alpha/src/documents/utils.ts (3)
packages/mobile-sdk-alpha/src/types/public.ts (1)
SelfClient(169-193)common/src/utils/types.ts (2)
isAadhaarDocument(165-169)isMRZDocument(171-177)packages/mobile-sdk-alpha/src/processing/mrz.ts (1)
extractNameFromMRZ(254-326)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-deps
- GitHub Check: android-build-test
- GitHub Check: e2e-ios
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
🔇 Additional comments (10)
packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx (1)
129-131: LGTM: 0x-prefix normalization is correct.The addition of 0x-prefix normalization ensures compatibility with components expecting hex strings. The implementation is defensive and correct.
packages/mobile-sdk-alpha/src/types/ui.ts (1)
22-22: LGTM: registeredAt field addition is well-typed.The optional
registeredAtfield with epoch ms semantics is correctly typed and documented. This aligns with the corresponding change incommon/src/utils/types.ts.packages/mobile-sdk-alpha/src/index.ts (1)
93-94: LGTM: Export follows SDK conventions.The new
extractNameFromDocumentexport is correctly placed with other document utilities and follows the established pattern for public API surface exposure.Based on learnings.
common/src/utils/types.ts (1)
46-46: LGTM: registeredAt field maintains cross-package type consistency.The addition of
registeredAt?: numberaligns with the parallel change inpackages/mobile-sdk-alpha/src/types/ui.ts, ensuring consistent type definitions across the codebase.packages/mobile-sdk-demo/src/screens/index.ts (1)
56-59: LGTM: refreshDocuments callback correctly wired.The mapping of
refreshDocumentstoonSuccessprovides clear semantics for post-registration refresh behavior and aligns with the broader refresh workflow improvements in this PR.packages/mobile-sdk-alpha/src/documents/utils.ts (2)
271-277: LGTM: Timestamp management is correct.The
registeredAttimestamp is correctly set toDate.now()on registration and cleared on unregistration. This provides accurate tracking of registration state changes.
48-85: Verify Aadhaar name parsing assumptions. SplittingextractedFields.nameon whitespace may not handle mononyms, multiple middle names, or compound surnames—add tests or expand parsing logic to cover these cases.packages/mobile-sdk-demo/src/hooks/useDocuments.ts (1)
30-40: LGTM: Registration-based sorting improves UX.Sorting registered documents first provides a better user experience by surfacing active/registered documents at the top of the list.
packages/mobile-sdk-demo/src/components/StandardHeader.tsx (2)
12-24: LGTM: rightAction prop correctly implemented.The new
rightActionprop is well-typed asReact.ReactNode, properly positioned in a flex layout, and maintains backward compatibility as an optional prop.
34-39: LGTM: topRow layout is semantically correct.The flexbox layout with space-between justification correctly positions the back button and right action at opposite ends while maintaining vertical alignment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx (1)
44-44: Document the unused headers parameter.The
headersparameter in the connect signature is accepted but never used. WebSocket constructor in browsers doesn't support custom headers in the standard API (they must be set server-side or via subprotocols).Either remove the unused parameter or document why it's present:
- connect: (url: string, opts?: { signal?: AbortSignal; headers?: Record<string, string> }): WsConn => { + connect: (url: string, opts?: { signal?: AbortSignal; headers?: Record<string, string> /* Reserved for future use */ }): WsConn => {Or if not needed in this environment, simplify:
- connect: (url: string, opts?: { signal?: AbortSignal; headers?: Record<string, string> }): WsConn => { + connect: (url: string, opts?: { signal?: AbortSignal }): WsConn => {
🧹 Nitpick comments (1)
packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx (1)
71-87: Consider type-safe event listener attachment.Multiple
as anytype assertions could be avoided with proper typing. While functional, this reduces type safety and IntelliSense support.Apply this diff to improve type safety:
- if (typeof socket.addEventListener === 'function') { - if (event === 'message') { - (socket.addEventListener as any)('message', handler as any); - } else if (event === 'error') { - (socket.addEventListener as any)('error', handler as any); - } else { - (socket.addEventListener as any)('close', handler as any); - } - } else { - if (event === 'message') { - (socket as any).onmessage = handler; - } else if (event === 'error') { - (socket as any).onerror = handler; - } else { - (socket as any).onclose = handler; - } - } + if (typeof socket.addEventListener === 'function') { + socket.addEventListener(event, handler as EventListener); + } else { + const onEvent = `on${event}` as keyof typeof socket; + (socket[onEvent] as any) = handler; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/mobile-sdk-demo/src/hooks/useDocuments.ts(3 hunks)packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx(2 hunks)packages/mobile-sdk-demo/src/screens/DocumentsList.tsx(7 hunks)packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
packages/mobile-sdk-demo/src/hooks/useDocuments.tspackages/mobile-sdk-demo/src/screens/DocumentsList.tsxpackages/mobile-sdk-demo/src/screens/RegisterDocument.tsxpackages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx
🧬 Code graph analysis (3)
packages/mobile-sdk-demo/src/screens/DocumentsList.tsx (4)
packages/mobile-sdk-demo/src/hooks/useDocuments.ts (1)
useDocuments(17-107)packages/mobile-sdk-alpha/src/documents/utils.ts (1)
extractNameFromDocument(48-85)packages/mobile-sdk-demo/src/utils/document.ts (1)
humanizeDocumentType(7-16)packages/mobile-sdk-demo/src/components/ScreenLayout.tsx (1)
ScreenLayout(19-26)
packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx (4)
app/src/providers/passportDataProvider.tsx (1)
setSelectedDocument(736-746)packages/mobile-sdk-alpha/src/documents/utils.ts (1)
extractNameFromDocument(48-85)packages/mobile-sdk-demo/src/utils/document.ts (1)
humanizeDocumentType(7-16)packages/mobile-sdk-demo/src/components/LogsPanel.tsx (1)
LogsPanel(14-32)
packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx (2)
packages/mobile-sdk-alpha/src/types/public.ts (1)
WsConn(204-210)packages/mobile-sdk-demo/src/utils/secureStorage.ts (1)
getOrCreateSecret(180-185)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-deps
- GitHub Check: e2e-ios
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
🔇 Additional comments (30)
packages/mobile-sdk-demo/src/providers/SelfClientProvider.tsx (2)
61-68: LGTM! AbortSignal cleanup properly implemented.The abort listener is now correctly removed when the socket closes, preventing the memory leak flagged in the previous review. The pattern wraps the close handler to ensure cleanup before invoking the original handler.
142-144: LGTM! Hex prefix normalization implemented correctly.The 0x-prefix normalization ensures consistency for components expecting standard hex format. Error handling remains secure without exposing the private key.
packages/mobile-sdk-demo/src/hooks/useDocuments.ts (5)
7-7: LGTM on the import addition.The DocumentCatalog type is correctly imported for use in the clearAllDocuments implementation.
23-23: LGTM on the clearing state.The new clearing state is properly initialized and used to track the clearAllDocuments operation.
30-40: LGTM on the document sorting.Sorting registered documents first provides a better UX by surfacing completed registrations at the top of the list.
69-104: LGTM on the atomic catalog clearing with rollback.The implementation now properly addresses the race condition concern from the previous review:
- Reads and persists the original catalog first
- Atomically clears the catalog by writing an empty one
- Attempts deletions with proper rollback on failure
- Handles errors and loading states correctly
This ensures consistency between catalog and storage even if interrupted.
106-106: LGTM on the return statement update.The hook's API is properly extended to include the new clearing state and clearAllDocuments action.
packages/mobile-sdk-demo/src/screens/DocumentsList.tsx (10)
5-5: LGTM on the import additions.The necessary imports for the new name extraction and client access functionality are correctly added.
Also applies to: 9-9
25-27: LGTM on the hook additions.The useSelfClient hook and new state/actions from useDocuments are properly integrated.
35-63: LGTM on the race condition fix.The name loading effect now properly implements the cancellation pattern suggested in the previous review:
- Uses a
cancelledflag to prevent stale state updates- Uses
Promise.allfor concurrent fetching (more efficient than sequential)- Clears documentNames when documents array is empty
- Returns a cleanup function that sets the cancellation flag
This eliminates the race condition where slower earlier runs could overwrite newer data.
65-80: LGTM on the clear all handler.The confirmation dialog and error handling are implemented correctly. The handler properly awaits clearAllDocuments and shows user-friendly error messages.
121-124: LGTM on the updated empty state text.The revised messaging is clearer and more helpful to users.
135-143: LGTM on the document name display.The document header now properly shows the extracted full name when available, providing better UX.
171-171: LGTM on the memoization dependency update.The documentNames dependency is correctly added to ensure the memoized content re-renders when names are loaded.
173-185: LGTM on the Clear All button.The button properly:
- Shows a loading indicator when clearing
- Disables appropriately when clearing or no documents exist
- Uses consistent styling with the rest of the UI
188-188: LGTM on the ScreenLayout integration.The Clear All button is properly passed as a rightAction to the header.
201-227: LGTM on the new styles.The styles for the clear button, document name display, and disabled state are well-defined and consistent with the existing design system.
Also applies to: 250-264
packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx (13)
6-6: LGTM on the import changes.Platform import is needed for platform-specific styling, and the rename from extractNameFromMRZ to extractNameFromDocument aligns with the broader document type support.
Also applies to: 9-9
39-42: LGTM on the auto-selection initialization.The selectedDocumentId now defaults to the newest unregistered document (via reverse()[0]) when no catalog selection exists, providing better UX.
47-47: LGTM on the documentNames state.The state is properly typed and initialized for storing extracted names.
65-79: LGTM on the auto-selection effect.The effect properly ensures that when no document is selected, it automatically selects the newest unregistered document, improving UX.
82-106: LGTM on the race condition fix.The name loading effect now properly implements the cancellation pattern suggested in the previous review:
- Uses a
cancelledflag to prevent stale state updates- Uses
Promise.allfor concurrent fetching (more efficient)- Filters to only load names for unregistered documents
- Returns a cleanup function that sets the cancellation flag
This eliminates the race condition where slower earlier runs could overwrite newer data.
131-152: LGTM on the one-shot completion handler.The new completion flow:
- Properly uses setOnComplete to register a single handler
- Refreshes the catalog before showing success
- Shows a user-friendly success alert
- Cleans up by resetting selection and actions
- Cleans up the handler on unmount
This replaces the previous timeout-based approach with a more reliable event-driven pattern.
170-171: LGTM on the picker selection helpers.The firstAvailableDocId and selectedIdForPicker helpers properly manage the picker's display state.
174-174: LGTM on the title update.Removing the [WiP] suffix indicates the feature is now production-ready.
176-208: LGTM on the picker integration.The picker:
- Conditionally renders only when documents are available
- Uses mode="dialog" for better mobile UX
- Properly integrates document names in the labels
- Shows a placeholder only when no documents are available
- Disables during registration
193-201: LGTM on the name integration in picker labels.The label construction properly shows the full name when available, falling back to just document type and ID when names aren't loaded yet or unavailable.
222-222: LGTM on the LogsPanel update.The props are correctly updated to use regState-based logs and showLogs.
226-226: LGTM on the conditional rendering updates.The document section and empty state now properly check availableDocuments.length to avoid showing UI when no documents exist.
Also applies to: 253-259
289-289: LGTM on the platform-specific overflow handling.The conditional overflow style prevents clipping issues on iOS while maintaining proper behavior on other platforms.
transphorm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/mobile-sdk-demo/src/screens/GenerateMock.tsx (1)
63-134: Review the forced 500ms minimum loading delay for UX implications.The artificial delay (lines 117-121) ensures a minimum loading display time, which can smooth perceived performance. However, this adds latency even when document generation is fast (<500ms).
Consider:
- Is the UX improvement worth the guaranteed delay for fast operations?
- Users on fast devices will experience unnecessary waiting
- This pattern can accumulate delays if multiple operations occur in sequence
Alternative approach: only add delay if the operation was very fast (e.g., <100ms), which might indicate a cached or synchronous result that could appear jarring:
- // Ensure minimum loading display time (500ms) for better UX - const elapsed = Date.now() - startTime; - if (elapsed < 500) { - await new Promise(resolve => setTimeout(resolve, 500 - elapsed)); - } + // Smooth UX for very fast operations + const elapsed = Date.now() - startTime; + if (elapsed < 100) { + await new Promise(resolve => setTimeout(resolve, 100 - elapsed)); + }packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx (2)
60-78: Consider consolidating the two auto-select effects.There are two separate effects handling document selection (lines 60-68 and 71-78), both with similar logic. The first sets selection when none exists, the second resets when the current selection becomes unavailable.
These could be merged into a single effect to reduce redundancy and improve maintainability:
- // Auto-select first available unregistered document (newest first) - useEffect(() => { - const availableDocuments = (catalog.documents || []).filter(doc => !doc.isRegistered).reverse(); - const firstUnregisteredDocId = availableDocuments[0]?.id; - - if (firstUnregisteredDocId && !selectedDocumentId) { - setSelectedDocumentId(firstUnregisteredDocId); - } - }, [catalog.documents, selectedDocumentId]); - - // Auto-select when catalog changes and current selection is no longer available - useEffect(() => { - const availableDocuments = (catalog.documents || []).filter(doc => !doc.isRegistered).reverse(); - const isCurrentSelectionAvailable = availableDocuments.some(doc => doc.id === selectedDocumentId); - - if (!isCurrentSelectionAvailable && availableDocuments.length > 0) { - setSelectedDocumentId(availableDocuments[0].id); - } - }, [catalog.documents, selectedDocumentId]); + // Auto-select first available unregistered document + useEffect(() => { + const availableDocuments = (catalog.documents || []).filter(doc => !doc.isRegistered).reverse(); + const firstUnregisteredDocId = availableDocuments[0]?.id; + const isCurrentSelectionAvailable = availableDocuments.some(doc => doc.id === selectedDocumentId); + + // Select first doc if: no selection exists OR current selection is unavailable + if (firstUnregisteredDocId && (!selectedDocumentId || !isCurrentSelectionAvailable)) { + setSelectedDocumentId(firstUnregisteredDocId); + } + }, [catalog.documents, selectedDocumentId]);
129-151: Verify one-shot completion handler cleanup on unmount.The effect sets
actions.setOnComplete(...)and cleans up withactions.setOnComplete(null)on unmount (line 150). However, if the Alert is shown and the component unmounts before the user presses OK, the callbacks in lines 140-145 will still execute against unmounted state.The
mounted.currentcheck on line 132 prevents the initial Alert, but the OK button callbacks (lines 141-144) could still run after unmount.Consider adding the mounted check inside the OK button handler as well:
{ text: 'OK', onPress: () => { - if (mounted.current) { + if (mounted.current) { setSelectedDocumentId(''); actions.reset(); } }, },Alternatively, verify that the OK handler won't fire if the Alert is already dismissed by unmount.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/mobile-sdk-demo/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/mobile-sdk-demo/ios/Podfile(1 hunks)packages/mobile-sdk-demo/package.json(1 hunks)packages/mobile-sdk-demo/src/components/PickerField.tsx(1 hunks)packages/mobile-sdk-demo/src/components/SimplePicker.tsx(1 hunks)packages/mobile-sdk-demo/src/screens/GenerateMock.tsx(6 hunks)packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/mobile-sdk-demo/src/components/PickerField.tsx
- packages/mobile-sdk-demo/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
packages/mobile-sdk-demo/src/components/SimplePicker.tsxpackages/mobile-sdk-demo/src/screens/RegisterDocument.tsxpackages/mobile-sdk-demo/src/screens/GenerateMock.tsx
🧬 Code graph analysis (3)
packages/mobile-sdk-demo/src/components/SimplePicker.tsx (2)
packages/mobile-sdk-demo/src/components/PickerField.tsx (1)
PickerItem(10-10)app/src/mocks/react-native-safe-area-context.js (1)
SafeAreaView(21-23)
packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx (4)
app/src/providers/passportDataProvider.tsx (1)
setSelectedDocument(736-746)packages/mobile-sdk-alpha/src/documents/utils.ts (1)
extractNameFromDocument(48-85)packages/mobile-sdk-demo/src/components/PickerField.tsx (1)
PickerField(12-31)packages/mobile-sdk-demo/src/utils/document.ts (1)
humanizeDocumentType(7-16)
packages/mobile-sdk-demo/src/screens/GenerateMock.tsx (2)
packages/mobile-sdk-demo/src/components/AlgorithmCountryFields.tsx (1)
AlgorithmCountryFields(13-46)packages/mobile-sdk-demo/src/components/PickerField.tsx (1)
PickerField(12-31)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-deps
- GitHub Check: e2e-ios
- GitHub Check: android-build-test
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
🔇 Additional comments (5)
packages/mobile-sdk-demo/ios/Podfile (1)
29-29: Verify Podfile commit and repository access.
- The GitHub API returned 404 for the fork/commit 9eff7c4e3a9037fdc1e03301584e0d5dcf14d76b; ensure the repo exists, is accessible, and the hash is correct.
- Use HTTPS instead of SSH in CI/CD environments:
pod "NFCPassportReader", git: "https://github.com/selfxyz/NFCPassportReader.git", commit: "9eff7c4e3a9037fdc1e03301584e0d5dcf14d76b"- Confirm NFC dependency updates belong in this PR versus a separate change.
packages/mobile-sdk-demo/src/components/SimplePicker.tsx (2)
18-35: LGTM! Clean picker implementation with proper disabled state handling.The component correctly gates modal opening with the
enabledprop and applies visual feedback viastyles.disabled. The pressable interaction and chevron icon provide clear affordance.
37-60: Modal dismissal on Android back button is handled correctly.The
onRequestCloseprop ensures the modal can be dismissed via Android's back button, which is a required behavior for Android modals. The modal animation and transparent background provide good UX.packages/mobile-sdk-demo/src/screens/GenerateMock.tsx (1)
124-128: Verify first-document navigation logic aligns with user expectations.The code auto-navigates to the register screen only when
catalog.documents.length === 1. This means:
- First document generated → auto-navigate to register
- Subsequent documents → show success alert, stay on generate screen
Ensure this behavior matches the intended user flow. Users might expect to always be taken to registration or might prefer to stay on the generation screen to create multiple documents.
If the intent is to navigate only when the catalog was previously empty, the condition is correct. If users should always have the option to register immediately, consider adding a confirmation dialog with "Register Now" / "Stay Here" options.
packages/mobile-sdk-demo/src/screens/RegisterDocument.tsx (1)
80-105: LGTM! Cancellation flag properly prevents stale name updates.The effect now includes a
cancelledflag that gates the state update (line 95), addressing the race condition identified in the previous review. The parallel Promise.all approach (lines 86-94) is more efficient than sequential fetching.Based on past review comments - the suggested fix has been correctly implemented.
Summary
Testing
https://chatgpt.com/codex/tasks/task_b_68ddbbe00fc0832dbe576278874aea78
Summary by CodeRabbit
New Features
Bug Fixes
Chores