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,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<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());
+  }
+
+  @Test
+  void testReloadModes_serviceThrowsSynchronously_returnsFailedFutureAndInvalidatesOlderLoad() {
+    CompletableFuture<List<BuiltInChatMode>> pendingModes = new CompletableFuture<>();
+    RuntimeException failure = new IllegalStateException("Synchronous load failure");
+    when(mockService.loadBuiltInModes()).thenReturn(pendingModes).thenThrow(failure);
+
+    CompletableFuture<Void> olderReload = manager.reloadModes();
+    CompletableFuture<Void> 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);
+  }
+}

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,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<Void> reloadModes() {
+    final long requestGeneration;
+    synchronized (this) {
+      requestGeneration = ++loadGeneration;
+    }
+
+    final CompletableFuture<List<BuiltInChatMode>> 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();
   }
-}
+}

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<List<BuiltInChatMode>> 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,26 +117,33 @@ 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<Void> 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");
 
     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<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 +232,4 @@ private void setInputNavigationForService(InputNavigation inputNavigation) {
       throw new RuntimeException("Failed to set inputNavigation field", e);
     }
   }
-}
+}

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);