Fix topAppBar flicker when text is long

[david-livefront]

🎟️ Tracking

N/A

📔 Objective

This PR updates the BitwardenTopAppBar to use a SubcomposeLayout in order to avoid jankiness when displaying a larger toolbar.

📸 Screenshots

Before	After
Screen_recording_20251030_123408.mp4	after.mp4

⏰ 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

Summary of Changes

This PR refactors BitwardenTopAppBar to eliminate visual flickering when displaying long titles. The implementation introduces a SubcomposeLayout that conditionally measures and renders either a standard TopAppBar or MediumTopAppBar based on whether the title text overflows. The refactoring also extracts common UI logic into private composable functions (InternalDefaultTopAppBar, InternalMediumTopAppBar, NavigationIconButton, TitleText, and topAppBarModifier).

Key changes:

• Uses SubcomposeLayout to avoid measuring/rendering both app bar variants
• Tracks overflow state via TextLayoutResult.hasVisualOverflow
• Resets overflow detection when title changes (via remember(key1 = title))
• Extracts repeated code into reusable private composables
• Removes @Suppress("LongMethod") annotation (no longer needed)

---

Critical Issues

1. ⚠️ Missing Test Coverage

Issue: This PR adds significant new logic (SubcomposeLayout, overflow detection, conditional rendering) but includes no unit tests.

Why it matters:

• The overflow detection logic (hasVisualOverflow) is critical for correct behavior
• The SubcomposeLayout measurement logic could break with constraint changes
• State management (titleTextHasOverflow) needs verification across configuration changes

Recommendation: Add Compose UI tests covering:

• Normal title (no overflow) → renders TopAppBar
• Long title (overflow) → renders MediumTopAppBar
• Title changes from short→long and long→short
• Configuration changes preserve correct state

Reference: While the :ui module currently lacks test files for components, this change's complexity warrants test coverage.

---

2. ⚠️ Potential Performance Issue with Repeated Measurements

Issue: When titleTextHasOverflow = false, the default TopAppBar is measured on every composition (line 133-149). The onTextLayout callback triggers a state change that causes recomposition, but if the title genuinely doesn't overflow, this creates unnecessary measurement cycles.

Code location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/appbar/BitwardenTopAppBar.kt:145

onTitleTextLayout = { titleTextHasOverflow = it.hasVisualOverflow }

Why it matters: On every initial render or title change, this will:

1. Compose InternalDefaultTopAppBar
2. Measure it
3. Invoke onTextLayout callback
4. If overflow changes from false→true, trigger recomposition
5. Recompose InternalMediumTopAppBar

Recommendation: Consider checking if the state actually changed before updating:

onTitleTextLayout = { result ->
    val hasOverflow = result.hasVisualOverflow
    if (titleTextHasOverflow != hasOverflow) {
        titleTextHasOverflow = hasOverflow
    }
}

This prevents unnecessary state writes when overflow status hasn't changed.

---

Suggested Improvements

3. 📝 Documentation Could Be More Specific

Issue: The inline comment at line 109-110 explains what but not why the flicker occurs.

Current:

// Without this sub-compose layout, there would be flickering when displaying the
// MediumTopAppBar because the regular TopAppBar would be displayed first.

Suggested improvement:

// SubcomposeLayout prevents flickering by measuring only one app bar variant at a time.
// Without it, the TopAppBar would be composed first, measured, trigger overflow detection,
// then be replaced by MediumTopAppBar, causing visible layout shift.

---

4. 🎨 Consider Memoizing NavigationIconButton Content

Issue: The NavigationIconButton composable is recreated on every recomposition even though its parameters rarely change.

Code location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/appbar/BitwardenTopAppBar.kt:223-237

Suggestion: While Compose's smart recomposition should handle this efficiently, consider if navigationIcon should be marked with @Stable or if the lambda passed to navigationIcon parameter of TopAppBar/MediumTopAppBar should be memoized.

This is a minor optimization but could reduce allocations in heavily animated scenarios.

---

5. 🎨 Inconsistent Parameter Ordering

Issue: The private composables have inconsistent parameter ordering:

• InternalMediumTopAppBar: title, scrollBehavior, navigationIcon, windowInsets, dividerStyle, actions, minimumHeight, modifier
• InternalDefaultTopAppBar: title, scrollBehavior, navigationIcon, windowInsets, dividerStyle, actions, minimumHeight, onTitleTextLayout, modifier

Suggestion: Consider standardizing the order. The common pattern in the codebase is to have modifier last, which both functions follow correctly. However, onTitleTextLayout being inserted before modifier breaks the visual grouping of common parameters.

---

6. 🤔 Subcompose SlotIds Are Strings

Issue: The slot IDs use string literals ("mediumTopAppBarContent", "defaultTopAppBarContent").

Code location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/appbar/BitwardenTopAppBar.kt:118, 135

Suggestion: While this works correctly, consider using an enum or sealed class for type safety:

private enum class TopAppBarSlot { Medium, Default }

// Then use:
.subcompose(slotId = TopAppBarSlot.Medium, content = { ... })

This prevents typos and makes refactoring easier.

---

Good Practices Observed

✅ Proper use of remember(key1 = title) to reset state on title changes
✅ Extracted repeated code into private composables (DRY principle)
✅ Consistent use of fastMap for performance-critical collection operations
✅ Maintained existing public API surface (backward compatible)
✅ Proper use of @OptIn(ExperimentalMaterial3Api::class) annotations
✅ Added preview composables for visual verification
✅ Follows existing code style and naming conventions

---

Action Items for Author

High Priority

• Add unit/UI tests for the new SubcomposeLayout logic and overflow detection
• Consider the performance optimization for onTitleTextLayout to avoid unnecessary state writes

Medium Priority

• Enhance inline documentation to explain why flickering occurs
• Consider using enum/sealed class for SubcomposeLayout slot IDs

Low Priority

• Review parameter ordering consistency across private composables
• Evaluate if NavigationIconButton content should be memoized

---

CI Status (3 pending, 1 passed)

• ✅ Lint workflow - PASSED
• ⏳ Test - In progress
• ⏳ Scan Pull Requests - In progress
• ⏳ Code Review - In progress (this job)

---

[github-actions]

Checkmarx One – Scan Summary & Details – 1d744814-4222-4423-aec7-e1f1a803a57d

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.81%. Comparing base (dd1dbd0) to head (1ed1b09).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6098   +/-   ##
=======================================
  Coverage   84.81%   84.81%           
=======================================
  Files         721      721           
  Lines       52811    52811           
  Branches     7668     7668           
=======================================
  Hits        44791    44791           
  Misses       5328     5328           
  Partials     2692     2692           

☔ 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