Skip to content

Conversation

@david-livefront
Copy link
Collaborator

@david-livefront david-livefront commented Nov 20, 2025

🎟️ Tracking

PM-28522

📔 Objective

This PR updates the LoginWithDeviceScreen, bringing it up to current standards.

📸 Screenshots

Before After

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

@claude
Copy link
Contributor

claude bot commented Nov 20, 2025

Claude finished @david-livefront's task —— View job


Code Review Complete

PR Description Quality

The PR description is well-structured and complete:

  • Clear objective explaining the modernization of LoginWithDeviceScreen
  • Visual comparison with before/after screenshots
  • Proper Jira tracking reference
  • Complete reminders checklist

Summary

This PR successfully modernizes the LoginWithDeviceScreen by:

  • Replacing custom text/button components with standardized Bitwarden UI components (BitwardenTextField, BitwardenOutlinedButton, BitwardenHyperTextLink)
  • Improving visual consistency with centered text alignment and updated typography
  • Simplifying state management by removing the isResendNotificationLoading flag in favor of dialog-based loading states
  • Enhancing the fingerprint phrase display with proper read-only text field styling
  • Better separation of concerns in string resources

The changes align well with the codebase's design system and improve maintainability. All tests have been updated appropriately, and the implementation follows established patterns.


Findings

Finding 1: Missing test coverage for new UI component behavior
LoginWithDeviceScreenTest.kt:95-100

The test for "view all log in options" was updated to use performFirstLinkClick() for the new BitwardenHyperTextLink component, but there's no test coverage for the BitwardenTextField component's read-only behavior with the fingerprint phrase. Consider adding a test to verify:

  • The text field is read-only
  • The TextToolbarType.NONE prevents copy/paste actions
  • The custom text color and style are applied correctly

Finding 2: Potential accessibility consideration for centered text
LoginWithDeviceScreen.kt:123-155

All title, subtitle, and description text now uses TextAlign.Center instead of TextAlign.Start. While this improves visual aesthetics, centered text can be harder to read for users with certain accessibility needs (dyslexia, cognitive disabilities). The Android accessibility guidelines generally recommend left-aligned text for better readability.

💭 Consider whether centered alignment is a deliberate design decision or if left-aligned text would be more accessible while still achieving the desired visual update.

Finding 3: Inconsistent string resource naming pattern
strings.xml:656-657

The new string need_another_option_view_all_login_options breaks from the original compound string pattern. The old approach used a single compound string (log_in_with_device_must_be_set_up_in_the_settings_of_the_bitwarden_app_need_another_option), while the new approach splits it into two resources. While this is more flexible, it's worth noting this creates an inconsistent pattern compared to similar compound strings elsewhere in the codebase.

💭 This is actually an improvement for localization purposes (shorter strings are easier to translate), but the inconsistency with existing patterns may warrant documentation or eventual refactoring of similar compound strings.


Positive Notes

👍 Excellent simplification of the loading state management - using DialogState.Loading instead of an inline loading indicator is much cleaner

👍 The new textColor parameter added to BitwardenTextField enhances component reusability

👍 Test updates properly reflect the UI changes, including the new link interaction pattern


CI Status: Lint workflow passed ✓ | Tests and security scans in progress

@github-actions
Copy link
Contributor

github-actions bot commented Nov 20, 2025

Logo
Checkmarx One – Scan Summary & Details5655243b-f44e-4966-8686-51132895301b

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.41%. Comparing base (f02b374) to head (f519524).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...eature/loginwithdevice/LoginWithDeviceViewModel.kt 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6184   +/-   ##
=======================================
  Coverage   85.41%   85.41%           
=======================================
  Files         755      755           
  Lines       54105    54090   -15     
  Branches     7798     7795    -3     
=======================================
- Hits        46212    46203    -9     
+ Misses       5180     5176    -4     
+ Partials     2713     2711    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

SaintPatrck
SaintPatrck previously approved these changes Nov 20, 2025
vvolkgang
vvolkgang previously approved these changes Nov 27, 2025
Copy link
Member

@vvolkgang vvolkgang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾 There's a conflict in the strings.xml file, otherwise looks great!

@david-livefront
Copy link
Collaborator Author

Thanks @SaintPatrck & @vvolkgang

@david-livefront david-livefront added this pull request to the merge queue Dec 1, 2025
Merged via the queue into main with commit ca7a65f Dec 1, 2025
15 checks passed
@david-livefront david-livefront deleted the PM-28522-login-with-device-ui branch December 1, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants