PM-27494: Update custom vault timeout UI

[david-livefront]

🎟️ Tracking

PM-27494

📔 Objective

This PR updates the way the Custom Vault Timeout Button looks on the Account Security Screen to match the latest designs.

📸 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]

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

---

Code Review Complete ✓

I've completed a comprehensive review of PR #6085, which updates the custom vault timeout UI to match new designs. The implementation creates a new reusable BitwardenTimePickerButton component and improves the user experience by displaying human-readable time durations instead of 24-hour clock times.

---

Summary of Changes

This PR refactors the custom vault timeout UI by:

1. Creating a new component: BitwardenTimePickerButton - a reusable time picker button that displays durations in human-readable format (e.g., "2 hours, 3 minutes")
2. Enhancing BitwardenTextSelectionButton: Added showChevron parameter for cases where the chevron icon should be hidden
3. Simplifying AccountSecurityScreen: Replaced inline time picker dialog logic with the new component, reducing code complexity
4. Updating string resources: Added new strings for displaying hours/minutes format and "Custom timeout" label
5. Updating tests: Modified test assertions to match the new human-readable format

---

Critical Issues

1. ⚠️ Missing KDoc documentation on new component

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/dropdown/BitwardenTimePickerButton.kt:1-174

The new BitwardenTimePickerButton component lacks comprehensive KDoc documentation for the class/file itself. According to the project's style guidelines (docs/STYLE_AND_BEST_PRACTICES.md), "All public classes, functions, and properties should include documentation in the KDoc style."

While individual parameters are documented in the KDoc, there should be a file-level or class-level KDoc comment explaining:

• The purpose and use case of this component
• When to use it vs. alternatives
• Example usage (if complex)

Recommendation: Add a comprehensive KDoc comment at the file level explaining the component's purpose and typical usage patterns.

---

2. ⚠️ Potential edge case: Zero-duration display

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/dropdown/BitwardenTimePickerButton.kt:142-149

When both hours and minutes are 0, the component displays "0 minutes". However, the comment states "We display this if there are only minutes or if both hours and minutes are 0." This might be confusing UX.

} else {
    // We display this if there are only minutes or if both hours and minutes are 0.
    pluralStringResource(
        id = BitwardenPlurals.minutes_format,
        count = minutes,
        formatArgs = arrayOf(minutes),
    )
}

Questions:

• Is zero-duration a valid vault timeout configuration?
• Should this case be handled differently (e.g., showing a validation message)?
• If zero is valid, should it display as "Immediately" instead of "0 minutes"?

Recommendation: Clarify the expected behavior for zero-duration timeouts and consider adding validation or special handling if appropriate.

---

3. ℹ️ Inconsistent parameter naming: isEnabled vs enabled

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/dropdown/BitwardenTimePickerButton.kt:52,68

The new component uses isEnabled as the parameter name, while the underlying BitwardenTextSelectionButton uses enabled. This creates an unnecessary mapping:

fun BitwardenTimePickerButton(
    // ...
    isEnabled: Boolean = true,
    // ...
) {
    BitwardenTextSelectionButton(
        // ...
        enabled = isEnabled,
        // ...
    )
}

Recommendation: Use consistent naming (enabled) throughout the component hierarchy unless there's a specific reason for the difference. Check if this follows a broader project convention.

---

Suggested Improvements

4. 🎨 Consider extracting time formatting logic

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/dropdown/BitwardenTimePickerButton.kt:118-149

The time formatting logic (lines 118-149) is quite verbose and embedded directly in the composable. This could be extracted into a separate function for better testability and reusability.

Example refactoring

/**
 * Formats hours and minutes into a human-readable string.
 */
