Consolidate RN test app Android MRZ scanner onto shared SDK handler#1817
Consolidate RN test app Android MRZ scanner onto shared SDK handler#1817transphorm merged 5 commits intodevfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates MRZ scanning into the KMP SDK: removes local MRZ parser and CameraX/ML Kit integration, replaces scanning with SDK's CameraMrzBridgeHandler and coroutine-based JSON results, adds publishLibraryVariants and mavenLocal() usage, and publishes KMP SDK to mavenLocal() in CI. Changes
Sequence Diagram(s)sequenceDiagram
participant Activity as SelfMrzScannerActivity
participant Bridge as CameraMrzBridgeHandler (KMP SDK)
participant Native as Native MRZ Engine
Activity->>Bridge: start scan with preview
Bridge->>Native: stream frames / analyze
Native-->>Bridge: JSON result {documentNumber, dateOfBirth, dateOfExpiry}
Bridge-->>Activity: progress updates / final JSON
Activity->>Activity: parse JSON -> RESULT_OK or RESULT_CANCELED
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46bd3d1102
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...ages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzScannerActivity.kt
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzScannerActivity.kt`:
- Around line 73-74: scanJob can race and allow scanMrzWithPreview() to set
RESULT_OK after the activity begins teardown; ensure you cancel the job on every
exit path and short-circuit the success branch by checking activity state.
Update scanMrzWithPreview() (and any callbacks that call setResult(RESULT_OK))
to first check isFinishing || isDestroyed and return early if true, and also
cancel scanJob in all exit/abort paths (not just onDestroy) so the coroutine
cannot resume and overwrite a canceled result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 294bf37e-e521-45f8-81a2-5934144fbecb
📒 Files selected for processing (6)
packages/kmp-sdk/shared/build.gradle.ktspackages/rn-sdk-test-app/android/app/build.gradlepackages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzParser.ktpackages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzScannerActivity.ktpackages/rn-sdk-test-app/android/build.gradlespecs/projects/sdk/workstreams/rn-sdk/SPEC-MRZ-CONSOLIDATION.md
💤 Files with no reviewable changes (1)
- packages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzParser.kt
...ages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzScannerActivity.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzScannerActivity.kt (1)
56-61:⚠️ Potential issue | 🟠 MajorCancel
scanJobon all exit paths to prevent result race.Both the back-press handler and cancel button call
setResult(RESULT_CANCELED)+finish()but don't cancelscanJob. There's a window betweenfinish()andonDestroy()wherescanMrzWithPreviewcould return and overwrite the canceled result withRESULT_OK.🔒 Proposed fix
onBackPressedDispatcher.addCallback(this, object : OnBackPressedCallback(true) { override fun handleOnBackPressed() { + scanJob?.cancel() setResult(RESULT_CANCELED) finish() } })And for the cancel button:
cancelButton.setOnClickListener { + scanJob?.cancel() setResult(RESULT_CANCELED) finish() }Additionally, consider guarding the success path with an
isFinishingcheck:- if (hasResult) return@launch + if (hasResult || isFinishing) return@launchAlso applies to: 184-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzScannerActivity.kt` around lines 56 - 61, The back-press callback and cancel button currently call setResult(RESULT_CANCELED) + finish() but do not cancel the coroutine/job handling the scan, causing scanJob to potentially complete later and overwrite the result; update both the OnBackPressedCallback handler and the cancel button click handler to cancel the scanJob (scanJob?.cancel()) before calling setResult and finish(), and also add a guard in the success path of scanMrzWithPreview to check activity.isFinishing (or a boolean like isFinishingGuard) before calling setResult(RESULT_OK) to avoid overwriting a canceled result; ensure onDestroy also cancels scanJob to cover all exit paths.
🧹 Nitpick comments (2)
specs/projects/sdk/workstreams/rn-sdk/SPEC-MRZ-CONSOLIDATION.md (1)
186-186: Minor: Consider clarifying error code test scope.The test table shows
camera.error-codes.contractas "Pass (Android)" but the description mentions it's a Unit test. If this is actually validated through manual testing or integration tests rather than unit tests, consider updating the Type column to reflect that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/projects/sdk/workstreams/rn-sdk/SPEC-MRZ-CONSOLIDATION.md` at line 186, Update the test specification row for `camera.error-codes.contract` to accurately reflect the test scope: change the Type column from "Unit" to the correct test type (e.g., "Integration" or "Manual") and adjust the Pass/platform cell if needed so it matches how the error codes were validated (e.g., "Pass (Android) - Integration" or "Pass (Android) - Manual"); ensure the `camera.error-codes.contract` description remains unchanged but the Type and Pass cells clearly indicate the actual validation method..github/workflows/rn-sdk-test-app-ci.yml (1)
67-69: Missing Gradle caching for KMP SDK publish step.The new
publishToMavenLocalstep runs without Gradle caching, which will slow down CI runs. Per coding guidelines, workflows should use.github/actions/cache-gradlefor Gradle operations.Consider adding the cache action before this step:
- name: Cache Gradle uses: ./.github/actions/cache-gradle - name: Publish KMP SDK to mavenLocal run: | cd packages/kmp-sdk && ./gradlew :shared:publishToMavenLocal --quietAlternatively, if the main build step at line 70-73 already benefits from a shared Gradle daemon/cache, you could consolidate both Gradle invocations. As per coding guidelines: "Use shared composite actions in .github/actions when caching dependencies in GitHub workflows instead of calling actions/cache directly... use cache-gradle for Gradle wrappers and caches."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rn-sdk-test-app-ci.yml around lines 67 - 69, Add Gradle caching before the "Publish KMP SDK to mavenLocal" step: insert the existing composite action .github/actions/cache-gradle immediately prior to the step that runs `cd packages/kmp-sdk && ./gradlew :shared:publishToMavenLocal --quiet` (the step named "Publish KMP SDK to mavenLocal"), or consolidate this gradle invocation with the main build gradle step so both use the shared cache/daemon; ensure the cache action is referenced as uses: ./.github/actions/cache-gradle so the publishToMavenLocal step benefits from Gradle caching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzScannerActivity.kt`:
- Around line 56-61: The back-press callback and cancel button currently call
setResult(RESULT_CANCELED) + finish() but do not cancel the coroutine/job
handling the scan, causing scanJob to potentially complete later and overwrite
the result; update both the OnBackPressedCallback handler and the cancel button
click handler to cancel the scanJob (scanJob?.cancel()) before calling setResult
and finish(), and also add a guard in the success path of scanMrzWithPreview to
check activity.isFinishing (or a boolean like isFinishingGuard) before calling
setResult(RESULT_OK) to avoid overwriting a canceled result; ensure onDestroy
also cancels scanJob to cover all exit paths.
---
Nitpick comments:
In @.github/workflows/rn-sdk-test-app-ci.yml:
- Around line 67-69: Add Gradle caching before the "Publish KMP SDK to
mavenLocal" step: insert the existing composite action
.github/actions/cache-gradle immediately prior to the step that runs `cd
packages/kmp-sdk && ./gradlew :shared:publishToMavenLocal --quiet` (the step
named "Publish KMP SDK to mavenLocal"), or consolidate this gradle invocation
with the main build gradle step so both use the shared cache/daemon; ensure the
cache action is referenced as uses: ./.github/actions/cache-gradle so the
publishToMavenLocal step benefits from Gradle caching.
In `@specs/projects/sdk/workstreams/rn-sdk/SPEC-MRZ-CONSOLIDATION.md`:
- Line 186: Update the test specification row for `camera.error-codes.contract`
to accurately reflect the test scope: change the Type column from "Unit" to the
correct test type (e.g., "Integration" or "Manual") and adjust the Pass/platform
cell if needed so it matches how the error codes were validated (e.g., "Pass
(Android) - Integration" or "Pass (Android) - Manual"); ensure the
`camera.error-codes.contract` description remains unchanged but the Type and
Pass cells clearly indicate the actual validation method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e90d062-e723-4c1e-80fc-4a243fae95d2
📒 Files selected for processing (3)
.github/workflows/rn-sdk-test-app-ci.ymlpackages/rn-sdk-test-app/android/app/src/main/java/com/selfxyz/demoapp/SelfMrzScannerActivity.ktspecs/projects/sdk/workstreams/rn-sdk/SPEC-MRZ-CONSOLIDATION.md
1 participant
This PR delivers the first-round Android MRZ consolidation in rn-sdk-test-app by replacing local parser/scanner logic with the shared SDK camera handler flow.
What changed
Scope / non-goals
Summary by CodeRabbit
Refactor
Bug Fixes / Chores
Documentation