Skip to content

PM-29871: Add additional callouts for external links in the app#6614

Merged
david-livefront merged 1 commit intomainfrom
PM-29871-external-links
Mar 9, 2026
Merged

PM-29871: Add additional callouts for external links in the app#6614
david-livefront merged 1 commit intomainfrom
PM-29871-external-links

Conversation

@david-livefront
Copy link
Collaborator

@david-livefront david-livefront commented Mar 4, 2026

🎟️ Tracking

PM-29871

📔 Objective

This PR adds additional callouts to external links within the app. This was done by adding the trailing , Eternal link announcement or by ensuring that all links have custom accessibility actions associated with them.

@david-livefront david-livefront requested a review from a team as a code owner March 4, 2026 21:52
@david-livefront david-livefront added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context ai-review Request a Claude code review t:bug Change Type - Bug labels Mar 4, 2026
@claude
Copy link
Contributor

claude bot commented Mar 4, 2026

Claude finished @david-livefront's task in 3m 59s —— View job


  • Gather context and understand the changes
  • Review code changes
  • Post review feedback

Review Summary

PR: PM-29871: Add additional callouts for external links in the app
Risk Level: Low

Overview

This PR improves accessibility by adding "External link" callouts to UI elements that navigate users outside the app. The changes touch 29 files and are well-structured around three main themes:

  1. Centralized help button abstraction — Renames TooltipData to BitwardenHelpButtonData with a new isExternalLink property, and introduces BitwardenHelpIconButton to consolidate rendering logic and external link content description formatting.

  2. Overflow menu external link support — Adds isExternalLink to OverflowMenuItemData and extracts BitwardenDropdownMenuItem to handle the accessibility announcement ("{text}, External link") with proper hideFromAccessibility() on the child Text to avoid double announcements.

  3. Hypertext link standardization — Replaces manual annotatedStringResource + CustomAccessibilityAction patterns in QR code screens and import logins with the existing BitwardenHyperTextLink composable, reducing code duplication.

Findings

No issues found. The changes are consistent across all affected components. Specific observations:

  • The isExternalLink values are correctly assigned at all call sites (including false for the Privileged Apps help which navigates in-app).
  • The QR code screen color change from hardcoded Color.White to BitwardenTheme.colorScheme.text.primary is safe because those screens force dark theme via CompositionLocalProvider.
  • The hideFromAccessibility() pattern in BitwardenDropdownMenuItem correctly prevents double announcements when isExternalLink is true.
  • Component refactoring in UI components (BitwardenTextField, BitwardenPasswordField, BitwardenSwitch, etc.) properly propagates the renamed BitwardenHelpButtonData parameter.
  • Test updates align with the parameter renames.

Verdict

LGTM — Clean accessibility improvement with good component abstractions and consistent application across the codebase.

* @property isExternalLink Indicates that this button will launch an external link.
*/
data class TooltipData(
data class BitwardenHelpButtonData(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved this to be more associated with the new BitwardenHelpIconButton. Additionally, I renamed it to match the new button and distinguish it from the existing Tooltips we already have in the app.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Logo
Checkmarx One – Scan Summary & Details759a9c38-c898-48aa-a972-46721708a113

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

@codecov
Copy link

codecov bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 92.15686% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.55%. Comparing base (fa4347d) to head (d11f8db).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...tor/ui/platform/feature/settings/SettingsScreen.kt 77.77% 2 Missing ⚠️
...en/ui/vault/feature/qrcodescan/QrCodeScanScreen.kt 88.88% 1 Missing ⚠️
...thenticator/feature/qrcodescan/QrCodeScanScreen.kt 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6614      +/-   ##
==========================================
+ Coverage   85.52%   85.55%   +0.03%     
==========================================
  Files         846      932      +86     
  Lines       58681    59451     +770     
  Branches     8348     8365      +17     
==========================================
+ Hits        50185    50862     +677     
- Misses       5604     5687      +83     
- Partials     2892     2902      +10     
Flag Coverage Δ
app-data 17.60% <0.00%> (+0.33%) ⬆️
app-ui-auth-tools 21.15% <5.88%> (+0.32%) ⬆️
app-ui-platform 15.67% <11.76%> (+0.61%) ⬆️
app-ui-vault 26.51% <43.13%> (-0.04%) ⬇️
authenticator 6.61% <31.37%> (+<0.01%) ⬆️
lib-core-network-bridge 4.27% <0.00%> (-0.01%) ⬇️
lib-data-ui 0.93% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@david-livefront david-livefront force-pushed the PM-29871-external-links branch 2 times, most recently from a7396a6 to 1975690 Compare March 5, 2026 19:07
@github-actions github-actions bot removed the t:bug Change Type - Bug label Mar 5, 2026
@david-livefront david-livefront added the t:bug Change Type - Bug label Mar 5, 2026
<string name="launch_browser">Launch browser</string>
<string name="continue_without_syncing">Continue without syncing</string>
<string name="external_link">External link</string>
<string name="external_link_format" comment="Used for accessibility to indicate that tapping this item will leave the app">%1$s, External link</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

SaintPatrck
SaintPatrck previously approved these changes Mar 9, 2026
@david-livefront david-livefront force-pushed the PM-29871-external-links branch from 1975690 to d11f8db Compare March 9, 2026 17:56
@github-actions github-actions bot removed the t:bug Change Type - Bug label Mar 9, 2026
@david-livefront david-livefront added the t:bug Change Type - Bug label Mar 9, 2026
@david-livefront
Copy link
Collaborator Author

Thanks @SaintPatrck

@david-livefront david-livefront added this pull request to the merge queue Mar 9, 2026
Merged via the queue into main with commit e260f1d Mar 9, 2026
27 checks passed
@david-livefront david-livefront deleted the PM-29871-external-links branch March 9, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants