Fix async built-in chat mode loading

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<List<BuiltInChatMode>> pendingModes = new CompletableFuture<>();
+    when(mockService.loadBuiltInModes()).thenReturn(pendingModes);
+
+    CompletableFuture<Void> 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<List<BuiltInChatMode>> olderModes = new CompletableFuture<>();
+    CompletableFuture<List<BuiltInChatMode>> latestModes = new CompletableFuture<>();
+    when(mockService.loadBuiltInModes()).thenReturn(olderModes, latestModes);
+
+    CompletableFuture<Void> olderReload = manager.reloadModes();
+    CompletableFuture<Void> 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<List<BuiltInChatMode>> pendingModes = new CompletableFuture<>();
+    when(mockService.loadBuiltInModes())
+        .thenReturn(CompletableFuture.completedFuture(List.of(agentMode)), pendingModes);
+    manager.reloadModes().join();
+    assertEquals(List.of(agentMode), manager.getBuiltInModes());
+
+    CompletableFuture<Void> 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);
+  }
+}

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<BuiltInChatMode> builtInModes;
+  private volatile List<BuiltInChatMode> builtInModes;
+  private long loadGeneration;
 
-  BuiltInChatModeManager() {
-    this.service = new BuiltInChatModeService();
-    this.builtInModes = new CopyOnWriteArrayList<>();
-    loadModesSync();
+  private BuiltInChatModeManager() {
+    this(new BuiltInChatModeService());
   }
 
-  private void loadModesSync() {
-    try {
-      List<BuiltInChatMode> 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<BuiltInChatMode> 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<Void> 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();
   }
-}
+}

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<ConversationMode[]> 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<String[]> 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);
     }
   }
-}
+}