From 83fb738ccdee717f542d08f16ab9e8ede8cd3250 Mon Sep 17 00:00:00 2001 From: m57 <41645758+Hy4ri@users.noreply.github.com> Date: Sun, 20 Sep 2026 12:31:35 +0300 Subject: [PATCH] perf(model): share catalog store and show pinned models immediately (#1202) --- .../model/components/ModelPickerDialogTest.kt | 19 ++ .../data/ws/ModelCatalogStore.kt | 228 ++++++++++++++++++ .../ui/chat/ChatModelSwitchDelegate.kt | 149 +++++++++--- .../hermescontrol/ui/chat/ChatViewModel.kt | 4 +- .../m57/hermescontrol/ui/model/ModelScreen.kt | 9 +- .../hermescontrol/ui/model/ModelViewModel.kt | 32 ++- .../ui/model/components/ModelPickerDialog.kt | 2 +- .../data/ws/ModelCatalogStoreTest.kt | 222 +++++++++++++++++ .../ui/chat/ChatModelSwitchDelegateTest.kt | 191 +++++++++++++-- .../ui/model/ModelViewModelTest.kt | 58 +++++ 10 files changed, 853 insertions(+), 61 deletions(-) create mode 100644 app/src/main/java/com/m57/hermescontrol/data/ws/ModelCatalogStore.kt create mode 100644 app/src/test/java/com/m57/hermescontrol/data/ws/ModelCatalogStoreTest.kt diff --git a/app/src/androidTest/java/com/m57/hermescontrol/ui/model/components/ModelPickerDialogTest.kt b/app/src/androidTest/java/com/m57/hermescontrol/ui/model/components/ModelPickerDialogTest.kt index cfffcad7..6849356a 100644 --- a/app/src/androidTest/java/com/m57/hermescontrol/ui/model/components/ModelPickerDialogTest.kt +++ b/app/src/androidTest/java/com/m57/hermescontrol/ui/model/components/ModelPickerDialogTest.kt @@ -26,6 +26,25 @@ class ModelPickerDialogTest { @get:Rule val composeTestRule = createComposeRule() + @Test + fun pinnedModels_areSelectableWhileCatalogLoads() { + var selectedModel = "" + composeTestRule.setContent { + HermesControlTheme { + ModelPickerDialog( + providers = emptyList(), + title = "Choose model", + isLoading = true, + pinnedModels = listOf(PinnedModel("openai", "gpt-4")), + onSelect = { _, model -> selectedModel = model }, + onDismiss = {}, + ) + } + } + composeTestRule.onNodeWithText("gpt-4").assertIsDisplayed().performClick() + assertEquals("gpt-4", selectedModel) + } + @Test fun providers_startCollapsed_andOnlyOneProviderExpands() { composeTestRule.setContent { diff --git a/app/src/main/java/com/m57/hermescontrol/data/ws/ModelCatalogStore.kt b/app/src/main/java/com/m57/hermescontrol/data/ws/ModelCatalogStore.kt new file mode 100644 index 00000000..e4e16e79 --- /dev/null +++ b/app/src/main/java/com/m57/hermescontrol/data/ws/ModelCatalogStore.kt @@ -0,0 +1,228 @@ +package com.m57.hermescontrol.data.ws + +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.remote.NetworkError +import com.m57.hermescontrol.data.remote.NetworkResult +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.update +import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock + +data class ModelCatalogState( + val scope: DataScope? = null, + val providers: List = emptyList(), + val hasLoaded: Boolean = false, + val isRefreshing: Boolean = false, + val error: NetworkError? = null, +) + +/** + * Shared in-memory catalog cache partitioned by [DataScope]. + * Deduplicates in-flight catalog loads and shares parsed [ModelProvider]s + * across Chat and Model screens. + */ +class ModelCatalogStore( + private val repository: ModelOptionsRepository = ModelOptionsRepository(), + private val getCurrentScope: () -> DataScope? = { AuthManager.currentDataScope() }, + private val clock: () -> Long = { System.currentTimeMillis() }, + private val scope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate), + private val ttlMs: Long = 60_000L, +) { + private val _state = MutableStateFlow(ModelCatalogState()) + val state: StateFlow = _state.asStateFlow() + + private val mutex = Mutex() + private var lastLoadedTime: Long = 0L + private var activeResult: CompletableDeferred>? = null + private val inFlightFetches = mutableListOf() + private var activeIsForced: Boolean = false + private var activeGeneration: Long = 0L + + fun attachScopeObserver( + externalScope: CoroutineScope, + dataScopeFlow: Flow, + ) { + externalScope.launch { + dataScopeFlow.collect { newScope -> + onScopeChanged(newScope) + } + } + } + + suspend fun onScopeChanged(newScope: DataScope?) { + mutex.withLock { + if (_state.value.scope != newScope) { + resetForScopeLocked(newScope) + } + } + } + + /** + * Invalidates all work belonging to the previous data scope. + * + * Unlike force-refresh supersession, a scope switch really does make the + * old network work invalid, so those fetches are cancelled. + */ + private fun resetForScopeLocked(newScope: DataScope?) { + inFlightFetches.forEach { it.cancel() } + inFlightFetches.clear() + + activeResult?.cancel() + activeResult = null + activeIsForced = false + activeGeneration++ + lastLoadedTime = 0L + _state.value = ModelCatalogState(scope = newScope) + } + + /** + * Starts one physical catalog request for the current logical load. + * + * Multiple callers await [resultGate]. A forced refresh may supersede a + * normal physical request without cancelling that gate, so callers already + * waiting on the normal request transparently receive the forced result. + */ + private fun startFetchLocked( + refresh: Boolean, + requestScope: DataScope?, + generation: Long, + resultGate: CompletableDeferred>, + ) { + inFlightFetches.removeAll { it.isCompleted } + inFlightFetches += + scope.launch { + val result = + try { + repository.load(refresh = refresh) + } catch (e: CancellationException) { + throw e + } catch (t: Throwable) { + NetworkResult.Failure( + NetworkError.Unknown(t.message ?: "Unknown error", t), + ) + } + + mutex.withLock { + // A newer forced request or scope switch owns publication. + if ( + generation != activeGeneration || + _state.value.scope != requestScope || + activeResult !== resultGate + ) { + return@withLock + } + + activeResult = null + activeIsForced = false + + when (result) { + is NetworkResult.Success -> { + lastLoadedTime = clock() + _state.update { + it.copy( + providers = result.data.providers.orEmpty(), + hasLoaded = true, + isRefreshing = false, + error = null, + ) + } + } + + is NetworkResult.Failure -> { + _state.update { + it.copy( + isRefreshing = false, + error = result.error, + ) + } + } + } + + // Complete only from the authoritative generation. + // Every consumer of this logical load receives the same result. + resultGate.complete(result) + } + } + } + + suspend fun ensureLoaded(forceRefresh: Boolean = false): NetworkResult { + val currentScope = getCurrentScope() + + val resultGate = + mutex.withLock { + // Scope switch check + if (_state.value.scope != currentScope) { + resetForScopeLocked(currentScope) + } + + val now = clock() + val isFresh = _state.value.hasLoaded && (now - lastLoadedTime < ttlMs) + + if (!forceRefresh && isFresh) { + return NetworkResult.Success(ModelOptionsResponse(_state.value.providers)) + } + + val existing = activeResult + if (existing?.isActive == true) { + if (forceRefresh && !activeIsForced) { + // Supersede the physical request, but deliberately keep + // the shared result gate alive. Existing consumers must + // not be cancelled just because another screen refreshed. + val generation = ++activeGeneration + activeIsForced = true + _state.update { it.copy(isRefreshing = true) } + startFetchLocked( + refresh = true, + requestScope = currentScope, + generation = generation, + resultGate = existing, + ) + } + + existing + } else { + val generation = ++activeGeneration + val newResult = CompletableDeferred>() + + activeResult = newResult + activeIsForced = forceRefresh + _state.update { it.copy(isRefreshing = true) } + + startFetchLocked( + refresh = forceRefresh, + requestScope = currentScope, + generation = generation, + resultGate = newResult, + ) + + newResult + } + } + + return resultGate.await() + } + + companion object { + val shared by lazy { + val store = ModelCatalogStore() + store.attachScopeObserver( + CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate), + AuthManager.dataScopeFlow, + ) + store + } + } +} diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegate.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegate.kt index 3d440021..9ccfcda5 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegate.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegate.kt @@ -1,18 +1,23 @@ package com.m57.hermescontrol.ui.chat import com.m57.hermescontrol.data.local.AuthManager +import com.m57.hermescontrol.data.local.DataScope import com.m57.hermescontrol.data.model.ModelCapabilities import com.m57.hermescontrol.data.model.ModelProvider import com.m57.hermescontrol.data.model.PinnedModel import com.m57.hermescontrol.data.remote.ApiClient import com.m57.hermescontrol.data.remote.NetworkResult import com.m57.hermescontrol.data.remote.safeApiCall +import com.m57.hermescontrol.data.ws.ModelCatalogStore import com.m57.hermescontrol.data.ws.WsMethods import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import java.util.concurrent.ConcurrentHashMap /** @@ -46,10 +51,11 @@ class ChatModelSwitchDelegate( private val handleSlashCommand: (command: String) -> Unit, private val fetchContextUsage: () -> Unit, private val onModelSwitchInitiated: () -> Unit = {}, + private val dataScopeFlow: StateFlow = AuthManager.dataScopeFlow, private val getModelOptionsCall: suspend ( refresh: Boolean, ) -> NetworkResult = - { refresh -> safeApiCall { ApiClient.hermesApi.getModelOptions(refresh = refresh) } }, + { refresh -> ModelCatalogStore.shared.ensureLoaded(forceRefresh = refresh) }, private val getPinnedModels: () -> List = { AuthManager.getPinnedModels() }, private val savePinnedModels: (List) -> Unit = { AuthManager.savePinnedModels(it) }, ) { @@ -62,6 +68,43 @@ class ChatModelSwitchDelegate( private var unconfirmedTargetModel: String? = null private var modelSwitchSequence: Long = 0L private var cachedModelOptions: List = emptyList() + private var cachedModelOptionsScope: DataScope? = dataScopeFlow.value + private var modelOptionsJob: Job? = null + private var modelOptionsJobScope: DataScope? = null + + fun attachScopeObserver(externalScope: CoroutineScope) { + externalScope.launch { + dataScopeFlow.collect { newScope -> + if (cachedModelOptionsScope == newScope) return@collect + + // A model catalog belongs to exactly one DataScope. Never let + // models or capability flags bleed across profile/server/auth + // boundaries while the new scope is reconnecting. + cachedModelOptions = emptyList() + cachedModelOptionsScope = newScope + fastRejectedModels.clear() + + if (modelOptionsJobScope != newScope) { + modelOptionsJob?.cancel() + modelOptionsJob = null + modelOptionsJobScope = null + } + + val shouldReload = newScope != null && uiState.value.showModelPicker + uiState.update { + it.copy( + modelPickerProviders = emptyList(), + modelPickerLoading = shouldReload, + currentModelCapabilities = null, + ) + } + + if (shouldReload) { + loadModelOptions(refresh = false) + } + } + } + } fun isModelPickerCommand(command: String): Boolean { val trimmed = command.trim() @@ -75,18 +118,13 @@ class ChatModelSwitchDelegate( } fun preloadModelOptions() { - scope.launch(ioDispatcher) { - val result = getModelOptionsCall(false) - if (result is NetworkResult.Success) { - cachedModelOptions = result.data.providers.orEmpty() - uiState.update { it.copy(modelPickerPinned = getPinnedModels()) } - syncCurrentModelCapabilities() - } - } + loadModelOptions(refresh = false) } fun openModelPicker() { - val hasCached = cachedModelOptions.isNotEmpty() + val currentScope = dataScopeFlow.value + val hasCached = + cachedModelOptionsScope == currentScope && cachedModelOptions.isNotEmpty() uiState.update { it.copy( showModelPicker = true, @@ -96,36 +134,77 @@ class ChatModelSwitchDelegate( ) } if (!hasCached) { - refreshModelOptions() + loadModelOptions(refresh = false) } } fun refreshModelOptions() { + loadModelOptions(refresh = true) + } + + private fun loadModelOptions(refresh: Boolean) { + val requestScope = dataScopeFlow.value + + if (cachedModelOptionsScope != requestScope) { + cachedModelOptions = emptyList() + cachedModelOptionsScope = requestScope + fastRejectedModels.clear() + uiState.update { + it.copy( + modelPickerProviders = emptyList(), + currentModelCapabilities = null, + ) + } + } + + // Reuse only work belonging to this exact DataScope. A reconnect into + // another profile/server must not inherit the previous preload. + if (modelOptionsJob?.isActive == true) { + if (modelOptionsJobScope == requestScope && !refresh) return + modelOptionsJob?.cancel() + } + uiState.update { it.copy(modelPickerLoading = true) } - scope.launch(ioDispatcher) { - val result = getModelOptionsCall(true) - when (result) { - is NetworkResult.Success -> { - cachedModelOptions = result.data.providers.orEmpty() - uiState.update { - it.copy( - modelPickerProviders = cachedModelOptions, - modelPickerLoading = false, - ) - } - syncCurrentModelCapabilities() + modelOptionsJobScope = requestScope + modelOptionsJob = + scope.launch { + val result = withContext(ioDispatcher) { getModelOptionsCall(refresh) } + + // The scope may have changed while IO was in flight. Never + // publish that old scope's result into the current picker. + if (dataScopeFlow.value != requestScope) { + return@launch } - is NetworkResult.Failure -> { - uiState.update { - it.copy( - modelPickerLoading = false, - errorMessage = "Failed to load models: ${result.error.message}", - ) + when (result) { + is NetworkResult.Success -> { + cachedModelOptions = result.data.providers.orEmpty() + cachedModelOptionsScope = requestScope + uiState.update { + it.copy( + modelPickerProviders = cachedModelOptions, + modelPickerPinned = getPinnedModels(), + modelPickerLoading = false, + ) + } + syncCurrentModelCapabilities() + } + + is NetworkResult.Failure -> { + uiState.update { + it.copy( + modelPickerLoading = false, + errorMessage = + if (it.showModelPicker || refresh) { + "Failed to load models: ${result.error.message}" + } else { + it.errorMessage + }, + ) + } } } } - } } fun closeModelPicker() { @@ -402,7 +481,13 @@ class ChatModelSwitchDelegate( fun getModelCapabilities( providerSlug: String, modelName: String, - ): ModelCapabilities? = cachedModelOptions.find { it.slug == providerSlug }?.capabilities?.get(modelName) + ): ModelCapabilities? { + if (cachedModelOptionsScope != dataScopeFlow.value) return null + return cachedModelOptions + .find { it.slug == providerSlug } + ?.capabilities + ?.get(modelName) + } fun getCurrentModelCapabilities(): ModelCapabilities? { val label = uiState.value.currentSessionModel ?: return null @@ -425,7 +510,7 @@ class ChatModelSwitchDelegate( } else { val provider = label.substring(0, idx) val model = label.substring(idx + 1) - val baseCaps = cachedModelOptions.find { it.slug == provider }?.capabilities?.get(model) + val baseCaps = getModelCapabilities(provider, model) if (fastRejectedModels.contains(label) && baseCaps?.fast == true) { baseCaps.copy(fast = false) } else { diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt index f0029c79..cf5ec8c8 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt @@ -479,7 +479,9 @@ class ChatViewModel( handleSlashCommand = { cmd -> handleSlashCommand(cmd) }, fetchContextUsage = { fetchContextUsage() }, onModelSwitchInitiated = { onModelSwitchInitiated() }, - ) + ).apply { + attachScopeObserver(viewModelScope) + } private val credentialPromptsDelegate = ChatCredentialPromptsDelegate( diff --git a/app/src/main/java/com/m57/hermescontrol/ui/model/ModelScreen.kt b/app/src/main/java/com/m57/hermescontrol/ui/model/ModelScreen.kt index fa497374..aed0803d 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/model/ModelScreen.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/model/ModelScreen.kt @@ -104,11 +104,13 @@ fun ModelScreen( onRefresh = { viewModel.loadAll(refresh = true) }, ) { paddingValues -> when { - state.isLoading && state.mainModelProvider.isEmpty() && state.providers.isEmpty() -> { + state.isLoading && state.mainModelProvider.isEmpty() && state.providers.isEmpty() && + state.pinnedModels.isEmpty() -> { SkeletonListState(modifier = Modifier.padding(paddingValues)) } - state.errorMessage != null && state.providers.isEmpty() && state.mainModelProvider.isEmpty() -> { + state.errorMessage != null && state.providers.isEmpty() && state.mainModelProvider.isEmpty() && + state.pinnedModels.isEmpty() -> { ErrorState( message = state.errorMessage ?: "", onRetry = { viewModel.loadAll() }, @@ -119,7 +121,8 @@ fun ModelScreen( !state.isLoading && !state.catalogLoading && state.providers.isEmpty() && - state.mainModelProvider.isEmpty() -> { + state.mainModelProvider.isEmpty() && + state.pinnedModels.isEmpty() -> { EmptyState( title = stringResource(R.string.model_empty_title), subtitle = stringResource(R.string.model_empty_desc), diff --git a/app/src/main/java/com/m57/hermescontrol/ui/model/ModelViewModel.kt b/app/src/main/java/com/m57/hermescontrol/ui/model/ModelViewModel.kt index fd8ae68b..7d99de54 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/model/ModelViewModel.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/model/ModelViewModel.kt @@ -13,6 +13,7 @@ import com.m57.hermescontrol.data.model.UpdateProfileModelRequest import com.m57.hermescontrol.data.remote.ApiClient import com.m57.hermescontrol.data.remote.NetworkResult import com.m57.hermescontrol.data.remote.safeApiCall +import com.m57.hermescontrol.data.ws.ModelCatalogStore import com.m57.hermescontrol.data.ws.ModelOptionsRepository import com.m57.hermescontrol.ui.common.ToastHost import kotlinx.coroutines.Dispatchers @@ -69,14 +70,28 @@ data class ModelUiState( ) class ModelViewModel( - private val modelOptionsRepository: ModelOptionsRepository = ModelOptionsRepository(), + private val catalogStore: ModelCatalogStore = ModelCatalogStore.shared, ) : ViewModel(), ToastHost { + constructor(repository: ModelOptionsRepository) : this(ModelCatalogStore(repository = repository)) + private val _uiState = MutableStateFlow(ModelUiState()) val uiState: StateFlow = _uiState.asStateFlow() init { _uiState.update { it.copy(pinnedModels = AuthManager.getPinnedModels()) } + viewModelScope.launch { + catalogStore.state.collect { catalogState -> + // Empty state is meaningful: it is how ModelCatalogStore + // invalidates the previous DataScope. Never ignore it. + _uiState.update { + it.copy( + providers = catalogState.providers, + catalogLoading = catalogState.isRefreshing, + ) + } + } + } } fun loadAll(refresh: Boolean = false) { @@ -100,10 +115,10 @@ class ModelViewModel( safeApiCall { ApiClient.hermesApi.getMoaModels() } } - // Phase 2: Launch model options concurrently + // Phase 2: Launch model options concurrently via shared catalog store val optionsDeferred = - async(Dispatchers.IO) { - modelOptionsRepository.load(refresh) + async { + catalogStore.ensureLoaded(forceRefresh = refresh) } // Await Phase 1 calls (~40-190ms total) and update UI immediately with available state @@ -160,12 +175,9 @@ class ModelViewModel( val optionsResult = optionsDeferred.await() when (optionsResult) { is NetworkResult.Success -> { - _uiState.update { - it.copy( - catalogLoading = false, - providers = optionsResult.data.providers.orEmpty(), - ) - } + // ModelCatalogStore.state is the single source of truth. + // Do not independently republish a result here because a + // DataScope switch may have invalidated it meanwhile. } is NetworkResult.Failure -> { diff --git a/app/src/main/java/com/m57/hermescontrol/ui/model/components/ModelPickerDialog.kt b/app/src/main/java/com/m57/hermescontrol/ui/model/components/ModelPickerDialog.kt index 0b576cb1..4bb075fb 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/model/components/ModelPickerDialog.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/model/components/ModelPickerDialog.kt @@ -102,7 +102,7 @@ fun ModelPickerDialog( }, text = { Column { - if (isLoading) { + if (isLoading && providers.isEmpty() && pinnedModels.isEmpty()) { LoadingState( subtitle = stringResource(R.string.model_picker_loading), modifier = Modifier.fillMaxWidth(), diff --git a/app/src/test/java/com/m57/hermescontrol/data/ws/ModelCatalogStoreTest.kt b/app/src/test/java/com/m57/hermescontrol/data/ws/ModelCatalogStoreTest.kt new file mode 100644 index 00000000..1d1ce110 --- /dev/null +++ b/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() + + 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() + val forcedGate = CompletableDeferred() + val requests = mutableListOf() + + 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) + } +} diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegateTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegateTest.kt index ec66a567..efe0c636 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegateTest.kt +++ b/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() + private var modelResponseGate: CompletableDeferred? = 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(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) } } diff --git a/app/src/test/java/com/m57/hermescontrol/ui/model/ModelViewModelTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/model/ModelViewModelTest.kt index 8c6fe7f5..19fce4d9 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/model/ModelViewModelTest.kt +++ b/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()