From 0404711d0cdf885e45eff592950344114b88286f Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Tue, 21 Jul 2026 15:44:12 +0800 Subject: [PATCH 1/2] Fix async built-in chat mode loading Load built-in chat modes after sign-in without blocking the SWT UI. Protect cached modes from stale reloads and refresh the preference observable asynchronously. Add deterministic concurrency and authentication lifecycle coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../chat/BuiltInChatModeManagerTests.java | 103 ++++++++++++++++++ .../core/chat/BuiltInChatModeManager.java | 56 ++++++---- .../services/UserPreferenceServiceTest.java | 94 ++++++++++++++-- .../chat/services/UserPreferenceService.java | 53 +++++---- 4 files changed, 253 insertions(+), 53 deletions(-) create mode 100644 com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManagerTests.java 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..87f619ad --- /dev/null +++ b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManagerTests.java @@ -0,0 +1,103 @@ +// 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.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; +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 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()); + } + + 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..69b05f7c 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,29 @@ 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; + } + + return service.loadBuiltInModes().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..a741f40b 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,24 @@ 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.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +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 +33,15 @@ import org.osgi.service.event.EventHandler; 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.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.ConversationModesParams; import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotStatusResult; +import com.microsoft.copilot.eclipse.ui.utils.SwtUtils; @ExtendWith(MockitoExtension.class) class UserPreferenceServiceTest { @@ -39,22 +53,48 @@ class UserPreferenceServiceTest { private AuthStatusManager mockAuthStatusManager; private UserPreferenceService userPreferenceService; + private CopilotCore originalPlugin; + private CopilotCore testPlugin; + private CopilotLanguageServerConnection originalLsConnection; @BeforeEach - void setUp() { + void setUp() throws Exception { when(mockAuthStatusManager.isSignedIn()).thenReturn(false); + + originalPlugin = CopilotCore.getPlugin(); + testPlugin = originalPlugin != null ? originalPlugin : new CopilotCore(); + Field languageServerField = CopilotCore.class.getDeclaredField("copilotLanguageServer"); + languageServerField.setAccessible(true); + originalLsConnection = (CopilotLanguageServerConnection) languageServerField.get(testPlugin); + languageServerField.set(testPlugin, mockLsConnection); + BuiltInChatModeManager.INSTANCE.clearModes(); } @AfterEach - void tearDown() { + void tearDown() throws Exception { if (userPreferenceService != null) { userPreferenceService.dispose(); } + BuiltInChatModeManager.INSTANCE.clearModes(); + + Field languageServerField = CopilotCore.class.getDeclaredField("copilotLanguageServer"); + languageServerField.setAccessible(true); + languageServerField.set(testPlugin, originalLsConnection); + + Field pluginField = CopilotCore.class.getDeclaredField("COPILOT_CORE_PLUGIN"); + pluginField.setAccessible(true); + pluginField.set(null, originalPlugin); } @Test void testAuthStatusChangedEventHandler_UserSignsOut_ClearsUserPreferenceCache() { // Arrange + ConversationMode agentMode = createBuiltInMode("Agent"); + when(mockLsConnection.listConversationModes(any(ConversationModesParams.class))) + .thenReturn(CompletableFuture.completedFuture(new ConversationMode[] { agentMode })); + BuiltInChatModeManager.INSTANCE.reloadModes().join(); + assertFalse(BuiltInChatModeManager.INSTANCE.getBuiltInModes().isEmpty()); + userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); // Set up initial state with input navigation @@ -72,11 +112,15 @@ void testAuthStatusChangedEventHandler_UserSignsOut_ClearsUserPreferenceCache() // Assert assertNull(getInputNavigationFromService(), "Input navigation should be cleared when user signs out"); + assertTrue(BuiltInChatModeManager.INSTANCE.getBuiltInModes().isEmpty(), + "Built-in modes should be cleared when user signs out"); } @Test void testAuthStatusChangedEventHandler_SignOutThenSignIn() { // Arrange + when(mockLsConnection.listConversationModes(any(ConversationModesParams.class))) + .thenReturn(CompletableFuture.completedFuture(new ConversationMode[0])); userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); EventHandler authHandler = getAuthStatusChangedEventHandler(); @@ -100,8 +144,10 @@ void testAuthStatusChangedEventHandler_SignOutThenSignIn() { } @Test - void testAuthStatusChangedEventHandler_UserSignsIn_ReloadsBuiltInModes() { + void testAuthStatusChangedEventHandler_UserSignsIn_ReloadsBuiltInModesWithoutBlocking() { // Arrange + CompletableFuture pendingModes = new CompletableFuture<>(); + when(mockLsConnection.listConversationModes(any(ConversationModesParams.class))).thenReturn(pendingModes); userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); EventHandler authHandler = getAuthStatusChangedEventHandler(); @@ -110,13 +156,16 @@ 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 - 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"); + // Assert + assertFalse(pendingModes.isDone(), "The event handler should not wait for the LSP response"); + verify(mockLsConnection).listConversationModes(any(ConversationModesParams.class)); + + pendingModes.complete(new ConversationMode[] { createBuiltInMode("Agent") }); + assertEquals(1, BuiltInChatModeManager.INSTANCE.getBuiltInModes().size()); + assertEquals("Agent", BuiltInChatModeManager.INSTANCE.getBuiltInModes().get(0).getDisplayName()); + assertArrayEquals(new String[] { "Agent" }, getAvailableChatModesFromObservable()); } /** @@ -141,6 +190,16 @@ private Event createAuthStatusEvent(String status, String user) { return new Event(CopilotEventConstants.TOPIC_AUTH_STATUS_CHANGED, eventProperties); } + private ConversationMode createBuiltInMode(String name) { + ConversationMode mode = new ConversationMode(); + mode.setId(name); + mode.setName(name); + mode.setKind(name); + mode.setBuiltIn(true); + mode.setDescription(name + " mode"); + return mode; + } + /** * Helper method to access private authStatusChangedEventHandler field for * testing @@ -155,6 +214,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 +254,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/UserPreferenceService.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java index 0ca4b969..a8c264b5 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 @@ -77,6 +77,9 @@ public UserPreferenceService(CopilotLanguageServerConnection lsConnection, AuthS initializeEventHandlers(); subscribeToEvents(); init(); + if (authStatusManager.isSignedIn()) { + reloadBuiltInModes(); + } } private void initializeEventHandlers() { @@ -86,25 +89,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.INSTANCE.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 +104,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 +124,24 @@ private void subscribeToEvents() { } } + private void reloadBuiltInModes() { + BuiltInChatModeManager.INSTANCE.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 @@ -341,10 +348,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(); From 4732e685264070ba29ef42181423564faedfcef7 Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Tue, 21 Jul 2026 16:09:29 +0800 Subject: [PATCH 2/2] Address async chat mode review feedback Convert synchronous mode-loader exceptions into failed futures so callers retain consistent async error handling. Inject the built-in mode manager from the chat service composition root and isolate preference-service tests from CopilotCore global state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../chat/BuiltInChatModeManagerTests.java | 21 ++++++ .../core/chat/BuiltInChatModeManager.java | 9 ++- .../services/UserPreferenceServiceTest.java | 74 +++++++------------ .../ui/chat/services/ChatServiceManager.java | 4 +- .../chat/services/UserPreferenceService.java | 18 +++-- 5 files changed, 71 insertions(+), 55 deletions(-) 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 index 87f619ad..4d125571 100644 --- 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 @@ -4,14 +4,18 @@ 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; @@ -92,6 +96,23 @@ void testClearModes_inFlightReloadCompletes_keepsCacheEmpty() { 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); 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 69b05f7c..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 @@ -66,7 +66,14 @@ public CompletableFuture reloadModes() { requestGeneration = ++loadGeneration; } - return service.loadBuiltInModes().thenAccept(modes -> { + 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); 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 a741f40b..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 @@ -9,8 +9,6 @@ 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.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -33,13 +31,12 @@ import org.osgi.service.event.EventHandler; import com.microsoft.copilot.eclipse.core.AuthStatusManager; -import com.microsoft.copilot.eclipse.core.CopilotCore; +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.ConversationModesParams; import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotStatusResult; import com.microsoft.copilot.eclipse.ui.utils.SwtUtils; @@ -52,50 +49,30 @@ class UserPreferenceServiceTest { @Mock private AuthStatusManager mockAuthStatusManager; + @Mock + private BuiltInChatModeManager mockBuiltInChatModeManager; + private UserPreferenceService userPreferenceService; - private CopilotCore originalPlugin; - private CopilotCore testPlugin; - private CopilotLanguageServerConnection originalLsConnection; + private final AtomicReference> builtInModes = new AtomicReference<>(List.of()); @BeforeEach - void setUp() throws Exception { + void setUp() { when(mockAuthStatusManager.isSignedIn()).thenReturn(false); - - originalPlugin = CopilotCore.getPlugin(); - testPlugin = originalPlugin != null ? originalPlugin : new CopilotCore(); - Field languageServerField = CopilotCore.class.getDeclaredField("copilotLanguageServer"); - languageServerField.setAccessible(true); - originalLsConnection = (CopilotLanguageServerConnection) languageServerField.get(testPlugin); - languageServerField.set(testPlugin, mockLsConnection); - BuiltInChatModeManager.INSTANCE.clearModes(); + when(mockBuiltInChatModeManager.getBuiltInModes()).thenAnswer(invocation -> builtInModes.get()); } @AfterEach - void tearDown() throws Exception { + void tearDown() { if (userPreferenceService != null) { userPreferenceService.dispose(); } - BuiltInChatModeManager.INSTANCE.clearModes(); - - Field languageServerField = CopilotCore.class.getDeclaredField("copilotLanguageServer"); - languageServerField.setAccessible(true); - languageServerField.set(testPlugin, originalLsConnection); - - Field pluginField = CopilotCore.class.getDeclaredField("COPILOT_CORE_PLUGIN"); - pluginField.setAccessible(true); - pluginField.set(null, originalPlugin); } @Test void testAuthStatusChangedEventHandler_UserSignsOut_ClearsUserPreferenceCache() { // Arrange - ConversationMode agentMode = createBuiltInMode("Agent"); - when(mockLsConnection.listConversationModes(any(ConversationModesParams.class))) - .thenReturn(CompletableFuture.completedFuture(new ConversationMode[] { agentMode })); - BuiltInChatModeManager.INSTANCE.reloadModes().join(); - assertFalse(BuiltInChatModeManager.INSTANCE.getBuiltInModes().isEmpty()); - - userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); + userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager, + mockBuiltInChatModeManager); // Set up initial state with input navigation setInputNavigationForService(new InputNavigation()); @@ -112,16 +89,15 @@ void testAuthStatusChangedEventHandler_UserSignsOut_ClearsUserPreferenceCache() // Assert assertNull(getInputNavigationFromService(), "Input navigation should be cleared when user signs out"); - assertTrue(BuiltInChatModeManager.INSTANCE.getBuiltInModes().isEmpty(), - "Built-in modes should be cleared when user signs out"); + verify(mockBuiltInChatModeManager).clearModes(); } @Test void testAuthStatusChangedEventHandler_SignOutThenSignIn() { // Arrange - when(mockLsConnection.listConversationModes(any(ConversationModesParams.class))) - .thenReturn(CompletableFuture.completedFuture(new ConversationMode[0])); - 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"); @@ -141,14 +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_ReloadsBuiltInModesWithoutBlocking() { // Arrange - CompletableFuture pendingModes = new CompletableFuture<>(); - when(mockLsConnection.listConversationModes(any(ConversationModesParams.class))).thenReturn(pendingModes); - 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"); @@ -159,12 +138,11 @@ void testAuthStatusChangedEventHandler_UserSignsIn_ReloadsBuiltInModesWithoutBlo assertTimeoutPreemptively(Duration.ofSeconds(1), () -> authHandler.handleEvent(signInEvent)); // Assert - assertFalse(pendingModes.isDone(), "The event handler should not wait for the LSP response"); - verify(mockLsConnection).listConversationModes(any(ConversationModesParams.class)); + assertFalse(pendingReload.isDone(), "The event handler should not wait for the mode reload"); + verify(mockBuiltInChatModeManager).reloadModes(); - pendingModes.complete(new ConversationMode[] { createBuiltInMode("Agent") }); - assertEquals(1, BuiltInChatModeManager.INSTANCE.getBuiltInModes().size()); - assertEquals("Agent", BuiltInChatModeManager.INSTANCE.getBuiltInModes().get(0).getDisplayName()); + builtInModes.set(List.of(createBuiltInMode("Agent"))); + pendingReload.complete(null); assertArrayEquals(new String[] { "Agent" }, getAvailableChatModesFromObservable()); } @@ -190,14 +168,14 @@ private Event createAuthStatusEvent(String status, String user) { return new Event(CopilotEventConstants.TOPIC_AUTH_STATUS_CHANGED, eventProperties); } - private ConversationMode createBuiltInMode(String name) { + 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 mode; + return new BuiltInChatMode(mode); } /** 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 a8c264b5..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(() -> { @@ -89,7 +97,7 @@ 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.INSTANCE.clearModes(); + builtInChatModeManager.clearModes(); refreshAvailableChatModes(); clearUserPreferenceCache(); this.inputNavigation = null; @@ -125,7 +133,7 @@ private void subscribeToEvents() { } private void reloadBuiltInModes() { - BuiltInChatModeManager.INSTANCE.reloadModes() + builtInChatModeManager.reloadModes() .thenRun(this::refreshAvailableChatModes) .exceptionally(ex -> { CopilotCore.LOGGER.error("Failed to reload built-in modes", ex); @@ -230,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 @@ -440,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; }