Wire WebView camera adapter and harden Android SDK launch flow#1805
Open
transphorm wants to merge 6 commits intodevfrom
Open
Wire WebView camera adapter and harden Android SDK launch flow#1805transphorm wants to merge 6 commits intodevfrom
transphorm wants to merge 6 commits intodevfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughAdds per-Activity binding and lifecycle-aware launcher management to the Android KMP SDK launch flow, standardizes lifecycle result codes, introduces a Compose SDK launch screen, replaces Bridge MRZ with camera.scanMRZ(params?), and adds a VerificationResult round-trip test and spec/status updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant MainActivity as MainActivity<br/>(ComponentActivity)
participant SelfSdk as SelfSdk
participant Launcher as ActivityResultLauncher
participant SelfVerActivity as SelfVerificationActivity
participant Callback as SelfSdkCallback
User->>MainActivity: onCreate()
MainActivity->>SelfSdk: bindActivity(this)
SelfSdk->>SelfSdk: currentActivity = MainActivity
User->>SelfSdk: launch(request, callback)
rect rgba(100,150,200,0.5)
Note over SelfSdk: Resolve activity and guard concurrent launches
SelfSdk->>SelfSdk: resolveActivity() → activity
SelfSdk->>SelfSdk: if pendingCallback → return VERIFICATION_IN_PROGRESS
SelfSdk->>SelfSdk: ensureLauncher(activity)
SelfSdk->>Launcher: registerForActivityResult()
SelfSdk->>SelfSdk: pendingCallback = callback
end
rect rgba(150,100,200,0.5)
Note over SelfSdk,SelfVerActivity: Launch verification UI
SelfSdk->>SelfVerActivity: startActivityForResult(intent with serialized config/request)
SelfVerActivity->>User: show verification UI
User->>SelfVerActivity: complete or cancel
SelfVerActivity->>Launcher: setResult(RESULT_CODE_*)
Launcher->>SelfSdk: onActivityResult()
end
rect rgba(100,200,150,0.5)
Note over SelfSdk,Callback: Result handling
SelfSdk->>SelfSdk: parse result / handle PARSE_ERROR, MISSING_RESULT, UNEXPECTED_RESULT
SelfSdk->>Callback: onSuccess/onError/onCancel
SelfSdk->>SelfSdk: pendingCallback = null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (3)
packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx (3)
60-63:⚠️ Potential issue | 🟠 MajorStop sending raw scan error text to analytics/UI.
Line 63 logs
err.messagedirectly; scanner/native errors can include sensitive document context. Emit stable error codes and generic user copy instead.🔒 Proposed fix
- } catch (err) { - const message = err instanceof Error ? err.message : 'MRZ scan failed'; - setError(message); - analytics.trackEvent('camera_mrz_scan_failed', { error: message }); + } catch (err) { + const userMessage = 'MRZ scan failed. Please try again.'; + setError(userMessage); + analytics.trackEvent('camera_mrz_scan_failed', { + errorCode: 'mrz_scan_failed', + });As per coding guidelines: "NEVER log sensitive data including PII..." and "ALWAYS redact/mask sensitive fields in logs..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx` around lines 60 - 63, The catch block in DocumentCameraScreen.tsx currently exposes raw err.message to setError and analytics.trackEvent; instead map errors to stable, non-sensitive error codes and a generic user-facing message: replace using err.message with a lookup (e.g., getMrzErrorCode(err) → returns codes like "MRZ_TIMEOUT", "MRZ_IMAGE_BLUR", "MRZ_UNKNOWN") and call setError with a generic copy ("MRZ scan failed, please try again") while sending only the stable code and non-sensitive metadata to analytics (e.g., analytics.trackEvent('camera_mrz_scan_failed', { error_code })) and keep the full err captured only in internal logs (internalLogger.error(err)) but never in UI/analytics.
37-71:⚠️ Potential issue | 🟠 MajorGuard pending MRZ scans from resolving after cancel/unmount.
The
useEffectat line 70 auto-starts scanning without cleanup. When user clicks cancel (line 101), the component navigates immediately (line 75), but the pendingcamera.scanMRZ()promise can still resolve afterward, triggering state updates (line 51 navigation, line 62 error) on the unmounted component. Since the camera bridge has no cancel/abort API, use a stale-scan guard:Stale-scan guard with generation and mounted tracking
const [scanning, setScanning] = useState(false); const [error, setError] = useState<string | null>(null); + const scanGenerationRef = React.useRef(0); + const mountedRef = React.useRef(true); const startMRZScan = useCallback(async () => { + const scanId = ++scanGenerationRef.current; setScanning(true); setError(null); analytics.trackEvent('camera_mrz_scan_started', { documentType, countryCode, }); try { const result = await camera.scanMRZ({ documentType, countryCode }); + if (!mountedRef.current || scanId !== scanGenerationRef.current) return; haptic.trigger('success'); analytics.trackEvent('camera_mrz_scan_success'); navigate('/onboarding/nfc', { state: { countryCode, documentType, passportNumber: result.documentNumber, dateOfBirth: result.dateOfBirth, dateOfExpiry: result.dateOfExpiry, }, }); } catch (err) { + if (!mountedRef.current || scanId !== scanGenerationRef.current) return; + const message = err instanceof Error ? err.message : 'MRZ scan failed'; setError(message); analytics.trackEvent('camera_mrz_scan_failed', { error: message }); } finally { - setScanning(false); + if (mountedRef.current && scanId === scanGenerationRef.current) { + setScanning(false); + } } }, [camera, navigate, analytics, haptic, documentType, countryCode]); useEffect(() => { + mountedRef.current = true; startMRZScan(); + return () => { + mountedRef.current = false; + scanGenerationRef.current += 1; + }; }, [startMRZScan]); const onCancel = useCallback(() => { + scanGenerationRef.current += 1; analytics.trackEvent('camera_screen_closed'); navigate('/'); }, [navigate, analytics]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx` around lines 37 - 71, The startMRZScan function can resolve after cancel/unmount causing state updates/navigation on an unmounted component; fix by adding a stale-scan guard: introduce a ref like scanGeneration (and optionally isMounted) incremented before calling camera.scanMRZ, capture the current generation in a local variable inside startMRZScan, and before calling haptic.trigger, analytics.trackEvent, navigate, setError or setScanning verify the captured generation still matches the current ref (and that isMounted is true in cleanup). Also mark isMounted = false in a cleanup returned from the useEffect so any pending promise resolution is ignored and avoid updating state or navigating when the scan is stale or component unmounted.
46-57:⚠️ Potential issue | 🟡 MinorAdd validation for required MRZ fields before NFC navigation.
The
camera.scanMRZreturn type declares non-nullable strings (documentNumber,dateOfBirth,dateOfExpiry), so this is not a nullability issue. However, defensive validation between the camera scan result and NFC navigation is reasonable given that the NFC screen already has empty string defaults—indicating the team anticipates incomplete data.Consider adding an early check:
const result = await camera.scanMRZ({ documentType, countryCode }); + if (!result.documentNumber?.trim() || !result.dateOfBirth?.trim() || !result.dateOfExpiry?.trim()) { + throw new Error('Incomplete MRZ data. Please rescan your document.'); + }This provides better UX by failing fast rather than progressing to NFC with empty fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx` around lines 46 - 57, In DocumentCameraScreen, add defensive validation after camera.scanMRZ returns to ensure result.documentNumber, result.dateOfBirth, and result.dateOfExpiry are non-empty before calling navigate('/onboarding/nfc'); if any are missing/empty, do not navigate—trigger a failure haptic (e.g., haptic.trigger('notificationFailed')), log/track the failure with analytics (e.g., analytics.trackEvent('camera_mrz_scan_incomplete')), and show a user-facing error/toast so the user can retry scanning; update the code paths around scanMRZ, haptic, analytics.trackEvent and navigate to enforce this early-return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt`:
- Around line 125-137: The launcher.launch(intent) call can throw synchronously
leaving pendingCallback set and blocking future verifications; wrap the launch
in a try-catch around the code that uses activityLauncher/launcher and if an
exception occurs clear pendingCallback and call callback.onFailure with a
SelfSdkError (use a code like "LAUNCH_FAILED" or the existing
"VERIFICATION_IN_PROGRESS" handling style) including the exception message so
the failure is surfaced; ensure the normal path still calls
launcher.launch(intent) and returns without clearing pendingCallback (only clear
it in the exception path) so verification state is consistent.
- Around line 160-162: The launcher is being registered with a non-deterministic
key using activity.hashCode(), which changes on Activity recreation and breaks
callback delivery; update the call that registers the launcher (the
activity.activityResultRegistry.register(...) invocation) to use a stable
deterministic key instead (for example a constant like "self-sdk-verification"
or a derived stable string such as
"${activity::class.java.name}-self-sdk-verification") so the registry preserves
the callback mapping across recreation and pendingCallback remains deliverable;
ensure every place that builds the key (and where pendingCallback is referenced)
uses the same stable identifier (e.g., define SELF_SDK_VERIFICATION_KEY and use
it in the register call and related logic).
In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/handlers/LifecycleBridgeHandler.kt`:
- Around line 74-90: Remove the business-logic branching in
LifecycleBridgeHandler that interprets type/success/errorCode/errorMessage;
instead always forward the raw lifecycle payload fields to the intent and let
TypeScript decide outcome. Concretely, stop using the if/else that sets
SelfVerificationActivity.RESULT_CODE_SUCCESS/RESULT_CODE_ERROR/RESULT_CODE_CANCELLED
based on type/success/errorCode, and instead always add the raw extras
(EXTRA_RESULT_TYPE if present, EXTRA_RESULT_DATA if present,
EXTRA_ERROR_CODE/EXTRA_ERROR_MESSAGE if present) to the intent and call
activity.setResult with a single neutral delivery code (e.g. keep
RESULT_CODE_SUCCESS as the transport result) so all success/error/cancel
semantics are resolved in the TypeScript layer.
In `@specs/person1-webview/SPEC.md`:
- Line 1340: The table row labeled "1E WebView App Shell" currently claims
camera and biometrics adapters are wired, but later sections still list those
items as out-of-scope; reconcile by updating the single source of truth: locate
the "1E WebView App Shell" entry and either change its status from "Done" to "In
Progress/Blocked" if camera/biometrics are not implemented, or remove/update the
later out-of-scope follow-ups that reference camera/biometrics so they reflect
completion; ensure both the table row and the later follow-up section describing
camera, biometrics, and camera adapters are consistent and reference the same
final status.
In `@specs/person2-native-shells/OVERVIEW.md`:
- Line 25: Update the OV ERVIEW.md and PR description to back the "KMP test app
validation on both platforms" checkbox with reproducible evidence: populate the
"Tested" and "How to QA" sections with the exact commands you ran (e.g., Android
assemble and iOS compile commands), terminal outputs or CI job URLs showing
success, and attach any device/emulator info; additionally document
platform-specific native module code paths touched in this change (briefly
reference the affected native modules/classes and their platform branches) so
reviewers can reproduce verification steps locally or via CI before marking the
item completed.
---
Outside diff comments:
In `@packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx`:
- Around line 60-63: The catch block in DocumentCameraScreen.tsx currently
exposes raw err.message to setError and analytics.trackEvent; instead map errors
to stable, non-sensitive error codes and a generic user-facing message: replace
using err.message with a lookup (e.g., getMrzErrorCode(err) → returns codes like
"MRZ_TIMEOUT", "MRZ_IMAGE_BLUR", "MRZ_UNKNOWN") and call setError with a generic
copy ("MRZ scan failed, please try again") while sending only the stable code
and non-sensitive metadata to analytics (e.g.,
analytics.trackEvent('camera_mrz_scan_failed', { error_code })) and keep the
full err captured only in internal logs (internalLogger.error(err)) but never in
UI/analytics.
- Around line 37-71: The startMRZScan function can resolve after cancel/unmount
causing state updates/navigation on an unmounted component; fix by adding a
stale-scan guard: introduce a ref like scanGeneration (and optionally isMounted)
incremented before calling camera.scanMRZ, capture the current generation in a
local variable inside startMRZScan, and before calling haptic.trigger,
analytics.trackEvent, navigate, setError or setScanning verify the captured
generation still matches the current ref (and that isMounted is true in
cleanup). Also mark isMounted = false in a cleanup returned from the useEffect
so any pending promise resolution is ignored and avoid updating state or
navigating when the scan is stale or component unmounted.
- Around line 46-57: In DocumentCameraScreen, add defensive validation after
camera.scanMRZ returns to ensure result.documentNumber, result.dateOfBirth, and
result.dateOfExpiry are non-empty before calling navigate('/onboarding/nfc'); if
any are missing/empty, do not navigate—trigger a failure haptic (e.g.,
haptic.trigger('notificationFailed')), log/track the failure with analytics
(e.g., analytics.trackEvent('camera_mrz_scan_incomplete')), and show a
user-facing error/toast so the user can retry scanning; update the code paths
around scanMRZ, haptic, analytics.trackEvent and navigate to enforce this
early-return behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.ktpackages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/handlers/LifecycleBridgeHandler.ktpackages/kmp-sdk/shared/src/commonTest/kotlin/xyz/self/sdk/models/ModelSerializationTest.ktpackages/kmp-test-app/composeApp/src/androidMain/kotlin/xyz/self/testapp/MainActivity.ktpackages/kmp-test-app/composeApp/src/commonMain/kotlin/xyz/self/testapp/App.ktpackages/kmp-test-app/composeApp/src/commonMain/kotlin/xyz/self/testapp/screens/SdkLaunchScreen.ktpackages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsxpackages/webview-bridge/src/adapters/camera.tspackages/webview-bridge/src/adapters/index.tsspecs/person1-webview/SPEC.mdspecs/person2-native-shells/OVERVIEW.mdspecs/person2-native-shells/SPEC.md
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt
Show resolved
Hide resolved
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt
Show resolved
Hide resolved
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/handlers/LifecycleBridgeHandler.kt
Show resolved
Hide resolved
transphorm
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt`:
- Around line 170-189: Move the ActivityResult registration out of the lazy
ensureLauncher() and perform it unconditionally inside bindActivity():
unregister any existing activityLauncher, set launcherOwner =
WeakReference(activity), and call activity.activityResultRegistry.register(...)
there to assign activityLauncher and its callback (preserving the existing
pendingCallback variable handling). Update launchInternal() to assume the
launcher is already registered and remove lazy registration logic in
ensureLauncher() (or call ensureLauncher() from bindActivity()); this guarantees
registration before STARTED and that recreated Activities receive results so
pendingCallback is not lost when the Activity is recreated.
In `@packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx`:
- Around line 43-44: The scanGenerationRef increment alone doesn't prevent
concurrent native scans; add an in-flight lock (e.g., isScanningRef or
scanLockRef) checked at the top of startMRZScan and set to true immediately
before calling camera.scanMRZ, then clear it in a finally block so duplicate
calls return early and the lock is always released on success/error; apply the
same pattern around other camera.scanMRZ usages (the blocks guarded by
scanGenerationRef) to ensure only one native scan runs at a time.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.ktpackages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsxspecs/person2-native-shells/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/person2-native-shells/SPEC.md
Contributor
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 (2)
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt (1)
98-264:⚠️ Potential issue | 🟠 MajorNative layer now contains business logic that should live in TypeScript per repo rule.
This segment implements verification-state policy, result classification, serialization/deserialization, and error contract mapping directly in Kotlin. That exceeds a thin native bridge and increases platform divergence risk.
As per coding guidelines "
{app,packages}/**/*.kt: Native handlers (Kotlin/Swift) should be thin wrappers with no business logic - all logic must live in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt` around lines 98 - 264, The Kotlin file implements business logic that must live in TypeScript; update the native bridge functions (launchInternal, ensureLauncher, handleActivityResult, serializeRequest, serializeConfig, deserializeResult) to be thin: stop classifying results or parsing JSON on Android, only pass raw payloads and error codes through the callback so TS can handle logic. Concretely, have serializeRequest/serializeConfig remain as simple passthrough JSON encoders if needed but remove deserializeResult usage in handleActivityResult and instead forward the raw EXTRA_RESULT_DATA (JSON string) and EXTRA_RESULT_TYPE or error extras directly to the pending SelfSdkCallback (e.g., onSuccess should receive rawResultJson and/or type, onFailure should receive the raw errorCode and errorMessage), remove any parsing/classification (PARSE_ERROR, MISSING_RESULT, UNEXPECTED_RESULT mapping) and ensure launchInternal only handles lifecycle/errors around launching the activity without embedding business rules.packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx (1)
52-55:⚠️ Potential issue | 🟠 MajorMake telemetry/haptics best-effort so they cannot block scan/cancel flow.
Line 52 runs
analytics.trackEvent(...)before thetry/finally; if it throws,scanInFlightRefis never released. Also, Lines 70-72 and Line 112 keep non-critical side effects in the critical control path, so exceptions can flip success into failure or block cancel navigation.🔧 Proposed fix
+ const safeTrackEvent = useCallback( + (event: string, props?: Record<string, unknown>) => { + try { + analytics.trackEvent(event, props); + } catch {} + }, + [analytics], + ); + + const safeSuccessHaptic = useCallback(() => { + try { + haptic.trigger('success'); + } catch {} + }, [haptic]); + const startMRZScan = useCallback(async () => { if (scanInFlightRef.current) return; scanInFlightRef.current = true; @@ - analytics.trackEvent('camera_mrz_scan_started', { + safeTrackEvent('camera_mrz_scan_started', { documentType, countryCode, }); @@ - haptic.trigger('success'); - analytics.trackEvent('camera_mrz_scan_success'); + safeSuccessHaptic(); + safeTrackEvent('camera_mrz_scan_success'); @@ - analytics.trackEvent('camera_mrz_scan_failed', { errorCode }); + safeTrackEvent('camera_mrz_scan_failed', { errorCode }); @@ - }, [camera, navigate, analytics, haptic, documentType, countryCode]); + }, [camera, navigate, safeTrackEvent, safeSuccessHaptic, documentType, countryCode]); const onCancel = useCallback(() => { scanGenerationRef.current += 1; - analytics.trackEvent('camera_screen_closed'); + safeTrackEvent('camera_screen_closed'); navigate('/'); - }, [navigate, analytics]); + }, [navigate, safeTrackEvent]);As per coding guidelines, "Always use try-catch for async operations with graceful degradation when native modules fail and comprehensive error boundaries."
Also applies to: 70-72, 92-92, 110-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx` around lines 52 - 55, The telemetry and haptics calls (analytics.trackEvent and the haptics/vibrate calls) are running in the critical scan/cancel control path and can throw, preventing scanInFlightRef from being released; make these operations best-effort by wrapping each call in their own try/catch or invoking them without awaiting and attaching .catch to swallow/log errors, ensure scanInFlightRef is set/cleared inside the existing try/finally around the scanning flow (leave the finally to always release scanInFlightRef), and move non-critical side effects out of the critical success/failure branches so failures in analytics or haptics cannot flip scan success or block navigation (refer to analytics.trackEvent, scanInFlightRef, and the haptics/vibrate calls to locate spots to wrap or decouple).
♻️ Duplicate comments (1)
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt (1)
135-153:⚠️ Potential issue | 🟠 MajorHandle all synchronous launch failures to avoid stuck in-flight state.
pendingCallbackis cleared only for two exception types. Iflauncher.launch(intent)throws any other synchronous exception, future launches can stay blocked byVERIFICATION_IN_PROGRESS.🔧 Proposed fix
- } catch (e: IllegalStateException) { + } catch (e: IllegalStateException) { pendingCallback = null callback.onFailure( SelfSdkError( code = "LAUNCH_FAILED", message = "Could not launch verification activity: ${e.message}", ), ) + } catch (t: Throwable) { + pendingCallback = null + callback.onFailure( + SelfSdkError( + code = "LAUNCH_FAILED", + message = "Could not launch verification activity: ${t.message ?: "Unknown launch error"}", + ), + ) }For androidx.activity.result.ActivityResultLauncher.launch(Intent), which exception types can be thrown synchronously, and is catching only ActivityNotFoundException + IllegalStateException considered sufficient for robust state management?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt` around lines 135 - 153, The try/catch only handles ActivityNotFoundException and IllegalStateException so other synchronous exceptions from launcher.launch(intent) can leave pendingCallback set and block VERIFICATION_IN_PROGRESS; update the block around launcher.launch(intent) to catch a general exception (e.g., Exception or Throwable) as a final fallback, ensure pendingCallback is set to null in that fallback, and call callback.onFailure with a SelfSdkError (use a generic code like "LAUNCH_FAILED" or "UNKNOWN_LAUNCH_ERROR" and include e.message) so any unexpected synchronous launch failure always clears state and reports an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt`:
- Around line 169-188: The current ensureLauncher implementation registers
activityLauncher via activity.activityResultRegistry.register(...) without a
LifecycleOwner, causing the launcher (and captured pendingCallback/Activity) to
leak if the Activity is destroyed; update ensureLauncher to perform
lifecycle-aware registration by either calling the register overload that takes
a LifecycleOwner (use the passed activity as the LifecycleOwner) or attach a
LifecycleObserver to launcherOwner/activity that calls
activityLauncher?.unregister() and clears launcherOwner/pendingCallback in
onDestroy; reference ensureLauncher, activityLauncher, launcherOwner,
pendingCallback, and the ActivityResultRegistry.register call when making the
change.
In `@packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx`:
- Around line 87-90: The MRZ error classification only checks err.message, so
update the errorCode assignment (the errorCode variable in
DocumentCameraScreen.tsx) to first check err.code === MRZ_INVALID_DATA_ERROR
and/or err.code === 'MRZ_INVALID_DATA' before falling back to testing
err.message === MRZ_INVALID_DATA_ERROR; ensure the logic covers both
native-provided err.code and JS Error.message so native bridge error codes are
correctly mapped to 'MRZ_INVALID_DATA' otherwise default to 'MRZ_SCAN_FAILED'.
---
Outside diff comments:
In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt`:
- Around line 98-264: The Kotlin file implements business logic that must live
in TypeScript; update the native bridge functions (launchInternal,
ensureLauncher, handleActivityResult, serializeRequest, serializeConfig,
deserializeResult) to be thin: stop classifying results or parsing JSON on
Android, only pass raw payloads and error codes through the callback so TS can
handle logic. Concretely, have serializeRequest/serializeConfig remain as simple
passthrough JSON encoders if needed but remove deserializeResult usage in
handleActivityResult and instead forward the raw EXTRA_RESULT_DATA (JSON string)
and EXTRA_RESULT_TYPE or error extras directly to the pending SelfSdkCallback
(e.g., onSuccess should receive rawResultJson and/or type, onFailure should
receive the raw errorCode and errorMessage), remove any parsing/classification
(PARSE_ERROR, MISSING_RESULT, UNEXPECTED_RESULT mapping) and ensure
launchInternal only handles lifecycle/errors around launching the activity
without embedding business rules.
In `@packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx`:
- Around line 52-55: The telemetry and haptics calls (analytics.trackEvent and
the haptics/vibrate calls) are running in the critical scan/cancel control path
and can throw, preventing scanInFlightRef from being released; make these
operations best-effort by wrapping each call in their own try/catch or invoking
them without awaiting and attaching .catch to swallow/log errors, ensure
scanInFlightRef is set/cleared inside the existing try/finally around the
scanning flow (leave the finally to always release scanInFlightRef), and move
non-critical side effects out of the critical success/failure branches so
failures in analytics or haptics cannot flip scan success or block navigation
(refer to analytics.trackEvent, scanInFlightRef, and the haptics/vibrate calls
to locate spots to wrap or decouple).
---
Duplicate comments:
In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt`:
- Around line 135-153: The try/catch only handles ActivityNotFoundException and
IllegalStateException so other synchronous exceptions from
launcher.launch(intent) can leave pendingCallback set and block
VERIFICATION_IN_PROGRESS; update the block around launcher.launch(intent) to
catch a general exception (e.g., Exception or Throwable) as a final fallback,
ensure pendingCallback is set to null in that fallback, and call
callback.onFailure with a SelfSdkError (use a generic code like "LAUNCH_FAILED"
or "UNKNOWN_LAUNCH_ERROR" and include e.message) so any unexpected synchronous
launch failure always clears state and reports an error.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.ktpackages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt`:
- Around line 158-176: bindActivity currently adds a new
DefaultLifecycleObserver on every call causing duplicate observers; to fix,
track the registered observer (e.g., add a private lifecycleObserver field) and
only call activity.lifecycle.addObserver when either lifecycleObserver is null
or is associated with a different activity, or unregister the previous observer
before adding a new one; ensure the observer you register clears
lifecycleObserver when onDestroy runs and still performs the existing cleanup
for activityLauncher, launcherOwner, boundActivity and pendingCallback so
duplicate onDestroy executions are prevented.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5df74bc-c426-49b1-9618-9128735a2769
📒 Files selected for processing (4)
packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.ktpackages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsxspecs/person1-webview/SPEC.mdspecs/person2-native-shells/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/webview-app/src/screens/onboarding/DocumentCameraScreen.tsx
- specs/person2-native-shells/SPEC.md
- specs/person1-webview/SPEC.md
Comment on lines
+158
to
+176
| private fun bindActivity(activity: ComponentActivity) { | ||
| boundActivity = WeakReference(activity) | ||
| ensureLauncher(activity) | ||
| activity.lifecycle.addObserver( | ||
| object : DefaultLifecycleObserver { | ||
| override fun onDestroy(owner: LifecycleOwner) { | ||
| if (launcherOwner?.get() === activity) { | ||
| activityLauncher?.unregister() | ||
| activityLauncher = null | ||
| launcherOwner = null | ||
| } | ||
| if (boundActivity?.get() === activity) { | ||
| boundActivity = null | ||
| } | ||
| pendingCallback = null | ||
| } | ||
| }, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate lifecycle observers accumulate on repeated bindActivity calls.
Each call to bindActivity adds a new DefaultLifecycleObserver unconditionally. If launch(activity, ...) is called multiple times with the same Activity, observers pile up. This leads to multiple onDestroy executions—potentially causing redundant unregister() calls and premature pendingCallback = null assignments.
Consider guarding observer registration to only add once per Activity instance.
🔧 Proposed fix
actual class SelfSdk private constructor(
private val config: SelfSdkConfig,
) {
private var activityLauncher: ActivityResultLauncher<Intent>? = null
private var launcherOwner: WeakReference<ComponentActivity>? = null
private var boundActivity: WeakReference<ComponentActivity>? = null
private var pendingCallback: SelfSdkCallback? = null
+ private var observedActivity: WeakReference<ComponentActivity>? = null
...
private fun bindActivity(activity: ComponentActivity) {
boundActivity = WeakReference(activity)
ensureLauncher(activity)
+ // Only add observer once per activity instance
+ if (observedActivity?.get() === activity) {
+ return
+ }
+ observedActivity = WeakReference(activity)
activity.lifecycle.addObserver(
object : DefaultLifecycleObserver {
override fun onDestroy(owner: LifecycleOwner) {
if (launcherOwner?.get() === activity) {
activityLauncher?.unregister()
activityLauncher = null
launcherOwner = null
}
if (boundActivity?.get() === activity) {
boundActivity = null
}
+ if (observedActivity?.get() === activity) {
+ observedActivity = null
+ }
pendingCallback = null
}
},
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/kmp-sdk/shared/src/androidMain/kotlin/xyz/self/sdk/api/SelfSdk.android.kt`
around lines 158 - 176, bindActivity currently adds a new
DefaultLifecycleObserver on every call causing duplicate observers; to fix,
track the registered observer (e.g., add a private lifecycleObserver field) and
only call activity.lifecycle.addObserver when either lifecycleObserver is null
or is associated with a different activity, or unregister the previous observer
before adding a new one; ensure the observer you register clears
lifecycleObserver when onDestroy runs and still performs the existing cleanup
for activityLauncher, launcherOwner, boundActivity and pendingCallback so
duplicate onDestroy executions are prevented.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Chunk 1E (WebView App Shell): Replace bridge-based MRZ scanning with the camera adapter API.
DocumentCameraScreennow receivesdocumentTypeandcountryCodefrom navigation state and passes them through tocamera.scanMRZ(). Adds stale-scan guard (mountedRef + scanGenerationRef) to prevent state updates after unmount/cancel, MRZ field validation before NFC navigation, and sanitized error handling (generic usermessage, stable error codes for analytics).
Chunk 2F (SDK Public API + Test App): Add activity binding and
ActivityResultLaunchermanagement toSelfSdk.android.ktsoSelfSdk.launch()works from anyComponentActivity. Includes in-flight verification guard (pendingCallbackcheck),ensureLauncher()with stable registry key, try-catch aroundlauncher.launch()to prevent stuck state, andWeakReference-based activity tracking. KMP test app gets a newSdkLaunchScreenthat exercises the fullconfigure → launchpath.Tests: Added
VerificationResultserialization round-trip test.Tested
Explain how the change has been tested (for example by manual testing, unit tests etc) or why it's not necessary (for example version bump).
How to QA
How can the change be tested in a repeatable manner?
Summary by CodeRabbit
New Features
New Screens
Tests
Refactor
Behavior