[PM-34125] feat: Add card text analysis pipeline#6720
Merged
SaintPatrck merged 6 commits intomainfrom Apr 6, 2026
Merged
Conversation
SaintPatrck
added
the
ai-review-vnext
github-actions
Bot
added
app:password-manager
Contributor
Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR adds a complete text analysis pipeline for credit card scanning, including ML Kit text recognition integration, regex-based card data parsing with Luhn validation, a camera overlay composable, and card brand detection. The implementation follows established codebase patterns (mirroring the Code Review DetailsNo findings to report. The code is well-structured and follows established patterns. Notable positives:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6720 +/- ##
==========================================
- Coverage 85.54% 85.47% -0.07%
==========================================
Files 949 956 +7
Lines 60571 60695 +124
Branches 8574 8634 +60
==========================================
+ Hits 51817 51882 +65
- Misses 5773 5800 +27
- Partials 2981 3013 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
New Issues (128)Checkmarx found the following issues in this Pull Request
|
SaintPatrck
force-pushed
the
card-scanner/2-text-analysis-pipeline
branch
from
2333a61 to
d776756
Compare
SaintPatrck
added
innovation
| ) { | ||
| Canvas(modifier = modifier) { | ||
| val strokeWidthPx = strokeWidth.toPx() | ||
| val cornerRadiusPx = 12.dp.toPx() |
Collaborator
There was a problem hiding this comment.
Should this be passed in as a param too?
Contributor
Author
There was a problem hiding this comment.
I don't think so. The corner radius is a fixed visual property of the card shape, not something the callers should modify.
| val number = panMatch | ||
| ?.value | ||
| ?.filter { it.isDigit() } | ||
| ?.takeIf { it.isValidLuhn() } |
Collaborator
There was a problem hiding this comment.
Do we want to return null if there is no number.
I noticed that later on you have a check for the number here anyways.
So maybe just return null if it's not valid
Contributor
Author
There was a problem hiding this comment.
I think that couples the parser logic too tightly to the consumers use case. The purpose of this parser is to extract all of the card data it can. It's up to the consumer to decide whether the extracted data is sufficient for its needs. This feel like the correct separation of concerns boundary to me.
Collaborator
There was a problem hiding this comment.
It just feels very weird to have a data class that has all nullable properties.
But I see your point, maybe that is OK in this case.
Contributor
Author
There was a problem hiding this comment.
I agree. I'm not against a check to ensure at least some data is parsed, otherwise we return null. That would at least ensure callers don't receive an empty object.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| override lateinit var onCardScanned: (CardScanData) -> Unit | ||
|
|
||
| override fun close() { | ||
| recognizer.close() |
Collaborator
There was a problem hiding this comment.
I am unsure how we will call this when it is provided via a LocalComposition
Contributor
Author
There was a problem hiding this comment.
Good point. I don't actually think we need it, anyways. At least not until we see actual memory issues. If we need to, we might be able to leverage remember and DisposableEffect to make sure it's closed properly. I'll revert for now and we can come back if issues start showing up.
Contributor
Author
|
After discussion with Design team, cardholder name extraction will be omitted. It is too error prone, and comparable features in other applications also omit cardholder name when scanning with a camera. |
david-livefront
previously approved these changes
Apr 2, 2026
Base automatically changed from
card-scanner/1-generalize-camera-preview
to
main
April 6, 2026 18:27
SaintPatrck
dismissed
david-livefront’s stale review
The base branch was changed.
Add the complete text analysis pipeline for credit card scanning: - CardNumberUtils: sanitize, Luhn validation, brand detection - CardDataParser: interface and implementation for OCR text parsing - CardTextAnalyzer: ML Kit-based camera frame analysis - CardScanOverlay: camera overlay composable - CardScanData: data class for parsed card fields - FakeCardTextAnalyzer: test fixture - LocalProviders: composition local for CardTextAnalyzer
- Move empty digits check into when block as first case - Remove unnecessary @Suppress("MagicNumber") from parseCardData - Remove unused default values from CardScanData - Use @OptIn(ExperimentalGetImage::class) with androidx.annotation.OptIn - Remove default CardDataParserImpl from CardTextAnalyzerImpl constructor
- Add Closeable to CardTextAnalyzerImpl to release ML Kit native resources - Override toString on CardScanData to mask sensitive fields - Add missing RuPay brand detection test coverage
Instead of returning a CardScanData with all null properties, return null to avoid creating meaningless objects. Consolidate tests to assert full objects rather than individual properties.
LocalComposition doesn't provide a lifecycle hook for cleanup, so close() would never be invoked. Removes Closeable to stay consistent with QrCodeAnalyzerImpl.
OCR-based name detection is unreliable and inconsistent per Design team decision. Remove the field, regex, and related tests.
SaintPatrck
force-pushed
the
card-scanner/2-text-analysis-pipeline
branch
from
35fa5f1 to
8299421
Compare
| } | ||
| } | ||
|
|
||
| @Suppress("MagicNumber") |
Collaborator
There was a problem hiding this comment.
Is this suppression needed?
david-livefront
approved these changes
Apr 6, 2026
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
2 participants
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.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-34125
📔 Objective
Add the complete text analysis pipeline for credit card scanning:
detectCardBrand) usingVaultCardBrandIncludes full test coverage for parser logic, Luhn validation, and sanitization.