fix(ux): keep Settings highlighted in bottom nav on drill-down sub-pages (issue #637) (#643)

* fix(ux): keep Settings highlighted in bottom nav on drill-down sub-pages (issue #637)

Map Settings drill-down destinations (SettingsConnection, SettingsAppearance, SettingsChat, SettingsNavBar, SettingsBehavior, SettingsAbout) back to SettingsScreen for bottom-nav selection only - navigation is unaffected.

- Add selectedBottomNavDestination() helper in Navigation.kt
- Extract bar-height logic into Modifier.bottomNavigationHeight() (ICON_AND_TEXT now uses heightIn(min=80.dp) so the standard bar can grow)
- BottomNavigationSelectionTest (JVM): selection mapping logic
- BottomNavigationLayoutTest (androidTest): per-mode bar height assertions

* docs(navigation): warn that new Settings sub-pages must be added to settingsDestinations

Addresses agy review (PR #643): documents the fragility of the hardcoded settingsDestinations set so future Settings drill-down screens don't silently drop the bottom-nav highlight. The sealed-class/marker-interface refactor was declined as scope creep (issue #637 explicitly scopes Settings-only; keys are flat data objects).

* fix: language selector

---------

Co-authored-by: M57 <Hy4ri@users.noreply.github.com>

app/src/androidTest/java/com/m57/hermescontrol/BottomNavigationLayoutTest.kt

@@ -0,0 +1,60 @@
+package com.m57.hermescontrol
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.platform.testTag
+import androidx.compose.ui.test.assertHeightIsAtLeast
+import androidx.compose.ui.test.assertHeightIsEqualTo
+import androidx.compose.ui.test.junit4.createComposeRule
+import androidx.compose.ui.test.onNodeWithTag
+import androidx.compose.ui.unit.dp
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.filters.MediumTest
+import com.m57.hermescontrol.theme.BottomNavDisplayMode
+import org.junit.Rule
+import org.junit.Test
+import org.junit.runner.RunWith
+
+/**
+ * Instrumented tests for [Modifier.bottomNavigationHeight].
+ *
+ * Each compact bottom-nav display mode maps to a specific bar height:
+ *  - [BottomNavDisplayMode.ICON_ONLY]      -> 56.dp (fixed)
+ *  - [BottomNavDisplayMode.TEXT_ONLY]      -> 44.dp (fixed)
+ *  - [BottomNavDisplayMode.ICON_AND_TEXT]  -> min 80.dp (can grow)
+ *
+ * We apply the modifier to a tagged [Box] and assert the resulting height on
+ * a real Compose host (this requires the instrumentation test runner, hence
+ * androidTest rather than the Robolectric-free JVM test source set).
+ */
+@RunWith(AndroidJUnit4::class)
+@MediumTest
+class BottomNavigationLayoutTest {
+    @get:Rule
+    val composeTestRule = createComposeRule()
+
+    @Test
+    fun iconOnly_barHeightIs56dp() {
+        composeTestRule.setContent {
+            Box(Modifier.testTag("bar").bottomNavigationHeight(BottomNavDisplayMode.ICON_ONLY))
+        }
+        composeTestRule.onNodeWithTag("bar").assertHeightIsEqualTo(56.dp)
+    }
+
+    @Test
+    fun textOnly_barHeightIs44dp() {
+        composeTestRule.setContent {
+            Box(Modifier.testTag("bar").bottomNavigationHeight(BottomNavDisplayMode.TEXT_ONLY))
+        }
+        composeTestRule.onNodeWithTag("bar").assertHeightIsEqualTo(44.dp)
+    }
+
+    @Test
+    fun iconAndText_barHeightIsAtLeast80dp() {
+        composeTestRule.setContent {
+            Box(Modifier.testTag("bar").bottomNavigationHeight(BottomNavDisplayMode.ICON_AND_TEXT))
+        }
+        // heightIn(min = 80.dp) — assert the measured height meets the minimum.
+        composeTestRule.onNodeWithTag("bar").assertHeightIsAtLeast(80.dp)
+    }
+}

app/src/main/java/com/m57/hermescontrol/Navigation.kt

@@ -8,6 +8,7 @@ import androidx.compose.foundation.layout.WindowInsets
 import androidx.compose.foundation.layout.consumeWindowInsets
 import androidx.compose.foundation.layout.fillMaxSize
 import androidx.compose.foundation.layout.height
+import androidx.compose.foundation.layout.heightIn
 import androidx.compose.foundation.layout.navigationBars
 import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.layout.size
@@ -68,6 +69,37 @@ import com.m57.hermescontrol.ui.authlogin.AuthLoginScreen as AuthLoginScreenCont
 import com.m57.hermescontrol.ui.landing.LandingScreen as LandingScreenContent
 import com.m57.hermescontrol.ui.pairing.PairingCodeEntryScreen as PairingCodeEntryScreenContent
 
+// NOTE: if a new Settings drill-down screen is added, its NavKey MUST be added
+// here too, or the bottom-nav Settings highlight will drop on that sub-page.
+// (Keys are flat data objects, not a sealed hierarchy, so no type-based check
+// is possible without a larger refactor — see issue #637.)
+private val settingsDestinations: Set<NavKey> =
+    setOf(
+        SettingsScreen,
+        SettingsConnection,
+        SettingsAppearance,
+        SettingsChat,
+        SettingsNavBar,
+        SettingsBehavior,
+        SettingsAbout,
+    )
+
+/**
+ * Keep Settings highlighted in the bottom nav while one of its drill-down
+ * sub-pages is visible. Navigation is unaffected — this only maps the
+ * *selection* key back to the Settings parent so the indicator doesn't drop.
+ */
+internal fun selectedBottomNavDestination(currentScreen: NavKey): NavKey =
+    if (currentScreen in settingsDestinations) SettingsScreen else currentScreen
+
+/** Maps a compact bottom-nav display mode to its bar height. */
+internal fun Modifier.bottomNavigationHeight(displayMode: BottomNavDisplayMode): Modifier =
+    when (displayMode) {
+        BottomNavDisplayMode.ICON_ONLY -> height(56.dp)
+        BottomNavDisplayMode.TEXT_ONLY -> height(44.dp)
+        BottomNavDisplayMode.ICON_AND_TEXT -> heightIn(min = 80.dp)
+    }
+
 private fun resolveBottomNavItems(names: List<String>): List<ScreenDefinition> =
     names.mapNotNull { name -> ScreenRegistry.ALL_SCREENS.firstOrNull { it.key::class.simpleName == name } }
 
@@ -173,6 +205,7 @@ fun MainNavigation(sessionId: String? = null) {
     NavigationController.backStack = backStack
 
     val currentScreen = backStack.lastOrNull() ?: startScreen
+    val selectedDestination = selectedBottomNavDestination(currentScreen)
     val drawerState = rememberDrawerState(initialValue = DrawerValue.Closed)
     val scope = rememberCoroutineScope()
 
@@ -298,14 +331,8 @@ fun MainNavigation(sessionId: String? = null) {
                 contentWindowInsets = WindowInsets.navigationBars,
                 bottomBar = {
                     if (showBottomBar) {
-                        val barHeight =
-                            when (bottomNavDisplayMode) {
-                                BottomNavDisplayMode.ICON_ONLY -> 56.dp
-                                BottomNavDisplayMode.TEXT_ONLY -> 44.dp
-                                BottomNavDisplayMode.ICON_AND_TEXT -> 80.dp
-                            }
                         NavigationBar(
-                            modifier = Modifier.height(barHeight),
+                            modifier = Modifier.bottomNavigationHeight(bottomNavDisplayMode),
                         ) {
                             bottomNavItems.forEach { item ->
                                 val showIcon =
@@ -315,7 +342,7 @@ fun MainNavigation(sessionId: String? = null) {
                                     bottomNavDisplayMode == BottomNavDisplayMode.ICON_AND_TEXT ||
                                         bottomNavDisplayMode == BottomNavDisplayMode.TEXT_ONLY
 
-                                val isSelected = currentScreen == item.key
+                                val isSelected = selectedDestination == item.key
 
                                 NavigationBarItem(
                                     selected = isSelected,

app/src/test/java/com/m57/hermescontrol/BottomNavigationSelectionTest.kt

@@ -0,0 +1,59 @@
+package com.m57.hermescontrol
+
+import androidx.navigation3.runtime.NavKey
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+/**
+ * Verifies that the bottom-nav selection key maps Settings drill-down
+ * destinations back to [SettingsScreen] so the indicator stays highlighted,
+ * while every other screen is returned unchanged.
+ */
+class BottomNavigationSelectionTest {
+    @Test
+    fun `SettingsScreen selects itself`() {
+        assertEquals(SettingsScreen, selectedBottomNavDestination(SettingsScreen))
+    }
+
+    @Test
+    fun `every Settings drill-down maps back to SettingsScreen`() {
+        val drillDowns: List<NavKey> =
+            listOf(
+                SettingsConnection,
+                SettingsAppearance,
+                SettingsChat,
+                SettingsNavBar,
+                SettingsBehavior,
+                SettingsAbout,
+            )
+        for (destination in drillDowns) {
+            assertEquals(
+                "Expected $destination to map to SettingsScreen",
+                SettingsScreen,
+                selectedBottomNavDestination(destination),
+            )
+        }
+    }
+
+    @Test
+    fun `non-Settings screens are returned unchanged`() {
+        val others: List<NavKey> =
+            listOf(
+                ChatScreen,
+                CronJobsScreen,
+                WebhooksScreen,
+                GatewayScreen,
+                SkillsScreen,
+                LandingScreen,
+                AuthLoginScreen,
+                PairingCodeEntryScreen,
+            )
+        for (screen in others) {
+            assertEquals(
+                "Expected $screen to be returned unchanged",
+                screen,
+                selectedBottomNavDestination(screen),
+            )
+        }
+    }
+}