@Composable
private fun formatDuration(hours: Int, minutes: Int): String {
    return when {
        hours != 0 && minutes != 0 -> stringResource(
            id = BitwardenString.hours_minutes_format,
            formatArgs = arrayOf(
                pluralStringResource(
                    id = BitwardenPlurals.hours_format,
                    count = hours,
                    formatArgs = arrayOf(hours),
                ),
                pluralStringResource(
                    id = BitwardenPlurals.minutes_format,
                    count = minutes,
                    formatArgs = arrayOf(minutes),
                ),
            ),
        )
        hours != 0 -> pluralStringResource(
            id = BitwardenPlurals.hours_format,
            count = hours,
            formatArgs = arrayOf(hours),
        )
        else -> pluralStringResource(
            id = BitwardenPlurals.minutes_format,
            count = minutes,
            formatArgs = arrayOf(minutes),
        )
    }
}

This would make the composable cleaner and the formatting logic easier to test independently.

---

5. 🎨 String resource comment clarity

Location: ui/src/main/res/values/strings.xml:501,502,506

The comments on the plural format strings state "Can be injected into a sentence with %1$s and %2$s", but these plurals only have a single %1$d format parameter. This comment seems copy-pasted and doesn't match the actual implementation.

<plurals name="hours_format" comment="Can be injected into a sentence with %1$s and %2$s">
    <item quantity="one">%1$d hour</item>
    <item quantity="other">%1$d hours</item>
</plurals>

Recommendation: Update comments to accurately describe the single integer parameter: comment="Formats a number of hours (e.g., '1 hour', '5 hours')".

---

6. 🎨 Code simplification opportunity in test updates

Location: app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt:1050,1059,1067,1079,1105,1126,1161

The test updates correctly reflect the new format, but there are multiple similar assertions that could potentially be extracted into a helper function to reduce duplication if more such tests are added in the future.

---

7. ℹ️ Consider adding a preview for BitwardenTimePickerButton

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/dropdown/BitwardenTimePickerButton.kt:174

The new component lacks a @Preview composable. Following the pattern from BitwardenTextSelectionButton.kt:209-220, adding preview functions would improve the development experience.

Recommendation: Add preview composables showing different states (with hours only, minutes only, both, zero duration, disabled state, etc.).

---

Good Practices Observed

• ✓ Proper use of rememberSaveable for dialog state preservation across configuration changes
• ✓ Clean separation of concerns with the new reusable component
• ✓ Comprehensive test updates matching UI changes
• ✓ Proper internationalization with plurals support
• ✓ Consistent with MVVM and Compose architecture patterns
• ✓ Good use of nullable parameters and default values for optional features
• ✓ Tests updated to match the new UI format

---

Action Items for Author

1. High Priority: Add comprehensive KDoc documentation to BitwardenTimePickerButton
2. Medium Priority: Clarify/validate the zero-duration behavior (is it valid? should it display differently?)
3. Low Priority: Consider parameter naming consistency (isEnabled vs enabled)
4. Optional: Extract time formatting logic for better testability
5. Optional: Update string resource comments for accuracy
6. Optional: Add @Preview composables to the new component

---

Security & Architecture Compliance

✓ Architecture: Follows MVVM + UDF patterns correctly
✓ Module Organization: New component properly placed in :ui module
✓ Compose Best Practices: Proper state hoisting and recomposition handling
✓ Dependency Injection: Not applicable for this UI component
✓ Error Handling: Validation logic properly implemented
✓ Security: No security concerns identified

---

Overall Assessment

This is a well-implemented PR that successfully improves the user experience by replacing the 24-hour clock display with human-readable duration text. The creation of a reusable BitwardenTimePickerButton component is a good architectural decision that will benefit future development.

The main issues are documentation-related rather than functional. The code is clean, follows project conventions, and includes appropriate test coverage.

Recommendation: Approve with minor suggestions for documentation improvements.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 726bc3ad-efce-4590-8bf6-b4c00629e836

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

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.80%. Comparing base (c0f8307) to head (ac90aaa).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
.../settings/accountsecurity/AccountSecurityScreen.kt	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6085      +/-   ##
==========================================
- Coverage   84.81%   84.80%   -0.01%     
==========================================
  Files         721      721              
  Lines       52830    52811      -19     
  Branches     7671     7669       -2     
==========================================
- Hits        44809    44789      -20     
- Misses       5331     5332       +1     
  Partials     2690     2690              

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

Thanks @SaintPatrck