Replace close button with drawer on TopBar

[jvsena42]

FIGMA - V59

Description

This PR replaces the close button with the drawer in all screens
To implement the Drawer on AllActivities Screen, It was necessary to lift it and the TabBar to the parent layer

Preview

Screen_recording_20251127_105934.webm

QA Notes

@piotr-iohk
The NavigationClose test tag was replaced with HeaderMenu

• Test all drawer options
• Navigate deep in a flow -> Check if the drawer is displayed
• Open a sheet dialog -> The tabor must stay behind

[piotr-iohk]

Related e2e update: synonymdev/bitkit-e2e-tests#55

[jvsena42]

sorry for so many changes, I had it to all at once, otherwise would introduce bugs

[claude]

PR Review: Replace close button with drawer on TopBar

Summary

This PR successfully replaces close buttons with a global drawer navigation system, elevating the drawer state to ContentView.kt and implementing a consistent navigation pattern across the app. The implementation involves significant architectural changes to support the drawer at the top level.

✅ Strengths

Architecture & Design

• Well-structured state elevation: The drawer state is properly elevated to ContentView.kt and provided via LocalDrawerState, following Compose best practices
• Good separation of concerns: The DrawerNavIcon component cleanly accesses the drawer state through composition locals
• Consistent UI pattern: Replacing the close button with the drawer provides a more uniform navigation experience
• Proper use of Material3: DrawerState and DrawerValue are used correctly

Code Quality

• Good composable organization: Components like DrawerNavIcon, DrawerMenu, and navigation helpers are well-separated
• Preview support: Preview mode check in DrawerNavIcon (line 97-98 in AppTopBar.kt) allows the component to work in Android Studio previews
• Test tags updated: Changed from NavigationClose to HeaderMenu as documented in QA notes
• Clean refactoring: Removed unused close actions across many screens

⚠️ Issues & Concerns

1. Potential Null Safety Issue (Medium Priority)

Location: AppTopBar.kt:103

onClick = { scope.launch { drawerState?.open() } },

Issue: While the null check exists, clicking the button when drawerState is null would launch a coroutine that does nothing. This could be confusing for debugging.

Suggestion: Consider adding explicit handling:

onClick = { 
    drawerState?.let { state ->
        scope.launch { state.open() }
    }
}

2. Inconsistent Navigation Pattern (Medium Priority)

Location: DrawerMenu.kt:141-146

onClick = {
    val isOnHome = rootNavController.currentBackStackEntry
        ?.destination?.hasRoute<Routes.Home>() ?: false
    if (!isOnHome) {
        rootNavController.navigateToHome()
    }
    scope.launch { drawerState.close() }
},

Issue: The "Wallet" menu item has special logic to avoid navigating if already on home, but other items use navigateIfNotCurrent(). This inconsistency could lead to different behaviors.

Suggestion: Use the navigateIfNotCurrent() helper consistently for all items, or document why "Wallet" needs special handling.

3. TODO Items Not Implemented (Low Priority)

Location: DrawerMenu.kt:164, 171

onClick = null, // TODO IMPLEMENT CONTACTS
onClick = null, // TODO IMPLEMENT PROFILE

Issue: Menu items are visible but disabled. This might confuse users.

Suggestion: Consider hiding these items until features are implemented, or add a "Coming Soon" indicator in the UI.

4. Navigation Function Not Used Consistently (Low Priority)

Location: ContentView.kt:1429-1435
The new navigateIfNotCurrent() helper is defined but not used in some places where it could prevent duplicate navigation attempts (e.g., in drawer menu items).

5. TabBar Visibility Logic (Low Priority)

Location: ContentView.kt:475-479

val showTabBar = currentRoute in listOf(
    Routes.Home::class.qualifiedName,
    Routes.AllActivity::class.qualifiedName,
)

Issue: Using qualifiedName for route matching is fragile - if routes are refactored or moved, this breaks silently.

Suggestion: Consider using hasRoute<T>() pattern or a more robust route checking mechanism.

🔍 Architecture Considerations

Positive Changes

1. State hoisting: Elevating DrawerState and TabBar to the parent layer (ContentView) is the correct approach for sharing state across the navigation hierarchy
2. AllActivity screen elevation: Moving AllActivityScreen to the top navigation graph allows it to access the drawer, which is architecturally sound
3. Composition locals usage: Using LocalDrawerState follows established patterns in the codebase

Performance

• No major concerns: The drawer state changes shouldn't cause unnecessary recompositions
• Animation performance: The slide-in/out animations and scrim overlay are implemented efficiently
• Good use of remember: Proper memoization in drawer swipe gesture handling

🔒 Security

• No security concerns identified: The changes are purely UI/navigation related
• No sensitive data exposure: Drawer content is appropriate

🧪 Test Coverage

Missing Tests

• No unit tests added for the new navigation patterns or drawer functionality
• UI tests may need updates: Since test tag changed from NavigationClose to HeaderMenu, existing UI tests need verification (QA notes mention this)

Recommendation: Add tests for:

1. Drawer state management (open/close)
2. Navigation from drawer menu items
3. navigateIfNotCurrent() helper function
4. TabBar visibility logic based on current route

Existing Test Impact

• Test tag changes were documented in QA notes, which is good practice
• Verify all UI tests that relied on NavigationClose are updated

📝 Code Quality Notes

Follows Repository Guidelines ✅

• ✅ ViewModels not injected into inappropriate places
• ✅ Proper use of StateFlow for state management
• ✅ Material3 design guidelines followed
• ✅ Modifiers in correct position (last parameter)
• ✅ No hardcoded strings (uses stringResource)
• ✅ Proper Hilt dependency injection patterns
• ✅ Good use of remember blocks for expensive computations
• ✅ Unused code removed after refactoring

Minor Code Quality Issues

1. Magic numbers: drawerWidth = 200.dp and z-index values could be extracted to a theme or constants file
2. Swipe threshold: threshold = 1500f in AllActivityScreen.kt:150 could be a named constant
3. Commit history: 27 commits for this feature - could potentially be squashed for cleaner history (though this is a team preference)

🎯 Recommendations

High Priority

1. Verify all UI tests pass with the NavigationClose → HeaderMenu test tag change
2. Add unit tests for critical navigation logic

Medium Priority

1. Improve null safety handling in DrawerNavIcon
2. Make navigation patterns consistent across drawer menu items
3. Document or hide unimplemented menu items (Contacts, Profile)

Low Priority

1. Extract magic numbers to constants
2. Consider making route visibility logic more robust
3. Add documentation for the drawer state management architecture

✅ Approval Status

This is a solid refactoring that improves UX consistency. The architectural changes are sound and follow Compose best practices. The identified issues are mostly minor and don't block the PR.

Recommendation: ✅ Approve with minor suggestions

The code quality is good, follows repository conventions, and the refactoring is well-executed. Address the test coverage and consider the null safety improvements, but these can be handled in follow-up work if needed.

---

Review generated with assistance from Claude Code

[ovitrif]

tAck, great heavy work 👏🏻