fix: stop bottom bar resetting on upgrade; make pull-tab a dismissable drawer

Two regressions from this session's audit batch: 1. Bottom nav bar reset. Adding stable @SerialName discriminators to BottomBarEntry changed the persisted polymorphic "type" value from the fully-qualified class name to "builtIn"/"favorite", so configs written by an earlier build no longer decoded — and the fallback returned an empty list, blanking the bar. decodeBottomBarItems now migrates the old fully-qualified discriminators to the short names (recovering the user's customized bar), and any unrecognizable value falls back to the defaults instead of empty. Locked with BottomBarEntrySerializationTest. 2. Pull-down sheet interfered with page taps. The expanded top sheet is a full-width drawer with no way to dismiss except the grabber, so it sat over the page. Hoist its expanded state into EmbeddedTabLayer (reset per tab) and draw a full-area dismiss scrim behind the open sheet; collapsed, only the small grabber is interactive and page taps pass through. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgMpRcWj6y82LxLiwcuzmN
2026-08-08 23:54:39 +00:00 · 2026-06-24 00:32:44 +00:00
parent ba2af75b4c
commit 41843dd8e8
4 changed files with 132 additions and 33 deletions
@@ -218,13 +218,25 @@ class UiSharedPreferences(
            if (raw.isBlank()) return emptyList()
            // Current format: a JSON list of BottomBarEntry (built-ins + favorites).
            runCatching { return JsonMapper.fromJson<List<BottomBarEntry>>(raw) }
-            // Legacy format: comma-joined NavBarItem enum names (before favorites/unified entries).
-            return runCatching {
-                raw
-                    .split(",")
-                    .mapNotNull { name -> runCatching { NavBarItem.valueOf(name) }.getOrNull() }
-                    .map { BottomBarEntry.BuiltIn(it) }
-            }.getOrNull()
+            // Configs written before the stable @SerialName discriminators used the fully-qualified
+            // class name as the polymorphic "type" value. Rewrite it to the short name and retry, so a
+            // customized bar survives the upgrade instead of silently resetting to defaults.
+            runCatching {
+                val migrated =
+                    raw
+                        .replace(LEGACY_BUILTIN_DISCRIMINATOR, "builtIn")
+                        .replace(LEGACY_FAVORITE_DISCRIMINATOR, "favorite")
+                return JsonMapper.fromJson<List<BottomBarEntry>>(migrated)
+            }
+            // Oldest format: comma-joined NavBarItem enum names (before favorites/unified entries).
+            val legacy = raw.split(",").mapNotNull { name -> runCatching { NavBarItem.valueOf(name) }.getOrNull() }
+            if (legacy.isNotEmpty()) return legacy.map { BottomBarEntry.BuiltIn(it) }
+            // Unrecognizable — fall back to the defaults rather than leaving the bar empty.
+            return DefaultBottomBarEntries
        }
+
+        // The pre-@SerialName polymorphic discriminators (fully-qualified class names) for migration.
+        private const val LEGACY_BUILTIN_DISCRIMINATOR = "com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry.BuiltIn"
+        private const val LEGACY_FAVORITE_DISCRIMINATOR = "com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry.Favorite"
    }
 }
@@ -23,6 +23,8 @@ package com.vitorpamplona.amethyst.ui.screen.loggedIn.embed
 import android.os.Build
 import android.view.ViewGroup
 import androidx.annotation.RequiresApi
+import androidx.compose.foundation.clickable
+import androidx.compose.foundation.interaction.MutableInteractionSource
 import androidx.compose.foundation.layout.Box
 import androidx.compose.foundation.layout.absoluteOffset
 import androidx.compose.foundation.layout.fillMaxSize
@@ -126,23 +128,36 @@ fun EmbeddedTabLayer(barFavoriteIds: List<String>) {
        }

        // The active tab's top pull-down sheet, drawn AFTER the surfaces so it sits on top of the
-        // (z-below) surface, anchored to the top of the active tab's reserved bounds.
+        // (z-below) surface, anchored to the top of the active tab's reserved bounds. Its expanded state
+        // is owned here (reset per tab) so we can draw a full-area dismiss scrim behind the open sheet —
+        // while collapsed, only the small grabber is interactive and page taps pass through.
        val chrome = EmbeddedTabHost.activeChrome
        if (chrome != null && bounds.width > 0f && bounds.height > 0f) {
+            var sheetExpanded by remember(activeId) { mutableStateOf(false) }
+
+            if (sheetExpanded) {
+                Box(
+                    Modifier
+                        .fillMaxSize()
+                        .clickable(
+                            interactionSource = remember { MutableInteractionSource() },
+                            indication = null,
+                        ) { sheetExpanded = false },
+                )
+            }
+
            with(density) {
-                // Key on the active tab so the sheet's expand/collapse state resets per tab instead of
-                // a previous tab's open sheet bleeding into the next.
-                key(activeId) {
-                    TopControlSheet(
-                        chrome = chrome,
-                        modifier =
-                            Modifier
-                                .absoluteOffset(
-                                    (bounds.left - layerOrigin.x).toDp(),
-                                    (bounds.top - layerOrigin.y).toDp(),
-                                ).width(bounds.width.toDp()),
-                    )
-                }
+                TopControlSheet(
+                    chrome = chrome,
+                    expanded = sheetExpanded,
+                    onExpandedChange = { sheetExpanded = it },
+                    modifier =
+                        Modifier
+                            .absoluteOffset(
+                                (bounds.left - layerOrigin.x).toDp(),
+                                (bounds.top - layerOrigin.y).toDp(),
+                            ).width(bounds.width.toDp()),
+                )
            }
        }
    }
