From 5bfa70d276a6843cc901bf5322c87c0dce55c35e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 20:23:39 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20search=20field=20audit=20=E2=80=94=20typ?= =?UTF-8?q?ed=20text=20rollback,=20blank=20chips,=20recomposition?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Desktop search screen: the field<->query sync compared the field to the displayText the effect was keyed on, which arrives through collectAsState a frame late. Two keystrokes between frames saw the field ahead of a stale key and setText() rolled the second one back. Compare against the query's current value instead; the binding moves into BindSearchField so a test drives the real code (reproduced failing: expected but was ). - A blank display/group/scope name no longer replaces the token: an empty profile name used to erase the chip entirely. - TokenizedSearchField only recomposes when the active picker changes, not on every keystroke, now that typing no longer needs recomposition. - The output transformation is keyed on the field state too, so a new SearchFieldState is never read through a stale caret lambda. - replaceRuns compares in place instead of allocating a substring. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013ESr8Pkt6WJsVkD5dUZEw6 --- .../ui/search/SearchTokenTransformation.kt | 9 ++- .../commons/ui/search/TokenizedSearchField.kt | 8 +- .../search/SearchTokenTransformationTest.kt | 9 +++ .../amethyst/desktop/ui/SearchScreen.kt | 28 +++++-- .../desktop/ui/search/SearchSyncRaceTest.kt | 77 +++++++++++++++++++ 5 files changed, 119 insertions(+), 12 deletions(-) create mode 100644 desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/ui/search/SearchSyncRaceTest.kt diff --git a/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/search/SearchTokenTransformation.kt b/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/search/SearchTokenTransformation.kt index 5ca00a7cb0..90bf807e4c 100644 --- a/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/search/SearchTokenTransformation.kt +++ b/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/search/SearchTokenTransformation.kt @@ -58,7 +58,8 @@ internal fun TextFieldBuffer.replaceRuns( ) { for (i in runs.indices.reversed()) { val run = runs[i] - if (!run.drawn.contentEquals(typed.subSequence(run.start, run.end))) replace(run.start, run.end, run.drawn) + val unchanged = run.drawn.length == run.end - run.start && typed.regionMatches(run.start, run.drawn, 0, run.drawn.length) + if (!unchanged) replace(run.start, run.end, run.drawn) } } @@ -129,7 +130,7 @@ class SearchTokenTransformation( ): String = when (seg) { is SearchSegment.Key -> { - val name = displayName(seg.pubkey) + val name = displayName(seg.pubkey)?.takeIf { it.isNotBlank() } val shown = name?.let { clip(it, 32) } ?: shortBech32(rawKeyOf(raw)) seg.field?.let { "${it.token}:$shown" } ?: shown } @@ -137,9 +138,9 @@ class SearchTokenTransformation( is SearchSegment.Pointer -> "to:${shortBech32(raw.substringAfter(':'))}" // A group id is a stranger's opaque string; its name is the only part a reader can // check against the room they meant. The id stays the value, so the query is unchanged. - is SearchSegment.Group -> "group:${groupName(seg.id)?.let { clip(it, 32) } ?: seg.id}" + is SearchSegment.Group -> "group:${groupName(seg.id)?.takeIf { it.isNotBlank() }?.let { clip(it, 32) } ?: seg.id}" // Likewise a geohash: "9q8yy" says nothing, "San Francisco" says what was filtered on. - is SearchSegment.Scope -> scopeName(seg.field, seg.value)?.let { "${seg.field}:${clip(it, 32)}" } ?: raw + is SearchSegment.Scope -> scopeName(seg.field, seg.value)?.takeIf { it.isNotBlank() }?.let { "${seg.field}:${clip(it, 32)}" } ?: raw // A kind typed as a number draws under the name the registry has for it, so a screen // that seeds `kind:20` shows "kind:picture" — the only form a reader can check. Only // an exact one-token match is used: `kind:30312` must not draw as the wider `live`. diff --git a/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/search/TokenizedSearchField.kt b/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/search/TokenizedSearchField.kt index c109034c58..f50efa7a6a 100644 --- a/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/search/TokenizedSearchField.kt +++ b/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/search/TokenizedSearchField.kt @@ -35,6 +35,7 @@ import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.remember @@ -117,7 +118,10 @@ fun TokenizedSearchField( decorationBox: (@Composable (@Composable () -> Unit) -> Unit)? = null, ) { val styles = rememberSearchTokenStyles() - val picker = state.activePicker + // Derived, so typing that leaves the picker as it was (plain words, a caret moving through + // them) does not recompose the field and everything under it on every keystroke. + val pickerState = remember(state) { derivedStateOf { state.activePicker } } + val picker = pickerState.value var highlighted by rememberSaveable(picker?.token?.start) { mutableIntStateOf(0) } // The one picker whose rows this component fills in itself: the kind vocabulary is a @@ -159,7 +163,7 @@ fun TokenizedSearchField( cursorBrush = SolidColor(MaterialTheme.colorScheme.primary), // The caret is read inside the transformation, not keyed on here: the field re-runs it // whenever a state it read changes, so a new instance per caret move is not needed. - outputTransformation = remember(styles, displayName, groupName, scopeName) { SearchTokenTransformation(state::settleCaret, styles, displayName, groupName, scopeName) }, + outputTransformation = remember(state, styles, displayName, groupName, scopeName) { SearchTokenTransformation(state::settleCaret, styles, displayName, groupName, scopeName) }, decorator = TextFieldDecorator { inner -> if (decorationBox != null) { diff --git a/commonsUI/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/ui/search/SearchTokenTransformationTest.kt b/commonsUI/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/ui/search/SearchTokenTransformationTest.kt index 7600f7ffa4..517808bdd0 100644 --- a/commonsUI/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/ui/search/SearchTokenTransformationTest.kt +++ b/commonsUI/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/ui/search/SearchTokenTransformationTest.kt @@ -220,4 +220,13 @@ class SearchTokenTransformationTest { assertEquals("hello world", transform("\"hello world")) assertAppliesCleanly("\"hello world") } + + @Test + fun aBlankNameIsNotAName() { + // A profile whose name is blank must not erase its chip: it keeps the short npub instead. + val out = transform(NPUB, name = { " " }) + assertTrue(out.startsWith("npub1") && out.contains("…"), out) + assertEquals("group:abc123", transform("group:abc123", groupName = { "" })) + assertEquals("geo:9q8yy", transform("geo:9q8yy", scopeName = { _, _ -> " " })) + } } diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/SearchScreen.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/SearchScreen.kt index 8a9eabe17b..e8e1157560 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/SearchScreen.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/SearchScreen.kt @@ -154,12 +154,7 @@ fun SearchScreen( val searchRelays by relayCategories.searchRelays.collectAsState() val displayText by state.displayText.collectAsState() - // The field drives the query. The reverse direction only fires for a form-driven change (the - // advanced panel, a loaded saved search), and the guard is what keeps the two from looping. - LaunchedEffect(fieldState.text) { state.updateFromText(fieldState.text) } - LaunchedEffect(displayText) { - if (fieldState.text != displayText) fieldState.setText(displayText) - } + BindSearchField(fieldState, state, displayText) // The people a half-written `from:`/`to:` token offers. Cache first, then the search relays. val userSearch = @@ -1003,3 +998,24 @@ private fun SearchResultCard( } } } + +/** + * Keeps the field and the query in step. The field drives the query; the reverse direction only + * fires for a form-driven change (the advanced panel, a loaded saved search). + * + * The guard compares against the query's *current* text, not the [displayText] this effect was + * keyed on: that value arrives through `collectAsState` a frame late, so two keystrokes landing + * between frames would otherwise see the field ahead of a stale key and roll the second one back. + */ +@Composable +internal fun BindSearchField( + fieldState: SearchFieldState, + state: AdvancedSearchBarState, + displayText: String, +) { + LaunchedEffect(fieldState.text) { state.updateFromText(fieldState.text) } + LaunchedEffect(displayText) { + val current = state.displayText.value + if (fieldState.text != current) fieldState.setText(current) + } +} diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/ui/search/SearchSyncRaceTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/ui/search/SearchSyncRaceTest.kt new file mode 100644 index 0000000000..9fdc815143 --- /dev/null +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/ui/search/SearchSyncRaceTest.kt @@ -0,0 +1,77 @@ +/* + * 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.desktop.ui.search + +import androidx.compose.foundation.text.input.insert +import androidx.compose.runtime.collectAsState +import androidx.compose.runtime.getValue +import androidx.compose.ui.test.junit4.v2.createComposeRule +import com.vitorpamplona.amethyst.commons.search.AdvancedSearchBarState +import com.vitorpamplona.amethyst.commons.ui.search.SearchFieldState +import com.vitorpamplona.amethyst.desktop.ui.BindSearchField +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import org.junit.Rule +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * The desktop search screen's two-way binding between the field and the query, driven frame by + * frame so two keystrokes can land between one frame and the next. + */ +class SearchSyncRaceTest { + @get:Rule + val compose = createComposeRule() + + @Test + fun twoKeystrokesBetweenFramesAreNotRolledBack() { + val field = SearchFieldState() + val state = AdvancedSearchBarState(CoroutineScope(SupervisorJob() + Dispatchers.Unconfined)) + compose.mainClock.autoAdvance = false + compose.setContent { + val displayText by state.displayText.collectAsState() + BindSearchField(field, state, displayText) + } + compose.mainClock.advanceTimeByFrame() + field.textState.edit { insert(length, "a") } + compose.mainClock.advanceTimeByFrame() + field.textState.edit { insert(length, "b") } + repeat(5) { compose.mainClock.advanceTimeByFrame() } + assertEquals("ab", field.text) + } + + @Test + fun aFormDrivenChangeStillReachesTheField() { + val field = SearchFieldState("bitcoin") + val state = AdvancedSearchBarState(CoroutineScope(SupervisorJob() + Dispatchers.Unconfined)) + compose.setContent { + val displayText by state.displayText.collectAsState() + BindSearchField(field, state, displayText) + } + compose.waitForIdle() + state.updateKinds(listOf(1)) + compose.waitForIdle() + assertEquals(state.displayText.value, field.text) + assertTrue(field.text.contains("kind:"), field.text) + } +}