[PM-34127] feat: Integrate card scanner with VaultAddEdit#6722
Draft
SaintPatrck wants to merge 5 commits intocard-scanner/3-card-scan-screenfrom
Draft
[PM-34127] feat: Integrate card scanner with VaultAddEdit#6722SaintPatrck wants to merge 5 commits intocard-scanner/3-card-scan-screenfrom
SaintPatrck wants to merge 5 commits intocard-scanner/3-card-scan-screenfrom
Conversation
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed 8 changed files (+370/-5) integrating the card scanner with the VaultAddEdit screen. Changes span ViewModel logic (scan result handling, feature flag gating), Compose UI (scan button, focus management), navigation wiring, handlers, and comprehensive tests covering success/error/partial scans, flag toggling, permissions, and post-scan focus behavior. Code Review DetailsNo issues found. The implementation follows all established codebase patterns:
|
| import com.bitwarden.vault.DecryptCipherListResult | ||
| import com.bitwarden.vault.FolderView | ||
| import com.x8bit.bitwarden.data.auth.repository.AuthRepository | ||
| import com.bitwarden.ui.platform.feature.cardscanner.manager.CardScanManager |
Contributor
There was a problem hiding this comment.
🔧 Suggested: Import ordering — the new com.bitwarden.ui.platform.feature.cardscanner.* imports are interspersed within the com.x8bit.bitwarden.* block (lines 28, 59, 67). They should be grouped with the other com.bitwarden.* imports above (lines 8–26) to maintain alphabetical ordering. Same issue in VaultUnlockedNavigation.kt (lines 62–63) and VaultAddEditViewModelTest.kt.
Running the IDE's "Optimize Imports" action should fix all of these.
| onNavigateToGeneratorModal = { onNavigateToGeneratorModalType = it }, | ||
| onNavigateToAttachments = { onNavigateToAttachmentsId = it }, | ||
| onNavigateToMoveToOrganization = { id, _ -> onNavigateToMoveToOrganizationId = id }, | ||
| onNavigateToCardScanScreen = {}, |
Contributor
There was a problem hiding this comment.
💡 Suggested: Consider adding screen-level tests for the scan card button:
ScanCardButtonis displayed whenisCardScannerEnabled = trueand cipher type is CardScanCardButtonis NOT displayed whenisCardScannerEnabled = falseNavigateToCardScanevent triggers theonNavigateToCardScanScreencallback
The ViewModel tests cover the business logic well, but screen tests would verify the UI wiring end-to-end (button visibility, permission launcher, navigation callback).
Contributor
|
New Issues (129)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## card-scanner/3-card-scan-screen #6722 +/- ##
===================================================================
- Coverage 85.64% 85.63% -0.01%
===================================================================
Files 955 955
Lines 60595 60691 +96
Branches 8598 8628 +30
===================================================================
+ Hits 51895 51974 +79
+ Misses 5695 5694 -1
- Partials 3005 3023 +18
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:
|
583a110 to
7c9d521
Compare
Comment on lines
+62
to
+63
| import com.x8bit.bitwarden.ui.vault.feature.cardscanner.cardScanDestination | ||
| import com.x8bit.bitwarden.ui.vault.feature.cardscanner.navigateToCardScanScreen |
Contributor
There was a problem hiding this comment.
🔧 Suggestion: Imports are not in alphabetical order. The cardscanner imports should come before importlogins (line 57) to maintain consistent import sorting with the rest of the file.
Suggested change
| import com.x8bit.bitwarden.ui.vault.feature.cardscanner.cardScanDestination | |
| import com.x8bit.bitwarden.ui.vault.feature.cardscanner.navigateToCardScanScreen | |
| import com.x8bit.bitwarden.ui.vault.feature.cardscanner.cardScanDestination | |
| import com.x8bit.bitwarden.ui.vault.feature.cardscanner.navigateToCardScanScreen | |
| import com.x8bit.bitwarden.ui.vault.feature.manualcodeentry.navigateToManualCodeEntryScreen |
Move these two lines up before the importlogins imports (before line 57).
7c9d521 to
204305e
Compare
| ), | ||
| ), | ||
| ) | ||
| val content = viewModel.stateFlow.value.viewState |
Collaborator
There was a problem hiding this comment.
Can we just assert against the entire state
be951b7 to
ff88fbb
Compare
204305e to
ca82c3b
Compare
Wire the card scanner into the card cipher add/edit flow: - VaultAddEditViewModel: inject CardScanManager, handle scan results, populate card fields with brand-aware CVV validation - VaultAddEditScreen: pass scanner enabled state and click handler - VaultAddEditCardItems: conditionally render Scan Card button - VaultUnlockedNavigation: register card scan route
Cardholder name OCR detection is unreliable. Remove name from scan results and instead focus the name field after a successful scan so the user can type it manually.
Prepares for rebase onto branch 2 which removes default parameter values from CardScanData.
FocusCardHolderName was being dropped by EventsEffect lifecycle filter during navigation back from card scan. Using DeferredBackgroundEvent ensures the event survives the filter and executes once the screen reaches RESUMED state. Also explicitly show the keyboard via SoftwareKeyboardController.
21a7f1a to
31ae25f
Compare
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-34127
📔 Objective
Integrate the card scanner with the VaultAddEdit screen:
CardScanManagerresults, map scanned card data to form fields (number, expiry, CVV, brand detection), and emit focus event for cardholder name inputcard-scanner-mobilefeature flag. After a successful scan, focus the cardholder name field and show the keyboard usingDeferredBackgroundEventto survive lifecycle filteringCardScanScreendestination and navigationIncludes ViewModel and Screen tests for scan result handling, partial scans, feature flag gating, and post-scan focus behavior.