PM-27597: Update Yubikey illustration to match the rest of the app #6087
Conversation
david-livefront
requested review from
a team and
brian-livefront
as code owners
|
Claude finished @david-livefront's task —— View job Code Review Complete ✅I've reviewed this PR which includes two main improvements:
SummaryThis PR successfully modernizes the YubiKey illustration and eliminates significant code duplication through intelligent refactoring. The changes follow established patterns and improve both user experience and code maintainability. Key ImprovementsVector Drawable Migration:
TOTP Parsing Consolidation:
String Updates:
Architecture Compliance
RecommendationsString Consistency (Minor):
See inline comment on Test Results
Code QualityStrengths:
Metrics:
ConclusionThis is a well-executed PR that improves both the visual presentation and code architecture. The YubiKey illustration update provides a modern, scalable asset while the TOTP parsing consolidation demonstrates excellent refactoring discipline. The only suggestion is the minor string consistency improvement mentioned above. Recommendation: ✅ Approve (with optional string consistency improvement) |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6087 +/- ##
==========================================
+ Coverage 84.91% 84.96% +0.04%
==========================================
Files 724 724
Lines 52820 52758 -62
Branches 7679 7659 -20
==========================================
- Hits 44853 44826 -27
+ Misses 5274 5249 -25
+ Partials 2693 2683 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hey, I know, PRs are no discussion board... so I'll just throw it in shortly:
BTW, the web vault not only shows all YubiKey options (4, 5 and NEO) but also the correct name "Yubico OTP": 
PS: Overall, it would be good to have one common name for the same thing in all places... currently:
(--> the web vault's version "Yubico OTP security key" would be my - and probably also Yubico's - favorite, as it's closest to their product name...) |
|
@pamperer562580892423 I have forwarded your concerns to our design team and am awaiting their feedback. |
david-livefront
force-pushed
the
standardize-yubikey-illustration
branch
from
1c00e9d to
736fe3d
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ui/src/main/res/values/strings.xml
Outdated
| <string name="verification_email_sent">Verification email sent</string> | ||
| <string name="yubi_key_instruction">To continue, hold your YubiKey NEO against the back of the device or insert your YubiKey into your device’s USB port, then touch its button.</string> | ||
| <string name="yubi_key_title">YubiKey security key</string> | ||
| <string name="yubi_key_instruction">To continue, hold your Yubico OTP security key against the back of the device or insert it into your device’s USB port, then touch its button.</string> |
There was a problem hiding this comment.
📝 String Update Analysis
The instruction text has been updated from mentioning "YubiKey NEO" specifically to "Yubico OTP security key". This is more accurate since:
- YubiKey 4 and 5 also support Yubico OTP (not just NEO)
- It correctly identifies the 2FA method as "Yubico OTP" rather than just "YubiKey"
However, there's a minor inconsistency worth noting:
- The title uses "Yubico security key" (line 183)
- The instruction uses "Yubico OTP security key" (line 182)
For consistency with the web vault (which uses "Yubico OTP security key") and clarity for users, consider updating line 183 to also include "OTP":
| <string name="yubi_key_instruction">To continue, hold your Yubico OTP security key against the back of the device or insert it into your device’s USB port, then touch its button.</string> | |
| <string name="yubi_key_title">Yubico OTP security key</string> |
This would align with:
- Web vault naming convention
- The more specific instruction text
- Industry-standard terminology from Yubico
| contentDescription = null, | ||
| modifier = Modifier | ||
| .standardHorizontalMargin() | ||
| .size(124.dp), |
There was a problem hiding this comment.
👍 Good Improvement
The migration from painterResource (used for raster images) to using it with the new vector drawable is correct. The image is now properly sized at 124.dp, which matches the vector drawable's viewport dimensions and provides consistent sizing with other 2FA method illustrations like the authenticator app icon.
The removal of unnecessary layout complexity (previous version had more nested layouts) also improves performance.
| val TwoFactorAuthMethod.imageRes: Int? | ||
| get() = when (this) { | ||
| TwoFactorAuthMethod.YUBI_KEY -> BitwardenDrawable.img_yubi_key | ||
| TwoFactorAuthMethod.YUBI_KEY -> BitwardenDrawable.ill_yubi_key |
There was a problem hiding this comment.
👍 Correct Resource Update
The reference has been properly updated from the raster image resources (img_yubi_key) to the new vector drawable (ill_yubi_key). This follows the established naming convention in the codebase:
img_*prefix for raster imagesill_*prefix for illustration vector drawables
The vector drawable provides:
- Better scalability across different screen densities
- Smaller binary size (single XML file vs 5 PNG files)
- Consistency with other 2FA illustrations
| @@ -0,0 +1,85 @@ | |||
| <vector xmlns:android="http://schemas.android.com/apk/res/android" | |||
There was a problem hiding this comment.
👍 Well-Designed Vector Drawable
The new vector illustration is well-structured:
- Proper viewport dimensions (124x124) matching the design specifications
- Clear semantic naming for paths (
primary,secondary,accent,outline,tertiary) - Follows Material Design color palette conventions
- Uses the established color scheme from the app's design system
The illustration effectively represents a YubiKey device with:
- A key/USB device representation
- Clear visual hierarchy with multiple color layers
- Consistent style with other 2FA illustrations in the app
This replaces ~2MB of PNG assets across 5 density buckets with a single ~3KB XML file.
| TwoFactorAuthMethod.EMAIL to BitwardenDrawable.ill_new_device_verification, | ||
| TwoFactorAuthMethod.DUO to null, | ||
| TwoFactorAuthMethod.YUBI_KEY to BitwardenDrawable.img_yubi_key, | ||
| TwoFactorAuthMethod.YUBI_KEY to BitwardenDrawable.ill_yubi_key, |
There was a problem hiding this comment.
✅ Test Coverage Updated
The test has been correctly updated to verify that TwoFactorAuthMethod.YUBI_KEY returns BitwardenDrawable.ill_yubi_key instead of the old raster image resource. This ensures:
- The extension property returns the correct drawable resource
- Test coverage is maintained at the same level
- Future refactoring won't break the image association
All tests are passing according to CI, confirming the changes work as expected.
david-livefront
changed the title
david-livefront
force-pushed
the
standardize-yubikey-illustration
branch
from
736fe3d to
d9a1f8a
Compare
| <string name="yubi_key_instruction">To continue, hold your YubiKey NEO against the back of the device or insert your YubiKey into your device’s USB port, then touch its button.</string> | ||
| <string name="yubi_key_title">YubiKey security key</string> | ||
| <string name="yubi_key_instruction">To continue, hold your Yubico security key against the back of the device or insert it into your device’s USB port, then touch its button.</string> | ||
| <string name="yubi_key_title">Yubico security key</string> |
There was a problem hiding this comment.
📝 String Consistency Suggestion
The title uses "Yubico security key" while the instruction text (line 182) uses "Yubico OTP security key". For consistency with:
- The web vault (which uses "Yubico OTP security key")
- The more specific instruction text
- Industry-standard terminology from Yubico
Consider updating to:
| <string name="yubi_key_title">Yubico security key</string> | |
| <string name="yubi_key_title">Yubico OTP security key</string> |
This would also address the user feedback from @pamperer562580892423 about naming consistency across platforms.
| <string name="verification_email_sent">Verification email sent</string> | ||
| <string name="yubi_key_instruction">To continue, hold your YubiKey NEO against the back of the device or insert your YubiKey into your device’s USB port, then touch its button.</string> | ||
| <string name="yubi_key_title">YubiKey security key</string> | ||
| <string name="yubi_key_instruction">To continue, hold your Yubico security key against the back of the device or insert it into your device’s USB port, then touch its button.</string> |
There was a problem hiding this comment.
👍 Improved Accuracy
Good update from "YubiKey NEO" to "Yubico OTP security key". This is more accurate because:
- YubiKey 4 and 5 also support Yubico OTP (not just NEO)
- It correctly identifies the 2FA method as "Yubico OTP" rather than just "YubiKey"
This addresses the concern raised by @pamperer562580892423 about the NEO-specific language.
| @@ -0,0 +1,85 @@ | |||
| <vector xmlns:android="http://schemas.android.com/apk/res/android" | |||
There was a problem hiding this comment.
👍 Excellent Vector Drawable Implementation
This new vector illustration is well-designed:
- ✅ Proper viewport dimensions (124x124) matching design specs
- ✅ Clear semantic naming for paths (
primary,secondary,accent,outline,tertiary) - ✅ Follows Material Design color palette conventions
- ✅ Consistent with app's design system
Benefits:
- Replaces ~2MB of PNG assets across 5 density buckets with a single ~3KB XML file
- Better scalability across different screen densities
- Consistency with other 2FA illustrations in the codebase
The illustration effectively represents a YubiKey device with clear visual hierarchy.
| contentDescription = null, | ||
| modifier = Modifier | ||
| .standardHorizontalMargin() | ||
| .size(124.dp), |
There was a problem hiding this comment.
👍 Improved Image Rendering
Good migration from raster to vector drawable:
- ✅
painterResourcenow used with vector drawable - ✅ Proper sizing at 124.dp matching viewport dimensions
- ✅ Consistent with other 2FA illustrations (authenticator app icon)
- ✅ Simplified layout complexity (removed unnecessary nesting)
The streamlined implementation improves both performance and maintainability.
| val TwoFactorAuthMethod.imageRes: Int? | ||
| get() = when (this) { | ||
| TwoFactorAuthMethod.YUBI_KEY -> BitwardenDrawable.img_yubi_key | ||
| TwoFactorAuthMethod.YUBI_KEY -> BitwardenDrawable.ill_yubi_key |
There was a problem hiding this comment.
👍 Correct Resource Update
Properly updated from raster (img_yubi_key) to vector drawable (ill_yubi_key). Follows established naming conventions:
img_*prefix for raster imagesill_*prefix for illustration vector drawables
The vector approach provides better scalability and smaller binary size.
| vaultRepository.emitTotpCodeResult(result) | ||
| val qrCode = action.qrCode | ||
| qrCode | ||
| .getTotpDataOrNull() |
There was a problem hiding this comment.
👍 Simplified ViewModel Logic
Excellent refactoring using the new TotpUriUtils.getTotpDataOrNull() extension:
Before: ~80 lines of URI parsing logic duplicated in this ViewModel
After: Single line delegation to shared utility
This aligns with MVVM architecture guidelines:
- ViewModels should focus on UI logic, not parsing utilities
- Shared parsing logic belongs in utility modules (
:ui) - Reduces cyclomatic complexity and improves testability
The error handling remains clear with the elvis operator handling invalid TOTP data.
| import com.bitwarden.authenticatorbridge.manager.AuthenticatorBridgeManager | ||
| import com.bitwarden.ui.platform.base.BaseViewModel | ||
| import com.bitwarden.ui.platform.base.util.isBase32 | ||
| import com.bitwarden.ui.platform.util.getTotpDataOrNull |
There was a problem hiding this comment.
👍 Consistent TOTP Parsing Across Modules
Good use of the shared getTotpDataOrNull() utility from :ui module. This ensures:
- ✅ Consistency: Both
:appand:authenticatormodules use identical TOTP parsing logic - ✅ Module Architecture: Authenticator module correctly depends on
:uifor shared utilities - ✅ Code Reuse: Eliminates ~80 lines of duplicated parsing code from this module
This follows the project's multi-module architecture where :ui provides common utilities for both application modules.
| TwoFactorAuthMethod.EMAIL to BitwardenDrawable.ill_new_device_verification, | ||
| TwoFactorAuthMethod.DUO to null, | ||
| TwoFactorAuthMethod.YUBI_KEY to BitwardenDrawable.img_yubi_key, | ||
| TwoFactorAuthMethod.YUBI_KEY to BitwardenDrawable.ill_yubi_key, |
There was a problem hiding this comment.
✅ Test Coverage Maintained
Test correctly updated to verify TwoFactorAuthMethod.YUBI_KEY returns the new ill_yubi_key drawable resource. This ensures:
- ✅ Extension property returns correct drawable
- ✅ Test coverage maintained at same level
- ✅ Future refactoring won't break the image association
All tests passing per CI confirms the changes work as expected.
|
Thanks @SaintPatrck |
|
@pamperer562580892423 We've updated the app copy, it does not match the web exactly but it no longer mentions |
|
@david-livefront I would have preferred "Yubico OTP security key" in the title also... but hey, thanks for hearing me out and addressing some of it! 🙂 |
🎟️ Tracking
PM-27597
📔 Objective
Update the
TwoFactorLoginScreento display a Yubi Key illustration instead of a raster image and standardize it with the rest of the 2-factor mechanisms.Figme: https://www.figma.com/design/WjD06w9dFlA67Ba27d1OSu/Dev-Ready--Bitwarden-Android?node-id=40001448-10884&t=JYXtbE7zOvtmnzFh-4
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes