diff --git a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManagerTests.java b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManagerTests.java new file mode 100644 index 00000000..4d125571 --- /dev/null +++ b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManagerTests.java @@ -0,0 +1,124 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.core.chat; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.when; + +import java.time.Duration; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import com.microsoft.copilot.eclipse.core.chat.service.BuiltInChatModeService; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ConversationMode; + +@ExtendWith(MockitoExtension.class) +class BuiltInChatModeManagerTests { + + @Mock + private BuiltInChatModeService mockService; + + private BuiltInChatModeManager manager; + + @BeforeEach + void setUp() { + manager = new BuiltInChatModeManager(mockService); + } + + @Test + void testReloadModes_pendingLoad_returnsImmediatelyAndPublishesOnCompletion() { + CompletableFuture> pendingModes = new CompletableFuture<>(); + when(mockService.loadBuiltInModes()).thenReturn(pendingModes); + + CompletableFuture reload = assertTimeoutPreemptively(Duration.ofSeconds(1), + manager::reloadModes); + + assertFalse(reload.isDone()); + assertTrue(manager.getBuiltInModes().isEmpty()); + + BuiltInChatMode agentMode = createBuiltInMode(BuiltInChatMode.AGENT_MODE_NAME); + pendingModes.complete(List.of(agentMode)); + reload.join(); + + assertEquals(List.of(agentMode), manager.getBuiltInModes()); + } + + @Test + void testReloadModes_olderRequestCompletesLast_keepsLatestResult() { + CompletableFuture> olderModes = new CompletableFuture<>(); + CompletableFuture> latestModes = new CompletableFuture<>(); + when(mockService.loadBuiltInModes()).thenReturn(olderModes, latestModes); + + CompletableFuture olderReload = manager.reloadModes(); + CompletableFuture latestReload = manager.reloadModes(); + + BuiltInChatMode agentMode = createBuiltInMode(BuiltInChatMode.AGENT_MODE_NAME); + latestModes.complete(List.of(agentMode)); + latestReload.join(); + assertEquals(List.of(agentMode), manager.getBuiltInModes()); + + olderModes.complete(List.of(createBuiltInMode(BuiltInChatMode.ASK_MODE_NAME))); + olderReload.join(); + + assertEquals(List.of(agentMode), manager.getBuiltInModes()); + } + + @Test + void testClearModes_inFlightReloadCompletes_keepsCacheEmpty() { + BuiltInChatMode agentMode = createBuiltInMode(BuiltInChatMode.AGENT_MODE_NAME); + CompletableFuture> pendingModes = new CompletableFuture<>(); + when(mockService.loadBuiltInModes()) + .thenReturn(CompletableFuture.completedFuture(List.of(agentMode)), pendingModes); + manager.reloadModes().join(); + assertEquals(List.of(agentMode), manager.getBuiltInModes()); + + CompletableFuture reload = manager.reloadModes(); + manager.clearModes(); + assertTrue(manager.getBuiltInModes().isEmpty()); + + pendingModes.complete(List.of(createBuiltInMode(BuiltInChatMode.ASK_MODE_NAME))); + reload.join(); + + assertTrue(manager.getBuiltInModes().isEmpty()); + } + + @Test + void testReloadModes_serviceThrowsSynchronously_returnsFailedFutureAndInvalidatesOlderLoad() { + CompletableFuture> pendingModes = new CompletableFuture<>(); + RuntimeException failure = new IllegalStateException("Synchronous load failure"); + when(mockService.loadBuiltInModes()).thenReturn(pendingModes).thenThrow(failure); + + CompletableFuture olderReload = manager.reloadModes(); + CompletableFuture failedReload = assertDoesNotThrow(manager::reloadModes); + + CompletionException exception = assertThrows(CompletionException.class, failedReload::join); + assertSame(failure, exception.getCause()); + + pendingModes.complete(List.of(createBuiltInMode(BuiltInChatMode.AGENT_MODE_NAME))); + olderReload.join(); + assertTrue(manager.getBuiltInModes().isEmpty()); + } + + private BuiltInChatMode createBuiltInMode(String name) { + ConversationMode mode = new ConversationMode(); + mode.setId(name); + mode.setName(name); + mode.setKind(name); + mode.setDescription(name + " mode"); + return new BuiltInChatMode(mode); + } +} diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManager.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManager.java index 079628bf..7668a501 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManager.java +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManager.java @@ -5,33 +5,28 @@ import java.util.ArrayList; import java.util.List; -import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CompletableFuture; import com.microsoft.copilot.eclipse.core.chat.service.BuiltInChatModeService; /** - * Singleton manager for built-in chat modes. Built-in modes are loaded once from the LSP API at startup. + * Singleton manager for asynchronously loaded built-in chat modes. */ -public enum BuiltInChatModeManager { - INSTANCE; +public final class BuiltInChatModeManager { + + public static final BuiltInChatModeManager INSTANCE = new BuiltInChatModeManager(); private final BuiltInChatModeService service; - private List builtInModes; + private volatile List builtInModes; + private long loadGeneration; - BuiltInChatModeManager() { - this.service = new BuiltInChatModeService(); - this.builtInModes = new CopyOnWriteArrayList<>(); - loadModesSync(); + private BuiltInChatModeManager() { + this(new BuiltInChatModeService()); } - private void loadModesSync() { - try { - List modes = service.loadBuiltInModes().get(); - this.builtInModes = new CopyOnWriteArrayList<>(modes); - } catch (Exception e) { - // Initialize with empty list on failure - this.builtInModes = new CopyOnWriteArrayList<>(); - } + BuiltInChatModeManager(BuiltInChatModeService service) { + this.service = service; + this.builtInModes = List.of(); } public List getBuiltInModes() { @@ -62,8 +57,36 @@ public BuiltInChatMode getBuiltInModeById(String id) { /** * Reloads built-in chat modes from the LSP API. This should be called when the user switches * to ensure the latest modes are available for the current user context. + * + * @return a future that completes after this load has been processed; stale results may be ignored + */ + public CompletableFuture reloadModes() { + final long requestGeneration; + synchronized (this) { + requestGeneration = ++loadGeneration; + } + + final CompletableFuture> modesFuture; + try { + modesFuture = service.loadBuiltInModes(); + } catch (RuntimeException e) { + return CompletableFuture.failedFuture(e); + } + + return modesFuture.thenAccept(modes -> { + synchronized (this) { + if (requestGeneration == loadGeneration) { + builtInModes = List.copyOf(modes); + } + } + }); + } + + /** + * Clears cached built-in modes and prevents in-flight loads from publishing stale results. */ - public void reloadModes() { - loadModesSync(); + public synchronized void clearModes() { + loadGeneration++; + builtInModes = List.of(); } -} \ No newline at end of file +} diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceServiceTest.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceServiceTest.java index 7b36a797..ce1cc864 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceServiceTest.java +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceServiceTest.java @@ -3,15 +3,22 @@ package com.microsoft.copilot.eclipse.ui.chat.services; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.lang.reflect.Field; +import java.time.Duration; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.e4.core.services.events.IEventBroker; import org.junit.jupiter.api.AfterEach; @@ -24,10 +31,14 @@ import org.osgi.service.event.EventHandler; import com.microsoft.copilot.eclipse.core.AuthStatusManager; +import com.microsoft.copilot.eclipse.core.chat.BuiltInChatMode; +import com.microsoft.copilot.eclipse.core.chat.BuiltInChatModeManager; import com.microsoft.copilot.eclipse.core.chat.InputNavigation; import com.microsoft.copilot.eclipse.core.events.CopilotEventConstants; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ConversationMode; import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotStatusResult; +import com.microsoft.copilot.eclipse.ui.utils.SwtUtils; @ExtendWith(MockitoExtension.class) class UserPreferenceServiceTest { @@ -38,11 +49,16 @@ class UserPreferenceServiceTest { @Mock private AuthStatusManager mockAuthStatusManager; + @Mock + private BuiltInChatModeManager mockBuiltInChatModeManager; + private UserPreferenceService userPreferenceService; + private final AtomicReference> builtInModes = new AtomicReference<>(List.of()); @BeforeEach void setUp() { when(mockAuthStatusManager.isSignedIn()).thenReturn(false); + when(mockBuiltInChatModeManager.getBuiltInModes()).thenAnswer(invocation -> builtInModes.get()); } @AfterEach @@ -55,7 +71,8 @@ void tearDown() { @Test void testAuthStatusChangedEventHandler_UserSignsOut_ClearsUserPreferenceCache() { // Arrange - userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); + userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager, + mockBuiltInChatModeManager); // Set up initial state with input navigation setInputNavigationForService(new InputNavigation()); @@ -72,12 +89,15 @@ void testAuthStatusChangedEventHandler_UserSignsOut_ClearsUserPreferenceCache() // Assert assertNull(getInputNavigationFromService(), "Input navigation should be cleared when user signs out"); + verify(mockBuiltInChatModeManager).clearModes(); } @Test void testAuthStatusChangedEventHandler_SignOutThenSignIn() { // Arrange - userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); + when(mockBuiltInChatModeManager.reloadModes()).thenReturn(CompletableFuture.completedFuture(null)); + userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager, + mockBuiltInChatModeManager); EventHandler authHandler = getAuthStatusChangedEventHandler(); assertNotNull(authHandler, "Auth status changed event handler should be available"); @@ -97,12 +117,17 @@ void testAuthStatusChangedEventHandler_SignOutThenSignIn() { // Assert - After sign in, input navigation should be restored assertNotNull(getInputNavigationFromService(), "Input navigation should be restored after sign in"); assertEquals("input2", getInputNavigationFromService().getLatestInput(), "Input navigation should be restored"); + verify(mockBuiltInChatModeManager).clearModes(); + verify(mockBuiltInChatModeManager).reloadModes(); } @Test - void testAuthStatusChangedEventHandler_UserSignsIn_ReloadsBuiltInModes() { + void testAuthStatusChangedEventHandler_UserSignsIn_ReloadsBuiltInModesWithoutBlocking() { // Arrange - userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); + CompletableFuture pendingReload = new CompletableFuture<>(); + when(mockBuiltInChatModeManager.reloadModes()).thenReturn(pendingReload); + userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager, + mockBuiltInChatModeManager); EventHandler authHandler = getAuthStatusChangedEventHandler(); assertNotNull(authHandler, "Auth status changed event handler should be available"); @@ -110,13 +135,15 @@ void testAuthStatusChangedEventHandler_UserSignsIn_ReloadsBuiltInModes() { Event signInEvent = createAuthStatusEvent(CopilotStatusResult.OK, "test-user"); // Act - Simulate user sign in - authHandler.handleEvent(signInEvent); + assertTimeoutPreemptively(Duration.ofSeconds(1), () -> authHandler.handleEvent(signInEvent)); + + // Assert + assertFalse(pendingReload.isDone(), "The event handler should not wait for the mode reload"); + verify(mockBuiltInChatModeManager).reloadModes(); - // Assert - No exception should be thrown and the handler should complete successfully - // Note: Since BuiltInChatModeManager is a singleton and we can't easily mock it in this test, - // we primarily verify that the event handler doesn't throw exceptions when reloading modes. - // The actual reloading functionality is tested through integration tests. - assertNotNull(authHandler, "Auth handler should still be functional after handling sign-in event"); + builtInModes.set(List.of(createBuiltInMode("Agent"))); + pendingReload.complete(null); + assertArrayEquals(new String[] { "Agent" }, getAvailableChatModesFromObservable()); } /** @@ -141,6 +168,16 @@ private Event createAuthStatusEvent(String status, String user) { return new Event(CopilotEventConstants.TOPIC_AUTH_STATUS_CHANGED, eventProperties); } + private BuiltInChatMode createBuiltInMode(String name) { + ConversationMode mode = new ConversationMode(); + mode.setId(name); + mode.setName(name); + mode.setKind(name); + mode.setBuiltIn(true); + mode.setDescription(name + " mode"); + return new BuiltInChatMode(mode); + } + /** * Helper method to access private authStatusChangedEventHandler field for * testing @@ -155,6 +192,21 @@ private EventHandler getAuthStatusChangedEventHandler() { } } + private String[] getAvailableChatModesFromObservable() { + AtomicReference availableModes = new AtomicReference<>(); + SwtUtils.invokeOnDisplayThread(() -> { + try { + Field field = UserPreferenceService.class.getDeclaredField("chatModeObservable"); + field.setAccessible(true); + Object observable = field.get(userPreferenceService); + availableModes.set((String[]) observable.getClass().getMethod("getValue").invoke(observable)); + } catch (Exception e) { + throw new RuntimeException("Failed to read chatModeObservable", e); + } + }); + return availableModes.get(); + } + /** * Helper method to access private inputNavigation field for testing */ @@ -180,4 +232,4 @@ private void setInputNavigationForService(InputNavigation inputNavigation) { throw new RuntimeException("Failed to set inputNavigation field", e); } } -} \ No newline at end of file +} diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatServiceManager.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatServiceManager.java index 62e6c4a6..d6982a1d 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatServiceManager.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatServiceManager.java @@ -5,6 +5,7 @@ import com.microsoft.copilot.eclipse.core.AuthStatusManager; import com.microsoft.copilot.eclipse.core.CopilotCore; +import com.microsoft.copilot.eclipse.core.chat.BuiltInChatModeManager; import com.microsoft.copilot.eclipse.core.chat.service.CustomizationFileService; import com.microsoft.copilot.eclipse.core.chat.service.IChatServiceManager; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; @@ -46,7 +47,8 @@ public ChatServiceManager() { this.authStatusManager = CopilotCore.getPlugin().getAuthStatusManager(); chatCompletionService = new ChatCompletionService(this.lsConnection, this.authStatusManager); modelService = new ModelService(this.lsConnection, this.authStatusManager); - userPreferenceService = new UserPreferenceService(this.lsConnection, this.authStatusManager); + userPreferenceService = new UserPreferenceService(this.lsConnection, this.authStatusManager, + BuiltInChatModeManager.INSTANCE); avatarService = new AvatarService(this.authStatusManager); agentToolService = new AgentToolService(this.lsConnection); fileToolService = new FileToolService(this.lsConnection); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java index 0ca4b969..738dd374 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java @@ -8,6 +8,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import org.apache.commons.lang3.StringUtils; import org.eclipse.core.databinding.observable.sideeffect.ISideEffect; @@ -47,6 +48,7 @@ * Service for managing chat modes and input navigation. */ public class UserPreferenceService extends ChatBaseService implements CopilotAuthStatusListener { + private final BuiltInChatModeManager builtInChatModeManager; private IObservableValue chatModeObservable; private IObservableValue activeChatModeObservable; // Controls which view to show: Ask or Agent private IObservableValue activeModeNameOrIdObservable; // Tracks current mode name/ID for UI elements @@ -63,9 +65,15 @@ public class UserPreferenceService extends ChatBaseService implements CopilotAut /** * Constructor for the UserPreferenceService. + * + * @param lsConnection the language server connection + * @param authStatusManager the authentication status manager + * @param builtInChatModeManager the manager for built-in chat modes */ - public UserPreferenceService(CopilotLanguageServerConnection lsConnection, AuthStatusManager authStatusManager) { + public UserPreferenceService(CopilotLanguageServerConnection lsConnection, AuthStatusManager authStatusManager, + BuiltInChatModeManager builtInChatModeManager) { super(lsConnection, authStatusManager); + this.builtInChatModeManager = Objects.requireNonNull(builtInChatModeManager); this.authStatusManager.addCopilotAuthStatusListener(this); ensureRealm(() -> { @@ -77,6 +85,9 @@ public UserPreferenceService(CopilotLanguageServerConnection lsConnection, AuthS initializeEventHandlers(); subscribeToEvents(); init(); + if (authStatusManager.isSignedIn()) { + reloadBuiltInModes(); + } } private void initializeEventHandlers() { @@ -86,25 +97,14 @@ private void initializeEventHandlers() { // If the user signs out, we need to clear the preference cache to avoid the current preference being used in // the next sign in account. if (statusResult.isNotSignedIn()) { + builtInChatModeManager.clearModes(); + refreshAvailableChatModes(); clearUserPreferenceCache(); this.inputNavigation = null; - } else { - // User has signed in - reload built-in modes to ensure we have the latest modes for this user - try { - BuiltInChatModeManager.INSTANCE.reloadModes(); - - // Update available chat modes in the observable to reflect any changes - ensureRealm(() -> { - if (!Arrays.deepEquals(getAvailableChatModes(), chatModeObservable.getValue())) { - chatModeObservable.setValue(getAvailableChatModes()); - } - }); - - // Reinitialize user preferences for the new user - init(); - } catch (Exception e) { - CopilotCore.LOGGER.error("Failed to reload built-in modes on user switch", e); - } + } else if (statusResult.isSignedIn()) { + // Reinitialize preferences immediately while built-in modes load in the background. + init(); + reloadBuiltInModes(); } } }; @@ -112,11 +112,8 @@ private void initializeEventHandlers() { featureFlagNotifiedEventHandler = event -> { Object property = event.getProperty(IEventBroker.DATA); if (property instanceof DidChangeFeatureFlagsParams params) { + refreshAvailableChatModes(); ensureRealm(() -> { - if (!Arrays.deepEquals(getAvailableChatModes(), chatModeObservable.getValue())) { - chatModeObservable.setValue(getAvailableChatModes()); - } - if (!params.isAgentModeEnabled()) { setActiveChatMode(ChatMode.Ask.toString()); } @@ -135,6 +132,24 @@ private void subscribeToEvents() { } } + private void reloadBuiltInModes() { + builtInChatModeManager.reloadModes() + .thenRun(this::refreshAvailableChatModes) + .exceptionally(ex -> { + CopilotCore.LOGGER.error("Failed to reload built-in modes", ex); + return null; + }); + } + + private void refreshAvailableChatModes() { + chatModeObservable.getRealm().asyncExec(() -> { + String[] availableChatModes = getAvailableChatModes(); + if (!Arrays.deepEquals(availableChatModes, chatModeObservable.getValue())) { + chatModeObservable.setValue(availableChatModes); + } + }); + } + private void init() { if (authStatusManager.isSignedIn()) { // Initialize chat mode preferences @@ -223,7 +238,7 @@ private String[] getAvailableChatModes() { // Add built-in modes from BuiltInChatModeManager FeatureFlags flags = CopilotCore.getPlugin().getFeatureFlags(); - List builtInModes = BuiltInChatModeManager.INSTANCE.getBuiltInModes(); + List builtInModes = builtInChatModeManager.getBuiltInModes(); if (flags != null && !flags.isAgentModeEnabled()) { // When agent mode is disabled, show only Ask mode @@ -341,10 +356,10 @@ public CustomChatMode getActiveCustomMode() { /** * Reload the available chat modes from the manager. This will sync custom modes from the Language Server and refresh - * the dropdown. Built-in modes are loaded once at startup and don't need reloading. + * the dropdown. Built-in modes are refreshed separately when authentication changes. */ public void reloadChatModes() { - // Sync custom modes from LS (built-in modes are loaded once at startup) + // Sync custom modes from LS. CustomChatModeManager.INSTANCE.syncCustomModesFromService().thenRun(() -> { ensureRealm(() -> { String[] currentModes = chatModeObservable.getValue(); @@ -433,7 +448,7 @@ private List buildChatModeGroups(String[] availableModeNames, List available = Arrays.asList(availableModeNames); List builtInItems = new ArrayList<>(); - for (BuiltInChatMode mode : BuiltInChatModeManager.INSTANCE.getBuiltInModes()) { + for (BuiltInChatMode mode : builtInChatModeManager.getBuiltInModes()) { if (!available.contains(mode.getDisplayName())) { continue; }