@@ -46,8 +46,6 @@ import androidx.compose.material3.Switch
 import androidx.compose.material3.Text
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.getValue
-import androidx.compose.runtime.mutableStateOf
-import androidx.compose.runtime.remember
 import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
@@ -69,10 +67,10 @@ import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols
@Composable
 fun TopControlSheet(
    chrome: EmbeddedTabChrome,
+    expanded: Boolean,
+    onExpandedChange: (Boolean) -> Unit,
    modifier: Modifier = Modifier,
 ) {
-    var expanded by remember { mutableStateOf(false) }
-
    Column(
        modifier = modifier.fillMaxWidth(),
        horizontalAlignment = Alignment.CenterHorizontally,
@@ -115,40 +113,41 @@ fun TopControlSheet(
                        )
                    }
                    SheetItem(MaterialSymbols.Refresh, stringResource(R.string.browser_reload)) {
-                        expanded = false
+                        onExpandedChange(false)
                        chrome.onReload()
                    }
                    chrome.onInfo?.let { info ->
                        SheetItem(MaterialSymbols.Info, stringResource(R.string.favorite_app_access_show)) {
-                            expanded = false
+                            onExpandedChange(false)
                            info()
                        }
                    }
                    SheetItem(MaterialSymbols.OpenInFull, stringResource(R.string.favorite_app_open_window)) {
-                        expanded = false
+                        onExpandedChange(false)
                        chrome.onOpenFull()
                    }
                }
            }
        }

-        // The grabber: a small rounded bar centered at the top edge. Pull down to open, up to close;
-        // tapping toggles. Wrapped in padding so the touch target is comfortable despite the thin bar.
+        // The grabber: a small rounded bar centered at the top edge. It is the ONLY touch target the sheet
+        // draws — the rest of the top strip stays transparent so page taps pass straight through to the
+        // surface below. Pull down to open, up to close; tapping toggles.
        Column(
            horizontalAlignment = Alignment.CenterHorizontally,
            modifier =
                Modifier
                    .clip(RoundedCornerShape(bottomStart = 12.dp, bottomEnd = 12.dp))
                    .background(MaterialTheme.colorScheme.surface.copy(alpha = 0.6f))
-                    .clickable { expanded = !expanded }
+                    .clickable { onExpandedChange(!expanded) }
                    .draggable(
                        orientation = Orientation.Vertical,
                        state =
                            rememberDraggableState { delta ->
                                if (delta > 1f) {
-                                    expanded = true
+                                    onExpandedChange(true)
                                } else if (delta < -1f) {
-                                    expanded = false
+                                    onExpandedChange(false)
                                }
                            },
                    ).padding(horizontal = 16.dp, vertical = 7.dp),
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2025 Vitor Pamplona
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
+ * Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
+ * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+package com.vitorpamplona.amethyst.navigation
+
+import com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry
+import com.vitorpamplona.amethyst.ui.navigation.bottombars.NavBarItem
+import com.vitorpamplona.quartz.nip01Core.core.JsonMapper
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * Locks the persisted bottom-bar format. [BottomBarEntry] uses stable `@SerialName` discriminators
+ * (`builtIn` / `favorite`) so the saved config survives class renames/moves. Configs written before the
+ * stable names used the fully-qualified class name as the polymorphic discriminator, so this also pins
+ * the migration that recovers them — otherwise upgrading silently resets the user's bottom bar.
+ */
+class BottomBarEntrySerializationTest {
+    private val sample =
+        listOf(
+            BottomBarEntry.BuiltIn(NavBarItem.HOME),
+            BottomBarEntry.Favorite("url:https://example.com"),
+        )
+
+    @Test
+    fun roundTripsWithStableDiscriminators() {
+        val json = JsonMapper.toJson(sample)
+        // Stable short names, NOT the fragile fully-qualified class name.
+        assertTrue("expected stable discriminators, got: $json", json.contains("\"builtIn\"") && json.contains("\"favorite\""))
+        assertEquals(sample, JsonMapper.fromJson<List<BottomBarEntry>>(json))
+    }
+
+    @Test
+    fun legacyFullyQualifiedDiscriminatorMigratesInsteadOfResetting() {
+        // What an earlier build of this branch persisted: the default polymorphic discriminator is the
+        // fully-qualified class name. Under the stable @SerialName this no longer decodes directly...
+        val legacy =
+            """
+            [
+              {"type":"com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry.BuiltIn","item":"HOME"},
+              {"type":"com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry.Favorite","favoriteId":"url:https://example.com"}
+            ]
+            """.trimIndent()
+
+        runCatching { JsonMapper.fromJson<List<BottomBarEntry>>(legacy) }
+            .onSuccess { error("legacy discriminator unexpectedly decoded directly: $it") }
+
+        // ...but the migration (rewrite FQN -> short name) recovers the exact same config.
+        val migrated =
+            legacy
+                .replace("com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry.BuiltIn", "builtIn")
+                .replace("com.vitorpamplona.amethyst.ui.navigation.bottombars.BottomBarEntry.Favorite", "favorite")
+        assertEquals(sample, JsonMapper.fromJson<List<BottomBarEntry>>(migrated))
+    }
+}