perf(model): share catalog store and show pinned models immediately (#1202)

app/src/test/java/com/m57/hermescontrol/data/ws/ModelCatalogStoreTest.kt

@@ -0,0 +1,222 @@
+package com.m57.hermescontrol.data.ws
+
+import com.m57.hermescontrol.data.local.DataScope
+import com.m57.hermescontrol.data.model.ModelCapabilities
+import com.m57.hermescontrol.data.model.ModelOptionsResponse
+import com.m57.hermescontrol.data.model.ModelProvider
+import com.m57.hermescontrol.data.remote.NetworkResult
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.async
+import kotlinx.coroutines.coroutineScope
+import kotlinx.coroutines.test.TestScope
+import kotlinx.coroutines.test.UnconfinedTestDispatcher
+import kotlinx.coroutines.test.runTest
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+@OptIn(ExperimentalCoroutinesApi::class)
+class ModelCatalogStoreTest {
+    private val testDispatcher = UnconfinedTestDispatcher()
+    private val testScope = TestScope(testDispatcher)
+
+    private val sampleScope =
+        DataScope(
+            connectionProfileId = "conn-1",
+            baseUrl = "http://localhost:9119",
+            activeProfileId = "default",
+            inMemoryAuthGeneration = 1L,
+        )
+
+    private val fakeResponse =
+        ModelOptionsResponse(
+            providers =
+                listOf(
+                    ModelProvider(
+                        slug = "openai",
+                        name = "OpenAI",
+                        models = listOf("gpt-4o"),
+                        capabilities = mapOf("gpt-4o" to ModelCapabilities(fast = true)),
+                    ),
+                ),
+        )
+
+    @Test
+    fun `two concurrent callers share single in-flight load`() =
+        testScope.runTest {
+            var loadCalls = 0
+            val gate = CompletableDeferred<Unit>()
+
+            val repo =
+                ModelOptionsRepository(
+                    connected = { false },
+                    rest = {
+                        loadCalls++
+                        gate.await()
+                        NetworkResult.Success(fakeResponse)
+                    },
+                )
+
+            val store =
+                ModelCatalogStore(
+                    repository = repo,
+                    getCurrentScope = { sampleScope },
+                    scope = testScope,
+                )
+
+            coroutineScope {
+                val deferred1 = async { store.ensureLoaded(forceRefresh = false) }
+                val deferred2 = async { store.ensureLoaded(forceRefresh = false) }
+
+                assertEquals(1, loadCalls)
+                gate.complete(Unit)
+                deferred1.await()
+                deferred2.await()
+            }
+
+            val state = store.state.value
+            assertEquals(1, state.providers.size)
+            assertTrue(state.hasLoaded)
+            assertFalse(state.isRefreshing)
+        }
+
+    @Test
+    fun `fresh cache returns immediately without extra network call`() =
+        testScope.runTest {
+            var loadCalls = 0
+            var now = 1000L
+
+            val repo =
+                ModelOptionsRepository(
+                    connected = { false },
+                    rest = {
+                        loadCalls++
+                        NetworkResult.Success(fakeResponse)
+                    },
+                )
+
+            val store =
+                ModelCatalogStore(
+                    repository = repo,
+                    getCurrentScope = { sampleScope },
+                    clock = { now },
+                    scope = testScope,
+                    ttlMs = 60_000L,
+                )
+
+            store.ensureLoaded(forceRefresh = false)
+            assertEquals(1, loadCalls)
+
+            // Advance clock by 10s (well within 60s TTL)
+            now += 10_000L
+            store.ensureLoaded(forceRefresh = false)
+            assertEquals(1, loadCalls)
+
+            // Force refresh should bypass TTL
+            store.ensureLoaded(forceRefresh = true)
+            assertEquals(2, loadCalls)
+        }
+
+    @Test
+    fun `forced refresh supersedes normal load without cancelling existing callers`() =
+        testScope.runTest {
+            val normalGate = CompletableDeferred<Unit>()
+            val forcedGate = CompletableDeferred<Unit>()
+            val requests = mutableListOf<Boolean>()
+
+            val forcedResponse =
+                ModelOptionsResponse(
+                    providers =
+                        listOf(
+                            ModelProvider(
+                                slug = "anthropic",
+                                name = "Anthropic",
+                                models = listOf("claude-sonnet"),
+                                capabilities = emptyMap(),
+                            ),
+                        ),
+                )
+
+            val repo =
+                ModelOptionsRepository(
+                    connected = { false },
+                    rest = { refresh ->
+                        requests += refresh
+                        if (refresh) {
+                            forcedGate.await()
+                            NetworkResult.Success(forcedResponse)
+                        } else {
+                            normalGate.await()
+                            NetworkResult.Success(fakeResponse)
+                        }
+                    },
+                )
+
+            val store =
+                ModelCatalogStore(
+                    repository = repo,
+                    getCurrentScope = { sampleScope },
+                    scope = testScope,
+                )
+
+            coroutineScope {
+                val normalCaller = async { store.ensureLoaded(forceRefresh = false) }
+                assertEquals(listOf(false), requests)
+
+                val forcedCaller = async { store.ensureLoaded(forceRefresh = true) }
+                assertEquals(listOf(false, true), requests)
+
+                // The superseded physical normal request may finish, but its
+                // callers remain attached to the logical request and wait for
+                // the authoritative forced result.
+                normalGate.complete(Unit)
+                assertFalse(normalCaller.isCancelled)
+                assertFalse(normalCaller.isCompleted)
+
+                forcedGate.complete(Unit)
+
+                val normalResult = normalCaller.await() as NetworkResult.Success
+                val forcedResult = forcedCaller.await() as NetworkResult.Success
+
+                assertEquals(forcedResponse.providers, normalResult.data.providers)
+                assertEquals(forcedResponse.providers, forcedResult.data.providers)
+            }
+
+            assertEquals(forcedResponse.providers, store.state.value.providers)
+            assertTrue(store.state.value.hasLoaded)
+            assertFalse(store.state.value.isRefreshing)
+        }
+
+    @Test
+    fun `scope change invalidates cached state and cancels in-flight work`() =
+        testScope.runTest {
+            var currentScope = sampleScope
+            val repo =
+                ModelOptionsRepository(
+                    connected = { false },
+                    rest = { NetworkResult.Success(fakeResponse) },
+                )
+
+            val store =
+                ModelCatalogStore(
+                    repository = repo,
+                    getCurrentScope = { currentScope },
+                    scope = testScope,
+                )
+
+            store.ensureLoaded(forceRefresh = false)
+            assertTrue(store.state.value.hasLoaded)
+            assertEquals(1, store.state.value.providers.size)
+
+            // Switch scope
+            val otherScope = sampleScope.copy(activeProfileId = "work")
+            currentScope = otherScope
+            store.onScopeChanged(otherScope)
+
+            assertEquals(0, store.state.value.providers.size)
+            assertFalse(store.state.value.hasLoaded)
+            assertEquals(otherScope, store.state.value.scope)
+        }
+}

app/src/test/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegateTest.kt

@@ -1,16 +1,20 @@
 package com.m57.hermescontrol.ui.chat
 
+import com.m57.hermescontrol.data.local.DataScope
 import com.m57.hermescontrol.data.model.ModelCapabilities
 import com.m57.hermescontrol.data.model.ModelOptionsResponse
 import com.m57.hermescontrol.data.model.ModelProvider
 import com.m57.hermescontrol.data.model.PinnedModel
+import com.m57.hermescontrol.data.remote.NetworkError
 import com.m57.hermescontrol.data.remote.NetworkResult
 import com.m57.hermescontrol.data.ws.WsMethods
+import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.test.StandardTestDispatcher
 import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.advanceUntilIdle
+import kotlinx.coroutines.test.runCurrent
 import kotlinx.coroutines.test.runTest
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
@@ -32,6 +36,19 @@ class ChatModelSwitchDelegateTest {
     private var contextRefetched = 0
     private var modelSwitchInitiated = 0
     private var pinnedList = mutableListOf(PinnedModel("anthropic", "claude-3"))
+    private val modelRequests = mutableListOf<Boolean>()
+    private var modelResponseGate: CompletableDeferred<Unit>? = null
+    private var modelFailure: NetworkResult.Failure? = null
+
+    private val initialDataScope =
+        DataScope(
+            connectionProfileId = "conn-1",
+            baseUrl = "http://localhost:9119",
+            activeProfileId = "default",
+            inMemoryAuthGeneration = 1L,
+        )
+
+    private val dataScope = MutableStateFlow<DataScope?>(initialDataScope)
 
     private val fakeResponse =
         ModelOptionsResponse(
@@ -62,7 +79,12 @@ class ChatModelSwitchDelegateTest {
             handleSlashCommand = { slashCommands.add(it) },
             fetchContextUsage = { contextRefetched++ },
             onModelSwitchInitiated = { modelSwitchInitiated++ },
-            getModelOptionsCall = { NetworkResult.Success(fakeResponse) },
+            dataScopeFlow = dataScope,
+            getModelOptionsCall = { refresh ->
+                modelRequests.add(refresh)
+                modelResponseGate?.await()
+                modelFailure ?: NetworkResult.Success(fakeResponse)
+            },
             getPinnedModels = { pinnedList },
             savePinnedModels = { pinnedList = it.toMutableList() },
         )
@@ -88,6 +110,131 @@ class ChatModelSwitchDelegateTest {
             assertFalse(uiState.value.modelPickerLoading)
         }
 
+    @Test
+    fun openModelPicker_duringPreload_reusesRequestAndPublishesResult() =
+        testScope.runTest {
+            modelResponseGate = CompletableDeferred()
+            delegate.preloadModelOptions()
+            runCurrent()
+            delegate.openModelPicker()
+            runCurrent()
+
+            val requestsWhileLoading = modelRequests.toList()
+            modelResponseGate?.complete(Unit)
+            advanceUntilIdle()
+
+            assertEquals(listOf(false), requestsWhileLoading)
+            assertEquals(fakeResponse.providers, uiState.value.modelPickerProviders)
+            assertFalse(uiState.value.modelPickerLoading)
+        }
+
+    @Test
+    fun coldOpen_usesServerCache_andReopenUsesMemoryCache() =
+        testScope.runTest {
+            delegate.openModelPicker()
+            advanceUntilIdle()
+            delegate.closeModelPicker()
+            delegate.openModelPicker()
+            assertFalse(uiState.value.modelPickerLoading)
+            advanceUntilIdle()
+            assertEquals(listOf(false), modelRequests)
+            assertEquals(fakeResponse.providers, uiState.value.modelPickerProviders)
+        }
+
+    @Test
+    fun scopeSwitch_clearsCachedModelsAndReloadsOpenPicker() =
+        testScope.runTest {
+            val scopeObserverJob = kotlinx.coroutines.Job()
+            val observerScope = kotlinx.coroutines.CoroutineScope(testDispatcher + scopeObserverJob)
+            delegate.attachScopeObserver(observerScope)
+
+            delegate.preloadModelOptions()
+            advanceUntilIdle()
+            delegate.openModelPicker()
+
+            assertEquals(fakeResponse.providers, uiState.value.modelPickerProviders)
+            assertFalse(uiState.value.modelPickerLoading)
+
+            modelResponseGate = CompletableDeferred()
+            dataScope.value = initialDataScope.copy(activeProfileId = "work")
+            runCurrent()
+
+            // Previous-scope rows disappear immediately, before the new
+            // catalog request is allowed to complete.
+            assertTrue(uiState.value.showModelPicker)
+            assertTrue(uiState.value.modelPickerProviders.isEmpty())
+            assertTrue(uiState.value.modelPickerLoading)
+            assertEquals(listOf(false, false), modelRequests)
+
+            modelResponseGate?.complete(Unit)
+            advanceUntilIdle()
+
+            assertEquals(fakeResponse.providers, uiState.value.modelPickerProviders)
+            assertFalse(uiState.value.modelPickerLoading)
+
+            scopeObserverJob.cancel()
+        }
+
+    @Test
+    fun closeAndReopenDuringPreload_doesNotDuplicateOrReopenAfterDismiss() =
+        testScope.runTest {
+            modelResponseGate = CompletableDeferred()
+            delegate.preloadModelOptions()
+            runCurrent()
+            delegate.openModelPicker()
+            delegate.closeModelPicker()
+            delegate.openModelPicker()
+            delegate.closeModelPicker()
+            runCurrent()
+            modelResponseGate?.complete(Unit)
+            advanceUntilIdle()
+            assertEquals(listOf(false), modelRequests)
+            assertFalse(uiState.value.showModelPicker)
+            assertFalse(uiState.value.modelPickerLoading)
+            assertEquals(fakeResponse.providers, uiState.value.modelPickerProviders)
+        }
+
+    @Test
+    fun explicitRefresh_supersedesPreload() =
+        testScope.runTest {
+            modelResponseGate = CompletableDeferred()
+            delegate.preloadModelOptions()
+            runCurrent()
+            delegate.refreshModelOptions()
+            runCurrent()
+            modelResponseGate?.complete(Unit)
+            advanceUntilIdle()
+            assertEquals(listOf(false, true), modelRequests)
+            assertEquals(fakeResponse.providers, uiState.value.modelPickerProviders)
+            assertFalse(uiState.value.modelPickerLoading)
+        }
+
+    @Test
+    fun failedPreload_isSilentAndCanRetryOnOpen() =
+        testScope.runTest {
+            modelFailure = NetworkResult.Failure(NetworkError.Http(503, "Unavailable"))
+            delegate.preloadModelOptions()
+            advanceUntilIdle()
+            assertNull(uiState.value.errorMessage)
+            assertFalse(uiState.value.modelPickerLoading)
+
+            modelFailure = null
+            delegate.openModelPicker()
+            advanceUntilIdle()
+            assertEquals(listOf(false, false), modelRequests)
+            assertEquals(fakeResponse.providers, uiState.value.modelPickerProviders)
+        }
+
+    @Test
+    fun failedOpen_clearsLoadingAndReportsError() =
+        testScope.runTest {
+            modelFailure = NetworkResult.Failure(NetworkError.Http(503, "Unavailable"))
+            delegate.openModelPicker()
+            advanceUntilIdle()
+            assertFalse(uiState.value.modelPickerLoading)
+            assertEquals("Failed to load models: Unavailable", uiState.value.errorMessage)
+        }
+
     @Test
     fun sendSlashModel_optimisticallyUpdatesModelAndDispatchesSlash() {
         delegate.sendSlashModel("openai", "gpt-4o")
@@ -261,33 +408,49 @@ class ChatModelSwitchDelegateTest {
                 id = "req-1",
                 result = mapOf("key" to "model", "confirm_required" to true),
             )
-            // Guarded: sequence 1 != current sequence 2, so confirmation is ignored
             assertNull(uiState.value.modelSwitchConfirmMessage)
         }
 
+    @Test
+    fun handleConfigSetError_staleSequence_isIgnored() =
+        testScope.runTest {
+            delegate.sendSlashModel("openai", "gpt-4o")
+            advanceUntilIdle()
+            delegate.handleModelSwitch("/model gpt-4o --provider openai --session")
+            advanceUntilIdle()
+
+            // Second switch arrives
+            delegate.sendSlashModel("anthropic", "claude-3-5-sonnet")
+            advanceUntilIdle()
+            delegate.handleModelSwitch("/model claude-3-5-sonnet --provider anthropic --session")
+            advanceUntilIdle()
+
+            // First switch fails — should not roll back the second switch
+            delegate.handleConfigSetError("req-1", "First switch failed")
+            assertEquals("anthropic/claude-3-5-sonnet", uiState.value.currentSessionModel)
+        }
+
     @Test
     fun rapidPicks_preservesOriginalConfirmedModelForRollback() =
         testScope.runTest {
-            // Initially confirmed model is anthropic/claude-3
-            delegate.onModelConfirmed("anthropic/claude-3")
-            uiState.value = uiState.value.copy(currentSessionModel = "anthropic/claude-3")
-
-            // Pick 1: switch to gpt-4o
+            // Pick model A
             delegate.sendSlashModel("openai", "gpt-4o")
             advanceUntilIdle()
             delegate.handleModelSwitch("/model gpt-4o --provider openai --session")
             advanceUntilIdle()
-            assertEquals("openai/gpt-4o", uiState.value.currentSessionModel)
 
-            // Pick 2: rapidly pick solar without gpt-4o ever confirming
-            delegate.sendSlashModel("nous", "solar")
+            // Pick model B before A confirms
+            delegate.sendSlashModel("anthropic", "claude-3-5-sonnet")
             advanceUntilIdle()
-            delegate.handleModelSwitch("/model solar --provider nous --session")
+            delegate.handleModelSwitch("/model claude-3-5-sonnet --provider anthropic --session")
             advanceUntilIdle()
-            assertEquals("nous/solar", uiState.value.currentSessionModel)
 
-            // Error on second switch (req-2) should roll back to confirmed model, not optimistic gpt-4o
-            delegate.handleConfigSetError("req-2", "Solar failed")
+            // Dismiss confirmation on B — should revert to original model before both switches
+            delegate.handleConfigSetResult(
+                id = "req-2",
+                result = mapOf("key" to "model", "confirm_required" to true),
+            )
+            delegate.dismissModelSwitchConfirm()
             assertEquals("anthropic/claude-3", uiState.value.currentSessionModel)
         }
 }

app/src/test/java/com/m57/hermescontrol/ui/model/ModelViewModelTest.kt

@@ -1,7 +1,13 @@
 package com.m57.hermescontrol.ui.model
 
 import com.m57.hermescontrol.data.local.AuthManager
+import com.m57.hermescontrol.data.local.DataScope
+import com.m57.hermescontrol.data.model.ModelOptionsResponse
+import com.m57.hermescontrol.data.model.ModelProvider
 import com.m57.hermescontrol.data.model.PinnedModel
+import com.m57.hermescontrol.data.remote.NetworkResult
+import com.m57.hermescontrol.data.ws.ModelCatalogStore
+import com.m57.hermescontrol.data.ws.ModelOptionsRepository
 import io.mockk.every
 import io.mockk.mockkObject
 import io.mockk.unmockkAll
@@ -11,6 +17,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.StandardTestDispatcher
 import kotlinx.coroutines.test.advanceUntilIdle
 import kotlinx.coroutines.test.resetMain
+import kotlinx.coroutines.test.runTest
 import kotlinx.coroutines.test.setMain
 import org.junit.After
 import org.junit.Assert.assertEquals
@@ -148,6 +155,57 @@ class ModelViewModelTest {
         verify { AuthManager.savePinnedModels(listOf(PinnedModel("anthropic", "claude-3"))) }
     }
 
+    @Test
+    fun `catalog scope reset clears providers from previous scope`() =
+        runTest(testDispatcher) {
+            var currentScope =
+                DataScope(
+                    connectionProfileId = "conn-1",
+                    baseUrl = "http://localhost:9119",
+                    activeProfileId = "default",
+                    inMemoryAuthGeneration = 1L,
+                )
+
+            val response =
+                ModelOptionsResponse(
+                    providers =
+                        listOf(
+                            ModelProvider(
+                                slug = "openai",
+                                name = "OpenAI",
+                                models = listOf("gpt-4o"),
+                                capabilities = emptyMap(),
+                            ),
+                        ),
+                )
+
+            val store =
+                ModelCatalogStore(
+                    repository =
+                        ModelOptionsRepository(
+                            connected = { false },
+                            rest = { NetworkResult.Success(response) },
+                        ),
+                    getCurrentScope = { currentScope },
+                    scope = this,
+                )
+
+            val viewModel = ModelViewModel(catalogStore = store)
+
+            store.ensureLoaded()
+            advanceUntilIdle()
+            assertEquals(response.providers, viewModel.uiState.value.providers)
+
+            currentScope = currentScope.copy(activeProfileId = "work")
+            store.onScopeChanged(currentScope)
+            advanceUntilIdle()
+
+            assertTrue(
+                viewModel.uiState.value.providers
+                    .isEmpty(),
+            )
+        }
+
     @Test
     fun `pinModel shows toast when max cap reached`() {
         val viewModel = createViewModel()